From f269bef7aa0fa1c15924060edb0b43c64f4f9b47 Mon Sep 17 00:00:00 2001 From: David Overton Date: Wed, 19 Jun 2024 23:31:01 +1000 Subject: [PATCH 1/2] Use field type in comparison target instead of column type --- .../src/query/column_ref.rs | 20 ++++---- .../src/query/make_selector.rs | 8 ++-- .../src/plan_for_query_request/helpers.rs | 47 +++++++++++++++++++ .../src/plan_for_query_request/mod.rs | 18 +++---- .../query_plan_error.rs | 7 +++ .../src/plan_for_query_request/tests.rs | 20 ++++---- crates/ndc-query-plan/src/query_plan.rs | 10 ++-- 7 files changed, 92 insertions(+), 38 deletions(-) diff --git a/crates/mongodb-agent-common/src/query/column_ref.rs b/crates/mongodb-agent-common/src/query/column_ref.rs index 2a584724..5ed7f25c 100644 --- a/crates/mongodb-agent-common/src/query/column_ref.rs +++ b/crates/mongodb-agent-common/src/query/column_ref.rs @@ -163,7 +163,7 @@ mod tests { let target = ComparisonTarget::Column { name: "imdb".into(), field_path: Some(vec!["rating".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::Double)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::Double)), path: Default::default(), }; let actual = ColumnRef::from_comparison_target(&target); @@ -177,7 +177,7 @@ mod tests { let target = ComparisonTarget::Column { name: "subtitles".into(), field_path: Some(vec!["english.us".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }; let actual = ColumnRef::from_comparison_target(&target); @@ -199,7 +199,7 @@ mod tests { let target = ComparisonTarget::Column { name: "meta.subtitles".into(), field_path: Some(vec!["english_us".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }; let actual = ColumnRef::from_comparison_target(&target); @@ -221,7 +221,7 @@ mod tests { let target = ComparisonTarget::Column { name: "meta".into(), field_path: Some(vec!["$unsafe".into(), "$also_unsafe".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }; let actual = ColumnRef::from_comparison_target(&target); @@ -248,7 +248,7 @@ mod tests { let target = ComparisonTarget::Column { name: "valid_key".into(), field_path: Some(vec!["also_valid".into(), "$not_valid".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }; let actual = ColumnRef::from_comparison_target(&target); @@ -270,7 +270,7 @@ mod tests { let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["prop1".into(), "prop2".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); @@ -284,7 +284,7 @@ mod tests { let target = ComparisonTarget::ColumnInScope { name: "$field".into(), field_path: Default::default(), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), scope: Scope::Named("scope_0".into()), }; let actual = ColumnRef::from_comparison_target(&target); @@ -306,7 +306,7 @@ mod tests { let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["$unsafe_name".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); @@ -329,7 +329,7 @@ mod tests { let target = ComparisonTarget::ColumnInScope { name: "$field".into(), field_path: Some(vec!["$unsafe_name1".into(), "$unsafe_name2".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); @@ -361,7 +361,7 @@ mod tests { let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["prop1".into(), "$unsafe_name".into()]), - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); diff --git a/crates/mongodb-agent-common/src/query/make_selector.rs b/crates/mongodb-agent-common/src/query/make_selector.rs index 416d4d31..8cda7c46 100644 --- a/crates/mongodb-agent-common/src/query/make_selector.rs +++ b/crates/mongodb-agent-common/src/query/make_selector.rs @@ -200,7 +200,7 @@ mod tests { column: ComparisonTarget::Column { name: "Name".to_owned(), field_path: None, - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: vec!["Albums".into(), "Tracks".into()], }, operator: ComparisonFunction::Equal, @@ -236,7 +236,7 @@ mod tests { column: ComparisonTarget::Column { name: "Name".to_owned(), field_path: None, - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: vec!["Albums".into(), "Tracks".into()], }, operator: UnaryComparisonOperator::IsNull, @@ -267,7 +267,7 @@ mod tests { column: ComparisonTarget::Column { name: "Name".to_owned(), field_path: None, - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }, operator: ComparisonFunction::Equal, @@ -275,7 +275,7 @@ mod tests { column: ComparisonTarget::Column { name: "Title".to_owned(), field_path: None, - column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + field_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), path: Default::default(), }, }, diff --git a/crates/ndc-query-plan/src/plan_for_query_request/helpers.rs b/crates/ndc-query-plan/src/plan_for_query_request/helpers.rs index fe6980e1..f9c6d4b9 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/helpers.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/helpers.rs @@ -21,6 +21,53 @@ pub fn find_object_field<'a, S>( }) } +pub fn find_object_field_path<'a, S>( + object_type: &'a plan::ObjectType, + field_name: &str, + field_path: &Option>, +) -> Result<&'a plan::Type> { + match field_path { + None => find_object_field(object_type, field_name), + Some(field_path) => find_object_field_path_helper(object_type, field_name, field_path), + } +} + +fn find_object_field_path_helper<'a, S>( + object_type: &'a plan::ObjectType, + field_name: &str, + field_path: &[String], +) -> Result<&'a plan::Type> { + let field_type = find_object_field(object_type, field_name)?; + match field_path { + [] => Ok(field_type), + [nested_field_name, rest @ ..] => { + let o = find_object_type(field_type, &object_type.name, field_name)?; + find_object_field_path_helper(o, nested_field_name, rest) + } + } +} + +fn find_object_type<'a, S>( + t: &'a plan::Type, + parent_type: &Option, + field_name: &str, +) -> Result<&'a plan::ObjectType> { + match t { + crate::Type::Scalar(_) => Err(QueryPlanError::ExpectedObjectTypeAtField { + parent_type: parent_type.to_owned(), + field_name: field_name.to_owned(), + got: "scalar".to_owned(), + }), + crate::Type::ArrayOf(_) => Err(QueryPlanError::ExpectedObjectTypeAtField { + parent_type: parent_type.to_owned(), + field_name: field_name.to_owned(), + got: "array".to_owned(), + }), + crate::Type::Nullable(t) => find_object_type(t, parent_type, field_name), + crate::Type::Object(object_type) => Ok(object_type), + } +} + pub fn lookup_relationship<'a>( relationships: &'a BTreeMap, relationship: &str, diff --git a/crates/ndc-query-plan/src/plan_for_query_request/mod.rs b/crates/ndc-query-plan/src/plan_for_query_request/mod.rs index 834b1a5f..766a7a89 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/mod.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/mod.rs @@ -19,7 +19,7 @@ use ndc::{ExistsInCollection, QueryRequest}; use ndc_models as ndc; use self::{ - helpers::{find_object_field, lookup_relationship}, + helpers::{find_object_field, find_object_field_path, lookup_relationship}, query_context::QueryContext, query_plan_error::QueryPlanError, query_plan_state::QueryPlanState, @@ -478,11 +478,11 @@ fn plan_for_binary_comparison( plan_for_comparison_target(plan_state, root_collection_object_type, object_type, column)?; let (operator, operator_definition) = plan_state .context - .find_comparison_operator(comparison_target.get_column_type(), &operator)?; + .find_comparison_operator(comparison_target.get_field_type(), &operator)?; let value_type = match operator_definition { - plan::ComparisonOperatorDefinition::Equal => comparison_target.get_column_type().clone(), + plan::ComparisonOperatorDefinition::Equal => comparison_target.get_field_type().clone(), plan::ComparisonOperatorDefinition::In => { - plan::Type::ArrayOf(Box::new(comparison_target.get_column_type().clone())) + plan::Type::ArrayOf(Box::new(comparison_target.get_field_type().clone())) } plan::ComparisonOperatorDefinition::Custom { argument_type } => argument_type.clone(), }; @@ -519,20 +519,20 @@ fn plan_for_comparison_target( path, requested_columns, )?; - let column_type = find_object_field(&target_object_type, &name)?.clone(); + let field_type = find_object_field_path(&target_object_type, &name, &field_path)?.clone(); Ok(plan::ComparisonTarget::Column { name, field_path, path, - column_type, + field_type, }) } ndc::ComparisonTarget::RootCollectionColumn { name, field_path } => { - let column_type = find_object_field(root_collection_object_type, &name)?.clone(); + let field_type = find_object_field_path(root_collection_object_type, &name, &field_path)?.clone(); Ok(plan::ComparisonTarget::ColumnInScope { name, field_path, - column_type, + field_type, scope: plan_state.scope.clone(), }) } @@ -603,7 +603,7 @@ fn plan_for_exists( comparison_target.column_name().to_owned(), plan::Field::Column { column: comparison_target.column_name().to_string(), - column_type: comparison_target.get_column_type().clone(), + column_type: comparison_target.get_field_type().clone(), fields: None, }, ) diff --git a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_error.rs b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_error.rs index 6c7483d2..f0107e00 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_error.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_error.rs @@ -59,6 +59,13 @@ pub enum QueryPlanError { #[error("Query referenced a relationship, \"{0}\", but did not include relation metadata in `collection_relationships`")] UnspecifiedRelation(String), + + #[error("Expected field {field_name} of object {} to be an object type. Got {got}.", parent_type.to_owned().unwrap_or("".to_owned()))] + ExpectedObjectTypeAtField { + parent_type: Option, + field_name: String, + got: String, + }, } fn at_path(path: &[String]) -> String { diff --git a/crates/ndc-query-plan/src/plan_for_query_request/tests.rs b/crates/ndc-query-plan/src/plan_for_query_request/tests.rs index a9ac5ad1..a9e40b39 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/tests.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/tests.rs @@ -129,7 +129,7 @@ fn translates_query_request_relationships() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::Column { name: "_id".into(), field_path: None, - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), path: vec!["class_department".into()], @@ -139,7 +139,7 @@ fn translates_query_request_relationships() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::Column { name: "math_department_id".into(), field_path: None, - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), path: vec!["school_directory".into()], @@ -394,7 +394,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::Column { name: "author_id".into(), field_path: Default::default(), - column_type: plan::Type::Scalar(plan_test_helpers::ScalarType::Int), + field_type: plan::Type::Scalar(plan_test_helpers::ScalarType::Int), path: Default::default(), }, operator: plan_test_helpers::ComparisonOperator::Equal, @@ -402,7 +402,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::ColumnInScope { name: "id".into(), field_path: Default::default(), - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), scope: plan::Scope::Root, @@ -413,7 +413,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::Column { name: "title".into(), field_path: Default::default(), - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::String, ), path: Default::default(), @@ -454,7 +454,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { plan::Expression::BinaryComparisonOperator { column: plan::ComparisonTarget::Column { name: "author_id".into(), - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), field_path: None, @@ -465,7 +465,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { column: plan::ComparisonTarget::ColumnInScope { name: "id".into(), scope: plan::Scope::Root, - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), field_path: None, @@ -475,7 +475,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { plan::Expression::BinaryComparisonOperator { column: plan::ComparisonTarget::Column { name: "title".into(), - column_type: plan::Type::Scalar( + field_type: plan::Type::Scalar( plan_test_helpers::ScalarType::String, ), field_path: None, @@ -609,7 +609,7 @@ fn translates_relationships_in_fields_predicates_and_orderings() -> Result<(), a column: plan::ComparisonTarget::Column { name: "title".into(), field_path: Default::default(), - column_type: plan::Type::Scalar(plan_test_helpers::ScalarType::String), + field_type: plan::Type::Scalar(plan_test_helpers::ScalarType::String), path: Default::default(), }, operator: plan_test_helpers::ComparisonOperator::Regex, @@ -873,7 +873,7 @@ fn translates_predicate_referencing_field_of_related_collection() -> anyhow::Res column: plan::ComparisonTarget::Column { name: "name".into(), field_path: None, - column_type: plan::Type::Scalar(plan_test_helpers::ScalarType::String), + field_type: plan::Type::Scalar(plan_test_helpers::ScalarType::String), path: vec!["author".into()], }, operator: ndc_models::UnaryComparisonOperator::IsNull, diff --git a/crates/ndc-query-plan/src/query_plan.rs b/crates/ndc-query-plan/src/query_plan.rs index 323b7f8e..750fc4f5 100644 --- a/crates/ndc-query-plan/src/query_plan.rs +++ b/crates/ndc-query-plan/src/query_plan.rs @@ -303,7 +303,7 @@ pub enum ComparisonTarget { /// Path to a nested field within an object column field_path: Option>, - column_type: Type, + field_type: Type, /// Any relationships to traverse to reach this column. These are translated from /// [ndc_models::PathElement] values in the [ndc_models::QueryRequest] to names of relation @@ -321,7 +321,7 @@ pub enum ComparisonTarget { /// Path to a nested field within an object column field_path: Option>, - column_type: Type, + field_type: Type, }, } @@ -342,10 +342,10 @@ impl ComparisonTarget { } impl ComparisonTarget { - pub fn get_column_type(&self) -> &Type { + pub fn get_field_type(&self) -> &Type { match self { - ComparisonTarget::Column { column_type, .. } => column_type, - ComparisonTarget::ColumnInScope { column_type, .. } => column_type, + ComparisonTarget::Column { field_type, .. } => field_type, + ComparisonTarget::ColumnInScope { field_type, .. } => field_type, } } } From b56e6e72cd91fb781fd35cc13117a324f98ac23f Mon Sep 17 00:00:00 2001 From: David Overton Date: Wed, 19 Jun 2024 23:37:19 +1000 Subject: [PATCH 2/2] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 541f980b..ba16f2df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ This changelog documents the changes between release versions. ## [Unreleased] +- Fix bug with operator lookup when filtering on nested fields ([#82](https://github.com/hasura/ndc-mongodb/pull/82)) + ## [0.1.0] - 2024-06-13 - Support filtering and sorting by fields of related collections ([#72](https://github.com/hasura/ndc-mongodb/pull/72))