Refactor argument-handling to use a struct

In this commit I refactor the argument-generation logic to move most of
the code out of the template and into the type-generator.  This logic
predates #51, and I didn't think to update it there, but I think it
benefits from similar treatment, for similar reasons.

Specifically, the main change is to treat variables as another struct
type we can generate, rather than handling them inline as a
`map[string]interface{}`.  Users still pass them the same way, but
instead of putting them into a `map[string]interface{}` and JSONifying
that, we generate a struct and put them there.

This turns out to simplify things quite a lot, because we already have a
lot of code to generate types.  Notably, the omitempty code goes from a
dozen lines to basically two, and fixes a bug (#43) in the process,
because now that we have a struct, `json.Marshal` will do our work for
us! (And, once we have syntax for it (#14), we'll be able to handle
field-level omitempty basically for free.)  More importantly, it will
simplify custom marshalers (#38, forthcoming) significantly, since we do
all that logic at the containing-struct level, but will need to apply it
to arguments.

It does require one breaking change, for folks implementing the
`graphql.Client` API (rather than just calling `NewClient`): we now pass
them variables as an `interface{}` rather than a
`map[string]interface{}`.  For most callers, including Khan/webapp, this
is basically a one-line change to the signature of their `MakeRequest`.

Issue: #38
Issue: #43

Test plan:
make check

Reviewers: marksandstrom, steve, jvoll, mahtab, adam, miguel

docs/CHANGELOG.md

@@ -22,10 +22,14 @@ When releasing a new version:
 
 ### Breaking changes:
 
+- The [`graphql.Client`](https://pkg.go.dev/github.com/Khan/genqlient/graphql#Client) interface now accepts `variables interface{}` (containing a JSON-marshalable value) rather than `variables map[string]interface{}`.  Clients implementing the interface themselves will need to change the signature; clients who simply call `graphql.NewClient` are unaffected.
+
 ### New features:
 
 ### Bug fixes:
 
+- The `omitempty` option now works correctly for struct- and map-typed variables, which is to say it never considers them empty (matching `encoding/json`). (#43)
+
 ## v0.1.0
 
 First open-sourced version.

docs/genqlient_directive.graphql

@@ -32,8 +32,9 @@
 # query, "d" applies to arg2 and arg3, and "e" applies to field1 and field2.
 directive genqlient(
 
-  # If set, this argument will be omitted if it's equal to its Go zero
-  # value, or is an empty slice.
+  # If set, this argument will be omitted if it has an empty value, defined
+  # (the same as in encoding/json) as false, 0, a nil pointer, a nil interface
+  # value, and any empty array, slice, map, or string.
   #
   # For example, given the following query:
   #  # @genqlient(omitempty: true)

generate/convert.go

@@ -7,7 +7,7 @@ package generate
 // into code, in types.go.
 //
 // The entrypoints are convertOperation, which builds the response-type for a
-// query, and convertInputType, which builds the argument-types.
+// query, and convertArguments, which builds the argument-types.
 
 import (
 	"fmt"
@@ -139,15 +139,63 @@ var builtinTypes = map[string]string{
 	"ID":      "string",
 }
 
-// convertInputType decides the Go type we will generate corresponding to an
-// argument to a GraphQL operation.
-func (g *generator) convertInputType(
-	typ *ast.Type,
-	options, queryOptions *genqlientDirective,
-) (goType, error) {
-	// note prefix is ignored here (see generator.typeName), as is selectionSet
-	// (for input types we use the whole thing).
-	return g.convertType(nil, typ, nil, options, queryOptions)
+// convertArguments builds the type of the GraphQL arguments to the given
+// operation.
+//
+// This type is not exposed to the user; it's just used internally in the
+// unmarshaler; and it's used as a container
+func (g *generator) convertArguments(
+	operation *ast.OperationDefinition,
+	queryOptions *genqlientDirective,
+) (*goStructType, error) {
+	if len(operation.VariableDefinitions) == 0 {
+		return nil, nil
+	}
+	name := "__" + operation.Name + "Input"
+	fields := make([]*goStructField, len(operation.VariableDefinitions))
+	for i, arg := range operation.VariableDefinitions {
+		_, directive, err := g.parsePrecedingComment(arg, arg.Position)
+		if err != nil {
+			return nil, err
+		}
+		options := queryOptions.merge(directive)
+
+		goName := upperFirst(arg.Variable)
+		// note prefix is ignored here (see generator.typeName), as is
+		// selectionSet (for input types we use the whole thing).
+		goType, err := g.convertType(nil, arg.Type, nil, options, queryOptions)
+		if err != nil {
+			return nil, err
+		}
+
+		fields[i] = &goStructField{
+			GoName:      goName,
+			GoType:      goType,
+			JSONName:    arg.Variable,
+			GraphQLName: arg.Variable,
+			Omitempty:   options.GetOmitempty(),
+		}
+	}
+	goType := &goStructType{
+		GoName:    name,
+		Fields:    fields,
+		Selection: nil,
+		descriptionInfo: descriptionInfo{
+			CommentOverride: fmt.Sprintf("%s is used internally by genqlient", name),
+			// fake name, used by addType
+			GraphQLName: name,
+		},
+	}
+	goTypeAgain, err := g.addType(goType, goType.GoName, operation.Position)
+	if err != nil {
+		return nil, err
+	}
+	goType, ok := goTypeAgain.(*goStructType)
+	if !ok {
+		return nil, errorf(
+			operation.Position, "internal error: input type was %T", goTypeAgain)
+	}
+	return goType, nil
 }
 
 // convertType decides the Go type we will generate corresponding to a
@@ -325,6 +373,8 @@ func (g *generator) convertDefinition(
 				JSONName:    field.Name,
 				GraphQLName: field.Name,
 				Description: field.Description,
+				// TODO(benkraft): set Omitempty once we have a way for the
+				// user to specify it.
 			}
 		}
 		return goType, nil

generate/generate.go

@@ -55,8 +55,11 @@ type operation struct {
 	Doc string `json:"-"`
 	// The body of the operation to send.
 	Body string `json:"query"`
-	// The arguments to the operation.
-	Args []argument `json:"-"`
+	// The type of the argument to the operation, which we use both internally
+	// and to construct the arguments.  We do it this way so we can use the
+	// machinery we have for handling (and, specifically, json-marshaling)
+	// types.
+	Input *goStructType `json:"-"`
 	// The type-name for the operation's response type.
 	ResponseName string `json:"-"`
 	// The original filename from which we got this query.
@@ -71,9 +74,8 @@ type exportedOperations struct {
 
 type argument struct {
 	GoName      string
-	GoType      string
+	GoType      goType
 	GraphQLName string
-	IsSlice     bool
 	Options     *genqlientDirective
 }
 
@@ -125,29 +127,6 @@ func (g *generator) WriteTypes(w io.Writer) error {
 	return nil
 }
 
-func (g *generator) getArgument(
-	arg *ast.VariableDefinition,
-	operationDirective *genqlientDirective,
-) (argument, error) {
-	_, directive, err := g.parsePrecedingComment(arg, arg.Position)
-	if err != nil {
-		return argument{}, err
-	}
-
-	graphQLName := arg.Variable
-	goTyp, err := g.convertInputType(arg.Type, directive, operationDirective)
-	if err != nil {
-		return argument{}, err
-	}
-	return argument{
-		GraphQLName: graphQLName,
-		GoName:      lowerFirst(graphQLName),
-		GoType:      goTyp.Reference(),
-		IsSlice:     arg.Type.Elem != nil,
-		Options:     operationDirective.merge(directive),
-	}, nil
-}
-
 // usedFragmentNames returns the named-fragments used by (i.e. spread into)
 // this operation.
 func (g *generator) usedFragments(op *ast.OperationDefinition) ast.FragmentDefinitionList {
@@ -277,12 +256,9 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 		return err
 	}
 
-	args := make([]argument, len(op.VariableDefinitions))
-	for i, arg := range op.VariableDefinitions {
-		args[i], err = g.getArgument(arg, directive)
-		if err != nil {
-			return err
-		}
+	inputType, err := g.convertArguments(op, directive)
+	if err != nil {
+		return err
 	}
 
 	responseType, err := g.convertOperation(op, directive)
@@ -313,7 +289,7 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 		// rather than in the template so exported operations will match
 		// *exactly* what we send to the server.
 		Body:           "\n" + builder.String(),
-		Args:           args,
+		Input:          inputType,
 		ResponseName:   responseType.Reference(),
 		SourceFilename: sourceFilename,
 		Config:         g.Config, // for the convenience of the template

generate/operation.go.tmpl

@@ -6,32 +6,20 @@ func {{.Name}}(
     {{- if not .Config.ClientGetter -}}
     client {{ref "github.com/Khan/genqlient/graphql.Client"}},
     {{end}}
-    {{- range .Args -}}
-    {{.GoName}} {{.GoType}},
+    {{- if .Input -}}
+    {{- range .Input.Fields -}}
+    {{/* the GraphQL name is here the user-specified variable-name */ -}}
+    {{.GraphQLName}} {{.GoType.Reference}},
+    {{end -}}
     {{end -}}
 ) (*{{.ResponseName}}, error) {
-    {{- if .Args -}}
-    variables := map[string]interface{}{
-        {{range .Args -}}
-        {{if not .Options.GetOmitempty -}}
-        "{{.GraphQLName}}": {{.GoName}},
+    {{- if .Input -}}
+    {{/* __input is not a legal variable name in GraphQL */ -}}
+    __input := {{.Input.GoName}}{
+        {{range .Input.Fields -}}
+        {{.GoName}}: {{.GraphQLName}},
         {{end -}}
-        {{end}}
-    }
-    {{range .Args -}}
-    {{if .Options.GetOmitempty -}}
-    {{if .IsSlice -}}
-    if len({{.GoName}}) > 0 {
-    {{else -}}
-    {{/* zero_{{.GoType}} would be a better name, but {{.GoType}} would require
-         munging since it might be, say, `time.Time`. */}}
-    var zero_{{.GoName}} {{.GoType}}
-    if {{.GoName}} != zero_{{.GoName}} {
-    {{end -}}
-        variables["{{.GraphQLName}}"] = {{.GoName}}
     }
-    {{end}}
-    {{end}}
     {{end -}}
 
     var err error
@@ -48,7 +36,7 @@ func {{.Name}}(
         "{{.Name}}",
         `{{.Body}}`,
         &retval,
-        {{if .Args}}variables{{else}}nil{{end}},
+        {{if .Input}}&__input{{else}}nil{{end}},
     )
     return &retval, err
 }