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

graphql: Allow user to define and pass arguments to fields. #5562

Merged
merged 8 commits into from
Jun 10, 2020

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Jun 2, 2020

This would allow users to pass in arguments to fields of remote endpoints.

Fixes GRAPHQL-366


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jun 2, 2020
@pawanrawal pawanrawal marked this pull request as ready for review June 4, 2020 07:22
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a 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?

Copy link
Contributor Author

@pawanrawal pawanrawal left a 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.

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MichaelJCompton)

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

:lgtm:

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.

@pawanrawal pawanrawal merged commit b29a977 into master Jun 10, 2020
@pawanrawal pawanrawal deleted the pawanrawal/graphql-366 branch June 10, 2020 12:31
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…o#5562)

This would allow users to pass in arguments to fields of remote endpoints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants