From c0759da78ee29fe57aa69292cc637ffdccbabeb7 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Wed, 30 Apr 2025 12:09:05 +0200 Subject: [PATCH] Merge pull request #25200 from overleaf/revert-25023-ac-promisify-compile-controller Revert "[web] Promisify ClsiCookieManager and CompileController" GitOrigin-RevId: 190ee8d2be23687f092e762c5199a34bcdf37cf9 --- .../src/Features/Compile/ClsiCookieManager.js | 414 +++++---- .../src/Features/Compile/CompileController.js | 856 ++++++++++-------- services/web/app/src/router.mjs | 4 +- .../src/Compile/ClsiCookieManagerTests.js | 295 ++++-- .../src/Compile/CompileControllerTests.js | 371 ++++---- 5 files changed, 1061 insertions(+), 879 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiCookieManager.js b/services/web/app/src/Features/Compile/ClsiCookieManager.js index fe21343e19..fc542fefaf 100644 --- a/services/web/app/src/Features/Compile/ClsiCookieManager.js +++ b/services/web/app/src/Features/Compile/ClsiCookieManager.js @@ -1,15 +1,12 @@ const { URL, URLSearchParams } = require('url') const OError = require('@overleaf/o-error') const Settings = require('@overleaf/settings') -const { - fetchNothing, - fetchStringWithResponse, - RequestFailedError, -} = require('@overleaf/fetch-utils') +const request = require('request').defaults({ timeout: 30 * 1000 }) 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 ?? '') !== '' @@ -19,208 +16,235 @@ if (Settings.redis.clsi_cookie_secondary != null) { rclientSecondary = RedisWrapper.client('clsi_cookie_secondary') } -const ClsiCookieManagerFactory = function (backendGroup) { - function buildKey(projectId, userId) { - if (backendGroup != null) { - return `clsiserver:${backendGroup}:${projectId}:${userId}` - } else { - return `clsiserver:${projectId}:${userId}` - } - } +module.exports = function (backendGroup) { + const cookieManager = { + buildKey(projectId, userId) { + if (backendGroup != null) { + return `clsiserver:${backendGroup}:${projectId}:${userId}` + } else { + return `clsiserver:${projectId}:${userId}` + } + }, - async function getServerId( - projectId, - userId, - compileGroup, - compileBackendClass - ) { - if (!clsiCookiesEnabled) { - return - } - const serverId = await rclient.get(buildKey(projectId, userId)) - - if (!serverId) { - return cookieManager.promises._populateServerIdViaRequest( - projectId, - userId, - compileGroup, - compileBackendClass - ) - } else { - return serverId - } - } - - async function _populateServerIdViaRequest( - projectId, - userId, - compileGroup, - compileBackendClass - ) { - const u = new URL(`${Settings.apis.clsi.url}/project/${projectId}/status`) - u.search = new URLSearchParams({ + getServerId( + projectId, + userId, compileGroup, compileBackendClass, - }).toString() - let res - try { - res = await fetchNothing(u.href, { - method: 'POST', - signal: AbortSignal.timeout(30_000), - }) - } catch (err) { - if (err instanceof RequestFailedError && err.response.status < 500) { - logger.warn( - { err, projectId }, - 'error requesting project status from clsi' - ) - res = err.response - } else { - OError.tag(err, 'error getting initial server id for project', { - project_id: projectId, - }) - throw err + 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) + } + }) + }, - if (!clsiCookiesEnabled) { - return - } - const serverId = cookieManager._parseServerIdFromResponse(res) - try { - await cookieManager.promises.setServerId( + _populateServerIdViaRequest( + projectId, + userId, + compileGroup, + compileBackendClass, + callback + ) { + const u = new URL(`${Settings.apis.clsi.url}/project/${projectId}/status`) + u.search = new URLSearchParams({ + 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', + }) + } + ) + }, + + _getTTLInSeconds(clsiServerId) { + return (clsiServerId || '').includes('-reg-') + ? Settings.clsiCookie.ttlInSecondsRegular + : Settings.clsiCookie.ttlInSeconds + }, + + setServerId( + projectId, + userId, + 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() + } + }) + }, + + getCookieJar( + projectId, + userId, + compileGroup, + compileBackendClass, + callback + ) { + if (!clsiCookiesEnabled) { + return callback(null, request.jar(), undefined) + } + this.getServerId( projectId, userId, compileGroup, compileBackendClass, - 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), + (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) } ) - 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 a51c2e80d3..5d2bbcda3e 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -1,3 +1,4 @@ +let CompileController const { URL, URLSearchParams } = require('url') const { pipeline } = require('stream/promises') const { Cookie } = require('tough-cookie') @@ -16,7 +17,7 @@ const ClsiCookieManager = require('./ClsiCookieManager')( const Path = require('path') const AnalyticsManager = require('../Analytics/AnalyticsManager') const SplitTestHandler = require('../SplitTests/SplitTestHandler') -const { expressify } = require('@overleaf/promise-utils') +const { callbackify } = require('@overleaf/promise-utils') const { fetchStreamWithResponse, RequestFailedError, @@ -33,19 +34,17 @@ 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', } } -async function getImageNameForProject(projectId) { - const project = await ProjectGetter.promises.getProject(projectId, { - imageName: 1, +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) }) - if (!project) { - throw new Error('project not found') - } - return project.imageName } async function getPdfCachingMinChunkSize(req, res) { @@ -124,9 +123,10 @@ async function _getSplitTestOptions(req, res) { pdfCachingMinChunkSize, } } +const getSplitTestOptionsCb = callbackify(_getSplitTestOptions) -const _CompileController = { - async compile(req, res) { +module.exports = CompileController = { + compile(req, res, next) { res.setTimeout(COMPILE_TIMEOUT_MS) const projectId = req.params.Project_id const isAutoCompile = !!req.query.auto_compile @@ -162,93 +162,105 @@ const _CompileController = { options.incrementalCompilesEnabled = true } - let { - compileFromClsiCache, - populateClsiCache, - enablePdfCaching, - pdfCachingMinChunkSize, - pdfDownloadDomain, - } = await _getSplitTestOptions(req, res) - options.compileFromClsiCache = compileFromClsiCache - options.populateClsiCache = populateClsiCache - options.enablePdfCaching = enablePdfCaching - if (enablePdfCaching) { - options.pdfCachingMinChunkSize = pdfCachingMinChunkSize - } + 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 + } - 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, + CompileManager.compile( + projectId, + userId, + options, + ( + error, 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, + 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, + }) } ) - } - - const outputFilesArchive = buildId - ? getOutputFilesArchiveSpecification(projectId, userId, buildId) - : null - - res.json({ - status, - outputFiles, - outputFilesArchive, - compileGroup: limits?.compileGroup, - clsiServerId, - validationProblems, - stats, - timings, - outputUrlPrefix, - pdfDownloadDomain, - pdfCachingMinChunkSize, }) }, - async stopCompile(req, res) { + stopCompile(req, res, next) { const projectId = req.params.Project_id const userId = SessionManager.getLoggedInUserId(req.session) - await CompileManager.promises.stopCompile(projectId, userId) - res.sendStatus(200) + CompileManager.stopCompile(projectId, userId, function (error) { + if (error) { + return next(error) + } + res.sendStatus(200) + }) }, // Used for submissions through the public API - async compileSubmission(req, res) { + compileSubmission(req, res, next) { res.setTimeout(COMPILE_TIMEOUT_MS) const submissionId = req.params.submission_id const options = {} @@ -269,163 +281,195 @@ const _CompileController = { options.compileBackendClass = Settings.apis.clsi.submissionBackendClass options.timeout = req.body?.timeout || Settings.defaultFeatures.compileTimeout - const { status, outputFiles, clsiServerId, validationProblems } = - await ClsiManager.promises.sendExternalRequest( - submissionId, - req.body, - options - ) - res.json({ - status, - outputFiles, - clsiServerId, - validationProblems, - }) + ClsiManager.sendExternalRequest( + submissionId, + req.body, + options, + function (error, status, outputFiles, clsiServerId, validationProblems) { + if (error) { + return next(error) + } + 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)) + }, - async downloadPdf(req, res) { + downloadPdf(req, res, next) { Metrics.inc('pdf-downloads') const projectId = req.params.Project_id - const rateLimit = () => + const rateLimit = function (callback) { pdfDownloadRateLimiter .consume(req.ip, 1, { method: 'ip' }) - .then(() => true) + .then(() => { + callback(null, true) + }) .catch(err => { if (err instanceof Error) { - throw err + callback(err) + } else { + callback(null, false) } - return false }) + } - const project = await ProjectGetter.promises.getProject(projectId, { - name: 1, + ProjectGetter.getProject(projectId, { name: 1 }, function (err, project) { + if (err) { + return next(err) + } + res.contentType('application/pdf') + const filename = `${CompileController._getSafeProjectName(project)}.pdf` + + if (req.query.popupDownload) { + res.setContentDisposition('attachment', { filename }) + } else { + res.setContentDisposition('inline', { filename }) + } + + 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 + ) + }) + } + }) }) - - res.contentType('application/pdf') - const filename = `${_CompileController._getSafeProjectName(project)}.pdf` - - if (req.query.popupDownload) { - res.setContentDisposition('attachment', { filename }) - } else { - res.setContentDisposition('inline', { filename }) - } - - let canContinue - try { - canContinue = await rateLimit() - } catch (err) { - logger.err({ err }, 'error checking rate limit for pdf download') - res.sendStatus(500) - return - } - - 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) - - 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, '_') }, - async deleteAuxFiles(req, res) { + deleteAuxFiles(req, res, next) { const projectId = req.params.Project_id const { clsiserverid } = req.query - const userId = await CompileController._getUserIdForCompile(req) - await CompileManager.promises.deleteAuxFiles( - projectId, - userId, - clsiserverid - ) - res.sendStatus(200) + 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) + } + ) + }) }, // this is only used by templates, so is not called with a userId - async compileAndDownloadPdf(req, res) { + compileAndDownloadPdf(req, res, next) { const projectId = req.params.project_id - - 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' + // 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 ) - 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 - ) + }) }, - async getFileFromClsi(req, res) { + getFileFromClsi(req, res, next) { const projectId = req.params.Project_id - const userId = CompileController._getUserIdForCompile(req) + CompileController._downloadAsUser(req, function (error, userId) { + if (error) { + return next(error) + } - const qs = {} + const qs = {} - const url = _CompileController._getFileUrl( - projectId, - userId, - req.params.build_id, - req.params.file - ) - await CompileController._proxyToClsi( - projectId, - 'output-file', - url, - qs, - req, - res - ) + const url = CompileController._getFileUrl( + projectId, + userId, + req.params.build_id, + req.params.file + ) + CompileController.proxyToClsi( + projectId, + 'output-file', + url, + qs, + req, + res, + next + ) + }) }, - async getFileFromClsiWithoutUser(req, res) { + getFileFromClsiWithoutUser(req, res, next) { const submissionId = req.params.submission_id - const url = _CompileController._getFileUrl( + const url = CompileController._getFileUrl( submissionId, null, req.params.build_id, @@ -438,14 +482,15 @@ const _CompileController = { Settings.defaultFeatures.compileGroup, compileBackendClass: Settings.apis.clsi.submissionBackendClass, } - await CompileController._proxyToClsiWithLimits( + CompileController.proxyToClsiWithLimits( submissionId, 'output-file', url, {}, limits, req, - res + res, + next ) }, @@ -473,42 +518,51 @@ const _CompileController = { return `${path}/${action}` }, - async proxySyncPdf(req, res) { + proxySyncPdf(req, res, next) { const projectId = req.params.Project_id const { page, h, v, editorId, buildId } = req.query if (!page?.match(/^\d+$/)) { - throw new Error('invalid page parameter') + return next(new Error('invalid page parameter')) } if (!h?.match(/^-?\d+\.\d+$/)) { - throw new Error('invalid h parameter') + return next(new Error('invalid h parameter')) } if (!v?.match(/^-?\d+\.\d+$/)) { - throw new Error('invalid v parameter') + return next(new Error('invalid v parameter')) } // whether this request is going to a per-user container - const userId = CompileController._getUserIdForCompile(req) + CompileController._compileAsUser(req, function (error, userId) { + if (error) { + return next(error) + } + getImageNameForProject(projectId, (error, imageName) => { + if (error) return next(error) - const imageName = await getImageNameForProject(projectId) + getSplitTestOptionsCb(req, res, (error, splitTestOptions) => { + if (error) return next(error) + const { compileFromClsiCache } = splitTestOptions - const { compileFromClsiCache } = await _getSplitTestOptions(req, res) + const url = CompileController._getUrl(projectId, userId, 'sync/pdf') - 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 - ) + CompileController.proxyToClsi( + projectId, + 'sync-to-pdf', + url, + { page, h, v, imageName, editorId, buildId, compileFromClsiCache }, + req, + res, + next + ) + }) + }) + }) }, - async proxySyncCode(req, res) { + proxySyncCode(req, res, next) { const projectId = req.params.Project_id const { file, line, column, editorId, buildId } = req.query if (file == null) { - throw new Error('missing file parameter') + return next(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 @@ -517,217 +571,225 @@ const _CompileController = { // allow those by replacing /./ with / const testPath = file.replace('/./', '/') if (Path.resolve('/', testPath) !== `/${testPath}`) { - throw new Error('invalid file parameter') + return next(new Error('invalid file parameter')) } if (!line?.match(/^\d+$/)) { - throw new Error('invalid line parameter') + return next(new Error('invalid line parameter')) } if (!column?.match(/^\d+$/)) { - throw new Error('invalid column parameter') + return next(new Error('invalid column parameter')) } - const userId = CompileController._getUserIdForCompile(req) + CompileController._compileAsUser(req, function (error, userId) { + if (error) { + return next(error) + } + getImageNameForProject(projectId, (error, imageName) => { + if (error) return next(error) - const imageName = await getImageNameForProject(projectId) + getSplitTestOptionsCb(req, res, (error, splitTestOptions) => { + if (error) return next(error) + const { compileFromClsiCache } = splitTestOptions - const { compileFromClsiCache } = await _getSplitTestOptions(req, res) - - const url = _CompileController._getUrl(projectId, userId, 'sync/code') - await CompileController._proxyToClsi( - projectId, - 'sync-to-code', - url, - { - file, - line, - column, - imageName, - editorId, - buildId, - compileFromClsiCache, - }, - req, - res - ) + 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 + ) + }) + }) + }) }, - 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 - ) + 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 + ) + }) }, - async _proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res) { - const persistenceOptions = await _getPersistenceOptions( + proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res, next) { + _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', + 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, }) - return - } - Metrics.inc('proxy_to_clsi', 1, { - path: action, - status: response.status, - }) + .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)) - } + 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' + ) + }) } - 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) { + wordCount(req, res, next) { const projectId = req.params.Project_id const file = req.query.file || false const { clsiserverid } = req.query - const userId = CompileController._getUserIdForCompile(req) - - const body = await CompileManager.promises.wordCount( - projectId, - userId, - file, - clsiserverid - ) - res.json(body) + 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) + } + ) + }) }, } -async function _getPersistenceOptions( +function _getPersistenceOptions( req, projectId, compileGroup, - compileBackendClass + compileBackendClass, + callback ) { const { clsiserverid } = req.query const userId = SessionManager.getLoggedInUserId(req) if (clsiserverid && typeof clsiserverid === 'string') { - return { + callback(null, { qs: { clsiserverid, compileGroup, compileBackendClass }, headers: {}, - } + }) } else { - const clsiServerId = await ClsiCookieManager.promises.getServerId( + ClsiCookieManager.getServerId( projectId, userId, compileGroup, - compileBackendClass + 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(), + } + : {}, + }) + } ) - 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), - - _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 9201ad4c55..5e1a21c063 100644 --- a/services/web/app/src/router.mjs +++ b/services/web/app/src/router.mjs @@ -1187,9 +1187,7 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { const sendRes = _.once(function (statusCode, message) { res.status(statusCode) plainTextResponse(res, message) - ClsiCookieManager.promises - .clearServerId(projectId, testUserId) - .catch(() => {}) + ClsiCookieManager.clearServerId(projectId, testUserId, () => {}) }) // 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 52aa65c404..b61991a100 100644 --- a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js @@ -1,19 +1,25 @@ const sinon = require('sinon') -const { expect } = require('chai') +const { assert, 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().resolves(), + setex: sinon.stub().callsArg(3), } this.project_id = '123423431321-proj-id' this.user_id = 'abc-user-id' - this.fetchUtils = { - fetchNothing: sinon.stub().returns(Promise.resolve()), + this.request = { + post: sinon.stub(), + cookie: realRequst.cookie, + jar: realRequst.jar, + defaults: () => { + return this.request + }, } this.settings = { redis: { @@ -35,7 +41,7 @@ describe('ClsiCookieManager', function () { client: () => this.redis, }), '@overleaf/settings': this.settings, - '@overleaf/fetch-utils': this.fetchUtils, + request: this.request, } this.ClsiCookieManager = SandboxedModule.require(modulePath, { requires: this.requires, @@ -43,56 +49,74 @@ describe('ClsiCookieManager', function () { }) describe('getServerId', function () { - it('should call get for the key', async function () { - this.redis.get.resolves('clsi-7') - const serverId = await this.ClsiCookieManager.promises.getServerId( + it('should call get for the key', function (done) { + this.redis.get.callsArgWith(1, null, 'clsi-7') + this.ClsiCookieManager.getServerId( this.project_id, this.user_id, '', - 'e2' + '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() + } ) - 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', async function () { - this.ClsiCookieManager.promises._populateServerIdViaRequest = sinon + it('should _populateServerIdViaRequest if no key is found', function (done) { + this.ClsiCookieManager._populateServerIdViaRequest = sinon .stub() - .resolves() - this.redis.get.resolves(null) - await this.ClsiCookieManager.promises.getServerId( - this.project_id, - this.user_id, - '' - ) - this.ClsiCookieManager.promises._populateServerIdViaRequest - .calledWith(this.project_id, this.user_id) - .should.equal(true) - }) - - it('should _populateServerIdViaRequest if no key is blank', async function () { - this.ClsiCookieManager.promises._populateServerIdViaRequest = sinon - .stub() - .resolves(null) - this.redis.get.resolves('') - await this.ClsiCookieManager.promises.getServerId( + .yields(null) + this.redis.get.callsArgWith(1, null) + this.ClsiCookieManager.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() + } + ) + }) + + it('should _populateServerIdViaRequest if no key is blank', function (done) { + this.ClsiCookieManager._populateServerIdViaRequest = sinon + .stub() + .yields(null) + this.redis.get.callsArgWith(1, null, '') + this.ClsiCookieManager.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() + } ) - 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.promises.setServerId = sinon.stub().resolves() + this.ClsiCookieManager.setServerId = sinon.stub().yields() }) describe('with a server id in the response', function () { @@ -104,54 +128,71 @@ describe('ClsiCookieManager', function () { ], }, } - this.fetchUtils.fetchNothing.returns(this.response) + this.request.post.callsArgWith(1, null, this.response) }) - it('should make a request to the clsi', async function () { - await this.ClsiCookieManager.promises._populateServerIdViaRequest( + it('should make a request to the clsi', function (done) { + this.ClsiCookieManager._populateServerIdViaRequest( this.project_id, this.user_id, 'standard', - 'e2' + '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() + } ) - 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', async function () { - const serverId = - await this.ClsiCookieManager.promises._populateServerIdViaRequest( - this.project_id, - this.user_id, - '', - 'e2' - ) - serverId.should.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() + } + ) }) }) describe('without a server id in the response', function () { beforeEach(function () { this.response = { headers: {} } - this.fetchUtils.fetchNothing.returns(this.response) + this.request.post.yields(null, this.response) }) - it('should not set the server id there is no server id in the response', async function () { + it('should not set the server id there is no server id in the response', function (done) { this.ClsiCookieManager._parseServerIdFromResponse = sinon .stub() .returns(null) - await this.ClsiCookieManager.promises.setServerId( + this.ClsiCookieManager.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null + null, + err => { + if (err) { + return done(err) + } + this.redis.setex.called.should.equal(false) + done() + } ) - this.redis.setex.called.should.equal(false) }) }) }) @@ -164,40 +205,52 @@ describe('ClsiCookieManager', function () { .returns('clsi-8') }) - it('should set the server id with a ttl', async function () { - await this.ClsiCookieManager.promises.setServerId( + it('should set the server id with a ttl', function (done) { + this.ClsiCookieManager.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null - ) - this.redis.setex.should.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSeconds, - 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() + } ) }) - it('should set the server id with the regular ttl for reg instance', async function () { + it('should set the server id with the regular ttl for reg instance', function (done) { this.clsiServerId = 'clsi-reg-8' - await this.ClsiCookieManager.promises.setServerId( + this.ClsiCookieManager.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null - ) - expect(this.redis.setex).to.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSecondsRegular, - 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() + } ) }) - it('should not set the server id if clsiCookies are not enabled', async function () { + it('should not set the server id if clsiCookies are not enabled', function (done) { delete this.settings.clsiCookie.key this.ClsiCookieManager = SandboxedModule.require(modulePath, { globals: { @@ -205,19 +258,25 @@ describe('ClsiCookieManager', function () { }, requires: this.requires, })() - await this.ClsiCookieManager.promises.setServerId( + this.ClsiCookieManager.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null + null, + err => { + if (err) { + return done(err) + } + this.redis.setex.called.should.equal(false) + done() + } ) - this.redis.setex.called.should.equal(false) }) - it('should also set in the secondary if secondary redis is enabled', async function () { - this.redis_secondary = { setex: sinon.stub().resolves() } + it('should also set in the secondary if secondary redis is enabled', function (done) { + this.redis_secondary = { setex: sinon.stub().callsArg(3) } this.settings.redis.clsi_cookie_secondary = {} this.RedisWrapper.client = sinon.stub() this.RedisWrapper.client.withArgs('clsi_cookie').returns(this.redis) @@ -233,18 +292,74 @@ describe('ClsiCookieManager', function () { this.ClsiCookieManager._parseServerIdFromResponse = sinon .stub() .returns('clsi-8') - await this.ClsiCookieManager.promises.setServerId( + this.ClsiCookieManager.setServerId( this.project_id, this.user_id, 'standard', 'e2', this.clsiServerId, - null + 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() + } ) - this.redis_secondary.setex.should.have.been.calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, - this.settings.clsiCookie.ttlInSeconds, - this.clsiServerId + }) + }) + + 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() + } ) }) }) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 25af7926b1..aefa197a17 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -1,3 +1,4 @@ +/* eslint-disable mocha/handle-done-callback */ const sinon = require('sinon') const { expect } = require('chai') const modulePath = '../../../../app/src/Features/Compile/CompileController.js' @@ -18,15 +19,8 @@ describe('CompileController', function () { compileTimeout: 100, }, } - this.CompileManager = { - promises: { - compile: sinon.stub(), - getProjectCompileLimits: sinon.stub(), - }, - } - this.ClsiManager = { - promises: {}, - } + this.CompileManager = { compile: sinon.stub() } + this.ClsiManager = {} this.UserGetter = { getUser: sinon.stub() } this.rateLimiter = { consume: sinon.stub().resolves(), @@ -53,11 +47,10 @@ describe('CompileController', function () { }, } this.ClsiCookieManager = { - promises: { - getServerId: sinon.stub().resolves('clsi-server-id-from-redis'), - }, + getServerId: sinon.stub().yields(null, '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), @@ -83,9 +76,8 @@ describe('CompileController', function () { 'stream/promises': { pipeline: this.pipeline }, '@overleaf/settings': this.settings, '@overleaf/fetch-utils': this.fetchUtils, - '../Project/ProjectGetter': (this.ProjectGetter = { - promises: {}, - }), + request: (this.request = sinon.stub()), + '../Project/ProjectGetter': (this.ProjectGetter = {}), '@overleaf/metrics': (this.Metrics = { inc: sinon.stub(), Timer: class { @@ -129,23 +121,25 @@ describe('CompileController', function () { beforeEach(function () { this.req.params = { Project_id: this.projectId } this.req.session = {} - this.CompileManager.promises.compile = sinon.stub().resolves({ - status: (this.status = 'success'), - outputFiles: (this.outputFiles = [ + this.CompileManager.compile = sinon.stub().callsArgWith( + 3, + null, + (this.status = 'success'), + (this.outputFiles = [ { path: 'output.pdf', url: `/project/${this.projectId}/user/${this.user_id}/build/id/output.pdf`, type: 'pdf', }, ]), - clsiServerId: undefined, - limits: undefined, - validationProblems: undefined, - stats: undefined, - timings: undefined, - outputUrlPrefix: undefined, - buildId: this.build_id, - }) + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + this.build_id + ) }) describe('pdfDownloadDomain', function () { @@ -154,8 +148,9 @@ describe('CompileController', function () { }) describe('when clsi does not emit zone prefix', function () { - beforeEach(async function () { - await this.CompileController.compile(this.req, this.res, this.next) + beforeEach(function (done) { + this.res.callback = done + this.CompileController.compile(this.req, this.res, this.next) }) it('should add domain verbatim', function () { @@ -182,25 +177,28 @@ describe('CompileController', function () { }) describe('when clsi emits a zone prefix', function () { - beforeEach(async function () { - this.CompileManager.promises.compile = sinon.stub().resolves({ - status: (this.status = 'success'), - outputFiles: (this.outputFiles = [ + beforeEach(function (done) { + this.res.callback = done + this.CompileManager.compile = sinon.stub().callsArgWith( + 3, + null, + (this.status = 'success'), + (this.outputFiles = [ { path: 'output.pdf', url: `/project/${this.projectId}/user/${this.user_id}/build/id/output.pdf`, type: 'pdf', }, ]), - 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) + undefined, // clsiServerId + undefined, // limits + undefined, // validationProblems + undefined, // stats + undefined, // timings + '/zone/b', + this.build_id + ) + this.CompileController.compile(this.req, this.res, this.next) }) it('should add the zone prefix', function () { @@ -229,8 +227,9 @@ describe('CompileController', function () { }) describe('when not an auto compile', function () { - beforeEach(async function () { - await this.CompileController.compile(this.req, this.res, this.next) + beforeEach(function (done) { + this.res.callback = done + this.CompileController.compile(this.req, this.res, this.next) }) it('should look up the user id', function () { @@ -240,7 +239,7 @@ describe('CompileController', function () { }) it('should do the compile without the auto compile flag', function () { - this.CompileManager.promises.compile.should.have.been.calledWith( + this.CompileManager.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -276,13 +275,14 @@ describe('CompileController', function () { }) describe('when an auto compile', function () { - beforeEach(async function () { + beforeEach(function (done) { + this.res.callback = done this.req.query = { auto_compile: 'true' } - await this.CompileController.compile(this.req, this.res, this.next) + this.CompileController.compile(this.req, this.res, this.next) }) it('should do the compile with the auto compile flag', function () { - this.CompileManager.promises.compile.should.have.been.calledWith( + this.CompileManager.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -299,13 +299,14 @@ describe('CompileController', function () { }) describe('with the draft attribute', function () { - beforeEach(async function () { + beforeEach(function (done) { + this.res.callback = done this.req.body = { draft: true } - await this.CompileController.compile(this.req, this.res, this.next) + this.CompileController.compile(this.req, this.res, this.next) }) it('should do the compile without the draft compile flag', function () { - this.CompileManager.promises.compile.should.have.been.calledWith( + this.CompileManager.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -323,13 +324,14 @@ describe('CompileController', function () { }) describe('with an editor id', function () { - beforeEach(async function () { + beforeEach(function (done) { + this.res.callback = done this.req.body = { editorId: 'the-editor-id' } - await this.CompileController.compile(this.req, this.res, this.next) + this.CompileController.compile(this.req, this.res, this.next) }) it('should pass the editor id to the compiler', function () { - this.CompileManager.promises.compile.should.have.been.calledWith( + this.CompileManager.compile.should.have.been.calledWith( this.projectId, this.user_id, { @@ -351,29 +353,25 @@ describe('CompileController', function () { this.submission_id = 'sub-1234' this.req.params = { submission_id: this.submission_id } this.req.body = {} - this.ClsiManager.promises.sendExternalRequest = sinon.stub().resolves({ - status: (this.status = 'success'), - outputFiles: (this.outputFiles = ['mock-output-files']), - clsiServerId: 'mock-server-id', - validationProblems: null, - }) + this.ClsiManager.sendExternalRequest = sinon + .stub() + .callsArgWith( + 3, + null, + (this.status = 'success'), + (this.outputFiles = ['mock-output-files']), + (this.clsiServerId = 'mock-server-id'), + (this.validationProblems = null) + ) }) - it('should set the content-type of the response to application/json', async function () { - await this.CompileController.compileSubmission( - this.req, - this.res, - this.next - ) + it('should set the content-type of the response to application/json', function () { + 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', async function () { - await this.CompileController.compileSubmission( - this.req, - this.res, - this.next - ) + it('should send a successful response reporting the status and files', function () { + this.CompileController.compileSubmission(this.req, this.res, this.next) this.res.statusCode.should.equal(200) this.res.body.should.equal( JSON.stringify({ @@ -395,7 +393,7 @@ describe('CompileController', function () { }) it('should use the supplied values', function () { - this.ClsiManager.promises.sendExternalRequest.should.have.been.calledWith( + this.ClsiManager.sendExternalRequest.should.have.been.calledWith( this.submission_id, { compileGroup: 'special', timeout: 600 }, { compileGroup: 'special', compileBackendClass: 'n2d', timeout: 600 } @@ -415,7 +413,7 @@ describe('CompileController', function () { }) it('should use the other options but default values for compileGroup and timeout', function () { - this.ClsiManager.promises.sendExternalRequest.should.have.been.calledWith( + this.ClsiManager.sendExternalRequest.should.have.been.calledWith( this.submission_id, { rootResourcePath: 'main.tex', @@ -439,21 +437,24 @@ 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.promises.getProject = sinon + this.ProjectGetter.getProject = sinon .stub() - .resolves(this.project) + .callsArgWith(2, null, this.project) }) describe('when downloading for embedding', function () { - beforeEach(async function () { - await this.CompileController.downloadPdf(this.req, this.res, this.next) + beforeEach(function (done) { + this.CompileController.proxyToClsi = sinon + .stub() + .callsFake(() => done()) + this.CompileController.downloadPdf(this.req, this.res, this.next) }) it('should look up the project', function () { - this.ProjectGetter.promises.getProject + this.ProjectGetter.getProject .calledWith(this.projectId, { name: 1 }) .should.equal(true) }) @@ -473,66 +474,43 @@ 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.res, + this.next ) .should.equal(true) }) }) describe('when a build-id is provided', function () { - beforeEach(async function () { + beforeEach(function (done) { this.req.params.build_id = this.build_id - await this.CompileController.downloadPdf(this.req, this.res, this.next) + this.CompileController.proxyToClsi = sinon + .stub() + .callsFake(() => done()) + 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.res, + this.next ) .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 () { @@ -546,12 +524,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(async function () { - await this.CompileController.getFileFromClsiWithoutUser( + beforeEach(function () { + this.CompileController.getFileFromClsiWithoutUser( this.req, this.res, this.next @@ -559,12 +537,15 @@ 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', + } ) }) }) @@ -580,7 +561,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, @@ -596,7 +577,7 @@ describe('CompileController', function () { describe('proxySyncCode', function () { let file, line, column, imageName, editorId, buildId - beforeEach(async function () { + beforeEach(function (done) { this.req.params = { Project_id: this.projectId } file = 'main.tex' line = String(Date.now()) @@ -606,17 +587,17 @@ describe('CompileController', function () { this.req.query = { file, line, column, editorId, buildId } imageName = 'foo/bar:tag-0' - this.ProjectGetter.promises.getProject = sinon - .stub() - .resolves({ imageName }) + this.ProjectGetter.getProject = sinon.stub().yields(null, { imageName }) - this.CompileController._proxyToClsi = sinon.stub().resolves() + this.next.callsFake(done) + this.res.callback = done + this.CompileController.proxyToClsi = sinon.stub().callsFake(() => done()) - await this.CompileController.proxySyncCode(this.req, this.res, this.next) + 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`, @@ -630,7 +611,8 @@ describe('CompileController', function () { compileFromClsiCache: false, }, this.req, - this.res + this.res, + this.next ) }) }) @@ -638,7 +620,7 @@ describe('CompileController', function () { describe('proxySyncPdf', function () { let page, h, v, imageName, editorId, buildId - beforeEach(async function () { + beforeEach(function (done) { this.req.params = { Project_id: this.projectId } page = String(Date.now()) h = String(Math.random()) @@ -648,17 +630,17 @@ describe('CompileController', function () { this.req.query = { page, h, v, editorId, buildId } imageName = 'foo/bar:tag-1' - this.ProjectGetter.promises.getProject = sinon - .stub() - .resolves({ imageName }) + this.ProjectGetter.getProject = sinon.stub().yields(null, { imageName }) - this.CompileController._proxyToClsi = sinon.stub() + this.next.callsFake(done) + this.res.callback = done + this.CompileController.proxyToClsi = sinon.stub().callsFake(() => done()) - await this.CompileController.proxySyncPdf(this.req, this.res, this.next) + 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`, @@ -672,12 +654,13 @@ describe('CompileController', function () { compileFromClsiCache: false, }, this.req, - this.res + this.res, + this.next ) }) }) - describe('_proxyToClsi', function () { + describe('proxyToClsi', function () { beforeEach(function () { this.req.method = 'mock-method' this.req.headers = { @@ -690,14 +673,15 @@ describe('CompileController', function () { describe('old pdf viewer', function () { describe('user with standard priority', function () { - beforeEach(async function () { - this.CompileManager.promises.getProjectCompileLimits = sinon + beforeEach(function (done) { + this.res.callback = done + this.CompileManager.getProjectCompileLimits = sinon .stub() - .resolves({ + .callsArgWith(1, null, { compileGroup: 'standard', compileBackendClass: 'e2', }) - await this.CompileController._proxyToClsi( + this.CompileController.proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -720,14 +704,15 @@ describe('CompileController', function () { }) describe('user with priority compile', function () { - beforeEach(async function () { - this.CompileManager.promises.getProjectCompileLimits = sinon + beforeEach(function (done) { + this.res.callback = done + this.CompileManager.getProjectCompileLimits = sinon .stub() - .resolves({ + .callsArgWith(1, null, { compileGroup: 'priority', compileBackendClass: 'c2d', }) - await this.CompileController._proxyToClsi( + this.CompileController.proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -746,15 +731,16 @@ describe('CompileController', function () { }) describe('user with standard priority via query string', function () { - beforeEach(async function () { + beforeEach(function (done) { + this.res.callback = done this.req.query = { compileGroup: 'standard' } - this.CompileManager.promises.getProjectCompileLimits = sinon + this.CompileManager.getProjectCompileLimits = sinon .stub() - .resolves({ + .callsArgWith(1, null, { compileGroup: 'standard', compileBackendClass: 'e2', }) - await this.CompileController._proxyToClsi( + this.CompileController.proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -777,15 +763,16 @@ describe('CompileController', function () { }) describe('user with non-existent priority via query string', function () { - beforeEach(async function () { + beforeEach(function (done) { + this.res.callback = done this.req.query = { compileGroup: 'foobar' } - this.CompileManager.promises.getProjectCompileLimits = sinon + this.CompileManager.getProjectCompileLimits = sinon .stub() - .resolves({ + .callsArgWith(1, null, { compileGroup: 'standard', compileBackendClass: 'e2', }) - await this.CompileController._proxyToClsi( + this.CompileController.proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -804,15 +791,16 @@ describe('CompileController', function () { }) describe('user with build parameter via query string', function () { - beforeEach(async function () { - this.CompileManager.promises.getProjectCompileLimits = sinon + beforeEach(function (done) { + this.res.callback = done + this.CompileManager.getProjectCompileLimits = sinon .stub() - .resolves({ + .callsArgWith(1, null, { compileGroup: 'standard', compileBackendClass: 'e2', }) this.req.query = { build: 1234 } - await this.CompileController._proxyToClsi( + this.CompileController.proxyToClsi( this.projectId, 'output-file', (this.url = '/test'), @@ -833,16 +821,16 @@ describe('CompileController', function () { }) describe('deleteAuxFiles', function () { - beforeEach(async function () { - this.CompileManager.promises.deleteAuxFiles = sinon.stub().resolves() + beforeEach(function () { + this.CompileManager.deleteAuxFiles = sinon.stub().yields() this.req.params = { Project_id: this.projectId } this.req.query = { clsiserverid: 'node-1' } this.res.sendStatus = sinon.stub() - await this.CompileController.deleteAuxFiles(this.req, this.res, this.next) + this.CompileController.deleteAuxFiles(this.req, this.res, this.next) }) it('should proxy to the CLSI', function () { - this.CompileManager.promises.deleteAuxFiles + this.CompileManager.deleteAuxFiles .calledWith(this.projectId, this.user_id, 'node-1') .should.equal(true) }) @@ -860,25 +848,26 @@ describe('CompileController', function () { }, } this.downloadPath = `/project/${this.projectId}/build/123/output/output.pdf` - this.CompileManager.promises.compile.resolves({ - status: 'success', - outputFiles: [{ path: 'output.pdf', url: this.downloadPath }], - }) - this.CompileController._proxyToClsi = sinon.stub() + this.CompileManager.compile.callsArgWith(3, null, 'success', [ + { + 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', async function () { - await this.CompileController.compileAndDownloadPdf(this.req, this.res) - this.CompileManager.promises.compile - .calledWith(this.projectId) - .should.equal(true) + 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 proxy the res to the clsi with correct url', async function () { - await this.CompileController.compileAndDownloadPdf(this.req, this.res) + it('should proxy the res to the clsi with correct url', function (done) { + this.CompileController.compileAndDownloadPdf(this.req, this.res) sinon.assert.calledWith( - this.CompileController._proxyToClsi, + this.CompileController.proxyToClsi, this.projectId, 'output-file', this.downloadPath, @@ -887,7 +876,7 @@ describe('CompileController', function () { this.res ) - this.CompileController._proxyToClsi + this.CompileController.proxyToClsi .calledWith( this.projectId, 'output-file', @@ -897,44 +886,38 @@ describe('CompileController', function () { this.res ) .should.equal(true) + done() }) - 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 - ) + it('should not download anything on compilation failures', function () { + this.CompileManager.compile.yields(new Error('failed')) + 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 }) - 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) + it('should not download anything on missing pdf', function () { + this.CompileManager.compile.yields(null, 'success', []) + 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(async function () { - this.CompileManager.promises.wordCount = sinon + beforeEach(function () { + this.CompileManager.wordCount = sinon .stub() - .resolves({ content: 'body' }) + .yields(null, { 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() - await this.CompileController.wordCount(this.req, this.res, this.next) + this.CompileController.wordCount(this.req, this.res, this.next) }) it('should proxy to the CLSI', function () { - this.CompileManager.promises.wordCount + this.CompileManager.wordCount .calledWith(this.projectId, this.user_id, false, 'node-42') .should.equal(true) })