-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Codecov Report
📣 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8960f62
to
7b45c5a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c28e40d
to
ac6e1de
Compare
8f4881f
to
4ad4899
Compare
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". |
There was a problem hiding this 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.
74c9913
to
a5b8ced
Compare
There was a problem hiding this 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.
e3baa9a
to
a5f26ab
Compare
There was a problem hiding this 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.
Co-authored-by: Jon Wire <iambipedal@gmail.com>
c8d75fc
to
7f9b75a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.