Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation validates when schema does not support mutations #3592

Open
benjaminjkraft opened this issue May 16, 2022 · 7 comments
Open

Mutation validates when schema does not support mutations #3592

benjaminjkraft opened this issue May 16, 2022 · 7 comments

Comments

@benjaminjkraft
Copy link

benjaminjkraft commented May 16, 2022

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
Copy link
Author

benjaminjkraft commented May 16, 2022

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
Copy link
Contributor

This is surprising behavior, probably do to this check:

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

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
Copy link
Author

benjaminjkraft commented May 18, 2022

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
Copy link
Contributor

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
Copy link
Member

IvanGoncharov commented May 18, 2022

@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.

@IvanGoncharov IvanGoncharov added PR: feature 🚀 requires increase of "minor" version number enhancement and removed PR: feature 🚀 requires increase of "minor" version number labels May 18, 2022
yaacovCR added a commit to yaacovCR/graphql-spec that referenced this issue May 20, 2022
@yaacovCR
Copy link
Contributor

yaacovCR commented May 20, 2022

@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
Copy link
Author

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

benjaminjkraft added a commit to benjaminjkraft/graphql-wg that referenced this issue May 20, 2022
benjie added a commit to graphql/graphql-wg that referenced this issue May 27, 2022
benjaminjkraft added a commit to benjaminjkraft/graphql-spec that referenced this issue Jun 1, 2022
Right now the spec says that, for example, if the schema does not define
a mutation root type, then the schema does not support mutations.  But
there's no validation rule for it, which means that many parsers
(including graphql-js) treat a mutation as valid against such a schema.
(Indeed, many end up considering *any* mutation as valid, since they
don't know what type to validate the root selection set against.)  This
commit adds a validation rule to make the schema text explicit.

Slated for discussion at the June 2 working group meeting.  See also
graphql/graphql-js#3592.
benjaminjkraft added a commit to benjaminjkraft/gqlparser that referenced this issue Jun 6, 2022
Turns out neither we (nor anyone else) were validating this, because the
spec didn't say explicitly to validate it.  So you could do
`mutation { bogus }` even if the schema has no mutation types, or worse,
any syntactically valid query if the schema is totally empty.

In graphql/graphql-spec#955 I'm adding it to the spec, and in
graphql/graphql-js#3592 someone else is adding it to graphql-js.  So we
should too, once those land!  At that point we should also likely
reimport the relevant tests, instead of the ones I wrote here, but I
figured I'd put the PR up now so folks can review the non-test parts if
you like.

Fixes vektah#221.
StevenACoffman pushed a commit to vektah/gqlparser that referenced this issue Jun 6, 2022
Turns out neither we (nor anyone else) were validating this, because the
spec didn't say explicitly to validate it.  So you could do
`mutation { bogus }` even if the schema has no mutation types, or worse,
any syntactically valid query if the schema is totally empty.

In graphql/graphql-spec#955 I'm adding it to the spec, and in
graphql/graphql-js#3592 someone else is adding it to graphql-js.  So we
should too, once those land!  At that point we should also likely
reimport the relevant tests, instead of the ones I wrote here, but I
figured I'd put the PR up now so folks can review the non-test parts if
you like.

Fixes #221.
mattstern31 pushed a commit to mattstern31/graphql-wg-membership-note that referenced this issue Dec 1, 2022
yaacovCR added a commit that referenced this issue Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants