Fix error handling for subgraphs

.changesets/fix_taffy_crank_flier_seaweed.md

@@ -0,0 +1,13 @@
+### Fix error handling for subgraphs ([Issue #3141](https://github.com/apollographql/router/issues/3141))
+
+The GraphQL spec is rather light on what should happen when we process responses from subgraphs. The current behaviour within the Router was inconsistently short circuiting response processing and this producing confusing errors.
+> #### Processing the response
+> 
+> If the response uses a non-200 status code and the media type of the response payload is application/json then the client MUST NOT rely on the body to be a well-formed GraphQL response since the source of the response may not be the server but instead some intermediary such as API gateways, proxies, firewalls, etc.
+
+The logic has been simplified and made consistent using the following rules:
+1. If the content type of the response is not `application/json` or `application/graphql-response+json` then we won't try to parse.
+2. If an HTTP status is not 2xx it will always be attached as a graphql error.
+3. If the response type is `application/json` and status is not 2xx and the body is not valid grapqhql the entire subgraph response will be attached as an error.
+
+By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/3328

apollo-router/src/error.rs

@@ -26,7 +26,7 @@ use crate::spec::SpecError;
 ///
 /// Note that these are not actually returned to the client, but are instead converted to JSON for
 /// [`struct@Error`].
-#[derive(Error, Display, Debug, Clone, Serialize)]
+#[derive(Error, Display, Debug, Clone, Serialize, Eq, PartialEq)]
 #[serde(untagged)]
 #[ignore_extra_doc_attributes]
 #[non_exhaustive]

apollo-router/src/response.rs

@@ -161,6 +161,15 @@ impl Response {
                 })?,
             None => vec![],
         };
+        // Graphql spec says:
+        // If the data entry in the response is not present, the errors entry in the response must not be empty.
+        // It must contain at least one error. The errors it contains should indicate why no data was able to be returned.
+        if data.is_none() && errors.is_empty() {
+            return Err(FetchError::SubrequestMalformedResponse {
+                service: service_name.to_string(),
+                reason: "graphql response without data must contain at least one error".to_string(),
+            });
+        }
 
         Ok(Response {
             label,
@@ -445,4 +454,16 @@ mod tests {
                 .build()
         );
     }
+
+    #[test]
+    fn test_no_data_and_no_errors() {
+        let response = Response::from_bytes("test", "{\"errors\":null}".into());
+        assert_eq!(
+            response.expect_err("no data and no errors"),
+            FetchError::SubrequestMalformedResponse {
+                service: "test".to_string(),
+                reason: "graphql response without data must contain at least one error".to_string(),
+            }
+        );
+    }
 }