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

GraphQLRequest builders default walk depth different in Android #681

Closed
lawmicha opened this issue Jul 30, 2020 · 5 comments
Closed

GraphQLRequest builders default walk depth different in Android #681

lawmicha opened this issue Jul 30, 2020 · 5 comments
Labels
api Issues related to the API category pending-triage Issue is pending triage

Comments

@lawmicha
Copy link
Member

lawmicha commented Jul 30, 2020

Description
Currently when using DataStore, it will use API with the Model-to-GraphQLRequest builders to construct a valid graphql document. API can also be used directly with the builders. The current behavior is to generate only the first level of fields for of the selection set (depth of 1). When there is a connection to the model, if it is required, then it will also generate the required associated fields. When it is optional, the fields are not generated.

Issue created from comment
On Android, we have had one similar request to make selection set depth configurable. Currently, we have this hardcoded to 2 for API and 1 for DataStore, but since the CLI asks for this as an input, perhaps we could use that value instead.

Originally posted by @richardmcclellan in aws-amplify/docs#2141 (comment)

@lawmicha
Copy link
Member Author

lawmicha commented Aug 7, 2020

discussed with team about this behavior

  • iOS currently sets walk-depth to 1 to avoid apps loading large amounts of associated data. Technically this is limited to 1000 since it does not exhaust the results of the comments (it has a nextToken as well that can be used)
  • discuss with Android for alignment on direction going foward such as which walk-depth should we default to, 1 or 2? iOS needs to bump to 2 or Android needs to reduce to 1.
  • expose a way to make it configurable, which opens up the door for some CLI input to be passed to the configurability.
  • expose a way to make it configurable, but reduced to single bool flag (like includeComments, which means it's a toggle between a walk-depth of 1 or 2.
  • build similar lazy loading functionality like what we have in DataStore to get the associated data (get post then access it's comments with lazy loading)

@lawmicha
Copy link
Member Author

lawmicha commented Aug 7, 2020

for now, regardless of walk depth, we should have support for querying the list of comments with a filter on post.id. Currently I don't think this is possible from DataStore or APi perspective because the coding key used like post.id = id does not correspond to postId that is the actual input to the API.

@drochetti
Copy link
Contributor

for now, regardless of walk depth, we should have support for querying the list of comments with a filter on post.id. Currently I don't think this is possible from DataStore or APi perspective because the coding key used like post.id = id does not correspond to postId that is the actual input to the API.

In DataStore this should be possible, but there's a bug in the QueryPredicate: #512

A workaround is to use field("commentPostId") instead of Comment.keys.post.


As for the Amplify.API I would be interested to understand what the limitation is and if there's something we can do do fix it.

@lawmicha lawmicha modified the milestone: 2.0.0 Jan 22, 2021
@lawmicha
Copy link
Member Author

iOS resolves this by only having a walk depth of 1 and allowing the developer to perform lazy loading on the second level: #1009

In terms of the limitation on the API, it is still there for older @connection directive usages where the API provisioned is missing a way to query a list child model by it's associated parent model. for @connection usages with the key and field then it is possible. I think the "fix" is already introduced from the new usage of @connection directive, and thus we should not document ways to provision using the older @connection directive if it is in a deprecation path

@lawmicha
Copy link
Member Author

lawmicha commented Jan 22, 2021

Will close this now as this is becoming outdated with lazy loading feature coming in for API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API category pending-triage Issue is pending triage
Projects
None yet
Development

No branches or pull requests

2 participants