hasura · hallettj · Oct 14, 2024 · Oct 10, 2024 · Oct 10, 2024 · Oct 10, 2024
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,8 @@ This changelog documents the changes between release versions.
 
 ### Changed
 
+- **BREAKING:** If `configuration.json` cannot be parsed the connector will fail to start. This change also prohibits unknown keys in that file. These changes will help to prevent typos configuration being silently ignored. ([#115](https://github.com/hasura/ndc-mongodb/pull/115))
+
 ### Fixed
 
 - Fixes for filtering by complex predicate that references variables, or field names that require escaping ([#111](https://github.com/hasura/ndc-mongodb/pull/111))

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -27,6 +27,8 @@ indexmap = { version = "2", features = [
 itertools = "^0.12.1"
 mongodb = { version = "2.8", features = ["tracing-unstable"] }
 schemars = "^0.8.12"
+serde = { version = "1", features = ["derive"] }
+serde_json = { version = "1.0", features = ["preserve_order", "raw_value"] }
 ref-cast = "1.0.23"
 
 # Connecting to MongoDB Atlas database with time series collections fails in the

diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml
@@ -20,8 +20,8 @@ indexmap = { workspace = true }
 itertools = { workspace = true }
 ndc-models = { workspace = true }
 nom = "^7.1.3"
-serde = { version = "1.0", features = ["derive"] }
-serde_json = { version = "1.0.113", features = ["raw_value"] }
+serde = { workspace = true }
+serde_json = { workspace = true }
 thiserror = "1.0.57"
 tokio = { version = "1.36.0", features = ["full"] }
 

diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs
@@ -61,7 +61,7 @@ async fn update(context: &Context, args: &UpdateArgs) -> anyhow::Result<()> {
     let connector_state = try_init_state_from_uri(context.connection_uri.as_ref()).await?;
 
     let configuration_options =
-        configuration::parse_configuration_options_file(&context.path).await;
+        configuration::parse_configuration_options_file(&context.path).await?;
     // Prefer arguments passed to cli, and fallback to the configuration file
     let sample_size = match args.sample_size {
         Some(size) => size,

diff --git a/crates/cli/src/native_query/mod.rs b/crates/cli/src/native_query/mod.rs
@@ -56,7 +56,7 @@ pub async fn run(context: &Context, command: Command) -> anyhow::Result<()> {
             let configuration = match read_directory(&context.path).await {
                 Ok(c) => c,
                 Err(err) => {
-                    eprintln!("Could not read connector configuration - configuration must be initialized before creating native queries.\n\n{err}");
+                    eprintln!("Could not read connector configuration - configuration must be initialized before creating native queries.\n\n{err:#}");
                     exit(ExitCode::CouldNotReadConfiguration.into())
                 }
             };

diff --git a/crates/configuration/Cargo.toml b/crates/configuration/Cargo.toml
@@ -14,9 +14,13 @@ mongodb = { workspace = true }
 ndc-models = { workspace = true }
 ref-cast = { workspace = true }
 schemars = { workspace = true }
-serde = { version = "1", features = ["derive"] }
-serde_json = { version = "1" }
+serde = { workspace = true }
+serde_json = { workspace = true }
 serde_yaml = "^0.9"
 tokio = "1"
 tokio-stream = { version = "^0.1", features = ["fs"] }
 tracing = "0.1"
+
+[dev-dependencies]
+async-tempfile = "^0.6.0"
+googletest = "^0.12.0"
diff --git a/crates/configuration/src/configuration.rs b/crates/configuration/src/configuration.rs
@@ -203,7 +203,7 @@ impl Configuration {
 }
 
 #[derive(Clone, Debug, Default, Deserialize, Serialize)]
-#[serde(rename_all = "camelCase")]
+#[serde(rename_all = "camelCase", deny_unknown_fields)]
 pub struct ConfigurationOptions {
     /// Options for introspection
     pub introspection_options: ConfigurationIntrospectionOptions,
@@ -215,7 +215,7 @@ pub struct ConfigurationOptions {
 }
 
 #[derive(Copy, Clone, Debug, Deserialize, Serialize)]
-#[serde(rename_all = "camelCase")]
+#[serde(rename_all = "camelCase", deny_unknown_fields)]
 pub struct ConfigurationIntrospectionOptions {
     // For introspection how many documents should be sampled per collection.
     pub sample_size: u32,
@@ -238,7 +238,7 @@ impl Default for ConfigurationIntrospectionOptions {
 }
 
 #[derive(Clone, Debug, Default, Deserialize, Serialize)]
-#[serde(rename_all = "camelCase")]
+#[serde(rename_all = "camelCase", deny_unknown_fields)]
 pub struct ConfigurationSerializationOptions {
     /// Extended JSON has two modes: canonical and relaxed. This option determines which mode is
     /// used for output. This setting has no effect on inputs (query arguments, etc.).

diff --git a/crates/configuration/src/directory.rs b/crates/configuration/src/directory.rs
@@ -63,7 +63,7 @@ pub async fn read_directory(
         .await?
         .unwrap_or_default();
 
-    let options = parse_configuration_options_file(dir).await;
+    let options = parse_configuration_options_file(dir).await?;
 
     native_mutations.extend(native_procedures.into_iter());
 
@@ -129,24 +129,35 @@ where
     }
 }
 
-pub async fn parse_configuration_options_file(dir: &Path) -> ConfigurationOptions {
-    let json_filename = CONFIGURATION_OPTIONS_BASENAME.to_owned() + ".json";
-    let json_config_file = parse_config_file(&dir.join(json_filename), JSON).await;
-    if let Ok(config_options) = json_config_file {
-        return config_options;
+pub async fn parse_configuration_options_file(dir: &Path) -> anyhow::Result<ConfigurationOptions> {
+    let json_filename = configuration_file_path(dir, JSON);
+    if fs::try_exists(&json_filename).await? {
+        return parse_config_file(json_filename, JSON).await;
     }
 
-    let yaml_filename = CONFIGURATION_OPTIONS_BASENAME.to_owned() + ".yaml";
-    let yaml_config_file = parse_config_file(&dir.join(yaml_filename), YAML).await;
-    if let Ok(config_options) = yaml_config_file {
-        return config_options;
+    let yaml_filename = configuration_file_path(dir, YAML);
+    if fs::try_exists(&yaml_filename).await? {
+        return parse_config_file(yaml_filename, YAML).await;
     }
 
+    tracing::warn!(
+        "{CONFIGURATION_OPTIONS_BASENAME}.json not found, using default connector settings"
+    );
+
     // If a configuration file does not exist use defaults and write the file
     let defaults: ConfigurationOptions = Default::default();
     let _ = write_file(dir, CONFIGURATION_OPTIONS_BASENAME, &defaults).await;
     let _ = write_config_metadata_file(dir).await;
-    defaults
+    Ok(defaults)
+}
+
+fn configuration_file_path(dir: &Path, format: FileFormat) -> PathBuf {
+    let mut file_path = dir.join(CONFIGURATION_OPTIONS_BASENAME);
+    match format {
+        FileFormat::Json => file_path.set_extension("json"),
+        FileFormat::Yaml => file_path.set_extension("yaml"),
+    };
+    file_path
 }
 
 async fn parse_config_file<T>(path: impl AsRef<Path>, format: FileFormat) -> anyhow::Result<T>
@@ -276,3 +287,45 @@ pub async fn get_config_file_changed(dir: impl AsRef<Path>) -> anyhow::Result<bo
         _ => Ok(true),
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use async_tempfile::TempDir;
+    use googletest::prelude::*;
+    use serde_json::json;
+    use tokio::fs;
+
+    use super::{read_directory, CONFIGURATION_OPTIONS_BASENAME};
+
+    #[googletest::test]
+    #[tokio::test]
+    async fn errors_on_typo_in_extended_json_mode_string() -> Result<()> {
+        let input = json!({
+            "introspectionOptions": {
+                "sampleSize": 1_000,
+                "noValidatorSchema": true,
+                "allSchemaNullable": false,
+            },
+            "serializationOptions": {
+                "extendedJsonMode": "no-such-mode",
+            },
+        });
+
+        let config_dir = TempDir::new().await?;
+        let mut config_file = config_dir.join(CONFIGURATION_OPTIONS_BASENAME);
+        config_file.set_extension("json");
+        fs::write(config_file, serde_json::to_vec(&input)?).await?;
+
+        let actual = read_directory(config_dir).await;
+
+        expect_that!(
+            actual,
+            err(predicate(|e: &anyhow::Error| e
+                .root_cause()
+                .to_string()
+                .contains("unknown variant `no-such-mode`")))
+        );
+
+        Ok(())
+    }
+}
diff --git a/crates/integration-tests/Cargo.toml b/crates/integration-tests/Cargo.toml
@@ -14,7 +14,7 @@ anyhow = "1"
 assert_json = "^0.1"
 insta = { version = "^1.38", features = ["yaml"] }
 reqwest = { version = "^0.12.4", features = ["json"] }
-serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+serde = { workspace = true }
+serde_json = { workspace = true }
 tokio = { version = "^1.37.0", features = ["full"] }
 url = "^2.5.0"
diff --git a/crates/mongodb-agent-common/Cargo.toml b/crates/mongodb-agent-common/Cargo.toml
@@ -26,8 +26,8 @@ ndc-models = { workspace = true }
 once_cell = "1"
 regex = "1"
 schemars = { version = "^0.8.12", features = ["smol_str"] }
-serde = { version = "1.0", features = ["derive"] }
-serde_json = { version = "1.0", features = ["preserve_order"] }
+serde = { workspace = true }
+serde_json = { workspace = true }
 serde_with = { version = "^3.7", features = ["base64", "hex"] }
 thiserror = "1"
 time = { version = "0.3.29", features = ["formatting", "parsing", "serde"] }

diff --git a/crates/mongodb-connector/Cargo.toml b/crates/mongodb-connector/Cargo.toml
@@ -19,8 +19,8 @@ itertools = { workspace = true }
 mongodb = { workspace = true }
 ndc-sdk = { workspace = true }
 prometheus = "*" # share version from ndc-sdk
-serde = { version = "1.0", features = ["derive"] }
-serde_json = { version = "1.0", features = ["preserve_order"] }
+serde = { workspace = true }
+serde_json = { workspace = true }
 thiserror = "1"
 tokio = { version = "1.28.1", features = ["full"] }
 tracing = "0.1"

diff --git a/crates/mongodb-connector/src/mongo_connector.rs b/crates/mongodb-connector/src/mongo_connector.rs
@@ -38,10 +38,11 @@ impl ConnectorSetup for MongoConnector {
             .map_err(|err| {
                 ErrorResponse::new(
                     StatusCode::INTERNAL_SERVER_ERROR,
-                    err.to_string(),
+                    format!("{err:#}"), // alternate selector (:#) includes root cause in string
                     json!({}),
                 )
             })?;
+        tracing::debug!(?configuration);
         Ok(MongoConfiguration(configuration))
     }
 

diff --git a/crates/mongodb-support/Cargo.toml b/crates/mongodb-support/Cargo.toml
@@ -9,6 +9,6 @@ enum-iterator = "^2.0.0"
 indexmap = { workspace = true }
 mongodb = { workspace = true }
 schemars = "^0.8.12"
-serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+serde = { workspace = true }
+serde_json = { workspace = true }
 thiserror = "1"
diff --git a/crates/ndc-query-plan/Cargo.toml b/crates/ndc-query-plan/Cargo.toml
@@ -10,7 +10,7 @@ indexmap = { workspace = true }
 itertools = { workspace = true }
 ndc-models = { workspace = true }
 nonempty = "^0.10"
-serde_json = "1"
+serde_json = { workspace = true }
 thiserror = "1"
 ref-cast = { workspace = true }