feat(idempotency): support composite primary key in DynamoDBPersistenceLayer #740

Tankanow · 2021-10-06T13:18:14Z

Issue #694

Description of changes:

Add new optional parameters, key_attr_value and sort_key_attr, to DynamoDBPersistenceLayer. If sort_key_attr is set, then DynamoDBPersistenceLayer will use a composite primary key with the partition key set to a static value, defaults to powertools#idempotency, and the sort key set to the hashed idempotency key.

Checklist

Meet tenets criteria
Update tests
Update docs
PR title follows conventional commit semantics

Breaking change checklist

RFC issue #:

Migration process documented
Implement 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.

Tankanow · 2021-10-06T13:19:12Z

aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py

@@ -20,6 +20,8 @@ def __init__(
        self,
        table_name: str,
        key_attr: str = "id",
+        key_attr_value: str = "powertools#idempotency",


@heitorlessa, I created this as a Draft PR to get your early feedback on the interface. Once we agree on the ergonomics, I'm happy to update documentation and add examples.

@heitorlessa, I didn't see any feedback so I moved this from Draft to Ready. I would still like to update documentation and examples before merging this PR, and would love some feedback on the API before I take the time to do that.

Because DynamoDB IAM resource conditions don't allow you to restrict row level access based on SK, where the function name is known, I'd suggest we change the primary key slightly to: idempotency#function-name.

If they want, for example, to restrict anyone from ever modifying idempotency tokens for X function that has a prod stage or something more specific to their business in the name they now can.

Here are the scenarios I understood by going through the PR, as C is new - please correct me if I misunderstood them.

Scenarios

A) No hash key set.: Use default hash key and value

from aws_lambda_powertools.utilities.idempotency import DynamoDBPersistenceLayer # KEY_ATTR="id" # KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer(table_name="IdempotencyTable")

B) Hash key set.: Use new hash key and default value

# KEY_ATTR="idempotency_key" # KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer(table_name="IdempotencyTable", key_attr="idempotency_key")

C) Hash and Sort key set.: Use new hash and sort key, and default value as sort key

# KEY_ATTR="pk" # SORT_KEY_ATTR="sk" # KEY_ATTR_VALUE="idempotency#test-func" # SORT_KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer( table_name="IdempotencyTable", key_attr="idempotency_key", sort_key_attr="idempotency_key")

@heitorlessa, re the scenarios: yes, C is new. I think there is some copy/pasta in the code, the key_attr and sort_key_attr should match the comments, as in:

# KEY_ATTR="pk" # SORT_KEY_ATTR="sk" # KEY_ATTR_VALUE="idempotency#test-func" # SORT_KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer( table_name="IdempotencyTable", key_attr="pk", # <- changed to match comment sort_key_attr="sk") # <- changed to match comment

There is also a related scenario in which the DynamoDBPersistenceLayer passes the desired value for key_attr_value, e.g.:

# KEY_ATTR="pk" # SORT_KEY_ATTR="sk" # KEY_ATTR_VALUE="known-static-partition-key-value" # SORT_KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer( table_name="IdempotencyTable", key_attr="pk", key_attr_value="known-static-partition-key-value", sort_key_attr="sk")

aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py

tests/functional/idempotency/conftest.py

heitorlessa · 2021-10-08T12:29:22Z

I’m looking at this today, just had an unexpected response from the launch of public Lambda Layers for this project yesterday ;-)

…

On Fri, 8 Oct 2021 at 14:20, Adam Tankanow ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py <#740 (comment)> : > @@ -20,6 +20,8 @@ def __init__( self, table_name: str, key_attr: str = "id", + key_attr_value: str = "powertools#idempotency", @heitorlessa <https://github.com/heitorlessa>, I didn't see any feedback so I moved this from Draft to Ready. I would still like to update documentation and examples before merging this PR, and would love some feedback on the API before I take the time to do that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBDLAITZF4XWVZEJIX3UF3OYDANCNFSM5FOSCTSQ> .

Tankanow · 2021-10-08T16:59:34Z

Sorry for poking Heitor. I wasn't sure if the Draft status of the PR kept it from your busy queue. On Fri, Oct 8, 2021 at 8:29 AM Heitor Lessa ***@***.***> wrote:

…

I’m looking at this today, just had an unexpected response from the launch of public Lambda Layers for this project yesterday ;-) On Fri, 8 Oct 2021 at 14:20, Adam Tankanow ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py > < #740 (comment) > > : > > > @@ -20,6 +20,8 @@ def __init__( > self, > table_name: str, > key_attr: str = "id", > + key_attr_value: str = "powertools#idempotency", > > @heitorlessa <https://github.com/heitorlessa>, I didn't see any feedback > so I moved this from Draft to Ready. I would still like to update > documentation and examples before merging this PR, and would love some > feedback on the API before I take the time to do that. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > < #740 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAZPQBDLAITZF4XWVZEJIX3UF3OYDANCNFSM5FOSCTSQ > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQAXC4HQYXOPQXLE66HB6DUF3P2ZANCNFSM5FOSCTSQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

heitorlessa · 2021-10-11T11:54:37Z

Not at all ;) I was thinking about the cost implications of having a single table for Idempotency -- as in, how would you know whether Idempotency is increasing the overall bill?

E.g. if you're Idempotency table is seeing a lot of activities, this is a good chance to verify whether you have a good strategy for tokens.

With a single table it's difficult to have this visibility due to other data entities being there.

That said, we still should support customers table that use a composite key -- I've got some unexpected meetings but haven't forgot about this yet, and it'll be part of the next release

Thank you for the patience @Tankanow

heitorlessa

Two small changes: 1/ Change PK to something IAM can restrict later, 2/ rollback test fixtures changes so we can do in a separate PR.

tests/functional/idempotency/conftest.py

heitorlessa · 2021-10-12T09:29:33Z

aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py

@@ -20,6 +20,8 @@ def __init__(
        self,
        table_name: str,
        key_attr: str = "id",
+        key_attr_value: str = "powertools#idempotency",


Because DynamoDB IAM resource conditions don't allow you to restrict row level access based on SK, where the function name is known, I'd suggest we change the primary key slightly to: idempotency#function-name.

If they want, for example, to restrict anyone from ever modifying idempotency tokens for X function that has a prod stage or something more specific to their business in the name they now can.

Here are the scenarios I understood by going through the PR, as C is new - please correct me if I misunderstood them.

Scenarios

A) No hash key set.: Use default hash key and value

from aws_lambda_powertools.utilities.idempotency import DynamoDBPersistenceLayer # KEY_ATTR="id" # KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer(table_name="IdempotencyTable")

B) Hash key set.: Use new hash key and default value

# KEY_ATTR="idempotency_key" # KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer(table_name="IdempotencyTable", key_attr="idempotency_key")

C) Hash and Sort key set.: Use new hash and sort key, and default value as sort key

# KEY_ATTR="pk" # SORT_KEY_ATTR="sk" # KEY_ATTR_VALUE="idempotency#test-func" # SORT_KEY_ATTR_VALUE="test-func#ec534ff6b4746107270b412333b59f52" persistence_layer = DynamoDBPersistenceLayer( table_name="IdempotencyTable", key_attr="idempotency_key", sort_key_attr="idempotency_key")

Tankanow · 2021-10-13T13:08:13Z

Not at all ;) I was thinking about the cost implications of having a single table for Idempotency -- as in, how would you know whether Idempotency is increasing the overall bill?

E.g. if you're Idempotency table is seeing a lot of activities, this is a good chance to verify whether you have a good strategy for tokens.

With a single table it's difficult to have this visibility due to other data entities being there.

That said, we still should support customers table that use a composite key -- I've got some unexpected meetings but haven't forgot about this yet, and it'll be part of the next release

Thank you for the patience @Tankanow

@heitorlessa,

This is such a good question; I'm delighted to discuss the cost implications! (Actually, my day job is to build an AWS cost intelligence tool on top of serverless tech). There are of many technical solutions here, though I think the key is to think in higher order KPIs. Some questions to ask are "What User Interactions relate to my idempotent queries/updates?" and "What Features, Customers, etc. do those User Interactions roll up to?" That way companies can tie the cost back to user value! The key for this library would be to ensure that clients can set the DynamoDB keys in such a way that they can easily do this analysis without full table scans.

Tankanow · 2021-10-13T21:57:28Z

aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py

@@ -64,10 +73,14 @@ def __init__(

        self._boto_config = boto_config or Config()
        self._boto3_session = boto3_session or boto3.session.Session()
+        if sort_key_attr == key_attr:


NOTE: I snuck in this extra check =).

heitorlessa · 2021-10-15T11:33:34Z

I'll look into this right after merging the new Event Handle Router, so we can merge this later ;)

to-mc · 2021-10-15T13:32:48Z

This looks good to me - small nitpicks only! Could we possibly change the name of key_attr_value to something more descriptive? Perhaps static_key_value? This could just be a symptom of Friday afternoon, but it took me a while to understand from the code that this was just allowing users to set a static value for the partition key when using a composite primary key.

Also, can we change the variable default to remove the "test-func" part. I understand that code is probably only exercised when testing (outside of a Lambda environment), but it feels wrong to have that in the "real" class. How about letting it be an empty string:

f"idempotency#{os.getenv(constants.LAMBDA_FUNCTION_NAME_ENV, '')}",

Tankanow · 2021-10-21T01:16:54Z

Thanks for you patience @heitorlessa and @cakepietoast.

I made the f"idempotency#{os.getenv(constants.LAMBDA_FUNCTION_NAME_ENV, '')}", change.

Re static_key_value vs key_attr_value, I'm not sure the former is more descriptive; I like the latter because it mirrors the key_attr argument.

to-mc · 2021-10-29T12:15:51Z

Thanks for you patience @heitorlessa and @cakepietoast.

I made the f"idempotency#{os.getenv(constants.LAMBDA_FUNCTION_NAME_ENV, '')}", change.

Re static_key_value vs key_attr_value, I'm not sure the former is more descriptive; I like the latter because it mirrors the key_attr argument.

Apologies for the late response on this one, I missed your reply. I understand your point that key_attr_value looks nice next to key_attr, but I have a hard time with the semantics. key_attr is the name of attribute to use as the partition key. partition_key_attr would have been a much better name, but its hard to change now without breaking compatibility - my mistake there is partly why I'm being a bit persistent on this topic 😄

"Key attribute value" sounds like we're setting the value of the "key attribute", which to my mind is saying the same thing as setting the "key attribute". What we're actually doing is setting a static value to use for the partition key, which is where my suggestion came from.

Do you feel any better about static_pk_value? If you don't mind changing that we'll get this merged and documented asap.

Tankanow · 2021-11-01T13:31:30Z

Thanks for you patience @heitorlessa and @cakepietoast.
I made the f"idempotency#{os.getenv(constants.LAMBDA_FUNCTION_NAME_ENV, '')}", change.
Re static_key_value vs key_attr_value, I'm not sure the former is more descriptive; I like the latter because it mirrors the key_attr argument.

Apologies for the late response on this one, I missed your reply. I understand your point that key_attr_value looks nice next to key_attr, but I have a hard time with the semantics. key_attr is the name of attribute to use as the partition key. partition_key_attr would have been a much better name, but its hard to change now without breaking compatibility - my mistake there is partly why I'm being a bit persistent on this topic 😄

"Key attribute value" sounds like we're setting the value of the "key attribute", which to my mind is saying the same thing as setting the "key attribute". What we're actually doing is setting a static value to use for the partition key, which is where my suggestion came from.

Do you feel any better about static_pk_value? If you don't mind changing that we'll get this merged and documented asap.

I like static_pk_value better. Making the change ASAP.

to-mc · 2021-11-03T12:59:13Z

I like static_pk_value better. Making the change ASAP.

Much appreciated!

Tankanow · 2021-11-03T14:56:16Z

I like static_pk_value better. Making the change ASAP.

Much appreciated!

Done. Ready for re-review.

to-mc

LGTM!

I reviewed it

ISSUE-694: add sort key to DynamoDBPersistenceLayer

441c832

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 6, 2021

boring-cyborg bot added area/utilities tests labels Oct 6, 2021

Tankanow commented Oct 6, 2021

View reviewed changes

Tankanow commented Oct 7, 2021

View reviewed changes

aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py Show resolved Hide resolved

tests/functional/idempotency/conftest.py Outdated Show resolved Hide resolved

Tankanow marked this pull request as ready for review October 8, 2021 12:18

heitorlessa self-assigned this Oct 8, 2021

heitorlessa added the feature New feature or functionality label Oct 8, 2021

heitorlessa previously requested changes Oct 12, 2021

View reviewed changes

ISSUE-694: change default pk; revert test changes for now

9548e22

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2021

Tankanow requested a review from heitorlessa October 13, 2021 21:55

Tankanow commented Oct 13, 2021

View reviewed changes

ISSUE-694: PR Updates - remove test-func default name

1947829

heitorlessa assigned to-mc Oct 29, 2021

heitorlessa removed their assignment Oct 29, 2021

ISSUE-694: Change key_attr_value based on PR feedback

7f6091b

pull-request-size bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 3, 2021

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 3, 2021

to-mc self-requested a review November 5, 2021 10:21

to-mc approved these changes Nov 5, 2021

View reviewed changes

to-mc merged commit 6c451f7 into aws-powertools:develop Nov 9, 2021

heitorlessa changed the title ~~improv: Support a composite primary key in DynamoDBPersistenceLayer~~ feat(idempotency): support composite primary key in DynamoDBPersistenceLayer Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(idempotency): support composite primary key in DynamoDBPersistenceLayer #740

feat(idempotency): support composite primary key in DynamoDBPersistenceLayer #740

Tankanow commented Oct 6, 2021 •

edited

Loading

Tankanow Oct 6, 2021

Tankanow Oct 8, 2021

heitorlessa Oct 12, 2021

Tankanow Oct 13, 2021

heitorlessa commented Oct 8, 2021 via email

Tankanow commented Oct 8, 2021 via email

heitorlessa commented Oct 11, 2021

heitorlessa left a comment

heitorlessa Oct 12, 2021

Tankanow commented Oct 13, 2021 •

edited

Loading

Tankanow Oct 13, 2021

heitorlessa commented Oct 15, 2021

to-mc commented Oct 15, 2021

Tankanow commented Oct 21, 2021

to-mc commented Oct 29, 2021

Tankanow commented Nov 1, 2021

to-mc commented Nov 3, 2021

Tankanow commented Nov 3, 2021

to-mc left a comment

feat(idempotency): support composite primary key in DynamoDBPersistenceLayer #740

feat(idempotency): support composite primary key in DynamoDBPersistenceLayer #740

Conversation

Tankanow commented Oct 6, 2021 • edited Loading

Description of changes:

Breaking change checklist

Tankanow Oct 6, 2021

Choose a reason for hiding this comment

Tankanow Oct 8, 2021

Choose a reason for hiding this comment

heitorlessa Oct 12, 2021

Choose a reason for hiding this comment

Scenarios

Tankanow Oct 13, 2021

Choose a reason for hiding this comment

heitorlessa commented Oct 8, 2021 via email

Tankanow commented Oct 8, 2021 via email

heitorlessa commented Oct 11, 2021

heitorlessa left a comment

Choose a reason for hiding this comment

heitorlessa Oct 12, 2021

Choose a reason for hiding this comment

Scenarios

Tankanow commented Oct 13, 2021 • edited Loading

Tankanow Oct 13, 2021

Choose a reason for hiding this comment

heitorlessa commented Oct 15, 2021

to-mc commented Oct 15, 2021

Tankanow commented Oct 21, 2021

to-mc commented Oct 29, 2021

Tankanow commented Nov 1, 2021

to-mc commented Nov 3, 2021

Tankanow commented Nov 3, 2021

to-mc left a comment

Choose a reason for hiding this comment

Tankanow commented Oct 6, 2021 •

edited

Loading

Tankanow commented Oct 13, 2021 •

edited

Loading