Merge pull request #12230 from overleaf/jpa-upsert-rev-check

[docstore] add rev-check to doc upsert and retry update once

GitOrigin-RevId: 754f005024ed809ae7365ef38f10a961c5546171

services/docstore/app/js/DocManager.js

@@ -227,6 +227,38 @@ module.exports = DocManager = {
   },
 
   updateDoc(projectId, docId, lines, version, ranges, callback) {
+    DocManager._tryUpdateDoc(
+      projectId,
+      docId,
+      lines,
+      version,
+      ranges,
+      (err, modified, rev) => {
+        if (err && err instanceof Errors.DocRevValueError) {
+          // Another updateDoc call was racing with ours.
+          // Retry once in a bit.
+          logger.warn(
+            { projectId, docId, err },
+            'detected concurrent updateDoc call'
+          )
+          setTimeout(() => {
+            DocManager._tryUpdateDoc(
+              projectId,
+              docId,
+              lines,
+              version,
+              ranges,
+              callback
+            )
+          }, 100 + Math.random() * 100)
+        } else {
+          callback(err, modified, rev)
+        }
+      }
+    )
+  },
+
+  _tryUpdateDoc(projectId, docId, lines, version, ranges, callback) {
     if (callback == null) {
       callback = function () {}
     }
@@ -297,6 +329,7 @@ module.exports = DocManager = {
             return MongoManager.upsertIntoDocCollection(
               projectId,
               docId,
+              doc?.rev,
               update,
               cb
             )

services/docstore/app/js/MongoManager.js

@@ -85,23 +85,57 @@ function getNonDeletedArchivedProjectDocs(projectId, maxResults, callback) {
     .toArray(callback)
 }
 
-function upsertIntoDocCollection(projectId, docId, updates, callback) {
-  const update = {
-    $set: updates,
-    $inc: {
-      rev: 1,
-    },
-    $unset: {
-      inS3: true,
-    },
+function upsertIntoDocCollection(
+  projectId,
+  docId,
+  previousRev,
+  updates,
+  callback
+) {
+  if (previousRev) {
+    db.docs.updateOne(
+      {
+        _id: ObjectId(docId),
+        project_id: ObjectId(projectId),
+        rev: previousRev,
+      },
+      {
+        $set: updates,
+        $inc: {
+          rev: 1,
+        },
+        $unset: {
+          inS3: true,
+        },
+      },
+      (err, result) => {
+        if (err) return callback(err)
+        if (result.matchedCount !== 1) {
+          return callback(new Errors.DocRevValueError())
+        }
+        callback()
+      }
+    )
+  } else {
+    db.docs.insertOne(
+      {
+        _id: ObjectId(docId),
+        project_id: ObjectId(projectId),
+        rev: 1,
+        ...updates,
+      },
+      err => {
+        if (err) {
+          if (err.code === 11000) {
+            // duplicate doc _id
+            return callback(new Errors.DocRevValueError())
+          }
+          return callback(err)
+        }
+        callback()
+      }
+    )
   }
-  update.$set.project_id = ObjectId(projectId)
-  db.docs.updateOne(
-    { _id: ObjectId(docId) },
-    update,
-    { upsert: true },
-    callback
-  )
 }
 
 function patchDoc(projectId, docId, meta, callback) {

services/docstore/test/unit/js/DocManagerTests.js

@@ -567,7 +567,7 @@ describe('DocManager', function () {
         ranges: this.originalRanges,
       }
 
-      this.MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(3)
+      this.MongoManager.upsertIntoDocCollection = sinon.stub().yields()
       this.MongoManager.setDocVersion = sinon.stub().yields()
       return (this.DocManager._getDoc = sinon.stub())
     })
@@ -600,7 +600,9 @@ describe('DocManager', function () {
 
       it('should upsert the document to the doc collection', function () {
         return this.MongoManager.upsertIntoDocCollection
-          .calledWith(this.project_id, this.doc_id, { lines: this.newDocLines })
+          .calledWith(this.project_id, this.doc_id, this.rev, {
+            lines: this.newDocLines,
+          })
           .should.equal(true)
       })
 
@@ -631,7 +633,9 @@ describe('DocManager', function () {
 
       it('should upsert the ranges', function () {
         return this.MongoManager.upsertIntoDocCollection
-          .calledWith(this.project_id, this.doc_id, { ranges: this.newRanges })
+          .calledWith(this.project_id, this.doc_id, this.rev, {
+            ranges: this.newRanges,
+          })
           .should.equal(true)
       })
 
@@ -842,7 +846,7 @@ describe('DocManager', function () {
       })
     })
 
-    return describe('when the doc does not exist', function () {
+    describe('when the doc does not exist', function () {
       beforeEach(function () {
         this.DocManager._getDoc = sinon.stub().callsArgWith(3, null, null, null)
         return this.DocManager.updateDoc(
@@ -857,7 +861,7 @@ describe('DocManager', function () {
 
       it('should upsert the document to the doc collection', function () {
         return this.MongoManager.upsertIntoDocCollection
-          .calledWith(this.project_id, this.doc_id, {
+          .calledWith(this.project_id, this.doc_id, undefined, {
             lines: this.newDocLines,
             ranges: this.originalRanges,
           })
@@ -874,5 +878,46 @@ describe('DocManager', function () {
         return this.callback.calledWith(null, true, 1).should.equal(true)
       })
     })
+
+    describe('when another update is racing', function () {
+      beforeEach(function (done) {
+        this.DocManager._getDoc = sinon.stub().yields(null, this.doc)
+        this.MongoManager.upsertIntoDocCollection
+          .onFirstCall()
+          .yields(new Errors.DocRevValueError())
+        this.MongoManager.upsertIntoDocCollection.onSecondCall().yields(null)
+        this.RangeManager.shouldUpdateRanges.returns(true)
+        this.callback.callsFake(done)
+        this.DocManager.updateDoc(
+          this.project_id,
+          this.doc_id,
+          this.newDocLines,
+          this.version + 1,
+          this.newRanges,
+          this.callback
+        )
+      })
+
+      it('should upsert the doc twice', function () {
+        this.MongoManager.upsertIntoDocCollection.should.have.been.calledWith(
+          this.project_id,
+          this.doc_id,
+          this.rev,
+          {
+            ranges: this.newRanges,
+            lines: this.newDocLines,
+          }
+        )
+        this.MongoManager.upsertIntoDocCollection.should.have.been.calledTwice
+      })
+
+      it('should update the version once', function () {
+        this.MongoManager.setDocVersion.should.have.been.calledOnce
+      })
+
+      it('should return the callback with the new rev', function () {
+        this.callback.should.have.been.calledWith(null, true, this.rev + 1)
+      })
+    })
   })
 })

services/docstore/test/unit/js/MongoManagerTests.js

@@ -13,6 +13,7 @@ describe('MongoManager', function () {
     this.db = {
       docs: {
         updateOne: sinon.stub().yields(null, { matchedCount: 1 }),
+        insertOne: sinon.stub().yields(null),
       },
       docOps: {},
     }
@@ -32,6 +33,7 @@ describe('MongoManager', function () {
     })
     this.projectId = ObjectId().toString()
     this.docId = ObjectId().toString()
+    this.rev = 42
     this.callback = sinon.stub()
     this.stubbedErr = new Error('hello world')
   })
@@ -215,7 +217,6 @@ describe('MongoManager', function () {
 
   describe('upsertIntoDocCollection', function () {
     beforeEach(function () {
-      this.db.docs.updateOne.yields(this.stubbedErr)
       this.oldRev = 77
     })
 
@@ -223,23 +224,29 @@ describe('MongoManager', function () {
       this.MongoManager.upsertIntoDocCollection(
         this.projectId,
         this.docId,
+        this.oldRev,
         { lines: this.lines },
         err => {
-          assert.equal(err, this.stubbedErr)
+          assert.equal(err, null)
           const args = this.db.docs.updateOne.args[0]
-          assert.deepEqual(args[0], { _id: ObjectId(this.docId) })
+          assert.deepEqual(args[0], {
+            _id: ObjectId(this.docId),
+            project_id: ObjectId(this.projectId),
+            rev: this.oldRev,
+          })
           assert.equal(args[1].$set.lines, this.lines)
           assert.equal(args[1].$inc.rev, 1)
-          assert.deepEqual(args[1].$set.project_id, ObjectId(this.projectId))
           done()
         }
       )
     })
 
-    it('should return the error', function (done) {
+    it('should handle update error', function (done) {
+      this.db.docs.updateOne.yields(this.stubbedErr)
       this.MongoManager.upsertIntoDocCollection(
         this.projectId,
         this.docId,
+        this.rev,
         { lines: this.lines },
         err => {
           err.should.equal(this.stubbedErr)
@@ -247,6 +254,54 @@ describe('MongoManager', function () {
         }
       )
     })
+
+    it('should insert without a previous rev', function (done) {
+      this.MongoManager.upsertIntoDocCollection(
+        this.projectId,
+        this.docId,
+        null,
+        { lines: this.lines, ranges: this.ranges },
+        err => {
+          expect(this.db.docs.insertOne).to.have.been.calledWith({
+            _id: ObjectId(this.docId),
+            project_id: ObjectId(this.projectId),
+            rev: 1,
+            lines: this.lines,
+            ranges: this.ranges,
+          })
+          expect(err).to.not.exist
+          done()
+        }
+      )
+    })
+
+    it('should handle generic insert error', function (done) {
+      this.db.docs.insertOne.yields(this.stubbedErr)
+      this.MongoManager.upsertIntoDocCollection(
+        this.projectId,
+        this.docId,
+        null,
+        { lines: this.lines, ranges: this.ranges },
+        err => {
+          expect(err).to.equal(this.stubbedErr)
+          done()
+        }
+      )
+    })
+
+    it('should handle duplicate insert error', function (done) {
+      this.db.docs.insertOne.yields({ code: 11000 })
+      this.MongoManager.upsertIntoDocCollection(
+        this.projectId,
+        this.docId,
+        null,
+        { lines: this.lines, ranges: this.ranges },
+        err => {
+          expect(err).to.be.instanceof(Errors.DocRevValueError)
+          done()
+        }
+      )
+    })
   })
 
   describe('destroyProject', function () {