From 17a95fd4de3b09c92b32d7c4cd9a58e084f33a20 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Mon, 22 Mar 2021 18:45:37 -0700 Subject: [PATCH] more TODOs, and especially clarify the situation for input type names --- DESIGN.md | 1 + README.md | 2 +- generate/config.go | 7 ++-- generate/generate.go | 10 +++--- generate/main.go | 10 +++--- generate/testdata/unexported.graphql | 5 +++ generate/testdata/unexported.graphql.go | 48 +++++++++++++++++++++++++ generate/types.go | 17 +++++---- generate/util.go | 15 ++++++++ generate/util_test.go | 22 ++++++++++++ 10 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 generate/testdata/unexported.graphql create mode 100644 generate/testdata/unexported.graphql.go diff --git a/DESIGN.md b/DESIGN.md index aadc53e..81b9dff 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -105,6 +105,7 @@ We'll do something similar to Apollo's naming scheme. Specifically: - The toplevel name will be `MyQueryResponse`, using the query-name. - Further names will be `MyQueryFieldTypeFieldType`. We will not attempt to be super smart about avoiding conflicts. - Fragments will have some naming scheme TBD but starting at the fragment. +- Input objects will have a name starting at the type, since they always have the same fields, and often have naming schemes like "MyFieldInput" already. All of this may be configurable later. diff --git a/README.md b/README.md index b430773..5c3e38f 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ Config options: - send hash rather than full query - whether names should be exported - default handling for optional fields (pointers, HasFoo, etc.) -- response/function-name format (e.g. force exported/unexported, change "Response" suffix, etc.) +- response/function-name format (e.g. force exported/unexported, change "Response" suffix, change how input objects work, etc.) - generate mocks? Other: diff --git a/generate/config.go b/generate/config.go index 60d2b63..441b814 100644 --- a/generate/config.go +++ b/generate/config.go @@ -2,6 +2,7 @@ package generate import ( "fmt" + "go/token" "io/ioutil" "path/filepath" @@ -42,7 +43,10 @@ func (c *Config) ValidateAndFillDefaults() error { } base := filepath.Base(abs) - // TODO: remove/replace bad chars, make sure there's something left? + if !token.IsIdentifier(base) { + return fmt.Errorf("unable to guess package-name: %v is not a valid identifier", base) + } + c.Package = base } @@ -69,7 +73,6 @@ func ReadAndValidateConfig(filename string) (*Config, error) { } // Make paths relative to config dir - // TODO: more principled typing here? basename := filepath.Dir(filename) config.Schema = filepath.Join(basename, config.Schema) config.Queries = filepath.Join(basename, config.Queries) diff --git a/generate/generate.go b/generate/generate.go index 492aa3e..ecc98a9 100644 --- a/generate/generate.go +++ b/generate/generate.go @@ -73,17 +73,15 @@ func (g *generator) Types() string { return strings.Join(defs, "\n\n") } -func (g *generator) getArgument(arg *ast.VariableDefinition) (argument, error) { +func (g *generator) getArgument(opName string, arg *ast.VariableDefinition) (argument, error) { graphQLName := arg.Variable - firstRest := strings.SplitN(graphQLName, "", 2) - goName := strings.ToLower(firstRest[0]) + firstRest[1] - goType, err := g.getTypeForInputType(arg.Type) + goType, err := g.getTypeForInputType(opName, arg.Type) if err != nil { return argument{}, err } return argument{ GraphQLName: graphQLName, - GoName: goName, + GoName: lowerFirst(graphQLName), GoType: goType, }, nil } @@ -121,7 +119,7 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error { args := make([]argument, len(op.VariableDefinitions)) for i, arg := range op.VariableDefinitions { var err error - args[i], err = g.getArgument(arg) + args[i], err = g.getArgument(op.Name, arg) if err != nil { return err } diff --git a/generate/main.go b/generate/main.go index 6c8d61f..4e77243 100644 --- a/generate/main.go +++ b/generate/main.go @@ -2,6 +2,7 @@ package generate import ( "fmt" + "io/ioutil" "os" ) @@ -16,15 +17,12 @@ func readConfigGenerateAndWrite(configFilename string) error { return err } - // Open out at the end -- decreases the chances we blank it if we err. - out, err := os.OpenFile(config.Generated, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) + err = ioutil.WriteFile(config.Generated, code, 0o644) if err != nil { - return fmt.Errorf("could not open generated file %v: %v", + return fmt.Errorf("could not write generated file %v: %v", config.Generated, err) } - - _, err = out.Write(code) - return err + return nil } func Main() { diff --git a/generate/testdata/unexported.graphql b/generate/testdata/unexported.graphql new file mode 100644 index 0000000..494bf6e --- /dev/null +++ b/generate/testdata/unexported.graphql @@ -0,0 +1,5 @@ +query unexported($query: UserQueryInput) { + user(query: $query) { + id + } +} diff --git a/generate/testdata/unexported.graphql.go b/generate/testdata/unexported.graphql.go new file mode 100644 index 0000000..3d6e0c1 --- /dev/null +++ b/generate/testdata/unexported.graphql.go @@ -0,0 +1,48 @@ +package test + +// Code generated by github.com/Khan/genql, DO NOT EDIT. + +import ( + "context" + + "github.com/Khan/genql/graphql" +) + +type unexportedResponse struct { + User unexportedUser `json:"user"` +} + +type unexportedUser struct { + Id string `json:"id"` +} + +type userQueryInput struct { + Email string `json:"email"` + Name string `json:"name"` + Id string `json:"id"` + Role userQueryInputRole `json:"role"` + Names []string `json:"names"` +} + +type userQueryInputRole string + +const ( + userQueryInputRoleStudent userQueryInputRole = "STUDENT" + userQueryInputRoleTeacher userQueryInputRole = "TEACHER" +) + +func unexported(client *graphql.Client, query userQueryInput) (*unexportedResponse, error) { + variables := map[string]interface{}{ + "query": query, + } + + var retval unexportedResponse + err := client.MakeRequest(context.Background(), ` +query unexported ($query: UserQueryInput) { + user(query: $query) { + id + } +} +`, &retval, variables) + return &retval, err +} diff --git a/generate/types.go b/generate/types.go index 846301a..51f0d4d 100644 --- a/generate/types.go +++ b/generate/types.go @@ -101,10 +101,13 @@ func (g *generator) addTypeForDefinition(namePrefix, nameOverride string, typ *a return name, nil } -func (g *generator) getTypeForInputType(typ *ast.Type) (string, error) { - typeName := upperFirst(typ.Name()) - builder := &typeBuilder{typeName: typeName, typeNamePrefix: typeName, generator: g} - err := builder.writeType("", typ, selectionsForType(g, typ)) +func (g *generator) getTypeForInputType(opName string, typ *ast.Type) (string, error) { + // Sort of a hack: case the input type name to match the op-name. + name := matchFirst(typ.Name(), opName) + // TODO: we have to pass name 4 times, yuck + builder := &typeBuilder{typeName: name, typeNamePrefix: name, generator: g} + fmt.Println(name) + err := builder.writeType(name, name, typ, selectionsForType(g, typ)) return builder.String(), err } @@ -199,7 +202,7 @@ func (builder *typeBuilder) writeField(field field) error { // `query q { a: f { b }, c: f { d } }` we need separate types for a // and c, even though they are the same type in GraphQL, because they // have different fields. - builder.typeNamePrefix+upperFirst(field.Alias()), typ, fields) + builder.typeNamePrefix+upperFirst(field.Alias()), "", typ, fields) if err != nil { return err } @@ -214,7 +217,7 @@ func (builder *typeBuilder) writeField(field field) error { return nil } -func (builder *typeBuilder) writeType(namePrefix string, typ *ast.Type, fields []field) error { +func (builder *typeBuilder) writeType(namePrefix, nameOverride string, typ *ast.Type, fields []field) error { // gqlgen does slightly different things here, but its implementation may // be useful to crib from: // https://github.com/99designs/gqlgen/blob/master/plugin/modelgen/models.go#L113 @@ -229,7 +232,7 @@ func (builder *typeBuilder) writeType(namePrefix string, typ *ast.Type, fields [ def := builder.schema.Types[typ.Name()] // Writes a typedef elsewhere (if not already defined) - name, err := builder.addTypeForDefinition(namePrefix, "", def, fields) + name, err := builder.addTypeForDefinition(namePrefix, nameOverride, def, fields) if err != nil { return err } diff --git a/generate/util.go b/generate/util.go index a08f75d..a3d3a1c 100644 --- a/generate/util.go +++ b/generate/util.go @@ -29,6 +29,21 @@ func upperFirst(s string) string { return changeFirst(strings.TrimLeft(s, "_"), unicode.ToUpper) } +func matchFirst(s, tmpl string) string { + c, n := utf8.DecodeRuneInString(s) + t, _ := utf8.DecodeRuneInString(tmpl) + if c == utf8.RuneError || n == utf8.RuneError { // empty or invalid + return s + } + + if unicode.IsUpper(t) { + c = unicode.ToUpper(c) + } else { + c = unicode.ToLower(c) + } + return string(c) + s[n:] +} + func goConstName(s string) string { if strings.TrimLeft(s, "_") == "" { return s diff --git a/generate/util_test.go b/generate/util_test.go index 886b729..ba59436 100644 --- a/generate/util_test.go +++ b/generate/util_test.go @@ -52,6 +52,28 @@ func TestUpperFirst(t *testing.T) { testStringFunc(t, upperFirst, tests) } +func TestMatchFirst(t *testing.T) { + tests := []struct { + name, in, out, match string + }{ + {"Empty", "", "", ""}, + {"LowerToUpper", "lower", "Lower", "Upper"}, + {"UpperToUpper", "Upper", "Upper", "Upper"}, + {"LowerToLower", "lower", "lower", "lower"}, + {"UpperToLower", "Upper", "upper", "lower"}, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + got := matchFirst(test.in, test.match) + if got != test.out { + t.Errorf("got %#v want %#v", got, test.out) + } + }) + } +} + func TestGoConstName(t *testing.T) { tests := []test{ {"Empty", "", ""},