Merge pull request #17745 from overleaf/em-promisify-snapshot-manager

Promisify SnapshotManager

GitOrigin-RevId: 1fa7124da3aa3e0be5db372e68e286d63f496a97

package-lock.json

@@ -43042,7 +43042,6 @@
         "@overleaf/settings": "*",
         "async": "^3.2.2",
         "aws-sdk": "^2.650.0",
-        "bluebird": "^3.7.2",
         "body-parser": "^1.19.0",
         "bunyan": "^1.8.15",
         "byline": "^4.2.1",
@@ -51641,7 +51640,6 @@
         "@overleaf/settings": "*",
         "async": "^3.2.2",
         "aws-sdk": "^2.650.0",
-        "bluebird": "^3.7.2",
         "body-parser": "^1.19.0",
         "bunyan": "^1.8.15",
         "byline": "^4.2.1",

services/project-history/app/js/HistoryStoreManager.js

@@ -36,10 +36,16 @@ _mocks.getMostRecentChunk = (projectId, historyId, callback) => {
   _requestChunk({ path, json: true }, callback)
 }
 
-export function getMostRecentChunk(...args) {
+/**
-  _mocks.getMostRecentChunk(...args)
+ * @param {Callback} callback
+ */
+export function getMostRecentChunk(projectId, historyId, callback) {
+  _mocks.getMostRecentChunk(projectId, historyId, callback)
 }
 
+/**
+ * @param {Callback} callback
+ */
 export function getChunkAtVersion(projectId, historyId, version, callback) {
   const path = `projects/${historyId}/versions/${version}/history`
   logger.debug(
@@ -172,6 +178,9 @@ export function getProjectBlob(historyId, blobHash, callback) {
   )
 }
 
+/**
+ * @param {Callback} callback
+ */
 export function getProjectBlobStream(historyId, blobHash, callback) {
   const url = `${Settings.overleaf.history.host}/projects/${historyId}/blobs/${blobHash}`
   logger.debug(

services/project-history/app/js/SnapshotManager.js

@@ -1,15 +1,17 @@
 // @ts-check
 
-import { promisify } from 'util'
+import { callbackify } from 'util'
 import Core from 'overleaf-editor-core'
 import { Readable as StringStream } from 'stream'
-import BPromise from 'bluebird'
 import OError from '@overleaf/o-error'
 import * as HistoryStoreManager from './HistoryStoreManager.js'
 import * as WebApiManager from './WebApiManager.js'
 import * as Errors from './Errors.js'
 
-/** @typedef {import('overleaf-editor-core').Snapshot} Snapshot */
+/**
+ * @typedef {import('stream').Readable} ReadableStream
+ * @typedef {import('overleaf-editor-core').Snapshot} Snapshot
+ */
 
 StringStream.prototype._read = function () {}
 
@@ -20,67 +22,46 @@ const MAX_REQUESTS = 4 // maximum number of parallel requests to v1 history serv
  * @param {string} projectId
  * @param {number} version
  * @param {string} pathname
- * @param {Function} callback
  */
-export function getFileSnapshotStream(projectId, version, pathname, callback) {
+async function getFileSnapshotStream(projectId, version, pathname) {
-  _getSnapshotAtVersion(projectId, version, (error, snapshot) => {
+  const snapshot = await _getSnapshotAtVersion(projectId, version)
-    if (error) {
+
-      return callback(OError.tag(error))
-    }
   const file = snapshot.getFile(pathname)
   if (file == null) {
-      error = new Errors.NotFoundError(`${pathname} not found`, {
+    throw new Errors.NotFoundError(`${pathname} not found`, {
       projectId,
       version,
       pathname,
     })
-      return callback(error)
   }
 
-    WebApiManager.getHistoryId(projectId, (err, historyId) => {
+  const historyId = await WebApiManager.promises.getHistoryId(projectId)
-      if (err) {
-        return callback(OError.tag(err))
-      }
   if (file.isEditable()) {
-        file
+    await file.load('eager', HistoryStoreManager.getBlobStore(historyId))
-          .load('eager', HistoryStoreManager.getBlobStore(historyId))
-          .then(() => {
     const stream = new StringStream()
     stream.push(file.getContent({ filterTrackedDeletes: true }))
     stream.push(null)
-            callback(null, stream)
+    return stream
-          })
-          .catch(err => callback(err))
   } else {
-        HistoryStoreManager.getProjectBlobStream(
+    return await HistoryStoreManager.promises.getProjectBlobStream(
       historyId,
-          file.getHash(),
+      file.getHash()
-          callback
     )
   }
-    })
-  })
 }
 
 // Returns project snapshot containing the document content for files with
 // text operations in the relevant chunk, and hashes for unmodified/binary
 // files. Used by git bridge to get the state of the project.
-export function getProjectSnapshot(projectId, version, callback) {
+async function getProjectSnapshot(projectId, version) {
-  _getSnapshotAtVersion(projectId, version, (error, snapshot) => {
+  const snapshot = await _getSnapshotAtVersion(projectId, version)
-    if (error) {
+  const historyId = await WebApiManager.promises.getHistoryId(projectId)
-      return callback(OError.tag(error))
+  await _loadFilesLimit(
-    }
-    WebApiManager.getHistoryId(projectId, (err, historyId) => {
-      if (err) {
-        return callback(OError.tag(err))
-      }
-      _loadFilesLimit(
     snapshot,
     'eager',
     HistoryStoreManager.getBlobStore(historyId)
   )
-        .then(() => {
+  return {
-          const data = {
     projectId,
     files: snapshot.getFileMap().map(file => {
       if (!file) {
@@ -95,87 +76,74 @@ export function getProjectSnapshot(projectId, version, callback) {
       return { data: { content } }
     }),
   }
-          callback(null, data)
-        })
-        .catch(callback)
-    })
-  })
 }
 
 /**
  *
  * @param {string} projectId
  * @param {number} version
- * @param {Function} callback
  */
-function _getSnapshotAtVersion(projectId, version, callback) {
+async function _getSnapshotAtVersion(projectId, version) {
-  WebApiManager.getHistoryId(projectId, (error, historyId) => {
+  const historyId = await WebApiManager.promises.getHistoryId(projectId)
-    if (error) {
+  const data = await HistoryStoreManager.promises.getChunkAtVersion(
-      return callback(OError.tag(error))
-    }
-    HistoryStoreManager.getChunkAtVersion(
     projectId,
     historyId,
-      version,
+    version
-      (error, data) => {
+  )
-        if (error) {
-          return callback(OError.tag(error))
-        }
   const chunk = Core.Chunk.fromRaw(data.chunk)
   const snapshot = chunk.getSnapshot()
-        const changes = chunk
+  const changes = chunk.getChanges().slice(0, version - chunk.getStartVersion())
-          .getChanges()
-          .slice(0, version - chunk.getStartVersion())
   snapshot.applyAll(changes)
-        callback(null, snapshot)
+  return snapshot
-      }
-    )
-  })
 }
 
-export function getLatestSnapshot(projectId, historyId, callback) {
+async function getLatestSnapshot(projectId, historyId) {
-  HistoryStoreManager.getMostRecentChunk(projectId, historyId, (err, data) => {
+  const data = await HistoryStoreManager.promises.getMostRecentChunk(
-    if (err) {
+    projectId,
-      return callback(err)
+    historyId
-    }
+  )
   if (data == null || data.chunk == null) {
-      return callback(new OError('undefined chunk'))
+    throw new OError('undefined chunk')
   }
+
   // apply all the changes in the chunk to get the current snapshot
   const chunk = Core.Chunk.fromRaw(data.chunk)
   const snapshot = chunk.getSnapshot()
   const changes = chunk.getChanges()
   snapshot.applyAll(changes)
-    snapshot
+  const snapshotFiles = await snapshot.loadFiles(
-      .loadFiles('lazy', HistoryStoreManager.getBlobStore(historyId))
+    'lazy',
-      .then(snapshotFiles => callback(null, snapshotFiles))
+    HistoryStoreManager.getBlobStore(historyId)
-      .catch(err => callback(err))
+  )
-  })
+  return snapshotFiles
 }
 
-function _loadFilesLimit(snapshot, kind, blobStore) {
+async function _loadFilesLimit(snapshot, kind, blobStore) {
-  // bluebird promises only support a limit on concurrency for map()
+  await snapshot.fileMap.mapAsync(async file => {
-  // so make an array of the files we need to load
-  const fileList = []
-  snapshot.fileMap.map(file => fileList.push(file))
-  // load the files in parallel with a limit on the concurrent requests
-  return BPromise.map(
-    fileList,
-    file => {
     // only load changed files or files with tracked changes, others can be
     // dereferenced from their blobs (this method is only used by the git
     // bridge which understands how to load blobs).
     if (!file.isEditable() || (file.getHash() && !file.getRangesHash())) {
       return
     }
-      return file.load(kind, blobStore)
+    await file.load(kind, blobStore)
-    },
+  }, MAX_REQUESTS)
-    { concurrency: MAX_REQUESTS }
+}
-  )
+
+// EXPORTS
+
+const getFileSnapshotStreamCb = callbackify(getFileSnapshotStream)
+const getProjectSnapshotCb = callbackify(getProjectSnapshot)
+const getLatestSnapshotCb = callbackify(getLatestSnapshot)
+
+export {
+  getFileSnapshotStreamCb as getFileSnapshotStream,
+  getProjectSnapshotCb as getProjectSnapshot,
+  getLatestSnapshotCb as getLatestSnapshot,
 }
 
 export const promises = {
-  getFileSnapshotStream: promisify(getFileSnapshotStream),
+  getFileSnapshotStream,
-  getProjectSnapshot: promisify(getProjectSnapshot),
+  getProjectSnapshot,
-  getLatestSnapshot: promisify(getLatestSnapshot),
+  getLatestSnapshot,
 }

services/project-history/package.json

@@ -27,7 +27,6 @@
     "@overleaf/settings": "*",
     "async": "^3.2.2",
     "aws-sdk": "^2.650.0",
-    "bluebird": "^3.7.2",
     "body-parser": "^1.19.0",
     "bunyan": "^1.8.15",
     "byline": "^4.2.1",

services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js

@@ -1,20 +1,7 @@
-/* eslint-disable
-    no-return-assign,
-    no-undef,
-    no-unused-vars,
-*/
-// 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
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 import sinon from 'sinon'
 import { expect } from 'chai'
 import { strict as esmock } from 'esmock'
 import Core from 'overleaf-editor-core'
-import BPromise from 'bluebird'
 import * as Errors from '../../../../app/js/Errors.js'
 
 const MODULE_PATH = '../../../../app/js/SnapshotManager.js'
@@ -23,12 +10,16 @@ describe('SnapshotManager', function () {
   beforeEach(async function () {
     this.HistoryStoreManager = {
       getBlobStore: sinon.stub(),
+      promises: {
         getChunkAtVersion: sinon.stub(),
         getMostRecentChunk: sinon.stub(),
         getProjectBlobStream: sinon.stub(),
+      },
     }
     this.WebApiManager = {
+      promises: {
         getHistoryId: sinon.stub(),
+      },
     }
     this.SnapshotManager = await esmock(MODULE_PATH, {
       'overleaf-editor-core': Core,
@@ -38,12 +29,12 @@ describe('SnapshotManager', function () {
     })
     this.projectId = 'project-id-123'
     this.historyId = 'ol-project-id-123'
-    return (this.callback = sinon.stub())
+    this.callback = sinon.stub()
   })
 
   describe('getFileSnapshotStream', function () {
     beforeEach(function () {
-      this.WebApiManager.getHistoryId.yields(null, this.historyId)
+      this.WebApiManager.promises.getHistoryId.resolves(this.historyId)
       this.ranges = {
         comments: [],
         trackedChanges: [
@@ -65,7 +56,7 @@ describe('SnapshotManager', function () {
           },
         ],
       }
-      this.HistoryStoreManager.getChunkAtVersion.yields(null, {
+      this.HistoryStoreManager.promises.getChunkAtVersion.resolves({
         chunk: {
           history: {
             snapshot: {
@@ -121,7 +112,7 @@ describe('SnapshotManager', function () {
     })
 
     describe('of a text file with no tracked changes', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
         this.HistoryStoreManager.getBlobStore.withArgs(this.historyId).returns({
           getString: (this.getString = sinon.stub().resolves(
             `\
@@ -134,37 +125,33 @@ Four five six\
           )),
           getObject: sinon.stub().rejects(),
         })
-        this.SnapshotManager.getFileSnapshotStream(
+        this.stream = await this.SnapshotManager.promises.getFileSnapshotStream(
           this.projectId,
           5,
-          'main.tex',
+          'main.tex'
-          (error, stream) => {
-            this.stream = stream
-            return done(error)
-          }
         )
       })
 
       it('should get the overleaf id', function () {
-        return this.WebApiManager.getHistoryId
+        this.WebApiManager.promises.getHistoryId
           .calledWith(this.projectId)
           .should.equal(true)
       })
 
       it('should get the chunk', function () {
-        return this.HistoryStoreManager.getChunkAtVersion
+        this.HistoryStoreManager.promises.getChunkAtVersion
           .calledWith(this.projectId, this.historyId, 5)
           .should.equal(true)
       })
 
       it('should get the blob of the starting snapshot', function () {
-        return this.getString
+        this.getString
           .calledWith('35c9bd86574d61dcadbce2fdd3d4a0684272c6ea')
           .should.equal(true)
       })
 
       it('should return a string stream with the text content', function () {
-        return expect(this.stream.read().toString()).to.equal(
+        expect(this.stream.read().toString()).to.equal(
           `\
 Hello world
 
@@ -188,48 +175,41 @@ Seven eight nine\
             })
         })
 
-        it('should call back with error', function (done) {
+        it('should call back with error', async function () {
-          this.SnapshotManager.getFileSnapshotStream(
+          await expect(
+            this.SnapshotManager.promises.getFileSnapshotStream(
               this.projectId,
               5,
-            'main.tex',
+              'main.tex'
-            error => {
-              expect(error).to.exist
-              expect(error.name).to.equal(this.error.name)
-              done()
-            }
             )
+          ).to.be.rejectedWith(this.error)
         })
       })
     })
 
     describe('of a text file with tracked changes', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
         this.HistoryStoreManager.getBlobStore.withArgs(this.historyId).returns({
           getString: (this.getString = sinon
             .stub()
             .resolves('the quick brown fox jumps over the lazy dog')),
           getObject: (this.getObject = sinon.stub().resolves(this.ranges)),
         })
-        this.SnapshotManager.getFileSnapshotStream(
+        this.stream = await this.SnapshotManager.promises.getFileSnapshotStream(
           this.projectId,
           5,
-          'file_with_ranges.tex',
+          'file_with_ranges.tex'
-          (error, stream) => {
-            this.stream = stream
-            done(error)
-          }
         )
       })
 
       it('should get the overleaf id', function () {
-        this.WebApiManager.getHistoryId
+        this.WebApiManager.promises.getHistoryId
           .calledWith(this.projectId)
           .should.equal(true)
       })
 
       it('should get the chunk', function () {
-        this.HistoryStoreManager.getChunkAtVersion
+        this.HistoryStoreManager.promises.getChunkAtVersion
           .calledWith(this.projectId, this.historyId, 5)
           .should.equal(true)
       })
@@ -254,35 +234,32 @@ Seven eight nine\
     })
 
     describe('of a binary file', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
-        this.HistoryStoreManager.getProjectBlobStream
+        this.HistoryStoreManager.promises.getProjectBlobStream
           .withArgs(this.historyId)
-          .yields(null, (this.stream = 'mock-stream'))
+          .resolves((this.stream = 'mock-stream'))
-        return this.SnapshotManager.getFileSnapshotStream(
+        this.returnedStream =
+          await this.SnapshotManager.promises.getFileSnapshotStream(
             this.projectId,
             5,
-          'binary.png',
+            'binary.png'
-          (error, returnedStream) => {
-            this.returnedStream = returnedStream
-            return done(error)
-          }
           )
       })
 
       it('should get the overleaf id', function () {
-        return this.WebApiManager.getHistoryId
+        this.WebApiManager.promises.getHistoryId
           .calledWith(this.projectId)
           .should.equal(true)
       })
 
       it('should get the chunk', function () {
-        return this.HistoryStoreManager.getChunkAtVersion
+        this.HistoryStoreManager.promises.getChunkAtVersion
           .calledWith(this.projectId, this.historyId, 5)
           .should.equal(true)
       })
 
       it('should get the blob of the starting snapshot', function () {
-        return this.HistoryStoreManager.getProjectBlobStream
+        this.HistoryStoreManager.promises.getProjectBlobStream
           .calledWith(
             this.historyId,
             'c6654ea913979e13e22022653d284444f284a172'
@@ -290,36 +267,27 @@ Seven eight nine\
           .should.equal(true)
       })
 
-      return it('should return a stream with the blob content', function () {
+      it('should return a stream with the blob content', function () {
-        return expect(this.returnedStream).to.equal(this.stream)
+        expect(this.returnedStream).to.equal(this.stream)
       })
     })
 
-    return describe("when the file doesn't exist", function () {
+    describe("when the file doesn't exist", function () {
-      beforeEach(function (done) {
+      it('should return a NotFoundError', async function () {
-        return this.SnapshotManager.getFileSnapshotStream(
+        await expect(
+          this.SnapshotManager.promises.getFileSnapshotStream(
             this.projectId,
             5,
-          'not-here.png',
+            'not-here.png'
-          (error, returnedStream) => {
-            this.error = error
-            this.returnedStream = returnedStream
-            return done()
-          }
           )
-      })
+        ).to.be.rejectedWith(Errors.NotFoundError)
-
-      return it('should return a NotFoundError', function () {
-        expect(this.error).to.exist
-        expect(this.error.message).to.equal('not-here.png not found')
-        return expect(this.error).to.be.an.instanceof(Errors.NotFoundError)
       })
     })
   })
 
   describe('getProjectSnapshot', function () {
     beforeEach(function () {
-      this.WebApiManager.getHistoryId.yields(null, this.historyId)
+      this.WebApiManager.promises.getHistoryId.resolves(this.historyId)
       this.ranges = {
         comments: [],
         trackedChanges: [
@@ -341,7 +309,7 @@ Seven eight nine\
           },
         ],
       }
-      return this.HistoryStoreManager.getChunkAtVersion.yields(null, {
+      this.HistoryStoreManager.promises.getChunkAtVersion.resolves({
         chunk: (this.chunk = {
           history: {
             snapshot: {
@@ -416,7 +384,7 @@ Seven eight nine\
     })
 
     describe('of project', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
         this.HistoryStoreManager.getBlobStore.withArgs(this.historyId).returns({
           getString: (this.getString = sinon.stub().resolves(
             `\
@@ -429,24 +397,20 @@ Four five six\
           )),
           getObject: (this.getObject = sinon.stub().resolves(this.ranges)),
         })
-        this.SnapshotManager.getProjectSnapshot(
+        this.data = await this.SnapshotManager.promises.getProjectSnapshot(
           this.projectId,
-          6,
+          6
-          (error, data) => {
-            this.data = data
-            done(error)
-          }
         )
       })
 
       it('should get the overleaf id', function () {
-        this.WebApiManager.getHistoryId
+        this.WebApiManager.promises.getHistoryId
           .calledWith(this.projectId)
           .should.equal(true)
       })
 
       it('should get the chunk', function () {
-        this.HistoryStoreManager.getChunkAtVersion
+        this.HistoryStoreManager.promises.getChunkAtVersion
           .calledWith(this.projectId, this.historyId, 6)
           .should.equal(true)
       })
@@ -507,21 +471,18 @@ Four five six\
         })
       })
 
-      it('should call back with error', function (done) {
+      it('should call back with error', async function () {
-        this.SnapshotManager.getProjectSnapshot(this.projectId, 5, error => {
+        expect(
-          expect(error).to.exist
+          this.SnapshotManager.promises.getProjectSnapshot(this.projectId, 5)
-          expect(error.message).to.equal(this.error.message)
+        ).to.be.rejectedWith(this.error.message)
-
-          done()
-        })
       })
     })
   })
 
-  return describe('getLatestSnapshot', function () {
+  describe('getLatestSnapshot', function () {
     describe('for a project', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
-        this.HistoryStoreManager.getMostRecentChunk.yields(null, {
+        this.HistoryStoreManager.promises.getMostRecentChunk.resolves({
           chunk: (this.chunk = {
             history: {
               snapshot: {
@@ -582,57 +543,39 @@ Four five six\
           )),
           getObject: sinon.stub().rejects(),
         })
-        this.SnapshotManager.getLatestSnapshot(
+        this.data = await this.SnapshotManager.promises.getLatestSnapshot(
           this.projectId,
-          this.historyId,
+          this.historyId
-          (error, data) => {
-            this.data = data
-            done(error)
-          }
         )
       })
 
       it('should get the chunk', function () {
-        return this.HistoryStoreManager.getMostRecentChunk
+        this.HistoryStoreManager.promises.getMostRecentChunk
           .calledWith(this.projectId, this.historyId)
           .should.equal(true)
       })
 
-      return it('should produce the snapshot file data', function () {
+      it('should produce the snapshot file data', function () {
         expect(this.data).to.have.all.keys(['main.tex', 'binary.png'])
         expect(this.data['main.tex']).to.exist
         expect(this.data['binary.png']).to.exist
         expect(this.data['main.tex'].getStringLength()).to.equal(59)
         expect(this.data['binary.png'].getByteLength()).to.equal(41)
-        return expect(this.data['binary.png'].getHash()).to.equal(
+        expect(this.data['binary.png'].getHash()).to.equal(
           'c6654ea913979e13e22022653d284444f284a172'
         )
       })
     })
 
-    return describe('when the chunk is empty', function () {
+    describe('when the chunk is empty', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
-        this.HistoryStoreManager.getMostRecentChunk.yields(null)
+        this.HistoryStoreManager.promises.getMostRecentChunk.resolves(null)
-        return this.SnapshotManager.getLatestSnapshot(
+        expect(
+          this.SnapshotManager.promises.getLatestSnapshot(
             this.projectId,
-          this.historyId,
+            this.historyId
-          (error, data) => {
-            this.error = error
-            this.data = data
-            return done()
-          }
           )
-      })
+        ).to.be.rejectedWith('undefined chunk')
-
-      it('should get the chunk', function () {
-        return this.HistoryStoreManager.getMostRecentChunk
-          .calledWith(this.projectId, this.historyId)
-          .should.equal(true)
-      })
-
-      return it('return an error', function () {
-        expect(this.error).to.exist
-        return expect(this.error.message).to.equal('undefined chunk')
       })
     })
   })