From acd821089a36a9638871feddbe01ff128db5d808 Mon Sep 17 00:00:00 2001 From: June Kelly Date: Wed, 27 Oct 2021 09:24:27 +0100 Subject: [PATCH] Merge pull request #5541 from overleaf/jk-retry-v1-requests-features-refresh [web] Retry v1 requests in refresh-features path GitOrigin-RevId: 1b0e952390d3de07fdfb115349a8a55965aabe1f --- .../Features/Institutions/InstitutionsAPI.js | 111 +++++++++--------- .../Subscription/V1SubscriptionManager.js | 96 +++++++-------- services/web/package-lock.json | 19 +-- services/web/package.json | 2 +- .../src/Institutions/InstitutionsAPITests.js | 7 +- .../V1SusbcriptionManagerTests.js | 15 ++- 6 files changed, 132 insertions(+), 118 deletions(-) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index 4f27fc3e91..0f27ad8b50 100644 --- 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', diff --git a/services/web/app/src/Features/Subscription/V1SubscriptionManager.js b/services/web/app/src/Features/Subscription/V1SubscriptionManager.js index 3f2ca77124..0fdef3e6c9 100644 --- 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)}` ) - } + ) } } - ) + }) }) }, } diff --git a/services/web/package-lock.json b/services/web/package-lock.json index 65161e059c..d816b6a886 100644 --- 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", diff --git a/services/web/package.json b/services/web/package.json index 62d08c67c2..e64bcc6146 100644 --- 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", diff --git a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js index f66e9f001d..010168d364 100644 --- 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) diff --git a/services/web/test/unit/src/Subscription/V1SusbcriptionManagerTests.js b/services/web/test/unit/src/Subscription/V1SusbcriptionManagerTests.js index 116750da1e..0693acf0e3 100644 --- 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)