From c1e6b2c990fa4026989a061a289c540fb7ea7615 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 22 Mar 2023 13:08:01 +0000 Subject: [PATCH] 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 | 33 ++++++++++ services/docstore/app/js/MongoManager.js | 66 ++++++++++++++----- .../docstore/test/unit/js/DocManagerTests.js | 55 ++++++++++++++-- .../test/unit/js/MongoManagerTests.js | 65 ++++++++++++++++-- 4 files changed, 193 insertions(+), 26 deletions(-) diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 8c28a10ae8..b68f928d77 100644 --- a/services/docstore/app/js/DocManager.js +++ b/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 ) diff --git a/services/docstore/app/js/MongoManager.js b/services/docstore/app/js/MongoManager.js index 466196b404..2abb135838 100644 --- a/services/docstore/app/js/MongoManager.js +++ b/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) { diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js index abf8afb03c..bbafbe1c69 100644 --- a/services/docstore/test/unit/js/DocManagerTests.js +++ b/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) + }) + }) }) }) diff --git a/services/docstore/test/unit/js/MongoManagerTests.js b/services/docstore/test/unit/js/MongoManagerTests.js index 95a84bcea8..97fe193400 100644 --- a/services/docstore/test/unit/js/MongoManagerTests.js +++ b/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 () {