Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(idempotency): pass lambda context remaining time to save inprogress #1540

Merged
merged 5 commits into from
Jun 22, 2023
Merged

fix(idempotency): pass lambda context remaining time to save inprogress #1540

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62a52a3
fix in_progress_expiration timestamp in idempotency handler
Jun 21, 2023
be77f07
add lambda context mock to tests
Jun 22, 2023
848d97d
adjust handler signature with context
Jun 22, 2023
a35c8ed
revert tests to create config, if not passed, undefined temporary
Jun 22, 2023
55b9081
decorator can now register context too
Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/idempotency/src/IdempotencyHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ export class IdempotencyHandler<U> {

try {
await this.persistenceStore.saveInProgress(
this.functionPayloadToBeHashed
this.functionPayloadToBeHashed,
this.idempotencyConfig.lambdaContext?.getRemainingTimeInMillis()
);
} catch (e) {
if (e instanceof IdempotencyItemAlreadyExistsError) {
Expand Down
1 change: 1 addition & 0 deletions packages/idempotency/src/idempotentDecorator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const idempotent = function (
const functionPayloadtoBeHashed = isFunctionOption(options)
? record[(options as IdempotencyFunctionOptions).dataKeywordArgument]
: record;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with the current signature (& the comment about future refactoring), can we not do any assertion when idempotentLambdaHandler is used on the second argument like we do for the fn wrapper?

Like, getting the second argument which might or might not be context (example), and then doing an assertion on its content (example).

Is it worth it? Should we not do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, adding args to check the context works. I have added a copy of isContext, it needs a better place. Exporting would mix wrapper and decorator imports. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for now, we should probably move it to commons together with the other type guards I'm adding in #1528.

Let's leave this for now.

const idempotencyConfig = options.config
? options.config
: new IdempotencyConfig({});
Expand Down
7 changes: 7 additions & 0 deletions packages/idempotency/tests/e2e/makeFunctionIdempotent.test.FunctionCode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -32,15 +33,19 @@ const processRecord = (record: Record<string, unknown>): 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<void> => {
idempotencyConfig.registerLambdaContext(_context);
for (const record of _event.records) {
const result = await processIdempotently(record);
logger.info(result.toString());
Expand All @@ -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<void> => {
idempotencyConfig.registerLambdaContext(_context);
for (const record of _event.records) {
const result = await processIdempotentlyCustomized(record);
logger.info(result.toString());
Expand Down
9 changes: 9 additions & 0 deletions packages/idempotency/tests/e2e/makeFunctionIdempotent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ describe('Idempotency e2e test function wrapper, default settings', () => {
{ id: 3, foo: 'bar' },
],
};
const invokeStart = Date.now();
await invokeFunction(
functionNameDefault,
2,
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand Down
34 changes: 34 additions & 0 deletions packages/idempotency/tests/unit/IdempotencyHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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', () => {
Expand Down
47 changes: 33 additions & 14 deletions packages/idempotency/tests/unit/idempotentDecorator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
IdempotencyPersistenceLayerError,
} from '../../src/Exceptions';
import { IdempotencyConfig } from '../../src';
import { Context } from 'aws-lambda';

const mockSaveInProgress = jest
.spyOn(BasePersistenceLayer.prototype, 'saveInProgress')
Expand All @@ -26,6 +27,14 @@ 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;

const mockConfig: IdempotencyConfig = new IdempotencyConfig({});

class PersistenceLayerTestClass extends BasePersistenceLayer {
protected _deleteRecord = jest.fn();
protected _getRecord = jest.fn();
Expand All @@ -39,21 +48,25 @@ class TestinClassWithLambdaHandler {
@idempotentLambdaHandler({
persistenceStore: new PersistenceLayerTestClass(),
})
public testing(record: Record<string, unknown>): string {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
public testing(record: Record<string, unknown>, context: Context): string {
functionalityToDecorate(record);

return 'Hi';
}
}

class TestingClassWithFunctionDecorator {
public handler(record: Record<string, unknown>): string {
public handler(record: Record<string, unknown>, context: Context): string {
mockConfig.registerLambdaContext(context);

return this.proccessRecord(record);
}

@idempotentFunction({
persistenceStore: new PersistenceLayerTestClass(),
dataKeywordArgument: 'testingKey',
config: mockConfig,
})
public proccessRecord(record: Record<string, unknown>): string {
functionalityToDecorate(record);
Expand All @@ -72,11 +85,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, mockLambaContext);
});

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 decorated', () => {
Expand All @@ -92,11 +108,11 @@ 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, mockLambaContext);
});

test('Then it will save the record to INPROGRESS', () => {
expect(mockSaveInProgress).toBeCalledWith(inputRecord);
expect(mockSaveInProgress).toBeCalledWith(inputRecord, undefined);
});

test('Then it will call the function that was decorated', () => {
Expand All @@ -122,14 +138,14 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler =
new IdempotencyRecord(idempotencyOptions)
);
try {
await classWithLambdaHandler.testing(inputRecord);
await classWithLambdaHandler.testing(inputRecord, mockLambaContext);
} 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, undefined);
});

test('Then it will get the previous execution record', () => {
Expand Down Expand Up @@ -159,14 +175,14 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler =
new IdempotencyRecord(idempotencyOptions)
);
try {
await classWithLambdaHandler.testing(inputRecord);
await classWithLambdaHandler.testing(inputRecord, mockLambaContext);
} 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, undefined);
});

test('Then it will get the previous execution record', () => {
Expand Down Expand Up @@ -195,11 +211,11 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler =
mockGetRecord.mockResolvedValue(
new IdempotencyRecord(idempotencyOptions)
);
await classWithLambdaHandler.testing(inputRecord);
await classWithLambdaHandler.testing(inputRecord, mockLambaContext);
});

test('Then it will attempt to save the record to INPROGRESS', () => {
expect(mockSaveInProgress).toBeCalledWith(inputRecord);
expect(mockSaveInProgress).toBeCalledWith(inputRecord, undefined);
});

test('Then it will get the previous execution record', () => {
Expand All @@ -215,7 +231,7 @@ describe('Given a class with a function to decorate', (classWithLambdaHandler =
class TestinClassWithLambdaHandlerWithConfig {
@idempotentLambdaHandler({
persistenceStore: new PersistenceLayerTestClass(),
config: new IdempotencyConfig({}),
config: new IdempotencyConfig({ lambdaContext: mockLambaContext }),
})
public testing(record: Record<string, unknown>): string {
functionalityToDecorate(record);
Expand All @@ -237,7 +253,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,
mockLambaContext.getRemainingTimeInMillis()
);
});

test('Then an IdempotencyPersistenceLayerError is thrown', () => {
Expand Down
34 changes: 29 additions & 5 deletions packages/idempotency/tests/unit/makeFunctionIdempotent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -25,6 +27,12 @@ const mockGetRecord = jest
.spyOn(BasePersistenceLayer.prototype, 'getRecord')
.mockImplementation();

const mockLambaContext: Context = {
am29d marked this conversation as resolved.
Show resolved Hide resolved
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();
Expand All @@ -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 = {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down