From 23279b11706f31d6a49065e8525ef06858c721ee Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 29 Aug 2025 09:39:40 -0400 Subject: [PATCH] Merge pull request #28182 from overleaf/em-promisify-http-controller Promisify HttpController in document-updater GitOrigin-RevId: fb87a38be856d64781401f7391b7f2a2c35c89fa --- .../app/js/DeleteQueueManager.js | 3 + .../app/js/DocumentManager.js | 3 +- .../document-updater/app/js/HttpController.js | 615 +++++++-------- .../DocumentManager/DocumentManagerTests.js | 4 +- .../js/HttpController/HttpControllerTests.js | 715 +++++++++--------- 5 files changed, 641 insertions(+), 699 deletions(-) diff --git a/services/document-updater/app/js/DeleteQueueManager.js b/services/document-updater/app/js/DeleteQueueManager.js index 39fa2750ec..5ba6d6c6e0 100644 --- a/services/document-updater/app/js/DeleteQueueManager.js +++ b/services/document-updater/app/js/DeleteQueueManager.js @@ -12,6 +12,7 @@ */ let DeleteQueueManager const Settings = require('@overleaf/settings') +const { promisifyAll } = require('@overleaf/promise-utils') const RedisManager = require('./RedisManager') const ProjectManager = require('./ProjectManager') const logger = require('@overleaf/logger') @@ -143,3 +144,5 @@ module.exports = DeleteQueueManager = { return doFlush() }, } + +DeleteQueueManager.promises = promisifyAll(DeleteQueueManager) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 442cf96164..dd91f8246c 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -481,7 +481,7 @@ const DocumentManager = { }) } - return { comment } + return comment }, async deleteComment(projectId, docId, commentId, userId) { @@ -823,7 +823,6 @@ module.exports = { 'projectHistoryId', 'type', ], - getCommentWithLock: ['comment'], }, }), promises: DocumentManager, diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 7e16a0b0dd..7c1e5d9de2 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -1,3 +1,4 @@ +const { expressify } = require('@overleaf/promise-utils') const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') const ProjectManager = require('./ProjectManager') @@ -8,12 +9,11 @@ const Settings = require('@overleaf/settings') const Metrics = require('./Metrics') const DeleteQueueManager = require('./DeleteQueueManager') const { getTotalSizeOfLines } = require('./Limits') -const async = require('async') const { StringFileData } = require('overleaf-editor-core') const { addTrackedDeletesToContent } = require('./Utils') const HistoryConversions = require('./HistoryConversions') -function getDoc(req, res, next) { +async function getDoc(req, res) { let fromVersion const docId = req.params.doc_id const projectId = req.params.project_id @@ -28,106 +28,99 @@ function getDoc(req, res, next) { fromVersion = -1 } - DocumentManager.getDocAndRecentOpsWithLock( - projectId, - docId, - fromVersion, - (error, lines, version, ops, ranges, pathname, _projectHistoryId, type) => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId, docId, historyRanges }, 'got doc via http') - if (lines == null || version == null) { - return next(new Errors.NotFoundError('document not found')) - } - if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { - const file = StringFileData.fromRaw(lines) - // TODO(24596): tc support for history-ot - lines = file.getLines() - } + let { lines, version, ops, ranges, pathname, type } = + await DocumentManager.promises.getDocAndRecentOpsWithLock( + projectId, + docId, + fromVersion + ) + timer.done() + logger.debug({ projectId, docId, historyRanges }, 'got doc via http') - if (historyRanges) { - const docContentWithTrackedDeletes = addTrackedDeletesToContent( - lines.join('\n'), - ranges?.changes ?? [] - ) - const docLinesWithTrackedDeletes = - docContentWithTrackedDeletes.split('\n') - const rangesWithTrackedDeletes = - HistoryConversions.toHistoryRanges(ranges) + if (lines == null || version == null) { + throw new Errors.NotFoundError('document not found') + } - res.json({ - id: docId, - lines: docLinesWithTrackedDeletes, - version, - ops, - ranges: rangesWithTrackedDeletes, - pathname, - ttlInS: RedisManager.DOC_OPS_TTL, - type, - }) - } else { - res.json({ - id: docId, - lines, - version, - ops, - ranges, - pathname, - ttlInS: RedisManager.DOC_OPS_TTL, - type, - }) - } - } - ) + if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { + const file = StringFileData.fromRaw(lines) + // TODO(24596): tc support for history-ot + lines = file.getLines() + } + + if (historyRanges) { + const docContentWithTrackedDeletes = addTrackedDeletesToContent( + lines.join('\n'), + ranges?.changes ?? [] + ) + const docLinesWithTrackedDeletes = docContentWithTrackedDeletes.split('\n') + const rangesWithTrackedDeletes = HistoryConversions.toHistoryRanges(ranges) + + res.json({ + id: docId, + lines: docLinesWithTrackedDeletes, + version, + ops, + ranges: rangesWithTrackedDeletes, + pathname, + ttlInS: RedisManager.DOC_OPS_TTL, + type, + }) + } else { + res.json({ + id: docId, + lines, + version, + ops, + ranges, + pathname, + ttlInS: RedisManager.DOC_OPS_TTL, + type, + }) + } } -function getComment(req, res, next) { +async function getComment(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id const commentId = req.params.comment_id logger.debug({ projectId, docId, commentId }, 'getting comment via http') - DocumentManager.getCommentWithLock( + const comment = await DocumentManager.promises.getCommentWithLock( projectId, docId, - commentId, - (error, comment) => { - if (error) { - return next(error) - } - if (comment == null) { - return next(new Errors.NotFoundError('comment not found')) - } - res.json(comment) - } + commentId ) + + if (comment == null) { + throw new Errors.NotFoundError('comment not found') + } + + res.json(comment) } // return the doc from redis if present, but don't load it from mongo -function peekDoc(req, res, next) { +async function peekDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id + logger.debug({ projectId, docId }, 'peeking at doc via http') - RedisManager.getDoc(projectId, docId, function (error, lines, version) { - if (error) { - return next(error) - } - if (lines == null || version == null) { - return next(new Errors.NotFoundError('document not found')) - } - if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { - const file = StringFileData.fromRaw(lines) - // TODO(24596): tc support for history-ot - lines = file.getLines() - } - res.json({ id: docId, lines, version }) - }) + let { lines, version } = await RedisManager.promises.getDoc(projectId, docId) + + if (lines == null || version == null) { + throw new Errors.NotFoundError('document not found') + } + + if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { + const file = StringFileData.fromRaw(lines) + // TODO(24596): tc support for history-ot + lines = file.getLines() + } + + res.json({ id: docId, lines, version }) } -function getProjectDocsAndFlushIfOld(req, res, next) { +async function getProjectDocsAndFlushIfOld(req, res) { const projectId = req.params.project_id const projectStateHash = req.query.state // exclude is string of existing docs "id:version,id:version,..." @@ -136,73 +129,73 @@ function getProjectDocsAndFlushIfOld(req, res, next) { logger.debug({ projectId, exclude: excludeItems }, 'getting docs via http') const timer = new Metrics.Timer('http.getAllDocs') const excludeVersions = {} + for (const item of excludeItems) { const [id, version] = item.split(':') excludeVersions[id] = version } + logger.debug( { projectId, projectStateHash, excludeVersions }, 'excluding versions' ) - ProjectManager.getProjectDocsAndFlushIfOld( - projectId, - projectStateHash, - excludeVersions, - (error, result) => { - timer.done() - if (error instanceof Errors.ProjectStateChangedError) { - res.sendStatus(409) // conflict - } else if (error) { - next(error) - } else { - logger.debug( - { - projectId, - result: result.map(doc => `${doc._id}:${doc.v}`), - }, - 'got docs via http' - ) - res.send(result) - } + + let result + try { + result = await ProjectManager.promises.getProjectDocsAndFlushIfOld( + projectId, + projectStateHash, + excludeVersions + ) + } catch (error) { + if (error instanceof Errors.ProjectStateChangedError) { + return res.sendStatus(409) // conflict + } else { + throw error } + } + + timer.done() + logger.debug( + { + projectId, + result: result.map(doc => `${doc._id}:${doc.v}`), + }, + 'got docs via http' ) + res.send(result) } -function getProjectLastUpdatedAt(req, res, next) { +async function getProjectLastUpdatedAt(req, res) { const projectId = req.params.project_id - ProjectManager.getProjectDocsTimestamps(projectId, (err, timestamps) => { - if (err) return next(err) + let timestamps = + await ProjectManager.promises.getProjectDocsTimestamps(projectId) - // Filter out nulls. This can happen when - // - docs get flushed between the listing and getting the individual docs ts - // - a doc flush failed half way (doc keys removed, project tracking not updated) - timestamps = timestamps.filter(ts => !!ts) + // Filter out nulls. This can happen when + // - docs get flushed between the listing and getting the individual docs ts + // - a doc flush failed half way (doc keys removed, project tracking not updated) + timestamps = timestamps.filter(ts => !!ts) - timestamps = timestamps.map(ts => parseInt(ts, 10)) - timestamps.sort((a, b) => (a > b ? 1 : -1)) - res.json({ lastUpdatedAt: timestamps.pop() }) - }) + timestamps = timestamps.map(ts => parseInt(ts, 10)) + timestamps.sort((a, b) => (a > b ? 1 : -1)) + res.json({ lastUpdatedAt: timestamps.pop() }) } -function clearProjectState(req, res, next) { +async function clearProjectState(req, res) { const projectId = req.params.project_id const timer = new Metrics.Timer('http.clearProjectState') logger.debug({ projectId }, 'clearing project state via http') - ProjectManager.clearProjectState(projectId, error => { - timer.done() - if (error) { - next(error) - } else { - res.sendStatus(200) - } - }) + await ProjectManager.promises.clearProjectState(projectId) + timer.done() + res.sendStatus(200) } -function setDoc(req, res, next) { +async function setDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id const { lines, source, user_id: userId, undoing } = req.body const lineSize = getTotalSizeOfLines(lines) + if (lineSize > Settings.max_doc_length) { logger.warn( { projectId, docId, source, lineSize, userId }, @@ -215,109 +208,97 @@ function setDoc(req, res, next) { 'setting doc via http' ) const timer = new Metrics.Timer('http.setDoc') - DocumentManager.setDocWithLock( + + const result = await DocumentManager.promises.setDocWithLock( projectId, docId, lines, source, userId, undoing, - true, - (error, result) => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId, docId }, 'set doc via http') - res.json(result) - } + true ) + timer.done() + logger.debug({ projectId, docId }, 'set doc via http') + res.json(result) } -function appendToDoc(req, res, next) { +async function appendToDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id const { lines, source, user_id: userId } = req.body const timer = new Metrics.Timer('http.appendToDoc') - DocumentManager.appendToDocWithLock( - projectId, - docId, - lines, - source, - userId, - (error, result) => { - timer.done() - if (error instanceof Errors.FileTooLargeError) { - logger.warn('refusing to append to file, it would become too large') - return res.sendStatus(422) - } - if (error) { - return next(error) - } - logger.debug( - { projectId, docId, lines, source, userId }, - 'appending to doc via http' - ) - res.json(result) + + let result + try { + result = await DocumentManager.promises.appendToDocWithLock( + projectId, + docId, + lines, + source, + userId + ) + } catch (error) { + if (error instanceof Errors.FileTooLargeError) { + logger.warn('refusing to append to file, it would become too large') + return res.sendStatus(422) + } else { + throw error } + } + + timer.done() + logger.debug( + { projectId, docId, lines, source, userId }, + 'appending to doc via http' ) + res.json(result) } -function flushDocIfLoaded(req, res, next) { +async function flushDocIfLoaded(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id logger.debug({ projectId, docId }, 'flushing doc via http') const timer = new Metrics.Timer('http.flushDoc') - DocumentManager.flushDocIfLoadedWithLock(projectId, docId, error => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId, docId }, 'flushed doc via http') - res.sendStatus(204) // No Content - }) + await DocumentManager.promises.flushDocIfLoadedWithLock(projectId, docId) + timer.done() + logger.debug({ projectId, docId }, 'flushed doc via http') + res.sendStatus(204) // No Content } -function deleteDoc(req, res, next) { +async function deleteDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' const timer = new Metrics.Timer('http.deleteDoc') logger.debug({ projectId, docId }, 'deleting doc via http') - DocumentManager.flushAndDeleteDocWithLock( - projectId, - docId, - { ignoreFlushErrors }, - error => { - timer.done() - // There is no harm in flushing project history if the previous call - // failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(projectId) - if (error) { - return next(error) - } - logger.debug({ projectId, docId }, 'deleted doc via http') - res.sendStatus(204) // No Content - } - ) + try { + await DocumentManager.promises.flushAndDeleteDocWithLock(projectId, docId, { + ignoreFlushErrors, + }) + } finally { + timer.done() + // There is no harm in flushing project history if the previous call + // failed and sometimes it is required + HistoryManager.flushProjectChangesAsync(projectId) + } + + logger.debug({ projectId, docId }, 'deleted doc via http') + res.sendStatus(204) // No Content } -function flushProject(req, res, next) { +async function flushProject(req, res) { const projectId = req.params.project_id logger.debug({ projectId }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') - ProjectManager.flushProjectWithLocks(projectId, error => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId }, 'flushed project via http') - res.sendStatus(204) // No Content - }) + await ProjectManager.promises.flushProjectWithLocks(projectId) + timer.done() + logger.debug({ projectId }, 'flushed project via http') + res.sendStatus(204) // No Content } -function deleteProject(req, res, next) { +async function deleteProject(req, res) { const projectId = req.params.project_id logger.debug({ projectId }, 'deleting project via http') const options = {} @@ -328,45 +309,32 @@ function deleteProject(req, res, next) { options.skip_history_flush = true } // don't flush history when realtime shuts down if (req.query.background) { - ProjectManager.queueFlushAndDeleteProject(projectId, error => { - if (error) { - return next(error) - } - logger.debug({ projectId }, 'queue delete of project via http') - res.sendStatus(204) - }) // No Content + await ProjectManager.promises.queueFlushAndDeleteProject(projectId) + logger.debug({ projectId }, 'queue delete of project via http') } else { const timer = new Metrics.Timer('http.deleteProject') - ProjectManager.flushAndDeleteProjectWithLocks(projectId, options, error => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId }, 'deleted project via http') - res.sendStatus(204) // No Content - }) + await ProjectManager.promises.flushAndDeleteProjectWithLocks( + projectId, + options + ) + timer.done() + logger.debug({ projectId }, 'deleted project via http') } + + res.sendStatus(204) } -function deleteMultipleProjects(req, res, next) { +async function deleteMultipleProjects(req, res) { const projectIds = req.body.project_ids || [] logger.debug({ projectIds }, 'deleting multiple projects via http') - async.eachSeries( - projectIds, - (projectId, cb) => { - logger.debug({ projectId }, 'queue delete of project via http') - ProjectManager.queueFlushAndDeleteProject(projectId, cb) - }, - error => { - if (error) { - return next(error) - } - res.sendStatus(204) // No Content - } - ) + for (const projectId of projectIds) { + logger.debug({ projectId }, 'queue delete of project via http') + await ProjectManager.promises.queueFlushAndDeleteProject(projectId) + } + res.sendStatus(204) // No Content } -function acceptChanges(req, res, next) { +async function acceptChanges(req, res) { const { project_id: projectId, doc_id: docId } = req.params let changeIds = req.body.change_ids if (changeIds == null) { @@ -377,20 +345,20 @@ function acceptChanges(req, res, next) { `accepting ${changeIds.length} changes via http` ) const timer = new Metrics.Timer('http.acceptChanges') - DocumentManager.acceptChangesWithLock(projectId, docId, changeIds, error => { - timer.done() - if (error) { - return next(error) - } - logger.debug( - { projectId, docId }, - `accepted ${changeIds.length} changes via http` - ) - res.sendStatus(204) // No Content - }) + await DocumentManager.promises.acceptChangesWithLock( + projectId, + docId, + changeIds + ) + timer.done() + logger.debug( + { projectId, docId }, + `accepted ${changeIds.length} changes via http` + ) + res.sendStatus(204) // No Content } -function rejectChanges(req, res, next) { +async function rejectChanges(req, res) { const { project_id: projectId, doc_id: docId } = req.params const changeIds = req.body.change_ids const userId = req.body.user_id @@ -399,25 +367,20 @@ function rejectChanges(req, res, next) { { projectId, docId }, `rejecting ${changeIds.length} changes via http` ) - DocumentManager.rejectChangesWithLock( + const response = await DocumentManager.promises.rejectChangesWithLock( projectId, docId, changeIds, - userId, - (error, response) => { - if (error) { - return next(error) - } - logger.debug( - { projectId, docId, changeIds, response }, - `rejected ${changeIds.length} changes via http` - ) - res.json(response) - } + userId ) + logger.debug( + { projectId, docId, changeIds, response }, + `rejected ${changeIds.length} changes via http` + ) + res.json(response) } -function resolveComment(req, res, next) { +async function resolveComment(req, res) { const { project_id: projectId, doc_id: docId, @@ -425,23 +388,18 @@ function resolveComment(req, res, next) { } = req.params const userId = req.body.user_id logger.debug({ projectId, docId, commentId }, 'resolving comment via http') - DocumentManager.updateCommentStateWithLock( + await DocumentManager.promises.updateCommentStateWithLock( projectId, docId, commentId, userId, - true, - error => { - if (error) { - return next(error) - } - logger.debug({ projectId, docId, commentId }, 'resolved comment via http') - res.sendStatus(204) // No Content - } + true ) + logger.debug({ projectId, docId, commentId }, 'resolved comment via http') + res.sendStatus(204) // No Content } -function reopenComment(req, res, next) { +async function reopenComment(req, res) { const { project_id: projectId, doc_id: docId, @@ -449,23 +407,18 @@ function reopenComment(req, res, next) { } = req.params const userId = req.body.user_id logger.debug({ projectId, docId, commentId }, 'reopening comment via http') - DocumentManager.updateCommentStateWithLock( + await DocumentManager.promises.updateCommentStateWithLock( projectId, docId, commentId, userId, - false, - error => { - if (error) { - return next(error) - } - logger.debug({ projectId, docId, commentId }, 'reopened comment via http') - res.sendStatus(204) // No Content - } + false ) + logger.debug({ projectId, docId, commentId }, 'reopened comment via http') + res.sendStatus(204) // No Content } -function deleteComment(req, res, next) { +async function deleteComment(req, res) { const { project_id: projectId, doc_id: docId, @@ -474,46 +427,36 @@ function deleteComment(req, res, next) { const userId = req.body.user_id logger.debug({ projectId, docId, commentId }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') - DocumentManager.deleteCommentWithLock( + await DocumentManager.promises.deleteCommentWithLock( projectId, docId, commentId, - userId, - error => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId, docId, commentId }, 'deleted comment via http') - res.sendStatus(204) // No Content - } + userId ) + timer.done() + logger.debug({ projectId, docId, commentId }, 'deleted comment via http') + res.sendStatus(204) // No Content } -function updateProject(req, res, next) { +async function updateProject(req, res) { const timer = new Metrics.Timer('http.updateProject') const projectId = req.params.project_id const { projectHistoryId, userId, updates = [], version, source } = req.body logger.debug({ projectId, updates, version }, 'updating project via http') - ProjectManager.updateProjectWithLocks( + await ProjectManager.promises.updateProjectWithLocks( projectId, projectHistoryId, userId, updates, version, - source, - error => { - timer.done() - if (error) { - return next(error) - } - logger.debug({ projectId }, 'updated project via http') - res.sendStatus(204) // No Content - } + source ) + timer.done() + logger.debug({ projectId }, 'updated project via http') + res.sendStatus(204) // No Content } -function resyncProjectHistory(req, res, next) { +async function resyncProjectHistory(req, res) { const projectId = req.params.project_id const { projectHistoryId, @@ -536,38 +479,36 @@ function resyncProjectHistory(req, res, next) { opts.resyncProjectStructureOnly = resyncProjectStructureOnly } - HistoryManager.resyncProjectHistory( + await HistoryManager.promises.resyncProjectHistory( projectId, projectHistoryId, docs, files, - opts, - error => { - if (error) { - return next(error) - } - logger.debug({ projectId }, 'queued project history resync via http') - res.sendStatus(204) - } + opts ) + logger.debug({ projectId }, 'queued project history resync via http') + res.sendStatus(204) } -function flushQueuedProjects(req, res, next) { +async function flushQueuedProjects(req, res) { res.setTimeout(10 * 60 * 1000) const options = { limit: req.query.limit || 1000, timeout: 5 * 60 * 1000, min_delete_age: req.query.min_delete_age || 5 * 60 * 1000, } - DeleteQueueManager.flushAndDeleteOldProjects(options, (err, flushed) => { - if (err) { - logger.err({ err }, 'error flushing old projects') - res.sendStatus(500) - } else { - logger.info({ flushed }, 'flush of queued projects completed') - res.send({ flushed }) + await DeleteQueueManager.promises.flushAndDeleteOldProjects( + options, + (err, flushed) => { + if (err) { + logger.err({ err }, 'error flushing old projects') + res.sendStatus(500) + } else { + logger.info({ flushed }, 'flush of queued projects completed') + res.send({ flushed }) + } } - }) + ) } /** @@ -576,51 +517,43 @@ function flushQueuedProjects(req, res, next) { * The project is blocked only if it's not already loaded in docupdater. The * response indicates whether the project has been blocked or not. */ -function blockProject(req, res, next) { +async function blockProject(req, res) { const projectId = req.params.project_id - RedisManager.blockProject(projectId, (err, blocked) => { - if (err) { - return next(err) - } - res.json({ blocked }) - }) + const blocked = await RedisManager.promises.blockProject(projectId) + res.json({ blocked }) } /** * Unblock a project */ -function unblockProject(req, res, next) { +async function unblockProject(req, res) { const projectId = req.params.project_id - RedisManager.unblockProject(projectId, (err, wasBlocked) => { - if (err) { - return next(err) - } - res.json({ wasBlocked }) - }) + const wasBlocked = await RedisManager.promises.unblockProject(projectId) + res.json({ wasBlocked }) } module.exports = { - getDoc, - peekDoc, - getProjectDocsAndFlushIfOld, - getProjectLastUpdatedAt, - clearProjectState, - appendToDoc, - setDoc, - flushDocIfLoaded, - deleteDoc, - flushProject, - deleteProject, - deleteMultipleProjects, - acceptChanges, - rejectChanges, - resolveComment, - reopenComment, - deleteComment, - updateProject, - resyncProjectHistory, - flushQueuedProjects, - blockProject, - unblockProject, - getComment, + getDoc: expressify(getDoc), + peekDoc: expressify(peekDoc), + getProjectDocsAndFlushIfOld: expressify(getProjectDocsAndFlushIfOld), + getProjectLastUpdatedAt: expressify(getProjectLastUpdatedAt), + clearProjectState: expressify(clearProjectState), + appendToDoc: expressify(appendToDoc), + setDoc: expressify(setDoc), + flushDocIfLoaded: expressify(flushDocIfLoaded), + deleteDoc: expressify(deleteDoc), + flushProject: expressify(flushProject), + deleteProject: expressify(deleteProject), + deleteMultipleProjects: expressify(deleteMultipleProjects), + acceptChanges: expressify(acceptChanges), + rejectChanges: expressify(rejectChanges), + resolveComment: expressify(resolveComment), + reopenComment: expressify(reopenComment), + deleteComment: expressify(deleteComment), + updateProject: expressify(updateProject), + resyncProjectHistory: expressify(resyncProjectHistory), + flushQueuedProjects: expressify(flushQueuedProjects), + blockProject: expressify(blockProject), + unblockProject: expressify(unblockProject), + getComment: expressify(getComment), } diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 1816579103..7a394c347b 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -870,9 +870,7 @@ describe('DocumentManager', function () { this.doc_id, 'mock-comment-id-1' ) - ).to.eventually.deep.equal({ - comment: { id: 'mock-comment-id-1' }, - }) + ).to.eventually.deep.equal({ id: 'mock-comment-id-1' }) }) it("should get the document's current ranges", function () { diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 9f99659d9e..58db0a80f8 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -5,32 +5,6 @@ const Errors = require('../../../../app/js/Errors.js') describe('HttpController', function () { beforeEach(function () { - this.HttpController = SandboxedModule.require(modulePath, { - requires: { - './DocumentManager': (this.DocumentManager = {}), - './HistoryManager': (this.HistoryManager = { - flushProjectChangesAsync: sinon.stub(), - }), - './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), - './ProjectManager': (this.ProjectManager = {}), - './DeleteQueueManager': (this.DeleteQueueManager = {}), - './RedisManager': (this.RedisManager = { - DOC_OPS_TTL: 42, - }), - './Metrics': (this.Metrics = {}), - './Errors': Errors, - './Utils': (this.Utils = { - addTrackedDeletesToContent: sinon.stub().returnsArg(0), - }), - './HistoryConversions': (this.HistoryConversions = { - toHistoryRanges: sinon.stub().returnsArg(0), - }), - '@overleaf/settings': { max_doc_length: 2 * 1024 * 1024 }, - }, - }) - this.Metrics.Timer = class Timer {} - this.Metrics.Timer.prototype.done = sinon.stub() - this.project_id = 'project-id-123' this.projectHistoryId = '123' this.doc_id = 'doc-id-123' @@ -41,6 +15,78 @@ describe('HttpController', function () { sendStatus: sinon.stub(), json: sinon.stub(), } + + this.DocumentManager = { + promises: { + getDocAndRecentOpsWithLock: sinon.stub(), + getCommentWithLock: sinon.stub(), + setDocWithLock: sinon.stub(), + flushDocIfLoadedWithLock: sinon.stub().resolves(), + flushAndDeleteDocWithLock: sinon.stub().resolves(), + acceptChangesWithLock: sinon.stub().resolves(), + updateCommentStateWithLock: sinon.stub().resolves(), + deleteCommentWithLock: sinon.stub().resolves(), + appendToDocWithLock: sinon.stub(), + }, + } + + this.HistoryManager = { + flushProjectChangesAsync: sinon.stub(), + promises: { + resyncProjectHistory: sinon.stub().resolves(), + }, + } + + this.ProjectHistoryRedisManager = { + promises: { + queueOps: sinon.stub().resolves(), + }, + } + + this.ProjectManager = { + promises: { + flushProjectWithLocks: sinon.stub().resolves(), + flushAndDeleteProjectWithLocks: sinon.stub().resolves(), + queueFlushAndDeleteProject: sinon.stub().resolves(), + getProjectDocsAndFlushIfOld: sinon.stub(), + updateProjectWithLocks: sinon.stub().resolves(), + }, + } + + this.DeleteQueueManager = {} + + this.RedisManager = { + DOC_OPS_TTL: 42, + } + + this.Metrics = { + Timer: class Timer {}, + } + this.Metrics.Timer.prototype.done = sinon.stub() + + this.Utils = { + addTrackedDeletesToContent: sinon.stub().returnsArg(0), + } + + this.HistoryConversions = { + toHistoryRanges: sinon.stub().returnsArg(0), + } + + this.HttpController = SandboxedModule.require(modulePath, { + requires: { + './DocumentManager': this.DocumentManager, + './HistoryManager': this.HistoryManager, + './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager, + './ProjectManager': this.ProjectManager, + './DeleteQueueManager': this.DeleteQueueManager, + './RedisManager': this.RedisManager, + './Metrics': this.Metrics, + './Errors': Errors, + './Utils': this.Utils, + './HistoryConversions': this.HistoryConversions, + '@overleaf/settings': { max_doc_length: 2 * 1024 * 1024 }, + }, + }) }) describe('getDoc', function () { @@ -62,27 +108,25 @@ describe('HttpController', function () { }) describe('when the document exists and no recent ops are requested', function () { - beforeEach(function () { - this.DocumentManager.getDocAndRecentOpsWithLock = sinon - .stub() - .callsArgWith( - 3, - null, - this.lines, - this.version, - [], - this.ranges, - this.pathname, - this.projectHistoryId, - 'sharejs-text-ot' - ) - this.HttpController.getDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.getDocAndRecentOpsWithLock.resolves({ + lines: this.lines, + version: this.version, + ops: [], + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + type: 'sharejs-text-ot', + }) + await this.HttpController.getDoc(this.req, this.res, this.next) }) it('should get the doc', function () { - this.DocumentManager.getDocAndRecentOpsWithLock - .calledWith(this.project_id, this.doc_id, -1) - .should.equal(true) + this.DocumentManager.promises.getDocAndRecentOpsWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + -1 + ) }) it('should return the doc as JSON', function () { @@ -117,28 +161,26 @@ describe('HttpController', function () { }) describe('when recent ops are requested', function () { - beforeEach(function () { - this.DocumentManager.getDocAndRecentOpsWithLock = sinon - .stub() - .callsArgWith( - 3, - null, - this.lines, - this.version, - this.ops, - this.ranges, - this.pathname, - this.projectHistoryId, - 'sharejs-text-ot' - ) + beforeEach(async function () { + this.DocumentManager.promises.getDocAndRecentOpsWithLock.resolves({ + lines: this.lines, + version: this.version, + ops: this.ops, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + type: 'sharejs-text-ot', + }) this.req.query = { fromVersion: `${this.fromVersion}` } - this.HttpController.getDoc(this.req, this.res, this.next) + await this.HttpController.getDoc(this.req, this.res, this.next) }) it('should get the doc', function () { - this.DocumentManager.getDocAndRecentOpsWithLock - .calledWith(this.project_id, this.doc_id, this.fromVersion) - .should.equal(true) + this.DocumentManager.promises.getDocAndRecentOpsWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.fromVersion + ) }) it('should return the doc as JSON', function () { @@ -173,28 +215,26 @@ describe('HttpController', function () { }) describe('when historyRanges query param is true', function () { - beforeEach(function () { - this.DocumentManager.getDocAndRecentOpsWithLock = sinon - .stub() - .callsArgWith( - 3, - null, - this.lines, - this.version, - [], - this.ranges, - this.pathname, - this.projectHistoryId, - 'sharejs-text-ot' - ) + beforeEach(async function () { + this.DocumentManager.promises.getDocAndRecentOpsWithLock.resolves({ + lines: this.lines, + version: this.version, + ops: [], + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + type: 'sharejs-text-ot', + }) this.req.query = { historyRanges: 'true' } - this.HttpController.getDoc(this.req, this.res, this.next) + await this.HttpController.getDoc(this.req, this.res, this.next) }) it('should get the doc', function () { - this.DocumentManager.getDocAndRecentOpsWithLock - .calledWith(this.project_id, this.doc_id, -1) - .should.equal(true) + this.DocumentManager.promises.getDocAndRecentOpsWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + -1 + ) }) it('should return the doc as JSON with history ranges processing', function () { @@ -237,11 +277,12 @@ describe('HttpController', function () { }) describe('when the document does not exist', function () { - beforeEach(function () { - this.DocumentManager.getDocAndRecentOpsWithLock = sinon - .stub() - .callsArgWith(3, null, null, null) - this.HttpController.getDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.getDocAndRecentOpsWithLock.resolves({ + lines: null, + version: null, + }) + await this.HttpController.getDoc(this.req, this.res, this.next) }) it('should call next with NotFoundError', function () { @@ -252,11 +293,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.getDocAndRecentOpsWithLock = sinon - .stub() - .callsArgWith(3, new Error('oops'), null, null) - this.HttpController.getDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.getDocAndRecentOpsWithLock.rejects( + new Error('oops') + ) + await this.HttpController.getDoc(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -289,17 +330,19 @@ describe('HttpController', function () { } }) - beforeEach(function () { - this.DocumentManager.getCommentWithLock = sinon - .stub() - .callsArgWith(3, null, this.ranges.comments[0]) - this.HttpController.getComment(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.getCommentWithLock.resolves( + this.ranges.comments[0] + ) + await this.HttpController.getComment(this.req, this.res, this.next) }) it('should get the comment', function () { - this.DocumentManager.getCommentWithLock - .calledWith(this.project_id, this.doc_id, this.comment_id) - .should.equal(true) + this.DocumentManager.promises.getCommentWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.comment_id + ) }) it('should return the comment as JSON', function () { @@ -346,25 +389,21 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.setDocWithLock = sinon - .stub() - .callsArgWith(7, null, { rev: '123' }) - this.HttpController.setDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.setDocWithLock.resolves({ rev: '123' }) + await this.HttpController.setDoc(this.req, this.res, this.next) }) it('should set the doc', function () { - this.DocumentManager.setDocWithLock - .calledWith( - this.project_id, - this.doc_id, - this.lines, - this.source, - this.user_id, - this.undoing, - true - ) - .should.equal(true) + this.DocumentManager.promises.setDocWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.lines, + this.source, + this.user_id, + this.undoing, + true + ) }) it('should return a json response with the document rev from web', function () { @@ -393,11 +432,9 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.setDocWithLock = sinon - .stub() - .callsArgWith(7, new Error('oops')) - this.HttpController.setDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.setDocWithLock.rejects(new Error('oops')) + await this.HttpController.setDoc(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -406,14 +443,14 @@ describe('HttpController', function () { }) describe('when the payload is too large', function () { - beforeEach(function () { + beforeEach(async function () { const lines = [] for (let _ = 0; _ <= 200000; _++) { lines.push('test test test') } this.req.body.lines = lines - this.DocumentManager.setDocWithLock = sinon.stub().callsArgWith(6) - this.HttpController.setDoc(this.req, this.res, this.next) + this.DocumentManager.promises.setDocWithLock.resolves() + await this.HttpController.setDoc(this.req, this.res, this.next) }) it('should send back a 406 response', function () { @@ -421,7 +458,7 @@ describe('HttpController', function () { }) it('should not call setDocWithLock', function () { - this.DocumentManager.setDocWithLock.callCount.should.equal(0) + this.DocumentManager.promises.setDocWithLock.should.not.have.been.called }) }) }) @@ -438,15 +475,14 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.ProjectManager.flushProjectWithLocks = sinon.stub().callsArgWith(1) - this.HttpController.flushProject(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.flushProject(this.req, this.res, this.next) }) it('should flush the project', function () { - this.ProjectManager.flushProjectWithLocks - .calledWith(this.project_id) - .should.equal(true) + this.ProjectManager.promises.flushProjectWithLocks.should.have.been.calledWith( + this.project_id + ) }) it('should return a successful No Content response', function () { @@ -468,11 +504,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.ProjectManager.flushProjectWithLocks = sinon - .stub() - .callsArgWith(1, new Error('oops')) - this.HttpController.flushProject(this.req, this.res, this.next) + beforeEach(async function () { + this.ProjectManager.promises.flushProjectWithLocks.rejects( + new Error('oops') + ) + await this.HttpController.flushProject(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -496,17 +532,19 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.flushDocIfLoadedWithLock = sinon - .stub() - .callsArgWith(2) - this.HttpController.flushDocIfLoaded(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.flushDocIfLoaded( + this.req, + this.res, + this.next + ) }) it('should flush the doc', function () { - this.DocumentManager.flushDocIfLoadedWithLock - .calledWith(this.project_id, this.doc_id) - .should.equal(true) + this.DocumentManager.promises.flushDocIfLoadedWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id + ) }) it('should return a successful No Content response', function () { @@ -528,11 +566,15 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.flushDocIfLoadedWithLock = sinon - .stub() - .callsArgWith(2, new Error('oops')) - this.HttpController.flushDocIfLoaded(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.flushDocIfLoadedWithLock.rejects( + new Error('oops') + ) + await this.HttpController.flushDocIfLoaded( + this.req, + this.res, + this.next + ) }) it('should call next with the error', function () { @@ -554,19 +596,16 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.flushAndDeleteDocWithLock = sinon - .stub() - .callsArgWith(3) - this.HttpController.deleteDoc(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should flush and delete the doc', function () { - this.DocumentManager.flushAndDeleteDocWithLock - .calledWith(this.project_id, this.doc_id, { - ignoreFlushErrors: false, - }) - .should.equal(true) + this.DocumentManager.promises.flushAndDeleteDocWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + { ignoreFlushErrors: false } + ) }) it('should flush project history', function () { @@ -594,16 +633,17 @@ describe('HttpController', function () { }) describe('ignoring errors', function () { - beforeEach(function () { + beforeEach(async function () { this.req.query.ignore_flush_errors = 'true' - this.DocumentManager.flushAndDeleteDocWithLock = sinon.stub().yields() - this.HttpController.deleteDoc(this.req, this.res, this.next) + await this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should delete the doc', function () { - this.DocumentManager.flushAndDeleteDocWithLock - .calledWith(this.project_id, this.doc_id, { ignoreFlushErrors: true }) - .should.equal(true) + this.DocumentManager.promises.flushAndDeleteDocWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + { ignoreFlushErrors: true } + ) }) it('should return a successful No Content response', function () { @@ -612,11 +652,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.flushAndDeleteDocWithLock = sinon - .stub() - .callsArgWith(3, new Error('oops')) - this.HttpController.deleteDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.flushAndDeleteDocWithLock.rejects( + new Error('oops') + ) + await this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should flush project history', function () { @@ -643,17 +683,14 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.ProjectManager.flushAndDeleteProjectWithLocks = sinon - .stub() - .callsArgWith(2) - this.HttpController.deleteProject(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.deleteProject(this.req, this.res, this.next) }) it('should delete the project', function () { - this.ProjectManager.flushAndDeleteProjectWithLocks - .calledWith(this.project_id) - .should.equal(true) + this.ProjectManager.promises.flushAndDeleteProjectWithLocks.should.have.been.calledWith( + this.project_id + ) }) it('should return a successful No Content response', function () { @@ -675,27 +712,24 @@ describe('HttpController', function () { }) describe('with the background=true option from realtime', function () { - beforeEach(function () { - this.ProjectManager.queueFlushAndDeleteProject = sinon - .stub() - .callsArgWith(1) + beforeEach(async function () { this.req.query = { background: true, shutdown: true } - this.HttpController.deleteProject(this.req, this.res, this.next) + await this.HttpController.deleteProject(this.req, this.res, this.next) }) it('should queue the flush and delete', function () { - this.ProjectManager.queueFlushAndDeleteProject - .calledWith(this.project_id) - .should.equal(true) + this.ProjectManager.promises.queueFlushAndDeleteProject.should.have.been.calledWith( + this.project_id + ) }) }) describe('when an errors occurs', function () { - beforeEach(function () { - this.ProjectManager.flushAndDeleteProjectWithLocks = sinon - .stub() - .callsArgWith(2, new Error('oops')) - this.HttpController.deleteProject(this.req, this.res, this.next) + beforeEach(async function () { + this.ProjectManager.promises.flushAndDeleteProjectWithLocks.rejects( + new Error('oops') + ) + await this.HttpController.deleteProject(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -718,17 +752,16 @@ describe('HttpController', function () { }) describe('successfully with a single change', function () { - beforeEach(function () { - this.DocumentManager.acceptChangesWithLock = sinon - .stub() - .callsArgWith(3) - this.HttpController.acceptChanges(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.acceptChanges(this.req, this.res, this.next) }) it('should accept the change', function () { - this.DocumentManager.acceptChangesWithLock - .calledWith(this.project_id, this.doc_id, [this.change_id]) - .should.equal(true) + this.DocumentManager.promises.acceptChangesWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + [this.change_id] + ) }) it('should return a successful No Content response', function () { @@ -750,7 +783,7 @@ describe('HttpController', function () { }) describe('succesfully with with multiple changes', function () { - beforeEach(function () { + beforeEach(async function () { this.change_ids = [ 'mock-change-od-1', 'mock-change-od-2', @@ -758,16 +791,15 @@ describe('HttpController', function () { 'mock-change-od-4', ] this.req.body = { change_ids: this.change_ids } - this.DocumentManager.acceptChangesWithLock = sinon - .stub() - .callsArgWith(3) - this.HttpController.acceptChanges(this.req, this.res, this.next) + await this.HttpController.acceptChanges(this.req, this.res, this.next) }) it('should accept the changes in the body payload', function () { - this.DocumentManager.acceptChangesWithLock - .calledWith(this.project_id, this.doc_id, this.change_ids) - .should.equal(true) + this.DocumentManager.promises.acceptChangesWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.change_ids + ) }) it('should log the request with the correct number of changes', function () { @@ -781,11 +813,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.acceptChangesWithLock = sinon - .stub() - .callsArgWith(3, new Error('oops')) - this.HttpController.acceptChanges(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.acceptChangesWithLock.rejects( + new Error('oops') + ) + await this.HttpController.acceptChanges(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -812,26 +844,18 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function (done) { - this.DocumentManager.updateCommentStateWithLock = sinon - .stub() - .callsArgWith(5) - - this.ProjectHistoryRedisManager.queueOps = sinon.stub() - this.res.sendStatus.callsFake(() => done()) - this.HttpController.resolveComment(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.resolveComment(this.req, this.res, this.next) }) it('should accept the change', function () { - this.DocumentManager.updateCommentStateWithLock - .calledWith( - this.project_id, - this.doc_id, - this.comment_id, - this.user_id, - this.resolved - ) - .should.equal(true) + this.DocumentManager.promises.updateCommentStateWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.comment_id, + this.user_id, + this.resolved + ) }) it('should return a successful No Content response', function () { @@ -853,11 +877,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.updateCommentStateWithLock = sinon - .stub() - .callsArgWith(5, new Error('oops')) - this.HttpController.resolveComment(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.updateCommentStateWithLock.rejects( + new Error('oops') + ) + await this.HttpController.resolveComment(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -884,25 +908,18 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.updateCommentStateWithLock = sinon - .stub() - .callsArgWith(5) - - this.ProjectHistoryRedisManager.queueOps = sinon.stub() - this.HttpController.reopenComment(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.reopenComment(this.req, this.res, this.next) }) it('should accept the change', function () { - this.DocumentManager.updateCommentStateWithLock - .calledWith( - this.project_id, - this.doc_id, - this.comment_id, - this.user_id, - this.resolved - ) - .should.equal(true) + this.DocumentManager.promises.updateCommentStateWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.comment_id, + this.user_id, + this.resolved + ) }) it('should return a successful No Content response', function () { @@ -924,11 +941,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.updateCommentStateWithLock = sinon - .stub() - .callsArgWith(5, new Error('oops')) - this.HttpController.reopenComment(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.updateCommentStateWithLock.rejects( + new Error('oops') + ) + await this.HttpController.reopenComment(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -954,24 +971,17 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.deleteCommentWithLock = sinon - .stub() - .callsArgWith(4) - - this.ProjectHistoryRedisManager.queueOps = sinon.stub() - this.HttpController.deleteComment(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.deleteComment(this.req, this.res, this.next) }) it('should accept the change', function () { - this.DocumentManager.deleteCommentWithLock - .calledWith( - this.project_id, - this.doc_id, - this.comment_id, - this.user_id - ) - .should.equal(true) + this.DocumentManager.promises.deleteCommentWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.comment_id, + this.user_id + ) }) it('should return a successful No Content response', function () { @@ -997,11 +1007,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.deleteCommentWithLock = sinon - .stub() - .callsArgWith(4, new Error('oops')) - this.HttpController.deleteComment(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.deleteCommentWithLock.rejects( + new Error('oops') + ) + await this.HttpController.deleteComment(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -1029,11 +1039,11 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.ProjectManager.getProjectDocsAndFlushIfOld = sinon - .stub() - .callsArgWith(3, null, this.docs) - this.HttpController.getProjectDocsAndFlushIfOld( + beforeEach(async function () { + this.ProjectManager.promises.getProjectDocsAndFlushIfOld.resolves( + this.docs + ) + await this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next @@ -1041,9 +1051,11 @@ describe('HttpController', function () { }) it('should get docs from the project manager', function () { - this.ProjectManager.getProjectDocsAndFlushIfOld - .calledWith(this.project_id, this.state, {}) - .should.equal(true) + this.ProjectManager.promises.getProjectDocsAndFlushIfOld.should.have.been.calledWith( + this.project_id, + this.state, + {} + ) }) it('should return a successful response', function () { @@ -1074,14 +1086,11 @@ describe('HttpController', function () { }) describe('when there is a conflict', function () { - beforeEach(function () { - this.ProjectManager.getProjectDocsAndFlushIfOld = sinon - .stub() - .callsArgWith( - 3, - new Errors.ProjectStateChangedError('project state changed') - ) - this.HttpController.getProjectDocsAndFlushIfOld( + beforeEach(async function () { + this.ProjectManager.promises.getProjectDocsAndFlushIfOld.rejects( + new Errors.ProjectStateChangedError('project state changed') + ) + await this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next @@ -1094,11 +1103,11 @@ describe('HttpController', function () { }) describe('when an error occurs', function () { - beforeEach(function () { - this.ProjectManager.getProjectDocsAndFlushIfOld = sinon - .stub() - .callsArgWith(3, new Error('oops')) - this.HttpController.getProjectDocsAndFlushIfOld( + beforeEach(async function () { + this.ProjectManager.promises.getProjectDocsAndFlushIfOld.rejects( + new Error('oops') + ) + await this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next @@ -1148,22 +1157,19 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.ProjectManager.updateProjectWithLocks = sinon.stub().yields() - this.HttpController.updateProject(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.updateProject(this.req, this.res, this.next) }) it('should accept the change', function () { - this.ProjectManager.updateProjectWithLocks - .calledWith( - this.project_id, - this.projectHistoryId, - this.userId, - this.updates, - this.version, - this.source - ) - .should.equal(true) + this.ProjectManager.promises.updateProjectWithLocks.should.have.been.calledWith( + this.project_id, + this.projectHistoryId, + this.userId, + this.updates, + this.version, + this.source + ) }) it('should return a successful No Content response', function () { @@ -1176,11 +1182,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.ProjectManager.updateProjectWithLocks = sinon - .stub() - .yields(new Error('oops')) - this.HttpController.updateProject(this.req, this.res, this.next) + beforeEach(async function () { + this.ProjectManager.promises.updateProjectWithLocks.rejects( + new Error('oops') + ) + await this.HttpController.updateProject(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -1209,21 +1215,22 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.HistoryManager.resyncProjectHistory = sinon.stub().callsArgWith(5) - this.HttpController.resyncProjectHistory(this.req, this.res, this.next) + beforeEach(async function () { + await this.HttpController.resyncProjectHistory( + this.req, + this.res, + this.next + ) }) it('should accept the change', function () { - this.HistoryManager.resyncProjectHistory - .calledWith( - this.project_id, - this.projectHistoryId, - this.docs, - this.files, - {} - ) - .should.equal(true) + this.HistoryManager.promises.resyncProjectHistory.should.have.been.calledWith( + this.project_id, + this.projectHistoryId, + this.docs, + this.files, + {} + ) }) it('should return a successful No Content response', function () { @@ -1232,11 +1239,15 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.HistoryManager.resyncProjectHistory = sinon - .stub() - .callsArgWith(5, new Error('oops')) - this.HttpController.resyncProjectHistory(this.req, this.res, this.next) + beforeEach(async function () { + this.HistoryManager.promises.resyncProjectHistory.rejects( + new Error('oops') + ) + await this.HttpController.resyncProjectHistory( + this.req, + this.res, + this.next + ) }) it('should call next with the error', function () { @@ -1267,23 +1278,21 @@ describe('HttpController', function () { }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.appendToDocWithLock = sinon - .stub() - .callsArgWith(5, null, { rev: '123' }) - this.HttpController.appendToDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.appendToDocWithLock.resolves({ + rev: '123', + }) + await this.HttpController.appendToDoc(this.req, this.res, this.next) }) it('should append to the doc', function () { - this.DocumentManager.appendToDocWithLock - .calledWith( - this.project_id, - this.doc_id, - this.lines, - this.source, - this.user_id - ) - .should.equal(true) + this.DocumentManager.promises.appendToDocWithLock.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.lines, + this.source, + this.user_id + ) }) it('should return a json response with the document rev from web', function () { @@ -1311,11 +1320,11 @@ describe('HttpController', function () { }) describe('when an errors occurs', function () { - beforeEach(function () { - this.DocumentManager.appendToDocWithLock = sinon - .stub() - .callsArgWith(5, new Error('oops')) - this.HttpController.appendToDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.appendToDocWithLock.rejects( + new Error('oops') + ) + await this.HttpController.appendToDoc(this.req, this.res, this.next) }) it('should call next with the error', function () { @@ -1324,11 +1333,11 @@ describe('HttpController', function () { }) describe('when the payload is too large', function () { - beforeEach(function () { - this.DocumentManager.appendToDocWithLock = sinon - .stub() - .callsArgWith(5, new Errors.FileTooLargeError()) - this.HttpController.appendToDoc(this.req, this.res, this.next) + beforeEach(async function () { + this.DocumentManager.promises.appendToDocWithLock.rejects( + new Errors.FileTooLargeError() + ) + await this.HttpController.appendToDoc(this.req, this.res, this.next) }) it('should send back a 422 response', function () {