From 39cd158d3316bcb4dacdd85425d6937e9aca2530 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Thu, 12 May 2022 17:00:55 -0500 Subject: [PATCH] Reject operation or argument names that are Go keywords (#195) GraphQL is pretty restrictive about its identifiers, so for the most part we can and do safely use GraphQL identifiers in the Go we generate with attention only to conflicts with other such identifiers. But we do need to check one thing, which is that the identifier isn't a Go keyword. (If it is, the generated code will almost certainly fail to compile, but often with a confusing error message.) In this commit I add such checks. The most likely place to run into trouble here is argument names, which are often one word and are used as-is. Operation names, if unexported, can also be keywords, although in practice they're usually multiword. Field names are always exported, thus safe. Generated type names are always prefixed, camel-cased, with the operation name, so they always contain an uppercase letter (even if the operation name is lowercase), but type-names specified by `typename` may collide, so we check those. In theory we could check type-names specified by `bind`, but these must be defined by the user, so their code will already fail to compile, so I didn't bother. I think that's all the places to consider, although it's hard to be sure. In summary, we check argument names, operation names, and user-specified type names. Test plan: make check --- docs/CHANGELOG.md | 2 ++ generate/convert.go | 7 +++++ generate/generate.go | 2 ++ .../errors/KeywordArgumentName.graphql | 3 ++ .../errors/KeywordArgumentName.schema.graphql | 1 + .../errors/KeywordOperationName.graphql | 1 + .../KeywordOperationName.schema.graphql | 1 + .../testdata/errors/KeywordTypeName.graphql | 6 ++++ .../errors/KeywordTypeName.schema.graphql | 2 ++ ...GenerateErrors-KeywordArgumentName-graphql | 1 + ...enerateErrors-KeywordOperationName-graphql | 1 + ...TestGenerateErrors-KeywordTypeName-graphql | 1 + generate/util.go | 29 +++++++++++++++++++ 13 files changed, 57 insertions(+) create mode 100644 generate/testdata/errors/KeywordArgumentName.graphql create mode 100644 generate/testdata/errors/KeywordArgumentName.schema.graphql create mode 100644 generate/testdata/errors/KeywordOperationName.graphql create mode 100644 generate/testdata/errors/KeywordOperationName.schema.graphql create mode 100644 generate/testdata/errors/KeywordTypeName.graphql create mode 100644 generate/testdata/errors/KeywordTypeName.schema.graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-KeywordArgumentName-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-KeywordOperationName-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-KeywordTypeName-graphql diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 5f629152..922ea665 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -34,6 +34,8 @@ When releasing a new version: ### Bug fixes: +- genqlient now explicitly rejects argument, operation, and type names which are Go keywords, rather than failing with an opaque error. + ## v0.4.0 Version 0.4.0 adds several new configuration options, as well as additional methods to simplify the use of interfaces. diff --git a/generate/convert.go b/generate/convert.go index fb256a2c..bde539d7 100644 --- a/generate/convert.go +++ b/generate/convert.go @@ -164,6 +164,10 @@ func (g *generator) convertArguments( name := "__" + operation.Name + "Input" fields := make([]*goStructField, len(operation.VariableDefinitions)) for i, arg := range operation.VariableDefinitions { + if goKeywords[arg.Variable] { + return nil, errorf(arg.Position, "variable name must not be a go keyword") + } + _, options, err := g.parsePrecedingComment(arg, nil, arg.Position, queryOptions) if err != nil { return nil, err @@ -317,6 +321,9 @@ func (g *generator) convertDefinition( // Determine the name to use for this type. var name string if options.TypeName != "" { + if goKeywords[options.TypeName] { + return nil, errorf(pos, "typename option must not be a go keyword") + } // If the user specified a name, use it! name = options.TypeName if namePrefix != nil && namePrefix.head == name && namePrefix.tail == nil { diff --git a/generate/generate.go b/generate/generate.go index 3c412870..1ab34dad 100644 --- a/generate/generate.go +++ b/generate/generate.go @@ -232,6 +232,8 @@ func (g *generator) preprocessQueryDocument(doc *ast.QueryDocument) { func (g *generator) addOperation(op *ast.OperationDefinition) error { if op.Name == "" { return errorf(op.Position, "operations must have operation-names") + } else if goKeywords[op.Name] { + return errorf(op.Position, "operation name must not be a go keyword") } queryDoc := &ast.QueryDocument{ diff --git a/generate/testdata/errors/KeywordArgumentName.graphql b/generate/testdata/errors/KeywordArgumentName.graphql new file mode 100644 index 00000000..6f3ccf5f --- /dev/null +++ b/generate/testdata/errors/KeywordArgumentName.graphql @@ -0,0 +1,3 @@ +query KeywordArgumentName($type: String) { + f(type: $type) +} diff --git a/generate/testdata/errors/KeywordArgumentName.schema.graphql b/generate/testdata/errors/KeywordArgumentName.schema.graphql new file mode 100644 index 00000000..ee2bd1f2 --- /dev/null +++ b/generate/testdata/errors/KeywordArgumentName.schema.graphql @@ -0,0 +1 @@ +type Query { f(type: String): String } diff --git a/generate/testdata/errors/KeywordOperationName.graphql b/generate/testdata/errors/KeywordOperationName.graphql new file mode 100644 index 00000000..cb07dd0a --- /dev/null +++ b/generate/testdata/errors/KeywordOperationName.graphql @@ -0,0 +1 @@ +query type { f } diff --git a/generate/testdata/errors/KeywordOperationName.schema.graphql b/generate/testdata/errors/KeywordOperationName.schema.graphql new file mode 100644 index 00000000..75cab938 --- /dev/null +++ b/generate/testdata/errors/KeywordOperationName.schema.graphql @@ -0,0 +1 @@ +type Query { f: String } diff --git a/generate/testdata/errors/KeywordTypeName.graphql b/generate/testdata/errors/KeywordTypeName.graphql new file mode 100644 index 00000000..293fe27e --- /dev/null +++ b/generate/testdata/errors/KeywordTypeName.graphql @@ -0,0 +1,6 @@ +query KeywordTypeName { + # @genqlient(typename: "type") + f { + g + } +} diff --git a/generate/testdata/errors/KeywordTypeName.schema.graphql b/generate/testdata/errors/KeywordTypeName.schema.graphql new file mode 100644 index 00000000..41e7814d --- /dev/null +++ b/generate/testdata/errors/KeywordTypeName.schema.graphql @@ -0,0 +1,2 @@ +type Query { f: T } +type T { g: String } diff --git a/generate/testdata/snapshots/TestGenerateErrors-KeywordArgumentName-graphql b/generate/testdata/snapshots/TestGenerateErrors-KeywordArgumentName-graphql new file mode 100644 index 00000000..b87f4198 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-KeywordArgumentName-graphql @@ -0,0 +1 @@ +testdata/errors/KeywordArgumentName.graphql:1: variable name must not be a go keyword diff --git a/generate/testdata/snapshots/TestGenerateErrors-KeywordOperationName-graphql b/generate/testdata/snapshots/TestGenerateErrors-KeywordOperationName-graphql new file mode 100644 index 00000000..d09ce4c7 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-KeywordOperationName-graphql @@ -0,0 +1 @@ +testdata/errors/KeywordOperationName.graphql:1: operation name must not be a go keyword diff --git a/generate/testdata/snapshots/TestGenerateErrors-KeywordTypeName-graphql b/generate/testdata/snapshots/TestGenerateErrors-KeywordTypeName-graphql new file mode 100644 index 00000000..a0aab9ed --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-KeywordTypeName-graphql @@ -0,0 +1 @@ +testdata/errors/KeywordTypeName.schema.graphql:1: typename option must not be a go keyword diff --git a/generate/util.go b/generate/util.go index ed745819..c424c7f1 100644 --- a/generate/util.go +++ b/generate/util.go @@ -46,3 +46,32 @@ func goConstName(s string) string { return ret }, s) } + +// https://go.dev/ref/spec#Keywords +var goKeywords = map[string]bool{ + "break": true, + "default": true, + "func": true, + "interface": true, + "select": true, + "case": true, + "defer": true, + "go": true, + "map": true, + "struct": true, + "chan": true, + "else": true, + "goto": true, + "package": true, + "switch": true, + "const": true, + "fallthrough": true, + "if": true, + "range": true, + "type": true, + "continue": true, + "for": true, + "import": true, + "return": true, + "var": true, +}