refactor: make error handling into separate methods per type of error

Unleash · Nov 16, 2023 · a6df198 · a6df198
1 parent 799a51c
commit a6df198
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 63 deletions.
diff --git 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<void> {
     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
@@ -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<void>((resolve) => setTimeout(() => {
+    const timeout = new Promise<void>((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
@@ -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<void>((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<void>((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();