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

Using directives on a field returning an interface does not allow nil #799

Closed
jszwedko opened this issue Jul 22, 2019 · 3 comments · Fixed by #819
Closed

Using directives on a field returning an interface does not allow nil #799

jszwedko opened this issue Jul 22, 2019 · 3 comments · Fixed by #819
Labels
bug Something isn't working

Comments

@jszwedko
Copy link

What happened?

Using a directive on a field that returns a GraphQL/Go interface type prohibits you from returning nil, even if the field is nullable, as the generated type assertion fails.

What did you expect?

Returning nil for an interface field that is nullable is possible when using a directive.

Minimal graphql.schema and models to reproduce

schema.graphql

directive @user on FIELD_DEFINITION

interface Node {
  id: ID!
}

type Query {
  node(id: ID!): Node @user
}

gqlgen.yml

# .gqlgen.yml example
#
# Refer to https://gqlgen.com/config/
# for detailed .gqlgen.yml documentation.

schema:
- schema.graphql
exec:
  filename: generated.go
model:
  filename: models_gen.go
resolver:
  filename: resolver.go
  type: Resolver
autobind: []

resolver.go

package gqlgen_test

import (
        "context"
)

type Resolver struct{}

func (r *Resolver) Query() QueryResolver {
        return &queryResolver{r}
}

type queryResolver struct{ *Resolver }

func (r *queryResolver) Node(ctx context.Context, s string) (Node, error) {
        return nil, nil
}

type Node interface {
        IsNode()
}

The issue appears to be with this part of the generated code:

  resTmp, err := ec.ResolverMiddleware(ctx, func(rctx context.Context) (interface{}, error) {
    directive0 := func(rctx context.Context) (interface{}, error) {
      ctx = rctx // use context from middleware stack in children
      return ec.resolvers.Query().Node(rctx, args["id"].(string))
    }
    directive1 := func(ctx context.Context) (interface{}, error) {
      return ec.directives.User(ctx, nil, directive0)
    }
    tmp, err := directive1(rctx)
    if err != nil {
      return nil, err
    }
    if data, ok := tmp.(Node); ok {
      return data, nil
    }
    return nil, fmt.Errorf(`unexpected type %T from directive, should be gqlgen-test.Node`, tmp)
  })

I believe it should check for nil before the type assertion check as type assertions will return false if the stored value is nil.

versions

module gqlgen-test

go 1.12

require (
        github.com/99designs/gqlgen v0.9.1
        github.com/vektah/gqlparser v1.1.2
)
@jszwedko
Copy link
Author

Scratch this, I see this is fixed in master (#768)

@jszwedko
Copy link
Author

I spoke too soon, I still see this issue with the head of master for interface types. Scalars seem fine with that PR's changes.

@jszwedko jszwedko reopened this Jul 22, 2019
@jszwedko
Copy link
Author

I wasn't able to get the tests setup to verify if this doesn't break anything, but the following patch seems to fix the issue:

diff --git a/codegen/config/binder.go b/codegen/config/binder.go
index 72956de..228cb97 100644
--- a/codegen/config/binder.go
+++ b/codegen/config/binder.go
@@ -225,9 +225,13 @@ func (t *TypeReference) IsPtr() bool {
 }
 
 func (t *TypeReference) IsNilable() bool {
-	_, isPtr := t.GO.(*types.Pointer)
-	_, isMap := t.GO.(*types.Map)
-	_, isInterface := t.GO.(*types.Interface)
+	goType := t.GO
+	if namedType, isNamed := goType.(*types.Named); isNamed {
+		goType = namedType.Underlying()
+	}
+	_, isPtr := goType.(*types.Pointer)
+	_, isMap := goType.(*types.Map)
+	_, isInterface := goType.(*types.Interface)
 	return isPtr || isMap || isInterface
 }

The issue is that the type was a types.Named.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants