Validation of Unions types and Enum values

[Cito]

In the context of GraphQL-Core, the following questions came up:

• Why is the types property of UnionTypeDefinitionNode marked as optional? The method parseUnionMemberTypes(), which is used to set the property, always returns an array, which may be empty if there are no types specified, but never returns null or undefined.
• The GraphQL specs say in the subsection "Type-Validation" of the section about Unions that "1. A Union type must include one or more unique member types. 2. The member types of a Union type must all be Object base types; Scalar, Interface and Union types must not be member types of a Union. Similarly, wrapping types must not be member types of a Union."
  I see that this is not really validated or enforced anywhere in GraphQL.js though. There is only a devAssert in defineTypes() that makes sure the types are passed as array and the corresponding test rejects a Union type with incorrectly typed types.
  Shouldn't GraphQL.js enfore the full type validation according to the specs in a schema validation rule?
• Similar questions for the values of EnumType.

[IvanGoncharov]

@Cito It was a deliberate decision to support empty types during the construction phase so you can write:

union SomeUnion

extend union SomeUnion = Foo | Bar

so goes for all other types.
So we don't validate it until you try to execute schema, same situation with implementing all fields from interfaces and other similar checks.
One idea to make it more explicit I have is to create new class inherit from GraphQLSchema something like GraphQLFinalizedSchema:

let schema: GraphQLSchema = buildSchema(`
  union SomeUnion

  type Query {
    union: SomeUnion
  }
`);

schema = extendSchema(`
  extend union SomeUnion = Foo | Bar
`);

const finalizedSchema: GraphQLFinalizedSchema = assertFinalizedSchema(schema),
const result = graphql({
  schema: finalizedSchema,
  source: '{ __typename } ',
});

So basically make validateSchema call more explicit instead of the current approach where it hidden inside execute and cached though __validateErrors.
But most importantly it will make this distinction between schema "in construction" and "finalized" schema to be explicitly reflected in the type system.

What do you think about this idea?

[danielrearden]

@IvanGoncharov Out of curiosity, what's the use case behind allowing "empty" types like this in the first place? If the intent behind type system extensions is to empower clients to extend remote schemas, an empty union or a composite type with no fields would never exist in that context -- any types exposed by the server would have to already be valid per the spec.

So the only time you would do something like the above example would be server-side when constructing the initial schema. Since it's not necessary to use extendSchema to make use of type extensions, combining two or more documents when building the schema can be done without needing to create an invalid intermediate schema object:

const typeDefsA = `
  union SomeUnion

  type Query {
    union: SomeUnion
  }
`
const typeDefsB = `
  extend union SomeUnion = Foo | Bar
`
const schema: GraphQLSchema = buildSchema(typeDefsA + typeDefsB);

What am I missing here?

[Cito]

@IvanGoncharov Right, I understand the idea to postpone the validation until the schema is "finished".

But currently, as far as I see that validation never happens, not even when the schema is executed. It also does not answer my first question why the types parameter is optional. Event when the types list is empty, it still exists as an empty array, and is never null or undefined, as allowed by the type definition (flow and TS).

Regarding the GraphQLFinalizedSchema - maybe just use another flag, similar to assumeValid, instead of creating a new class?

[IvanGoncharov]

If the intent behind type system extensions is to empower clients to extend remote schemas, an empty union or a composite type with no fields would never exist in that context -- any types exposed by the server would have to already be valid per the spec.

@danielrearden Nope, you can't execute introspection query against the incomplete schema.

So the only time you would do something like the above example would be server-side when constructing the initial schema.

Yes, exactly.

Since it's not necessary to use extendSchema to make use of type extensions, combining two or more documents when building the schema can be done without needing to create an invalid intermediate schema object:

Technically, yes. In practice, it creates a bunch of issues starting from error messages that will show invalid error locations (bet that can be a workaround with concatAST) to more complex problems with validation.

More importantly, it prevents more complex scenarios where you combine SDL-first and code first.
For example:

const sdlSchema = buildSchema(`
  input SomeVeryLongInputType {
    # ...
  }
`);

const schema = new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'Query',
    fields: {
      foo: {
        type: GraphQLString
        args: {
          arg: { type: sdlSchema.getType('SomeVeryLongInputType') }
        },
      },
    },
  }),
});

So in this example, sdlSchema is invalid since it missing Query type but it still useful to be able to create it same logic goes for empty types.

[IvanGoncharov]

But currently, as far as I see that validation never happens, not even when the schema is executed.

@Cito It's done here:

graphql-js/src/execution/execute.js

Line 277 in 7ee63b0

assertValidSchema(schema);

graphql-js/src/type/validate.js

Line 450 in 7ee63b0

`Union type ${union.name} must define one or more member types.`,

It also does not answer my first question why the types parameter is optional.

If you mean in AST it's the situation with all arrays in AST and track in #2203 probably we will make a breaking change in 16.0.0 making it non-optional but adding some utility functions similar to how it's done in babel, eslint, etc.

Regarding the GraphQLFinalizedSchema - maybe just use another flag, similar to assumeValid, instead of creating a new class?

It' simply not obvious and create confusion about what functions accept finalized schema and what functions can work with intermediate one. Returning back to your answer

But currently, as far as I see that validation never happens, not even when the schema is executed.

So it's really not obvious that execute validates schema inside that's why I think we need to create a new wrapper class. With other functions it even less obvious e.g. introspectionFromSchema.
But printSchema or sortSchema is perfectly fine to call on the incomplete schema.

[Cito]

@IvanGoncharov - you're right, somehow I totally overlooked that. And yes, #2203 answers the other question.

So as for me, this issue can be closed. Maybe you want to open a new one for the question whether the status of schemas and which status the functions expect can be made more obvious. Btw, this is a bit similar to how you need to discern "well-formed" and "valid" XML documents.

[IvanGoncharov]

@Cito Great, thanks for XML example 👍
I will split out a more generic issue once I will have a little bit more time to write proper description so will keep this one open for now.

BTW. What do you think about this code?

graphql-js/src/graphql.js

Lines 178 to 181 in 1e1d75e

	const schemaValidationErrors = validateSchema(schema);
	if (schemaValidationErrors.length > 0) {
	return { errors: schemaValidationErrors };
	}

I think it strange to return schema validation errors to the API clients so it's a second reason why I want to force users to do this check after they built the entire schema but before running the server.
What do you think?

[Cito]

I think it strange to return schema validation errors to the API clients so it's a second reason why I want to force users to do this check after they built the entire schema but before running the server.

Totally agree.