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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(datastore): Update outbox comparison logic to fix update versioning problems #12813

Merged
merged 3 commits into from
Jan 10, 2024
Merged

fix(datastore): Update outbox comparison logic to fix update versioning problems #12813

merged 3 commits into from
Jan 10, 2024

Conversation

stocaaro
Copy link
Contributor

@stocaaro stocaaro commented Jan 8, 2024 •
edited

Description of changes

Outcome:

  • To allow multiple partial updates, outbox checks compare all updated fields for deep equality with the returned object, but ignore extra returned fields when gating outbox _version maintenance.
  • As a result, our rapid update use-cases will not drop updates

Issue #, if available

Description of how you validated changes

Validated by updating tests and testing against sample apps manaully for both Automerge and OCC usecases

Related changes

  1. refactor(datastore): Move conflict resolution tests to their own file聽#12728
  2. refactor(datastore): Refactor conflict resolution tests optimize for concise/readable聽#12732
  3. refactor(datastore): Refactor test names and make subscription events more sample app like聽#12740
  4. test(datastore): Add OptimisticConcurrency tests聽#12795
  5. 馃尡 fix(datastore): Update outbox comparison logic to fix update versioning problems

Checklist

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

@stocaaro
Squashed commit of the following:
1c1d216 
commit 63507e6
Author: Aaron S <stocaaro@stocad.com>
Date:   Mon Jan 8 14:51:15 2024 -0600

    fix(datastore): Consecutive updates bug fix

commit 5492d52
Author: Aaron S <stocaaro@stocad.com>
Date:   Mon Jan 8 13:45:32 2024 -0600

    Remove dead function

commit 8af79b8
Merge: 3fd5f7d 4763b55
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Mon Jan 8 13:38:31 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit 3fd5f7d
Merge: fe33aa9 d0911f5
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Mon Jan 8 12:58:07 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit fe33aa9
Author: Aaron S <stocaaro@stocad.com>
Date:   Mon Jan 8 12:31:42 2024 -0600

    refactor: Test service fake, remove dead code

commit 965c5b5
Merge: 10ccded 483bf03
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Fri Jan 5 16:39:30 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit 10ccded
Merge: 409f0ab dc4cf29
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Fri Jan 5 10:45:07 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit 409f0ab
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Fri Jan 5 10:45:00 2024 -0600

    Update packages/datastore/__tests__/conflictResolutionBehavior.test.ts

    Co-authored-by: David McAfee <mcafd@amazon.com>

commit 61dfff8
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Fri Jan 5 10:44:49 2024 -0600

    Update packages/datastore/__tests__/helpers/fakes/graphqlService.ts

    Co-authored-by: David McAfee <mcafd@amazon.com>

commit 105fe37
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 12:17:54 2024 -0600

    Fix lint issues

commit 5d59946
Merge: 0972d0f 46038f7
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Thu Jan 4 12:12:48 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit 0972d0f
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 11:59:48 2024 -0600

    Update OCC tests so all external updates succeed

commit b669961
Merge: 23b2ac0 44fdfe8
Author: Aaron S <94858815+stocaaro@users.noreply.github.com>
Date:   Thu Jan 4 11:27:42 2024 -0600

    Merge branch 'main' into 04-consecutive-updates-take2

commit 23b2ac0
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 11:13:20 2024 -0600

    Disambiguate names

commit 08e60d5
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 11:10:21 2024 -0600

    merge

commit 520b8fe
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 11:09:12 2024 -0600

    merge

commit 1ec13d7
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Jan 4 11:02:38 2024 -0600

    Refactor to align events with sample app and clarify fake service count interface

commit f1f883b
Author: Aaron S <stocaaro@stocad.com>
Date:   Thu Dec 21 16:39:25 2023 -0600

    Updates from comments

commit f197d77
Author: Aaron S <stocaaro@stocad.com>
Date:   Wed Jan 3 11:04:17 2024 -0600

    test(datastore): Refactor test names and make subscriptions more latent / realistic

commit 354ab21
Author: Aaron S <stocaaro@stocad.com>
Date:   Wed Jan 3 11:03:06 2024 -0600

    Update packages/datastore/__tests__/helpers/UpdateSequenceHarness.ts

    Co-authored-by: David McAfee <mcafd@amazon.com>

commit 876181e
Author: Aaron S <stocaaro@stocad.com>
Date:   Wed Jan 3 11:02:10 2024 -0600

    Updates from comments

commit 318549d
Author: Aaron S <stocaaro@stocad.com>
Date:   Wed Dec 20 12:05:44 2023 -0600

    Improve comments and get rid of unused features
@stocaaro stocaaro requested a review from a team as a code owner January 8, 2024 21:53
iartemiev
iartemiev previously approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

馃檶馃徎

svidgen
svidgen reviewed Jan 8, 2024
View reviewed changes
Copy link
Contributor

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

There are a couple tests that still read like they have unexpected results. E.g., this line here.

Unless I'm being dense, if those are expected not to change, can we document why the unexpected looking results on those are actually the correct behavior?

packages/datastore/src/util.ts Outdated
* @param valB - The object that may be an equal superset of valA.
* @returns True if valA is a equal subset of valB and False otherwise.
*/
export function objectMatches(
Copy link
Contributor

Choose a reason for hiding this comment

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

The docstring helps, but the naming here is misleading, IMO. This might be low risk in this codebase, but is there semantic/purposeful naming we can give this? Or something more verbose (even at the risk of looking like Java).

Copy link
Contributor Author

@stocaaro stocaaro Jan 9, 2024 •
edited

Choose a reason for hiding this comment

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

Alright. Chased names around for a bit and settled on directedValueEquality, with a input names changed to fromObject and againstObject (which I found to be slightly clearer than source and target, being unable to decided with should be the root object for the directed equality comparison.

@stocaaro stocaaro mentioned this pull request Jan 8, 2024
Closed
3 tasks
@stocaaro
Copy link
Contributor Author

stocaaro commented Jan 9, 2024

Regarding the unclear test. I had two choices. This test gets clearer if the external update title "wins" and is left at the end of the test. I think the test is more interesting / covers a deeper scenario with more nuance as written, so I opted to add a detailed comment instead of altering the test for clarity.

iartemiev
iartemiev previously approved these changes Jan 9, 2024
View reviewed changes
@stocaaro stocaaro merged commit 45b88e2 into aws-amplify:main Jan 10, 2024
30 checks passed
@stocaaro stocaaro mentioned this pull request Jan 10, 2024
Closed
@alex-breen alex-breen mentioned this pull request Jan 26, 2024
Open
3 tasks
@renovate renovate bot mentioned this pull request Apr 24, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svidgen svidgen svidgen approved these changes

@iartemiev iartemiev iartemiev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@stocaaro @svidgen @iartemiev