diff --git a/apollo-router-core/src/layers/apq.rs b/apollo-router-core/src/layers/apq.rs index fff4babe56..665e3ef320 100644 --- a/apollo-router-core/src/layers/apq.rs +++ b/apollo-router-core/src/layers/apq.rs @@ -244,7 +244,6 @@ mod apq_tests { async fn it_doesnt_update_the_cache_if_the_hash_is_not_valid() { let hash = Cow::from("ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b36"); let hash2 = hash.clone(); - let hash3 = hash.clone(); let expected_apq_miss_error = crate::Error { message: "PersistedQueryNotFound".to_string(), @@ -285,34 +284,9 @@ mod apq_tests { .build() .expect("expecting valid request")) }); - mock_service_builder - // the second last one should have the right APQ header and the full query string - // even though the query string wasn't provided by the client - .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(); - - assert_eq!(persisted_query.sha256hash, hash3); - - assert!(body.query.is_some()); - - let hash = hex::decode(hash3.as_bytes()).unwrap(); - - assert!(!query_matches_hash( - body.query.clone().unwrap_or_default().as_str(), - hash.as_slice() - )); - - 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(); diff --git a/apollo-router-core/src/query_planner/mod.rs b/apollo-router-core/src/query_planner/mod.rs index fd652b70f7..608ceeea94 100644 --- a/apollo-router-core/src/query_planner/mod.rs +++ b/apollo-router-core/src/query_planner/mod.rs @@ -594,6 +594,46 @@ mod tests { ); } + /// This test panics in the product subgraph. HOWEVER, this does not result in a panic in the + /// test, since the buffer() functionality in the tower stack "loses" the panic and we end up + /// with a closed service. + /// + /// See: https://github.com/tower-rs/tower/issues/455 + /// + /// 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] + 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(), + }; + + let mut mock_products_service = plugin::utils::test::MockSubgraphService::new(); + mock_products_service.expect_call().times(1).withf(|_| { + panic!("this panic should be propagated to the test harness"); + }); + + let result = query_plan + .execute( + &Context::new(), + &ServiceRegistry::new(HashMap::from([( + "product".into(), + ServiceBuilder::new() + .buffer(1) + .service(mock_products_service.build().boxed()), + )])), + http_compat::Request::mock(), + &Schema::from_str(test_schema!()).unwrap(), + ) + .await; + assert_eq!(result.errors.len(), 1); + let reason: String = serde_json_bytes::from_value( + result.errors[0].extensions.get("reason").unwrap().clone(), + ) + .unwrap(); + assert_eq!(reason, "service closed".to_string()); + } + #[tokio::test] async fn fetch_includes_operation_name() { let query_plan: QueryPlan = QueryPlan { @@ -612,7 +652,8 @@ mod tests { == Some("topProducts_product_0".into()); inner_succeeded.store(matches, Ordering::SeqCst); matches - }); + }) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); query_plan .execute( &Context::new(), @@ -647,7 +688,8 @@ mod tests { let matches = request.subgraph_request.method() == Method::POST; inner_succeeded.store(matches, Ordering::SeqCst); matches - }); + }) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); query_plan .execute( &Context::new(),