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: remove custom directive from internal schema #5354

Merged
merged 4 commits into from
May 6, 2020

Conversation

abhimanyusinghgaur
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur commented May 4, 2020

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 change is Reviewable

Docs Preview: Dgraph Preview

@abhimanyusinghgaur abhimanyusinghgaur added the area/graphql Issues related to GraphQL support on Dgraph. label May 4, 2020
@abhimanyusinghgaur abhimanyusinghgaur marked this pull request as ready for review May 4, 2020 05:25
Copy link
Contributor

@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.

:lgtm_strong: nice and simple change!

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?

Copy link
Contributor Author

@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, 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
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:

...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?

Copy link
Contributor Author

@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.

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

@abhimanyusinghgaur abhimanyusinghgaur merged commit e2fa461 into master May 6, 2020
@abhimanyusinghgaur abhimanyusinghgaur deleted the strip-custom-directive branch May 6, 2020 12:32
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
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.
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.

None yet

3 participants