Fix defaulting in router config

apollo-router/Cargo.toml

@@ -187,6 +187,7 @@ schemars = { version = "0.8.15", features = ["url"] }
 shellexpand = "3.1.0"
 sha2 = "0.10.8"
 serde = { version = "1.0.188", features = ["derive", "rc"] }
+serde_derive_default = "0.1"
 serde_json_bytes = { version = "0.2.2", features = ["preserve_order"] }
 serde_json = { version = "1.0.107", features = [
     "preserve_order",

apollo-router/src/configuration/mod.rs

@@ -667,8 +667,7 @@ impl Supergraph {
 
 /// Configuration for operation limits, parser limits, HTTP limits, etc.
 #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
-#[serde(deny_unknown_fields)]
-#[serde(default)]
+#[serde(deny_unknown_fields, default)]
 pub(crate) struct Limits {
     /// If set, requests with operations deeper than this maximum
     /// are rejected with a HTTP 400 Bad Request response and GraphQL error with
@@ -776,16 +775,13 @@ pub(crate) struct Router {
 
 /// Automatic Persisted Queries (APQ) configuration
 #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
-#[serde(deny_unknown_fields)]
+#[serde(deny_unknown_fields, default)]
 pub(crate) struct Apq {
     /// Activates Automatic Persisted Queries (enabled by default)
-    #[serde(default = "default_apq")]
     pub(crate) enabled: bool,
 
-    #[serde(default)]
     pub(crate) router: Router,
 
-    #[serde(default)]
     pub(crate) subgraph: SubgraphConfiguration<SubgraphApq>,
 }
 
@@ -803,17 +799,12 @@ impl Apq {
 
 /// Subgraph level Automatic Persisted Queries (APQ) configuration
 #[derive(Debug, Clone, Default, Deserialize, Serialize, JsonSchema)]
-#[serde(deny_unknown_fields)]
+#[serde(deny_unknown_fields, default)]
 pub(crate) struct SubgraphApq {
     /// Enable
-    #[serde(default = "default_subgraph_apq")]
     pub(crate) enabled: bool,
 }
 
-fn default_subgraph_apq() -> bool {
-    false
-}
-
 fn default_apq() -> bool {
     true
 }

apollo-router/src/configuration/persisted_queries.rs

@@ -4,18 +4,15 @@ use serde::Serialize;
 
 /// Persisted Queries (PQ) configuration
 #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
-#[serde(deny_unknown_fields)]
+#[serde(deny_unknown_fields, default)]
 pub struct PersistedQueries {
     /// Activates Persisted Queries (disabled by default)
-    #[serde(default = "default_pq")]
     pub enabled: bool,
 
     /// Enabling this field configures the router to log any freeform GraphQL request that is not in the persisted query list
-    #[serde(default = "default_log_unknown")]
     pub log_unknown: bool,
 
     /// Restricts execution of operations that are not found in the Persisted Query List
-    #[serde(default)]
     pub safelist: PersistedQueriesSafelist,
 }
 
@@ -38,14 +35,12 @@ impl PersistedQueries {
 
 /// Persisted Queries (PQ) Safelisting configuration
 #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
-#[serde(deny_unknown_fields)]
+#[serde(deny_unknown_fields, default)]
 pub struct PersistedQueriesSafelist {
     /// Enables using the persisted query list as a safelist (disabled by default)
-    #[serde(default = "default_safelist")]
     pub enabled: bool,
 
     /// Enabling this field configures the router to reject any request that does not include the persisted query ID
-    #[serde(default = "default_require_id")]
     pub require_id: bool,
 }
 

apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__subgraph_override.snap

@@ -17,7 +17,7 @@ expression: schema
         "all": {
           "description": "options applying to all subgraphs",
           "default": {
-            "a": false,
+            "a": true,
             "b": 0
           },
           "type": "object",

apollo-router/src/configuration/tests.rs

@@ -1,5 +1,6 @@
 use std::collections::HashMap;
 use std::fs;
+use std::path::Path;
 use std::path::PathBuf;
 
 use http::Uri;
@@ -773,16 +774,17 @@ struct TestSubgraphOverride {
     subgraph: SubgraphConfiguration<PluginConfig>,
 }
 
-#[derive(Debug, Clone, Default, Deserialize, Serialize, JsonSchema)]
+#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
+#[serde(default)]
 struct PluginConfig {
-    #[serde(default = "set_true")]
     a: bool,
-    #[serde(default)]
     b: u8,
 }
 
-fn set_true() -> bool {
-    true
+impl Default for PluginConfig {
+    fn default() -> Self {
+        Self { a: true, b: 0 }
+    }
 }
 
 #[test]
@@ -853,3 +855,103 @@ fn test_subgraph_override_json() {
     // since products did not set the `a` field, it should take the override value from `all`
     assert!(!data.subgraph.subgraphs.get("products").unwrap().a);
 }
+
+#[test]
+fn test_deserialize_derive_default() {
+    // There are two types of serde defaulting:
+    //
+    // * container
+    // * field
+    //
+    // Container level defaulting will use an instance of the default implementation and take missing fields from it.
+    // Field level defaulting uses either the default implementation or optionally user supplied function to initialize missing fields.
+    //
+    // When using field level defaulting it is essential that the Default implementation of a struct exactly match the serde annotations.
+    //
+    // This test checks to ensure that field level defaulting is not used in conjunction with a Default implementation
+
+    // Walk every source file and check that #[derive(Default)] is not used.
+    let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    path.push("src");
+    fn it(path: &Path) -> impl Iterator<Item = DirEntry> {
+        WalkDir::new(path).into_iter().filter_map(|e| e.ok())
+    }
+
+    // Check for derive where Deserialize is used
+    let deserialize_regex =
+        Regex::new(r"^\s*#[\s\n]*\[derive\s*\((.*,)?\s*Deserialize\s*(,.*)?\)\s*\]\s*$").unwrap();
+    let default_regex =
+        Regex::new(r"^\s*#[\s\n]*\[derive\s*\((.*,)?\s*Default\s*(,.*)?\)\s*\]\s*$").unwrap();
+
+    let mut errors = Vec::new();
+    for source_file in it(&path).filter(|e| e.file_name().to_string_lossy().ends_with(".rs")) {
+        // Read the source file into a vec of lines
+        let source = fs::read_to_string(source_file.path()).expect("failed to read file");
+        let lines: Vec<&str> = source.lines().collect();
+        for (line_number, line) in lines.iter().enumerate() {
+            if deserialize_regex.is_match(line) {
+                // Get the struct name
+                if let Some(struct_name) = find_struct_name(&lines, line_number) {
+                    let manual_implementation = format!("impl Default for {} ", struct_name);
+
+                    let has_field_level_defaults =
+                        has_field_level_serde_defaults(&lines, line_number);
+                    let has_manual_default_impl =
+                        lines.iter().any(|f| f.contains(&manual_implementation));
+                    let has_derive_default_impl = default_regex.is_match(line);
+
+                    if (has_manual_default_impl || has_derive_default_impl)
+                        && has_field_level_defaults
+                    {
+                        errors.push(format!(
+                                    "{}:{} struct {} has field level #[serde(default=\"...\")] and also implements Default. Either use #[derive(serde_derive_default::Default)] at the container OR move the defaults into the Default implementation and use #[serde(default)] at the container level",
+                                    source_file
+                                        .path()
+                                        .strip_prefix(path.parent().unwrap().parent().unwrap())
+                                        .unwrap()
+                                        .to_string_lossy(),
+                                    line_number + 1,
+                                    struct_name));
+                    }
+                }
+            }
+        }
+    }
+
+    if !errors.is_empty() {
+        panic!("Serde errors found:\n{}", errors.join("\n"));
+    }
+}
+
+fn has_field_level_serde_defaults(lines: &[&str], line_number: usize) -> bool {
+    let serde_field_default = Regex::new(
+        r#"^\s*#[\s\n]*\[serde\s*\((.*,)?\s*default\s*=\s*"[a-zA-Z0-9_:]+"\s*(,.*)?\)\s*\]\s*$"#,
+    )
+    .unwrap();
+    lines
+        .iter()
+        .skip(line_number + 1)
+        .take(500)
+        .take_while(|line| !line.contains('}'))
+        .any(|line| serde_field_default.is_match(line))
+}
+
+fn find_struct_name(lines: &[&str], line_number: usize) -> Option<String> {
+    let struct_enum_union_regex =
+        Regex::new(r"^.*(struct|enum|union)\s([a-zA-Z0-9_]+).*$").unwrap();
+
+    lines
+        .iter()
+        .skip(line_number + 1)
+        .take(5)
+        .filter_map(|line| {
+            struct_enum_union_regex.captures(line).and_then(|c| {
+                if c.get(1).unwrap().as_str() == "struct" {
+                    Some(c.get(2).unwrap().as_str().to_string())
+                } else {
+                    None
+                }
+            })
+        })
+        .next()
+}

apollo-router/src/context/mod.rs

@@ -39,17 +39,17 @@ pub(crate) type Entries = Arc<DashMap<String, Value>>;
 /// plugins should restrict themselves to the [`Context::get`] and [`Context::upsert`]
 /// functions to minimise the possibility of mis-sequenced updates.
 #[derive(Clone, Deserialize, Serialize, Derivative)]
+#[serde(default)]
 #[derivative(Debug)]
 pub struct Context {
     // Allows adding custom entries to the context.
     entries: Entries,
 
-    #[serde(skip, default)]
+    #[serde(skip)]
     pub(crate) private_entries: Arc<parking_lot::Mutex<Extensions>>,
 
     /// Creation time
     #[serde(skip)]
-    #[serde(default = "Instant::now")]
     pub(crate) created_at: Instant,
 
     #[serde(skip)]

apollo-router/src/plugins/authentication/mod.rs

@@ -110,7 +110,7 @@ struct AuthenticationPlugin {
     subgraph: Option<SubgraphAuth>,
 }
 
-#[derive(Clone, Debug, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Deserialize, JsonSchema, serde_derive_default::Default)]
 #[serde(deny_unknown_fields)]
 struct JWTConf {
     /// List of JWKS used to verify tokens
@@ -135,17 +135,6 @@ struct JwksConf {
     #[serde(default)]
     algorithms: Option<Vec<Algorithm>>,
 }
-
-impl Default for JWTConf {
-    fn default() -> Self {
-        Self {
-            jwks: Default::default(),
-            header_name: default_header_name(),
-            header_value_prefix: default_header_value_prefix(),
-        }
-    }
-}
-
 /// Authentication
 #[derive(Clone, Debug, Default, Deserialize, JsonSchema)]
 #[serde(deny_unknown_fields)]

apollo-router/src/plugins/coprocessor/mod.rs

@@ -254,7 +254,7 @@ pub(super) struct SubgraphResponseConf {
 }
 
 /// Configures the externalization plugin
-#[derive(Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Deserialize, JsonSchema)]
 #[serde(deny_unknown_fields)]
 struct Conf {
     /// The url you'd like to offload processing to

apollo-router/src/plugins/subscription.rs

@@ -64,24 +64,19 @@ pub(crate) struct SubscriptionConfig {
     pub(crate) mode: SubscriptionModeConfig,
     /// Enable the deduplication of subscription (for example if we detect the exact same request to subgraph we won't open a new websocket to the subgraph in passthrough mode)
     /// (default: true)
-    #[serde(default = "enable_deduplication_default")]
     pub(crate) enable_deduplication: bool,
     /// This is a limit to only have maximum X opened subscriptions at the same time. By default if it's not set there is no limit.
     pub(crate) max_opened_subscriptions: Option<usize>,
     /// It represent the capacity of the in memory queue to know how many events we can keep in a buffer
     pub(crate) queue_capacity: Option<usize>,
 }
 
-fn enable_deduplication_default() -> bool {
-    true
-}
-
 impl Default for SubscriptionConfig {
     fn default() -> Self {
         Self {
             enabled: true,
             mode: Default::default(),
-            enable_deduplication: enable_deduplication_default(),
+            enable_deduplication: true,
             max_opened_subscriptions: None,
             queue_capacity: None,
         }