From 4fb0d0fdb06bcd66a4cc7112fe7fd438c709e13e Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 26 Jul 2022 16:58:25 +0200 Subject: [PATCH 1/8] remove Buffer from MockSubgraphService --- apollo-router/src/plugin/test/mod.rs | 40 +++++--------------------- apollo-router/src/query_planner/mod.rs | 23 ++++----------- 2 files changed, 13 insertions(+), 50 deletions(-) diff --git a/apollo-router/src/plugin/test/mod.rs b/apollo-router/src/plugin/test/mod.rs index 15d6937264..f01e2fe6f3 100644 --- a/apollo-router/src/plugin/test/mod.rs +++ b/apollo-router/src/plugin/test/mod.rs @@ -29,6 +29,7 @@ use crate::query_planner::BridgeQueryPlanner; use crate::query_planner::CachingQueryPlanner; use crate::services::layers::apq::APQLayer; use crate::services::layers::ensure_query_presence::EnsureQueryPresence; +use crate::services::subgraph_service::MakeSubgraphService; use crate::services::subgraph_service::SubgraphServiceFactory; use crate::services::ExecutionCreator; use crate::services::Plugins; @@ -93,38 +94,19 @@ impl PluginTestHarness { schema: IntoSchema, mock_router_service: Option, mock_query_planner_service: Option, - mock_subgraph_services: HashMap, + mut subgraph_services: HashMap>, ) -> Result { - let mut subgraph_services = mock_subgraph_services - .into_iter() - .map(|(k, v)| (k, Buffer::new(v.build().boxed(), DEFAULT_BUFFER_SIZE))) - .collect::>(); // If we're using the canned schema then add some canned results if let IntoSchema::Canned = schema { subgraph_services .entry("products".to_string()) - .or_insert_with(|| { - Buffer::new( - mock::canned::products_subgraph().boxed(), - DEFAULT_BUFFER_SIZE, - ) - }); + .or_insert_with(|| Arc::new(mock::canned::products_subgraph())); subgraph_services .entry("accounts".to_string()) - .or_insert_with(|| { - Buffer::new( - mock::canned::accounts_subgraph().boxed(), - DEFAULT_BUFFER_SIZE, - ) - }); + .or_insert_with(|| Arc::new(mock::canned::accounts_subgraph())); subgraph_services .entry("reviews".to_string()) - .or_insert_with(|| { - Buffer::new( - mock::canned::reviews_subgraph().boxed(), - DEFAULT_BUFFER_SIZE, - ) - }); + .or_insert_with(|| Arc::new(mock::canned::reviews_subgraph())); } let schema = Arc::new(Schema::from(schema)); @@ -210,13 +192,7 @@ impl PluginTestHarness { #[derive(Clone)] pub struct MockSubgraphFactory { - pub(crate) subgraphs: HashMap< - String, - Buffer< - BoxService, - SubgraphRequest, - >, - >, + pub(crate) subgraphs: HashMap>, pub(crate) plugins: Arc, } @@ -233,9 +209,7 @@ impl SubgraphServiceFactory for MockSubgraphFactory { self.plugins .iter() .rev() - .fold(service.clone().boxed(), |acc, (_, e)| { - e.subgraph_service(name, acc) - }) + .fold(service.make(), |acc, (_, e)| e.subgraph_service(name, acc)) }) } } diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 90b150c391..4668de06ff 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -1202,13 +1202,12 @@ mod tests { use std::sync::Arc; use http::Method; - use tower::ServiceBuilder; - use tower::ServiceExt; use super::*; use crate::json_ext::PathElement; use crate::plugin::test::MockSubgraphFactory; use crate::query_planner::fetch::FetchNode; + use crate::services::subgraph_service::MakeSubgraphService; macro_rules! test_query_plan { () => { @@ -1267,9 +1266,7 @@ mod tests { let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_products_service.build().boxed()), + Arc::new(mock_products_service.build()) as Arc, )]), plugins: Default::default(), }); @@ -1328,9 +1325,7 @@ mod tests { let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_products_service.build().boxed()), + Arc::new(mock_products_service.build()) as Arc, )]), plugins: Default::default(), }); @@ -1383,9 +1378,7 @@ mod tests { let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_products_service.build().boxed()), + Arc::new(mock_products_service.build()) as Arc, )]), plugins: Default::default(), }); @@ -1514,15 +1507,11 @@ mod tests { subgraphs: HashMap::from([ ( "X".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_x_service.build().boxed()), + Arc::new(mock_x_service.build()) as Arc, ), ( "Y".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_y_service.build().boxed()), + Arc::new(mock_y_service.build()) as Arc, ), ]), plugins: Default::default(), From bf000e6ed777302b2fb4e5892c2848d302b70574 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 15:51:52 +0200 Subject: [PATCH 2/8] make mock services that can panic the services based on tower-test were isolated in a task, which means that any panic due to mockall would stay in that task and not affect the rest of the tests. This rewrites the mock service system to provide clonable mock services that can panic --- apollo-router/src/layers/async_checkpoint.rs | 67 +++++----- apollo-router/src/layers/cache.rs | 40 +++++- .../src/layers/map_future_with_context.rs | 2 +- apollo-router/src/layers/sync_checkpoint.rs | 18 +-- apollo-router/src/plugin/test/mod.rs | 37 +++--- apollo-router/src/plugin/test/service.rs | 43 +++--- apollo-router/src/plugins/csrf.rs | 6 +- apollo-router/src/plugins/forbid_mutations.rs | 11 +- apollo-router/src/plugins/headers.rs | 15 +-- apollo-router/src/plugins/override_url.rs | 2 +- apollo-router/src/plugins/rhai.rs | 26 ++-- apollo-router/src/plugins/telemetry/mod.rs | 8 +- apollo-router/src/query_planner/mod.rs | 125 +++++++++++------- .../layers/allow_only_http_post_mutations.rs | 16 +-- apollo-router/src/services/layers/apq.rs | 33 ++--- .../services/layers/ensure_query_presence.rs | 12 +- 16 files changed, 245 insertions(+), 216 deletions(-) diff --git a/apollo-router/src/layers/async_checkpoint.rs b/apollo-router/src/layers/async_checkpoint.rs index 38c1134dcb..26dc7af467 100644 --- a/apollo-router/src/layers/async_checkpoint.rs +++ b/apollo-router/src/layers/async_checkpoint.rs @@ -153,23 +153,24 @@ mod async_checkpoint_tests { let expected_label = "from_mock_service"; let mut execution_service = MockExecutionService::new(); - - execution_service - .expect_call() - .times(1) - .returning(move |_req: crate::ExecutionRequest| { - Ok(ExecutionResponse::fake_builder() - .label(expected_label.to_string()) - .build()) - }); - - let service = execution_service.build(); + execution_service.expect_clone().return_once(move || { + let mut execution_service = MockExecutionService::new(); + execution_service.expect_call().times(1).returning( + move |_req: crate::ExecutionRequest| { + Ok(ExecutionResponse::fake_builder() + .label(expected_label.to_string()) + .build()) + }, + ); + + execution_service + }); let service_stack = ServiceBuilder::new() .checkpoint_async(|req: crate::ExecutionRequest| async { Ok(ControlFlow::Continue(req)) }) - .service(service); + .service(execution_service); let request = ExecutionRequest::fake_builder().build(); @@ -191,20 +192,22 @@ mod async_checkpoint_tests { let expected_label = "from_mock_service"; let mut router_service = MockExecutionService::new(); - router_service - .expect_call() - .times(1) - .returning(move |_req| { - Ok(ExecutionResponse::fake_builder() - .label(expected_label.to_string()) - .build()) - }); - - let service = router_service.build(); + router_service.expect_clone().return_once(move || { + let mut router_service = MockExecutionService::new(); + router_service + .expect_call() + .times(1) + .returning(move |_req| { + Ok(ExecutionResponse::fake_builder() + .label(expected_label.to_string()) + .build()) + }); + router_service + }); let service_stack = AsyncCheckpointLayer::new(|req| async { Ok(ControlFlow::Continue(req)) }) - .layer(service); + .layer(router_service); let request = ExecutionRequest::fake_builder().build(); @@ -224,9 +227,10 @@ mod async_checkpoint_tests { #[tokio::test] async fn test_return() { let expected_label = "returned_before_mock_service"; - let router_service = MockExecutionService::new(); - - let service = router_service.build(); + let mut router_service = MockExecutionService::new(); + router_service + .expect_clone() + .return_once(move || MockExecutionService::new()); let service_stack = AsyncCheckpointLayer::new(|_req| async { Ok(ControlFlow::Break( @@ -235,7 +239,7 @@ mod async_checkpoint_tests { .build(), )) }) - .layer(service); + .layer(router_service); let request = ExecutionRequest::fake_builder().build(); @@ -255,15 +259,16 @@ mod async_checkpoint_tests { #[tokio::test] async fn test_error() { let expected_error = "checkpoint_error"; - let router_service = MockExecutionService::new(); - - let service = router_service.build(); + let mut router_service = MockExecutionService::new(); + router_service + .expect_clone() + .return_once(move || MockExecutionService::new()); let service_stack = AsyncCheckpointLayer::new( move |_req| async move { Err(BoxError::from(expected_error)) }, ) - .layer(service); + .layer(router_service); let request = ExecutionRequest::fake_builder().build(); diff --git a/apollo-router/src/layers/cache.rs b/apollo-router/src/layers/cache.rs index 58c65d5b74..a65ec9ce58 100644 --- a/apollo-router/src/layers/cache.rs +++ b/apollo-router/src/layers/cache.rs @@ -170,6 +170,7 @@ where } #[cfg(test)] +#[allow(unreachable_pub)] mod test { use std::time::Duration; @@ -225,6 +226,10 @@ mod test { }) }); + mock_service + .expect_clone() + .return_once(move || MockABService::new()); + let mut service = create_service(mock_service); let expected = Ok(B { @@ -255,13 +260,16 @@ mod test { } #[tokio::test] - async fn is_should_cache_err() { + async fn it_should_cache_err() { let mut mock_service = MockABService::new(); mock_service .expect_call() .times(1) .returning(move |a| Err(BoxError::from(format!("{} err", a.key)))); + mock_service + .expect_clone() + .return_once(move || MockABService::new()); let mut service = create_service(mock_service); @@ -295,7 +303,7 @@ mod test { mock_service .expect_call() - .times(2) + .times(1) .with(eq(A { key: "Not cacheable".into(), value: "Needed".into(), @@ -306,6 +314,27 @@ mod test { value: "there".into(), }) }); + mock_service.expect_clone().return_once(move || { + let mut mock_service = MockABService::new(); + mock_service + .expect_call() + .times(1) + .with(eq(A { + key: "Not cacheable".into(), + value: "Needed".into(), + })) + .returning(move |a| { + Ok(B { + key: a.key, + value: "there".into(), + }) + }); + mock_service + .expect_clone() + .return_once(move || MockABService::new()); + + mock_service + }); let mut service = create_service(mock_service); @@ -324,7 +353,7 @@ mod test { } #[tokio::test] - async fn it_should_dedupe_in_flight_calls() { + async fn it_should_deduplicate_in_flight_calls() { let mut mock_service = MockABService::new(); mock_service @@ -340,6 +369,9 @@ mod test { value: "there".into(), }) }); + mock_service + .expect_clone() + .return_once(move || MockABService::new()); let mut service = create_service(mock_service); @@ -387,7 +419,7 @@ mod test { }, )) .filter_async(Slow::default()) - .service(mock_service.build()) + .service(mock_service) .map_err(|e: BoxError| e.to_string()) } } diff --git a/apollo-router/src/layers/map_future_with_context.rs b/apollo-router/src/layers/map_future_with_context.rs index bdbca1bd4b..c7c39f3fb2 100644 --- a/apollo-router/src/layers/map_future_with_context.rs +++ b/apollo-router/src/layers/map_future_with_context.rs @@ -114,7 +114,7 @@ mod test { }) }, ) - .service(mock_service.build()); + .service(mock_service); let result = service .ready() diff --git a/apollo-router/src/layers/sync_checkpoint.rs b/apollo-router/src/layers/sync_checkpoint.rs index d3a97ee130..3f84ad0563 100644 --- a/apollo-router/src/layers/sync_checkpoint.rs +++ b/apollo-router/src/layers/sync_checkpoint.rs @@ -191,11 +191,9 @@ mod checkpoint_tests { .build()) }); - let service = execution_service.build(); - let service_stack = ServiceBuilder::new() .checkpoint(|req: crate::ExecutionRequest| Ok(ControlFlow::Continue(req))) - .service(service); + .service(execution_service); let request = ExecutionRequest::fake_builder().build(); @@ -226,10 +224,8 @@ mod checkpoint_tests { .build()) }); - let service = router_service.build(); - let service_stack = - CheckpointLayer::new(|req| Ok(ControlFlow::Continue(req))).layer(service); + CheckpointLayer::new(|req| Ok(ControlFlow::Continue(req))).layer(router_service); let request = ExecutionRequest::fake_builder().build(); @@ -251,8 +247,6 @@ mod checkpoint_tests { let expected_label = "returned_before_mock_service"; let router_service = MockExecutionService::new(); - let service = router_service.build(); - let service_stack = CheckpointLayer::new(|_req| { Ok(ControlFlow::Break( ExecutionResponse::fake_builder() @@ -260,7 +254,7 @@ mod checkpoint_tests { .build(), )) }) - .layer(service); + .layer(router_service); let request = ExecutionRequest::fake_builder().build(); @@ -282,10 +276,8 @@ mod checkpoint_tests { let expected_error = "checkpoint_error"; let router_service = MockExecutionService::new(); - let service = router_service.build(); - - let service_stack = - CheckpointLayer::new(move |_req| Err(BoxError::from(expected_error))).layer(service); + let service_stack = CheckpointLayer::new(move |_req| Err(BoxError::from(expected_error))) + .layer(router_service); let request = ExecutionRequest::fake_builder().build(); diff --git a/apollo-router/src/plugin/test/mod.rs b/apollo-router/src/plugin/test/mod.rs index f01e2fe6f3..f09eff8c76 100644 --- a/apollo-router/src/plugin/test/mod.rs +++ b/apollo-router/src/plugin/test/mod.rs @@ -124,7 +124,7 @@ impl PluginTestHarness { .boxed(); let query_planner_service = plugin.query_planning_service( mock_query_planner_service - .map(|s| s.build().boxed()) + .map(|s| s.boxed()) .unwrap_or(query_planner), ); @@ -136,27 +136,22 @@ impl PluginTestHarness { let plugins = Arc::new(plugins); let apq = APQLayer::with_cache(DeduplicatingCache::new().await); - let router_service = mock_router_service - .map(|s| s.build().boxed()) - .unwrap_or_else(|| { - BoxService::new( - RouterService::builder() - .query_planner_service(Buffer::new( - query_planner_service, - DEFAULT_BUFFER_SIZE, - )) - .execution_service_factory(ExecutionCreator { - schema: schema.clone(), + let router_service = mock_router_service.map(|s| s.boxed()).unwrap_or_else(|| { + BoxService::new( + RouterService::builder() + .query_planner_service(Buffer::new(query_planner_service, DEFAULT_BUFFER_SIZE)) + .execution_service_factory(ExecutionCreator { + schema: schema.clone(), + plugins: plugins.clone(), + subgraph_creator: Arc::new(MockSubgraphFactory { plugins: plugins.clone(), - subgraph_creator: Arc::new(MockSubgraphFactory { - plugins: plugins.clone(), - subgraphs: subgraph_services, - }), - }) - .schema(schema.clone()) - .build(), - ) - }); + subgraphs: subgraph_services, + }), + }) + .schema(schema.clone()) + .build(), + ) + }); let router_service = ServiceBuilder::new() .layer(apq) .layer(EnsureQueryPresence::default()) diff --git a/apollo-router/src/plugin/test/service.rs b/apollo-router/src/plugin/test/service.rs index 0ac6491e84..ac840e2819 100644 --- a/apollo-router/src/plugin/test/service.rs +++ b/apollo-router/src/plugin/test/service.rs @@ -1,3 +1,4 @@ +#![allow(dead_code, unreachable_pub)] use crate::ExecutionRequest; use crate::ExecutionResponse; use crate::QueryPlannerRequest; @@ -12,29 +13,31 @@ use crate::SubgraphResponse; macro_rules! mock_service { ($name:ident, $request_type:ty, $response_type:ty) => { paste::item! { - #[mockall::automock] - #[allow(dead_code, unreachable_pub)] - pub trait [<$name Service>] { - fn call(&self, req: $request_type) -> Result<$response_type, tower::BoxError>; - } + mockall::mock! { + #[derive(Debug)] + #[allow(dead_code)] + pub [<$name Service>] { + pub fn call(&mut self, req: $request_type) -> Result<$response_type, tower::BoxError>; + } - impl [] { - #[allow(unreachable_pub)] - pub fn build(self) -> tower_test::mock::Mock<$request_type,$response_type> { - let (service, mut handle) = tower_test::mock::spawn(); + #[allow(dead_code)] + impl Clone for [<$name Service>] { + fn clone(&self) -> []; + } + } - tokio::spawn(async move { - loop { - while let Some((request, responder)) = handle.next_request().await { - match self.call(request) { - Ok(response) => responder.send_response(response), - Err(err) => responder.send_error(err), - } - } - } - }); + // mockall does not handle well the lifetime on Context + impl tower::Service<$request_type> for [] { + type Response = $response_type; + type Error = tower::BoxError; + type Future = futures::future::BoxFuture<'static, Result>; - service.into_inner() + fn poll_ready(&mut self, _cx: &mut std::task::Context<'_>) -> std::task::Poll> { + std::task::Poll::Ready(Ok(())) + } + fn call(&mut self, req: $request_type) -> Self::Future { + let r = self.call(req); + Box::pin(async move { r }) } } } diff --git a/apollo-router/src/plugins/csrf.rs b/apollo-router/src/plugins/csrf.rs index 8752e4a50b..6daf76a3f2 100644 --- a/apollo-router/src/plugins/csrf.rs +++ b/apollo-router/src/plugins/csrf.rs @@ -304,11 +304,10 @@ mod csrf_tests { .unwrap()) }); - let mock = mock_service.build(); let service_stack = Csrf::new(config) .await .unwrap() - .router_service(mock.boxed()); + .router_service(mock_service.boxed()); let res = service_stack .oneshot(request) .await @@ -321,11 +320,10 @@ mod csrf_tests { } async fn assert_rejected(config: CSRFConfig, request: RouterRequest) { - let mock = MockRouterService::new().build(); let service_stack = Csrf::new(config) .await .unwrap() - .router_service(mock.boxed()); + .router_service(MockRouterService::new().boxed()); let res = service_stack .oneshot(request) .await diff --git a/apollo-router/src/plugins/forbid_mutations.rs b/apollo-router/src/plugins/forbid_mutations.rs index 3e7117b4aa..bdab63d792 100644 --- a/apollo-router/src/plugins/forbid_mutations.rs +++ b/apollo-router/src/plugins/forbid_mutations.rs @@ -85,12 +85,10 @@ mod forbid_http_get_mutations_tests { .times(1) .returning(move |_| Ok(ExecutionResponse::fake_builder().build())); - let mock = mock_service.build(); - let service_stack = ForbidMutations::new(true) .await .expect("couldnt' create forbid mutations plugin") - .execution_service(mock.boxed()); + .execution_service(mock_service.boxed()); let request = create_request(Method::GET, OperationKind::Query); @@ -113,11 +111,10 @@ mod forbid_http_get_mutations_tests { }; let expected_status = StatusCode::BAD_REQUEST; - let mock = MockExecutionService::new().build(); let service_stack = ForbidMutations::new(true) .await .expect("couldnt' create forbid mutations plugin") - .execution_service(mock.boxed()); + .execution_service(MockExecutionService::new().boxed()); let request = create_request(Method::GET, OperationKind::Mutation); let mut actual_error = service_stack.oneshot(request).await.unwrap(); @@ -135,12 +132,10 @@ mod forbid_http_get_mutations_tests { .times(1) .returning(move |_| Ok(ExecutionResponse::fake_builder().build())); - let mock = mock_service.build(); - let service_stack = ForbidMutations::new(false) .await .expect("couldnt' create forbid mutations plugin") - .execution_service(mock.boxed()); + .execution_service(mock_service.boxed()); let request = create_request(Method::GET, OperationKind::Mutation); diff --git a/apollo-router/src/plugins/headers.rs b/apollo-router/src/plugins/headers.rs index 7ec330ba97..6258320e02 100644 --- a/apollo-router/src/plugins/headers.rs +++ b/apollo-router/src/plugins/headers.rs @@ -384,7 +384,7 @@ mod test { name: "c".try_into()?, value: "d".try_into()?, })]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -399,8 +399,7 @@ mod test { .returning(example_response); let mut service = - HeadersLayer::new(vec![Operation::Remove(Remove::Named("aa".try_into()?))]) - .layer(mock.build()); + HeadersLayer::new(vec![Operation::Remove(Remove::Named("aa".try_into()?))]).layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -417,7 +416,7 @@ mod test { let mut service = HeadersLayer::new(vec![Operation::Remove(Remove::Matching( Regex::from_str("a[ab]")?, ))]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -442,7 +441,7 @@ mod test { let mut service = HeadersLayer::new(vec![Operation::Propagate(Propagate::Matching { matching: Regex::from_str("d[ab]")?, })]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -468,7 +467,7 @@ mod test { rename: None, default: None, })]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -494,7 +493,7 @@ mod test { rename: Some("ea".try_into()?), default: None, })]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) @@ -520,7 +519,7 @@ mod test { rename: None, default: Some("defaulted".try_into()?), })]) - .layer(mock.build()); + .layer(mock); service.ready().await?.call(example_request()).await?; Ok(()) diff --git a/apollo-router/src/plugins/override_url.rs b/apollo-router/src/plugins/override_url.rs index feee4f1af2..ff8615129e 100644 --- a/apollo-router/src/plugins/override_url.rs +++ b/apollo-router/src/plugins/override_url.rs @@ -97,7 +97,7 @@ mod tests { .await .unwrap(); let mut subgraph_service = - dyn_plugin.subgraph_service("test_one", BoxService::new(mock_service.build())); + dyn_plugin.subgraph_service("test_one", BoxService::new(mock_service)); let context = Context::new(); context.insert("test".to_string(), 5i64).unwrap(); let subgraph_req = SubgraphRequest::fake_builder().context(context); diff --git a/apollo-router/src/plugins/rhai.rs b/apollo-router/src/plugins/rhai.rs index ddd444faef..2aff75a14d 100644 --- a/apollo-router/src/plugins/rhai.rs +++ b/apollo-router/src/plugins/rhai.rs @@ -1410,7 +1410,7 @@ mod tests { ) .await .unwrap(); - let mut router_service = dyn_plugin.router_service(BoxService::new(mock_service.build())); + let mut router_service = dyn_plugin.router_service(BoxService::new(mock_service)); let context = Context::new(); context.insert("test", 5i64).unwrap(); let router_req = RouterRequest::fake_builder().context(context).build()?; @@ -1445,14 +1445,19 @@ mod tests { #[tokio::test] async fn rhai_plugin_execution_service_error() -> Result<(), BoxError> { let mut mock_service = MockExecutionService::new(); - mock_service - .expect_call() - .times(1) - .returning(move |req: ExecutionRequest| { - Ok(ExecutionResponse::fake_builder() - .context(req.context) - .build()) - }); + mock_service.expect_clone().return_once(move || { + let mut mock_service = MockExecutionService::new(); + mock_service + .expect_call() + .times(1) + .returning(move |req: ExecutionRequest| { + Ok(ExecutionResponse::fake_builder() + .context(req.context) + .build()) + }); + + mock_service + }); let dyn_plugin: Box = crate::plugin::plugins() .get("apollo.rhai") @@ -1462,8 +1467,7 @@ mod tests { ) .await .unwrap(); - let mut router_service = - dyn_plugin.execution_service(BoxService::new(mock_service.build())); + let mut router_service = dyn_plugin.execution_service(BoxService::new(mock_service)); let fake_req = http_ext::Request::fake_builder() .header("x-custom-header", "CUSTOM_VALUE") .body(Request::builder().query(String::new()).build()) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 4150d2d6b4..e78715e392 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -1213,7 +1213,7 @@ mod tests { ) .await .unwrap(); - let mut router_service = dyn_plugin.router_service(BoxService::new(mock_service.build())); + let mut router_service = dyn_plugin.router_service(BoxService::new(mock_service)); let router_req = RouterRequest::fake_builder().header("test", "my_value_set"); let _router_response = router_service @@ -1227,10 +1227,8 @@ mod tests { .await .unwrap(); - let mut subgraph_service = dyn_plugin.subgraph_service( - "my_subgraph_name", - BoxService::new(mock_subgraph_service.build()), - ); + let mut subgraph_service = + dyn_plugin.subgraph_service("my_subgraph_name", BoxService::new(mock_subgraph_service)); let subgraph_req = SubgraphRequest::fake_builder() .subgraph_request( http_ext::Request::fake_builder() diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 4668de06ff..d10a54cbda 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -1247,6 +1247,7 @@ mod tests { /// The query planner reports the failed subgraph fetch as an error with a reason of "service /// closed", which is what this test expects. #[tokio::test] + #[should_panic(expected = "this panic should be propagated to the test harness")] async fn mock_subgraph_service_withf_panics_should_be_reported_as_service_closed() { let query_plan: QueryPlan = QueryPlan { root: serde_json::from_str(test_query_plan!()).unwrap(), @@ -1261,12 +1262,19 @@ mod tests { mock_products_service.expect_call().times(1).withf(|_| { panic!("this panic should be propagated to the test harness"); }); + mock_products_service.expect_clone().return_once(|| { + let mut mock_products_service = plugin::test::MockSubgraphService::new(); + mock_products_service.expect_call().times(1).withf(|_| { + panic!("this panic should be propagated to the test harness"); + }); + mock_products_service + }); let (sender, _) = futures::channel::mpsc::channel(10); let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - Arc::new(mock_products_service.build()) as Arc, + Arc::new(mock_products_service) as Arc, )]), plugins: Default::default(), }); @@ -1309,23 +1317,27 @@ mod tests { let inner_succeeded = Arc::clone(&succeeded); let mut mock_products_service = plugin::test::MockSubgraphService::new(); - mock_products_service - .expect_call() - .times(1) - .withf(move |request| { - let matches = request.subgraph_request.body().operation_name - == Some("topProducts_product_0".into()); - inner_succeeded.store(matches, Ordering::SeqCst); - matches - }) - .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + mock_products_service.expect_clone().return_once(|| { + let mut mock_products_service = plugin::test::MockSubgraphService::new(); + mock_products_service + .expect_call() + .times(1) + .withf(move |request| { + let matches = request.subgraph_request.body().operation_name + == Some("topProducts_product_0".into()); + inner_succeeded.store(matches, Ordering::SeqCst); + matches + }) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + mock_products_service + }); let (sender, _) = futures::channel::mpsc::channel(10); let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - Arc::new(mock_products_service.build()) as Arc, + Arc::new(mock_products_service) as Arc, )]), plugins: Default::default(), }); @@ -1364,21 +1376,27 @@ mod tests { let inner_succeeded = Arc::clone(&succeeded); let mut mock_products_service = plugin::test::MockSubgraphService::new(); - mock_products_service - .expect_call() - .times(1) - .withf(move |request| { - let matches = request.subgraph_request.method() == Method::POST; - inner_succeeded.store(matches, Ordering::SeqCst); - matches - }) - .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + + mock_products_service.expect_clone().return_once(|| { + let mut mock_products_service = plugin::test::MockSubgraphService::new(); + mock_products_service + .expect_call() + .times(1) + .withf(move |request| { + let matches = request.subgraph_request.method() == Method::POST; + inner_succeeded.store(matches, Ordering::SeqCst); + matches + }) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + mock_products_service + }); + let (sender, _) = futures::channel::mpsc::channel(10); let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([( "product".into(), - Arc::new(mock_products_service.build()) as Arc, + Arc::new(mock_products_service) as Arc, )]), plugins: Default::default(), }); @@ -1473,32 +1491,41 @@ mod tests { }; let mut mock_x_service = plugin::test::MockSubgraphService::new(); - mock_x_service - .expect_call() - .times(1) - .withf(move |_request| true) - .returning(|_| { - Ok(SubgraphResponse::fake_builder() - .data(serde_json::json! {{ - "t": {"id": 1234, - "__typename": "T", - "x": "X" - } - }}) - .build()) - }); + mock_x_service.expect_clone().return_once(|| { + let mut mock_x_service = plugin::test::MockSubgraphService::new(); + mock_x_service + .expect_call() + .times(1) + .withf(move |_request| true) + .returning(|_| { + Ok(SubgraphResponse::fake_builder() + .data(serde_json::json! {{ + "t": {"id": 1234, + "__typename": "T", + "x": "X" + } + }}) + .build()) + }); + mock_x_service + }); + let mut mock_y_service = plugin::test::MockSubgraphService::new(); - mock_y_service - .expect_call() - .times(1) - .withf(move |_request| true) - .returning(|_| { - Ok(SubgraphResponse::fake_builder() - .data(serde_json::json! {{ - "_entities": [{"y": "Y", "__typename": "T"}] - }}) - .build()) - }); + mock_y_service.expect_clone().return_once(|| { + let mut mock_y_service = plugin::test::MockSubgraphService::new(); + mock_y_service + .expect_call() + .times(1) + .withf(move |_request| true) + .returning(|_| { + Ok(SubgraphResponse::fake_builder() + .data(serde_json::json! {{ + "_entities": [{"y": "Y", "__typename": "T"}] + }}) + .build()) + }); + mock_y_service + }); let (sender, mut receiver) = futures::channel::mpsc::channel(10); @@ -1507,11 +1534,11 @@ mod tests { subgraphs: HashMap::from([ ( "X".into(), - Arc::new(mock_x_service.build()) as Arc, + Arc::new(mock_x_service) as Arc, ), ( "Y".into(), - Arc::new(mock_y_service.build()) as Arc, + Arc::new(mock_y_service) as Arc, ), ]), plugins: Default::default(), 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 af7c9e24e2..115a7ce07a 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 @@ -84,9 +84,7 @@ mod forbid_http_get_mutations_tests { .times(1) .returning(move |_| Ok(ExecutionResponse::fake_builder().build())); - let mock = mock_service.build(); - - let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock); + let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock_service); let http_post_query_plan_request = create_request(Method::POST, OperationKind::Query); @@ -109,9 +107,7 @@ mod forbid_http_get_mutations_tests { .times(1) .returning(move |_| Ok(ExecutionResponse::fake_builder().build())); - let mock = mock_service.build(); - - let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock); + let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock_service); let http_post_query_plan_request = create_request(Method::POST, OperationKind::Mutation); @@ -134,9 +130,7 @@ mod forbid_http_get_mutations_tests { .times(1) .returning(move |_| Ok(ExecutionResponse::fake_builder().build())); - let mock = mock_service.build(); - - let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock); + let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock_service); let http_post_query_plan_request = create_request(Method::GET, OperationKind::Query); @@ -161,8 +155,8 @@ mod forbid_http_get_mutations_tests { let expected_status = StatusCode::METHOD_NOT_ALLOWED; let expected_allow_header = "POST"; - let mock = MockExecutionService::new().build(); - let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock); + let mut service_stack = + AllowOnlyHttpPostMutationsLayer::default().layer(MockExecutionService::new()); let forbidden_requests = [ Method::GET, diff --git a/apollo-router/src/services/layers/apq.rs b/apollo-router/src/services/layers/apq.rs index 520d4173ce..1cff8a77d6 100644 --- a/apollo-router/src/services/layers/apq.rs +++ b/apollo-router/src/services/layers/apq.rs @@ -229,10 +229,8 @@ mod apq_tests { .expect("expecting valid request")) }); - let mock = mock_service.build(); - let apq = APQLayer::with_cache(DeduplicatingCache::new().await); - let mut service_stack = apq.layer(mock); + let mut service_stack = apq.layer(mock_service); let extensions = HashMap::from([( "persistedQuery".to_string(), @@ -296,36 +294,31 @@ mod apq_tests { .unwrap(), }; - let mut mock_service_builder = MockRouterService::new(); + let mut mock_service = MockRouterService::new(); // the first one should have lead to an APQ error // claiming the server doesn't have a query string for a given hash // it should have not been forwarded to our mock service // the second one should have the right APQ header and the full query string - mock_service_builder - .expect_call() - .times(1) - .returning(move |req| { - let body = req.originating_request.body(); - let as_json = body.extensions.get("persistedQuery").unwrap(); + mock_service.expect_call().times(1).returning(move |req| { + let body = req.originating_request.body(); + let as_json = body.extensions.get("persistedQuery").unwrap(); - let persisted_query: PersistedQuery = - serde_json_bytes::from_value(as_json.clone()).unwrap(); + let persisted_query: PersistedQuery = + serde_json_bytes::from_value(as_json.clone()).unwrap(); - assert_eq!(persisted_query.sha256hash, hash2); + assert_eq!(persisted_query.sha256hash, hash2); - assert!(body.query.is_some()); + assert!(body.query.is_some()); - Ok(RouterResponse::fake_builder() - .build() - .expect("expecting valid request")) - }); + Ok(RouterResponse::fake_builder() + .build() + .expect("expecting valid request")) + }); // the last call should be an APQ error. // the provided hash was wrong, so the query wasn't inserted into the cache. - let mock_service = mock_service_builder.build(); - let apq = APQLayer::with_cache(DeduplicatingCache::new().await); let mut service_stack = apq.layer(mock_service); diff --git a/apollo-router/src/services/layers/ensure_query_presence.rs b/apollo-router/src/services/layers/ensure_query_presence.rs index c1fb20271e..10783f3880 100644 --- a/apollo-router/src/services/layers/ensure_query_presence.rs +++ b/apollo-router/src/services/layers/ensure_query_presence.rs @@ -74,8 +74,7 @@ mod ensure_query_presence_tests { .expect("expecting valid request")) }); - let mock = mock_service.build(); - let service_stack = EnsureQueryPresence::default().layer(mock); + let service_stack = EnsureQueryPresence::default().layer(mock_service); let request: crate::RouterRequest = RouterRequest::fake_builder() .query("{__typename}".to_string()) @@ -89,10 +88,7 @@ mod ensure_query_presence_tests { async fn it_fails_on_empty_query() { let expected_error = "Must provide query string."; - let mock_service = MockRouterService::new(); - let mock = mock_service.build(); - - let service_stack = EnsureQueryPresence::default().layer(mock); + let service_stack = EnsureQueryPresence::default().layer(MockRouterService::new()); let request: crate::RouterRequest = RouterRequest::fake_builder() .query("".to_string()) @@ -115,9 +111,7 @@ mod ensure_query_presence_tests { async fn it_fails_on_no_query() { let expected_error = "Must provide query string."; - let mock_service = MockRouterService::new(); - let mock = mock_service.build(); - let service_stack = EnsureQueryPresence::default().layer(mock); + let service_stack = EnsureQueryPresence::default().layer(MockRouterService::new()); let request: crate::RouterRequest = RouterRequest::fake_builder() .build() From 7ec50a62ee465143237ce14ee9d43937df385c42 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 16:14:01 +0200 Subject: [PATCH 3/8] lint --- apollo-router/src/layers/async_checkpoint.rs | 4 ++-- apollo-router/src/layers/cache.rs | 18 +++++------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/apollo-router/src/layers/async_checkpoint.rs b/apollo-router/src/layers/async_checkpoint.rs index 26dc7af467..0a96c8bbb9 100644 --- a/apollo-router/src/layers/async_checkpoint.rs +++ b/apollo-router/src/layers/async_checkpoint.rs @@ -230,7 +230,7 @@ mod async_checkpoint_tests { let mut router_service = MockExecutionService::new(); router_service .expect_clone() - .return_once(move || MockExecutionService::new()); + .return_once(MockExecutionService::new); let service_stack = AsyncCheckpointLayer::new(|_req| async { Ok(ControlFlow::Break( @@ -262,7 +262,7 @@ mod async_checkpoint_tests { let mut router_service = MockExecutionService::new(); router_service .expect_clone() - .return_once(move || MockExecutionService::new()); + .return_once(MockExecutionService::new); let service_stack = AsyncCheckpointLayer::new( diff --git a/apollo-router/src/layers/cache.rs b/apollo-router/src/layers/cache.rs index a65ec9ce58..0071a1af7c 100644 --- a/apollo-router/src/layers/cache.rs +++ b/apollo-router/src/layers/cache.rs @@ -170,7 +170,7 @@ where } #[cfg(test)] -#[allow(unreachable_pub)] +#[allow(dead_code, unreachable_pub)] mod test { use std::time::Duration; @@ -226,9 +226,7 @@ mod test { }) }); - mock_service - .expect_clone() - .return_once(move || MockABService::new()); + mock_service.expect_clone().return_once(MockABService::new); let mut service = create_service(mock_service); @@ -267,9 +265,7 @@ mod test { .expect_call() .times(1) .returning(move |a| Err(BoxError::from(format!("{} err", a.key)))); - mock_service - .expect_clone() - .return_once(move || MockABService::new()); + mock_service.expect_clone().return_once(MockABService::new); let mut service = create_service(mock_service); @@ -329,9 +325,7 @@ mod test { value: "there".into(), }) }); - mock_service - .expect_clone() - .return_once(move || MockABService::new()); + mock_service.expect_clone().return_once(MockABService::new); mock_service }); @@ -369,9 +363,7 @@ mod test { value: "there".into(), }) }); - mock_service - .expect_clone() - .return_once(move || MockABService::new()); + mock_service.expect_clone().return_once(MockABService::new); let mut service = create_service(mock_service); From 3054580817d9ac4d535236a4d60d5988ee9006bd Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 16:14:27 +0200 Subject: [PATCH 4/8] examples --- examples/add-timestamp-header/src/main.rs | 8 +++----- .../src/allow_client_id_from_file.rs | 12 +++++------ examples/cookies-to-headers/src/main.rs | 8 +++----- .../src/forbid_anonymous_operations.rs | 12 +++++------ examples/jwt-auth/src/jwt.rs | 20 +++++++++---------- examples/op-name-to-header/src/main.rs | 8 +++----- examples/rhai-logging/src/main.rs | 8 +++----- .../src/propagate_status_code.rs | 8 -------- 8 files changed, 31 insertions(+), 53 deletions(-) diff --git a/examples/add-timestamp-header/src/main.rs b/examples/add-timestamp-header/src/main.rs index 9e4a4a230a..ecf27e3251 100644 --- a/examples/add-timestamp-header/src/main.rs +++ b/examples/add-timestamp-header/src/main.rs @@ -24,13 +24,14 @@ mod tests { #[tokio::test] async fn test_router_service_adds_timestamp_header() { // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once - mock.expect_call() + mock_service + .expect_call() .once() .returning(move |req: RouterRequest| { // Preserve our context from request to response @@ -41,9 +42,6 @@ mod tests { .unwrap()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - let conf: Conf = serde_json::from_value(serde_json::json!({ "scripts": "src", "main": "add_timestamp_header.rhai", diff --git a/examples/async-auth/src/allow_client_id_from_file.rs b/examples/async-auth/src/allow_client_id_from_file.rs index 9da9498dd5..eaa6d44dbb 100644 --- a/examples/async-auth/src/allow_client_id_from_file.rs +++ b/examples/async-auth/src/allow_client_id_from_file.rs @@ -207,7 +207,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know AllowClientIdFromFile did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, AllowClientIdFromFile is `decorating` or `wrapping` our mock_service. let service_stack = AllowClientIdFromFile::new(AllowClientIdConfig { @@ -247,7 +247,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know AllowClientIdFromFile did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, AllowClientIdFromFile is `decorating` or `wrapping` our mock_service. let service_stack = AllowClientIdFromFile::new(AllowClientIdConfig { @@ -287,13 +287,14 @@ mod tests { let valid_client_id = "jeremy"; // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once, with the expected operation_name - mock.expect_call() + mock_service + .expect_call() .times(1) .returning(move |req: RouterRequest| { assert_eq!( @@ -314,9 +315,6 @@ mod tests { .unwrap()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - // In this service_stack, AllowClientIdFromFile is `decorating` or `wrapping` our mock_service. let service_stack = AllowClientIdFromFile::new(AllowClientIdConfig { path: "allowedClientIds.json".to_string(), diff --git a/examples/cookies-to-headers/src/main.rs b/examples/cookies-to-headers/src/main.rs index 6d94f05dc1..448a60ea97 100644 --- a/examples/cookies-to-headers/src/main.rs +++ b/examples/cookies-to-headers/src/main.rs @@ -47,13 +47,14 @@ mod tests { #[tokio::test] async fn test_subgraph_processes_cookies() { // create a mock service we will use to test our plugin - let mut mock = test::MockSubgraphService::new(); + let mut mock_service = test::MockSubgraphService::new(); // The expected reply is going to be JSON returned in the SubgraphResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once - mock.expect_call() + mock_service + .expect_call() .once() .returning(move |req: SubgraphRequest| { // Let's make sure our request contains our new headers @@ -76,9 +77,6 @@ mod tests { .build()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - let conf: Conf = serde_json::from_value(serde_json::json!({ "scripts": "src", "main": "cookies_to_headers.rhai", diff --git a/examples/forbid-anonymous-operations/src/forbid_anonymous_operations.rs b/examples/forbid-anonymous-operations/src/forbid_anonymous_operations.rs index 4a8332c396..047172a3f0 100644 --- a/examples/forbid-anonymous-operations/src/forbid_anonymous_operations.rs +++ b/examples/forbid-anonymous-operations/src/forbid_anonymous_operations.rs @@ -126,7 +126,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know ForbidAnonymousOperations did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, ForbidAnonymousOperations is `decorating` or `wrapping` our mock_service. let service_stack = @@ -161,7 +161,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know ForbidAnonymousOperations did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, ForbidAnonymousOperations is `decorating` or `wrapping` our mock_service. let service_stack = @@ -196,13 +196,14 @@ mod tests { let operation_name = "validOperationName"; // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once, with the expected operation_name - mock.expect_call() + mock_service + .expect_call() .times(1) .returning(move |req: RouterRequest| { assert_eq!( @@ -222,9 +223,6 @@ mod tests { .unwrap()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - // In this service_stack, ForbidAnonymousOperations is `decorating` or `wrapping` our mock_service. let service_stack = ForbidAnonymousOperations::default().router_service(mock_service.boxed()); diff --git a/examples/jwt-auth/src/jwt.rs b/examples/jwt-auth/src/jwt.rs index 2fe83f46f7..179d249d87 100644 --- a/examples/jwt-auth/src/jwt.rs +++ b/examples/jwt-auth/src/jwt.rs @@ -416,7 +416,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, JwtAuth is `decorating` or `wrapping` our mock_service. let service_stack = JwtAuth::default().router_service(mock_service.boxed()); @@ -450,7 +450,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, JwtAuth is `decorating` or `wrapping` our mock_service. let service_stack = JwtAuth::default().router_service(mock_service.boxed()); @@ -485,7 +485,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, JwtAuth is `decorating` or `wrapping` our mock_service. let service_stack = JwtAuth::default().router_service(mock_service.boxed()); @@ -520,7 +520,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // In this service_stack, JwtAuth is `decorating` or `wrapping` our mock_service. let service_stack = JwtAuth::default().router_service(mock_service.boxed()); @@ -556,13 +556,14 @@ mod tests { #[tokio::test] async fn test_hmac_jwtauth_accepts_valid_tokens() { // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once - mock.expect_call() + mock_service + .expect_call() .once() .returning(move |req: RouterRequest| { // Let's make sure our request contains (some of) our JWTClaims @@ -581,9 +582,6 @@ mod tests { .expect("expecting valid request")) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - // Create valid configuration for testing HMAC algorithm HS256 let key = "629709bdc3bd794312ccc3a1c47beb03ac7310bc02d32d4587e59b5ad81c99ba"; let conf: Conf = serde_json::from_value(serde_json::json!({ @@ -639,7 +637,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // Create valid configuration for testing HMAC algorithm HS256 let key = "629709bdc3bd794312ccc3a1c47beb03ac7310bc02d32d4587e59b5ad81c99ba"; @@ -691,7 +689,7 @@ mod tests { // It does not have any behavior, because we do not expect it to be called. // If it is called, the test will panic, // letting us know JwtAuth did not behave as expected. - let mock_service = test::MockRouterService::new().build(); + let mock_service = test::MockRouterService::new(); // Create valid configuration for testing HMAC algorithm HS256 let key = "629709bdc3bd794312ccc3a1c47beb03ac7310bc02d32d4587e59b5ad81c99ba"; diff --git a/examples/op-name-to-header/src/main.rs b/examples/op-name-to-header/src/main.rs index cc0f759c42..0b1444b1bb 100644 --- a/examples/op-name-to-header/src/main.rs +++ b/examples/op-name-to-header/src/main.rs @@ -24,13 +24,14 @@ mod tests { #[tokio::test] async fn test_subgraph_processes_operation_name() { // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once - mock.expect_call() + mock_service + .expect_call() .once() .returning(move |req: RouterRequest| { // Let's make sure our request contains our new header @@ -47,9 +48,6 @@ mod tests { .unwrap()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - let conf: Conf = serde_json::from_value(serde_json::json!({ "scripts": "src", "main": "op_name_to_header.rhai", diff --git a/examples/rhai-logging/src/main.rs b/examples/rhai-logging/src/main.rs index 81942bb89c..58511df10b 100644 --- a/examples/rhai-logging/src/main.rs +++ b/examples/rhai-logging/src/main.rs @@ -25,13 +25,14 @@ mod tests { #[tokio::test] async fn test_subgraph_processes_operation_name() { // create a mock service we will use to test our plugin - let mut mock = test::MockRouterService::new(); + let mut mock_service = test::MockRouterService::new(); // The expected reply is going to be JSON returned in the RouterResponse { data } section. let expected_mock_response_data = "response created within the mock"; // Let's set up our mock to make sure it will be called once - mock.expect_call() + mock_service + .expect_call() .once() .returning(move |_req: RouterRequest| { Ok(RouterResponse::fake_builder() @@ -40,9 +41,6 @@ mod tests { .unwrap()) }); - // The mock has been set up, we can now build a service from it - let mock_service = mock.build(); - let conf: Conf = serde_json::from_value(serde_json::json!({ "scripts": "src", "main": "rhai_logging.rhai", diff --git a/examples/status-code-propagation/src/propagate_status_code.rs b/examples/status-code-propagation/src/propagate_status_code.rs index 01a6cb1545..4e44dfd2e1 100644 --- a/examples/status-code-propagation/src/propagate_status_code.rs +++ b/examples/status-code-propagation/src/propagate_status_code.rs @@ -141,8 +141,6 @@ mod tests { .build()) }); - let mock_service = mock_service.build(); - // In this service_stack, PropagateStatusCode is `decorating` or `wrapping` our mock_service. let service_stack = PropagateStatusCode::new(PropagateStatusCodeConfig { status_codes: vec![500, 403, 401], @@ -176,8 +174,6 @@ mod tests { .build()) }); - let mock_service = mock_service.build(); - // In this service_stack, PropagateStatusCode is `decorating` or `wrapping` our mock_service. let service_stack = PropagateStatusCode::new(PropagateStatusCodeConfig { status_codes: vec![500, 403, 401], @@ -222,8 +218,6 @@ mod tests { .unwrap()) }); - let mock_service = mock_service.build(); - // StatusCode::INTERNAL_SERVER_ERROR should have precedence here let service_stack = PropagateStatusCode::new(PropagateStatusCodeConfig { status_codes: vec![500, 403, 401], @@ -262,8 +256,6 @@ mod tests { .unwrap()) }); - let mock_service = mock_service.build(); - // In this service_stack, PropagateStatusCode is `decorating` or `wrapping` our mock_service. let service_stack = PropagateStatusCode::new(PropagateStatusCodeConfig { status_codes: vec![500, 403, 401], From 5b18431a9911497b5caf4d1dc54d4ff38f9521a4 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 16:23:42 +0200 Subject: [PATCH 5/8] changelog --- NEXT_CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index c28a293c22..e21d1237c3 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -35,6 +35,15 @@ This generic type complicates the API with limited benefit because we use BoxStr By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1420 +### Remove Buffer from Mock*Service ([PR #1440](https://github.com/apollographql/router/pull/1440) + +This removes the usage of `tower_test::mock::Mock` in mocked services because it isolated the service in a task +so panics triggered by mockall were not transmitted up to the unit test that should catch it. +This rewrites the mocked services API to remove the `build()` method, and make them clonable if needed, +using an `expect_clone` call with mockall. + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1440 + ## 🚀 Features ### Experimental support for the `@defer` directive ([PR #1182](https://github.com/apollographql/router/pull/1182) From 0e9bf9bdd3fdf49e32ab1699a8a97e862589ac82 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 16:31:43 +0200 Subject: [PATCH 6/8] fix tests --- apollo-router/src/plugins/telemetry/mod.rs | 2 +- apollo-router/src/query_planner/mod.rs | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index c2201c3f59..cf708934e1 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -1358,7 +1358,7 @@ mod tests { // Another subgraph let mut subgraph_service = dyn_plugin.subgraph_service( "my_subgraph_name_error", - BoxService::new(mock_subgraph_service_in_error.build()), + BoxService::new(mock_subgraph_service_in_error), ); let subgraph_req = SubgraphRequest::fake_builder() .subgraph_request( diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 8deac7e158..7d5bc6a009 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -1639,10 +1639,15 @@ mod tests { }; let mut mock_a_service = plugin::test::MockSubgraphService::new(); - mock_a_service - .expect_call() - .times(1) - .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + mock_a_service.expect_clone().returning(|| { + let mut mock_a_service = plugin::test::MockSubgraphService::new(); + mock_a_service + .expect_call() + .times(1) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + + mock_a_service + }); // the first fetch returned null, so there should never be a call to B let mut mock_b_service = plugin::test::MockSubgraphService::new(); @@ -1652,15 +1657,11 @@ mod tests { subgraphs: HashMap::from([ ( "A".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_a_service.build().boxed()), + Arc::new(mock_a_service) as Arc, ), ( "B".into(), - ServiceBuilder::new() - .buffer(1) - .service(mock_b_service.build().boxed()), + Arc::new(mock_b_service) as Arc, ), ]), plugins: Default::default(), From a8f184773ed513b670eb52a62a9ba648cf1a74ed Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 8 Aug 2022 16:38:13 +0200 Subject: [PATCH 7/8] remove tower-test from the executable build --- apollo-router/Cargo.toml | 1 - licenses.html | 2 -- 2 files changed, 3 deletions(-) diff --git a/apollo-router/Cargo.toml b/apollo-router/Cargo.toml index d5d4f4cb1a..5c0104e170 100644 --- a/apollo-router/Cargo.toml +++ b/apollo-router/Cargo.toml @@ -135,7 +135,6 @@ tower-http = { version = "0.3.4", features = [ "decompression-gzip", ] } tower-service = "0.3.2" -tower-test = "0.4.0" tracing = "=0.1.34" tracing-core = "=0.1.26" tracing-futures = { version = "0.2.5", features = ["futures-03"] } diff --git a/licenses.html b/licenses.html index e357b4d51a..c870860551 100644 --- a/licenses.html +++ b/licenses.html @@ -12332,7 +12332,6 @@

Used by:

  • tower
  • tower-layer
  • tower-service
    • -
  • tower-test
    • Copyright (c) 2019 Tower Contributors
 
@@ -12398,7 +12397,6 @@ 
    Used by:
    
                 
    MIT License
    
                 
    Used by:
    
                 
                 
    Copyright (c) 2021 Tokio Contributors

From ba31e703f9d71156fc4c03ab8b9de09870a1f81b Mon Sep 17 00:00:00 2001
From: Geoffroy Couprie 
Date: Fri, 12 Aug 2022 14:29:38 +0200
Subject: [PATCH 8/8] fix test

---
 apollo-router/src/plugins/traffic_shaping/mod.rs | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs
index 973d18ef8f..4130061035 100644
--- a/apollo-router/src/plugins/traffic_shaping/mod.rs
+++ b/apollo-router/src/plugins/traffic_shaping/mod.rs
@@ -526,13 +526,17 @@ mod test {
 
         let plugin = get_traffic_shaping_plugin(&config).await;
         let mut mock_service = MockRouterService::new();
-        mock_service.expect_call().times(2).returning(move |_| {
-            Ok(RouterResponse::fake_builder()
-                .data(json!({ "test": 1234_u32 }))
-                .build()
-                .unwrap())
+        mock_service.expect_clone().returning(|| {
+            let mut mock_service = MockRouterService::new();
+
+            mock_service.expect_call().times(0..2).returning(move |_| {
+                Ok(RouterResponse::fake_builder()
+                    .data(json!({ "test": 1234_u32 }))
+                    .build()
+                    .unwrap())
+            });
+            mock_service
         });
-        let mock_service = mock_service.build();
 
         let _response = plugin
             .router_service(mock_service.clone().boxed())