feat(idempotency): Add persistence layer and DynamoDB implementation #1110

jeffrey-baker-vg · 2022-10-06T17:47:46Z

Description of your changes

This PR includes the creation the the persistence layer code in the Idempotency package necessary to provide the the most basic idempotency functionality. This includes:

The ability to create and update Idempotency records in all possible statuses
Hashing the input to the function and using that hash to create the idempotency key
Defaulting the persistence layer to a DynamoDB table and creating the concrete class to make updates to this table

This PR is the first step toward adding basic idempotency functionality. Once the code is merged, we plan to create the makeFunctionIdempotent function wrapper, which will use this persistence layer to implement the minimum code required to make a function idempotent.

How to verify this change

Related issues, RFCs

Issue number: 447

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

Breaking change checklist

I have documented the migration process
I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

dreamorosi · 2022-10-06T19:33:56Z

Hi team, thank you for the PR!

Before we can start reviewing it could you please take some time to rebase the PR, address the merge conflicts, and also fill in the PR details? It would be really helpful given the amount of new code to have some insights.

Really appreciate your time and effort on this.

jeffrey-baker-vg · 2022-10-06T19:43:15Z

Hi team, thank you for the PR!

Before we can start reviewing it could you please take some time to rebase the PR, address the merge conflicts, and also fill in the PR details? It would be really helpful given the amount of new code to have some insights.

Really appreciate your time and effort on this.

Absolutely! As you can probably tell, this is very incomplete. This pull request was actually intended for my forked repo, with the idea being that I would just open a pull request and add to it so the team could stay in sync as we make changes to multiple files. I must have been multi-tasking when I opened the PR, since it was definitely not intended to go here yet.

ijemmy · 2022-10-11T12:52:53Z

@dreamorosi

Just to clarify, @jeffrey-baker-vg will rebase and add more code regarding persistence layer later. For now, please ignore this issue.

I labelled this on-hold to avoid confusion.

…ions

…r get

…, and json response

ijemmy

This looks very good now. I really appreciate the comprehensive test cases. Given the number of users, I believe this will save us from regression bugs in the future.

The biggest comments are the constructors of DynamoDBPersistenceLayer and IdempotencyRecord. I think we should make them more TypeScript style and be consistent with other TypeScript utilities. While we base our implementation on Python one, anything that is not exposed to public can be changed as we see fit.

Apart from that, I have some questions and small nit picking on test cases' names. I put a "suggestion" for quick fix, you can just accept and put the commit in. (We'll squash merge in the end.)

Another change request
Can you remove changes in files under layer-publisher folder? (12' and package-lock.json)

ijemmy · 2022-10-27T11:02:15Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

@@ -1,18 +1,81 @@
 /* eslint-disable @typescript-eslint/no-empty-function */


Can we remove this now?

ijemmy · 2022-10-31T15:27:13Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+  public constructor(private tableName: string, private key_attr: string = 'id',
+    private status_attr: string = 'status', private expiry_attr: string = 'expiration',
+    private in_progress_expiry_attr: string = 'in_progress_expiry_attr',
+    private data_attr: string = 'data') {


A few things:

Let's keep the variable naming convention in TypeScript way ,camelCase rather than snakeCase. For the value inserted in DynamoDB (e.g. in_progress_expiry_attr, let's stick with what Python team uses for consistency)

Can we make this one line per variable for readability?

Can we pass an object DynamoDBPersistenceLayerOption instead? It's more common for TypeScript to use an argument object rather than a long list of parameter. This also eliminate the need of "order" and easier to use. Logger also uses the same convention.

Python's logger:

TypeScript Logger

+1 on this, let's make this an object with its dedicated type like in the other utilities.

ijemmy · 2022-10-31T15:36:15Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+    const output: GetCommandOutput = await table.get(
+      {
+        TableName: this.tableName, Key: { [this.key_attr]: idempotencyKey }
+      }
+    );


Do we need a strong consistency read here?

Also I'm curious about the naming convention with the _ prefix.

If it's used to specify the access pattern let's use the private, protected, public, etc. instead.

Removing the _ creates a naming conflict between this and its parent class. We could change the name from getRecord to something else to solve this. In the Python library the parent also defines a more a generic getRecord and relies on the implementation of the abstract class to define a more specific _getRecord that does any logic specific to the concrete persistence layer.

Understood, thanks.

ijemmy · 2022-10-31T15:40:28Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+  protected async _putRecord(_record: IdempotencyRecord): Promise<void> {
+    const table: DynamoDBDocument = this.getTable();
+
+    const item = { [this.key_attr]: _record.idempotencyKey, [this.expiry_attr]: _record.expiryTimestamp, [this.status_attr]: _record.getStatus() };


Can we break this into multiple lines for readability?

ijemmy · 2022-10-31T15:42:21Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+    const notInProgress = 'NOT #status = :inprogress';
+    const conditionalExpression = `${idempotencyKeyDoesNotExist} OR ${idempotencyKeyExpired} OR ${notInProgress}`;
+    try {
+      await table.put({ TableName: this.tableName, Item: item, ExpressionAttributeNames: { '#id': this.key_attr, '#expiry': this.expiry_attr, '#status': this.status_attr }, ExpressionAttributeValues: { ':now': Date.now() / 1000, ':inprogress': IdempotencyRecordStatus.INPROGRESS }, ConditionExpression: conditionalExpression });


Can we break this into one line per field?

await table.put({ TableName: this.tableName, Item: item, ExpressionAttributeNames: { //... }, //... });

I would consider setting up linting with the same config as the rest of the project and then run npm run lint-fix so that all these are taken care of.

Do you have any specific recommendations for configurating the linting for this package like the rest of the project? I tried adding this package to the lerna.json so it would run the same lint-fix command on the idempotency package, but it did not fix the issue. Running lint fix on the project itself also doesn't do anything different.

@vgphoenixcampos Embarrassingly, we actually don't have this. I use the default one on VSCode but it doesn't break objects into multiple lines.

@dreamorosi How do you this? I'm thinking about using prettier as part of lint-fix target or commit hook. It has plugins for both VSCode and IntelliJ.

Then we will have a massive PR with only formatting :D

Yea, let's park this topic for now and prioritise the logic, you can disregard my comment for now.

I like prettier or similar but I'd like to find a way to do it without messing entirely the history. I know there might be ways but it requires some investigation. For now let's just work with eslint with the config that is already in the project.

packages/idempotency/tests/unit/persistence/PersistenceLayer.test.ts

dreamorosi

Thanks a lot folks for all the work on this PR, this is looking better and better every time I check it.

I have left a number of comments on the code which are mostly in line with the review that Jemmy has already left you. None of the comment is major in terms of impact and most of the changes I'd like to see are related to coding style/structure rather than logic - which is good and expected.

Finally, I also tried to run the tests locally after checking out your branch but was not able to. I get the following errors after running npm I (expand to see):

``` ~/Codes/aws-lambda-powertools-typescript/packages/idempotency pr/jeffrey-baker-vg/1110 14s ❯ npm i

@aws-lambda-powertools/idempotency@0.0.11 prepare
npm run build

@aws-lambda-powertools/idempotency@0.0.11 build
tsc

node_modules/@types/sinon/index.d.ts:1447:53 - error TS2694: Namespace '"/Users/aamorosi/Codes/aws-lambda-powertools-typescript/packages/idempotency/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'FakeTimerInstallOpts'.

1447 useFakeTimers: boolean | Partial<FakeTimers.FakeTimerInstallOpts>;
~~~~~~~~~~~~~~~~~~~~

node_modules/@types/sinon/index.d.ts:1569:67 - error TS2694: Namespace '"/Users/aamorosi/Codes/aws-lambda-powertools-typescript/packages/idempotency/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'FakeTimerInstallOpts'.

1569 useFakeTimers(config?: number | Date | Partial<FakeTimers.FakeTimerInstallOpts>): SinonFakeTimers;
~~~~~~~~~~~~~~~~~~~~

src/persistence/DynamoDbPersistenceLayer.ts:3:52 - error TS2307: Cannot find module '@aws-sdk/lib-dynamodb' or its corresponding type declarations.

3 import { DynamoDBDocument, GetCommandOutput } from '@aws-sdk/lib-dynamodb';
~~~~~~~~~~~~~~~~~~~~~~~

Found 3 errors in 2 files.

Errors Files
2 node_modules/@types/sinon/index.d.ts:1447
1 src/persistence/DynamoDbPersistenceLayer.ts:3
npm ERR! code 2
npm ERR! path /Users/aamorosi/Codes/aws-lambda-powertools-typescript/packages/idempotency
npm ERR! command failed
npm ERR! command sh -c npm run build

npm ERR! A complete log of this run can be found in:
npm ERR! /Users/aamorosi/.npm/_logs/2022-11-02T14_23_18_191Z-debug-0.log

</details>

package.json

dreamorosi · 2022-11-02T13:36:39Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+  public constructor(private tableName: string, private key_attr: string = 'id',
+    private status_attr: string = 'status', private expiry_attr: string = 'expiration',
+    private in_progress_expiry_attr: string = 'in_progress_expiry_attr',
+    private data_attr: string = 'data') {


+1 on this, let's make this an object with its dedicated type like in the other utilities.

dreamorosi · 2022-11-02T13:38:00Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+    const output: GetCommandOutput = await table.get(
+      {
+        TableName: this.tableName, Key: { [this.key_attr]: idempotencyKey }
+      }
+    );


Also I'm curious about the naming convention with the _ prefix.

If it's used to specify the access pattern let's use the private, protected, public, etc. instead.

dreamorosi · 2022-11-02T13:39:19Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+    const notInProgress = 'NOT #status = :inprogress';
+    const conditionalExpression = `${idempotencyKeyDoesNotExist} OR ${idempotencyKeyExpired} OR ${notInProgress}`;
+    try {
+      await table.put({ TableName: this.tableName, Item: item, ExpressionAttributeNames: { '#id': this.key_attr, '#expiry': this.expiry_attr, '#status': this.status_attr }, ExpressionAttributeValues: { ':now': Date.now() / 1000, ':inprogress': IdempotencyRecordStatus.INPROGRESS }, ConditionExpression: conditionalExpression });


I would consider setting up linting with the same config as the rest of the project and then run npm run lint-fix so that all these are taken care of.

packages/idempotency/src/persistence/IdempotencyRecord.ts

dreamorosi · 2022-11-02T13:50:48Z

packages/idempotency/src/persistence/DynamoDbPersistenceLayer.ts

+    if (!this._table)
+      this._table = DynamoDBDocument.from(new DynamoDB({}), { marshallOptions: { removeUndefinedValues: true } });


On principle I like the lazy init as well but I'm not sure the naming/location is correct.

Looking at how boto3 has a Table high-level client the method name of getTable & logic makes sense. However in the JS SDK this is a generic DynamoDB client so I would expect to find this at this.client/this.provider instead of this._table.

To further this, when used (few lines above) you still have to always pass a TableName property anyway. So essentially I would rename this method to getClient and change usages to things like client.update rather than table.update.

Aside from the naming/location however, I think we should consider having the client as a class prop. This would give customers the chance to pass their own client with custom configs (i.e. different region or credentials) as well as apply tracing to it.

packages/idempotency/src/persistence/IdempotencyRecord.ts

packages/idempotency/src/persistence/PersistenceLayer.ts

dreamorosi · 2022-11-02T13:55:20Z

packages/idempotency/tests/helpers/populateEnvironmentVariables.ts

@@ -0,0 +1,2 @@
+// Reserved variables
+process.env.AWS_REGION = 'us-east-1';


Are we sure that this file is still being used? In the diff above I see that the code that calls it was removed

dreamorosi · 2022-11-02T13:55:58Z

packages/idempotency/src/persistence/PersistenceLayerInterface.ts

+  saveInProgress(data: unknown): Promise<void>
+  saveSuccess(data: unknown, result: unknown): Promise<void>
+  deleteRecord(data: unknown): Promise<void>
+  getRecord(data: unknown): Promise<IdempotencyRecord>


on this, let's make an effort to type as much as we can & use generics as needed. unknown should be the last options.

Co-authored-by: ijemmy <ijemmy@users.noreply.github.com>

…ols-typescript

…for readability

ijemmy

Thank you very much. :)

packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts

dreamorosi

Thank you for the PR and for addressing our comments.

We really appreciate the contribution to this project!

ijemmy added the on-hold This item is on-hold and will be revisited in the future label Oct 11, 2022

vgphoenixcampos and others added 26 commits October 27, 2022 16:45

feat: initial idempotency classes

51104f6

feat: refactor persistence layer classes into their own folder

d54e22e

feat: rename idempotency config to differentiate from idempotency opt…

6ea753e

…ions

feat: added type for a generic function

c5f2144

feat: remove idempotency configuration for this FR

ab254c6

feat: refactored type of function to accept any combo of parameters

8677376

feat: adding PersistenceLayer

b955ac4

feat: PersistenceLayer unit tests for saveInProgress

c8776d2

feat: added saveSuccess

f258d55

feat: added getRecord

edc9b33

feat: added delete record

dfc72c0

feat: branch coverage and cleaning up imports

4d63218

feat: added more tests

d92efb1

feat: deleted unused methods

5258d10

feat: added comments

5d82215

feat: implement get command for dynamo persistence layer

95004c2

feat: implement get command for dynamo persistence layer

c5a9335

feat: allow for data attr to be passed and return in persistence laye…

3d1f52c

…r get

feat: added implementation for delete, update, put

0a6e839

feat: create condition on put for not in progress status

b9ae754

feat: use inprogress enum for status

2cd4390

feat: added error when unable to get record for idempotency key

01bcc99

feat: added error for conditional write of an existing record

c1a6c60

feat: tests added for put record on dynamo persistence layer

1011002

feat: implemented the idempotency record functions for status, expiry…

adcfab3

…, and json response

test: check if the status is expired

9f0535e

vgphoenixcampos and others added 3 commits October 27, 2022 17:07

feat: change how time is measured to seconds

d1dad17

feat: change type of the response/result to a record

b9afc2a

feat:updated imports

0ec8301

jeffrey-baker-vg force-pushed the main branch from 6b962ae to 0ec8301 Compare October 28, 2022 13:26

ijemmy suggested changes Oct 31, 2022

View reviewed changes

ijemmy requested a review from dreamorosi October 31, 2022 18:40

ijemmy removed the on-hold This item is on-hold and will be revisited in the future label Oct 31, 2022

dreamorosi requested changes Nov 2, 2022

View reviewed changes

vgphoenixcampos and others added 12 commits November 4, 2022 11:19

refactor: create constructor object for dynamo persistence layer

8e97a61

fix: remove temp eslint disable

afb9523

fix: adjust verbiage on test blocks

8a031ca

Co-authored-by: ijemmy <ijemmy@users.noreply.github.com>

style: put constructor parameters onto one line for readability

f476e21

fix: update dynamo persistence layer tests to use new construtor options

a044fa6

Merge branch 'main' of github.com:jeffrey-baker-vg/aws-lambda-powerto…

cefae2d

…ols-typescript

fix: remove unneeded eslint ignore from persistence layer

dd979a8

style: put parameters for dynamo client command object onto one line …

b290d19

…for readability

fix: move lib-dynamo dep under the correct package

f76c720

refactor: change idempotency record to use options object in contructor

b4931bf

feat: add consistent read to dynamo persistence layer

9b4787a

fix: revert changes to layer-publisher package-lock

08598f9

ijemmy approved these changes Nov 10, 2022

View reviewed changes

packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts Show resolved Hide resolved

dreamorosi self-requested a review November 10, 2022 16:46

dreamorosi assigned jeffrey-baker-vg Nov 10, 2022

dreamorosi approved these changes Nov 10, 2022

View reviewed changes

ijemmy changed the title ~~Feat: Persistence Layer~~ feat(idempotency): Add persistence layer and DynamoDB implementation Nov 11, 2022

github-actions bot added the feature label Nov 11, 2022

dreamorosi merged commit 0a6676a into aws-powertools:main Nov 11, 2022

This was referenced Nov 11, 2022

Maintenance: add Idempotency utility to unit tests #1159

Closed

Maintenance: integrate utility with CI/CD unit test processes #1100

Closed

ijemmy added the idempotency This item relates to the Idempotency Utility label Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(idempotency): Add persistence layer and DynamoDB implementation #1110

feat(idempotency): Add persistence layer and DynamoDB implementation #1110

jeffrey-baker-vg commented Oct 6, 2022 •

edited

dreamorosi commented Oct 6, 2022

jeffrey-baker-vg commented Oct 6, 2022

ijemmy commented Oct 11, 2022

ijemmy left a comment

ijemmy Oct 27, 2022

ijemmy Oct 31, 2022

dreamorosi Nov 2, 2022

ijemmy Oct 31, 2022

dreamorosi Nov 2, 2022

vgphoenixcampos Nov 4, 2022

dreamorosi Nov 9, 2022

ijemmy Oct 31, 2022

ijemmy Oct 31, 2022

dreamorosi Nov 2, 2022

vgphoenixcampos Nov 4, 2022

ijemmy Nov 9, 2022

dreamorosi Nov 9, 2022

dreamorosi left a comment

dreamorosi Nov 2, 2022

dreamorosi Nov 2, 2022

dreamorosi Nov 2, 2022

dreamorosi Nov 2, 2022

dreamorosi Nov 2, 2022

dreamorosi Nov 2, 2022

ijemmy left a comment

dreamorosi left a comment

		@@ -1,18 +1,81 @@
		/* eslint-disable @typescript-eslint/no-empty-function */

		if (!this._table)
		this._table = DynamoDBDocument.from(new DynamoDB({}), { marshallOptions: { removeUndefinedValues: true } });

		@@ -0,0 +1,2 @@
		// Reserved variables
		process.env.AWS_REGION = 'us-east-1';

feat(idempotency): Add persistence layer and DynamoDB implementation #1110

feat(idempotency): Add persistence layer and DynamoDB implementation #1110

Conversation

jeffrey-baker-vg commented Oct 6, 2022 • edited

Description of your changes

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

dreamorosi commented Oct 6, 2022

jeffrey-baker-vg commented Oct 6, 2022

ijemmy commented Oct 11, 2022

ijemmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamorosi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijemmy left a comment

Choose a reason for hiding this comment

dreamorosi left a comment

Choose a reason for hiding this comment

jeffrey-baker-vg commented Oct 6, 2022 •

edited