diff --git a/crates/integration-tests/src/tests/filtering.rs b/crates/integration-tests/src/tests/filtering.rs index 18ae718f..7ef45a21 100644 --- a/crates/integration-tests/src/tests/filtering.rs +++ b/crates/integration-tests/src/tests/filtering.rs @@ -1,6 +1,7 @@ use insta::assert_yaml_snapshot; +use ndc_test_helpers::{binop, field, query, query_request, target, variable}; -use crate::graphql_query; +use crate::{connector::Connector, graphql_query, run_connector_query}; #[tokio::test] async fn filters_on_extended_json_using_string_comparison() -> anyhow::Result<()> { @@ -52,3 +53,53 @@ async fn filters_by_comparisons_on_elements_of_array_field() -> anyhow::Result<( ); Ok(()) } + +#[tokio::test] +async fn filters_by_comparisons_on_elements_of_array_of_scalars() -> anyhow::Result<()> { + assert_yaml_snapshot!( + graphql_query( + r#" + query MyQuery { + movies(where: { cast: { _eq: "Albert Austin" } }) { + title + cast + } + } + "# + ) + .run() + .await? + ); + Ok(()) +} + +#[tokio::test] +async fn filters_by_comparisons_on_elements_of_array_of_scalars_against_variable( +) -> anyhow::Result<()> { + // Skip this test in MongoDB 5 because the example fails there. We're getting an error: + // + // > Kind: Command failed: Error code 5491300 (Location5491300): $documents' is not allowed in user requests, labels: {} + // + // This doesn't affect native queries that don't use the $documents stage. + if let Ok(image) = std::env::var("MONGODB_IMAGE") { + if image == "mongo:5" { + return Ok(()); + } + } + + assert_yaml_snapshot!( + run_connector_query( + Connector::SampleMflix, + query_request() + .variables([[("cast_member", "Albert Austin")]]) + .collection("movies") + .query( + query() + .predicate(binop("_eq", target!("cast"), variable!(cast_member))) + .fields([field!("title"), field!("cast")]), + ) + ) + .await? + ); + Ok(()) +} diff --git a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars.snap b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars.snap new file mode 100644 index 00000000..faf3986e --- /dev/null +++ b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars.snap @@ -0,0 +1,13 @@ +--- +source: crates/integration-tests/src/tests/filtering.rs +expression: "graphql_query(r#\"\n query MyQuery {\n movies(where: { cast: { _eq: \"Albert Austin\" } }) {\n title\n cast\n }\n }\n \"#).run().await?" +--- +data: + movies: + - title: The Immigrant + cast: + - Charles Chaplin + - Edna Purviance + - Eric Campbell + - Albert Austin +errors: ~ diff --git a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars_against_variable.snap b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars_against_variable.snap new file mode 100644 index 00000000..46425908 --- /dev/null +++ b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__filtering__filters_by_comparisons_on_elements_of_array_of_scalars_against_variable.snap @@ -0,0 +1,11 @@ +--- +source: crates/integration-tests/src/tests/filtering.rs +expression: "run_connector_query(Connector::SampleMflix,\n query_request().variables([[(\"cast_member\",\n \"Albert Austin\")]]).collection(\"movies\").query(query().predicate(binop(\"_eq\",\n target!(\"cast\"),\n variable!(cast_member))).fields([field!(\"title\"),\n field!(\"cast\")]))).await?" +--- +- rows: + - cast: + - Charles Chaplin + - Edna Purviance + - Eric Campbell + - Albert Austin + title: The Immigrant diff --git a/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs b/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs index a6ed333c..f3312356 100644 --- a/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs +++ b/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs @@ -91,6 +91,9 @@ fn scalar_type_name(t: &Type) -> Option<&'static str> { match t { Type::Scalar(MongoScalarType::Bson(s)) => Some(s.graphql_name()), Type::Scalar(MongoScalarType::ExtendedJSON) => Some(EXTENDED_JSON_TYPE_NAME), + Type::ArrayOf(t) if matches!(**t, Type::Scalar(_) | Type::Nullable(_)) => { + scalar_type_name(t) + } Type::Nullable(t) => scalar_type_name(t), _ => None, } diff --git a/crates/mongodb-agent-common/src/query/make_selector.rs b/crates/mongodb-agent-common/src/query/make_selector.rs index 0139ccec..fbb73834 100644 --- a/crates/mongodb-agent-common/src/query/make_selector.rs +++ b/crates/mongodb-agent-common/src/query/make_selector.rs @@ -147,11 +147,28 @@ pub fn make_selector(expr: &Expression) -> Result { ColumnRef::MatchKey(key) => doc! { key: { "$eq": null } }, - ColumnRef::Expression(expr) => doc! { - "$expr": { - "$eq": [expr, null] + ColumnRef::Expression(expr) => { + // Special case for array-to-scalar comparisons - this is required because implicit + // existential quantification over arrays for scalar comparisons does not work in + // aggregation expressions. + if column.get_field_type().is_array() { + doc! { + "$expr": { + "$reduce": { + "input": expr, + "initialValue": false, + "in": { "$eq": ["$$this", null] } + }, + }, + } + } else { + doc! { + "$expr": { + "$eq": [expr, null] + } + } } - }, + } }; Ok(traverse_relationship_path( column.relationship_path(), @@ -189,9 +206,26 @@ fn make_binary_comparison_selector( let comparison_value = bson_from_scalar_value(value, value_type)?; let match_doc = match ColumnRef::from_comparison_target(target_column) { ColumnRef::MatchKey(key) => operator.mongodb_match_query(key, comparison_value), - ColumnRef::Expression(expr) => doc! { - "$expr": operator.mongodb_aggregation_expression(expr, comparison_value) - }, + ColumnRef::Expression(expr) => { + // Special case for array-to-scalar comparisons - this is required because implicit + // existential quantification over arrays for scalar comparisons does not work in + // aggregation expressions. + if target_column.get_field_type().is_array() && !value_type.is_array() { + doc! { + "$expr": { + "$reduce": { + "input": expr, + "initialValue": false, + "in": operator.mongodb_aggregation_expression("$$this", comparison_value) + }, + }, + } + } else { + doc! { + "$expr": operator.mongodb_aggregation_expression(expr, comparison_value) + } + } + } }; traverse_relationship_path(target_column.relationship_path(), match_doc) } @@ -200,12 +234,28 @@ fn make_binary_comparison_selector( variable_type, } => { let comparison_value = variable_to_mongo_expression(name, variable_type); - let match_doc = doc! { - "$expr": operator.mongodb_aggregation_expression( - column_expression(target_column), - comparison_value - ) - }; + let match_doc = + // Special case for array-to-scalar comparisons - this is required because implicit + // existential quantification over arrays for scalar comparisons does not work in + // aggregation expressions. + if target_column.get_field_type().is_array() && !variable_type.is_array() { + doc! { + "$expr": { + "$reduce": { + "input": column_expression(target_column), + "initialValue": false, + "in": operator.mongodb_aggregation_expression("$$this", comparison_value) + }, + }, + } + } else { + doc! { + "$expr": operator.mongodb_aggregation_expression( + column_expression(target_column), + comparison_value + ) + } + }; traverse_relationship_path(target_column.relationship_path(), match_doc) } }; 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 a32e6326..9ec88145 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 @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use ndc_models as ndc; -use crate as plan; +use crate::{self as plan}; use super::query_plan_error::QueryPlanError; @@ -107,3 +107,45 @@ pub fn lookup_relationship<'a>( .get(relationship) .ok_or_else(|| QueryPlanError::UnspecifiedRelation(relationship.to_owned())) } + +/// Special case handling for array comparisons! Normally we assume that the right operand of Equal +/// is the same type as the left operand. BUT MongoDB allows comparing arrays to scalar values in +/// which case the condition passes if any array element is equal to the given scalar value. So +/// this function needs to return a scalar type if the user is expecting array-to-scalar +/// comparison, or an array type if the user is expecting array-to-array comparison. Or if the +/// column does not have an array type we fall back to the default assumption that the value type +/// should be the same as the column type. +/// +/// For now this assumes that if the column has an array type, the value type is a scalar type. +/// That's the simplest option since we don't support array-to-array comparisons yet. +/// +/// TODO: When we do support array-to-array comparisons we will need to either: +/// +/// - input the [ndc::ComparisonValue] into this function, and any query request variables; check +/// that the given JSON value or variable values are not array values, and if so assume the value +/// type should be a scalar type +/// - or get the GraphQL Engine to include a type with [ndc::ComparisonValue] in which case we can +/// use that as the value type +/// +/// It is important that queries behave the same when given an inline value or variables. So we +/// can't just check the value of an [ndc::ComparisonValue::Scalar], and punt on an +/// [ndc::ComparisonValue::Variable] input. The latter requires accessing query request variables, +/// and it will take a little more work to thread those through the code to make them available +/// here. +pub fn value_type_in_possible_array_equality_comparison( + column_type: plan::Type, +) -> plan::Type +where + S: Clone, +{ + match column_type { + plan::Type::ArrayOf(t) => *t, + plan::Type::Nullable(t) => match *t { + v @ plan::Type::ArrayOf(_) => { + value_type_in_possible_array_equality_comparison(v.clone()) + } + t => plan::Type::Nullable(Box::new(t)), + }, + _ => column_type, + } +} 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 6e2f7395..faedbb69 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 @@ -15,7 +15,7 @@ mod tests; use std::{collections::VecDeque, iter::once}; use crate::{self as plan, type_annotated_field, ObjectType, QueryPlan, Scope}; -use helpers::find_nested_collection_type; +use helpers::{find_nested_collection_type, value_type_in_possible_array_equality_comparison}; use indexmap::IndexMap; use itertools::Itertools; use ndc::{ExistsInCollection, QueryRequest}; @@ -516,7 +516,10 @@ fn plan_for_binary_comparison( .context .find_comparison_operator(comparison_target.get_field_type(), &operator)?; let value_type = match operator_definition { - plan::ComparisonOperatorDefinition::Equal => comparison_target.get_field_type().clone(), + plan::ComparisonOperatorDefinition::Equal => { + let column_type = comparison_target.get_field_type().clone(); + value_type_in_possible_array_equality_comparison(column_type) + } plan::ComparisonOperatorDefinition::In => { plan::Type::ArrayOf(Box::new(comparison_target.get_field_type().clone())) } diff --git a/crates/ndc-query-plan/src/type_system.rs b/crates/ndc-query-plan/src/type_system.rs index 5d67904e..7fea0395 100644 --- a/crates/ndc-query-plan/src/type_system.rs +++ b/crates/ndc-query-plan/src/type_system.rs @@ -24,6 +24,14 @@ impl Type { t => Type::Nullable(Box::new(t)), } } + + pub fn is_array(&self) -> bool { + match self { + Type::ArrayOf(_) => true, + Type::Nullable(t) => t.is_array(), + _ => false, + } + } } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/fixtures/hasura/sample_mflix/metadata/models/Movies.hml b/fixtures/hasura/sample_mflix/metadata/models/Movies.hml index 87479299..b251029c 100644 --- a/fixtures/hasura/sample_mflix/metadata/models/Movies.hml +++ b/fixtures/hasura/sample_mflix/metadata/models/Movies.hml @@ -574,8 +574,12 @@ definition: booleanExpressionType: ObjectIdComparisonExp - fieldName: awards booleanExpressionType: MoviesAwardsComparisonExp + - fieldName: cast + booleanExpressionType: StringComparisonExp - fieldName: fullplot booleanExpressionType: StringComparisonExp + - fieldName: genres + booleanExpressionType: StringComparisonExp - fieldName: imdb booleanExpressionType: MoviesImdbComparisonExp - fieldName: lastupdated