Merge pull request 99designs#298 from 99designs/handle-response-nulls

Nulls in required fields should cause errors and bubble
cgxxv · Aug 22, 2018 · e8eab66 · e8eab66
2 parents ebaab0f + 6b895ab
commit e8eab66
Show file tree

Hide file tree

Showing 24 changed files with 2,235 additions and 22 deletions.
diff --git a/client/client.go b/client/client.go
@@ -73,7 +73,7 @@ func (p *Client) mkRequest(query string, options ...Option) Request {
 	return r
 }
 
-func (p *Client) Post(query string, response interface{}, options ...Option) error {
+func (p *Client) Post(query string, response interface{}, options ...Option) (resperr error) {
 	r := p.mkRequest(query, options...)
 	requestBody, err := json.Marshal(r)
 	if err != nil {
@@ -109,11 +109,13 @@ func (p *Client) Post(query string, response interface{}, options ...Option) err
 		return fmt.Errorf("decode: %s", err.Error())
 	}
 
+	// we want to unpack even if there is an error, so we can see partial responses
+	unpackErr := unpack(respDataRaw.Data, response)
+
 	if respDataRaw.Errors != nil {
 		return RawJsonError{respDataRaw.Errors}
 	}
-
-	return unpack(respDataRaw.Data, response)
+	return unpackErr
 }
 
 type RawJsonError struct {

diff --git a/codegen/object.go b/codegen/object.go
@@ -7,6 +7,8 @@ import (
 	"strings"
 	"text/template"
 	"unicode"
+
+	"github.com/vektah/gqlparser/ast"
 )
 
 type GoFieldType int
@@ -183,13 +185,26 @@ func (f *Field) CallArgs() string {
 
 // should be in the template, but its recursive and has a bunch of args
 func (f *Field) WriteJson() string {
-	return f.doWriteJson("res", f.Type.Modifiers, false, 1)
+	return f.doWriteJson("res", f.Type.Modifiers, f.ASTType, false, 1)
 }
 
-func (f *Field) doWriteJson(val string, remainingMods []string, isPtr bool, depth int) string {
+func (f *Field) doWriteJson(val string, remainingMods []string, astType *ast.Type, isPtr bool, depth int) string {
 	switch {
 	case len(remainingMods) > 0 && remainingMods[0] == modPtr:
-		return fmt.Sprintf("if %s == nil { return graphql.Null }\n%s", val, f.doWriteJson(val, remainingMods[1:], true, depth+1))
+		return tpl(`
+			if {{.val}} == nil {
+				{{- if .nonNull }}
+					if !ec.HasError(rctx) {
+						ec.Errorf(ctx, "must not be null")
+					}
+				{{- end }}
+				return graphql.Null 
+			}
+			{{.next }}`, map[string]interface{}{
+			"val":     val,
+			"nonNull": astType.NonNull,
+			"next":    f.doWriteJson(val, remainingMods[1:], astType, true, depth+1),
+		})
 
 	case len(remainingMods) > 0 && remainingMods[0] == modList:
 		if isPtr {
@@ -211,7 +226,7 @@ func (f *Field) doWriteJson(val string, remainingMods []string, isPtr bool, dept
 			"val":   val,
 			"arr":   arr,
 			"index": index,
-			"next":  f.doWriteJson(val+"["+index+"]", remainingMods[1:], false, depth+1),
+			"next":  f.doWriteJson(val+"["+index+"]", remainingMods[1:], astType.Elem, false, depth+1),
 		})
 
 	case f.IsScalar:

diff --git a/codegen/object_build.go b/codegen/object_build.go
@@ -108,7 +108,7 @@ func (cfg *Config) buildObject(types NamedTypes, typ *ast.Definition, imports *I
 	for _, field := range typ.Fields {
 		if typ == cfg.schema.Query && field.Name == "__type" {
 			obj.Fields = append(obj.Fields, Field{
-				Type:           &Type{types["__Schema"], []string{modPtr}, nil},
+				Type:           &Type{types["__Schema"], []string{modPtr}, ast.NamedType("__Schema", nil), nil},
 				GQLName:        "__schema",
 				NoErr:          true,
 				GoFieldType:    GoFieldMethod,
@@ -120,14 +120,14 @@ func (cfg *Config) buildObject(types NamedTypes, typ *ast.Definition, imports *I
 		}
 		if typ == cfg.schema.Query && field.Name == "__schema" {
 			obj.Fields = append(obj.Fields, Field{
-				Type:           &Type{types["__Type"], []string{modPtr}, nil},
+				Type:           &Type{types["__Type"], []string{modPtr}, ast.NamedType("__Schema", nil), nil},
 				GQLName:        "__type",
 				NoErr:          true,
 				GoFieldType:    GoFieldMethod,
 				GoReceiverName: "ec",
 				GoFieldName:    "introspectType",
 				Args: []FieldArgument{
-					{GQLName: "name", Type: &Type{types["String"], []string{}, nil}, Object: &Object{}},
+					{GQLName: "name", Type: &Type{types["String"], []string{}, ast.NamedType("String", nil), nil}, Object: &Object{}},
 				},
 				Object: obj,
 			})

diff --git a/codegen/templates/data.go b/codegen/templates/data.go
diff --git a/codegen/templates/field.gotpl b/codegen/templates/field.gotpl
@@ -49,6 +49,11 @@
 			{{- end }}
 		})
 		if resTmp == nil {
+			{{- if $field.ASTType.NonNull }}
+				if !ec.HasError(rctx) {
+					ec.Errorf(ctx, "must not be null")
+				}
+			{{- end }}
 			return graphql.Null
 		}
 		res := resTmp.({{$field.Signature}})

diff --git a/codegen/templates/object.gotpl b/codegen/templates/object.gotpl
@@ -34,6 +34,7 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se
 
 	{{if $object.IsConcurrent}} var wg sync.WaitGroup {{end}}
 	out := graphql.NewOrderedMap(len(fields))
+	invalid := false
 	for i, field := range fields {
 		out.Keys[i] = field.Alias
 
@@ -47,6 +48,11 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se
 				go func(i int, field graphql.CollectedField) {
 			{{- end }}
 				out.Values[i] = ec._{{$object.GQLType}}_{{$field.GQLName}}(ctx, field{{if not $object.Root}}, obj{{end}})
+				{{- if $field.ASTType.NonNull }}
+					if out.Values[i] == graphql.Null {
+						invalid = true
+					}
+				{{- end }}
 			{{- if $field.IsConcurrent }}
 					wg.Done()
 				}(i, field)
@@ -57,6 +63,7 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se
 		}
 	}
 	{{if $object.IsConcurrent}} wg.Wait() {{end}}
+	if invalid { return graphql.Null }
 	return out
 }
 {{- end }}