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

fix(GraphQL): fix bug in custom resolver, now body need not have all the fields. #6054

Merged
merged 14 commits into from
Jul 29, 2020

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Jul 22, 2020

This PR fix bug in custom resolver, now body need not have all the fields(both nullable and non-nullable).

https://discuss.dgraph.io/t/evaluation-of-custom-field-failed-while-substituting-variables-into-body-for-remote-endpoint-with-an-error/8700


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jul 22, 2020
Copy link

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


graphql/schema/wrappers.go, line 1850 at r1 (raw file):

	val, ok := variables[key[1:]]
	if !ok {
		return nil, nil, ok

Can you do nil, nil, false? It will be clearer when we are reading the code.
and val, nil, true below
Also, can you check what is the default value returned by map for interface{} if the element is not present?
If it's nil then you can just have a single line. return val, nil, ok


graphql/schema/wrappers.go, line 1877 at r1 (raw file):

			if ok {
				object[k] = vval

Extra line


graphql/schema/wrappers_test.go, line 427 at r1 (raw file):

		},
		{
			"skip variable that is not  present and shouldn't return error ",

Extra space. Probably rename it to "Skip missing variable in the HTTP body"

Copy link
Contributor Author

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


graphql/schema/wrappers.go, line 1850 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Can you do nil, nil, false? It will be clearer when we are reading the code.
and val, nil, true below
Also, can you check what is the default value returned by map for interface{} if the element is not present?
If it's nil then you can just have a single line. return val, nil, ok

yeah we map return nil for interface{} if element not present. Changed to return val, nil, ok.


graphql/schema/wrappers.go, line 1877 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Extra line

removed.


graphql/schema/wrappers_test.go, line 427 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Extra space. Probably rename it to "Skip missing variable in the HTTP body"

renamed.

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: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)


graphql/schema/wrappers_test.go, line 430 at r2 (raw file):

			map[string]interface{}{"postID": "0x9"},
			map[string]interface{}{"author": "$id", "post": map[string]interface{}{"id": "$postID"}},
			map[string]interface{}{"author": "$id", "post": map[string]interface{}{"id": "0x9"}},

Doesn't look like it was skipped though. The expected map should not have had the "author": "$id" key value pair.

@MichaelJCompton
Copy link
Contributor

It does look like the test isn't quite right. What I expect is that if the author's id is missing in the variables, then "author" shouldn't get serialized into the result body at all.

Copy link
Contributor Author

@JatinDev543 JatinDev543 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, 4 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, and @pawanrawal)


graphql/schema/wrappers_test.go, line 430 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Doesn't look like it was skipped though. The expected map should not have had the "author": "$id" key value pair.

fixed it.

@JatinDev543
Copy link
Contributor Author

It does look like the test isn't quite right. What I expect is that if the author's id is missing in the variables, then "author" shouldn't get serialized into the result body at all.

fixed it.

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.

Can you verify that optional arguments in custom query/mutation are either sent as null when supplied null or not sent if they are not given?

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @arijitAD, @jatindevdg, and @MichaelJCompton)


graphql/e2e/custom_logic/custom_logic_test.go, line 1023 at r3 (raw file):

			@custom(
				http: {
					url: "http://mock:8888/userName"

Add a new endpoint to the mock server called /userNameWithoutAddress. In that verify that the body should not contain address.


graphql/schema/wrappers.go, line 1899 at r3 (raw file):

				return err
			}
			//if ok {

Remove this

@JatinDev543
Copy link
Contributor Author

JatinDev543 commented Jul 28, 2020

Can you verify that optional arguments in custom query/mutation are either sent as null when supplied null or not sent if they are not given?

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @arijitAD, @jatindevdg, and @MichaelJCompton)

graphql/e2e/custom_logic/custom_logic_test.go, line 1023 at r3 (raw file):

			@custom(
				http: {
					url: "http://mock:8888/userName"

Add a new endpoint to the mock server called /userNameWithoutAddress. In that verify that the body should not contain address.

added test.

graphql/schema/wrappers.go, line 1899 at r3 (raw file):

				return err
			}
			//if ok {

Remove this

removed.

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: is master merged into this branch? CI seems to be failing

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)


graphql/e2e/custom_logic/cmd/main.go, line 1213 at r4 (raw file):

	http.HandleFunc("/favMovies/", getFavMoviesHandler)
	http.HandleFunc("/favMoviesPost/", postFavMoviesHandler)
	http.HandleFunc("/favMoviesPostWithBody/", PostFavMoviesWithBodyHandler)

This doesn't have to start with capital P.

@JatinDev543 JatinDev543 merged commit 72e6a47 into master Jul 29, 2020
@JatinDev543 JatinDev543 deleted the jatin/graphql-582 branch July 29, 2020 06:36
JatinDev543 added a commit that referenced this pull request Aug 12, 2020
…the fields. (#6054)

This PR fix bug in custom resolver, now body need not have all the fields(both nullable and non-nullable).

(cherry picked from commit 72e6a47)
JatinDev543 added a commit that referenced this pull request Aug 13, 2020
…the fields. (#6054) (#6166)

This PR fix bug in custom resolver, now body need not have all the fields(both nullable and non-nullable).

(cherry picked from commit 72e6a47)
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.

4 participants