From 14b04ad4b8d4bb85d6dcb90d57fd9f9680057b4b Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Wed, 27 May 2026 12:40:17 +0200 Subject: [PATCH] [project-history] Removed `request` dependency (#32686) * [project-history] Removed `request` dependency GitOrigin-RevId: 086bbbf2efeea6026127653a1f68ca6bf0476de6 --- .../app/js/HistoryStoreManager.js | 90 ++++----- services/project-history/package.json | 3 +- .../HistoryStoreManagerTests.js | 171 ++++++++++-------- .../js/HttpController/HttpControllerTests.js | 2 - .../unit/js/RetryManager/RetryManagerTests.js | 2 - yarn.lock | 1 - 6 files changed, 138 insertions(+), 131 deletions(-) diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 96afac59a9..664f8decbb 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -1,6 +1,5 @@ import { promisify } from 'node:util' import fs from 'node:fs' -import request from 'request' import stream from 'node:stream' import logger from '@overleaf/logger' import _ from 'lodash' @@ -9,6 +8,7 @@ import OError from '@overleaf/o-error' import Settings from '@overleaf/settings' import { fetchStream, + fetchString, fetchNothing, RequestFailedError, } from '@overleaf/fetch-utils' @@ -574,33 +574,6 @@ export function getBlobStore(projectId) { return new BlobStore(projectId) } -function _requestOptions(options) { - const requestOptions = { - method: options.method || 'GET', - url: `${Settings.overleaf.history.host}/${options.path}`, - timeout: HTTP_REQUEST_TIMEOUT, - auth: { - user: Settings.overleaf.history.user, - pass: Settings.overleaf.history.pass, - sendImmediately: true, - }, - } - - if (options.json != null) { - requestOptions.json = options.json - } - - if (options.body != null) { - requestOptions.body = options.body - } - - if (options.qs != null) { - requestOptions.qs = options.qs - } - - return requestOptions -} - /** * @return {RequestInit} */ @@ -614,25 +587,54 @@ function getHistoryFetchOptions() { } } -function _requestHistoryService(options, callback) { - const requestOptions = _requestOptions(options) - request(requestOptions, (error, res, body) => { - if (error) { - return callback(OError.tag(error)) +function _buildUrl(options) { + const url = new URL(`${Settings.overleaf.history.host}/${options.path}`) + if (options.qs) { + for (const [key, value] of Object.entries(options.qs)) { + url.searchParams.set(key, String(value)) } + } + return url.href +} - if (res.statusCode >= 200 && res.statusCode < 300) { - callback(null, body) - } else { - const { method, url, qs } = requestOptions - error = new OError( - // Keep the status code in the message. It is used by the ErrorRecorder. - `history store a non-success status code: ${res.statusCode}`, - { method, url, qs, statusCode: res.statusCode } - ) - callback(error) +function _requestHistoryService(options, callback) { + const url = _buildUrl(options) + const method = options.method || 'GET' + const fetchOptions = { + method, + ...getHistoryFetchOptions(), + } + + if (options.json != null && options.json !== true) { + fetchOptions.json = options.json + } + + if (options.body != null) { + fetchOptions.body = options.body + } + + const useJson = options.json != null + + fetchString(url, fetchOptions).then( + body => { + if (useJson && body) { + callback(null, JSON.parse(body)) + } else { + callback(null, body) + } + }, + err => { + if (err instanceof RequestFailedError) { + const error = new OError( + // Keep the status code in the message. It is used by the ErrorRecorder. + `history store a non-success status code: ${err.response.status}`, + { method, url, qs: options.qs, statusCode: err.response.status } + ) + return callback(error) + } + callback(OError.tag(err)) } - }) + ) } export const cloneProject = callbackify(_cloneProject) diff --git a/services/project-history/package.json b/services/project-history/package.json index 1aa2cb8b1e..2eb92985b8 100644 --- a/services/project-history/package.json +++ b/services/project-history/package.json @@ -36,8 +36,7 @@ "minimist": "^1.2.8", "mongodb-legacy": "6.1.3", "overleaf-editor-core": "workspace:*", - "p-queue": "^8.1.0", - "request": "2.88.2" + "p-queue": "^8.1.0" }, "devDependencies": { "@overleaf/migrations": "workspace:*", diff --git a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js index db5b87d65c..3a6771c053 100644 --- a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js +++ b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js @@ -27,17 +27,6 @@ describe('HistoryStoreManager', function () { }, }, } - this.latestChunkRequestArgs = sinon.match({ - method: 'GET', - url: `${this.settings.overleaf.history.host}/projects/${this.historyId}/latest/history`, - json: true, - auth: { - user: this.settings.overleaf.history.user, - pass: this.settings.overleaf.history.pass, - sendImmediately: true, - }, - }) - this.callback = sinon.stub() this.LocalFileWriter = { @@ -53,12 +42,11 @@ describe('HistoryStoreManager', function () { this.FetchUtils = { fetchStream: sinon.stub(), + fetchString: sinon.stub(), fetchNothing: sinon.stub().resolves(), RequestFailedError, } - this.request = sinon.stub() - this.logger = { debug: sinon.stub(), warn: sinon.stub(), @@ -66,7 +54,6 @@ describe('HistoryStoreManager', function () { this.HistoryStoreManager = await esmock(MODULE_PATH, { '@overleaf/fetch-utils': this.FetchUtils, - request: this.request, '@overleaf/settings': this.settings, '../../../../app/js/LocalFileWriter.js': this.LocalFileWriter, '../../../../app/js/WebApiManager.js': this.WebApiManager, @@ -77,7 +64,7 @@ describe('HistoryStoreManager', function () { describe('getMostRecentChunk', function () { describe('successfully', function () { - beforeEach(function () { + beforeEach(function (done) { this.chunk = { chunk: { startVersion: 0, @@ -89,13 +76,27 @@ describe('HistoryStoreManager', function () { }, }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.chunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.chunk)) this.HistoryStoreManager.getMostRecentChunk( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } + ) + }) + + it('should request latest history from the overleaf history service', function () { + expect(this.FetchUtils.fetchString).to.have.been.calledWithMatch( + `${this.settings.overleaf.history.host}/projects/${this.historyId}/latest/history`, + { + method: 'GET', + basicAuth: { + user: this.settings.overleaf.history.user, + password: this.settings.overleaf.history.pass, + }, + } ) }) @@ -107,7 +108,7 @@ describe('HistoryStoreManager', function () { describe('getMostRecentVersion', function () { describe('successfully', function () { - beforeEach(function () { + beforeEach(function (done) { this.chunk = { chunk: { startVersion: 5, @@ -123,13 +124,14 @@ describe('HistoryStoreManager', function () { }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.chunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.chunk)) this.HistoryStoreManager.getMostRecentVersion( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) @@ -141,10 +143,23 @@ describe('HistoryStoreManager', function () { { v2Authors: ['5678'], timestamp: '2017-10-17T10:44:40.227Z' } ) }) + + it('should request latest history from the overleaf history service', function () { + expect(this.FetchUtils.fetchString).to.have.been.calledWithMatch( + `${this.settings.overleaf.history.host}/projects/${this.historyId}/latest/history`, + { + method: 'GET', + basicAuth: { + user: this.settings.overleaf.history.user, + password: this.settings.overleaf.history.pass, + }, + } + ) + }) }) describe('out of order doc ops', function () { - beforeEach(function () { + beforeEach(function (done) { this.chunk = { chunk: { startVersion: 5, @@ -172,13 +187,14 @@ describe('HistoryStoreManager', function () { }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.chunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.chunk)) this.HistoryStoreManager.getMostRecentVersion( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) @@ -204,7 +220,7 @@ describe('HistoryStoreManager', function () { }) describe('out of order project structure versions', function () { - beforeEach(function () { + beforeEach(function (done) { this.chunk = { chunk: { startVersion: 5, @@ -222,13 +238,14 @@ describe('HistoryStoreManager', function () { }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.chunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.chunk)) this.HistoryStoreManager.getMostRecentVersion( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) @@ -256,7 +273,7 @@ describe('HistoryStoreManager', function () { }) describe('out of order project structure and doc versions', function () { - beforeEach(function () { + beforeEach(function (done) { this.chunk = { chunk: { startVersion: 5, @@ -313,13 +330,14 @@ describe('HistoryStoreManager', function () { }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.chunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.chunk)) this.HistoryStoreManager.getMostRecentVersion( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) @@ -350,20 +368,21 @@ describe('HistoryStoreManager', function () { }) describe('with an unexpected response', function () { - beforeEach(function () { + beforeEach(function (done) { this.badChunk = { chunk: { foo: 123, // valid chunk should have startVersion property bar: 456, }, } - this.request - .withArgs(this.latestChunkRequestArgs) - .yields(null, { statusCode: 200 }, this.badChunk) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.badChunk)) this.HistoryStoreManager.getMostRecentVersion( this.projectId, this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) @@ -613,28 +632,25 @@ describe('HistoryStoreManager', function () { describe('getProjectBlob', function () { describe('successfully', function () { - beforeEach(function () { + beforeEach(function (done) { this.blobContent = 'test content' - this.blobHash = 'test hash' + this.blobHash = 'testhash' - this.request.yields(null, { statusCode: 200 }, this.blobContent) + this.FetchUtils.fetchString.resolves(this.blobContent) this.HistoryStoreManager.getProjectBlob( this.historyId, this.blobHash, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) it('should get the blob from the overleaf history service', function () { - expect(this.request).to.have.been.calledWithMatch({ - method: 'GET', - url: `${this.settings.overleaf.history.host}/projects/${this.historyId}/blobs/${this.blobHash}`, - auth: { - user: this.settings.overleaf.history.user, - pass: this.settings.overleaf.history.pass, - sendImmediately: true, - }, - }) + expect(this.FetchUtils.fetchString).to.have.been.calledWithMatch( + `${this.settings.overleaf.history.host}/projects/${this.historyId}/blobs/${this.blobHash}` + ) }) it('should call the callback with the blob', function () { @@ -677,32 +693,27 @@ describe('HistoryStoreManager', function () { describe('initializeProject', function () { describe('successfully', function () { - beforeEach(function () { + beforeEach(function (done) { this.response_body = { projectId: this.historyId } - this.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - this.response_body - ) + this.FetchUtils.fetchString.resolves(JSON.stringify(this.response_body)) this.HistoryStoreManager.initializeProject( this.historyId, - this.callback + (...args) => { + this.callback(...args) + done() + } ) }) it('should send the change to the history store', function () { - expect(this.request).to.have.been.calledWithMatch({ - method: 'POST', - url: `${this.settings.overleaf.history.host}/projects`, - auth: { - user: this.settings.overleaf.history.user, - pass: this.settings.overleaf.history.pass, - sendImmediately: true, - }, - json: { projectId: this.historyId }, - }) + expect(this.FetchUtils.fetchString).to.have.been.calledWithMatch( + `${this.settings.overleaf.history.host}/projects`, + { + method: 'POST', + json: { projectId: this.historyId }, + } + ) }) it('should call the callback with the new overleaf id', function () { @@ -713,15 +724,15 @@ describe('HistoryStoreManager', function () { describe('deleteProject', function () { beforeEach(function (done) { - this.request.yields(null, { statusCode: 204 }, '') + this.FetchUtils.fetchString.resolves('') this.HistoryStoreManager.deleteProject(this.historyId, done) }) it('should ask the history store to delete the project', function () { - expect(this.request).to.have.been.calledWithMatch({ - method: 'DELETE', - url: `${this.settings.overleaf.history.host}/projects/${this.historyId}`, - }) + expect(this.FetchUtils.fetchString).to.have.been.calledWithMatch( + `${this.settings.overleaf.history.host}/projects/${this.historyId}`, + { method: 'DELETE' } + ) }) }) }) diff --git a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js index d00797e2c2..7d819c22cb 100644 --- a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js @@ -53,10 +53,8 @@ describe('HttpController', function () { } this.RetryManager = {} this.FlushManager = {} - this.request = {} this.pipeline = sinon.stub() this.HttpController = await esmock(MODULE_PATH, { - request: this.request, stream: { pipeline: this.pipeline }, '../../../../app/js/UpdatesProcessor.js': this.UpdatesProcessor, '../../../../app/js/SummarizedUpdatesManager.js': diff --git a/services/project-history/test/unit/js/RetryManager/RetryManagerTests.js b/services/project-history/test/unit/js/RetryManager/RetryManagerTests.js index f0aec32123..b0645430e6 100644 --- a/services/project-history/test/unit/js/RetryManager/RetryManagerTests.js +++ b/services/project-history/test/unit/js/RetryManager/RetryManagerTests.js @@ -89,7 +89,6 @@ describe('RetryManager', function () { }, }, } - this.request = {} this.RetryManager = await esmock(MODULE_PATH, { '../../../../app/js/WebApiManager.js': this.WebApiManager, '../../../../app/js/RedisManager.js': this.RedisManager, @@ -97,7 +96,6 @@ describe('RetryManager', function () { '../../../../app/js/SyncManager.js': this.SyncManager, '../../../../app/js/UpdatesProcessor.js': this.UpdatesProcessor, '@overleaf/settings': this.settings, - request: this.request, }) }) diff --git a/yarn.lock b/yarn.lock index 59d35e1c38..f49023a09b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6886,7 +6886,6 @@ __metadata: nock: "npm:^13.5.3" overleaf-editor-core: "workspace:*" p-queue: "npm:^8.1.0" - request: "npm:2.88.2" sinon: "npm:~9.0.1" sinon-chai: "npm:^3.7.0" timekeeper: "npm:2.2.0"