From e30be9e80bb5d0bc21cbb43510386e50084ccdd2 Mon Sep 17 00:00:00 2001 From: Nicholas Cioli Date: Fri, 23 Feb 2024 16:40:08 -0500 Subject: [PATCH 1/3] Prioritize errors in response resolves #4375 Previously, the router would always respond with data first, and then show any errors in the request. The GraphQL spec suggests having the errors show up before data, so this commit makes errors output first before data. --- ...factory__tests__defer_is_not_buffered.snap | 16 +++++----- ...cated__tests__unauthenticated_request.snap | 22 +++++++------- ...ation__tests__authenticated_directive.snap | 22 +++++++------- ...ests__authenticated_directive_dry_run.snap | 22 +++++++------- ...ticated_directive_reject_unauthorized.snap | 4 +-- ...horization__tests__scopes_directive-2.snap | 22 +++++++------- ...horization__tests__scopes_directive-4.snap | 22 +++++++------- ...uthorization__tests__scopes_directive.snap | 14 ++++----- ...tion__tests__scopes_directive_dry_run.snap | 22 +++++++------- ..._scopes_directive_reject_unauthorized.snap | 4 +-- .../src/plugins/include_subgraph_errors.rs | 6 ++-- ...etry__logging__test__when_header@logs.snap | 3 +- apollo-router/src/response.rs | 30 ++++++++++++++++--- ...sts__escaped_quotes_in_string_literal.snap | 2 +- ...aph__tests__errors_on_nullified_paths.snap | 22 +++++++------- ...__supergraph__tests__missing_entities.snap | 14 ++++----- ...subscription_callback_schema_reload-3.snap | 8 ++--- ...__tests__subscription_with_callback-3.snap | 10 +++---- ...bscription_with_callback_with_limit-3.snap | 10 +++---- ...bscription_with_callback_with_limit-4.snap | 6 ++-- .../tests/integration/file_upload.rs | 16 +++++----- 21 files changed, 159 insertions(+), 138 deletions(-) diff --git a/apollo-router/src/axum_factory/snapshots/apollo_router__axum_factory__tests__defer_is_not_buffered.snap b/apollo-router/src/axum_factory/snapshots/apollo_router__axum_factory__tests__defer_is_not_buffered.snap index e60d87a783..bbcb442b8c 100644 --- a/apollo-router/src/axum_factory/snapshots/apollo_router__axum_factory__tests__defer_is_not_buffered.snap +++ b/apollo-router/src/axum_factory/snapshots/apollo_router__axum_factory__tests__defer_is_not_buffered.snap @@ -4,6 +4,14 @@ expression: parts --- [ { + "errors": [ + { + "message": "couldn't find mock for query {\"query\":\"query TopProducts__reviews__1($representations:[_Any!]!){_entities(representations:$representations){...on Product{reviews{__typename id product{__typename upc}}}}}\",\"operationName\":\"TopProducts__reviews__1\",\"variables\":{\"representations\":[{\"__typename\":\"Product\",\"upc\":\"1\"},{\"__typename\":\"Product\",\"upc\":\"2\"}]}}", + "extensions": { + "code": "FETCH_ERROR" + } + } + ], "data": { "topProducts": [ { @@ -18,14 +26,6 @@ expression: parts } ] }, - "errors": [ - { - "message": "couldn't find mock for query {\"query\":\"query TopProducts__reviews__1($representations:[_Any!]!){_entities(representations:$representations){...on Product{reviews{__typename id product{__typename upc}}}}}\",\"operationName\":\"TopProducts__reviews__1\",\"variables\":{\"representations\":[{\"__typename\":\"Product\",\"upc\":\"1\"},{\"__typename\":\"Product\",\"upc\":\"2\"}]}}", - "extensions": { - "code": "FETCH_ERROR" - } - } - ], "hasNext": true }, { diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__unauthenticated_request.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__unauthenticated_request.snap index 67efe8e47e..3f670366e8 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__unauthenticated_request.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__unauthenticated_request.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/authenticated.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": null - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -25,5 +15,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": null + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive.snap index a3394abe3b..669c0d23a8 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": null, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": null - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -35,5 +25,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": null, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": null + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_dry_run.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_dry_run.snap index efe7e914ba..f2e278d809 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_dry_run.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_dry_run.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": "1234" - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -35,5 +25,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": "1234" + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap index a91cd967cb..772c0af8ee 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap @@ -3,7 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": {}, "errors": [ { "message": "Unauthorized field or type", @@ -26,5 +25,6 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": {} } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-2.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-2.snap index 0b4b8966eb..1d35335a8a 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-2.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-2.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": null - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -25,5 +15,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": null + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-4.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-4.snap index 0b4b8966eb..1d35335a8a 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-4.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-4.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": null - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -25,5 +15,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": null + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive.snap index cb1a1b4cc4..24ee89bce5 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive.snap @@ -3,12 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": null - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -20,5 +14,11 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": null + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_dry_run.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_dry_run.snap index 70db02d7f3..35d50b39ae 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_dry_run.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_dry_run.snap @@ -3,16 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": { - "orga": { - "id": 1, - "creatorUser": { - "id": 0, - "name": "Ada", - "phone": "1234" - } - } - }, "errors": [ { "message": "Unauthorized field or type", @@ -35,5 +25,15 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": { + "orga": { + "id": 1, + "creatorUser": { + "id": 0, + "name": "Ada", + "phone": "1234" + } + } + } } diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap index 0b38850400..0d12faaa54 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap @@ -3,7 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs expression: response --- { - "data": {}, "errors": [ { "message": "Unauthorized field or type", @@ -15,5 +14,6 @@ expression: response "code": "UNAUTHORIZED_FIELD_OR_TYPE" } } - ] + ], + "data": {} } diff --git a/apollo-router/src/plugins/include_subgraph_errors.rs b/apollo-router/src/plugins/include_subgraph_errors.rs index 2d612c918e..6fdfd9be7b 100644 --- a/apollo-router/src/plugins/include_subgraph_errors.rs +++ b/apollo-router/src/plugins/include_subgraph_errors.rs @@ -102,19 +102,19 @@ mod test { use crate::Configuration; static UNREDACTED_PRODUCT_RESPONSE: Lazy = Lazy::new(|| { - Bytes::from_static(r#"{"data":{"topProducts":null},"errors":[{"message":"couldn't find mock for query {\"query\":\"query ErrorTopProducts__products__0($first:Int){topProducts(first:$first){__typename upc name}}\",\"operationName\":\"ErrorTopProducts__products__0\",\"variables\":{\"first\":2}}","extensions":{"test":"value","code":"FETCH_ERROR"}}]}"#.as_bytes()) + Bytes::from_static(r#"{"errors":[{"message":"couldn't find mock for query {\"query\":\"query ErrorTopProducts__products__0($first:Int){topProducts(first:$first){__typename upc name}}\",\"operationName\":\"ErrorTopProducts__products__0\",\"variables\":{\"first\":2}}","extensions":{"test":"value","code":"FETCH_ERROR"}}],"data":{"topProducts":null}}"#.as_bytes()) }); static REDACTED_PRODUCT_RESPONSE: Lazy = Lazy::new(|| { Bytes::from_static( - r#"{"data":{"topProducts":null},"errors":[{"message":"Subgraph errors redacted"}]}"# + r#"{"errors":[{"message":"Subgraph errors redacted"}],"data":{"topProducts":null}}"# .as_bytes(), ) }); static REDACTED_ACCOUNT_RESPONSE: Lazy = Lazy::new(|| { Bytes::from_static( - r#"{"data":null,"errors":[{"message":"Subgraph errors redacted"}]}"#.as_bytes(), + r#"{"errors":[{"message":"Subgraph errors redacted"}],"data":null}"#.as_bytes(), ) }); diff --git a/apollo-router/src/plugins/telemetry/logging/snapshots/apollo_router__plugins__telemetry__logging__test__when_header@logs.snap b/apollo-router/src/plugins/telemetry/logging/snapshots/apollo_router__plugins__telemetry__logging__test__when_header@logs.snap index 967cfa8405..5a5228d2dc 100644 --- a/apollo-router/src/plugins/telemetry/logging/snapshots/apollo_router__plugins__telemetry__logging__test__when_header@logs.snap +++ b/apollo-router/src/plugins/telemetry/logging/snapshots/apollo_router__plugins__telemetry__logging__test__when_header@logs.snap @@ -42,7 +42,6 @@ expression: yaml name: supergraph otel.kind: INTERNAL - fields: - http.response.body: "Response { label: None, data: Some(Object({\"data\": String(\"res\")})), path: None, errors: [], extensions: {}, has_next: None, subscribed: None, created_at: None, incremental: [] }" + http.response.body: "Response { label: None, errors: [], data: Some(Object({\"data\": String(\"res\")})), path: None, extensions: {}, has_next: None, subscribed: None, created_at: None, incremental: [] }" level: INFO message: Supergraph GraphQL response - diff --git a/apollo-router/src/response.rs b/apollo-router/src/response.rs index ad0da7f268..9dcd4fbf44 100644 --- a/apollo-router/src/response.rs +++ b/apollo-router/src/response.rs @@ -23,6 +23,10 @@ pub struct Response { #[serde(skip_serializing_if = "Option::is_none", default)] pub label: Option, + /// The optional graphql errors encountered. + #[serde(skip_serializing_if = "Vec::is_empty", default)] + pub errors: Vec, + /// The response data. #[serde(skip_serializing_if = "Option::is_none", default)] pub data: Option, @@ -31,10 +35,6 @@ pub struct Response { #[serde(skip_serializing_if = "Option::is_none", default)] pub path: Option, - /// The optional graphql errors encountered. - #[serde(skip_serializing_if = "Vec::is_empty", default)] - pub errors: Vec, - /// The optional graphql extensions. #[serde(skip_serializing_if = "Object::is_empty", default)] pub extensions: Object, @@ -466,4 +466,26 @@ mod tests { } ); } + + #[test] + fn test_errors_come_first() { + let response = Response::builder() + .error( + Error::builder() + .message("some random error") + .extension_code("RANDOM_ERROR") + .build(), + ) + .data(json!({ + "random": "data" + })) + .build(); + + let json = serde_json::to_string(&response).unwrap(); + let errors_location = json.find(r#""errors":"#).unwrap(); + let data_location = json.find(r#""data":"#).unwrap(); + + // Make sure that errors are present before data + assert!(errors_location < data_location) + } } diff --git a/apollo-router/src/services/router/snapshots/apollo_router__services__router__tests__escaped_quotes_in_string_literal.snap b/apollo-router/src/services/router/snapshots/apollo_router__services__router__tests__escaped_quotes_in_string_literal.snap index 9fcfad90a4..6c0d94f3d5 100644 --- a/apollo-router/src/services/router/snapshots/apollo_router__services__router__tests__escaped_quotes_in_string_literal.snap +++ b/apollo-router/src/services/router/snapshots/apollo_router__services__router__tests__escaped_quotes_in_string_literal.snap @@ -5,6 +5,7 @@ expression: "(graphql_response, &subgraph_query_log)" ( Response { label: None, + errors: [], data: Some( Object({ "topProducts": Array([ @@ -24,7 +25,6 @@ expression: "(graphql_response, &subgraph_query_log)" }), ), path: None, - errors: [], extensions: {}, has_next: None, subscribed: None, diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__errors_on_nullified_paths.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__errors_on_nullified_paths.snap index 6783b413d2..107d02d78b 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__errors_on_nullified_paths.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__errors_on_nullified_paths.snap @@ -1,17 +1,8 @@ --- -source: apollo-router/src/services/supergraph_service.rs +source: apollo-router/src/services/supergraph/tests.rs expression: stream.next_response().await.unwrap() --- { - "data": { - "foo": { - "id": 1, - "bar": { - "id": 2, - "something": null - } - } - }, "errors": [ { "message": "Could not fetch bar", @@ -23,5 +14,14 @@ expression: stream.next_response().await.unwrap() "code": "NOT_FOUND" } } - ] + ], + "data": { + "foo": { + "id": 1, + "bar": { + "id": 2, + "something": null + } + } + } } diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__missing_entities.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__missing_entities.snap index 33f7508979..ac7347a8fe 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__missing_entities.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__missing_entities.snap @@ -1,8 +1,13 @@ --- -source: apollo-router/src/services/supergraph_service.rs +source: apollo-router/src/services/supergraph/tests.rs expression: stream.next_response().await.unwrap() --- { + "errors": [ + { + "message": "error" + } + ], "data": { "currentUser": { "id": "0", @@ -11,10 +16,5 @@ expression: stream.next_response().await.unwrap() "name": null } } - }, - "errors": [ - { - "message": "error" - } - ] + } } diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_callback_schema_reload-3.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_callback_schema_reload-3.snap index 0e6d9f126c..95e263f2d4 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_callback_schema_reload-3.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_callback_schema_reload-3.snap @@ -1,9 +1,8 @@ --- -source: apollo-router/src/services/supergraph_service.rs -expression: stream.next_response().await.unwrap() +source: apollo-router/src/services/supergraph/tests.rs +expression: "tokio::time::timeout(Duration::from_secs(1),\n stream.next_response()).await.unwrap().unwrap()" --- { - "data": null, "errors": [ { "message": "subscription has been closed due to a schema reload", @@ -11,5 +10,6 @@ expression: stream.next_response().await.unwrap() "code": "SUBSCRIPTION_SCHEMA_RELOAD" } } - ] + ], + "data": null } diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback-3.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback-3.snap index dc0cc0735e..91ec435c11 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback-3.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback-3.snap @@ -1,11 +1,8 @@ --- -source: apollo-router/src/services/supergraph_service.rs +source: apollo-router/src/services/supergraph/tests.rs expression: stream.next_response().await.unwrap() --- { - "data": { - "userWasCreated": null - }, "errors": [ { "message": "cannot fetch the name", @@ -13,5 +10,8 @@ expression: stream.next_response().await.unwrap() "code": "INVALID" } } - ] + ], + "data": { + "userWasCreated": null + } } diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-3.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-3.snap index dc0cc0735e..91ec435c11 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-3.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-3.snap @@ -1,11 +1,8 @@ --- -source: apollo-router/src/services/supergraph_service.rs +source: apollo-router/src/services/supergraph/tests.rs expression: stream.next_response().await.unwrap() --- { - "data": { - "userWasCreated": null - }, "errors": [ { "message": "cannot fetch the name", @@ -13,5 +10,8 @@ expression: stream.next_response().await.unwrap() "code": "INVALID" } } - ] + ], + "data": { + "userWasCreated": null + } } diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-4.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-4.snap index 56196cf126..8c90183f69 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-4.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__subscription_with_callback_with_limit-4.snap @@ -1,9 +1,8 @@ --- -source: apollo-router/src/services/supergraph_service.rs +source: apollo-router/src/services/supergraph/tests.rs expression: res --- { - "data": null, "errors": [ { "message": "can't open new subscription, limit reached", @@ -11,5 +10,6 @@ expression: res "code": "SUBSCRIPTION_MAX_LIMIT" } } - ] + ], + "data": null } diff --git a/apollo-router/tests/integration/file_upload.rs b/apollo-router/tests/integration/file_upload.rs index cef8bd1d81..e7aed47b5e 100644 --- a/apollo-router/tests/integration/file_upload.rs +++ b/apollo-router/tests/integration/file_upload.rs @@ -641,13 +641,6 @@ async fn it_fails_invalid_file_order() -> Result<(), BoxError> { .run_test(|response| { insta::assert_json_snapshot!(response, @r###" { - "data": { - "file0": { - "filename": "file0", - "body": "file0 contents" - }, - "file1": null - }, "errors": [ { "message": "HTTP fetch failed from 'uploads2': HTTP fetch failed from 'uploads2': error from user's HttpBody stream: error reading a body from connection: Missing files in the request: '1'.", @@ -658,7 +651,14 @@ async fn it_fails_invalid_file_order() -> Result<(), BoxError> { "reason": "HTTP fetch failed from 'uploads2': error from user's HttpBody stream: error reading a body from connection: Missing files in the request: '1'." } } - ] + ], + "data": { + "file0": { + "filename": "file0", + "body": "file0 contents" + }, + "file1": null + } } "###); }) From 9dc6d7a4545ea8a47afda610ba7a121a4c76f88d Mon Sep 17 00:00:00 2001 From: Nicholas Cioli Date: Fri, 23 Feb 2024 16:48:53 -0500 Subject: [PATCH 2/3] Add changeset for error response --- .changesets/fix_nc_prioritize_errors_in_response.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changesets/fix_nc_prioritize_errors_in_response.md diff --git a/.changesets/fix_nc_prioritize_errors_in_response.md b/.changesets/fix_nc_prioritize_errors_in_response.md new file mode 100644 index 0000000000..5d55d7e10e --- /dev/null +++ b/.changesets/fix_nc_prioritize_errors_in_response.md @@ -0,0 +1,7 @@ +### Prioritize errors in GraphQL response ([Issue #4375](https://github.com/apollographql/router/issues/4375)) + +Previously, the router would return data from an operation before any potential errors in the request. +[As recommended in the GraphQL spec](https://spec.graphql.org/draft/#note-6f005), the suggested route +is to try to return errors first before data in the response. The router now does so. + +By [@nicholascioli](https://github.com/nicholascioli) in https://github.com/apollographql/router/pull/4728 From f84c0aa290c3e5f39a4f4e4db51eeb5dce36e52c Mon Sep 17 00:00:00 2001 From: Nicholas Cioli Date: Mon, 4 Mar 2024 09:04:25 -0500 Subject: [PATCH 3/3] Fix missing integration test changes --- ...s__test__entity_cache_authorization-2.snap | 24 ++++----- ...s__test__entity_cache_authorization-3.snap | 26 +++++----- ...dis__test__entity_cache_authorization.snap | 50 +++++++++---------- 3 files changed, 50 insertions(+), 50 deletions(-) diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-2.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-2.snap index 4a042094c7..c4315a3062 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-2.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-2.snap @@ -3,6 +3,17 @@ source: apollo-router/tests/integration/redis.rs expression: response --- { + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "me" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + } + ], "data": { "me": null, "topProducts": [ @@ -35,16 +46,5 @@ expression: response ] } ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] + } } diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-3.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-3.snap index 2bee2eb129..cf3841f189 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-3.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization-3.snap @@ -3,6 +3,18 @@ source: apollo-router/tests/integration/redis.rs expression: response --- { + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "me", + "name" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + } + ], "data": { "me": { "id": "1", @@ -38,17 +50,5 @@ expression: response ] } ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me", - "name" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] + } } diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization.snap index 1c4519a0aa..3db50b5ceb 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__redis__test__entity_cache_authorization.snap @@ -3,6 +3,30 @@ source: apollo-router/tests/integration/redis.rs expression: response --- { + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "me" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + }, + { + "message": "Unauthorized field or type", + "path": [ + "topProducts", + "@", + "reviews", + "@", + "author" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + } + ], "data": { "me": null, "topProducts": [ @@ -29,29 +53,5 @@ expression: response ] } ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - }, - { - "message": "Unauthorized field or type", - "path": [ - "topProducts", - "@", - "reviews", - "@", - "author" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] + } }