From 6f70f7710c7af9ee88afc3ca8053d3618cc6cca8 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 9 Sep 2025 08:10:57 -0400 Subject: [PATCH] Merge pull request #28311 from overleaf/em-promisify-redis-manager Promisify RedisManager in document-updater GitOrigin-RevId: 87c86d3b8904e17e76756789d2ea3e09fab26d02 --- .../document-updater/app/js/RedisManager.js | 974 ++++++++---------- .../unit/js/RedisManager/RedisManagerTests.js | 598 +++++------ 2 files changed, 677 insertions(+), 895 deletions(-) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index b14bd76f2b..2c9ca4934f 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -2,11 +2,10 @@ const Settings = require('@overleaf/settings') const RedisWrapper = require('@overleaf/redis-wrapper') const logger = require('@overleaf/logger') const OError = require('@overleaf/o-error') -const { promisifyAll } = require('@overleaf/promise-utils') +const { callbackifyAll } = require('@overleaf/promise-utils') const metrics = require('./Metrics') const Errors = require('./Errors') const crypto = require('node:crypto') -const async = require('async') const { docIsTooLarge } = require('./Limits') const rclient = RedisWrapper.createClient(Settings.redis.documentupdater) @@ -29,9 +28,7 @@ const MAX_RANGES_SIZE = 3 * MEGABYTES const keys = Settings.redis.documentupdater.key_schema const RedisManager = { - rclient, - - putDocInMemory( + async putDocInMemory( projectId, docId, docLines, @@ -40,23 +37,16 @@ const RedisManager = { resolvedCommentIds, pathname, projectHistoryId, - historyRangesSupport, - _callback + historyRangesSupport ) { const timer = new metrics.Timer('redis.put-doc') - const callback = error => { - timer.done() - _callback(error) - } const shareJSTextOT = Array.isArray(docLines) const docLinesArray = docLines docLines = JSON.stringify(docLines) if (docLines.indexOf('\u0000') !== -1) { - const error = new Error('null bytes found in doc lines') // this check was added to catch memory corruption in JSON.stringify. // It sometimes returned null bytes at the end of the string. - logger.error({ err: error, docId, docLines }, error.message) - return callback(error) + throw new OError('null bytes found in doc lines', { docId }) } // Do an optimised size check on the docLines using the serialised // length as an upper bound @@ -66,9 +56,11 @@ const RedisManager = { docIsTooLarge(sizeBound, docLinesArray, Settings.max_doc_length) ) { const docSize = docLines.length - const err = new Error('blocking doc insert into redis: doc is too large') - logger.error({ projectId, docId, err, docSize }, err.message) - return callback(err) + throw new OError('blocking doc insert into redis: doc is too large', { + projectId, + docId, + docSize, + }) } const docHash = RedisManager._computeHash(docLines) // record bytes sent to redis @@ -77,104 +69,74 @@ const RedisManager = { { projectId, docId, version, docHash, pathname, projectHistoryId }, 'putting doc in redis' ) - RedisManager._serializeRanges(ranges, (error, ranges) => { - if (error) { - logger.error({ err: error, docId, projectId }, error.message) - return callback(error) - } + ranges = RedisManager._serializeRanges(ranges) - // update docsInProject set before writing doc contents - const multi = rclient.multi() - multi.exists(keys.projectBlock({ project_id: projectId })) - multi.sadd(keys.docsInProject({ project_id: projectId }), docId) - multi.exec((err, reply) => { - if (err) { - return callback(err) - } - const projectBlocked = reply[0] === 1 - if (projectBlocked) { - // We don't clean up the spurious docId added in the docsInProject - // set. There is a risk that the docId was successfully added by a - // concurrent process. This set is used when unloading projects. An - // extra docId will not prevent the project from being uploaded, but - // a missing docId means that the doc might stay in Redis forever. - return callback( - new OError('Project blocked from loading docs', { projectId }) - ) - } + // update docsInProject set before writing doc contents + const projectBlockMulti = rclient.multi() + projectBlockMulti.exists(keys.projectBlock({ project_id: projectId })) + projectBlockMulti.sadd(keys.docsInProject({ project_id: projectId }), docId) + const reply = await projectBlockMulti.exec() + const projectBlocked = reply[0] === 1 + if (projectBlocked) { + // We don't clean up the spurious docId added in the docsInProject + // set. There is a risk that the docId was successfully added by a + // concurrent process. This set is used when unloading projects. An + // extra docId will not prevent the project from being uploaded, but + // a missing docId means that the doc might stay in Redis forever. + throw new OError('Project blocked from loading docs', { projectId }) + } - RedisManager.setHistoryRangesSupportFlag( - docId, - historyRangesSupport, - err => { - if (err) { - return callback(err) - } + await RedisManager.setHistoryRangesSupportFlag(docId, historyRangesSupport) - if (!pathname) { - metrics.inc('pathname', 1, { - path: 'RedisManager.setDoc', - status: pathname === '' ? 'zero-length' : 'undefined', - }) - } - - // Make sure that this MULTI operation only operates on doc - // specific keys, i.e. keys that have the doc id in curly braces. - // The curly braces identify a hash key for Redis and ensures that - // the MULTI's operations are all done on the same node in a - // cluster environment. - const multi = rclient.multi() - multi.mset({ - [keys.docLines({ doc_id: docId })]: docLines, - [keys.projectKey({ doc_id: docId })]: projectId, - [keys.docVersion({ doc_id: docId })]: version, - [keys.docHash({ doc_id: docId })]: docHash, - [keys.ranges({ doc_id: docId })]: ranges, - [keys.pathname({ doc_id: docId })]: pathname, - [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId, - }) - if (historyRangesSupport) { - multi.del(keys.resolvedCommentIds({ doc_id: docId })) - if (resolvedCommentIds.length > 0) { - multi.sadd( - keys.resolvedCommentIds({ doc_id: docId }), - ...resolvedCommentIds - ) - } - } - multi.exec(err => { - if (err) { - callback( - OError.tag(err, 'failed to write doc to Redis in MULTI', { - previousErrors: err.previousErrors.map(e => ({ - name: e.name, - message: e.message, - command: e.command, - })), - }) - ) - } else { - callback() - } - }) - } - ) + if (!pathname) { + metrics.inc('pathname', 1, { + path: 'RedisManager.setDoc', + status: pathname === '' ? 'zero-length' : 'undefined', }) - }) - }, + } - removeDocFromMemory(projectId, docId, _callback) { - logger.debug({ projectId, docId }, 'removing doc from redis') - const callback = err => { - if (err) { - logger.err({ projectId, docId, err }, 'error removing doc from redis') - _callback(err) - } else { - logger.debug({ projectId, docId }, 'removed doc from redis') - _callback() + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. + const multi = rclient.multi() + multi.mset({ + [keys.docLines({ doc_id: docId })]: docLines, + [keys.projectKey({ doc_id: docId })]: projectId, + [keys.docVersion({ doc_id: docId })]: version, + [keys.docHash({ doc_id: docId })]: docHash, + [keys.ranges({ doc_id: docId })]: ranges, + [keys.pathname({ doc_id: docId })]: pathname, + [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId, + }) + if (historyRangesSupport) { + multi.del(keys.resolvedCommentIds({ doc_id: docId })) + if (resolvedCommentIds.length > 0) { + multi.sadd( + keys.resolvedCommentIds({ doc_id: docId }), + ...resolvedCommentIds + ) } } + try { + await multi.exec() + } catch (err) { + throw OError.tag(err, 'failed to write doc to Redis in MULTI', { + previousErrors: err.previousErrors.map(e => ({ + name: e.name, + message: e.message, + command: e.command, + })), + }) + } + timer.done() + }, + + async removeDocFromMemory(projectId, docId) { + logger.debug({ projectId, docId }, 'removing doc from redis') + // Make sure that this MULTI operation only operates on doc // specific keys, i.e. keys that have the doc id in curly braces. // The curly braces identify a hash key for Redis and ensures that @@ -195,34 +157,27 @@ const RedisManager = { keys.lastUpdatedBy({ doc_id: docId }), keys.resolvedCommentIds({ doc_id: docId }) ) - multi.exec((error, response) => { - if (error) { - return callback(error) - } - const length = response?.[0] - if (length > 0) { - // record bytes freed in redis - metrics.summary('redis.docLines', length, { status: 'del' }) - } + const response = await multi.exec() + const length = response?.[0] + if (length > 0) { + // record bytes freed in redis + metrics.summary('redis.docLines', length, { status: 'del' }) + } - // Make sure that this MULTI operation only operates on project - // specific keys, i.e. keys that have the project id in curly braces. - // The curly braces identify a hash key for Redis and ensures that - // the MULTI's operations are all done on the same node in a - // cluster environment. - multi = rclient.multi() - multi.srem(keys.docsInProject({ project_id: projectId }), docId) - multi.del(keys.projectState({ project_id: projectId })) - multi.exec(err => { - if (err) { - return callback(err) - } - rclient.srem(keys.historyRangesSupport(), docId, callback) - }) - }) + // Make sure that this MULTI operation only operates on project + // specific keys, i.e. keys that have the project id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. + multi = rclient.multi() + multi.srem(keys.docsInProject({ project_id: projectId }), docId) + multi.del(keys.projectState({ project_id: projectId })) + await multi.exec() + + await rclient.srem(keys.historyRangesSupport(), docId) }, - checkOrSetProjectState(projectId, newState, callback) { + async checkOrSetProjectState(projectId, newState) { // Make sure that this MULTI operation only operates on project // specific keys, i.e. keys that have the project id in curly braces. // The curly braces identify a hash key for Redis and ensures that @@ -231,23 +186,19 @@ const RedisManager = { const multi = rclient.multi() multi.getset(keys.projectState({ project_id: projectId }), newState) multi.expire(keys.projectState({ project_id: projectId }), 30 * minutes) - multi.exec((error, response) => { - if (error) { - return callback(error) - } - logger.debug( - { projectId, newState, oldState: response[0] }, - 'checking project state' - ) - callback(null, response[0] !== newState) - }) + const response = await multi.exec() + logger.debug( + { projectId, newState, oldState: response[0] }, + 'checking project state' + ) + return response[0] !== newState }, - clearProjectState(projectId, callback) { - rclient.del(keys.projectState({ project_id: projectId }), callback) + async clearProjectState(projectId) { + await rclient.del(keys.projectState({ project_id: projectId })) }, - getDoc(projectId, docId, callback) { + async getDoc(projectId, docId) { const timer = new metrics.Timer('redis.get-doc') const collectKeys = [ keys.docLines({ doc_id: docId }), @@ -261,454 +212,356 @@ const RedisManager = { keys.lastUpdatedAt({ doc_id: docId }), keys.lastUpdatedBy({ doc_id: docId }), ] - rclient.mget(...collectKeys, (error, result) => { - if (error) { - return callback(error) + let [ + docLines, + version, + storedHash, + docProjectId, + ranges, + pathname, + projectHistoryId, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy, + ] = await rclient.mget(...collectKeys) + const result = await rclient.sismember(keys.historyRangesSupport(), docId) + const historyRangesSupport = result === 1 + + const resolvedCommentIds = await rclient.smembers( + keys.resolvedCommentIds({ doc_id: docId }) + ) + + const timeSpan = timer.done() + // check if request took too long and bail out. only do this for + // get, because it is the first call in each update, so if this + // passes we'll assume others have a reasonable chance to succeed. + if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { + throw new OError('redis getDoc exceeded timeout', { projectId, docId }) + } + // record bytes loaded from redis + if (docLines != null) { + metrics.summary('redis.docLines', docLines.length, { + status: 'get', + }) + } + // check sha1 hash value if present + if (docLines != null && storedHash != null) { + const computedHash = RedisManager._computeHash(docLines) + if (logHashReadErrors && computedHash !== storedHash) { + logger.error( + { + projectId, + docId, + docProjectId, + computedHash, + storedHash, + docLines, + }, + 'hash mismatch on retrieved document' + ) } - let [ - docLines, - version, - storedHash, + } + + docLines = JSON.parse(docLines) + ranges = RedisManager._deserializeRanges(ranges) + + version = parseInt(version || 0, 10) + // check doc is in requested project + if (docProjectId != null && docProjectId !== projectId) { + throw new Errors.NotFoundError('document not found', { + projectId, + docId, docProjectId, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy, - ] = result - rclient.sismember(keys.historyRangesSupport(), docId, (error, result) => { - if (error) { - return callback(error) - } - rclient.smembers( - keys.resolvedCommentIds({ doc_id: docId }), - (error, resolvedCommentIds) => { - if (error) { - return callback(error) - } - - const historyRangesSupport = result === 1 - - const timeSpan = timer.done() - // check if request took too long and bail out. only do this for - // get, because it is the first call in each update, so if this - // passes we'll assume others have a reasonable chance to succeed. - if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { - error = new Error('redis getDoc exceeded timeout') - return callback(error) - } - // record bytes loaded from redis - if (docLines != null) { - metrics.summary('redis.docLines', docLines.length, { - status: 'get', - }) - } - // check sha1 hash value if present - if (docLines != null && storedHash != null) { - const computedHash = RedisManager._computeHash(docLines) - if (logHashReadErrors && computedHash !== storedHash) { - logger.error( - { - projectId, - docId, - docProjectId, - computedHash, - storedHash, - docLines, - }, - 'hash mismatch on retrieved document' - ) - } - } - - try { - docLines = JSON.parse(docLines) - ranges = RedisManager._deserializeRanges(ranges) - } catch (e) { - return callback(e) - } - - version = parseInt(version || 0, 10) - // check doc is in requested project - if (docProjectId != null && docProjectId !== projectId) { - logger.error( - { projectId, docId, docProjectId }, - 'doc not in project' - ) - return callback(new Errors.NotFoundError('document not found')) - } - - if (docLines && version && !pathname) { - metrics.inc('pathname', 1, { - path: 'RedisManager.getDoc', - status: pathname === '' ? 'zero-length' : 'undefined', - }) - } - - callback( - null, - docLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy, - historyRangesSupport, - resolvedCommentIds - ) - } - ) }) - }) + } + + if (docLines && version && !pathname) { + metrics.inc('pathname', 1, { + path: 'RedisManager.getDoc', + status: pathname === '' ? 'zero-length' : 'undefined', + }) + } + + return { + lines: docLines, + version, + ranges, + pathname, + projectHistoryId, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy, + historyRangesSupport, + resolvedCommentIds, + } }, - getDocVersion(docId, callback) { - rclient.mget(keys.docVersion({ doc_id: docId }), (error, result) => { - if (error) { - return callback(error) - } - let [version] = result || [] - version = parseInt(version, 10) - callback(null, version) - }) + async getDocVersion(docId) { + const result = await rclient.mget(keys.docVersion({ doc_id: docId })) + let [version] = result || [] + version = parseInt(version, 10) + return version }, - getDocLines(docId, callback) { - rclient.get(keys.docLines({ doc_id: docId }), (error, docLines) => { - if (error) { - return callback(error) - } - callback(null, docLines) - }) + async getDocLines(docId) { + const docLines = await rclient.get(keys.docLines({ doc_id: docId })) + return docLines }, - getPreviousDocOps(docId, start, end, callback) { + async getPreviousDocOps(docId, start, end) { const timer = new metrics.Timer('redis.get-prev-docops') - rclient.llen(keys.docOps({ doc_id: docId }), (error, length) => { - if (error) { - return callback(error) - } - rclient.get(keys.docVersion({ doc_id: docId }), (error, version) => { - if (error) { - return callback(error) - } - version = parseInt(version, 10) - const firstVersionInRedis = version - length + const length = await rclient.llen(keys.docOps({ doc_id: docId })) + let version = await rclient.get(keys.docVersion({ doc_id: docId })) + version = parseInt(version, 10) + const firstVersionInRedis = version - length - if (start < firstVersionInRedis || end > version) { - error = new Errors.OpRangeNotAvailableError( - 'doc ops range is not loaded in redis', - { firstVersionInRedis, version, ttlInS: RedisManager.DOC_OPS_TTL } - ) - logger.debug( - { err: error, docId, length, version, start, end }, - 'doc ops range is not loaded in redis' - ) - return callback(error) - } + if (start < firstVersionInRedis || end > version) { + throw new Errors.OpRangeNotAvailableError( + 'doc ops range is not loaded in redis', + { firstVersionInRedis, version, ttlInS: RedisManager.DOC_OPS_TTL } + ) + } - start = start - firstVersionInRedis - if (end > -1) { - end = end - firstVersionInRedis - } + start = start - firstVersionInRedis + if (end > -1) { + end = end - firstVersionInRedis + } - if (isNaN(start) || isNaN(end)) { - error = new Error('inconsistent version or lengths') - logger.error( - { err: error, docId, length, version, start, end }, - 'inconsistent version or length' - ) - return callback(error) - } - - rclient.lrange( - keys.docOps({ doc_id: docId }), - start, - end, - (error, jsonOps) => { - let ops - if (error) { - return callback(error) - } - try { - ops = jsonOps.map(jsonOp => JSON.parse(jsonOp)) - } catch (e) { - return callback(e) - } - const timeSpan = timer.done() - if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { - error = new Error('redis getPreviousDocOps exceeded timeout') - return callback(error) - } - callback(null, ops) - } - ) + if (isNaN(start) || isNaN(end)) { + throw new OError('inconsistent version or lengths', { + docId, + length, + version, + start, + end, }) - }) + } + + const jsonOps = await rclient.lrange( + keys.docOps({ doc_id: docId }), + start, + end + ) + const ops = jsonOps.map(jsonOp => JSON.parse(jsonOp)) + const timeSpan = timer.done() + if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { + throw new Error('redis getPreviousDocOps exceeded timeout') + } + return ops }, DOC_OPS_TTL: 60 * minutes, DOC_OPS_MAX_LENGTH: 100, - updateDocument( + async updateDocument( projectId, docId, docLines, newVersion, appliedOps, ranges, - updateMeta, - callback + updateMeta ) { if (appliedOps == null) { appliedOps = [] } const shareJSTextOT = Array.isArray(docLines) - 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 currentVersion = await RedisManager.getDocVersion(docId) + if (currentVersion + appliedOps.length !== newVersion) { + throw new OError(`Version mismatch. '${docId}' is corrupted.`, { + docId, + currentVersion, + newVersion, + opsLength: appliedOps.length, + }) + } - 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') + const jsonOps = appliedOps.map(op => JSON.stringify(op)) + for (const op of jsonOps) { + if (op.indexOf('\u0000') !== -1) { // 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 ( - shareJSTextOT && // editor-core has a size check in TextOperation.apply and TextOperation.applyToLength. - 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( - { + throw new OError('null bytes found in jsonOps', { 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) - } + jsonOps, + }) + } + } - // Make sure that this MULTI operation only operates on doc - // specific keys, i.e. keys that have the doc id in curly braces. - // The curly braces identify a hash key for Redis and ensures that - // the MULTI's operations are all done on the same node in a - // cluster environment. - 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 not set ("NX" flag). - multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX') - multi.exec((error, result) => { - if (error) { - return callback(error) - } - callback() - }) + const newDocLines = JSON.stringify(docLines) + if (newDocLines.indexOf('\u0000') !== -1) { + // this check was added to catch memory corruption in JSON.stringify + throw new OError('null bytes found in doc lines', { + docId, + newDocLines, }) + } + // Do an optimised size check on the docLines using the serialised + // length as an upper bound + const sizeBound = newDocLines.length + if ( + shareJSTextOT && // editor-core has a size check in TextOperation.apply and TextOperation.applyToLength. + docIsTooLarge(sizeBound, docLines, Settings.max_doc_length) + ) { + const docSize = newDocLines.length + throw new OError('blocking doc update: doc is too large', { + projectId, + docId, + docSize, + }) + } + 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', }) + const jsonRanges = RedisManager._serializeRanges(ranges) + if (jsonRanges && jsonRanges.indexOf('\u0000') !== -1) { + // this check was added to catch memory corruption in JSON.stringify + throw new OError('null bytes found in ranges', { docId }) + } + + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. + 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 })]: jsonRanges, + [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 not set ("NX" flag). + multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX') + await multi.exec() }, - renameDoc(projectId, docId, userId, update, projectHistoryId, callback) { - RedisManager.getDoc(projectId, docId, (error, lines, version) => { - if (error) { - return callback(error) - } - if (lines != null && version != null) { - if (!update.newPathname) { - logger.warn( - { projectId, docId, update }, - 'missing pathname in RedisManager.renameDoc' - ) - metrics.inc('pathname', 1, { - path: 'RedisManager.renameDoc', - status: update.newPathname === '' ? 'zero-length' : 'undefined', - }) - } - rclient.set( - keys.pathname({ doc_id: docId }), - update.newPathname, - callback + async renameDoc(projectId, docId, userId, update, projectHistoryId) { + const { lines, version } = await RedisManager.getDoc(projectId, docId) + if (lines != null && version != null) { + if (!update.newPathname) { + logger.warn( + { projectId, docId, update }, + 'missing pathname in RedisManager.renameDoc' ) - } else { - callback() + metrics.inc('pathname', 1, { + path: 'RedisManager.renameDoc', + status: update.newPathname === '' ? 'zero-length' : 'undefined', + }) } - }) - }, - - clearUnflushedTime(docId, callback) { - rclient.del(keys.unflushedTime({ doc_id: docId }), callback) - }, - - updateCommentState(docId, commentId, resolved, callback) { - if (resolved) { - rclient.sadd( - keys.resolvedCommentIds({ doc_id: docId }), - commentId, - callback - ) - } else { - rclient.srem( - keys.resolvedCommentIds({ doc_id: docId }), - commentId, - callback - ) + await rclient.set(keys.pathname({ doc_id: docId }), update.newPathname) } }, - getDocIdsInProject(projectId, callback) { - rclient.smembers(keys.docsInProject({ project_id: projectId }), callback) + async clearUnflushedTime(docId) { + await rclient.del(keys.unflushedTime({ doc_id: docId })) + }, + + async updateCommentState(docId, commentId, resolved) { + if (resolved) { + await rclient.sadd(keys.resolvedCommentIds({ doc_id: docId }), commentId) + } else { + await rclient.srem(keys.resolvedCommentIds({ doc_id: docId }), commentId) + } + }, + + async getDocIdsInProject(projectId) { + return await rclient.smembers(keys.docsInProject({ project_id: projectId })) }, /** * Get lastupdatedat timestamps for an array of docIds */ - getDocTimestamps(docIds, callback) { - async.mapSeries( - docIds, - (docId, cb) => rclient.get(keys.lastUpdatedAt({ doc_id: docId }), cb), - callback - ) + async getDocTimestamps(docIds) { + const timestamps = [] + for (const docId of docIds) { + const timestamp = await rclient.get(keys.lastUpdatedAt({ doc_id: docId })) + timestamps.push(timestamp) + } + return timestamps }, /** * Store the project id in a sorted set ordered by time with a random offset * to smooth out spikes */ - queueFlushAndDeleteProject(projectId, callback) { + async queueFlushAndDeleteProject(projectId) { const SMOOTHING_OFFSET = Settings.smoothingOffset > 0 ? Math.round(Settings.smoothingOffset * Math.random()) : 0 - rclient.zadd( + await rclient.zadd( keys.flushAndDeleteQueue(), Date.now() + SMOOTHING_OFFSET, - projectId, - callback + projectId ) }, /** * Find the oldest queued flush that is before the cutoff time */ - getNextProjectToFlushAndDelete(cutoffTime, callback) { - rclient.zrangebyscore( + async getNextProjectToFlushAndDelete(cutoffTime) { + const projectsReady = await rclient.zrangebyscore( keys.flushAndDeleteQueue(), 0, cutoffTime, 'WITHSCORES', 'LIMIT', 0, - 1, - (err, reply) => { - if (err) { - return callback(err) - } - // return if no projects ready to be processed - if (!reply || reply.length === 0) { - return callback() - } - // pop the oldest entry (get and remove in a multi) - const multi = rclient.multi() - // Poor man's version of ZPOPMIN, which is only available in Redis 5. - multi.zrange(keys.flushAndDeleteQueue(), 0, 0, 'WITHSCORES') - multi.zremrangebyrank(keys.flushAndDeleteQueue(), 0, 0) - multi.zcard(keys.flushAndDeleteQueue()) // the total length of the queue (for metrics) - multi.exec((err, reply) => { - if (err) { - return callback(err) - } - if (!reply || reply.length === 0) { - return callback() - } - const [key, timestamp] = reply[0] - const queueLength = reply[2] - callback(null, key, timestamp, queueLength) - }) - } + 1 ) + // return if no projects ready to be processed + if (!projectsReady || projectsReady.length === 0) { + return + } + // pop the oldest entry (get and remove in a multi) + const multi = rclient.multi() + // Poor man's version of ZPOPMIN, which is only available in Redis 5. + multi.zrange(keys.flushAndDeleteQueue(), 0, 0, 'WITHSCORES') + multi.zremrangebyrank(keys.flushAndDeleteQueue(), 0, 0) + multi.zcard(keys.flushAndDeleteQueue()) // the total length of the queue (for metrics) + const reply = await multi.exec() + if (!reply || reply.length === 0) { + return + } + const [key, timestamp] = reply[0] + const queueLength = reply[2] + return { projectId: key, flushTimestamp: timestamp, queueLength } }, - setHistoryRangesSupportFlag(docId, historyRangesSupport, callback) { + async setHistoryRangesSupportFlag(docId, historyRangesSupport) { if (historyRangesSupport) { - rclient.sadd(keys.historyRangesSupport(), docId, callback) + await rclient.sadd(keys.historyRangesSupport(), docId) } else { - rclient.srem(keys.historyRangesSupport(), docId, callback) + await rclient.srem(keys.historyRangesSupport(), docId) } }, - blockProject(projectId, callback) { + async blockProject(projectId) { // Make sure that this MULTI operation only operates on project // specific keys, i.e. keys that have the project id in curly braces. // The curly braces identify a hash key for Redis and ensures that @@ -721,45 +574,34 @@ const RedisManager = { '1' ) multi.scard(keys.docsInProject({ project_id: projectId })) - multi.exec((err, reply) => { - if (err) { - return callback(err) - } - const docsInProject = reply[1] - if (docsInProject > 0) { - // Too late to lock the project - rclient.del(keys.projectBlock({ project_id: projectId }), err => { - if (err) { - return callback(err) - } - callback(null, false) - }) - } else { - callback(null, true) - } - }) + const reply = await multi.exec() + const docsInProject = reply[1] + if (docsInProject > 0) { + // Too late to lock the project + await rclient.del(keys.projectBlock({ project_id: projectId })) + return false + } + return true }, - unblockProject(projectId, callback) { - rclient.del(keys.projectBlock({ project_id: projectId }), (err, reply) => { - if (err) { - return callback(err) - } - const wasBlocked = reply === 1 - callback(null, wasBlocked) - }) + async unblockProject(projectId) { + const reply = await rclient.del( + keys.projectBlock({ project_id: projectId }) + ) + const wasBlocked = reply === 1 + return wasBlocked }, - _serializeRanges(ranges, callback) { + _serializeRanges(ranges) { let jsonRanges = JSON.stringify(ranges) if (jsonRanges && jsonRanges.length > MAX_RANGES_SIZE) { - return callback(new Error('ranges are too large')) + throw new Error('ranges are too large') } if (jsonRanges === '{}') { // Most doc will have empty ranges so don't fill redis with lots of '{}' keys jsonRanges = null } - callback(null, jsonRanges) + return jsonRanges }, _deserializeRanges(ranges) { @@ -783,26 +625,28 @@ const RedisManager = { }, } -module.exports = RedisManager -module.exports.promises = promisifyAll(RedisManager, { - without: ['_deserializeRanges', '_computeHash'], - multiResult: { - getDoc: [ - 'lines', - 'version', - 'ranges', - 'pathname', - 'projectHistoryId', - 'unflushedTime', - 'lastUpdatedAt', - 'lastUpdatedBy', - 'historyRangesSupport', - 'resolvedCommentIds', - ], - getNextProjectToFlushAndDelete: [ - 'projectId', - 'flushTimestamp', - 'queueLength', - ], - }, -}) +module.exports = { + rclient, + ...callbackifyAll(RedisManager, { + multiResult: { + getDoc: [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + 'unflushedTime', + 'lastUpdatedAt', + 'lastUpdatedBy', + 'historyRangesSupport', + 'resolvedCommentIds', + ], + getNextProjectToFlushAndDelete: [ + 'projectId', + 'flushTimestamp', + 'queueLength', + ], + }, + }), + promises: RedisManager, +} diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 125dd3d08c..7e3919e6f8 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -9,8 +9,8 @@ const MODULE_PATH = '../../../../app/js/RedisManager.js' describe('RedisManager', function () { beforeEach(function () { - this.multi = { exec: sinon.stub().yields() } - this.rclient = { multi: () => this.multi, srem: sinon.stub().yields() } + this.multi = { exec: sinon.stub().resolves() } + this.rclient = { multi: () => this.multi, srem: sinon.stub().resolves() } tk.freeze(new Date()) this.RedisManager = SandboxedModule.require(MODULE_PATH, { requires: { @@ -102,7 +102,6 @@ describe('RedisManager', function () { this.project_id = 'project-id-123' this.projectHistoryId = '123' this.historyRangesSupport = false - this.callback = sinon.stub() }) afterEach(function () { @@ -125,7 +124,7 @@ describe('RedisManager', function () { this.pathname = '/a/b/c.tex' this.rclient.mget = sinon .stub() - .yields(null, [ + .resolves([ this.jsonlines, this.version, this.hash, @@ -138,16 +137,19 @@ describe('RedisManager', function () { this.rclient.sismember = sinon.stub() this.rclient.sismember .withArgs('HistoryRangesSupport', this.docId) - .yields(null, 0) + .resolves(0) this.rclient.smembers = sinon.stub() this.rclient.smembers .withArgs(`ResolvedCommentIds:${this.docId}`) - .yields(null, this.resolvedCommentIds) + .resolves(this.resolvedCommentIds) }) describe('successfully', function () { - beforeEach(function () { - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) + beforeEach(async function () { + this.result = await this.RedisManager.promises.getDoc( + this.project_id, + this.docId + ) }) it('should get all the details in one call to redis', function () { @@ -168,19 +170,18 @@ describe('RedisManager', function () { }) it('should return the document', function () { - this.callback.should.have.been.calledWithExactly( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushed_time, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.historyRangesSupport, - this.resolvedCommentIds - ) + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushed_time, + lastUpdatedAt: this.lastUpdatedAt, + lastUpdatedBy: this.lastUpdatedBy, + historyRangesSupport: this.historyRangesSupport, + resolvedCommentIds: this.resolvedCommentIds, + }) }) it('should not log any errors', function () { @@ -189,18 +190,21 @@ describe('RedisManager', function () { }) describe('with a corrupted document', function () { - beforeEach(function () { + beforeEach(async function () { this.badHash = 'INVALID-HASH-VALUE' this.rclient.mget = sinon .stub() - .yields(null, [ + .resolves([ this.jsonlines, this.version, this.badHash, this.project_id, this.json_ranges, ]) - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) + this.result = await this.RedisManager.promises.getDoc( + this.project_id, + this.docId + ) }) it('should log a hash error', function () { @@ -208,48 +212,47 @@ describe('RedisManager', function () { }) it('should return the document', function () { - this.callback - .calledWith(null, this.lines, this.version, this.ranges) - .should.equal(true) + expect(this.result).to.deep.include({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + }) }) }) describe('with a slow request to redis', function () { beforeEach(function () { this.clock = sinon.useFakeTimers() - this.rclient.mget = (...args) => { - const cb = args.pop() + this.rclient.mget = async (...args) => { this.clock.tick(6000) - cb(null, [ + return [ this.jsonlines, this.version, this.another_project_id, this.json_ranges, this.pathname, this.unflushed_time, - ]) + ] } - - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) }) afterEach(function () { this.clock.restore() }) - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should return an error', async function () { + await expect( + this.RedisManager.promises.getDoc(this.project_id, this.docId) + ).to.be.rejected }) }) describe('getDoc with an invalid project id', function () { - beforeEach(function () { + beforeEach(async function () { this.another_project_id = 'project-id-456' this.rclient.mget = sinon .stub() - .yields(null, [ + .resolves([ this.jsonlines, this.version, this.hash, @@ -258,45 +261,46 @@ describe('RedisManager', function () { this.pathname, this.unflushed_time, ]) - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) }) - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.RedisManager.promises.getDoc(this.project_id, this.docId) + ).to.be.rejectedWith(Errors.NotFoundError) }) }) describe('with history ranges support', function () { - beforeEach(function () { + beforeEach(async function () { this.rclient.sismember .withArgs('HistoryRangesSupport', this.docId) - .yields(null, 1) - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) + .resolves(1) + this.result = await this.RedisManager.promises.getDoc( + this.project_id, + this.docId + ) }) it('should return the document with the history ranges flag set', function () { - this.callback.should.have.been.calledWithExactly( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushed_time, - this.lastUpdatedAt, - this.lastUpdatedBy, - true, - this.resolvedCommentIds - ) + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushed_time, + lastUpdatedAt: this.lastUpdatedAt, + lastUpdatedBy: this.lastUpdatedBy, + historyRangesSupport: true, + resolvedCommentIds: this.resolvedCommentIds, + }) }) }) }) describe('getPreviousDocOpsTests', function () { describe('with a start and an end value', function () { - beforeEach(function () { + beforeEach(async function () { this.first_version_in_redis = 30 this.version = 70 this.length = this.version - this.first_version_in_redis @@ -304,16 +308,13 @@ describe('RedisManager', function () { this.end = 60 this.ops = [{ mock: 'op-1' }, { mock: 'op-2' }] this.jsonOps = this.ops.map(op => JSON.stringify(op)) - this.rclient.llen = sinon.stub().callsArgWith(1, null, this.length) - this.rclient.get = sinon - .stub() - .callsArgWith(1, null, this.version.toString()) - this.rclient.lrange = sinon.stub().callsArgWith(3, null, this.jsonOps) - this.RedisManager.getPreviousDocOps( + this.rclient.llen = sinon.stub().resolves(this.length) + this.rclient.get = sinon.stub().resolves(this.version.toString()) + this.rclient.lrange = sinon.stub().resolves(this.jsonOps) + this.result = await this.RedisManager.promises.getPreviousDocOps( this.docId, this.start, - this.end, - this.callback + this.end ) }) @@ -338,12 +339,12 @@ describe('RedisManager', function () { }) it('should return the docs with the doc ops deserialized', function () { - this.callback.calledWith(null, this.ops).should.equal(true) + this.result.should.deep.equal(this.ops) }) }) describe('with an end value of -1', function () { - beforeEach(function () { + beforeEach(async function () { this.first_version_in_redis = 30 this.version = 70 this.length = this.version - this.first_version_in_redis @@ -351,16 +352,13 @@ describe('RedisManager', function () { this.end = -1 this.ops = [{ mock: 'op-1' }, { mock: 'op-2' }] this.jsonOps = this.ops.map(op => JSON.stringify(op)) - this.rclient.llen = sinon.stub().callsArgWith(1, null, this.length) - this.rclient.get = sinon - .stub() - .callsArgWith(1, null, this.version.toString()) - this.rclient.lrange = sinon.stub().callsArgWith(3, null, this.jsonOps) - this.RedisManager.getPreviousDocOps( + this.rclient.llen = sinon.stub().resolves(this.length) + this.rclient.get = sinon.stub().resolves(this.version.toString()) + this.rclient.lrange = sinon.stub().resolves(this.jsonOps) + this.result = await this.RedisManager.promises.getPreviousDocOps( this.docId, this.start, - this.end, - this.callback + this.end ) }) @@ -375,12 +373,12 @@ describe('RedisManager', function () { }) it('should return the docs with the doc ops deserialized', function () { - this.callback.calledWith(null, this.ops).should.equal(true) + this.result.should.deep.equal(this.ops) }) }) describe('when the requested range is not in Redis', function () { - beforeEach(function () { + beforeEach(async function () { this.first_version_in_redis = 30 this.version = 70 this.length = this.version - this.first_version_in_redis @@ -388,44 +386,35 @@ describe('RedisManager', function () { this.end = -1 this.ops = [{ mock: 'op-1' }, { mock: 'op-2' }] this.jsonOps = this.ops.map(op => JSON.stringify(op)) - this.rclient.llen = sinon.stub().callsArgWith(1, null, this.length) - this.rclient.get = sinon - .stub() - .callsArgWith(1, null, this.version.toString()) - this.rclient.lrange = sinon.stub().callsArgWith(3, null, this.jsonOps) - this.RedisManager.getPreviousDocOps( - this.docId, - this.start, - this.end, - this.callback - ) + this.rclient.llen = sinon.stub().resolves(this.length) + this.rclient.get = sinon.stub().resolves(this.version.toString()) + this.rclient.lrange = sinon.stub().resolves(this.jsonOps) + try { + await this.RedisManager.promises.getPreviousDocOps( + this.docId, + this.start, + this.end + ) + } catch (err) { + this.err = err + } }) - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.OpRangeNotAvailableError)) - .should.equal(true) + it('should throw an error', function () { + expect(this.err).to.be.instanceOf(Errors.OpRangeNotAvailableError) }) it('should send details for metrics', function () { - this.callback.should.have.been.calledWith( - sinon.match({ - info: { - firstVersionInRedis: this.first_version_in_redis, - version: this.version, - ttlInS: this.RedisManager.DOC_OPS_TTL, - }, - }) - ) - }) - - it('should log out the problem as a debug message', function () { - this.logger.debug.called.should.equal(true) + expect(this.err.info).to.deep.equal({ + firstVersionInRedis: this.first_version_in_redis, + version: this.version, + ttlInS: this.RedisManager.DOC_OPS_TTL, + }) }) }) describe('with a slow request to redis', function () { - beforeEach(function () { + beforeEach(async function () { this.first_version_in_redis = 30 this.version = 70 this.length = this.version - this.first_version_in_redis @@ -433,31 +422,27 @@ describe('RedisManager', function () { this.end = 60 this.ops = [{ mock: 'op-1' }, { mock: 'op-2' }] this.jsonOps = this.ops.map(op => JSON.stringify(op)) - this.rclient.llen = sinon.stub().callsArgWith(1, null, this.length) - this.rclient.get = sinon - .stub() - .callsArgWith(1, null, this.version.toString()) + this.rclient.llen = sinon.stub().resolves(this.length) + this.rclient.get = sinon.stub().resolves(this.version.toString()) this.clock = sinon.useFakeTimers() - this.rclient.lrange = (key, start, end, cb) => { + this.rclient.lrange = async (key, start, end) => { this.clock.tick(6000) - cb(null, this.jsonOps) + return this.jsonOps } - this.RedisManager.getPreviousDocOps( - this.docId, - this.start, - this.end, - this.callback - ) }) afterEach(function () { this.clock.restore() }) - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should return an error', async function () { + await expect( + this.RedisManager.promises.getPreviousDocOps( + this.docId, + this.start, + this.end + ) + ).to.be.rejected }) }) }) @@ -476,7 +461,7 @@ describe('RedisManager', function () { this.doc_update_list_length = sinon.stub() this.project_update_list_length = sinon.stub() - this.RedisManager.getDocVersion = sinon.stub() + this.RedisManager.promises.getDocVersion = sinon.stub() this.multi.mset = sinon.stub() this.multi.set = sinon.stub() this.multi.rpush = sinon.stub() @@ -485,7 +470,7 @@ describe('RedisManager', function () { this.multi.del = sinon.stub() this.multi.exec = sinon .stub() - .callsArgWith(0, null, [ + .resolves([ null, null, null, @@ -497,26 +482,25 @@ describe('RedisManager', function () { }) describe('with a consistent version', function () { - beforeEach(function () { - this.RedisManager.getDocVersion + beforeEach(async function () { + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( + .resolves(this.version - this.ops.length) + await this.RedisManager.promises.updateDocument( this.project_id, this.docId, this.lines, this.version, this.ops, this.ranges, - this.updateMeta, - this.callback + this.updateMeta ) }) it('should get the current doc version to check for consistency', function () { - this.RedisManager.getDocVersion - .calledWith(this.docId) - .should.equal(true) + this.RedisManager.promises.getDocVersion.should.have.been.calledWith( + this.docId + ) }) it('should set most details in a single MSET call', function () { @@ -564,79 +548,67 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should call the callback', function () { - this.callback.should.have.been.called - }) - 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 + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( + .resolves(this.version - this.ops.length) + }) + + it('should succeed', async function () { + await this.RedisManager.promises.updateDocument( this.project_id, this.docId, this.lines, this.version, this.ops, this.ranges, - this.updateMeta, - this.callback + this.updateMeta ) }) - - it('should call the callback', function () { - this.callback.should.have.been.called - }) }) }) describe('with an inconsistent version', function () { - beforeEach(function () { - this.RedisManager.getDocVersion + beforeEach(async function () { + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length - 1) - this.RedisManager.updateDocument( - this.project_id, - this.docId, - this.lines, - this.version, - this.ops, - this.ranges, - this.updateMeta, - this.callback - ) + .resolves(this.version - this.ops.length - 1) + await expect( + this.RedisManager.promises.updateDocument( + this.project_id, + this.docId, + this.lines, + this.version, + this.ops, + this.ranges, + this.updateMeta + ) + ).to.be.rejected }) it('should not call multi.exec', function () { this.multi.exec.called.should.equal(false) }) - - it('should call the callback with an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) }) describe('with no updates', function () { - beforeEach(function () { - this.RedisManager.getDocVersion + beforeEach(async function () { + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version) - this.RedisManager.updateDocument( + .resolves(this.version) + await this.RedisManager.promises.updateDocument( this.project_id, this.docId, this.lines, this.version, [], this.ranges, - this.updateMeta, - this.callback + this.updateMeta ) }) @@ -665,19 +637,18 @@ describe('RedisManager', function () { }) describe('with empty ranges', function () { - beforeEach(function () { - this.RedisManager.getDocVersion + beforeEach(async function () { + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( + .resolves(this.version - this.ops.length) + await this.RedisManager.promises.updateDocument( this.project_id, this.docId, this.lines, this.version, this.ops, {}, - this.updateMeta, - this.callback + this.updateMeta ) }) @@ -697,84 +668,71 @@ describe('RedisManager', function () { describe('with null bytes in the serialized doc lines', function () { beforeEach(function () { - this.RedisManager.getDocVersion + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) + .resolves(this.version - this.ops.length) this.stringifyStub = sinon .stub(JSON, 'stringify') .callsFake(() => '["bad bytes! \u0000 <- here"]') - this.RedisManager.updateDocument( - this.project_id, - this.docId, - this.lines, - this.version, - this.ops, - this.ranges, - this.updateMeta, - this.callback - ) }) afterEach(function () { this.stringifyStub.restore() }) - it('should log an error', function () { - this.logger.error.called.should.equal(true) - }) - - it('should call the callback with an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.RedisManager.promises.updateDocument( + this.project_id, + this.docId, + this.lines, + this.version, + this.ops, + this.ranges, + this.updateMeta + ) + ).to.be.rejected }) }) describe('with ranges that are too big', function () { beforeEach(function () { - this.RedisManager.getDocVersion + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager._serializeRanges = sinon + .resolves(this.version - this.ops.length) + this.RedisManager.promises._serializeRanges = sinon .stub() - .yields(new Error('ranges are too large')) - this.RedisManager.updateDocument( - this.project_id, - this.docId, - this.lines, - this.version, - this.ops, - this.ranges, - this.updateMeta, - this.callback - ) + .throws(new Error('ranges are too large')) }) - it('should log an error', function () { - this.logger.error.called.should.equal(true) - }) - - it('should call the callback with the error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.RedisManager.promises.updateDocument( + this.project_id, + this.docId, + this.lines, + this.version, + this.ops, + this.ranges, + this.updateMeta + ) + ).to.be.rejected }) }) describe('without user id from meta', function () { - beforeEach(function () { - this.RedisManager.getDocVersion + beforeEach(async function () { + this.RedisManager.promises.getDocVersion .withArgs(this.docId) - .yields(null, this.version - this.ops.length) - this.RedisManager.updateDocument( + .resolves(this.version - this.ops.length) + await this.RedisManager.promises.updateDocument( this.project_id, this.docId, this.lines, this.version, this.ops, this.ranges, - {}, - this.callback + {} ) }) @@ -799,8 +757,8 @@ describe('RedisManager', function () { this.multi.sadd = sinon.stub() this.multi.del = sinon.stub() this.multi.exists = sinon.stub() - this.multi.exec.onCall(0).yields(null, [0]) - this.rclient.sadd = sinon.stub().yields() + this.multi.exec.onCall(0).resolves([0]) + this.rclient.sadd = sinon.stub().resolves() this.lines = ['one', 'two', 'three', 'これは'] this.version = 42 this.hash = crypto @@ -813,8 +771,8 @@ describe('RedisManager', function () { }) describe('with non-empty ranges', function () { - beforeEach(function (done) { - this.RedisManager.putDocInMemory( + beforeEach(async function () { + await this.RedisManager.promises.putDocInMemory( this.project_id, this.docId, this.lines, @@ -823,8 +781,7 @@ describe('RedisManager', function () { this.resolvedCommentIds, this.pathname, this.projectHistoryId, - this.historyRangesSupport, - done + this.historyRangesSupport ) }) @@ -867,8 +824,8 @@ describe('RedisManager', function () { }) describe('with empty ranges', function () { - beforeEach(function (done) { - this.RedisManager.putDocInMemory( + beforeEach(async function () { + await this.RedisManager.promises.putDocInMemory( this.project_id, this.docId, this.lines, @@ -877,8 +834,7 @@ describe('RedisManager', function () { [], this.pathname, this.projectHistoryId, - this.historyRangesSupport, - done + this.historyRangesSupport ) }) @@ -900,69 +856,57 @@ describe('RedisManager', function () { this.stringifyStub = sinon .stub(JSON, 'stringify') .callsFake(() => '["bad bytes! \u0000 <- here"]') - this.RedisManager.putDocInMemory( - this.project_id, - this.docId, - this.lines, - this.version, - this.ranges, - this.resolvedCommentIds, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport, - this.callback - ) }) afterEach(function () { this.stringifyStub.restore() }) - it('should log an error', function () { - this.logger.error.called.should.equal(true) - }) - - it('should call the callback with an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.RedisManager.promises.putDocInMemory( + this.project_id, + this.docId, + this.lines, + this.version, + this.ranges, + this.resolvedCommentIds, + this.pathname, + this.projectHistoryId, + this.historyRangesSupport + ) + ).to.be.rejected }) }) describe('with ranges that are too big', function () { beforeEach(function () { - this.RedisManager._serializeRanges = sinon + this.RedisManager.promises._serializeRanges = sinon .stub() - .yields(new Error('ranges are too large')) - this.RedisManager.putDocInMemory( - this.project_id, - this.docId, - this.lines, - this.version, - this.ranges, - this.resolvedCommentIds, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport, - this.callback - ) + .throws(new Error('ranges are too large')) }) - it('should log an error', function () { - this.logger.error.called.should.equal(true) - }) - - it('should call the callback with the error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.RedisManager.promises.putDocInMemory( + this.project_id, + this.docId, + this.lines, + this.version, + this.ranges, + this.resolvedCommentIds, + this.pathname, + this.projectHistoryId, + this.historyRangesSupport + ) + ).to.be.rejected }) }) describe('with history ranges support', function () { - beforeEach(function (done) { + beforeEach(async function () { this.historyRangesSupport = true - this.RedisManager.putDocInMemory( + await this.RedisManager.promises.putDocInMemory( this.project_id, this.docId, this.lines, @@ -971,8 +915,7 @@ describe('RedisManager', function () { this.resolvedCommentIds, this.pathname, this.projectHistoryId, - this.historyRangesSupport, - done + this.historyRangesSupport ) }) @@ -995,27 +938,21 @@ describe('RedisManager', function () { }) describe('when the project is blocked', function () { - beforeEach(function (done) { - this.multi.exec.onCall(0).yields(null, [1]) - this.RedisManager.putDocInMemory( - this.project_id, - this.docId, - this.lines, - this.version, - this.ranges, - this.resolvedCommentIds, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport, - err => { - this.error = err - done() - } - ) - }) - - it('should throw an error', function () { - expect(this.error.message).to.equal('Project blocked from loading docs') + beforeEach(async function () { + this.multi.exec.onCall(0).resolves([1]) + await expect( + this.RedisManager.promises.putDocInMemory( + this.project_id, + this.docId, + this.lines, + this.version, + this.ranges, + this.resolvedCommentIds, + this.pathname, + this.projectHistoryId, + this.historyRangesSupport + ) + ).to.be.rejectedWith('Project blocked from loading docs') }) it('should not store the doc', function () { @@ -1025,12 +962,15 @@ describe('RedisManager', function () { }) describe('removeDocFromMemory', function () { - beforeEach(function (done) { + beforeEach(async function () { this.multi.strlen = sinon.stub() this.multi.del = sinon.stub() this.multi.srem = sinon.stub() - this.multi.exec.yields() - this.RedisManager.removeDocFromMemory(this.project_id, this.docId, done) + this.multi.exec.resolves() + await this.RedisManager.promises.removeDocFromMemory( + this.project_id, + this.docId + ) }) it('should check the length of the current doclines', function () { @@ -1070,9 +1010,9 @@ describe('RedisManager', function () { }) describe('clearProjectState', function () { - beforeEach(function (done) { - this.rclient.del = sinon.stub().callsArg(1) - this.RedisManager.clearProjectState(this.project_id, done) + beforeEach(async function () { + this.rclient.del = sinon.stub().resolves() + await this.RedisManager.promises.clearProjectState(this.project_id) }) it('should delete the project state', function () { @@ -1084,8 +1024,8 @@ describe('RedisManager', function () { describe('renameDoc', function () { beforeEach(function () { - this.rclient.rpush = sinon.stub().yields() - this.rclient.set = sinon.stub().yields() + this.rclient.rpush = sinon.stub().resolves() + this.rclient.set = sinon.stub().resolves() this.update = { id: this.docId, pathname: (this.pathname = 'pathname'), @@ -1094,17 +1034,16 @@ describe('RedisManager', function () { }) describe('the document is cached in redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon + beforeEach(async function () { + this.RedisManager.promises.getDoc = sinon .stub() - .callsArgWith(2, null, 'lines', 'version') - this.RedisManager.renameDoc( + .resolves({ lines: 'lines', version: 'version' }) + await this.RedisManager.promises.renameDoc( this.project_id, this.docId, this.userId, this.update, - this.projectHistoryId, - this.callback + this.projectHistoryId ) }) @@ -1116,17 +1055,16 @@ describe('RedisManager', function () { }) describe('the document is not cached in redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon + beforeEach(async function () { + this.RedisManager.promises.getDoc = sinon .stub() - .callsArgWith(2, null, null, null) - this.RedisManager.renameDoc( + .resolves({ lines: null, version: null }) + await this.RedisManager.promises.renameDoc( this.project_id, this.docId, this.userId, this.update, - this.projectHistoryId, - this.callback + this.projectHistoryId ) }) @@ -1136,17 +1074,17 @@ describe('RedisManager', function () { }) describe('getDocVersion', function () { - beforeEach(function () { + beforeEach(async 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) + .resolves([`${this.version}`]) + this.result = await this.RedisManager.promises.getDocVersion(this.docId) }) it('should return the document version', function () { - this.callback.calledWithExactly(null, this.version).should.equal(true) + this.result.should.equal(this.version) }) }) })