From b750948e00b347f821fc7a594b90be91c37efe8d Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Mon, 10 Jul 2023 11:11:50 -0400 Subject: [PATCH] Merge pull request #13780 from overleaf/em-fetch-utils-project-history Use fetch-utils in project-history GitOrigin-RevId: 96afc5f3961210baa7ad597645b725fe2d8d3529 --- package-lock.json | 4 +- .../app/js/HistoryStoreManager.js | 86 ++++++++----------- services/project-history/package.json | 2 +- .../HistoryStoreManagerTests.js | 28 +++--- 4 files changed, 54 insertions(+), 66 deletions(-) diff --git a/package-lock.json b/package-lock.json index c6946f6caa..d7efb0a8f2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -40605,6 +40605,7 @@ "services/project-history": { "name": "@overleaf/project-history", "dependencies": { + "@overleaf/fetch-utils": "*", "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", @@ -40626,7 +40627,6 @@ "lodash": "^4.17.20", "mongo-uri": "^0.1.2", "mongodb": "^4.11.0", - "node-fetch": "^2.6.0", "overleaf-editor-core": "*", "redis": "~0.10.1", "request": "^2.88.2", @@ -49797,6 +49797,7 @@ "@overleaf/project-history": { "version": "file:services/project-history", "requires": { + "@overleaf/fetch-utils": "*", "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", @@ -49823,7 +49824,6 @@ "mongodb": "^4.11.0", "multer": "^1.4.2", "nock": "^12.0.3", - "node-fetch": "^2.6.0", "overleaf-editor-core": "*", "redis": "~0.10.1", "request": "^2.88.2", diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 72f71205ab..97947fa46c 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -7,11 +7,15 @@ import BPromise from 'bluebird' import { URL } from 'url' import OError from '@overleaf/o-error' import Settings from '@overleaf/settings' +import { + fetchStream, + fetchNothing, + RequestFailedError, +} from '@overleaf/fetch-utils' import * as Versions from './Versions.js' import * as Errors from './Errors.js' import * as LocalFileWriter from './LocalFileWriter.js' import * as HashManager from './HashManager.js' -import fetch from 'node-fetch' const HTTP_REQUEST_TIMEOUT = 300 * 1000 // 5 minutes @@ -174,17 +178,9 @@ export function getProjectBlobStream(historyId, blobHash, callback) { { historyId, blobHash }, 'getting blob stream from history service' ) - fetch(url, getHistoryFetchOptions()) - .then(res => { - if (!res.ok) { - const err = new OError( - `history store a non-success status code: ${res.status}` - ) - err.statusCode = res.status - logger.warn({ err, url }, 'cannot get project blob') - return callback(err) - } - callback(null, res.body) + fetchStream(url, getHistoryFetchOptions()) + .then(stream => { + callback(null, stream) }) .catch(err => callback(OError.tag(err))) } @@ -258,20 +254,22 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { } const fileId = urlMatch[2] const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileId}` - fetch(filestoreURL, { signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT) }) - .then(response => { - const statusCode = response.status - if (statusCode >= 200 && statusCode < 300) { - LocalFileWriter.bufferOnDisk( - response.body, - filestoreURL, - `project-${projectId}-file-${fileId}`, - (fsPath, cb) => { - _createBlob(historyId, fsPath, cb) - }, - callback - ) - } else if (statusCode === 404) { + fetchStream(filestoreURL, { + signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT), + }) + .then(stream => { + LocalFileWriter.bufferOnDisk( + stream, + filestoreURL, + `project-${projectId}-file-${fileId}`, + (fsPath, cb) => { + _createBlob(historyId, fsPath, cb) + }, + callback + ) + }) + .catch(err => { + if (err instanceof RequestFailedError && err.response.status === 404) { logger.warn( { projectId, historyId, filestoreURL }, 'File contents not found in filestore. Storing in history as an empty file' @@ -288,16 +286,9 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { ) emptyStream.push(null) // send an EOF signal } else { - const error = new OError( - `bad response from filestore: ${statusCode}`, - { filestoreURL, statusCode } - ) - callback(error) + callback(OError.tag(err, 'error from filestore', { filestoreURL })) } }) - .catch(err => - callback(OError.tag(err, 'error from filestore', { filestoreURL })) - ) } else { const error = new OError('invalid update for blob creation') callback(error) @@ -318,19 +309,17 @@ function _createBlob(historyId, fsPath, _callback) { 'sending blob to history service' ) const url = `${Settings.overleaf.history.host}/projects/${historyId}/blobs/${hash}` - fetch(url, { method: 'PUT', body: outStream, ...getHistoryFetchOptions() }) + fetchNothing(url, { + method: 'PUT', + body: outStream, + ...getHistoryFetchOptions(), + }) .then(res => { - if (!res.ok) { - const err = new OError( - `history store a non-success status code: ${res.status}` - ) - err.statusCode = res.status - logger.warn({ err, url }, 'cannot create project blob') - return callback(err) - } callback(null, hash) }) - .catch(err => callback(OError.tag(err))) + .catch(err => { + callback(OError.tag(err)) + }) }) } @@ -413,12 +402,9 @@ function _requestOptions(options) { function getHistoryFetchOptions() { return { signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT), - headers: { - Authorization: - 'Basic ' + - Buffer.from( - `${Settings.overleaf.history.user}:${Settings.overleaf.history.pass}` - ).toString('base64'), + basicAuth: { + user: Settings.overleaf.history.user, + password: Settings.overleaf.history.pass, }, } } diff --git a/services/project-history/package.json b/services/project-history/package.json index 39d0b80b51..6c2fc182a1 100644 --- a/services/project-history/package.json +++ b/services/project-history/package.json @@ -17,6 +17,7 @@ "lint:fix": "eslint --fix ." }, "dependencies": { + "@overleaf/fetch-utils": "*", "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", @@ -38,7 +39,6 @@ "lodash": "^4.17.20", "mongo-uri": "^0.1.2", "mongodb": "^4.11.0", - "node-fetch": "^2.6.0", "overleaf-editor-core": "*", "redis": "~0.10.1", "request": "^2.88.2", diff --git a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js index 174476cc6a..ccd85bb4db 100644 --- a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js +++ b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js @@ -2,6 +2,7 @@ import sinon from 'sinon' import { expect } from 'chai' import { strict as esmock } from 'esmock' import EventEmitter from 'events' +import { RequestFailedError } from '@overleaf/fetch-utils' import * as Errors from '../../../../app/js/Errors.js' const MODULE_PATH = '../../../../app/js/HistoryStoreManager.js' @@ -36,21 +37,29 @@ describe('HistoryStoreManager', function () { }) this.callback = sinon.stub() + this.LocalFileWriter = { bufferOnDisk: sinon.stub(), } + this.WebApiManager = { getHistoryId: sinon.stub(), } this.WebApiManager.getHistoryId .withArgs(this.projectId) .yields(null, this.historyId) + + this.FetchUtils = { + fetchStream: sinon.stub(), + fetchNothing: sinon.stub().resolves(), + RequestFailedError, + } + this.request = sinon.stub() this.request.get = sinon.stub() - this.fetch = sinon.stub().resolves() this.HistoryStoreManager = await esmock(MODULE_PATH, { - 'node-fetch': this.fetch, + '@overleaf/fetch-utils': this.FetchUtils, request: this.request, '@overleaf/settings': this.settings, '../../../../app/js/LocalFileWriter.js': this.LocalFileWriter, @@ -366,10 +375,7 @@ describe('HistoryStoreManager', function () { this.fileStream = {} this.hash = 'random-hash' this.LocalFileWriter.bufferOnDisk.callsArgWith(4, null, this.hash) - this.fetch.resolves({ - status: 200, - body: this.fileStream, - }) + this.FetchUtils.fetchStream.resolves(this.fileStream) }) describe('for a file update with any filestore location', function () { @@ -394,7 +400,7 @@ describe('HistoryStoreManager', function () { }) it('should request the file from the filestore in settings', function () { - expect(this.fetch).to.have.been.calledWithMatch( + expect(this.FetchUtils.fetchStream).to.have.been.calledWithMatch( `${this.settings.apis.filestore.url}/project/${this.projectId}/file/${this.file_id}` ) }) @@ -467,11 +473,7 @@ describe('HistoryStoreManager', function () { this.historyResponse = new EventEmitter() this.blobHash = 'test hash' - this.fetch.resolves({ - status: 200, - ok: true, - body: this.historyResponse, - }) + this.FetchUtils.fetchStream.resolves(this.historyResponse) this.HistoryStoreManager.getProjectBlobStream( this.historyId, this.blobHash, @@ -486,7 +488,7 @@ describe('HistoryStoreManager', function () { }) it('should get the blob from the overleaf history service', function () { - expect(this.fetch).to.have.been.calledWithMatch( + expect(this.FetchUtils.fetchStream).to.have.been.calledWithMatch( `${this.settings.overleaf.history.host}/projects/${this.historyId}/blobs/${this.blobHash}` ) })