From e7203ed0f8f27d6e8083672db27c4d14e15dbef1 Mon Sep 17 00:00:00 2001 From: Lucie Germain <116178070+Luvacie84@users.noreply.github.com> Date: Thu, 3 Nov 2022 11:20:18 +0100 Subject: [PATCH] Merge pull request #10232 from overleaf/jpa-lg-silent-no-op-external-update [document-updater] skip emitting no-op external updates GitOrigin-RevId: 3fc29740fb8c2bc8b1ba21428a0e8d0d34b6eaf8 --- .../app/js/DocumentManager.js | 13 ++- .../acceptance/js/SettingADocumentTests.js | 57 +++++++++++++ .../DocumentManager/DocumentManagerTests.js | 82 +++++++++++++++++++ 3 files changed, 151 insertions(+), 1 deletion(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index a8f5106daa..c6f12739fb 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -229,7 +229,18 @@ module.exports = DocumentManager = { method: alreadyLoaded ? 'flush' : 'evict', path: source, }) - UpdateManager.applyUpdate(projectId, docId, update, error => { + const applyUpdateIfNeeded = cb => { + if (op.length === 0) { + // Do not notify the frontend about a noop update. + // We still want to execute the callback code below + // to evict the doc if we loaded it into redis for + // this update, otherwise the doc would never be + // removed from redis. + return cb(null) + } + UpdateManager.applyUpdate(projectId, docId, update, cb) + } + applyUpdateIfNeeded(error => { if (error) { return callback(error) } diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 7d66e5e742..e903f1b81e 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -13,7 +13,11 @@ const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') describe('Setting a document', function () { + let numberOfReceivedUpdates = 0 before(function (done) { + DocUpdaterClient.subscribeToAppliedOps(() => { + numberOfReceivedUpdates++ + }) this.lines = ['one', 'two', 'three'] this.version = 42 this.update = { @@ -45,6 +49,7 @@ describe('Setting a document', function () { describe('when the updated doc exists in the doc updater', function () { before(function (done) { + numberOfReceivedUpdates = 0 this.project_id = DocUpdaterClient.randomId() this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { @@ -96,6 +101,10 @@ describe('Setting a document', function () { this.statusCode.should.equal(200) }) + it('should emit two updates (from sendUpdate and setDocLines)', function () { + expect(numberOfReceivedUpdates).to.equal(2) + }) + it('should send the updated doc lines and version to the web api', function () { MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.newLines) @@ -146,12 +155,56 @@ describe('Setting a document', function () { it('should return the mongo rev in the json response', function () { this.body.should.deep.equal({ rev: '123' }) }) + + describe('when doc has the same contents', function () { + beforeEach(function (done) { + numberOfReceivedUpdates = 0 + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false, + (error, res, body) => { + if (error) { + return done(error) + } + this.statusCode = res.statusCode + this.body = body + done() + } + ) + }) + + it('should not bump the version in doc updater', function (done) { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, doc) => { + if (error) { + return done(error) + } + doc.version.should.equal(this.version + 2) + done() + } + ) + }) + + it('should not emit any updates', function (done) { + setTimeout(() => { + expect(numberOfReceivedUpdates).to.equal(0) + done() + }, 100) // delay by 100ms: make sure we do not check too early! + }) + }) }) describe('when the updated doc does not exist in the doc updater', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.doc_id = DocUpdaterClient.randomId() + numberOfReceivedUpdates = 0 MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version, @@ -184,6 +237,10 @@ describe('Setting a document', function () { this.statusCode.should.equal(200) }) + it('should emit an update', function () { + expect(numberOfReceivedUpdates).to.equal(1) + }) + it('should send the updated doc lines to the web api', function () { MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.newLines) diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index e65e8bd96c..3abb8b9638 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -479,6 +479,88 @@ describe('DocumentManager', function () { this.DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(3) }) + describe('when not loaded but with the same content', function () { + beforeEach(function () { + this.DiffCodec.diffAsShareJsOp = sinon.stub().yields(null, []) + this.DocumentManager.getDoc = sinon + .stub() + .yields( + null, + this.beforeLines, + this.version, + this.ranges, + this.pathname, + this.projectHistoryId, + this.unflushedTime, + false + ) + this.DocumentManager.setDoc( + this.project_id, + this.doc_id, + this.beforeLines, + this.source, + this.user_id, + false, + this.callback + ) + }) + + it('should not apply the diff as a ShareJS op', function () { + this.UpdateManager.applyUpdate.called.should.equal(false) + }) + + it('should increment the external update metric', function () { + this.Metrics.inc + .calledWith('external-update', 1, { + status: 'noop', + method: 'evict', + path: this.source, + }) + .should.equal(true) + }) + + it('should flush and delete the doc from redis', function () { + this.DocumentManager.flushAndDeleteDoc + .calledWith(this.project_id, this.doc_id) + .should.equal(true) + }) + }) + + describe('when already loaded with the same content', function () { + beforeEach(function () { + this.DiffCodec.diffAsShareJsOp = sinon.stub().yields(null, []) + this.DocumentManager.setDoc( + this.project_id, + this.doc_id, + this.beforeLines, + this.source, + this.user_id, + false, + this.callback + ) + }) + + it('should not apply the diff as a ShareJS op', function () { + this.UpdateManager.applyUpdate.called.should.equal(false) + }) + + it('should increment the external update metric', function () { + this.Metrics.inc + .calledWith('external-update', 1, { + status: 'noop', + method: 'flush', + path: this.source, + }) + .should.equal(true) + }) + + it('should flush the doc to Mongo', function () { + this.DocumentManager.flushDocIfLoaded + .calledWith(this.project_id, this.doc_id) + .should.equal(true) + }) + }) + describe('when already loaded', function () { beforeEach(function () { this.DocumentManager.setDoc(