From 06b93aac50fb8f7fe797b6239af091475f9d546a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 6 Sep 2023 16:23:23 +0200 Subject: [PATCH] Merge pull request #14377 from overleaf/tw-docupdater-flush add check for unflushedTime GitOrigin-RevId: e09d63b4de09e30ceb82792526224ad8b2415119 --- package-lock.json | 2 + .../app/js/DocumentManager.js | 12 +- .../document-updater/app/js/RedisManager.js | 6 +- services/document-updater/package.json | 1 + .../acceptance/js/DeletingAProjectTests.js | 121 ++++++++++++++++-- .../test/acceptance/js/RangesTests.js | 43 ++++++- services/document-updater/test/setup.js | 3 + .../unit/js/RedisManager/RedisManagerTests.js | 6 + 8 files changed, 176 insertions(+), 18 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..6044f1e31f 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -288,17 +288,23 @@ module.exports = DocumentManager = { return callback(error) } if (lines == null || version == null) { + Metrics.inc('flush-doc-if-loaded', 1, { status: 'not-loaded' }) logger.debug( { projectId, docId }, 'doc is not loaded so not flushing' ) // TODO: return a flag to bail out, as we go on to remove doc from memory? callback(null) + } else if (unflushedTime == null) { + Metrics.inc('flush-doc-if-loaded', 1, { status: 'unmodified' }) + logger.debug( + { projectId, docId }, + 'doc is not modified so not flushing' + ) + callback(null) } else { logger.debug({ projectId, docId, version }, 'flushing doc') - Metrics.inc('flush-doc-if-loaded', { - status: unflushedTime != null ? 'modified' : 'unmodified', - }) + Metrics.inc('flush-doc-if-loaded', 1, { status: 'modified' }) PersistenceManager.setDoc( projectId, docId, diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index db157c7b70..1f72cb8600 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -454,11 +454,9 @@ module.exports = RedisManager = { multi.rpush(keys.docOps({ doc_id: docId }), ...jsonOps) // index 5 // expire must come after rpush since before it will be a no-op if the list is empty multi.expire(keys.docOps({ doc_id: docId }), RedisManager.DOC_OPS_TTL) // index 6 - // Set the unflushed timestamp to the current time if the doc - // 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: docId }), Date.now(), 'NX') } + // Set the unflushed timestamp to the current time if not set ("NX" flag). + multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX') multi.exec((error, result) => { if (error) { return callback(error) 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/DeletingAProjectTests.js b/services/document-updater/test/acceptance/js/DeletingAProjectTests.js index f7ad454c69..cca0b4d862 100644 --- a/services/document-updater/test/acceptance/js/DeletingAProjectTests.js +++ b/services/document-updater/test/acceptance/js/DeletingAProjectTests.js @@ -15,7 +15,7 @@ const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') describe('Deleting a project', function () { - before(function (done) { + beforeEach(function (done) { let docId0, docId1 this.project_id = DocUpdaterClient.randomId() this.docs = [ @@ -60,8 +60,87 @@ describe('Deleting a project', function () { DocUpdaterApp.ensureRunning(done) }) + describe('without updates', function () { + beforeEach(function (done) { + sinon.spy(MockWebApi, 'setDocument') + sinon.spy(MockProjectHistoryApi, 'flushProject') + + async.series( + this.docs.map(doc => { + return callback => { + DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => { + callback(error) + }) + } + }), + error => { + if (error != null) { + throw error + } + setTimeout(() => { + DocUpdaterClient.deleteProject( + this.project_id, + (error, res, body) => { + if (error) return done(error) + this.statusCode = res.statusCode + done() + } + ) + }, 200) + } + ) + }) + + afterEach(function () { + MockWebApi.setDocument.restore() + MockProjectHistoryApi.flushProject.restore() + }) + + it('should return a 204 status code', function () { + this.statusCode.should.equal(204) + }) + + it('should not send any document to the web api', function () { + MockWebApi.setDocument.should.not.have.been.called + }) + + it('should need to reload the docs if read again', function (done) { + sinon.spy(MockWebApi, 'getDocument') + async.series( + this.docs.map(doc => { + return callback => { + MockWebApi.getDocument + .calledWith(this.project_id, doc.id) + .should.equal(false) + DocUpdaterClient.getDoc( + this.project_id, + doc.id, + (error, res, returnedDoc) => { + if (error) return done(error) + MockWebApi.getDocument + .calledWith(this.project_id, doc.id) + .should.equal(true) + callback() + } + ) + } + }), + () => { + MockWebApi.getDocument.restore() + done() + } + ) + }) + + it('should flush each doc in project history', function () { + MockProjectHistoryApi.flushProject + .calledWith(this.project_id) + .should.equal(true) + }) + }) + describe('with documents which have been updated', function () { - before(function (done) { + beforeEach(function (done) { sinon.spy(MockWebApi, 'setDocument') sinon.spy(MockProjectHistoryApi, 'flushProject') @@ -101,7 +180,7 @@ describe('Deleting a project', function () { ) }) - after(function () { + afterEach(function () { MockWebApi.setDocument.restore() MockProjectHistoryApi.flushProject.restore() }) @@ -154,14 +233,26 @@ describe('Deleting a project', function () { }) describe('with the background=true parameter from realtime and no request to flush the queue', function () { - before(function (done) { + beforeEach(function (done) { sinon.spy(MockWebApi, 'setDocument') sinon.spy(MockProjectHistoryApi, 'flushProject') async.series( this.docs.map(doc => { return callback => { - DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback) + DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => { + if (error != null) { + return callback(error) + } + DocUpdaterClient.sendUpdate( + this.project_id, + doc.id, + doc.update, + error => { + callback(error) + } + ) + }) } }), error => { @@ -182,7 +273,7 @@ describe('Deleting a project', function () { ) }) - after(function () { + afterEach(function () { MockWebApi.setDocument.restore() MockProjectHistoryApi.flushProject.restore() }) @@ -201,14 +292,26 @@ describe('Deleting a project', function () { }) describe('with the background=true parameter from realtime and a request to flush the queue', function () { - before(function (done) { + beforeEach(function (done) { sinon.spy(MockWebApi, 'setDocument') sinon.spy(MockProjectHistoryApi, 'flushProject') async.series( this.docs.map(doc => { return callback => { - DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback) + DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => { + if (error != null) { + return callback(error) + } + DocUpdaterClient.sendUpdate( + this.project_id, + doc.id, + doc.update, + error => { + callback(error) + } + ) + }) } }), error => { @@ -230,7 +333,7 @@ describe('Deleting a project', function () { ) }) - after(function () { + afterEach(function () { MockWebApi.setDocument.restore() MockProjectHistoryApi.flushProject.restore() }) diff --git a/services/document-updater/test/acceptance/js/RangesTests.js b/services/document-updater/test/acceptance/js/RangesTests.js index 97be0be20b..809e8e4b62 100644 --- a/services/document-updater/test/acceptance/js/RangesTests.js +++ b/services/document-updater/test/acceptance/js/RangesTests.js @@ -346,7 +346,8 @@ describe('Ranges', function () { }) describe('accepting a change', function () { - before(function (done) { + beforeEach(function (done) { + sinon.spy(MockWebApi, 'setDocument') this.project_id = DocUpdaterClient.randomId() this.user_id = DocUpdaterClient.randomId() this.id_seed = '587357bd35e64f6157' @@ -401,8 +402,11 @@ describe('Ranges', function () { } ) }) + afterEach(function () { + MockWebApi.setDocument.restore() + }) - return it('should remove the change after accepting', function (done) { + it('should remove the change after accepting', function (done) { return DocUpdaterClient.acceptChange( this.project_id, this.doc.id, @@ -425,6 +429,41 @@ describe('Ranges', function () { } ) }) + + it('should persist the ranges after accepting', function (done) { + DocUpdaterClient.flushDoc(this.project_id, this.doc.id, err => { + if (err) return done(err) + DocUpdaterClient.acceptChange( + this.project_id, + this.doc.id, + this.id_seed + '000001', + error => { + if (error != null) { + throw error + } + + DocUpdaterClient.flushDoc(this.project_id, this.doc.id, err => { + if (err) return done(err) + DocUpdaterClient.getDoc( + this.project_id, + this.doc.id, + (error, res, data) => { + if (error != null) { + throw error + } + expect(data.ranges.changes).to.be.undefined + + MockWebApi.setDocument + .calledWith(this.project_id, this.doc.id, ['a456aa'], 1, {}) + .should.equal(true) + done() + } + ) + }) + } + ) + }) + }) }) describe('deleting a comment range', function () { diff --git a/services/document-updater/test/setup.js b/services/document-updater/test/setup.js index 9b9b4c029e..b0dfbc7028 100644 --- a/services/document-updater/test/setup.js +++ b/services/document-updater/test/setup.js @@ -4,6 +4,9 @@ const sinon = require('sinon') // Chai configuration chai.should() +// Load sinon-chai assertions so expect(stubFn).to.have.been.calledWith('abc') +// has a nicer failure messages +chai.use(require('sinon-chai')) // Global stubs const sandbox = sinon.createSandbox() diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 75e5c7d7f8..a31987d2a1 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -606,6 +606,12 @@ describe('RedisManager', function () { ) }) + it('should set the unflushed time (potential ranges changes)', function () { + this.multi.set + .calledWith(`UnflushedTime:${this.docId}`, Date.now(), 'NX') + .should.equal(true) + }) + it('should not try to enqueue doc updates', function () { this.multi.rpush.called.should.equal(false) })