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

Author: benjaminjkraft

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
 

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

generate/testdata/errors/InvalidSchema.go renamed to generate/testdata/errors/InvalidSchemaSyntax.go

@@ -0,0 +1 @@
+query MyQuery { f g }

generate/testdata/errors/InvalidSchema.graphql renamed to generate/testdata/errors/InvalidSchemaSyntax.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
+}

generate/testdata/errors/InvalidSchema.schema.graphql renamed to generate/testdata/errors/InvalidSchemaSyntax.schema.graphql

@@ -0,0 +1 @@
+query MyQuery { f g }

generate/testdata/errors/InvalidSchemaWithBuiltins.graphql

@@ -0,0 +1,4 @@
+type Query {
+  f: String
+  g: Bogus
+}