-
Notifications
You must be signed in to change notification settings - Fork 490
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
Properly handle nil interface resolvers #239
Conversation
Thanks for the contribution! Can we add a test case for this? |
Hello- We've ran into this issue on our local server. What kind of test case would be appropriate in order to merge this fix? |
I added a test case (as requested in #239 (comment)) in another PR based on this PR: wandb#1. @tonyghita If you'd like to merge this now, feel free to just cherry-pick my commit or whatever. Thanks! |
Prior to the fix, the new TestNilInterface test would fail with: graphql: panic occurred: reflect: Method on nil interface value
Thanks @sqs for fixing this up! @tonyghita is this good to merge now? |
graphql_test.go
Outdated
"testing" | ||
"time" | ||
|
||
"github.com/graph-gophers/graphql-go" | ||
graphqlerrors "github.com/graph-gophers/graphql-go/errors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you, please, rename this graphqlerrors
-> gqlerrors
to be shorter and to be consistent with gqltesting
?
Thank you for your contribution! |
Properly handle nil interface resolvers
fixes #233
Calling
reflect.ValueOf(nil)
returns the zeroValue
, which has typeInvalid
.Calling
reflect.ValueOf
an interface typed nil returns aValue
with typeInterface
.Both of these cases skipped the null checks and fail later on when trying to resolve fields.