From 72f8e073c92736982966fb8bcd50ac3a87018258 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Mon, 30 Aug 2021 10:45:09 -0700 Subject: [PATCH] Review comments, and a few more test cases --- generate/convert.go | 2 +- generate/names.go | 37 +++++++++++++++++++++------------ generate/names_test.go | 46 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/generate/convert.go b/generate/convert.go index 9f553155..b8ad0198 100644 --- a/generate/convert.go +++ b/generate/convert.go @@ -54,7 +54,7 @@ func (g *generator) convertOperation( // thing, because we want to do a few things differently, and because we // know we have an object type, so we can include only that case. fields, err := g.convertSelectionSet( - &prefixList{last: operation.Name}, operation.SelectionSet, baseType, queryOptions) + newPrefixList(operation.Name), operation.SelectionSet, baseType, queryOptions) if err != nil { return nil, err } diff --git a/generate/names.go b/generate/names.go index 13d3b298..e19419aa 100644 --- a/generate/names.go +++ b/generate/names.go @@ -65,18 +65,21 @@ package generate // in `query Q { user { user { id } } }` we do need both QUser and QUserUser -- // they have different fields. // -// Note that there are (at least) two potential collisions from this algorithm: +// Note that there are a few potential collisions from this algorithm: // - When generating Go types for GraphQL interface types, we generate both // ...MyFieldMyInterfaceType and ...MyFieldMyImplType. If an interface's // name is a suffix of its implementation's name, and both are suffixes of a // field of that type, we'll shorten both, resulting in a collision. -// - Given a query like +// - Names of different casing (e.g. fields `myField` and `MyField`) can +// collide (the first is standard usage but both are legal). +// - We don't put a special character between parts, so fields like // query Q { -// ab { ... } # type: C -// a { ... } # type: BC +// ab { ... } # type: C +// abc { ... } # type: C +// a { ... } # type: BC // } -// we generate QABC to represent both fields. -// Both cases seem fairly rare in practice; eventually we'll likely allow users +// can collide. +// All cases seem fairly rare in practice; eventually we'll likely allow users // the ability to specify their own names, which they could use to avoid this // (see https://github.com/Khan/genqlient/issues/12). // TODO(benkraft): We should probably at least try to detect it and bail. @@ -99,19 +102,27 @@ import ( "github.com/vektah/gqlparser/v2/ast" ) -// Yes, a linked list! We could use a stack -- it would probably be marginally +// Yes, a linked list! Of name-prefixes in *reverse* order, i.e. from end to +// start. +// +// We could use a stack -- it would probably be marginally // more efficient -- but then the caller would have to know more about how to // manage it safely. Using a list, and treating it as immutable, makes it // easy. type prefixList struct { - last string // the list goes back-to-front, so this is the *last* prefix - rest *prefixList + head string // the list goes back-to-front, so this is the *last* prefix + tail *prefixList +} + +// creates a new one-element list +func newPrefixList(item string) *prefixList { + return &prefixList{head: item} } func joinPrefixList(prefix *prefixList) string { var reversed []string - for ; prefix != nil; prefix = prefix.rest { - reversed = append(reversed, prefix.last) + for ; prefix != nil; prefix = prefix.tail { + reversed = append(reversed, prefix.head) } l := len(reversed) for i := 0; i < l/2; i++ { @@ -131,10 +142,10 @@ func typeNameParts(prefix *prefixList, typeName string) *prefixList { typeName = upperFirst(typeName) // If the prefix has just one part, that's the operation-name. There's no // need to add "Query" or "Mutation". (Zero should never happen.) - if prefix == nil || prefix.rest == nil || + if prefix == nil || prefix.tail == nil || // If the last prefix field ends with this type's name, omit the // type-name (see the "shortened" case in the top-of-file comment). - strings.HasSuffix(prefix.last, typeName) { + strings.HasSuffix(prefix.head, typeName) { return prefix } return &prefixList{typeName, prefix} diff --git a/generate/names_test.go b/generate/names_test.go index bb87a539..befb6559 100644 --- a/generate/names_test.go +++ b/generate/names_test.go @@ -27,6 +27,16 @@ func TestTypeNames(t *testing.T) { "OperationUser", []*ast.Field{fakeField("Query", "user")}, "User", + }, { + // We don't shorten field-names. + "OperationOperationUser", + []*ast.Field{fakeField("Query", "operationUser")}, + "User", + }, { + // We don't shorten across multiple prefixes. + "OperationUserOperationUser", + []*ast.Field{fakeField("Query", "user")}, + "OperationUser", }, { "OperationFavoriteUser", []*ast.Field{fakeField("Query", "favoriteUser")}, @@ -48,7 +58,7 @@ func TestTypeNames(t *testing.T) { for _, test := range tests { test := test t.Run(test.expectedTypeName, func(t *testing.T) { - prefix := &prefixList{last: "Operation"} + prefix := newPrefixList("Operation") for _, field := range test.fields { prefix = nextPrefix(prefix, field) } @@ -60,3 +70,37 @@ func TestTypeNames(t *testing.T) { }) } } + +func TestTypeNameCollisions(t *testing.T) { + tests := []struct { + fields []*ast.Field + leafTypeName string + }{ + {[]*ast.Field{fakeField("Query", "user")}, "UserInterface"}, + {[]*ast.Field{fakeField("Query", "user")}, "User"}, + {[]*ast.Field{fakeField("Query", "user")}, "QueryUser"}, + {[]*ast.Field{fakeField("Query", "queryUser")}, "User"}, + // Known issues, described in names.go file-documentation: + // Interface/implementation collision: + // {[]*ast.Field{fakeField("Query", "queryUser")}, "QueryUser"}, + // Case collision: + // {[]*ast.Field{fakeField("Query", "QueryUser")}, "User"}, + // Overlapping-parts collision: + // {[]*ast.Field{fakeField("Query", "userQuery")}, "User"}, + } + seen := map[string]int{} // name -> index of test that had it + for i, test := range tests { + prefix := newPrefixList("Operation") + for _, field := range test.fields { + prefix = nextPrefix(prefix, field) + } + actualTypeName := makeTypeName(prefix, test.leafTypeName) + + otherIndex, ok := seen[actualTypeName] + if ok { + t.Errorf("name collision:\ncase %2d: %#v\ncase %2d: %#v", + i, test, otherIndex, tests[otherIndex]) + } + seen[actualTypeName] = i + } +}