Merge pull request #5541 from overleaf/jk-retry-v1-requests-features-refresh

[web] Retry v1 requests in refresh-features path GitOrigin-RevId: 1b0e952390d3de07fdfb115349a8a55965aabe1f
2024-11-21 20:47:08 -05:00 · 2021-10-27 09:24:27 +01:00 · 2021-10-27 09:24:27 +01:00 · 12b90cac69
commit 12b90cac69
parent 4688cb7b54
6 changed files with 132 additions and 118 deletions
--- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js
+++ b/services/web/app/src/Features/Institutions/InstitutionsAPI.js
@ -2,7 +2,7 @@ const OError = require('@overleaf/o-error')
 const logger = require('logger-sharelatex')
 const metrics = require('@overleaf/metrics')
 const settings = require('@overleaf/settings')
-const request = require('request')
+const request = require('requestretry')
 const { promisifyAll } = require('../../util/promises')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 const {
@ -204,67 +204,66 @@ const InstitutionsAPI = {
  },
 }

-function makeAffiliationRequest(requestOptions, callback) {
+function makeAffiliationRequest(options, callback) {
  if (!settings.apis.v1.url) {
    return callback(null)
  } // service is not configured
-  if (!requestOptions.extraSuccessStatusCodes) {
-    requestOptions.extraSuccessStatusCodes = []
+  if (!options.extraSuccessStatusCodes) {
+    options.extraSuccessStatusCodes = []
  }
-  request(
-    {
-      method: requestOptions.method,
-      url: `${settings.apis.v1.url}${requestOptions.path}`,
-      body: requestOptions.body,
-      auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass },
-      json: true,
-      timeout: 20 * 1000,
-    },
-    function (error, response, body) {
-      if (error) {
-        return callback(
-          new V1ConnectionError('error getting affiliations from v1').withCause(
-            error
-          )
+  const requestOptions = {
+    method: options.method,
+    url: `${settings.apis.v1.url}${options.path}`,
+    body: options.body,
+    auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass },
+    json: true,
+    timeout: 20 * 1000,
+  }
+  if (options.method === 'GET') {
+    requestOptions.maxAttempts = 3
+    requestOptions.retryDelay = 500
+  } else {
+    requestOptions.maxAttempts = 0
+  }
+  request(requestOptions, function (error, response, body) {
+    if (error) {
+      return callback(
+        new V1ConnectionError('error getting affiliations from v1').withCause(
+          error
        )
-      }
-      if (response && response.statusCode >= 500) {
-        return callback(
-          new V1ConnectionError({
-            message: 'error getting affiliations from v1',
-            info: {
-              status: response.statusCode,
-              body: body,
-            },
-          })
-        )
-      }
-      let isSuccess = response.statusCode >= 200 && response.statusCode < 300
-      if (!isSuccess) {
-        isSuccess = requestOptions.extraSuccessStatusCodes.includes(
-          response.statusCode
-        )
-      }
-      if (!isSuccess) {
-        let errorMessage
-        if (body && body.errors) {
-          errorMessage = `${response.statusCode}: ${body.errors}`
-        } else {
-          errorMessage = `${requestOptions.defaultErrorMessage}: ${response.statusCode}`
-        }
-
-        logger.warn(
-          { path: requestOptions.path, body: requestOptions.body },
-          errorMessage
-        )
-        return callback(
-          new OError(errorMessage, { statusCode: response.statusCode })
-        )
-      }
-
-      callback(null, body)
+      )
    }
-  )
+    if (response && response.statusCode >= 500) {
+      return callback(
+        new V1ConnectionError({
+          message: 'error getting affiliations from v1',
+          info: {
+            status: response.statusCode,
+            body: body,
+          },
+        })
+      )
+    }
+    let isSuccess = response.statusCode >= 200 && response.statusCode < 300
+    if (!isSuccess) {
+      isSuccess = options.extraSuccessStatusCodes.includes(response.statusCode)
+    }
+    if (!isSuccess) {
+      let errorMessage
+      if (body && body.errors) {
+        errorMessage = `${response.statusCode}: ${body.errors}`
+      } else {
+        errorMessage = `${options.defaultErrorMessage}: ${response.statusCode}`
+      }
+
+      logger.warn({ path: options.path, body: options.body }, errorMessage)
+      return callback(
+        new OError(errorMessage, { statusCode: response.statusCode })
+      )
+    }
+
+    callback(null, body)
+  })
 }
 ;[
  'getInstitutionAffiliations',
--- a/services/web/app/src/Features/Subscription/V1SubscriptionManager.js
+++ b/services/web/app/src/Features/Subscription/V1SubscriptionManager.js
@ -13,7 +13,7 @@
 */
 let V1SubscriptionManager
 const UserGetter = require('../User/UserGetter')
-const request = require('request')
+const request = require('requestretry')
 const settings = require('@overleaf/settings')
 const { V1ConnectionError, NotFoundError } = require('../Errors/Errors')
 const { promisifyAll } = require('../../util/promises')
@ -156,56 +156,60 @@ module.exports = V1SubscriptionManager = {
        return callback(null, null, null)
      }
      const url = options.url(v1Id)
-      return request(
-        {
-          baseUrl: settings.apis.v1.url,
-          url,
-          method: options.method,
-          auth: {
-            user: settings.apis.v1.user,
-            pass: settings.apis.v1.pass,
-            sendImmediately: true,
-          },
-          json: true,
-          timeout: 15 * 1000,
+      const requestOptions = {
+        baseUrl: settings.apis.v1.url,
+        url,
+        method: options.method,
+        auth: {
+          user: settings.apis.v1.user,
+          pass: settings.apis.v1.pass,
+          sendImmediately: true,
        },
-        function (error, response, body) {
-          if (error != null) {
-            return callback(
-              new V1ConnectionError({
-                message: 'no v1 connection',
-                info: { url },
-              }).withCause(error)
-            )
-          }
-          if (response && response.statusCode >= 500) {
-            return callback(
-              new V1ConnectionError({
-                message: 'error from v1',
-                info: {
-                  status: response.statusCode,
-                  body: body,
-                },
-              })
-            )
-          }
-          if (response.statusCode >= 200 && response.statusCode < 300) {
-            return callback(null, body, v1Id)
+        json: true,
+        timeout: 15 * 1000,
+      }
+      if (options.method === 'GET') {
+        requestOptions.maxAttempts = 3
+        requestOptions.retryDelay = 500
+      } else {
+        requestOptions.maxAttempts = 0
+      }
+      request(requestOptions, function (error, response, body) {
+        if (error != null) {
+          return callback(
+            new V1ConnectionError({
+              message: 'no v1 connection',
+              info: { url },
+            }).withCause(error)
+          )
+        }
+        if (response && response.statusCode >= 500) {
+          return callback(
+            new V1ConnectionError({
+              message: 'error from v1',
+              info: {
+                status: response.statusCode,
+                body: body,
+              },
+            })
+          )
+        }
+        if (response.statusCode >= 200 && response.statusCode < 300) {
+          return callback(null, body, v1Id)
+        } else {
+          if (response.statusCode === 404) {
+            return callback(new NotFoundError(`v1 user not found: ${userId}`))
          } else {
-            if (response.statusCode === 404) {
-              return callback(new NotFoundError(`v1 user not found: ${userId}`))
-            } else {
-              return callback(
-                new Error(
-                  `non-success code from v1: ${response.statusCode} ${
-                    options.method
-                  } ${options.url(v1Id)}`
-                )
+            return callback(
+              new Error(
+                `non-success code from v1: ${response.statusCode} ${
+                  options.method
+                } ${options.url(v1Id)}`
              )
-            }
+            )
          }
        }
-      )
+      })
    })
  },
 }
--- a/services/web/package-lock.json
+++ b/services/web/package-lock.json
@ -32457,14 +32457,12 @@
      }
    },
    "requestretry": {
-      "version": "1.13.0",
-      "resolved": "https://registry.npmjs.org/requestretry/-/requestretry-1.13.0.tgz",
-      "integrity": "sha512-Lmh9qMvnQXADGAQxsXHP4rbgO6pffCfuR8XUBdP9aitJcLQJxhp7YZK4xAVYXnPJ5E52mwrfiKQtKonPL8xsmg==",
+      "version": "6.0.0",
+      "resolved": "https://registry.npmjs.org/requestretry/-/requestretry-6.0.0.tgz",
+      "integrity": "sha512-X7O+BMlfHgzetfSDtgQIMinLn1BuT+95W12iffDzyOS+HLoBEIQqCZv++UTChUWVjOu+pudbocD76+4j+jK9ww==",
      "requires": {
-        "extend": "^3.0.0",
-        "lodash": "^4.15.0",
-        "request": "^2.74.0",
-        "when": "^3.7.7"
+        "extend": "^3.0.2",
+        "lodash": "^4.17.15"
      }
    },
    "require-at": {
@ -33622,7 +33620,7 @@
    "sliced": {
      "version": "1.0.1",
      "resolved": "https://registry.npmjs.org/sliced/-/sliced-1.0.1.tgz",
-      "integrity": "sha1-CzpmK10Ewxd7GSa+qCsD+Dei70E="
+      "integrity": "sha512-VZBmZP8WU3sMOZm1bdgTadsQbcscK0UM8oKxKVBs4XAhUo2Xxzm/OFMGBkPusxw9xL3Uy8LrzEqGqJhclsr0yA=="
    },
    "slugify": {
      "version": "1.4.0",
@ -38469,11 +38467,6 @@
        "webidl-conversions": "^6.1.0"
      }
    },
-    "when": {
-      "version": "3.7.8",
-      "resolved": "https://registry.npmjs.org/when/-/when-3.7.8.tgz",
-      "integrity": "sha512-5cZ7mecD3eYcMiCH4wtRPA5iFJZ50BJYDfckI5RRpQiktMiYTcn0ccLTZOvcbBume+1304fQztxeNzNS9Gvrnw=="
-    },
    "which": {
      "version": "1.3.1",
      "resolved": "https://registry.npmjs.org/which/-/which-1.3.1.tgz",
--- a/services/web/package.json
+++ b/services/web/package.json
@ -163,7 +163,7 @@
    "referer-parser": "0.0.3",
    "request": "^2.88.2",
    "request-promise-native": "^1.0.8",
-    "requestretry": "^1.13.0",
+    "requestretry": "^6.0.0",
    "rimraf": "2.2.6",
    "rolling-rate-limiter": "^0.2.10",
    "sanitize-html": "^1.27.1",
--- a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js
+++ b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js
@ -21,7 +21,7 @@ describe('InstitutionsAPI', function () {
          timeAsyncMethod: sinon.stub(),
        },
        '@overleaf/settings': this.settings,
-        request: this.request,
+        requestretry: this.request,
        '../Notifications/NotificationsBuilder': {
          ipMatcherAffiliation: sinon
            .stub()
@ -52,6 +52,9 @@ describe('InstitutionsAPI', function () {
          const expectedUrl = `v1.url/api/v2/institutions/${this.institutionId}/affiliations`
          requestOptions.url.should.equal(expectedUrl)
          requestOptions.method.should.equal('GET')
+          requestOptions.maxAttempts.should.exist
+          requestOptions.maxAttempts.should.not.equal(0)
+          requestOptions.retryDelay.should.exist
          expect(requestOptions.body).not.to.exist
          body.should.equal(responseBody)
          done()
@ -126,6 +129,7 @@ describe('InstitutionsAPI', function () {
          const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations`
          requestOptions.url.should.equal(expectedUrl)
          requestOptions.method.should.equal('GET')
+          requestOptions.maxAttempts.should.equal(3)
          expect(requestOptions.body).not.to.exist
          body.should.equal(responseBody)
          done()
@ -207,6 +211,7 @@ describe('InstitutionsAPI', function () {
          const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations`
          requestOptions.url.should.equal(expectedUrl)
          requestOptions.method.should.equal('POST')
+          requestOptions.maxAttempts.should.equal(0)

          const { body } = requestOptions
          Object.keys(body).length.should.equal(7)
--- a/services/web/test/unit/src/Subscription/V1SusbcriptionManagerTests.js
+++ b/services/web/test/unit/src/Subscription/V1SusbcriptionManagerTests.js
@ -40,7 +40,7 @@ describe('V1SubscriptionManager', function () {
            mendeley: true,
          },
        }),
-        request: (this.request = sinon.stub()),
+        requestretry: (this.request = sinon.stub()),
      },
    })
    this.userId = 'abcd'
@ -236,6 +236,7 @@ describe('V1SubscriptionManager', function () {
          return this.V1SubscriptionManager._v1Request(
            this.user_id,
            {
+              method: 'GET',
              url() {
                return '/foo'
              },
@ -252,6 +253,18 @@ describe('V1SubscriptionManager', function () {
        })
      })

+      it('should have supplied retry options to request', function (done) {
+        return this.call((err, body, v1Id) => {
+          const requestOptions = this.request.lastCall.args[0]
+          expect(requestOptions.url).to.equal('/foo')
+          expect(requestOptions.maxAttempts).to.exist
+          expect(requestOptions.maxAttempts > 0).to.be.true
+          expect(requestOptions.retryDelay).to.exist
+          expect(requestOptions.retryDelay > 0).to.be.true
+          return done()
+        })
+      })
+
      it('should return the v1 user id', function (done) {
        return this.call((err, body, v1Id) => {
          expect(v1Id).to.equal(this.v1UserId)