From 66942af6c16f863c4e41928462bb624aa0047488 Mon Sep 17 00:00:00 2001 From: Micah Yeager Date: Fri, 14 Jun 2024 08:37:13 -0400 Subject: [PATCH 1/3] feat: implement automatic retries --- index.js | 46 ++++++++++++++++++++++++++++++++++++++++++++-- index.test.js | 29 ++++++++++++++++++++++++++++- jest.config.js | 10 ++++++++++ 3 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 jest.config.js diff --git a/index.js b/index.js index 153f969..a98229c 100644 --- a/index.js +++ b/index.js @@ -32,7 +32,14 @@ class AutotaskRestApi { * @param {object} options * @param {string} options.base_url the REST API base url. (Default https://webservices2.autotask.net/ATServicesRest/) * @param {string} options.version Autotask REST API decimal version (e.g. 1.0). (Default 1.0); - * + * @param {object?} options.retry Retry options for the connector. + * @param {boolean?} options.retry.enabled If true, will retry the request as configured. (Default true) + * @param {number?} options.retry.attempts If retry_on_error is true, the number of times to retry the request. + * (Default 10) + * @param {number?} options.retry.delay If retry_on_error is true, the number of milliseconds to wait before trying + * again. (Default 1000) + * @param {number?} options.retry.delay_factor If retry_on_error is true, the factor by which to increase the delay + * between retries. (Default 2) */ constructor(user, secret, code, options){ if(!user)throw new Error(`An API user is required.`); @@ -46,6 +53,12 @@ class AutotaskRestApi { this.base_url = `https://webservices.autotask.net/ATServicesRest/`; //As returned by zoneInformation.url this.version = '1.0'; + this.retryOptions = options?.retry ?? {}; + this.retryOptions.enabled ??= true; + this.retryOptions.attempts ??= 10; + this.retryOptions.delay ??= 1000; + this.retryOptions.delay_factor ??= 2; + if(options){ if(options.base_url){ this.base_url = options.base_url; @@ -376,6 +389,35 @@ class AutotaskRestApi { * @param {boolean} opts.ImpersonationResourceId specifies an Autotask Resource ID to impersonate on a create/update operation */ async _fetch(method, endpoint, query, payload, opts){ + let attempts = 0; + /** + * Wrapper around `fetch` that retries on 429 and 5xx errors. + * + * @param {string | URL | Request} input Passed directly to `fetch`. + * @param {RequestInit?} init Passed directly to `fetch`. + * @returns {Promise} Same as `fetch`. + * @see fetch + */ + const fetchWithRetry = async (input, init) => { + attempts++; + const response = await fetch(input, init); + // Exit early if no errors. + if (response.ok) return response; + + // Retry on 429 or 5xx errors, if configured to do so. + if (this.retryOptions.enabled && attempts < this.retryOptions.attempts && response.status === 429) { + // Exponential backoff. + const delay = this.retryOptions.delay * Math.pow(this.retryOptions.delay_factor, attempts); + await new Promise(resolve => setTimeout(resolve, delay)); + // Retry. + return fetchWithRetry(input, init); + } + + // If we're here, we've encountered a non-retryable error or exhausted our retries. In this case, just return the + // response as-is. + return response; + } + try{ if(!this.zoneInfo){ //Lazy init zone info on the fly. @@ -430,7 +472,7 @@ class AutotaskRestApi { fetchParms.agent = new https.Agent({ secureOptions: crypto.constants.SSL_OP_LEGACY_SERVER_CONNECT }); - let response = await fetch(`${full_url}`, fetchParms); + let response = await fetchWithRetry(`${full_url}`, fetchParms); if(response.ok){ let result = await response.json(); diff --git a/index.test.js b/index.test.js index f0c78a8..e00f4ad 100644 --- a/index.test.js +++ b/index.test.js @@ -1,5 +1,5 @@ require('dotenv').config(); -let {AutotaskRestApi, FilterOperators} = require('.'); +let {AutotaskRestApi, FilterOperators, AutotaskApiError} = require('.'); var autotask = null; beforeAll(async ()=>{ @@ -25,6 +25,33 @@ it('can get by id', async () => { expect(company.id).toBe(0); }); +describe('retries', () => { + it('should retry on 429', async () => { + // Mock fetch to return 429 once, then reset. + const fetchMock = jest.spyOn(global, 'fetch').mockImplementationOnce(() => Promise.resolve({ + status: 429, ok: false, json: () => Promise.resolve({}), + })); + + let result = await autotask.Companies.get(0); + let company = result.item + + expect(company).toBeDefined(); + expect(company.id).toBe(0); + expect(fetchMock).toHaveBeenCalledTimes(2); + }) + + it('should not retry if disabled', async () => { + // Mock fetch to return 429 once, then reset. + const fetchMock = jest.spyOn(global, 'fetch').mockImplementationOnce(() => Promise.resolve({ + status: 429, ok: false, text: () => Promise.resolve('') + })); + jest.replaceProperty(autotask.retryOptions, 'enabled', false); + + await expect(autotask.Companies.get(0)).rejects.toThrow(AutotaskApiError); + expect(fetchMock).toHaveBeenCalledTimes(1); + }) +}) + test('can query multiple.', async () => { let result = await autotask.Companies.query({filter:[ diff --git a/jest.config.js b/jest.config.js new file mode 100644 index 0000000..bcd03f7 --- /dev/null +++ b/jest.config.js @@ -0,0 +1,10 @@ +/** @type {import('jest').Config} */ +const config = { + // Since these are integration tests with a relatively slow API, increase the timeout to 20s. + testTimeout: 20_000, + clearMocks: true, + // Increase timeout from the default of 1s to 5s to allow time for fetch's TCPWRAP and TLSWRAP handles to close. + openHandlesTimeout: 5_000 +}; + +module.exports = config; From b97a0199ea0ae0aef0a39c3f09ee5a491d62a0ac Mon Sep 17 00:00:00 2001 From: Micah Yeager Date: Fri, 14 Jun 2024 16:08:29 -0400 Subject: [PATCH 2/3] docs: remove reference to unused retry parameter --- index.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index a98229c..20dbb5d 100644 --- a/index.js +++ b/index.js @@ -34,12 +34,11 @@ class AutotaskRestApi { * @param {string} options.version Autotask REST API decimal version (e.g. 1.0). (Default 1.0); * @param {object?} options.retry Retry options for the connector. * @param {boolean?} options.retry.enabled If true, will retry the request as configured. (Default true) - * @param {number?} options.retry.attempts If retry_on_error is true, the number of times to retry the request. - * (Default 10) - * @param {number?} options.retry.delay If retry_on_error is true, the number of milliseconds to wait before trying - * again. (Default 1000) - * @param {number?} options.retry.delay_factor If retry_on_error is true, the factor by which to increase the delay - * between retries. (Default 2) + * @param {number?} options.retry.attempts If enabled, the number of times to retry the request. (Default 10) + * @param {number?} options.retry.delay If enabled, the number of milliseconds to wait before trying again. + * (Default 1000) + * @param {number?} options.retry.delay_factor If enabled, the factor by which to increase the delay between retries. + * (Default 2) */ constructor(user, secret, code, options){ if(!user)throw new Error(`An API user is required.`); From 1c77d01d2aa8f69ae9ad81a856323047d7822524 Mon Sep 17 00:00:00 2001 From: Micah Yeager Date: Fri, 14 Jun 2024 16:12:46 -0400 Subject: [PATCH 3/3] refactor: remove Jest options that aren't specifically needed for this --- jest.config.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/jest.config.js b/jest.config.js index bcd03f7..e34a1ae 100644 --- a/jest.config.js +++ b/jest.config.js @@ -1,10 +1,6 @@ /** @type {import('jest').Config} */ const config = { - // Since these are integration tests with a relatively slow API, increase the timeout to 20s. - testTimeout: 20_000, clearMocks: true, - // Increase timeout from the default of 1s to 5s to allow time for fetch's TCPWRAP and TLSWRAP handles to close. - openHandlesTimeout: 5_000 }; module.exports = config;