diff --git a/Cargo.lock b/Cargo.lock index 2fcb30ded0..8f514c596b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -177,6 +177,7 @@ dependencies = [ "libc", "lru", "maplit", + "mediatype", "miette 5.3.0", "mime", "mockall", @@ -3015,6 +3016,12 @@ version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "490cc448043f947bae3cbee9c203358d62dbee0db12107a74be5c30ccfd09771" +[[package]] +name = "mediatype" +version = "0.19.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac0db9ec602cd88b078efab731831a15461f90eb100c4a7da457d8cd6a917d2f" + [[package]] name = "memchr" version = "2.4.1" diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 398e5fe18c..d2544f0c8b 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -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)) diff --git a/apollo-router/Cargo.toml b/apollo-router/Cargo.toml index aab697d43f..6e4cf6cb6e 100644 --- a/apollo-router/Cargo.toml +++ b/apollo-router/Cargo.toml @@ -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" diff --git a/apollo-router/src/axum_http_server_factory.rs b/apollo-router/src/axum_http_server_factory.rs index 7c447a2dd9..1b5c7c041f 100644 --- a/apollo-router/src/axum_http_server_factory.rs +++ b/apollo-router/src/axum_http_server_factory.rs @@ -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; @@ -425,13 +429,7 @@ async fn handle_get( http_request: Request, 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(); } @@ -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( diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 8fdc848717..12de60dd35 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -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; @@ -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) @@ -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, diff --git a/apollo-router/tests/integration_tests.rs b/apollo-router/tests/integration_tests.rs index addb3d1a7c..64521ce502 100644 --- a/apollo-router/tests/integration_tests.rs +++ b/apollo-router/tests/integration_tests.rs @@ -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; @@ -545,6 +546,7 @@ async fn defer_path() { } }"#, ) + .header(ACCEPT, "multipart/mixed") .build() .expect("expecting valid request"); @@ -587,6 +589,7 @@ async fn defer_path_in_array() { } }"#, ) + .header(ACCEPT, "multipart/mixed") .build() .expect("expecting valid request"); @@ -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 { reqwest::Client::new() .post("https://federation-demo-gateway.fly.dev/") diff --git a/apollo-router/tests/snapshots/integration_tests__defer_query_without_accept.snap b/apollo-router/tests/snapshots/integration_tests__defer_query_without_accept.snap new file mode 100644 index 0000000000..e7d0daf9ad --- /dev/null +++ b/apollo-router/tests/snapshots/integration_tests__defer_query_without_accept.snap @@ -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 + } + ] +}