From 61db35ac8f4cd0be102652b77eff1be4de83dfb5 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Wed, 30 Apr 2025 17:36:55 +0200 Subject: [PATCH] Merge pull request #25207 from overleaf/ac-promisify-compile-controller-2 [web] Promisify ClsiCookieManager and CompileController (reapply and fix) GitOrigin-RevId: 0737f30c24bf92b33327dc7d0e015ac2cd7d751d --- .../src/Features/Compile/ClsiCookieManager.js | 398 ++++---- .../src/Features/Compile/CompileController.js | 868 ++++++++---------- services/web/app/src/router.mjs | 4 +- .../src/Compile/ClsiCookieManagerTests.js | 283 ++---- .../src/Compile/CompileControllerTests.js | 371 ++++---- 5 files changed, 868 insertions(+), 1056 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiCookieManager.js b/services/web/app/src/Features/Compile/ClsiCookieManager.js index fc542fefaf..7a0fd862ea 100644 --- a/services/web/app/src/Features/Compile/ClsiCookieManager.js +++ b/services/web/app/src/Features/Compile/ClsiCookieManager.js @@ -1,12 +1,15 @@ const { URL, URLSearchParams } = require('url') const OError = require('@overleaf/o-error') const Settings = require('@overleaf/settings') -const request = require('request').defaults({ timeout: 30 * 1000 }) +const { + fetchNothing, + fetchStringWithResponse, + RequestFailedError, +} = require('@overleaf/fetch-utils') const RedisWrapper = require('../../infrastructure/RedisWrapper') const Cookie = require('cookie') const logger = require('@overleaf/logger') const Metrics = require('@overleaf/metrics') -const { promisifyAll } = require('@overleaf/promise-utils') const clsiCookiesEnabled = (Settings.clsiCookie?.key ?? '') !== '' @@ -16,235 +19,200 @@ if (Settings.redis.clsi_cookie_secondary != null) { rclientSecondary = RedisWrapper.client('clsi_cookie_secondary') } -module.exports = function (backendGroup) { - const cookieManager = { - buildKey(projectId, userId) { - if (backendGroup != null) { - return `clsiserver:${backendGroup}:${projectId}:${userId}` - } else { - return `clsiserver:${projectId}:${userId}` - } - }, +const ClsiCookieManagerFactory = function (backendGroup) { + function buildKey(projectId, userId) { + if (backendGroup != null) { + return `clsiserver:${backendGroup}:${projectId}:${userId}` + } else { + return `clsiserver:${projectId}:${userId}` + } + } - getServerId( - projectId, - userId, - compileGroup, - compileBackendClass, - callback - ) { - if (!clsiCookiesEnabled) { - return callback() - } - rclient.get(this.buildKey(projectId, userId), (err, serverId) => { - if (err) { - return callback(err) - } - if (serverId == null || serverId === '') { - this._populateServerIdViaRequest( - projectId, - userId, - compileGroup, - compileBackendClass, - callback - ) - } else { - callback(null, serverId) - } - }) - }, + async function getServerId( + projectId, + userId, + compileGroup, + compileBackendClass + ) { + if (!clsiCookiesEnabled) { + return + } + const serverId = await rclient.get(buildKey(projectId, userId)) - _populateServerIdViaRequest( - projectId, - userId, - compileGroup, - compileBackendClass, - callback - ) { - const u = new URL(`${Settings.apis.clsi.url}/project/${projectId}/status`) - u.search = new URLSearchParams({ + if (!serverId) { + return await cookieManager.promises._populateServerIdViaRequest( + projectId, + userId, compileGroup, - compileBackendClass, - }).toString() - request.post(u.href, (err, res, body) => { - if (err) { - OError.tag(err, 'error getting initial server id for project', { - project_id: projectId, - }) - return callback(err) - } - if (!clsiCookiesEnabled) { - return callback() - } - const serverId = this._parseServerIdFromResponse(res) - this.setServerId( - projectId, - userId, - compileGroup, - compileBackendClass, - serverId, - null, - function (err) { - if (err) { - logger.warn( - { err, projectId }, - 'error setting server id via populate request' - ) - } - callback(err, serverId) - } - ) - }) - }, - - _parseServerIdFromResponse(response) { - const cookies = Cookie.parse(response.headers['set-cookie']?.[0] || '') - return cookies?.[Settings.clsiCookie.key] - }, - - checkIsLoadSheddingEvent(clsiserverid, compileGroup, compileBackendClass) { - request.get( - { - url: `${Settings.apis.clsi.url}/instance-state`, - qs: { clsiserverid, compileGroup, compileBackendClass }, - }, - (err, res, body) => { - if (err) { - Metrics.inc('clsi-lb-switch-backend', 1, { - status: 'error', - }) - logger.warn({ err, clsiserverid }, 'cannot probe clsi VM') - return - } - const isStillRunning = - res.statusCode === 200 && body === `${clsiserverid},UP\n` - Metrics.inc('clsi-lb-switch-backend', 1, { - status: isStillRunning ? 'load-shedding' : 'cycle', - }) - } + compileBackendClass ) - }, + } else { + return serverId + } + } - _getTTLInSeconds(clsiServerId) { - return (clsiServerId || '').includes('-reg-') - ? Settings.clsiCookie.ttlInSecondsRegular - : Settings.clsiCookie.ttlInSeconds - }, - - setServerId( - projectId, - userId, + async function _populateServerIdViaRequest( + projectId, + userId, + compileGroup, + compileBackendClass + ) { + const u = new URL(`${Settings.apis.clsi.url}/project/${projectId}/status`) + u.search = new URLSearchParams({ compileGroup, compileBackendClass, - serverId, - previous, - callback - ) { - if (!clsiCookiesEnabled) { - return callback() - } - if (serverId == null) { - // We don't get a cookie back if it hasn't changed - return rclient.expire( - this.buildKey(projectId, userId), - this._getTTLInSeconds(previous), - err => callback(err) - ) - } - if (!previous) { - // Initial assignment of a user+project or after clearing cache. - Metrics.inc('clsi-lb-assign-initial-backend') - } else { - this.checkIsLoadSheddingEvent( - previous, - compileGroup, - compileBackendClass - ) - } - if (rclientSecondary != null) { - this._setServerIdInRedis( - rclientSecondary, - projectId, - userId, - serverId, - () => {} - ) - } - this._setServerIdInRedis(rclient, projectId, userId, serverId, err => - callback(err) - ) - }, - - _setServerIdInRedis(rclient, projectId, userId, serverId, callback) { - rclient.setex( - this.buildKey(projectId, userId), - this._getTTLInSeconds(serverId), - serverId, - callback - ) - }, - - clearServerId(projectId, userId, callback) { - if (!clsiCookiesEnabled) { - return callback() - } - rclient.del(this.buildKey(projectId, userId), err => { - if (err) { - // redis errors need wrapping as the instance may be shared - return callback( - new OError( - 'Failed to clear clsi persistence', - { projectId, userId }, - err - ) - ) - } else { - return callback() - } + }).toString() + let res + try { + res = await fetchNothing(u.href, { + method: 'POST', + signal: AbortSignal.timeout(30_000), }) - }, + } catch (err) { + OError.tag(err, 'error getting initial server id for project', { + project_id: projectId, + }) + throw err + } - getCookieJar( - projectId, - userId, - compileGroup, - compileBackendClass, - callback - ) { - if (!clsiCookiesEnabled) { - return callback(null, request.jar(), undefined) - } - this.getServerId( + if (!clsiCookiesEnabled) { + return + } + const serverId = cookieManager._parseServerIdFromResponse(res) + try { + await cookieManager.promises.setServerId( projectId, userId, compileGroup, compileBackendClass, - (err, serverId) => { - if (err != null) { - OError.tag(err, 'error getting server id', { - project_id: projectId, - }) - return callback(err) - } - const serverCookie = request.cookie( - `${Settings.clsiCookie.key}=${serverId}` - ) - const jar = request.jar() - jar.setCookie(serverCookie, Settings.apis.clsi.url) - callback(null, jar, serverId) + serverId, + null + ) + return serverId + } catch (err) { + logger.warn( + { err, projectId }, + 'error setting server id via populate request' + ) + throw err + } + } + + function _parseServerIdFromResponse(response) { + const cookies = Cookie.parse(response.headers['set-cookie']?.[0] || '') + return cookies?.[Settings.clsiCookie.key] + } + + async function checkIsLoadSheddingEvent( + clsiserverid, + compileGroup, + compileBackendClass + ) { + let status + try { + const { response, body } = await fetchStringWithResponse( + `${Settings.apis.clsi.url}/instance-state`, + { + method: 'GET', + query: { clsiserverid, compileGroup, compileBackendClass }, + signal: AbortSignal.timeout(30_000), } ) + status = + response.status === 200 && body === `${clsiserverid},UP\n` + ? 'load-shedding' + : 'cycle' + } catch (err) { + if (err instanceof RequestFailedError && err.response.status === 404) { + status = 'cycle' + } else { + status = 'error' + logger.warn({ err, clsiserverid }, 'cannot probe clsi VM') + } + } + Metrics.inc('clsi-lb-switch-backend', 1, { status }) + } + + function _getTTLInSeconds(clsiServerId) { + return (clsiServerId || '').includes('-reg-') + ? Settings.clsiCookie.ttlInSecondsRegular + : Settings.clsiCookie.ttlInSeconds + } + + async function setServerId( + projectId, + userId, + compileGroup, + compileBackendClass, + serverId, + previous + ) { + if (!clsiCookiesEnabled) { + return + } + if (serverId == null) { + // We don't get a cookie back if it hasn't changed + return await rclient.expire( + buildKey(projectId, userId), + _getTTLInSeconds(previous) + ) + } + if (!previous) { + // Initial assignment of a user+project or after clearing cache. + Metrics.inc('clsi-lb-assign-initial-backend') + } else { + await checkIsLoadSheddingEvent( + previous, + compileGroup, + compileBackendClass + ) + } + if (rclientSecondary != null) { + await _setServerIdInRedis( + rclientSecondary, + projectId, + userId, + serverId + ).catch(() => {}) + } + await _setServerIdInRedis(rclient, projectId, userId, serverId) + } + + async function _setServerIdInRedis(rclient, projectId, userId, serverId) { + await rclient.setex( + buildKey(projectId, userId), + _getTTLInSeconds(serverId), + serverId + ) + } + + async function clearServerId(projectId, userId) { + if (!clsiCookiesEnabled) { + return + } + try { + await rclient.del(buildKey(projectId, userId)) + } catch (err) { + // redis errors need wrapping as the instance may be shared + throw new OError( + 'Failed to clear clsi persistence', + { projectId, userId }, + err + ) + } + } + + const cookieManager = { + _parseServerIdFromResponse, + promises: { + getServerId, + clearServerId, + _populateServerIdViaRequest, + setServerId, }, } - cookieManager.promises = promisifyAll(cookieManager, { - without: [ - '_parseServerIdFromResponse', - 'checkIsLoadSheddingEvent', - '_getTTLInSeconds', - ], - multiResult: { - getCookieJar: ['jar', 'clsiServerId'], - }, - }) + return cookieManager } + +module.exports = ClsiCookieManagerFactory diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 5d2bbcda3e..5e6e007bca 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -1,4 +1,3 @@ -let CompileController const { URL, URLSearchParams } = require('url') const { pipeline } = require('stream/promises') const { Cookie } = require('tough-cookie') @@ -17,7 +16,7 @@ const ClsiCookieManager = require('./ClsiCookieManager')( const Path = require('path') const AnalyticsManager = require('../Analytics/AnalyticsManager') const SplitTestHandler = require('../SplitTests/SplitTestHandler') -const { callbackify } = require('@overleaf/promise-utils') +const { expressify } = require('@overleaf/promise-utils') const { fetchStreamWithResponse, RequestFailedError, @@ -34,17 +33,19 @@ function getOutputFilesArchiveSpecification(projectId, userId, buildId) { const fileName = 'output.zip' return { path: fileName, - url: CompileController._getFileUrl(projectId, userId, buildId, fileName), + url: _CompileController._getFileUrl(projectId, userId, buildId, fileName), type: 'zip', } } -function getImageNameForProject(projectId, callback) { - ProjectGetter.getProject(projectId, { imageName: 1 }, (err, project) => { - if (err) return callback(err) - if (!project) return callback(new Error('project not found')) - callback(null, project.imageName) +async function getImageNameForProject(projectId) { + const project = await ProjectGetter.promises.getProject(projectId, { + imageName: 1, }) + if (!project) { + throw new Error('project not found') + } + return project.imageName } async function getPdfCachingMinChunkSize(req, res) { @@ -123,10 +124,9 @@ async function _getSplitTestOptions(req, res) { pdfCachingMinChunkSize, } } -const getSplitTestOptionsCb = callbackify(_getSplitTestOptions) -module.exports = CompileController = { - compile(req, res, next) { +const _CompileController = { + async compile(req, res) { res.setTimeout(COMPILE_TIMEOUT_MS) const projectId = req.params.Project_id const isAutoCompile = !!req.query.auto_compile @@ -162,105 +162,93 @@ module.exports = CompileController = { options.incrementalCompilesEnabled = true } - getSplitTestOptionsCb(req, res, (err, splitTestOptions) => { - if (err) return next(err) - let { - compileFromClsiCache, - populateClsiCache, - enablePdfCaching, - pdfCachingMinChunkSize, - pdfDownloadDomain, - } = splitTestOptions - options.compileFromClsiCache = compileFromClsiCache - options.populateClsiCache = populateClsiCache - options.enablePdfCaching = enablePdfCaching - if (enablePdfCaching) { - options.pdfCachingMinChunkSize = pdfCachingMinChunkSize - } + let { + compileFromClsiCache, + populateClsiCache, + enablePdfCaching, + pdfCachingMinChunkSize, + pdfDownloadDomain, + } = await _getSplitTestOptions(req, res) + options.compileFromClsiCache = compileFromClsiCache + options.populateClsiCache = populateClsiCache + options.enablePdfCaching = enablePdfCaching + if (enablePdfCaching) { + options.pdfCachingMinChunkSize = pdfCachingMinChunkSize + } - CompileManager.compile( - projectId, - userId, - options, - ( - error, + const { + status, + outputFiles, + clsiServerId, + limits, + validationProblems, + stats, + timings, + outputUrlPrefix, + buildId, + } = await CompileManager.promises + .compile(projectId, userId, options) + .catch(error => { + Metrics.inc('compile-error') + throw error + }) + + Metrics.inc('compile-status', 1, { status }) + if (pdfDownloadDomain && outputUrlPrefix) { + pdfDownloadDomain += outputUrlPrefix + } + + if (limits) { + // For a compile request to be sent to clsi we need limits. + // If we get here without having the limits object populated, it is + // a reasonable assumption to make that nothing was compiled. + // We need to know the limits in order to make use of the events. + AnalyticsManager.recordEventForSession( + req.session, + 'compile-result-backend', + { + projectId, + ownerAnalyticsId: limits.ownerAnalyticsId, status, - outputFiles, - clsiServerId, - limits, - validationProblems, - stats, - timings, - outputUrlPrefix, - buildId - ) => { - if (error) { - Metrics.inc('compile-error') - return next(error) - } - Metrics.inc('compile-status', 1, { status }) - if (pdfDownloadDomain && outputUrlPrefix) { - pdfDownloadDomain += outputUrlPrefix - } - - if (limits) { - // For a compile request to be sent to clsi we need limits. - // If we get here without having the limits object populated, it is - // a reasonable assumption to make that nothing was compiled. - // We need to know the limits in order to make use of the events. - AnalyticsManager.recordEventForSession( - req.session, - 'compile-result-backend', - { - projectId, - ownerAnalyticsId: limits.ownerAnalyticsId, - status, - compileTime: timings?.compileE2E, - timeout: limits.timeout === 60 ? 'short' : 'long', - server: clsiServerId?.includes('-c2d-') ? 'faster' : 'normal', - isAutoCompile, - isInitialCompile: stats?.isInitialCompile === 1, - restoredClsiCache: stats?.restoredClsiCache === 1, - stopOnFirstError, - } - ) - } - - const outputFilesArchive = buildId - ? getOutputFilesArchiveSpecification(projectId, userId, buildId) - : null - - res.json({ - status, - outputFiles, - outputFilesArchive, - compileGroup: limits?.compileGroup, - clsiServerId, - validationProblems, - stats, - timings, - outputUrlPrefix, - pdfDownloadDomain, - pdfCachingMinChunkSize, - }) + compileTime: timings?.compileE2E, + timeout: limits.timeout === 60 ? 'short' : 'long', + server: clsiServerId?.includes('-c2d-') ? 'faster' : 'normal', + isAutoCompile, + isInitialCompile: stats?.isInitialCompile === 1, + restoredClsiCache: stats?.restoredClsiCache === 1, + stopOnFirstError, } ) + } + + const outputFilesArchive = buildId + ? getOutputFilesArchiveSpecification(projectId, userId, buildId) + : null + + res.json({ + status, + outputFiles, + outputFilesArchive, + compileGroup: limits?.compileGroup, + clsiServerId, + validationProblems, + stats, + timings, + outputUrlPrefix, + pdfDownloadDomain, + pdfCachingMinChunkSize, }) }, - stopCompile(req, res, next) { + async stopCompile(req, res) { const projectId = req.params.Project_id const userId = SessionManager.getLoggedInUserId(req.session) - CompileManager.stopCompile(projectId, userId, function (error) { - if (error) { - return next(error) - } - res.sendStatus(200) - }) + await CompileManager.promises.stopCompile(projectId, userId) + res.sendStatus(200) }, // Used for submissions through the public API - compileSubmission(req, res, next) { + async compileSubmission(req, res) { res.setTimeout(COMPILE_TIMEOUT_MS) const submissionId = req.params.submission_id const options = {} @@ -281,195 +269,163 @@ module.exports = CompileController = { options.compileBackendClass = Settings.apis.clsi.submissionBackendClass options.timeout = req.body?.timeout || Settings.defaultFeatures.compileTimeout - ClsiManager.sendExternalRequest( - submissionId, - req.body, - options, - function (error, status, outputFiles, clsiServerId, validationProblems) { - if (error) { - return next(error) - } - res.json({ - status, - outputFiles, - clsiServerId, - validationProblems, - }) - } - ) + const { status, outputFiles, clsiServerId, validationProblems } = + await ClsiManager.promises.sendExternalRequest( + submissionId, + req.body, + options + ) + res.json({ + status, + outputFiles, + clsiServerId, + validationProblems, + }) }, - _getSplitTestOptions, - _getUserIdForCompile(req) { if (!Settings.disablePerUserCompiles) { return SessionManager.getLoggedInUserId(req.session) } return null }, - _compileAsUser(req, callback) { - callback(null, CompileController._getUserIdForCompile(req)) - }, - _downloadAsUser(req, callback) { - callback(null, CompileController._getUserIdForCompile(req)) - }, - downloadPdf(req, res, next) { + async downloadPdf(req, res) { Metrics.inc('pdf-downloads') const projectId = req.params.Project_id - const rateLimit = function (callback) { + const rateLimit = () => pdfDownloadRateLimiter .consume(req.ip, 1, { method: 'ip' }) - .then(() => { - callback(null, true) - }) + .then(() => true) .catch(err => { if (err instanceof Error) { - callback(err) - } else { - callback(null, false) + throw err } + return false }) + + const project = await ProjectGetter.promises.getProject(projectId, { + name: 1, + }) + + res.contentType('application/pdf') + const filename = `${_CompileController._getSafeProjectName(project)}.pdf` + + if (req.query.popupDownload) { + res.setContentDisposition('attachment', { filename }) + } else { + res.setContentDisposition('inline', { filename }) } - ProjectGetter.getProject(projectId, { name: 1 }, function (err, project) { - if (err) { - return next(err) - } - res.contentType('application/pdf') - const filename = `${CompileController._getSafeProjectName(project)}.pdf` + let canContinue + try { + canContinue = await rateLimit() + } catch (err) { + logger.err({ err }, 'error checking rate limit for pdf download') + res.sendStatus(500) + return + } - if (req.query.popupDownload) { - res.setContentDisposition('attachment', { filename }) - } else { - res.setContentDisposition('inline', { filename }) - } + if (!canContinue) { + logger.debug({ projectId, ip: req.ip }, 'rate limit hit downloading pdf') + res.sendStatus(500) // should it be 429? + } else { + const userId = CompileController._getUserIdForCompile(req) - rateLimit(function (err, canContinue) { - if (err) { - logger.err({ err }, 'error checking rate limit for pdf download') - res.sendStatus(500) - } else if (!canContinue) { - logger.debug( - { projectId, ip: req.ip }, - 'rate limit hit downloading pdf' - ) - res.sendStatus(500) - } else { - CompileController._downloadAsUser(req, function (error, userId) { - if (error) { - return next(error) - } - const url = CompileController._getFileUrl( - projectId, - userId, - req.params.build_id, - 'output.pdf' - ) - CompileController.proxyToClsi( - projectId, - 'output-file', - url, - {}, - req, - res, - next - ) - }) - } - }) - }) + const url = _CompileController._getFileUrl( + projectId, + userId, + req.params.build_id, + 'output.pdf' + ) + await CompileController._proxyToClsi( + projectId, + 'output-file', + url, + {}, + req, + res + ) + } }, _getSafeProjectName(project) { return project.name.replace(/[^\p{L}\p{Nd}]/gu, '_') }, - deleteAuxFiles(req, res, next) { + async deleteAuxFiles(req, res) { const projectId = req.params.Project_id const { clsiserverid } = req.query - CompileController._compileAsUser(req, function (error, userId) { - if (error) { - return next(error) - } - CompileManager.deleteAuxFiles( - projectId, - userId, - clsiserverid, - function (error) { - if (error) { - return next(error) - } - res.sendStatus(200) - } - ) - }) + const userId = await CompileController._getUserIdForCompile(req) + await CompileManager.promises.deleteAuxFiles( + projectId, + userId, + clsiserverid + ) + res.sendStatus(200) }, // this is only used by templates, so is not called with a userId - compileAndDownloadPdf(req, res, next) { + async compileAndDownloadPdf(req, res) { const projectId = req.params.project_id - // pass userId as null, since templates are an "anonymous" compile - CompileManager.compile(projectId, null, {}, (err, _status, outputFiles) => { - if (err) { - logger.err( - { err, projectId }, - 'something went wrong compile and downloading pdf' - ) - res.sendStatus(500) - return - } - const pdf = outputFiles.find(f => f.path === 'output.pdf') - if (!pdf) { - logger.warn( - { projectId }, - 'something went wrong compile and downloading pdf: no pdf' - ) - res.sendStatus(500) - return - } - CompileController.proxyToClsi( - projectId, - 'output-file', - pdf.url, - {}, - req, - res, - next + + let outputFiles + try { + ;({ outputFiles } = await CompileManager.promises + // pass userId as null, since templates are an "anonymous" compile + .compile(projectId, null, {})) + } catch (err) { + logger.err( + { err, projectId }, + 'something went wrong compile and downloading pdf' ) - }) + res.sendStatus(500) + return + } + const pdf = outputFiles.find(f => f.path === 'output.pdf') + if (!pdf) { + logger.warn( + { projectId }, + 'something went wrong compile and downloading pdf: no pdf' + ) + res.sendStatus(500) + return + } + await CompileController._proxyToClsi( + projectId, + 'output-file', + pdf.url, + {}, + req, + res + ) }, - getFileFromClsi(req, res, next) { + async getFileFromClsi(req, res) { const projectId = req.params.Project_id - CompileController._downloadAsUser(req, function (error, userId) { - if (error) { - return next(error) - } + const userId = CompileController._getUserIdForCompile(req) - const qs = {} + const qs = {} - const url = CompileController._getFileUrl( - projectId, - userId, - req.params.build_id, - req.params.file - ) - CompileController.proxyToClsi( - projectId, - 'output-file', - url, - qs, - req, - res, - next - ) - }) + const url = _CompileController._getFileUrl( + projectId, + userId, + req.params.build_id, + req.params.file + ) + await CompileController._proxyToClsi( + projectId, + 'output-file', + url, + qs, + req, + res + ) }, - getFileFromClsiWithoutUser(req, res, next) { + async getFileFromClsiWithoutUser(req, res) { const submissionId = req.params.submission_id - const url = CompileController._getFileUrl( + const url = _CompileController._getFileUrl( submissionId, null, req.params.build_id, @@ -482,15 +438,14 @@ module.exports = CompileController = { Settings.defaultFeatures.compileGroup, compileBackendClass: Settings.apis.clsi.submissionBackendClass, } - CompileController.proxyToClsiWithLimits( + await CompileController._proxyToClsiWithLimits( submissionId, 'output-file', url, {}, limits, req, - res, - next + res ) }, @@ -518,51 +473,42 @@ module.exports = CompileController = { return `${path}/${action}` }, - proxySyncPdf(req, res, next) { + async proxySyncPdf(req, res) { const projectId = req.params.Project_id const { page, h, v, editorId, buildId } = req.query if (!page?.match(/^\d+$/)) { - return next(new Error('invalid page parameter')) + throw new Error('invalid page parameter') } if (!h?.match(/^-?\d+\.\d+$/)) { - return next(new Error('invalid h parameter')) + throw new Error('invalid h parameter') } if (!v?.match(/^-?\d+\.\d+$/)) { - return next(new Error('invalid v parameter')) + throw new Error('invalid v parameter') } // whether this request is going to a per-user container - CompileController._compileAsUser(req, function (error, userId) { - if (error) { - return next(error) - } - getImageNameForProject(projectId, (error, imageName) => { - if (error) return next(error) + const userId = CompileController._getUserIdForCompile(req) - getSplitTestOptionsCb(req, res, (error, splitTestOptions) => { - if (error) return next(error) - const { compileFromClsiCache } = splitTestOptions + const imageName = await getImageNameForProject(projectId) - const url = CompileController._getUrl(projectId, userId, 'sync/pdf') + const { compileFromClsiCache } = await _getSplitTestOptions(req, res) - CompileController.proxyToClsi( - projectId, - 'sync-to-pdf', - url, - { page, h, v, imageName, editorId, buildId, compileFromClsiCache }, - req, - res, - next - ) - }) - }) - }) + const url = _CompileController._getUrl(projectId, userId, 'sync/pdf') + + await CompileController._proxyToClsi( + projectId, + 'sync-to-pdf', + url, + { page, h, v, imageName, editorId, buildId, compileFromClsiCache }, + req, + res + ) }, - proxySyncCode(req, res, next) { + async proxySyncCode(req, res) { const projectId = req.params.Project_id const { file, line, column, editorId, buildId } = req.query if (file == null) { - return next(new Error('missing file parameter')) + throw new Error('missing file parameter') } // Check that we are dealing with a simple file path (this is not // strictly needed because synctex uses this parameter as a label @@ -571,225 +517,219 @@ module.exports = CompileController = { // allow those by replacing /./ with / const testPath = file.replace('/./', '/') if (Path.resolve('/', testPath) !== `/${testPath}`) { - return next(new Error('invalid file parameter')) + throw new Error('invalid file parameter') } if (!line?.match(/^\d+$/)) { - return next(new Error('invalid line parameter')) + throw new Error('invalid line parameter') } if (!column?.match(/^\d+$/)) { - return next(new Error('invalid column parameter')) + throw new Error('invalid column parameter') } - CompileController._compileAsUser(req, function (error, userId) { - if (error) { - return next(error) - } - getImageNameForProject(projectId, (error, imageName) => { - if (error) return next(error) + const userId = CompileController._getUserIdForCompile(req) - getSplitTestOptionsCb(req, res, (error, splitTestOptions) => { - if (error) return next(error) - const { compileFromClsiCache } = splitTestOptions + const imageName = await getImageNameForProject(projectId) - const url = CompileController._getUrl(projectId, userId, 'sync/code') - CompileController.proxyToClsi( - projectId, - 'sync-to-code', - url, - { - file, - line, - column, - imageName, - editorId, - buildId, - compileFromClsiCache, - }, - req, - res, - next - ) - }) - }) - }) - }, + const { compileFromClsiCache } = await _getSplitTestOptions(req, res) - proxyToClsi(projectId, action, url, qs, req, res, next) { - CompileManager.getProjectCompileLimits(projectId, function (error, limits) { - if (error) { - return next(error) - } - CompileController.proxyToClsiWithLimits( - projectId, - action, - url, - qs, - limits, - req, - res, - next - ) - }) - }, - - proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res, next) { - _getPersistenceOptions( - req, + const url = _CompileController._getUrl(projectId, userId, 'sync/code') + await CompileController._proxyToClsi( projectId, - limits.compileGroup, - limits.compileBackendClass, - (err, persistenceOptions) => { - if (err) { - OError.tag(err, 'error getting cookie jar for clsi request') - return next(err) - } - url = new URL(`${Settings.apis.clsi.url}${url}`) - url.search = new URLSearchParams({ - ...persistenceOptions.qs, - ...qs, - }).toString() - const timer = new Metrics.Timer( - 'proxy_to_clsi', - 1, - { path: action }, - [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000] - ) - Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' }) - fetchStreamWithResponse(url.href, { - method: req.method, - signal: AbortSignal.timeout(60 * 1000), - headers: persistenceOptions.headers, - }) - .then(({ stream, response }) => { - if (req.destroyed) { - // The client has disconnected already, avoid trying to write into the broken connection. - Metrics.inc('proxy_to_clsi', 1, { - path: action, - status: 'req-aborted', - }) - return - } - Metrics.inc('proxy_to_clsi', 1, { - path: action, - status: response.status, - }) - - for (const key of ['Content-Length', 'Content-Type']) { - if (response.headers.has(key)) { - res.setHeader(key, response.headers.get(key)) - } - } - res.writeHead(response.status) - return pipeline(stream, res) - }) - .then(() => { - timer.labels.status = 'success' - timer.done() - }) - .catch(err => { - const reqAborted = Boolean(req.destroyed) - const status = reqAborted ? 'req-aborted-late' : 'error' - timer.labels.status = status - const duration = timer.done() - Metrics.inc('proxy_to_clsi', 1, { path: action, status }) - const streamingStarted = Boolean(res.headersSent) - if (!streamingStarted) { - if (err instanceof RequestFailedError) { - res.sendStatus(err.response.status) - } else { - res.sendStatus(500) - } - } - if ( - streamingStarted && - reqAborted && - err.code === 'ERR_STREAM_PREMATURE_CLOSE' - ) { - // Ignore noisy spurious error - return - } - if ( - err instanceof RequestFailedError && - ['sync-to-code', 'sync-to-pdf', 'output-file'].includes(action) - ) { - // Ignore noisy error - // https://github.com/overleaf/internal/issues/15201 - return - } - logger.warn( - { - err, - projectId, - url, - action, - reqAborted, - streamingStarted, - duration, - }, - 'CLSI proxy error' - ) - }) - } + 'sync-to-code', + url, + { + file, + line, + column, + imageName, + editorId, + buildId, + compileFromClsiCache, + }, + req, + res ) }, - wordCount(req, res, next) { + async _proxyToClsi(projectId, action, url, qs, req, res) { + const limits = + await CompileManager.promises.getProjectCompileLimits(projectId) + return CompileController._proxyToClsiWithLimits( + projectId, + action, + url, + qs, + limits, + req, + res + ) + }, + + async _proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res) { + const persistenceOptions = await _getPersistenceOptions( + req, + projectId, + limits.compileGroup, + limits.compileBackendClass + ).catch(err => { + OError.tag(err, 'error getting cookie jar for clsi request') + throw err + }) + + url = new URL(`${Settings.apis.clsi.url}${url}`) + url.search = new URLSearchParams({ + ...persistenceOptions.qs, + ...qs, + }).toString() + const timer = new Metrics.Timer( + 'proxy_to_clsi', + 1, + { path: action }, + [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000] + ) + Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' }) + try { + const { stream, response } = await fetchStreamWithResponse(url.href, { + method: req.method, + signal: AbortSignal.timeout(60 * 1000), + headers: persistenceOptions.headers, + }) + if (req.destroyed) { + // The client has disconnected already, avoid trying to write into the broken connection. + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: 'req-aborted', + }) + return + } + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: response.status, + }) + + for (const key of ['Content-Length', 'Content-Type']) { + if (response.headers.has(key)) { + res.setHeader(key, response.headers.get(key)) + } + } + res.writeHead(response.status) + await pipeline(stream, res) + timer.labels.status = 'success' + timer.done() + } catch (err) { + const reqAborted = Boolean(req.destroyed) + const status = reqAborted ? 'req-aborted-late' : 'error' + timer.labels.status = status + const duration = timer.done() + Metrics.inc('proxy_to_clsi', 1, { path: action, status }) + const streamingStarted = Boolean(res.headersSent) + if (!streamingStarted) { + if (err instanceof RequestFailedError) { + res.sendStatus(err.response.status) + } else { + res.sendStatus(500) + } + } + if ( + streamingStarted && + reqAborted && + err.code === 'ERR_STREAM_PREMATURE_CLOSE' + ) { + // Ignore noisy spurious error + return + } + if ( + err instanceof RequestFailedError && + ['sync-to-code', 'sync-to-pdf', 'output-file'].includes(action) + ) { + // Ignore noisy error + // https://github.com/overleaf/internal/issues/15201 + return + } + logger.warn( + { + err, + projectId, + url, + action, + reqAborted, + streamingStarted, + duration, + }, + 'CLSI proxy error' + ) + } + }, + + async wordCount(req, res) { const projectId = req.params.Project_id const file = req.query.file || false const { clsiserverid } = req.query - CompileController._compileAsUser(req, function (error, userId) { - if (error) { - return next(error) - } - CompileManager.wordCount( - projectId, - userId, - file, - clsiserverid, - function (error, body) { - if (error) { - return next(error) - } - res.json(body) - } - ) - }) + const userId = CompileController._getUserIdForCompile(req) + + const body = await CompileManager.promises.wordCount( + projectId, + userId, + file, + clsiserverid + ) + res.json(body) }, } -function _getPersistenceOptions( +async function _getPersistenceOptions( req, projectId, compileGroup, - compileBackendClass, - callback + compileBackendClass ) { const { clsiserverid } = req.query const userId = SessionManager.getLoggedInUserId(req) if (clsiserverid && typeof clsiserverid === 'string') { - callback(null, { + return { qs: { clsiserverid, compileGroup, compileBackendClass }, headers: {}, - }) + } } else { - ClsiCookieManager.getServerId( + const clsiServerId = await ClsiCookieManager.promises.getServerId( projectId, userId, compileGroup, - compileBackendClass, - (err, clsiServerId) => { - if (err) return callback(err) - callback(null, { - qs: { compileGroup, compileBackendClass }, - headers: clsiServerId - ? { - Cookie: new Cookie({ - key: Settings.clsiCookie.key, - value: clsiServerId, - }).cookieString(), - } - : {}, - }) - } + compileBackendClass ) + return { + qs: { compileGroup, compileBackendClass }, + headers: clsiServerId + ? { + Cookie: new Cookie({ + key: Settings.clsiCookie.key, + value: clsiServerId, + }).cookieString(), + } + : {}, + } } } + +const CompileController = { + compile: expressify(_CompileController.compile), + stopCompile: expressify(_CompileController.stopCompile), + compileSubmission: expressify(_CompileController.compileSubmission), + downloadPdf: expressify(_CompileController.downloadPdf), // + compileAndDownloadPdf: expressify(_CompileController.compileAndDownloadPdf), + deleteAuxFiles: expressify(_CompileController.deleteAuxFiles), + getFileFromClsi: expressify(_CompileController.getFileFromClsi), + getFileFromClsiWithoutUser: expressify( + _CompileController.getFileFromClsiWithoutUser + ), + proxySyncPdf: expressify(_CompileController.proxySyncPdf), + proxySyncCode: expressify(_CompileController.proxySyncCode), + wordCount: expressify(_CompileController.wordCount), + + _getSafeProjectName: _CompileController._getSafeProjectName, + _getSplitTestOptions, + _getUserIdForCompile: _CompileController._getUserIdForCompile, + _proxyToClsi: _CompileController._proxyToClsi, + _proxyToClsiWithLimits: _CompileController._proxyToClsiWithLimits, +} + +module.exports = CompileController diff --git a/services/web/app/src/router.mjs b/services/web/app/src/router.mjs index 5e1a21c063..9201ad4c55 100644 --- a/services/web/app/src/router.mjs +++ b/services/web/app/src/router.mjs @@ -1187,7 +1187,9 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { const sendRes = _.once(function (statusCode, message) { res.status(statusCode) plainTextResponse(res, message) - ClsiCookieManager.clearServerId(projectId, testUserId, () => {}) + ClsiCookieManager.promises + .clearServerId(projectId, testUserId) + .catch(() => {}) }) // force every compile to a new server // set a timeout let handler = setTimeout(function () { diff --git a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js index b61991a100..52aa65c404 100644 --- a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js @@ -1,25 +1,19 @@ const sinon = require('sinon') -const { assert, expect } = require('chai') +const { expect } = require('chai') const modulePath = '../../../../app/src/Features/Compile/ClsiCookieManager.js' const SandboxedModule = require('sandboxed-module') -const realRequst = require('request') describe('ClsiCookieManager', function () { beforeEach(function () { this.redis = { auth() {}, get: sinon.stub(), - setex: sinon.stub().callsArg(3), + setex: sinon.stub().resolves(), } this.project_id = '123423431321-proj-id' this.user_id = 'abc-user-id' - this.request = { - post: sinon.stub(), - cookie: realRequst.cookie, - jar: realRequst.jar, - defaults: () => { - return this.request - }, + this.fetchUtils = { + fetchNothing: sinon.stub().returns(Promise.resolve()), } this.settings = { redis: { @@ -41,7 +35,7 @@ describe('ClsiCookieManager', function () { client: () => this.redis, }), '@overleaf/settings': this.settings, - request: this.request, + '@overleaf/fetch-utils': this.fetchUtils, } this.ClsiCookieManager = SandboxedModule.require(modulePath, { requires: this.requires, @@ -49,74 +43,56 @@ describe('ClsiCookieManager', function () { }) describe('getServerId', function () { - it('should call get for the key', function (done) { - this.redis.get.callsArgWith(1, null, 'clsi-7') - this.ClsiCookieManager.getServerId( + it('should call get for the key', async function () { + this.redis.get.resolves('clsi-7') + const serverId = await this.ClsiCookieManager.promises.getServerId( this.project_id, this.user_id, '', - 'e2', - (err, serverId) => { - if (err) { - return done(err) - } - this.redis.get - .calledWith(`clsiserver:${this.project_id}:${this.user_id}`) - .should.equal(true) - serverId.should.equal('clsi-7') - done() - } + 'e2' ) + this.redis.get + .calledWith(`clsiserver:${this.project_id}:${this.user_id}`) + .should.equal(true) + serverId.should.equal('clsi-7') }) - it('should _populateServerIdViaRequest if no key is found', function (done) { - this.ClsiCookieManager._populateServerIdViaRequest = sinon + it('should _populateServerIdViaRequest if no key is found', async function () { + this.ClsiCookieManager.promises._populateServerIdViaRequest = sinon .stub() - .yields(null) - this.redis.get.callsArgWith(1, null) - this.ClsiCookieManager.getServerId( + .resolves() + this.redis.get.resolves(null) + await this.ClsiCookieManager.promises.getServerId( this.project_id, this.user_id, - '', - (err, serverId) => { - if (err) { - return done(err) - } - this.ClsiCookieManager._populateServerIdViaRequest - .calledWith(this.project_id, this.user_id) - .should.equal(true) - done() - } + '' ) + this.ClsiCookieManager.promises._populateServerIdViaRequest + .calledWith(this.project_id, this.user_id) + .should.equal(true) }) - it('should _populateServerIdViaRequest if no key is blank', function (done) { - this.ClsiCookieManager._populateServerIdViaRequest = sinon + it('should _populateServerIdViaRequest if no key is blank', async function () { + this.ClsiCookieManager.promises._populateServerIdViaRequest = sinon .stub() - .yields(null) - this.redis.get.callsArgWith(1, null, '') - this.ClsiCookieManager.getServerId( + .resolves(null) + this.redis.get.resolves('') + await this.ClsiCookieManager.promises.getServerId( this.project_id, this.user_id, '', - 'e2', - (err, serverId) => { - if (err) { - return done(err) - } - this.ClsiCookieManager._populateServerIdViaRequest - .calledWith(this.project_id, this.user_id) - .should.equal(true) - done() - } + 'e2' ) + this.ClsiCookieManager.promises._populateServerIdViaRequest + .calledWith(this.project_id, this.user_id) + .should.equal(true) }) }) describe('_populateServerIdViaRequest', function () { beforeEach(function () { this.clsiServerId = 'server-id' - this.ClsiCookieManager.setServerId = sinon.stub().yields() + this.ClsiCookieManager.promises.setServerId = sinon.stub().resolves() }) describe('with a server id in the response', function () { @@ -128,71 +104,54 @@ describe('ClsiCookieManager', function () { ], }, } - this.request.post.callsArgWith(1, null, this.response) + this.fetchUtils.fetchNothing.returns(this.response) }) - it('should make a request to the clsi', function (done) { - this.ClsiCookieManager._populateServerIdViaRequest( + it('should make a request to the clsi', async function () { + await this.ClsiCookieManager.promises._populateServerIdViaRequest( this.project_id, this.user_id, 'standard', - 'e2', - (err, serverId) => { - if (err) { - return done(err) - } - const args = this.ClsiCookieManager.setServerId.args[0] - args[0].should.equal(this.project_id) - args[1].should.equal(this.user_id) - args[2].should.equal('standard') - args[3].should.equal('e2') - args[4].should.deep.equal(this.clsiServerId) - done() - } + 'e2' ) + const args = this.ClsiCookieManager.promises.setServerId.args[0] + args[0].should.equal(this.project_id) + args[1].should.equal(this.user_id) + args[2].should.equal('standard') + args[3].should.equal('e2') + args[4].should.deep.equal(this.clsiServerId) }) - it('should return the server id', function (done) { - this.ClsiCookieManager._populateServerIdViaRequest( - this.project_id, - this.user_id, - '', - 'e2', - (err, serverId) => { - if (err) { - return done(err) - } - serverId.should.equal(this.clsiServerId) - done() - } - ) + it('should return the server id', async function () { + const serverId = + await this.ClsiCookieManager.promises._populateServerIdViaRequest( + this.project_id, + this.user_id, + '', + 'e2' + ) + serverId.should.equal(this.clsiServerId) }) }) describe('without a server id in the response', function () { beforeEach(function () { this.response = { headers: {} } - this.request.post.yields(null, this.response) + this.fetchUtils.fetchNothing.returns(this.response) }) - it('should not set the server id there is no server id in the response', function (done) { + it('should not set the server id there is no server id in the response', async function () { this.ClsiCookieManager._parseServerIdFromResponse = sinon .stub() .returns(null) - this.ClsiCookieManager.setServerId( + await this.ClsiCookieManager.promises.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null, - err => { - if (err) { - return done(err) - } - this.redis.setex.called.should.equal(false) - done() - } + null ) + this.redis.setex.called.should.equal(false) }) }) }) @@ -205,52 +164,40 @@ describe('ClsiCookieManager', function () { .returns('clsi-8') }) - it('should set the server id with a ttl', function (done) { - this.ClsiCookieManager.setServerId( + it('should set the server id with a ttl', async function () { + await this.ClsiCookieManager.promises.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null, - err => { - if (err) { - return done(err) - } - this.redis.setex.should.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSeconds, - this.clsiServerId - ) - done() - } + null + ) + this.redis.setex.should.have.been.calledWith( + `clsiserver:${this.project_id}:${this.user_id}`, + this.settings.clsiCookie.ttlInSeconds, + this.clsiServerId ) }) - it('should set the server id with the regular ttl for reg instance', function (done) { + it('should set the server id with the regular ttl for reg instance', async function () { this.clsiServerId = 'clsi-reg-8' - this.ClsiCookieManager.setServerId( + await this.ClsiCookieManager.promises.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null, - err => { - if (err) { - return done(err) - } - expect(this.redis.setex).to.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSecondsRegular, - this.clsiServerId - ) - done() - } + null + ) + expect(this.redis.setex).to.have.been.calledWith( + `clsiserver:${this.project_id}:${this.user_id}`, + this.settings.clsiCookie.ttlInSecondsRegular, + this.clsiServerId ) }) - it('should not set the server id if clsiCookies are not enabled', function (done) { + it('should not set the server id if clsiCookies are not enabled', async function () { delete this.settings.clsiCookie.key this.ClsiCookieManager = SandboxedModule.require(modulePath, { globals: { @@ -258,25 +205,19 @@ describe('ClsiCookieManager', function () { }, requires: this.requires, })() - this.ClsiCookieManager.setServerId( + await this.ClsiCookieManager.promises.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null, - err => { - if (err) { - return done(err) - } - this.redis.setex.called.should.equal(false) - done() - } + null ) + this.redis.setex.called.should.equal(false) }) - it('should also set in the secondary if secondary redis is enabled', function (done) { - this.redis_secondary = { setex: sinon.stub().callsArg(3) } + it('should also set in the secondary if secondary redis is enabled', async function () { + this.redis_secondary = { setex: sinon.stub().resolves() } this.settings.redis.clsi_cookie_secondary = {} this.RedisWrapper.client = sinon.stub() this.RedisWrapper.client.withArgs('clsi_cookie').returns(this.redis) @@ -292,74 +233,18 @@ describe('ClsiCookieManager', function () { this.ClsiCookieManager._parseServerIdFromResponse = sinon .stub() .returns('clsi-8') - this.ClsiCookieManager.setServerId( + await this.ClsiCookieManager.promises.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null, - err => { - if (err) { - return done(err) - } - this.redis_secondary.setex.should.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSeconds, - this.clsiServerId - ) - done() - } + null ) - }) - }) - - describe('getCookieJar', function () { - beforeEach(function () { - this.ClsiCookieManager.getServerId = sinon.stub().yields(null, 'clsi-11') - }) - - it('should return a jar with the cookie set populated from redis', function (done) { - this.ClsiCookieManager.getCookieJar( - this.project_id, - this.user_id, - '', - 'e2', - (err, jar) => { - if (err) { - return done(err) - } - jar._jar.store.idx['clsi.example.com']['/'][ - this.settings.clsiCookie.key - ].key.should.equal - jar._jar.store.idx['clsi.example.com']['/'][ - this.settings.clsiCookie.key - ].value.should.equal('clsi-11') - done() - } - ) - }) - - it('should return empty cookie jar if clsiCookies are not enabled', function (done) { - delete this.settings.clsiCookie.key - this.ClsiCookieManager = SandboxedModule.require(modulePath, { - globals: { - console, - }, - requires: this.requires, - })() - this.ClsiCookieManager.getCookieJar( - this.project_id, - this.user_id, - '', - 'e2', - (err, jar) => { - if (err) { - return done(err) - } - assert.deepEqual(jar, realRequst.jar()) - done() - } + this.redis_secondary.setex.should.have.been.calledWith( + `clsiserver:${this.project_id}:${this.user_id}`, + this.settings.clsiCookie.ttlInSeconds, + this.clsiServerId ) }) }) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index aefa197a17..25af7926b1 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -1,4 +1,3 @@ -/* eslint-disable mocha/handle-done-callback */ const sinon = require('sinon') const { expect } = require('chai') const modulePath = '../../../../app/src/Features/Compile/CompileController.js' @@ -19,8 +18,15 @@ describe('CompileController', function () { compileTimeout: 100, }, } - this.CompileManager = { compile: sinon.stub() } - this.ClsiManager = {} + this.CompileManager = { + promises: { + compile: sinon.stub(), + getProjectCompileLimits: sinon.stub(), + }, + } + this.ClsiManager = { + promises: {}, + } this.UserGetter = { getUser: sinon.stub() } this.rateLimiter = { consume: sinon.stub().resolves(), @@ -47,10 +53,11 @@ describe('CompileController', function () { }, } this.ClsiCookieManager = { - getServerId: sinon.stub().yields(null, 'clsi-server-id-from-redis'), + promises: { + getServerId: sinon.stub().resolves('clsi-server-id-from-redis'), + }, } this.SessionManager = { - getLoggedInUser: sinon.stub().callsArgWith(1, null, this.user), getLoggedInUserId: sinon.stub().returns(this.user_id), getSessionUser: sinon.stub().returns(this.user), isUserLoggedIn: sinon.stub().returns(true), @@ -76,8 +83,9 @@ describe('CompileController', function () { 'stream/promises': { pipeline: this.pipeline }, '@overleaf/settings': this.settings, '@overleaf/fetch-utils': this.fetchUtils, - request: (this.request = sinon.stub()), - '../Project/ProjectGetter': (this.ProjectGetter = {}), + '../Project/ProjectGetter': (this.ProjectGetter = { + promises: {}, + }), '@overleaf/metrics': (this.Metrics = { inc: sinon.stub(), Timer: class { @@ -121,25 +129,23 @@ describe('CompileController', function () { beforeEach(function () { this.req.params = { Project_id: this.projectId } this.req.session = {} - this.CompileManager.compile = sinon.stub().callsArgWith( - 3, - null, - (this.status = 'success'), - (this.outputFiles = [ + this.CompileManager.promises.compile = sinon.stub().resolves({ + status: (this.status = 'success'), + outputFiles: (this.outputFiles = [ { path: 'output.pdf', url: `/project/${this.projectId}/user/${this.user_id}/build/id/output.pdf`, type: 'pdf', }, ]), - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - this.build_id - ) + clsiServerId: undefined, + limits: undefined, + validationProblems: undefined, + stats: undefined, + timings: undefined, + outputUrlPrefix: undefined, + buildId: this.build_id, + }) }) describe('pdfDownloadDomain', function () { @@ -148,9 +154,8 @@ describe('CompileController', function () { }) describe('when clsi does not emit zone prefix', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileController.compile(this.req, this.res, this.next) + beforeEach(async function () { + await this.CompileController.compile(this.req, this.res, this.next) }) it('should add domain verbatim', function () { @@ -177,28 +182,25 @@ describe('CompileController', function () { }) describe('when clsi emits a zone prefix', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileManager.compile = sinon.stub().callsArgWith( - 3, - null, - (this.status = 'success'), - (this.outputFiles = [ + beforeEach(async function () { + this.CompileManager.promises.compile = sinon.stub().resolves({ + status: (this.status = 'success'), + outputFiles: (this.outputFiles = [ { path: 'output.pdf', url: `/project/${this.projectId}/user/${this.user_id}/build/id/output.pdf`, type: 'pdf', }, ]), - undefined, // clsiServerId - undefined, // limits - undefined, // validationProblems - undefined, // stats - undefined, // timings - '/zone/b', - this.build_id - ) - this.CompileController.compile(this.req, this.res, this.next) + clsiServerId: undefined, + limits: undefined, + validationProblems: undefined, + stats: undefined, + timings: undefined, + outputUrlPrefix: '/zone/b', + buildId: this.build_id, + }) + await this.CompileController.compile(this.req, this.res, this.next) }) it('should add the zone prefix', function () { @@ -227,9 +229,8 @@ describe('CompileController', function () { }) describe('when not an auto compile', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileController.compile(this.req, this.res, this.next) + beforeEach(async function () { + await this.CompileController.compile(this.req, this.res, this.next) }) it('should look up the user id', function () { @@ -239,7 +240,7 @@ describe('CompileController', function () { }) it('should do the compile without the auto compile flag', function () { - this.CompileManager.compile.should.have.been.calledWith( + this.CompileManager.promises.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -275,14 +276,13 @@ describe('CompileController', function () { }) describe('when an auto compile', function () { - beforeEach(function (done) { - this.res.callback = done + beforeEach(async function () { this.req.query = { auto_compile: 'true' } - this.CompileController.compile(this.req, this.res, this.next) + await this.CompileController.compile(this.req, this.res, this.next) }) it('should do the compile with the auto compile flag', function () { - this.CompileManager.compile.should.have.been.calledWith( + this.CompileManager.promises.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -299,14 +299,13 @@ describe('CompileController', function () { }) describe('with the draft attribute', function () { - beforeEach(function (done) { - this.res.callback = done + beforeEach(async function () { this.req.body = { draft: true } - this.CompileController.compile(this.req, this.res, this.next) + await this.CompileController.compile(this.req, this.res, this.next) }) it('should do the compile without the draft compile flag', function () { - this.CompileManager.compile.should.have.been.calledWith( + this.CompileManager.promises.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -324,14 +323,13 @@ describe('CompileController', function () { }) describe('with an editor id', function () { - beforeEach(function (done) { - this.res.callback = done + beforeEach(async function () { this.req.body = { editorId: 'the-editor-id' } - this.CompileController.compile(this.req, this.res, this.next) + await this.CompileController.compile(this.req, this.res, this.next) }) it('should pass the editor id to the compiler', function () { - this.CompileManager.compile.should.have.been.calledWith( + this.CompileManager.promises.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -353,25 +351,29 @@ describe('CompileController', function () { this.submission_id = 'sub-1234' this.req.params = { submission_id: this.submission_id } this.req.body = {} - this.ClsiManager.sendExternalRequest = sinon - .stub() - .callsArgWith( - 3, - null, - (this.status = 'success'), - (this.outputFiles = ['mock-output-files']), - (this.clsiServerId = 'mock-server-id'), - (this.validationProblems = null) - ) + this.ClsiManager.promises.sendExternalRequest = sinon.stub().resolves({ + status: (this.status = 'success'), + outputFiles: (this.outputFiles = ['mock-output-files']), + clsiServerId: 'mock-server-id', + validationProblems: null, + }) }) - it('should set the content-type of the response to application/json', function () { - this.CompileController.compileSubmission(this.req, this.res, this.next) + it('should set the content-type of the response to application/json', async function () { + await this.CompileController.compileSubmission( + this.req, + this.res, + this.next + ) this.res.contentType.calledWith('application/json').should.equal(true) }) - it('should send a successful response reporting the status and files', function () { - this.CompileController.compileSubmission(this.req, this.res, this.next) + it('should send a successful response reporting the status and files', async function () { + await this.CompileController.compileSubmission( + this.req, + this.res, + this.next + ) this.res.statusCode.should.equal(200) this.res.body.should.equal( JSON.stringify({ @@ -393,7 +395,7 @@ describe('CompileController', function () { }) it('should use the supplied values', function () { - this.ClsiManager.sendExternalRequest.should.have.been.calledWith( + this.ClsiManager.promises.sendExternalRequest.should.have.been.calledWith( this.submission_id, { compileGroup: 'special', timeout: 600 }, { compileGroup: 'special', compileBackendClass: 'n2d', timeout: 600 } @@ -413,7 +415,7 @@ describe('CompileController', function () { }) it('should use the other options but default values for compileGroup and timeout', function () { - this.ClsiManager.sendExternalRequest.should.have.been.calledWith( + this.ClsiManager.promises.sendExternalRequest.should.have.been.calledWith( this.submission_id, { rootResourcePath: 'main.tex', @@ -437,24 +439,21 @@ describe('CompileController', function () { describe('downloadPdf', function () { beforeEach(function () { + this.CompileController._proxyToClsi = sinon.stub().resolves() this.req.params = { Project_id: this.projectId } - this.project = { name: 'test namè; 1' } - this.ProjectGetter.getProject = sinon + this.ProjectGetter.promises.getProject = sinon .stub() - .callsArgWith(2, null, this.project) + .resolves(this.project) }) describe('when downloading for embedding', function () { - beforeEach(function (done) { - this.CompileController.proxyToClsi = sinon - .stub() - .callsFake(() => done()) - this.CompileController.downloadPdf(this.req, this.res, this.next) + beforeEach(async function () { + await this.CompileController.downloadPdf(this.req, this.res, this.next) }) it('should look up the project', function () { - this.ProjectGetter.getProject + this.ProjectGetter.promises.getProject .calledWith(this.projectId, { name: 1 }) .should.equal(true) }) @@ -474,43 +473,66 @@ describe('CompileController', function () { }) it('should proxy the PDF from the CLSI', function () { - this.CompileController.proxyToClsi + this.CompileController._proxyToClsi .calledWith( this.projectId, 'output-file', `/project/${this.projectId}/user/${this.user_id}/output/output.pdf`, {}, this.req, - this.res, - this.next + this.res ) .should.equal(true) }) }) describe('when a build-id is provided', function () { - beforeEach(function (done) { + beforeEach(async function () { this.req.params.build_id = this.build_id - this.CompileController.proxyToClsi = sinon - .stub() - .callsFake(() => done()) - this.CompileController.downloadPdf(this.req, this.res, this.next) + await this.CompileController.downloadPdf(this.req, this.res, this.next) }) it('should proxy the PDF from the CLSI, with a build-id', function () { - this.CompileController.proxyToClsi + this.CompileController._proxyToClsi .calledWith( this.projectId, 'output-file', `/project/${this.projectId}/user/${this.user_id}/build/${this.build_id}/output/output.pdf`, {}, this.req, - this.res, - this.next + this.res ) .should.equal(true) }) }) + + describe('when rate-limited', function () { + beforeEach(async function () { + this.rateLimiter.consume.rejects({ + msBeforeNext: 250, + remainingPoints: 0, + consumedPoints: 5, + isFirstInDuration: false, + }) + }) + it('should return 500', async function () { + await this.CompileController.downloadPdf(this.req, this.res, this.next) + // should it be 429 instead? + this.res.sendStatus.calledWith(500).should.equal(true) + this.CompileController._proxyToClsi.should.not.have.been.called + }) + }) + + describe('when rate-limit errors', function () { + beforeEach(async function () { + this.rateLimiter.consume.rejects(new Error('uh oh')) + }) + it('should return 500', async function () { + await this.CompileController.downloadPdf(this.req, this.res, this.next) + this.res.sendStatus.calledWith(500).should.equal(true) + this.CompileController._proxyToClsi.should.not.have.been.called + }) + }) }) describe('getFileFromClsiWithoutUser', function () { @@ -524,12 +546,12 @@ describe('CompileController', function () { } this.req.body = {} this.expected_url = `/project/${this.submission_id}/build/${this.build_id}/output/${this.file}` - this.CompileController.proxyToClsiWithLimits = sinon.stub() + this.CompileController._proxyToClsiWithLimits = sinon.stub() }) describe('without limits specified', function () { - beforeEach(function () { - this.CompileController.getFileFromClsiWithoutUser( + beforeEach(async function () { + await this.CompileController.getFileFromClsiWithoutUser( this.req, this.res, this.next @@ -537,15 +559,12 @@ describe('CompileController', function () { }) it('should proxy to CLSI with correct URL and default limits', function () { - this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith( + this.CompileController._proxyToClsiWithLimits.should.have.been.calledWith( this.submission_id, 'output-file', this.expected_url, {}, - { - compileGroup: 'standard', - compileBackendClass: 'n2d', - } + { compileGroup: 'standard', compileBackendClass: 'n2d' } ) }) }) @@ -561,7 +580,7 @@ describe('CompileController', function () { }) it('should proxy to CLSI with correct URL and specified limits', function () { - this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith( + this.CompileController._proxyToClsiWithLimits.should.have.been.calledWith( this.submission_id, 'output-file', this.expected_url, @@ -577,7 +596,7 @@ describe('CompileController', function () { describe('proxySyncCode', function () { let file, line, column, imageName, editorId, buildId - beforeEach(function (done) { + beforeEach(async function () { this.req.params = { Project_id: this.projectId } file = 'main.tex' line = String(Date.now()) @@ -587,17 +606,17 @@ describe('CompileController', function () { this.req.query = { file, line, column, editorId, buildId } imageName = 'foo/bar:tag-0' - this.ProjectGetter.getProject = sinon.stub().yields(null, { imageName }) + this.ProjectGetter.promises.getProject = sinon + .stub() + .resolves({ imageName }) - this.next.callsFake(done) - this.res.callback = done - this.CompileController.proxyToClsi = sinon.stub().callsFake(() => done()) + this.CompileController._proxyToClsi = sinon.stub().resolves() - this.CompileController.proxySyncCode(this.req, this.res, this.next) + await this.CompileController.proxySyncCode(this.req, this.res, this.next) }) it('should proxy the request with an imageName', function () { - expect(this.CompileController.proxyToClsi).to.have.been.calledWith( + expect(this.CompileController._proxyToClsi).to.have.been.calledWith( this.projectId, 'sync-to-code', `/project/${this.projectId}/user/${this.user_id}/sync/code`, @@ -611,8 +630,7 @@ describe('CompileController', function () { compileFromClsiCache: false, }, this.req, - this.res, - this.next + this.res ) }) }) @@ -620,7 +638,7 @@ describe('CompileController', function () { describe('proxySyncPdf', function () { let page, h, v, imageName, editorId, buildId - beforeEach(function (done) { + beforeEach(async function () { this.req.params = { Project_id: this.projectId } page = String(Date.now()) h = String(Math.random()) @@ -630,17 +648,17 @@ describe('CompileController', function () { this.req.query = { page, h, v, editorId, buildId } imageName = 'foo/bar:tag-1' - this.ProjectGetter.getProject = sinon.stub().yields(null, { imageName }) + this.ProjectGetter.promises.getProject = sinon + .stub() + .resolves({ imageName }) - this.next.callsFake(done) - this.res.callback = done - this.CompileController.proxyToClsi = sinon.stub().callsFake(() => done()) + this.CompileController._proxyToClsi = sinon.stub() - this.CompileController.proxySyncPdf(this.req, this.res, this.next) + await this.CompileController.proxySyncPdf(this.req, this.res, this.next) }) it('should proxy the request with an imageName', function () { - expect(this.CompileController.proxyToClsi).to.have.been.calledWith( + expect(this.CompileController._proxyToClsi).to.have.been.calledWith( this.projectId, 'sync-to-pdf', `/project/${this.projectId}/user/${this.user_id}/sync/pdf`, @@ -654,13 +672,12 @@ describe('CompileController', function () { compileFromClsiCache: false, }, this.req, - this.res, - this.next + this.res ) }) }) - describe('proxyToClsi', function () { + describe('_proxyToClsi', function () { beforeEach(function () { this.req.method = 'mock-method' this.req.headers = { @@ -673,15 +690,14 @@ describe('CompileController', function () { describe('old pdf viewer', function () { describe('user with standard priority', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileManager.getProjectCompileLimits = sinon + beforeEach(async function () { + this.CompileManager.promises.getProjectCompileLimits = sinon .stub() - .callsArgWith(1, null, { + .resolves({ compileGroup: 'standard', compileBackendClass: 'e2', }) - this.CompileController.proxyToClsi( + await this.CompileController._proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -704,15 +720,14 @@ describe('CompileController', function () { }) describe('user with priority compile', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileManager.getProjectCompileLimits = sinon + beforeEach(async function () { + this.CompileManager.promises.getProjectCompileLimits = sinon .stub() - .callsArgWith(1, null, { + .resolves({ compileGroup: 'priority', compileBackendClass: 'c2d', }) - this.CompileController.proxyToClsi( + await this.CompileController._proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -731,16 +746,15 @@ describe('CompileController', function () { }) describe('user with standard priority via query string', function () { - beforeEach(function (done) { - this.res.callback = done + beforeEach(async function () { this.req.query = { compileGroup: 'standard' } - this.CompileManager.getProjectCompileLimits = sinon + this.CompileManager.promises.getProjectCompileLimits = sinon .stub() - .callsArgWith(1, null, { + .resolves({ compileGroup: 'standard', compileBackendClass: 'e2', }) - this.CompileController.proxyToClsi( + await this.CompileController._proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -763,16 +777,15 @@ describe('CompileController', function () { }) describe('user with non-existent priority via query string', function () { - beforeEach(function (done) { - this.res.callback = done + beforeEach(async function () { this.req.query = { compileGroup: 'foobar' } - this.CompileManager.getProjectCompileLimits = sinon + this.CompileManager.promises.getProjectCompileLimits = sinon .stub() - .callsArgWith(1, null, { + .resolves({ compileGroup: 'standard', compileBackendClass: 'e2', }) - this.CompileController.proxyToClsi( + await this.CompileController._proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -791,16 +804,15 @@ describe('CompileController', function () { }) describe('user with build parameter via query string', function () { - beforeEach(function (done) { - this.res.callback = done - this.CompileManager.getProjectCompileLimits = sinon + beforeEach(async function () { + this.CompileManager.promises.getProjectCompileLimits = sinon .stub() - .callsArgWith(1, null, { + .resolves({ compileGroup: 'standard', compileBackendClass: 'e2', }) this.req.query = { build: 1234 } - this.CompileController.proxyToClsi( + await this.CompileController._proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -821,16 +833,16 @@ describe('CompileController', function () { }) describe('deleteAuxFiles', function () { - beforeEach(function () { - this.CompileManager.deleteAuxFiles = sinon.stub().yields() + beforeEach(async function () { + this.CompileManager.promises.deleteAuxFiles = sinon.stub().resolves() this.req.params = { Project_id: this.projectId } this.req.query = { clsiserverid: 'node-1' } this.res.sendStatus = sinon.stub() - this.CompileController.deleteAuxFiles(this.req, this.res, this.next) + await this.CompileController.deleteAuxFiles(this.req, this.res, this.next) }) it('should proxy to the CLSI', function () { - this.CompileManager.deleteAuxFiles + this.CompileManager.promises.deleteAuxFiles .calledWith(this.projectId, this.user_id, 'node-1') .should.equal(true) }) @@ -848,26 +860,25 @@ describe('CompileController', function () { }, } this.downloadPath = `/project/${this.projectId}/build/123/output/output.pdf` - this.CompileManager.compile.callsArgWith(3, null, 'success', [ - { - path: 'output.pdf', - url: this.downloadPath, - }, - ]) - this.CompileController.proxyToClsi = sinon.stub() + this.CompileManager.promises.compile.resolves({ + status: 'success', + outputFiles: [{ path: 'output.pdf', url: this.downloadPath }], + }) + this.CompileController._proxyToClsi = sinon.stub() this.res = { send: () => {}, sendStatus: sinon.stub() } }) - it('should call compile in the compile manager', function (done) { - this.CompileController.compileAndDownloadPdf(this.req, this.res) - this.CompileManager.compile.calledWith(this.projectId).should.equal(true) - done() + it('should call compile in the compile manager', async function () { + await this.CompileController.compileAndDownloadPdf(this.req, this.res) + this.CompileManager.promises.compile + .calledWith(this.projectId) + .should.equal(true) }) - it('should proxy the res to the clsi with correct url', function (done) { - this.CompileController.compileAndDownloadPdf(this.req, this.res) + it('should proxy the res to the clsi with correct url', async function () { + await this.CompileController.compileAndDownloadPdf(this.req, this.res) sinon.assert.calledWith( - this.CompileController.proxyToClsi, + this.CompileController._proxyToClsi, this.projectId, 'output-file', this.downloadPath, @@ -876,7 +887,7 @@ describe('CompileController', function () { this.res ) - this.CompileController.proxyToClsi + this.CompileController._proxyToClsi .calledWith( this.projectId, 'output-file', @@ -886,38 +897,44 @@ describe('CompileController', function () { this.res ) .should.equal(true) - done() }) - it('should not download anything on compilation failures', function () { - this.CompileManager.compile.yields(new Error('failed')) - this.CompileController.compileAndDownloadPdf(this.req, this.res) + it('should not download anything on compilation failures', async function () { + this.CompileManager.promises.compile.rejects(new Error('failed')) + await this.CompileController.compileAndDownloadPdf( + this.req, + this.res, + this.next + ) this.res.sendStatus.should.have.been.calledWith(500) - this.CompileController.proxyToClsi.should.not.have.been.called + this.CompileController._proxyToClsi.should.not.have.been.called }) - it('should not download anything on missing pdf', function () { - this.CompileManager.compile.yields(null, 'success', []) - this.CompileController.compileAndDownloadPdf(this.req, this.res) + it('should not download anything on missing pdf', async function () { + this.CompileManager.promises.compile.resolves({ + status: 'success', + outputFiles: [], + }) + await this.CompileController.compileAndDownloadPdf(this.req, this.res) this.res.sendStatus.should.have.been.calledWith(500) - this.CompileController.proxyToClsi.should.not.have.been.called + this.CompileController._proxyToClsi.should.not.have.been.called }) }) describe('wordCount', function () { - beforeEach(function () { - this.CompileManager.wordCount = sinon + beforeEach(async function () { + this.CompileManager.promises.wordCount = sinon .stub() - .yields(null, { content: 'body' }) + .resolves({ content: 'body' }) this.req.params = { Project_id: this.projectId } this.req.query = { clsiserverid: 'node-42' } this.res.json = sinon.stub() this.res.contentType = sinon.stub() - this.CompileController.wordCount(this.req, this.res, this.next) + await this.CompileController.wordCount(this.req, this.res, this.next) }) it('should proxy to the CLSI', function () { - this.CompileManager.wordCount + this.CompileManager.promises.wordCount .calledWith(this.projectId, this.user_id, false, 'node-42') .should.equal(true) })