-
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
graphql: Allow user to define and pass arguments to fields. #5562
Conversation
…o the generated schema.
af4754f
to
59be150
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.
Mostly looks good to me.
Just got a few queries.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)
graphql/schema/custom_http_config_test.yaml, line 77 at r2 (raw file):
Quoted 6 lines of code…
query { getCountry1(id: "0x1") { name code(first: 10, filter: {ids: "0x123", name: { eq: "github" }}) } }
will this work too:
query ($first: Int) {
getCountry1(id: "0x1") {
name
code(first: $first, filter: {ids: "0x123", name: { eq: "github" }})
}
}
with $first: 10
?
graphql/schema/rules.go, line 635 at r2 (raw file):
} func fieldArgumentCheck(typ *ast.Definition, field *ast.FieldDefinition) *gqlerror.Error {
For local types, the field arguments would be used only in case if the local type is being used with @custom
, the field arguments can't be used with dgraph, right?
should we add a validation for this case too?
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 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur and @MichaelJCompton)
graphql/schema/custom_http_config_test.yaml, line 77 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
query { getCountry1(id: "0x1") { name code(first: 10, filter: {ids: "0x123", name: { eq: "github" }}) } }
will this work too:
query ($first: Int) { getCountry1(id: "0x1") { name code(first: $first, filter: {ids: "0x123", name: { eq: "github" }}) } }
with
$first: 10
?
Good point, this doesn't work right now for remote queries. I'll file this as a JIRA issue and handle separately. It requires propogating the variables defined in the original query and writing them to the remote query def along the variable values themselves.
graphql/schema/rules.go, line 635 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
For local types, the field arguments would be used only in case if the local type is being used with
@custom
, the field arguments can't be used with dgraph, right?
should we add a validation for this case too?
Yeah, that is right for Dgraph types the arguments won't do anything if they are being used for Dgraph query. It is hard to add validation for that case because while validating a field within a type I don't really know if the type is being used as a return type for a custom query/field.
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MichaelJCompton)
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.
Add an e2e test with args.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pawanrawal)
graphql/schema/wrappers.go, line 1959 at r2 (raw file):
writer.WriteString("{\n") for i := 0; i < len(field.SelectionSet); i++ { // TODO - Check if this type conversion is safe.
I think fragments etc should be resolved to fields here ... so just remove this
graphql/schema/testdata/schemagen/input/type-with-arguments-on-field.graphql, line 3 at r2 (raw file):
interface Abstract { id: ID! name(random: Int!, size: String): String!
Looks a bit strange, because this won't do anything, but I can see that it allows custom logic here.
…o#5562) This would allow users to pass in arguments to fields of remote endpoints.
This would allow users to pass in arguments to fields of remote endpoints.
Fixes GRAPHQL-366
This change is
Docs Preview: