Remove non-GraphQL body variants from RouterResponse

[SimonSapin]

Fixes #1140

The body type inside RouterResponse is now graphql::Response instead of ResponseBody.

ResponseBody is an enum for either graphql::Response, a JSON value, or a String. However except for one unit test only the GraphQL variant was ever used.

The diff is big but (again except for that test) the only meaningful change is:

--- a/apollo-router/src/services/mod.rs
+++ b/apollo-router/src/services/mod.rs
 /// [`Context`] and [`http_ext::Response<ResponseBody>`] for the response.
 ///
 /// This consists of the response body and the context.
 #[derive(Clone, Debug)]
-pub struct RouterResponse<T: Stream<Item = ResponseBody>> {
+pub struct RouterResponse<T: Stream<Item = Response>> {
     pub response: http_ext::Response<T>,
     pub context: Context,
 }

The test in question is it_checks_the_shape_of_router_request where the response body was changed from a single JSON string to a GraphQL response with that same string in data.

BodyResponse is still used for custom endpoints in plugins (which is where non-GraphQL responses are used), but that part of the API does not use RouterResponse so it is not affected by this PR.

Everything else was made with search-and-replace or following compiler errors, and so should be fairly mechanical.

[netlify]

✅ Deploy Preview for apollo-router-docs canceled.

Name	Link
🔨 Latest commit	f9f4b36
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/62baf2cf1a006100080f5429

[SimonSapin]

With this change it’s possible that macros for the rhai plugin could be simplified, cc @garypen. (But this PR is already big and that change can be done later without affecting public APIs.)

[Geal]

what was the reason for introducing it? (ie commits/PR/issues around that time?)

[bnjjj]

IIRC I think it was first introduced with the APQ layer and the need to return an error as a string when we had errors with APQ

[SimonSapin]

The ResponseBody enum was added in #416:

The rationale behind this is that with the types as they were it was impossible to send a response payload of anything other than a graphql response. This would potentially cause issues for users wanting to send back specific error codes or messages for something such as auth.

[Geal]

so this sounds like we would still need the ability to return an arbitrary response, but maybe we don't need 3 variants for that?

[BrynCooke]

I'm not sure. As far as I can see the APQ plugin returns a graphql response. I would be tempted to say that if we can't find a concrete example then we should not allow non graphql responses.
Can we think of a concrete example?

[Geal]

we may not have examples in the router, but maybe some behaviour is already expected in the gateway or server. Are there errors that are not sent in the same format as a graphql response?

[BrynCooke]

Approved, but feel we should follow up with a PR to remove ResponseBody completely.
The only place left is:

/// Handler represents a [`Plugin`] endpoint.
#[derive(Clone)]
pub struct Handler {
    service: Buffer<
        BoxService<http_ext::Request<Bytes>, http_ext::Response<ResponseBody>, BoxError>,
        http_ext::Request<Bytes>,
    >,
}

Where http_ext::Response<ResponseBody> is replaced with http_ext::Response<Bytes>

[BrynCooke]

we may not have examples in the router, but maybe some behaviour is already expected in the gateway or server. Are there errors that are not sent in the same format as a graphql response?

While I can't prove it, the docs from apollo server seem to point towards using a graphql error as a response even in the case where http status code is modified: https://www.apollographql.com/docs/apollo-server/data/errors/

@abernix Do you know of a situation where users should be returning plain text from a graphql request?

[SimonSapin]

Chatting with @abernix, it sounds like responding to a GraphQL request with a non-GraphQL response has been problematic for some clients in the past and sticking to a GraphQL response (possibly with errors key and no data) is preferable.

[Geal]

ok, then using graphql responses everywhere sounds better then