Merge pull request #31142 from overleaf/ar-promisify-web-api-manager

[real-time] promisify web api manager

GitOrigin-RevId: da2677c05dd7d066b0a625763d4158b28671615e

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"

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/

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 \

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,
   },
 }

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"

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)
       })
     })
   })

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],
+      })
     })
   })
 })

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',