diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index e7fa040914..8a27c18c69 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -29,7 +29,10 @@ const DocumentManager = { ranges, pathname, projectHistoryId, - unflushedTime + unflushedTime, + lastUpdatedAt, + lastUpdatedBy, + historyRangesSupport ) => { if (error) { return callback(error) @@ -42,7 +45,15 @@ const DocumentManager = { PersistenceManager.getDoc( projectId, docId, - (error, lines, version, ranges, pathname, projectHistoryId) => { + ( + error, + lines, + version, + ranges, + pathname, + projectHistoryId, + historyRangesSupport + ) => { if (error) { return callback(error) } @@ -54,6 +65,7 @@ const DocumentManager = { version, pathname, projectHistoryId, + historyRangesSupport, }, 'got doc from persistence API' ) @@ -65,6 +77,7 @@ const DocumentManager = { ranges, pathname, projectHistoryId, + historyRangesSupport, error => { if (error) { return callback(error) @@ -77,7 +90,8 @@ const DocumentManager = { pathname, projectHistoryId, null, - false + false, + historyRangesSupport ) } ) @@ -92,7 +106,8 @@ const DocumentManager = { pathname, projectHistoryId, unflushedTime, - true + true, + historyRangesSupport ) } } @@ -698,6 +713,7 @@ module.exports.promises = promisifyAll(DocumentManager, { 'projectHistoryId', 'unflushedTime', 'alreadyLoaded', + 'historyRangesSupport', ], getDocWithLock: [ 'lines', @@ -707,6 +723,7 @@ 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 34e2ff89b9..74a97baede 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -101,7 +101,8 @@ function getDoc(projectId, docId, options = {}, _callback) { body.version, body.ranges, body.pathname, - body.projectHistoryId?.toString() + body.projectHistoryId?.toString(), + body.historyRangesSupport || false ) } else if (res.statusCode === 404) { callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) @@ -186,6 +187,7 @@ 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 d69e11e103..b8eb05aeb1 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -38,6 +38,7 @@ const RedisManager = { ranges, pathname, projectHistoryId, + historyRangesSupport, _callback ) { const timer = new metrics.Timer('redis.put-doc') @@ -89,18 +90,20 @@ const RedisManager = { }) } - 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 })]: 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) } ) }) @@ -132,6 +135,7 @@ 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) @@ -198,68 +202,76 @@ const RedisManager = { lastUpdatedAt, lastUpdatedBy, ] = result - 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' - ) + rclient.sismember(keys.historyRangesSupport(), docId, (error, result) => { + if (error) { + return callback(error) } - } + const historyRangesSupport = result === 1 - try { - docLines = JSON.parse(docLines) - ranges = RedisManager._deserializeRanges(ranges) - } catch (e) { - return callback(e) - } + 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' + ) + } + } - 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')) - } + try { + docLines = JSON.parse(docLines) + ranges = RedisManager._deserializeRanges(ranges) + } catch (e) { + return callback(e) + } - if (docLines && version && !pathname) { - metrics.inc('pathname', 1, { - path: 'RedisManager.getDoc', - status: pathname === '' ? 'zero-length' : 'undefined', - }) - } + 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')) + } - callback( - null, - docLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy - ) + 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 + ) + }) }) }, @@ -631,6 +643,7 @@ 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 5cdac22640..3d6282eb34 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/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 78cc98f4d3..2e0e071ac9 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -42,6 +42,7 @@ describe('DocumentManager', function () { this.lastUpdatedAt = Date.now() this.lastUpdatedBy = 'last-author-id' this.source = 'external-source' + this.historyRangesSupport = false }) afterEach(function () { @@ -335,7 +336,10 @@ describe('DocumentManager', function () { this.ranges, this.pathname, this.projectHistoryId, - this.unflushedTime + this.unflushedTime, + this.lastUpdatedAt, + this.lastUpdatedBy, + this.historyRangesSupport ) this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) }) @@ -356,7 +360,8 @@ describe('DocumentManager', function () { this.pathname, this.projectHistoryId, this.unflushedTime, - true + true, + this.historyRangesSupport ) .should.equal(true) }) @@ -380,7 +385,8 @@ describe('DocumentManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId + this.projectHistoryId, + this.historyRangesSupport ) this.RedisManager.putDocInMemory = sinon.stub().yields() this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) @@ -407,7 +413,8 @@ describe('DocumentManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId + this.projectHistoryId, + this.historyRangesSupport ) .should.equal(true) }) @@ -422,7 +429,8 @@ describe('DocumentManager', function () { this.pathname, this.projectHistoryId, null, - false + false, + this.historyRangesSupport ) .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 3fd2065cd4..8c82677474 100644 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js @@ -32,6 +32,7 @@ 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'), @@ -49,6 +50,7 @@ describe('PersistenceManager', function () { ranges: this.ranges, pathname: this.pathname, projectHistoryId: this.projectHistoryId, + historyRangesSupport: this.historyRangesSupport, } }) @@ -94,7 +96,8 @@ describe('PersistenceManager', function () { this.version, this.ranges, this.pathname, - this.projectHistoryId + this.projectHistoryId, + this.historyRangesSupport ) .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 a31987d2a1..b68333d678 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' + }, }, }, }, @@ -91,6 +94,7 @@ describe('RedisManager', function () { this.docId = 'doc-id-123' this.project_id = 'project-id-123' this.projectHistoryId = '123' + this.historyRangesSupport = false this.callback = sinon.stub() }) @@ -123,6 +127,10 @@ 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 () { @@ -148,19 +156,18 @@ describe('RedisManager', function () { }) it('should return the document', function () { - this.callback - .calledWithExactly( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushed_time, - this.lastUpdatedAt, - this.lastUpdatedBy - ) - .should.equal(true) + 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 + ) }) it('should not log any errors', function () { @@ -247,6 +254,30 @@ 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 () { @@ -765,7 +796,8 @@ 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 @@ -787,12 +819,13 @@ 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.rclient.mset + this.multi.mset .calledWith({ [`doclines:${this.docId}`]: JSON.stringify(this.lines), [`ProjectId:${this.docId}`]: this.project_id, @@ -814,6 +847,10 @@ 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 () { @@ -826,22 +863,21 @@ describe('RedisManager', function () { {}, this.pathname, this.projectHistoryId, + this.historyRangesSupport, done ) }) it('should unset ranges', function () { - 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) + 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, + }) }) }) @@ -858,6 +894,7 @@ describe('RedisManager', function () { this.ranges, this.pathname, this.projectHistoryId, + this.historyRangesSupport, this.callback ) }) @@ -890,6 +927,7 @@ describe('RedisManager', function () { this.ranges, this.pathname, this.projectHistoryId, + this.historyRangesSupport, this.callback ) }) @@ -904,6 +942,30 @@ 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 () { @@ -941,6 +1003,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 () {