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

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

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

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

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 }