fix: QueryPlanner: run plugins regardless whether cache hit or not

fixes #1541 This commit lets the QueryPlanner cache wrap only the `bridge_query_planner`, so that plugins get a chance to run whether the cache will hit or not.
apollographql · Aug 18, 2022 · 281f928 · 281f928
1 parent 77621dd
commit 281f928
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 17 deletions.
diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md
@@ -412,6 +412,12 @@ By [@SimonSapin](https://github.com/SimonSapin)
 
 ## 🐛 Fixes
 
+### QueryPlanner: run plugins regardless whether cache hit or not ([#1541](https://github.com/apollographql/router/issues/1541))
+
+QueryPlanner plugins will now fully run regardless of whether the QueryPlanner cache hits. This will allow you to get consistent plugin behavior on first and subsequent query execution.
+
+By [@o0Ignition0o](https://github.com/o0Ignition0o)
+
 ### Accept SIGTERM as shutdown signal ([PR #1497](https://github.com/apollographql/router/pull/1497))
 
 This will make containers stop faster as they will not have to wait until a SIGKILL to stop the router.

diff --git a/apollo-router/src/plugins/expose_query_plan.rs b/apollo-router/src/plugins/expose_query_plan.rs
@@ -236,6 +236,13 @@ mod tests {
     async fn it_expose_query_plan() {
         let plugin = get_plugin(&serde_json::json!(true)).await;
         let router = build_mock_router(plugin).await;
+        execute_router_test(
+            VALID_QUERY,
+            &*EXPECTED_RESPONSE_WITH_QUERY_PLAN,
+            router.clone(),
+        )
+        .await;
+        // if this test fails, it means the query planner cache is trying to cast too wide a net again.
         execute_router_test(VALID_QUERY, &*EXPECTED_RESPONSE_WITH_QUERY_PLAN, router).await;
     }
 

diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs
@@ -357,21 +357,19 @@ impl PluggableRouterServiceBuilder {
             BridgeQueryPlanner::new(self.schema.clone(), introspection, configuration)
                 .await
                 .map_err(ServiceBuildError::QueryPlannerError)?;
-        let query_planner_service = ServiceBuilder::new().service(
-            CachingQueryPlanner::new(
-                Buffer::new(
-                    self.plugins
-                        .iter_mut()
-                        .rev()
-                        .fold(bridge_query_planner.boxed(), |acc, (_, e)| {
-                            e.query_planner_service(acc)
-                        }),
-                    DEFAULT_BUFFER_SIZE,
-                ),
-                plan_cache_limit,
-            )
-            .await,
-        );
+        let query_planner_service = ServiceBuilder::new().service(Buffer::new(
+            self.plugins
+                .iter_mut()
+                .rev()
+                .fold(
+                    CachingQueryPlanner::new(bridge_query_planner, plan_cache_limit)
+                        .await
+                        .boxed(),
+                    |acc, (_, e)| e.query_planner_service(acc),
+                )
+                .boxed(),
+            DEFAULT_BUFFER_SIZE,
+        ));
 
         let plugins = Arc::new(self.plugins);
 
@@ -395,8 +393,7 @@ impl PluggableRouterServiceBuilder {
 /// A collection of services and data which may be used to create a "router".
 #[derive(Clone)]
 pub(crate) struct RouterCreator {
-    query_planner_service:
-        CachingQueryPlanner<Buffer<query_planner::BoxService, QueryPlannerRequest>>,
+    query_planner_service: Buffer<query_planner::BoxService, QueryPlannerRequest>,
     subgraph_creator: Arc<SubgraphCreator>,
     schema: Arc<Schema>,
     plugins: Arc<Plugins>,