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

don't allow custom fields in subscription #5324

Merged
merged 3 commits into from May 5, 2020
Merged

don't allow custom fields in subscription #5324

merged 3 commits into from May 5, 2020

Conversation

poonai
Copy link
Contributor

@poonai poonai commented Apr 29, 2020

Signed-off-by: Tiger rbalajis25@gmail.com


This change is Reviewable

Docs Preview: Dgraph Preview

Signed-off-by: Tiger <rbalajis25@gmail.com>
@poonai poonai requested review from manishrjain and a team as code owners April 29, 2020 07:57
@pawanrawal pawanrawal self-requested a review April 29, 2020 10:05
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.

Like it ... and a nice test too 👍

I think you should be digging deeper into the query though.

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @balajijinnah, @manishrjain, and @pawanrawal)


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

		tid: ID!
		age: Int!
		name: String

let's make this test go deeper and add a test for a custom query as well - once you make the changes suggested


graphql/resolve/resolver.go, line 427 at r1 (raw file):

	queries := op.Queries()
	if len(queries) == 0 {
		return errors.Errorf("Given query %s is not a valid subscription request",

I suspect that GraphQL validation has already handled this case - check it out and see. r.schema.Operation(req) should have done it.


graphql/resolve/resolver.go, line 431 at r1 (raw file):

	}

	for _, q := range op.Queries() {

I think you'll need to check the query itself as well because those can be custom


graphql/resolve/resolver.go, line 432 at r1 (raw file):

	for _, q := range op.Queries() {
		for _, field := range q.SelectionSet() {

Won't this need to go in recursively into the query? e.g. if its like

blaa {
  moreblaa {
    something
    thisOne
  }
}

and it's the thisOne field that has the custom directive. Currently looks like it only checks if it's a custom at the top level The custom fields means we have to dig all the way in.


graphql/resolve/resolver.go, line 434 at r1 (raw file):

		for _, field := range q.SelectionSet() {
			if has, _ := field.HasCustomDirective(); has {
				return errors.Errorf("Custom field `%s` is not supported in graphql subscription",

can you add the location to the error. It should be available in the field. There's some functions in schema/errors.go and in x to help constructing the errors with locations

Signed-off-by: Tiger <rbalajis25@gmail.com>
@poonai poonai changed the base branch from pawanrawal/graphql-custom-logic to master May 4, 2020 04:26
Copy link
Contributor Author

@poonai poonai 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: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @danielmai, @manishrjain, @martinmr, @MichaelJCompton, and @pawanrawal)


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

Previously, MichaelJCompton (Michael Compton) wrote…

let's make this test go deeper and add a test for a custom query as well - once you make the changes suggested

Done.


graphql/resolve/resolver.go, line 427 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

I suspect that GraphQL validation has already handled this case - check it out and see. r.schema.Operation(req) should have done it.

Done.


graphql/resolve/resolver.go, line 431 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

I think you'll need to check the query itself as well because those can be custom

Subscriptions are not generated for custom quries


graphql/resolve/resolver.go, line 432 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Won't this need to go in recursively into the query? e.g. if its like

blaa {
  moreblaa {
    something
    thisOne
  }
}

and it's the thisOne field that has the custom directive. Currently looks like it only checks if it's a custom at the top level The custom fields means we have to dig all the way in.

Done.


graphql/resolve/resolver.go, line 434 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

can you add the location to the error. It should be available in the field. There's some functions in schema/errors.go and in x to help constructing the errors with locations

Done.

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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, @martinmr, @MichaelJCompton, and @pawanrawal)

@poonai poonai merged commit 0024241 into master May 5, 2020
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
@joshua-goldstein joshua-goldstein deleted the balaji/disallow branch August 11, 2022 20:41
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.

None yet

2 participants