Skip to content

Commit

Permalink
Reject operation or argument names that are Go keywords (#195)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
benjaminjkraft authored May 12, 2022
1 parent 482a59b commit 39cd158
Show file tree
Hide file tree
Showing 13 changed files with 57 additions and 0 deletions.
2 changes: 2 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions generate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions generate/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
3 changes: 3 additions & 0 deletions generate/testdata/errors/KeywordArgumentName.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
query KeywordArgumentName($type: String) {
f(type: $type)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
type Query { f(type: String): String }
1 change: 1 addition & 0 deletions generate/testdata/errors/KeywordOperationName.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
query type { f }
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
type Query { f: String }
6 changes: 6 additions & 0 deletions generate/testdata/errors/KeywordTypeName.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
query KeywordTypeName {
# @genqlient(typename: "type")
f {
g
}
}
2 changes: 2 additions & 0 deletions generate/testdata/errors/KeywordTypeName.schema.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
type Query { f: T }
type T { g: String }
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
testdata/errors/KeywordArgumentName.graphql:1: variable name must not be a go keyword
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
testdata/errors/KeywordOperationName.graphql:1: operation name must not be a go keyword
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
testdata/errors/KeywordTypeName.schema.graphql:1: typename option must not be a go keyword
29 changes: 29 additions & 0 deletions generate/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

0 comments on commit 39cd158

Please sign in to comment.