diff --git a/package-lock.json b/package-lock.json index 24ddd621e0..7cdb861fa4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -44410,7 +44410,6 @@ "mocha": "^10.2.0", "sandboxed-module": "^2.0.4", "sinon": "^9.2.4", - "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0" } }, @@ -53410,7 +53409,6 @@ "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 41b336eb90..5c0798fdfd 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -39,27 +39,50 @@ module.exports = DocumentManager = { { projectId, docId }, 'doc not in redis so getting from persistence API' ) - 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 => { + PersistenceManager.getDoc( + projectId, + docId, + (error, lines, version, ranges, pathname, projectHistoryId) => { if (error) { return callback(error) } - callback( - null, - doc.lines, - doc.version, - doc.ranges || {}, - doc.pathname, - doc.projectHistoryId, - null, - false + logger.debug( + { + projectId, + docId, + lines, + version, + pathname, + projectHistoryId, + }, + 'got doc from persistence API' ) - }) - }) + 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, @@ -494,7 +517,7 @@ module.exports = DocumentManager = { projectId, docId, { peek: true }, - (error, doc) => { + (error, lines, version, ranges, pathname, projectHistoryId) => { if (error) { logger.error( { projectId, docId, getDocError: error }, @@ -504,10 +527,10 @@ module.exports = DocumentManager = { } ProjectHistoryRedisManager.queueResyncDocContent( projectId, - doc.projectHistoryId, + projectHistoryId, docId, - doc.lines, - doc.version, + lines, + 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 bd8fa3375b..b5abb2b44f 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, { - lines: body.lines, - version: body.version, - ranges: body.ranges, - pathname: body.pathname, - projectHistoryId: body.projectHistoryId?.toString(), - historyRangesSupport: body.historyRangesSupport || false, - }) + callback( + null, + body.lines, + body.version, + body.ranges, + body.pathname, + body.projectHistoryId?.toString() + ) } else if (res.statusCode === 404) { - callback(new Errors.NotFoundError(`doc not found: ${urlPath}`)) + callback(new Errors.NotFoundError(`doc not 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 found: ${urlPath}`)) + callback(new Errors.NotFoundError(`doc not 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 604d19a593..db157c7b70 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -30,44 +30,47 @@ const keys = Settings.redis.documentupdater.key_schema module.exports = RedisManager = { rclient, - putDocInMemory(projectId, docId, doc, _callback) { + putDocInMemory( + projectId, + docId, + docLines, + version, + ranges, + pathname, + projectHistoryId, + _callback + ) { const timer = new metrics.Timer('redis.put-doc') const callback = error => { timer.done() _callback(error) } - const docLinesJson = JSON.stringify(doc.lines) - if (docLinesJson.indexOf('\u0000') !== -1) { + 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: docLinesJson }, error.message) + logger.error({ err: error, docId, docLines }, error.message) return callback(error) } // Do an optimised size check on the docLines using the serialised // length as an upper bound - const sizeBound = docLinesJson.length - if (docIsTooLarge(sizeBound, doc.lines, Settings.max_doc_length)) { - const docSize = docLinesJson.length + const sizeBound = docLines.length + if (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) } - const docHash = RedisManager._computeHash(docLinesJson) + const docHash = RedisManager._computeHash(docLines) // record bytes sent to redis - metrics.summary('redis.docLines', docLinesJson.length, { status: 'set' }) + metrics.summary('redis.docLines', docLines.length, { status: 'set' }) logger.debug( - { - projectId, - docId, - version: doc.version, - docHash, - pathname: doc.pathname, - projectHistoryId: doc.projectHistoryId, - }, + { projectId, docId, version, docHash, pathname, projectHistoryId }, 'putting doc in redis' ) - RedisManager._serializeRanges(doc.ranges, (error, ranges) => { + RedisManager._serializeRanges(ranges, (error, ranges) => { if (error) { logger.error({ err: error, docId, projectId }, error.message) return callback(error) @@ -79,27 +82,25 @@ module.exports = RedisManager = { error => { if (error) return callback(error) - if (!doc.pathname) { + if (!pathname) { metrics.inc('pathname', 1, { path: 'RedisManager.setDoc', - status: doc.pathname === '' ? 'zero-length' : 'undefined', + status: pathname === '' ? 'zero-length' : 'undefined', }) } - 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) + 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 + ) } ) }) @@ -131,7 +132,6 @@ 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 1a2e2b5745..0bcd90ab28 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/package.json b/services/document-updater/package.json index 22175fecdc..05ac7428bf 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -39,7 +39,6 @@ "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 3e50284a3a..ccceb67444 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 found: /project/${this.project_id}/doc/${this.doc_id}`, + error: `doc not 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 b213e1da51..9b9b4c029e 100644 --- a/services/document-updater/test/setup.js +++ b/services/document-updater/test/setup.js @@ -1,11 +1,9 @@ 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 5d1b9a6a57..f7aa85499b 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -42,14 +42,6 @@ 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 () { @@ -381,7 +373,15 @@ describe('DocumentManager', function () { .callsArgWith(2, null, null, null, null, null, null) this.PersistenceManager.getDoc = sinon .stub() - .callsArgWith(2, null, this.doc) + .callsArgWith( + 2, + null, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId + ) this.RedisManager.putDocInMemory = sinon.stub().yields() this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) }) @@ -400,7 +400,15 @@ describe('DocumentManager', function () { it('should set the doc in Redis', function () { this.RedisManager.putDocInMemory - .calledWith(this.project_id, this.doc_id, this.doc) + .calledWith( + this.project_id, + this.doc_id, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId + ) .should.equal(true) }) @@ -1101,7 +1109,16 @@ 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.doc) + this.PersistenceManager.getDoc = sinon + .stub() + .yields( + null, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId + ) 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 3260e7deb4..2d0c622fec 100644 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js @@ -32,14 +32,6 @@ 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'), @@ -95,7 +87,16 @@ describe('PersistenceManager', function () { }) it('should call the callback with the doc lines, version and ranges', function () { - this.callback.should.have.been.calledWith(null, this.doc) + this.callback + .calledWith( + null, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId + ) + .should.equal(true) }) 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 df07ceebb4..75e5c7d7f8 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' - }, }, }, }, @@ -762,20 +759,16 @@ 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.doc = { - lines: ['one', 'two', 'three', 'これは'], - version: 42, - ranges: { comments: 'mock', entries: 'mock' }, - pathname: '/a/b/c.tex', - projectHistoryId: this.projectHistoryId, - } + this.lines = ['one', 'two', 'three', 'これは'] + this.version = 42 this.hash = crypto .createHash('sha1') - .update(JSON.stringify(this.doc.lines), 'utf8') + .update(JSON.stringify(this.lines), 'utf8') .digest('hex') + this.ranges = { comments: 'mock', entries: 'mock' } + this.pathname = '/a/b/c.tex' }) describe('with non-empty ranges', function () { @@ -783,21 +776,25 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.doc, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId, 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.doc.lines), + [`doclines:${this.docId}`]: JSON.stringify(this.lines), [`ProjectId:${this.docId}`]: this.project_id, - [`DocVersion:${this.docId}`]: this.doc.version, + [`DocVersion:${this.docId}`]: this.version, [`DocHash:${this.docId}`]: this.hash, - [`Ranges:${this.docId}`]: JSON.stringify(this.doc.ranges), - [`Pathname:${this.docId}`]: this.doc.pathname, - [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId, + [`Ranges:${this.docId}`]: JSON.stringify(this.ranges), + [`Pathname:${this.docId}`]: this.pathname, + [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, }) .should.equal(true) }) @@ -811,33 +808,32 @@ 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.doc, + this.lines, + this.version, + {}, + this.pathname, + this.projectHistoryId, done ) }) it('should unset ranges', function () { - this.multi.mset + this.rclient.mset .calledWith({ - [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines), + [`doclines:${this.docId}`]: JSON.stringify(this.lines), [`ProjectId:${this.docId}`]: this.project_id, - [`DocVersion:${this.docId}`]: this.doc.version, + [`DocVersion:${this.docId}`]: this.version, [`DocHash:${this.docId}`]: this.hash, [`Ranges:${this.docId}`]: null, - [`Pathname:${this.docId}`]: this.doc.pathname, - [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId, + [`Pathname:${this.docId}`]: this.pathname, + [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, }) .should.equal(true) }) @@ -851,7 +847,11 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.doc, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId, this.callback ) }) @@ -879,7 +879,11 @@ describe('RedisManager', function () { this.RedisManager.putDocInMemory( this.project_id, this.docId, - this.doc, + this.lines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId, this.callback ) }) @@ -894,25 +898,6 @@ 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 () { @@ -920,6 +905,7 @@ 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) }) @@ -949,13 +935,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 () {