From a42c9b8166586c56b7827101a925f3244a016570 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Mon, 22 Mar 2021 18:11:51 -0700 Subject: [PATCH] clean up various TODOs and comments --- .github/workflows/go.yml | 2 +- DESIGN.md | 2 ++ Makefile | 3 +++ README.md | 6 ++++-- example/generated.go | 20 ++++++++++---------- generate/config.go | 4 ++-- generate/generate.go | 22 ++++++++++++++-------- generate/parse.go | 3 --- generate/template.go | 7 +++++-- generate/types.go | 32 ++++++++++++++------------------ generate/unmarshal.go | 2 +- graphql/client.go | 20 +++++++------------- 12 files changed, 63 insertions(+), 60 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 86c83e0..a7be108 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -12,7 +12,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - go: [ '1.13', '1.14' ] + go: [ '1.13', '1.14', '1.15', '1.16' ] steps: - name: Set up Go diff --git a/DESIGN.md b/DESIGN.md index 7239794..aadc53e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -97,6 +97,8 @@ In other tools: - gqlgen doesn't have this problem, because on the server each GraphQL type maps to a unique Go type; and to the extent that different queries need different fields this is handled at the level of which fields are serialized or which resolvers are called. - shurcooL/graphql allows either way. +A side advantage of an approach that prefixes the query name to all the type names, for us, is that it means that the caller can decide in a very simple way whether to make the name exported, and avoids conflicts between queries. + **Decision:** In general, it seems like even in languages with better ergonomics for unnamed types, Apollo's approach is somewhat reasonable. And unnamed types will get really hairy for large queries in Go. On some level, even if the naming scheme is bad, it won't be as bad as unnamed types -- if you don't need to refer to the intermediate type, you don't care, and if you do, it's better than a giant inline struct. (But it's hard to change later without a flag as existing code may depend on those types.) We'll do something similar to Apollo's naming scheme. Specifically: diff --git a/Makefile b/Makefile index 6e5e01e..308fa14 100644 --- a/Makefile +++ b/Makefile @@ -2,4 +2,7 @@ example: go generate ./... go run ./example/cmd/example/main.go csilvers +check: + go test ./... + .PHONY: example diff --git a/README.md b/README.md index e8713a5..b430773 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ func getViewer(ctx context.Context, client *graphql.Client) (*getViewerResponse, // your code (error handling omitted for brevity) graphqlClient := graphql.NewClient("https://example.com/graphql", http.DefaultClient) viewerResp, _ := getViewer(context.Background(), graphqlClient) -fmt.Println("you are", *viewerResp.Viewer.MyName) +fmt.Println("you are", viewerResp.Viewer.MyName) //go:generate go run github.com/Khan/genql ``` @@ -80,11 +80,13 @@ 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.) +- generate mocks? Other: - (*) error-checking/validation/etc. everywhere - (*) a name that's more clearly distinct from other libraries out there and conveys what this does - (+) more tests - (+) documentation -- custom scalar types +- custom scalar types (or custom mappings for standard scalars, if you want a special ID type say) - allow mapping a custom type to a particular val (if you want to use a named type for some string, say) diff --git a/example/generated.go b/example/generated.go index a8fb3eb..75afe97 100644 --- a/example/generated.go +++ b/example/generated.go @@ -8,24 +8,24 @@ import ( "github.com/Khan/genql/graphql" ) -type GetUserResponse struct { - User GetUserUser `json:"user"` +type getUserResponse struct { + User getUserUser `json:"user"` } -type GetUserUser struct { +type getUserUser struct { TheirName string `json:"theirName"` } -type GetViewerResponse struct { - Viewer GetViewerViewerUser `json:"viewer"` +type getViewerResponse struct { + Viewer getViewerViewerUser `json:"viewer"` } -type GetViewerViewerUser struct { +type getViewerViewerUser struct { MyName string } -func getViewer(ctx context.Context, client *graphql.Client) (*GetViewerResponse, error) { - var retval GetViewerResponse +func getViewer(ctx context.Context, client *graphql.Client) (*getViewerResponse, error) { + var retval getViewerResponse err := client.MakeRequest(ctx, ` query getViewer { viewer { @@ -37,12 +37,12 @@ query getViewer { } // getUser gets the given user's name from their username. -func getUser(ctx context.Context, client *graphql.Client, login string) (*GetUserResponse, error) { +func getUser(ctx context.Context, client *graphql.Client, login string) (*getUserResponse, error) { variables := map[string]interface{}{ "Login": login, } - var retval GetUserResponse + var retval getUserResponse err := client.MakeRequest(ctx, ` query getUser ($Login: String!) { user(login: $Login) { diff --git a/generate/config.go b/generate/config.go index 41f7591..60d2b63 100644 --- a/generate/config.go +++ b/generate/config.go @@ -21,8 +21,8 @@ type Config struct { Package string `yaml:"package"` // The filename with the GraphQL schema (in SDL format); defaults to // schema.graphql - // TODO: allow other formats - // TODO: allow URLs + // TODO: Allow fetching a schema via introspection (will need to figure out + // how to convert that to SDL). Schema string `yaml:"schema"` // The filename with the queries; defaults to queries.graphql Queries string `yaml:"queries"` diff --git a/generate/generate.go b/generate/generate.go index 7a08cf2..492aa3e 100644 --- a/generate/generate.go +++ b/generate/generate.go @@ -56,14 +56,20 @@ func newGenerator(config *Config, schema *ast.Schema) *generator { } func (g *generator) Types() string { - defs := make([]string, 0, len(g.typeMap)) - for _, def := range g.typeMap { - defs = append(defs, def) + names := make([]string, 0, len(g.typeMap)) + for name := range g.typeMap { + names = append(names, name) + } + // Sort alphabetically by type-name. Sorting somehow deterministically is + // important to ensure generated code is deterministic. Alphabetical is + // nice because it's easy, and in the current naming scheme, it's even + // vaguely aligned to the structure of the queries. + sort.Strings(names) + + defs := make([]string, 0, len(g.typeMap)) + for _, name := range names { + defs = append(defs, g.typeMap[name]) } - // Make sure we have a stable order. (It's somewhat - // arbitrary but in practice mostly alphabetical.) - // TODO: ideally we'd do a nice semantic ordering. - sort.Strings(defs) return strings.Join(defs, "\n\n") } @@ -84,7 +90,7 @@ func (g *generator) getArgument(arg *ast.VariableDefinition) (argument, error) { func (g *generator) getDocComment(op *ast.OperationDefinition) string { var commentLines []string - var sourceLines = strings.Split(op.Position.Src.Input, "\n") + sourceLines := strings.Split(op.Position.Src.Input, "\n") for i := op.Position.Line - 1; i > 0; i-- { line := sourceLines[i-1] if strings.HasPrefix(line, "#") { diff --git a/generate/parse.go b/generate/parse.go index 51f3e5e..62ad944 100644 --- a/generate/parse.go +++ b/generate/parse.go @@ -11,9 +11,6 @@ import ( ) func getSchema(filename string) (*ast.Schema, error) { - // TODO: IRL we have to get the schema from GraphQL (maybe we can generate - // that once we can bootstrap) where it comes as JSON, not SDL, so we have - // to convert (or add gqlparser support to convert) text, err := ioutil.ReadFile(filename) if err != nil { return nil, fmt.Errorf("unreadable schema file %v: %v", filename, err) diff --git a/generate/template.go b/generate/template.go index 95ca779..2901bf8 100644 --- a/generate/template.go +++ b/generate/template.go @@ -7,8 +7,11 @@ import ( ) // TODO: package templates into the binary using one of those asset thingies -var _, thisFilename, _, _ = runtime.Caller(0) -var thisDir = filepath.Dir(thisFilename) +// (e.g. embed, if we wait until 1.16 to do this) +var ( + _, thisFilename, _, _ = runtime.Caller(0) + thisDir = filepath.Dir(thisFilename) +) func mustTemplate(relFilename string) *template.Template { return template.Must(template.ParseFiles(filepath.Join(thisDir, relFilename))) diff --git a/generate/types.go b/generate/types.go index 23fdc0c..846301a 100644 --- a/generate/types.go +++ b/generate/types.go @@ -29,12 +29,11 @@ func (g *generator) baseTypeForOperation(operation ast.Operation) *ast.Definitio func (g *generator) getTypeForOperation(operation *ast.OperationDefinition) (name string, err error) { // TODO: configure ResponseName format - namePrefix := upperFirst(operation.Name) - name = namePrefix + "Response" + name = operation.Name + "Response" if def, ok := g.typeMap[name]; ok { // TODO: check for and handle conflicts a better way - return "", fmt.Errorf("%s already defined:\n%s", name, def) + return "", fmt.Errorf("%s defined twice:\n%s", name, def) } fields, err := selections(operation.SelectionSet) @@ -43,15 +42,16 @@ func (g *generator) getTypeForOperation(operation *ast.OperationDefinition) (nam } return g.addTypeForDefinition( - namePrefix, name, g.baseTypeForOperation(operation.Operation), fields) + operation.Name, name, g.baseTypeForOperation(operation.Operation), fields) } var builtinTypes = map[string]string{ - "Int": "int", // TODO: technically int32 is always enough, use that? + // GraphQL guarantees int32 is enough, but using int seems more idiomatic + "Int": "int", "Float": "float64", "String": "string", "Boolean": "bool", - "ID": "string", // TODO: named type for IDs? + "ID": "string", } func (g *generator) addTypeForDefinition(namePrefix, nameOverride string, typ *ast.Definition, fields []field) (name string, err error) { @@ -117,11 +117,9 @@ type field interface { type outputField struct{ field *ast.Field } func (s outputField) Alias() string { - if s.field.Alias != "" { - return s.field.Alias - } - // TODO: is this case needed? tests don't seem to get here. - return s.field.Name + // gqlparser sets Alias even if the field is not aliased, see e.g. + // https://github.com/vektah/gqlparser/blob/c06d8e0d135f285e37e7f1ff397f10e049733eb3/parser/query.go#L150 + return s.field.Alias } func (s outputField) Type() *ast.Type { @@ -192,11 +190,11 @@ func (builder *typeBuilder) writeField(field field) error { } err = builder.writeType( - // Note we don't deduplicate here -- if our prefix is GetUser and the - // field name is User, we do GetUserUser. This is important because if - // you have a field called user on a type called User we need - // `query q { user { user { id } } }` to generate two types, QUser and - // QUserUser. + // Note we don't deduplicate suffixes here -- if our prefix is GetUser + // and the field name is User, we do GetUserUser. This is important + // because if you have a field called user on a type called User we + // need `query q { user { user { id } } }` to generate two types, QUser + // and QUserUser. // Note also this is the alias, not the field-name, because if we have // `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 @@ -273,7 +271,6 @@ func (builder *typeBuilder) writeTypedef(typedef *ast.Definition, fields []field return err } - // HACK HACK HACK builder.typeMap[name] += fmt.Sprintf( "\nfunc (v %v) %v() {}", name, implementsMethodName) } @@ -285,7 +282,6 @@ func (builder *typeBuilder) writeTypedef(typedef *ast.Definition, fields []field builder.WriteString("string\n") builder.WriteString("const (\n") for _, val := range typedef.EnumValues { - // TODO: casing should be configurable fmt.Fprintf(builder, "%s %s = \"%s\"\n", builder.typeNamePrefix+goConstName(val.Name), builder.typeName, val.Name) diff --git a/generate/unmarshal.go b/generate/unmarshal.go index 5356426..4d1a246 100644 --- a/generate/unmarshal.go +++ b/generate/unmarshal.go @@ -33,7 +33,7 @@ func (builder *typeBuilder) maybeWriteUnmarshal(fields []field) error { for _, typedef := range builder.schema.GetPossibleTypes(typedef) { fieldInfo.ConcreteTypes = append(fieldInfo.ConcreteTypes, concreteType{ - // TODO: this is quite fragile (and maybe wrong if the + // TODO: this is quite fragile (and wrong if the // field name + type name are the same) GoName: builder.typeNamePrefix + fieldInfo.GoName + upperFirst(typedef.Name), GraphQLName: typedef.Name, diff --git a/graphql/client.go b/graphql/client.go index 8fc76fb..9e7d1db 100644 --- a/graphql/client.go +++ b/graphql/client.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" "net/http" "github.com/vektah/gqlparser/gqlerror" @@ -29,8 +28,12 @@ type payload struct { Variables map[string]interface{} `json:"variables"` } +type response struct { + Data json.RawMessage `json:"data"` + Errors gqlerror.List `json:"errors"` +} + func (client *Client) MakeRequest(ctx context.Context, query string, retval interface{}, variables map[string]interface{}) error { - // TODO: streaming reads and writes body, err := json.Marshal(payload{ Query: query, Variables: variables, @@ -54,21 +57,12 @@ func (client *Client) MakeRequest(ctx context.Context, query string, retval inte } defer resp.Body.Close() - body, err = ioutil.ReadAll(resp.Body) - if err != nil { - return err - } - if resp.StatusCode != http.StatusOK { return fmt.Errorf("returned error %v: %v", resp.Status, string(body)) } - var dataAndErrors struct { - Data json.RawMessage `json:"data"` - Errors gqlerror.List `json:"errors"` - } - - err = json.Unmarshal(body, &dataAndErrors) + var dataAndErrors response + err = json.NewDecoder(resp.Body).Decode(&dataAndErrors) if err != nil { return err }