-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(GraphQL): Support using custom DQL with @groupby #7476
Conversation
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.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)
graphql/schema/rules.go, line 2122 at r1 (raw file):
"Type %s; Argument %s inside @remoteResponse directive must be defined."
also mention the field in the error message
graphql/schema/wrappers.go, line 923 at r1 (raw file):
fd := f.field.ObjectDefinition.Fields.ForName(f.Name()) remoteResponseName := remoteResponseDirectiveArgument(fd)
The ForName()
lookups here may turn out to be expensive.
Let's store the remoteResponse
names in our schema wrapper like we store dgraphPredicate
, customDirectives
, lambdaDirectives
mapping there and then remove the directive from the field definition once it has been added to the mapping.
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.
Reviewed 59 of 59 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)
Fixes GRAPHQL-754.
This PR adds support for the @remoteResponse directive in the GraphQL schema. Users can define this directive on the fields of a @Remote type in order to map the response JSON key of a custom query to a GraphQL field.
For example, for the given GraphQL schema:-
the below @Custom DQL query is made possible:-
This change is