fix(@aws-amplify/datastore): fixes observeQuery not removing newly-filtered items from snapshot

[svidgen]

Description of changes

observeQuery() was not removing items from the snapshot if they later become invalid through an update. Those updates were being pre-filtered by an inner observe, leaving observeQuery with no signal to remove them from or emit a new snapshot.

Issue #, if available

#9325 #9573

Description of how you validated changes

New tests added that simulate two update-to-remove scenarios; one during generation of the initial snapshot(s), and another after sync is done.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

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

[codecov-commenter]

Codecov Report

Merging #9879 (cd09343) into main (61d60c7) will increase coverage by 0.02%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #9879      +/-   ##
==========================================
+ Coverage   83.46%   83.49%   +0.02%     
==========================================
  Files         253      253              
  Lines       18196    18212      +16     
  Branches     3899     3908       +9     
==========================================
+ Hits        15188    15206      +18     
+ Misses       2918     2916       -2     
  Partials       90       90              

Impacted Files	Coverage Δ
packages/datastore/src/datastore/datastore.ts	83.50% <92.30%> (+0.53%)	⬆️
.../datastore/src/storage/adapter/IndexedDBAdapter.ts	86.85% <0.00%> (+0.25%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[lgtm-com]

This pull request introduces 1 alert when merging 9c239b4 into 94813a9 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[manueliglesias]

The logic looks good to me, I would love to have more robust test even if that means defining a model per test to prevent cross talk

[david-mcafee]

Left a question re: observe

[david-mcafee]

According to the PR description: "observeQuery() was not removing items from the snapshot if they later become invalid through an update. Those updates were being pre-filtered by an inner observe, leaving observeQuery with no signal to remove them from or emit a new snapshot" - does this mean that observe is not filtering the items as we'd expect it to? i.e. is observe broken, or am I perhaps misunderstanding?

[svidgen]

Heh. It is confusing! The problem was that the criteria were being passed to observe and observe was doing exactly what it's supposed to.

The TLDR is that observeQuery was calling observe and passing the criteria through. So, if an item was in an initial result set, updated to not be in the result set, observe would never tell observeQuery about it, because the updated item would not match observe's give criteria.

In a little more detail, if it helps:

Old flow:

1. app calls observeQuery(Model, m => m.x('eq', 'abc'))
2. observeQuery calls query(Model, m => m.x('eq', 'abc'))
3. observeQuery calls observe(Model, m => m.x('eq', 'abc'))
4. app calls DataStore.save(new Model({x: 'abc'}))
5. observe sees it
6. observe checks to see if it matches (m => m.x('eq', 'abc'))
7. observe passes it to observeQuery
8. observeQuery adds it to the snapshot
9. app calls DataStore.save(Model.copyOf(model, d => d.x = 'NOT abc!!!'))
10. observe sees it
11. observe checks to see if it matches (m => m.x('eq', 'abc'))
12. observe discards it
13. observeQuery gets no update: the snapshot now incorrectly still includes the outdated {x: 'abc'} item

The new flow moves the criteria test.

1. app calls observeQuery(Model, m => m.x('eq', 'abc'))
2. observeQuery calls query(Model, m => m.x('eq', 'abc'))
3. observeQuery calls observe(Model, Predicates.ALL) (effectively)
4. app calls DataStore.save(new Model({x: 'abc'}))
5. observe sees it
6. observe passes it to observeQuery
7. observeQuery checks to see if it matches m => m.x('eq', 'abc')
8. observeQuery adds it to the snapshot
9. app calls DataStore.save(Model.copyOf(model, d => d.x = 'NOT abc!!!'))
10. observe sees it
11. observe passes it to observeQuery
12. observeQuery checks to see if it matches m => m.x('eq', 'abc')
13. observeQuery correctly removes the item from the snapshot

[undefobj]

Reading this update what occurs to me is that the foundational data structure here which this change is managing bookkeeping is the "snapshot". Is this an in-memory array? While the above updated flow makes sense in the workflow of the app notifications to observers, it's a bit different from the Mutation queue in the outbox that DataStore.save() also writes to in this sequence. In the case of the outbox flow you have a CREATE followed by an UPDATE WITH CONDITION which means the original item will be written to a key and then the update will be appended to the end of the queue. This is to be expected as it gives us a causal chain of events. It's worth thinking about if we have two logic streams now - one publishing the event stream to the cloud, one publishing the subscriber stream to local observers - with differing logic and filtering rules, and if this will cause maintenance or logic race conditions in the future. Not enough to block the merge of this PR, but we should whiteboard this and look at the longer term ramifications.

[svidgen]

@david-mcafee Does this answer your question?

@undefobj In slightly more detail, it's a Map that gets Array'd for the snapshot. I think I agree that these are different logic stream, but I think they need to be. Writes to the outbox are merged by PK alone (in this case). The same has to occur for the observeQuery snapshots, but with the added consideration for the observeQuery criteria, which has the effect of moving items in and out of the set. The closer analogous problem that concerns me is whether we encounter the same problem for selective sync, especially once we're leveraging RTF.

[david-mcafee]

LGTM! 🎉

[manueliglesias]

LGTM 👍

[manueliglesias]

Can we avoid this naughtiness by using different models for each test?

It may make the test more verbose but will make them more isolated from each other

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server amplify-help forum.