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

feat(datastore): add support for AppSync RTF #11000

Merged
merged 10 commits into from
Mar 7, 2023
Merged

feat(datastore): add support for AppSync RTF #11000

merged 10 commits into from
Mar 7, 2023

Conversation

iartemiev
Copy link
Member

@iartemiev iartemiev commented Feb 20, 2023

Description of changes

Add support for AppSync Enhanced Subscription Filtering (aka Runtime Filtering / RTF)

Adds compatibility with dynamic auth on list fields, e.g. "list of owners", "list of groups".

Allows DataStore selective sync predicates to be applied as a backend subscription filter, instead of receiving all authorized subscription messages and filtering them clientside as was done before this PR.

RTF has several service constraints (listed here), therefore the current clientside filtering behavior will remain as a fallback in case the service rejects the provided filter expression.

In order to keep this change non-breaking, if DataStore receives an RTF-specific error message from the service at connect time, it will retry the subscription without a filter argument, falling back to filtering the subscription messages in the client.

Additional fix
The selective sync -> DDB Query use case is currently broken in main, because the filter expression we send as a sync query param has a bunch of redundant and groups nested and the generated sync query request resolver isn't able to match the filter expression to a query expression. This PR flattens the resulting GQL filter to restore this use case (see predicateToGraphqlFilter)

Issue #, if available

#7534

Description of how you validated changes

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.

@iartemiev iartemiev requested review from a team as code owners February 20, 2023 13:47
@iartemiev iartemiev removed the request for review from a team February 20, 2023 14:08
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2023

Codecov Report

Merging #11000 (7f9b75a) into main (b7f35a4) will decrease coverage by 0.10%.
The diff coverage is 71.69%.

📣 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   #11000      +/-   ##
==========================================
- Coverage   82.24%   82.15%   -0.10%     
==========================================
  Files         193      193              
  Lines       19367    19507     +140     
  Branches     4185     4228      +43     
==========================================
+ Hits        15928    16025      +97     
- Misses       3151     3194      +43     
  Partials      288      288              
Impacted Files Coverage Δ
packages/datastore/src/datastore/datastore.ts 88.39% <ø> (ø)
packages/datastore/src/types.ts 86.47% <ø> (ø)
...ages/datastore/src/sync/processors/subscription.ts 80.95% <50.00%> (-5.21%) ⬇️
packages/datastore/src/sync/utils.ts 89.16% <77.41%> (-4.72%) ⬇️
packages/datastore/__tests__/helpers.ts 84.17% <100.00%> (ø)

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

@iartemiev iartemiev force-pushed the ds-sub-filtering branch 2 times, most recently from 8960f62 to 7b45c5a Compare February 20, 2023 17:40
@undefobj

This comment was marked as resolved.

@iartemiev iartemiev marked this pull request as draft February 20, 2023 19:54
@svidgen svidgen added the DataStore Related to DataStore category label Feb 20, 2023
@iartemiev iartemiev force-pushed the ds-sub-filtering branch 2 times, most recently from c28e40d to ac6e1de Compare March 2, 2023 17:39
@iartemiev iartemiev marked this pull request as ready for review March 2, 2023 17:39
@iartemiev
Copy link
Member Author

FYI - tried to use "hide as resolved" for Richard's comment above, because I've updated the PR with the change he requested, but GH is just showing it as "hidden".

Copy link
Member

@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.

Might need another offline sync on this. I think I understood the balance we wanted to strike between client-side helpfulness + validation differently.

@iartemiev iartemiev force-pushed the ds-sub-filtering branch 7 times, most recently from 74c9913 to a5b8ced Compare March 6, 2023 14:51
Copy link
Member

@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.

Sorry, I guess I lied about being just about ready to approve. A few suggestions/requests. I think the only blocker for me is the typo.

@iartemiev iartemiev requested a review from svidgen March 6, 2023 18:07
svidgen
svidgen previously approved these changes Mar 6, 2023
Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

Looks great! I have some questions and a single change I would like to see regarding deeply nested code.

Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@iartemiev iartemiev merged commit a050c1a into main Mar 7, 2023
@iartemiev iartemiev deleted the ds-sub-filtering branch March 7, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataStore Related to DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants