Add validation rule that operation types exist #955
Conversation
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.
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Excellent! I'm not convinced we need the changes to section 3, but the section 5 changes look solid short of some bikeshedding over inserting "All Operation Definitions" versus elevating the "Operation Type Existence" header. |
|
Ok! I don't feel strongly about mentioning it in section 3. BTW, the other thing to discuss is whether the rule that a schema must have a query and that the root types must be object types should have explicit validation language as well. It seems like the schema side is a lot less consistent about that, so I wasn't sure. We can discuss tomorrow. |
|
The validation section (section 5) is about validation of requests; the schema has its own validation rules in the relevant places, for example the rules about root operation types can be found here: |
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.
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.
It seems to me that validation of a document is different than validation of a schema. Section 5 validates "if a request is syntactically correct" and " is unambiguous and mistake‐free in the context of a given GraphQL schema". So I don't believe section 5 is intended to cover validation of a schema, and I would not add any schema validation language there. Rather, section 3 "describes the capabilities of a GraphQL server", and already has language stating that a query root type must exist, etc. Perhaps it is worth considering rewriting all of section 3 to include schema validation rules more like the "formal specification" syntax used in section 5. However, this may be more challenging in that a schema is not required to be represented by a SDL. |
|
Yeah, to be clear, we would not add schema validation to section 5, if we wanted to add that it would go in section 3 and there's some precedent for validation rules there. As you say making that validation more formal is probably a bigger project, so I left it out for now; as currently written this is all about document validation. |
Co-authored-by: Benjie Gillam <[email protected]>
Co-authored-by: Benjie Gillam <[email protected]>
Co-authored-by: Benjie <[email protected]>
|
I have:
I think it's good to go now! 🤞 |
Co-authored-by: Shane Krueger <[email protected]>
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 2022 working group meeting.
Replaces #947. See also graphql/graphql-js#3592.
Update October 2024: this PR has been refreshed including changes proposed in #1098 and Benjie's editorial edits.
Fixes #1097