fix: support opt-out of strict variable validation

.changesets/breaking_plane_drawers_comedy_projector.md

@@ -0,0 +1,32 @@
+### Add config to opt-out of stricter variable validation ([PR #8884](https://github.com/apollographql/router/pull/8884))
+
+Variable validation will become **_stricter by default_** due to [PR#8821](https://github.com/apollographql/router/pull/8821). 
+This PR fixed a gap in variable validation whereby the presence of unknown fields on an input object variable were not causing a request error as they should have.
+
+This stricter validation **_may cause breakages_** for customers. 
+
+To alleviate that potential pain point while customers update their variables to be compliant, this change introduces a router config option to retain the previous level of validation and issue a warning log instead of an error.
+
+> [!WARNING]
+> If you need to opt out, you must set the config option to `warn` instead.
+
+Enabled:
+```yaml
+supergraph:
+  strict_variable_validation: enforce
+```
+
+Disabled:
+```yaml
+supergraph:
+  strict_variable_validation: warn
+```
+
+Docs have also been updated to reflect this change.
+
+<!-- [ROUTER-1602] -->
+---
+
+[ROUTER-1602]: https://apollographql.atlassian.net/browse/ROUTER-1602?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
+
+By [@carodewig](https://github.com/carodewig) and [@conwuegb](https://github.com/conwuegb) in https://github.com/apollographql/router/pull/8884

apollo-router/src/configuration/mod.rs

@@ -49,6 +49,7 @@ use self::subgraph::SubgraphConfiguration;
 use crate::ApolloRouterError;
 use crate::cache::DEFAULT_CACHE_CAPACITY;
 use crate::configuration::cooperative_cancellation::CooperativeCancellation;
+use crate::configuration::mode::WarnOrEnforceMode;
 use crate::graphql;
 use crate::plugin::plugins;
 use crate::plugins::chaos;
@@ -743,6 +744,10 @@ pub(crate) struct Supergraph {
     /// Log a message if the client closes the connection before the response is sent.
     /// Default: false.
     pub(crate) experimental_log_on_broken_pipe: bool,
+
+    /// TODO(@caroline) docs
+    #[serde(default = "default_strict_variable_validation")]
+    pub(crate) strict_variable_validation: WarnOrEnforceMode,
 }
 
 const fn default_generate_query_fragments() -> bool {
@@ -767,6 +772,7 @@ impl Supergraph {
         early_cancel: Option<bool>,
         experimental_log_on_broken_pipe: Option<bool>,
         insert_result_coercion_errors: Option<bool>,
+        strict_variable_validation: Option<WarnOrEnforceMode>,
     ) -> Self {
         Self {
             listen: listen.unwrap_or_else(default_graphql_listen),
@@ -781,6 +787,8 @@ impl Supergraph {
             early_cancel: early_cancel.unwrap_or_default(),
             experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(),
             enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(),
+            strict_variable_validation: strict_variable_validation
+                .unwrap_or_else(default_strict_variable_validation),
         }
     }
 }
@@ -800,6 +808,7 @@ impl Supergraph {
         early_cancel: Option<bool>,
         experimental_log_on_broken_pipe: Option<bool>,
         insert_result_coercion_errors: Option<bool>,
+        strict_variable_validation: Option<WarnOrEnforceMode>,
     ) -> Self {
         Self {
             listen: listen.unwrap_or_else(test_listen),
@@ -814,6 +823,8 @@ impl Supergraph {
             early_cancel: early_cancel.unwrap_or_default(),
             experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(),
             enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(),
+            strict_variable_validation: strict_variable_validation
+                .unwrap_or_else(default_strict_variable_validation),
         }
     }
 }
@@ -1506,6 +1517,10 @@ fn default_connection_shutdown_timeout() -> Duration {
     Duration::from_secs(60)
 }
 
+fn default_strict_variable_validation() -> WarnOrEnforceMode {
+    WarnOrEnforceMode::Enforce
+}
+
 #[derive(Clone, Debug, Default, Error, Display, Serialize, Deserialize, JsonSchema)]
 #[serde(deny_unknown_fields, rename_all = "snake_case")]
 pub(crate) enum BatchingMode {

apollo-router/src/configuration/mode.rs

@@ -10,3 +10,12 @@ pub(crate) enum Mode {
     Measure,
     Enforce,
 }
+
+// Don't add a default here. Instead, Default should be implemented for
+// individual cases of WarnOrEnforceMode<T>.
+#[derive(Debug, Clone, Copy, Deserialize, Serialize, JsonSchema)]
+#[serde(rename_all = "snake_case")]
+pub(crate) enum WarnOrEnforceMode {
+    Warn,
+    Enforce,
+}

apollo-router/src/services/supergraph/service.rs

@@ -25,6 +25,7 @@ use crate::Context;
 use crate::batching::BatchQuery;
 use crate::configuration::Batching;
 use crate::configuration::PersistedQueriesPrewarmQueryPlanCache;
+use crate::configuration::mode::WarnOrEnforceMode;
 use crate::error::CacheResolverError;
 use crate::graphql;
 use crate::graphql::IntoGraphQLErrors;
@@ -79,6 +80,7 @@ pub(crate) struct SupergraphService {
     query_planner_service: CachingQueryPlanner<QueryPlannerService>,
     execution_service: execution::BoxCloneService,
     schema: Arc<Schema>,
+    strict_variable_validation: WarnOrEnforceMode,
 }
 
 #[buildstructor::buildstructor]
@@ -88,11 +90,13 @@ impl SupergraphService {
         query_planner_service: CachingQueryPlanner<QueryPlannerService>,
         execution_service: execution::BoxCloneService,
         schema: Arc<Schema>,
+        strict_variable_validation: WarnOrEnforceMode,
     ) -> Self {
         SupergraphService {
             query_planner_service,
             execution_service,
             schema,
+            strict_variable_validation,
         }
     }
 }
@@ -122,22 +126,27 @@ impl Service<SupergraphRequest> for SupergraphService {
         let schema = self.schema.clone();
 
         let context_cloned = req.context.clone();
-        let fut = service_call(planning, self.execution_service.clone(), schema, req).or_else(
-            |error: BoxError| async move {
-                let errors = vec![
-                    crate::error::Error::builder()
-                        .message(error.to_string())
-                        .extension_code("INTERNAL_SERVER_ERROR")
-                        .build(),
-                ];
-
-                Ok(SupergraphResponse::infallible_builder()
-                    .errors(errors)
-                    .status_code(StatusCode::INTERNAL_SERVER_ERROR)
-                    .context(context_cloned)
-                    .build())
-            },
-        );
+        let fut = service_call(
+            planning,
+            self.execution_service.clone(),
+            schema,
+            req,
+            self.strict_variable_validation,
+        )
+        .or_else(|error: BoxError| async move {
+            let errors = vec![
+                crate::error::Error::builder()
+                    .message(error.to_string())
+                    .extension_code("INTERNAL_SERVER_ERROR")
+                    .build(),
+            ];
+
+            Ok(SupergraphResponse::infallible_builder()
+                .errors(errors)
+                .status_code(StatusCode::INTERNAL_SERVER_ERROR)
+                .context(context_cloned)
+                .build())
+        });
 
         Box::pin(fut)
     }
@@ -148,6 +157,7 @@ async fn service_call(
     execution_service: execution::BoxCloneService,
     schema: Arc<Schema>,
     req: SupergraphRequest,
+    strict_variable_validation: WarnOrEnforceMode, // todo
 ) -> Result<SupergraphResponse, BoxError> {
     let context = req.context;
     let body = req.supergraph_request.body();
@@ -306,7 +316,11 @@ async fn service_call(
                 );
                 *response.response.status_mut() = StatusCode::NOT_ACCEPTABLE;
                 Ok(response)
-            } else if let Some(err) = plan.query.validate_variables(body, &schema).err() {
+            } else if let Some(err) = plan
+                .query
+                .validate_variables(body, &schema, strict_variable_validation)
+                .err()
+            {
                 let mut res = SupergraphResponse::new_from_graphql_response(err, context);
                 *res.response.status_mut() = StatusCode::BAD_REQUEST;
                 Ok(res)
@@ -583,6 +597,7 @@ impl PluggableSupergraphServiceBuilder {
             .query_planner_service(query_planner_service.clone())
             .execution_service(execution_service)
             .schema(schema.clone())
+            .strict_variable_validation(configuration.supergraph.strict_variable_validation)
             .build();
 
         let supergraph_service =

apollo-router/src/spec/field_type.rs

@@ -5,6 +5,7 @@ use serde::Serialize;
 use serde::de::Error as _;
 
 use super::query::parse_hir_value;
+use crate::configuration::mode::WarnOrEnforceMode;
 use crate::json_ext::Value;
 use crate::json_ext::ValueExt;
 use crate::spec::Schema;
@@ -123,6 +124,7 @@ fn validate_input_value(
     value: Option<&Value>,
     schema: &Schema,
     path: &JsonValuePath<'_>,
+    strict_variable_validation: WarnOrEnforceMode, // todo
 ) -> Result<(), InvalidInputValue> {
     let fmt_path = |var_path: &JsonValuePath<'_>| match var_path {
         JsonValuePath::Variable { .. } => format!("variable `{var_path}`"),
@@ -161,12 +163,24 @@ fn validate_input_value(
                         index: i,
                         parent: path,
                     };
-                    validate_input_value(inner_type, Some(x), schema, &path)?
+                    validate_input_value(
+                        inner_type,
+                        Some(x),
+                        schema,
+                        &path,
+                        strict_variable_validation,
+                    )?
                 }
                 return Ok(());
             } else {
                 // For coercion from single value to list
-                return validate_input_value(inner_type, Some(value), schema, path);
+                return validate_input_value(
+                    inner_type,
+                    Some(value),
+                    schema,
+                    path,
+                    strict_variable_validation,
+                );
             }
         }
     };
@@ -217,7 +231,7 @@ fn validate_input_value(
                 ))
             };
 
-            let unknown = obj.keys().find_map(|k| {
+            let mut unknown_input_fields = obj.keys().filter_map(|k| {
                 let k = k.as_str();
                 if !def.fields.contains_key(k) {
                     Some(k)
@@ -226,8 +240,18 @@ fn validate_input_value(
                 }
             });
 
-            if let Some(unknown) = unknown {
-                return Err(unknown_field(unknown));
+            match strict_variable_validation {
+                WarnOrEnforceMode::Enforce => {
+                    if let Some(field) = unknown_input_fields.next() {
+                        return Err(unknown_field(field));
+                    }
+                }
+                WarnOrEnforceMode::Warn => {
+                    let unknown_fields: Vec<&str> = unknown_input_fields.collect();
+                    if !unknown_fields.is_empty() {
+                        tracing::warn!(variables = ?unknown_fields, "encountered unexpected variable(s)"); // consider just doing first? based on comment at top of fn
+                    }
+                }
             }
 
             // Validate all fields present on def
@@ -242,9 +266,21 @@ fn validate_input_value(
                             .default_value
                             .as_ref()
                             .and_then(|v| parse_hir_value(v));
-                        validate_input_value(&field.ty, default.as_ref(), schema, &path)
+                        validate_input_value(
+                            &field.ty,
+                            default.as_ref(),
+                            schema,
+                            &path,
+                            strict_variable_validation,
+                        )
                     }
-                    value => validate_input_value(&field.ty, value, schema, &path),
+                    value => validate_input_value(
+                        &field.ty,
+                        value,
+                        schema,
+                        &path,
+                        strict_variable_validation,
+                    ),
                 }
             })
         }
@@ -264,8 +300,9 @@ impl FieldType {
         value: Option<&Value>,
         schema: &Schema,
         path: &JsonValuePath<'_>,
+        strict_variable_validation: WarnOrEnforceMode,
     ) -> Result<(), InvalidInputValue> {
-        validate_input_value(&self.0, value, schema, path)
+        validate_input_value(&self.0, value, schema, path, strict_variable_validation)
     }
 
     pub(crate) fn is_non_null(&self) -> bool {

apollo-router/src/spec/query.rs

@@ -24,6 +24,7 @@ use self::subselections::SubSelectionValue;
 use super::Fragment;
 use super::QueryHash;
 use crate::Configuration;
+use crate::configuration::mode::WarnOrEnforceMode;
 use crate::error::FetchError;
 use crate::graphql::Error;
 use crate::graphql::Request;
@@ -1035,6 +1036,7 @@ impl Query {
         &self,
         request: &Request,
         schema: &Schema,
+        strict_variable_validation: WarnOrEnforceMode, // todo
     ) -> Result<(), Response> {
         if LevelFilter::current() >= LevelFilter::DEBUG {
             let known_variables = self
@@ -1078,7 +1080,7 @@ impl Query {
                     let path = super::JsonValuePath::Variable {
                         name: name.as_str(),
                     };
-                    ty.validate_input_value(value, schema, &path)
+                    ty.validate_input_value(value, schema, &path, strict_variable_validation)
                         .err()
                         .map(|message| {
                             FetchError::ValidationInvalidTypeVariable {