From 95609f77a1a7451e02a6a1a96a262650276a01d0 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Mon, 30 Aug 2021 10:18:52 -0700 Subject: [PATCH] If requested, validate binding-types get the right fields (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: One sharp edge of the new `bindings` setting (when used for composite types) is this: the (presumably struct) type to which you're binding may expect to have particular fields, but it's GraphQL so you could have requested some other set of fields. Now, if you ask us, we check. Specifically, I've added a new setting under the `bindings` items, which says: everywhere we query this must select these fields. (Or use its own inline `# @genqlient(bind: ...)`.) It must select exactly those fields, in order, no more, no less. This was fairly easy to implement; actually comparing the selections was surprisingly much code but it's all pretty straightforward. ## Test plan: make check Author: benjaminjkraft Reviewers: dnerdy, aberkan, csilvers, MiguelCastillo Required Reviewers: Approved by: dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint Pull request URL: https://github.com/Khan/genqlient/pull/70 --- generate/binding_validation.go | 111 ++++++++++++++++++ generate/config.go | 20 ++++ generate/convert.go | 7 ++ generate/generate_test.go | 25 +++- .../errors/BindingWithIncorrectSelection.go | 7 ++ .../BindingWithIncorrectSelection.graphql | 3 + ...ndingWithIncorrectSelection.schema.graphql | 8 ++ generate/testdata/queries/Pokemon.graphql | 3 +- ...ateErrors-BindingWithIncorrectSelection.go | 1 + ...rors-BindingWithIncorrectSelection.graphql | 1 + 10 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 generate/binding_validation.go create mode 100644 generate/testdata/errors/BindingWithIncorrectSelection.go create mode 100644 generate/testdata/errors/BindingWithIncorrectSelection.graphql create mode 100644 generate/testdata/errors/BindingWithIncorrectSelection.schema.graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.go create mode 100644 generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.graphql diff --git a/generate/binding_validation.go b/generate/binding_validation.go new file mode 100644 index 00000000..9f5f8e28 --- /dev/null +++ b/generate/binding_validation.go @@ -0,0 +1,111 @@ +package generate + +// This file is responsible for doing the validation for type-bindings, if they +// are so configured (see TypeBinding). + +import ( + "fmt" + + "github.com/vektah/gqlparser/v2/ast" + "github.com/vektah/gqlparser/v2/parser" +) + +// selectionsMatch recursively compares the two selection-sets, and returns an +// error if they differ. +// +// It does not check arguments and directives, only field names, aliases, +// order, and fragment-structure. It does not recurse into named fragments, it +// only checks that their names match. +// +// TODO(benkraft): Should we check arguments/directives? +func selectionsMatch( + pos *ast.Position, + expectedSelectionSet, actualSelectionSet ast.SelectionSet, +) error { + if len(expectedSelectionSet) != len(actualSelectionSet) { + return errorf( + pos, "expected %d fields, got %d", + len(expectedSelectionSet), len(actualSelectionSet)) + } + + for i, expected := range expectedSelectionSet { + switch expected := expected.(type) { + case *ast.Field: + actual, ok := actualSelectionSet[i].(*ast.Field) + switch { + case !ok: + return errorf(actual.Position, + "expected selection #%d to be field, got %T", + i, actualSelectionSet[i]) + case actual.Name != expected.Name: + return errorf(actual.Position, + "expected field %d to be %s, got %s", + i, expected.Name, actual.Name) + case actual.Alias != expected.Alias: + return errorf(actual.Position, + "expected field %d's alias to be %s, got %s", + i, expected.Alias, actual.Alias) + } + err := selectionsMatch(actual.Position, expected.SelectionSet, actual.SelectionSet) + if err != nil { + return fmt.Errorf("in %s sub-selection: %w", actual.Alias, err) + } + case *ast.InlineFragment: + actual, ok := actualSelectionSet[i].(*ast.InlineFragment) + switch { + case !ok: + return errorf(actual.Position, + "expected selection %d to be inline fragment, got %T", + i, actualSelectionSet[i]) + case actual.TypeCondition != expected.TypeCondition: + return errorf(actual.Position, + "expected fragment %d to be on type %s, got %s", + i, expected.TypeCondition, actual.TypeCondition) + } + err := selectionsMatch(actual.Position, expected.SelectionSet, actual.SelectionSet) + if err != nil { + return fmt.Errorf("in inline fragment on %s: %w", actual.TypeCondition, err) + } + case *ast.FragmentSpread: + actual, ok := actualSelectionSet[i].(*ast.FragmentSpread) + switch { + case !ok: + return errorf(actual.Position, + "expected selection %d to be fragment spread, got %T", + i, actualSelectionSet[i]) + case actual.Name != expected.Name: + return errorf(actual.Position, + "expected fragment %d to be ...%s, got ...%s", + i, expected.Name, actual.Name) + } + } + } + return nil +} + +// validateBindingSelection checks that if you requested in your type-binding +// that this type must always request certain fields, then in fact it does. +func (g *generator) validateBindingSelection( + typeName string, + binding *TypeBinding, + pos *ast.Position, + selectionSet ast.SelectionSet, +) error { + if binding.ExpectExactFields == "" { + return nil // no validation requested + } + + // HACK: we parse the selection as if it were a query, which is basically + // the same (for syntax purposes; it of course wouldn't validate) + doc, gqlErr := parser.ParseQuery(&ast.Source{Input: binding.ExpectExactFields}) + if gqlErr != nil { + return errorf( + nil, "invalid type-binding %s.expect_exact_fields: %w", typeName, gqlErr) + } + + err := selectionsMatch(pos, doc.Operations[0].SelectionSet, selectionSet) + if err != nil { + return fmt.Errorf("invalid selection for type-binding %s: %w", typeName, err) + } + return nil +} diff --git a/generate/config.go b/generate/config.go index 7ea3fcfa..08c2a0c1 100644 --- a/generate/config.go +++ b/generate/config.go @@ -104,6 +104,26 @@ type TypeBinding struct { // map[string]interface{} // github.com/you/yourpkg/subpkg.MyType Type string `yaml:"type"` + // If set, a GraphQL selection which must exactly match the fields + // requested whenever this type is used. Only applies if the GraphQL type + // is a composite output type (object, interface, or union). + // + // This is useful if Type is a struct whose UnmarshalJSON or other methods + // expect that you requested certain fields. You can specify those fields + // like + // MyType: + // type: path/to/my.GoType + // expect_exact_fields: "{ id name }" + // and then genqlient will reject if you make a query + // { fieldOfMytype { id title } } + // The fields must match exactly, including the ordering: "{ name id }" + // will be rejected. But the arguments and directives, if any, need not + // match. + // + // TODO(benkraft): Also add ExpectIncludesFields and ExpectSubsetOfFields, + // or something, if you want to say, for example, that you have to request + // certain fields but others are optional. + ExpectExactFields string `yaml:"expect_exact_fields"` } // baseDir returns the directory of the config-file (relative to which diff --git a/generate/convert.go b/generate/convert.go index 8d471176..d1531b84 100644 --- a/generate/convert.go +++ b/generate/convert.go @@ -190,6 +190,13 @@ 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 def.Kind == ast.Object || def.Kind == ast.Interface || def.Kind == ast.Union { + err := g.validateBindingSelection( + name, globalBinding, pos, selectionSet) + if err != nil { + return nil, err + } + } goRef, err := g.addRef(globalBinding.Type) return &goOpaqueType{goRef}, err } diff --git a/generate/generate_test.go b/generate/generate_test.go index a9091273..9c219153 100644 --- a/generate/generate_test.go +++ b/generate/generate_test.go @@ -52,11 +52,14 @@ func TestGenerate(t *testing.T) { Generated: goFilename, ExportOperations: queriesFilename, Bindings: map[string]*TypeBinding{ - "ID": {Type: "github.com/Khan/genqlient/internal/testutil.ID"}, - "DateTime": {Type: "time.Time"}, - "Junk": {Type: "interface{}"}, - "ComplexJunk": {Type: "[]map[string]*[]*map[string]interface{}"}, - "Pokemon": {Type: "github.com/Khan/genqlient/internal/testutil.Pokemon"}, + "ID": {Type: "github.com/Khan/genqlient/internal/testutil.ID"}, + "DateTime": {Type: "time.Time"}, + "Junk": {Type: "interface{}"}, + "ComplexJunk": {Type: "[]map[string]*[]*map[string]interface{}"}, + "Pokemon": { + Type: "github.com/Khan/genqlient/internal/testutil.Pokemon", + ExpectExactFields: "{ species level }", + }, "PokemonInput": {Type: "github.com/Khan/genqlient/internal/testutil.Pokemon"}, }, AllowBrokenFeatures: true, @@ -117,6 +120,14 @@ func TestGenerate(t *testing.T) { } } +// TestGenerate is a snapshot-based test of error text. +// +// For each .go or .graphql file in testdata/errors, and corresponding +// .schema.graphql file, it asserts that the given query returns an error, and +// that that error's string-text matches the snapshot. The snapshotting is +// useful to ensure we don't accidentally make the text less readable, drop the +// line numbers, etc. We include both .go and .graphql tests, to make sure the +// line numbers work in both cases. func TestGenerateErrors(t *testing.T) { files, err := ioutil.ReadDir(errorsDir) if err != nil { @@ -143,6 +154,10 @@ func TestGenerateErrors(t *testing.T) { Bindings: map[string]*TypeBinding{ "ValidScalar": {Type: "string"}, "InvalidScalar": {Type: "bogus"}, + "Pokemon": { + Type: "github.com/Khan/genqlient/internal/testutil.Pokemon", + ExpectExactFields: "{ species level }", + }, }, AllowBrokenFeatures: true, }) diff --git a/generate/testdata/errors/BindingWithIncorrectSelection.go b/generate/testdata/errors/BindingWithIncorrectSelection.go new file mode 100644 index 00000000..9b5ae1a2 --- /dev/null +++ b/generate/testdata/errors/BindingWithIncorrectSelection.go @@ -0,0 +1,7 @@ +package errors + +const _ = `# @genqlient +query GetPokemonWrongFields { + pokemon { species } +} +` diff --git a/generate/testdata/errors/BindingWithIncorrectSelection.graphql b/generate/testdata/errors/BindingWithIncorrectSelection.graphql new file mode 100644 index 00000000..38eba3d9 --- /dev/null +++ b/generate/testdata/errors/BindingWithIncorrectSelection.graphql @@ -0,0 +1,3 @@ +query GetPokemonWrongFields { + pokemon { species species } +} diff --git a/generate/testdata/errors/BindingWithIncorrectSelection.schema.graphql b/generate/testdata/errors/BindingWithIncorrectSelection.schema.graphql new file mode 100644 index 00000000..ca01037a --- /dev/null +++ b/generate/testdata/errors/BindingWithIncorrectSelection.schema.graphql @@ -0,0 +1,8 @@ +type Query { + pokemon: Pokemon +} + +type Pokemon { + species: String! + level: Int! +} diff --git a/generate/testdata/queries/Pokemon.graphql b/generate/testdata/queries/Pokemon.graphql index 5b404142..f41db71a 100644 --- a/generate/testdata/queries/Pokemon.graphql +++ b/generate/testdata/queries/Pokemon.graphql @@ -8,7 +8,8 @@ query GetPokemonSiblings($input: PokemonInput!) { roles name # this is mapped globally to internal/testutil.Pokemon: - pokemon { species level } + # note field ordering matters, but whitespace shouldn't. + pokemon { species level } # this overrides said mapping: # @genqlient(bind: "-") genqlientPokemon: pokemon { species level } diff --git a/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.go b/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.go new file mode 100644 index 00000000..6768bbf7 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.go @@ -0,0 +1 @@ +invalid selection for type-binding GetPokemonWrongFieldsPokemon: testdata/errors/BindingWithIncorrectSelection.schema.graphql:2: expected 2 fields, got 1 diff --git a/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.graphql b/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.graphql new file mode 100644 index 00000000..fbaaea2c --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-BindingWithIncorrectSelection.graphql @@ -0,0 +1 @@ +invalid selection for type-binding GetPokemonWrongFieldsPokemon: testdata/errors/BindingWithIncorrectSelection.graphql:2: expected field 1 to be level, got species