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(datastore): Fix retry local after a deletion #2532

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

mattcreaser
Copy link
Member

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:

#2261

Description of changes:

If a conflict occurs during a deletion and a retry local strategy is used in the conflict handler then we want to invoke a delete operation. This was previously always doing an update operation, which would result in the deletion being lost.

How did you test these changes?

  • Followed these reproduction steps:
    • Configure DataStore with a conflictHandler that always returns a retryLocal() decision.
    • Sync a model to the device
    • Put the device into airplane mode
    • Delete the model on the device
    • In DynamoDB, increase the version number of that model
    • Turn off airplane mode on the device

When the device tries to publish the pending mutation it will result in a conflict because the version numbers do not match. The conflict handler will initiate a retry of the local deletion. The expected behaviour is that this retry will delete the record.

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mattcreaser mattcreaser requested a review from a team as a code owner July 26, 2023 14:49
tylerjroach
tylerjroach previously approved these changes Jul 26, 2023
Copy link
Member

@tylerjroach tylerjroach left a comment

Choose a reason for hiding this comment

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

Nice find!

If a conflict occurs during a deletion and a retry local strategy is used in the conflict handler then we want to invoke a delete operation. This was previously always doing an update operation, which would result in the deletion being lost.
@mattcreaser mattcreaser enabled auto-merge (squash) July 26, 2023 16:05
@codecov-commenter
Copy link

Codecov Report

Merging #2532 (f690c16) into main (6b30f97) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #2532      +/-   ##
==========================================
+ Coverage   40.63%   40.64%   +0.01%     
==========================================
  Files         873      873              
  Lines       27551    27554       +3     
  Branches     3873     3874       +1     
==========================================
+ Hits        11195    11199       +4     
+ Misses      15156    15155       -1     
  Partials     1200     1200              

@mattcreaser mattcreaser merged commit 16a21d5 into main Jul 27, 2023
1 check passed
@mattcreaser mattcreaser deleted the mattcreaser/delete-conflict branch July 27, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants