Merge pull request #17735 from overleaf/em-promisify-web-api-manager

Promisify WebApiManager

GitOrigin-RevId: 95addc9442845252aa51c353676486b2dbce0662

package-lock.json

@@ -43058,8 +43058,7 @@
         "mongodb-legacy": "^6.0.1",
         "overleaf-editor-core": "*",
         "redis": "~0.10.1",
-        "request": "^2.88.2",
-        "requestretry": "^7.1.0"
+        "request": "^2.88.2"
       },
       "devDependencies": {
         "chai": "^4.3.6",
@@ -51663,7 +51662,6 @@
         "overleaf-editor-core": "*",
         "redis": "~0.10.1",
         "request": "^2.88.2",
-        "requestretry": "^7.1.0",
         "sinon": "~9.0.1",
         "sinon-chai": "^3.7.0",
         "timekeeper": "2.2.0",

services/project-history/app/js/UpdatesProcessor.js

@@ -237,7 +237,7 @@ export function _getHistoryId(projectId, updates, callback) {
     }
   }
 
-  WebApiManager.getHistoryId(projectId, (error, idFromWeb, cached) => {
+  WebApiManager.getHistoryId(projectId, (error, idFromWeb) => {
     if (error != null && idFromUpdates != null) {
       // present only on updates
       // 404s from web are an error
@@ -266,7 +266,6 @@ export function _getHistoryId(projectId, updates, callback) {
           projectId,
           idFromWeb,
           idFromUpdates,
-          idWasCached: cached,
           updates,
         },
         'inconsistent project history id between updates and web'

services/project-history/app/js/WebApiManager.js

@@ -1,96 +1,104 @@
-import { promisify } from 'util'
-import request from 'requestretry' // allow retry on error https://github.com/FGRibreau/node-request-retry
+import { callbackify } from 'util'
+import { setTimeout } from 'timers/promises'
 import logger from '@overleaf/logger'
 import Metrics from '@overleaf/metrics'
-import OError from '@overleaf/o-error'
 import Settings from '@overleaf/settings'
+import {
+  fetchNothing,
+  fetchJson,
+  RequestFailedError,
+} from '@overleaf/fetch-utils'
 import * as Errors from './Errors.js'
 import * as RedisManager from './RedisManager.js'
 
-// Don't let HTTP calls hang for a long time
-const DEFAULT_MAX_HTTP_REQUEST_LENGTH = 16000 // 16 seconds
+let RETRY_TIMEOUT_MS = 5000
 
-export function getHistoryId(projectId, callback) {
+async function getHistoryId(projectId) {
   Metrics.inc('history_id_cache_requests_total')
-  RedisManager.getCachedHistoryId(projectId, (err, cachedHistoryId) => {
-    if (err) return callback(err)
-    if (cachedHistoryId) {
-      Metrics.inc('history_id_cache_hits_total')
-      callback(null, cachedHistoryId, true)
-    } else {
-      _getProjectDetails(projectId, function (error, project) {
-        if (error) {
-          return callback(error)
-        }
-        const historyId =
-          project.overleaf &&
-          project.overleaf.history &&
-          project.overleaf.history.id
-        if (historyId != null) {
-          RedisManager.setCachedHistoryId(projectId, historyId, err => {
-            if (err) return callback(err)
-            callback(null, historyId, false)
-          })
-        } else {
-          callback(null, historyId, false)
-        }
-      })
+  const cachedHistoryId =
+    await RedisManager.promises.getCachedHistoryId(projectId)
+  if (cachedHistoryId) {
+    Metrics.inc('history_id_cache_hits_total')
+    return cachedHistoryId
+  } else {
+    const project = await _getProjectDetails(projectId)
+    const historyId =
+      project.overleaf &&
+      project.overleaf.history &&
+      project.overleaf.history.id
+    if (historyId != null) {
+      await RedisManager.promises.setCachedHistoryId(projectId, historyId)
     }
-  })
+    return historyId
+  }
 }
 
-export function requestResync(projectId, callback) {
-  const path = `/project/${projectId}/history/resync`
-  _sendRequest(
-    { path, timeout: 6 * 60000, maxAttempts: 1, method: 'POST' },
-    callback
-  )
+async function requestResync(projectId) {
+  try {
+    await fetchNothing(
+      `${Settings.apis.web.url}/project/${projectId}/history/resync`,
+      {
+        method: 'POST',
+        signal: AbortSignal.timeout(6 * 60000),
+        basicAuth: {
+          user: Settings.apis.web.user,
+          password: Settings.apis.web.pass,
+        },
+      }
+    )
+  } catch (err) {
+    if (err instanceof RequestFailedError && err.response.status === 404) {
+      throw new Errors.NotFoundError('got a 404 from web api').withCause(err)
+    } else {
+      throw err
+    }
+  }
 }
 
-function _getProjectDetails(projectId, callback) {
-  const path = `/project/${projectId}/details`
+async function _getProjectDetails(projectId, callback) {
   logger.debug({ projectId }, 'getting project details from web')
-  _sendRequest({ path, json: true }, callback)
-}
-
-function _sendRequest(options, callback) {
-  const url = `${Settings.apis.web.url}${options.path}`
-  request(
-    {
-      method: options.method || 'GET',
-      url,
-      json: options.json || false,
-      timeout: options.timeout || DEFAULT_MAX_HTTP_REQUEST_LENGTH,
-      maxAttempts: options.maxAttempts || 2, // for node-request-retry
-      auth: {
-        user: Settings.apis.web.user,
-        pass: Settings.apis.web.pass,
-        sendImmediately: true,
-      },
-    },
-    function (error, res, body) {
-      if (error != null) {
-        return callback(OError.tag(error))
-      }
-      if (res.statusCode === 404) {
-        logger.debug({ url }, 'got 404 from web api')
-        error = new Errors.NotFoundError('got a 404 from web api')
-        return callback(error)
-      }
-      if (res.statusCode >= 200 && res.statusCode < 300) {
-        callback(null, body)
+  let attempts = 0
+  while (true) {
+    attempts += 1
+    try {
+      return await fetchJson(
+        `${Settings.apis.web.url}/project/${projectId}/details`,
+        {
+          signal: AbortSignal.timeout(16000),
+          basicAuth: {
+            user: Settings.apis.web.user,
+            password: Settings.apis.web.pass,
+          },
+        }
+      )
+    } catch (err) {
+      if (err instanceof RequestFailedError && err.response.status === 404) {
+        throw new Errors.NotFoundError('got a 404 from web api').withCause(err)
+      } else if (attempts < 2) {
+        // retry after 5 seconds
+        await setTimeout(RETRY_TIMEOUT_MS)
       } else {
-        error = new OError(
-          `web returned a non-success status code: ${res.statusCode} (attempts: ${res.attempts})`,
-          { url, res }
-        )
-        callback(error)
+        throw err
       }
     }
-  )
+  }
+}
+
+/**
+ * Adjust the retry timeout in tests
+ */
+export async function setRetryTimeoutMs(timeoutMs) {
+  RETRY_TIMEOUT_MS = timeoutMs
 }
 
+// EXPORTS
+
+const getHistoryIdCb = callbackify(getHistoryId)
+const requestResyncCb = callbackify(requestResync)
+
+export { getHistoryIdCb as getHistoryId, requestResyncCb as requestResync }
+
 export const promises = {
-  getHistoryId: promisify(getHistoryId),
-  requestResync: promisify(requestResync),
+  getHistoryId,
+  requestResync,
 }

services/project-history/package.json

@@ -43,8 +43,7 @@
     "mongodb-legacy": "^6.0.1",
     "overleaf-editor-core": "*",
     "redis": "~0.10.1",
-    "request": "^2.88.2",
-    "requestretry": "^7.1.0"
+    "request": "^2.88.2"
   },
   "devDependencies": {
     "chai": "^4.3.6",

services/project-history/test/unit/js/WebApiManager/WebApiManagerTests.js

@@ -1,13 +1,12 @@
-import async from 'async'
 import sinon from 'sinon'
 import { expect } from 'chai'
 import { strict as esmock } from 'esmock'
+import { RequestFailedError } from '@overleaf/fetch-utils'
 
 const MODULE_PATH = '../../../../app/js/WebApiManager.js'
 
 describe('WebApiManager', function () {
   beforeEach(async function () {
-    this.request = sinon.stub()
     this.settings = {
       apis: {
         web: {
@@ -17,146 +16,137 @@ describe('WebApiManager', function () {
         },
       },
     }
-    this.callback = sinon.stub()
     this.userId = 'mock-user-id'
     this.projectId = 'mock-project-id'
     this.project = { features: 'mock-features' }
     this.olProjectId = 12345
     this.Metrics = { inc: sinon.stub() }
     this.RedisManager = {
-      getCachedHistoryId: sinon.stub(),
-      setCachedHistoryId: sinon.stub().yields(),
+      promises: {
+        getCachedHistoryId: sinon.stub(),
+        setCachedHistoryId: sinon.stub().resolves(),
+      },
+    }
+    this.FetchUtils = {
+      fetchNothing: sinon.stub().resolves(),
+      fetchJson: sinon.stub(),
+      RequestFailedError,
     }
     this.WebApiManager = await esmock(MODULE_PATH, {
-      requestretry: this.request,
+      '@overleaf/fetch-utils': this.FetchUtils,
       '@overleaf/settings': this.settings,
       '@overleaf/metrics': this.Metrics,
       '../../../../app/js/RedisManager.js': this.RedisManager,
     })
+    this.WebApiManager.setRetryTimeoutMs(100)
   })
 
   describe('getHistoryId', function () {
     describe('when there is no cached value and the web request is successful', function () {
       beforeEach(function () {
-        this.RedisManager.getCachedHistoryId
+        this.RedisManager.promises.getCachedHistoryId
           .withArgs(this.projectId) // first call, no cached value returned
           .onCall(0)
-          .yields()
-        this.RedisManager.getCachedHistoryId
+          .resolves(null)
+        this.RedisManager.promises.getCachedHistoryId
           .withArgs(this.projectId) // subsequent calls, return cached value
-          .yields(null, this.olProjectId)
-        this.RedisManager.getCachedHistoryId
+          .resolves(this.olProjectId)
+        this.RedisManager.promises.getCachedHistoryId
           .withArgs('mock-project-id-2') // no cached value for other project
-          .yields()
-        this.request.yields(
-          null,
-          { statusCode: 200 },
-          { overleaf: { history: { id: this.olProjectId } } }
-        )
+          .resolves(null)
+        this.FetchUtils.fetchJson.resolves({
+          overleaf: { history: { id: this.olProjectId } },
+        })
       })
 
-      it('should only request project details once per project', function (done) {
-        async.times(
-          5,
-          (n, cb) => {
-            this.WebApiManager.getHistoryId(this.projectId, cb)
-          },
-          () => {
-            this.request.callCount.should.equal(1)
+      it('should only request project details once per project', async function () {
+        for (let i = 0; i < 5; i++) {
+          await this.WebApiManager.promises.getHistoryId(this.projectId)
+        }
+        this.FetchUtils.fetchJson.should.have.been.calledOnce
 
-            this.WebApiManager.getHistoryId('mock-project-id-2', () => {
-              this.request.callCount.should.equal(2)
-              done()
-            })
-          }
-        )
+        await this.WebApiManager.promises.getHistoryId('mock-project-id-2')
+        this.FetchUtils.fetchJson.should.have.been.calledTwice
       })
 
-      it('should cache the history id', function (done) {
-        this.WebApiManager.getHistoryId(
-          this.projectId,
-          (error, olProjectId) => {
-            if (error) return done(error)
-            this.RedisManager.setCachedHistoryId
-              .calledWith(this.projectId, olProjectId)
-              .should.equal(true)
-            done()
-          }
+      it('should cache the history id', async function () {
+        const olProjectId = await this.WebApiManager.promises.getHistoryId(
+          this.projectId
         )
+        this.RedisManager.promises.setCachedHistoryId
+          .calledWith(this.projectId, olProjectId)
+          .should.equal(true)
       })
 
-      it('should call the callback with the project', function (done) {
-        this.WebApiManager.getHistoryId(
-          this.projectId,
-          (error, olProjectId) => {
-            expect(error).to.be.null
-            expect(
-              this.request.calledWithMatch({
-                method: 'GET',
-                url: `${this.settings.apis.web.url}/project/${this.projectId}/details`,
-                json: true,
-                auth: {
-                  user: this.settings.apis.web.user,
-                  pass: this.settings.apis.web.pass,
-                  sendImmediately: true,
-                },
-              })
-            ).to.be.true
-            expect(olProjectId).to.equal(this.olProjectId)
-            done()
+      it("should return the project's history id", async function () {
+        const olProjectId = await this.WebApiManager.promises.getHistoryId(
+          this.projectId
+        )
+
+        expect(this.FetchUtils.fetchJson).to.have.been.calledWithMatch(
+          `${this.settings.apis.web.url}/project/${this.projectId}/details`,
+          {
+            basicAuth: {
+              user: this.settings.apis.web.user,
+              password: this.settings.apis.web.pass,
+            },
           }
         )
+        expect(olProjectId).to.equal(this.olProjectId)
       })
     })
 
     describe('when the web API returns an error', function () {
       beforeEach(function () {
         this.error = new Error('something went wrong')
-        this.request.yields(this.error)
-        this.RedisManager.getCachedHistoryId.yields()
-        this.WebApiManager.getHistoryId(this.projectId, this.callback)
+        this.FetchUtils.fetchJson.rejects(this.error)
+        this.RedisManager.promises.getCachedHistoryId.resolves(null)
       })
 
-      it('should return an error to the callback', function () {
-        this.callback.calledWith(this.error).should.equal(true)
+      it('should throw an error', async function () {
+        await expect(
+          this.WebApiManager.promises.getHistoryId(this.projectId)
+        ).to.be.rejectedWith(this.error)
       })
     })
 
     describe('when web returns a 404', function () {
       beforeEach(function () {
-        this.request.callsArgWith(1, null, { statusCode: 404 }, '')
-        this.RedisManager.getCachedHistoryId.yields()
-        this.WebApiManager.getHistoryId(this.projectId, this.callback)
+        this.FetchUtils.fetchJson.rejects(
+          new RequestFailedError(
+            'http://some-url',
+            {},
+            { status: 404 },
+            'Not found'
+          )
+        )
+        this.RedisManager.promises.getCachedHistoryId.resolves(null)
       })
 
-      it('should return the callback with an error', function () {
-        this.callback
-          .calledWith(sinon.match.has('message', 'got a 404 from web api'))
-          .should.equal(true)
+      it('should throw an error', async function () {
+        await expect(
+          this.WebApiManager.promises.getHistoryId(this.projectId)
+        ).to.be.rejectedWith('got a 404 from web api')
       })
     })
 
     describe('when web returns a failure error code', function () {
       beforeEach(function () {
-        this.RedisManager.getCachedHistoryId.yields()
-        this.request.callsArgWith(
-          1,
-          null,
-          { statusCode: 500, attempts: 42 },
-          ''
+        this.RedisManager.promises.getCachedHistoryId.resolves(null)
+        this.FetchUtils.fetchJson.rejects(
+          new RequestFailedError(
+            'http://some-url',
+            {},
+            { status: 500 },
+            'Error'
+          )
         )
-        this.WebApiManager.getHistoryId(this.projectId, this.callback)
       })
 
-      it('should return the callback with an error', function () {
-        this.callback
-          .calledWith(
-            sinon.match.has(
-              'message',
-              'web returned a non-success status code: 500 (attempts: 42)'
-            )
-          )
-          .should.equal(true)
+      it('should throw an error', async function () {
+        await expect(
+          this.WebApiManager.promises.getHistoryId(this.projectId)
+        ).to.be.rejectedWith(RequestFailedError)
       })
     })
   })