fix(idempotency): check error identity via names (#2747)

dreamorosi · web-flow · commit 55c387816bae · 2024-07-09T09:14:21.000+02:00
diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts
@@ -12,6 +12,7 @@ import {
   IdempotencyInconsistentStateError,
   IdempotencyItemAlreadyExistsError,
   IdempotencyPersistenceLayerError,
+  IdempotencyUnknownError,
 } from './errors.js';
 import { BasePersistenceLayer } from './persistence/BasePersistenceLayer.js';
 import { IdempotencyRecord } from './persistence/IdempotencyRecord.js';
@@ -176,8 +177,13 @@ export class IdempotencyHandler<Func extends AnyFunction> {
 
         return await this.getFunctionResult();
       } catch (error) {
+        if (!(error instanceof Error))
+          throw new IdempotencyUnknownError(
+            'An unknown error occurred while processing the request.',
+            { cause: error }
+          );
         if (
-          error instanceof IdempotencyInconsistentStateError &&
+          error.name === 'IdempotencyInconsistentStateError' &&
           retryNo < MAX_RETRIES
         ) {
           // Retry
@@ -241,7 +247,12 @@ export class IdempotencyHandler<Func extends AnyFunction> {
         break;
       } catch (error) {
         if (
-          error instanceof IdempotencyInconsistentStateError &&
+          /**
+           * It's safe to cast the error here because this catch block is only
+           * reached when an error is thrown in code paths that we control,
+           * and we only throw instances of `Error`.
+           */
+          (error as Error).name === 'IdempotencyInconsistentStateError' &&
           retryNo < MAX_RETRIES
         ) {
           // Retry
@@ -313,10 +324,10 @@ export class IdempotencyHandler<Func extends AnyFunction> {
       await this.#persistenceStore.deleteRecord(
         this.#functionPayloadToBeHashed
       );
-    } catch (e) {
+    } catch (error) {
       throw new IdempotencyPersistenceLayerError(
         'Failed to delete record from idempotency store',
-        e as Error
+        { cause: error }
       );
     }
   };
@@ -345,9 +356,15 @@ export class IdempotencyHandler<Func extends AnyFunction> {
       );
 
       return returnValue;
-    } catch (e) {
-      if (e instanceof IdempotencyItemAlreadyExistsError) {
-        let idempotencyRecord = e.existingRecord;
+    } catch (error) {
+      if (!(error instanceof Error))
+        throw new IdempotencyUnknownError(
+          'An unknown error occurred while processing the request.',
+          { cause: error }
+        );
+      if (error.name === 'IdempotencyItemAlreadyExistsError') {
+        let idempotencyRecord = (error as IdempotencyItemAlreadyExistsError)
+          .existingRecord;
         if (idempotencyRecord !== undefined) {
           // If the error includes the existing record, we can use it to validate
           // the record being processed and cache it in memory.
@@ -374,7 +391,7 @@ export class IdempotencyHandler<Func extends AnyFunction> {
       } else {
         throw new IdempotencyPersistenceLayerError(
           'Failed to save in progress record to idempotency store',
-          e as Error
+          { cause: error }
         );
       }
     }
@@ -393,10 +410,10 @@ export class IdempotencyHandler<Func extends AnyFunction> {
         this.#functionPayloadToBeHashed,
         result
       );
-    } catch (e) {
+    } catch (error) {
       throw new IdempotencyPersistenceLayerError(
         'Failed to update success record to idempotency store',
-        e as Error
+        { cause: error }
       );
     }
   };
diff --git a/packages/idempotency/src/errors.ts b/packages/idempotency/src/errors.ts
@@ -1,68 +1,122 @@
 import type { IdempotencyRecord } from './persistence/IdempotencyRecord.js';
 
+/**
+ * Base error for idempotency errors.
+ *
+ * Generally this error should not be thrown directly unless you are throwing a generic and unknown error.
+ */
+class IdempotencyUnknownError extends Error {
+  public constructor(message?: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyUnknownError';
+  }
+}
+
 /**
  * Item attempting to be inserted into persistence store already exists and is not expired
  */
-class IdempotencyItemAlreadyExistsError extends Error {
+class IdempotencyItemAlreadyExistsError extends IdempotencyUnknownError {
   public existingRecord?: IdempotencyRecord;
 
-  public constructor(message?: string, existingRecord?: IdempotencyRecord) {
-    super(message);
+  public constructor(
+    message?: string,
+    existingRecord?: IdempotencyRecord,
+    options?: ErrorOptions
+  ) {
+    super(message, options);
+    this.name = 'IdempotencyItemAlreadyExistsError';
     this.existingRecord = existingRecord;
   }
 }
 
 /**
  * Item does not exist in persistence store
  */
-class IdempotencyItemNotFoundError extends Error {}
+class IdempotencyItemNotFoundError extends IdempotencyUnknownError {
+  public constructor(message?: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyItemNotFoundError';
+  }
+}
 
 /**
  * Execution with idempotency key is already in progress
  */
-class IdempotencyAlreadyInProgressError extends Error {}
+class IdempotencyAlreadyInProgressError extends IdempotencyUnknownError {
+  public existingRecord?: IdempotencyRecord;
+
+  public constructor(
+    message?: string,
+    existingRecord?: IdempotencyRecord,
+    options?: ErrorOptions
+  ) {
+    super(message, options);
+    this.name = 'IdempotencyAlreadyInProgressError';
+    this.existingRecord = existingRecord;
+  }
+}
 
 /**
  * An invalid status was provided
  */
-class IdempotencyInvalidStatusError extends Error {}
+class IdempotencyInvalidStatusError extends IdempotencyUnknownError {
+  public constructor(message?: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyInvalidStatusError';
+  }
+}
 
 /**
  * Payload does not match stored idempotency record
  */
-class IdempotencyValidationError extends Error {
+class IdempotencyValidationError extends IdempotencyUnknownError {
   public existingRecord?: IdempotencyRecord;
 
-  public constructor(message?: string, existingRecord?: IdempotencyRecord) {
-    super(message);
+  public constructor(
+    message?: string,
+    existingRecord?: IdempotencyRecord,
+    options?: ErrorOptions
+  ) {
+    super(message, options);
+    this.name = 'IdempotencyValidationError';
     this.existingRecord = existingRecord;
   }
 }
 
 /**
  * State is inconsistent across multiple requests to persistence store
  */
-class IdempotencyInconsistentStateError extends Error {}
+class IdempotencyInconsistentStateError extends IdempotencyUnknownError {
+  public constructor(message?: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyInconsistentStateError';
+  }
+}
 
 /**
  * Unrecoverable error from the data store
  */
-class IdempotencyPersistenceLayerError extends Error {
+class IdempotencyPersistenceLayerError extends IdempotencyUnknownError {
   public readonly cause: Error | undefined;
 
-  public constructor(message: string, cause: Error) {
-    const errorMessage = `${message}. This error was caused by: ${cause.message}.`;
-    super(errorMessage);
-    this.cause = cause;
+  public constructor(message: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyPersistenceLayerError';
   }
 }
 
 /**
  * Payload does not contain an idempotent key
  */
-class IdempotencyKeyError extends Error {}
+class IdempotencyKeyError extends IdempotencyUnknownError {
+  public constructor(message?: string, options?: ErrorOptions) {
+    super(message, options);
+    this.name = 'IdempotencyKeyError';
+  }
+}
 
 export {
+  IdempotencyUnknownError,
   IdempotencyItemAlreadyExistsError,
   IdempotencyItemNotFoundError,
   IdempotencyAlreadyInProgressError,
diff --git a/packages/idempotency/src/index.ts b/packages/idempotency/src/index.ts
@@ -7,6 +7,7 @@ export {
   IdempotencyInconsistentStateError,
   IdempotencyPersistenceLayerError,
   IdempotencyKeyError,
+  IdempotencyUnknownError,
 } from './errors.js';
 export { IdempotencyConfig } from './IdempotencyConfig.js';
 export { makeIdempotent } from './makeIdempotent.js';
diff --git a/packages/idempotency/tests/unit/makeHandlerIdempotent.test.ts b/packages/idempotency/tests/unit/makeHandlerIdempotent.test.ts
@@ -12,6 +12,7 @@ import {
   IdempotencyPersistenceLayerError,
   IdempotencyConfig,
   IdempotencyRecordStatus,
+  IdempotencyUnknownError,
 } from '../../src/index.js';
 import middy from '@middy/core';
 import { MAX_RETRIES } from '../../src/constants.js';
@@ -246,6 +247,39 @@ describe('Middleware: makeHandlerIdempotent', () => {
     );
     expect(getRecordSpy).toHaveBeenCalledTimes(MAX_RETRIES + 1);
   });
+  it('throws immediately if an object other than an error was thrown', async () => {
+    // Prepare
+    const handler = middy(
+      async (_event: unknown, _context: Context): Promise<boolean> => {
+        return true;
+      }
+    ).use(makeHandlerIdempotent(mockIdempotencyOptions));
+    jest
+      .spyOn(mockIdempotencyOptions.persistenceStore, 'saveInProgress')
+      .mockImplementationOnce(() => {
+        // eslint-disable-next-line no-throw-literal
+        throw 'Something went wrong';
+      });
+    const stubRecordInconsistent = new IdempotencyRecord({
+      idempotencyKey: 'idempotencyKey',
+      expiryTimestamp: Date.now() + 10000,
+      inProgressExpiryTimestamp: 0,
+      responseData: { response: false },
+      payloadHash: 'payloadHash',
+      status: IdempotencyRecordStatus.EXPIRED,
+    });
+    const getRecordSpy = jest
+      .spyOn(mockIdempotencyOptions.persistenceStore, 'getRecord')
+      .mockResolvedValue(stubRecordInconsistent);
+
+    // Act & Assess
+    await expect(handler(event, context)).rejects.toThrow(
+      new IdempotencyUnknownError(
+        'An unknown error occurred while processing the request.'
+      )
+    );
+    expect(getRecordSpy).toHaveBeenCalledTimes(0);
+  });
   it('does not do anything if idempotency is disabled', async () => {
     // Prepare
     process.env.POWERTOOLS_IDEMPOTENCY_DISABLED = 'true';
diff --git a/packages/idempotency/tests/unit/makeIdempotent.test.ts b/packages/idempotency/tests/unit/makeIdempotent.test.ts
@@ -11,6 +11,7 @@ import {
   IdempotencyPersistenceLayerError,
   IdempotencyConfig,
   IdempotencyRecordStatus,
+  IdempotencyUnknownError,
 } from '../../src/index.js';
 import context from '@aws-lambda-powertools/testing-utils/context';
 import { MAX_RETRIES } from '../../src/constants.js';
@@ -265,13 +266,38 @@ describe('Function: makeIdempotent', () => {
       .mockResolvedValue(stubRecordInconsistent);
 
     // Act & Assess
-    await expect(handler(event, context)).rejects.toThrowError(
+    await expect(handler(event, context)).rejects.toThrow(
       new IdempotencyInconsistentStateError(
         'Item has expired during processing and may not longer be valid.'
       )
     );
     expect(getRecordSpy).toHaveBeenCalledTimes(MAX_RETRIES + 1);
   });
+  it('throws immediately if an object other than an error was thrown', async () => {
+    // Prepare
+    const handler = makeIdempotent(
+      async (_event: unknown, _context: Context) => {
+        // eslint-disable-next-line no-throw-literal
+        throw 'Something went wrong';
+      },
+      {
+        ...mockIdempotencyOptions,
+        config: new IdempotencyConfig({}),
+      }
+    );
+    const saveSuccessSpy = jest.spyOn(
+      mockIdempotencyOptions.persistenceStore,
+      'saveSuccess'
+    );
+
+    // Act & Assess
+    await expect(handler(event, context)).rejects.toThrow(
+      new IdempotencyUnknownError(
+        'An unknown error occurred while processing the request.'
+      )
+    );
+    expect(saveSuccessSpy).toHaveBeenCalledTimes(0);
+  });
   it('does not do anything if idempotency is disabled', async () => {
     // Prepare
     process.env.POWERTOOLS_IDEMPOTENCY_DISABLED = 'true';
