Merge pull request #15648 from overleaf/em-promisify-doc-manager

Promisify DocManager

GitOrigin-RevId: c9ab368086492900e1617d5d96943d405f25883d

package-lock.json

@@ -38001,6 +38001,7 @@
         "@overleaf/metrics": "*",
         "@overleaf/o-error": "*",
         "@overleaf/object-persistor": "*",
+        "@overleaf/promise-utils": "*",
         "@overleaf/settings": "*",
         "@overleaf/stream-utils": "^0.1.0",
         "async": "^3.2.2",
@@ -46489,6 +46490,7 @@
         "@overleaf/metrics": "*",
         "@overleaf/o-error": "*",
         "@overleaf/object-persistor": "*",
+        "@overleaf/promise-utils": "*",
         "@overleaf/settings": "*",
         "@overleaf/stream-utils": "^0.1.0",
         "async": "^3.2.2",

services/docstore/app/js/DocManager.js

@@ -1,16 +1,3 @@
-/* eslint-disable
-    no-dupe-keys,
-    no-undef,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let DocManager
 const MongoManager = require('./MongoManager')
 const Errors = require('./Errors')
 const logger = require('@overleaf/logger')
@@ -18,394 +5,286 @@ const _ = require('lodash')
 const DocArchive = require('./DocArchiveManager')
 const RangeManager = require('./RangeManager')
 const Settings = require('@overleaf/settings')
+const { callbackifyAll } = require('@overleaf/promise-utils')
+const { setTimeout } = require('timers/promises')
 
-module.exports = DocManager = {
+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.
-  _getDoc(projectId, docId, filter, callback) {
+  async _getDoc(projectId, docId, filter) {
     if (filter == null) {
       filter = {}
     }
-    if (callback == null) {
-      callback = function () {}
-    }
     if (filter.inS3 !== true) {
-      return callback(new Error('must include inS3 when getting doc'))
+      throw new Error('must include inS3 when getting doc')
     }
 
-    return MongoManager.findDoc(projectId, docId, filter, function (err, doc) {
-      if (err != null) {
-        return callback(err)
-      } else if (doc == null) {
-        return callback(
-          new Errors.NotFoundError(
-            `No such doc: ${docId} in project ${projectId}`
-          )
-        )
-      } else if (doc != null ? doc.inS3 : undefined) {
-        return DocArchive.unarchiveDoc(projectId, docId, function (err) {
-          if (err != null) {
-            logger.err({ err, projectId, docId }, 'error unarchiving doc')
-            return callback(err)
-          }
-          return DocManager._getDoc(projectId, docId, filter, callback)
-        })
-      } else {
-        if (filter.version) {
-          return MongoManager.getDocVersion(docId, function (error, version) {
-            if (error != null) {
-              return callback(error)
-            }
-            doc.version = version
-            return callback(err, doc)
-          })
-        } else {
-          return callback(err, doc)
-        }
-      }
-    })
-  },
+    const doc = await MongoManager.promises.findDoc(projectId, docId, filter)
 
-  isDocDeleted(projectId, docId, callback) {
-    MongoManager.findDoc(
-      projectId,
-      docId,
-      { deleted: true },
-      function (err, doc) {
-        if (err) {
-          return callback(err)
-        }
-        if (!doc) {
-          return callback(
-            new Errors.NotFoundError(
-              `No such project/doc: ${projectId}/${docId}`
-            )
-          )
-        }
-        // `doc.deleted` is `undefined` for non deleted docs
-        callback(null, Boolean(doc.deleted))
-      }
-    )
+    if (doc == null) {
+      throw new Errors.NotFoundError(
+        `No such doc: ${docId} in project ${projectId}`
+      )
+    }
+
+    if (doc.inS3) {
+      await DocArchive.promises.unarchiveDoc(projectId, docId)
+      return await DocManager._getDoc(projectId, docId, filter)
+    }
+
+    if (filter.version) {
+      const version = await MongoManager.promises.getDocVersion(docId)
+      doc.version = version
+    }
+
+    return doc
   },
 
-  getFullDoc(projectId, docId, callback) {
-    if (callback == null) {
-      callback = function () {}
+  async isDocDeleted(projectId, docId) {
+    const doc = await MongoManager.promises.findDoc(projectId, docId, {
+      deleted: true,
+    })
+
+    if (!doc) {
+      throw new Errors.NotFoundError(
+        `No such project/doc: ${projectId}/${docId}`
+      )
     }
-    return DocManager._getDoc(
-      projectId,
-      docId,
-      {
-        lines: true,
-        rev: true,
-        deleted: true,
-        version: true,
-        ranges: true,
-        inS3: true,
-      },
-      function (err, doc) {
-        if (err != null) {
-          return callback(err)
-        }
-        return callback(err, doc)
-      }
-    )
+
+    // `doc.deleted` is `undefined` for non deleted docs
+    return Boolean(doc.deleted)
+  },
+
+  async getFullDoc(projectId, docId) {
+    const doc = await DocManager._getDoc(projectId, docId, {
+      lines: true,
+      rev: true,
+      deleted: true,
+      version: true,
+      ranges: true,
+      inS3: true,
+    })
+    return doc
   },
 
   // returns the doc without any version information
-  _peekRawDoc(projectId, docId, callback) {
-    MongoManager.findDoc(
-      projectId,
-      docId,
-      {
-        lines: true,
-        rev: true,
-        deleted: true,
-        version: true,
-        ranges: true,
-        inS3: true,
-      },
-      (err, doc) => {
-        if (err) return callback(err)
-        if (doc == null) {
-          return callback(
-            new Errors.NotFoundError(
-              `No such doc: ${docId} in project ${projectId}`
-            )
-          )
-        }
-        if (doc && !doc.inS3) {
-          return callback(null, doc)
-        }
-        // skip the unarchiving to mongo when getting a doc
-        DocArchive.getDoc(projectId, docId, function (err, archivedDoc) {
-          if (err != null) {
-            logger.err(
-              { err, projectId, docId },
-              'error getting doc from archive'
-            )
-            return callback(err)
-          }
-          Object.assign(doc, archivedDoc)
-          callback(null, doc)
-        })
-      }
-    )
+  async _peekRawDoc(projectId, docId) {
+    const doc = await MongoManager.promises.findDoc(projectId, docId, {
+      lines: true,
+      rev: true,
+      deleted: true,
+      version: true,
+      ranges: true,
+      inS3: true,
+    })
+
+    if (doc == null) {
+      throw new Errors.NotFoundError(
+        `No such doc: ${docId} in project ${projectId}`
+      )
+    }
+
+    if (doc.inS3) {
+      // skip the unarchiving to mongo when getting a doc
+      const archivedDoc = await DocArchive.promises.getDoc(projectId, docId)
+      Object.assign(doc, archivedDoc)
+    }
+
+    return doc
   },
 
   // get the doc from mongo if possible, or from the persistent store otherwise,
   // without unarchiving it (avoids unnecessary writes to mongo)
-  peekDoc(projectId, docId, callback) {
-    DocManager._peekRawDoc(projectId, docId, (err, doc) => {
-      if (err) {
-        return callback(err)
-      }
-      MongoManager.withRevCheck(
-        doc,
-        MongoManager.getDocVersion,
-        function (error, version) {
-          // If the doc has been modified while we were retrieving it, we
-          // will get a DocModified error
-          if (error != null) {
-            return callback(error)
-          }
-          doc.version = version
-          return callback(err, doc)
-        }
-      )
-    })
+  async peekDoc(projectId, docId) {
+    const doc = await DocManager._peekRawDoc(projectId, docId)
+    const version = await MongoManager.promises.getDocVersion(docId)
+    await MongoManager.promises.checkRevUnchanged(doc)
+    doc.version = version
+    return doc
   },
 
-  getDocLines(projectId, docId, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    return DocManager._getDoc(
-      projectId,
-      docId,
-      { lines: true, inS3: true },
-      function (err, doc) {
-        if (err != null) {
-          return callback(err)
-        }
-        return callback(err, doc)
-      }
-    )
+  async getDocLines(projectId, docId) {
+    const doc = await DocManager._getDoc(projectId, docId, {
+      lines: true,
+      inS3: true,
+    })
+    return doc
   },
 
-  getAllDeletedDocs(projectId, filter, callback) {
-    MongoManager.getProjectsDeletedDocs(projectId, filter, callback)
+  async getAllDeletedDocs(projectId, filter) {
+    return await MongoManager.promises.getProjectsDeletedDocs(projectId, filter)
   },
 
-  getAllNonDeletedDocs(projectId, filter, callback) {
-    if (callback == null) {
-      callback = function () {}
+  async getAllNonDeletedDocs(projectId, filter) {
+    await DocArchive.promises.unArchiveAllDocs(projectId)
+    const docs = await MongoManager.promises.getProjectsDocs(
+      projectId,
+      { include_deleted: false },
+      filter
+    )
+    if (docs == null) {
+      throw new Errors.NotFoundError(`No docs for project ${projectId}`)
     }
-    return DocArchive.unArchiveAllDocs(projectId, function (error) {
-      if (error != null) {
-        return callback(error)
-      }
-      return MongoManager.getProjectsDocs(
-        projectId,
-        { include_deleted: false },
-        filter,
-        function (error, docs) {
-          if (typeof err !== 'undefined' && err !== null) {
-            return callback(error)
-          } else if (docs == null) {
-            return callback(
-              new Errors.NotFoundError(`No docs for project ${projectId}`)
-            )
-          } else {
-            return callback(null, docs)
-          }
-        }
-      )
-    })
+    return docs
   },
 
-  updateDoc(projectId, docId, lines, version, ranges, callback) {
-    DocManager._tryUpdateDoc(
-      projectId,
-      docId,
-      lines,
-      version,
-      ranges,
-      (err, modified, rev) => {
-        if (err && err instanceof Errors.DocRevValueError) {
+  async updateDoc(projectId, docId, lines, version, ranges) {
+    const MAX_ATTEMPTS = 2
+    for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) {
+      try {
+        const { modified, rev } = await DocManager._tryUpdateDoc(
+          projectId,
+          docId,
+          lines,
+          version,
+          ranges
+        )
+        return { modified, rev }
+      } catch (err) {
+        if (err instanceof Errors.DocRevValueError && attempt < MAX_ATTEMPTS) {
           // 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)
+          await setTimeout(100 + Math.random() * 100)
+          continue
         } else {
-          callback(err, modified, rev)
+          throw err
         }
       }
-    )
+    }
   },
 
-  _tryUpdateDoc(projectId, docId, lines, version, ranges, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  async _tryUpdateDoc(projectId, docId, lines, version, ranges) {
     if (lines == null || version == null || ranges == null) {
-      return callback(new Error('no lines, version or ranges provided'))
+      throw new Error('no lines, version or ranges provided')
     }
 
-    return DocManager._getDoc(
-      projectId,
-      docId,
-      {
+    let doc
+    try {
+      doc = await DocManager._getDoc(projectId, docId, {
         version: true,
         rev: true,
         lines: true,
-        version: true,
         ranges: true,
         inS3: true,
-      },
-      function (err, doc) {
-        let updateLines, updateRanges, updateVersion
-        if (err != null && !(err instanceof Errors.NotFoundError)) {
-          logger.err(
-            { projectId, docId, err },
-            'error getting document for update'
-          )
-          return callback(err)
-        }
-
-        ranges = RangeManager.jsonRangesToMongo(ranges)
+      })
+    } catch (err) {
+      if (err instanceof Errors.NotFoundError) {
+        doc = null
+      } else {
+        throw err
+      }
+    }
 
-        if (doc == null) {
-          // If the document doesn't exist, we'll make sure to create/update all parts of it.
-          updateLines = true
-          updateVersion = true
-          updateRanges = true
-        } else {
-          if (doc.version > version) {
-            // Reject update when the version was decremented.
-            // Potential reasons: racing flush, broken history.
-            return callback(
-              new Errors.DocVersionDecrementedError('rejecting stale update', {
-                updateVersion: version,
-                flushedVersion: doc.version,
-              })
-            )
-          }
-          updateLines = !_.isEqual(doc.lines, lines)
-          updateVersion = doc.version !== version
-          updateRanges = RangeManager.shouldUpdateRanges(doc.ranges, ranges)
-        }
+    ranges = RangeManager.jsonRangesToMongo(ranges)
 
-        let modified = false
-        let rev = (doc != null ? doc.rev : undefined) || 0
-
-        const updateLinesAndRangesIfNeeded = function (cb) {
-          if (updateLines || updateRanges) {
-            const update = {}
-            if (updateLines) {
-              update.lines = lines
-            }
-            if (updateRanges) {
-              update.ranges = ranges
-            }
-            logger.debug({ projectId, docId }, 'updating doc lines and ranges')
-
-            modified = true
-            rev += 1 // rev will be incremented in mongo by MongoManager.upsertIntoDocCollection
-            return MongoManager.upsertIntoDocCollection(
-              projectId,
-              docId,
-              doc?.rev,
-              update,
-              cb
-            )
-          } else {
-            logger.debug(
-              { projectId, docId },
-              'doc lines have not changed - not updating'
-            )
-            return cb()
-          }
-        }
+    let updateLines, updateRanges, updateVersion
+    if (doc == null) {
+      // If the document doesn't exist, we'll make sure to create/update all parts of it.
+      updateLines = true
+      updateVersion = true
+      updateRanges = true
+    } else {
+      if (doc.version > version) {
+        // Reject update when the version was decremented.
+        // Potential reasons: racing flush, broken history.
+        throw new Errors.DocVersionDecrementedError('rejecting stale update', {
+          updateVersion: version,
+          flushedVersion: doc.version,
+        })
+      }
+      updateLines = !_.isEqual(doc.lines, lines)
+      updateVersion = doc.version !== version
+      updateRanges = RangeManager.shouldUpdateRanges(doc.ranges, ranges)
+    }
 
-        const updateVersionIfNeeded = function (cb) {
-          if (updateVersion) {
-            logger.debug(
-              {
-                projectId,
-                docId,
-                oldVersion: doc != null ? doc.version : undefined,
-                newVersion: version,
-              },
-              'updating doc version'
-            )
-            modified = true
-            return MongoManager.setDocVersion(docId, version, cb)
-          } else {
-            logger.debug(
-              { projectId, docId, version },
-              'doc version has not changed - not updating'
-            )
-            return cb()
-          }
-        }
+    let modified = false
+    let rev = doc?.rev || 0
 
-        return updateLinesAndRangesIfNeeded(function (error) {
-          if (error != null) {
-            return callback(error)
-          }
-          return updateVersionIfNeeded(function (error) {
-            if (error != null) {
-              return callback(error)
-            }
-            return callback(null, modified, rev)
-          })
-        })
+    if (updateLines || updateRanges) {
+      const update = {}
+      if (updateLines) {
+        update.lines = lines
       }
-    )
+      if (updateRanges) {
+        update.ranges = ranges
+      }
+      logger.debug({ projectId, docId }, 'updating doc lines and ranges')
+
+      modified = true
+      rev += 1 // rev will be incremented in mongo by MongoManager.upsertIntoDocCollection
+      await MongoManager.promises.upsertIntoDocCollection(
+        projectId,
+        docId,
+        doc?.rev,
+        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'
+      )
+    }
+
+    return { modified, rev }
   },
 
-  patchDoc(projectId, docId, meta, callback) {
+  async patchDoc(projectId, docId, meta) {
     const projection = { _id: 1, deleted: true }
-    MongoManager.findDoc(projectId, docId, projection, (error, doc) => {
-      if (error != null) {
-        return callback(error)
-      }
-      if (!doc) {
-        return callback(
-          new Errors.NotFoundError(
-            `No such project/doc to delete: ${projectId}/${docId}`
-          )
-        )
-      }
+    const doc = await MongoManager.promises.findDoc(
+      projectId,
+      docId,
+      projection
+    )
+    if (!doc) {
+      throw new Errors.NotFoundError(
+        `No such project/doc to delete: ${projectId}/${docId}`
+      )
+    }
 
-      if (meta.deleted && Settings.docstore.archiveOnSoftDelete) {
-        // The user will not read this doc anytime soon. Flush it out of mongo.
-        DocArchive.archiveDoc(projectId, docId, err => {
-          if (err) {
-            logger.warn(
-              { projectId, docId, err },
-              'archiving a single doc in the background failed'
-            )
-          }
-        })
-      }
+    if (meta.deleted && Settings.docstore.archiveOnSoftDelete) {
+      // The user will not read this doc anytime soon. Flush it out of mongo.
+      DocArchive.promises.archiveDoc(projectId, docId).catch(err => {
+        logger.warn(
+          { projectId, docId, err },
+          'archiving a single doc in the background failed'
+        )
+      })
+    }
 
-      MongoManager.patchDoc(projectId, docId, meta, callback)
-    })
+    await MongoManager.promises.patchDoc(projectId, docId, meta)
   },
 }
+
+module.exports = {
+  ...callbackifyAll(DocManager, {
+    multiResult: {
+      updateDoc: ['modified', 'rev'],
+    },
+  }),
+  promises: DocManager,
+}

services/docstore/app/js/MongoManager.js

@@ -280,34 +280,34 @@ function getDocRev(docId, callback) {
   )
 }
 
-// Helper method  to support optimistic locking. Call the provided method for
-// an existing doc and return the result if the rev in mongo is unchanged when
-// checked afterwards. If the rev has changed, return a DocModifiedError.
-function withRevCheck(doc, method, callback) {
-  method(doc._id, function (err, result) {
+/**
+ * Helper method  to support optimistic locking.
+ *
+ * Check that the rev of an existing doc is unchanged. If the rev has
+ * changed, return a DocModifiedError.
+ */
+function checkRevUnchanged(doc, callback) {
+  getDocRev(doc._id, function (err, currentRev) {
     if (err) return callback(err)
-    getDocRev(doc._id, function (err, currentRev) {
-      if (err) return callback(err)
-      if (isNaN(currentRev) || isNaN(doc.rev)) {
-        return callback(
-          new Errors.DocRevValueError('doc rev is NaN', {
-            doc_id: doc._id,
-            rev: doc.rev,
-            currentRev,
-          })
-        )
-      }
-      if (doc.rev !== currentRev) {
-        return callback(
-          new Errors.DocModifiedError('doc rev has changed', {
-            doc_id: doc._id,
-            rev: doc.rev,
-            currentRev,
-          })
-        )
-      }
-      callback(null, result)
-    })
+    if (isNaN(currentRev) || isNaN(doc.rev)) {
+      return callback(
+        new Errors.DocRevValueError('doc rev is NaN', {
+          doc_id: doc._id,
+          rev: doc.rev,
+          currentRev,
+        })
+      )
+    }
+    if (doc.rev !== currentRev) {
+      return callback(
+        new Errors.DocModifiedError('doc rev has changed', {
+          doc_id: doc._id,
+          rev: doc.rev,
+          currentRev,
+        })
+      )
+    }
+    callback()
   })
 }
 
@@ -342,7 +342,7 @@ module.exports = {
   markDocAsArchived,
   getDocVersion,
   setDocVersion,
-  withRevCheck,
+  checkRevUnchanged,
   destroyProject,
 }
 

services/docstore/package.json

@@ -20,6 +20,7 @@
     "@overleaf/metrics": "*",
     "@overleaf/o-error": "*",
     "@overleaf/object-persistor": "*",
+    "@overleaf/promise-utils": "*",
     "@overleaf/settings": "*",
     "@overleaf/stream-utils": "^0.1.0",
     "async": "^3.2.2",

services/docstore/test/setup.js

@@ -3,6 +3,7 @@ const sinon = require('sinon')
 const sinonChai = require('sinon-chai')
 const chaiAsPromised = require('chai-as-promised')
 const SandboxedModule = require('sandboxed-module')
+const timersPromises = require('timers/promises')
 
 process.env.BACKEND = 'gcs'
 
@@ -29,6 +30,7 @@ const stubs = {
 SandboxedModule.configure({
   requires: {
     '@overleaf/logger': stubs.logger,
+    'timers/promises': timersPromises,
   },
   globals: { Buffer, JSON, Math, console, process },
 })

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

@@ -403,60 +403,42 @@ describe('MongoManager', function () {
     })
   })
 
-  describe('withRevCheck', function () {
+  describe('checkRevUnchanged', function () {
     this.beforeEach(function () {
       this.doc = { _id: ObjectId(), name: 'mock-doc', rev: 1 }
-      this.testFunction = sinon.stub().yields(null, 'foo')
     })
 
     it('should call the callback when the rev has not changed', function (done) {
       this.db.docs.findOne = sinon.stub().callsArgWith(2, null, { rev: 1 })
-      this.MongoManager.withRevCheck(
-        this.doc,
-        this.testFunction,
-        (err, result) => {
-          result.should.equal('foo')
-          assert.isNull(err)
-          done()
-        }
-      )
+      this.MongoManager.checkRevUnchanged(this.doc, err => {
+        assert.isUndefined(err)
+        done()
+      })
     })
 
     it('should return an error when the rev has changed', function (done) {
       this.db.docs.findOne = sinon.stub().callsArgWith(2, null, { rev: 2 })
-      this.MongoManager.withRevCheck(
-        this.doc,
-        this.testFunction,
-        (err, result) => {
-          err.should.be.instanceof(Errors.DocModifiedError)
-          done()
-        }
-      )
+      this.MongoManager.checkRevUnchanged(this.doc, err => {
+        err.should.be.instanceof(Errors.DocModifiedError)
+        done()
+      })
     })
 
     it('should return a value error if incoming rev is NaN', function (done) {
       this.db.docs.findOne = sinon.stub().callsArgWith(2, null, { rev: 2 })
       this.doc = { _id: ObjectId(), name: 'mock-doc', rev: NaN }
-      this.MongoManager.withRevCheck(
-        this.doc,
-        this.testFunction,
-        (err, result) => {
-          err.should.be.instanceof(Errors.DocRevValueError)
-          done()
-        }
-      )
+      this.MongoManager.checkRevUnchanged(this.doc, err => {
+        err.should.be.instanceof(Errors.DocRevValueError)
+        done()
+      })
     })
 
     it('should return a value error if checked doc rev is NaN', function (done) {
       this.db.docs.findOne = sinon.stub().callsArgWith(2, null, { rev: NaN })
-      this.MongoManager.withRevCheck(
-        this.doc,
-        this.testFunction,
-        (err, result) => {
-          err.should.be.instanceof(Errors.DocRevValueError)
-          done()
-        }
-      )
+      this.MongoManager.checkRevUnchanged(this.doc, err => {
+        err.should.be.instanceof(Errors.DocRevValueError)
+        done()
+      })
     })
   })