Mutation validates when schema does not support mutations

[benjaminjkraft]

graphql-js currently considers the following bogus operation valid against the given schema:

# schema:
type Query { f: String }

# operation:
mutation { bogus }

In particular, if the schema does not define a root type for one of the GraphQL operation types (here mutation), graphql-js does not against such operations. In fact, this can even happen with queries:

# schema:
type T { f: String }

# operation:
query { bogus }

(edit: validate does complain here, although buildSchema does not.)

Of course, any server that tries to resolve such a query will crash, or at best return null. The spec does say that schemas must have a query root operation type (so the second schema is invalid) and that schemas with no mutation root operation type do not support mutations (so the first operation is invalid).

In practice, this has come up for me primarily in testing graphql libraries -- while most real-world schemas support both queries and mutations, and users will typically look before assuming a server supports subscriptions, it's easy in writing test schemas to forget that. But I'm sure there do exist servers which don't have any mutations, too; I think my previous job had some such internal services, although since they were behind an Apollo federated gateway users would never see this.

I may have some time to write a PR for this. One question is whether it's actually safe to start validating against schemas with no query root operation type; it wouldn't surprise me if there are plenty of cases where users validate an extension schema (which may legitimately have no query root operation type) on its own, and currently it will validate. So it may be better to avoid the potentially-breaking change and validate both cases at the query stage. But I'd love to hear from maintainers as to their thoughts on that point. (edit: irrelevant if we keep the validation in validate rather than buildSchema.)

BTW, my suspicion is many other GraphQL libraries have this issue; I originally discovered it in vektah/gqlparser#221.

[benjaminjkraft]

Heh, from the same section of the spec, here's another schema that graphql-js validates:

enum Query {
  A
  B
}

Of course I doubt anyone will write that by accident, so it may not really matter to fix.

(edit: again the schema itself builds, but no query validates against it, so this is probably okay.)

[yaacovCR]

This is surprising behavior, probably do to this check:

graphql-js/src/validation/rules/FieldsOnCorrectTypeRule.ts

Line 38 in e3aaab0

if (type) {

Some of the errors you mention above (missing query type, for example) are picked up when the schema (rather than the operation) is validated, which can be done explicitly or by default implicitly prior to first call to execute. Or at least I thought so; when you say some of the above schemas validate, I would have to assume you are validating some operation against that schema… ie calling validate and not validateSchema, see

graphql-js/src/type/validate.ts

Line 120 in e3aaab0

context.reportError('Query root type must be provided.', schema.astNode);

although operation validation should also fail presumably and this seems like a bug!

Query could be an enum as long as some other object type was set as the default query type

[benjaminjkraft]

I would have to assume you are validating some operation against that schema… ie calling validate and not validateSchema

I'm calling parse, buildSchema, then validate, i.e.

const schema = buildSchema('...');
const op = parse('...');
const errors = validate(schema, op);

However, I was wrong about schemas with no query root type: buildSchema runs, but validate rejects any operation with "Query root type must be provided.". So while the error isn't where I might expect it, at least it's there; sorry for the confusion.

Query could be an enum as long as some other object type was set as the default query type

Sorry, to be clear this happens when there's no such declaration, i.e. the entire schema is enum Query { A B }. But that does again get caught when you try to validate an operation against it.

So assuming it's intended that (some?) schema-level validation errors show up only when you validate an operation (or maybe users are supposed to call validateSchema?), I think the only issue is the first one, namely that validate doesn't complain if you send a mutation to a schema that has none.

[yaacovCR]

Got it, thanks for clarifying. This is probable because of that check above, but perhaps simply removing it is not the answer. The strict view of FieldsOnCorrectType rule assumes the existence of the parent type because it’s only checking fields. We could automatically fail if the parent type does not exist or we could stay strict and introduce a new rule KnownOperationType

[IvanGoncharov]

@benjaminjkraft Yes, it should be solved by a new validation rule.
It should be added both here and to graphql spec:
https://github.com/graphql/graphql-spec/blob/main/spec/Section%205%20--%20Validation.md

You said you wanted to work on PR, happy to help with both of them.
For spec PRs we have an RFC procedure but PRs like this are usually fast-tracked.
For both PRs you can use existing validation rules as a reference and if you are stuck on something please feel free to open Draft PR and ask for help.

[yaacovCR]

@IvanGoncharov thanks for confirming direction!

@benjaminjkraft Thanks for raising this issue! Would you like to try out the canary release addressing this? Let me know if this is not the behavior you expected/desired, or feel free to build/change the implementation and/or spec text.

Assuming this is what you had in mind, the rule would have to be advanced at the next graphql-wg meeting. If you are able to attend, you might be interested in adding yourself and this issue to the next agenda and leading the discussion?

Thanks again for raising this!

[benjaminjkraft]

That looks great, thanks for making the PRs! I should be able to attend, I'll add to the agenda.