From 2ccc1aeea53dada3d881454ef7d41df601356f72 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Tue, 6 Jun 2023 12:19:55 +0200 Subject: [PATCH] Merge pull request #13264 from overleaf/msm-docupdater-cleanup [docupdater] Cleanup track-changes code GitOrigin-RevId: 906a4f57a9e7348f47af67cff44a32ae1bb2debb --- .../app/js/DocumentManager.js | 41 +-- .../document-updater/app/js/HistoryManager.js | 173 ++-------- .../app/js/HistoryRedisManager.js | 41 --- .../app/js/PersistenceManager.js | 3 +- .../document-updater/app/js/RedisManager.js | 318 +++++++----------- .../document-updater/app/js/UpdateManager.js | 281 +++++++--------- .../config/settings.defaults.js | 19 -- .../js/ApplyingUpdatesToADocTests.js | 290 ++++------------ .../acceptance/js/DeletingADocumentTests.js | 108 +++--- .../acceptance/js/DeletingAProjectTests.js | 133 +++----- .../acceptance/js/SettingADocumentTests.js | 17 - .../js/helpers/MockTrackChangesApi.js | 45 --- .../document-updater/test/stress/js/run.js | 1 - .../DocumentManager/DocumentManagerTests.js | 18 +- .../js/HistoryManager/HistoryManagerTests.js | 239 ++----------- .../HistoryRedisManagerTests.js | 99 ------ .../unit/js/RedisManager/RedisManagerTests.js | 268 +++++---------- .../js/UpdateManager/UpdateManagerTests.js | 197 +++++------ 18 files changed, 646 insertions(+), 1645 deletions(-) delete mode 100644 services/document-updater/app/js/HistoryRedisManager.js delete mode 100644 services/document-updater/test/acceptance/js/helpers/MockTrackChangesApi.js delete mode 100644 services/document-updater/test/unit/js/HistoryRedisManager/HistoryRedisManagerTests.js diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index c6f12739fb..9975877d05 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -42,15 +42,7 @@ module.exports = DocumentManager = { PersistenceManager.getDoc( projectId, docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - projectHistoryType - ) => { + (error, lines, version, ranges, pathname, projectHistoryId) => { if (error) { return callback(error) } @@ -62,7 +54,6 @@ module.exports = DocumentManager = { version, pathname, projectHistoryId, - projectHistoryType, }, 'got doc from persistence API' ) @@ -78,24 +69,15 @@ module.exports = DocumentManager = { if (error) { return callback(error) } - RedisManager.setHistoryType( - docId, - projectHistoryType, - error => { - if (error) { - return callback(error) - } - callback( - null, - lines, - version, - ranges || {}, - pathname, - projectHistoryId, - null, - false - ) - } + callback( + null, + lines, + version, + ranges || {}, + pathname, + projectHistoryId, + null, + false ) } ) @@ -358,9 +340,6 @@ module.exports = DocumentManager = { } } - // Flush in the background since it requires a http request - HistoryManager.flushDocChangesAsync(projectId, docId) - RedisManager.removeDocFromMemory(projectId, docId, error => { if (error) { return callback(error) diff --git a/services/document-updater/app/js/HistoryManager.js b/services/document-updater/app/js/HistoryManager.js index 7118e92b0a..5597498bfd 100644 --- a/services/document-updater/app/js/HistoryManager.js +++ b/services/document-updater/app/js/HistoryManager.js @@ -1,82 +1,15 @@ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ let HistoryManager const async = require('async') const logger = require('@overleaf/logger') const request = require('request') const Settings = require('@overleaf/settings') -const HistoryRedisManager = require('./HistoryRedisManager') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') -const RedisManager = require('./RedisManager') const metrics = require('./Metrics') module.exports = HistoryManager = { - flushDocChangesAsync(projectId, docId) { - if ( - (Settings.apis != null ? Settings.apis.trackchanges : undefined) == null - ) { - logger.warn( - { docId }, - 'track changes API is not configured, so not flushing' - ) - return - } - if (Settings.disableTrackChanges) { - logger.debug({ docId }, 'track changes is disabled, so not flushing') - return - } - return RedisManager.getHistoryType( - docId, - function (err, projectHistoryType) { - if (err != null) { - logger.warn({ err, docId }, 'error getting history type') - } - // if there's an error continue and flush to track-changes for safety - if ( - Settings.disableDoubleFlush && - projectHistoryType === 'project-history' - ) { - return logger.debug( - { docId, projectHistoryType }, - 'skipping track-changes flush' - ) - } else { - metrics.inc('history-flush', 1, { status: 'track-changes' }) - const url = `${Settings.apis.trackchanges.url}/project/${projectId}/doc/${docId}/flush` - logger.debug( - { projectId, docId, url, projectHistoryType }, - 'flushing doc in track changes api' - ) - return request.post(url, function (error, res, body) { - if (error != null) { - return logger.error( - { error, docId, projectId }, - 'track changes doc to track changes api' - ) - } else if (res.statusCode < 200 && res.statusCode >= 300) { - return logger.error( - { docId, projectId }, - `track changes api returned a failure status code: ${res.statusCode}` - ) - } - }) - } - } - ) - }, - // flush changes in the background flushProjectChangesAsync(projectId) { - if (!Settings.apis?.project_history?.enabled) { - return - } - return HistoryManager.flushProjectChanges( + HistoryManager.flushProjectChanges( projectId, { background: true }, function () {} @@ -88,9 +21,6 @@ module.exports = HistoryManager = { if (callback == null) { callback = function () {} } - if (!Settings.apis?.project_history?.enabled) { - return callback() - } if (options.skip_history_flush) { logger.debug({ projectId }, 'skipping flush of project history') return callback() @@ -102,21 +32,18 @@ module.exports = HistoryManager = { qs.background = true } // pass on the background flush option if present logger.debug({ projectId, url, qs }, 'flushing doc in project history api') - return request.post({ url, qs }, function (error, res, body) { - if (error != null) { - logger.error( - { error, projectId }, - 'project history doc to track changes api' - ) - return callback(error) + request.post({ url, qs }, function (error, res, body) { + if (error) { + logger.error({ error, projectId }, 'project history api request failed') + callback(error) } else if (res.statusCode < 200 && res.statusCode >= 300) { logger.error( { projectId }, `project history api returned a failure status code: ${res.statusCode}` ) - return callback(error) + callback(error) } else { - return callback() + callback() } }) }, @@ -124,80 +51,30 @@ module.exports = HistoryManager = { FLUSH_DOC_EVERY_N_OPS: 100, FLUSH_PROJECT_EVERY_N_OPS: 500, - recordAndFlushHistoryOps( - projectId, - docId, - ops, - docOpsLength, - projectOpsLength, - callback - ) { + recordAndFlushHistoryOps(projectId, ops, projectOpsLength) { if (ops == null) { ops = [] } - if (callback == null) { - callback = function () {} - } if (ops.length === 0) { - return callback() + return } // record updates for project history - if (Settings.apis?.project_history?.enabled) { - if ( - HistoryManager.shouldFlushHistoryOps( - projectOpsLength, - ops.length, - HistoryManager.FLUSH_PROJECT_EVERY_N_OPS - ) - ) { - // Do this in the background since it uses HTTP and so may be too - // slow to wait for when processing a doc update. - logger.debug( - { projectOpsLength, projectId }, - 'flushing project history api' - ) - HistoryManager.flushProjectChangesAsync(projectId) - } - } - - // if the doc_ops_length is undefined it means the project is not using track-changes - // so we can bail out here - if (Settings.disableTrackChanges || typeof docOpsLength === 'undefined') { - logger.debug( - { projectId, docId }, - 'skipping flush to track-changes, only using project-history' + if ( + HistoryManager.shouldFlushHistoryOps( + projectOpsLength, + ops.length, + HistoryManager.FLUSH_PROJECT_EVERY_N_OPS ) - return callback() + ) { + // Do this in the background since it uses HTTP and so may be too + // slow to wait for when processing a doc update. + logger.debug( + { projectOpsLength, projectId }, + 'flushing project history api' + ) + HistoryManager.flushProjectChangesAsync(projectId) } - - // record updates for track-changes - return HistoryRedisManager.recordDocHasHistoryOps( - projectId, - docId, - ops, - function (error) { - if (error != null) { - return callback(error) - } - if ( - HistoryManager.shouldFlushHistoryOps( - docOpsLength, - ops.length, - HistoryManager.FLUSH_DOC_EVERY_N_OPS - ) - ) { - // Do this in the background since it uses HTTP and so may be too - // slow to wait for when processing a doc update. - logger.debug( - { docOpsLength, docId, projectId }, - 'flushing track changes api' - ) - HistoryManager.flushDocChangesAsync(projectId, docId) - } - return callback() - } - ) }, shouldFlushHistoryOps(length, opsLength, threshold) { @@ -217,13 +94,13 @@ module.exports = HistoryManager = { MAX_PARALLEL_REQUESTS: 4, resyncProjectHistory(projectId, projectHistoryId, docs, files, callback) { - return ProjectHistoryRedisManager.queueResyncProjectStructure( + ProjectHistoryRedisManager.queueResyncProjectStructure( projectId, projectHistoryId, docs, files, function (error) { - if (error != null) { + if (error) { return callback(error) } const DocumentManager = require('./DocumentManager') @@ -235,7 +112,7 @@ module.exports = HistoryManager = { cb ) } - return async.eachLimit( + async.eachLimit( docs, HistoryManager.MAX_PARALLEL_REQUESTS, resyncDoc, diff --git a/services/document-updater/app/js/HistoryRedisManager.js b/services/document-updater/app/js/HistoryRedisManager.js deleted file mode 100644 index e658c94a99..0000000000 --- a/services/document-updater/app/js/HistoryRedisManager.js +++ /dev/null @@ -1,41 +0,0 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let HistoryRedisManager -const Settings = require('@overleaf/settings') -const { rclient } = require('./RedisManager') // docsWithHistoryOps lives in main redis -const Keys = Settings.redis.history.key_schema -const logger = require('@overleaf/logger') - -module.exports = HistoryRedisManager = { - recordDocHasHistoryOps(projectId, docId, ops, callback) { - if (ops == null) { - ops = [] - } - if (callback == null) { - callback = function () {} - } - if (ops.length === 0) { - return callback(new Error('cannot push no ops')) // This should never be called with no ops, but protect against a redis error if we sent an empty array to rpush - } - logger.debug({ projectId, docId }, 'marking doc in project for history ops') - return rclient.sadd( - Keys.docsWithHistoryOps({ project_id: projectId }), - docId, - function (error) { - if (error != null) { - return callback(error) - } - return callback() - } - ) - }, -} diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index 74ca5635c2..b5abb2b44f 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -99,8 +99,7 @@ function getDoc(projectId, docId, options = {}, _callback) { body.version, body.ranges, body.pathname, - body.projectHistoryId?.toString(), - body.projectHistoryType + body.projectHistoryId?.toString() ) } else if (res.statusCode === 404) { callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 23279e72e1..db157c7b70 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -26,7 +26,6 @@ const MEGABYTES = 1024 * 1024 const MAX_RANGES_SIZE = 3 * MEGABYTES const keys = Settings.redis.documentupdater.key_schema -const historyKeys = Settings.redis.history.key_schema // note: this is track changes, not project-history module.exports = RedisManager = { rclient, @@ -129,7 +128,6 @@ module.exports = RedisManager = { keys.ranges({ doc_id: docId }), keys.pathname({ doc_id: docId }), keys.projectHistoryId({ doc_id: docId }), - keys.projectHistoryType({ doc_id: docId }), keys.unflushedTime({ doc_id: docId }), keys.lastUpdatedAt({ doc_id: docId }), keys.lastUpdatedBy({ doc_id: docId }) @@ -266,18 +264,14 @@ module.exports = RedisManager = { }, getDocVersion(docId, callback) { - rclient.mget( - keys.docVersion({ doc_id: docId }), - keys.projectHistoryType({ doc_id: docId }), - (error, result) => { - if (error) { - return callback(error) - } - let [version, projectHistoryType] = result || [] - version = parseInt(version, 10) - callback(null, version, projectHistoryType) + rclient.mget(keys.docVersion({ doc_id: docId }), (error, result) => { + if (error) { + return callback(error) } - ) + let [version] = result || [] + version = parseInt(version, 10) + callback(null, version) + }) }, getDocLines(docId, callback) { @@ -353,26 +347,6 @@ module.exports = RedisManager = { }) }, - getHistoryType(docId, callback) { - rclient.get( - keys.projectHistoryType({ doc_id: docId }), - (error, projectHistoryType) => { - if (error) { - return callback(error) - } - callback(null, projectHistoryType) - } - ) - }, - - setHistoryType(docId, projectHistoryType, callback) { - rclient.set( - keys.projectHistoryType({ doc_id: docId }), - projectHistoryType, - callback - ) - }, - DOC_OPS_TTL: 60 * minutes, DOC_OPS_MAX_LENGTH: 100, updateDocument( @@ -388,163 +362,129 @@ module.exports = RedisManager = { if (appliedOps == null) { appliedOps = [] } - RedisManager.getDocVersion( - docId, - (error, currentVersion, projectHistoryType) => { - if (error) { - return callback(error) - } - if (currentVersion + appliedOps.length !== newVersion) { - error = new Error(`Version mismatch. '${docId}' is corrupted.`) - logger.error( - { - err: error, - docId, - currentVersion, - newVersion, - opsLength: appliedOps.length, - }, - 'version mismatch' - ) - return callback(error) - } - - const jsonOps = appliedOps.map(op => JSON.stringify(op)) - for (const op of jsonOps) { - if (op.indexOf('\u0000') !== -1) { - error = new Error('null bytes found in jsonOps') - // this check was added to catch memory corruption in JSON.stringify - logger.error({ err: error, docId, jsonOps }, error.message) - return callback(error) - } - } - - const newDocLines = JSON.stringify(docLines) - if (newDocLines.indexOf('\u0000') !== -1) { - error = new Error('null bytes found in doc lines') - // this check was added to catch memory corruption in JSON.stringify - logger.error({ err: error, docId, newDocLines }, error.message) - return callback(error) - } - // Do an optimised size check on the docLines using the serialised - // length as an upper bound - const sizeBound = newDocLines.length - if (docIsTooLarge(sizeBound, docLines, Settings.max_doc_length)) { - const err = new Error('blocking doc update: doc is too large') - const docSize = newDocLines.length - logger.error({ projectId, docId, err, docSize }, err.message) - return callback(err) - } - const newHash = RedisManager._computeHash(newDocLines) - - const opVersions = appliedOps.map(op => op?.v) - logger.debug( - { - docId, - version: newVersion, - hash: newHash, - opVersions, - }, - 'updating doc in redis' - ) - // record bytes sent to redis in update - metrics.summary('redis.docLines', newDocLines.length, { - status: 'update', - }) - RedisManager._serializeRanges(ranges, (error, ranges) => { - if (error) { - logger.error({ err: error, docId }, error.message) - return callback(error) - } - if (ranges && ranges.indexOf('\u0000') !== -1) { - error = new Error('null bytes found in ranges') - // this check was added to catch memory corruption in JSON.stringify - logger.error({ err: error, docId, ranges }, error.message) - return callback(error) - } - const multi = rclient.multi() - multi.mset({ - [keys.docLines({ doc_id: docId })]: newDocLines, - [keys.docVersion({ doc_id: docId })]: newVersion, - [keys.docHash({ doc_id: docId })]: newHash, - [keys.ranges({ doc_id: docId })]: ranges, - [keys.lastUpdatedAt({ doc_id: docId })]: Date.now(), - [keys.lastUpdatedBy({ doc_id: docId })]: - updateMeta && updateMeta.user_id, - }) - multi.ltrim( - keys.docOps({ doc_id: docId }), - -RedisManager.DOC_OPS_MAX_LENGTH, - -1 - ) // index 3 - // push the ops last so we can get the lengths at fixed index position 7 - if (jsonOps.length > 0) { - multi.rpush(keys.docOps({ doc_id: docId }), ...jsonOps) // index 5 - // expire must come after rpush since before it will be a no-op if the list is empty - multi.expire( - keys.docOps({ doc_id: docId }), - RedisManager.DOC_OPS_TTL - ) // index 6 - if ( - Settings.disableTrackChanges || - projectHistoryType === 'project-history' - ) { - metrics.inc('history-queue', 1, { status: 'skip-track-changes' }) - logger.debug( - { docId }, - 'skipping push of uncompressed ops for project using project-history' - ) - } else { - // project is using old track-changes history service - metrics.inc('history-queue', 1, { status: 'track-changes' }) - multi.rpush( - historyKeys.uncompressedHistoryOps({ doc_id: docId }), - ...jsonOps - ) // index 7 - } - // Set the unflushed timestamp to the current time if the doc - // hasn't been modified before (the content in mongo has been - // valid up to this point). Otherwise leave it alone ("NX" flag). - multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX') - } - multi.exec((error, result) => { - if (error) { - return callback(error) - } - - let docUpdateCount - if ( - Settings.disableTrackChanges || - projectHistoryType === 'project-history' - ) { - docUpdateCount = undefined // only using project history, don't bother with track-changes - } else { - // project is using old track-changes history service - docUpdateCount = result[4] - } - - if (jsonOps.length > 0 && Settings.apis?.project_history?.enabled) { - metrics.inc('history-queue', 1, { status: 'project-history' }) - ProjectHistoryRedisManager.queueOps( - projectId, - ...jsonOps, - (error, projectUpdateCount) => { - if (error) { - // The full project history can re-sync a project in case - // updates went missing. - // Just record the error here and acknowledge the write-op. - metrics.inc('history-queue-error') - } - callback(null, docUpdateCount, projectUpdateCount) - } - ) - } else { - callback(null, docUpdateCount) - } - }) - }) + RedisManager.getDocVersion(docId, (error, currentVersion) => { + if (error) { + return callback(error) } - ) + if (currentVersion + appliedOps.length !== newVersion) { + error = new Error(`Version mismatch. '${docId}' is corrupted.`) + logger.error( + { + err: error, + docId, + currentVersion, + newVersion, + opsLength: appliedOps.length, + }, + 'version mismatch' + ) + return callback(error) + } + + const jsonOps = appliedOps.map(op => JSON.stringify(op)) + for (const op of jsonOps) { + if (op.indexOf('\u0000') !== -1) { + error = new Error('null bytes found in jsonOps') + // this check was added to catch memory corruption in JSON.stringify + logger.error({ err: error, docId, jsonOps }, error.message) + return callback(error) + } + } + + const newDocLines = JSON.stringify(docLines) + if (newDocLines.indexOf('\u0000') !== -1) { + error = new Error('null bytes found in doc lines') + // this check was added to catch memory corruption in JSON.stringify + logger.error({ err: error, docId, newDocLines }, error.message) + return callback(error) + } + // Do an optimised size check on the docLines using the serialised + // length as an upper bound + const sizeBound = newDocLines.length + if (docIsTooLarge(sizeBound, docLines, Settings.max_doc_length)) { + const err = new Error('blocking doc update: doc is too large') + const docSize = newDocLines.length + logger.error({ projectId, docId, err, docSize }, err.message) + return callback(err) + } + const newHash = RedisManager._computeHash(newDocLines) + + const opVersions = appliedOps.map(op => op?.v) + logger.debug( + { + docId, + version: newVersion, + hash: newHash, + opVersions, + }, + 'updating doc in redis' + ) + // record bytes sent to redis in update + metrics.summary('redis.docLines', newDocLines.length, { + status: 'update', + }) + RedisManager._serializeRanges(ranges, (error, ranges) => { + if (error) { + logger.error({ err: error, docId }, error.message) + return callback(error) + } + if (ranges && ranges.indexOf('\u0000') !== -1) { + error = new Error('null bytes found in ranges') + // this check was added to catch memory corruption in JSON.stringify + logger.error({ err: error, docId, ranges }, error.message) + return callback(error) + } + const multi = rclient.multi() + multi.mset({ + [keys.docLines({ doc_id: docId })]: newDocLines, + [keys.docVersion({ doc_id: docId })]: newVersion, + [keys.docHash({ doc_id: docId })]: newHash, + [keys.ranges({ doc_id: docId })]: ranges, + [keys.lastUpdatedAt({ doc_id: docId })]: Date.now(), + [keys.lastUpdatedBy({ doc_id: docId })]: + updateMeta && updateMeta.user_id, + }) + multi.ltrim( + keys.docOps({ doc_id: docId }), + -RedisManager.DOC_OPS_MAX_LENGTH, + -1 + ) // index 3 + // push the ops last so we can get the lengths at fixed index position 7 + if (jsonOps.length > 0) { + multi.rpush(keys.docOps({ doc_id: docId }), ...jsonOps) // index 5 + // expire must come after rpush since before it will be a no-op if the list is empty + multi.expire(keys.docOps({ doc_id: docId }), RedisManager.DOC_OPS_TTL) // index 6 + // Set the unflushed timestamp to the current time if the doc + // hasn't been modified before (the content in mongo has been + // valid up to this point). Otherwise leave it alone ("NX" flag). + multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX') + } + multi.exec((error, result) => { + if (error) { + return callback(error) + } + + if (jsonOps.length > 0) { + metrics.inc('history-queue', 1, { status: 'project-history' }) + ProjectHistoryRedisManager.queueOps( + projectId, + ...jsonOps, + (error, projectUpdateCount) => { + if (error) { + // The full project history can re-sync a project in case + // updates went missing. + // Just record the error here and acknowledge the write-op. + metrics.inc('history-queue-error') + } + callback(null, projectUpdateCount) + } + ) + } else { + callback(null) + } + }) + }) + }) }, renameDoc(projectId, docId, userId, update, projectHistoryId, callback) { diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 6ba3228107..06aa56b957 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -6,10 +6,8 @@ /* * decaffeinate suggestions: * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns * DS201: Simplify complex destructure assignments * DS205: Consider reworking code to avoid use of IIFEs - * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ let UpdateManager @@ -31,44 +29,37 @@ const Profiler = require('./Profiler') module.exports = UpdateManager = { processOutstandingUpdates(projectId, docId, callback) { - if (callback == null) { + if (!callback) { callback = function () {} } const timer = new Metrics.Timer('updateManager.processOutstandingUpdates') - return UpdateManager.fetchAndApplyUpdates( - projectId, - docId, - function (error) { - timer.done() - if (error != null) { - return callback(error) - } - return callback() - } - ) + UpdateManager.fetchAndApplyUpdates(projectId, docId, function (error) { + timer.done() + callback(error) + }) }, processOutstandingUpdatesWithLock(projectId, docId, callback) { - if (callback == null) { + if (!callback) { callback = function () {} } const profile = new Profiler('processOutstandingUpdatesWithLock', { project_id: projectId, doc_id: docId, }) - return LockManager.tryLock(docId, (error, gotLock, lockValue) => { - if (error != null) { + LockManager.tryLock(docId, (error, gotLock, lockValue) => { + if (error) { return callback(error) } if (!gotLock) { return callback() } profile.log('tryLock') - return UpdateManager.processOutstandingUpdates( + UpdateManager.processOutstandingUpdates( projectId, docId, function (error) { - if (error != null) { + if (error) { return UpdateManager._handleErrorInsideLock( docId, lockValue, @@ -77,12 +68,12 @@ module.exports = UpdateManager = { ) } profile.log('processOutstandingUpdates') - return LockManager.releaseLock(docId, lockValue, error => { - if (error != null) { + LockManager.releaseLock(docId, lockValue, error => { + if (error) { return callback(error) } profile.log('releaseLock').end() - return UpdateManager.continueProcessingUpdatesWithLock( + UpdateManager.continueProcessingUpdatesWithLock( projectId, docId, callback @@ -94,59 +85,56 @@ module.exports = UpdateManager = { }, continueProcessingUpdatesWithLock(projectId, docId, callback) { - if (callback == null) { + if (!callback) { callback = function () {} } - return RealTimeRedisManager.getUpdatesLength(docId, (error, length) => { - if (error != null) { + RealTimeRedisManager.getUpdatesLength(docId, (error, length) => { + if (error) { return callback(error) } if (length > 0) { - return UpdateManager.processOutstandingUpdatesWithLock( + UpdateManager.processOutstandingUpdatesWithLock( projectId, docId, callback ) } else { - return callback() + callback() } }) }, fetchAndApplyUpdates(projectId, docId, callback) { - if (callback == null) { + if (!callback) { callback = function () {} } const profile = new Profiler('fetchAndApplyUpdates', { project_id: projectId, doc_id: docId, }) - return RealTimeRedisManager.getPendingUpdatesForDoc( - docId, - (error, updates) => { - if (error != null) { - return callback(error) - } - logger.debug( - { projectId, docId, count: updates.length }, - 'processing updates' - ) - if (updates.length === 0) { - return callback() - } - profile.log('getPendingUpdatesForDoc') - const doUpdate = (update, cb) => - UpdateManager.applyUpdate(projectId, docId, update, function (err) { - profile.log('applyUpdate') - return cb(err) - }) - const finalCallback = function (err) { - profile.log('async done').end() - return callback(err) - } - return async.eachSeries(updates, doUpdate, finalCallback) + RealTimeRedisManager.getPendingUpdatesForDoc(docId, (error, updates) => { + if (error) { + return callback(error) } - ) + logger.debug( + { projectId, docId, count: updates.length }, + 'processing updates' + ) + if (updates.length === 0) { + return callback() + } + profile.log('getPendingUpdatesForDoc') + const doUpdate = (update, cb) => + UpdateManager.applyUpdate(projectId, docId, update, function (err) { + profile.log('applyUpdate') + cb(err) + }) + const finalCallback = function (err) { + profile.log('async done').end() + callback(err) + } + async.eachSeries(updates, doUpdate, finalCallback) + }) }, applyUpdate(projectId, docId, update, _callback) { @@ -154,7 +142,7 @@ module.exports = UpdateManager = { _callback = function () {} } const callback = function (error) { - if (error != null) { + if (error) { RealTimeRedisManager.sendData({ project_id: projectId, doc_id: docId, @@ -163,7 +151,7 @@ module.exports = UpdateManager = { profile.log('sendData') } profile.end() - return _callback(error) + _callback(error) } const profile = new Profiler('applyUpdate', { @@ -172,12 +160,12 @@ module.exports = UpdateManager = { }) UpdateManager._sanitizeUpdate(update) profile.log('sanitizeUpdate', { sync: true }) - return DocumentManager.getDoc( + DocumentManager.getDoc( projectId, docId, function (error, lines, version, ranges, pathname, projectHistoryId) { profile.log('getDoc') - if (error != null) { + if (error) { return callback(error) } if (lines == null || version == null) { @@ -187,7 +175,7 @@ module.exports = UpdateManager = { } const previousVersion = version const incomingUpdateVersion = update.v - return ShareJsUpdateManager.applyUpdate( + ShareJsUpdateManager.applyUpdate( projectId, docId, update, @@ -199,10 +187,10 @@ module.exports = UpdateManager = { // doc version, otherwise getPreviousDocOps is called. sync: incomingUpdateVersion === previousVersion, }) - if (error != null) { + if (error) { return callback(error) } - return RangesManager.applyUpdate( + RangesManager.applyUpdate( projectId, docId, ranges, @@ -216,10 +204,10 @@ module.exports = UpdateManager = { lines ) profile.log('RangesManager.applyUpdate', { sync: true }) - if (error != null) { + if (error) { return callback(error) } - return RedisManager.updateDocument( + RedisManager.updateDocument( projectId, docId, updatedDocLines, @@ -227,68 +215,61 @@ module.exports = UpdateManager = { appliedOps, newRanges, update.meta, - function (error, docOpsLength, projectOpsLength) { + function (error, projectOpsLength) { profile.log('RedisManager.updateDocument') - if (error != null) { + if (error) { return callback(error) } - return HistoryManager.recordAndFlushHistoryOps( + HistoryManager.recordAndFlushHistoryOps( projectId, - docId, appliedOps, - docOpsLength, - projectOpsLength, - function (error) { - profile.log('recordAndFlushHistoryOps') - if (error != null) { - return callback(error) - } - if (rangesWereCollapsed) { - Metrics.inc('doc-snapshot') - logger.debug( - { - projectId, - docId, - previousVersion, - lines, - ranges, - update, - }, - 'update collapsed some ranges, snapshotting previous content' - ) - // Do this last, since it's a mongo call, and so potentially longest running - // If it overruns the lock, it's ok, since all of our redis work is done - return SnapshotManager.recordSnapshot( - projectId, - docId, - previousVersion, - pathname, - lines, - ranges, - function (error) { - if (error != null) { - logger.error( - { - err: error, - projectId, - docId, - version, - lines, - ranges, - }, - 'error recording snapshot' - ) - return callback(error) - } else { - return callback() - } - } - ) - } else { - return callback() - } - } + projectOpsLength ) + profile.log('recordAndFlushHistoryOps') + if (rangesWereCollapsed) { + Metrics.inc('doc-snapshot') + logger.debug( + { + projectId, + docId, + previousVersion, + lines, + ranges, + update, + }, + 'update collapsed some ranges, snapshotting previous content' + ) + // Do this last, since it's a mongo call, and so potentially longest running + // If it overruns the lock, it's ok, since all of our redis work is done + SnapshotManager.recordSnapshot( + projectId, + docId, + previousVersion, + pathname, + lines, + ranges, + function (error) { + if (error) { + logger.error( + { + err: error, + projectId, + docId, + version, + lines, + ranges, + }, + 'error recording snapshot' + ) + callback(error) + } else { + callback() + } + } + ) + } else { + callback() + } } ) } @@ -309,14 +290,14 @@ module.exports = UpdateManager = { }) return LockManager.getLock(docId, function (error, lockValue) { profile.log('getLock') - if (error != null) { + if (error) { return callback(error) } - return UpdateManager.processOutstandingUpdates( + UpdateManager.processOutstandingUpdates( projectId, docId, function (error) { - if (error != null) { + if (error) { return UpdateManager._handleErrorInsideLock( docId, lockValue, @@ -325,12 +306,12 @@ module.exports = UpdateManager = { ) } profile.log('processOutstandingUpdates') - return method( + method( projectId, docId, ...Array.from(args), function (error, ...responseArgs) { - if (error != null) { + if (error) { return UpdateManager._handleErrorInsideLock( docId, lockValue, @@ -339,34 +320,30 @@ module.exports = UpdateManager = { ) } profile.log('method') - return LockManager.releaseLock( - docId, - lockValue, - function (error) { - if (error != null) { - return callback(error) - } - profile.log('releaseLock').end() - callback(null, ...Array.from(responseArgs)) - // We held the lock for a while so updates might have queued up - return UpdateManager.continueProcessingUpdatesWithLock( - projectId, - docId, - err => { - if (err) { - // The processing may fail for invalid user updates. - // This can be very noisy, put them on level DEBUG - // and record a metric. - Metrics.inc('background-processing-updates-error') - logger.debug( - { err, projectId, docId }, - 'error processing updates in background' - ) - } - } - ) + LockManager.releaseLock(docId, lockValue, function (error) { + if (error) { + return callback(error) } - ) + profile.log('releaseLock').end() + callback(null, ...Array.from(responseArgs)) + // We held the lock for a while so updates might have queued up + UpdateManager.continueProcessingUpdatesWithLock( + projectId, + docId, + err => { + if (err) { + // The processing may fail for invalid user updates. + // This can be very noisy, put them on level DEBUG + // and record a metric. + Metrics.inc('background-processing-updates-error') + logger.debug( + { err, projectId, docId }, + 'error processing updates in background' + ) + } + } + ) + }) } ) } @@ -375,10 +352,10 @@ module.exports = UpdateManager = { }, _handleErrorInsideLock(docId, lockValue, originalError, callback) { - if (callback == null) { + if (!callback) { callback = function () {} } - return LockManager.releaseLock(docId, lockValue, lockError => + LockManager.releaseLock(docId, lockValue, lockError => callback(originalError) ) }, diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index 6b8a2ae89b..0bcd90ab28 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -14,11 +14,7 @@ module.exports = { user: process.env.WEB_API_USER || 'sharelatex', pass: process.env.WEB_API_PASSWORD || 'password', }, - trackchanges: { - url: `http://${process.env.TRACK_CHANGES_HOST || 'localhost'}:3015`, - }, project_history: { - enabled: true, url: `http://${process.env.PROJECT_HISTORY_HOST || 'localhost'}:3054`, }, }, @@ -44,14 +40,6 @@ module.exports = { maxRetriesPerRequest: parseInt( process.env.REDIS_MAX_RETRIES_PER_REQUEST || '20' ), - key_schema: { - uncompressedHistoryOps({ doc_id: docId }) { - return `UncompressedHistoryOps:{${docId}}` - }, - docsWithHistoryOps({ project_id: projectId }) { - return `DocsWithHistoryOps:{${projectId}}` - }, - }, }, project_history: { @@ -137,9 +125,6 @@ module.exports = { projectHistoryId({ doc_id: docId }) { return `ProjectHistoryId:{${docId}}` }, - projectHistoryType({ doc_id: docId }) { - return `ProjectHistoryType:{${docId}}` - }, projectState({ project_id: projectId }) { return `ProjectState:{${projectId}}` }, @@ -181,8 +166,4 @@ module.exports = { continuousBackgroundFlush: process.env.CONTINUOUS_BACKGROUND_FLUSH === 'true', smoothingOffset: process.env.SMOOTHING_OFFSET || 1000, // milliseconds - - disableDoubleFlush: process.env.DISABLE_DOUBLE_FLUSH === 'true', // don't flush track-changes for projects using project-history - - disableTrackChanges: process.env.DISABLE_TRACK_CHANGES === 'true', // stop sending any updates to track-changes } diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js index 1c2040f914..ccceb67444 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js @@ -3,7 +3,6 @@ /* * decaffeinate suggestions: * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -11,9 +10,6 @@ const sinon = require('sinon') const { expect } = require('chai') const async = require('async') const Settings = require('@overleaf/settings') -const rclientHistory = require('@overleaf/redis-wrapper').createClient( - Settings.redis.history -) // note: this is track changes, not project-history const rclientProjectHistory = require('@overleaf/redis-wrapper').createClient( Settings.redis.project_history ) @@ -21,10 +17,8 @@ const rclientDU = require('@overleaf/redis-wrapper').createClient( Settings.redis.documentupdater ) const Keys = Settings.redis.documentupdater.key_schema -const HistoryKeys = Settings.redis.history.key_schema const ProjectHistoryKeys = Settings.redis.project_history.key_schema -const MockTrackChangesApi = require('./helpers/MockTrackChangesApi') const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') @@ -44,7 +38,7 @@ describe('Applying updates to a doc', function () { v: this.version, } this.result = ['one', 'one and a half', 'two', 'three'] - return DocUpdaterApp.ensureRunning(done) + DocUpdaterApp.ensureRunning(done) }) describe('when the document is not loaded', function () { @@ -67,18 +61,17 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) after(function () { - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should load the document from the web API', function () { - return MockWebApi.getDocument + MockWebApi.getDocument .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -88,38 +81,11 @@ describe('Applying updates to a doc', function () { this.project_id, this.doc_id, (error, res, doc) => { - if (error) return done(error) + if (error) done(error) doc.lines.should.deep.equal(this.result) - return done() + done() } ) - return null - }) - - it('should push the applied updates to the track changes api', function (done) { - rclientHistory.lrange( - HistoryKeys.uncompressedHistoryOps({ doc_id: this.doc_id }), - 0, - -1, - (error, updates) => { - if (error != null) { - throw error - } - JSON.parse(updates[0]).op.should.deep.equal(this.update.op) - return rclientHistory.sismember( - HistoryKeys.docsWithHistoryOps({ project_id: this.project_id }), - this.doc_id, - (error, result) => { - if (error != null) { - throw error - } - result.should.equal(1) - return done() - } - ) - } - ) - return null }) it('should push the applied updates to the project history changes api', function (done) { @@ -132,10 +98,9 @@ describe('Applying updates to a doc', function () { throw error } JSON.parse(updates[0]).op.should.deep.equal(this.update.op) - return done() + done() } ) - return null }) it('should set the first op timestamp', function (done) { @@ -150,13 +115,12 @@ describe('Applying updates to a doc', function () { result = parseInt(result, 10) result.should.be.within(this.startTime, Date.now()) this.firstOpTimestamp = result - return done() + done() } ) - return null }) - return describe('when sending another update', function () { + describe('when sending another update', function () { before(function (done) { this.timeout = 10000 this.second_update = Object.create(this.update) @@ -169,13 +133,12 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) - return it('should not change the first op timestamp', function (done) { + it('should not change the first op timestamp', function (done) { rclientProjectHistory.get( ProjectHistoryKeys.projectHistoryFirstOpTimestamp({ project_id: this.project_id, @@ -186,10 +149,9 @@ describe('Applying updates to a doc', function () { } result = parseInt(result, 10) result.should.equal(this.firstOpTimestamp) - return done() + done() } ) - return null }) }) }) @@ -210,7 +172,7 @@ describe('Applying updates to a doc', function () { throw error } sinon.spy(MockWebApi, 'getDocument') - return DocUpdaterClient.sendUpdate( + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, this.update, @@ -218,19 +180,18 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return null }) after(function () { - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should not need to call the web api', function () { - return MockWebApi.getDocument.called.should.equal(false) + MockWebApi.getDocument.called.should.equal(false) }) it('should update the doc', function (done) { @@ -240,35 +201,12 @@ describe('Applying updates to a doc', function () { (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.result) - return done() + done() } ) - return null }) - it('should push the applied updates to the track changes api', function (done) { - rclientHistory.lrange( - HistoryKeys.uncompressedHistoryOps({ doc_id: this.doc_id }), - 0, - -1, - (error, updates) => { - if (error) return done(error) - JSON.parse(updates[0]).op.should.deep.equal(this.update.op) - return rclientHistory.sismember( - HistoryKeys.docsWithHistoryOps({ project_id: this.project_id }), - this.doc_id, - (error, result) => { - if (error) return done(error) - result.should.equal(1) - return done() - } - ) - } - ) - return null - }) - - return it('should push the applied updates to the project history changes api', function (done) { + it('should push the applied updates to the project history changes api', function (done) { rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -276,10 +214,9 @@ describe('Applying updates to a doc', function () { (error, updates) => { if (error) return done(error) JSON.parse(updates[0]).op.should.deep.equal(this.update.op) - return done() + done() } ) - return null }) }) @@ -293,14 +230,13 @@ describe('Applying updates to a doc', function () { MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version, - projectHistoryType: 'project-history', }) DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, error => { if (error != null) { throw error } sinon.spy(MockWebApi, 'getDocument') - return DocUpdaterClient.sendUpdate( + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, this.update, @@ -308,15 +244,14 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return null }) after(function () { - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should update the doc', function (done) { @@ -326,27 +261,12 @@ describe('Applying updates to a doc', function () { (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.result) - return done() + done() } ) - return null }) - it('should not push any applied updates to the track changes api', function (done) { - rclientHistory.lrange( - HistoryKeys.uncompressedHistoryOps({ doc_id: this.doc_id }), - 0, - -1, - (error, updates) => { - if (error) return done(error) - updates.length.should.equal(0) - return done() - } - ) - return null - }) - - return it('should push the applied updates to the project history changes api', function (done) { + it('should push the applied updates to the project history changes api', function (done) { rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -354,10 +274,9 @@ describe('Applying updates to a doc', function () { (error, updates) => { if (error) return done(error) JSON.parse(updates[0]).op.should.deep.equal(this.update.op) - return done() + done() } ) - return null }) }) @@ -387,7 +306,7 @@ describe('Applying updates to a doc', function () { { doc_id: this.doc_id, v: 10, op: [{ i: 'd', p: 10 }] }, ] this.my_result = ['hello world', '', ''] - return done() + done() }) it('should be able to continue applying updates when the project has been deleted', function (done) { @@ -395,7 +314,7 @@ describe('Applying updates to a doc', function () { const actions = [] for (update of Array.from(this.updates.slice(0, 6))) { ;(update => { - return actions.push(callback => + actions.push(callback => DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, @@ -410,7 +329,7 @@ describe('Applying updates to a doc', function () { ) for (update of Array.from(this.updates.slice(6))) { ;(update => { - return actions.push(callback => + actions.push(callback => DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, @@ -425,47 +344,19 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.my_result) - return done() + done() } ) }) - return null }) - it('should push the applied updates to the track changes api', function (done) { - rclientHistory.lrange( - HistoryKeys.uncompressedHistoryOps({ doc_id: this.doc_id }), - 0, - -1, - (error, updates) => { - if (error) return done(error) - updates = Array.from(updates).map(u => JSON.parse(u)) - for (let i = 0; i < this.updates.length; i++) { - const appliedUpdate = this.updates[i] - appliedUpdate.op.should.deep.equal(updates[i].op) - } - - return rclientHistory.sismember( - HistoryKeys.docsWithHistoryOps({ project_id: this.project_id }), - this.doc_id, - (error, result) => { - if (error) return done(error) - result.should.equal(1) - return done() - } - ) - } - ) - return null - }) - - return it('should store the doc ops in the correct order', function (done) { + it('should store the doc ops in the correct order', function (done) { rclientDU.lrange( Keys.docOps({ doc_id: this.doc_id }), 0, @@ -477,14 +368,13 @@ describe('Applying updates to a doc', function () { const appliedUpdate = this.updates[i] appliedUpdate.op.should.deep.equal(updates[i].op) } - return done() + done() } ) - return null }) }) - return describe('when older ops come in after the delete', function () { + describe('when older ops come in after the delete', function () { before(function (done) { ;[this.project_id, this.doc_id] = Array.from([ DocUpdaterClient.randomId(), @@ -504,15 +394,15 @@ describe('Applying updates to a doc', function () { { doc_id: this.doc_id, v: 0, op: [{ i: 'world', p: 1 }] }, ] this.my_result = ['hello', 'world', ''] - return done() + done() }) - return it('should be able to continue applying updates when the project has been deleted', function (done) { + it('should be able to continue applying updates when the project has been deleted', function (done) { let update const actions = [] for (update of Array.from(this.updates.slice(0, 5))) { ;(update => { - return actions.push(callback => + actions.push(callback => DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, @@ -527,7 +417,7 @@ describe('Applying updates to a doc', function () { ) for (update of Array.from(this.updates.slice(5))) { ;(update => { - return actions.push(callback => + actions.push(callback => DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, @@ -542,17 +432,16 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.my_result) - return done() + done() } ) }) - return null }) }) }) @@ -585,10 +474,9 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) it('should not update the doc', function (done) { @@ -598,17 +486,16 @@ describe('Applying updates to a doc', function () { (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.lines) - return done() + done() } ) - return null }) - return it('should send a message with an error', function () { + it('should send a message with an error', function () { this.messageCallback.called.should.equal(true) const [channel, message] = Array.from(this.messageCallback.args[0]) channel.should.equal('applied-ops') - return JSON.parse(message).should.deep.include({ + JSON.parse(message).should.deep.include({ project_id: this.project_id, doc_id: this.doc_id, error: 'Delete component does not match', @@ -616,61 +503,6 @@ describe('Applying updates to a doc', function () { }) }) - describe('with enough updates to flush to the track changes api', function () { - before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) - const updates = [] - for (let v = 0; v <= 199; v++) { - // Should flush after 100 ops - updates.push({ - doc_id: this.doc_id, - op: [{ i: v.toString(), p: 0 }], - v, - }) - } - - sinon.spy(MockTrackChangesApi, 'flushDoc') - - MockWebApi.insertDoc(this.project_id, this.doc_id, { - lines: this.lines, - version: 0, - }) - - // Send updates in chunks to causes multiple flushes - const actions = [] - for (let i = 0; i <= 19; i++) { - ;(i => { - return actions.push(cb => { - return DocUpdaterClient.sendUpdates( - this.project_id, - this.doc_id, - updates.slice(i * 10, (i + 1) * 10), - cb - ) - }) - })(i) - } - async.series(actions, error => { - if (error != null) { - throw error - } - return setTimeout(done, 2000) - }) - return null - }) - - after(function () { - return MockTrackChangesApi.flushDoc.restore() - }) - - return it('should flush the doc twice', function () { - return MockTrackChangesApi.flushDoc.calledTwice.should.equal(true) - }) - }) - describe('when there is no version in Mongo', function () { before(function (done) { ;[this.project_id, this.doc_id] = Array.from([ @@ -694,23 +526,21 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) - return it('should update the doc (using version = 0)', function (done) { + it('should update the doc (using version = 0)', function (done) { DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.result) - return done() + done() } ) - return null }) }) @@ -750,8 +580,8 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(() => { - return DocUpdaterClient.sendUpdate( + setTimeout(() => { + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, { @@ -772,13 +602,12 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }, 200) } ) - return null }) it('should update the doc', function (done) { @@ -788,24 +617,21 @@ describe('Applying updates to a doc', function () { (error, res, doc) => { if (error) return done(error) doc.lines.should.deep.equal(this.result) - return done() + done() } ) - return null }) - return it('should return a message about duplicate ops', function () { + it('should return a message about duplicate ops', function () { this.messageCallback.calledTwice.should.equal(true) this.messageCallback.args[0][0].should.equal('applied-ops') expect(JSON.parse(this.messageCallback.args[0][1]).op.dup).to.be.undefined this.messageCallback.args[1][0].should.equal('applied-ops') - return expect( - JSON.parse(this.messageCallback.args[1][1]).op.dup - ).to.equal(true) + expect(JSON.parse(this.messageCallback.args[1][1]).op.dup).to.equal(true) }) }) - return describe('when sending updates for a non-existing doc id', function () { + describe('when sending updates for a non-existing doc id', function () { before(function (done) { ;[this.project_id, this.doc_id] = Array.from([ DocUpdaterClient.randomId(), @@ -829,10 +655,9 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) it('should not update or create a doc', function (done) { @@ -842,17 +667,16 @@ describe('Applying updates to a doc', function () { (error, res, doc) => { if (error) return done(error) res.statusCode.should.equal(404) - return done() + done() } ) - return null }) - return it('should send a message with an error', function () { + it('should send a message with an error', function () { this.messageCallback.called.should.equal(true) const [channel, message] = Array.from(this.messageCallback.args[0]) channel.should.equal('applied-ops') - return JSON.parse(message).should.deep.include({ + JSON.parse(message).should.deep.include({ project_id: this.project_id, doc_id: this.doc_id, error: `doc not not found: /project/${this.project_id}/doc/${this.doc_id}`, diff --git a/services/document-updater/test/acceptance/js/DeletingADocumentTests.js b/services/document-updater/test/acceptance/js/DeletingADocumentTests.js index 9142e923e6..24aef32bf2 100644 --- a/services/document-updater/test/acceptance/js/DeletingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/DeletingADocumentTests.js @@ -3,12 +3,10 @@ /* * decaffeinate suggestions: * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const sinon = require('sinon') -const MockTrackChangesApi = require('./helpers/MockTrackChangesApi') const MockProjectHistoryApi = require('./helpers/MockProjectHistoryApi') const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') @@ -30,14 +28,12 @@ describe('Deleting a document', function () { } this.result = ['one', 'one and a half', 'two', 'three'] - sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') - return DocUpdaterApp.ensureRunning(done) + DocUpdaterApp.ensureRunning(done) }) after(function () { - MockTrackChangesApi.flushDoc.restore() - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) describe('when the updated doc exists in the doc updater', function () { @@ -53,49 +49,45 @@ describe('Deleting a document', function () { lines: this.lines, version: this.version, }) - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc_id, - error => { - if (error != null) { - throw error - } - return DocUpdaterClient.sendUpdate( - this.project_id, - this.doc_id, - this.update, - error => { - if (error != null) { - throw error - } - return setTimeout(() => { - return DocUpdaterClient.deleteDoc( - this.project_id, - this.doc_id, - (error, res, body) => { - if (error) return done(error) - this.statusCode = res.statusCode - return setTimeout(done, 200) - } - ) - }, 200) - } - ) + DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, error => { + if (error != null) { + throw error } - ) + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + this.update, + error => { + if (error != null) { + throw error + } + setTimeout(() => { + DocUpdaterClient.deleteDoc( + this.project_id, + this.doc_id, + (error, res, body) => { + if (error) return done(error) + this.statusCode = res.statusCode + setTimeout(done, 200) + } + ) + }, 200) + } + ) + }) }) after(function () { MockWebApi.setDocument.restore() - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should send the updated document and version to the web api', function () { - return MockWebApi.setDocument + MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.result, this.version + 1) .should.equal(true) }) @@ -103,7 +95,7 @@ describe('Deleting a document', function () { it('should need to reload the doc if read again', function (done) { MockWebApi.getDocument.resetHistory() MockWebApi.getDocument.called.should.equals(false) - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, doc) => { @@ -111,25 +103,19 @@ describe('Deleting a document', function () { MockWebApi.getDocument .calledWith(this.project_id, this.doc_id) .should.equal(true) - return done() + done() } ) }) - it('should flush track changes', function () { - return MockTrackChangesApi.flushDoc - .calledWith(this.doc_id) - .should.equal(true) - }) - - return it('should flush project history', function () { - return MockProjectHistoryApi.flushProject + it('should flush project history', function () { + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(true) }) }) - return describe('when the doc is not in the doc updater', function () { + describe('when the doc is not in the doc updater', function () { before(function (done) { ;[this.project_id, this.doc_id] = Array.from([ DocUpdaterClient.randomId(), @@ -140,33 +126,33 @@ describe('Deleting a document', function () { }) sinon.spy(MockWebApi, 'setDocument') sinon.spy(MockWebApi, 'getDocument') - return DocUpdaterClient.deleteDoc( + DocUpdaterClient.deleteDoc( this.project_id, this.doc_id, (error, res, body) => { if (error) return done(error) this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) after(function () { MockWebApi.setDocument.restore() - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should not need to send the updated document to the web api', function () { - return MockWebApi.setDocument.called.should.equal(false) + MockWebApi.setDocument.called.should.equal(false) }) it('should need to reload the doc if read again', function (done) { MockWebApi.getDocument.called.should.equals(false) - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, doc) => { @@ -174,19 +160,13 @@ describe('Deleting a document', function () { MockWebApi.getDocument .calledWith(this.project_id, this.doc_id) .should.equal(true) - return done() + done() } ) }) - it('should flush track changes', function () { - return MockTrackChangesApi.flushDoc - .calledWith(this.doc_id) - .should.equal(true) - }) - - return it('should flush project history', function () { - return MockProjectHistoryApi.flushProject + it('should flush project history', function () { + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(true) }) diff --git a/services/document-updater/test/acceptance/js/DeletingAProjectTests.js b/services/document-updater/test/acceptance/js/DeletingAProjectTests.js index 565c224f94..f7ad454c69 100644 --- a/services/document-updater/test/acceptance/js/DeletingAProjectTests.js +++ b/services/document-updater/test/acceptance/js/DeletingAProjectTests.js @@ -3,14 +3,12 @@ /* * decaffeinate suggestions: * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const sinon = require('sinon') const async = require('async') -const MockTrackChangesApi = require('./helpers/MockTrackChangesApi') const MockProjectHistoryApi = require('./helpers/MockProjectHistoryApi') const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') @@ -59,48 +57,43 @@ describe('Deleting a project', function () { }) } - return DocUpdaterApp.ensureRunning(done) + DocUpdaterApp.ensureRunning(done) }) describe('with documents which have been updated', function () { before(function (done) { sinon.spy(MockWebApi, 'setDocument') - sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') - return async.series( + async.series( this.docs.map(doc => { return callback => { - return DocUpdaterClient.preloadDoc( - this.project_id, - doc.id, - error => { - if (error != null) { - return callback(error) - } - return DocUpdaterClient.sendUpdate( - this.project_id, - doc.id, - doc.update, - error => { - return callback(error) - } - ) + DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => { + if (error != null) { + return callback(error) } - ) + DocUpdaterClient.sendUpdate( + this.project_id, + doc.id, + doc.update, + error => { + callback(error) + } + ) + }) } }), error => { if (error != null) { throw error } - return setTimeout(() => { - return DocUpdaterClient.deleteProject( + setTimeout(() => { + DocUpdaterClient.deleteProject( this.project_id, (error, res, body) => { if (error) return done(error) this.statusCode = res.statusCode - return done() + done() } ) }, 200) @@ -110,16 +103,15 @@ describe('Deleting a project', function () { after(function () { MockWebApi.setDocument.restore() - MockTrackChangesApi.flushDoc.restore() - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should send each document to the web api', function () { - return Array.from(this.docs).map(doc => + Array.from(this.docs).map(doc => MockWebApi.setDocument .calledWith(this.project_id, doc.id, doc.updatedLines) .should.equal(true) @@ -128,13 +120,13 @@ describe('Deleting a project', function () { it('should need to reload the docs if read again', function (done) { sinon.spy(MockWebApi, 'getDocument') - return async.series( + async.series( this.docs.map(doc => { return callback => { MockWebApi.getDocument .calledWith(this.project_id, doc.id) .should.equal(false) - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, doc.id, (error, res, returnedDoc) => { @@ -142,26 +134,20 @@ describe('Deleting a project', function () { MockWebApi.getDocument .calledWith(this.project_id, doc.id) .should.equal(true) - return callback() + callback() } ) } }), () => { MockWebApi.getDocument.restore() - return done() + done() } ) }) - it('should flush each doc in track changes', function () { - return Array.from(this.docs).map(doc => - MockTrackChangesApi.flushDoc.calledWith(doc.id).should.equal(true) - ) - }) - - return it('should flush each doc in project history', function () { - return MockProjectHistoryApi.flushProject + it('should flush each doc in project history', function () { + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(true) }) @@ -170,30 +156,25 @@ describe('Deleting a project', function () { describe('with the background=true parameter from realtime and no request to flush the queue', function () { before(function (done) { sinon.spy(MockWebApi, 'setDocument') - sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') - return async.series( + async.series( this.docs.map(doc => { return callback => { - return DocUpdaterClient.preloadDoc( - this.project_id, - doc.id, - callback - ) + DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback) } }), error => { if (error != null) { throw error } - return setTimeout(() => { - return DocUpdaterClient.deleteProjectOnShutdown( + setTimeout(() => { + DocUpdaterClient.deleteProjectOnShutdown( this.project_id, (error, res, body) => { if (error) return done(error) this.statusCode = res.statusCode - return done() + done() } ) }, 200) @@ -203,58 +184,45 @@ describe('Deleting a project', function () { after(function () { MockWebApi.setDocument.restore() - MockTrackChangesApi.flushDoc.restore() - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should not send any documents to the web api', function () { - return MockWebApi.setDocument.called.should.equal(false) + MockWebApi.setDocument.called.should.equal(false) }) - it('should not flush any docs in track changes', function () { - return MockTrackChangesApi.flushDoc.called.should.equal(false) - }) - - return it('should not flush to project history', function () { - return MockProjectHistoryApi.flushProject.called.should.equal(false) + it('should not flush to project history', function () { + MockProjectHistoryApi.flushProject.called.should.equal(false) }) }) - return describe('with the background=true parameter from realtime and a request to flush the queue', function () { + describe('with the background=true parameter from realtime and a request to flush the queue', function () { before(function (done) { sinon.spy(MockWebApi, 'setDocument') - sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') - return async.series( + async.series( this.docs.map(doc => { return callback => { - return DocUpdaterClient.preloadDoc( - this.project_id, - doc.id, - callback - ) + DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback) } }), error => { if (error != null) { throw error } - return setTimeout(() => { - return DocUpdaterClient.deleteProjectOnShutdown( + setTimeout(() => { + DocUpdaterClient.deleteProjectOnShutdown( this.project_id, (error, res, body) => { if (error) return done(error) this.statusCode = res.statusCode // after deleting the project and putting it in the queue, flush the queue - return setTimeout( - () => DocUpdaterClient.flushOldProjects(done), - 2000 - ) + setTimeout(() => DocUpdaterClient.flushOldProjects(done), 2000) } ) }, 200) @@ -264,30 +232,23 @@ describe('Deleting a project', function () { after(function () { MockWebApi.setDocument.restore() - MockTrackChangesApi.flushDoc.restore() - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should send each document to the web api', function () { - return Array.from(this.docs).map(doc => + Array.from(this.docs).map(doc => MockWebApi.setDocument .calledWith(this.project_id, doc.id, doc.updatedLines) .should.equal(true) ) }) - it('should flush each doc in track changes', function () { - return Array.from(this.docs).map(doc => - MockTrackChangesApi.flushDoc.calledWith(doc.id).should.equal(true) - ) - }) - - return it('should flush to project history', function () { - return MockProjectHistoryApi.flushProject.called.should.equal(true) + it('should flush to project history', function () { + MockProjectHistoryApi.flushProject.called.should.equal(true) }) }) }) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index e903f1b81e..5b0c4ab281 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -6,7 +6,6 @@ const docUpdaterRedis = require('@overleaf/redis-wrapper').createClient( ) const Keys = Settings.redis.documentupdater.key_schema -const MockTrackChangesApi = require('./helpers/MockTrackChangesApi') const MockProjectHistoryApi = require('./helpers/MockProjectHistoryApi') const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') @@ -35,14 +34,12 @@ describe('Setting a document', function () { this.source = 'dropbox' this.user_id = 'user-id-123' - sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') sinon.spy(MockWebApi, 'setDocument') DocUpdaterApp.ensureRunning(done) }) after(function () { - MockTrackChangesApi.flushDoc.restore() MockProjectHistoryApi.flushProject.restore() MockWebApi.setDocument.restore() }) @@ -92,7 +89,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) @@ -228,7 +224,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) @@ -247,10 +242,6 @@ describe('Setting a document', function () { .should.equal(true) }) - it('should flush track changes', function () { - MockTrackChangesApi.flushDoc.calledWith(this.doc_id).should.equal(true) - }) - it('should flush project history', function () { MockProjectHistoryApi.flushProject .calledWith(this.project_id) @@ -319,7 +310,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) @@ -332,10 +322,6 @@ describe('Setting a document', function () { MockWebApi.setDocument.called.should.equal(false) }) - it('should not flush track changes', function () { - MockTrackChangesApi.flushDoc.called.should.equal(false) - }) - it('should not flush project history', function () { MockProjectHistoryApi.flushProject.called.should.equal(false) }) @@ -376,7 +362,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) @@ -458,7 +443,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) @@ -521,7 +505,6 @@ describe('Setting a document', function () { }) after(function () { - MockTrackChangesApi.flushDoc.resetHistory() MockProjectHistoryApi.flushProject.resetHistory() MockWebApi.setDocument.resetHistory() }) diff --git a/services/document-updater/test/acceptance/js/helpers/MockTrackChangesApi.js b/services/document-updater/test/acceptance/js/helpers/MockTrackChangesApi.js deleted file mode 100644 index 8d19af0415..0000000000 --- a/services/document-updater/test/acceptance/js/helpers/MockTrackChangesApi.js +++ /dev/null @@ -1,45 +0,0 @@ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let MockTrackChangesApi -const express = require('express') -const app = express() - -module.exports = MockTrackChangesApi = { - flushDoc(docId, callback) { - if (callback == null) { - callback = function () {} - } - return callback() - }, - - run() { - app.post('/project/:project_id/doc/:doc_id/flush', (req, res, next) => { - return this.flushDoc(req.params.doc_id, error => { - if (error != null) { - return res.sendStatus(500) - } else { - return res.sendStatus(204) - } - }) - }) - - return app - .listen(3015, error => { - if (error != null) { - throw error - } - }) - .on('error', error => { - console.error('error starting MockTrackChangesApi:', error.message) - return process.exit(1) - }) - }, -} - -MockTrackChangesApi.run() diff --git a/services/document-updater/test/stress/js/run.js b/services/document-updater/test/stress/js/run.js index e3850da46e..cbf9369ef9 100644 --- a/services/document-updater/test/stress/js/run.js +++ b/services/document-updater/test/stress/js/run.js @@ -14,7 +14,6 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const DocUpdaterClient = require('../../acceptance/js/helpers/DocUpdaterClient') -// MockTrackChangesApi = require "../../acceptance/js/helpers/MockTrackChangesApi" // MockWebApi = require "../../acceptance/js/helpers/MockWebApi" const assert = require('assert') const async = require('async') diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 3abb8b9638..f7aa85499b 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -19,7 +19,6 @@ describe('DocumentManager', function () { './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), './PersistenceManager': (this.PersistenceManager = {}), './HistoryManager': (this.HistoryManager = { - flushDocChangesAsync: sinon.stub(), flushProjectChangesAsync: sinon.stub(), }), './Metrics': this.Metrics, @@ -32,7 +31,6 @@ describe('DocumentManager', function () { }) this.project_id = 'project-id-123' this.projectHistoryId = 'history-id-123' - this.projectHistoryType = 'project-history' this.doc_id = 'doc-id-123' this.user_id = 1234 this.callback = sinon.stub() @@ -82,12 +80,6 @@ describe('DocumentManager', function () { it('should time the execution', function () { this.Metrics.Timer.prototype.done.called.should.equal(true) }) - - it('should flush to the history api', function () { - this.HistoryManager.flushDocChangesAsync - .calledWithExactly(this.project_id, this.doc_id) - .should.equal(true) - }) }) describe('when a flush error occurs', function () { @@ -388,11 +380,9 @@ describe('DocumentManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId, - this.projectHistoryType + this.projectHistoryId ) this.RedisManager.putDocInMemory = sinon.stub().yields() - this.RedisManager.setHistoryType = sinon.stub().yields() this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) }) @@ -422,12 +412,6 @@ describe('DocumentManager', function () { .should.equal(true) }) - it('should set the history type in Redis', function () { - this.RedisManager.setHistoryType - .calledWith(this.doc_id, this.projectHistoryType) - .should.equal(true) - }) - it('should call the callback with the doc info', function () { this.callback .calledWith( diff --git a/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js b/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js index ecc5aacf02..ce98c4eaa8 100644 --- a/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js +++ b/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js @@ -1,14 +1,6 @@ /* eslint-disable mocha/no-nested-tests, - no-return-assign, */ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') const modulePath = require('path').join( @@ -24,85 +16,18 @@ describe('HistoryManager', function () { '@overleaf/settings': (this.Settings = { apis: { project_history: { - enabled: true, url: 'http://project_history.example.com', }, - trackchanges: { - url: 'http://trackchanges.example.com', - }, }, }), './DocumentManager': (this.DocumentManager = {}), - './HistoryRedisManager': (this.HistoryRedisManager = {}), './RedisManager': (this.RedisManager = {}), './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), './Metrics': (this.metrics = { inc: sinon.stub() }), }, }) this.project_id = 'mock-project-id' - this.doc_id = 'mock-doc-id' - return (this.callback = sinon.stub()) - }) - - describe('flushDocChangesAsync', function () { - beforeEach(function () { - return (this.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 204 })) - }) - - describe('when the project uses track changes', function () { - beforeEach(function () { - this.RedisManager.getHistoryType = sinon - .stub() - .yields(null, 'track-changes') - return this.HistoryManager.flushDocChangesAsync( - this.project_id, - this.doc_id - ) - }) - - return it('should send a request to the track changes api', function () { - return this.request.post - .calledWith( - `${this.Settings.apis.trackchanges.url}/project/${this.project_id}/doc/${this.doc_id}/flush` - ) - .should.equal(true) - }) - }) - - describe('when the project uses project history and double flush is not disabled', function () { - beforeEach(function () { - this.RedisManager.getHistoryType = sinon - .stub() - .yields(null, 'project-history') - return this.HistoryManager.flushDocChangesAsync( - this.project_id, - this.doc_id - ) - }) - - return it('should send a request to the track changes api', function () { - return this.request.post.called.should.equal(true) - }) - }) - - return describe('when the project uses project history and double flush is disabled', function () { - beforeEach(function () { - this.Settings.disableDoubleFlush = true - this.RedisManager.getHistoryType = sinon - .stub() - .yields(null, 'project-history') - return this.HistoryManager.flushDocChangesAsync( - this.project_id, - this.doc_id - ) - }) - - return it('should not send a request to the track changes api', function () { - return this.request.post.called.should.equal(false) - }) - }) + this.callback = sinon.stub() }) describe('flushProjectChangesAsync', function () { @@ -111,11 +36,11 @@ describe('HistoryManager', function () { .stub() .callsArgWith(1, null, { statusCode: 204 }) - return this.HistoryManager.flushProjectChangesAsync(this.project_id) + this.HistoryManager.flushProjectChangesAsync(this.project_id) }) - return it('should send a request to the project history api', function () { - return this.request.post + it('should send a request to the project history api', function () { + this.request.post .calledWith({ url: `${this.Settings.apis.project_history.url}/project/${this.project_id}/flush`, qs: { background: true }, @@ -130,7 +55,7 @@ describe('HistoryManager', function () { this.request.post = sinon .stub() .callsArgWith(1, null, { statusCode: 204 }) - return this.HistoryManager.flushProjectChanges( + this.HistoryManager.flushProjectChanges( this.project_id, { background: true, @@ -139,8 +64,8 @@ describe('HistoryManager', function () { ) }) - return it('should send a request to the project history api', function () { - return this.request.post + it('should send a request to the project history api', function () { + this.request.post .calledWith({ url: `${this.Settings.apis.project_history.url}/project/${this.project_id}/flush`, qs: { background: true }, @@ -149,10 +74,10 @@ describe('HistoryManager', function () { }) }) - return describe('with the skip_history_flush option', function () { + describe('with the skip_history_flush option', function () { beforeEach(function (done) { this.request.post = sinon.stub() - return this.HistoryManager.flushProjectChanges( + this.HistoryManager.flushProjectChanges( this.project_id, { skip_history_flush: true, @@ -161,8 +86,8 @@ describe('HistoryManager', function () { ) }) - return it('should not send a request to the project history api', function () { - return this.request.post.called.should.equal(false) + it('should not send a request to the project history api', function () { + this.request.post.called.should.equal(false) }) }) }) @@ -171,45 +96,21 @@ describe('HistoryManager', function () { beforeEach(function () { this.ops = ['mock-ops'] this.project_ops_length = 10 - this.doc_ops_length = 5 this.HistoryManager.flushProjectChangesAsync = sinon.stub() - this.HistoryRedisManager.recordDocHasHistoryOps = sinon.stub().callsArg(3) - return (this.HistoryManager.flushDocChangesAsync = sinon.stub()) }) describe('with no ops', function () { beforeEach(function () { - return this.HistoryManager.recordAndFlushHistoryOps( + this.HistoryManager.recordAndFlushHistoryOps( this.project_id, - this.doc_id, [], - this.doc_ops_length, - this.project_ops_length, - this.callback + this.project_ops_length ) }) it('should not flush project changes', function () { - return this.HistoryManager.flushProjectChangesAsync.called.should.equal( - false - ) - }) - - it('should not record doc has history ops', function () { - return this.HistoryRedisManager.recordDocHasHistoryOps.called.should.equal( - false - ) - }) - - it('should not flush doc changes', function () { - return this.HistoryManager.flushDocChangesAsync.called.should.equal( - false - ) - }) - - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + this.HistoryManager.flushProjectChangesAsync.called.should.equal(false) }) }) @@ -219,43 +120,19 @@ describe('HistoryManager', function () { this.HistoryManager.shouldFlushHistoryOps .withArgs(this.project_ops_length) .returns(true) - this.HistoryManager.shouldFlushHistoryOps - .withArgs(this.doc_ops_length) - .returns(false) - return this.HistoryManager.recordAndFlushHistoryOps( + this.HistoryManager.recordAndFlushHistoryOps( this.project_id, - this.doc_id, this.ops, - this.doc_ops_length, - this.project_ops_length, - this.callback + this.project_ops_length ) }) it('should flush project changes', function () { - return this.HistoryManager.flushProjectChangesAsync + this.HistoryManager.flushProjectChangesAsync .calledWith(this.project_id) .should.equal(true) }) - - it('should record doc has history ops', function () { - return this.HistoryRedisManager.recordDocHasHistoryOps.calledWith( - this.project_id, - this.doc_id, - this.ops - ) - }) - - it('should not flush doc changes', function () { - return this.HistoryManager.flushDocChangesAsync.called.should.equal( - false - ) - }) - - return it('should call the callback', function () { - return this.callback.called.should.equal(true) - }) }) describe('with enough ops to flush doc changes', function () { @@ -264,76 +141,22 @@ describe('HistoryManager', function () { this.HistoryManager.shouldFlushHistoryOps .withArgs(this.project_ops_length) .returns(false) - this.HistoryManager.shouldFlushHistoryOps - .withArgs(this.doc_ops_length) - .returns(true) - return this.HistoryManager.recordAndFlushHistoryOps( + this.HistoryManager.recordAndFlushHistoryOps( this.project_id, - this.doc_id, this.ops, - this.doc_ops_length, - this.project_ops_length, - this.callback + this.project_ops_length ) }) it('should not flush project changes', function () { - return this.HistoryManager.flushProjectChangesAsync.called.should.equal( - false - ) - }) - - it('should record doc has history ops', function () { - return this.HistoryRedisManager.recordDocHasHistoryOps.calledWith( - this.project_id, - this.doc_id, - this.ops - ) - }) - - it('should flush doc changes', function () { - return this.HistoryManager.flushDocChangesAsync - .calledWith(this.project_id, this.doc_id) - .should.equal(true) - }) - - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + this.HistoryManager.flushProjectChangesAsync.called.should.equal(false) }) }) - describe('when recording doc has history ops errors', function () { - beforeEach(function () { - this.error = new Error('error') - this.HistoryRedisManager.recordDocHasHistoryOps = sinon - .stub() - .callsArgWith(3, this.error) - - return this.HistoryManager.recordAndFlushHistoryOps( - this.project_id, - this.doc_id, - this.ops, - this.doc_ops_length, - this.project_ops_length, - this.callback - ) - }) - - it('should not flush doc changes', function () { - return this.HistoryManager.flushDocChangesAsync.called.should.equal( - false - ) - }) - - return it('should call the callback with the error', function () { - return this.callback.calledWith(this.error).should.equal(true) - }) - }) - - return describe('shouldFlushHistoryOps', function () { + describe('shouldFlushHistoryOps', function () { it('should return false if the number of ops is not known', function () { - return this.HistoryManager.shouldFlushHistoryOps( + this.HistoryManager.shouldFlushHistoryOps( null, ['a', 'b', 'c'].length, 1 @@ -354,18 +177,18 @@ describe('HistoryManager', function () { // Currently there are 15 ops // Previously we were on 12 ops // We've reached a new multiple of 5 - return this.HistoryManager.shouldFlushHistoryOps( + this.HistoryManager.shouldFlushHistoryOps( 15, ['a', 'b', 'c'].length, 5 ).should.equal(true) }) - return it('should return true if the updates took past the threshold', function () { + it('should return true if the updates took past the threshold', function () { // Currently there are 19 ops // Previously we were on 16 ops // We didn't pass over a multiple of 5 - return this.HistoryManager.shouldFlushHistoryOps( + this.HistoryManager.shouldFlushHistoryOps( 17, ['a', 'b', 'c'].length, 5 @@ -374,7 +197,7 @@ describe('HistoryManager', function () { }) }) - return describe('resyncProjectHistory', function () { + describe('resyncProjectHistory', function () { beforeEach(function () { this.projectHistoryId = 'history-id-1234' this.docs = [ @@ -394,7 +217,7 @@ describe('HistoryManager', function () { .stub() .yields() this.DocumentManager.resyncDocContentsWithLock = sinon.stub().yields() - return this.HistoryManager.resyncProjectHistory( + this.HistoryManager.resyncProjectHistory( this.project_id, this.projectHistoryId, this.docs, @@ -404,7 +227,7 @@ describe('HistoryManager', function () { }) it('should queue a project structure reync', function () { - return this.ProjectHistoryRedisManager.queueResyncProjectStructure + this.ProjectHistoryRedisManager.queueResyncProjectStructure .calledWith( this.project_id, this.projectHistoryId, @@ -415,13 +238,13 @@ describe('HistoryManager', function () { }) it('should queue doc content reyncs', function () { - return this.DocumentManager.resyncDocContentsWithLock + this.DocumentManager.resyncDocContentsWithLock .calledWith(this.project_id, this.docs[0].doc, this.docs[0].path) .should.equal(true) }) - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + it('should call the callback', function () { + this.callback.called.should.equal(true) }) }) }) diff --git a/services/document-updater/test/unit/js/HistoryRedisManager/HistoryRedisManagerTests.js b/services/document-updater/test/unit/js/HistoryRedisManager/HistoryRedisManagerTests.js deleted file mode 100644 index 3c4723701c..0000000000 --- a/services/document-updater/test/unit/js/HistoryRedisManager/HistoryRedisManagerTests.js +++ /dev/null @@ -1,99 +0,0 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const sinon = require('sinon') -const modulePath = '../../../../app/js/HistoryRedisManager.js' -const SandboxedModule = require('sandboxed-module') -const Errors = require('../../../../app/js/Errors') - -describe('HistoryRedisManager', function () { - beforeEach(function () { - this.rclient = { - auth() {}, - exec: sinon.stub(), - } - this.rclient.multi = () => this.rclient - this.HistoryRedisManager = SandboxedModule.require(modulePath, { - requires: { - './RedisManager': { rclient: this.rclient }, - '@overleaf/settings': { - redis: { - history: (this.settings = { - key_schema: { - uncompressedHistoryOps({ doc_id: docId }) { - return `UncompressedHistoryOps:${docId}` - }, - docsWithHistoryOps({ project_id: projectId }) { - return `DocsWithHistoryOps:${projectId}` - }, - }, - }), - }, - }, - }, - }) - this.doc_id = 'doc-id-123' - this.project_id = 'project-id-123' - return (this.callback = sinon.stub()) - }) - - return describe('recordDocHasHistoryOps', function () { - beforeEach(function () { - this.ops = [{ op: [{ i: 'foo', p: 4 }] }, { op: [{ i: 'bar', p: 56 }] }] - return (this.rclient.sadd = sinon.stub().yields()) - }) - - describe('with ops', function () { - beforeEach(function (done) { - return this.HistoryRedisManager.recordDocHasHistoryOps( - this.project_id, - this.doc_id, - this.ops, - (...args) => { - this.callback(...Array.from(args || [])) - return done() - } - ) - }) - - return it('should add the doc_id to the set of which records the project docs', function () { - return this.rclient.sadd - .calledWith(`DocsWithHistoryOps:${this.project_id}`, this.doc_id) - .should.equal(true) - }) - }) - - return describe('with no ops', function () { - beforeEach(function (done) { - return this.HistoryRedisManager.recordDocHasHistoryOps( - this.project_id, - this.doc_id, - [], - (...args) => { - this.callback(...Array.from(args || [])) - return done() - } - ) - }) - - it('should not add the doc_id to the set of which records the project docs', function () { - return this.rclient.sadd.called.should.equal(false) - }) - - return it('should call the callback with an error', function () { - return this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) - }) -}) diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 30ced07946..75e5c7d7f8 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -15,9 +15,6 @@ describe('RedisManager', function () { './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), '@overleaf/settings': (this.settings = { documentupdater: { logHashErrors: { write: true, read: true } }, - apis: { - project_history: { enabled: true }, - }, redis: { documentupdater: { key_schema: { @@ -54,9 +51,6 @@ describe('RedisManager', function () { projectHistoryId({ doc_id: docId }) { return `ProjectHistoryId:${docId}` }, - projectHistoryType({ doc_id: docId }) { - return `ProjectHistoryType:${docId}` - }, projectState({ project_id: projectId }) { return `ProjectState:${projectId}` }, @@ -71,16 +65,6 @@ describe('RedisManager', function () { }, }, }, - history: { - key_schema: { - uncompressedHistoryOps({ doc_id: docId }) { - return `UncompressedHistoryOps:${docId}` - }, - docsWithHistoryOps({ project_id: projectId }) { - return `DocsWithHistoryOps:${projectId}` - }, - }, - }, }, }), '@overleaf/redis-wrapper': { @@ -463,142 +447,94 @@ describe('RedisManager', function () { }) describe('with a consistent version', function () { - beforeEach(function () {}) - - describe('with project history enabled', function () { - beforeEach(function () { - this.settings.apis.project_history.enabled = true - this.RedisManager.getDocVersion - .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( - this.project_id, - this.docId, - this.lines, - this.version, - this.ops, - this.ranges, - this.updateMeta, - this.callback - ) - }) - - it('should get the current doc version to check for consistency', function () { - this.RedisManager.getDocVersion - .calledWith(this.docId) - .should.equal(true) - }) - - it('should set most details in a single MSET call', function () { - this.multi.mset - .calledWith({ - [`doclines:${this.docId}`]: JSON.stringify(this.lines), - [`DocVersion:${this.docId}`]: this.version, - [`DocHash:${this.docId}`]: this.hash, - [`Ranges:${this.docId}`]: JSON.stringify(this.ranges), - [`lastUpdatedAt:${this.docId}`]: Date.now(), - [`lastUpdatedBy:${this.docId}`]: 'last-author-fake-id', - }) - .should.equal(true) - }) - - it('should set the unflushed time', function () { - this.multi.set - .calledWith(`UnflushedTime:${this.docId}`, Date.now(), 'NX') - .should.equal(true) - }) - - it('should push the doc op into the doc ops list', function () { - this.multi.rpush - .calledWith( - `DocOps:${this.docId}`, - JSON.stringify(this.ops[0]), - JSON.stringify(this.ops[1]) - ) - .should.equal(true) - }) - - it('should renew the expiry ttl on the doc ops array', function () { - this.multi.expire - .calledWith(`DocOps:${this.docId}`, this.RedisManager.DOC_OPS_TTL) - .should.equal(true) - }) - - it('should truncate the list to 100 members', function () { - this.multi.ltrim - .calledWith( - `DocOps:${this.docId}`, - -this.RedisManager.DOC_OPS_MAX_LENGTH, - -1 - ) - .should.equal(true) - }) - - it('should push the updates into the history ops list', function () { - this.multi.rpush - .calledWith( - `UncompressedHistoryOps:${this.docId}`, - JSON.stringify(this.ops[0]), - JSON.stringify(this.ops[1]) - ) - .should.equal(true) - }) - - it('should push the updates into the project history ops list', function () { - this.ProjectHistoryRedisManager.queueOps - .calledWith(this.project_id, JSON.stringify(this.ops[0])) - .should.equal(true) - }) - - it('should call the callback', function () { + beforeEach(function () { + this.RedisManager.getDocVersion + .withArgs(this.docId) + .yields(null, this.version - this.ops.length) + this.RedisManager.updateDocument( + this.project_id, + this.docId, + this.lines, + this.version, + this.ops, + this.ranges, + this.updateMeta, this.callback - .calledWith( - null, - this.doc_update_list_length, - this.project_update_list_length - ) - .should.equal(true) - }) - - it('should not log any errors', function () { - this.logger.error.calledWith().should.equal(false) - }) + ) }) - describe('with project history disabled', function () { - beforeEach(function () { - this.settings.apis.project_history.enabled = false - this.RedisManager.getDocVersion - .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( - this.project_id, - this.docId, - this.lines, - this.version, - this.ops, - this.ranges, - this.updateMeta, - this.callback + it('should get the current doc version to check for consistency', function () { + this.RedisManager.getDocVersion + .calledWith(this.docId) + .should.equal(true) + }) + + it('should set most details in a single MSET call', function () { + this.multi.mset + .calledWith({ + [`doclines:${this.docId}`]: JSON.stringify(this.lines), + [`DocVersion:${this.docId}`]: this.version, + [`DocHash:${this.docId}`]: this.hash, + [`Ranges:${this.docId}`]: JSON.stringify(this.ranges), + [`lastUpdatedAt:${this.docId}`]: Date.now(), + [`lastUpdatedBy:${this.docId}`]: 'last-author-fake-id', + }) + .should.equal(true) + }) + + it('should set the unflushed time', function () { + this.multi.set + .calledWith(`UnflushedTime:${this.docId}`, Date.now(), 'NX') + .should.equal(true) + }) + + it('should push the doc op into the doc ops list', function () { + this.multi.rpush + .calledWith( + `DocOps:${this.docId}`, + JSON.stringify(this.ops[0]), + JSON.stringify(this.ops[1]) ) - }) + .should.equal(true) + }) - it('should not push the updates into the project history ops list', function () { - this.ProjectHistoryRedisManager.queueOps.called.should.equal(false) - }) + it('should renew the expiry ttl on the doc ops array', function () { + this.multi.expire + .calledWith(`DocOps:${this.docId}`, this.RedisManager.DOC_OPS_TTL) + .should.equal(true) + }) - it('should call the callback', function () { - this.callback - .calledWith(null, this.doc_update_list_length) - .should.equal(true) - }) + it('should truncate the list to 100 members', function () { + this.multi.ltrim + .calledWith( + `DocOps:${this.docId}`, + -this.RedisManager.DOC_OPS_MAX_LENGTH, + -1 + ) + .should.equal(true) + }) + + it('should push the updates into the project history ops list', function () { + this.ProjectHistoryRedisManager.queueOps + .calledWith(this.project_id, JSON.stringify(this.ops[0])) + .should.equal(true) + }) + + it('should call the callback', function () { + this.callback + .calledWith(null, this.project_update_list_length) + .should.equal(true) + }) + + it('should not log any errors', function () { + this.logger.error.calledWith().should.equal(false) }) describe('with a doc using project history only', function () { beforeEach(function () { this.RedisManager.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length, 'project-history') + .yields(null, this.version - this.ops.length) this.RedisManager.updateDocument( this.project_id, this.docId, @@ -611,12 +547,6 @@ describe('RedisManager', function () { ) }) - it('should not push the updates to the track-changes ops list', function () { - this.multi.rpush - .calledWith(`UncompressedHistoryOps:${this.docId}`) - .should.equal(false) - }) - it('should push the updates into the project history ops list', function () { this.ProjectHistoryRedisManager.queueOps .calledWith(this.project_id, JSON.stringify(this.ops[0])) @@ -625,7 +555,7 @@ describe('RedisManager', function () { it('should call the callback with the project update count only', function () { this.callback - .calledWith(null, undefined, this.project_update_list_length) + .calledWith(null, this.project_update_list_length) .should.equal(true) }) }) @@ -993,7 +923,6 @@ describe('RedisManager', function () { `Ranges:${this.docId}`, `Pathname:${this.docId}`, `ProjectHistoryId:${this.docId}`, - `ProjectHistoryType:${this.docId}`, `UnflushedTime:${this.docId}`, `lastUpdatedAt:${this.docId}`, `lastUpdatedBy:${this.docId}` @@ -1083,44 +1012,15 @@ describe('RedisManager', function () { describe('getDocVersion', function () { beforeEach(function () { this.version = 12345 + this.rclient.mget = sinon + .stub() + .withArgs(`DocVersion:${this.docId}`) + .callsArgWith(1, null, [`${this.version}`]) + this.RedisManager.getDocVersion(this.docId, this.callback) }) - describe('when the document does not have a project history type set', function () { - beforeEach(function () { - this.rclient.mget = sinon - .stub() - .withArgs( - `DocVersion:${this.docId}`, - `ProjectHistoryType:${this.docId}` - ) - .callsArgWith(2, null, [`${this.version}`]) - this.RedisManager.getDocVersion(this.docId, this.callback) - }) - - it('should return the document version and an undefined history type', function () { - this.callback - .calledWithExactly(null, this.version, undefined) - .should.equal(true) - }) - }) - - describe('when the document has a project history type set', function () { - beforeEach(function () { - this.rclient.mget = sinon - .stub() - .withArgs( - `DocVersion:${this.docId}`, - `ProjectHistoryType:${this.docId}` - ) - .callsArgWith(2, null, [`${this.version}`, 'project-history']) - this.RedisManager.getDocVersion(this.docId, this.callback) - }) - - it('should return the document version and history type', function () { - this.callback - .calledWithExactly(null, this.version, 'project-history') - .should.equal(true) - }) + it('should return the document version', function () { + this.callback.calledWithExactly(null, this.version).should.equal(true) }) }) }) diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 21bb98bb0b..249c840082 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -1,5 +1,4 @@ /* eslint-disable - no-return-assign, no-unused-vars, */ // TODO: This file was created by bulk-decaffeinate. @@ -7,7 +6,6 @@ /* * decaffeinate suggestions: * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns * DS206: Consider reworking classes to avoid initClass * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -22,7 +20,7 @@ describe('UpdateManager', function () { this.projectHistoryId = 'history-id-123' this.doc_id = 'document-id-123' this.callback = sinon.stub() - return (this.UpdateManager = SandboxedModule.require(modulePath, { + this.UpdateManager = SandboxedModule.require(modulePath, { requires: { './LockManager': (this.LockManager = {}), './RedisManager': (this.RedisManager = {}), @@ -56,13 +54,13 @@ describe('UpdateManager', function () { return Profiler })()), }, - })) + }) }) describe('processOutstandingUpdates', function () { beforeEach(function () { this.UpdateManager.fetchAndApplyUpdates = sinon.stub().callsArg(2) - return this.UpdateManager.processOutstandingUpdates( + this.UpdateManager.processOutstandingUpdates( this.project_id, this.doc_id, this.callback @@ -70,17 +68,17 @@ describe('UpdateManager', function () { }) it('should apply the updates', function () { - return this.UpdateManager.fetchAndApplyUpdates + this.UpdateManager.fetchAndApplyUpdates .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should call the callback', function () { - return this.callback.called.should.equal(true) + this.callback.called.should.equal(true) }) - return it('should time the execution', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the execution', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -94,14 +92,12 @@ describe('UpdateManager', function () { this.UpdateManager.continueProcessingUpdatesWithLock = sinon .stub() .callsArg(2) - return (this.UpdateManager.processOutstandingUpdates = sinon - .stub() - .callsArg(2)) + this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) }) describe('successfully', function () { beforeEach(function () { - return this.UpdateManager.processOutstandingUpdatesWithLock( + this.UpdateManager.processOutstandingUpdatesWithLock( this.project_id, this.doc_id, this.callback @@ -109,19 +105,17 @@ describe('UpdateManager', function () { }) it('should acquire the lock', function () { - return this.LockManager.tryLock - .calledWith(this.doc_id) - .should.equal(true) + this.LockManager.tryLock.calledWith(this.doc_id).should.equal(true) }) it('should free the lock', function () { - return this.LockManager.releaseLock + this.LockManager.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) it('should process the outstanding updates', function () { - return this.UpdateManager.processOutstandingUpdates + this.UpdateManager.processOutstandingUpdates .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -130,28 +124,28 @@ describe('UpdateManager', function () { this.UpdateManager.processOutstandingUpdates .calledAfter(this.LockManager.tryLock) .should.equal(true) - return this.UpdateManager.processOutstandingUpdates + this.UpdateManager.processOutstandingUpdates .calledBefore(this.LockManager.releaseLock) .should.equal(true) }) it('should continue processing new updates that may have come in', function () { - return this.UpdateManager.continueProcessingUpdatesWithLock + this.UpdateManager.continueProcessingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - return it('should return the callback', function () { - return this.callback.called.should.equal(true) + it('should return the callback', function () { + this.callback.called.should.equal(true) }) }) - return describe('when processOutstandingUpdates returns an error', function () { + describe('when processOutstandingUpdates returns an error', function () { beforeEach(function () { this.UpdateManager.processOutstandingUpdates = sinon .stub() .callsArgWith(2, (this.error = new Error('Something went wrong'))) - return this.UpdateManager.processOutstandingUpdatesWithLock( + this.UpdateManager.processOutstandingUpdatesWithLock( this.project_id, this.doc_id, this.callback @@ -159,22 +153,22 @@ describe('UpdateManager', function () { }) it('should free the lock', function () { - return this.LockManager.releaseLock + this.LockManager.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - return it('should return the error in the callback', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should return the error in the callback', function () { + this.callback.calledWith(this.error).should.equal(true) }) }) }) - return describe('when the lock is taken', function () { + describe('when the lock is taken', function () { beforeEach(function () { this.LockManager.tryLock = sinon.stub().callsArgWith(1, null, false) this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) - return this.UpdateManager.processOutstandingUpdatesWithLock( + this.UpdateManager.processOutstandingUpdatesWithLock( this.project_id, this.doc_id, this.callback @@ -182,13 +176,11 @@ describe('UpdateManager', function () { }) it('should return the callback', function () { - return this.callback.called.should.equal(true) + this.callback.called.should.equal(true) }) - return it('should not process the updates', function () { - return this.UpdateManager.processOutstandingUpdates.called.should.equal( - false - ) + it('should not process the updates', function () { + this.UpdateManager.processOutstandingUpdates.called.should.equal(false) }) }) }) @@ -202,7 +194,7 @@ describe('UpdateManager', function () { this.UpdateManager.processOutstandingUpdatesWithLock = sinon .stub() .callsArg(2) - return this.UpdateManager.continueProcessingUpdatesWithLock( + this.UpdateManager.continueProcessingUpdatesWithLock( this.project_id, this.doc_id, this.callback @@ -210,17 +202,17 @@ describe('UpdateManager', function () { }) it('should process the outstanding updates', function () { - return this.UpdateManager.processOutstandingUpdatesWithLock + this.UpdateManager.processOutstandingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - return it('should return the callback', function () { - return this.callback.called.should.equal(true) + it('should return the callback', function () { + this.callback.called.should.equal(true) }) }) - return describe('when there are no outstanding updates', function () { + describe('when there are no outstanding updates', function () { beforeEach(function () { this.RealTimeRedisManager.getUpdatesLength = sinon .stub() @@ -228,7 +220,7 @@ describe('UpdateManager', function () { this.UpdateManager.processOutstandingUpdatesWithLock = sinon .stub() .callsArg(2) - return this.UpdateManager.continueProcessingUpdatesWithLock( + this.UpdateManager.continueProcessingUpdatesWithLock( this.project_id, this.doc_id, this.callback @@ -236,13 +228,13 @@ describe('UpdateManager', function () { }) it('should not try to process the outstanding updates', function () { - return this.UpdateManager.processOutstandingUpdatesWithLock.called.should.equal( + this.UpdateManager.processOutstandingUpdatesWithLock.called.should.equal( false ) }) - return it('should return the callback', function () { - return this.callback.called.should.equal(true) + it('should return the callback', function () { + this.callback.called.should.equal(true) }) }) }) @@ -259,7 +251,7 @@ describe('UpdateManager', function () { this.UpdateManager.applyUpdate = sinon .stub() .callsArgWith(3, null, this.updatedDocLines, this.version) - return this.UpdateManager.fetchAndApplyUpdates( + this.UpdateManager.fetchAndApplyUpdates( this.project_id, this.doc_id, this.callback @@ -267,25 +259,25 @@ describe('UpdateManager', function () { }) it('should get the pending updates', function () { - return this.RealTimeRedisManager.getPendingUpdatesForDoc + this.RealTimeRedisManager.getPendingUpdatesForDoc .calledWith(this.doc_id) .should.equal(true) }) it('should apply the updates', function () { - return Array.from(this.updates).map(update => + Array.from(this.updates).map(update => this.UpdateManager.applyUpdate .calledWith(this.project_id, this.doc_id, update) .should.equal(true) ) }) - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + it('should call the callback', function () { + this.callback.called.should.equal(true) }) }) - return describe('when there are no updates', function () { + describe('when there are no updates', function () { beforeEach(function () { this.updates = [] this.RealTimeRedisManager.getPendingUpdatesForDoc = sinon @@ -293,7 +285,7 @@ describe('UpdateManager', function () { .callsArgWith(1, null, this.updates) this.UpdateManager.applyUpdate = sinon.stub() this.RedisManager.setDocument = sinon.stub() - return this.UpdateManager.fetchAndApplyUpdates( + this.UpdateManager.fetchAndApplyUpdates( this.project_id, this.doc_id, this.callback @@ -301,11 +293,11 @@ describe('UpdateManager', function () { }) it('should not call applyUpdate', function () { - return this.UpdateManager.applyUpdate.called.should.equal(false) + this.UpdateManager.applyUpdate.called.should.equal(false) }) - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + it('should call the callback', function () { + this.callback.called.should.equal(true) }) }) }) @@ -323,7 +315,6 @@ describe('UpdateManager', function () { { v: 42, op: 'mock-op-42' }, { v: 45, op: 'mock-op-45' }, ] - this.doc_ops_length = sinon.stub() this.project_ops_length = sinon.stub() this.pathname = '/a/b/c.tex' this.DocumentManager.getDoc = sinon @@ -344,17 +335,15 @@ describe('UpdateManager', function () { .yields(null, this.updatedDocLines, this.version, this.appliedOps) this.RedisManager.updateDocument = sinon .stub() - .yields(null, this.doc_ops_length, this.project_ops_length) + .yields(null, this.project_ops_length) this.RealTimeRedisManager.sendData = sinon.stub() this.UpdateManager._addProjectHistoryMetadataToOps = sinon.stub() - return (this.HistoryManager.recordAndFlushHistoryOps = sinon - .stub() - .callsArg(5)) + this.HistoryManager.recordAndFlushHistoryOps = sinon.stub() }) describe('normally', function () { beforeEach(function () { - return this.UpdateManager.applyUpdate( + this.UpdateManager.applyUpdate( this.project_id, this.doc_id, this.update, @@ -363,7 +352,7 @@ describe('UpdateManager', function () { }) it('should apply the updates via ShareJS', function () { - return this.ShareJsUpdateManager.applyUpdate + this.ShareJsUpdateManager.applyUpdate .calledWith( this.project_id, this.doc_id, @@ -375,7 +364,7 @@ describe('UpdateManager', function () { }) it('should update the ranges', function () { - return this.RangesManager.applyUpdate + this.RangesManager.applyUpdate .calledWith( this.project_id, this.doc_id, @@ -387,7 +376,7 @@ describe('UpdateManager', function () { }) it('should save the document', function () { - return this.RedisManager.updateDocument + this.RedisManager.updateDocument .calledWith( this.project_id, this.doc_id, @@ -401,7 +390,7 @@ describe('UpdateManager', function () { }) it('should add metadata to the ops', function () { - return this.UpdateManager._addProjectHistoryMetadataToOps + this.UpdateManager._addProjectHistoryMetadataToOps .calledWith( this.appliedOps, this.pathname, @@ -412,26 +401,20 @@ describe('UpdateManager', function () { }) it('should push the applied ops into the history queue', function () { - return this.HistoryManager.recordAndFlushHistoryOps - .calledWith( - this.project_id, - this.doc_id, - this.appliedOps, - this.doc_ops_length, - this.project_ops_length - ) + this.HistoryManager.recordAndFlushHistoryOps + .calledWith(this.project_id, this.appliedOps, this.project_ops_length) .should.equal(true) }) - return it('should call the callback', function () { - return this.callback.called.should.equal(true) + it('should call the callback', function () { + this.callback.called.should.equal(true) }) }) describe('with UTF-16 surrogate pairs in the update', function () { beforeEach(function () { this.update = { op: [{ p: 42, i: '\uD835\uDC00' }] } - return this.UpdateManager.applyUpdate( + this.UpdateManager.applyUpdate( this.project_id, this.doc_id, this.update, @@ -439,13 +422,13 @@ describe('UpdateManager', function () { ) }) - return it('should apply the update but with surrogate pairs removed', function () { + it('should apply the update but with surrogate pairs removed', function () { this.ShareJsUpdateManager.applyUpdate .calledWith(this.project_id, this.doc_id, this.update) .should.equal(true) // \uFFFD is 'replacement character' - return this.update.op[0].i.should.equal('\uFFFD\uFFFD') + this.update.op[0].i.should.equal('\uFFFD\uFFFD') }) }) @@ -453,7 +436,7 @@ describe('UpdateManager', function () { beforeEach(function () { this.error = new Error('something went wrong') this.ShareJsUpdateManager.applyUpdate = sinon.stub().yields(this.error) - return this.UpdateManager.applyUpdate( + this.UpdateManager.applyUpdate( this.project_id, this.doc_id, this.update, @@ -462,7 +445,7 @@ describe('UpdateManager', function () { }) it('should call RealTimeRedisManager.sendData with the error', function () { - return this.RealTimeRedisManager.sendData + this.RealTimeRedisManager.sendData .calledWith({ project_id: this.project_id, doc_id: this.doc_id, @@ -471,18 +454,18 @@ describe('UpdateManager', function () { .should.equal(true) }) - return it('should call the callback with the error', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should call the callback with the error', function () { + this.callback.calledWith(this.error).should.equal(true) }) }) - return describe('when ranges get collapsed', function () { + describe('when ranges get collapsed', function () { beforeEach(function () { this.RangesManager.applyUpdate = sinon .stub() .yields(null, this.updated_ranges, true) this.SnapshotManager.recordSnapshot = sinon.stub().yields() - return this.UpdateManager.applyUpdate( + this.UpdateManager.applyUpdate( this.project_id, this.doc_id, this.update, @@ -494,8 +477,8 @@ describe('UpdateManager', function () { this.Metrics.inc.calledWith('doc-snapshot').should.equal(true) }) - return it('should call SnapshotManager.recordSnapshot', function () { - return this.SnapshotManager.recordSnapshot + it('should call SnapshotManager.recordSnapshot', function () { + this.SnapshotManager.recordSnapshot .calledWith( this.project_id, this.doc_id, @@ -510,7 +493,7 @@ describe('UpdateManager', function () { }) describe('_addProjectHistoryMetadataToOps', function () { - return it('should add projectHistoryId, pathname and doc_length metadata to the ops', function () { + it('should add projectHistoryId, pathname and doc_length metadata to the ops', function () { const lines = ['some', 'test', 'data'] const appliedOps = [ { @@ -535,7 +518,7 @@ describe('UpdateManager', function () { this.projectHistoryId, lines ) - return appliedOps.should.deep.equal([ + appliedOps.should.deep.equal([ { projectHistoryId: this.projectHistoryId, v: 42, @@ -573,7 +556,7 @@ describe('UpdateManager', function () { }) }) - return describe('lockUpdatesAndDo', function () { + describe('lockUpdatesAndDo', function () { beforeEach(function () { this.method = sinon.stub().callsArgWith(3, null, this.response_arg1) this.callback = sinon.stub() @@ -583,14 +566,14 @@ describe('UpdateManager', function () { this.LockManager.getLock = sinon .stub() .callsArgWith(1, null, this.lockValue) - return (this.LockManager.releaseLock = sinon.stub().callsArg(2)) + this.LockManager.releaseLock = sinon.stub().callsArg(2) }) describe('successfully', function () { beforeEach(function () { this.UpdateManager.continueProcessingUpdatesWithLock = sinon.stub() this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) - return this.UpdateManager.lockUpdatesAndDo( + this.UpdateManager.lockUpdatesAndDo( this.method, this.project_id, this.doc_id, @@ -600,37 +583,33 @@ describe('UpdateManager', function () { }) it('should lock the doc', function () { - return this.LockManager.getLock - .calledWith(this.doc_id) - .should.equal(true) + this.LockManager.getLock.calledWith(this.doc_id).should.equal(true) }) it('should process any outstanding updates', function () { - return this.UpdateManager.processOutstandingUpdates + this.UpdateManager.processOutstandingUpdates .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should call the method', function () { - return this.method + this.method .calledWith(this.project_id, this.doc_id, this.arg1) .should.equal(true) }) it('should return the method response to the callback', function () { - return this.callback - .calledWith(null, this.response_arg1) - .should.equal(true) + this.callback.calledWith(null, this.response_arg1).should.equal(true) }) it('should release the lock', function () { - return this.LockManager.releaseLock + this.LockManager.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - return it('should continue processing updates', function () { - return this.UpdateManager.continueProcessingUpdatesWithLock + it('should continue processing updates', function () { + this.UpdateManager.continueProcessingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -641,7 +620,7 @@ describe('UpdateManager', function () { this.UpdateManager.processOutstandingUpdates = sinon .stub() .callsArgWith(2, (this.error = new Error('Something went wrong'))) - return this.UpdateManager.lockUpdatesAndDo( + this.UpdateManager.lockUpdatesAndDo( this.method, this.project_id, this.doc_id, @@ -651,17 +630,17 @@ describe('UpdateManager', function () { }) it('should free the lock', function () { - return this.LockManager.releaseLock + this.LockManager.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - return it('should return the error in the callback', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should return the error in the callback', function () { + this.callback.calledWith(this.error).should.equal(true) }) }) - return describe('when the method returns an error', function () { + describe('when the method returns an error', function () { beforeEach(function () { this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) this.method = sinon @@ -671,7 +650,7 @@ describe('UpdateManager', function () { (this.error = new Error('something went wrong')), this.response_arg1 ) - return this.UpdateManager.lockUpdatesAndDo( + this.UpdateManager.lockUpdatesAndDo( this.method, this.project_id, this.doc_id, @@ -681,13 +660,13 @@ describe('UpdateManager', function () { }) it('should free the lock', function () { - return this.LockManager.releaseLock + this.LockManager.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - return it('should return the error in the callback', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should return the error in the callback', function () { + this.callback.calledWith(this.error).should.equal(true) }) }) })