From e38a212de24fa43be51a94be14331a7144942fcc Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Mon, 6 Jun 2022 16:50:16 -0700 Subject: [PATCH] Be more precise in deciding whether to add the schema prelude (#205) GraphQL schemas have some builtin types, like `String`. The spec says your SDL must not include those, but in practice some schemas do. (This is probably because introspection must include them, and some tools that create SDL from introspection don't know they're supposed to filter them out.) Anyway, we've since #145 had logic to handle this; we just parse with and without the prelude that defines them and see which works. The problem is that this makes for very confusing error messages if you have an invalid schema. (Or if you have a schema that you think is valid but gqlparser doesn't, which is the more common case in the wild; see for example #200.) Right now if both ways error we take the without-prelude error, which if you didn't define the builtins is just `undefined type String`; if we took the with-prelude error then if you did define the builtins you'd just get `type String defined twice`. So we actually have to be smart if we want good error messages for everyone. So in this commit we are smart: we check if your schema defines `String`, and include the prelude only if it does not. To do this I basically inlined `gqlparser.LoadSchema` (twice), so that in between parsing and validation we can check if you have `String` and if not add the prelude. This should in theory be both more efficient (we don't have to do everything twice) and give better error messages, although it's a bit more code. Fixes #175. Test plan: make check --- docs/CHANGELOG.md | 1 + generate/parse.go | 43 +++++--- ...nvalidSchema.go => InvalidSchemaSyntax.go} | 0 ...ma.graphql => InvalidSchemaSyntax.graphql} | 0 ...hql => InvalidSchemaSyntax.schema.graphql} | 0 .../errors/InvalidSchemaWithBuiltins.graphql | 1 + .../InvalidSchemaWithBuiltins.schema.graphql | 101 ++++++++++++++++++ .../InvalidSchemaWithoutBuiltins.graphql | 1 + ...nvalidSchemaWithoutBuiltins.schema.graphql | 4 + .../TestGenerateErrors-InvalidSchema-go | 1 - .../TestGenerateErrors-InvalidSchema-graphql | 1 - .../TestGenerateErrors-InvalidSchemaSyntax-go | 1 + ...GenerateErrors-InvalidSchemaSyntax-graphql | 1 + ...teErrors-InvalidSchemaWithBuiltins-graphql | 1 + ...rrors-InvalidSchemaWithoutBuiltins-graphql | 1 + 15 files changed, 143 insertions(+), 14 deletions(-) rename generate/testdata/errors/{InvalidSchema.go => InvalidSchemaSyntax.go} (100%) rename generate/testdata/errors/{InvalidSchema.graphql => InvalidSchemaSyntax.graphql} (100%) rename generate/testdata/errors/{InvalidSchema.schema.graphql => InvalidSchemaSyntax.schema.graphql} (100%) create mode 100644 generate/testdata/errors/InvalidSchemaWithBuiltins.graphql create mode 100644 generate/testdata/errors/InvalidSchemaWithBuiltins.schema.graphql create mode 100644 generate/testdata/errors/InvalidSchemaWithoutBuiltins.graphql create mode 100644 generate/testdata/errors/InvalidSchemaWithoutBuiltins.schema.graphql delete mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-go delete mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-go create mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithBuiltins-graphql create mode 100644 generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithoutBuiltins-graphql diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f0d2f5d7..692d7dde 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -36,6 +36,7 @@ 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. +- genqlient now gives better error messages if it thinks your schema is invalid. ## v0.4.0 diff --git a/generate/parse.go b/generate/parse.go index 2f62392b..4569f25c 100644 --- a/generate/parse.go +++ b/generate/parse.go @@ -10,11 +10,10 @@ import ( "strconv" "strings" - "github.com/vektah/gqlparser/v2" "github.com/vektah/gqlparser/v2/ast" - "github.com/vektah/gqlparser/v2/gqlerror" "github.com/vektah/gqlparser/v2/parser" "github.com/vektah/gqlparser/v2/validator" + _ "github.com/vektah/gqlparser/v2/validator/rules" ) func getSchema(globs StringList) (*ast.Schema, error) { @@ -32,19 +31,39 @@ func getSchema(globs StringList) (*ast.Schema, error) { sources[i] = &ast.Source{Name: filename, Input: string(text)} } - // Multi step schema validation - // Step 1 assume schema implicitly declares types that are required by the graphql spec - // Step 2 assume schema explicitly declares types that are required by the graphql spec - var ( - schema *ast.Schema - graphqlError *gqlerror.Error - ) - schema, graphqlError = gqlparser.LoadSchema(sources...) + // Ideally here we'd just call gqlparser.LoadSchema. But the schema we are + // given may or may not contain the builtin types String, Int, etc. (The + // spec says it shouldn't, but introspection will return those types, and + // some introspection-to-SDL tools aren't smart enough to remove them.) So + // we inline LoadSchema and insert some checks. + document, graphqlError := parser.ParseSchemas(sources...) if graphqlError != nil { - schema, graphqlError = validator.LoadSchema(sources...) + // Schema doesn't even parse. + return nil, errorf(nil, "invalid schema: %v", graphqlError) + } + + // Check if we have a builtin type. (String is an arbitrary choice.) + hasBuiltins := false + for _, def := range document.Definitions { + if def.Name == "String" { + hasBuiltins = true + break + } + } + + if !hasBuiltins { + // modified from parser.ParseSchemas + var preludeAST *ast.SchemaDocument + preludeAST, graphqlError = parser.ParseSchema(validator.Prelude) if graphqlError != nil { - return nil, errorf(nil, "invalid schema: %v", graphqlError) + return nil, errorf(nil, "invalid prelude (probably a gqlparser bug): %v", graphqlError) } + document.Merge(preludeAST) + } + + schema, graphqlError := validator.ValidateSchemaDocument(document) + if graphqlError != nil { + return nil, errorf(nil, "invalid schema: %v", graphqlError) } return schema, nil diff --git a/generate/testdata/errors/InvalidSchema.go b/generate/testdata/errors/InvalidSchemaSyntax.go similarity index 100% rename from generate/testdata/errors/InvalidSchema.go rename to generate/testdata/errors/InvalidSchemaSyntax.go diff --git a/generate/testdata/errors/InvalidSchema.graphql b/generate/testdata/errors/InvalidSchemaSyntax.graphql similarity index 100% rename from generate/testdata/errors/InvalidSchema.graphql rename to generate/testdata/errors/InvalidSchemaSyntax.graphql diff --git a/generate/testdata/errors/InvalidSchema.schema.graphql b/generate/testdata/errors/InvalidSchemaSyntax.schema.graphql similarity index 100% rename from generate/testdata/errors/InvalidSchema.schema.graphql rename to generate/testdata/errors/InvalidSchemaSyntax.schema.graphql diff --git a/generate/testdata/errors/InvalidSchemaWithBuiltins.graphql b/generate/testdata/errors/InvalidSchemaWithBuiltins.graphql new file mode 100644 index 00000000..36cf9f05 --- /dev/null +++ b/generate/testdata/errors/InvalidSchemaWithBuiltins.graphql @@ -0,0 +1 @@ +query MyQuery { f g } diff --git a/generate/testdata/errors/InvalidSchemaWithBuiltins.schema.graphql b/generate/testdata/errors/InvalidSchemaWithBuiltins.schema.graphql new file mode 100644 index 00000000..297c2381 --- /dev/null +++ b/generate/testdata/errors/InvalidSchemaWithBuiltins.schema.graphql @@ -0,0 +1,101 @@ +type Query { + f: String + g: Bogus +} + +scalar Int +scalar Float +scalar String +scalar Boolean +scalar ID + +directive @include(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT +directive @skip(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT +directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE +directive @specifiedBy(url: String!) on SCALAR + +type __Schema { + description: String + types: [__Type!]! + queryType: __Type! + mutationType: __Type + subscriptionType: __Type + directives: [__Directive!]! +} + +type __Type { + kind: __TypeKind! + name: String + description: String + fields(includeDeprecated: Boolean = false): [__Field!] + interfaces: [__Type!] + possibleTypes: [__Type!] + enumValues(includeDeprecated: Boolean = false): [__EnumValue!] + inputFields: [__InputValue!] + ofType: __Type + specifiedByURL: String +} + +type __Field { + name: String! + description: String + args: [__InputValue!]! + type: __Type! + isDeprecated: Boolean! + deprecationReason: String +} + +type __InputValue { + name: String! + description: String + type: __Type! + defaultValue: String +} + +type __EnumValue { + name: String! + description: String + isDeprecated: Boolean! + deprecationReason: String +} + +enum __TypeKind { + SCALAR + OBJECT + INTERFACE + UNION + ENUM + INPUT_OBJECT + LIST + NON_NULL +} + +type __Directive { + name: String! + description: String + locations: [__DirectiveLocation!]! + args: [__InputValue!]! + isRepeatable: Boolean! +} + +enum __DirectiveLocation { + QUERY + MUTATION + SUBSCRIPTION + FIELD + FRAGMENT_DEFINITION + FRAGMENT_SPREAD + INLINE_FRAGMENT + VARIABLE_DEFINITION + SCHEMA + SCALAR + OBJECT + FIELD_DEFINITION + ARGUMENT_DEFINITION + INTERFACE + UNION + ENUM + ENUM_VALUE + INPUT_OBJECT + INPUT_FIELD_DEFINITION +} diff --git a/generate/testdata/errors/InvalidSchemaWithoutBuiltins.graphql b/generate/testdata/errors/InvalidSchemaWithoutBuiltins.graphql new file mode 100644 index 00000000..36cf9f05 --- /dev/null +++ b/generate/testdata/errors/InvalidSchemaWithoutBuiltins.graphql @@ -0,0 +1 @@ +query MyQuery { f g } diff --git a/generate/testdata/errors/InvalidSchemaWithoutBuiltins.schema.graphql b/generate/testdata/errors/InvalidSchemaWithoutBuiltins.schema.graphql new file mode 100644 index 00000000..e81082f1 --- /dev/null +++ b/generate/testdata/errors/InvalidSchemaWithoutBuiltins.schema.graphql @@ -0,0 +1,4 @@ +type Query { + f: String + g: Bogus +} diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-go b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-go deleted file mode 100644 index 6c9eca41..00000000 --- a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-go +++ /dev/null @@ -1 +0,0 @@ -testdata/errors/InvalidSchema.schema.graphql:4: invalid schema: Expected :, found } diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-graphql b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-graphql deleted file mode 100644 index 6c9eca41..00000000 --- a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchema-graphql +++ /dev/null @@ -1 +0,0 @@ -testdata/errors/InvalidSchema.schema.graphql:4: invalid schema: Expected :, found } diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-go b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-go new file mode 100644 index 00000000..0120ceec --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-go @@ -0,0 +1 @@ +testdata/errors/InvalidSchemaSyntax.schema.graphql:4: invalid schema: Expected :, found } diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-graphql b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-graphql new file mode 100644 index 00000000..0120ceec --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaSyntax-graphql @@ -0,0 +1 @@ +testdata/errors/InvalidSchemaSyntax.schema.graphql:4: invalid schema: Expected :, found } diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithBuiltins-graphql b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithBuiltins-graphql new file mode 100644 index 00000000..1c33e003 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithBuiltins-graphql @@ -0,0 +1 @@ +testdata/errors/InvalidSchemaWithBuiltins.schema.graphql:3: invalid schema: Undefined type Bogus. diff --git a/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithoutBuiltins-graphql b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithoutBuiltins-graphql new file mode 100644 index 00000000..3c07e629 --- /dev/null +++ b/generate/testdata/snapshots/TestGenerateErrors-InvalidSchemaWithoutBuiltins-graphql @@ -0,0 +1 @@ +testdata/errors/InvalidSchemaWithoutBuiltins.schema.graphql:3: invalid schema: Undefined type Bogus.