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.

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

feat(idempotency): Add raise_on_no_idempotency_key flag #297

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Feb 22, 2021

Description of changes:

Add an option to raise an error when idempotency key value is None

GIVEN a persistence_store with event_key_jmespath = body and option raise_on_no_idempotency_key
WHEN getting the hashed idempotency key with an event without a body key value
THEN raise IdempotencyKeyError

Checklist

Breaking change checklist

Raising any kind of error when not passing in the data for the idemptency data is not what we currently do

GIVEN a persistence_store with event_key_jmespath = `body`
WHEN getting the hashed idempotency key with an event without a `body` key
THEN raise IdempotencyValidationError
@michaelbrewer
Copy link
Contributor Author

@cakepietoast @heitorlessa - you might want to debate this.

For example when event_key_jmespath = "[body, queryStringParameters]"
and the event is {} then the data is [None, None]

@codecov-io
Copy link

codecov-io commented Feb 22, 2021

Codecov Report

Merging #297 (8308fe1) into develop (f1a8832) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #297   +/-   ##
========================================
  Coverage    99.71%   99.71%           
========================================
  Files           86       86           
  Lines         3143     3145    +2     
  Branches       149      150    +1     
========================================
+ Hits          3134     3136    +2     
  Misses           5        5           
  Partials         4        4           
Impacted Files Coverage Δ
...wertools/utilities/idempotency/persistence/base.py 99.20% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1a8832...8308fe1. Read the comment docs.

@michaelbrewer
Copy link
Contributor Author

@cakepietoast @heitorlessa - you might want to debate this.

For example when event_key_jmespath = "[body, queryStringParameters]"
and the event is {} then the data is [None, None]

Also if we do raise an error, should it be a new type for Idempotency key errors?

@michaelbrewer michaelbrewer changed the title feat(idempotencty): Raise error when event data is None feat(idempotency): Option to raise an error when idempotency key is None Feb 25, 2021
Add `raise_on_no_idempotency_key` to enforce a idempotency key value is
passed into the lambda.
@michaelbrewer michaelbrewer changed the title feat(idempotency): Option to raise an error when idempotency key is None feat(idempotency): Add raise_on_no_idempotency_key flag Feb 25, 2021
@michaelbrewer
Copy link
Contributor Author

michaelbrewer commented Feb 25, 2021

@heitorlessa @cakepietoast - i added a raise_on_no_idempotency_key option and a new exception IdempotencyKeyError

@michaelbrewer
Copy link
Contributor Author

@heitorlessa any concerns or changes needed for this PR ?

@heitorlessa
Copy link
Contributor

heitorlessa commented Mar 2, 2021 via email

@heitorlessa
Copy link
Contributor

Checking this now

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

LGTM after logic to catch iterables like empty tuple and dicts as well as doc

tests/functional/idempotency/test_idempotency.py Outdated Show resolved Hide resolved
### Making idempotency key required

By default, events without any idempotency key don't raise any exception and just trigger a warning.
If you want to ensure that at an idempotency is found, you can pass in `raise_on_no_idempotency_key` as True and an
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean by "If you want to ensure that at an idempotency is found" - could you simplify this to non-native English speakers, please?

Take in account for empty data structures, including non-lists iterables like tuples, dictionaries

Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>
@heitorlessa
Copy link
Contributor

Merging it - Thanks a lot @michaelbrewer 🎉

@heitorlessa heitorlessa merged commit 6e293b1 into aws-powertools:develop Mar 3, 2021
@michaelbrewer michaelbrewer deleted the feat-indemptency-none-for-idempotency_key branch March 3, 2021 13:52
@heitorlessa heitorlessa added enhancement feature New feature or functionality and removed enhancement labels Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants