Skip to content

Commit

Permalink
switch configuration types to maps (#12)
Browse files Browse the repository at this point in the history 
In some earlier feedback @dmoverton pointed out it would be better to store native queries in maps, as opposed to vecs. I thought the same logic would apply to some of the other configuration fields. It's also been bugging me that the configuration formats didn't match pattern in ddn and ndc-postgres in that we were using lists of named objects instead of maps.

I switched over some types from `Vec<StructWithNameField>` to `BTreeMap<String, StructWithoutNameField>`. To smooth things out I also added a `WithName` type that can wrap any struct, and adds a `name` field that appears at the same level as the underlying struct's fields in serialized formats. That lets us deserialize either without a name (for example if we're getting names from map keys in `schema.json` instead of from map value fields), or with a name (like if we split up schema configuration to put info for each collection into it's own file, we'll want to read the name from a field in the file.) I used that method to update `read_subdir_configs` in `directory.rs` so that it automatically grabs a name from each file.

The changes ended up being more extensive than I'd hoped so let me know if it's too much. But since this changes configuration formats I thought if we want to make this change it's better not to wait.

https://hasurahq.atlassian.net/browse/MDB-91
  • Loading branch information
@hallettj
hallettj authored Mar 25, 2024
1 parent b034baf commit 2bcbfd3
Show file tree
Hide file tree
Showing 19 changed files with 699 additions and 644 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion crates/cli/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ anyhow = "1.0.80"
clap = { version = "4.5.1", features = ["derive", "env"] }
futures-util = "0.3.28"
indexmap = { version = "1", features = ["serde"] } # must match the version that ndc-client uses
itertools = "^0.12.1"
serde = { version = "1.0", features = ["derive"] }
serde_json = { version = "1.0.113", features = ["raw_value"] }
thiserror = "1.0.57"
tokio = { version = "1.36.0", features = ["full"] }
these = "2.0.0"

[dev-dependencies]
proptest = "1"
proptest = "1"
180 changes: 109 additions & 71 deletions crates/cli/src/introspection/sampling.rs
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
use std::collections::BTreeMap;

use super::type_unification::{
unify_object_types, unify_schema, unify_type, TypeUnificationContext, TypeUnificationResult,
};
use configuration::{
schema::{Collection, ObjectField, ObjectType, Type},
Schema,
schema::{self, Type},
Schema, WithName,
};
use futures_util::TryStreamExt;
use mongodb::bson::{doc, Bson, Document};
use mongodb_agent_common::interface_types::MongoConfig;
use mongodb_support::BsonScalarType::{self, *};

type ObjectField = WithName<schema::ObjectField>;
type ObjectType = WithName<schema::ObjectType>;

/// Sample from all collections in the database and return a Schema.
/// Return an error if there are any errors accessing the database
/// or if the types derived from the sample documents for a collection
Expand All @@ -19,8 +24,8 @@ pub async fn sample_schema_from_db(
config: &MongoConfig,
) -> anyhow::Result<Schema> {
let mut schema = Schema {
collections: vec![],
object_types: vec![],
collections: BTreeMap::new(),
object_types: BTreeMap::new(),
};
let db = config.client.database(&config.database);
let mut collections_cursor = db.list_collections(None, None).await?;
Expand Down Expand Up @@ -54,15 +59,17 @@ async fn sample_schema_from_collection(
unify_object_types(collected_object_types, object_types)?
};
}
let collection_info = Collection {
name: collection_name.to_string(),
description: None,
r#type: collection_name.to_string(),
};
let collection_info = WithName::named(
collection_name.to_string(),
schema::Collection {
description: None,
r#type: collection_name.to_string(),
},
);

Ok(Schema {
collections: vec![collection_info],
object_types: collected_object_types,
collections: WithName::into_map([collection_info]),
object_types: WithName::into_map(collected_object_types),
})
}

Expand All @@ -83,11 +90,13 @@ fn make_object_type(
(object_type_defs.concat(), object_fields)
};

let object_type = ObjectType {
name: object_type_name.to_string(),
description: None,
fields: object_fields,
};
let object_type = WithName::named(
object_type_name.to_string(),
schema::ObjectType {
description: None,
fields: WithName::into_map(object_fields),
},
);

object_type_defs.push(object_type);
Ok(object_type_defs)
Expand All @@ -101,11 +110,13 @@ fn make_object_field(
let object_type_name = format!("{type_prefix}{field_name}");
let (collected_otds, field_type) = make_field_type(&object_type_name, field_name, field_value)?;

let object_field = ObjectField {
name: field_name.to_owned(),
description: None,
r#type: field_type,
};
let object_field = WithName::named(
field_name.to_owned(),
schema::ObjectField {
description: None,
r#type: field_type,
},
);

Ok((collected_otds, object_field))
}
Expand Down Expand Up @@ -164,7 +175,12 @@ fn make_field_type(

#[cfg(test)]
mod tests {
use configuration::schema::{ObjectField, ObjectType, Type};
use std::collections::BTreeMap;

use configuration::{
schema::{ObjectField, ObjectType, Type},
WithName,
};
use mongodb::bson::doc;
use mongodb_support::BsonScalarType;

Expand All @@ -176,24 +192,30 @@ mod tests {
fn simple_doc() -> Result<(), anyhow::Error> {
let object_name = "foo";
let doc = doc! {"my_int": 1, "my_string": "two"};
let result = make_object_type(object_name, &doc);
let result = make_object_type(object_name, &doc).map(WithName::into_map::<BTreeMap<_, _>>);

let expected = Ok(vec![ObjectType {
name: object_name.to_owned(),
fields: vec![
ObjectField {
name: "my_int".to_owned(),
r#type: Type::Scalar(BsonScalarType::Int),
description: None,
},
ObjectField {
name: "my_string".to_owned(),
r#type: Type::Scalar(BsonScalarType::String),
description: None,
},
],
description: None,
}]);
let expected = Ok(BTreeMap::from([(
object_name.to_owned(),
ObjectType {
fields: BTreeMap::from([
(
"my_int".to_owned(),
ObjectField {
r#type: Type::Scalar(BsonScalarType::Int),
description: None,
},
),
(
"my_string".to_owned(),
ObjectField {
r#type: Type::Scalar(BsonScalarType::String),
description: None,
},
),
]),
description: None,
},
)]));

assert_eq!(expected, result);

Expand All @@ -204,40 +226,56 @@ mod tests {
fn array_of_objects() -> Result<(), anyhow::Error> {
let object_name = "foo";
let doc = doc! {"my_array": [{"foo": 42, "bar": ""}, {"bar": "wut", "baz": 3.77}]};
let result = make_object_type(object_name, &doc);
let result = make_object_type(object_name, &doc).map(WithName::into_map::<BTreeMap<_, _>>);

let expected = Ok(vec![
ObjectType {
name: "foo_my_array".to_owned(),
fields: vec![
ObjectField {
name: "foo".to_owned(),
r#type: Type::Nullable(Box::new(Type::Scalar(BsonScalarType::Int))),
description: None,
},
ObjectField {
name: "bar".to_owned(),
r#type: Type::Scalar(BsonScalarType::String),
description: None,
},
ObjectField {
name: "baz".to_owned(),
r#type: Type::Nullable(Box::new(Type::Scalar(BsonScalarType::Double))),
description: None,
},
],
description: None,
},
ObjectType {
name: object_name.to_owned(),
fields: vec![ObjectField {
name: "my_array".to_owned(),
r#type: Type::ArrayOf(Box::new(Type::Object("foo_my_array".to_owned()))),
let expected = Ok(BTreeMap::from([
(
"foo_my_array".to_owned(),
ObjectType {
fields: BTreeMap::from([
(
"foo".to_owned(),
ObjectField {
r#type: Type::Nullable(Box::new(Type::Scalar(BsonScalarType::Int))),
description: None,
},
),
(
"bar".to_owned(),
ObjectField {
r#type: Type::Scalar(BsonScalarType::String),
description: None,
},
),
(
"baz".to_owned(),
ObjectField {
r#type: Type::Nullable(Box::new(Type::Scalar(
BsonScalarType::Double,
))),
description: None,
},
),
]),
description: None,
}],
description: None,
},
]);
},
),
(
object_name.to_owned(),
ObjectType {
fields: BTreeMap::from([(
"my_array".to_owned(),
ObjectField {
r#type: Type::ArrayOf(Box::new(Type::Object(
"foo_my_array".to_owned(),
))),
description: None,
},
)]),
description: None,
},
),
]));

assert_eq!(expected, result);

Expand Down
Loading

0 comments on commit 2bcbfd3

Please sign in to comment.