Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin the supported defer specification version to 20220824 #1652

Merged
merged 8 commits into from
Aug 31, 2022

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Aug 30, 2022

Fix #1648
Fix #1656
Fix #1659

Pinned to this commit:
graphql/graphql-spec@01d7b98

Since the router will ship before the @defer specification is done, we
add a parameter to the Accept and Content-Type headers to indicate
which specification version is accepted

Fixed to this commit:
graphql/graphql-spec@01d7b98

Since the router will ship before the `@defer` specification is done, we
add a parameter to the `Accept` and `Content-Type` headers to indicate
which specification version is accepted
@github-actions

This comment has been minimized.

@Geal Geal changed the title Fix the supported defer specification version to 20220824 Pin the supported defer specification version to 20220824 Aug 30, 2022
NEXT_CHANGELOG.md Outdated Show resolved Hide resolved
apollo-router/src/services/supergraph_service.rs Outdated Show resolved Hide resolved
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return an HTTP 406 with the error case?


// set the supported `@defer` specification version to https://github.com/graphql/graphql-spec/pull/742/commits/01d7b98f04810c9a9db4c0e53d3c4d54dbf10b82
pub(crate) const MULTIPART_DEFER_SPEC_PARAMETER: &str = "deferSpec";
pub(crate) const MULTIPART_DEFER_SPEC_VALUE: &str = "20220824";
Copy link
Contributor

@prasek prasek Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub(crate) const MULTIPART_DEFER_SPEC_VALUE: &str = "20220824";
pub(crate) const MULTIPART_DEFER_SPEC_VALUE: &str = "20220822";

clients are sending deferSpec=20220822 can we use that instead?

Copy link
Member

@abernix abernix Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prasek The motivation for picking 20220824 as the value seems justified/warranted since, as noted above, we're pinning to a commit of the specification on August 24th. If we're going to have a deferSpec that looks like YYYYMMDD then it should probably match the spec as of that date. I think clients should make the change here even though I realize this is a bit of a moving target at the moment. It should settle down soon.

On the other hand, if we want a bit more flexibility here (there is probably flexibility) perhaps we should choose a deferSpec that isn't so precisely date-based — like 202208.001.

Copy link
Member

@abernix abernix Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I feel like the clarified behavior in that defer spec commit referenced above is noteworthy and worth abiding by, but if we agree together that it's not then let's ensure our implementation (on both sides) abides by the specification on the 22nd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes to the defer specification from August 24th are used in #1640 so that will be the version we use, because we need the clarification on the if argument default behaviour

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. We can communicate that to the client teams. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good 👍

@abernix
Copy link
Member

abernix commented Aug 31, 2022

A comment from @glasser in #1656 (comment)

                list.any(|mime| mime.as_ref() == Ok(&multipart_mixed))

This Eq check compares to see if the types are exactly the same including parameters. You probably want to check to see if mime.essence() (which is the type without the parameters) equals multipart_mixed (and then of course look for deferSpec too).

Referring to:

fn accepts_multipart(headers: &HeaderMap) -> bool {
let multipart_mixed = MediaType::new(MULTIPART, MIXED);
headers.get_all(ACCEPT).iter().any(|value| {
value
.to_str()
.map(|accept_str| {
let mut list = MediaTypeList::new(accept_str);
list.any(|mime| mime.as_ref() == Ok(&multipart_mixed))
})
.unwrap_or(false)
})
}

Geal and others added 5 commits August 31, 2022 09:57
Co-authored-by: Jesse Rosenberger <[email protected]>
Fix #1546 
Fix #1584 
Fix #1633

Co-authored-by: Jesse Rosenberger <[email protected]>
if there were other arguments to the multipart/mixed type in the Accept
header, an equality test would fail
@Geal
Copy link
Contributor Author

Geal commented Aug 31, 2022

Can we return an HTTP 406 with the error case?

yes, it is done in c8a009e

@Geal
Copy link
Contributor Author

Geal commented Aug 31, 2022

A comment from @glasser in #1656 (comment)

fixed in e3752f3

@Geal Geal merged commit 8410179 into main Aug 31, 2022
@Geal Geal deleted the geal/defer-accept-version branch August 31, 2022 12:03
mime.as_ref()
.map(|mime| {
mime.ty == MULTIPART
&& mime.subty == MIXED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW my impression is that this particular library separates out "the thing after the +" so this woudl also match multipart/mixed+foo which is probably not ideal. I would suggest just doing mime.essence() == multipartmixed (essence strips parameters)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants