From f434bc3825d0ecc10fe1a953f3402f80ed7f9a1a Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Wed, 4 Feb 2026 13:27:41 +0000 Subject: [PATCH] Merge pull request #31142 from overleaf/ar-promisify-web-api-manager [real-time] promisify web api manager GitOrigin-RevId: da2677c05dd7d066b0a625763d4158b28671615e --- package-lock.json | 2 +- services/real-time/Dockerfile | 2 + services/real-time/Makefile | 1 + services/real-time/app/js/WebApiManager.js | 107 ++++++------ services/real-time/package.json | 2 +- .../test/acceptance/js/DrainManagerTests.js | 49 +++--- .../test/acceptance/js/HttpControllerTests.js | 72 +++----- .../test/unit/js/WebApiManager.test.js | 159 ++++++++---------- 8 files changed, 188 insertions(+), 206 deletions(-) diff --git a/package-lock.json b/package-lock.json index bb481eedf5..d3daa0b2f1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -57039,6 +57039,7 @@ "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", + "@overleaf/promise-utils": "*", "@overleaf/redis-wrapper": "*", "@overleaf/settings": "*", "@overleaf/validation-tools": "*", @@ -57052,7 +57053,6 @@ "express-session": "^1.17.1", "lodash": "^4.17.21", "proxy-addr": "^2.0.7", - "request": "2.88.2", "socket.io": "github:overleaf/socket.io#0.9.19-overleaf-12", "socket.io-client": "github:overleaf/socket.io-client#0.9.17-overleaf-5", "zod-validation-error": "^4.0.1" diff --git a/services/real-time/Dockerfile b/services/real-time/Dockerfile index 1734fe971e..3041de916f 100644 --- a/services/real-time/Dockerfile +++ b/services/real-time/Dockerfile @@ -17,6 +17,7 @@ COPY libraries/fetch-utils/package.json /overleaf/libraries/fetch-utils/package. COPY libraries/logger/package.json /overleaf/libraries/logger/package.json COPY libraries/metrics/package.json /overleaf/libraries/metrics/package.json COPY libraries/o-error/package.json /overleaf/libraries/o-error/package.json +COPY libraries/promise-utils/package.json /overleaf/libraries/promise-utils/package.json COPY libraries/redis-wrapper/package.json /overleaf/libraries/redis-wrapper/package.json COPY libraries/settings/package.json /overleaf/libraries/settings/package.json COPY libraries/validation-tools/package.json /overleaf/libraries/validation-tools/package.json @@ -28,6 +29,7 @@ COPY libraries/fetch-utils/ /overleaf/libraries/fetch-utils/ COPY libraries/logger/ /overleaf/libraries/logger/ COPY libraries/metrics/ /overleaf/libraries/metrics/ COPY libraries/o-error/ /overleaf/libraries/o-error/ +COPY libraries/promise-utils/ /overleaf/libraries/promise-utils/ COPY libraries/redis-wrapper/ /overleaf/libraries/redis-wrapper/ COPY libraries/settings/ /overleaf/libraries/settings/ COPY libraries/validation-tools/ /overleaf/libraries/validation-tools/ diff --git a/services/real-time/Makefile b/services/real-time/Makefile index c2331749dd..8446c34fab 100644 --- a/services/real-time/Makefile +++ b/services/real-time/Makefile @@ -19,6 +19,7 @@ IMAGE_CACHE ?= $(IMAGE_REPO):cache-$(shell cat \ $(MONOREPO)/libraries/logger/package.json \ $(MONOREPO)/libraries/metrics/package.json \ $(MONOREPO)/libraries/o-error/package.json \ + $(MONOREPO)/libraries/promise-utils/package.json \ $(MONOREPO)/libraries/redis-wrapper/package.json \ $(MONOREPO)/libraries/settings/package.json \ $(MONOREPO)/libraries/validation-tools/package.json \ diff --git a/services/real-time/app/js/WebApiManager.js b/services/real-time/app/js/WebApiManager.js index 08b5cdb99c..61e1670ce4 100644 --- a/services/real-time/app/js/WebApiManager.js +++ b/services/real-time/app/js/WebApiManager.js @@ -1,8 +1,10 @@ -import request from 'request' +import { callbackifyMultiResult } from '@overleaf/promise-utils' import OError from '@overleaf/o-error' import settings from '@overleaf/settings' import logger from '@overleaf/logger' import Errors from './Errors.js' +import Path from 'node:path' +import { fetchJson, RequestFailedError } from '@overleaf/fetch-utils' const { CodedError, @@ -11,55 +13,62 @@ const { WebApiRequestFailedError, } = Errors -export default { - joinProject(projectId, user, callback) { - const userId = user._id - logger.debug({ projectId, userId }, 'sending join project request to web') - const url = `${settings.apis.web.url}/project/${projectId}/join` - request.post( - { - url, - auth: { - user: settings.apis.web.user, - pass: settings.apis.web.pass, - sendImmediately: true, - }, - json: { - userId, - anonymousAccessToken: user.anonymousAccessToken, - }, - jar: false, +async function joinProject(projectId, user) { + const userId = user._id + logger.debug({ projectId, userId }, 'sending join project request to web') + const url = new URL(settings.apis.web.url) + url.pathname = Path.posix.join('project', projectId, 'join') + let data + try { + data = await fetchJson(url, { + method: 'POST', + basicAuth: { + user: settings.apis.web.user, + password: settings.apis.web.pass, }, - function (error, response, data) { - if (error) { - OError.tag(error, 'join project request failed') - return callback(error) - } - if (response.statusCode >= 200 && response.statusCode < 300) { - if (!(data && data.project)) { - return callback(new CorruptedJoinProjectResponseError()) - } - const userMetadata = { - isRestrictedUser: data.isRestrictedUser, - isTokenMember: data.isTokenMember, - isInvitedMember: data.isInvitedMember, - } - callback(null, data.project, data.privilegeLevel, userMetadata) - } else if (response.statusCode === 429) { - callback( - new CodedError( - 'rate-limit hit when joining project', - 'TooManyRequests' - ) - ) - } else if (response.statusCode === 403) { - callback(new NotAuthorizedError()) - } else if (response.statusCode === 404) { - callback(new CodedError('project not found', 'ProjectNotFound')) - } else { - callback(new WebApiRequestFailedError(response.statusCode)) - } + json: { + userId, + anonymousAccessToken: user.anonymousAccessToken, + }, + }) + } catch (error) { + if (error instanceof RequestFailedError) { + if (error.response.status === 429) { + throw new CodedError( + 'rate-limit hit when joining project', + 'TooManyRequests' + ) + } else if (error.response.status === 403) { + throw new NotAuthorizedError() + } else if (error.response.status === 404) { + throw new CodedError('project not found', 'ProjectNotFound') } - ) + throw new WebApiRequestFailedError(error.response.status) + } + throw OError.tag(error, 'join project request failed') + } + if (!(data && data.project)) { + throw new CorruptedJoinProjectResponseError() + } + const userMetadata = { + isRestrictedUser: data.isRestrictedUser, + isTokenMember: data.isTokenMember, + isInvitedMember: data.isInvitedMember, + } + return { + project: data.project, + privilegeLevel: data.privilegeLevel, + userMetadata, + } +} + +export default { + joinProject: callbackifyMultiResult(joinProject, [ + 'project', + 'privilegeLevel', + 'userMetadata', + ]), + promises: { + joinProject, }, } diff --git a/services/real-time/package.json b/services/real-time/package.json index cf3daf942d..29b4e92190 100644 --- a/services/real-time/package.json +++ b/services/real-time/package.json @@ -20,6 +20,7 @@ "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", + "@overleaf/promise-utils": "*", "@overleaf/redis-wrapper": "*", "@overleaf/settings": "*", "@overleaf/validation-tools": "*", @@ -33,7 +34,6 @@ "express-session": "^1.17.1", "lodash": "^4.17.21", "proxy-addr": "^2.0.7", - "request": "2.88.2", "socket.io": "github:overleaf/socket.io#0.9.19-overleaf-12", "socket.io-client": "github:overleaf/socket.io-client#0.9.17-overleaf-5", "zod-validation-error": "^4.0.1" diff --git a/services/real-time/test/acceptance/js/DrainManagerTests.js b/services/real-time/test/acceptance/js/DrainManagerTests.js index 8b10be3eed..b05f627122 100644 --- a/services/real-time/test/acceptance/js/DrainManagerTests.js +++ b/services/real-time/test/acceptance/js/DrainManagerTests.js @@ -10,16 +10,12 @@ import RealTimeClient from './helpers/RealTimeClient.js' import FixturesManager from './helpers/FixturesManager.js' import { expect } from 'chai' import async from 'async' -import request from 'request' +import { fetchNothing } from '@overleaf/fetch-utils' -const drain = function (rate, callback) { - request.post( - { - url: `http://127.0.0.1:3026/drain?rate=${rate}`, - }, - (error, response, data) => callback(error, data) - ) - return null +const drain = async function (rate) { + await fetchNothing(`http://127.0.0.1:3026/drain?rate=${rate}`, { + method: 'POST', + }) } describe('DrainManagerTests', function () { @@ -34,7 +30,7 @@ describe('DrainManagerTests', function () { (e, { project_id: projectId, user_id: userId }) => { this.project_id = projectId this.user_id = userId - return done() + done() } ) return null @@ -43,23 +39,23 @@ describe('DrainManagerTests', function () { before(function (done) { // cleanup to speedup reconnecting this.timeout(10000) - return RealTimeClient.disconnectAllClients(done) + RealTimeClient.disconnectAllClients(done) }) // trigger and check cleanup it('should have disconnected all previous clients', function (done) { - return RealTimeClient.getConnectedClients((error, data) => { + RealTimeClient.getConnectedClients((error, data) => { if (error) { return done(error) } expect(data.length).to.equal(0) - return done() + done() }) }) - return describe('with two clients in the project', function () { + describe('with two clients in the project', function () { beforeEach(function (done) { - return async.series( + async.series( [ cb => { this.clientA = RealTimeClient.connect(this.project_id, cb) @@ -73,34 +69,37 @@ describe('DrainManagerTests', function () { ) }) - return describe('starting to drain', function () { + describe('starting to drain', function () { beforeEach(function (done) { - return async.parallel( + async.parallel( [ cb => { - return this.clientA.on('reconnectGracefully', cb) + this.clientA.on('reconnectGracefully', cb) }, cb => { - return this.clientB.on('reconnectGracefully', cb) + this.clientB.on('reconnectGracefully', cb) }, - cb => drain(2, cb), + cb => + drain(2) + .then(() => cb()) + .catch(cb), ], done ) }) - afterEach(function (done) { - return drain(0, done) + afterEach(async function () { + await drain(0) }) // reset drain it('should not timeout', function () { - return expect(true).to.equal(true) + expect(true).to.equal(true) }) - return it('should not have disconnected', function () { + it('should not have disconnected', function () { expect(this.clientA.socket.connected).to.equal(true) - return expect(this.clientB.socket.connected).to.equal(true) + expect(this.clientB.socket.connected).to.equal(true) }) }) }) diff --git a/services/real-time/test/acceptance/js/HttpControllerTests.js b/services/real-time/test/acceptance/js/HttpControllerTests.js index 8a7ce19266..6916a1299c 100644 --- a/services/real-time/test/acceptance/js/HttpControllerTests.js +++ b/services/real-time/test/acceptance/js/HttpControllerTests.js @@ -6,32 +6,26 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ import async from 'async' -import Request from 'request' +import { + fetchJson, + fetchNothing, + RequestFailedError, +} from '@overleaf/fetch-utils' import { expect } from 'chai' import RealTimeClient from './helpers/RealTimeClient.js' import FixturesManager from './helpers/FixturesManager.js' -const request = Request.defaults({ - baseUrl: 'http://127.0.0.1:3026', -}) - describe('HttpControllerTests', function () { describe('without a user', function () { - it('should return 404 for the client view', function (done) { + it('should return 404 for the client view', async function () { const clientId = 'not-existing' - request.get( - { - url: `/clients/${clientId}`, - json: true, - }, - (error, response, data) => { - if (error) { - return done(error) - } - expect(response.statusCode).to.equal(404) - done() - } - ) + try { + await fetchNothing(`http://127.0.0.1:3026/clients/${clientId}`) + expect.fail('request should have failed') + } catch (error) { + expect(error).to.be.instanceof(RequestFailedError) + expect(error.response.status).to.equal(404) + } }) }) @@ -75,32 +69,22 @@ describe('HttpControllerTests', function () { ) }) - it('should send a client view', function (done) { - request.get( - { - url: `/clients/${this.client.socket.sessionid}`, - json: true, - }, - (error, response, data) => { - if (error) { - return done(error) - } - expect(response.statusCode).to.equal(200) - expect(data.connected_time).to.exist - delete data.connected_time - // .email is not set in the session - delete data.email - expect(data).to.deep.equal({ - client_id: this.client.socket.sessionid, - first_name: 'Joe', - last_name: 'Bloggs', - project_id: this.project_id, - user_id: this.user_id, - rooms: [this.project_id, this.doc_id], - }) - done() - } + it('should send a client view', async function () { + const data = await fetchJson( + `http://127.0.0.1:3026/clients/${this.client.socket.sessionid}` ) + expect(data.connected_time).to.exist + delete data.connected_time + // .email is not set in the session + delete data.email + expect(data).to.deep.equal({ + client_id: this.client.socket.sessionid, + first_name: 'Joe', + last_name: 'Bloggs', + project_id: this.project_id, + user_id: this.user_id, + rooms: [this.project_id, this.doc_id], + }) }) }) }) diff --git a/services/real-time/test/unit/js/WebApiManager.test.js b/services/real-time/test/unit/js/WebApiManager.test.js index 27f502c235..3b0667854c 100644 --- a/services/real-time/test/unit/js/WebApiManager.test.js +++ b/services/real-time/test/unit/js/WebApiManager.test.js @@ -1,16 +1,6 @@ import { vi, describe, beforeEach, it } from 'vitest' -/* eslint-disable - no-return-assign, - 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 { RequestFailedError } from '@overleaf/fetch-utils' const modulePath = '../../../app/js/WebApiManager.js' @@ -21,9 +11,12 @@ describe('WebApiManager', function () { ctx.user = { _id: ctx.user_id } ctx.callback = sinon.stub() - vi.doMock('request', () => ({ - default: (ctx.request = {}), - })) + ctx.fetchUtils = { + fetchJson: sinon.stub(), + RequestFailedError, + } + + vi.doMock('@overleaf/fetch-utils', () => ctx.fetchUtils) vi.doMock('@overleaf/settings', () => ({ default: (ctx.settings = { @@ -37,10 +30,10 @@ describe('WebApiManager', function () { }), })) - return (ctx.WebApiManager = (await import(modulePath)).default) + ctx.WebApiManager = (await import(modulePath)).default }) - return describe('joinProject', function () { + describe('joinProject', function () { describe('successfully', function () { beforeEach(function (ctx) { ctx.response = { @@ -50,36 +43,29 @@ describe('WebApiManager', function () { isTokenMember: true, isInvitedMember: true, } - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 200 }, ctx.response) - return ctx.WebApiManager.joinProject( - ctx.project_id, - ctx.user, - ctx.callback - ) + ctx.fetchUtils.fetchJson.resolves(ctx.response) + ctx.WebApiManager.joinProject(ctx.project_id, ctx.user, ctx.callback) }) it('should send a request to web to join the project', function (ctx) { - return ctx.request.post - .calledWith({ - url: `${ctx.settings.apis.web.url}/project/${ctx.project_id}/join`, - auth: { + ctx.fetchUtils.fetchJson.should.have.been.calledWith( + new URL(`/project/${ctx.project_id}/join`, ctx.settings.apis.web.url), + { + method: 'POST', + basicAuth: { user: ctx.settings.apis.web.user, - pass: ctx.settings.apis.web.pass, - sendImmediately: true, + password: ctx.settings.apis.web.pass, }, json: { userId: ctx.user_id, anonymousAccessToken: undefined, }, - jar: false, - }) - .should.equal(true) + } + ) }) - return it('should return the project, privilegeLevel, and restricted flag', function (ctx) { - return ctx.callback + it('should return the project, privilegeLevel, and restricted flag', function (ctx) { + ctx.callback .calledWith(null, ctx.response.project, ctx.response.privilegeLevel, { isRestrictedUser: ctx.response.isRestrictedUser, isTokenMember: ctx.response.isTokenMember, @@ -104,26 +90,25 @@ describe('WebApiManager', function () { isTokenMember: false, isInvitedMember: false, } - ctx.request.post = sinon - .stub() - .yields(null, { statusCode: 200 }, ctx.response) + ctx.fetchUtils.fetchJson.resolves(ctx.response) ctx.WebApiManager.joinProject(ctx.project_id, ctx.user, ctx.callback) }) it('should send a request to web to join the project', function (ctx) { - ctx.request.post.should.have.been.calledWith({ - url: `${ctx.settings.apis.web.url}/project/${ctx.project_id}/join`, - auth: { - user: ctx.settings.apis.web.user, - pass: ctx.settings.apis.web.pass, - sendImmediately: true, - }, - json: { - userId: ctx.user_id, - anonymousAccessToken: ctx.token, - }, - jar: false, - }) + ctx.fetchUtils.fetchJson.should.have.been.calledWith( + new URL(`/project/${ctx.project_id}/join`, ctx.settings.apis.web.url), + { + method: 'POST', + basicAuth: { + user: ctx.settings.apis.web.user, + password: ctx.settings.apis.web.pass, + }, + json: { + userId: ctx.user_id, + anonymousAccessToken: ctx.token, + }, + } + ) }) it('should return the project, privilegeLevel, and restricted flag', function (ctx) { @@ -142,9 +127,13 @@ describe('WebApiManager', function () { describe('when web replies with a 403', function () { beforeEach(function (ctx) { - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 403 }, null) + ctx.fetchUtils.fetchJson.rejects( + new RequestFailedError( + `/project/${ctx.project_id}/join`, + { method: 'POST' }, + { status: 403 } + ) + ) ctx.WebApiManager.joinProject(ctx.project_id, ctx.user_id, ctx.callback) }) @@ -161,9 +150,13 @@ describe('WebApiManager', function () { describe('when web replies with a 404', function () { beforeEach(function (ctx) { - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 404 }, null) + ctx.fetchUtils.fetchJson.rejects( + new RequestFailedError( + `/project/${ctx.project_id}/join`, + { method: 'POST' }, + { status: 404 } + ) + ) ctx.WebApiManager.joinProject(ctx.project_id, ctx.user_id, ctx.callback) }) @@ -181,18 +174,18 @@ describe('WebApiManager', function () { describe('with an error from web', function () { beforeEach(function (ctx) { - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 500 }, null) - return ctx.WebApiManager.joinProject( - ctx.project_id, - ctx.user_id, - ctx.callback + ctx.fetchUtils.fetchJson.rejects( + new RequestFailedError( + `/project/${ctx.project_id}/join`, + { method: 'POST' }, + { status: 500 } + ) ) + ctx.WebApiManager.joinProject(ctx.project_id, ctx.user_id, ctx.callback) }) - return it('should call the callback with an error', function (ctx) { - return ctx.callback + it('should call the callback with an error', function (ctx) { + ctx.callback .calledWith( sinon.match({ message: 'non-success status code from web', @@ -205,18 +198,12 @@ describe('WebApiManager', function () { describe('with no data from web', function () { beforeEach(function (ctx) { - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 200 }, null) - return ctx.WebApiManager.joinProject( - ctx.project_id, - ctx.user_id, - ctx.callback - ) + ctx.fetchUtils.fetchJson.resolves(null) + ctx.WebApiManager.joinProject(ctx.project_id, ctx.user_id, ctx.callback) }) - return it('should call the callback with an error', function (ctx) { - return ctx.callback + it('should call the callback with an error', function (ctx) { + ctx.callback .calledWith( sinon.match({ message: 'no data returned from joinProject request', @@ -226,20 +213,20 @@ describe('WebApiManager', function () { }) }) - return describe('when the project is over its rate limit', function () { + describe('when the project is over its rate limit', function () { beforeEach(function (ctx) { - ctx.request.post = sinon - .stub() - .callsArgWith(1, null, { statusCode: 429 }, null) - return ctx.WebApiManager.joinProject( - ctx.project_id, - ctx.user_id, - ctx.callback + ctx.fetchUtils.fetchJson.rejects( + new RequestFailedError( + `/project/${ctx.project_id}/join`, + { method: 'POST' }, + { status: 429 } + ) ) + ctx.WebApiManager.joinProject(ctx.project_id, ctx.user_id, ctx.callback) }) - return it('should call the callback with a TooManyRequests error code', function (ctx) { - return ctx.callback + it('should call the callback with a TooManyRequests error code', function (ctx) { + ctx.callback .calledWith( sinon.match({ message: 'rate-limit hit when joining project',