apollographql · nicholascioli · Feb 23, 2024 · Feb 23, 2024 · Mar 4, 2024
diff --git a/.changesets/fix_nc_prioritize_errors_in_response.md 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
diff --git a/...src/axum_factory/snapshots/apollo_router__axum_factory__tests__defer_is_not_buffered.snap b/...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__plugins__authorization__authenticated__tests__unauthenticated_request.snap b/...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/...tion/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive.snap b/...tion/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/...pshots/apollo_router__plugins__authorization__tests__authenticated_directive_dry_run.snap b/...pshots/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/...o_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap b/...o_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/...orization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-2.snap b/...orization/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/...orization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive-4.snap b/...orization/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/...thorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive.snap b/...thorization/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/...ion/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_dry_run.snap b/...ion/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/...s/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap b/...s/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
@@ -102,19 +102,19 @@ mod test {
     use crate::Configuration;
 
     static UNREDACTED_PRODUCT_RESPONSE: Lazy<Bytes> = 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<Bytes> = 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<Bytes> = 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/...logging/snapshots/apollo_router__plugins__telemetry__logging__test__when_header@logs.snap b/...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
@@ -23,6 +23,10 @@ pub struct Response {
     #[serde(skip_serializing_if = "Option::is_none", default)]
     pub label: Option<String>,
 
+    /// The optional graphql errors encountered.
+    #[serde(skip_serializing_if = "Vec::is_empty", default)]
+    pub errors: Vec<Error>,
+
     /// The response data.
     #[serde(skip_serializing_if = "Option::is_none", default)]
     pub data: Option<Value>,
@@ -31,10 +35,6 @@ pub struct Response {
     #[serde(skip_serializing_if = "Option::is_none", default)]
     pub path: Option<Path>,
 
-    /// The optional graphql errors encountered.
-    #[serde(skip_serializing_if = "Vec::is_empty", default)]
-    pub errors: Vec<Error>,
-
     /// 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/...r/snapshots/apollo_router__services__router__tests__escaped_quotes_in_string_literal.snap b/...r/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,