From b4ba8fc16306d24a3c715c69c09a778341024436 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Thu, 25 May 2023 17:10:59 +0200 Subject: [PATCH 01/15] Reintroduce query planner plugins We may need to observe and edit the query between the query plan caching and the query planner, so this brings back the query planner plugins we had initially --- .../query_planner/caching_query_planner.rs | 55 +++++++++++++++---- apollo-router/src/services/query_planner.rs | 43 +++++++++++++++ .../src/services/supergraph_service.rs | 5 +- 3 files changed, 91 insertions(+), 12 deletions(-) diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 7b63c933c8..fb1cb1b9b0 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -6,11 +6,15 @@ use std::sync::Arc; use std::task; use futures::future::BoxFuture; +use indexmap::IndexMap; +use query_planner::QueryPlannerPlugin; use router_bridge::planner::Planner; use router_bridge::planner::UsageReporting; use sha2::Digest; use sha2::Sha256; +use tower::ServiceBuilder; use tower::ServiceExt; +use tower_service::Service; use tracing::Instrument; use crate::cache::DeduplicatingCache; @@ -18,11 +22,15 @@ use crate::error::CacheResolverError; use crate::error::QueryPlannerError; use crate::query_planner::BridgeQueryPlanner; use crate::query_planner::QueryPlanResult; +use crate::services::query_planner; use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::QueryPlannerResponse; use crate::Context; +/// An [`IndexMap`] of available plugins. +pub(crate) type Plugins = IndexMap>; + /// A query planner wrapper that caches results. /// /// The query planner performs LRU caching. @@ -33,21 +41,24 @@ pub(crate) struct CachingQueryPlanner { >, delegate: T, schema_id: Option, + plugins: Arc, } impl CachingQueryPlanner where T: tower::Service< - QueryPlannerRequest, - Response = QueryPlannerResponse, - Error = QueryPlannerError, - >, + QueryPlannerRequest, + Response = QueryPlannerResponse, + Error = QueryPlannerError, + > + Send, + >::Future: Send, { /// Creates a new query planner that caches the results of another [`QueryPlanner`]. pub(crate) async fn new( delegate: T, schema_id: Option, config: &crate::configuration::QueryPlanning, + plugins: Plugins, ) -> CachingQueryPlanner { let cache = Arc::new( DeduplicatingCache::from_configuration(&config.experimental_cache, "query planner") @@ -57,6 +68,7 @@ where cache, delegate, schema_id, + plugins: Arc::new(plugins), } } @@ -71,6 +83,15 @@ where pub(crate) async fn warm_up(&mut self, cache_keys: Vec<(String, Option)>) { let schema_id = self.schema_id.clone(); + let mut service = ServiceBuilder::new().service( + self.plugins + .iter() + .rev() + .fold(self.delegate.clone().boxed(), |acc, (_, e)| { + e.query_planner_service(acc) + }), + ); + let mut count = 0usize; for (query, operation) in cache_keys { let caching_key = CachingQueryKey { @@ -88,7 +109,7 @@ where context: context.clone(), }; - let res = match self.delegate.ready().await { + let res = match service.ready().await { Ok(service) => service.call(request).await, Err(_) => break, }; @@ -119,7 +140,8 @@ impl CachingQueryPlanner { } } -impl tower::Service for CachingQueryPlanner +impl tower::Service + for CachingQueryPlanner where T: tower::Service< QueryPlannerRequest, @@ -136,7 +158,7 @@ where task::Poll::Ready(Ok(())) } - fn call(&mut self, request: QueryPlannerRequest) -> Self::Future { + fn call(&mut self, request: query_planner::CachingRequest) -> Self::Future { let mut qp = self.clone(); let schema_id = self.schema_id.clone(); Box::pin(async move { @@ -149,6 +171,17 @@ where let context = request.context.clone(); let entry = qp.cache.get(&caching_key).await; if entry.is_first() { + let query_planner::CachingRequest { + query, + operation_name, + context, + } = request; + let request = QueryPlannerRequest::builder() + .query(query) + .and_operation_name(operation_name) + .context(context) + .build(); + // some clients might timeout and cancel the request before query planning is finished, // so we execute it in a task that can continue even after the request was canceled and // the join handle was dropped. That way, the next similar query will use the cache instead @@ -339,12 +372,13 @@ mod tests { delegate, None, &crate::configuration::QueryPlanning::default(), + IndexMap::new(), ) .await; for _ in 0..5 { assert!(planner - .call(QueryPlannerRequest::new( + .call(query_planner::CachingRequest::new( "query1".into(), Some("".into()), Context::new() @@ -353,7 +387,7 @@ mod tests { .is_err()); } assert!(planner - .call(QueryPlannerRequest::new( + .call(query_planner::CachingRequest::new( "query2".into(), Some("".into()), Context::new() @@ -399,12 +433,13 @@ mod tests { delegate, None, &crate::configuration::QueryPlanning::default(), + IndexMap::new(), ) .await; for _ in 0..5 { assert!(planner - .call(QueryPlannerRequest::new( + .call(query_planner::CachingRequest::new( "".into(), Some("".into()), Context::new() diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index 5f2452347a..c86ed4cf9f 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -4,10 +4,12 @@ use std::sync::Arc; +use async_trait::async_trait; use serde::Deserialize; use serde::Serialize; use static_assertions::assert_impl_all; +use crate::error::QueryPlannerError; use crate::graphql; use crate::query_planner::QueryPlan; use crate::Context; @@ -36,6 +38,33 @@ impl Request { } } +/// [`Context`] for the request. +#[derive(Clone, Debug)] +pub(crate) struct CachingRequest { + pub(crate) query: String, + pub(crate) operation_name: Option, + pub(crate) context: Context, +} + +#[buildstructor::buildstructor] +impl CachingRequest { + /// This is the constructor (or builder) to use when constructing a real QueryPlannerRequest. + /// + /// Required parameters are required in non-testing code to create a QueryPlannerRequest. + #[builder] + pub(crate) fn new( + query: String, + operation_name: Option, + context: Context, + ) -> CachingRequest { + Self { + query, + operation_name, + context, + } + } +} + assert_impl_all!(Response: Send); /// [`Context`] and [`QueryPlan`] for the response. pub(crate) struct Response { @@ -71,3 +100,17 @@ impl Response { } } } + +pub(crate) type BoxService = tower::util::BoxService; +pub(crate) type BoxCloneService = + tower::util::BoxCloneService; +pub(crate) type ServiceResult = Result; +pub(crate) type Body = hyper::Body; +pub(crate) type Error = hyper::Error; + +#[async_trait] +pub(crate) trait QueryPlannerPlugin: Send + Sync + 'static { + /// This service runs right after the query planner cache, which means that it will be called once per unique + /// query, unless the cache entry was evicted + fn query_planner_service(&self, service: BoxService) -> BoxService; +} diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index d33f69afeb..1cfb5301a9 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -38,10 +38,10 @@ use crate::plugins::traffic_shaping::APOLLO_TRAFFIC_SHAPING; use crate::query_planner::BridgeQueryPlanner; use crate::query_planner::CachingQueryPlanner; use crate::query_planner::QueryPlanResult; +use crate::services::query_planner; use crate::services::supergraph; use crate::services::ExecutionRequest; use crate::services::ExecutionResponse; -use crate::services::QueryPlannerRequest; use crate::services::QueryPlannerResponse; use crate::services::SupergraphRequest; use crate::services::SupergraphResponse; @@ -247,7 +247,7 @@ async fn plan_query( ) -> Result { planning .call( - QueryPlannerRequest::builder() + query_planner::CachingRequest::builder() .query( body.query .clone() @@ -325,6 +325,7 @@ impl PluggableSupergraphServiceBuilder { self.planner, schema.schema_id.clone(), &configuration.supergraph.query_planning, + IndexMap::new(), ) .await; From 7db1c4e9ed5f7cb913393c59afc6e0140ffcca77 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Thu, 25 May 2023 18:07:44 +0200 Subject: [PATCH 02/15] create a compiler and pass it around --- .../query_planner/caching_query_planner.rs | 80 +++++++++++++------ apollo-router/src/services/query_planner.rs | 19 ++++- .../src/services/supergraph_service.rs | 4 +- apollo-router/src/spec/query.rs | 19 ++++- 4 files changed, 89 insertions(+), 33 deletions(-) diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index fb1cb1b9b0..6c73b3255c 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -26,6 +26,9 @@ use crate::services::query_planner; use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::QueryPlannerResponse; +use crate::spec::Query; +use crate::spec::Schema; +use crate::Configuration; use crate::Context; /// An [`IndexMap`] of available plugins. @@ -40,8 +43,9 @@ pub(crate) struct CachingQueryPlanner { DeduplicatingCache>>, >, delegate: T, - schema_id: Option, + schema: Arc, plugins: Arc, + configuration: Arc, } impl CachingQueryPlanner @@ -56,18 +60,22 @@ where /// Creates a new query planner that caches the results of another [`QueryPlanner`]. pub(crate) async fn new( delegate: T, - schema_id: Option, - config: &crate::configuration::QueryPlanning, + schema: Arc, + configuration: Arc, plugins: Plugins, ) -> CachingQueryPlanner { let cache = Arc::new( - DeduplicatingCache::from_configuration(&config.experimental_cache, "query planner") - .await, + DeduplicatingCache::from_configuration( + &configuration.supergraph.query_planning.experimental_cache, + "query planner", + ) + .await, ); Self { cache, delegate, - schema_id, + schema, + configuration, plugins: Arc::new(plugins), } } @@ -81,7 +89,7 @@ where } pub(crate) async fn warm_up(&mut self, cache_keys: Vec<(String, Option)>) { - let schema_id = self.schema_id.clone(); + let schema_id = self.schema.schema_id.clone(); let mut service = ServiceBuilder::new().service( self.plugins @@ -103,10 +111,15 @@ where let entry = self.cache.get(&caching_key).await; if entry.is_first() { + let (compiler, file_id) = + Query::make_compiler(&query, &self.schema, &self.configuration); + let request = QueryPlannerRequest { query, operation_name: operation, context: context.clone(), + compiler, + file_id, }; let res = match service.ready().await { @@ -160,7 +173,9 @@ where fn call(&mut self, request: query_planner::CachingRequest) -> Self::Future { let mut qp = self.clone(); - let schema_id = self.schema_id.clone(); + let schema_id = self.schema.schema_id.clone(); + let schema = self.schema.clone(); + let configuration = self.configuration.clone(); Box::pin(async move { let caching_key = CachingQueryKey { schema_id, @@ -176,10 +191,15 @@ where operation_name, context, } = request; + + let (compiler, file_id) = Query::make_compiler(&query, &schema, &configuration); + let request = QueryPlannerRequest::builder() .query(query) .and_operation_name(operation_name) .context(context) + .compiler(compiler) + .file_id(file_id) .build(); // some clients might timeout and cancel the request before query planning is finished, @@ -368,27 +388,33 @@ mod tests { planner }); - let mut planner = CachingQueryPlanner::new( - delegate, - None, - &crate::configuration::QueryPlanning::default(), - IndexMap::new(), - ) - .await; + let configuration = Arc::new(crate::Configuration::default()); + let schema = Arc::new( + Schema::parse( + include_str!("testdata/schema.graphql"), + &configuration, + None, + ) + .unwrap(), + ); + + let mut planner = + CachingQueryPlanner::new(delegate, schema, configuration, IndexMap::new()).await; for _ in 0..5 { assert!(planner .call(query_planner::CachingRequest::new( - "query1".into(), + "{ me { id } }".into(), Some("".into()), Context::new() )) .await .is_err()); } + assert!(planner .call(query_planner::CachingRequest::new( - "query2".into(), + "{ me { id name } }".into(), Some("".into()), Context::new() )) @@ -429,18 +455,22 @@ mod tests { planner }); - let mut planner = CachingQueryPlanner::new( - delegate, - None, - &crate::configuration::QueryPlanning::default(), - IndexMap::new(), - ) - .await; + let configuration = Arc::new(crate::Configuration::default()); + let schema = Arc::new( + Schema::parse( + include_str!("testdata/schema.graphql"), + &configuration, + None, + ) + .unwrap(), + ); + let mut planner = + CachingQueryPlanner::new(delegate, schema, configuration, IndexMap::new()).await; for _ in 0..5 { assert!(planner .call(query_planner::CachingRequest::new( - "".into(), + "{ me { id } }".into(), Some("".into()), Context::new() )) diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index c86ed4cf9f..6bf6d215f6 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -4,7 +4,10 @@ use std::sync::Arc; +use apollo_compiler::ApolloCompiler; +use apollo_compiler::FileId; use async_trait::async_trait; +use derivative::Derivative; use serde::Deserialize; use serde::Serialize; use static_assertions::assert_impl_all; @@ -16,11 +19,15 @@ use crate::Context; assert_impl_all!(Request: Send); /// [`Context`] for the request. -#[derive(Clone, Debug)] +#[derive(Derivative)] +#[derivative(Debug)] pub(crate) struct Request { pub(crate) query: String, pub(crate) operation_name: Option, pub(crate) context: Context, + #[derivative(Debug = "ignore")] + pub(crate) compiler: ApolloCompiler, + pub(crate) file_id: FileId, } #[buildstructor::buildstructor] @@ -29,11 +36,19 @@ impl Request { /// /// Required parameters are required in non-testing code to create a QueryPlannerRequest. #[builder] - pub(crate) fn new(query: String, operation_name: Option, context: Context) -> Request { + pub(crate) fn new( + query: String, + operation_name: Option, + context: Context, + compiler: ApolloCompiler, + file_id: FileId, + ) -> Request { Self { query, operation_name, context, + compiler, + file_id, } } } diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 1cfb5301a9..60b9b85d0d 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -323,8 +323,8 @@ impl PluggableSupergraphServiceBuilder { let schema = self.planner.schema(); let query_planner_service = CachingQueryPlanner::new( self.planner, - schema.schema_id.clone(), - &configuration.supergraph.query_planning, + schema.clone(), + configuration.clone(), IndexMap::new(), ) .await; diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 0670f77f21..2ac21ec2ec 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -9,6 +9,7 @@ use std::collections::HashSet; use apollo_compiler::hir; use apollo_compiler::ApolloCompiler; use apollo_compiler::AstDatabase; +use apollo_compiler::FileId; use apollo_compiler::HirDatabase; use derivative::Derivative; use serde::de::Visitor; @@ -261,16 +262,26 @@ impl Query { vec![] } - pub(crate) fn parse( - query: impl Into, + pub(crate) fn make_compiler( + query: &str, schema: &Schema, configuration: &Configuration, - ) -> Result { - let query = query.into(); + ) -> (ApolloCompiler, FileId) { let mut compiler = ApolloCompiler::new() .recursion_limit(configuration.preview_operation_limits.parser_max_recursion) .token_limit(configuration.preview_operation_limits.parser_max_tokens); + compiler.set_type_system_hir(schema.type_system.clone()); let id = compiler.add_executable(&query, "query"); + (compiler, id) + } + + pub(crate) fn parse( + query: impl Into, + schema: &Schema, + configuration: &Configuration, + ) -> Result { + let query = query.into(); + let (compiler, id) = Self::make_compiler(&query, schema, configuration); let ast = compiler.db.ast(id); // Trace log recursion limit data From eaf5ce0da4914b8483e9155b09a6b77c52d66729 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Thu, 25 May 2023 18:37:31 +0200 Subject: [PATCH 03/15] plan the filtered query --- .../src/query_planner/bridge_query_planner.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index e7d36281ac..9070cca392 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -5,6 +5,7 @@ use std::fmt::Debug; use std::sync::Arc; use std::time::Instant; +use apollo_compiler::InputDatabase; use futures::future::BoxFuture; use router_bridge::planner::IncrementalDeliverySupport; use router_bridge::planner::PlanSuccess; @@ -224,11 +225,20 @@ impl Service for BridgeQueryPlanner { } fn call(&mut self, req: QueryPlannerRequest) -> Self::Future { + let QueryPlannerRequest { + query, + operation_name, + context, + compiler, + file_id, + } = req; + let filtered_query = compiler.db.source_code(file_id); + let this = self.clone(); let fut = async move { let start = Instant::now(); let res = this - .get((req.query.clone(), req.operation_name.to_owned())) + .get((filtered_query.to_string(), operation_name.to_owned())) .await; let duration = start.elapsed().as_secs_f64(); tracing::info!(histogram.apollo_router_query_planning_time = duration,); @@ -236,18 +246,18 @@ impl Service for BridgeQueryPlanner { match res { Ok(query_planner_content) => Ok(QueryPlannerResponse::builder() .content(query_planner_content) - .context(req.context) + .context(context) .build()), Err(e) => { match &e { QueryPlannerError::PlanningErrors(pe) => { - req.context + context .private_entries .lock() .insert(pe.usage_reporting.clone()); } QueryPlannerError::SpecError(e) => { - req.context.private_entries.lock().insert(UsageReporting { + context.private_entries.lock().insert(UsageReporting { stats_report_key: e.get_error_key().to_string(), referenced_fields_by_type: HashMap::new(), }); From e6f4f4d073fc871bf4800cbea04d7de1acd4852a Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 30 May 2023 14:47:15 +0200 Subject: [PATCH 04/15] lint --- apollo-router/src/services/query_planner.rs | 4 ++++ apollo-router/src/spec/query.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index 6bf6d215f6..c4c425c9ed 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -117,10 +117,14 @@ impl Response { } pub(crate) type BoxService = tower::util::BoxService; +#[allow(dead_code)] pub(crate) type BoxCloneService = tower::util::BoxCloneService; +#[allow(dead_code)] pub(crate) type ServiceResult = Result; +#[allow(dead_code)] pub(crate) type Body = hyper::Body; +#[allow(dead_code)] pub(crate) type Error = hyper::Error; #[async_trait] diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 2ac21ec2ec..6c2bc08c45 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -271,7 +271,7 @@ impl Query { .recursion_limit(configuration.preview_operation_limits.parser_max_recursion) .token_limit(configuration.preview_operation_limits.parser_max_tokens); compiler.set_type_system_hir(schema.type_system.clone()); - let id = compiler.add_executable(&query, "query"); + let id = compiler.add_executable(query, "query"); (compiler, id) } From a7a00d8bd1780807a9b31affdacd1a83e5d71daa Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 30 May 2023 15:44:32 +0200 Subject: [PATCH 05/15] generate selections for the filtered query Query planner plugins can modify the query before sending it through query planning. They might require different sets of fields on the same entity, and the query plan might add its own list of fields (keys for federated queries). As an example, We could have a User entity with its email field used as key, and want to remove private information from the query. So we could have the query: { topProducts { name reviews { author { email username } } } } So here we would filter the query as follows: { topProducts { name reviews { author { username } } } } But since the email is used as key, it would appear in the JSON object that accumulates response data. To avoid sending unrequested data to the client, we have the response formatting phase, that uses selections extracted from the query, to pick only what is needed from the JSON object, and send it to the client. Here we need to apply the selections of the filtered query, to make sure the email is not returned to the client (the original query would let it go through). But we also need to apply the selections from the original query, to have the null propagation algorithm apply and return a response in the shape that matches the client query. There is probably a better way to do it than applying selections twice, but here we are sure the behaviour will be correct, and so far the formatting phase is fast enough, we can spend a bit more time there --- .../src/query_planner/bridge_query_planner.rs | 45 ++++++++++++++----- .../src/services/execution_service.rs | 16 +++++-- apollo-router/src/spec/query.rs | 4 ++ 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 9070cca392..5fa4fae3cd 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -238,7 +238,7 @@ impl Service for BridgeQueryPlanner { let fut = async move { let start = Instant::now(); let res = this - .get((filtered_query.to_string(), operation_name.to_owned())) + .get(query, filtered_query.to_string(), operation_name.to_owned()) .await; let duration = start.elapsed().as_secs_f64(); tracing::info!(histogram.apollo_router_query_planning_time = duration,); @@ -275,8 +275,15 @@ impl Service for BridgeQueryPlanner { } impl BridgeQueryPlanner { - async fn get(&self, key: QueryKey) -> Result { - let selections = self.parse_selections(key.clone()).await?; + async fn get( + &self, + query: String, + filtered_query: String, + operation_name: Option, + ) -> Result { + let mut selections = self + .parse_selections((query.clone(), operation_name.clone())) + .await?; if selections.contains_introspection() { // If we have only one operation containing only the root field `__typename` @@ -297,11 +304,18 @@ impl BridgeQueryPlanner { response: Box::new(graphql::Response::builder().data(data).build()), }); } else { - return self.introspection(key.0).await; + return self.introspection(query).await; } } - self.plan(key.0, key.1, selections).await + if filtered_query != query { + selections.filtered_query = Some(Arc::new( + self.parse_selections((filtered_query.clone(), operation_name.clone())) + .await?, + )); + } + + self.plan(filtered_query, operation_name, selections).await } } @@ -336,7 +350,11 @@ mod tests { .await .unwrap(); let result = planner - .get((include_str!("testdata/query.graphql").into(), None)) + .get( + include_str!("testdata/query.graphql").into(), + include_str!("testdata/query.graphql").into(), + None, + ) .await .unwrap(); if let QueryPlannerContent::Plan { plan, .. } = result { @@ -355,10 +373,11 @@ mod tests { .await .unwrap(); let err = planner - .get(( + .get( + "fragment UnusedTestFragment on User { id } query { me { id } }".to_string(), "fragment UnusedTestFragment on User { id } query { me { id } }".to_string(), None, - )) + ) .await .unwrap_err(); @@ -417,7 +436,7 @@ mod tests { let planner = BridgeQueryPlanner::new(EXAMPLE_SCHEMA.to_string(), Default::default()) .await .unwrap(); - let result = planner.get(("".into(), None)).await; + let result = planner.get("".into(), "".into(), None).await; assert_eq!( "couldn't plan query: query validation errors: Syntax Error: Unexpected .", @@ -431,7 +450,7 @@ mod tests { .await .unwrap(); let result = planner - .get(("{ x: __typename }".into(), None)) + .get("{ x: __typename }".into(), "{ x: __typename }".into(), None) .await .unwrap(); if let QueryPlannerContent::Introspection { response } = result { @@ -450,7 +469,11 @@ mod tests { .await .unwrap(); let result = planner - .get(("{ x: __typename __typename }".into(), None)) + .get( + "{ x: __typename __typename }".into(), + "{ x: __typename __typename }".into(), + None, + ) .await .unwrap(); if let QueryPlannerContent::Introspection { response } = result { diff --git a/apollo-router/src/services/execution_service.rs b/apollo-router/src/services/execution_service.rs index a48a55c668..15339e1b92 100644 --- a/apollo-router/src/services/execution_service.rs +++ b/apollo-router/src/services/execution_service.rs @@ -111,14 +111,24 @@ impl Service for ExecutionService { let has_next = response.has_next.unwrap_or(true); tracing::debug_span!("format_response").in_scope(|| { - let paths = query.format_response( + let mut paths = Vec::new(); + if let Some(filtered_query) = query.filtered_query.as_ref() { + paths = filtered_query.format_response( + &mut response, + operation_name.as_deref(), + is_deferred, + variables.clone(), + schema.api_schema(), + ); + } + + paths.extend(query.format_response( &mut response, operation_name.as_deref(), is_deferred, variables.clone(), schema.api_schema(), - ); - + ).into_iter()); nullified_paths.extend(paths.into_iter()); }); diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 6c2bc08c45..1a901d8961 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -5,6 +5,7 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::sync::Arc; use apollo_compiler::hir; use apollo_compiler::ApolloCompiler; @@ -54,6 +55,8 @@ pub(crate) struct Query { pub(crate) operations: Vec, #[derivative(PartialEq = "ignore", Hash = "ignore")] pub(crate) subselections: HashMap, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + pub(crate) filtered_query: Option>, } #[derive(Debug, Derivative, Default)] @@ -314,6 +317,7 @@ impl Query { fragments, operations, subselections: HashMap::new(), + filtered_query: None, }) } From 5914b8ddcd4569d0eb097413241d5c72132eb69b Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 30 May 2023 17:19:44 +0200 Subject: [PATCH 06/15] changeset --- .changesets/feat_geal_query_planner_plugins.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changesets/feat_geal_query_planner_plugins.md diff --git a/.changesets/feat_geal_query_planner_plugins.md b/.changesets/feat_geal_query_planner_plugins.md new file mode 100644 index 0000000000..3f6ad7ad65 --- /dev/null +++ b/.changesets/feat_geal_query_planner_plugins.md @@ -0,0 +1,6 @@ +### Query planner plugins ([Issue #3150](https://github.com/apollographql/router/issues/3150)) + +We may need to observe and edit the query between the query plan caching and the query planner, so this brings back the query planner plugins we had initially, with a different architecture. +The plugins need an ApolloCompiler instance to perform useful work on the query, so the caching layer, in case of cache miss, will generate a compiler instance and transmit it as part of the request going through query planner plugins. At the end of the chain, the query planner extracts the modified query from the compiler, uses it to generate a query plan, and generates the selections of both the original and filtered query for response formatting. This is done to ensure that the response does not leak data removed in the filtered query, but still keeps a shape expected by the original query, using the null propagation. + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/3177 \ No newline at end of file From 093e5c0ca94da11fb48a38becfe5a6b1b3736ff8 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 30 May 2023 18:17:15 +0200 Subject: [PATCH 07/15] fix salsa's log level in the test harness the compiler is not created inside a blocking spawned task so now it appears again in the spans. Salsa's events are filtered in the router, but were not in the test harness, which failed some tests --- apollo-router/src/test_harness.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/test_harness.rs b/apollo-router/src/test_harness.rs index b19b5e5bd3..16b270d9d5 100644 --- a/apollo-router/src/test_harness.rs +++ b/apollo-router/src/test_harness.rs @@ -89,14 +89,18 @@ impl<'a> TestHarness<'a> { /// Specifies the logging level. Note that this function may not be called more than once. /// log_level is in RUST_LOG format. pub fn log_level(self, log_level: &'a str) -> Self { - init_telemetry(log_level).expect("failed to setup logging"); + // manually filter salsa logs because some of them run at the INFO level https://github.com/salsa-rs/salsa/issues/425 + let log_level = format!("{log_level},salsa=error"); + init_telemetry(&log_level).expect("failed to setup logging"); self } /// Specifies the logging level. Note that this function will silently fail if called more than once. /// log_level is in RUST_LOG format. pub fn try_log_level(self, log_level: &'a str) -> Self { - let _ = init_telemetry(log_level); + // manually filter salsa logs because some of them run at the INFO level https://github.com/salsa-rs/salsa/issues/425 + let log_level = format!("{log_level},salsa=error"); + let _ = init_telemetry(&log_level); self } From 69819d1fbc1016d31bdf2e1e73ec881a3a29de2f Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Wed, 31 May 2023 10:11:30 +0200 Subject: [PATCH 08/15] filter salsa in tracing tests --- apollo-router/tests/tracing_tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apollo-router/tests/tracing_tests.rs b/apollo-router/tests/tracing_tests.rs index 37ad6e794d..24ac02425e 100644 --- a/apollo-router/tests/tracing_tests.rs +++ b/apollo-router/tests/tracing_tests.rs @@ -39,6 +39,7 @@ async fn make_request(request: supergraph::Request) { #[test_span(tokio::test)] #[target(apollo_router=tracing::Level::DEBUG)] +#[target(salsa=tracing::Level::ERROR)] async fn traced_basic_request() { make_request( supergraph::Request::fake_builder() @@ -52,6 +53,7 @@ async fn traced_basic_request() { #[test_span(tokio::test)] #[target(apollo_router=tracing::Level::DEBUG)] +#[target(salsa=tracing::Level::ERROR)] async fn traced_basic_composition() { make_request( supergraph::Request::fake_builder() From a54583e68a0912afd479b12abc6194918d99412b Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Wed, 31 May 2023 11:01:46 +0200 Subject: [PATCH 09/15] fix salsa filtering again --- .../snapshots/tracing_tests__traced_basic_composition.snap | 2 +- .../snapshots/tracing_tests__traced_basic_request.snap | 2 +- apollo-router/tests/snapshots/tracing_tests__variables.snap | 2 +- apollo-router/tests/tracing_tests.rs | 6 ++++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/apollo-router/tests/snapshots/tracing_tests__traced_basic_composition.snap b/apollo-router/tests/snapshots/tracing_tests__traced_basic_composition.snap index 51da4c49cc..4d2ab57135 100644 --- a/apollo-router/tests/snapshots/tracing_tests__traced_basic_composition.snap +++ b/apollo-router/tests/snapshots/tracing_tests__traced_basic_composition.snap @@ -9,7 +9,7 @@ expression: get_spans() "metadata": { "name": "root", "target": "tracing_tests", - "level": "INFO", + "level": "ERROR", "module_path": "tracing_tests", "fields": { "names": [] diff --git a/apollo-router/tests/snapshots/tracing_tests__traced_basic_request.snap b/apollo-router/tests/snapshots/tracing_tests__traced_basic_request.snap index cd7792d6bc..e0730af528 100644 --- a/apollo-router/tests/snapshots/tracing_tests__traced_basic_request.snap +++ b/apollo-router/tests/snapshots/tracing_tests__traced_basic_request.snap @@ -9,7 +9,7 @@ expression: get_spans() "metadata": { "name": "root", "target": "tracing_tests", - "level": "INFO", + "level": "ERROR", "module_path": "tracing_tests", "fields": { "names": [] diff --git a/apollo-router/tests/snapshots/tracing_tests__variables.snap b/apollo-router/tests/snapshots/tracing_tests__variables.snap index ded86a220e..036fb85e27 100644 --- a/apollo-router/tests/snapshots/tracing_tests__variables.snap +++ b/apollo-router/tests/snapshots/tracing_tests__variables.snap @@ -9,7 +9,7 @@ expression: get_spans() "metadata": { "name": "root", "target": "tracing_tests", - "level": "INFO", + "level": "ERROR", "module_path": "tracing_tests", "fields": { "names": [] diff --git a/apollo-router/tests/tracing_tests.rs b/apollo-router/tests/tracing_tests.rs index 24ac02425e..a0e755e2e2 100644 --- a/apollo-router/tests/tracing_tests.rs +++ b/apollo-router/tests/tracing_tests.rs @@ -38,8 +38,8 @@ async fn make_request(request: supergraph::Request) { } #[test_span(tokio::test)] +#[level(tracing::Level::ERROR)] #[target(apollo_router=tracing::Level::DEBUG)] -#[target(salsa=tracing::Level::ERROR)] async fn traced_basic_request() { make_request( supergraph::Request::fake_builder() @@ -52,8 +52,8 @@ async fn traced_basic_request() { } #[test_span(tokio::test)] +#[level(tracing::Level::ERROR)] #[target(apollo_router=tracing::Level::DEBUG)] -#[target(salsa=tracing::Level::ERROR)] async fn traced_basic_composition() { make_request( supergraph::Request::fake_builder() @@ -68,6 +68,8 @@ async fn traced_basic_composition() { } #[test_span(tokio::test(flavor = "multi_thread"))] +#[level(tracing::Level::ERROR)] +#[target(apollo_router=tracing::Level::INFO)] async fn variables() { make_request( supergraph::Request::fake_builder() From 5f8e8d9c1913e649b44e00cda07a009c0334b175 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Thu, 1 Jun 2023 17:16:50 +0200 Subject: [PATCH 10/15] use the original query's operation signature we want the filtered query to appear under the original query's Studio entry, and it is indexed by the `statsReportKey` field in the usage reporting structure. We call the bridge again with the original query to generate that signature without going through the entire planning process again --- Cargo.lock | 4 +-- apollo-router/Cargo.toml | 2 +- .../src/query_planner/bridge_query_planner.rs | 32 ++++++++++++++++--- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ce4d506181..0b0e106b68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4878,9 +4878,9 @@ dependencies = [ [[package]] name = "router-bridge" -version = "0.2.6+v2.4.7" +version = "0.2.7+v2.4.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbcd2822ebb7b954f2444eb6691d1e4eb354cf276eb4f6dfbe59216819859623" +checksum = "e810d1bce6761c679cd70a48a2574035e8e15e798f5f7694c7c27644e10b84d5" dependencies = [ "anyhow", "async-channel", diff --git a/apollo-router/Cargo.toml b/apollo-router/Cargo.toml index 60b5f3b10a..fa15d5857b 100644 --- a/apollo-router/Cargo.toml +++ b/apollo-router/Cargo.toml @@ -170,7 +170,7 @@ reqwest = { version = "0.11.18", default-features = false, features = [ "json", "stream", ] } -router-bridge = "0.2.6+v2.4.7" +router-bridge = "0.2.7+v2.4.7" rust-embed="6.6.1" rustls = "0.20.8" rustls-pemfile = "1.0.2" diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 5fa4fae3cd..d58cd9d5fd 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -163,17 +163,30 @@ impl BridgeQueryPlanner { async fn plan( &self, query: String, + filtered_query: String, operation: Option, mut selections: Query, ) -> Result { let planner_result = self .planner - .plan(query, operation) + .plan(filtered_query.clone(), operation.clone()) .await .map_err(QueryPlannerError::RouterBridgeError)? .into_result() .map_err(QueryPlannerError::from)?; + // the `statsReportKey` field should match the original query instead of the filtered query, to index them all under the same query + let operation_signature = if query != filtered_query { + Some( + self.planner + .operation_signature(query, operation) + .await + .map_err(QueryPlannerError::RouterBridgeError)?, + ) + } else { + None + }; + match planner_result { PlanSuccess { data: @@ -181,10 +194,15 @@ impl BridgeQueryPlanner { query_plan: QueryPlan { node: Some(node) }, formatted_query_plan, }, - usage_reporting, + mut usage_reporting, } => { let subselections = node.parse_subselections(&self.schema)?; selections.subselections = subselections; + + if let Some(sig) = operation_signature { + usage_reporting.stats_report_key = sig; + } + Ok(QueryPlannerContent::Plan { plan: Arc::new(super::QueryPlan { usage_reporting, @@ -201,9 +219,13 @@ impl BridgeQueryPlanner { query_plan: QueryPlan { node: None }, .. }, - usage_reporting, + mut usage_reporting, } => { failfast_debug!("empty query plan"); + if let Some(sig) = operation_signature { + usage_reporting.stats_report_key = sig; + } + Err(QueryPlannerError::EmptyPlan(usage_reporting)) } } @@ -315,7 +337,8 @@ impl BridgeQueryPlanner { )); } - self.plan(filtered_query, operation_name, selections).await + self.plan(query, filtered_query, operation_name, selections) + .await } } @@ -412,6 +435,7 @@ mod tests { // that the query planner would return an empty plan error if it received an // introspection query .plan( + include_str!("testdata/unknown_introspection_query.graphql").into(), include_str!("testdata/unknown_introspection_query.graphql").into(), None, Query::default(), From 8ea0393fc72c3e8d94924e84423e9dee16ed318e Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 9 Jun 2023 11:17:04 +0200 Subject: [PATCH 11/15] Fix rustc/clippy errors --- .../src/query_planner/bridge_query_planner.rs | 4 ++-- .../src/query_planner/caching_query_planner.rs | 16 +++++----------- apollo-router/src/services/supergraph_service.rs | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 937240ab37..339e2d5998 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -396,7 +396,7 @@ mod tests { let result = plan( EXAMPLE_SCHEMA, include_str!("testdata/query.graphql"), - include_str!("testdata/query.graphql").into(), + include_str!("testdata/query.graphql"), None, ) .await @@ -462,8 +462,8 @@ mod tests { .plan( include_str!("testdata/unknown_introspection_query.graphql").to_string(), include_str!("testdata/unknown_introspection_query.graphql").to_string(), - query, None, + query, ) .await .unwrap_err(); diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index dc7e73e95b..e2c6ba697e 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -46,7 +46,6 @@ pub(crate) struct CachingQueryPlanner { delegate: T, schema: Arc, plugins: Arc, - configuration: Arc, } impl CachingQueryPlanner @@ -62,7 +61,7 @@ where pub(crate) async fn new( delegate: T, schema: Arc, - configuration: Arc, + configuration: &Configuration, plugins: Plugins, ) -> CachingQueryPlanner { let cache = Arc::new( @@ -76,7 +75,6 @@ where cache, delegate, schema, - configuration, plugins: Arc::new(plugins), } } @@ -399,7 +397,7 @@ mod tests { ); let mut planner = - CachingQueryPlanner::new(delegate, schema, configuration, IndexMap::new()).await; + CachingQueryPlanner::new(delegate, schema, &configuration, IndexMap::new()).await; let configuration = Configuration::default(); @@ -491,13 +489,9 @@ mod tests { Query::make_compiler("query Me { me { username } }", &schema, &configuration).0, )); - let mut planner = CachingQueryPlanner::new( - delegate, - Arc::new(schema), - Arc::new(configuration), - IndexMap::new(), - ) - .await; + let mut planner = + CachingQueryPlanner::new(delegate, Arc::new(schema), &configuration, IndexMap::new()) + .await; for _ in 0..5 { assert!(planner diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 3b77f8aad0..ef692a1c76 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -352,7 +352,7 @@ impl PluggableSupergraphServiceBuilder { let query_planner_service = CachingQueryPlanner::new( self.planner, schema.clone(), - configuration.clone(), + &configuration, IndexMap::new(), ) .await; From f08d9dd88f35b66d0d3d7d55182bec532a254c88 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 13 Jun 2023 12:00:05 +0200 Subject: [PATCH 12/15] Apply suggestions from code review Co-authored-by: Jeremy Lempereur Co-authored-by: Simon Sapin --- .changesets/feat_geal_query_planner_plugins.md | 2 +- apollo-router/src/query_planner/bridge_query_planner.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changesets/feat_geal_query_planner_plugins.md b/.changesets/feat_geal_query_planner_plugins.md index 3f6ad7ad65..0e2e30b3ef 100644 --- a/.changesets/feat_geal_query_planner_plugins.md +++ b/.changesets/feat_geal_query_planner_plugins.md @@ -1,6 +1,6 @@ ### Query planner plugins ([Issue #3150](https://github.com/apollographql/router/issues/3150)) -We may need to observe and edit the query between the query plan caching and the query planner, so this brings back the query planner plugins we had initially, with a different architecture. +We may need to observe and edit the query between query plan caching and the query planner, so this brings back the query planner plugins we had initially, with a different architecture. The plugins need an ApolloCompiler instance to perform useful work on the query, so the caching layer, in case of cache miss, will generate a compiler instance and transmit it as part of the request going through query planner plugins. At the end of the chain, the query planner extracts the modified query from the compiler, uses it to generate a query plan, and generates the selections of both the original and filtered query for response formatting. This is done to ensure that the response does not leak data removed in the filtered query, but still keeps a shape expected by the original query, using the null propagation. By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/3177 \ No newline at end of file diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 339e2d5998..79edbf89fc 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -267,15 +267,15 @@ impl Service for BridgeQueryPlanner { let fut = async move { let start = Instant::now(); + let compiler = compiler.lock().await; let file_id = compiler - .lock() - .await .db .source_file(QUERY_EXECUTABLE.into()) .ok_or(QueryPlannerError::SpecError(SpecError::ParsingError( "missing input file for query".to_string(), )))?; - let filtered_query = compiler.lock().await.db.source_code(file_id); + let filtered_query = compiler.db.source_code(file_id); + drop(compiler) let res = this .get( From 3ee3355893dc6844a9baa417196cf9fbf3611429 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 13 Jun 2023 12:04:18 +0200 Subject: [PATCH 13/15] fix mutex usage --- apollo-router/src/query_planner/bridge_query_planner.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 79edbf89fc..d188def699 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -267,15 +267,15 @@ impl Service for BridgeQueryPlanner { let fut = async move { let start = Instant::now(); - let compiler = compiler.lock().await; - let file_id = compiler + let compiler_guard = compiler.lock().await; + let file_id = compiler_guard .db .source_file(QUERY_EXECUTABLE.into()) .ok_or(QueryPlannerError::SpecError(SpecError::ParsingError( "missing input file for query".to_string(), )))?; - let filtered_query = compiler.db.source_code(file_id); - drop(compiler) + let filtered_query = compiler_guard.db.source_code(file_id); + drop(compiler_guard); let res = this .get( From 60dc1550b3579273ac6162c2979f7f21c356ce33 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 13 Jun 2023 12:04:40 +0200 Subject: [PATCH 14/15] rename query to original_query --- .../src/query_planner/bridge_query_planner.rs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index d188def699..22f9cc5426 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -170,7 +170,7 @@ impl BridgeQueryPlanner { async fn plan( &self, - query: String, + original_query: String, filtered_query: String, operation: Option, @@ -185,10 +185,10 @@ impl BridgeQueryPlanner { .map_err(QueryPlannerError::from)?; // the `statsReportKey` field should match the original query instead of the filtered query, to index them all under the same query - let operation_signature = if query != filtered_query { + let operation_signature = if original_query != filtered_query { Some( self.planner - .operation_signature(query, operation) + .operation_signature(original_query, operation) .await .map_err(QueryPlannerError::RouterBridgeError)?, ) @@ -257,7 +257,7 @@ impl Service for BridgeQueryPlanner { fn call(&mut self, req: QueryPlannerRequest) -> Self::Future { let QueryPlannerRequest { - query, + query: original_query, operation_name, context, compiler, @@ -279,7 +279,7 @@ impl Service for BridgeQueryPlanner { let res = this .get( - query, + original_query, filtered_query.to_string(), operation_name.to_owned(), compiler, @@ -322,13 +322,16 @@ impl Service for BridgeQueryPlanner { impl BridgeQueryPlanner { async fn get( &self, - query: String, + original_query: String, filtered_query: String, operation_name: Option, compiler: Arc>, ) -> Result { let mut selections = self - .parse_selections((query.clone(), operation_name.clone()), compiler.clone()) + .parse_selections( + (original_query.clone(), operation_name.clone()), + compiler.clone(), + ) .await?; if selections.contains_introspection() { @@ -350,18 +353,18 @@ impl BridgeQueryPlanner { response: Box::new(graphql::Response::builder().data(data).build()), }); } else { - return self.introspection(query).await; + return self.introspection(original_query).await; } } - if filtered_query != query { + if filtered_query != original_query { selections.filtered_query = Some(Arc::new( self.parse_selections((filtered_query.clone(), operation_name.clone()), compiler) .await?, )); } - self.plan(query, filtered_query, operation_name, selections) + self.plan(original_query, filtered_query, operation_name, selections) .await } } @@ -532,7 +535,7 @@ mod tests { async fn plan( schema: &str, - query: &str, + original_query: &str, filtered_query: &str, operation_name: Option, ) -> Result { @@ -544,11 +547,11 @@ mod tests { .await .unwrap(); - let (compiler, _) = Query::make_compiler(query, &planner.schema(), &configuration); + let (compiler, _) = Query::make_compiler(original_query, &planner.schema(), &configuration); planner .get( - query.to_string(), + original_query.to_string(), filtered_query.to_string(), operation_name, Arc::new(Mutex::new(compiler)), From 313846f4062756ebc7af7b97da75f9cb943b086f Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Wed, 14 Jun 2023 09:47:40 +0200 Subject: [PATCH 15/15] Update .changesets/feat_geal_query_planner_plugins.md Co-authored-by: Gary Pennington --- .changesets/feat_geal_query_planner_plugins.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/feat_geal_query_planner_plugins.md b/.changesets/feat_geal_query_planner_plugins.md index 0e2e30b3ef..00d491b811 100644 --- a/.changesets/feat_geal_query_planner_plugins.md +++ b/.changesets/feat_geal_query_planner_plugins.md @@ -1,6 +1,6 @@ ### Query planner plugins ([Issue #3150](https://github.com/apollographql/router/issues/3150)) -We may need to observe and edit the query between query plan caching and the query planner, so this brings back the query planner plugins we had initially, with a different architecture. +We may need to modify a query between query plan caching and the query planner. This leads to the requirement to provide a query planner plugin capability. This capability is private to the router for now. The plugins need an ApolloCompiler instance to perform useful work on the query, so the caching layer, in case of cache miss, will generate a compiler instance and transmit it as part of the request going through query planner plugins. At the end of the chain, the query planner extracts the modified query from the compiler, uses it to generate a query plan, and generates the selections of both the original and filtered query for response formatting. This is done to ensure that the response does not leak data removed in the filtered query, but still keeps a shape expected by the original query, using the null propagation. By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/3177 \ No newline at end of file