Add a mechanism to specify options on input-type fields (#124)

## Summary:
This has been a bit of a thorn since we started using genqlient in
production: just as you might want to specify, say, `omitempty` on an
argument, you might equally want to specify it on an input-type field.
But there's no obvious syntax to do that, because the input-type field
does not appear in the query (only the schema) so there's nowhere to put
the `# @genqlient` directive.

This commit, at last, fixes that problem, via a new option, `for`, which
you use in an option applied to the entire operation (or fragment), and
says, "actually, apply this directive to the given field, not the entire
operation".  (It's mainly useful for input types, but I allowed it for
output types too; I could imagine it being convenient if you want to say
you always use a certain type or type-name for a certain field.)  It
works basically like you expect: the inline options take precedence over
`for` take precedence over query-global options.

The implementation was fairly straightforward once I did a little
refactoring, mostly in the directive-parsing and directive-merging
(which are now combined, since merging is now a bit more complicated).
With that in place, and extended to support `for`, we need only add the
same wiring to input-fields that we have for other places you can put
directives.  I did not attempt to solve the issue I've now documented
as #123, wherein conflicting options can lead to confusing behavior;
the new `for` is a new and perhaps more attractive avenue to cause it
but the issue remains the same and requires nontrivial refactoring
(described in the issue) to solve.  (The breakage isn't horrible for the
most part; the option will just apply, or not apply, where you don't
expect it to.)

But while applying that logic, I noticed a problem, which is that we
were inconsistently cascading operation-level options down to
input-object fields.  (I think this came out of the fact that initially
I thought to cascade them, then realized that this could cause problems
like #123 and intended to walk them back, but then accidentally only
"fixed" it for `omitempty`.  I guess until this change, operation-level
options were rare enough, and input-field options messy enough, that no
one noticed.)  So in this commit I bring things back into consistency,
by saying that they do cascade: with at least a sketch of a path forward
to solve #123 via better validation, I think that's by far the clearest
behavior.

Issue: https://github.com/Khan/genqlient/issues/14

## Test plan:
make check


Author: benjaminjkraft

Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo

Required Reviewers: 

Approved By: csilvers, StevenACoffman

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint

Pull Request URL: https://github.com/Khan/genqlient/pull/124

generate/convert.go

@@ -164,11 +164,10 @@ func (g *generator) convertArguments(
 	name := "__" + operation.Name + "Input"
 	fields := make([]*goStructField, len(operation.VariableDefinitions))
 	for i, arg := range operation.VariableDefinitions {
-		_, directive, err := g.parsePrecedingComment(arg, arg.Position)
+		_, options, err := g.parsePrecedingComment(arg, nil, arg.Position, queryOptions)
 		if err != nil {
 			return nil, err
 		}
-		options := queryOptions.merge(directive)
 
 		goName := upperFirst(arg.Variable)
 		// Some of the arguments don't apply here, namely the name-prefix (see
@@ -386,11 +385,13 @@ func (g *generator) convertDefinition(
 		}
 
 		for i, field := range def.Fields {
+			_, fieldOptions, err := g.parsePrecedingComment(
+				field, def, field.Position, queryOptions)
+			if err != nil {
+				return nil, err
+			}
+
 			goName := upperFirst(field.Name)
-			// There are no field-specific options for inputs (yet, see #14),
-			// but we still need to merge with an empty directive to clear out
-			// any query-options that shouldn't apply here (namely "typename").
-			fieldOptions := queryOptions.merge(newGenqlientDirective(pos))
 			// Several of the arguments don't really make sense here:
 			// (note field.Type is necessarily a scalar, input, or enum)
 			// - namePrefix is ignored for input types and enums (see
@@ -398,7 +399,7 @@ func (g *generator) convertDefinition(
 			//   names)
 			// - selectionSet is ignored for input types, because we
 			//   just use all fields of the type; and it's nonexistent
-			//   for scalars and enums, our only other possible types,
+			//   for scalars and enums, our only other possible types
 			// TODO(benkraft): Can we refactor to avoid passing the values that
 			// will be ignored?  We know field.Type is a scalar, enum, or input
 			// type.  But plumbing that is a bit tricky in practice.
@@ -414,8 +415,7 @@ 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.
+				Omitempty:   fieldOptions.GetOmitempty(),
 			}
 		}
 		return goType, nil
@@ -506,12 +506,11 @@ func (g *generator) convertSelectionSet(
 ) ([]*goStructField, error) {
 	fields := make([]*goStructField, 0, len(selectionSet))
 	for _, selection := range selectionSet {
-		_, selectionDirective, err := g.parsePrecedingComment(
-			selection, selection.GetPosition())
+		_, selectionOptions, err := g.parsePrecedingComment(
+			selection, nil, selection.GetPosition(), queryOptions)
 		if err != nil {
 			return nil, err
 		}
-		selectionOptions := queryOptions.merge(selectionDirective)
 
 		switch selection := selection.(type) {
 		case *ast.Field:
@@ -705,6 +704,8 @@ func (g *generator) convertFragmentSpread(
 		}
 	}
 
+	// TODO(benkraft): Set directive here if we ever allow @genqlient
+	// directives on fragment-spreads.
 	return &goStructField{GoName: "" /* i.e. embedded */, GoType: typ}, nil
 }
 
@@ -713,7 +714,7 @@ func (g *generator) convertFragmentSpread(
 func (g *generator) convertNamedFragment(fragment *ast.FragmentDefinition) (goType, error) {
 	typ := g.schema.Types[fragment.TypeCondition]
 
-	comment, directive, err := g.parsePrecedingComment(fragment, fragment.Position)
+	comment, directive, err := g.parsePrecedingComment(fragment, nil, fragment.Position, nil)
 	if err != nil {
 		return nil, err
 	}

generate/generate.go

@@ -244,7 +244,7 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 	f := formatter.NewFormatter(&builder)
 	f.FormatQueryDocument(queryDoc)
 
-	commentLines, directive, err := g.parsePrecedingComment(op, op.Position)
+	commentLines, directive, err := g.parsePrecedingComment(op, nil, op.Position, nil)
 	if err != nil {
 		return err
 	}

generate/genqlient_directive.go

@@ -18,14 +18,55 @@ type genqlientDirective struct {
 	Flatten   *bool
 	Bind      string
 	TypeName  string
+	// FieldDirectives contains the directives to be
+	// applied to specific fields via the "for" option.
+	// Map from type-name -> field-name -> directive.
+	FieldDirectives map[string]map[string]*genqlientDirective
 }
 
 func newGenqlientDirective(pos *ast.Position) *genqlientDirective {
 	return &genqlientDirective{
-		pos: pos,
+		pos:             pos,
+		FieldDirectives: make(map[string]map[string]*genqlientDirective),
 	}
 }
 
+// Helper for String, returns the directive but without the @genqlient().
+func (dir *genqlientDirective) argsString() string {
+	var parts []string
+	if dir.Omitempty != nil {
+		parts = append(parts, fmt.Sprintf("omitempty: %v", *dir.Omitempty))
+	}
+	if dir.Pointer != nil {
+		parts = append(parts, fmt.Sprintf("pointer: %v", *dir.Pointer))
+	}
+	if dir.Struct != nil {
+		parts = append(parts, fmt.Sprintf("struct: %v", *dir.Struct))
+	}
+	if dir.Flatten != nil {
+		parts = append(parts, fmt.Sprintf("flatten: %v", *dir.Flatten))
+	}
+	if dir.Bind != "" {
+		parts = append(parts, fmt.Sprintf("bind: %v", dir.Bind))
+	}
+	if dir.TypeName != "" {
+		parts = append(parts, fmt.Sprintf("typename: %v", dir.TypeName))
+	}
+	return strings.Join(parts, ", ")
+}
+
+// String is useful for debugging.
+func (dir *genqlientDirective) String() string {
+	lines := []string{fmt.Sprintf("@genqlient(%s)", dir.argsString())}
+	for typeName, dirs := range dir.FieldDirectives {
+		for fieldName, fieldDir := range dirs {
+			lines = append(lines, fmt.Sprintf("@genqlient(for: %s.%s, %s)",
+				typeName, fieldName, fieldDir.argsString()))
+		}
+	}
+	return strings.Join(lines, "\n")
+}
+
 func (dir *genqlientDirective) GetOmitempty() bool { return dir.Omitempty != nil && *dir.Omitempty }
 func (dir *genqlientDirective) GetPointer() bool   { return dir.Pointer != nil && *dir.Pointer }
 func (dir *genqlientDirective) GetStruct() bool    { return dir.Struct != nil && *dir.Struct }
@@ -77,7 +118,40 @@ func (dir *genqlientDirective) add(graphQLDirective *ast.Directive, pos *ast.Pos
 		return errorf(pos, "the only valid comment-directive is @genqlient, got %v", graphQLDirective.Name)
 	}
 
+	// First, see if this directive has a "for" option;
+	// if it does, the rest of our work will operate on the
+	// appropriate place in FieldDirectives.
 	var err error
+	forField := ""
+	for _, arg := range graphQLDirective.Arguments {
+		if arg.Name == "for" {
+			if forField != "" {
+				return errorf(pos, `@genqlient directive had "for:" twice`)
+			}
+			err = setString("for", &forField, arg.Value, pos)
+			if err != nil {
+				return err
+			}
+		}
+	}
+	if forField != "" {
+		forParts := strings.Split(forField, ".")
+		if len(forParts) != 2 {
+			return errorf(pos, `for must be of the form "MyType.myField"`)
+		}
+		typeName, fieldName := forParts[0], forParts[1]
+
+		fieldDir := newGenqlientDirective(pos)
+		if dir.FieldDirectives[typeName] == nil {
+			dir.FieldDirectives[typeName] = make(map[string]*genqlientDirective)
+		}
+		dir.FieldDirectives[typeName][fieldName] = fieldDir
+
+		// Now, the rest of the function will operate on fieldDir.
+		dir = fieldDir
+	}
+
+	// Now parse the rest of the arguments.
 	for _, arg := range graphQLDirective.Arguments {
 		switch arg.Name {
 		// TODO(benkraft): Use reflect and struct tags?
@@ -93,6 +167,8 @@ func (dir *genqlientDirective) add(graphQLDirective *ast.Directive, pos *ast.Pos
 			err = setString("bind", &dir.Bind, arg.Value, pos)
 		case "typename":
 			err = setString("typename", &dir.TypeName, arg.Value, pos)
+		case "for":
+			// handled above
 		default:
 			return errorf(pos, "unknown argument %v for @genqlient", arg.Name)
 		}
@@ -100,10 +176,46 @@ func (dir *genqlientDirective) add(graphQLDirective *ast.Directive, pos *ast.Pos
 			return err
 		}
 	}
+
 	return nil
 }
 
 func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) error {
+	// TODO(benkraft): This function has a lot of duplicated checks, figure out
+	// how to organize them better to avoid the duplication.
+	for typeName, byField := range dir.FieldDirectives {
+		typ, ok := schema.Types[typeName]
+		if !ok {
+			return errorf(dir.pos, `for got invalid type-name "%s"`, typeName)
+		}
+		for fieldName, fieldDir := range byField {
+			var field *ast.FieldDefinition
+			for _, typeField := range typ.Fields {
+				if typeField.Name == fieldName {
+					field = typeField
+					break
+				}
+			}
+			if field == nil {
+				return errorf(fieldDir.pos,
+					`for got invalid field-name "%s" for type "%s"`,
+					fieldName, typeName)
+			}
+
+			// All options except struct and flatten potentially apply.  (I
+			// mean in theory you could apply them here, but since they require
+			// per-use validation, it would be a bit tricky, and the use case
+			// is not clear.)
+			if fieldDir.Struct != nil || fieldDir.Flatten != nil {
+				return errorf(fieldDir.pos, "struct and flatten can't be used via for")
+			}
+
+			if fieldDir.Omitempty != nil && field.Type.NonNull {
+				return errorf(fieldDir.pos, "omitempty may only be used on optional arguments")
+			}
+		}
+	}
+
 	switch node := node.(type) {
 	case *ast.OperationDefinition:
 		if dir.Bind != "" {
@@ -139,6 +251,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 			return errorf(dir.pos, "flatten is only applicable to fields, not variable-definitions")
 		}
 
+		if len(dir.FieldDirectives) > 0 {
+			return errorf(dir.pos, "for is only applicable to operations and arguments")
+		}
+
 		return nil
 	case *ast.Field:
 		if dir.Omitempty != nil {
@@ -158,6 +274,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 			}
 		}
 
+		if len(dir.FieldDirectives) > 0 {
+			return errorf(dir.pos, "for is only applicable to operations and arguments")
+		}
+
 		return nil
 	default:
 		return errorf(dir.pos, "invalid @genqlient directive location: %T", node)
@@ -244,68 +364,119 @@ func validateFlattenOption(
 	return index, nil
 }
 
-// merge joins the directive applied to this node (the argument) and the one
-// applied to the entire operation (the receiver) and returns a new
-// directive-object representing the options to apply to this node (where in
-// general we take the node's option, then the operation's, then the default).
-func (dir *genqlientDirective) merge(other *genqlientDirective) *genqlientDirective {
-	retval := *dir
-	if other.Omitempty != nil {
-		retval.Omitempty = other.Omitempty
+func fillDefaultBool(target **bool, defaults ...*bool) {
+	if *target != nil {
+		return
 	}
-	if other.Pointer != nil {
-		retval.Pointer = other.Pointer
+
+	for _, val := range defaults {
+		if val != nil {
+			*target = val
+			return
+		}
 	}
-	if other.Struct != nil {
-		retval.Struct = other.Struct
+}
+
+func fillDefaultString(target *string, defaults ...string) {
+	if *target != "" {
+		return
 	}
-	if other.Flatten != nil {
-		retval.Flatten = other.Flatten
+
+	for _, val := range defaults {
+		if val != "" {
+			*target = val
+			return
+		}
 	}
-	if other.Bind != "" {
-		retval.Bind = other.Bind
+}
+
+// merge updates the receiver, which is a directive applied to some node, with
+// the information from the directive applied to the fragment or operation
+// containing that node.  (The update is in-place.)
+//
+// Note this has slightly different semantics than .add(), see inline for
+// details.
+//
+// parent is as described in parsePrecedingComment.  operationDirective is the
+// directive applied to this operation or fragment.
+func (dir *genqlientDirective) mergeOperationDirective(
+	node interface{},
+	parentIfInputField *ast.Definition,
+	operationDirective *genqlientDirective,
+) {
+	// We'll set forField to the `@genqlient(for: "<this field>", ...)`
+	// directive from our operation/fragment, if any.
+	var forField *genqlientDirective
+	switch field := node.(type) {
+	case *ast.Field: // query field
+		typeName := field.ObjectDefinition.Name
+		forField = operationDirective.FieldDirectives[typeName][field.Name]
+	case *ast.FieldDefinition: // input-type field
+		forField = operationDirective.FieldDirectives[parentIfInputField.Name][field.Name]
 	}
-	// For typename, the local directive always wins: when specified on the query
-	// options typename applies to the response-struct, not to all parts of the
-	// query.
-	retval.TypeName = other.TypeName
-	return &retval
+	// Just to simplify nil-checking in the code below:
+	if forField == nil {
+		forField = newGenqlientDirective(nil)
+	}
+
+	// Now fill defaults; in general local directive wins over the "for" field
+	// directive wins over the operation directive.
+	fillDefaultBool(&dir.Omitempty, forField.Omitempty, operationDirective.Omitempty)
+	fillDefaultBool(&dir.Pointer, forField.Pointer, operationDirective.Pointer)
+	// struct and flatten aren't settable via "for".
+	fillDefaultBool(&dir.Struct, operationDirective.Struct)
+	fillDefaultBool(&dir.Flatten, operationDirective.Flatten)
+	fillDefaultString(&dir.Bind, forField.Bind, operationDirective.Bind)
+	// typename isn't settable on the operation (when set there it replies to
+	// the response-type).
+	fillDefaultString(&dir.TypeName, forField.TypeName)
 }
 
 // parsePrecedingComment looks at the comment right before this node, and
 // returns the genqlient directive applied to it (or an empty one if there is
 // none), the remaining human-readable comment (or "" if there is none), and an
 // error if the directive is invalid.
+//
+// queryOptions are the options to be applied to this entire query (or
+// fragment); the local options will be merged into those.  It should be nil if
+// we are parsing the directive on the entire query.
+//
+// parentIfInputField need only be set if node is an input-type field; it
+// should be the type containing this field.  (We can get this from gqlparser
+// in other cases, but not input-type fields.)
 func (g *generator) parsePrecedingComment(
 	node interface{},
+	parentIfInputField *ast.Definition,
 	pos *ast.Position,
+	queryOptions *genqlientDirective,
 ) (comment string, directive *genqlientDirective, err error) {
 	directive = newGenqlientDirective(pos)
 	hasDirective := false
-	if pos == nil || pos.Src == nil { // node was added by genqlient itself
-		return "", directive, nil // treated as if there were no comment
-	}
 
+	// For directives on genqlient-generated nodes, we don't actually need to
+	// parse anything.  (But we do need to merge below.)
 	var commentLines []string
-	sourceLines := strings.Split(pos.Src.Input, "\n")
-	for i := pos.Line - 1; i > 0; i-- {
-		line := strings.TrimSpace(sourceLines[i-1])
-		trimmed := strings.TrimSpace(strings.TrimPrefix(line, "#"))
-		if strings.HasPrefix(line, "# @genqlient") {
-			hasDirective = true
-			var graphQLDirective *ast.Directive
-			graphQLDirective, err = parseDirective(trimmed, pos)
-			if err != nil {
-				return "", nil, err
+	if pos != nil && pos.Src != nil {
+		sourceLines := strings.Split(pos.Src.Input, "\n")
+		for i := pos.Line - 1; i > 0; i-- {
+			line := strings.TrimSpace(sourceLines[i-1])
+			trimmed := strings.TrimSpace(strings.TrimPrefix(line, "#"))
+			if strings.HasPrefix(line, "# @genqlient") {
+				hasDirective = true
+				var graphQLDirective *ast.Directive
+				graphQLDirective, err = parseDirective(trimmed, pos)
+				if err != nil {
+					return "", nil, err
+				}
+				err = directive.add(graphQLDirective, pos)
+				if err != nil {
+					return "", nil, err
+				}
+			} else if strings.HasPrefix(line, "#") {
+				commentLines = append(commentLines, trimmed)
+			} else {
+				break
 			}
-			err = directive.add(graphQLDirective, pos)
-			if err != nil {
-				return "", nil, err
-			}
-		} else if strings.HasPrefix(line, "#") {
-			commentLines = append(commentLines, trimmed)
-		} else {
-			break
 		}
 	}
 
@@ -316,6 +487,11 @@ func (g *generator) parsePrecedingComment(
 		}
 	}
 
+	if queryOptions != nil {
+		// If we are part of an operation/fragment, merge its options in.
+		directive.mergeOperationDirective(node, parentIfInputField, queryOptions)
+	}
+
 	reverse(commentLines)
 
 	return strings.TrimSpace(strings.Join(commentLines, "\n")), directive, nil

generate/testdata/queries/Omitempty.graphql

@@ -1,4 +1,5 @@
 # @genqlient(omitempty: true)
+# @genqlient(for: "UserQueryInput.id", omitempty: false)
 query OmitEmptyQuery(
   $query: UserQueryInput,
   $queries: [UserQueryInput],

generate/testdata/queries/Pointers.graphql

@@ -1,4 +1,6 @@
 # @genqlient(pointer: true)
+# @genqlient(for: "UserQueryInput.id", pointer: false)
+# @genqlient(for: "User.id", pointer: false)
 query PointersQuery(
   $query: UserQueryInput,
   # @genqlient(pointer: false)
@@ -6,6 +8,7 @@ query PointersQuery(
   $tz: String,
 ) {
   user(query: $query) {
+    # @genqlient(pointer: true)
     id
     roles
     name

generate/testdata/snapshots/TestGenerate-MultipleDirectives.graphql-MultipleDirectives.graphql.go

@@ -17,13 +17,13 @@ import (
 // Or maybe ideally it wouldn't.
 // Really I'm just talking to make this documentation longer.
 type MyInput struct {
-	Email *string `json:"email"`
-	Name  *string `json:"name"`
+	Email *string `json:"email,omitempty"`
+	Name  *string `json:"name,omitempty"`
 	// id looks the user up by ID.  It's a great way to look up users.
-	Id         *testutil.ID      `json:"id"`
-	Role       *Role             `json:"role"`
-	Names      []*string         `json:"names"`
-	HasPokemon *testutil.Pokemon `json:"hasPokemon"`
+	Id         *testutil.ID      `json:"id,omitempty"`
+	Role       *Role             `json:"role,omitempty"`
+	Names      []*string         `json:"names,omitempty"`
+	HasPokemon *testutil.Pokemon `json:"hasPokemon,omitempty"`
 	Birthdate  *time.Time        `json:"-"`
 }
 
@@ -163,13 +163,13 @@ const (
 // Or maybe ideally it wouldn't.
 // Really I'm just talking to make this documentation longer.
 type UserQueryInput struct {
-	Email *string `json:"email"`
-	Name  *string `json:"name"`
+	Email *string `json:"email,omitempty"`
+	Name  *string `json:"name,omitempty"`
 	// id looks the user up by ID.  It's a great way to look up users.
-	Id         *testutil.ID      `json:"id"`
-	Role       *Role             `json:"role"`
-	Names      []*string         `json:"names"`
-	HasPokemon *testutil.Pokemon `json:"hasPokemon"`
+	Id         *testutil.ID      `json:"id,omitempty"`
+	Role       *Role             `json:"role,omitempty"`
+	Names      []*string         `json:"names,omitempty"`
+	HasPokemon *testutil.Pokemon `json:"hasPokemon,omitempty"`
 	Birthdate  *time.Time        `json:"-"`
 }
 

generate/testdata/snapshots/TestGenerate-Omitempty.graphql-Omitempty.graphql.go

@@ -65,13 +65,13 @@ const (
 // Or maybe ideally it wouldn't.
 // Really I'm just talking to make this documentation longer.
 type UserQueryInput struct {
-	Email string `json:"email"`
-	Name  string `json:"name"`
+	Email string `json:"email,omitempty"`
+	Name  string `json:"name,omitempty"`
 	// id looks the user up by ID.  It's a great way to look up users.
 	Id         testutil.ID      `json:"id"`
-	Role       Role             `json:"role"`
-	Names      []string         `json:"names"`
-	HasPokemon testutil.Pokemon `json:"hasPokemon"`
+	Role       Role             `json:"role,omitempty"`
+	Names      []string         `json:"names,omitempty"`
+	HasPokemon testutil.Pokemon `json:"hasPokemon,omitempty"`
 	Birthdate  time.Time        `json:"-"`
 }
 

generate/testdata/snapshots/TestGenerate-Pointers.graphql-Pointers.graphql.go

@@ -19,7 +19,7 @@ type PointersQueryOtherUser struct {
 	// id is the user's ID.
 	//
 	// It is stable, unique, and opaque, like all good IDs.
-	Id *testutil.ID `json:"id"`
+	Id testutil.ID `json:"id"`
 }
 
 // PointersQueryResponse is returned by PointersQuery on success.
@@ -75,7 +75,7 @@ type UserQueryInput struct {
 	Email *string `json:"email"`
 	Name  *string `json:"name"`
 	// id looks the user up by ID.  It's a great way to look up users.
-	Id         *testutil.ID      `json:"id"`
+	Id         testutil.ID       `json:"id"`
 	Role       *Role             `json:"role"`
 	Names      []*string         `json:"names"`
 	HasPokemon *testutil.Pokemon `json:"hasPokemon"`
@@ -121,7 +121,7 @@ type __premarshalUserQueryInput struct {
 
 	Name *string `json:"name"`
 
-	Id *testutil.ID `json:"id"`
+	Id testutil.ID `json:"id"`
 
 	Role *Role `json:"role"`