Skip to content

Commit

Permalink
queries with defer must have the Accept: multipart/mixed header (#1610)
Browse files Browse the repository at this point in the history
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 `@defer` directive is used but they don't support it
  • Loading branch information
Geal committed Aug 26, 2022
1 parent 162a777 commit cf40d8d
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 18 deletions.
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,14 @@ possible, and not all at once after a while.

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1604

### Queries with defer must have the Accept: multipart/mixed header ([PR #1610](https://github.com/apollographql/router/issues/1610))

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.

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1610

## 🛠 Maintenance

### Depend on published `router-bridge` ([PR #1613](https://github.com/apollographql/router/issues/1613))
Expand Down
1 change: 1 addition & 0 deletions apollo-router/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ url = { version = "2.2.2", features = ["serde"] }
urlencoding = "2.1.0"
yaml-rust = "0.4.5"
pin-project-lite = "0.2.9"
mediatype = "0.19.7"

[target.'cfg(macos)'.dependencies]
uname = "0.1.1"
Expand Down
35 changes: 18 additions & 17 deletions apollo-router/src/axum_http_server_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ use http::Request;
use http::Uri;
use hyper::server::conn::Http;
use hyper::Body;
use mediatype::names::HTML;
use mediatype::names::TEXT;
use mediatype::MediaType;
use mediatype::MediaTypeList;
use opentelemetry::global;
use opentelemetry::trace::SpanKind;
use opentelemetry::trace::TraceContextExt;
Expand Down Expand Up @@ -425,13 +429,7 @@ async fn handle_get(
http_request: Request<Body>,
display_landing_page: bool,
) -> impl IntoResponse {
if http_request
.headers()
.get(&http::header::ACCEPT)
.map(prefers_html)
.unwrap_or_default()
&& display_landing_page
{
if prefers_html(http_request.headers()) && display_landing_page {
return display_home_page().into_response();
}

Expand Down Expand Up @@ -601,16 +599,19 @@ where
}
}

fn prefers_html(accept_header: &HeaderValue) -> bool {
accept_header
.to_str()
.map(|accept_str| {
accept_str
.split(',')
.map(|a| a.trim())
.any(|a| a == "text/html")
})
.unwrap_or_default()
fn prefers_html(headers: &HeaderMap) -> bool {
let text_html = MediaType::new(TEXT, HTML);

headers.get_all(&http::header::ACCEPT).iter().any(|value| {
value
.to_str()
.map(|accept_str| {
let mut list = MediaTypeList::new(accept_str);

list.any(|mime| mime.as_ref() == Ok(&text_html))
})
.unwrap_or(false)
})
}

async fn decompress_request_body(
Expand Down
40 changes: 39 additions & 1 deletion apollo-router/src/services/supergraph_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@ use futures::stream::once;
use futures::stream::BoxStream;
use futures::stream::StreamExt;
use futures::TryFutureExt;
use http::header::ACCEPT;
use http::HeaderMap;
use http::StatusCode;
use indexmap::IndexMap;
use lazy_static::__Deref;
use mediatype::names::MIXED;
use mediatype::names::MULTIPART;
use mediatype::MediaType;
use mediatype::MediaTypeList;
use opentelemetry::trace::SpanKind;
use tower::util::BoxService;
use tower::BoxError;
Expand Down Expand Up @@ -153,7 +159,24 @@ 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 && !accepts_multipart(req.originating_request.headers()) {
tracing::error!("tried to send a defer request without accept: multipart/mixed");
let mut resp = http::Response::new(
once(ready(
graphql::Response::builder()
.errors(vec![crate::error::Error::builder()
.message(String::from("the router received a query with the @defer directive but the client does not accept multipart/mixed HTTP responses"))
.build()])
.build(),
))
.boxed(),
);
*resp.status_mut() = StatusCode::BAD_REQUEST;
Ok(SupergraphResponse {
response: resp,
context,
})
} else if let Some(err) = query.validate_variables(body, &schema).err() {
let mut res = SupergraphResponse::new_from_graphql_response(err, context);
*res.response.status_mut() = StatusCode::BAD_REQUEST;
Ok(res)
Expand Down Expand Up @@ -282,6 +305,21 @@ where
}
}

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)
})
}

/// Builder which generates a plugin pipeline.
///
/// This is at the heart of the delegation of responsibility model for the router. A schema,
Expand Down
43 changes: 43 additions & 0 deletions apollo-router/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use apollo_router::plugin::Plugin;
use apollo_router::plugin::PluginInit;
use apollo_router::services::subgraph;
use apollo_router::services::supergraph;
use http::header::ACCEPT;
use http::Method;
use http::StatusCode;
use maplit::hashmap;
Expand Down Expand Up @@ -545,6 +546,7 @@ async fn defer_path() {
}
}"#,
)
.header(ACCEPT, "multipart/mixed")
.build()
.expect("expecting valid request");

Expand Down Expand Up @@ -587,6 +589,7 @@ async fn defer_path_in_array() {
}
}"#,
)
.header(ACCEPT, "multipart/mixed")
.build()
.expect("expecting valid request");

Expand All @@ -601,6 +604,46 @@ async fn defer_path_in_array() {
insta::assert_json_snapshot!(second);
}

#[tokio::test(flavor = "multi_thread")]
async fn defer_query_without_accept() {
let config = serde_json::json!({
"server": {
"experimental_defer_support": true
},
"plugins": {
"experimental.include_subgraph_errors": {
"all": true
}
}
});
let request = supergraph::Request::fake_builder()
.query(
r#"{
me {
reviews {
id
author {
id
... @defer(label: "author name") {
name
}
}
}
}
}"#,
)
.header(ACCEPT, "application/json")
.build()
.expect("expecting valid request");

let (router, _) = setup_router_and_registry(config).await;

let mut stream = router.oneshot(request).await.unwrap();

let first = stream.next_response().await.unwrap();
insta::assert_json_snapshot!(first);
}

async fn query_node(request: &supergraph::Request) -> Result<graphql::Response, String> {
reqwest::Client::new()
.post("https://federation-demo-gateway.fly.dev/")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: apollo-router/tests/integration_tests.rs
assertion_line: 646
expression: first
---
{
"errors": [
{
"message": "the router received a query with the @defer directive but the client does not accept multipart/mixed HTTP responses",
"locations": [],
"path": null
}
]
}

0 comments on commit cf40d8d

Please sign in to comment.