Refactor argument-handling to use a struct (#103)
## Summary: 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 two breaking changes: 1. 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`, and it should be a lot more future-proof. 2. genqlient's handling of the `omitempty` option has changed to match that of `encoding/json`, in particular it now never considers structs "empty". The difference was never intentional (I just didn't realize that behavior of `encoding/json`); arguably our behavior was more useful but I think that's outweighed by the value of consistency with `encoding/json` as well as the simpler and more correct implementation (fixing #43 is actually quite nontrivial otherwise). Once we have custom unmarshaler support (#38), users will be able to map a zero value to JSON null if they wish, which is mostly if not entirely equivalent for GraphQL's purposes. Issue: https://github.com/Khan/genqlient/issues/38 Issue: https://github.com/Khan/genqlient/issues/43 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14) Pull Request URL: https://github.com/Khan/genqlient/pull/103
This commit is contained in:
+9
-40
@@ -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.
|
||||
@@ -69,14 +72,6 @@ type exportedOperations struct {
|
||||
Operations []*operation `json:"operations"`
|
||||
}
|
||||
|
||||
type argument struct {
|
||||
GoName string
|
||||
GoType string
|
||||
GraphQLName string
|
||||
IsSlice bool
|
||||
Options *genqlientDirective
|
||||
}
|
||||
|
||||
func newGenerator(
|
||||
config *Config,
|
||||
schema *ast.Schema,
|
||||
@@ -125,29 +120,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 +249,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 +282,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
|
||||
|
||||
Reference in New Issue
Block a user