From ab17eb150d8e4f573ceec707543714c277c94127 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 12 Apr 2024 07:52:27 -0400 Subject: [PATCH] Merge pull request #17745 from overleaf/em-promisify-snapshot-manager Promisify SnapshotManager GitOrigin-RevId: 1fa7124da3aa3e0be5db372e68e286d63f496a97 --- package-lock.json | 2 - .../app/js/HistoryStoreManager.js | 13 +- .../project-history/app/js/SnapshotManager.js | 250 ++++++++---------- services/project-history/package.json | 1 - .../SnapshotManager/SnapshotManagerTests.js | 221 ++++++---------- 5 files changed, 202 insertions(+), 285 deletions(-) diff --git a/package-lock.json b/package-lock.json index 406f7d891c..dd4c4b8dbb 100644 --- a/package-lock.json +++ b/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", diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 16ba1ac79b..e0dc2d7b0c 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/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( diff --git a/services/project-history/app/js/SnapshotManager.js b/services/project-history/app/js/SnapshotManager.js index 80ef9b2c2e..95e69bdd1a 100644 --- a/services/project-history/app/js/SnapshotManager.js +++ b/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,162 +22,128 @@ 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) { - _getSnapshotAtVersion(projectId, version, (error, snapshot) => { - if (error) { - return callback(OError.tag(error)) - } - const file = snapshot.getFile(pathname) - if (file == null) { - error = new Errors.NotFoundError(`${pathname} not found`, { - projectId, - version, - pathname, - }) - return callback(error) - } +async function getFileSnapshotStream(projectId, version, pathname) { + const snapshot = await _getSnapshotAtVersion(projectId, version) - WebApiManager.getHistoryId(projectId, (err, historyId) => { - if (err) { - return callback(OError.tag(err)) - } - if (file.isEditable()) { - file - .load('eager', HistoryStoreManager.getBlobStore(historyId)) - .then(() => { - const stream = new StringStream() - stream.push(file.getContent({ filterTrackedDeletes: true })) - stream.push(null) - callback(null, stream) - }) - .catch(err => callback(err)) - } else { - HistoryStoreManager.getProjectBlobStream( - historyId, - file.getHash(), - callback - ) - } + const file = snapshot.getFile(pathname) + if (file == null) { + throw new Errors.NotFoundError(`${pathname} not found`, { + projectId, + version, + pathname, }) - }) + } + + const historyId = await WebApiManager.promises.getHistoryId(projectId) + if (file.isEditable()) { + await file.load('eager', HistoryStoreManager.getBlobStore(historyId)) + const stream = new StringStream() + stream.push(file.getContent({ filterTrackedDeletes: true })) + stream.push(null) + return stream + } else { + return await HistoryStoreManager.promises.getProjectBlobStream( + historyId, + file.getHash() + ) + } } // 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) { - _getSnapshotAtVersion(projectId, version, (error, snapshot) => { - if (error) { - return callback(OError.tag(error)) - } - WebApiManager.getHistoryId(projectId, (err, historyId) => { - if (err) { - return callback(OError.tag(err)) +async function getProjectSnapshot(projectId, version) { + const snapshot = await _getSnapshotAtVersion(projectId, version) + const historyId = await WebApiManager.promises.getHistoryId(projectId) + await _loadFilesLimit( + snapshot, + 'eager', + HistoryStoreManager.getBlobStore(historyId) + ) + return { + projectId, + files: snapshot.getFileMap().map(file => { + if (!file) { + return null } - _loadFilesLimit( - snapshot, - 'eager', - HistoryStoreManager.getBlobStore(historyId) - ) - .then(() => { - const data = { - projectId, - files: snapshot.getFileMap().map(file => { - if (!file) { - return null - } - const content = file.getContent({ - filterTrackedDeletes: true, - }) - if (content === null) { - return { data: { hash: file.getHash() } } - } - return { data: { content } } - }), - } - callback(null, data) - }) - .catch(callback) - }) - }) + const content = file.getContent({ + filterTrackedDeletes: true, + }) + if (content === null) { + return { data: { hash: file.getHash() } } + } + return { data: { content } } + }), + } } /** * * @param {string} projectId * @param {number} version - * @param {Function} callback */ -function _getSnapshotAtVersion(projectId, version, callback) { - WebApiManager.getHistoryId(projectId, (error, historyId) => { - if (error) { - return callback(OError.tag(error)) - } - HistoryStoreManager.getChunkAtVersion( - projectId, - historyId, - version, - (error, data) => { - if (error) { - return callback(OError.tag(error)) - } - const chunk = Core.Chunk.fromRaw(data.chunk) - const snapshot = chunk.getSnapshot() - const changes = chunk - .getChanges() - .slice(0, version - chunk.getStartVersion()) - snapshot.applyAll(changes) - callback(null, snapshot) - } - ) - }) -} - -export function getLatestSnapshot(projectId, historyId, callback) { - HistoryStoreManager.getMostRecentChunk(projectId, historyId, (err, data) => { - if (err) { - return callback(err) - } - if (data == null || data.chunk == null) { - return callback(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 - .loadFiles('lazy', HistoryStoreManager.getBlobStore(historyId)) - .then(snapshotFiles => callback(null, snapshotFiles)) - .catch(err => callback(err)) - }) -} - -function _loadFilesLimit(snapshot, kind, blobStore) { - // bluebird promises only support a limit on concurrency for map() - // 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) - }, - { concurrency: MAX_REQUESTS } +async function _getSnapshotAtVersion(projectId, version) { + const historyId = await WebApiManager.promises.getHistoryId(projectId) + const data = await HistoryStoreManager.promises.getChunkAtVersion( + projectId, + historyId, + version ) + const chunk = Core.Chunk.fromRaw(data.chunk) + const snapshot = chunk.getSnapshot() + const changes = chunk.getChanges().slice(0, version - chunk.getStartVersion()) + snapshot.applyAll(changes) + return snapshot +} + +async function getLatestSnapshot(projectId, historyId) { + const data = await HistoryStoreManager.promises.getMostRecentChunk( + projectId, + historyId + ) + if (data == null || data.chunk == null) { + 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) + const snapshotFiles = await snapshot.loadFiles( + 'lazy', + HistoryStoreManager.getBlobStore(historyId) + ) + return snapshotFiles +} + +async function _loadFilesLimit(snapshot, kind, blobStore) { + await snapshot.fileMap.mapAsync(async 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 + } + await file.load(kind, blobStore) + }, 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), - getProjectSnapshot: promisify(getProjectSnapshot), - getLatestSnapshot: promisify(getLatestSnapshot), + getFileSnapshotStream, + getProjectSnapshot, + getLatestSnapshot, } diff --git a/services/project-history/package.json b/services/project-history/package.json index 1def05f55a..4732474c1b 100644 --- a/services/project-history/package.json +++ b/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", diff --git a/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js b/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js index 4ecd9c759c..a71e1b12da 100644 --- a/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js +++ b/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(), - getChunkAtVersion: sinon.stub(), - getMostRecentChunk: sinon.stub(), - getProjectBlobStream: sinon.stub(), + promises: { + getChunkAtVersion: sinon.stub(), + getMostRecentChunk: sinon.stub(), + getProjectBlobStream: sinon.stub(), + }, } this.WebApiManager = { - getHistoryId: sinon.stub(), + 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', - (error, stream) => { - this.stream = stream - return done(error) - } + 'main.tex' ) }) 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) { - this.SnapshotManager.getFileSnapshotStream( - this.projectId, - 5, - 'main.tex', - error => { - expect(error).to.exist - expect(error.name).to.equal(this.error.name) - done() - } - ) + it('should call back with error', async function () { + await expect( + this.SnapshotManager.promises.getFileSnapshotStream( + this.projectId, + 5, + 'main.tex' + ) + ).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', - (error, stream) => { - this.stream = stream - done(error) - } + 'file_with_ranges.tex' ) }) 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) { - this.HistoryStoreManager.getProjectBlobStream + beforeEach(async function () { + this.HistoryStoreManager.promises.getProjectBlobStream .withArgs(this.historyId) - .yields(null, (this.stream = 'mock-stream')) - return this.SnapshotManager.getFileSnapshotStream( - this.projectId, - 5, - 'binary.png', - (error, returnedStream) => { - this.returnedStream = returnedStream - return done(error) - } - ) + .resolves((this.stream = 'mock-stream')) + this.returnedStream = + await this.SnapshotManager.promises.getFileSnapshotStream( + this.projectId, + 5, + 'binary.png' + ) }) 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 () { - return expect(this.returnedStream).to.equal(this.stream) + it('should return a stream with the blob content', function () { + expect(this.returnedStream).to.equal(this.stream) }) }) - return describe("when the file doesn't exist", function () { - beforeEach(function (done) { - return this.SnapshotManager.getFileSnapshotStream( - this.projectId, - 5, - 'not-here.png', - (error, returnedStream) => { - this.error = error - this.returnedStream = returnedStream - return done() - } - ) - }) - - 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("when the file doesn't exist", function () { + it('should return a NotFoundError', async function () { + await expect( + this.SnapshotManager.promises.getFileSnapshotStream( + this.projectId, + 5, + 'not-here.png' + ) + ).to.be.rejectedWith(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, - (error, data) => { - this.data = data - done(error) - } + 6 ) }) 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) { - this.SnapshotManager.getProjectSnapshot(this.projectId, 5, error => { - expect(error).to.exist - expect(error.message).to.equal(this.error.message) - - done() - }) + it('should call back with error', async function () { + expect( + this.SnapshotManager.promises.getProjectSnapshot(this.projectId, 5) + ).to.be.rejectedWith(this.error.message) }) }) }) - return describe('getLatestSnapshot', function () { + describe('getLatestSnapshot', function () { describe('for a project', function () { - beforeEach(function (done) { - this.HistoryStoreManager.getMostRecentChunk.yields(null, { + beforeEach(async function () { + 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, - (error, data) => { - this.data = data - done(error) - } + this.historyId ) }) 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 () { - beforeEach(function (done) { - this.HistoryStoreManager.getMostRecentChunk.yields(null) - return this.SnapshotManager.getLatestSnapshot( - this.projectId, - this.historyId, - (error, data) => { - this.error = error - this.data = data - return done() - } - ) - }) - - 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') + describe('when the chunk is empty', function () { + beforeEach(async function () { + this.HistoryStoreManager.promises.getMostRecentChunk.resolves(null) + expect( + this.SnapshotManager.promises.getLatestSnapshot( + this.projectId, + this.historyId + ) + ).to.be.rejectedWith('undefined chunk') }) }) })