From 4af1a89120487286cbf9e0f5c4b346709a6c6157 Mon Sep 17 00:00:00 2001 From: Alexander Melnyk Date: Sun, 23 Apr 2023 14:10:40 +0200 Subject: [PATCH 1/4] add retry and tests --- .../idempotency/src/IdempotencyHandler.ts | 71 ++++++--- .../idempotency/src/idempotentDecorator.ts | 9 +- .../tests/unit/IdempotencyHandler.test.ts | 136 ++++++++++++++++++ .../tests/unit/makeFunctionIdempotent.test.ts | 42 +++--- 4 files changed, 214 insertions(+), 44 deletions(-) create mode 100644 packages/idempotency/tests/unit/IdempotencyHandler.test.ts diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index bc34c048d7..b8e46ce0f5 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -1,41 +1,76 @@ +import type { AnyFunctionWithRecord, IdempotencyOptions } from './types'; import { IdempotencyRecordStatus } from './types'; -import type { - AnyFunctionWithRecord, - IdempotencyOptions, -} from './types'; import { + IdempotencyAlreadyInProgressError, IdempotencyInconsistentStateError, IdempotencyItemAlreadyExistsError, - IdempotencyAlreadyInProgressError, - IdempotencyPersistenceLayerError + IdempotencyPersistenceLayerError, } from './Exceptions'; -import { IdempotencyRecord } from './persistence/IdempotencyRecord'; +import { IdempotencyRecord } from './persistence'; export class IdempotencyHandler { public constructor( private functionToMakeIdempotent: AnyFunctionWithRecord, - private functionPayloadToBeHashed: Record, + private functionPayloadToBeHashed: Record, private idempotencyOptions: IdempotencyOptions, - private fullFunctionPayload: Record - ) {} + private fullFunctionPayload: Record, + ) { + } - public determineResultFromIdempotencyRecord(idempotencyRecord: IdempotencyRecord): Promise | U { + public determineResultFromIdempotencyRecord( + idempotencyRecord: IdempotencyRecord + ): Promise | U { if (idempotencyRecord.getStatus() === IdempotencyRecordStatus.EXPIRED) { - throw new IdempotencyInconsistentStateError('Item has expired during processing and may not longer be valid.'); - } else if (idempotencyRecord.getStatus() === IdempotencyRecordStatus.INPROGRESS){ - throw new IdempotencyAlreadyInProgressError(`There is already an execution in progress with idempotency key: ${idempotencyRecord.idempotencyKey}`); + throw new IdempotencyInconsistentStateError( + 'Item has expired during processing and may not longer be valid.' + ); + } else if ( + idempotencyRecord.getStatus() === IdempotencyRecordStatus.INPROGRESS + ) { + if ( + idempotencyRecord.inProgressExpiryTimestamp && + idempotencyRecord.inProgressExpiryTimestamp < + new Date().getUTCMilliseconds() + ) { + throw new IdempotencyInconsistentStateError( + 'Item is in progress but the in progress expiry timestamp has expired.' + ); + } else { + throw new IdempotencyAlreadyInProgressError( + `There is already an execution in progress with idempotency key: ${idempotencyRecord.idempotencyKey}` + ); + } } else { // Currently recalling the method as this fulfills FR1. FR3 will address using the previously stored value https://github.com/awslabs/aws-lambda-powertools-typescript/issues/447 - return this.functionToMakeIdempotent(this.fullFunctionPayload); + return this.functionToMakeIdempotent(this.fullFunctionPayload); + } + } + + public async handle(): Promise { + const MAX_RETRIES = 2; + for (let i = 1; i <= MAX_RETRIES; i++) { + try { + return await this.processIdempotency(); + } catch (e) { + if (!(e instanceof IdempotencyAlreadyInProgressError) || i === MAX_RETRIES) { + throw e; + } + } } + throw new Error('This should never happen'); } public async processIdempotency(): Promise { try { - await this.idempotencyOptions.persistenceStore.saveInProgress(this.functionPayloadToBeHashed); + await this.idempotencyOptions.persistenceStore.saveInProgress( + this.functionPayloadToBeHashed, + ); } catch (e) { if (e instanceof IdempotencyItemAlreadyExistsError) { - const idempotencyRecord: IdempotencyRecord = await this.idempotencyOptions.persistenceStore.getRecord(this.functionPayloadToBeHashed); + const idempotencyRecord: IdempotencyRecord = + await this.idempotencyOptions.persistenceStore.getRecord( + this.functionPayloadToBeHashed + ); return this.determineResultFromIdempotencyRecord(idempotencyRecord); } else { @@ -45,4 +80,4 @@ export class IdempotencyHandler { return this.functionToMakeIdempotent(this.fullFunctionPayload); } -} \ No newline at end of file +} diff --git a/packages/idempotency/src/idempotentDecorator.ts b/packages/idempotency/src/idempotentDecorator.ts index 4db95a86b0..7e35cfd0de 100644 --- a/packages/idempotency/src/idempotentDecorator.ts +++ b/packages/idempotency/src/idempotentDecorator.ts @@ -11,12 +11,11 @@ const idempotent = function (options: IdempotencyOptions) { descriptor.value = function(record: GenericTempRecord){ const idempotencyHandler = new IdempotencyHandler(childFunction, record[options.dataKeywordArgument], options, record); - return idempotencyHandler.processIdempotency(); + return idempotencyHandler.handle(); }; - + return descriptor; - }; + }; }; - + export { idempotent }; - \ No newline at end of file diff --git a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts new file mode 100644 index 0000000000..b4ac931a88 --- /dev/null +++ b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts @@ -0,0 +1,136 @@ +/** + * Test Idempotency Handler + * + * @group unit/idempotency/IdempotencyHandler + */ + +import { + IdempotencyAlreadyInProgressError, + IdempotencyItemAlreadyExistsError, + IdempotencyPersistenceLayerError +} from '../../src/Exceptions'; +import { IdempotencyOptions, IdempotencyRecordStatus } from '../../src/types'; +import { BasePersistenceLayer, IdempotencyRecord } from '../../src/persistence'; +import { IdempotencyHandler } from '../../src/IdempotencyHandler'; + +class PersistenceLayerTestClass extends BasePersistenceLayer { + protected _deleteRecord = jest.fn(); + protected _getRecord = jest.fn(); + protected _putRecord = jest.fn(); + protected _updateRecord = jest.fn(); +} + +const mockFunctionToMakeIdempotent = jest.fn(); +const mockFunctionPayloadToBeHashed = {}; +const mockIdempotencyOptions: IdempotencyOptions = { + persistenceStore: new PersistenceLayerTestClass(), + dataKeywordArgument: 'testingKey' +}; +const mockFullFunctionPayload = {}; + +const idempotentHandler = new IdempotencyHandler( + mockFunctionToMakeIdempotent, + mockFunctionPayloadToBeHashed, + mockIdempotencyOptions, + mockFullFunctionPayload, +); + +describe('Class IdempotencyHandler', () => { + beforeEach(() => jest.resetAllMocks()); + + describe('Method: determineResultFromIdempotencyRecord', () => { + test('when record is in progress and within expiry window, it rejects with IdempotencyAlreadyInProgressError', async () => { + + const stubRecord = new IdempotencyRecord({ + idempotencyKey: 'idempotencyKey', + expiryTimestamp: Date.now() + 10000, // should be in the future + inProgressExpiryTimestamp: 0, // less than current time in milliseconds + responseData: { responseData: 'responseData' }, + payloadHash: 'payloadHash', + status: IdempotencyRecordStatus.INPROGRESS + }); + + expect(stubRecord.isExpired()).toBe(false); + expect(stubRecord.getStatus()).toBe(IdempotencyRecordStatus.INPROGRESS); + + try { + await idempotentHandler.determineResultFromIdempotencyRecord(stubRecord); + } catch (e) { + expect(e).toBeInstanceOf(IdempotencyAlreadyInProgressError); + } + }); + }); + + describe('Method: handle', () => { + + afterAll(() => jest.restoreAllMocks()); // restore processIdempotency for other tests + + test('when IdempotencyAlreadyInProgressError is thrown, it retries two times', async () => { + const mockProcessIdempotency = jest.spyOn(IdempotencyHandler.prototype, 'processIdempotency').mockRejectedValue(new IdempotencyAlreadyInProgressError('There is already an execution in progress')); + await expect( + idempotentHandler.handle() + ).rejects.toThrow(IdempotencyAlreadyInProgressError); + expect(mockProcessIdempotency).toHaveBeenCalledTimes(2); + }); + + test('when non IdempotencyAlreadyInProgressError is thrown, it rejects', async () => { + + const mockProcessIdempotency = jest.spyOn(IdempotencyHandler.prototype, 'processIdempotency').mockRejectedValue(new Error('Some other error')); + + await expect( + idempotentHandler.handle() + ).rejects.toThrow(Error); + expect(mockProcessIdempotency).toHaveBeenCalledTimes(1); + }); + + }); + + describe('Method: processIdempotency', () => { + + test('when persistenceStore saves successfuly, it resolves', async () => { + const mockSaveInProgress = jest.spyOn(mockIdempotencyOptions.persistenceStore, 'saveInProgress').mockResolvedValue(); + + mockFunctionToMakeIdempotent.mockImplementation(() => Promise.resolve('result')); + + await expect( + idempotentHandler.processIdempotency() + ).resolves.toBe('result'); + expect(mockSaveInProgress).toHaveBeenCalledTimes(1); + }); + + test('when persistences store throws any error, it wraps the error to IdempotencyPersistencesLayerError', async () => { + const mockSaveInProgress = jest.spyOn(mockIdempotencyOptions.persistenceStore, 'saveInProgress').mockRejectedValue(new Error('Some error')); + const mockDetermineResultFromIdempotencyRecord = jest.spyOn(IdempotencyHandler.prototype, 'determineResultFromIdempotencyRecord').mockResolvedValue('result'); + + await expect( + idempotentHandler.processIdempotency() + ).rejects.toThrow(IdempotencyPersistenceLayerError); + expect(mockSaveInProgress).toHaveBeenCalledTimes(1); + expect(mockDetermineResultFromIdempotencyRecord).toHaveBeenCalledTimes(0); + }); + + test('when idempotency item already exists, it returns the existing record', async () => { + const mockSaveInProgress = jest.spyOn(mockIdempotencyOptions.persistenceStore, 'saveInProgress').mockRejectedValue(new IdempotencyItemAlreadyExistsError('There is already an execution in progress')); + + const stubRecord = new IdempotencyRecord({ + idempotencyKey: 'idempotencyKey', + expiryTimestamp: 0, + inProgressExpiryTimestamp: 0, + responseData: { responseData: 'responseData' }, + payloadHash: 'payloadHash', + status: IdempotencyRecordStatus.INPROGRESS + }); + const mockGetRecord = jest.spyOn(mockIdempotencyOptions.persistenceStore, 'getRecord').mockImplementation(() => Promise.resolve(stubRecord)); + const mockDetermineResultFromIdempotencyRecord = jest.spyOn(IdempotencyHandler.prototype, 'determineResultFromIdempotencyRecord').mockResolvedValue('result'); + + await expect( + idempotentHandler.processIdempotency() + ).resolves.toBe('result'); + expect(mockSaveInProgress).toHaveBeenCalledTimes(1); + expect(mockGetRecord).toHaveBeenCalledTimes(1); + expect(mockDetermineResultFromIdempotencyRecord).toHaveBeenCalledTimes(1); + }); + }); + +}); + diff --git a/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts b/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts index cb531bae23..c8d90b251b 100644 --- a/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts +++ b/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts @@ -4,17 +4,14 @@ * @group unit/idempotency/makeFunctionIdempotent */ import { IdempotencyOptions } from '../../src/types/IdempotencyOptions'; -import { IdempotencyRecord, BasePersistenceLayer } from '../../src/persistence'; +import { BasePersistenceLayer, IdempotencyRecord } from '../../src/persistence'; import { makeFunctionIdempotent } from '../../src/makeFunctionIdempotent'; +import type { AnyIdempotentFunction, IdempotencyRecordOptions } from '../../src/types'; import { IdempotencyRecordStatus } from '../../src/types'; -import type { - AnyIdempotentFunction, - IdempotencyRecordOptions -} from '../../src/types'; import { - IdempotencyItemAlreadyExistsError, IdempotencyAlreadyInProgressError, IdempotencyInconsistentStateError, + IdempotencyItemAlreadyExistsError, IdempotencyPersistenceLayerError } from '../../src/Exceptions'; @@ -29,8 +26,11 @@ class PersistenceLayerTestClass extends BasePersistenceLayer { } describe('Given a function to wrap', (functionToWrap = jest.fn()) => { - beforeEach(()=> jest.clearAllMocks()); - describe('Given options for idempotency', (options: IdempotencyOptions = { persistenceStore: new PersistenceLayerTestClass(), dataKeywordArgument: 'testingKey' }) => { + beforeEach(() => jest.clearAllMocks()); + describe('Given options for idempotency', (options: IdempotencyOptions = { + persistenceStore: new PersistenceLayerTestClass(), + dataKeywordArgument: 'testingKey' + }) => { const keyValueToBeSaved = 'thisWillBeSaved'; const inputRecord = { testingKey: keyValueToBeSaved, otherKey: 'thisWillNot' }; describe('When wrapping a function with no previous executions', () => { @@ -66,11 +66,11 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { resultingError = e as Error; } }); - + test('Then it will attempt to save the record to INPROGRESS', () => { expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); }); - + test('Then it will get the previous execution record', () => { expect(mockGetRecord).toBeCalledWith(keyValueToBeSaved); }); @@ -79,7 +79,7 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { expect(functionToWrap).not.toBeCalled(); }); - test('Then an IdempotencyAlreadyInProgressError is thrown', ()=> { + test('Then an IdempotencyAlreadyInProgressError is thrown', () => { expect(resultingError).toBeInstanceOf(IdempotencyAlreadyInProgressError); }); }); @@ -101,11 +101,11 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { resultingError = e as Error; } }); - + test('Then it will attempt to save the record to INPROGRESS', () => { expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); }); - + test('Then it will get the previous execution record', () => { expect(mockGetRecord).toBeCalledWith(keyValueToBeSaved); }); @@ -113,8 +113,8 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { test('Then the function that was wrapped is not called again', () => { expect(functionToWrap).not.toBeCalled(); }); - - test('Then an IdempotencyInconsistentStateError is thrown', ()=> { + + test('Then an IdempotencyInconsistentStateError is thrown', () => { expect(resultingError).toBeInstanceOf(IdempotencyInconsistentStateError); }); }); @@ -127,15 +127,15 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { idempotencyKey: 'key', status: IdempotencyRecordStatus.COMPLETED }; - mockGetRecord.mockResolvedValue(new IdempotencyRecord(idempotencyOptions)); + mockGetRecord.mockResolvedValue(new IdempotencyRecord(idempotencyOptions)); resultingFunction = makeFunctionIdempotent(functionToWrap, options); await resultingFunction(inputRecord); }); - + test('Then it will attempt to save the record to INPROGRESS', () => { expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); }); - + test('Then it will get the previous execution record', () => { expect(mockGetRecord).toBeCalledWith(keyValueToBeSaved); }); @@ -158,12 +158,12 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { resultingError = e as Error; } }); - + test('Then it will attempt to save the record to INPROGRESS', () => { expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); }); - - test('Then an IdempotencyPersistenceLayerError is thrown', ()=> { + + test('Then an IdempotencyPersistenceLayerError is thrown', () => { expect(resultingError).toBeInstanceOf(IdempotencyPersistenceLayerError); }); }); From d5a0d7bfab8e0c902f2ef592bdf31e35bce2a91b Mon Sep 17 00:00:00 2001 From: Alexander Melnyk Date: Sun, 23 Apr 2023 14:11:21 +0200 Subject: [PATCH 2/4] add retry and tests --- .../idempotency/src/IdempotencyHandler.ts | 10 +++- .../tests/unit/IdempotencyHandler.test.ts | 46 ++++++++++++++++++- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index b8e46ce0f5..f76ad475c5 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -46,7 +46,16 @@ export class IdempotencyHandler { } } + /** + * Main entry point for the handler + * IdempotencyInconsistentStateError can happen under rare but expected cases + * when persistent state changes in the small time between put & get requests. + * In most cases we can retry successfully on this exception. + */ + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore public async handle(): Promise { + const MAX_RETRIES = 2; for (let i = 1; i <= MAX_RETRIES; i++) { try { @@ -57,7 +66,6 @@ export class IdempotencyHandler { } } } - throw new Error('This should never happen'); } public async processIdempotency(): Promise { diff --git a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts index b4ac931a88..aeff922501 100644 --- a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts +++ b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts @@ -5,7 +5,7 @@ */ import { - IdempotencyAlreadyInProgressError, + IdempotencyAlreadyInProgressError, IdempotencyInconsistentStateError, IdempotencyItemAlreadyExistsError, IdempotencyPersistenceLayerError } from '../../src/Exceptions'; @@ -43,7 +43,7 @@ describe('Class IdempotencyHandler', () => { const stubRecord = new IdempotencyRecord({ idempotencyKey: 'idempotencyKey', - expiryTimestamp: Date.now() + 10000, // should be in the future + expiryTimestamp: Date.now() + 1000, // should be in the future inProgressExpiryTimestamp: 0, // less than current time in milliseconds responseData: { responseData: 'responseData' }, payloadHash: 'payloadHash', @@ -59,6 +59,48 @@ describe('Class IdempotencyHandler', () => { expect(e).toBeInstanceOf(IdempotencyAlreadyInProgressError); } }); + + test('when record is in progress and outside expiry window, it rejects with IdempotencyInconsistentStateError', async () => { + + const stubRecord = new IdempotencyRecord({ + idempotencyKey: 'idempotencyKey', + expiryTimestamp: Date.now() + 1000, // should be in the future + inProgressExpiryTimestamp: new Date().getUTCMilliseconds() - 1000, // should be in the past + responseData: { responseData: 'responseData' }, + payloadHash: 'payloadHash', + status: IdempotencyRecordStatus.INPROGRESS + }); + + expect(stubRecord.isExpired()).toBe(false); + expect(stubRecord.getStatus()).toBe(IdempotencyRecordStatus.INPROGRESS); + + try { + await idempotentHandler.determineResultFromIdempotencyRecord(stubRecord); + } catch (e) { + expect(e).toBeInstanceOf(IdempotencyInconsistentStateError); + } + }); + + test('when record is expired, it rejects with IdempotencyInconsistentStateError', async () => { + + const stubRecord = new IdempotencyRecord({ + idempotencyKey: 'idempotencyKey', + expiryTimestamp: new Date().getUTCMilliseconds() - 1000, // should be in the past + inProgressExpiryTimestamp: 0, // less than current time in milliseconds + responseData: { responseData: 'responseData' }, + payloadHash: 'payloadHash', + status: IdempotencyRecordStatus.EXPIRED + }); + + expect(stubRecord.isExpired()).toBe(true); + expect(stubRecord.getStatus()).toBe(IdempotencyRecordStatus.EXPIRED); + + try { + await idempotentHandler.determineResultFromIdempotencyRecord(stubRecord); + } catch (e) { + expect(e).toBeInstanceOf(IdempotencyInconsistentStateError); + } + }); }); describe('Method: handle', () => { From 4a69c16f7925efa8f2205f0ea0627b088bc49756 Mon Sep 17 00:00:00 2001 From: Alexander Melnyk Date: Mon, 24 Apr 2023 09:41:08 +0200 Subject: [PATCH 3/4] reroute to the new entry point --- packages/idempotency/src/makeFunctionIdempotent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/idempotency/src/makeFunctionIdempotent.ts b/packages/idempotency/src/makeFunctionIdempotent.ts index 75c44b47c3..77e0a9e994 100644 --- a/packages/idempotency/src/makeFunctionIdempotent.ts +++ b/packages/idempotency/src/makeFunctionIdempotent.ts @@ -13,7 +13,7 @@ const makeFunctionIdempotent = function ( const wrappedFn: AnyIdempotentFunction = function (record: GenericTempRecord): Promise { const idempotencyHandler: IdempotencyHandler = new IdempotencyHandler(fn, record[options.dataKeywordArgument], options, record); - return idempotencyHandler.processIdempotency(); + return idempotencyHandler.handle(); }; return wrappedFn; From 34fe762cf1c193b3fe0753aa2a3504a4496420ab Mon Sep 17 00:00:00 2001 From: Alexander Melnyk Date: Tue, 25 Apr 2023 18:26:28 +0200 Subject: [PATCH 4/4] add throw outside of for loop with covarage ignore statement. --- packages/idempotency/src/IdempotencyHandler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index f76ad475c5..651301bbba 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -52,8 +52,6 @@ export class IdempotencyHandler { * when persistent state changes in the small time between put & get requests. * In most cases we can retry successfully on this exception. */ - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore public async handle(): Promise { const MAX_RETRIES = 2; @@ -66,6 +64,8 @@ export class IdempotencyHandler { } } } + /* istanbul ignore next */ + throw new Error('This should never happen'); } public async processIdempotency(): Promise {