queries with defer must have the Accept: multipart/mixed header #1610
Conversation
Since deferred responses can come back as multipart responses, we must check that the client supports that content type. This will allow older clients to show a meaningful error message instead of a parsing error if the `@defer` directive is used but they don't support it
Geal
changed the title
This comment has been minimized.
This comment has been minimized.
|
it looks like rustfmt gives up on formatting |
| @@ -153,7 +154,26 @@ where | |||
| QueryPlannerContent::Plan { query, plan } => { | |||
| let can_be_deferred = plan.root.contains_defer(); | |||
|
|
|||
| if let Some(err) = query.validate_variables(body, &schema).err() { | |||
| if can_be_deferred && !req.originating_request.headers().get_all(ACCEPT).iter().filter_map(|value| value.to_str().ok()).flat_map(|value| value.split(',') | |||
There was a problem hiding this comment.
I think this is a fine starting point but I'd suggest at least an issue-and-a-comment noting that this isn't a full spec complaint accept parser. (most importantly it will get confused if you have any "parameters" in accept like accept: multipart/mixed; foo=bar, application/json; less importantly it can get even more confused if any parameters use quoted strings, like accept: application/json; someParam="foo, multipart/mixed, bar".)
(and yes, the way that parameters bind to types in Accept header involves semicolons that bind tighter than commas, unlike in English/French/etc)
There was a problem hiding this comment.
right, this is something I planned to fix too. I'm a bit disappointed that the http crate does not provide a spec compliant iterator over values for well known headers
There was a problem hiding this comment.
I'm opening #1618 to track this
Geal
force-pushed
the
geal/accept-multipart
branch
from
7d12590
to
cd544e7
Compare
Geal
dismissed
glasser’s stale review
the mediatype crate is used now, dismissing the review to unblock the release
Follow up on #1610 rustfmt does not apply anymore on supergraph_service.rs, which is a hint that the code is way too complex. This PR moves the `Service:call` body to an async function, and splits part of the functionality
Fix #1618
Since deferred responses can come back as multipart responses, we must
check that the client supports that content type. This will allow older
clients to show a meaningful error message instead of a parsing error if
the
@deferdirective is used but they don't support it