Allow non-trivial root __typename-only queries when introspection is disabled#6282
Allow non-trivial root __typename-only queries when introspection is disabled#6282SimonSapin merged 2 commits intodevfrom
Conversation
Fixes #6154 These tests were using `#[should_panic]` without an `expected` message. At some point the underlying bug was fixed but these tests still panicked for a different reason, so we didn’t notice at the time.
✅ Docs Preview ReadyNo new or changed pages found. |
|
@SimonSapin, please consider creating a changeset entry in |
|
CI performance tests
|
| .configuration_json(json!({ | ||
| "supergraph": { | ||
| "introspection": true, | ||
| }, |
There was a problem hiding this comment.
To make sure.
These tests don't work without this flag, right?
If the query only has __typename, is it treated as an introspection query?
There was a problem hiding this comment.
Yes without this these tests return INTROSPECTION_DISABLED
… but I didn’t realize opening this PR: that’s also a bug! Even if we internally use the introspection execution engine to handle __typename in fragment, it probably shouldn’t be rejected when (schema-)introspection is disabled
I’ll tweak the rejection logic in this PR
…disabled … even though the same execution machinery as introspection is used.
SimonSapin
changed the title
|
I’ve tweaked the execution logic and rephrased the PR title and description, please take another look |
| ControlFlow::Break(self.cached_introspection(schema, key, doc).await)? | ||
| // root __typename only, probably a small query | ||
| // Execute it without caching: | ||
| ControlFlow::Break(Self::execute_introspection(schema, doc))? |
There was a problem hiding this comment.
Shouldn't this be handled by the maybe_lone_root_typename call earlier in the function? Or maybe we can remove that function (which doesn't handle fragments), and only rely on this branch now?
There was a problem hiding this comment.
maybe_lone_root_typename handles exactly {__typename} (which is very common) and ~nothing else. This handles cases where __typename is the only field, but fragments or directives may be used.
It’s true that the maybe_lone_root_typename fast path becomes less impactful with this PR since the cache is bypassed either way. I wouldn’t oppose remove it, though I don’t know if this fast path is still faster than going through the executor machinery.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
LGTM but i'd like to have the question clarified before merging
4 participants
Even though we use the same executor as for schema introspection, queries that only use
__typenameshould not be rejected when schema introspection is disabled.Also fixes #6154: these tests were using
#[should_panic]without anexpectedmessage. At some point the underlying bug was fixed but these tests still panicked for a different reason, so we didn’t notice at the time.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩