From 8967a9997ce7e9e0fe179780c232a235bf191ffa Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Wed, 21 Dec 2022 15:07:59 +0100 Subject: [PATCH 1/3] fix: add more details when graphql request is invalid Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/services/router_service.rs | 30 +++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/apollo-router/src/services/router_service.rs b/apollo-router/src/services/router_service.rs index 4e520c6c27..471ce94f12 100644 --- a/apollo-router/src/services/router_service.rs +++ b/apollo-router/src/services/router_service.rs @@ -166,18 +166,25 @@ where let supergraph_service = self.supergraph_creator.create(); let fut = async move { - let graphql_request: Result = if parts.method == Method::GET { + let graphql_request: Result = if parts.method == Method::GET { parts .uri .query() - .and_then(|q| graphql::Request::from_urlencoded_query(q.to_string()).ok()) - .ok_or("missing query string") + .map(|q| { + graphql::Request::from_urlencoded_query(q.to_string()).map_err(|e| { + format!("failed to decode a valid GraphQL request from path {}", e) + }) + }) + .unwrap_or_else(|| Err("missing query string".to_string())) } else { hyper::body::to_bytes(body) .await - .map_err(|_| ()) - .and_then(|bytes| serde_json::from_reader(bytes.reader()).map_err(|_| ())) - .map_err(|_| "failed to parse the request body as JSON") + .map_err(|e| format!("failed to get the request body: {}", e)) + .and_then(|bytes| { + serde_json::from_reader(bytes.reader()).map_err(|err| { + format!("failed to deserialize the request body into JSON: {}", err) + }) + }) }; match graphql_request { @@ -324,7 +331,16 @@ where Ok(router::Response { response: http::Response::builder() .status(StatusCode::BAD_REQUEST) - .body(Body::from("Invalid GraphQL request")) + .body(Body::from( + serde_json::to_string( + &graphql::Error::builder() + .message(String::from("Invalid GraphQL request")) + .extension_code("INVALID_GRAPHQL_REQUEST") + .extension("details", error) + .build(), + ) + .unwrap_or_else(|_| String::from("Invalid GraphQL request")), + )) .expect("cannot fail"), context, }) From 61c24d2bb31df720f55f1f90995cc4a6815be89a Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Wed, 21 Dec 2022 16:00:56 +0100 Subject: [PATCH 2/3] changelog Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- NEXT_CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 6b5ab99a2e..6438759736 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -130,6 +130,21 @@ By [@neominik](https://github.com/neominik) in https://github.com/apollographql/ ## 🛠 Maintenance +### Add more details when GraphQL request is invalid ([Issue #2301](https://github.com/apollographql/router/issues/2301)) + +Add more context to the error we're throwing if your GraphQL request is invalid, here is an exemple if you pass `"variables": "null"` in your JSON payload. +```json +{ + "message": "Invalid GraphQL request", + "extensions": { + "details": "failed to deserialize the request body into JSON: invalid type: string \"null\", expected a map at line 1 column 100", + "code": "INVALID_GRAPHQL_REQUEST" + } +} +``` + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/2306 + ### Add outgoing request URLs for the subgraph calls in the OTEL spans ([Issue #2280](https://github.com/apollographql/router/issues/2280)) Add attribute named `http.url` containing the subgraph URL in span `subgraph_request`. From 019b4ac189ba47b91b908d8be14c5e700df1915c Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Thu, 22 Dec 2022 10:05:03 +0100 Subject: [PATCH 3/3] keep the previous message for prometheus, update the changelog to show how the whole graphql response looks like. --- NEXT_CHANGELOG.md | 16 ++++++---- apollo-router/src/services/router_service.rs | 32 +++++++++++++++----- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 6438759736..ea63ce2712 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -132,14 +132,18 @@ By [@neominik](https://github.com/neominik) in https://github.com/apollographql/ ### Add more details when GraphQL request is invalid ([Issue #2301](https://github.com/apollographql/router/issues/2301)) -Add more context to the error we're throwing if your GraphQL request is invalid, here is an exemple if you pass `"variables": "null"` in your JSON payload. +Add more context to the error we're throwing if your GraphQL request is invalid, here is an exemple response if you pass `"variables": "null"` in your JSON payload. ```json { - "message": "Invalid GraphQL request", - "extensions": { - "details": "failed to deserialize the request body into JSON: invalid type: string \"null\", expected a map at line 1 column 100", - "code": "INVALID_GRAPHQL_REQUEST" - } + "errors": [ + { + "message": "Invalid GraphQL request", + "extensions": { + "details": "failed to deserialize the request body into JSON: invalid type: string \"null\", expected a map at line 1 column 100", + "code": "INVALID_GRAPHQL_REQUEST" + } + } + ] } ``` diff --git a/apollo-router/src/services/router_service.rs b/apollo-router/src/services/router_service.rs index 471ce94f12..b8b1b09654 100644 --- a/apollo-router/src/services/router_service.rs +++ b/apollo-router/src/services/router_service.rs @@ -166,23 +166,41 @@ where let supergraph_service = self.supergraph_creator.create(); let fut = async move { - let graphql_request: Result = if parts.method == Method::GET { + let graphql_request: Result = if parts.method + == Method::GET + { parts .uri .query() .map(|q| { graphql::Request::from_urlencoded_query(q.to_string()).map_err(|e| { - format!("failed to decode a valid GraphQL request from path {}", e) + ( + "failed to decode a valid GraphQL request from path", + format!("failed to decode a valid GraphQL request from path {}", e), + ) }) }) - .unwrap_or_else(|| Err("missing query string".to_string())) + .unwrap_or_else(|| { + Err(("missing query string", "missing query string".to_string())) + }) } else { hyper::body::to_bytes(body) .await - .map_err(|e| format!("failed to get the request body: {}", e)) + .map_err(|e| { + ( + "failed to get the request body", + format!("failed to get the request body: {}", e), + ) + }) .and_then(|bytes| { serde_json::from_reader(bytes.reader()).map_err(|err| { - format!("failed to deserialize the request body into JSON: {}", err) + ( + "failed to deserialize the request body into JSON", + format!( + "failed to deserialize the request body into JSON: {}", + err + ), + ) }) }) }; @@ -320,7 +338,7 @@ where } } } - Err(error) => { + Err((error, extension_details)) => { // BAD REQUEST ::tracing::error!( monotonic_counter.apollo_router_http_requests_total = 1u64, @@ -336,7 +354,7 @@ where &graphql::Error::builder() .message(String::from("Invalid GraphQL request")) .extension_code("INVALID_GRAPHQL_REQUEST") - .extension("details", error) + .extension("details", extension_details) .build(), ) .unwrap_or_else(|_| String::from("Invalid GraphQL request")),