From 3fa1245860afdb72fdf989ab47386a068388d59c Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 5 Sep 2023 08:41:49 -0400 Subject: [PATCH] Merge pull request #14567 from overleaf/em-history-ranges-flag Add historyRangesSupport flag to projects GitOrigin-RevId: 1e3f24a7c6f209bbd34eaaf4caee56dc7061b3da --- package-lock.json | 2 + .../app/js/DocumentManager.js | 65 ++++-------- .../app/js/PersistenceManager.js | 20 ++-- .../document-updater/app/js/RedisManager.js | 70 ++++++------- .../config/settings.defaults.js | 3 + services/document-updater/package.json | 1 + .../js/ApplyingUpdatesToADocTests.js | 2 +- services/document-updater/test/setup.js | 2 + .../DocumentManager/DocumentManagerTests.js | 39 +++----- .../PersistenceManagerTests.js | 19 ++-- .../unit/js/RedisManager/RedisManagerTests.js | 99 +++++++++++-------- .../Features/Documents/DocumentController.js | 6 ++ .../src/Documents/DocumentControllerTests.js | 1 + 13 files changed, 162 insertions(+), 167 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7cdb861fa4..24ddd621e0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -44410,6 +44410,7 @@ "mocha": "^10.2.0", "sandboxed-module": "^2.0.4", "sinon": "^9.2.4", + "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0" } }, @@ -53409,6 +53410,7 @@ "requestretry": "^7.1.0", "sandboxed-module": "^2.0.4", "sinon": "^9.2.4", + "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0" }, "dependencies": { diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 5c0798fdfd..41b336eb90 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -39,50 +39,27 @@ module.exports = DocumentManager = { { projectId, docId }, 'doc not in redis so getting from persistence API' ) - PersistenceManager.getDoc( - projectId, - docId, - (error, lines, version, ranges, pathname, projectHistoryId) => { + PersistenceManager.getDoc(projectId, docId, (error, doc) => { + if (error) { + return callback(error) + } + logger.debug({ doc }, 'got doc from persistence API') + RedisManager.putDocInMemory(projectId, docId, doc, error => { if (error) { return callback(error) } - logger.debug( - { - projectId, - docId, - lines, - version, - pathname, - projectHistoryId, - }, - 'got doc from persistence API' + callback( + null, + doc.lines, + doc.version, + doc.ranges || {}, + doc.pathname, + doc.projectHistoryId, + null, + false ) - RedisManager.putDocInMemory( - projectId, - docId, - lines, - version, - ranges, - pathname, - projectHistoryId, - error => { - if (error) { - return callback(error) - } - callback( - null, - lines, - version, - ranges || {}, - pathname, - projectHistoryId, - null, - false - ) - } - ) - } - ) + }) + }) } else { callback( null, @@ -517,7 +494,7 @@ module.exports = DocumentManager = { projectId, docId, { peek: true }, - (error, lines, version, ranges, pathname, projectHistoryId) => { + (error, doc) => { if (error) { logger.error( { projectId, docId, getDocError: error }, @@ -527,10 +504,10 @@ module.exports = DocumentManager = { } ProjectHistoryRedisManager.queueResyncDocContent( projectId, - projectHistoryId, + doc.projectHistoryId, docId, - lines, - version, + doc.lines, + doc.version, path, // use the path from the resyncProjectStructure update callback ) diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index b5abb2b44f..bd8fa3375b 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -93,16 +93,16 @@ function getDoc(projectId, docId, options = {}, _callback) { status: body.pathname === '' ? 'zero-length' : 'undefined', }) } - callback( - null, - body.lines, - body.version, - body.ranges, - body.pathname, - body.projectHistoryId?.toString() - ) + callback(null, { + lines: body.lines, + version: body.version, + ranges: body.ranges, + pathname: body.pathname, + projectHistoryId: body.projectHistoryId?.toString(), + historyRangesSupport: body.historyRangesSupport || false, + }) } else if (res.statusCode === 404) { - callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) + callback(new Errors.NotFoundError(`doc not found: ${urlPath}`)) } else if (res.statusCode === 413) { callback( new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) @@ -160,7 +160,7 @@ function setDoc( if (res.statusCode >= 200 && res.statusCode < 300) { callback(null, body) } else if (res.statusCode === 404) { - callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) + callback(new Errors.NotFoundError(`doc not found: ${urlPath}`)) } else if (res.statusCode === 413) { callback( new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index db157c7b70..604d19a593 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -30,47 +30,44 @@ const keys = Settings.redis.documentupdater.key_schema module.exports = RedisManager = { rclient, - putDocInMemory( - projectId, - docId, - docLines, - version, - ranges, - pathname, - projectHistoryId, - _callback - ) { + putDocInMemory(projectId, docId, doc, _callback) { const timer = new metrics.Timer('redis.put-doc') const callback = error => { timer.done() _callback(error) } - const docLinesArray = docLines - docLines = JSON.stringify(docLines) - if (docLines.indexOf('\u0000') !== -1) { + const docLinesJson = JSON.stringify(doc.lines) + if (docLinesJson.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) + logger.error({ err: error, docId, docLines: docLinesJson }, error.message) return callback(error) } // Do an optimised size check on the docLines using the serialised // length as an upper bound - const sizeBound = docLines.length - if (docIsTooLarge(sizeBound, docLinesArray, Settings.max_doc_length)) { - const docSize = docLines.length + const sizeBound = docLinesJson.length + if (docIsTooLarge(sizeBound, doc.lines, Settings.max_doc_length)) { + const docSize = docLinesJson.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) } - const docHash = RedisManager._computeHash(docLines) + const docHash = RedisManager._computeHash(docLinesJson) // record bytes sent to redis - metrics.summary('redis.docLines', docLines.length, { status: 'set' }) + metrics.summary('redis.docLines', docLinesJson.length, { status: 'set' }) logger.debug( - { projectId, docId, version, docHash, pathname, projectHistoryId }, + { + projectId, + docId, + version: doc.version, + docHash, + pathname: doc.pathname, + projectHistoryId: doc.projectHistoryId, + }, 'putting doc in redis' ) - RedisManager._serializeRanges(ranges, (error, ranges) => { + RedisManager._serializeRanges(doc.ranges, (error, ranges) => { if (error) { logger.error({ err: error, docId, projectId }, error.message) return callback(error) @@ -82,25 +79,27 @@ module.exports = RedisManager = { error => { if (error) return callback(error) - if (!pathname) { + if (!doc.pathname) { metrics.inc('pathname', 1, { path: 'RedisManager.setDoc', - status: pathname === '' ? 'zero-length' : 'undefined', + status: doc.pathname === '' ? 'zero-length' : 'undefined', }) } - rclient.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, - }, - callback - ) + const multi = rclient.multi() + multi.mset({ + [keys.docLines({ doc_id: docId })]: docLinesJson, + [keys.projectKey({ doc_id: docId })]: projectId, + [keys.docVersion({ doc_id: docId })]: doc.version, + [keys.docHash({ doc_id: docId })]: docHash, + [keys.ranges({ doc_id: docId })]: ranges, + [keys.pathname({ doc_id: docId })]: doc.pathname, + [keys.projectHistoryId({ doc_id: docId })]: doc.projectHistoryId, + }) + if (doc.historyRangesSupport) { + multi.sadd(keys.historyRangesSupport(), docId) + } + multi.exec(callback) } ) }) @@ -132,6 +131,7 @@ module.exports = RedisManager = { keys.lastUpdatedAt({ doc_id: docId }), keys.lastUpdatedBy({ doc_id: docId }) ) + multi.srem(keys.historyRangesSupport(), docId) multi.exec((error, response) => { if (error) { return callback(error) diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index 0bcd90ab28..1a2e2b5745 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -140,6 +140,9 @@ module.exports = { flushAndDeleteQueue() { return 'DocUpdaterFlushAndDeleteQueue' }, + historyRangesSupport() { + return 'HistoryRangesSupport' + }, }, }, }, diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 05ac7428bf..22175fecdc 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -39,6 +39,7 @@ "mocha": "^10.2.0", "sandboxed-module": "^2.0.4", "sinon": "^9.2.4", + "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0" } } diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js index ccceb67444..3e50284a3a 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js @@ -679,7 +679,7 @@ describe('Applying updates to a doc', function () { 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}`, + error: `doc not found: /project/${this.project_id}/doc/${this.doc_id}`, }) }) }) diff --git a/services/document-updater/test/setup.js b/services/document-updater/test/setup.js index 9b9b4c029e..b213e1da51 100644 --- a/services/document-updater/test/setup.js +++ b/services/document-updater/test/setup.js @@ -1,9 +1,11 @@ const chai = require('chai') const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const sinonChai = require('sinon-chai') // Chai configuration chai.should() +chai.use(sinonChai) // Global stubs const sandbox = sinon.createSandbox() diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index f7aa85499b..5d1b9a6a57 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -42,6 +42,14 @@ describe('DocumentManager', function () { this.lastUpdatedAt = Date.now() this.lastUpdatedBy = 'last-author-id' this.source = 'external-source' + this.doc = { + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + historyRangesSupport: false, + } }) afterEach(function () { @@ -373,15 +381,7 @@ describe('DocumentManager', function () { .callsArgWith(2, null, null, null, null, null, null) this.PersistenceManager.getDoc = sinon .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) + .callsArgWith(2, null, this.doc) this.RedisManager.putDocInMemory = sinon.stub().yields() this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) }) @@ -400,15 +400,7 @@ describe('DocumentManager', function () { it('should set the doc in Redis', function () { this.RedisManager.putDocInMemory - .calledWith( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) + .calledWith(this.project_id, this.doc_id, this.doc) .should.equal(true) }) @@ -1109,16 +1101,7 @@ describe('DocumentManager', function () { beforeEach(function () { this.pathnameFromProjectStructureUpdate = '/foo/bar.tex' this.RedisManager.getDoc = sinon.stub().callsArgWith(2, null) - this.PersistenceManager.getDoc = sinon - .stub() - .yields( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) + this.PersistenceManager.getDoc = sinon.stub().yields(null, this.doc) this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub() this.DocumentManager.resyncDocContents( this.project_id, diff --git a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js index 2d0c622fec..3260e7deb4 100644 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js @@ -32,6 +32,14 @@ describe('PersistenceManager', function () { this.pathname = '/a/b/c.tex' this.lastUpdatedAt = Date.now() this.lastUpdatedBy = 'last-author-id' + this.doc = { + lines: this.lines, + version: this.version, + ranges: this.ranges, + projectHistoryId: this.projectHistoryId, + pathname: this.pathname, + historyRangesSupport: false, + } this.Settings.apis = { web: { url: (this.url = 'www.example.com'), @@ -87,16 +95,7 @@ describe('PersistenceManager', function () { }) it('should call the callback with the doc lines, version and ranges', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - .should.equal(true) + this.callback.should.have.been.calledWith(null, this.doc) }) it('should time the execution', function () { diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 75e5c7d7f8..df07ceebb4 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -7,7 +7,7 @@ const tk = require('timekeeper') describe('RedisManager', function () { beforeEach(function () { - this.multi = { exec: sinon.stub() } + this.multi = { exec: sinon.stub().yields() } this.rclient = { multi: () => this.multi } tk.freeze(new Date()) this.RedisManager = SandboxedModule.require(modulePath, { @@ -63,6 +63,9 @@ describe('RedisManager', function () { lastUpdatedAt({ doc_id: docId }) { return `lastUpdatedAt:${docId}` }, + historyRangesSupport() { + return 'HistoryRangesSupport' + }, }, }, }, @@ -759,16 +762,20 @@ describe('RedisManager', function () { describe('putDocInMemory', function () { beforeEach(function () { - this.rclient.mset = sinon.stub().yields(null) + this.multi.mset = sinon.stub() + this.multi.sadd = sinon.stub() this.rclient.sadd = sinon.stub().yields() - this.lines = ['one', 'two', 'three', 'これは'] - this.version = 42 + this.doc = { + lines: ['one', 'two', 'three', 'これは'], + version: 42, + ranges: { comments: 'mock', entries: 'mock' }, + pathname: '/a/b/c.tex', + projectHistoryId: this.projectHistoryId, + } this.hash = crypto .createHash('sha1') - .update(JSON.stringify(this.lines), 'utf8') + .update(JSON.stringify(this.doc.lines), 'utf8') .digest('hex') - this.ranges = { comments: 'mock', entries: 'mock' } - this.pathname = '/a/b/c.tex' }) describe('with non-empty ranges', function () { @@ -776,25 +783,21 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, + this.doc, done ) }) it('should set all the details in a single MSET call', function () { - this.rclient.mset + this.multi.mset .calledWith({ - [`doclines:${this.docId}`]: JSON.stringify(this.lines), + [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines), [`ProjectId:${this.docId}`]: this.project_id, - [`DocVersion:${this.docId}`]: this.version, + [`DocVersion:${this.docId}`]: this.doc.version, [`DocHash:${this.docId}`]: this.hash, - [`Ranges:${this.docId}`]: JSON.stringify(this.ranges), - [`Pathname:${this.docId}`]: this.pathname, - [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, + [`Ranges:${this.docId}`]: JSON.stringify(this.doc.ranges), + [`Pathname:${this.docId}`]: this.doc.pathname, + [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId, }) .should.equal(true) }) @@ -808,32 +811,33 @@ describe('RedisManager', function () { it('should not log any errors', function () { this.logger.error.calledWith().should.equal(false) }) + + it('should not add the document to the HistoryRangesSupport set in Redis', function () { + this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport') + }) }) describe('with empty ranges', function () { beforeEach(function (done) { + this.doc.ranges = {} this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.lines, - this.version, - {}, - this.pathname, - this.projectHistoryId, + this.doc, done ) }) it('should unset ranges', function () { - this.rclient.mset + this.multi.mset .calledWith({ - [`doclines:${this.docId}`]: JSON.stringify(this.lines), + [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines), [`ProjectId:${this.docId}`]: this.project_id, - [`DocVersion:${this.docId}`]: this.version, + [`DocVersion:${this.docId}`]: this.doc.version, [`DocHash:${this.docId}`]: this.hash, [`Ranges:${this.docId}`]: null, - [`Pathname:${this.docId}`]: this.pathname, - [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, + [`Pathname:${this.docId}`]: this.doc.pathname, + [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId, }) .should.equal(true) }) @@ -847,11 +851,7 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, + this.doc, this.callback ) }) @@ -879,11 +879,7 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, + this.doc, this.callback ) }) @@ -898,6 +894,25 @@ describe('RedisManager', function () { .should.equal(true) }) }) + + describe('with history ranges support', function () { + beforeEach(function () { + this.doc.historyRangesSupport = true + this.RedisManager.putDocInMemory( + this.project_id, + this.docId, + this.doc, + this.callback + ) + }) + + it('should add the document to the HistoryRangesSupport set in Redis', function () { + this.multi.sadd.should.have.been.calledWith( + 'HistoryRangesSupport', + this.docId + ) + }) + }) }) describe('removeDocFromMemory', function () { @@ -905,7 +920,6 @@ describe('RedisManager', 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) }) @@ -935,6 +949,13 @@ describe('RedisManager', function () { .calledWith(`DocsIn:${this.project_id}`, this.docId) .should.equal(true) }) + + it('should remove the docId from the HistoryRangesSupport set', function () { + this.multi.srem.should.have.been.calledWith( + 'HistoryRangesSupport', + this.docId + ) + }) }) describe('clearProjectState', function () { diff --git a/services/web/app/src/Features/Documents/DocumentController.js b/services/web/app/src/Features/Documents/DocumentController.js index a5652f5091..b2001135f4 100644 --- a/services/web/app/src/Features/Documents/DocumentController.js +++ b/services/web/app/src/Features/Documents/DocumentController.js @@ -35,6 +35,11 @@ async function getDocument(req, res) { plainTextResponse(res, lines.join('\n')) } else { const projectHistoryId = _.get(project, 'overleaf.history.id') + const historyRangesSupport = _.get( + project, + 'overleaf.history.rangesSupportEnabled', + false + ) // all projects are now migrated to Full Project History, keeping the field // for API compatibility @@ -47,6 +52,7 @@ async function getDocument(req, res) { pathname: path.fileSystem, projectHistoryId, projectHistoryType, + historyRangesSupport, }) } } diff --git a/services/web/test/unit/src/Documents/DocumentControllerTests.js b/services/web/test/unit/src/Documents/DocumentControllerTests.js index dbcfbb91c0..b3c3f14fa1 100644 --- a/services/web/test/unit/src/Documents/DocumentControllerTests.js +++ b/services/web/test/unit/src/Documents/DocumentControllerTests.js @@ -95,6 +95,7 @@ describe('DocumentController', function () { pathname: this.pathname, projectHistoryId: this.project.overleaf.history.id, projectHistoryType: 'project-history', + historyRangesSupport: false, }) ) })