From 41d77fd9ca3404154c12f56833ee69e019b89d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Mon, 9 Oct 2023 06:59:50 +0200 Subject: [PATCH 1/8] feat(node): add basic retry mechanism --- packages/node/package.json | 5 +- packages/node/src/lib/novu.interface.ts | 8 +++ packages/node/src/lib/novu.ts | 44 +++++++++++- pnpm-lock.yaml | 95 +++++++++++++++---------- 4 files changed, 114 insertions(+), 38 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 95881a06969..08a9eb3758a 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -43,9 +43,11 @@ }, "dependencies": { "@novu/shared": "^0.20.0-alpha.0", + "axios-retry": "^3.8.0", "handlebars": "^4.7.7", "lodash.get": "^4.4.2", - "lodash.merge": "^4.6.2" + "lodash.merge": "^4.6.2", + "uuid": "^9.0.1" }, "devDependencies": { "@sendgrid/mail": "^7.4.6", @@ -53,6 +55,7 @@ "@types/lodash.get": "^4.4.6", "@types/lodash.merge": "^4.6.6", "@types/node": "^14.6.0", + "@types/uuid": "^8.3.4", "axios": "^1.4.0", "codecov": "^3.5.0", "jest": "^27.0.6", diff --git a/packages/node/src/lib/novu.interface.ts b/packages/node/src/lib/novu.interface.ts index 95b8e4e69e5..9bdef72f436 100644 --- a/packages/node/src/lib/novu.interface.ts +++ b/packages/node/src/lib/novu.interface.ts @@ -1,7 +1,15 @@ import { AxiosInstance } from 'axios'; +interface IRetryConfig { + initialDelay: number; + waitMin: number; + waitMax: number; + retryMax: number; +} + export interface INovuConfiguration { backendUrl?: string; + retryConfig?: Partial; } export class WithHttp { diff --git a/packages/node/src/lib/novu.ts b/packages/node/src/lib/novu.ts index 5d721db6807..57597c64eee 100644 --- a/packages/node/src/lib/novu.ts +++ b/packages/node/src/lib/novu.ts @@ -1,4 +1,6 @@ import axios, { AxiosInstance } from 'axios'; +import axiosRetry from 'axios-retry'; +import { v4 as uuid } from 'uuid'; import { Subscribers } from './subscribers/subscribers'; import { EventEmitter } from 'events'; import { Changes } from './changes/changes'; @@ -34,13 +36,53 @@ export class Novu extends EventEmitter { super(); this.apiKey = apiKey; - this.http = axios.create({ + const axiosInstance = axios.create({ baseURL: this.buildBackendUrl(config), headers: { Authorization: `ApiKey ${this.apiKey}`, }, }); + axiosInstance.interceptors.request.use((axiosConfig) => { + const idempotencyKey = axiosConfig.headers['Idempotency-Key']; + // that means intercepted request is retried, so don't generate new idempotency key + if (idempotencyKey) { + return axiosConfig; + } + + axiosConfig.headers['Idempotency-Key'] = uuid(); + + return axiosConfig; + }); + + const retryConfig = config?.retryConfig || {}; + const retries = retryConfig.retryMax || 0; + const minDelay = retryConfig.waitMin || 1; + const maxDelay = retryConfig.waitMax || 30; + const initialDelay = retryConfig.initialDelay || minDelay; + + function backoff(retryCount: number) { + if (retryCount === 1) { + return initialDelay; + } + + const delay = retryCount * minDelay; + if (delay > maxDelay) { + return maxDelay; + } + + return delay; + } + + axiosRetry(axiosInstance, { + retries, + retryDelay(retryCount) { + return backoff(retryCount) * 1000; // return delay in milliseconds + }, + }); + + this.http = axiosInstance; + this.subscribers = new Subscribers(this.http); this.environments = new Environments(this.http); this.events = new Events(this.http); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a13cddc0096..957442e66ac 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -119,7 +119,7 @@ importers: eslint: 8.38.0 eslint-config-airbnb-typescript: 17.0.0_awz7iagzjrkgdisnaft5immp6i eslint-config-prettier: 8.8.0_eslint@8.38.0 - eslint-import-resolver-webpack: 0.13.2_2shkfpyvap4zatu4dplbemujdy + eslint-import-resolver-webpack: 0.13.2_re6elnmjmviqdg6e3dtzej3oae eslint-plugin-eslint-comments: 3.2.0_eslint@8.38.0 eslint-plugin-functional: 3.7.2_ze6bmax3gcsfve3yrzu6npguhe eslint-plugin-import: 2.27.5_ow2j3yqjjpzplq4flnkwqs3k4u @@ -1669,7 +1669,9 @@ importers: '@types/lodash.get': ^4.4.6 '@types/lodash.merge': ^4.6.6 '@types/node': ^14.6.0 + '@types/uuid': ^8.3.4 axios: ^1.4.0 + axios-retry: ^3.8.0 codecov: ^3.5.0 handlebars: ^4.7.7 jest: ^27.0.6 @@ -1682,17 +1684,21 @@ importers: ts-jest: ^27.0.5 typedoc: ^0.24.0 typescript: 4.9.5 + uuid: ^9.0.1 dependencies: '@novu/shared': link:../../libs/shared + axios-retry: 3.8.0 handlebars: 4.7.7 lodash.get: 4.4.2 lodash.merge: 4.6.2 + uuid: 9.0.1 devDependencies: '@sendgrid/mail': 7.7.0 '@types/jest': 29.5.2 '@types/lodash.get': 4.4.7 '@types/lodash.merge': 4.6.7 '@types/node': 14.18.42 + '@types/uuid': 8.3.4 axios: 1.4.0 codecov: 3.8.3 jest: 27.5.1 @@ -22534,6 +22540,13 @@ packages: is-retry-allowed: 1.2.0 dev: false + /axios-retry/3.8.0: + resolution: {integrity: sha512-CfIsQyWNc5/AE7x/UEReRUadiBmQeoBpSEC+4QyGLJMswTsP1tz0GW2YYPnE7w9+ESMef5zOgLDFpHynNyEZ1w==} + dependencies: + '@babel/runtime': 7.21.0 + is-retry-allowed: 2.2.0 + dev: false + /axios/0.21.4: resolution: {integrity: sha512-ut5vewkiu8jjGBdqpM44XxjuCjq9LAKeHVmoVfHVzy8eHgxxq8SbAVQNovDA8mVi05kP0Ea/n/UzcSHcTJQfNg==} dependencies: @@ -24789,7 +24802,7 @@ packages: dependencies: '@types/node': 14.18.42 cosmiconfig: 8.2.0 - ts-node: 10.9.1_wh2cnrlliuuxb2etxm2m3ttgna + ts-node: 10.9.1_u2ngtadnsu6rqlw26gb7xq6vqq typescript: 4.9.5 dev: true @@ -27022,7 +27035,7 @@ packages: transitivePeerDependencies: - supports-color - /eslint-import-resolver-webpack/0.13.2_2shkfpyvap4zatu4dplbemujdy: + /eslint-import-resolver-webpack/0.13.2_re6elnmjmviqdg6e3dtzej3oae: resolution: {integrity: sha512-XodIPyg1OgE2h5BDErz3WJoK7lawxKTJNhgPNafRST6csC/MZC+L5P6kKqsZGRInpbgc02s/WZMrb4uGJzcuRg==} engines: {node: '>= 6'} peerDependencies: @@ -27041,7 +27054,7 @@ packages: lodash: 4.17.21 resolve: 1.22.2 semver: 5.7.1 - webpack: 5.78.0_@swc+core@1.3.49 + webpack: 5.88.2_@swc+core@1.3.49 transitivePeerDependencies: - supports-color dev: true @@ -27099,7 +27112,7 @@ packages: debug: 3.2.7 eslint: 8.38.0 eslint-import-resolver-node: 0.3.7 - eslint-import-resolver-webpack: 0.13.2_2shkfpyvap4zatu4dplbemujdy + eslint-import-resolver-webpack: 0.13.2_re6elnmjmviqdg6e3dtzej3oae transitivePeerDependencies: - supports-color dev: true @@ -31160,6 +31173,11 @@ packages: engines: {node: '>=0.10.0'} dev: false + /is-retry-allowed/2.2.0: + resolution: {integrity: sha512-XVm7LOeLpTW4jV19QSH38vkswxoLud8sQ57YwJVTPWdiaI9I8keEhGFpBlslyVsgdQy4Opg8QOLb8YRgsyZiQg==} + engines: {node: '>=10'} + dev: false + /is-root/2.1.0: resolution: {integrity: sha512-AGOriNp96vNBd3HtU+RzFEc75FfR5ymiYv8E553I71SCeXBiMsVDUtdio1OEFvrPyLIQ9tVR5RxXIFe5PUFjMg==} engines: {node: '>=6'} @@ -31747,7 +31765,7 @@ packages: pretty-format: 27.5.1 slash: 3.0.0 strip-json-comments: 3.1.1 - ts-node: 10.9.1_wh2cnrlliuuxb2etxm2m3ttgna + ts-node: 10.9.1_j6r65ghnzvzk7vhdh4hyogrm4a transitivePeerDependencies: - bufferutil - canvas @@ -43544,7 +43562,7 @@ packages: webpack: 5.88.2 dev: true - /terser-webpack-plugin/5.3.9_wvz5dn57l37py5yhasbcqqk6hi: + /terser-webpack-plugin/5.3.9_zpsjxul5gtyjq5vu5uxru46xsq: resolution: {integrity: sha512-ZuXsqE07EcggTWQjXUj+Aot/OMcD0bMKGgF63f7UxYcu5/AJF53aIpK1YoP5xR9l6s/Hy2b+t1AM0bLNPRuhwA==} engines: {node: '>= 10.13.0'} peerDependencies: @@ -43566,7 +43584,7 @@ packages: schema-utils: 3.3.0 serialize-javascript: 6.0.1 terser: 5.19.3 - webpack: 5.78.0_@swc+core@1.3.49 + webpack: 5.88.2_@swc+core@1.3.49 dev: true /terser/4.8.1: @@ -44020,7 +44038,7 @@ packages: '@types/jest': 29.5.2 bs-logger: 0.2.6 fast-json-stable-stringify: 2.1.0 - jest: 27.5.1_ts-node@10.9.1 + jest: 27.5.1 jest-util: 27.5.1 json5: 2.2.3 lodash.memoize: 4.1.2 @@ -45126,6 +45144,11 @@ packages: resolution: {integrity: sha512-MXcSTerfPa4uqyzStbRoTgt5XIe3x5+42+q1sDuy3R5MDk66URdLMOZe5aPX/SQd+kuYAh0FdP/pO28IkQyTeg==} hasBin: true + /uuid/9.0.1: + resolution: {integrity: sha512-b+1eJOlsR9K8HJpow9Ok3fiWOWSIcIzXodvv0rQjVoOVNpWMpxf1wZNpt4y9h10odCNrqnYp1OBzRktckBe3sA==} + hasBin: true + dev: false + /uvu/0.5.6: resolution: {integrity: sha512-+g8ENReyr8YsOc6fv/NVJs2vFdHBnBNdfE49rshrTzDWOlUx4Gq7KOS2GD8eqhy2j+Ejq29+SbKH8yjkAqXqoA==} engines: {node: '>=8'} @@ -45795,7 +45818,7 @@ packages: - esbuild - uglify-js - /webpack/5.78.0_@swc+core@1.3.49: + /webpack/5.78.0_u5c4qderjagc6tepfyiby6xhau: resolution: {integrity: sha512-gT5DP72KInmE/3azEaQrISjTvLYlSM0j1Ezhht/KLVkrqtv10JoP/RXhwmX/frrutOPuSq3o5Vq0ehR/4Vmd1g==} engines: {node: '>=10.13.0'} hasBin: true @@ -45826,16 +45849,15 @@ packages: neo-async: 2.6.2 schema-utils: 3.3.0 tapable: 2.2.1 - terser-webpack-plugin: 5.3.9_wvz5dn57l37py5yhasbcqqk6hi + terser-webpack-plugin: 5.3.9_dnqqsr3phzjhopay4d6e5ziqz4 watchpack: 2.4.0 webpack-sources: 3.2.3 transitivePeerDependencies: - '@swc/core' - esbuild - uglify-js - dev: true - /webpack/5.78.0_u5c4qderjagc6tepfyiby6xhau: + /webpack/5.78.0_w67ycjwq2niq3jlxgktvf5aow4: resolution: {integrity: sha512-gT5DP72KInmE/3azEaQrISjTvLYlSM0j1Ezhht/KLVkrqtv10JoP/RXhwmX/frrutOPuSq3o5Vq0ehR/4Vmd1g==} engines: {node: '>=10.13.0'} hasBin: true @@ -45868,14 +45890,16 @@ packages: tapable: 2.2.1 terser-webpack-plugin: 5.3.9_dnqqsr3phzjhopay4d6e5ziqz4 watchpack: 2.4.0 + webpack-cli: 5.1.4_jwpra7xq62zw3tehur2jwszlsq webpack-sources: 3.2.3 transitivePeerDependencies: - '@swc/core' - esbuild - uglify-js + dev: true - /webpack/5.78.0_w67ycjwq2niq3jlxgktvf5aow4: - resolution: {integrity: sha512-gT5DP72KInmE/3azEaQrISjTvLYlSM0j1Ezhht/KLVkrqtv10JoP/RXhwmX/frrutOPuSq3o5Vq0ehR/4Vmd1g==} + /webpack/5.82.1_w67ycjwq2niq3jlxgktvf5aow4: + resolution: {integrity: sha512-C6uiGQJ+Gt4RyHXXYt+v9f+SN1v83x68URwgxNQ98cvH8kxiuywWGP4XeNZ1paOzZ63aY3cTciCEQJNFUljlLw==} engines: {node: '>=10.13.0'} hasBin: true peerDependencies: @@ -45885,16 +45909,16 @@ packages: optional: true dependencies: '@types/eslint-scope': 3.7.4 - '@types/estree': 0.0.51 - '@webassemblyjs/ast': 1.11.1 - '@webassemblyjs/wasm-edit': 1.11.1 - '@webassemblyjs/wasm-parser': 1.11.1 - acorn: 8.10.0 - acorn-import-assertions: 1.9.0_acorn@8.10.0 - browserslist: 4.21.10 + '@types/estree': 1.0.0 + '@webassemblyjs/ast': 1.11.5 + '@webassemblyjs/wasm-edit': 1.11.5 + '@webassemblyjs/wasm-parser': 1.11.5 + acorn: 8.8.2 + acorn-import-assertions: 1.8.0_acorn@8.8.2 + browserslist: 4.21.5 chrome-trace-event: 1.0.3 enhanced-resolve: 5.15.0 - es-module-lexer: 0.9.3 + es-module-lexer: 1.2.1 eslint-scope: 5.1.1 events: 3.3.0 glob-to-regexp: 0.4.1 @@ -45903,9 +45927,9 @@ packages: loader-runner: 4.3.0 mime-types: 2.1.35 neo-async: 2.6.2 - schema-utils: 3.3.0 + schema-utils: 3.1.2 tapable: 2.2.1 - terser-webpack-plugin: 5.3.9_dnqqsr3phzjhopay4d6e5ziqz4 + terser-webpack-plugin: 5.3.9_aka7ue4sjkoeo6uo4dlqntmpgy watchpack: 2.4.0 webpack-cli: 5.1.4_jwpra7xq62zw3tehur2jwszlsq webpack-sources: 3.2.3 @@ -45915,8 +45939,8 @@ packages: - uglify-js dev: true - /webpack/5.82.1_w67ycjwq2niq3jlxgktvf5aow4: - resolution: {integrity: sha512-C6uiGQJ+Gt4RyHXXYt+v9f+SN1v83x68URwgxNQ98cvH8kxiuywWGP4XeNZ1paOzZ63aY3cTciCEQJNFUljlLw==} + /webpack/5.88.2: + resolution: {integrity: sha512-JmcgNZ1iKj+aiR0OvTYtWQqJwq37Pf683dY9bVORwVbUrDhLhdn/PlO2sHsFHPkj7sHNQF3JwaAkp49V+Sq1tQ==} engines: {node: '>=10.13.0'} hasBin: true peerDependencies: @@ -45926,13 +45950,13 @@ packages: optional: true dependencies: '@types/eslint-scope': 3.7.4 - '@types/estree': 1.0.0 + '@types/estree': 1.0.1 '@webassemblyjs/ast': 1.11.5 '@webassemblyjs/wasm-edit': 1.11.5 '@webassemblyjs/wasm-parser': 1.11.5 - acorn: 8.8.2 - acorn-import-assertions: 1.8.0_acorn@8.8.2 - browserslist: 4.21.5 + acorn: 8.10.0 + acorn-import-assertions: 1.9.0_acorn@8.10.0 + browserslist: 4.21.10 chrome-trace-event: 1.0.3 enhanced-resolve: 5.15.0 es-module-lexer: 1.2.1 @@ -45944,11 +45968,10 @@ packages: loader-runner: 4.3.0 mime-types: 2.1.35 neo-async: 2.6.2 - schema-utils: 3.1.2 + schema-utils: 3.3.0 tapable: 2.2.1 - terser-webpack-plugin: 5.3.9_aka7ue4sjkoeo6uo4dlqntmpgy + terser-webpack-plugin: 5.3.9_webpack@5.88.2 watchpack: 2.4.0 - webpack-cli: 5.1.4_jwpra7xq62zw3tehur2jwszlsq webpack-sources: 3.2.3 transitivePeerDependencies: - '@swc/core' @@ -45956,7 +45979,7 @@ packages: - uglify-js dev: true - /webpack/5.88.2: + /webpack/5.88.2_@swc+core@1.3.49: resolution: {integrity: sha512-JmcgNZ1iKj+aiR0OvTYtWQqJwq37Pf683dY9bVORwVbUrDhLhdn/PlO2sHsFHPkj7sHNQF3JwaAkp49V+Sq1tQ==} engines: {node: '>=10.13.0'} hasBin: true @@ -45987,7 +46010,7 @@ packages: neo-async: 2.6.2 schema-utils: 3.3.0 tapable: 2.2.1 - terser-webpack-plugin: 5.3.9_webpack@5.88.2 + terser-webpack-plugin: 5.3.9_zpsjxul5gtyjq5vu5uxru46xsq watchpack: 2.4.0 webpack-sources: 3.2.3 transitivePeerDependencies: From b66965a2630942cdfcabd10d72aa4e9b75694628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Wed, 11 Oct 2023 23:51:35 +0200 Subject: [PATCH 2/8] feat: improve retry and add basic tests --- packages/node/package.json | 3 +- packages/node/src/lib/novu.ts | 42 +------------------ packages/node/src/lib/retries.spec.ts | 56 +++++++++++++++++++++++++ packages/node/src/lib/retries.ts | 60 +++++++++++++++++++++++++++ pnpm-lock.yaml | 16 +++++-- 5 files changed, 133 insertions(+), 44 deletions(-) create mode 100644 packages/node/src/lib/retries.spec.ts create mode 100644 packages/node/src/lib/retries.ts diff --git a/packages/node/package.json b/packages/node/package.json index 08a9eb3758a..1e52c6c1b26 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -59,6 +59,7 @@ "axios": "^1.4.0", "codecov": "^3.5.0", "jest": "^27.0.6", + "nock": "^13.1.3", "npm-run-all": "^4.1.5", "open-cli": "^6.0.1", "rimraf": "^3.0.2", @@ -80,7 +81,7 @@ "preset": "ts-jest", "testEnvironment": "node", "moduleNameMapper": { - "axios": "axios/dist/node/axios.cjs" + "^axios$": "axios/dist/node/axios.cjs" } }, "prettier": { diff --git a/packages/node/src/lib/novu.ts b/packages/node/src/lib/novu.ts index 57597c64eee..50417949436 100644 --- a/packages/node/src/lib/novu.ts +++ b/packages/node/src/lib/novu.ts @@ -1,6 +1,4 @@ import axios, { AxiosInstance } from 'axios'; -import axiosRetry from 'axios-retry'; -import { v4 as uuid } from 'uuid'; import { Subscribers } from './subscribers/subscribers'; import { EventEmitter } from 'events'; import { Changes } from './changes/changes'; @@ -15,6 +13,7 @@ import { Topics } from './topics/topics'; import { Integrations } from './integrations/integrations'; import { Messages } from './messages/messages'; import { Tenants } from './tenants/tenants'; +import { makeRetriable } from './retries'; export class Novu extends EventEmitter { private readonly apiKey?: string; @@ -35,7 +34,6 @@ export class Novu extends EventEmitter { constructor(apiKey: string, config?: INovuConfiguration) { super(); this.apiKey = apiKey; - const axiosInstance = axios.create({ baseURL: this.buildBackendUrl(config), headers: { @@ -43,43 +41,7 @@ export class Novu extends EventEmitter { }, }); - axiosInstance.interceptors.request.use((axiosConfig) => { - const idempotencyKey = axiosConfig.headers['Idempotency-Key']; - // that means intercepted request is retried, so don't generate new idempotency key - if (idempotencyKey) { - return axiosConfig; - } - - axiosConfig.headers['Idempotency-Key'] = uuid(); - - return axiosConfig; - }); - - const retryConfig = config?.retryConfig || {}; - const retries = retryConfig.retryMax || 0; - const minDelay = retryConfig.waitMin || 1; - const maxDelay = retryConfig.waitMax || 30; - const initialDelay = retryConfig.initialDelay || minDelay; - - function backoff(retryCount: number) { - if (retryCount === 1) { - return initialDelay; - } - - const delay = retryCount * minDelay; - if (delay > maxDelay) { - return maxDelay; - } - - return delay; - } - - axiosRetry(axiosInstance, { - retries, - retryDelay(retryCount) { - return backoff(retryCount) * 1000; // return delay in milliseconds - }, - }); + makeRetriable(axiosInstance, config); this.http = axiosInstance; diff --git a/packages/node/src/lib/retries.spec.ts b/packages/node/src/lib/retries.spec.ts new file mode 100644 index 00000000000..4c059c5e497 --- /dev/null +++ b/packages/node/src/lib/retries.spec.ts @@ -0,0 +1,56 @@ +import nock from 'nock'; +import { Novu } from '../index'; + +const BACKEND_URL = 'http://example.com'; + +jest.setTimeout(15000); + +describe('Novu Node.js package - Retries and idempotency key', () => { + afterEach(() => { + nock.cleanAll(); + nock.enableNetConnect(); + }); + + const novu = new Novu('fake-key', { + backendUrl: BACKEND_URL, + retryConfig: { + retryMax: 3, + waitMax: 2, + waitMin: 1, + }, + }); + + it('should retry trigger', async () => { + // prepare backend api mock endpoints: + nock(BACKEND_URL) + .post('/v1/events/trigger') + .times(3) + .reply(500, { message: 'Server Exception' }); + + nock(BACKEND_URL) + .post('/v1/events/trigger') + .reply(201, { acknowledged: true, transactionId: '1003' }); + + const result = await novu.trigger('fake-workflow', { + to: { subscriberId: '123' }, + payload: {}, + }); + + expect(result.status).toEqual(201); + expect(result.request.headers['idempotency-key']).toBeDefined(); + }); + + it('should retry getting topics list', async () => { + nock(BACKEND_URL) + .get('/v1/topics') + .times(3) + .reply(500, { message: 'Server Exception' }); + + nock(BACKEND_URL).get('/v1/topics').reply(200, [{}, {}]); + + const result = await novu.topics.list({}); + + expect(result.status).toEqual(200); + expect(result.request.headers['idempotency-key']).toBeUndefined(); + }); +}); diff --git a/packages/node/src/lib/retries.ts b/packages/node/src/lib/retries.ts new file mode 100644 index 00000000000..0d0d3733b98 --- /dev/null +++ b/packages/node/src/lib/retries.ts @@ -0,0 +1,60 @@ +import { AxiosInstance } from 'axios'; +import axiosRetry, { isNetworkError, isRetryableError } from 'axios-retry'; +import { v4 as uuid } from 'uuid'; +import { INovuConfiguration } from './novu.interface'; + +const NON_IDEMPOTENT_METHODS = ['post', 'patch']; + +export function makeRetriable( + axios: AxiosInstance, + config?: INovuConfiguration +) { + axios.interceptors.request.use((axiosConfig) => { + // don't attach idempotency key for idempotent methods + if ( + axiosConfig.method && + !NON_IDEMPOTENT_METHODS.includes(axiosConfig.method) + ) { + return axiosConfig; + } + + const idempotencyKey = axiosConfig.headers['Idempotency-Key']; + // that means intercepted request is retried, so don't generate new idempotency key + if (idempotencyKey) { + return axiosConfig; + } + + axiosConfig.headers['Idempotency-Key'] = uuid(); + + return axiosConfig; + }); + + const retryConfig = config?.retryConfig || {}; + const retries = retryConfig.retryMax || 0; + const minDelay = retryConfig.waitMin || 1; + const maxDelay = retryConfig.waitMax || 30; + const initialDelay = retryConfig.initialDelay || minDelay; + + function backoff(retryCount: number) { + if (retryCount === 1) { + return initialDelay; + } + + const delay = retryCount * minDelay; + if (delay > maxDelay) { + return maxDelay; + } + + return delay; + } + + axiosRetry(axios, { + retries, + retryDelay(retryCount) { + return backoff(retryCount) * 1000; // return delay in milliseconds + }, + retryCondition(err) { + return isNetworkError(err) || isRetryableError(err); + }, + }); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 957442e66ac..ffead53ca74 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1677,6 +1677,7 @@ importers: jest: ^27.0.6 lodash.get: ^4.4.2 lodash.merge: ^4.6.2 + nock: ^13.1.3 npm-run-all: ^4.1.5 open-cli: ^6.0.1 rimraf: ^3.0.2 @@ -1686,7 +1687,7 @@ importers: typescript: 4.9.5 uuid: ^9.0.1 dependencies: - '@novu/shared': link:../../libs/shared + '@novu/shared': 0.20.0-alpha.1 axios-retry: 3.8.0 handlebars: 4.7.7 lodash.get: 4.4.2 @@ -1702,6 +1703,7 @@ importers: axios: 1.4.0 codecov: 3.8.3 jest: 27.5.1 + nock: 13.3.0 npm-run-all: 4.1.5 open-cli: 6.0.1 rimraf: 3.0.2 @@ -13369,6 +13371,16 @@ packages: vue-resize: 2.0.0-alpha.1_vue@3.2.47 dev: false + /@novu/shared/0.20.0-alpha.1: + resolution: {integrity: sha512-dSNQV338YV7rdw676jfGz3N5xzuKb/iHh6Hkwzsek8pi7+ECqinb7H1ENOEY0f8p5MYD7yGb+zV2ey83pBPkjg==} + dependencies: + axios: 1.4.0 + class-transformer: 0.5.1 + class-validator: 0.14.0 + transitivePeerDependencies: + - debug + dev: false + /@novu/stateless/0.12.0: resolution: {integrity: sha512-2GVX07XtR3owk8Dg/66l+kyUvgYR5+uKMheiFCVRuTCqiDfVFww1G3ixIEs2FGIajtLfDdNyuH+PV25EoqmKVg==} engines: {node: '>=10'} @@ -35769,7 +35781,6 @@ packages: propagate: 2.0.1 transitivePeerDependencies: - supports-color - dev: false /node-abort-controller/3.1.1: resolution: {integrity: sha512-AGK2yQKIjRuqnc6VkX2Xj5d+QW8xZ87pa1UK6yA6ouUyuxfHuMP6umE5QK7UmTeOAymo+Zx1Fxiuw9rVx8taHQ==} @@ -38769,7 +38780,6 @@ packages: /propagate/2.0.1: resolution: {integrity: sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag==} engines: {node: '>= 8'} - dev: false /property-information/5.6.0: resolution: {integrity: sha512-YUHSPk+A30YPv+0Qf8i9Mbfe/C0hdPXk1s1jPVToV8pk8BQtpw10ct89Eo7OWkutrwqvT0eicAxlOg3dOAu8JA==} From 503257c4d51576f25ce9362ca7f8f0a0fa8364c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Thu, 12 Oct 2023 23:08:17 +0200 Subject: [PATCH 3/8] feat(node): add retry condition callback and more tests --- packages/node/src/index.ts | 1 + packages/node/src/lib/novu.interface.ts | 15 ++++--- packages/node/src/lib/novu.ts | 4 +- packages/node/src/lib/retries.spec.ts | 59 ++++++++++++++++++++++--- packages/node/src/lib/retries.ts | 36 ++++++++++++--- 5 files changed, 97 insertions(+), 18 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 64cbe3e9569..5ce92c76017 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -30,3 +30,4 @@ export * from './lib/feeds/feeds.interface'; export * from './lib/topics/topic.interface'; export * from './lib/integrations/integrations.interface'; export * from './lib/messages/messages.interface'; +export { defaultRetryCondition } from './lib/retries'; diff --git a/packages/node/src/lib/novu.interface.ts b/packages/node/src/lib/novu.interface.ts index 9bdef72f436..953238f47ec 100644 --- a/packages/node/src/lib/novu.interface.ts +++ b/packages/node/src/lib/novu.interface.ts @@ -1,15 +1,16 @@ -import { AxiosInstance } from 'axios'; +import { AxiosError, AxiosInstance } from 'axios'; -interface IRetryConfig { - initialDelay: number; - waitMin: number; - waitMax: number; - retryMax: number; +export interface IRetryConfig { + initialDelay?: number; + waitMin?: number; + waitMax?: number; + retryMax?: number; + retryCondition?: (err: AxiosError) => boolean; } export interface INovuConfiguration { backendUrl?: string; - retryConfig?: Partial; + retryConfig?: IRetryConfig; } export class WithHttp { diff --git a/packages/node/src/lib/novu.ts b/packages/node/src/lib/novu.ts index 50417949436..616b641f434 100644 --- a/packages/node/src/lib/novu.ts +++ b/packages/node/src/lib/novu.ts @@ -41,7 +41,9 @@ export class Novu extends EventEmitter { }, }); - makeRetriable(axiosInstance, config); + if (config?.retryConfig) { + makeRetriable(axiosInstance, config); + } this.http = axiosInstance; diff --git a/packages/node/src/lib/retries.spec.ts b/packages/node/src/lib/retries.spec.ts index 4c059c5e497..0108a14f20e 100644 --- a/packages/node/src/lib/retries.spec.ts +++ b/packages/node/src/lib/retries.spec.ts @@ -2,9 +2,17 @@ import nock from 'nock'; import { Novu } from '../index'; const BACKEND_URL = 'http://example.com'; +const TOPICS_PATH = '/v1/topics'; +const TRIGGER_PATH = '/v1/events/trigger'; jest.setTimeout(15000); +class NetworkError extends Error { + constructor(public code: string) { + super('Network Error'); + } +} + describe('Novu Node.js package - Retries and idempotency key', () => { afterEach(() => { nock.cleanAll(); @@ -15,7 +23,7 @@ describe('Novu Node.js package - Retries and idempotency key', () => { backendUrl: BACKEND_URL, retryConfig: { retryMax: 3, - waitMax: 2, + waitMax: 1, waitMin: 1, }, }); @@ -23,12 +31,12 @@ describe('Novu Node.js package - Retries and idempotency key', () => { it('should retry trigger', async () => { // prepare backend api mock endpoints: nock(BACKEND_URL) - .post('/v1/events/trigger') + .post(TRIGGER_PATH) .times(3) .reply(500, { message: 'Server Exception' }); nock(BACKEND_URL) - .post('/v1/events/trigger') + .post(TRIGGER_PATH) .reply(201, { acknowledged: true, transactionId: '1003' }); const result = await novu.trigger('fake-workflow', { @@ -42,15 +50,56 @@ describe('Novu Node.js package - Retries and idempotency key', () => { it('should retry getting topics list', async () => { nock(BACKEND_URL) - .get('/v1/topics') + .get(TOPICS_PATH) .times(3) .reply(500, { message: 'Server Exception' }); - nock(BACKEND_URL).get('/v1/topics').reply(200, [{}, {}]); + nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); const result = await novu.topics.list({}); expect(result.status).toEqual(200); expect(result.request.headers['idempotency-key']).toBeUndefined(); }); + + it('should retry on various errors until it reach successfull response', async () => { + nock(BACKEND_URL) + .get(TOPICS_PATH) + .reply(429, { message: 'Too many requests' }); + + nock(BACKEND_URL) + .get(TOPICS_PATH) + .reply(408, { message: 'Request Timeout' }); + + nock(BACKEND_URL) + .get(TOPICS_PATH) + .replyWithError(new NetworkError('ECONNRESET')); + + nock(BACKEND_URL) + .get(TOPICS_PATH) + .replyWithError(new NetworkError('ETIMEDOUT')); + + nock(BACKEND_URL) + .get(TOPICS_PATH) + .replyWithError(new NetworkError('ECONNREFUSED')); + + nock(BACKEND_URL) + .get(TOPICS_PATH) + .reply(504, { message: 'Gateway timeout' }); + + nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); + + const novuClient = new Novu('fake-key', { + backendUrl: BACKEND_URL, + retryConfig: { + initialDelay: 0, + waitMin: 1, + waitMax: 1, + retryMax: 6, + }, + }); + + const result = await novuClient.topics.list({}); + expect(result.status).toEqual(200); + }); }); diff --git a/packages/node/src/lib/retries.ts b/packages/node/src/lib/retries.ts index 0d0d3733b98..f472f09ce6d 100644 --- a/packages/node/src/lib/retries.ts +++ b/packages/node/src/lib/retries.ts @@ -1,5 +1,5 @@ -import { AxiosInstance } from 'axios'; -import axiosRetry, { isNetworkError, isRetryableError } from 'axios-retry'; +import { AxiosError, AxiosInstance } from 'axios'; +import axiosRetry, { isNetworkError } from 'axios-retry'; import { v4 as uuid } from 'uuid'; import { INovuConfiguration } from './novu.interface'; @@ -34,6 +34,7 @@ export function makeRetriable( const minDelay = retryConfig.waitMin || 1; const maxDelay = retryConfig.waitMax || 30; const initialDelay = retryConfig.initialDelay || minDelay; + const retryCondition = retryConfig.retryCondition || defaultRetryCondition; function backoff(retryCount: number) { if (retryCount === 1) { @@ -53,8 +54,33 @@ export function makeRetriable( retryDelay(retryCount) { return backoff(retryCount) * 1000; // return delay in milliseconds }, - retryCondition(err) { - return isNetworkError(err) || isRetryableError(err); - }, + retryCondition, }); } + +const RETRIABLE_HTTP_CODES = [408, 429]; + +export function defaultRetryCondition(err: AxiosError): boolean { + // retry on TCP/IP error codes like ECONNRESET + if (isNetworkError(err)) { + return true; + } + + if (err.code === 'ECONNABORTED') { + return false; + } + + if (!err.response) { + return true; + } + + if (err.response.status >= 500 && err.response.status <= 599) { + return true; + } + + if (RETRIABLE_HTTP_CODES.includes(err.response.status)) { + return true; + } + + return false; +} From 7475e2289ab460809c027d850a8e5376ee249c6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Fri, 13 Oct 2023 03:48:26 +0200 Subject: [PATCH 4/8] feat(node): regenerate idempotency key on status 422 --- packages/node/src/lib/retries.spec.ts | 43 +++++++++++++++++++++++++-- packages/node/src/lib/retries.ts | 14 +++++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/packages/node/src/lib/retries.spec.ts b/packages/node/src/lib/retries.spec.ts index 0108a14f20e..8602f9b62c6 100644 --- a/packages/node/src/lib/retries.spec.ts +++ b/packages/node/src/lib/retries.spec.ts @@ -7,6 +7,8 @@ const TRIGGER_PATH = '/v1/events/trigger'; jest.setTimeout(15000); +const allEqual = (arr: Array) => arr.every((val) => val === arr[0]); + class NetworkError extends Error { constructor(public code: string) { super('Network Error'); @@ -30,10 +32,42 @@ describe('Novu Node.js package - Retries and idempotency key', () => { it('should retry trigger', async () => { // prepare backend api mock endpoints: + const idepotencyKeys: string[] = []; + nock(BACKEND_URL) .post(TRIGGER_PATH) .times(3) - .reply(500, { message: 'Server Exception' }); + .reply(function (_url, _body) { + idepotencyKeys.push(this.req.getHeader('idempotency-key') as string); + + return [500, { message: 'Server Exception' }]; + }); + + nock(BACKEND_URL) + .post(TRIGGER_PATH) + .reply(201, { acknowledged: true, transactionId: '1003' }); + + const result = await novu.trigger('fake-workflow', { + to: { subscriberId: '123' }, + payload: {}, + }); + + expect(allEqual(idepotencyKeys)).toBeTruthy(); + expect(result.status).toEqual(201); + expect(result.request.headers['idempotency-key']).toBeDefined(); + }); + + it('should retry and regenerate idempotency key on status 422', async () => { + const idepotencyKeys: string[] = []; + + nock(BACKEND_URL) + .post(TRIGGER_PATH) + .times(3) + .reply(function (_url, _body) { + idepotencyKeys.push(this.req.getHeader('idempotency-key') as string); + + return [422, { message: 'Unprocessable Content' }]; + }); nock(BACKEND_URL) .post(TRIGGER_PATH) @@ -44,6 +78,7 @@ describe('Novu Node.js package - Retries and idempotency key', () => { payload: {}, }); + expect(allEqual(idepotencyKeys)).toBeFalsy(); expect(result.status).toEqual(201); expect(result.request.headers['idempotency-key']).toBeDefined(); }); @@ -87,6 +122,10 @@ describe('Novu Node.js package - Retries and idempotency key', () => { .get(TOPICS_PATH) .reply(504, { message: 'Gateway timeout' }); + nock(BACKEND_URL) + .get(TOPICS_PATH) + .reply(422, { message: 'Unprocessable Content' }); + nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); const novuClient = new Novu('fake-key', { @@ -95,7 +134,7 @@ describe('Novu Node.js package - Retries and idempotency key', () => { initialDelay: 0, waitMin: 1, waitMax: 1, - retryMax: 6, + retryMax: 7, }, }); diff --git a/packages/node/src/lib/retries.ts b/packages/node/src/lib/retries.ts index f472f09ce6d..8c303da1eaa 100644 --- a/packages/node/src/lib/retries.ts +++ b/packages/node/src/lib/retries.ts @@ -51,14 +51,24 @@ export function makeRetriable( axiosRetry(axios, { retries, + retryCondition, retryDelay(retryCount) { return backoff(retryCount) * 1000; // return delay in milliseconds }, - retryCondition, + onRetry(_retryCount, error, requestConfig) { + if ( + error.response?.status === 422 && + requestConfig.headers && + requestConfig.method && + NON_IDEMPOTENT_METHODS.includes(requestConfig.method) + ) { + requestConfig.headers['Idempotency-Key'] = uuid(); + } + }, }); } -const RETRIABLE_HTTP_CODES = [408, 429]; +const RETRIABLE_HTTP_CODES = [408, 429, 422]; export function defaultRetryCondition(err: AxiosError): boolean { // retry on TCP/IP error codes like ECONNRESET From 6670e327da333fdb624459f37c0cf27c2d4b9e2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Sat, 21 Oct 2023 03:38:48 +0200 Subject: [PATCH 5/8] test(node): add cases for common 4xx errors --- packages/node/src/lib/retries.spec.ts | 17 +++++++++++++++++ packages/node/src/lib/retries.ts | 4 ---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/node/src/lib/retries.spec.ts b/packages/node/src/lib/retries.spec.ts index 8602f9b62c6..3250eb00cf2 100644 --- a/packages/node/src/lib/retries.spec.ts +++ b/packages/node/src/lib/retries.spec.ts @@ -1,3 +1,4 @@ +import { AxiosError } from 'axios'; import nock from 'nock'; import { Novu } from '../index'; @@ -97,6 +98,22 @@ describe('Novu Node.js package - Retries and idempotency key', () => { expect(result.request.headers['idempotency-key']).toBeUndefined(); }); + it('should not retry on common 4xx errors', async () => { + nock(BACKEND_URL) + .get(TOPICS_PATH) + .times(3) + .reply(404, { message: 'Not Found' }); + + nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); + + try { + await novu.topics.list({}); + throw new Error(); + } catch (err) { + expect((err as AxiosError).response?.status).toEqual(404); + } + }); + it('should retry on various errors until it reach successfull response', async () => { nock(BACKEND_URL) .get(TOPICS_PATH) diff --git a/packages/node/src/lib/retries.ts b/packages/node/src/lib/retries.ts index 8c303da1eaa..757cd77e81c 100644 --- a/packages/node/src/lib/retries.ts +++ b/packages/node/src/lib/retries.ts @@ -76,10 +76,6 @@ export function defaultRetryCondition(err: AxiosError): boolean { return true; } - if (err.code === 'ECONNABORTED') { - return false; - } - if (!err.response) { return true; } From 071d59f06c169d321a295b7834741a15dab26446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Fri, 3 Nov 2023 03:50:33 +0100 Subject: [PATCH 6/8] feat(node): improve retries feature --- packages/node/src/index.ts | 2 +- packages/node/src/lib/novu.ts | 4 +- .../lib/{retries.spec.ts => retry.spec.ts} | 88 ++++++++++++++----- .../node/src/lib/{retries.ts => retry.ts} | 46 +++++----- 4 files changed, 93 insertions(+), 47 deletions(-) rename packages/node/src/lib/{retries.spec.ts => retry.spec.ts} (56%) rename packages/node/src/lib/{retries.ts => retry.ts} (61%) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 5ce92c76017..e1104a7e0de 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -30,4 +30,4 @@ export * from './lib/feeds/feeds.interface'; export * from './lib/topics/topic.interface'; export * from './lib/integrations/integrations.interface'; export * from './lib/messages/messages.interface'; -export { defaultRetryCondition } from './lib/retries'; +export { defaultRetryCondition } from './lib/retry'; diff --git a/packages/node/src/lib/novu.ts b/packages/node/src/lib/novu.ts index 616b641f434..1cb15573088 100644 --- a/packages/node/src/lib/novu.ts +++ b/packages/node/src/lib/novu.ts @@ -13,7 +13,7 @@ import { Topics } from './topics/topics'; import { Integrations } from './integrations/integrations'; import { Messages } from './messages/messages'; import { Tenants } from './tenants/tenants'; -import { makeRetriable } from './retries'; +import { makeRetryable } from './retry'; export class Novu extends EventEmitter { private readonly apiKey?: string; @@ -42,7 +42,7 @@ export class Novu extends EventEmitter { }); if (config?.retryConfig) { - makeRetriable(axiosInstance, config); + makeRetryable(axiosInstance, config); } this.http = axiosInstance; diff --git a/packages/node/src/lib/retries.spec.ts b/packages/node/src/lib/retry.spec.ts similarity index 56% rename from packages/node/src/lib/retries.spec.ts rename to packages/node/src/lib/retry.spec.ts index 3250eb00cf2..3db018fb78b 100644 --- a/packages/node/src/lib/retries.spec.ts +++ b/packages/node/src/lib/retry.spec.ts @@ -1,4 +1,3 @@ -import { AxiosError } from 'axios'; import nock from 'nock'; import { Novu } from '../index'; @@ -8,7 +7,7 @@ const TRIGGER_PATH = '/v1/events/trigger'; jest.setTimeout(15000); -const allEqual = (arr: Array) => arr.every((val) => val === arr[0]); +const allEqual = (arr: Array) => arr.every((val) => val === arr[0]); class NetworkError extends Error { constructor(public code: string) { @@ -16,7 +15,7 @@ class NetworkError extends Error { } } -describe('Novu Node.js package - Retries and idempotency key', () => { +describe('Novu Node.js package - Retries and idempotency-key', () => { afterEach(() => { nock.cleanAll(); nock.enableNetConnect(); @@ -31,15 +30,14 @@ describe('Novu Node.js package - Retries and idempotency key', () => { }, }); - it('should retry trigger', async () => { - // prepare backend api mock endpoints: - const idepotencyKeys: string[] = []; + it('should retry trigger and generate idempotency-key only once for request', async () => { + const idempotencyKeys: string[] = []; nock(BACKEND_URL) .post(TRIGGER_PATH) .times(3) .reply(function (_url, _body) { - idepotencyKeys.push(this.req.getHeader('idempotency-key') as string); + idempotencyKeys.push(this.req.getHeader('idempotency-key') as string); return [500, { message: 'Server Exception' }]; }); @@ -53,19 +51,44 @@ describe('Novu Node.js package - Retries and idempotency key', () => { payload: {}, }); - expect(allEqual(idepotencyKeys)).toBeTruthy(); + // all idempotency keys are supposed to be same. + expect(allEqual(idempotencyKeys)).toBeTruthy(); expect(result.status).toEqual(201); expect(result.request.headers['idempotency-key']).toBeDefined(); }); - it('should retry and regenerate idempotency key on status 422', async () => { - const idepotencyKeys: string[] = []; + it('should generate different idempotency-key for each request', async () => { + nock(BACKEND_URL) + .post(TRIGGER_PATH) + .reply(500, { message: 'Server Exception' }); + + nock(BACKEND_URL) + .post(TRIGGER_PATH) + .times(10) + .reply(201, { acknowledged: true, transactionId: '1003' }); + + const idempotencyKeys: string[] = []; + + for (let i = 0; i < 10; i++) { + const result = await novu.trigger('fake-workflow', { + to: { subscriberId: '123' }, + payload: {}, + }); + + idempotencyKeys.push(result.request?.headers['idempotency-key']); + } + + expect(allEqual(idempotencyKeys)).toEqual(false); + }); + + it('should retry on status 422 and regenerate idempotency-key for every retry', async () => { + const idempotencyKeys: string[] = []; nock(BACKEND_URL) .post(TRIGGER_PATH) .times(3) .reply(function (_url, _body) { - idepotencyKeys.push(this.req.getHeader('idempotency-key') as string); + idempotencyKeys.push(this.req.getHeader('idempotency-key') as string); return [422, { message: 'Unprocessable Content' }]; }); @@ -79,7 +102,8 @@ describe('Novu Node.js package - Retries and idempotency key', () => { payload: {}, }); - expect(allEqual(idepotencyKeys)).toBeFalsy(); + // idempotency key should be regenerated for every retry for http status 422. + expect(allEqual(idempotencyKeys)).toBeFalsy(); expect(result.status).toEqual(201); expect(result.request.headers['idempotency-key']).toBeDefined(); }); @@ -98,23 +122,43 @@ describe('Novu Node.js package - Retries and idempotency key', () => { expect(result.request.headers['idempotency-key']).toBeUndefined(); }); - it('should not retry on common 4xx errors', async () => { + it('should fail after reaching max retries', async () => { nock(BACKEND_URL) .get(TOPICS_PATH) - .times(3) - .reply(404, { message: 'Not Found' }); + .times(4) + .reply(500, { message: 'Server Exception' }); nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); - try { - await novu.topics.list({}); - throw new Error(); - } catch (err) { - expect((err as AxiosError).response?.status).toEqual(404); - } + await expect(novu.topics.list({})).rejects.toMatchObject({ + response: { status: 500 }, + }); }); - it('should retry on various errors until it reach successfull response', async () => { + const NON_RECOVERABLE_ERRORS: Array<[number, string]> = [ + [400, 'Bad Request'], + [401, 'Unauthorized'], + [403, 'Forbidden'], + [404, 'Not Found'], + [405, 'Method not allowed'], + [413, 'Payload Too Large'], + [414, 'URI Too Long'], + [415, 'Unsupported Media Type'], + ]; + + test.each<[number, string]>(NON_RECOVERABLE_ERRORS)( + 'should not retry on non-recoverable %i error', + async (status, message) => { + nock(BACKEND_URL).get(TOPICS_PATH).times(3).reply(status, { message }); + nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]); + + await expect(novu.topics.list({})).rejects.toMatchObject({ + response: { status }, + }); + } + ); + + it('should retry on various errors until it reach successful response', async () => { nock(BACKEND_URL) .get(TOPICS_PATH) .reply(429, { message: 'Too many requests' }); diff --git a/packages/node/src/lib/retries.ts b/packages/node/src/lib/retry.ts similarity index 61% rename from packages/node/src/lib/retries.ts rename to packages/node/src/lib/retry.ts index 757cd77e81c..c9376d453f6 100644 --- a/packages/node/src/lib/retries.ts +++ b/packages/node/src/lib/retry.ts @@ -4,35 +4,37 @@ import { v4 as uuid } from 'uuid'; import { INovuConfiguration } from './novu.interface'; const NON_IDEMPOTENT_METHODS = ['post', 'patch']; +const IDEMPOTENCY_KEY = 'Idempotency-Key'; // header key -export function makeRetriable( +const DEFAULT_RETRY_MAX = 0; +const DEFAULT_WAIT_MIN = 1; +const DEFAULT_WAIT_MAX = 30; + +export function makeRetryable( axios: AxiosInstance, config?: INovuConfiguration ) { axios.interceptors.request.use((axiosConfig) => { - // don't attach idempotency key for idempotent methods if ( axiosConfig.method && - !NON_IDEMPOTENT_METHODS.includes(axiosConfig.method) + NON_IDEMPOTENT_METHODS.includes(axiosConfig.method) ) { - return axiosConfig; - } + const idempotencyKey = axiosConfig.headers[IDEMPOTENCY_KEY]; + // that means intercepted request is retried, so don't generate new idempotency key + if (idempotencyKey) { + return axiosConfig; + } - const idempotencyKey = axiosConfig.headers['Idempotency-Key']; - // that means intercepted request is retried, so don't generate new idempotency key - if (idempotencyKey) { - return axiosConfig; + axiosConfig.headers[IDEMPOTENCY_KEY] = uuid(); } - axiosConfig.headers['Idempotency-Key'] = uuid(); - return axiosConfig; }); const retryConfig = config?.retryConfig || {}; - const retries = retryConfig.retryMax || 0; - const minDelay = retryConfig.waitMin || 1; - const maxDelay = retryConfig.waitMax || 30; + const retries = retryConfig.retryMax || DEFAULT_RETRY_MAX; + const minDelay = retryConfig.waitMin || DEFAULT_WAIT_MIN; + const maxDelay = retryConfig.waitMax || DEFAULT_WAIT_MAX; const initialDelay = retryConfig.initialDelay || minDelay; const retryCondition = retryConfig.retryCondition || defaultRetryCondition; @@ -62,13 +64,13 @@ export function makeRetriable( requestConfig.method && NON_IDEMPOTENT_METHODS.includes(requestConfig.method) ) { - requestConfig.headers['Idempotency-Key'] = uuid(); + requestConfig.headers[IDEMPOTENCY_KEY] = uuid(); } }, }); } -const RETRIABLE_HTTP_CODES = [408, 429, 422]; +const RETRYABLE_HTTP_CODES = [408, 429, 422]; export function defaultRetryCondition(err: AxiosError): boolean { // retry on TCP/IP error codes like ECONNRESET @@ -76,15 +78,15 @@ export function defaultRetryCondition(err: AxiosError): boolean { return true; } - if (!err.response) { - return true; - } - - if (err.response.status >= 500 && err.response.status <= 599) { + if ( + err.response && + err.response.status >= 500 && + err.response.status <= 599 + ) { return true; } - if (RETRIABLE_HTTP_CODES.includes(err.response.status)) { + if (err.response && RETRYABLE_HTTP_CODES.includes(err.response.status)) { return true; } From a3e150d2c1c9f0c806f0064854d8d9a4b92bf22a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Thu, 9 Nov 2023 05:16:02 +0100 Subject: [PATCH 7/8] test(node): add more tests for retries feature --- packages/node/src/lib/retry.spec.ts | 90 +++++++++++++++++++++++++---- packages/node/src/lib/retry.ts | 3 +- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/packages/node/src/lib/retry.spec.ts b/packages/node/src/lib/retry.spec.ts index 3db018fb78b..19270066d10 100644 --- a/packages/node/src/lib/retry.spec.ts +++ b/packages/node/src/lib/retry.spec.ts @@ -1,5 +1,7 @@ +import { AxiosError } from 'axios'; import nock from 'nock'; -import { Novu } from '../index'; +import { Novu, defaultRetryCondition } from '../index'; +import { RETRYABLE_HTTP_CODES } from './retry'; const BACKEND_URL = 'http://example.com'; const TOPICS_PATH = '/v1/topics'; @@ -7,7 +9,9 @@ const TRIGGER_PATH = '/v1/events/trigger'; jest.setTimeout(15000); -const allEqual = (arr: Array) => arr.every((val) => val === arr[0]); +const hasAllEqual = (arr: Array) => arr.every((val) => val === arr[0]); +const hasUniqueOnly = (arr: Array) => + Array.from(new Set(arr)).length === arr.length; class NetworkError extends Error { constructor(public code: string) { @@ -15,6 +19,15 @@ class NetworkError extends Error { } } +class HttpError extends Error { + readonly response: { status: number }; + + constructor(status: number) { + super('Http Error'); + this.response = { status }; + } +} + describe('Novu Node.js package - Retries and idempotency-key', () => { afterEach(() => { nock.cleanAll(); @@ -25,8 +38,8 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { backendUrl: BACKEND_URL, retryConfig: { retryMax: 3, - waitMax: 1, - waitMin: 1, + waitMax: 0.5, + waitMin: 0.2, }, }); @@ -52,7 +65,7 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { }); // all idempotency keys are supposed to be same. - expect(allEqual(idempotencyKeys)).toBeTruthy(); + expect(hasAllEqual(idempotencyKeys)).toEqual(true); expect(result.status).toEqual(201); expect(result.request.headers['idempotency-key']).toBeDefined(); }); @@ -78,7 +91,7 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { idempotencyKeys.push(result.request?.headers['idempotency-key']); } - expect(allEqual(idempotencyKeys)).toEqual(false); + expect(hasUniqueOnly(idempotencyKeys)).toEqual(true); }); it('should retry on status 422 and regenerate idempotency-key for every retry', async () => { @@ -87,7 +100,7 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { nock(BACKEND_URL) .post(TRIGGER_PATH) .times(3) - .reply(function (_url, _body) { + .reply(function () { idempotencyKeys.push(this.req.getHeader('idempotency-key') as string); return [422, { message: 'Unprocessable Content' }]; @@ -103,7 +116,7 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { }); // idempotency key should be regenerated for every retry for http status 422. - expect(allEqual(idempotencyKeys)).toBeFalsy(); + expect(hasUniqueOnly(idempotencyKeys)).toEqual(true); expect(result.status).toEqual(201); expect(result.request.headers['idempotency-key']).toBeDefined(); }); @@ -193,8 +206,8 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { backendUrl: BACKEND_URL, retryConfig: { initialDelay: 0, - waitMin: 1, - waitMax: 1, + waitMin: 0.2, + waitMax: 0.5, retryMax: 7, }, }); @@ -202,4 +215,61 @@ describe('Novu Node.js package - Retries and idempotency-key', () => { const result = await novuClient.topics.list({}); expect(result.status).toEqual(200); }); + + describe('defaultRetryCondition function', () => { + test.each<[number, string]>(NON_RECOVERABLE_ERRORS)( + 'should return false when HTTP status is %i', + (status) => { + const err = new HttpError(status); + expect(defaultRetryCondition(err as AxiosError)).toEqual(false); + } + ); + + test.each(RETRYABLE_HTTP_CODES)( + 'should return true when HTTP status is %i', + (status) => { + const err = new HttpError(status); + expect(defaultRetryCondition(err as AxiosError)).toEqual(true); + } + ); + + it('should return true when HTTP status is 500', () => { + const err = new HttpError(500); + expect(defaultRetryCondition(err as AxiosError)).toEqual(true); + }); + + it('should return true when network code is ECONNRESET', () => { + const err = new NetworkError('ECONNRESET'); + expect(defaultRetryCondition(err as AxiosError)).toEqual(true); + }); + + it('shoud return false on unknown error', () => { + const err = new Error('Unexpected error'); + expect(defaultRetryCondition(err as AxiosError)).toEqual(false); + }); + }); + + describe('hasAllEqual helper function', () => { + it('should return true when all items are equal', () => { + const arr = ['a', 'a', 'a', 'a']; + expect(hasAllEqual(arr)).toEqual(true); + }); + + it('should return false when items are not equal', () => { + const arr = ['a', 'b', 'b', 'b']; + expect(hasAllEqual(arr)).toEqual(false); + }); + }); + + describe('hasUniqueOnly helper function', () => { + it('should return true when all items are unique', () => { + const arr = ['a', 'b', 'c', 'd']; + expect(hasUniqueOnly(arr)).toEqual(true); + }); + + it('should return false when items are not unique', () => { + const arr = ['a', 'a', 'c', 'd']; + expect(hasUniqueOnly(arr)).toEqual(false); + }); + }); }); diff --git a/packages/node/src/lib/retry.ts b/packages/node/src/lib/retry.ts index c9376d453f6..22d79fe4a8a 100644 --- a/packages/node/src/lib/retry.ts +++ b/packages/node/src/lib/retry.ts @@ -3,6 +3,7 @@ import axiosRetry, { isNetworkError } from 'axios-retry'; import { v4 as uuid } from 'uuid'; import { INovuConfiguration } from './novu.interface'; +export const RETRYABLE_HTTP_CODES = [408, 422, 429]; const NON_IDEMPOTENT_METHODS = ['post', 'patch']; const IDEMPOTENCY_KEY = 'Idempotency-Key'; // header key @@ -70,8 +71,6 @@ export function makeRetryable( }); } -const RETRYABLE_HTTP_CODES = [408, 429, 422]; - export function defaultRetryCondition(err: AxiosError): boolean { // retry on TCP/IP error codes like ECONNRESET if (isNetworkError(err)) { From 024d8cdaa0c78a061bcee74f8a53c00c9fd02c0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dziuba?= Date: Mon, 20 Nov 2023 21:47:11 +0100 Subject: [PATCH 8/8] fix: retryable to cspell --- .cspell.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.cspell.json b/.cspell.json index 0edd7496b5b..14363ce5df6 100644 --- a/.cspell.json +++ b/.cspell.json @@ -519,6 +519,9 @@ "idempotency", "IDEMPOTENCY", "Idempotency", + "Retryable", + "RETRYABLE", + "retryable", "messagebird", "Datetime", "pubid",