From 55c387816baee98829441526a0de001044d67344 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Tue, 9 Jul 2024 09:14:21 +0200 Subject: [PATCH] fix(idempotency): check error identity via names (#2747) --- .../idempotency/src/IdempotencyHandler.ts | 37 +++++--- packages/idempotency/src/errors.ts | 86 +++++++++++++++---- packages/idempotency/src/index.ts | 1 + .../tests/unit/makeHandlerIdempotent.test.ts | 34 ++++++++ .../tests/unit/makeIdempotent.test.ts | 28 +++++- 5 files changed, 159 insertions(+), 27 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index c016ec814e..df92a1d7fc 100644 --- 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 { 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 { 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 { 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 { ); 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 { } 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 { 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 index 5f210adfcd..929b7b7090 100644 --- a/packages/idempotency/src/errors.ts +++ b/packages/idempotency/src/errors.ts @@ -1,13 +1,30 @@ 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; } } @@ -15,26 +32,53 @@ class IdempotencyItemAlreadyExistsError extends Error { /** * 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; } } @@ -42,27 +86,37 @@ class IdempotencyValidationError extends Error { /** * 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 index ced4539b1d..9d96f93163 100644 --- 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 index 32854e1bb4..a6696b9e38 100644 --- 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 => { + 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 index 40e5a3605b..c89291024e 100644 --- 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';