From be648325d7c4c617bbff6273b68269a84458e676 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Fri, 16 Sep 2022 10:32:27 +0200 Subject: [PATCH] Get variable default values from the query for query plan condition nodes (#1640) The query plan condition nodes, generated by the `if` argument of the `@defer` directive, were not using the default value of the variable passed in argument. Additionally, this fixes default value validation for all queries. Co-authored-by: Jesse Rosenberger --- NEXT_CHANGELOG.md | 8 +++ .../src/query_planner/bridge_query_planner.rs | 2 +- .../query_planner/caching_query_planner.rs | 2 +- apollo-router/src/query_planner/mod.rs | 27 +++++++--- apollo-router/src/services/query_planner.rs | 10 +--- .../src/services/supergraph_service.rs | 15 +++--- apollo-router/src/spec/query.rs | 40 +++++++++++---- apollo-router/src/spec/query/tests.rs | 5 ++ apollo-router/tests/integration_tests.rs | 51 +++++++++++++++++++ ...ation_tests__defer_default_variable-2.snap | 18 +++++++ ...ation_tests__defer_default_variable-3.snap | 13 +++++ ...gration_tests__defer_default_variable.snap | 12 +++++ 12 files changed, 170 insertions(+), 33 deletions(-) create mode 100644 apollo-router/tests/snapshots/integration_tests__defer_default_variable-2.snap create mode 100644 apollo-router/tests/snapshots/integration_tests__defer_default_variable-3.snap create mode 100644 apollo-router/tests/snapshots/integration_tests__defer_default_variable.snap diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index a7121296c4..b70b798324 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -87,6 +87,14 @@ and readiness probes continued to use the default path of `/` and so the start f By @damienpontifex in #1788 +### Get variable default values from the query for query plan condition nodes ([PR #1640](https://github.com/apollographql/router/issues/1640)) + +The query plan condition nodes, generated by the `if` argument of the `@defer` directive, were +not using the default value of the variable passed in argument. + +Additionally, this fixes default value validation for all queries. + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1640 ## 🛠 Maintenance diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 45ed665527..a0509dfbd4 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -129,11 +129,11 @@ impl BridgeQueryPlanner { usage_reporting, root: node, formatted_query_plan, + query: Arc::new(selections), options: QueryPlanOptions { enable_deduplicate_variables: self.deduplicate_variables, }, }), - query: Arc::new(selections), }) } PlanSuccess { diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index a0ac893b17..0a0381907d 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -272,9 +272,9 @@ mod tests { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), }, + query: Arc::new(Query::default()), }; let qp_content = QueryPlannerContent::Plan { - query: Arc::new(Query::default()), plan: Arc::new(query_plan), }; diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 054b68a051..f044daf71e 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -56,6 +56,7 @@ pub struct QueryPlan { pub(crate) root: PlanNode, /// String representation of the query plan (not a json representation) pub(crate) formatted_query_plan: Option, + pub(crate) query: Arc, options: QueryPlanOptions, } @@ -75,6 +76,7 @@ impl QueryPlan { }), root: root.unwrap_or_else(|| PlanNode::Sequence { nodes: Vec::new() }), formatted_query_plan: Default::default(), + query: Arc::new(Query::default()), options: QueryPlanOptions::default(), } } @@ -303,6 +305,7 @@ impl QueryPlan { schema, supergraph_request, deferred_fetches: &deferred_fetches, + query: &self.query, options: &self.options, }, &root, @@ -330,6 +333,7 @@ pub(crate) struct ExecutionParameters<'a, SF> { schema: &'a Schema, supergraph_request: &'a Arc>, deferred_fetches: &'a HashMap)>>, + query: &'a Arc, options: &'a QueryPlanOptions, } @@ -492,6 +496,7 @@ impl PlanNode { let sf = parameters.service_factory.clone(); let ctx = parameters.context.clone(); let opt = parameters.options.clone(); + let query = parameters.query.clone(); let mut primary_receiver = primary_sender.subscribe(); let mut value = parent_value.clone(); let fut = async move { @@ -526,6 +531,7 @@ impl PlanNode { schema: &sc, supergraph_request: &orig, deferred_fetches: &deferred_fetches, + query: &query, options: &opt, }, &Path::default(), @@ -611,6 +617,7 @@ impl PlanNode { supergraph_request: parameters.supergraph_request, deferred_fetches: &deferred_fetches, options: parameters.options, + query: parameters.query, }, current_dir, &value, @@ -641,12 +648,15 @@ impl PlanNode { value = Value::default(); errors = Vec::new(); - if let Some(&Value::Bool(true)) = parameters - .supergraph_request - .body() - .variables - .get(condition.as_str()) - { + let v: &Value = parameters + .supergraph_request + .body() + .variables + .get(condition.as_str()).or_else(|| parameters.query.default_variable_value(parameters + .supergraph_request.body().operation_name.as_deref(),condition.as_str())) + .expect("defer expects a `Boolean!` so the variable should be non null too and present or with a default value"); + + if let &Value::Bool(true) = v { //FIXME: should we show an error if the if_node was not present? if let Some(node) = if_clause { let span = tracing::info_span!("condition_if"); @@ -1272,6 +1282,7 @@ mod tests { root: serde_json::from_str(test_query_plan!()).unwrap(), formatted_query_plan: Default::default(), options: QueryPlanOptions::default(), + query: Arc::new(Query::default()), usage_reporting: UsageReporting { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), @@ -1325,6 +1336,7 @@ mod tests { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), }, + query: Arc::new(Query::default()), options: QueryPlanOptions::default(), }; @@ -1379,6 +1391,7 @@ mod tests { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), }, + query: Arc::new(Query::default()), options: QueryPlanOptions::default(), }; @@ -1492,6 +1505,7 @@ mod tests { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), }, + query: Arc::new(Query::default()), options: QueryPlanOptions::default(), }; @@ -1642,6 +1656,7 @@ mod tests { stats_report_key: "this is a test report key".to_string(), referenced_fields_by_type: Default::default(), }, + query: Arc::new(Query::default()), options: QueryPlanOptions::default(), }; diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index 679fade8f7..192771fa02 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -6,7 +6,6 @@ use static_assertions::assert_impl_all; use crate::graphql; use crate::query_planner::QueryPlan; -use crate::spec::Query; use crate::Context; assert_impl_all!(Request: Send); @@ -45,13 +44,8 @@ pub(crate) struct Response { /// Query, QueryPlan and Introspection data. #[derive(Debug, Clone)] pub(crate) enum QueryPlannerContent { - Plan { - query: Arc, - plan: Arc, - }, - Introspection { - response: Box, - }, + Plan { plan: Arc }, + Introspection { response: Box }, IntrospectionDisabled, } diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index f6e310df3c..f9b3ad08bd 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -27,6 +27,7 @@ use tower::ServiceExt; use tower_service::Service; use tracing_futures::Instrument; +use super::execution::QueryPlan; use super::new_service::NewService; use super::subgraph_service::MakeSubgraphService; use super::subgraph_service::SubgraphCreator; @@ -51,7 +52,6 @@ use crate::router_factory::Endpoint; use crate::router_factory::SupergraphServiceFactory; use crate::services::layers::apq::APQLayer; use crate::services::layers::ensure_query_presence::EnsureQueryPresence; -use crate::spec::Query; use crate::Configuration; use crate::Context; use crate::ExecutionRequest; @@ -201,7 +201,8 @@ where *response.response.status_mut() = StatusCode::BAD_REQUEST; Ok(response) } - Some(QueryPlannerContent::Plan { query, plan }) => { + + Some(QueryPlannerContent::Plan { plan }) => { let can_be_deferred = plan.root.contains_defer(); if can_be_deferred && !accepts_multipart(req.supergraph_request.headers()) { @@ -212,7 +213,7 @@ where .build(), context); *response.response.status_mut() = StatusCode::NOT_ACCEPTABLE; Ok(response) - } else if let Some(err) = query.validate_variables(body, &schema).err() { + } else if let Some(err) = plan.query.validate_variables(body, &schema).err() { let mut res = SupergraphResponse::new_from_graphql_response(err, context); *res.response.status_mut() = StatusCode::BAD_REQUEST; Ok(res) @@ -223,7 +224,7 @@ where .oneshot( ExecutionRequest::builder() .supergraph_request(req.supergraph_request) - .query_plan(plan) + .query_plan(plan.clone()) .context(context) .build(), ) @@ -231,7 +232,7 @@ where process_execution_response( execution_response, - query, + plan, operation_name, variables, schema, @@ -298,7 +299,7 @@ fn accepts_multipart(headers: &HeaderMap) -> bool { fn process_execution_response( execution_response: ExecutionResponse, - query: Arc, + plan: Arc, operation_name: Option, variables: Map, schema: Arc, @@ -311,7 +312,7 @@ fn process_execution_response( let stream = response_stream .map(move |mut response: Response| { tracing::debug_span!("format_response").in_scope(|| { - query.format_response( + plan.query.format_response( &mut response, operation_name.as_deref(), variables.clone(), diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index c4bb9dd0ad..c649aad4e5 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -51,14 +51,7 @@ impl Query { ) { let data = std::mem::take(&mut response.data); if let Some(Value::Object(mut input)) = data { - let operation = match operation_name { - Some(name) => self - .operations - .iter() - // we should have an error if the only operation is anonymous but the query specifies a name - .find(|op| op.name.is_some() && op.name.as_deref().unwrap() == name), - None => self.operations.get(0), - }; + let operation = self.operation(operation_name); if let Some(subselection) = &response.subselection { // Get subselection from hashmap match self.subselections.get(&( @@ -776,8 +769,12 @@ impl Query { let errors = operation_variable_types .iter() - .filter_map(|(name, (ty, _))| { - let value = request.variables.get(*name).unwrap_or(&Value::Null); + .filter_map(|(name, (ty, default_value))| { + let value = request + .variables + .get(*name) + .or(default_value.as_ref()) + .unwrap_or(&Value::Null); ty.validate_input_value(value, schema).err().map(|_| { FetchError::ValidationInvalidTypeVariable { name: name.to_string(), @@ -801,6 +798,29 @@ impl Query { pub(crate) fn contains_introspection(&self) -> bool { self.operations.iter().any(Operation::is_introspection) } + + pub(crate) fn default_variable_value( + &self, + operation_name: Option<&str>, + variable_name: &str, + ) -> Option<&Value> { + self.operation(operation_name).and_then(|op| { + op.variables + .get(variable_name) + .and_then(|(_, value)| value.as_ref()) + }) + } + + fn operation(&self, operation_name: Option<&str>) -> Option<&Operation> { + match operation_name { + Some(name) => self + .operations + .iter() + // we should have an error if the only operation is anonymous but the query specifies a name + .find(|op| op.name.is_some() && op.name.as_deref().unwrap() == name), + None => self.operations.get(0), + } + } } /// Intermediate structure for arguments passed through the entire formatting diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 4fafebdbf6..e74bd4a386 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1014,6 +1014,8 @@ fn variable_validation() { // https://spec.graphql.org/June2018/#sec-Int assert_validation!(schema, "query($foo:Int){x}", json!({})); assert_validation_error!(schema, "query($foo:Int!){x}", json!({})); + assert_validation!(schema, "query($foo:Int=1){x}", json!({})); + assert_validation!(schema, "query($foo:Int!=1){x}", json!({})); // When expected as an input type, only integer input values are accepted. assert_validation!(schema, "query($foo:Int){x}", json!({"foo":2})); assert_validation!(schema, "query($foo:Int){x}", json!({ "foo": i32::MAX })); @@ -1071,6 +1073,9 @@ fn variable_validation() { assert_validation_error!(schema, "query($foo:Boolean!){x}", json!({"foo": 0})); assert_validation_error!(schema, "query($foo:Boolean!){x}", json!({"foo": "no"})); + assert_validation!(schema, "query($foo:Boolean=true){x}", json!({})); + assert_validation!(schema, "query($foo:Boolean!=true){x}", json!({})); + // https://spec.graphql.org/June2018/#sec-ID assert_validation!(schema, "query($foo:ID){x}", json!({})); assert_validation_error!(schema, "query($foo:ID!){x}", json!({})); diff --git a/apollo-router/tests/integration_tests.rs b/apollo-router/tests/integration_tests.rs index 59843f98b6..4ff1b4e513 100644 --- a/apollo-router/tests/integration_tests.rs +++ b/apollo-router/tests/integration_tests.rs @@ -684,6 +684,57 @@ async fn defer_query_without_accept() { insta::assert_json_snapshot!(first); } +#[tokio::test(flavor = "multi_thread")] +async fn defer_default_variable() { + let config = serde_json::json!({ + "include_subgraph_errors": { + "all": true + } + }); + + let query = r#"query X($if: Boolean! = true){ + me { + id + ...@defer(label: "name", if: $if) { + name + } + } + }"#; + + let request = supergraph::Request::fake_builder() + .query(query) + .header(ACCEPT, "multipart/mixed; deferSpec=20220824") + .build() + .expect("expecting valid request"); + + let (router, _) = setup_router_and_registry(config.clone()).await; + + let mut stream = router.oneshot(request).await.unwrap(); + + let first = stream.next_response().await.unwrap(); + insta::assert_json_snapshot!(first); + + let second = stream.next_response().await.unwrap(); + insta::assert_json_snapshot!(second); + + let request = supergraph::Request::fake_builder() + .query(query) + .variable("if", false) + .header(ACCEPT, "multipart/mixed; deferSpec=20220824") + .build() + .expect("expecting valid request"); + + let (router, _) = setup_router_and_registry(config).await; + + let mut stream = router.oneshot(request).await.unwrap(); + + let first = stream.next_response().await.unwrap(); + insta::assert_json_snapshot!(first); + + let second = stream.next_response().await; + assert!(second.is_none()); +} + async fn query_node(request: &supergraph::Request) -> Result { reqwest::Client::new() .post("https://federation-demo-gateway.fly.dev/") diff --git a/apollo-router/tests/snapshots/integration_tests__defer_default_variable-2.snap b/apollo-router/tests/snapshots/integration_tests__defer_default_variable-2.snap new file mode 100644 index 0000000000..e0ee201e4b --- /dev/null +++ b/apollo-router/tests/snapshots/integration_tests__defer_default_variable-2.snap @@ -0,0 +1,18 @@ +--- +source: apollo-router/tests/integration_tests.rs +expression: second +--- +{ + "hasNext": false, + "incremental": [ + { + "label": "name", + "data": { + "name": "Ada Lovelace" + }, + "path": [ + "me" + ] + } + ] +} diff --git a/apollo-router/tests/snapshots/integration_tests__defer_default_variable-3.snap b/apollo-router/tests/snapshots/integration_tests__defer_default_variable-3.snap new file mode 100644 index 0000000000..2329bdd6ca --- /dev/null +++ b/apollo-router/tests/snapshots/integration_tests__defer_default_variable-3.snap @@ -0,0 +1,13 @@ +--- +source: apollo-router/tests/integration_tests.rs +expression: first +--- +{ + "data": { + "me": { + "id": "1", + "name": "Ada Lovelace" + } + }, + "hasNext": false +} diff --git a/apollo-router/tests/snapshots/integration_tests__defer_default_variable.snap b/apollo-router/tests/snapshots/integration_tests__defer_default_variable.snap new file mode 100644 index 0000000000..65ea4d19b4 --- /dev/null +++ b/apollo-router/tests/snapshots/integration_tests__defer_default_variable.snap @@ -0,0 +1,12 @@ +--- +source: apollo-router/tests/integration_tests.rs +expression: first +--- +{ + "data": { + "me": { + "id": "1" + } + }, + "hasNext": true +}