Merge pull request #10232 from overleaf/jpa-lg-silent-no-op-external-update

[document-updater] skip emitting no-op external updates

GitOrigin-RevId: 3fc29740fb8c2bc8b1ba21428a0e8d0d34b6eaf8

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)
             }

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)

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(