From d47c3ec64d926d49f3799f361d54a11627d16cc1 Mon Sep 17 00:00:00 2001 From: Alexander Schueren Date: Thu, 22 Jun 2023 14:38:23 +0200 Subject: [PATCH] fix(idempotency): pass lambda context remaining time to save inprogress (#1540) * fix in_progress_expiration timestamp in idempotency handler * add lambda context mock to tests * adjust handler signature with context * revert tests to create config, if not passed, undefined temporary * decorator can now register context too --- .../idempotency/src/IdempotencyHandler.ts | 3 +- .../idempotency/src/idempotentDecorator.ts | 19 ++++++- ...akeFunctionIdempotent.test.FunctionCode.ts | 7 +++ .../tests/e2e/makeFunctionIdempotent.test.ts | 9 +++ .../tests/unit/IdempotencyHandler.test.ts | 34 +++++++++++ .../tests/unit/idempotentDecorator.test.ts | 56 ++++++++++++++----- .../tests/unit/makeFunctionIdempotent.test.ts | 34 +++++++++-- 7 files changed, 141 insertions(+), 21 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index 7fece6ba25..07b0786732 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -141,7 +141,8 @@ export class IdempotencyHandler { try { await this.persistenceStore.saveInProgress( - this.functionPayloadToBeHashed + this.functionPayloadToBeHashed, + this.idempotencyConfig.lambdaContext?.getRemainingTimeInMillis() ); } catch (e) { if (e instanceof IdempotencyItemAlreadyExistsError) { diff --git a/packages/idempotency/src/idempotentDecorator.ts b/packages/idempotency/src/idempotentDecorator.ts index 020c3e2cd9..9b97f89bbb 100644 --- a/packages/idempotency/src/idempotentDecorator.ts +++ b/packages/idempotency/src/idempotentDecorator.ts @@ -5,6 +5,16 @@ import { } from './types'; import { IdempotencyHandler } from './IdempotencyHandler'; import { IdempotencyConfig } from './IdempotencyConfig'; +import { Context } from 'aws-lambda'; + +const isContext = (arg: unknown): arg is Context => { + return ( + arg !== undefined && + arg !== null && + typeof arg === 'object' && + 'getRemainingTimeInMillis' in arg + ); +}; /** * use this function to narrow the type of options between IdempotencyHandlerOptions and IdempotencyFunctionOptions @@ -28,13 +38,20 @@ const idempotent = function ( descriptor: PropertyDescriptor ) { const childFunction = descriptor.value; - descriptor.value = function (record: GenericTempRecord) { + descriptor.value = function ( + record: GenericTempRecord, + ...args: unknown[] + ) { const functionPayloadtoBeHashed = isFunctionOption(options) ? record[(options as IdempotencyFunctionOptions).dataKeywordArgument] : record; const idempotencyConfig = options.config ? options.config : new IdempotencyConfig({}); + const context = args[0]; + if (isContext(context)) { + idempotencyConfig.registerLambdaContext(context); + } const idempotencyHandler = new IdempotencyHandler({ functionToMakeIdempotent: childFunction, functionPayloadToBeHashed: functionPayloadtoBeHashed, diff --git a/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.FunctionCode.ts b/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.FunctionCode.ts index af2d3062c6..8e947f7950 100644 --- a/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.FunctionCode.ts +++ b/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.FunctionCode.ts @@ -2,6 +2,7 @@ import type { Context } from 'aws-lambda'; import { DynamoDBPersistenceLayer } from '../../src/persistence/DynamoDBPersistenceLayer'; import { makeFunctionIdempotent } from '../../src'; import { Logger } from '@aws-lambda-powertools/logger'; +import { IdempotencyConfig } from '../../src'; const IDEMPOTENCY_TABLE_NAME = process.env.IDEMPOTENCY_TABLE_NAME || 'table_name'; @@ -32,15 +33,19 @@ const processRecord = (record: Record): string => { return 'Processing done: ' + record['foo']; }; +const idempotencyConfig = new IdempotencyConfig({}); + const processIdempotently = makeFunctionIdempotent(processRecord, { persistenceStore: dynamoDBPersistenceLayer, dataKeywordArgument: 'foo', + config: idempotencyConfig, }); export const handler = async ( _event: EventRecords, _context: Context ): Promise => { + idempotencyConfig.registerLambdaContext(_context); for (const record of _event.records) { const result = await processIdempotently(record); logger.info(result.toString()); @@ -52,12 +57,14 @@ export const handler = async ( const processIdempotentlyCustomized = makeFunctionIdempotent(processRecord, { persistenceStore: ddbPersistenceLayerCustomized, dataKeywordArgument: 'foo', + config: idempotencyConfig, }); export const handlerCustomized = async ( _event: EventRecords, _context: Context ): Promise => { + idempotencyConfig.registerLambdaContext(_context); for (const record of _event.records) { const result = await processIdempotentlyCustomized(record); logger.info(result.toString()); diff --git a/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.ts b/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.ts index 8ee55021d9..cd4344b024 100644 --- a/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.ts +++ b/packages/idempotency/tests/e2e/makeFunctionIdempotent.test.ts @@ -109,6 +109,7 @@ describe('Idempotency e2e test function wrapper, default settings', () => { { id: 3, foo: 'bar' }, ], }; + const invokeStart = Date.now(); await invokeFunction( functionNameDefault, 2, @@ -136,6 +137,10 @@ describe('Idempotency e2e test function wrapper, default settings', () => { }) ); expect(resultFirst?.Item?.data).toEqual('Processing done: bar'); + expect(resultFirst?.Item?.expiration).toBeGreaterThan(Date.now() / 1000); + expect(resultFirst?.Item?.in_progress_expiration).toBeGreaterThan( + invokeStart + ); expect(resultFirst?.Item?.status).toEqual('COMPLETED'); const resultSecond = await ddb.send( @@ -145,6 +150,10 @@ describe('Idempotency e2e test function wrapper, default settings', () => { }) ); expect(resultSecond?.Item?.data).toEqual('Processing done: baz'); + expect(resultSecond?.Item?.expiration).toBeGreaterThan(Date.now() / 1000); + expect(resultSecond?.Item?.in_progress_expiration).toBeGreaterThan( + invokeStart + ); expect(resultSecond?.Item?.status).toEqual('COMPLETED'); }, TEST_CASE_TIMEOUT diff --git a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts index be0dada29e..5ff0558189 100644 --- a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts +++ b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts @@ -14,6 +14,7 @@ import { BasePersistenceLayer, IdempotencyRecord } from '../../src/persistence'; import { IdempotencyHandler } from '../../src/IdempotencyHandler'; import { IdempotencyConfig } from '../../src/'; import { MAX_RETRIES } from '../../src/constants'; +import { Context } from 'aws-lambda'; class PersistenceLayerTestClass extends BasePersistenceLayer { protected _deleteRecord = jest.fn(); @@ -247,6 +248,39 @@ describe('Class IdempotencyHandler', () => { expect(mockGetRecord).toHaveBeenCalledTimes(0); expect(mockSaveSuccessfulResult).toHaveBeenCalledTimes(0); }); + + test('when lambdaContext is registered, we pass it to saveInProgress', async () => { + const mockSaveInProgress = jest.spyOn( + mockIdempotencyOptions.persistenceStore, + 'saveInProgress' + ); + + const mockLambaContext: Context = { + getRemainingTimeInMillis(): number { + return 1000; // we expect this number to be passed to saveInProgress + }, + } as Context; + const idempotencyHandlerWithContext = new IdempotencyHandler({ + functionToMakeIdempotent: mockFunctionToMakeIdempotent, + functionPayloadToBeHashed: mockFunctionPayloadToBeHashed, + persistenceStore: mockIdempotencyOptions.persistenceStore, + fullFunctionPayload: mockFullFunctionPayload, + idempotencyConfig: new IdempotencyConfig({ + lambdaContext: mockLambaContext, + }), + }); + + mockFunctionToMakeIdempotent.mockImplementation(() => + Promise.resolve('result') + ); + + await expect(idempotencyHandlerWithContext.processIdempotency()).resolves; + + expect(mockSaveInProgress).toBeCalledWith( + mockFunctionPayloadToBeHashed, + mockLambaContext.getRemainingTimeInMillis() + ); + }); }); describe('Method: getFunctionResult', () => { diff --git a/packages/idempotency/tests/unit/idempotentDecorator.test.ts b/packages/idempotency/tests/unit/idempotentDecorator.test.ts index 2e15f8c623..9a1b2dc643 100644 --- a/packages/idempotency/tests/unit/idempotentDecorator.test.ts +++ b/packages/idempotency/tests/unit/idempotentDecorator.test.ts @@ -15,6 +15,8 @@ import { IdempotencyPersistenceLayerError, } from '../../src/Exceptions'; import { IdempotencyConfig } from '../../src'; +import { Context } from 'aws-lambda'; +import { helloworldContext } from '@aws-lambda-powertools/commons/lib/samples/resources/contexts'; const mockSaveInProgress = jest .spyOn(BasePersistenceLayer.prototype, 'saveInProgress') @@ -26,6 +28,10 @@ const mockGetRecord = jest .spyOn(BasePersistenceLayer.prototype, 'getRecord') .mockImplementation(); +const dummyContext = helloworldContext; + +const mockConfig: IdempotencyConfig = new IdempotencyConfig({}); + class PersistenceLayerTestClass extends BasePersistenceLayer { protected _deleteRecord = jest.fn(); protected _getRecord = jest.fn(); @@ -39,7 +45,8 @@ class TestinClassWithLambdaHandler { @idempotentLambdaHandler({ persistenceStore: new PersistenceLayerTestClass(), }) - public testing(record: Record): string { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + public testing(record: Record, context: Context): string { functionalityToDecorate(record); return 'Hi'; @@ -47,13 +54,16 @@ class TestinClassWithLambdaHandler { } class TestingClassWithFunctionDecorator { - public handler(record: Record): string { + public handler(record: Record, context: Context): string { + mockConfig.registerLambdaContext(context); + return this.proccessRecord(record); } @idempotentFunction({ persistenceStore: new PersistenceLayerTestClass(), dataKeywordArgument: 'testingKey', + config: mockConfig, }) public proccessRecord(record: Record): string { functionalityToDecorate(record); @@ -72,11 +82,14 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = describe('When wrapping a function with no previous executions', () => { beforeEach(async () => { - await classWithFunctionDecorator.handler(inputRecord); + await classWithFunctionDecorator.handler(inputRecord, dummyContext); }); test('Then it will save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then it will call the function that was decorated', () => { @@ -92,11 +105,14 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = }); describe('When wrapping a function with no previous executions', () => { beforeEach(async () => { - await classWithLambdaHandler.testing(inputRecord); + await classWithLambdaHandler.testing(inputRecord, dummyContext); }); test('Then it will save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(inputRecord); + expect(mockSaveInProgress).toBeCalledWith( + inputRecord, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then it will call the function that was decorated', () => { @@ -122,14 +138,17 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = new IdempotencyRecord(idempotencyOptions) ); try { - await classWithLambdaHandler.testing(inputRecord); + await classWithLambdaHandler.testing(inputRecord, dummyContext); } catch (e) { resultingError = e as Error; } }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(inputRecord); + expect(mockSaveInProgress).toBeCalledWith( + inputRecord, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -159,14 +178,17 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = new IdempotencyRecord(idempotencyOptions) ); try { - await classWithLambdaHandler.testing(inputRecord); + await classWithLambdaHandler.testing(inputRecord, dummyContext); } catch (e) { resultingError = e as Error; } }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(inputRecord); + expect(mockSaveInProgress).toBeCalledWith( + inputRecord, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -195,11 +217,14 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = mockGetRecord.mockResolvedValue( new IdempotencyRecord(idempotencyOptions) ); - await classWithLambdaHandler.testing(inputRecord); + await classWithLambdaHandler.testing(inputRecord, dummyContext); }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(inputRecord); + expect(mockSaveInProgress).toBeCalledWith( + inputRecord, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -215,7 +240,7 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = class TestinClassWithLambdaHandlerWithConfig { @idempotentLambdaHandler({ persistenceStore: new PersistenceLayerTestClass(), - config: new IdempotencyConfig({}), + config: new IdempotencyConfig({ lambdaContext: dummyContext }), }) public testing(record: Record): string { functionalityToDecorate(record); @@ -237,7 +262,10 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler = }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(inputRecord); + expect(mockSaveInProgress).toBeCalledWith( + inputRecord, + dummyContext.getRemainingTimeInMillis() + ); }); test('Then an IdempotencyPersistenceLayerError is thrown', () => { diff --git a/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts b/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts index 4310a648a1..e5fb2f6037 100644 --- a/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts +++ b/packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts @@ -17,6 +17,8 @@ import { IdempotencyItemAlreadyExistsError, IdempotencyPersistenceLayerError, } from '../../src/Exceptions'; +import { IdempotencyConfig } from '../../src'; +import { Context } from 'aws-lambda'; const mockSaveInProgress = jest .spyOn(BasePersistenceLayer.prototype, 'saveInProgress') @@ -25,6 +27,12 @@ const mockGetRecord = jest .spyOn(BasePersistenceLayer.prototype, 'getRecord') .mockImplementation(); +const mockLambaContext: Context = { + getRemainingTimeInMillis(): number { + return 1000; // we expect this number to be passed to saveInProgress + }, +} as Context; + class PersistenceLayerTestClass extends BasePersistenceLayer { protected _deleteRecord = jest.fn(); protected _getRecord = jest.fn(); @@ -37,6 +45,7 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { describe('Given options for idempotency', (options: IdempotencyFunctionOptions = { persistenceStore: new PersistenceLayerTestClass(), dataKeywordArgument: 'testingKey', + config: new IdempotencyConfig({ lambdaContext: mockLambaContext }), }) => { const keyValueToBeSaved = 'thisWillBeSaved'; const inputRecord = { @@ -51,7 +60,10 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { }); test('Then it will save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + mockLambaContext.getRemainingTimeInMillis() + ); }); test('Then it will call the function that was wrapped with the whole input record', () => { @@ -82,7 +94,10 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + mockLambaContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -123,7 +138,10 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + mockLambaContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -159,7 +177,10 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + mockLambaContext.getRemainingTimeInMillis() + ); }); test('Then it will get the previous execution record', () => { @@ -185,7 +206,10 @@ describe('Given a function to wrap', (functionToWrap = jest.fn()) => { }); test('Then it will attempt to save the record to INPROGRESS', () => { - expect(mockSaveInProgress).toBeCalledWith(keyValueToBeSaved); + expect(mockSaveInProgress).toBeCalledWith( + keyValueToBeSaved, + mockLambaContext.getRemainingTimeInMillis() + ); }); test('Then an IdempotencyPersistenceLayerError is thrown', () => {