Don't crash when interface resolver returns a typed nil

[nmaquet]

When implementing a resolver that returns an interface, it is convenient to be able to return a nil pointer of the underlying struct. This currently causes gqlgen's generated code to panic (nil pointer dereference).

Consider the schema:

interface Animal { name: String! }
type Dog implements Animal { name: String! age: Float }
type Query {
    animal: Animal
}

The following implementation leads to a crash:

func (r *queryResolver) Animal(ctx context.Context) (Animal, error) {
    var ptr *Dog
    return ptr, nil 
}

The fix is very simple, we have added an extra check in interface.gotpl, like so:

case *{{$implementor.Type | ref}}:
    if obj == nil {
        return graphql.Null
    }
    return ec._{{$implementor.Name}}(ctx, sel, obj)

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[coveralls]

Coverage remained the same at 62.119% when pulling fcfe595 on nmaquet:check-nil-interface-ptrs into 15b3058 on 99designs:master.

[vvakame]

typed nil is not means nil.
so, tests are succeeded with below changes without your codegen/interface.gotpl patches.

diff --git a/codegen/testserver/interfaces.go b/codegen/testserver/interfaces.go
index 18488d8..1aa520b 100644
--- a/codegen/testserver/interfaces.go
+++ b/codegen/testserver/interfaces.go
@@ -17,6 +17,9 @@ type Circle struct {
 }

 func (c *Circle) Area() float64 {
+       if c == nil {
+               return 0
+       }
        return c.Radius * math.Pi * math.Pi
 }

If you wanna convert typed nil to nil, You can do it in resolver middleware.
Is it enough?

[nmaquet]

@vvakame Thank you for looking into this.

so, tests are succeeded with below changes without your codegen/interface.gotpl patches.
Yes, you are correct, but there is still a problem I think. I have added another test that shows that, when using automatically generated field resolvers, returning a typed nil from an interface resolver will lead to a crash in gqlgen's generated code.

Look at this field resolver:
https://github.com/nmaquet/gqlgen/blob/check-nil-interface-ptrs/codegen/testserver/generated.go#L3517

This line will cause a panic when executing the query unless an extra check is done.

[vvakame]

🤔
I suggest you to implement nil checks in here.

gqlgen/codegen/field.gotpl

Lines 122 to 124 in 15b3058

	{{- else if .IsVariable -}}
	return {{.GoReceiverName}}.{{.GoFieldName}}, nil
	{{- end }}

Do you think this is a good idea?