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

Feat(GraphQL): Add generate directive to graphql schema #6760

Merged
merged 4 commits into from
Oct 28, 2020

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Oct 19, 2020

Motivation:
Currently, GraphQL schema exposes all queries (get, query) and mutations (add, update, delete). It has been requested to resolve only some of these resolvers by Users.
This PR adds this feature of adding only specific resolvers to GraphQL schema.
Additionally, one can also control the subscription query using generate directive.
Related Discuss Issue: https://discuss.dgraph.io/t/expose-only-specific-resolvers/10497/12

Testing:
Added tests in graphql/schema/testdata/schemagen

Fixes GRAPHQL-710


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added area/enterprise Related to proprietary features area/graphql Issues related to GraphQL support on Dgraph. labels Oct 19, 2020
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.

I have just got the following points:

  1. Let's just keep one file for testing all different bits for @generate directive in schemagen tests. We can put all the different cases in one single file using different types. That helps keep the number of those files at a minimum, otherwise, we have to update each file every time we do something with the inbuilt schema, and it just takes unnecessary time.
  2. As discussed yesterday, let's also skip adding the following things to schema:
    a. AddTypeInput and AddTypePayload if add mutation is disabled.
    b. UpdateTypeInput, TypePatch and UpdateTypePayload if update mutation is disabled.
    c. DeleteTypePayload if delete mutation is disabled.

Reviewed 7 of 59 files at r1.
Reviewable status: 7 of 59 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)


graphql/schema/gqlschema.go, line 712 at r1 (raw file):

if params.generateUpdateMutation == true {

this could just be:

if params.generateUpdateMutation {

Same for all other places like this.


graphql/schema/schemagen_test.go, line 98 at r1 (raw file):

			if diff := cmp.Diff(string(str2), newSchemaStr); diff != "" {
				// fmt.Printf("Generated Schema (%s):\n%s\n", testFile.Name(), newSchemaStr)
				_ = ioutil.WriteFile(outputFileName+"tp", []byte(newSchemaStr), 0644)

stale code

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.

Ah! forgot to mention that we should also make password query configurable using generate directive. Default behaviour is to be generated if the type has a password field.
So,

@generate(
  query: {
    ...
    password: true/false
  }
  ...
)

Reviewable status: 7 of 59 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)

Copy link
Contributor Author

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: 2 of 58 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)


graphql/schema/gqlschema.go, line 712 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
if params.generateUpdateMutation == true {

this could just be:

if params.generateUpdateMutation {

Same for all other places like this.

Done.


graphql/schema/schemagen_test.go, line 98 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

stale code

Done.

@vmrajas vmrajas force-pushed the rajas/specific_resolvers branch from 00a0b69 to ac34082 Compare October 20, 2020 19:15
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:

Nice set of tests!

There is just one schemagen test failling in CI. Also, make sure to first merge master into this branch before merging it with master and then let the CI pass.

Reviewed 61 of 65 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @vmrajas)


graphql/e2e/schema/generatedSchema.graphql, line 169 at r2 (raw file):

	get: Boolean
	query: Boolean
	password: Boolean

Hope this is ignored for types where this is not applicable?


graphql/e2e/schema/generatedSchema.graphql, line 173 at r2 (raw file):

input GenerateMutationParams {
	add: Boolean

What happens when add is given for an interface?


graphql/schema/rules.go, line 1236 at r2 (raw file):

}

func generateDirectiveValidation(schema *ast.Schema, typ *ast.Definition) gqlerror.List {

Strange, we have to do all this validation ourselves? Doesn't happen in the gqlparser library?

Copy link
Contributor Author

@vmrajas vmrajas 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 @manishrjain, @MichaelJCompton, and @pawanrawal)


graphql/e2e/schema/generatedSchema.graphql, line 169 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Hope this is ignored for types where this is not applicable?

Yes, it is ignored.


graphql/e2e/schema/generatedSchema.graphql, line 173 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What happens when add is given for an interface?

It won't generate the add mutation.


graphql/schema/rules.go, line 1236 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Strange, we have to do all this validation ourselves? Doesn't happen in the gqlparser library?

Yes, we need to do these ourselves. As is the case with other directives.

@vmrajas vmrajas force-pushed the rajas/specific_resolvers branch from ac34082 to 4d70f62 Compare October 28, 2020 12:20
@vmrajas vmrajas merged commit 633d3a8 into master Oct 28, 2020
@vmrajas vmrajas deleted the rajas/specific_resolvers branch October 28, 2020 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/enterprise Related to proprietary features area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants