hasura · hallettj · Jan 17, 2025 · Jan 15, 2025 · Jan 15, 2025 · Jan 15, 2025
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@ This changelog documents the changes between release versions.
 
 - Upgrade dependencies to get fix for RUSTSEC-2024-0421, a vulnerability in domain name comparisons ([#138](https://github.com/hasura/ndc-mongodb/pull/138))
 - Aggregations on empty document sets now produce `null` instead of failing with an error ([#136](https://github.com/hasura/ndc-mongodb/pull/136))
+- Handle collection validators with object fields that do not list properties ([#140](https://github.com/hasura/ndc-mongodb/pull/140))
 
 #### Fix for RUSTSEC-2024-0421 / CVE-2024-12224
 
@@ -31,6 +32,25 @@ it uses the affected library exclusively to connect to MongoDB databases, and
 database URLs are supplied by trusted administrators. But better to be safe than
 sorry.
 
+#### Validators with object fields that do not list properties
+
+If a collection validator species an property of type `object`, but does not specify a list of nested properties for that object then we will infer the `ExtendedJSON` type for that property. For a collection created with this set of options would have the type `ExtendedJSON` for its `reactions` field:
+
+```json
+{
+  "validator": {
+    "$jsonSchema": {
+      "bsonType": "object",
+      "properties": {
+        "reactions": { "bsonType": "object" },
+      }
+    }
+  }
+}
+```
+
+If the validator specifies a map of nested properties, but that map is empty, then we interpret that as an empty object type.
+
 ## [1.5.0] - 2024-12-05
 
 ### Added

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml
@@ -32,6 +32,9 @@ thiserror = "1.0.57"
 tokio = { version = "1.36.0", features = ["full"] }
 
 [dev-dependencies]
+mongodb-agent-common = { path = "../mongodb-agent-common", features = ["test-helpers"] }
+
+async-tempfile = "^0.6.0"
 googletest = "^0.12.0"
 pretty_assertions = "1"
 proptest = "1"

diff --git a/crates/cli/src/introspection/sampling.rs b/crates/cli/src/introspection/sampling.rs
@@ -9,8 +9,11 @@ use configuration::{
 };
 use futures_util::TryStreamExt;
 use mongodb::bson::{doc, Bson, Document};
-use mongodb_agent_common::state::ConnectorState;
-use mongodb_support::BsonScalarType::{self, *};
+use mongodb_agent_common::mongodb::{CollectionTrait as _, DatabaseTrait};
+use mongodb_support::{
+    aggregate::{Pipeline, Stage},
+    BsonScalarType::{self, *},
+};
 
 type ObjectField = WithName<ndc_models::FieldName, schema::ObjectField>;
 type ObjectType = WithName<ndc_models::ObjectTypeName, schema::ObjectType>;
@@ -23,11 +26,10 @@ pub async fn sample_schema_from_db(
     sample_size: u32,
     all_schema_nullable: bool,
     config_file_changed: bool,
-    state: &ConnectorState,
+    db: &impl DatabaseTrait,
     existing_schemas: &HashSet<std::string::String>,
 ) -> anyhow::Result<BTreeMap<std::string::String, Schema>> {
     let mut schemas = BTreeMap::new();
-    let db = state.database();
     let mut collections_cursor = db.list_collections().await?;
 
     while let Some(collection_spec) = collections_cursor.try_next().await? {
@@ -37,7 +39,7 @@ pub async fn sample_schema_from_db(
                 &collection_name,
                 sample_size,
                 all_schema_nullable,
-                state,
+                db,
             )
             .await?;
             if let Some(collection_schema) = collection_schema {
@@ -54,14 +56,17 @@ async fn sample_schema_from_collection(
     collection_name: &str,
     sample_size: u32,
     all_schema_nullable: bool,
-    state: &ConnectorState,
+    db: &impl DatabaseTrait,
 ) -> anyhow::Result<Option<Schema>> {
-    let db = state.database();
     let options = None;
     let mut cursor = db
-        .collection::<Document>(collection_name)
-        .aggregate(vec![doc! {"$sample": { "size": sample_size }}])
-        .with_options(options)
+        .collection(collection_name)
+        .aggregate(
+            Pipeline::new(vec![Stage::Other(doc! {
+                "$sample": { "size": sample_size }
+            })]),
+            options,
+        )
         .await?;
     let mut collected_object_types = vec![];
     let is_collection_type = true;

diff --git a/crates/cli/src/introspection/validation_schema.rs b/crates/cli/src/introspection/validation_schema.rs
@@ -7,8 +7,8 @@ use configuration::{
 use futures_util::TryStreamExt;
 use mongodb::bson::from_bson;
 use mongodb_agent_common::{
+    mongodb::DatabaseTrait,
     schema::{get_property_description, Property, ValidatorSchema},
-    state::ConnectorState,
 };
 use mongodb_support::BsonScalarType;
 
@@ -19,9 +19,8 @@ type ObjectType = WithName<ndc_models::ObjectTypeName, schema::ObjectType>;
 type ObjectField = WithName<ndc_models::FieldName, schema::ObjectField>;
 
 pub async fn get_metadata_from_validation_schema(
-    state: &ConnectorState,
+    db: &impl DatabaseTrait,
 ) -> Result<BTreeMap<String, Schema>, MongoAgentError> {
-    let db = state.database();
     let mut collections_cursor = db.list_collections().await?;
 
     let mut schemas: Vec<WithName<String, Schema>> = vec![];
@@ -152,10 +151,12 @@ fn make_field_type(object_type_name: &str, prop_schema: &Property) -> (Vec<Objec
 
     match prop_schema {
         Property::Object {
-            bson_type: _,
+            properties: None, ..
+        } => (vec![], Type::ExtendedJSON),
+        Property::Object {
             description: _,
             required,
-            properties,
+            properties: Some(properties),
         } => {
             let type_prefix = format!("{object_type_name}_");
             let (otds, otd_fields): (Vec<Vec<ObjectType>>, Vec<ObjectField>) = properties
@@ -177,7 +178,6 @@ fn make_field_type(object_type_name: &str, prop_schema: &Property) -> (Vec<Objec
             (collected_otds, Type::Object(object_type_name.to_string()))
         }
         Property::Array {
-            bson_type: _,
             description: _,
             items,
         } => {

diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs
@@ -3,6 +3,8 @@
 mod exit_codes;
 mod introspection;
 mod logging;
+#[cfg(test)]
+mod tests;
 
 #[cfg(feature = "native-query-subcommand")]
 mod native_query;
@@ -13,7 +15,7 @@ use clap::{Parser, Subcommand};
 
 // Exported for use in tests
 pub use introspection::type_from_bson;
-use mongodb_agent_common::state::try_init_state_from_uri;
+use mongodb_agent_common::{mongodb::DatabaseTrait, state::try_init_state_from_uri};
 #[cfg(feature = "native-query-subcommand")]
 pub use native_query::native_query_from_pipeline;
 
@@ -49,7 +51,10 @@ pub struct Context {
 /// Run a command in a given directory.
 pub async fn run(command: Command, context: &Context) -> anyhow::Result<()> {
     match command {
-        Command::Update(args) => update(context, &args).await?,
+        Command::Update(args) => {
+            let connector_state = try_init_state_from_uri(context.connection_uri.as_ref()).await?;
+            update(context, &args, &connector_state.database()).await?
+        }
 
         #[cfg(feature = "native-query-subcommand")]
         Command::NativeQuery(command) => native_query::run(context, command).await?,
@@ -58,12 +63,14 @@ pub async fn run(command: Command, context: &Context) -> anyhow::Result<()> {
 }
 
 /// Update the configuration in the current directory by introspecting the database.
-async fn update(context: &Context, args: &UpdateArgs) -> anyhow::Result<()> {
-    let connector_state = try_init_state_from_uri(context.connection_uri.as_ref()).await?;
-
+async fn update(
+    context: &Context,
+    args: &UpdateArgs,
+    database: &impl DatabaseTrait,
+) -> anyhow::Result<()> {
     let configuration_options =
         configuration::parse_configuration_options_file(&context.path).await?;
-    // Prefer arguments passed to cli, and fallback to the configuration file
+    // Prefer arguments passed to cli, and fall back to the configuration file
     let sample_size = match args.sample_size {
         Some(size) => size,
         None => configuration_options.introspection_options.sample_size,
@@ -88,7 +95,7 @@ async fn update(context: &Context, args: &UpdateArgs) -> anyhow::Result<()> {
 
     if !no_validator_schema {
         let schemas_from_json_validation =
-            introspection::get_metadata_from_validation_schema(&connector_state).await?;
+            introspection::get_metadata_from_validation_schema(database).await?;
         configuration::write_schema_directory(&context.path, schemas_from_json_validation).await?;
     }
 
@@ -97,7 +104,7 @@ async fn update(context: &Context, args: &UpdateArgs) -> anyhow::Result<()> {
         sample_size,
         all_schema_nullable,
         config_file_changed,
-        &connector_state,
+        database,
         &existing_schemas,
     )
     .await?;

diff --git a/crates/cli/src/tests.rs b/crates/cli/src/tests.rs
@@ -0,0 +1,129 @@
+use async_tempfile::TempDir;
+use configuration::read_directory;
+use mongodb::bson::{self, doc, from_document};
+use mongodb_agent_common::mongodb::{test_helpers::mock_stream, MockDatabaseTrait};
+use ndc_models::{CollectionName, FieldName, ObjectField, ObjectType, Type};
+use pretty_assertions::assert_eq;
+
+use crate::{update, Context, UpdateArgs};
+
+#[tokio::test]
+async fn required_field_from_validator_is_non_nullable() -> anyhow::Result<()> {
+    let collection_object_type = collection_schema_from_validator(doc! {
+        "bsonType": "object",
+        "required": ["title"],
+        "properties": {
+            "title": { "bsonType": "string", "maxLength": 100 },
+            "author": { "bsonType": "string", "maxLength": 100 },
+        }
+    })
+    .await?;
+
+    assert_eq!(
+        collection_object_type
+            .fields
+            .get(&FieldName::new("title".into())),
+        Some(&ObjectField {
+            r#type: Type::Named {
+                name: "String".into()
+            },
+            arguments: Default::default(),
+            description: Default::default(),
+        })
+    );
+
+    assert_eq!(
+        collection_object_type
+            .fields
+            .get(&FieldName::new("author".into())),
+        Some(&ObjectField {
+            r#type: Type::Nullable {
+                underlying_type: Box::new(Type::Named {
+                    name: "String".into()
+                })
+            },
+            arguments: Default::default(),
+            description: Default::default(),
+        })
+    );
+
+    Ok(())
+}
+
+#[tokio::test]
+async fn validator_object_with_no_properties_becomes_extended_json_object() -> anyhow::Result<()> {
+    let collection_object_type = collection_schema_from_validator(doc! {
+        "bsonType": "object",
+        "title": "posts validator",
+        "additionalProperties": false,
+        "properties": {
+            "reactions": { "bsonType": "object" },
+        }
+    })
+    .await?;
+
+    assert_eq!(
+        collection_object_type
+            .fields
+            .get(&FieldName::new("reactions".into())),
+        Some(&ObjectField {
+            r#type: Type::Nullable {
+                underlying_type: Box::new(Type::Named {
+                    name: "ExtendedJSON".into()
+                })
+            },
+            arguments: Default::default(),
+            description: Default::default(),
+        })
+    );
+
+    Ok(())
+}
+
+async fn collection_schema_from_validator(validator: bson::Document) -> anyhow::Result<ObjectType> {
+    let mut db = MockDatabaseTrait::new();
+    let config_dir = TempDir::new().await?;
+
+    let context = Context {
+        path: config_dir.to_path_buf(),
+        connection_uri: None,
+        display_color: false,
+    };
+
+    let args = UpdateArgs {
+        sample_size: Some(100),
+        no_validator_schema: None,
+        all_schema_nullable: Some(false),
+    };
+
+    db.expect_list_collections().returning(move || {
+        let collection_spec = doc! {
+            "name": "posts",
+            "type": "collection",
+            "options": {
+                "validator": {
+                    "$jsonSchema": &validator
+                }
+            },
+            "info": { "readOnly": false },
+        };
+        Ok(mock_stream(vec![Ok(
+            from_document(collection_spec).unwrap()
+        )]))
+    });
+
+    update(&context, &args, &db).await?;
+
+    let configuration = read_directory(config_dir).await?;
+
+    let collection = configuration
+        .collections
+        .get(&CollectionName::new("posts".into()))
+        .expect("posts collection");
+    let collection_object_type = configuration
+        .object_types
+        .get(&collection.collection_type)
+        .expect("posts object type");
+
+    Ok(collection_object_type.clone())
+}