From 2272ad8a12600b612efa950d97518bc9a76910cf Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Wed, 21 Apr 2021 11:32:49 -0700 Subject: [PATCH] allow map[string]interface{}, and other such things, as scalars --- generate/generate_test.go | 7 +-- generate/imports.go | 54 ++++++++++++++----- .../testdata/errors/InvalidScalar.go.error | 2 +- .../errors/InvalidScalar.graphql.error | 2 +- .../testdata/queries/EmptyInterface.graphql | 2 +- .../queries/EmptyInterface.graphql.go | 4 +- .../queries/EmptyInterface.graphql.json | 2 +- generate/testdata/queries/schema.graphql | 3 +- 8 files changed, 54 insertions(+), 22 deletions(-) diff --git a/generate/generate_test.go b/generate/generate_test.go index 2c0a0db..8774b26 100644 --- a/generate/generate_test.go +++ b/generate/generate_test.go @@ -98,9 +98,10 @@ func TestGenerate(t *testing.T) { Generated: goFilename, ExportOperations: queriesFilename, Scalars: map[string]string{ - "ID": "github.com/me/mypkg.ID", - "DateTime": "time.Time", - "Junk": "interface{}", + "ID": "github.com/me/mypkg.ID", + "DateTime": "time.Time", + "Junk": "interface{}", + "ComplexJunk": "[]map[string]*[]*map[string]interface{}", }, }) if err != nil { diff --git a/generate/imports.go b/generate/imports.go index 73288a0..67f38f5 100644 --- a/generate/imports.go +++ b/generate/imports.go @@ -1,7 +1,9 @@ package generate import ( + "fmt" "go/types" + "regexp" "strconv" "strings" ) @@ -35,24 +37,49 @@ func (g *generator) ref(fullyQualifiedName string) (qualifiedName string, err er return g.getRef(fullyQualifiedName, false) } +var _sliceOrMapPrefixRegexp = regexp.MustCompile(`^(\*|\[\d*\]|map\[string\])*`) + func (g *generator) getRef(fullyQualifiedName string, addImport bool) (qualifiedName string, err error) { - i := strings.LastIndex(fullyQualifiedName, ".") + // Ideally, we want to allow a reference to basically an arbitrary symbol. + // But that's very hard, because it might be quite complicated, like + // struct{ F []map[mypkg.K]otherpkg.V } + // Now in practice, using an unnamed struct is not a great idea, but we do + // want to allow as much as we can that encoding/json knows how to work + // with, since you would reasonably expect us to accept, say, + // map[string][]interface{}. So we allow: + // - any named type (mypkg.T) + // - any predeclared basic type (string, int, etc.) + // - interface{} + // - for any allowed type T, *T, []T, [N]T, and map[string]T + // which effectively excludes: + // - unnamed struct types + // - map[K]V where K is a named type wrapping string + // - any nonstandard spelling of those (interface {/* hi */}, + // map[ string ]T) + // TODO: document that somewhere visible + + errorMsg := `invalid type-name "%v" (%v); expected a builtin, ` + + `path/to/package.Name, interface{}, or a slice, map, or pointer of those` + + if strings.Contains(fullyQualifiedName, " ") { + // TODO: pass in pos here and below + return "", errorf(nil, errorMsg, fullyQualifiedName, "contains spaces") + } + + prefix := _sliceOrMapPrefixRegexp.FindString(fullyQualifiedName) + nameToImport := fullyQualifiedName[len(prefix):] + + i := strings.LastIndex(nameToImport, ".") if i == -1 { - // We allow any builtin type, or interface{}. In principle it would be - // fine to allow any interface or struct, but (1) they might refer to a - // type that needs an import, and (2) that just honestly seems - // confusing, why would you want it. But the empty interface, - // specifically, is useful. - if fullyQualifiedName != "interface{}" && types.Universe.Lookup(fullyQualifiedName) == nil { - // TODO: pass in pos here - return "", errorf(nil, - `unknown name "%v"; expected a builtin or path/to/package.Name`, fullyQualifiedName) + if nameToImport != "interface{}" && types.Universe.Lookup(nameToImport) == nil { + return "", errorf(nil, errorMsg, fullyQualifiedName, + fmt.Sprintf(`unknown type-name "%v"`, nameToImport)) } return fullyQualifiedName, nil } - pkgPath := fullyQualifiedName[:i] - localName := fullyQualifiedName[i+1:] + pkgPath := nameToImport[:i] + localName := nameToImport[i+1:] var alias string if addImport { alias = g.addImportFor(pkgPath) @@ -60,10 +87,11 @@ func (g *generator) getRef(fullyQualifiedName string, addImport bool) (qualified var ok bool alias, ok = g.imports[pkgPath] if !ok { + // This is an internal error, not a user error. return "", errorf(nil, `no alias defined for package "%v"`, pkgPath) } } - return alias + "." + localName, nil + return prefix + alias + "." + localName, nil } // Returns the import-clause to use in the generated code. diff --git a/generate/testdata/errors/InvalidScalar.go.error b/generate/testdata/errors/InvalidScalar.go.error index 5ab3aa0..fe1b1c8 100644 --- a/generate/testdata/errors/InvalidScalar.go.error +++ b/generate/testdata/errors/InvalidScalar.go.error @@ -1 +1 @@ -unknown name "bogus"; expected a builtin or path/to/package.Name \ No newline at end of file +invalid type-name "bogus" (unknown type-name "bogus"); expected a builtin, path/to/package.Name, interface{}, or a slice, map, or pointer of those \ No newline at end of file diff --git a/generate/testdata/errors/InvalidScalar.graphql.error b/generate/testdata/errors/InvalidScalar.graphql.error index 5ab3aa0..fe1b1c8 100644 --- a/generate/testdata/errors/InvalidScalar.graphql.error +++ b/generate/testdata/errors/InvalidScalar.graphql.error @@ -1 +1 @@ -unknown name "bogus"; expected a builtin or path/to/package.Name \ No newline at end of file +invalid type-name "bogus" (unknown type-name "bogus"); expected a builtin, path/to/package.Name, interface{}, or a slice, map, or pointer of those \ No newline at end of file diff --git a/generate/testdata/queries/EmptyInterface.graphql b/generate/testdata/queries/EmptyInterface.graphql index 30ede1d..9295b19 100644 --- a/generate/testdata/queries/EmptyInterface.graphql +++ b/generate/testdata/queries/EmptyInterface.graphql @@ -1 +1 @@ -query EmptyInterface { getJunk } +query EmptyInterface { getJunk getComplexJunk } diff --git a/generate/testdata/queries/EmptyInterface.graphql.go b/generate/testdata/queries/EmptyInterface.graphql.go index 80cafd4..0a8702f 100644 --- a/generate/testdata/queries/EmptyInterface.graphql.go +++ b/generate/testdata/queries/EmptyInterface.graphql.go @@ -7,7 +7,8 @@ import ( ) type EmptyInterfaceResponse struct { - GetJunk interface{} `json:"getJunk"` + GetJunk interface{} `json:"getJunk"` + GetComplexJunk []map[string]*[]*map[string]interface{} `json:"getComplexJunk"` } func EmptyInterface( @@ -20,6 +21,7 @@ func EmptyInterface( ` query EmptyInterface { getJunk + getComplexJunk } `, &retval, diff --git a/generate/testdata/queries/EmptyInterface.graphql.json b/generate/testdata/queries/EmptyInterface.graphql.json index cb71e68..b4186f5 100644 --- a/generate/testdata/queries/EmptyInterface.graphql.json +++ b/generate/testdata/queries/EmptyInterface.graphql.json @@ -2,7 +2,7 @@ "operations": [ { "operationName": "EmptyInterface", - "query": "\nquery EmptyInterface {\n\tgetJunk\n}\n", + "query": "\nquery EmptyInterface {\n\tgetJunk\n\tgetComplexJunk\n}\n", "sourceLocation": "testdata/queries/EmptyInterface.graphql" } ] diff --git a/generate/testdata/queries/schema.graphql b/generate/testdata/queries/schema.graphql index 5e334bc..a177f6c 100644 --- a/generate/testdata/queries/schema.graphql +++ b/generate/testdata/queries/schema.graphql @@ -1,6 +1,6 @@ scalar DateTime - scalar Junk +scalar ComplexJunk enum Role { STUDENT @@ -67,6 +67,7 @@ type Query { convert(dt: DateTime!, tz: String): DateTime! maybeConvert(dt: DateTime, tz: String): DateTime getJunk: Junk + getComplexJunk: ComplexJunk listOfListsOfLists: [[[String!]!]!]! }