Missing validation of schema directive argument types

[vwkd]

It seems there is no validation for the argument types of custom schema directives.

Passing an argument with the wrong type to a custom schema directive doesn't throw an error during schema building, e.g. with buildASTSchema or buildSchema. Meanwhile, built-in schema directives like deprecated seem to validate their argument types correctly.

This results in the arguments of a custom schema directive possibly ending up with an unexpected type and your server breaking at runtime when you figure out the bug in your schema. Notably, the type ends up being EnumValue if it doesn't exist as this seems to be the default type.

Steps to reproduce

1. Run with deno, e.g. open deno repl and paste in, or run from file with deno run file.ts

import { buildSchema } from "npm:[email protected]";

const source1 = `
  type Query {
    # ups... forgot the quotes around the string FOOBAR
    baz: Boolean @foo(bar: FOOBAR)
  }

  directive @foo(bar: String!) on FIELD_DEFINITION
`;

// should throw but doesn't
const schema1 = buildSchema(source1);

console.log(schema1.getQueryType()!.getFields()["baz"].astNode!.directives![0].arguments![0].value.kind);
// EnumValue

const source2 = `
  type Query {
    # ups... forgot the quotes around the string FOOBAR
    baz: Boolean @deprecated(reason: FOOBAR)
  }
`;

// correctly throws
const schema2 = buildSchema(source2);
// error: Uncaught GraphQLError: Argument "reason" has invalid value FOOBAR.

Expected result

The buildSchema(source1) call throws like the buildSchema(source2) does.

Actual result

The buildSchema(source1) call doesn't throw.

[captbaritone]

I was also surprised by this behavior. Context is that I use graphql-js in Grats to validate that the schema the user has created is valid, and I am adding support for adding directives to the schema (field definitions, argument definitions, etc) and was surprised when invalid arguments passed to a directive argument on a field definition was not reported as invalid.

Looking at the spec, I don't see where exactly where it explicitly says that directive arguments in the schema get validated. Maybe I just don't know where to look. I was expecting to find it in section 3.3 which states:

A GraphQL schema must itself be internally valid. This section describes the rules for this validation process where relevant.

Is it possible this is a gap in the spec?

I was able to confirm that this behavior still reproduces on 17.0.0-alpha.7.

Update: It looks like this is a gap in the spec: #1389

[benjie]

Agree it should be in section 3; but not sure whether it should be throughout or centralized. We define directive definition validation here:

https://spec.graphql.org/draft/#sec-Type-System.Directives.Validation

For each of the possible schema directive locations (not runtime locations) we need to add to their validation rules that each of the directives applied pass the same or similar checks as in https://spec.graphql.org/draft/#sec-Validation.Directives and https://spec.graphql.org/draft/#sec-Values.

E.g. we already have "for each field of an Object type..."

https://github.com/graphql/graphql-spec/blob/2073bc888d92692cd465db47cc544109db9c4181/spec/Section%203%20--%20Type%20System.md#L907-L922

We could add "For each directive applied to an Object type..." and do so for each of the directive locations; but I think that would get a bit redundant.

Instead, we should maybe add another section immediately below https://spec.graphql.org/draft/#sec-Type-System.Directives.Validation that is something like "Application validation" that validates the applied directives across all of their locations.

I think the main issue is we don't really have a "validate SDL" part of the spec, and the schema itself (in spec terms, not necessarily implementations) doesn't contain "applied directives" - the idea is that tooling would apply the directives by making changes to the schema, but the resulting schema would not "store" that these changes have happened. E.g. fooConnection: [Foo] @connection would have the effect of turning this into fooConnection: FooConnection and the @connection directive would evaporate. There are a few exceptions such as @deprecated where they get converted into something like isDeprecated/deprecationReason and then that can be converted back again when printing the schema SDL.

[captbaritone]

the idea is that tooling would apply the directives by making changes to the schema, but the resulting schema would not "store" that these changes have happened

Yeah, I think that's a good explanation for why we find ourselves in this state.

That said, I think that we now have several examples of directives that are used to encode additional information that consumers/clients want to be able to see. @semanticNonNull and the directives used as part of the composite schema spec both seem like they fit that bill to me.

(This next bit is veering out on a tangent a bit)
My understanding is that directives were intentionally added to GraphQL as an escape hatch (pressure release?) to allow users to extend the language. If that's their intended use, I think we need to consider this use case more clearly at the spec level. That would include adding this validation (so that clients don't need to validate directives as they read them) and also some support for reading schema directives via introspection.

[yaacovCR]

My understanding is that one complication is that directives could be used to construct the type system, for example via combination of SDL documents (such as via an import directive). Validating directives with the type system may be tricky considering that the type system may not be fully built. We could require that any original directive uses be validated based on the final type system. We could also validate directives that use only specified types.

[benjie]

We should note that there are two use cases for schema directives:

1. to change how a schema entity is formed or behaves (e.g. foo: [Foo] @connection to convert to connection, or bar: Bar @requires(level: ADMIN) for access control)
2. to add metadata to a schema entity (e.g. for use by tooling)

Currently GraphQL only caters to the former (with the nuance that @specifiedBy and @deprecated are arguably metadata); for discussion around the latter check out graphql/graphql-spec#300 and related discussions.

---

To your point, @yaacovCR, I think that validating a schema SDL would be opt-in, so tooling that works in interesting ways (e.g. merging remote SDL) would be down to the tooling to validate or not the inputs. Nothing in the spec requires an SDL document, or any GraphQL document for that matter, to be valid on its own; it's dependent on how it's used - currently we only require that an executable document be valid in order to execute it (and even then, there are caveats around when you can skip validation). You're right that it's essential we continue to reflect this nuance if/when writing up SDL validation into the spec.