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

fix: correct behaviour to avoid caching "INPROGRESS" records #295

Merged
merged 7 commits into from Feb 20, 2021

Conversation

to-mc
Copy link
Contributor

@to-mc to-mc commented Feb 19, 2021

Issue #, if available:

Description of changes:

This fixes an issue with caching in the unreleased idempotency util. We were mistakenly caching in progress records, meaning execution environments could get "stuck" with in progress records in the cache. Records in this state shouldn't be included in the cache, as we've no way to reflect changes from outside the execution environment in the local caches.

Checklist

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.

@to-mc to-mc added area/utilities bug Something isn't working documentation Improvements or additions to documentation labels Feb 19, 2021
@michaelbrewer
Copy link
Contributor

@cakepietoast haha. that was quick :)

Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelbrewer
Copy link
Contributor

@cakepietoast @heitorlessa maybe to help with code review we can try to use more GIVEN, WHEN & THENs.

At least just to cover the basic behavors we put into the RFCs. Coz then we are less likely to miss things

Copy link
Contributor

@nmoutschen nmoutschen left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

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

I have attached my proposal changes

changes.diff.zip

@michaelbrewer
Copy link
Contributor

@cakepietoast @heitorlessa

I spend a little time on this and attached a diff of changes

Changes:

base.py

  • Make STATUS_CONSTANTS a frozen dict (by using MappingProxyType)
  • status - Make status call internal mutually exclusive
  • _validate_payload - Fix doc string typo
  • _validate_payload - Simplifies conditional logic using De Morgan's Identity
  • _save_to_cache, _retrieve_from_cache, _delete_from_cache to check use_local_cache and exit early
  • _save_to_cache - to not save for status == "INPROGRESS"
  • _save_to_cache - add doc string to explain has "INPROGRESS" is not saved
  • save_success - Fix doc string typo
  • save_success - Just call _save_to_cache directly
  • save_inprogress - Just call _retrieve_from_cache directly
  • delete_record - Just call _delete_from_cache directly
  • get_record - Simplify logic by calling _retrieve_from_cache and _save_to_cache directly

test_idempotency.py

  • test_idempotent_lambda_in_progress_with_cache update check to save_to_cache_spy.assert_called()
  • test_in_progress_never_saved_to_cache - add new test to check "INPROGRESS" is not saved to local cache
  • test_user_local_disabled - add new test to check if all local cache options work when use_local_cache is False

test_json_encoder.py

  • test_jsonencode_calls_default - Add missing test case for unhandled types

Attached diff

@github-actions github-actions bot removed area/utilities documentation Improvements or additions to documentation labels Feb 20, 2021
@codecov-io
Copy link

codecov-io commented Feb 20, 2021

Codecov Report

Merging #295 (a7f8199) into develop (03f7dcd) will increase coverage by 0.06%.
The diff coverage is 95.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #295      +/-   ##
===========================================
+ Coverage    99.64%   99.71%   +0.06%     
===========================================
  Files           86       86              
  Lines         3142     3143       +1     
  Branches       151      149       -2     
===========================================
+ Hits          3131     3134       +3     
+ Misses           6        5       -1     
+ Partials         5        4       -1     
Impacted Files Coverage Δ
...wertools/utilities/idempotency/persistence/base.py 99.18% <95.45%> (+<0.01%) ⬆️
aws_lambda_powertools/shared/json_encoder.py 100.00% <0.00%> (+20.00%) ⬆️

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 03f7dcd...a7f8199. Read the comment docs.

@to-mc to-mc merged commit f1a8832 into develop Feb 20, 2021
@to-mc to-mc deleted the fix/idempotency_cache branch February 20, 2021 20:34
@to-mc
Copy link
Contributor Author

to-mc commented Feb 20, 2021

@cakepietoast @heitorlessa maybe to help with code review we can try to use more GIVEN, WHEN & THENs.

At least just to cover the basic behavors we put into the RFCs. Coz then we are less likely to miss things

Yep, you're completely right on this. Its been on my todo list with this PR since the beginning but it got lost amongst the rest. I'll get this added as soon as I get chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants