Validate that the operation's root type is valid #225
Conversation
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.
| // update this switch block to add the new operation type. | ||
| panic(fmt.Sprintf(`got unknown operation type "%s"`, operation.Operation)) | ||
| } | ||
| if def == nil { |
There was a problem hiding this comment.
Am I wrong to think that def cannot be nil here due to an unsupported operation type, given that it would have panicked above in the default switch?
I'm not sure under what circumstances that walker.Schema.{Subscription|Mutation|Query} would be nil, but I haven't played with this area in a while.
There was a problem hiding this comment.
This gets hit in the case we are interested in -- you wrote mutation { ... } but the schema has no type Mutation (and, if we are being precise, no schema { mutation: SomeOtherType }). The panic is only if you wrote bogus { ... }, i.e. a totally invalid operation type, and that we should have rejected in parsing (which is why it's a panic).
There was a problem hiding this comment.
Ah, right. the error message case is when it is invalid in the case of this particular schema.
|
Overall I very much like this, and I'm very much looking forward to having the imported tests updated. Any improvements that have been made upstream would be welcome, since they were a little... weak before. |
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.