From 67f2575caea6c0f38c10a85e56c76b77e9c29e05 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Wed, 9 Feb 2022 17:54:31 -0700 Subject: [PATCH] Reject the use of both typename and bind (#172) In #133, Craig added support for a new use of typename, where it applies to a scalar and means that genqlient should generate a named type, e.g. `# @genqlient(typename: "MyString")` on a node of type string will generate and use `type MyString string`. But this gets a bit confusing if you mix it with `bind`; should `typename: "MyString", bind: "int32"` generate `type MyString int32`, or should one override the other, or what? Of course in practice you're not likely to write that all in one place, but you could via a global binding, or a `for` directive, and in that case probably it was a mistake. In #138, we looked at making them work together correctly, but it added complexity and got even more confusing. So instead, here, we just ban it; we can always add it back if it proves useful. (Or, you can make the `typename` win over a global binding by locally unbinding it via `bind: "-"`.) This required changes in surprisingly many places; I already knew the directive-validation code was due for a refactor but that will happen some other day. The tests show that it works, in any case. Interestingly, this problem actually could have arisen for a struct binding already, before #133. But all the same reasons it's confusing seem to apply, so I just banned it there too. This is technically a breaking change although I doubt anyone will hit it. Test plan: make check --- docs/CHANGELOG.md | 1 + docs/genqlient_directive.graphql | 4 ++++ generate/convert.go | 5 +++++ generate/genqlient_directive.go | 21 +++++++++++++++++++ ...ConflictingTypeNameAndForFieldBind.graphql | 7 +++++++ ...tingTypeNameAndForFieldBind.schema.graphql | 8 +++++++ .../ConflictingTypeNameAndGlobalBind.graphql | 8 +++++++ ...ictingTypeNameAndGlobalBind.schema.graphql | 10 +++++++++ .../ConflictingTypeNameAndLocalBind.graphql | 6 ++++++ ...lictingTypeNameAndLocalBind.schema.graphql | 8 +++++++ ...ConflictingTypeNameAndForFieldBind-graphql | 1 + ...s-ConflictingTypeNameAndGlobalBind-graphql | 1 + ...rs-ConflictingTypeNameAndLocalBind-graphql | 1 + 13 files changed, 81 insertions(+) create mode 100644 generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql create mode 100644 generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql create mode 100644 generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql create mode 100644 generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql create mode 100644 generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql create mode 100644 generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f1bd00d6..52aafc0e 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -23,6 +23,7 @@ When releasing a new version: ### Breaking changes: - The `Config` fields `Schema` and `Operations` are now both of type `StringList`. This does not affect configuration via `genqlient.yaml`, only via the Go API. +- The `typename` and `bind` options may no longer be combined; doing so will now result in an error. In practice, any such use was likely in error (and the rules for which would win were confusing and undocumented). ### New features: diff --git a/docs/genqlient_directive.graphql b/docs/genqlient_directive.graphql index 57e8b2fc..e06945df 100644 --- a/docs/genqlient_directive.graphql +++ b/docs/genqlient_directive.graphql @@ -226,6 +226,10 @@ directive genqlient( # Note that unlike most directives, if applied to the entire operation, # typename affects the overall response type, rather than being propagated # down to all child fields (which would cause conflicts). + # + # To avoid confusion, typename may not be combined with local or global + # bindings; to use typename instead of a global binding do + # `typename: "MyTypeName", bind: "-"`. typename: String # Multiple genqlient directives are allowed in the same location, as long as diff --git a/generate/convert.go b/generate/convert.go index d4a21f1f..fb256a2c 100644 --- a/generate/convert.go +++ b/generate/convert.go @@ -289,6 +289,11 @@ func (g *generator) convertDefinition( // unless the binding is "-" which means "ignore the global binding". globalBinding, ok := g.Config.Bindings[def.Name] if ok && options.Bind != "-" { + if options.TypeName != "" { + return nil, errorf(pos, + "typename option conflicts with global binding for %s; "+ + "use `bind: \"-\"` to override it", def.Name) + } if def.Kind == ast.Object || def.Kind == ast.Interface || def.Kind == ast.Union { err := g.validateBindingSelection( def.Name, globalBinding, pos, selectionSet) diff --git a/generate/genqlient_directive.go b/generate/genqlient_directive.go index 06bc24b4..886215a2 100644 --- a/generate/genqlient_directive.go +++ b/generate/genqlient_directive.go @@ -213,6 +213,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er if fieldDir.Omitempty != nil && field.Type.NonNull { return errorf(fieldDir.pos, "omitempty may only be used on optional arguments") } + + if fieldDir.TypeName != "" && fieldDir.Bind != "" && fieldDir.Bind != "-" { + return errorf(fieldDir.pos, "typename and bind may not be used together") + } } } @@ -255,6 +259,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er return errorf(dir.pos, "for is only applicable to operations and arguments") } + if dir.TypeName != "" && dir.Bind != "" && dir.Bind != "-" { + return errorf(dir.pos, "typename and bind may not be used together") + } + return nil case *ast.Field: if dir.Omitempty != nil { @@ -278,6 +286,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er return errorf(dir.pos, "for is only applicable to operations and arguments") } + if dir.TypeName != "" && dir.Bind != "" && dir.Bind != "-" { + return errorf(dir.pos, "typename and bind may not be used together") + } + return nil default: return errorf(dir.pos, "invalid @genqlient directive location: %T", node) @@ -490,6 +502,15 @@ func (g *generator) parsePrecedingComment( if queryOptions != nil { // If we are part of an operation/fragment, merge its options in. directive.mergeOperationDirective(node, parentIfInputField, queryOptions) + + // TODO(benkraft): Really we should do all the validation after + // merging, probably? But this is the only check that can fail only + // after merging, and it's a bit tricky because the "does not apply" + // checks may need to happen before merging so we know where the + // directive "is". + if directive.TypeName != "" && directive.Bind != "" && directive.Bind != "-" { + return "", nil, errorf(directive.pos, "typename and bind may not be used together") + } } reverse(commentLines) diff --git a/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql new file mode 100644 index 00000000..9bf64843 --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql @@ -0,0 +1,7 @@ +# @genqlient(for: "User.name", bind: "[]byte") +query ConflictingTypeNameAndGlobalBind { + user { + # @genqlient(typename: "MyID") + name + } +} diff --git a/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql new file mode 100644 index 00000000..ac8ac540 --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql @@ -0,0 +1,8 @@ +type Query { + user: User +} + +type User { + id: ID! + name: String! +} diff --git a/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql new file mode 100644 index 00000000..34910729 --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql @@ -0,0 +1,8 @@ +query ConflictingTypeNameAndGlobalBind { + user { + # @genqlient(typename: "OtherScalar") + name + } +} + + diff --git a/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql new file mode 100644 index 00000000..42dfe463 --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql @@ -0,0 +1,10 @@ +type Query { + user: User +} + +type User { + id: ID! + name: ValidScalar! +} + +scalar ValidScalar diff --git a/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql new file mode 100644 index 00000000..adb8fbed --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql @@ -0,0 +1,6 @@ +query ConflictingTypeNameAndGlobalBind { + user { + # @genqlient(bind: "[]byte", typename: "MyID") + name + } +} diff --git a/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql new file mode 100644 index 00000000..ac8ac540 --- /dev/null +++ b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql @@ -0,0 +1,8 @@ +type Query { + user: User +} + +type User { + id: ID! + name: String! +} diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql new file mode 100644 index 00000000..cfaee0f7 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql @@ -0,0 +1 @@ +testdata/errors/ConflictingTypeNameAndForFieldBind.graphql:5: typename and bind may not be used together diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql new file mode 100644 index 00000000..164bc4c5 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql @@ -0,0 +1 @@ +testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql:8: typename option conflicts with global binding for ValidScalar; use `bind: "-"` to override it diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql new file mode 100644 index 00000000..2ef277af --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql @@ -0,0 +1 @@ +testdata/errors/ConflictingTypeNameAndLocalBind.graphql:4: typename and bind may not be used together