Skip to content

fix(idempotency): revert dict mutation that impacted static_pk_value feature #1970

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.

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

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Mar 1, 2023

Issue number: #1968

NOTE: This is literally a contribution from @Tankanow. Had to cut a PR to get it merged quicker due to the regression.

Summary

Changes

Please provide a summary of what's being changed

Reverts a regression added in #1899 when making DynamoDB client thread-safe.

static_pk_value is used by customers using Single Table design pattern with DynamoDB, where a composite key is used with a static value for its "primary key" attr. This change reverts a dictionary mutation that discarded the value from static_pk_value and used key_id instead.

In simple terms, same effect as:

a = {"my_key": "must_be_static"}
b = {
    **a,
    "my_key": "this is the final value"  # this shouldn't exist as it'll always win last insertion
}

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@heitorlessa heitorlessa requested a review from a team as a code owner March 1, 2023 20:18
@heitorlessa heitorlessa requested review from rubenfonseca and removed request for a team March 1, 2023 20:18
@boring-cyborg boring-cyborg bot added the tests label Mar 1, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 1, 2023
@github-actions github-actions bot added the bug Something isn't working label Mar 1, 2023
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 1, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (a318e8e) 97.43% compared to head (f87778a) 97.44%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1970      +/-   ##
===========================================
+ Coverage    97.43%   97.44%   +0.01%     
===========================================
  Files          146      146              
  Lines         6660     6660              
  Branches       478      478              
===========================================
+ Hits          6489     6490       +1     
  Misses         134      134              
+ Partials        37       36       -1     
Impacted Files Coverage Δ
...ools/utilities/idempotency/persistence/dynamodb.py 100.00% <ø> (+1.23%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@heitorlessa heitorlessa added the idempotency Idempotency utility label Mar 1, 2023
Copy link
Contributor

@Tankanow Tankanow left a comment

Choose a reason for hiding this comment

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

I don't have the power, but FWIW, Approved. 😄

@heitorlessa heitorlessa merged commit ca9686d into aws-powertools:develop Mar 1, 2023
@heitorlessa heitorlessa deleted the fix/idempotency-static-pk-value-regression branch March 1, 2023 20:42
@heitorlessa
Copy link
Contributor Author

Added co-authorship to the merge commit but as your email isn't public the no-reply might not credit it correctly (I'll do in the release regardless)

ca9686d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working idempotency Idempotency utility size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants