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(@aws-amplify/datastore): extra fields in selective sync selection set on API's from older CLI #10949

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

svidgen
Copy link
Member

@svidgen svidgen commented Feb 8, 2023

Description of changes

Fixes an issue where the selection set for selective sync expressions was incorrect for apps built with some older versions of the CLI. Related-model key fields were included in the selection set, but were not recognized by cloud storage. This issue was introduced in with the nested predicates feature, which added the implicitly created FK fields to the table metadata fields list so they could be seen by the predicate builder.

This PR removes the logic that augments the fields list and add a new allFields list. All pre-existing logic and components (prior to lazy loading/nested predicates) will revert to use only the explicitly defined fields list. The predicates builder will then use allFields.

To support this, storage predicate construction is refactored slightly to avoid using the storage predicate (v1 predicate) builder. The old builder complains when a field from allFields shows up, even though the field exist. So, we are instead constructing an AST and transforming it.

The resulting storage predicate can, at times, contain joins against null or undefined values. This happens when a relationship is optional. So, the IndexedDBAdapter was also updated to avoid attempting a index.get() on these values, which otherwise causes IndexedDB to throw a fit.

(Refactors around storage predicate generation and related IndexedDB fixes were handled upstream.)

Issue #, if available

#10785

Description of how you validated changes

  1. Added testing.
  2. Manual sample apps validation: Red/Green'd many-to-many app with added selective syncs. That PR can be merged after this one.

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.

@svidgen svidgen requested a review from a team as a code owner February 8, 2023 23:26
@david-mcafee david-mcafee requested review from a team and david-mcafee February 9, 2023 20:33
@svidgen svidgen force-pushed the fix-selection-set-fks-isolated branch from a7aed2f to 215e8be Compare February 10, 2023 20:07
@svidgen svidgen requested review from a team as code owners February 10, 2023 20:07
@svidgen svidgen requested review from dpilch, david-mcafee and iartemiev and removed request for a team February 10, 2023 20:13
@codecov-commenter
Copy link

Codecov Report

Merging #10949 (8603e0c) into main (f6dfaac) will increase coverage by 0.02%.
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   #10949      +/-   ##
==========================================
+ Coverage   81.54%   81.57%   +0.02%     
==========================================
  Files         198      198              
  Lines       19610    19609       -1     
  Branches     4237     4227      -10     
==========================================
+ Hits        15991    15996       +5     
+ Misses       3329     3323       -6     
  Partials      290      290              
Impacted Files Coverage Δ
packages/datastore/src/storage/relationship.ts 93.67% <ø> (ø)
packages/datastore/src/types.ts 86.47% <ø> (ø)
packages/datastore/__tests__/helpers.ts 85.45% <100.00%> (+0.89%) ⬆️
packages/datastore/src/datastore/datastore.ts 88.36% <100.00%> (-0.02%) ⬇️
packages/datastore/src/predicates/next.ts 94.94% <100.00%> (ø)
...kages/amazon-cognito-identity-js/src/BigInteger.js 89.71% <0.00%> (+0.40%) ⬆️

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

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.

5 participants