diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 4c0d144529..73d85f60d5 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -88,19 +88,18 @@ module.exports = RedisManager = { rclient.sadd(keys.docsInProject({ project_id }), doc_id, (error) => { if (error) return callback(error) - const multi = rclient.multi() - multi.set(keys.docLines({ doc_id }), docLines) - multi.set(keys.projectKey({ doc_id }), project_id) - multi.set(keys.docVersion({ doc_id }), version) - multi.set(keys.docHash({ doc_id }), docHash) - if (ranges != null) { - multi.set(keys.ranges({ doc_id }), ranges) - } else { - multi.del(keys.ranges({ doc_id })) - } - multi.set(keys.pathname({ doc_id }), pathname) - multi.set(keys.projectHistoryId({ doc_id }), projectHistoryId) - multi.exec(callback) + rclient.mset( + { + [keys.docLines({ doc_id })]: docLines, + [keys.projectKey({ doc_id })]: project_id, + [keys.docVersion({ doc_id })]: version, + [keys.docHash({ doc_id })]: docHash, + [keys.ranges({ doc_id })]: ranges, + [keys.pathname({ doc_id })]: pathname, + [keys.projectHistoryId({ doc_id })]: projectHistoryId + }, + callback + ) }) }) }, @@ -119,17 +118,19 @@ module.exports = RedisManager = { let multi = rclient.multi() multi.strlen(keys.docLines({ doc_id })) - multi.del(keys.docLines({ doc_id })) - multi.del(keys.projectKey({ doc_id })) - multi.del(keys.docVersion({ doc_id })) - multi.del(keys.docHash({ doc_id })) - multi.del(keys.ranges({ doc_id })) - multi.del(keys.pathname({ doc_id })) - multi.del(keys.projectHistoryId({ doc_id })) - multi.del(keys.projectHistoryType({ doc_id })) - multi.del(keys.unflushedTime({ doc_id })) - multi.del(keys.lastUpdatedAt({ doc_id })) - multi.del(keys.lastUpdatedBy({ doc_id })) + multi.del( + keys.docLines({ doc_id }), + keys.projectKey({ doc_id }), + keys.docVersion({ doc_id }), + keys.docHash({ doc_id }), + keys.ranges({ doc_id }), + keys.pathname({ doc_id }), + keys.projectHistoryId({ doc_id }), + keys.projectHistoryType({ doc_id }), + keys.unflushedTime({ doc_id }), + keys.lastUpdatedAt({ doc_id }), + keys.lastUpdatedBy({ doc_id }) + ) return multi.exec(function (error, response) { if (error != null) { return callback(error) @@ -483,19 +484,19 @@ module.exports = RedisManager = { return callback(error) } const multi = rclient.multi() - multi.set(keys.docLines({ doc_id }), newDocLines) // index 0 - multi.set(keys.docVersion({ doc_id }), newVersion) // index 1 - multi.set(keys.docHash({ doc_id }), newHash) // index 2 + multi.mset({ + [keys.docLines({ doc_id })]: newDocLines, + [keys.docVersion({ doc_id })]: newVersion, + [keys.docHash({ doc_id })]: newHash, + [keys.ranges({ doc_id })]: ranges, + [keys.lastUpdatedAt({ doc_id })]: Date.now(), + [keys.lastUpdatedBy({ doc_id })]: updateMeta && updateMeta.user_id + }) multi.ltrim( keys.docOps({ doc_id }), -RedisManager.DOC_OPS_MAX_LENGTH, -1 ) // index 3 - if (ranges != null) { - multi.set(keys.ranges({ doc_id }), ranges) // index 4 - } else { - multi.del(keys.ranges({ doc_id })) // also index 4 - } // push the ops last so we can get the lengths at fixed index position 7 if (jsonOps.length > 0) { multi.rpush(keys.docOps({ doc_id }), ...Array.from(jsonOps)) // index 5 @@ -519,12 +520,6 @@ module.exports = RedisManager = { // hasn't been modified before (the content in mongo has been // valid up to this point). Otherwise leave it alone ("NX" flag). multi.set(keys.unflushedTime({ doc_id }), Date.now(), 'NX') - multi.set(keys.lastUpdatedAt({ doc_id }), Date.now()) // index 8 - if (updateMeta != null ? updateMeta.user_id : undefined) { - multi.set(keys.lastUpdatedBy({ doc_id }), updateMeta.user_id) // index 9 - } else { - multi.del(keys.lastUpdatedBy({ doc_id })) // index 9 - } } return multi.exec(function (error, result) { let docUpdateCount @@ -536,7 +531,7 @@ module.exports = RedisManager = { docUpdateCount = undefined // only using project history, don't bother with track-changes } else { // project is using old track-changes history service - docUpdateCount = result[7] // length of uncompressedHistoryOps queue (index 7) + docUpdateCount = result[4] } if ( diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index d8f21844cd..29329e8411 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -153,7 +153,6 @@ describe('RedisManager', function () { this.projectHistoryId.toString(), this.unflushed_time ]) - return (this.rclient.sadd = sinon.stub().yields(null, 0)) }) describe('successfully', function () { @@ -469,6 +468,7 @@ describe('RedisManager', function () { this.project_update_list_length = sinon.stub() this.RedisManager.getDocVersion = sinon.stub() + this.multi.mset = sinon.stub() this.multi.set = sinon.stub() this.multi.rpush = sinon.stub() this.multi.expire = sinon.stub() @@ -477,9 +477,6 @@ describe('RedisManager', function () { this.multi.exec = sinon .stub() .callsArgWith(0, null, [ - this.hash, - null, - null, null, null, null, @@ -524,27 +521,16 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should set the doclines', function () { - return this.multi.set - .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines)) - .should.equal(true) - }) - - it('should set the version', function () { - return this.multi.set - .calledWith(`DocVersion:${this.doc_id}`, this.version) - .should.equal(true) - }) - - it('should set the hash', function () { - return this.multi.set - .calledWith(`DocHash:${this.doc_id}`, this.hash) - .should.equal(true) - }) - - it('should set the ranges', function () { - return this.multi.set - .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges)) + it('should set most details in a single MSET call', function () { + this.multi.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges), + [`lastUpdatedAt:${this.doc_id}`]: Date.now(), + [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id' + }) .should.equal(true) }) @@ -554,18 +540,6 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should set the last updated time', function () { - return this.multi.set - .calledWith(`lastUpdatedAt:${this.doc_id}`, Date.now()) - .should.equal(true) - }) - - it('should set the last updater', function () { - return this.multi.set - .calledWith(`lastUpdatedBy:${this.doc_id}`, 'last-author-fake-id') - .should.equal(true) - }) - it('should push the doc op into the doc ops list', function () { return this.multi.rpush .calledWith( @@ -747,8 +721,15 @@ describe('RedisManager', function () { }) return it('should still set the doclines', function () { - return this.multi.set - .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines)) + this.multi.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges), + [`lastUpdatedAt:${this.doc_id}`]: Date.now(), + [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id' + }) .should.equal(true) }) }) @@ -770,15 +751,16 @@ describe('RedisManager', function () { ) }) - it('should not set the ranges', function () { - return this.multi.set - .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges)) - .should.equal(false) - }) - - return it('should delete the ranges key', function () { - return this.multi.del - .calledWith(`Ranges:${this.doc_id}`) + it('should set empty ranges', function () { + this.multi.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: null, + [`lastUpdatedAt:${this.doc_id}`]: Date.now(), + [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id' + }) .should.equal(true) }) }) @@ -866,15 +848,16 @@ describe('RedisManager', function () { ) }) - it('should set the last updater to null', function () { - return this.multi.del - .calledWith(`lastUpdatedBy:${this.doc_id}`) - .should.equal(true) - }) - - return it('should still set the last updated time', function () { - return this.multi.set - .calledWith(`lastUpdatedAt:${this.doc_id}`, Date.now()) + it('should unset last updater', function () { + this.multi.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges), + [`lastUpdatedAt:${this.doc_id}`]: Date.now(), + [`lastUpdatedBy:${this.doc_id}`]: undefined + }) .should.equal(true) }) }) @@ -882,16 +865,14 @@ describe('RedisManager', function () { describe('putDocInMemory', function () { beforeEach(function () { - this.multi.set = sinon.stub() + this.rclient.mset = sinon.stub().yields(null) this.rclient.sadd = sinon.stub().yields() - this.multi.del = sinon.stub() this.lines = ['one', 'two', 'three', 'これは'] this.version = 42 this.hash = crypto .createHash('sha1') .update(JSON.stringify(this.lines), 'utf8') .digest('hex') - this.multi.exec = sinon.stub().callsArgWith(0, null, [this.hash]) this.ranges = { comments: 'mock', entries: 'mock' } return (this.pathname = '/a/b/c.tex') }) @@ -910,45 +891,17 @@ describe('RedisManager', function () { ) }) - it('should set the lines', function () { - return this.multi.set - .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines)) - .should.equal(true) - }) - - it('should set the version', function () { - return this.multi.set - .calledWith(`DocVersion:${this.doc_id}`, this.version) - .should.equal(true) - }) - - it('should set the hash', function () { - return this.multi.set - .calledWith(`DocHash:${this.doc_id}`, this.hash) - .should.equal(true) - }) - - it('should set the ranges', function () { - return this.multi.set - .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges)) - .should.equal(true) - }) - - it('should set the project_id for the doc', function () { - return this.multi.set - .calledWith(`ProjectId:${this.doc_id}`, this.project_id) - .should.equal(true) - }) - - it('should set the pathname for the doc', function () { - return this.multi.set - .calledWith(`Pathname:${this.doc_id}`, this.pathname) - .should.equal(true) - }) - - it('should set the projectHistoryId for the doc', function () { - return this.multi.set - .calledWith(`ProjectHistoryId:${this.doc_id}`, this.projectHistoryId) + it('should set all the details in a single MSET call', function () { + this.rclient.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`ProjectId:${this.doc_id}`]: this.project_id, + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges), + [`Pathname:${this.doc_id}`]: this.pathname, + [`ProjectHistoryId:${this.doc_id}`]: this.projectHistoryId + }) .should.equal(true) }) @@ -977,17 +930,19 @@ describe('RedisManager', function () { ) }) - it('should delete the ranges key', function () { - return this.multi.del - .calledWith(`Ranges:${this.doc_id}`) + it('should unset ranges', function () { + this.rclient.mset + .calledWith({ + [`doclines:${this.doc_id}`]: JSON.stringify(this.lines), + [`ProjectId:${this.doc_id}`]: this.project_id, + [`DocVersion:${this.doc_id}`]: this.version, + [`DocHash:${this.doc_id}`]: this.hash, + [`Ranges:${this.doc_id}`]: null, + [`Pathname:${this.doc_id}`]: this.pathname, + [`ProjectHistoryId:${this.doc_id}`]: this.projectHistoryId + }) .should.equal(true) }) - - return it('should not set the ranges', function () { - return this.multi.set - .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges)) - .should.equal(false) - }) }) describe('with null bytes in the serialized doc lines', function () { @@ -1070,33 +1025,21 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should delete the lines', function () { + it('should delete the details in a singe call', function () { return this.multi.del - .calledWith(`doclines:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete the version', function () { - return this.multi.del - .calledWith(`DocVersion:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete the hash', function () { - return this.multi.del - .calledWith(`DocHash:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete the unflushed time', function () { - return this.multi.del - .calledWith(`UnflushedTime:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete the project_id for the doc', function () { - return this.multi.del - .calledWith(`ProjectId:${this.doc_id}`) + .calledWith( + `doclines:${this.doc_id}`, + `ProjectId:${this.doc_id}`, + `DocVersion:${this.doc_id}`, + `DocHash:${this.doc_id}`, + `Ranges:${this.doc_id}`, + `Pathname:${this.doc_id}`, + `ProjectHistoryId:${this.doc_id}`, + `ProjectHistoryType:${this.doc_id}`, + `UnflushedTime:${this.doc_id}`, + `lastUpdatedAt:${this.doc_id}`, + `lastUpdatedBy:${this.doc_id}` + ) .should.equal(true) }) @@ -1105,30 +1048,6 @@ describe('RedisManager', function () { .calledWith(`DocsIn:${this.project_id}`, this.doc_id) .should.equal(true) }) - - it('should delete the pathname for the doc', function () { - return this.multi.del - .calledWith(`Pathname:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete the pathname for the doc', function () { - return this.multi.del - .calledWith(`ProjectHistoryId:${this.doc_id}`) - .should.equal(true) - }) - - it('should delete lastUpdatedAt', function () { - return this.multi.del - .calledWith(`lastUpdatedAt:${this.doc_id}`) - .should.equal(true) - }) - - return it('should delete lastUpdatedBy', function () { - return this.multi.del - .calledWith(`lastUpdatedBy:${this.doc_id}`) - .should.equal(true) - }) }) describe('clearProjectState', function () {