From 9d78fd0945a6f10790cfed3ed2fee59e93924b39 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 3 Apr 2024 10:08:16 -0400 Subject: [PATCH] Merge pull request #17690 from overleaf/em-promisify-project-history-redis-manager Promisify ProjectHistoryRedisManager GitOrigin-RevId: 11908c6cc102653b34e2014af230e3912cd8b432 --- .../app/js/ProjectHistoryRedisManager.js | 60 +++++------ .../ProjectHistoryRedisManagerTests.js | 99 +++++++++---------- 2 files changed, 67 insertions(+), 92 deletions(-) diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 47c8386383..9babdd26a3 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -1,5 +1,5 @@ const Settings = require('@overleaf/settings') -const { promisifyAll } = require('@overleaf/promise-utils') +const { callbackifyAll } = require('@overleaf/promise-utils') const projectHistoryKeys = Settings.redis?.project_history?.key_schema const rclient = require('@overleaf/redis-wrapper').createClient( Settings.redis.project_history @@ -7,12 +7,11 @@ const rclient = require('@overleaf/redis-wrapper').createClient( const logger = require('@overleaf/logger') const metrics = require('./Metrics') const { docIsTooLarge } = require('./Limits') +const OError = require('@overleaf/o-error') const ProjectHistoryRedisManager = { - queueOps(projectId, ...rest) { + async queueOps(projectId, ...ops) { // Record metric for ops pushed onto queue - const callback = rest.pop() - const ops = rest for (const op of ops) { metrics.summary('redis.projectHistoryOps', op.length, { status: 'push' }) } @@ -30,24 +29,18 @@ const ProjectHistoryRedisManager = { }), Date.now() ) - multi.exec(function (error, result) { - if (error) { - return callback(error) - } - // return the number of entries pushed onto the project history queue - callback(null, result[0]) - }) + const result = await multi.exec() + return result[0] }, - queueRenameEntity( + async queueRenameEntity( projectId, projectHistoryId, entityType, entityId, userId, projectUpdate, - source, - callback + source ) { projectUpdate = { pathname: projectUpdate.pathname, @@ -73,18 +66,17 @@ const ProjectHistoryRedisManager = { ) const jsonUpdate = JSON.stringify(projectUpdate) - ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate, callback) + return await ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate) }, - queueAddEntity( + async queueAddEntity( projectId, projectHistoryId, entityType, entityId, userId, projectUpdate, - source, - callback + source ) { projectUpdate = { pathname: projectUpdate.pathname, @@ -111,16 +103,10 @@ const ProjectHistoryRedisManager = { ) const jsonUpdate = JSON.stringify(projectUpdate) - ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate, callback) + return await ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate) }, - queueResyncProjectStructure( - projectId, - projectHistoryId, - docs, - files, - callback - ) { + async queueResyncProjectStructure(projectId, projectHistoryId, docs, files) { logger.debug({ projectId, docs, files }, 'queue project structure resync') const projectUpdate = { resyncProjectStructure: { docs, files }, @@ -130,17 +116,16 @@ const ProjectHistoryRedisManager = { }, } const jsonUpdate = JSON.stringify(projectUpdate) - ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate, callback) + return await ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate) }, - queueResyncDocContent( + async queueResyncDocContent( projectId, projectHistoryId, docId, lines, version, - pathname, - callback + pathname ) { logger.debug( { projectId, docId, lines, version, pathname }, @@ -163,15 +148,16 @@ const ProjectHistoryRedisManager = { // project update length as an upper bound const sizeBound = jsonUpdate.length if (docIsTooLarge(sizeBound, lines, Settings.max_doc_length)) { - const err = new Error( - 'blocking resync doc content insert into project history queue: doc is too large' + throw new OError( + 'blocking resync doc content insert into project history queue: doc is too large', + { projectId, docId, docSize: sizeBound } ) - logger.error({ projectId, docId, err, docSize: sizeBound }, err.message) - return callback(err) } - ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate, callback) + return await ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate) }, } -module.exports = ProjectHistoryRedisManager -module.exports.promises = promisifyAll(ProjectHistoryRedisManager) +module.exports = { + ...callbackifyAll(ProjectHistoryRedisManager), + promises: ProjectHistoryRedisManager, +} diff --git a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js index 0cb4fd9a79..dcb2dbe5a9 100644 --- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js +++ b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js @@ -1,4 +1,5 @@ const sinon = require('sinon') +const { expect } = require('chai') const modulePath = '../../../../app/js/ProjectHistoryRedisManager.js' const SandboxedModule = require('sandboxed-module') const tk = require('timekeeper') @@ -8,7 +9,6 @@ describe('ProjectHistoryRedisManager', function () { this.project_id = 'project-id-123' this.projectHistoryId = 'history-id-123' this.user_id = 'user-id-123' - this.callback = sinon.stub() this.rclient = {} this.source = 'editor' tk.freeze(new Date()) @@ -48,17 +48,15 @@ describe('ProjectHistoryRedisManager', function () { }) describe('queueOps', function () { - beforeEach(function () { + beforeEach(async function () { this.ops = ['mock-op-1', 'mock-op-2'] - this.multi = { exec: sinon.stub() } + this.multi = { exec: sinon.stub().resolves([1]) } this.multi.rpush = sinon.stub() this.multi.setnx = sinon.stub() this.rclient.multi = () => this.multi - // @rclient = multi: () => @multi - this.ProjectHistoryRedisManager.queueOps( + await this.ProjectHistoryRedisManager.promises.queueOps( this.project_id, - ...this.ops, - this.callback + ...this.ops ) }) @@ -83,7 +81,7 @@ describe('ProjectHistoryRedisManager', function () { }) describe('queueRenameEntity', function () { - beforeEach(function () { + beforeEach(async function () { this.file_id = 1234 this.rawUpdate = { @@ -92,16 +90,17 @@ describe('ProjectHistoryRedisManager', function () { version: (this.version = 2), } - this.ProjectHistoryRedisManager.queueOps = sinon.stub() - this.ProjectHistoryRedisManager.queueRenameEntity( + this.ProjectHistoryRedisManager.promises.queueOps = sinon + .stub() + .resolves() + await this.ProjectHistoryRedisManager.promises.queueRenameEntity( this.project_id, this.projectHistoryId, 'file', this.file_id, this.user_id, this.rawUpdate, - this.source, - this.callback + this.source ) }) @@ -119,19 +118,14 @@ describe('ProjectHistoryRedisManager', function () { file: this.file_id, } - this.ProjectHistoryRedisManager.queueOps - .calledWithExactly( - this.project_id, - JSON.stringify(update), - this.callback - ) + this.ProjectHistoryRedisManager.promises.queueOps + .calledWithExactly(this.project_id, JSON.stringify(update)) .should.equal(true) }) }) describe('queueAddEntity', function () { - beforeEach(function () { - this.rclient.rpush = sinon.stub().yields() + beforeEach(async function () { this.doc_id = 1234 this.rawUpdate = { @@ -141,16 +135,17 @@ describe('ProjectHistoryRedisManager', function () { url: (this.url = 'filestore.example.com'), } - this.ProjectHistoryRedisManager.queueOps = sinon.stub() - this.ProjectHistoryRedisManager.queueAddEntity( + this.ProjectHistoryRedisManager.promises.queueOps = sinon + .stub() + .resolves() + await this.ProjectHistoryRedisManager.promises.queueAddEntity( this.project_id, this.projectHistoryId, 'doc', this.doc_id, this.user_id, this.rawUpdate, - this.source, - this.callback + this.source ) }) @@ -169,12 +164,8 @@ describe('ProjectHistoryRedisManager', function () { doc: this.doc_id, } - this.ProjectHistoryRedisManager.queueOps - .calledWithExactly( - this.project_id, - JSON.stringify(update), - this.callback - ) + this.ProjectHistoryRedisManager.promises.queueOps + .calledWithExactly(this.project_id, JSON.stringify(update)) .should.equal(true) }) @@ -202,19 +193,20 @@ describe('ProjectHistoryRedisManager', function () { }, } - this.ProjectHistoryRedisManager.queueOps = sinon.stub() + this.ProjectHistoryRedisManager.promises.queueOps = sinon + .stub() + .resolves() }) describe('with a good doc', function () { - beforeEach(function () { - this.ProjectHistoryRedisManager.queueResyncDocContent( + beforeEach(async function () { + await this.ProjectHistoryRedisManager.promises.queueResyncDocContent( this.project_id, this.projectHistoryId, this.doc_id, this.lines, this.version, - this.pathname, - this.callback + this.pathname ) }) it('should check if the doc is too large', function () { @@ -228,34 +220,31 @@ describe('ProjectHistoryRedisManager', function () { }) it('should queue an update', function () { - this.ProjectHistoryRedisManager.queueOps - .calledWithExactly( - this.project_id, - JSON.stringify(this.update), - this.callback - ) + this.ProjectHistoryRedisManager.promises.queueOps + .calledWithExactly(this.project_id, JSON.stringify(this.update)) .should.equal(true) }) }) describe('with a doc that is too large', function () { - beforeEach(function () { + beforeEach(async function () { this.Limits.docIsTooLarge.returns(true) - this.ProjectHistoryRedisManager.queueResyncDocContent( - this.project_id, - this.projectHistoryId, - this.doc_id, - this.lines, - this.version, - this.pathname, - this.callback - ) + await expect( + this.ProjectHistoryRedisManager.promises.queueResyncDocContent( + this.project_id, + this.projectHistoryId, + this.doc_id, + this.lines, + this.version, + this.pathname + ) + ).to.be.rejected }) + it('should not queue an update if the doc is too large', function () { - this.ProjectHistoryRedisManager.queueOps.called.should.equal(false) - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) + this.ProjectHistoryRedisManager.promises.queueOps.called.should.equal( + false + ) }) }) })