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: Recover from panic within goroutines used for resolving custom fields. #5329

Merged
merged 5 commits into from
Apr 30, 2020

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Apr 29, 2020

We recover from any panics encountered within go-routines used for resolving custom fields and return the error back to the client. @MichaelJCompton couldn't find an easy way to add a test for this. I did test this by adding a panic to makeRequest and then running an e2e to verify all errors are returned properly. Resolves GRAPHQL-409.


This change is Reviewable

@pawanrawal pawanrawal requested review from manishrjain and a team as code owners April 29, 2020 14:35
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:

I also don't see an easy way to test with the way it's structured at the moment. However, we test the pattern pretty nicely elsewhere, so if it did turn out to be a problem here we could restructure.

One thing that has dropped off the list is that we aren't adding the GraphQL error paths in a few places now. I've added a ticket to backlog to see if we can add those where missing.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)


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

func internalServerError(err error, f schema.Field) *x.GqlError {
	return x.GqlErrorf("Evaluation of custom field failed because of an error: %s for field: %s"+

should we be wrapping the original error here - GQLWrapLocationf ? So it'll say it like

"evaluation of custom field f on type T failed because ..."

@pawanrawal pawanrawal merged commit 9055c8f into master Apr 30, 2020
@pawanrawal pawanrawal deleted the pawanrawal/graphql-409 branch April 30, 2020 15:14
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…om fields. (dgraph-io#5329)

We recover from any panics encountered within go-routines used for resolving custom fields and return the error back to the client. Resolves GRAPHQL-409.
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.

2 participants