Inconsistent handling of directives in buildASTSchema and buildClientSchema

[Cito]

I noticed a difference in how specified (standard) directives are added by buildASTSchema vs buildClientSchema.

In the former function, the specified directives are added automatically if they are missing.

In the latter function, the specified directives are not added automatically if they are missing.

I think this should be handled in the same way by both method - this different behavior caused some head-scratching here.

The main problem is buildClientSchema not adding the specified directives. Maybe we can change this to add them, first optionally, and then in the next major version by default?

[IvanGoncharov]

@Cito Good question!

The problem here is that buildClientSchema should build a schema that is as close as possible to introspection.
Without it, we can't detect servers adding/removing directives so we can't do "feature" detection based on directives.
That said, some tools save introspection as SDL and use buildASTSchema to convert it back to schema so we need some feature parity between them.
My proposal is to not add directives by default in both functions and also in new GraphQLSchema({}) call.

Instead, we should refactor out new class GraphQLExecutableSchema (inherited from GraphQLSchema) that do:

• validateSchema - and remove __validationErrors hack. This also solves issue where typically schema validated on the first request and not on server initialization
• Add standard directives if they missing

It can look like this:

const schema = buildClientSchema(introspection);
const modyfiedSchema = extendSchema(schema, parse(extensionSDL));
const executableSchema = new GraphQLExecutableSchema(modyfiedSchema); 
// ^ validation is done here + directives are added here

const response = execute({
  executableSchema: executableSchema,
  // graphql/validate/execute/etc. are not responsible for validating schema anymore
});

This way we will load "SDL" and "introspection" snapshots as is but will inject directives only if the schema is intended to be used execution.

As an example of why we need such a non-trivial solution let's take GraphiQL as an example. GraphiQL should be able to validate queries based on directives provided by the server, not on directives injected by graphql-js.

@Cito What do you think?

P.S. Setup described in graphql-python/gql#278 is invalid since "introspection" should be the result of execution it should include all standard directives.
That said I agree that such inconsistencies are bad and we should fix them.

[IvanGoncharov]

Also, I expected GraphQLSchema => GraphQLExecutableSchema conversion to be implemented in GraphQL servers so user code and an absolute majority of schema generating/transforming libraries to be unaffected.
Another benefit for shifting the injection of directives closer to execute (basically inside GraphQL servers) is that it allows servers to make a decision on supporting or not supporting directive-based features (e.g. @stream/@defer).

[IvanGoncharov]

BTW. We also have inconsistency with assigning root type. For example:

const schema = buildSchema(`
  type Query {
    foo: String
  }
`);

const fullSchema = extendSchema(schema, parse(`
  type QueryRoot {
    query: Query
  }
  
  schema {
    query: QueryRoot
    # ^ error here because we already have `Query` as query root type
  }
`);

But if you pass it as single SDL everything works fine:

const schema = buildSchema(`
  type Query {
    foo: String
  }

  type QueryRoot {
    query: Query
  }
  
  schema {
    query: QueryRoot
    # ^ error here because we already have `Query` as query root type
  }
`);

My proposal is to have GraphQLExecutableSchema as a single point to do all non-trivial logic/defaults (including making Query a root type).

[yaacovCR]

Also, I expected GraphQLSchema => GraphQLExecutableSchema conversion to be implemented in GraphQL servers so user code and an absolute majority of schema generating/transforming libraries to be unaffected.
Another benefit for shifting the injection of directives closer to execute (basically inside GraphQL servers) is that it allows servers to make a decision on supporting or not supporting directive-based features (e.g. @stream/@defer).

I agree, this should be in user code, and it could be class based, or a simple function. A new type is not strictly necessary, but may be beneficial. This is basically similar to the path taken by graphql executor where we assume users will call validateSchema as a separate step when necessary.

[yaacovCR]

BTW. We also have inconsistency with assigning root type. For example:

const schema = buildSchema(`
  type Query {
    foo: String
  }
`);

const fullSchema = extendSchema(schema, parse(`
  type QueryRoot {
    query: Query
  }
  
  schema {
    query: QueryRoot
    # ^ error here because we already have `Query` as query root type
  }
`);

But if you pass it as single SDL everything works fine:

const schema = buildSchema(`
  type Query {
    foo: String
  }

  type QueryRoot {
    query: Query
  }
  
  schema {
    query: QueryRoot
    # ^ error here because we already have `Query` as query root type
  }
`);

The original question was about inconsistencies between buildASTSchema and buildClientSchema, but this supposes an inconsistency between passing a single SDL string to buildSchema (really eventually builtASTSchema) versus two strings, with the two strings, of course, using extendSchema. This "inconsistency" is really because the second string is not an "extension," it's a transformation, switching the query root type from 'Query' to 'RootQuery'. What one would near here is a transformSchema (like graphql-tools mapSchema) and presumably a transform keyword instead of the extend schema. This is a really interesting idea, but I don't see how it's directly related.

My proposal is to have GraphQLExecutableSchema as a single point to do all non-trivial logic/defaults (including making Query a root type).

I read this as you possibly suggesting that within the core GraphQLSchema, the Query root type would not be defined at all (possibly never defined, possibly just not set up as "Query" by default, and that the userland GraphQLExecutableSchema class would be responsible for this? Or is that a typo, and you meant to say the following:

My proposal is to have GraphQLExecutableSchema as a single point to do all non-trivial logic/defaults (including making QueryRoot a root type).

This would make sense to me, that any non trivial transformation logic should be in userland, whether it's in a new class or a new function. Are you able to clarify? Thanks!