add a test which panics in a mock subgraph and handles the response

[garypen]

The panic is captured in the tower-http stack and translated into an
INTERNAL_SERVER_ERROR (500). By the time this reaches the query planner,
the error is interpreted as a "service closed", which is what the test
expects.

fixes: #878

[netlify]

✅ Deploy Preview for apollo-router-docs canceled.

Name	Link
🔨 Latest commit	95dced1
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/626905c6c4e16f0008784f5f

[github-actions]

@garypen your pull request is missing a changelog!

[o0Ignition0o]

LGTM! Have you been able to see which tests in our suite are currently passing for the wrong reasons ?

[bnjjj]

I added a comment here #878 (comment) I don't know if it's related

[BrynCooke]

Is it not the case that this does not use tower_http because we are just executing directly against the query plan?

[garypen]

There are some spurious panics and the concerning thing is that mockall panicking doesn't seem to cause the tests to fail.

The faulty tests I found are:

thread 'layers::apq::apq_tests::it_doesnt_update_the_cache_if_the_hash_is_not_valid' panicked at 'MockRouterService::call: Expectation(<anything>) called 0 time(s) which is fewer than expected 1', apollo-router-core/src/plugin/utils/test/service.rs:39:1
thread 'query_planner::tests::fetch_includes_operation_name' panicked at 'MockSubgraphService::call: Expectation(<function>) Returning default values requires the "nightly" feature', apollo-router-core/src/plugin/utils/test/service.rs:42:1
thread 'query_planner::tests::fetch_makes_post_requests' panicked at 'MockSubgraphService::call: Expectation(<function>) Returning default values requires the "nightly" feature', apollo-router-core/src/plugin/utils/test/service.rs:42:1

Fixing the last two tests is straightforward. They are both missing a returning clause which results in the mockall framework generating some code to produce a "Default". These tests are both fixable by the addition of a returning clause. The first test is failing because it expects to be invoked 1 and isn't invoked at all. I'm not sure if the expectation is broken or the test in that case.

This is a problem that needs some more thought, since we'll never know when a mockall panic may creep into the tests (without manually inspecting logs for panics.)...

[garypen]

Is it not the case that this does not use tower_http because we are just executing directly against the query plan?

I'm not sure, is the quick answer. "Something" is handling the panics and tower-http seemed the most likely explanation, but it really needs more digging to confirm this.

[o0Ignition0o]

        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())
            });

is the mock that doesnt seem to be called.

[o0Ignition0o]

Ok the first test was a mistake I've made.
When the hash is not valid, the first and the third call should be rejected, so the second mock should never trigger.
I got bitten by the silent errors when I wrote this unit test, and thus assumed things were working as expected.

diff --git a/apollo-router-core/src/layers/apq.rs b/apollo-router-core/src/layers/apq.rs
index fff4babe..665e3ef3 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();

will do

[garypen]

        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())
            });

is the mock that doesnt seem to be called.

I applied this change in: 283f51a