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: Return better error message if a type only contains ID field #5531

Merged
merged 4 commits into from
May 29, 2020

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented May 28, 2020

Fixes GRAPHQL-449


This change is Reviewable

Docs Preview: Dgraph Preview

@pawanrawal pawanrawal marked this pull request as ready for review May 28, 2020 09:56
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.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

		name: String!
	}
	type Movie {

Why did we need to change this? @remote types won't be checked by the rule right as we store them in dgraph?


graphql/schema/gqlschema_test.yml, line 2051 at r1 (raw file):

Quoted 4 lines of code…
      input AuthorUpdate {
        id: ID!
        name: String!
      }

Someone may want to identify a query input with just an ID. So, input types should still be allowed with only ID!, as we don't store them in dgraph.


graphql/schema/gqlschema_test.yml, line 2057 at r1 (raw file):

Quoted 13 lines of code…
  -
    name: "@custom directive with variable definitions in operation in graphql"
    input: |
      type Author {
        id: ID!
        age: Int!
        name: String! @custom(http: {
          url: "http://google.com/",
          method: "POST",
          graphql: "query ($id: ID!, $age: Int!) { getAuthor(id: $id, age: $age) }",
          skipIntrospection: true
        })
      }

Why did we remove this?

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: all files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

Why did we need to change this? @remote types won't be checked by the rule right as we store them in dgraph?

sorry, read that line as: Why did we need to change this? @Remote types won't be checked by the rule right as we don't store them in dgraph?

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.

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur and @MichaelJCompton)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

sorry, read that line as: Why did we need to change this? @Remote types won't be checked by the rule right as we don't store them in dgraph?

Actually this seemed a bit weird that a @remote type has custom fields. We don't resolve custom fields within remote type yet. Not remote types themselves are responses of custom fields. Maybe we should even throw an error for a remote type having custom fields for now?


graphql/schema/gqlschema_test.yml, line 2051 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
      input AuthorUpdate {
        id: ID!
        name: String!
      }

Someone may want to identify a query input with just an ID. So, input types should still be allowed with only ID!, as we don't store them in dgraph.

Ok


graphql/schema/gqlschema_test.yml, line 2057 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
  -
    name: "@custom directive with variable definitions in operation in graphql"
    input: |
      type Author {
        id: ID!
        age: Int!
        name: String! @custom(http: {
          url: "http://google.com/",
          method: "POST",
          graphql: "query ($id: ID!, $age: Int!) { getAuthor(id: $id, age: $age) }",
          skipIntrospection: true
        })
      }

Why did we remove this?

Thanks for flagging, got removed because of bad merge.

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.

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Actually this seemed a bit weird that a @remote type has custom fields. We don't resolve custom fields within remote type yet. Not remote types themselves are responses of custom fields. Maybe we should even throw an error for a remote type having custom fields for now?

Yes, I think we should throw that error. It would be better.

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:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur and @pawanrawal)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

Yes, I think we should throw that error. It would be better.

Yeah, sounds right to throw an error if there's custom things in remote types until we support it. Can you add a card to jira.

@pawanrawal pawanrawal merged commit 60882e7 into master May 29, 2020
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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur and @MichaelJCompton)


graphql/e2e/custom_logic/custom_logic_test.go, line 226 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Yeah, sounds right to throw an error if there's custom things in remote types until we support it. Can you add a card to jira.

Created GRAPHQL-486

@pawanrawal pawanrawal deleted the pawanrawal/graphql-449 branch July 10, 2020 12:22
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants