Get variable default values from the query for query plan condition n…

…odes (#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 <[email protected]>

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
 

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 {

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

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<String>,
+    pub(crate) query: Arc<Query>,
     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<http::Request<Request>>,
     deferred_fetches: &'a HashMap<String, Sender<(Value, Vec<Error>)>>,
+    query: &'a Arc<Query>,
     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(),
         };
 

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<Query>,
-        plan: Arc<QueryPlan>,
-    },
-    Introspection {
-        response: Box<graphql::Response>,
-    },
+    Plan { plan: Arc<QueryPlan> },
+    Introspection { response: Box<graphql::Response> },
     IntrospectionDisabled,
 }
 

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,15 +224,15 @@ where
                     .oneshot(
                         ExecutionRequest::builder()
                             .supergraph_request(req.supergraph_request)
-                            .query_plan(plan)
+                            .query_plan(plan.clone())
                             .context(context)
                             .build(),
                     )
                     .await?;
 
                 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<Query>,
+    plan: Arc<QueryPlan>,
     operation_name: Option<String>,
     variables: Map<ByteString, Value>,
     schema: Arc<Schema>,
@@ -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(),

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

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

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<graphql::Response, String> {
     reqwest::Client::new()
         .post("https://federation-demo-gateway.fly.dev/")