API 1.0 - Asymmetry in request response datastructures #1140
Assignees
Comments
SimonSapin
added a commit
that referenced
this issue
Jun 24, 2022
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: ```diff --- 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.
SimonSapin
added a commit
that referenced
this issue
Jun 24, 2022
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: ```diff --- 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.
|
This turned out less hard than I first expected as there was no need to touch how custom endpoints work: #1307 |
SimonSapin
added a commit
that referenced
this issue
Jun 28, 2022
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: ```diff --- 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.
SimonSapin
added a commit
that referenced
this issue
Jun 28, 2022
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: ```diff --- 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.
SimonSapin
added a commit
that referenced
this issue
Jun 29, 2022
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: ```diff --- 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.
SimonSapin
added a commit
that referenced
this issue
Jun 29, 2022
Follow up to #1140 Have `plugin::Handler`, its only remaining user, use `Bytes` instead.
SimonSapin
added a commit
that referenced
this issue
Jun 29, 2022
Follow up to #1140 Have `plugin::Handler`, its only remaining user, use `Bytes` instead.
SimonSapin
added a commit
that referenced
this issue
Jun 29, 2022
Follow up to #1140 Have `plugin::Handler`, its only remaining user, use `Bytes` instead.
SimonSapin
added a commit
that referenced
this issue
Jun 29, 2022
Follow up to #1140 Have `plugin::Handler`, its only remaining user, use `Bytes` instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
RouterResponse currently has an enum that allows it to deal with non grpahql responses. This is a pain for users when writing tests.
Either we need to improve the API and add failable casting of the enum, or better yet remove the enum altogether.
Removal of the enum could involve adding another service to plugins to deal with non-graphql requests.
Do this before #1138
The text was updated successfully, but these errors were encountered: