From af0ca223b7f6a381cb45e0c75ca856f3ba207777 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 2 Aug 2023 11:19:24 +0100 Subject: [PATCH 1/3] Replace panics with expect. For lock poisoning this is fine as we can't continue safely if a panic has occurred. --- .../persisted_queries/manifest_poller.rs | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/services/layers/persisted_queries/manifest_poller.rs b/apollo-router/src/services/layers/persisted_queries/manifest_poller.rs index 8f21979d4b..937912a76c 100644 --- a/apollo-router/src/services/layers/persisted_queries/manifest_poller.rs +++ b/apollo-router/src/services/layers/persisted_queries/manifest_poller.rs @@ -92,16 +92,18 @@ impl PersistedQueryManifestPoller { } pub(crate) fn get_operation_body(&self, persisted_query_id: &str) -> Option { - let persisted_query_manifest = self.persisted_query_manifest.read().unwrap_or_else(|e| { - panic!("could not acquire read lock on persisted query manifest: {e}") - }); + let persisted_query_manifest = self + .persisted_query_manifest + .read() + .expect("could not acquire read lock on persisted query manifest"); persisted_query_manifest.get(persisted_query_id).cloned() } pub(crate) fn is_operation_persisted(&self, query: &str) -> bool { - let persisted_query_bodies = self.persisted_query_bodies.read().unwrap_or_else(|e| { - panic!("could not acquire read lock on persisted query body se: {e}") - }); + let persisted_query_bodies = self + .persisted_query_bodies + .read() + .expect("could not acquire read lock on persisted query body set"); persisted_query_bodies.contains(query) } } @@ -169,15 +171,9 @@ async fn poll_uplink( // update the set of pq bodies from the values in the new collection *existing_bodies = new_bodies; }) - .unwrap_or_else(|e| { - panic!( - "could not acquire write lock on persisted query body set: {e}" - ) - }); + .expect("could not acquire write lock on persisted query body set"); }) - .unwrap_or_else(|e| { - panic!("could not acquire write lock on persisted query manifest: {e}") - }); + .expect("could not acquire write lock on persisted query manifest"); if !resolved_first_pq_manifest { send_startup_event( From ad1314f17eaee7db91fc9128e49619b95ac53636 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 2 Aug 2023 11:19:47 +0100 Subject: [PATCH 2/3] Simplify tests by removing panics and using expect. --- .../services/layers/persisted_queries/mod.rs | 130 ++++++++---------- 1 file changed, 55 insertions(+), 75 deletions(-) diff --git a/apollo-router/src/services/layers/persisted_queries/mod.rs b/apollo-router/src/services/layers/persisted_queries/mod.rs index 26907b0ae4..0901a7231d 100644 --- a/apollo-router/src/services/layers/persisted_queries/mod.rs +++ b/apollo-router/src/services/layers/persisted_queries/mod.rs @@ -446,11 +446,10 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_none()); let result = pq_layer.supergraph_request(incoming_request); - if let Ok(request) = result { - assert_eq!(request.supergraph_request.body().query, Some(body)); - } else { - panic!("pq layer returned response instead of putting the query on the request"); - } + let request = result + .ok() + .expect("pq layer returned response instead of putting the query on the request"); + assert_eq!(request.supergraph_request.body().query, Some(body)); } #[tokio::test(flavor = "multi_thread")] @@ -481,11 +480,10 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_none()); let result = pq_layer.supergraph_request(incoming_request); - if let Ok(request) = result { - assert!(request.supergraph_request.body().query.is_none()); - } else { - panic!("pq layer returned response instead of continuing to APQ layer"); - } + let request = result + .ok() + .expect("pq layer returned response instead of continuing to APQ layer"); + assert!(request.supergraph_request.body().query.is_none()); } #[tokio::test(flavor = "multi_thread")] @@ -516,19 +514,16 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_none()); - let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!( - response.errors, - vec![graphql_err_operation_not_found(invalid_id)] - ); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = pq_layer + .supergraph_request(incoming_request) + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!( + response.errors, + vec![graphql_err_operation_not_found(invalid_id)] + ); } #[tokio::test(flavor = "multi_thread")] @@ -620,18 +615,15 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_some()); let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!( - response.errors, - vec![graphql_err_operation_not_in_safelist()] - ); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = result + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!( + response.errors, + vec![graphql_err_operation_not_in_safelist()] + ); } #[tokio::test(flavor = "multi_thread")] @@ -669,18 +661,15 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_none()); let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!( - response.errors, - vec![graphql_err_operation_not_found(invalid_id)] - ); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = result + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!( + response.errors, + vec![graphql_err_operation_not_found(invalid_id)] + ); } #[tokio::test(flavor = "multi_thread")] @@ -786,15 +775,12 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_some()); let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!(response.errors, vec![graphql_err_pq_id_required()]); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = result + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!(response.errors, vec![graphql_err_pq_id_required()]); } #[tokio::test(flavor = "multi_thread")] @@ -887,15 +873,12 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_some()); let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = result + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); } #[tokio::test(flavor = "multi_thread")] @@ -921,16 +904,13 @@ mod tests { assert!(incoming_request.supergraph_request.body().query.is_some()); - let result = pq_layer.supergraph_request(incoming_request); - if let Err(mut response) = result { - if let Some(response) = response.next_response().await { - assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); - } else { - panic!("could not get response from pq layer"); - } - } else { - panic!("pq layer returned request instead of returning an error response"); - } + let response = pq_layer + .supergraph_request(incoming_request) + .expect_err("pq layer returned request instead of returning an error response") + .next_response() + .await + .expect("could not get response from pq layer"); + assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); } #[tokio::test(flavor = "multi_thread")] From dcf3db6187f7622a46134a59955b2033609b5503 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 2 Aug 2023 11:26:39 +0100 Subject: [PATCH 3/3] Changelog --- .changesets/maint_contract_fiancee_ark_grey.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changesets/maint_contract_fiancee_ark_grey.md diff --git a/.changesets/maint_contract_fiancee_ark_grey.md b/.changesets/maint_contract_fiancee_ark_grey.md new file mode 100644 index 0000000000..cd812be36b --- /dev/null +++ b/.changesets/maint_contract_fiancee_ark_grey.md @@ -0,0 +1,5 @@ +### Remove some panic! calls from the pq code. ([PR #3527](https://github.com/apollographql/router/pull/3527)) + +Replace a few `panic!` calls with `expect()` in the persisted query code for code clarity. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/3527