diff --git a/src/lib/axios-retry-config.ts b/src/lib/axios-retry-config.ts deleted file mode 100644 index bb60aee5..00000000 --- a/src/lib/axios-retry-config.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { AxiosError, AxiosRequestConfig } from 'axios'; -import isRetryAllowed from 'is-retry-allowed'; - -const axiosRetryConfig = { - retries: 3, - retryDelay: () => 1000, - retryCondition: (err: AxiosError) => { - const status = err.response?.status; - return (!status || status === 409 || status >= 500) && isRetryAllowed(err); - }, - onRetry: (retryCount: number, error: AxiosError, requestConfig: AxiosRequestConfig) => { - console.log(`${requestConfig.url} failure. Retrying (${retryCount})`); - }, -}; - -export default axiosRetryConfig; diff --git a/src/lib/cht-api.ts b/src/lib/cht-api.ts index d6e18313..9762a4cd 100644 --- a/src/lib/cht-api.ts +++ b/src/lib/cht-api.ts @@ -1,7 +1,7 @@ import _ from 'lodash'; import axios, { AxiosHeaders } from 'axios'; import axiosRetry from 'axios-retry'; -import axiosRetryConfig from './axios-retry-config'; +import { axiosRetryConfig } from './retry-logic'; import { UserPayload } from '../services/user-payload'; import { AuthenticationInfo, Config, ContactType } from '../config'; diff --git a/src/lib/retry-logic.ts b/src/lib/retry-logic.ts new file mode 100644 index 00000000..d29afb8a --- /dev/null +++ b/src/lib/retry-logic.ts @@ -0,0 +1,70 @@ +import { AxiosError, AxiosRequestConfig } from 'axios'; +import isRetryAllowed from 'is-retry-allowed'; +import { UserPayload } from '../services/user-payload'; +import { ChtApi } from './cht-api'; + +const RETRY_COUNT = 4; + +export const axiosRetryConfig = { + retries: RETRY_COUNT, + retryDelay: () => 1000, + retryCondition: (err: AxiosError) => { + const status = err.response?.status; + return (!status || status >= 500) && isRetryAllowed(err); + }, + onRetry: (retryCount: number, error: AxiosError, requestConfig: AxiosRequestConfig) => { + console.log(`${requestConfig.url} failure. Retrying (${retryCount})`); + }, +}; + +export async function retryOnUpdateConflict(funcWithPut: () => Promise): Promise { + for (let retryCount = 0; retryCount < RETRY_COUNT; retryCount++) { + try { + return await funcWithPut(); + } catch (err : any) { + const statusCode = err.response?.status; + if (statusCode === 409) { + console.log(`Retrying on update-conflict (${retryCount})`); + continue; + } + + throw err; + } + } + + throw Error('update-conflict 409 persisted'); +} + +export async function createUserWithRetries(userPayload: UserPayload, chtApi: ChtApi): Promise<{ username: string; password: string }> { + for (let retryCount = 0; retryCount < RETRY_COUNT; ++retryCount) { + try { + await chtApi.createUser(userPayload); + return userPayload; + } catch (err: any) { + if (axiosRetryConfig.retryCondition(err)) { + continue; + } + + if (err.response?.status !== 400) { + throw err; + } + + const translationKey = err.response?.data?.error?.translationKey; + console.error('createUser retry because', translationKey); + if (translationKey === 'username.taken') { + userPayload.makeUsernameMoreComplex(); + continue; + } + + const RETRY_PASSWORD_TRANSLATIONS = ['password.length.minimum', 'password.weak']; + if (RETRY_PASSWORD_TRANSLATIONS.includes(translationKey)) { + userPayload.regeneratePassword(); + continue; + } + + throw err; + } + } + + throw new Error('could not create user ' + userPayload.contact); +} diff --git a/src/services/upload-manager.ts b/src/services/upload-manager.ts index 97ae19e6..2d108039 100644 --- a/src/services/upload-manager.ts +++ b/src/services/upload-manager.ts @@ -1,6 +1,6 @@ import EventEmitter from 'events'; -import axiosRetryConfig from '../lib/axios-retry-config'; +import * as RetryLogic from '../lib/retry-logic'; import { ChtApi, PlacePayload } from '../lib/cht-api'; import { Config } from '../config'; import Place, { PlaceUploadState } from './place'; @@ -11,7 +11,6 @@ import { UploadReplacementPlace } from './upload.replacement'; import { UserPayload } from './user-payload'; const UPLOAD_BATCH_SIZE = 10; -const RETRY_COUNT = 5; export interface Uploader { handleContact (payload: PlacePayload): Promise; @@ -56,7 +55,8 @@ export class UploadManager extends EventEmitter { place.creationDetails.placeId = placeId; } - await uploader.linkContactAndPlace(place, place.creationDetails?.placeId); + const createdPlaceId = place.creationDetails.placeId; // closure required for typescript + await RetryLogic.retryOnUpdateConflict(() => uploader.linkContactAndPlace(place, createdPlaceId)); if (!place.creationDetails.contactId) { throw Error('creationDetails.contactId not set'); @@ -64,7 +64,7 @@ export class UploadManager extends EventEmitter { if (!place.creationDetails.username) { const userPayload = new UserPayload(place, place.creationDetails.placeId, place.creationDetails.contactId); - const { username, password } = await tryCreateUser(userPayload, chtApi); + const { username, password } = await RetryLogic.createUserWithRetries(userPayload, chtApi); place.creationDetails.username = username; place.creationDetails.password = password; } @@ -102,37 +102,3 @@ export class UploadManager extends EventEmitter { this.emit('places_state_change', state); }; } - -async function tryCreateUser (userPayload: UserPayload, chtApi: ChtApi): Promise<{ username: string; password: string }> { - for (let retryCount = 0; retryCount < RETRY_COUNT; ++retryCount) { - try { - await chtApi.createUser(userPayload); - return userPayload; - } catch (err: any) { - if (axiosRetryConfig.retryCondition(err)) { - continue; - } - - if (err.response?.status !== 400) { - throw err; - } - - const translationKey = err.response?.data?.error?.translationKey; - console.error('createUser retry because', translationKey); - if (translationKey === 'username.taken') { - userPayload.makeUsernameMoreComplex(); - continue; - } - - const RETRY_PASSWORD_TRANSLATIONS = ['password.length.minimum', 'password.weak']; - if (RETRY_PASSWORD_TRANSLATIONS.includes(translationKey)) { - userPayload.regeneratePassword(); - continue; - } - - throw err; - } - } - - throw new Error('could not create user ' + userPayload.contact); -} diff --git a/src/services/upload.replacement.ts b/src/services/upload.replacement.ts index fb4e7dcd..524f64c6 100644 --- a/src/services/upload.replacement.ts +++ b/src/services/upload.replacement.ts @@ -1,5 +1,6 @@ import { ChtApi, PlacePayload } from '../lib/cht-api'; import Place from './place'; +import { retryOnUpdateConflict } from '../lib/retry-logic'; import { Uploader } from './upload-manager'; export class UploadReplacementPlace implements Uploader { @@ -21,7 +22,7 @@ export class UploadReplacementPlace implements Uploader { throw Error('contactId and placeId are required'); } - const updatedPlaceId = await this.chtApi.updatePlace(payload, contactId); + const updatedPlaceId = await retryOnUpdateConflict(() => this.chtApi.updatePlace(payload, contactId)); const disabledUsers = await this.chtApi.disableUsersWithPlace(placeId); place.creationDetails.disabledUsers = disabledUsers; diff --git a/test/lib/axios-retry-config.spec.ts b/test/lib/axios-retry-config.spec.ts deleted file mode 100644 index b2723206..00000000 --- a/test/lib/axios-retry-config.spec.ts +++ /dev/null @@ -1,26 +0,0 @@ -import { expect } from 'chai'; -import AxiosRetryConfig from '../../src/lib/axios-retry-config'; -import { UploadManagerRetryScenarios } from '../services/upload-manager.spec'; - -const scenarios = [ - { desc: '503', axiosError: { response: { status: 503 } } }, - { desc: 'axios timeout', axiosError: { code: 'ECONNABORTED' } }, - { desc: 'service timeout', axiosError: { code: 'ECONNRESET' } }, - { desc: 'update conflict', axiosError: { code: 'ERR_BAD_REQUEST', response: { status: 409 } } }, -]; - -describe('lib/axios-retry-config', () => { - for (const scenario of scenarios) { - it(scenario.desc, () => { - const doRetry = AxiosRetryConfig.retryCondition(scenario.axiosError as any); - expect(doRetry).to.eq(true); - }); - } - - for (const scenario of UploadManagerRetryScenarios) { - it(scenario.desc, () => { - const doRetry = AxiosRetryConfig.retryCondition(scenario.axiosError as any); - expect(doRetry).to.eq(false); - }); - } -}); diff --git a/test/lib/retry-logic.spec.ts b/test/lib/retry-logic.spec.ts new file mode 100644 index 00000000..2143fc51 --- /dev/null +++ b/test/lib/retry-logic.spec.ts @@ -0,0 +1,125 @@ +import Chai from 'chai'; +import sinon from 'sinon'; + +import * as RetryLogic from '../../src/lib/retry-logic'; +import { UserPayload } from '../../src/services/user-payload'; +import { ChtApi } from '../../src/lib/cht-api'; +import { mockSimpleContactType } from '../mocks'; + +import chaiAsPromised from 'chai-as-promised'; +Chai.use(chaiAsPromised); + +const { expect } = Chai; + +const RetryScenarios = [ + { desc: '503', axiosError: { response: { status: 503 } }, retry: 'axios' }, + { desc: 'axios timeout', axiosError: { code: 'ECONNABORTED' }, retry: 'axios' }, + { desc: 'service timeout', axiosError: { code: 'ECONNRESET' }, retry: 'axios' }, + + { desc: 'update conflict', axiosError: { code: 'ERR_BAD_REQUEST', response: { status: 409 } }, retry: 'update-conflict' }, + + { + desc: 'username taken is not retried', + axiosError: { + code: 'ERR_BAD_REQUEST', + response: { + status: 400, + data: { error: { message: 'Username "chu" already taken.', translationKey: 'username.taken' } }, + } + }, + retry: 'upload-manager' + }, + { + desc: 'password too short', + axiosError: { + code: 'ERR_BAD_REQUEST', + response: { + status: 400, + data: { error: { message: 'The password must be at least 8 characters long.', translationKey: 'password.length.minimum' } }, + } + }, + retry: 'upload-manager' + }, + { + desc: 'password too weak', + axiosError: { + code: 'ERR_BAD_REQUEST', + response: { + status: 400, + data: { + error: { + message: 'The password is too easy to guess. Include a range of types of characters to increase the score.', + translationKey: 'password.weak' + } + }, + } + }, + retry: 'upload-manager' + }, +]; + +export const UploadManagerRetryScenario = RetryScenarios[RetryScenarios.length - 1]; +const UpdateConflictScenario = RetryScenarios.find(s => s.retry === 'update-conflict'); + +describe('lib/retry-logic', () => { + describe('axiosRetryConfig', () => { + for (const scenario of RetryScenarios) { + it(scenario.desc, () => { + const doRetry = RetryLogic.axiosRetryConfig.retryCondition(scenario.axiosError as any); + expect(doRetry).to.eq(scenario.retry === 'axios'); + }); + } + }); + + describe('retryOnUpdateConflict', () => { + for (const scenario of RetryScenarios) { + it(scenario.desc, async () => { + const output = 'foo'; + const testFunction = sinon.stub() + .rejects(scenario.axiosError) + .onSecondCall().resolves(output); + const execute = RetryLogic.retryOnUpdateConflict(testFunction); + const expectRetry = scenario.retry === 'update-conflict'; + if (expectRetry) { + await expect(execute).to.eventually.eq(output); + expect(testFunction.callCount).to.eq(expectRetry ? 2 : 1); + } else { + await expect(execute).to.eventually.be.rejectedWith(scenario.axiosError); + } + }); + } + + it ('throws after persistent conflict', async () => { + const testFunction = sinon.stub().rejects(UpdateConflictScenario?.axiosError); + const execute = RetryLogic.retryOnUpdateConflict(testFunction); + await expect(execute).to.eventually.be.rejectedWith('persisted'); + expect(testFunction.callCount).to.eq(4); + }); + }); + + describe('createUserWithRetries', () => { + for (const scenario of RetryScenarios) { + it(scenario.desc, async() => { + const chtApi = { + createUser: sinon.stub().throws(scenario.axiosError), + }; + const place = { + generateUsername: sinon.stub().returns('username'), + type: mockSimpleContactType('string', 'bar'), + contact: { properties: {} }, + }; + const userPayload = new UserPayload(place as Place, 'place_id', 'contact_id'); + + const execute = RetryLogic.createUserWithRetries(userPayload as UserPayload, chtApi as ChtApi); + const expectRetry = ['upload-manager', 'axios'].includes(scenario.retry); + if (expectRetry) { + await expect(execute).to.eventually.be.rejectedWith('could not create user'); + expect(chtApi.createUser.callCount).to.eq(4); + } else { + await expect(execute).to.eventually.be.rejectedWith(scenario.axiosError); + expect(chtApi.createUser.callCount).to.eq(1); + } + }); + } + }); +}); diff --git a/test/services/upload-manager.spec.ts b/test/services/upload-manager.spec.ts index 8d614527..d8e94273 100644 --- a/test/services/upload-manager.spec.ts +++ b/test/services/upload-manager.spec.ts @@ -10,44 +10,7 @@ import { ChtApi, RemotePlace } from '../../src/lib/cht-api'; import RemotePlaceCache from '../../src/lib/remote-place-cache'; import { Config } from '../../src/config'; import RemotePlaceResolver from '../../src/lib/remote-place-resolver'; - -export const UploadManagerRetryScenarios = [ - { - desc: 'username taken is not retried', - axiosError: { - code: 'ERR_BAD_REQUEST', - response: { - status: 400, - data: { error: { message: 'Username "chu" already taken.', translationKey: 'username.taken' } }, - } - }, - }, - { - desc: 'password too short', - axiosError: { - code: 'ERR_BAD_REQUEST', - response: { - status: 400, - data: { error: { message: 'The password must be at least 8 characters long.', translationKey: 'password.length.minimum' } }, - } - }, - }, - { - desc: 'password too weak', - axiosError: { - code: 'ERR_BAD_REQUEST', - response: { - status: 400, - data: { - error: { - message: 'The password is too easy to guess. Include a range of types of characters to increase the score.', - translationKey: 'password.weak' - } - }, - } - }, - }, -]; +import { UploadManagerRetryScenario } from '../lib/retry-logic.spec'; describe('upload-manager.ts', () => { beforeEach(() => { @@ -268,22 +231,20 @@ describe('upload-manager.ts', () => { expect(chtApi.disableUsersWithPlace.called).to.be.false; }); - for (const scenario of UploadManagerRetryScenarios) { - it(`retry: ${scenario.desc}`, async() => { - const { remotePlace, sessionCache, chtApi } = await createMocks(); + it(`createUser is retried`, async() => { + const { remotePlace, sessionCache, chtApi } = await createMocks(); - chtApi.createUser.throws(scenario.axiosError); - - const chu_name = 'new chu'; - const chu = await createChu(remotePlace, chu_name, sessionCache, chtApi); - - const uploadManager = new UploadManager(); - await uploadManager.doUpload(sessionCache.getPlaces(), chtApi); - expect(chu.isCreated).to.be.false; - expect(chtApi.createUser.callCount).to.be.gt(2); // retried - expect(chu.uploadError).to.include('could not create user'); - }); - } + chtApi.createUser.throws(UploadManagerRetryScenario.axiosError); + + const chu_name = 'new chu'; + const chu = await createChu(remotePlace, chu_name, sessionCache, chtApi); + + const uploadManager = new UploadManager(); + await uploadManager.doUpload(sessionCache.getPlaces(), chtApi); + expect(chu.isCreated).to.be.false; + expect(chtApi.createUser.callCount).to.be.gt(2); // retried + expect(chu.uploadError).to.include('could not create user'); + }); }); async function createChu(remotePlace: RemotePlace, chu_name: string, sessionCache: any, chtApi: ChtApi) {