From 96f29c0f6d2e623fb088e88200934c5df8da9924 Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Wed, 19 Sep 2018 10:16:44 +0200 Subject: [PATCH] Optimize SQL requests of videos AP endpoints --- server/controllers/activitypub/client.ts | 16 +++-- server/helpers/custom-validators/videos.ts | 3 +- server/lib/activitypub/videos.ts | 2 +- .../middlewares/validators/video-comments.ts | 2 +- server/middlewares/validators/videos.ts | 70 ++++++++++--------- server/models/video/video-format-utils.ts | 5 +- server/models/video/video.ts | 2 +- 7 files changed, 56 insertions(+), 44 deletions(-) diff --git a/server/controllers/activitypub/client.ts b/server/controllers/activitypub/client.ts index 2e168ea78..6229c44aa 100644 --- a/server/controllers/activitypub/client.ts +++ b/server/controllers/activitypub/client.ts @@ -6,7 +6,13 @@ import { CONFIG, ROUTE_CACHE_LIFETIME } from '../../initializers' import { buildAnnounceWithVideoAudience } from '../../lib/activitypub/send' import { audiencify, getAudience } from '../../lib/activitypub/audience' import { buildCreateActivity } from '../../lib/activitypub/send/send-create' -import { asyncMiddleware, executeIfActivityPub, localAccountValidator, localVideoChannelValidator } from '../../middlewares' +import { + asyncMiddleware, + executeIfActivityPub, + localAccountValidator, + localVideoChannelValidator, + videosCustomGetValidator +} from '../../middlewares' import { videosGetValidator, videosShareValidator } from '../../middlewares/validators' import { videoCommentGetValidator } from '../../middlewares/validators/video-comments' import { AccountModel } from '../../models/account/account' @@ -54,7 +60,7 @@ activityPubClientRouter.get('/videos/watch/:id/activity', executeIfActivityPub(asyncMiddleware(videoController)) ) activityPubClientRouter.get('/videos/watch/:id/announces', - executeIfActivityPub(asyncMiddleware(videosGetValidator)), + executeIfActivityPub(asyncMiddleware(videosCustomGetValidator('only-video'))), executeIfActivityPub(asyncMiddleware(videoAnnouncesController)) ) activityPubClientRouter.get('/videos/watch/:id/announces/:accountId', @@ -62,15 +68,15 @@ activityPubClientRouter.get('/videos/watch/:id/announces/:accountId', executeIfActivityPub(asyncMiddleware(videoAnnounceController)) ) activityPubClientRouter.get('/videos/watch/:id/likes', - executeIfActivityPub(asyncMiddleware(videosGetValidator)), + executeIfActivityPub(asyncMiddleware(videosCustomGetValidator('only-video'))), executeIfActivityPub(asyncMiddleware(videoLikesController)) ) activityPubClientRouter.get('/videos/watch/:id/dislikes', - executeIfActivityPub(asyncMiddleware(videosGetValidator)), + executeIfActivityPub(asyncMiddleware(videosCustomGetValidator('only-video'))), executeIfActivityPub(asyncMiddleware(videoDislikesController)) ) activityPubClientRouter.get('/videos/watch/:id/comments', - executeIfActivityPub(asyncMiddleware(videosGetValidator)), + executeIfActivityPub(asyncMiddleware(videosCustomGetValidator('only-video'))), executeIfActivityPub(asyncMiddleware(videoCommentsController)) ) activityPubClientRouter.get('/videos/watch/:videoId/comments/:commentId', diff --git a/server/helpers/custom-validators/videos.ts b/server/helpers/custom-validators/videos.ts index dd207c787..c9ef8445d 100644 --- a/server/helpers/custom-validators/videos.ts +++ b/server/helpers/custom-validators/videos.ts @@ -152,7 +152,8 @@ function checkUserCanManageVideo (user: UserModel, video: VideoModel, right: Use return true } -async function isVideoExist (id: string, res: Response, fetchType: 'all' | 'only-video' | 'id' | 'none' = 'all') { +export type VideoFetchType = 'all' | 'only-video' | 'id' | 'none' +async function isVideoExist (id: string, res: Response, fetchType: VideoFetchType = 'all') { let video: VideoModel | null if (fetchType === 'all') { diff --git a/server/lib/activitypub/videos.ts b/server/lib/activitypub/videos.ts index 783f78d3e..5150c9975 100644 --- a/server/lib/activitypub/videos.ts +++ b/server/lib/activitypub/videos.ts @@ -61,7 +61,7 @@ function fetchRemoteVideoStaticFile (video: VideoModel, path: string, reject: Fu async function fetchRemoteVideoDescription (video: VideoModel) { const host = video.VideoChannel.Account.Actor.Server.host - const path = video.getDescriptionPath() + const path = video.getDescriptionAPIPath() const options = { uri: REMOTE_SCHEME.HTTP + '://' + host + path, json: true diff --git a/server/middlewares/validators/video-comments.ts b/server/middlewares/validators/video-comments.ts index 4b15eed23..693852499 100644 --- a/server/middlewares/validators/video-comments.ts +++ b/server/middlewares/validators/video-comments.ts @@ -78,7 +78,7 @@ const videoCommentGetValidator = [ logger.debug('Checking videoCommentGetValidator parameters.', { parameters: req.params }) if (areValidationErrors(req, res)) return - if (!await isVideoExist(req.params.videoId, res)) return + if (!await isVideoExist(req.params.videoId, res, 'id')) return if (!await isVideoCommentExist(req.params.commentId, res.locals.video, res)) return return next() diff --git a/server/middlewares/validators/videos.ts b/server/middlewares/validators/videos.ts index 9befbc9ee..8aa7b3a39 100644 --- a/server/middlewares/validators/videos.ts +++ b/server/middlewares/validators/videos.ts @@ -26,7 +26,8 @@ import { isVideoPrivacyValid, isVideoRatingTypeValid, isVideoSupportValid, - isVideoTagsValid + isVideoTagsValid, + VideoFetchType } from '../../helpers/custom-validators/videos' import { getDurationFromVideoFile } from '../../helpers/ffmpeg-utils' import { logger } from '../../helpers/logger' @@ -128,47 +129,49 @@ const videosUpdateValidator = getCommonVideoAttributes().concat([ } ]) -const videosGetValidator = [ - param('id').custom(isIdOrUUIDValid).not().isEmpty().withMessage('Should have a valid id'), +const videosCustomGetValidator = (fetchType: VideoFetchType) => { + return [ + param('id').custom(isIdOrUUIDValid).not().isEmpty().withMessage('Should have a valid id'), - async (req: express.Request, res: express.Response, next: express.NextFunction) => { - logger.debug('Checking videosGet parameters', { parameters: req.params }) + async (req: express.Request, res: express.Response, next: express.NextFunction) => { + logger.debug('Checking videosGet parameters', { parameters: req.params }) - if (areValidationErrors(req, res)) return - if (!await isVideoExist(req.params.id, res)) return + if (areValidationErrors(req, res)) return + if (!await isVideoExist(req.params.id, res, fetchType)) return - const video: VideoModel = res.locals.video + const video: VideoModel = res.locals.video - // Video private or blacklisted - if (video.privacy === VideoPrivacy.PRIVATE || video.VideoBlacklist) { - return authenticate(req, res, () => { - const user: UserModel = res.locals.oauth.token.User + // Video private or blacklisted + if (video.privacy === VideoPrivacy.PRIVATE || video.VideoBlacklist) { + return authenticate(req, res, () => { + const user: UserModel = res.locals.oauth.token.User - // Only the owner or a user that have blacklist rights can see the video - if (video.VideoChannel.Account.userId !== user.id && !user.hasRight(UserRight.MANAGE_VIDEO_BLACKLIST)) { - return res.status(403) - .json({ error: 'Cannot get this private or blacklisted video.' }) - .end() - } + // Only the owner or a user that have blacklist rights can see the video + if (video.VideoChannel.Account.userId !== user.id && !user.hasRight(UserRight.MANAGE_VIDEO_BLACKLIST)) { + return res.status(403) + .json({ error: 'Cannot get this private or blacklisted video.' }) + .end() + } - return next() - }) + return next() + }) + } - return + // Video is public, anyone can access it + if (video.privacy === VideoPrivacy.PUBLIC) return next() + + // Video is unlisted, check we used the uuid to fetch it + if (video.privacy === VideoPrivacy.UNLISTED) { + if (isUUIDValid(req.params.id)) return next() + + // Don't leak this unlisted video + return res.status(404).end() + } } + ] +} - // Video is public, anyone can access it - if (video.privacy === VideoPrivacy.PUBLIC) return next() - - // Video is unlisted, check we used the uuid to fetch it - if (video.privacy === VideoPrivacy.UNLISTED) { - if (isUUIDValid(req.params.id)) return next() - - // Don't leak this unlisted video - return res.status(404).end() - } - } -] +const videosGetValidator = videosCustomGetValidator('all') const videosRemoveValidator = [ param('id').custom(isIdOrUUIDValid).not().isEmpty().withMessage('Should have a valid id'), @@ -366,6 +369,7 @@ export { videosAddValidator, videosUpdateValidator, videosGetValidator, + videosCustomGetValidator, videosRemoveValidator, videosShareValidator, diff --git a/server/models/video/video-format-utils.ts b/server/models/video/video-format-utils.ts index fae38507b..a9a58624d 100644 --- a/server/models/video/video-format-utils.ts +++ b/server/models/video/video-format-utils.ts @@ -112,12 +112,13 @@ function videoModelToFormattedDetailsJSON (video: VideoModel): VideoDetails { } }) + const tags = video.Tags ? video.Tags.map(t => t.name) : [] const detailsJson = { support: video.support, - descriptionPath: video.getDescriptionPath(), + descriptionPath: video.getDescriptionAPIPath(), channel: video.VideoChannel.toFormattedJSON(), account: video.VideoChannel.Account.toFormattedJSON(), - tags: video.Tags.map(t => t.name), + tags, commentsEnabled: video.commentsEnabled, waitTranscoding: video.waitTranscoding, state: { diff --git a/server/models/video/video.ts b/server/models/video/video.ts index c7cd2890c..ce2153f87 100644 --- a/server/models/video/video.ts +++ b/server/models/video/video.ts @@ -1406,7 +1406,7 @@ export class VideoModel extends Model { return getVideoFileResolution(originalFilePath) } - getDescriptionPath () { + getDescriptionAPIPath () { return `/api/${API_VERSION}/videos/${this.uuid}/description` }