-
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: remove custom directive from internal schema #5354
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 35 of 35 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur, @manishrjain, and @MichaelJCompton)
graphql/schema/gqlschema.go, line 84 at r1 (raw file):
method: HTTPMethod! body: String graphql: String
Even though the definition was incorrect, schema validation didn't fail? Is it again because schema extras are added later after the actual validation?
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: all files reviewed, 1 unresolved discussion (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema.go, line 84 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Even though the definition was incorrect, schema validation didn't fail? Is it again because schema extras are added later after the actual validation?
Yes
…m-directive # Conflicts: # graphql/schema/gqlschema.go # graphql/schema/testdata/schemagen/output/comments-and-descriptions.graphql # graphql/schema/testdata/schemagen/output/custom-mutation.graphql # graphql/schema/testdata/schemagen/output/custom-nested-types.graphql # graphql/schema/testdata/schemagen/output/custom-query-mixed-types.graphql # graphql/schema/testdata/schemagen/output/custom-query-not-dgraph-type.graphql # graphql/schema/testdata/schemagen/output/custom-query-with-dgraph-type.graphql # graphql/schema/testdata/schemagen/output/deprecated.graphql # graphql/schema/testdata/schemagen/output/dgraph-reverse-directive-on-concrete-type-with-interfaces.graphql # graphql/schema/testdata/schemagen/output/dgraph-reverse-directive-with-interfaces.graphql # graphql/schema/testdata/schemagen/output/field-with-id-directive.graphql # graphql/schema/testdata/schemagen/output/field-with-reverse-predicate-in-dgraph-directive.graphql # graphql/schema/testdata/schemagen/output/hasInverse-with-interface-having-directive.graphql # graphql/schema/testdata/schemagen/output/hasInverse-with-interface.graphql # graphql/schema/testdata/schemagen/output/hasInverse-with-type-having-directive.graphql # graphql/schema/testdata/schemagen/output/hasInverse.graphql # graphql/schema/testdata/schemagen/output/ignore-unsupported-directive.graphql # graphql/schema/testdata/schemagen/output/interface-with-id-directive.graphql # graphql/schema/testdata/schemagen/output/interface-with-no-ids.graphql # graphql/schema/testdata/schemagen/output/interfaces-with-types-and-password.graphql # graphql/schema/testdata/schemagen/output/interfaces-with-types.graphql # graphql/schema/testdata/schemagen/output/no-id-field-with-searchables.graphql # graphql/schema/testdata/schemagen/output/no-id-field.graphql # graphql/schema/testdata/schemagen/output/password-type.graphql # graphql/schema/testdata/schemagen/output/searchables-references.graphql # graphql/schema/testdata/schemagen/output/searchables.graphql # graphql/schema/testdata/schemagen/output/single-type-with-enum.graphql # graphql/schema/testdata/schemagen/output/single-type.graphql # graphql/schema/testdata/schemagen/output/type-implements-multiple-interfaces.graphql # graphql/schema/testdata/schemagen/output/type-reference.graphql # graphql/schema/testdata/schemagen/output/type-with-custom-field-on-dgraph-type.graphql # graphql/schema/testdata/schemagen/output/type-with-custom-fields-on-remote-type.graphql # graphql/schema/wrappers.go
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.
...we can't have a test that shows that it's not in the output schema right? because you can't actually query for that in graphql schema introspection?
Reviewable status: 1 of 36 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema.go, line 84 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Yes
I actually wonder is it because gqlparser doesn't check directive inputs properly, we've bumped into that before.
graphql/schema/wrappers.go, line 205 at r2 (raw file):
// something like field.Directives.ForName("custom"), which results in iterating over all the // directives of the field. customDirectives map[string]map[string]*ast.Directive
so it includes Query and Mutation?
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.
Yes
Reviewable status: 1 of 36 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/wrappers.go, line 205 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
so it includes Query and Mutation?
Yes
) This PR removes custom directive from internal schema and stores it in a separate mapping. @Custom was never exposed externally through introspection, as GraphQL spec doesn't specify a way to query directives for a field in introspection. Also, as getGQLSchema resolver in /admin will be protected with Guardian auth, getGQLSchema will still have the @Custom directive in its outputs for both schema and generatedSchema. This PR also updates schemaExtras to reflect the current definition for @Custom. Fixes #GRAPHQL-347.
This PR removes custom directive from internal schema and stores it in a separate mapping.
@custom
was never exposed externally through introspection, as GraphQL spec doesn't specify a way to query directives for a field in introspection. Also, asgetGQLSchema
resolver in/admin
will be protected with Guardian auth,getGQLSchema
will still have the@custom
directive in its outputs for bothschema
andgeneratedSchema
.This PR also updates
schemaExtras
to reflect the current definition for@custom
.Fixes #GRAPHQL-347
This change is
Docs Preview: