Improve errors when subgraph service returns a non-2xx status code

[col]

The Apollo Router handles non-2xx status codes from a subgraph service differently from the original Apollo Gateway. I would prefer this behaviour to remain consistent. I also think the current Router behaviour is insufficient as it currently ignores the status code completely and only returns an errors based on the content-type not being application/json.

As an example, if a subgraph service returns 401 Unauthorized (content-type: text/html), the Gateway would respond with something like this:

{
  "errors": [
    {
      "message": "401: Unauthorized",
      "extensions": {
        "code": "UNAUTHENTICATED",
        "response": {
          "url": "http://my-service/graphql",
          "status": 401,
          "statusText": "Unauthorized",
          "body": ""
        }
      }
    }
  ],
  "data": {
    "someField": null
  }
}

This is useful as it provide relevant info in the error response.

However the router will currently respond with:

{
  "data": null,
  "errors": [
    {
      "message": "HTTP fetch failed from 'my-service': subgraph didn't return JSON (expected content-type: application/json or content-type: application/graphql+json; found content-type: \"text/html\")",
      "path": [],
      "extensions": {
        "type": "SubrequestHttpError",
        "service": "my-service",
        "reason": "HTTP fetch failed from 'my-service': subgraph didn't return JSON (expected content-type: application/json or content-type: application/graphql+json; found content-type: \"text/html\")"
      }
    }
  ]
}

This is far less useful. I think the content-type is secondary issue to the fact that the status code was not 2xx but there is no mention of that in this error.

I understand that IF a subgraph chooses to return a non-2xx status code with a content-type of application/json then the response body, which probably contains a custom error, should still be parsed normally.

[o0Ignition0o]

Hey, thanks for opening this issue!

I like the idea of adding more information on non 2XX errors if and only if the content-type is invalid.

[abernix]

I think #2118 was meant to close this, but happy to re-open if that isn't the case! Thanks for the issue, and the PR!