fix(datastore): stale observeQuery snapshot with sort param #11119

iartemiev · 2023-03-22T19:03:02Z

Description of changes

Relevant impl change is only in datastore.ts (link to block) remaining changes are for testing purposes.

We were applying sort before merging existing and changed snapshot items into the new snapshot.
This was causing stale items to surface in the snapshot.

New PR applies sort after we merge items, fixing the issue.

Issue #, if available

#11101

Description of how you validated changes

Manual
Unit test

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.

manueliglesias

LGTM!

Just make sure to remove the .only from the test

packages/datastore/__tests__/DataStore.ts

david-mcafee

LGTM!

svidgen

Sorting after arrayatizing the final set makes complete sense, so I see no reason to block this. But, I'm actually not clear on why sorting first would result in stale snapshots. Was the test red-greened?

packages/datastore/__tests__/DataStore.ts

svidgen · 2023-03-22T19:28:40Z

packages/datastore/__tests__/DataStore.ts

+		const sub = DataStore.observeQuery(Post, Predicates.ALL, {
+			sort: s => s.updatedAt(SortDirection.DESCENDING),
+		}).subscribe(({ items }) => {
+			if (items.length === 0) {
+				return;
+			}
+
+			const [item] = items;
+			const expected = expectedTitles.shift();
+
+			expect(item!.title).toEqual(expected);
+
+			if (expectedTitles.length === 0) {
+				sub.unsubscribe();
+				done();
+			}
+		});


We're using this observeQuery test pattern enough that it might warrant a little test util function.

I am not at all asking for it in this PR. But, maybe the next time someone touches it, we consider that small refactor first. A util may just end up obfuscating the tests. 🤷

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

codecov-commenter · 2023-03-22T20:15:51Z

Codecov Report

Merging #11119 (addb879) into main (c78cf70) will increase coverage by 0.04%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #11119      +/-   ##
==========================================
+ Coverage   82.18%   82.22%   +0.04%     
==========================================
  Files         204      204              
  Lines       19372    19377       +5     
  Branches     4204     4204              
==========================================
+ Hits        15920    15932      +12     
+ Misses       3166     3159       -7     
  Partials      286      286

Impacted Files	Coverage Δ
...ges/datastore/__tests__/helpers/schemas/default.ts	`100.00% <ø> (ø)`
packages/datastore/__tests__/commonAdapterTests.ts	`98.26% <100.00%> (ø)`
...atastore/__tests__/helpers/fakes/graphqlService.ts	`82.62% <100.00%> (+0.33%)`	⬆️
packages/datastore/src/datastore/datastore.ts	`89.42% <100.00%> (+0.62%)`	⬆️
packages/datastore/src/util.ts	`95.97% <100.00%> (+0.06%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

svidgen

Thanks!

svidgen · 2023-03-22T20:31:28Z

packages/datastore/__tests__/DataStore.ts

-				jest.advanceTimersByTime(2000);
+				await pause(2000);


Thanks for fixing this!

svidgen · 2023-03-22T20:31:33Z

packages/datastore/__tests__/DataStore.ts

-				jest.advanceTimersByTime(2000);
+				await pause(2000);


svidgen · 2023-03-22T20:31:43Z

packages/datastore/__tests__/DataStore.ts

+	beforeEach(() => {
+		warpTime();
+	});
+
+	afterEach(() => {
+		unwarpTime();
+	});


* feat(cognito): remove required domain param when create CookieStorage * test: enable integ test * test(cognito): update unit test * Revert "test: enable integ test" This reverts commit e3db965. * feat(cognito): make cookiestorage constructor parameter optional * chore: Docs updates (aws-amplify#11082) * chore(release): Publish [ci skip] - @aws-amplify/analytics@6.0.19 - @aws-amplify/api-graphql@3.1.7 - @aws-amplify/api-rest@3.0.19 - @aws-amplify/api@5.0.19 - @aws-amplify/auth@5.1.13 - aws-amplify@5.0.19 - @aws-amplify/cache@5.0.19 - @aws-amplify/core@5.1.2 - @aws-amplify/datastore-storage-adapter@2.0.19 - @aws-amplify/datastore@4.1.1 - @aws-amplify/geo@2.0.19 - @aws-amplify/interactions@5.0.19 - @aws-amplify/notifications@1.0.19 - @aws-amplify/predictions@5.0.19 - @aws-amplify/pubsub@5.1.2 - @aws-amplify/pushnotification@5.0.19 - @aws-amplify/storage@5.1.9 - @aws-amplify/xr@4.0.19 * chore(release): update version.ts [ci skip] * refactor(datastore): storage adapters (aws-amplify#11073) * chore: Docs updates (aws-amplify#11091) * chore: Tweaked `main` pre-id. (aws-amplify#11093) * chore(deps): bump activesupport from 6.1.7.2 to 7.0.4.3 in /docs Bumps [activesupport](https://github.com/rails/rails) from 6.1.7.2 to 7.0.4.3. - [Release notes](https://github.com/rails/rails/releases) - [Changelog](https://github.com/rails/rails/blob/v7.0.4.3/activesupport/CHANGELOG.md) - [Commits](rails/rails@v6.1.7.2...v7.0.4.3) --- updated-dependencies: - dependency-name: activesupport dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * chore(release): Publish [ci skip] - @aws-amplify/analytics@6.0.20 - @aws-amplify/api-graphql@3.1.8 - @aws-amplify/api-rest@3.0.20 - @aws-amplify/api@5.0.20 - @aws-amplify/auth@5.1.14 - aws-amplify@5.0.20 - @aws-amplify/cache@5.0.20 - @aws-amplify/core@5.1.3 - @aws-amplify/datastore-storage-adapter@2.0.20 - @aws-amplify/datastore@4.1.2 - @aws-amplify/geo@2.0.20 - @aws-amplify/interactions@5.0.20 - @aws-amplify/notifications@1.0.20 - @aws-amplify/predictions@5.0.20 - @aws-amplify/pubsub@5.1.3 - @aws-amplify/pushnotification@5.0.20 - @aws-amplify/storage@5.1.10 - @aws-amplify/xr@4.0.20 * chore(release): update version.ts [ci skip] * chore(@aws-amplify/datastore): initial decomposition of test helpers (aws-amplify#11096) * moved test helpers to folder * first pass, rough decomposition of test helpers file * added readme for helpers * test helpers readme typo * Update packages/datastore/__tests__/helpers/README.md Co-authored-by: Ivan Artemiev <29709626+iartemiev@users.noreply.github.com> --------- Co-authored-by: Ivan Artemiev <29709626+iartemiev@users.noreply.github.com> * fix: add warning message when trying to use SQLite with CPK enabled (aws-amplify#11027) * fix(data): update SQLite utils to use 'instr' instead of 'like' when constructing WHERE conditions from predicates (aws-amplify#11108) * chore: Docs updates (aws-amplify#11111) * docs(cognito): excluding subdomains when domain cookie attr is omitted * chore(release): Publish [ci skip] - @aws-amplify/analytics@6.0.21 - @aws-amplify/api-graphql@3.1.9 - @aws-amplify/api-rest@3.0.21 - @aws-amplify/api@5.0.21 - @aws-amplify/auth@5.1.15 - aws-amplify@5.0.21 - @aws-amplify/cache@5.0.21 - @aws-amplify/core@5.1.4 - @aws-amplify/datastore-storage-adapter@2.0.21 - @aws-amplify/datastore@4.1.3 - @aws-amplify/geo@2.0.21 - @aws-amplify/interactions@5.0.21 - @aws-amplify/notifications@1.0.21 - @aws-amplify/predictions@5.0.21 - @aws-amplify/pubsub@5.1.4 - @aws-amplify/pushnotification@5.0.21 - @aws-amplify/storage@5.1.11 - @aws-amplify/xr@4.0.21 * chore(release): update version.ts [ci skip] * test: increase coverage for delete connection by model field (aws-amplify#11099) * Update config.yml Removes `Usage Question` link that sends users to an old 404 page for discussions * fix(datastore): stale observeQuery snapshot with sort param (aws-amplify#11119) * chore(notifications): Integrate refactored Android common utils * chore: Docs updates * Update config.yml Switched usage question to point at discord rather than deleting it * chore(release): Publish [ci skip] - amazon-cognito-identity-js@6.2.0 - @aws-amplify/analytics@6.0.22 - @aws-amplify/api-graphql@3.1.10 - @aws-amplify/api-rest@3.0.22 - @aws-amplify/api@5.0.22 - @aws-amplify/auth@5.2.0 - aws-amplify@5.0.22 - @aws-amplify/cache@5.0.22 - @aws-amplify/core@5.1.5 - @aws-amplify/datastore-storage-adapter@2.0.22 - @aws-amplify/datastore@4.1.4 - @aws-amplify/geo@2.0.22 - @aws-amplify/interactions@5.0.22 - @aws-amplify/notifications@1.0.22 - @aws-amplify/predictions@5.0.22 - @aws-amplify/pubsub@5.1.5 - @aws-amplify/pushnotification@5.0.22 - @aws-amplify/storage@5.1.12 - @aws-amplify/xr@4.0.22 * chore(release): update version.ts [ci skip] * fix(@aws-amplify/datastore): mutation hub event drops during reconnect (aws-amplify#11132) * fix(@aws-amplify/datastore): fixes mutation hub event dropped during races with reconnect * add explicit observer completion, expanded test coverage * fix(@aws-amplify/datastore): adds serialization for empty predicates (aws-amplify#11133) adds serialization for empty predicates * test(notifications): Add native Android unit tests (aws-amplify#11139) * test(notifications): Add native Android unit tests * Disable android tests from being automatically run in CI --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Allan Zheng <zheallan@amazon.com> Co-authored-by: aws-amplify-bot <aws@amazon.com> Co-authored-by: Ivan Artemiev <29709626+iartemiev@users.noreply.github.com> Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jon Wire <iambipedal@gmail.com> Co-authored-by: Dane Pilcher <dppilche@amazon.com> Co-authored-by: David McAfee <mcafd@amazon.com> Co-authored-by: Olya Balashova <42189299+helgabalashova@users.noreply.github.com> Co-authored-by: Bannon Tanner <bannonta@amazon.com> Co-authored-by: helgabalashova <helga.stolyarova@gmail.com> Co-authored-by: Aaron S <94858815+stocaaro@users.noreply.github.com>

iartemiev requested a review from a team as a code owner March 22, 2023 19:03

manueliglesias previously approved these changes Mar 22, 2023

View reviewed changes

packages/datastore/__tests__/DataStore.ts Outdated Show resolved Hide resolved

iartemiev dismissed manueliglesias’s stale review via acb0abd March 22, 2023 19:30

iartemiev force-pushed the fix-issue-11101 branch 2 times, most recently from acb0abd to 5c9cfa0 Compare March 22, 2023 19:36

david-mcafee previously approved these changes Mar 22, 2023

View reviewed changes

svidgen previously approved these changes Mar 22, 2023

View reviewed changes

iartemiev dismissed stale reviews from svidgen and david-mcafee via addb879 March 22, 2023 20:02

iartemiev force-pushed the fix-issue-11101 branch from 5c9cfa0 to addb879 Compare March 22, 2023 20:02

iartemiev force-pushed the fix-issue-11101 branch from addb879 to 7e7c8a4 Compare March 22, 2023 20:21

fix(datastore): stale observeQuery snapshot with sort param

3ba3004

iartemiev force-pushed the fix-issue-11101 branch from 7e7c8a4 to 3ba3004 Compare March 22, 2023 20:27

svidgen approved these changes Mar 22, 2023

View reviewed changes

manueliglesias approved these changes Mar 22, 2023

View reviewed changes

iartemiev merged commit 4e3b8fc into aws-amplify:main Mar 22, 2023

iartemiev deleted the fix-issue-11101 branch March 22, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datastore): stale observeQuery snapshot with sort param #11119

fix(datastore): stale observeQuery snapshot with sort param #11119

iartemiev commented Mar 22, 2023 •

edited

manueliglesias left a comment

david-mcafee left a comment

svidgen left a comment

svidgen Mar 22, 2023

codecov-commenter commented Mar 22, 2023

svidgen left a comment

svidgen Mar 22, 2023

svidgen Mar 22, 2023

svidgen Mar 22, 2023

fix(datastore): stale observeQuery snapshot with sort param #11119

fix(datastore): stale observeQuery snapshot with sort param #11119

Conversation

iartemiev commented Mar 22, 2023 • edited

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

manueliglesias left a comment

Choose a reason for hiding this comment

david-mcafee left a comment

Choose a reason for hiding this comment

svidgen left a comment

Choose a reason for hiding this comment

svidgen Mar 22, 2023

Choose a reason for hiding this comment

codecov-commenter commented Mar 22, 2023

Codecov Report

svidgen left a comment

Choose a reason for hiding this comment

svidgen Mar 22, 2023

Choose a reason for hiding this comment

svidgen Mar 22, 2023

Choose a reason for hiding this comment

svidgen Mar 22, 2023

Choose a reason for hiding this comment

iartemiev commented Mar 22, 2023 •

edited