gqlparser accepts a schema with no query type and validates any query against it #221
Labels
help wanted
Extra attention is needed
Comments
|
A related issue: gqlparser will validate any mutation against a (legal) schema with no mutation type: # schema
type Query { f: String }
# operation
mutation { g { h } } |
6 tasks
|
Yeah, this is an area for improvement. PRs welcome. |
|
Ok, maybe I can figure out how to fix this. I'll take a look. |
|
Ok, I have a draft change for this, but on further investigation this also affects graphql-js, so I'm gonna submit it there first to make sure we're fixing it in a consistent way, especially w/r/t whether a schema with no query type is valid. |
|
Yeah, as you probably saw, we import the graphql-js tests https://github.com/vektah/gqlparser/tree/master/validator/imported so fixing it upstream would help a wider community beyond even the Go ecosystem. |
benjaminjkraft
added a commit
to benjaminjkraft/gqlparser
that referenced
this issue
Jun 6, 2022
Somehow we were not validating this! 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. Ideally
we'd prohibit schemas without a query type entirely (spec says they need
one) but that caused some problems in tests and it wouldn't surprise me
if it causes problems in real life too (since an extension schema,
validated on its own, may look the same). So we just prevent it at
query time.
Fixes vektah#221.
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
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Consider the following schema:
Note the lack of a
Querytype (or aschemadeclaration pointing to an alternative entrypoint); you can't make any queries against it. However:Querywhich is not a valid object type.{ f { g } }will validate; this is clearly wrong.Indeed the same thing happens with an entirely empty schema.
Here's a test case for your convenience (I just shoved it in a random file since I wasn't sure how the tests are organized):
The text was updated successfully, but these errors were encountered: