diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index ccf5c94d48..bb208d731d 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -35,6 +35,13 @@ Let's remove it since it's simple to add back if later required. By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/1569 +### Request and Response types from apollo_router::http_ext are private ([Issue #1589](https://github.com/apollographql/router/issues/1589)) + +These types were wrappers around the `Request` and `Response` types from the `http` crate. +Now the latter are used directly instead. + +By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollographql/router/pull/1589 + ### QueryPlan::usage_reporting and QueryPlannerContent are private ([Issue #1556](https://github.com/apollographql/router/issues/1556)) These items have been removed from the public API of `apollo_router::services::execution`. diff --git a/apollo-router/src/axum_http_server_factory.rs b/apollo-router/src/axum_http_server_factory.rs index e5ee5825eb..688ebda838 100644 --- a/apollo-router/src/axum_http_server_factory.rs +++ b/apollo-router/src/axum_http_server_factory.rs @@ -417,8 +417,8 @@ async fn custom_plugin_handler( async fn handle_get( Host(host): Host, service: BoxService< - http_ext::Request, - http_ext::Response>, + http::Request, + http::Response>, BoxError, >, http_request: Request, @@ -455,8 +455,8 @@ async fn handle_post( OriginalUri(uri): OriginalUri, Json(request): Json, service: BoxService< - http_ext::Request, - http_ext::Response>, + http::Request, + http::Response>, BoxError, >, header_map: HeaderMap, @@ -489,8 +489,8 @@ async fn run_graphql_request( ) -> impl IntoResponse where RS: Service< - http_ext::Request, - Response = http_ext::Response>, + http::Request, + Response = http::Response>, Error = BoxError, > + Send, { @@ -498,7 +498,7 @@ where Ok(mut service) => { let (head, body) = http_request.into_parts(); - match service.call(Request::from_parts(head, body).into()).await { + match service.call(Request::from_parts(head, body)).await { Err(e) => { if let Some(source_err) = e.source() { if source_err.is::() { @@ -516,7 +516,7 @@ where .into_response() } Ok(response) => { - let (mut parts, mut stream) = http::Response::from(response).into_parts(); + let (mut parts, mut stream) = response.into_parts(); parts.headers.insert( "content-type", HeaderValue::from_static("multipart/mixed;boundary=\"graphql\""), @@ -731,7 +731,6 @@ mod tests { use super::*; use crate::configuration::Cors; - use crate::http_ext::Request; use crate::services::new_service::NewService; use crate::services::transport; @@ -777,13 +776,13 @@ mod tests { mock! { #[derive(Debug)] SupergraphService { - fn service_call(&mut self, req: Request) -> Result>, BoxError>; + fn service_call(&mut self, req: http::Request) -> Result>, BoxError>; } } type MockSupergraphServiceType = tower_test::mock::Mock< - http_ext::Request, - http_ext::Response + Send>>>, + http::Request, + http::Response + Send>>>, >; #[derive(Clone)] @@ -791,7 +790,7 @@ mod tests { inner: MockSupergraphServiceType, } - impl NewService> for TestSupergraphServiceFactory { + impl NewService> for TestSupergraphServiceFactory { type Service = MockSupergraphServiceType; fn new_service(&self) -> Self::Service { @@ -803,8 +802,8 @@ mod tests { type SupergraphService = MockSupergraphServiceType; type Future = <, - >>::Service as Service>>::Future; + http::Request, + >>::Service as Service>>::Future; fn custom_endpoints(&self) -> HashMap { HashMap::new() @@ -980,7 +979,7 @@ mod tests { .times(2) .returning(move |_req| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1071,7 +1070,7 @@ mod tests { }) .returning(move |_req| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1133,7 +1132,7 @@ mod tests { .times(2) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1235,7 +1234,7 @@ mod tests { .times(2) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1304,7 +1303,7 @@ mod tests { .times(2) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1373,7 +1372,7 @@ mod tests { .times(4) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1465,7 +1464,7 @@ mod tests { }) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1525,7 +1524,7 @@ mod tests { }) .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1564,7 +1563,7 @@ mod tests { reason: "Mock error".to_string(), } .to_response(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1668,7 +1667,7 @@ mod tests { .returning(move |_| { let example_response = example_response.clone(); - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body(example_response) @@ -1940,7 +1939,7 @@ Content-Type: application/json\r .expect_service_call() .times(2) .returning(move |req| { - Ok(http_ext::Response::from_response_to_stream( + Ok(http_ext::from_response_to_stream( http::Response::builder() .status(200) .body( diff --git a/apollo-router/src/context.rs b/apollo-router/src/context.rs index 6984881326..e4c02b4336 100644 --- a/apollo-router/src/context.rs +++ b/apollo-router/src/context.rs @@ -16,7 +16,7 @@ use crate::json_ext::Value; /// Holds [`Context`] entries. pub(crate) type Entries = Arc>; -/// Context for a [`crate::http_ext::Request`] +/// A map of arbitrary JSON values, for use by plugins. /// /// Context makes use of [`DashMap`] under the hood which tries to handle concurrency /// by allowing concurrency across threads without requiring locking. This is great diff --git a/apollo-router/src/http_ext.rs b/apollo-router/src/http_ext.rs index 534a3836bd..8fd649b64a 100644 --- a/apollo-router/src/http_ext.rs +++ b/apollo-router/src/http_ext.rs @@ -12,13 +12,9 @@ use std::ops::DerefMut; use axum::body::boxed; use axum::response::IntoResponse; use bytes::Bytes; -use futures::future::ready; -use futures::stream::once; -use futures::stream::BoxStream; use http::header::HeaderName; use http::header::{self}; use http::HeaderValue; -use http::Method; use multimap::MultiMap; use crate::graphql; @@ -121,10 +117,46 @@ impl TryFrom for HeaderValue { } } +pub(crate) fn header_map( + from: MultiMap, +) -> Result, http::Error> { + let mut http = http::HeaderMap::new(); + for (key, values) in from { + let name = http::header::HeaderName::try_from(key)?; + for value in values { + http.append(name.clone(), value.try_into()?); + } + } + Ok(http) +} + +/// Ignores `http::Extensions` +pub(crate) fn clone_http_request(request: &http::Request) -> http::Request { + let mut new = http::Request::builder() + .method(request.method().clone()) + .uri(request.uri().clone()) + .version(request.version()) + .body(request.body().clone()) + .unwrap(); + *new.headers_mut() = request.headers().clone(); + new +} + +/// Ignores `http::Extensions` +pub(crate) fn clone_http_response(response: &http::Response) -> http::Response { + let mut new = http::Response::builder() + .status(response.status()) + .version(response.version()) + .body(response.body().clone()) + .unwrap(); + *new.headers_mut() = response.headers().clone(); + new +} + /// Wrap an http Request. #[derive(Debug)] -pub struct Request { - inner: http::Request, +pub(crate) struct Request { + pub(crate) inner: http::Request, } // Most of the required functionality is provided by our Deref and DerefMut implementations. @@ -133,26 +165,25 @@ impl Request { /// This is the constructor (or builder) to use when constructing a real Request. /// /// Required parameters are required in non-testing code to create a Request. - #[builder(visibility = "pub")] - fn new( + #[builder] + pub(crate) fn new( headers: MultiMap, uri: http::Uri, method: http::Method, body: T, ) -> http::Result> { - let mut builder = http::request::Builder::new().method(method).uri(uri); - for (key, values) in headers { - let header_name: HeaderName = key.try_into()?; - for value in values { - let header_value: HeaderValue = value.try_into()?; - builder = builder.header(header_name.clone(), header_value); - } - } - let req = builder.body(body)?; - + let mut req = http::request::Builder::new() + .method(method) + .uri(uri) + .body(body)?; + *req.headers_mut() = header_map(headers)?; Ok(Self { inner: req }) } +} +#[cfg(test)] +#[buildstructor::buildstructor] +impl Request { /// This is the constructor (or builder) to use when constructing a "fake" Request. /// /// This does not enforce the provision of the uri and method that is required for a fully functional @@ -160,8 +191,8 @@ impl Request { /// difficult to construct and not required for the purposes of the test. /// /// In addition, fake requests are expected to be valid, and will panic if given invalid values. - #[builder(visibility = "pub")] - fn fake_new( + #[builder] + pub(crate) fn fake_new( headers: MultiMap, uri: Option, method: Option, @@ -170,7 +201,7 @@ impl Request { Self::new( headers, uri.unwrap_or_default(), - method.unwrap_or(Method::GET), + method.unwrap_or(http::Method::GET), body, ) } @@ -196,6 +227,14 @@ impl From> for Request { } } +impl From<&'_ http::Request> for Request { + fn from(req: &'_ http::Request) -> Self { + Request { + inner: clone_http_request(req), + } + } +} + impl From> for http::Request { fn from(request: Request) -> http::Request { request.inner @@ -204,23 +243,9 @@ impl From> for http::Request { impl Clone for Request { fn clone(&self) -> Self { - // note: we cannot clone the extensions because we cannot know - // which types were stored - let mut req = http::Request::builder() - .method(self.inner.method().clone()) - .version(self.inner.version()) - .uri(self.inner.uri().clone()); - req.headers_mut().unwrap().extend( - self.inner - .headers() - .iter() - .map(|(name, value)| (name.clone(), value.clone())), - ); - - let req = req - .body(self.inner.body().clone()) - .expect("cloning a valid request creates a valid request"); - Self { inner: req } + Self { + inner: clone_http_request(&self.inner), + } } } @@ -272,26 +297,19 @@ impl Eq for Request {} /// Wrap an http Response. #[derive(Debug, Default)] -pub struct Response { - pub inner: http::Response, +pub(crate) struct Response { + pub(crate) inner: http::Response, } -impl Response { - pub fn map(self, f: F) -> Response - where - F: FnOnce(T) -> U, - { - self.inner.map(f).into() - } -} +#[cfg(test)] +pub(crate) fn from_response_to_stream( + http: http::response::Response, +) -> http::Response> { + use futures::future::ready; + use futures::stream::once; + use futures::StreamExt; -impl Response> { - pub fn from_response_to_stream(http: http::response::Response) -> Self { - let (parts, body) = http.into_parts(); - Response { - inner: http::Response::from_parts(parts, Box::pin(once(ready(body)))), - } - } + http.map(|body| once(ready(body)).boxed()) } impl Deref for Response { @@ -314,6 +332,14 @@ impl From> for Response { } } +impl From<&'_ http::Response> for Response { + fn from(req: &'_ http::Response) -> Self { + Response { + inner: clone_http_response(req), + } + } +} + impl From> for http::Response { fn from(response: Response) -> http::Response { response.inner @@ -322,22 +348,9 @@ impl From> for http::Response { impl Clone for Response { fn clone(&self) -> Self { - // note: we cannot clone the extensions because we cannot know - // which types were stored - let mut res = http::Response::builder() - .status(self.inner.status()) - .version(self.inner.version()); - res.headers_mut().unwrap().extend( - self.inner - .headers() - .iter() - .map(|(name, value)| (name.clone(), value.clone())), - ); - - let res = res - .body(self.inner.body().clone()) - .expect("cloning a valid response creates a valid response"); - Self { inner: res } + Self { + inner: clone_http_response(&self.inner), + } } } diff --git a/apollo-router/src/plugin/test/mock/subgraph.rs b/apollo-router/src/plugin/test/mock/subgraph.rs index 977e84ec44..e81f0bacb7 100644 --- a/apollo-router/src/plugin/test/mock/subgraph.rs +++ b/apollo-router/src/plugin/test/mock/subgraph.rs @@ -58,13 +58,7 @@ impl Service for MockSubgraph { .status(StatusCode::OK) .body(response.clone()) .expect("Response is serializable; qed"); - - // Create a compatible Response - let compat_response = crate::http_ext::Response { - inner: http_response, - }; - - SubgraphResponse::new_from_response(compat_response, req.context) + SubgraphResponse::new_from_response(http_response, req.context) } else { let error = crate::error::Error::builder() .message("couldn't find mock for query".to_string()) diff --git a/apollo-router/src/plugins/expose_query_plan.rs b/apollo-router/src/plugins/expose_query_plan.rs index 83fc98b123..1a11d684de 100644 --- a/apollo-router/src/plugins/expose_query_plan.rs +++ b/apollo-router/src/plugins/expose_query_plan.rs @@ -77,7 +77,7 @@ impl Plugin for ExposeQueryPlan { res = match res { Ok(mut res) => { if is_enabled { - let (parts, stream) = http::Response::from(res.response).into_parts(); + let (parts, stream) = res.response.into_parts(); let (mut first, rest) = stream.into_future().await; if let Some(first) = &mut first { @@ -92,8 +92,7 @@ impl Plugin for ExposeQueryPlan { res.response = http::Response::from_parts( parts, once(ready(first.unwrap_or_default())).chain(rest).boxed(), - ) - .into(); + ); } Ok(res) diff --git a/apollo-router/src/plugins/headers.rs b/apollo-router/src/plugins/headers.rs index 8da961b94b..a64cfd5eb3 100644 --- a/apollo-router/src/plugins/headers.rs +++ b/apollo-router/src/plugins/headers.rs @@ -254,8 +254,6 @@ mod test { use super::*; use crate::graphql::Request; - use crate::graphql::Response; - use crate::http_ext; use crate::plugin::test::MockSubgraphService; use crate::plugins::headers::Config; use crate::plugins::headers::HeadersLayer; @@ -525,10 +523,7 @@ mod test { fn example_response(_: SubgraphRequest) -> Result { Ok(SubgraphResponse::new_from_response( - http::Response::builder() - .body(Response::builder().build()) - .unwrap() - .into(), + http::Response::default(), Context::new(), )) } @@ -536,7 +531,7 @@ mod test { fn example_request() -> SubgraphRequest { SubgraphRequest { originating_request: Arc::new( - http_ext::Request::fake_builder() + http::Request::builder() .header("da", "vda") .header("db", "vdb") .header("db", "vdb") @@ -544,10 +539,9 @@ mod test { .header(CONTENT_LENGTH, "2") .header(CONTENT_TYPE, "graphql") .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), ), - subgraph_request: http_ext::Request::fake_builder() + subgraph_request: http::Request::builder() .header("aa", "vaa") .header("ab", "vab") .header("ac", "vac") @@ -555,7 +549,6 @@ mod test { .header(CONTENT_LENGTH, "22") .header(CONTENT_TYPE, "graphql") .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), operation_kind: OperationKind::Query, context: Context::new(), diff --git a/apollo-router/src/plugins/rhai.rs b/apollo-router/src/plugins/rhai.rs index aff4e19c16..b503a293a8 100644 --- a/apollo-router/src/plugins/rhai.rs +++ b/apollo-router/src/plugins/rhai.rs @@ -118,7 +118,7 @@ mod router_plugin_mod { pub(crate) fn get_subgraph( obj: &mut SharedMut, ) -> Result, Box> { - Ok(obj.with_mut(|request| request.subgraph_request.clone())) + Ok(obj.with_mut(|request| (&request.subgraph_request).into())) } #[rhai_fn(set = "subgraph", return_raw)] @@ -127,7 +127,7 @@ mod router_plugin_mod { sub: http_ext::Request, ) -> Result<(), Box> { obj.with_mut(|request| { - request.subgraph_request = sub; + request.subgraph_request = sub.inner; Ok(()) }) } @@ -838,7 +838,7 @@ impl ServiceStep { // we split the response stream into headers+first response, then a stream of deferred responses // for which we will implement mapping later let SupergraphResponse { response, context } = router_response; - let (parts, stream) = http::Response::from(response).into_parts(); + let (parts, stream) = response.into_parts(); let (first, rest) = stream.into_future().await; if first.is_none() { @@ -912,8 +912,7 @@ impl ServiceStep { let response = http::Response::from_parts( parts, once(ready(body)).chain(mapped_stream).boxed(), - ) - .into(); + ); Ok(SupergraphResponse { context: ctx, response, @@ -951,7 +950,7 @@ impl ServiceStep { // we split the response stream into headers+first response, then a stream of deferred responses // for which we will implement mapping later let ExecutionResponse { response, context } = execution_response; - let (parts, stream) = http::Response::from(response).into_parts(); + let (parts, stream) = response.into_parts(); let (first, rest) = stream.into_future().await; if first.is_none() { @@ -1024,8 +1023,7 @@ impl ServiceStep { let response = http::Response::from_parts( parts, once(ready(body)).chain(mapped_stream).boxed(), - ) - .into(); + ); Ok(ExecutionResponse { context: ctx, response, diff --git a/apollo-router/src/plugins/telemetry/metrics/mod.rs b/apollo-router/src/plugins/telemetry/metrics/mod.rs index 9d28a76a8d..705d7880c1 100644 --- a/apollo-router/src/plugins/telemetry/metrics/mod.rs +++ b/apollo-router/src/plugins/telemetry/metrics/mod.rs @@ -287,7 +287,7 @@ impl AttributesForwardConf { }; } } - let (parts, stream) = http::Response::from(response.response).into_parts(); + let (parts, stream) = response.response.into_parts(); let (first, rest) = stream.into_future().await; // Fill from response if let Some(from_response) = &self.response { @@ -322,8 +322,7 @@ impl AttributesForwardConf { let response = http::Response::from_parts( parts, once(ready(first.unwrap_or_default())).chain(rest).boxed(), - ) - .into(); + ); (SupergraphResponse { context, response }, attributes) } diff --git a/apollo-router/src/plugins/traffic_shaping/deduplication.rs b/apollo-router/src/plugins/traffic_shaping/deduplication.rs index 73e74e5edf..232a6961ad 100644 --- a/apollo-router/src/plugins/traffic_shaping/deduplication.rs +++ b/apollo-router/src/plugins/traffic_shaping/deduplication.rs @@ -36,7 +36,18 @@ where } type WaitMap = - Arc, Sender>>>>; + Arc, Sender>>>>; + +struct CloneSubgraphResponse(SubgraphResponse); + +impl Clone for CloneSubgraphResponse { + fn clone(&self) -> Self { + Self(SubgraphResponse { + response: http_ext::Response::from(&self.0.response).inner, + context: self.0.context.clone(), + }) + } +} pub(crate) struct QueryDeduplicationService { service: S, @@ -61,7 +72,7 @@ where ) -> Result { loop { let mut locked_wait_map = wait_map.lock().await; - match locked_wait_map.get_mut(&request.subgraph_request) { + match locked_wait_map.get_mut(&(&request.subgraph_request).into()) { Some(waiter) => { // Register interest in key let mut receiver = waiter.subscribe(); @@ -72,7 +83,7 @@ where return value .map(|response| { SubgraphResponse::new_from_response( - response.response, + response.0.response, request.context, ) }) @@ -85,11 +96,11 @@ where None => { let (tx, _rx) = broadcast::channel(1); - locked_wait_map.insert(request.subgraph_request.clone(), tx.clone()); + locked_wait_map.insert((&request.subgraph_request).into(), tx.clone()); drop(locked_wait_map); let context = request.context.clone(); - let http_request = request.subgraph_request.clone(); + let http_request = (&request.subgraph_request).into(); let res = { // when _drop_signal is dropped, either by getting out of the block, returning // the error from ready_oneshot or by cancellation, the drop_sentinel future will @@ -101,7 +112,12 @@ where locked_wait_map.remove(&http_request); }); - service.ready_oneshot().await?.call(request).await + service + .ready_oneshot() + .await? + .call(request) + .await + .map(CloneSubgraphResponse) }; // Let our waiters know @@ -118,7 +134,7 @@ where .expect("can only fail if the task is aborted or if the internal code panics, neither is possible here; qed"); return res.map(|response| { - SubgraphResponse::new_from_response(response.response, context) + SubgraphResponse::new_from_response(response.0.response, context) }); } } diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 73fe8c929f..0428b81169 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -267,7 +267,7 @@ impl QueryPlan { &self, context: &'a Context, service_factory: &'a Arc, - originating_request: &'a Arc>, + originating_request: &'a Arc>, schema: &'a Schema, sender: futures::channel::mpsc::Sender, ) -> Response @@ -312,7 +312,7 @@ pub(crate) struct ExecutionParameters<'a, SF> { context: &'a Context, service_factory: &'a Arc, schema: &'a Schema, - originating_request: &'a Arc>, + originating_request: &'a Arc>, deferred_fetches: &'a HashMap)>>, options: &'a QueryPlanOptions, } @@ -814,7 +814,7 @@ pub(crate) mod fetch { variable_usages: &[String], data: &Value, current_dir: &Path, - request: &Arc>, + request: &Arc>, schema: &Schema, enable_deduplicate_variables: bool, ) -> Option { @@ -979,22 +979,20 @@ pub(crate) mod fetch { .expect("we already checked that the service exists during planning; qed"); // TODO not sure if we need a RouterReponse here as we don't do anything with it - let (_parts, response) = http::Response::from( - service - .oneshot(subgraph_request) - .instrument(tracing::trace_span!("subfetch_stream")) - .await - // TODO this is a problem since it restores details about failed service - // when errors have been redacted in the include_subgraph_errors module. - // Unfortunately, not easy to fix here, because at this point we don't - // know if we should be redacting errors for this subgraph... - .map_err(|e| FetchError::SubrequestHttpError { - service: service_name.to_string(), - reason: e.to_string(), - })? - .response, - ) - .into_parts(); + let (_parts, response) = service + .oneshot(subgraph_request) + .instrument(tracing::trace_span!("subfetch_stream")) + .await + // TODO this is a problem since it restores details about failed service + // when errors have been redacted in the include_subgraph_errors module. + // Unfortunately, not easy to fix here, because at this point we don't + // know if we should be redacting errors for this subgraph... + .map_err(|e| FetchError::SubrequestHttpError { + service: service_name.to_string(), + reason: e.to_string(), + })? + .response + .into_parts(); super::log::trace_subfetch(service_name, operation, &variables, &response); @@ -1278,13 +1276,7 @@ mod tests { .execute( &Context::new(), &sf, - &Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ), + &Default::default(), &Schema::parse(test_schema!(), &Default::default()).unwrap(), sender, ) @@ -1342,13 +1334,7 @@ mod tests { .execute( &Context::new(), &sf, - &Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ), + &Default::default(), &Schema::parse(test_schema!(), &Default::default()).unwrap(), sender, ) @@ -1402,13 +1388,7 @@ mod tests { .execute( &Context::new(), &sf, - &Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ), + &Default::default(), &Schema::parse(test_schema!(), &Default::default()).unwrap(), sender, ) @@ -1544,19 +1524,7 @@ mod tests { }); let response = query_plan - .execute( - &Context::new(), - &sf, - &Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ), - &schema, - sender, - ) + .execute(&Context::new(), &sf, &Default::default(), &schema, sender) .await; // primary response @@ -1684,13 +1652,7 @@ mod tests { .execute( &Context::new(), &sf, - &Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ), + &Default::default(), &Schema::parse(schema, &Default::default()).unwrap(), sender, ) diff --git a/apollo-router/src/router_factory.rs b/apollo-router/src/router_factory.rs index 9cb46a4d05..b48fcc3afe 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -11,8 +11,6 @@ use tower_service::Service; use crate::configuration::Configuration; use crate::configuration::ConfigurationError; use crate::graphql; -use crate::http_ext::Request; -use crate::http_ext::Response; use crate::plugin::DynPlugin; use crate::plugin::Handler; use crate::services::new_service::NewService; @@ -26,15 +24,15 @@ use crate::Schema; /// Instances of this traits are used by the HTTP server to generate a new /// SupergraphService on each request pub(crate) trait SupergraphServiceFactory: - NewService, Service = Self::SupergraphService> + NewService, Service = Self::SupergraphService> + Clone + Send + Sync + 'static { type SupergraphService: Service< - Request, - Response = Response>, + http::Request, + Response = http::Response>, Error = BoxError, Future = Self::Future, > + Send; diff --git a/apollo-router/src/services/execution.rs b/apollo-router/src/services/execution.rs index 77f61a30a9..821dfdeae8 100644 --- a/apollo-router/src/services/execution.rs +++ b/apollo-router/src/services/execution.rs @@ -16,7 +16,6 @@ use tower::BoxError; use crate::error::Error; use crate::graphql; -use crate::http_ext; use crate::http_ext::IntoHeaderName; use crate::http_ext::IntoHeaderValue; use crate::json_ext::Object; @@ -31,10 +30,9 @@ pub type ServiceResult = Result; pub use crate::query_planner::QueryPlan; assert_impl_all!(Request: Send); -/// [`Context`] and [`QueryPlan`] for the request. pub struct Request { /// Original request to the Router. - pub originating_request: http_ext::Request, + pub originating_request: http::Request, pub query_plan: Arc, @@ -49,7 +47,7 @@ impl Request { /// set and be correct to create a ExecutionRequest. #[builder(visibility = "pub")] fn new( - originating_request: http_ext::Request, + originating_request: http::Request, query_plan: Arc, context: Context, ) -> Request { @@ -67,18 +65,12 @@ impl Request { /// difficult to construct and not required for the pusposes of the test. #[builder(visibility = "pub")] fn fake_new( - originating_request: Option>, + originating_request: Option>, query_plan: Option, context: Option, ) -> Request { Request::new( - originating_request.unwrap_or_else(|| { - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed") - }), + originating_request.unwrap_or_default(), Arc::new(query_plan.unwrap_or_else(|| QueryPlan::fake_builder().build())), context.unwrap_or_default(), ) @@ -86,11 +78,8 @@ impl Request { } assert_impl_all!(Response: Send); -/// [`Context`] and [`http_ext::Response`] for the response. -/// -/// This consists of the execution response and the context. pub struct Response { - pub response: http_ext::Response>, + pub response: http::Response>, pub context: Context, } @@ -121,20 +110,12 @@ impl Response { .build(); // Build an http Response - let http_response = http::Response::builder() + let response = http::Response::builder() .status(status_code.unwrap_or(StatusCode::OK)) .body(once(ready(res)).boxed()) .expect("Response is serializable; qed"); - // Create a compatible Response - let compat_response = http_ext::Response { - inner: http_response, - }; - - Self { - response: compat_response, - context, - } + Self { response, context } } /// This is the constructor (or builder) to use when constructing a "fake" ExecutionResponse. @@ -193,7 +174,7 @@ impl Response { /// In this case, you already have a valid request and just wish to associate it with a context /// and create a ExecutionResponse. pub fn new_from_response( - response: http_ext::Response>, + response: http::Response>, context: Context, ) -> Self { Self { response, context } diff --git a/apollo-router/src/services/execution_service.rs b/apollo-router/src/services/execution_service.rs index e6b626aa7a..be83754afe 100644 --- a/apollo-router/src/services/execution_service.rs +++ b/apollo-router/src/services/execution_service.rs @@ -73,7 +73,7 @@ where let stream = once(ready(first)).chain(rest).boxed(); Ok(ExecutionResponse::new_from_response( - http::Response::new(stream as BoxStream<'static, Response>).into(), + http::Response::new(stream as BoxStream<'static, Response>), ctx, )) } diff --git a/apollo-router/src/services/layers/allow_only_http_post_mutations.rs b/apollo-router/src/services/layers/allow_only_http_post_mutations.rs index 115a7ce07a..1ce7ce70a1 100644 --- a/apollo-router/src/services/layers/allow_only_http_post_mutations.rs +++ b/apollo-router/src/services/layers/allow_only_http_post_mutations.rs @@ -46,7 +46,7 @@ where .status_code(StatusCode::METHOD_NOT_ALLOWED) .context(req.context) .build(); - res.response.inner.headers_mut().insert( + res.response.headers_mut().insert( "Allow".parse::().unwrap(), "POST".parse().unwrap(), ); diff --git a/apollo-router/src/services/subgraph.rs b/apollo-router/src/services/subgraph.rs index b076668074..f3f1e75aaa 100644 --- a/apollo-router/src/services/subgraph.rs +++ b/apollo-router/src/services/subgraph.rs @@ -11,7 +11,6 @@ use tower::BoxError; use crate::error::Error; use crate::graphql; -use crate::http_ext; use crate::json_ext::Object; use crate::json_ext::Path; use crate::query_planner::fetch::OperationKind; @@ -22,12 +21,11 @@ pub type BoxCloneService = tower::util::BoxCloneService; assert_impl_all!(Request: Send); -/// [`Context`], [`OperationKind`] and [`http_ext::Request`] for the request. pub struct Request { /// Original request to the Router. - pub originating_request: Arc>, + pub originating_request: Arc>, - pub subgraph_request: http_ext::Request, + pub subgraph_request: http::Request, pub operation_kind: OperationKind, @@ -41,8 +39,8 @@ impl Request { /// Required parameters are required in non-testing code to create a Request. #[builder(visibility = "pub")] fn new( - originating_request: Arc>, - subgraph_request: http_ext::Request, + originating_request: Arc>, + subgraph_request: http::Request, operation_kind: OperationKind, context: Context, ) -> Request { @@ -61,28 +59,14 @@ impl Request { /// difficult to construct and not required for the pusposes of the test. #[builder(visibility = "pub")] fn fake_new( - originating_request: Option>>, - subgraph_request: Option>, + originating_request: Option>>, + subgraph_request: Option>, operation_kind: Option, context: Option, ) -> Request { Request::new( - originating_request.unwrap_or_else(|| { - Arc::new( - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed"), - ) - }), - subgraph_request.unwrap_or_else(|| { - http_ext::Request::fake_builder() - .headers(Default::default()) - .body(Default::default()) - .build() - .expect("fake builds should always work; qed") - }), + originating_request.unwrap_or_default(), + subgraph_request.unwrap_or_default(), operation_kind.unwrap_or(OperationKind::Query), context.unwrap_or_default(), ) @@ -90,12 +74,9 @@ impl Request { } assert_impl_all!(Response: Send); -/// [`Context`] and [`http_ext::Response`] for the response. -/// -/// This consists of the subgraph response and the context. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct Response { - pub response: http_ext::Response, + pub response: http::Response, pub context: Context, } @@ -107,7 +88,7 @@ impl Response { /// In this case, you already have a valid response and just wish to associate it with a context /// and create a Response. pub fn new_from_response( - response: http_ext::Response, + response: http::Response, context: Context, ) -> Response { Self { response, context } @@ -137,20 +118,12 @@ impl Response { .build(); // Build an http Response - let http_response = http::Response::builder() + let response = http::Response::builder() .status(status_code.unwrap_or(StatusCode::OK)) .body(res) .expect("Response is serializable; qed"); - // Create a compatible Response - let compat_response = http_ext::Response { - inner: http_response, - }; - - Self { - response: compat_response, - context, - } + Self { response, context } } /// This is the constructor (or builder) to use when constructing a "fake" Response. diff --git a/apollo-router/src/services/subgraph_service.rs b/apollo-router/src/services/subgraph_service.rs index 21de461eac..2b44aafd8d 100644 --- a/apollo-router/src/services/subgraph_service.rs +++ b/apollo-router/src/services/subgraph_service.rs @@ -107,7 +107,7 @@ impl tower::Service for SubgraphService { let service_name = (*self.service).to_owned(); Box::pin(async move { - let (parts, body) = http::Request::from(subgraph_request).into_parts(); + let (parts, body) = subgraph_request.into_parts(); let body = serde_json::to_string(&body).expect("JSON serialization should not fail"); @@ -220,10 +220,7 @@ impl tower::Service for SubgraphService { let resp = http::Response::from_parts(parts, graphql); - Ok(crate::SubgraphResponse::new_from_response( - resp.into(), - context, - )) + Ok(crate::SubgraphResponse::new_from_response(resp, context)) }) } } @@ -356,7 +353,6 @@ mod tests { use super::*; use crate::graphql::Request; use crate::graphql::Response; - use crate::http_ext; use crate::query_planner::fetch::OperationKind; use crate::Context; use crate::SubgraphRequest; @@ -454,19 +450,17 @@ mod tests { let err = subgraph_service .oneshot(SubgraphRequest { originating_request: Arc::new( - http_ext::Request::fake_builder() + http::Request::builder() .header(HOST, "host") .header(CONTENT_TYPE, "application/json") .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), ), - subgraph_request: http_ext::Request::fake_builder() + subgraph_request: http::Request::builder() .header(HOST, "rhost") .header(CONTENT_TYPE, "application/json") .uri(url) .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), operation_kind: OperationKind::Query, context: Context::new(), @@ -489,19 +483,17 @@ mod tests { let err = subgraph_service .oneshot(SubgraphRequest { originating_request: Arc::new( - http_ext::Request::fake_builder() + http::Request::builder() .header(HOST, "host") .header(CONTENT_TYPE, "application/json") .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), ), - subgraph_request: http_ext::Request::fake_builder() + subgraph_request: http::Request::builder() .header(HOST, "rhost") .header(CONTENT_TYPE, "application/json") .uri(url) .body(Request::builder().query("query").build()) - .build() .expect("expecting valid request"), operation_kind: OperationKind::Query, context: Context::new(), @@ -524,20 +516,18 @@ mod tests { let resp = subgraph_service .oneshot(SubgraphRequest { originating_request: Arc::new( - http_ext::Request::fake_builder() + http::Request::builder() .header(HOST, "host") .header(CONTENT_TYPE, "application/json") .body(Request::builder().query("query".to_string()).build()) - .build() .expect("expecting valid request"), ), - subgraph_request: http_ext::Request::fake_builder() + subgraph_request: http::Request::builder() .header(HOST, "rhost") .header(CONTENT_TYPE, "application/json") .header(CONTENT_ENCODING, "gzip") .uri(url) .body(Request::builder().query("query".to_string()).build()) - .build() .expect("expecting valid request"), operation_kind: OperationKind::Query, context: Context::new(), diff --git a/apollo-router/src/services/supergraph.rs b/apollo-router/src/services/supergraph.rs index ae629598ca..f1c3aa0cdb 100644 --- a/apollo-router/src/services/supergraph.rs +++ b/apollo-router/src/services/supergraph.rs @@ -18,7 +18,7 @@ use tower::BoxError; use crate::error::Error; use crate::graphql; -use crate::http_ext; +use crate::http_ext::header_map; use crate::http_ext::IntoHeaderName; use crate::http_ext::IntoHeaderValue; use crate::json_ext::Path; @@ -34,14 +34,14 @@ assert_impl_all!(Request: Send); /// This consists of the parsed graphql Request, HTTP headers and contextual data for extensions. pub struct Request { /// Original request to the Router. - pub originating_request: http_ext::Request, + pub originating_request: http::Request, /// Context for extension pub context: Context, } -impl From> for Request { - fn from(originating_request: http_ext::Request) -> Self { +impl From> for Request { + fn from(originating_request: http::Request) -> Self { Self { originating_request, context: Context::new(), @@ -73,14 +73,11 @@ impl Request { .variables(variables) .extensions(extensions) .build(); - - let originating_request = http_ext::Request::builder() - .headers(headers) + let mut originating_request = http::Request::builder() .uri(uri) .method(method) - .body(gql_request) - .build()?; - + .body(gql_request)?; + *originating_request.headers_mut() = header_map(headers)?; Ok(Self { originating_request, context, @@ -160,11 +157,8 @@ impl Request { } assert_impl_all!(Response: Send); -/// [`Context`] and [`http_ext::Response`] for the response. -/// -/// This consists of the response body and the context. pub struct Response { - pub response: http_ext::Response>, + pub response: http::Response>, pub context: Context, } @@ -205,17 +199,9 @@ impl Response { } } - let http_response = builder.body(once(ready(res)).boxed())?; - - // Create a compatible Response - let compat_response = http_ext::Response { - inner: http_response, - }; + let response = builder.body(once(ready(res)).boxed())?; - Ok(Self { - response: compat_response, - context, - }) + Ok(Self { response, context }) } /// This is the constructor (or builder) to use when constructing a "fake" Response. @@ -271,7 +257,7 @@ impl Response { pub fn new_from_graphql_response(response: graphql::Response, context: Context) -> Self { Self { - response: http::Response::new(once(ready(response)).boxed()).into(), + response: http::Response::new(once(ready(response)).boxed()), context, } } @@ -283,7 +269,7 @@ impl Response { } pub fn new_from_response( - response: http_ext::Response>, + response: http::Response>, context: Context, ) -> Self { Self { response, context } diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 7920d827a0..8fdc848717 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -31,7 +31,6 @@ use crate::error::QueryPlannerError; use crate::error::ServiceBuildError; use crate::graphql; use crate::graphql::Response; -use crate::http_ext::Request; use crate::introspection::Introspection; use crate::json_ext::ValueExt; use crate::plugin::DynPlugin; @@ -147,7 +146,7 @@ where *resp.status_mut() = StatusCode::BAD_REQUEST; Ok(SupergraphResponse { - response: resp.into(), + response: resp, context, }) } @@ -164,14 +163,14 @@ where let ExecutionResponse { response, context } = execution .oneshot( ExecutionRequest::builder() - .originating_request(req.originating_request.clone()) + .originating_request(req.originating_request) .query_plan(plan) .context(context) .build(), ) .await?; - let (parts, response_stream) = http::Response::from(response).into_parts(); + let (parts, response_stream) = response.into_parts(); let stream = response_stream .map(move |mut response: Response| { @@ -248,7 +247,6 @@ where }.in_current_span() .boxed(), ) - .into(), }) } } @@ -395,31 +393,31 @@ pub(crate) struct RouterCreator { apq: APQLayer, } -impl NewService> for RouterCreator { +impl NewService> for RouterCreator { type Service = BoxService< - Request, - crate::http_ext::Response>, + http::Request, + http::Response>, BoxError, >; fn new_service(&self) -> Self::Service { - BoxService::new( - self.make() - .map_request(|http_request: Request| http_request.into()) - .map_response(|response| response.response), - ) + self.make() + .map_request(|http_request: http::Request| http_request.into()) + .map_response(|response| response.response) + .boxed() } } impl SupergraphServiceFactory for RouterCreator { type SupergraphService = BoxService< - Request, - crate::http_ext::Response>, + http::Request, + http::Response>, BoxError, >; - type Future = <>>::Service as Service< - Request, - >>::Future; + type Future = + <>>::Service as Service< + http::Request, + >>::Future; fn custom_endpoints(&self) -> std::collections::HashMap { self.plugins diff --git a/apollo-router/src/state_machine.rs b/apollo-router/src/state_machine.rs index ef6de6f11b..5ae4df8d7e 100644 --- a/apollo-router/src/state_machine.rs +++ b/apollo-router/src/state_machine.rs @@ -432,8 +432,6 @@ mod tests { use super::*; use crate::graphql; - use crate::http_ext::Request; - use crate::http_ext::Response; use crate::http_server_factory::Listener; use crate::plugin::DynPlugin; use crate::plugin::Handler; @@ -658,10 +656,10 @@ mod tests { impl SupergraphServiceFactory for MyRouterFactory { type SupergraphService = MockMyRouter; - type Future = >>::Future; + type Future = >>::Future; fn custom_endpoints(&self) -> std::collections::HashMap; } - impl NewService> for MyRouterFactory { + impl NewService> for MyRouterFactory { type Service = MockMyRouter; fn new_service(&self) -> MockMyRouter; } @@ -675,7 +673,7 @@ mod tests { #[derive(Debug)] MyRouter { fn poll_ready(&mut self) -> Poll>; - fn service_call(&mut self, req: Request) -> >>::Future; + fn service_call(&mut self, req: http::Request) -> >>::Future; } impl Clone for MyRouter { @@ -684,15 +682,15 @@ mod tests { } //mockall does not handle well the lifetime on Context - impl Service> for MockMyRouter { - type Response = Response>; + impl Service> for MockMyRouter { + type Response = http::Response>; type Error = BoxError; type Future = BoxFuture<'static, Result>; fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { self.poll_ready() } - fn call(&mut self, req: Request) -> Self::Future { + fn call(&mut self, req: http::Request) -> Self::Future { self.service_call(req) } }