From 886dbf504c287d0b87247bdafba63a0543a44327 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 7 Feb 2024 11:35:41 -0500 Subject: [PATCH] Merge pull request #16976 from overleaf/revert-16877-em-history-ranges-support Revert "Store in Redis whether docs support ranges in the history" GitOrigin-RevId: c682cb49814c59aaec068106ae39c76f0de95493 --- .../app/js/DocumentManager.js | 25 +-- .../app/js/PersistenceManager.js | 4 +- .../document-updater/app/js/RedisManager.js | 151 ++++++++---------- .../config/settings.defaults.js | 3 - .../DocumentManager/DocumentManagerTests.js | 18 +-- .../PersistenceManagerTests.js | 5 +- .../unit/js/RedisManager/RedisManagerTests.js | 123 ++++---------- 7 files changed, 107 insertions(+), 222 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 8a27c18c69..e7fa040914 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -29,10 +29,7 @@ const DocumentManager = { ranges, pathname, projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy, - historyRangesSupport + unflushedTime ) => { if (error) { return callback(error) @@ -45,15 +42,7 @@ const DocumentManager = { PersistenceManager.getDoc( projectId, docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - historyRangesSupport - ) => { + (error, lines, version, ranges, pathname, projectHistoryId) => { if (error) { return callback(error) } @@ -65,7 +54,6 @@ const DocumentManager = { version, pathname, projectHistoryId, - historyRangesSupport, }, 'got doc from persistence API' ) @@ -77,7 +65,6 @@ const DocumentManager = { ranges, pathname, projectHistoryId, - historyRangesSupport, error => { if (error) { return callback(error) @@ -90,8 +77,7 @@ const DocumentManager = { pathname, projectHistoryId, null, - false, - historyRangesSupport + false ) } ) @@ -106,8 +92,7 @@ const DocumentManager = { pathname, projectHistoryId, unflushedTime, - true, - historyRangesSupport + true ) } } @@ -713,7 +698,6 @@ module.exports.promises = promisifyAll(DocumentManager, { 'projectHistoryId', 'unflushedTime', 'alreadyLoaded', - 'historyRangesSupport', ], getDocWithLock: [ 'lines', @@ -723,7 +707,6 @@ module.exports.promises = promisifyAll(DocumentManager, { 'projectHistoryId', 'unflushedTime', 'alreadyLoaded', - 'historyRangesSupport', ], getDocAndFlushIfOld: ['lines', 'version'], getDocAndFlushIfOldWithLock: ['lines', 'version'], diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index 74a97baede..34e2ff89b9 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -101,8 +101,7 @@ function getDoc(projectId, docId, options = {}, _callback) { body.version, body.ranges, body.pathname, - body.projectHistoryId?.toString(), - body.historyRangesSupport || false + body.projectHistoryId?.toString() ) } else if (res.statusCode === 404) { callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) @@ -187,7 +186,6 @@ module.exports = { 'ranges', 'pathname', 'projectHistoryId', - 'historyRangesSupport', ]), setDoc: promisify(setDoc), }, diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index b8eb05aeb1..d69e11e103 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -38,7 +38,6 @@ const RedisManager = { ranges, pathname, projectHistoryId, - historyRangesSupport, _callback ) { const timer = new metrics.Timer('redis.put-doc') @@ -90,20 +89,18 @@ const RedisManager = { }) } - 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.sadd(keys.historyRangesSupport(), docId) - } - multi.exec(callback) + 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 + ) } ) }) @@ -135,7 +132,6 @@ const 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) @@ -202,76 +198,68 @@ const RedisManager = { lastUpdatedAt, lastUpdatedBy, ] = result - rclient.sismember(keys.historyRangesSupport(), docId, (error, result) => { - if (error) { - return callback(error) + 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' + ) } - 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) + } - 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')) + } - 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', + }) + } - 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 - ) - }) + callback( + null, + docLines, + version, + ranges, + pathname, + projectHistoryId, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy + ) }) }, @@ -643,7 +631,6 @@ module.exports.promises = promisifyAll(RedisManager, { 'unflushedTime', 'lastUpdatedAt', 'lastUpdatedBy', - 'historyRangesSupport', ], getNextProjectToFlushAndDelete: [ 'projectId', diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index 3d6282eb34..5cdac22640 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -140,9 +140,6 @@ module.exports = { flushAndDeleteQueue() { return 'DocUpdaterFlushAndDeleteQueue' }, - historyRangesSupport() { - return 'HistoryRangesSupport' - }, }, }, }, diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 2e0e071ac9..78cc98f4d3 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -42,7 +42,6 @@ describe('DocumentManager', function () { this.lastUpdatedAt = Date.now() this.lastUpdatedBy = 'last-author-id' this.source = 'external-source' - this.historyRangesSupport = false }) afterEach(function () { @@ -336,10 +335,7 @@ describe('DocumentManager', function () { this.ranges, this.pathname, this.projectHistoryId, - this.unflushedTime, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.historyRangesSupport + this.unflushedTime ) this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) }) @@ -360,8 +356,7 @@ describe('DocumentManager', function () { this.pathname, this.projectHistoryId, this.unflushedTime, - true, - this.historyRangesSupport + true ) .should.equal(true) }) @@ -385,8 +380,7 @@ describe('DocumentManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId, - this.historyRangesSupport + this.projectHistoryId ) this.RedisManager.putDocInMemory = sinon.stub().yields() this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) @@ -413,8 +407,7 @@ describe('DocumentManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId, - this.historyRangesSupport + this.projectHistoryId ) .should.equal(true) }) @@ -429,8 +422,7 @@ describe('DocumentManager', function () { this.pathname, this.projectHistoryId, null, - false, - this.historyRangesSupport + false ) .should.equal(true) }) diff --git a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js index 8c82677474..3fd2065cd4 100644 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js @@ -32,7 +32,6 @@ describe('PersistenceManager', function () { this.pathname = '/a/b/c.tex' this.lastUpdatedAt = Date.now() this.lastUpdatedBy = 'last-author-id' - this.historyRangesSupport = false this.Settings.apis = { web: { url: (this.url = 'www.example.com'), @@ -50,7 +49,6 @@ describe('PersistenceManager', function () { ranges: this.ranges, pathname: this.pathname, projectHistoryId: this.projectHistoryId, - historyRangesSupport: this.historyRangesSupport, } }) @@ -96,8 +94,7 @@ describe('PersistenceManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId, - this.historyRangesSupport + this.projectHistoryId ) .should.equal(true) }) diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index b68333d678..a31987d2a1 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().yields() } + this.multi = { exec: sinon.stub() } this.rclient = { multi: () => this.multi } tk.freeze(new Date()) this.RedisManager = SandboxedModule.require(modulePath, { @@ -63,9 +63,6 @@ describe('RedisManager', function () { lastUpdatedAt({ doc_id: docId }) { return `lastUpdatedAt:${docId}` }, - historyRangesSupport() { - return 'HistoryRangesSupport' - }, }, }, }, @@ -94,7 +91,6 @@ describe('RedisManager', function () { this.docId = 'doc-id-123' this.project_id = 'project-id-123' this.projectHistoryId = '123' - this.historyRangesSupport = false this.callback = sinon.stub() }) @@ -127,10 +123,6 @@ describe('RedisManager', function () { this.projectHistoryId.toString(), this.unflushed_time, ]) - this.rclient.sismember = sinon.stub() - this.rclient.sismember - .withArgs('HistoryRangesSupport', this.docId) - .yields(null, 0) }) describe('successfully', function () { @@ -156,18 +148,19 @@ 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.callback + .calledWithExactly( + null, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId, + this.unflushed_time, + this.lastUpdatedAt, + this.lastUpdatedBy + ) + .should.equal(true) }) it('should not log any errors', function () { @@ -254,30 +247,6 @@ describe('RedisManager', function () { .should.equal(true) }) }) - - describe('with history ranges support', function () { - beforeEach(function () { - this.rclient.sismember - .withArgs('HistoryRangesSupport', this.docId) - .yields(null, 1) - this.RedisManager.getDoc(this.project_id, this.docId, this.callback) - }) - - 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 - ) - }) - }) }) describe('getPreviousDocOpsTests', function () { @@ -796,8 +765,7 @@ describe('RedisManager', function () { describe('putDocInMemory', function () { beforeEach(function () { - this.multi.mset = sinon.stub() - this.multi.sadd = sinon.stub() + this.rclient.mset = sinon.stub().yields(null) this.rclient.sadd = sinon.stub().yields() this.lines = ['one', 'two', 'three', 'これは'] this.version = 42 @@ -819,13 +787,12 @@ describe('RedisManager', function () { this.ranges, this.pathname, this.projectHistoryId, - this.historyRangesSupport, done ) }) it('should set all the details in a single MSET call', function () { - this.multi.mset + this.rclient.mset .calledWith({ [`doclines:${this.docId}`]: JSON.stringify(this.lines), [`ProjectId:${this.docId}`]: this.project_id, @@ -847,10 +814,6 @@ 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 () { @@ -863,21 +826,22 @@ describe('RedisManager', function () { {}, this.pathname, this.projectHistoryId, - this.historyRangesSupport, done ) }) it('should unset ranges', function () { - this.multi.mset.should.have.been.calledWith({ - [`doclines:${this.docId}`]: JSON.stringify(this.lines), - [`ProjectId:${this.docId}`]: this.project_id, - [`DocVersion:${this.docId}`]: this.version, - [`DocHash:${this.docId}`]: this.hash, - [`Ranges:${this.docId}`]: null, - [`Pathname:${this.docId}`]: this.pathname, - [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, - }) + this.rclient.mset + .calledWith({ + [`doclines:${this.docId}`]: JSON.stringify(this.lines), + [`ProjectId:${this.docId}`]: this.project_id, + [`DocVersion:${this.docId}`]: this.version, + [`DocHash:${this.docId}`]: this.hash, + [`Ranges:${this.docId}`]: null, + [`Pathname:${this.docId}`]: this.pathname, + [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, + }) + .should.equal(true) }) }) @@ -894,7 +858,6 @@ describe('RedisManager', function () { this.ranges, this.pathname, this.projectHistoryId, - this.historyRangesSupport, this.callback ) }) @@ -927,7 +890,6 @@ describe('RedisManager', function () { this.ranges, this.pathname, this.projectHistoryId, - this.historyRangesSupport, this.callback ) }) @@ -942,30 +904,6 @@ describe('RedisManager', function () { .should.equal(true) }) }) - - describe('with history ranges support', function () { - beforeEach(function (done) { - this.historyRangesSupport = true - this.RedisManager.putDocInMemory( - this.project_id, - this.docId, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport, - done - ) - }) - - 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 () { @@ -1003,13 +941,6 @@ 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 () {