From a6df198c43b26730210c064f9e5916987851f96a Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 16 Nov 2023 13:48:30 +0100 Subject: [PATCH] refactor: make error handling into separate methods per type of error --- src/repository/index.ts | 108 ++++++++++++++++++++++-------------- src/test/metrics.test.ts | 2 +- src/test/repository.test.ts | 43 +++++++------- 3 files changed, 90 insertions(+), 63 deletions(-) diff --git a/src/repository/index.ts b/src/repository/index.ts index 9b6e63e3..01ba7bb9 100644 --- a/src/repository/index.ts +++ b/src/repository/index.ts @@ -266,13 +266,75 @@ Message: ${err.message}`, return this.nextFetch(); } + // Emits correct error message based on what failed, + // and returns 0 as the next fetch interval (stop polling) + private unrecoverableError(url: string, statusCode: number): number { + this.failures += 1; + if (statusCode === 404) { + this.emit( + UnleashEvents.Error, + new Error( + // eslint-disable-next-line max-len + `${url} responded NOT_FOUND (404) which means your API url most likely needs correction. Stopping refresh of toggles`, + ), + ); + } else if (statusCode === 401 || statusCode === 403) { + this.emit( + UnleashEvents.Error, + new Error( + // eslint-disable-next-line max-len + `${url} responded ${statusCode} which means your API key is not allowed to connect. Stopping refresh of toggles`, + ), + ); + } + return 0; + } + + // We got a status code we know what to do with, so will log correct message + // and return the new interval. + private recoverableError(url: string, statusCode: number): number { + let nextFetch = this.backoff(); + if (statusCode === 429) { + this.emit( + UnleashEvents.Warn, + // eslint-disable-next-line max-len + `${url} responded TOO_MANY_CONNECTIONS (429). Backing off`, + ); + } else if (statusCode === 500 || + statusCode === 502 || + statusCode === 503 || + statusCode === 504) { + this.emit( + UnleashEvents.Warn, + `${url} responded ${statusCode}. Backing off`, + ); + } + return nextFetch; + } + + private handleErrorCases(url: string, statusCode: number): number { + if (statusCode === 401 || statusCode === 403 || statusCode === 404) { + return this.unrecoverableError(url, statusCode); + } else if ( + statusCode === 429 || + statusCode === 500 || + statusCode === 502 || + statusCode === 503 || + statusCode === 504 + ) { + return this.recoverableError(url, statusCode); + } else { + const error = new Error(`Response was not statusCode 2XX, but was ${statusCode}`); + this.emit(UnleashEvents.Error, error); + return this.refreshInterval; + } + } + async fetch(): Promise { if (this.stopped || !(this.refreshInterval > 0)) { return; } - let nextFetch = this.refreshInterval; - try { let mergedTags; if (this.tags) { @@ -310,47 +372,7 @@ Message: ${err.message}`, this.emit(UnleashEvents.Error, err); } } else { - if (res.status === 401 || res.status === 403) { - this.emit( - UnleashEvents.Error, - new Error( - // eslint-disable-next-line max-len - `${url} responded ${res.status} which means your API key is not allowed to connect. Stopping refresh of toggles`, - ), - ); - this.failures += 1; - nextFetch = 0; - } else if (res.status === 404) { - this.emit( - UnleashEvents.Error, - new Error( - // eslint-disable-next-line max-len - `${url} responded NOT_FOUND (${res.status}) which means your API url most likely needs correction. Stopping refresh of toggles`, - ), - ); - nextFetch = 0; - } else if (res.status === 429) { - nextFetch = this.backoff(); - this.emit( - UnleashEvents.Warn, - // eslint-disable-next-line max-len - `${url} responded TOO_MANY_CONNECTIONS (${res.status}). Waiting for ${nextFetch}ms before trying again.`, - ); - } else if ( - res.status === 500 || - res.status === 502 || - res.status === 503 || - res.status === 504 - ) { - nextFetch = this.backoff(); - this.emit( - UnleashEvents.Warn, - `${url} responded ${res.status}. Waiting for ${nextFetch}ms before trying again.`, - ); - } else { - const error = new Error(`Response was not statusCode 2XX, but was ${res.status}`); - this.emit(UnleashEvents.Error, error); - } + nextFetch = this.handleErrorCases(url, res.status); } } catch (err) { const e = err as { code: string }; diff --git a/src/test/metrics.test.ts b/src/test/metrics.test.ts index ab54efef..d24820be 100644 --- a/src/test/metrics.test.ts +++ b/src/test/metrics.test.ts @@ -77,7 +77,7 @@ test('should sendMetrics and register when metricsInterval is a positive number' metrics.count('toggle-x', false); metrics.count('toggle-y', true); metrics.start(); - let timeout = new Promise((resolve) => setTimeout(() => { + const timeout = new Promise((resolve) => setTimeout(() => { t.fail("Failed to successfully both send and register"); resolve(); }, 1000)); diff --git a/src/test/repository.test.ts b/src/test/repository.test.ts index 5cf7a02d..0256106a 100644 --- a/src/test/repository.test.ts +++ b/src/test/repository.test.ts @@ -249,29 +249,34 @@ test('request with customHeadersFunction should take precedence over customHeade repo.start(); })); -test('should handle 429 request error and emit warn event', (t) => - new Promise((resolve) => { - const url = 'http://unleash-test-6-429.app'; - nock(url).persist().get('/client/features').reply(429, 'blabla'); - const repo = new Repository({ - url, - appName, - instanceId, - refreshInterval: 10, - // @ts-expect-error - bootstrapProvider: new DefaultBootstrapProvider({}), - storageProvider: new InMemStorageProvider(), - }); +test.only('should handle 429 request error and emit warn event', async (t) => { + const url = 'http://unleash-test-6-429.app'; + nock(url).persist().get('/client/features').reply(429, 'blabla'); + const repo = new Repository({ + url, + appName, + instanceId, + refreshInterval: 10, + // @ts-expect-error + bootstrapProvider: new DefaultBootstrapProvider({}), + storageProvider: new InMemStorageProvider(), + }); + const warning = new Promise((resolve) => { repo.on('warn', (warn) => { t.truthy(warn); - // eslint-disable-next-line max-len - t.is(warn, `${url}/client/features responded TOO_MANY_CONNECTIONS (429). Waiting for 20ms before trying again.`); - t.is(repo.nextFetch(), 20); + t.is(warn, `${url}/client/features responded TOO_MANY_CONNECTIONS (429). Backing off`); t.is(repo.getFailures(), 1); + t.is(repo.nextFetch(), 20); resolve(); }); - repo.start(); - })); + }); + const timeout = new Promise((resolve) => setTimeout(() => { + t.fail("Failed to get warning about connections"); + resolve(); + }, 5000)); + await repo.start(); + await Promise.race([warning, timeout]); +}); test('should handle 401 request error and emit error event', (t) => new Promise((resolve) => { @@ -332,7 +337,7 @@ test('should handle 500 request error and emit warn event', (t) => }); repo.on('warn', (warn) => { t.truthy(warn); - t.is(warn, `${url}/client/features responded 500. Waiting for 20ms before trying again.`); + t.is(warn, `${url}/client/features responded 500. Backing off`); resolve(); }); repo.start();