From 2cc84488a8864a28e9206433d1d006c698d5d945 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 15 Nov 2023 07:30:14 -0500 Subject: [PATCH] Merge pull request #15693 from overleaf/em-write-version-to-docs Write version directly to docs collection GitOrigin-RevId: 1133c78368197c3c2b160c224bbeb5bfb46b8729 --- services/docstore/app/js/DocManager.js | 43 +++++---------- services/docstore/app/js/MongoManager.js | 33 +++--------- .../docstore/test/unit/js/DocManagerTests.js | 53 ++++++------------- .../test/unit/js/MongoManagerTests.js | 31 +---------- 4 files changed, 38 insertions(+), 122 deletions(-) diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 8735300c9a..478aee295d 100644 --- a/services/docstore/app/js/DocManager.js +++ b/services/docstore/app/js/DocManager.js @@ -9,10 +9,6 @@ const { callbackifyAll } = require('@overleaf/promise-utils') const { setTimeout } = require('timers/promises') const DocManager = { - // TODO: For historical reasons, the doc version is currently stored in the docOps - // collection (which is all that this collection contains). In future, we should - // migrate this version property to be part of the docs collection, to guarantee - // consitency between lines and version when writing/reading, and for a simpler schema. async _getDoc(projectId, docId, filter) { if (filter == null) { filter = {} @@ -208,7 +204,7 @@ const DocManager = { let modified = false let rev = doc?.rev || 0 - if (updateLines || updateRanges) { + if (updateLines || updateRanges || updateVersion) { const update = {} if (updateLines) { update.lines = lines @@ -216,10 +212,19 @@ const DocManager = { if (updateRanges) { update.ranges = ranges } - logger.debug({ projectId, docId }, 'updating doc lines and ranges') + if (updateVersion) { + update.version = version + } + logger.debug( + { projectId, docId, oldVersion: doc?.version, newVersion: version }, + 'updating doc' + ) + + if (updateLines || updateRanges) { + rev += 1 // rev will be incremented in mongo by MongoManager.upsertIntoDocCollection + } modified = true - rev += 1 // rev will be incremented in mongo by MongoManager.upsertIntoDocCollection await MongoManager.promises.upsertIntoDocCollection( projectId, docId, @@ -227,29 +232,7 @@ const DocManager = { update ) } else { - logger.debug( - { projectId, docId }, - 'doc lines have not changed - not updating' - ) - } - - if (updateVersion) { - logger.debug( - { - projectId, - docId, - oldVersion: doc?.version, - newVersion: version, - }, - 'updating doc version' - ) - modified = true - await MongoManager.promises.setDocVersion(docId, version) - } else { - logger.debug( - { projectId, docId, version }, - 'doc version has not changed - not updating' - ) + logger.debug({ projectId, docId }, 'doc has not changed - not updating') } return { modified, rev } diff --git a/services/docstore/app/js/MongoManager.js b/services/docstore/app/js/MongoManager.js index 8297b790c8..acce8dbea6 100644 --- a/services/docstore/app/js/MongoManager.js +++ b/services/docstore/app/js/MongoManager.js @@ -93,21 +93,20 @@ function upsertIntoDocCollection( callback ) { if (previousRev) { + const update = { + $set: updates, + $unset: { inS3: true }, + } + if (updates.lines || updates.ranges) { + update.$inc = { rev: 1 } + } db.docs.updateOne( { _id: ObjectId(docId), project_id: ObjectId(projectId), rev: previousRev, }, - { - $set: updates, - $inc: { - rev: 1, - }, - $unset: { - inS3: true, - }, - }, + update, (err, result) => { if (err) return callback(err) if (result.matchedCount !== 1) { @@ -248,21 +247,6 @@ function getDocVersion(docId, callback) { ) } -function setDocVersion(docId, version, callback) { - db.docOps.updateOne( - { - doc_id: ObjectId(docId), - }, - { - $set: { version }, - }, - { - upsert: true, - }, - callback - ) -} - function getDocRev(docId, callback) { db.docs.findOne( { @@ -341,7 +325,6 @@ module.exports = { getDocForArchiving, markDocAsArchived, getDocVersion, - setDocVersion, checkRevUnchanged, destroyProject, } diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js index 69a8c98c04..852ac8fbaa 100644 --- a/services/docstore/test/unit/js/DocManagerTests.js +++ b/services/docstore/test/unit/js/DocManagerTests.js @@ -20,7 +20,6 @@ describe('DocManager', function () { getProjectsDocs: sinon.stub(), patchDoc: sinon.stub().resolves(), upsertIntoDocCollection: sinon.stub().resolves(), - setDocVersion: sinon.stub().resolves(), }, } this.DocArchiveManager = { @@ -545,10 +544,6 @@ describe('DocManager', function () { .should.equal(true) }) - it('should not update the version', function () { - this.MongoManager.promises.setDocVersion.called.should.equal(false) - }) - it('should return the new rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: this.rev + 1 }) }) @@ -575,10 +570,6 @@ describe('DocManager', function () { .should.equal(true) }) - it('should not update the version', function () { - this.MongoManager.promises.setDocVersion.called.should.equal(false) - }) - it('should return the new rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: this.rev + 1 }) }) @@ -596,16 +587,13 @@ describe('DocManager', function () { ) }) - it('should not change the lines or ranges', function () { - this.MongoManager.promises.upsertIntoDocCollection.called.should.equal( - false - ) - }) - it('should update the version', function () { - this.MongoManager.promises.setDocVersion - .calledWith(this.doc_id, this.version + 1) - .should.equal(true) + this.MongoManager.promises.upsertIntoDocCollection.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.rev, + { version: this.version + 1 } + ) }) it('should return the old rev', function () { @@ -625,16 +613,12 @@ describe('DocManager', function () { ) }) - it('should not update the ranges or lines', function () { + it('should not update the ranges or lines or version', function () { this.MongoManager.promises.upsertIntoDocCollection.called.should.equal( false ) }) - it('should not update the version', function () { - this.MongoManager.promises.setDocVersion.called.should.equal(false) - }) - it('should return the old rev and modified == false', function () { expect(this.result).to.deep.equal({ modified: false, rev: this.rev }) }) @@ -754,18 +738,16 @@ describe('DocManager', function () { }) it('should upsert the document to the doc collection', function () { - this.MongoManager.promises.upsertIntoDocCollection - .calledWith(this.project_id, this.doc_id, undefined, { + this.MongoManager.promises.upsertIntoDocCollection.should.have.been.calledWith( + this.project_id, + this.doc_id, + undefined, + { lines: this.newDocLines, ranges: this.originalRanges, - }) - .should.equal(true) - }) - - it('should set the version', function () { - this.MongoManager.promises.setDocVersion - .calledWith(this.doc_id, this.version) - .should.equal(true) + version: this.version, + } + ) }) it('should return the new rev', function () { @@ -797,16 +779,13 @@ describe('DocManager', function () { { ranges: this.newRanges, lines: this.newDocLines, + version: this.version + 1, } ) this.MongoManager.promises.upsertIntoDocCollection.should.have.been .calledTwice }) - it('should update the version once', function () { - this.MongoManager.promises.setDocVersion.should.have.been.calledOnce - }) - it('should return the new rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: this.rev + 1 }) }) diff --git a/services/docstore/test/unit/js/MongoManagerTests.js b/services/docstore/test/unit/js/MongoManagerTests.js index 6e90253847..8eeda93f82 100644 --- a/services/docstore/test/unit/js/MongoManagerTests.js +++ b/services/docstore/test/unit/js/MongoManagerTests.js @@ -36,6 +36,7 @@ describe('MongoManager', function () { this.rev = 42 this.callback = sinon.stub() this.stubbedErr = new Error('hello world') + this.lines = ['Three French hens', 'Two turtle doves'] }) describe('findDoc', function () { @@ -373,36 +374,6 @@ describe('MongoManager', function () { }) }) - describe('setDocVersion', function () { - beforeEach(function () { - this.version = 42 - this.db.docOps.updateOne = sinon.stub().callsArg(3) - this.MongoManager.setDocVersion(this.docId, this.version, this.callback) - }) - - it('should update the doc version', function () { - this.db.docOps.updateOne - .calledWith( - { - doc_id: ObjectId(this.docId), - }, - { - $set: { - version: this.version, - }, - }, - { - upsert: true, - } - ) - .should.equal(true) - }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) - }) - describe('checkRevUnchanged', function () { this.beforeEach(function () { this.doc = { _id: ObjectId(), name: 'mock-doc', rev: 1 }