From a95eaa4c4fafd63953fb78eb0e0b4b3df9590029 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Mon, 29 Jul 2024 12:50:26 +0100 Subject: [PATCH] Allow object types to be used as comparison operator arguments (#895) ### What This allows object types to be used as arguments for comparison operators. This is useful for Elasticsearch's `range` operator, which allows passing an object like `{ gt: 1, lt: 100 }` to an `integer` field in order to filter items that are greater than `1` and less than `100`. This PR has the nice side effect of dropping the requirement to use information from scalar `BooleanExpressionType`s in place of `DataConnectorScalarTypes`, which we only required because we were not looking up the comparable operator information in scalar boolean expression types correctly. ### How Previously, when using `ObjectBooleanExpressionType` and `DataConnectorScalarRepresentation`, we had no information about the argument types of comparison operators (ie, what values should I pass to `_eq`?), and so inferred this by looking up the comparison operator in the data connector schema, then looking for a `DataConnectorScalarRepresentation` that tells us what OpenDD type that maps to. Now, with `BooleanExpressionType`, we have this information provided in OpenDD itself: ```yaml kind: BooleanExpressionType version: v1 definition: name: Int_comparison_exp operand: scalar: type: Int comparisonOperators: - name: _eq argumentType: Int! # This is an OpenDD type - name: _within argumentType: WithinInput! - name: _in argumentType: "[Int!]!" ``` Now we look up this information properly, as well as tightening up some validation around relationships that was making us fall back to the old way of doing things where the user had failed to provide a `comparableRelationship` entry. This means a) we can actually use object types as comparable operator types b) scalar boolean expression types aren't used outside the world of boolean expressions, which is a lot easier to reason about. V3_GIT_ORIGIN_REV_ID: ad5896c7f3dbf89a38e7a11ca9ae855a197211e3 --- v3/changelog.md | 3 + .../boolean_expression_type/metadata.json | 2 +- .../common_metadata.json | 16 +- .../metadata-resolve/src/helpers/argument.rs | 160 ++++++++++++------ v3/crates/metadata-resolve/src/helpers/mod.rs | 1 - .../src/stages/boolean_expressions/graphql.rs | 16 +- .../src/stages/boolean_expressions/object.rs | 1 - .../data_connector_scalar_types/error.rs | 8 +- .../stages/data_connector_scalar_types/mod.rs | 48 ------ v3/crates/metadata-resolve/src/stages/mod.rs | 9 +- .../object_boolean_expressions/helpers.rs} | 3 +- .../stages/object_boolean_expressions/mod.rs | 3 +- .../scalar_boolean_expressions/error.rs | 11 +- .../stages/scalar_boolean_expressions/mod.rs | 8 +- .../scalar_boolean_expressions/scalar.rs | 34 +++- .../scalar_boolean_expressions/types.rs | 6 +- v3/crates/metadata-resolve/src/types/error.rs | 11 +- .../basic/resolved.snap | 12 +- .../no_graphql/resolved.snap | 12 +- .../range/metadata.json | 33 +++- .../range/resolved.snap | 138 ++++++++++++++- .../two_data_sources/resolved.snap | 12 +- 22 files changed, 379 insertions(+), 168 deletions(-) rename v3/crates/metadata-resolve/src/{helpers/model.rs => stages/object_boolean_expressions/helpers.rs} (95%) diff --git a/v3/changelog.md b/v3/changelog.md index 30fef777dac..8132c9d66e5 100644 --- a/v3/changelog.md +++ b/v3/changelog.md @@ -10,6 +10,9 @@ ### Fixed +- Fix use of object types as comparison operator arguments by correctly + utilising user-provided OpenDD types. + ### Changed - Introduced `AuthConfig` `v2`. This new version removes role emulation in diff --git a/v3/crates/engine/tests/execute/commands/functions/boolean_expression_command_argument/boolean_expression_type/metadata.json b/v3/crates/engine/tests/execute/commands/functions/boolean_expression_command_argument/boolean_expression_type/metadata.json index fe65b32a714..79b3278fc76 100644 --- a/v3/crates/engine/tests/execute/commands/functions/boolean_expression_command_argument/boolean_expression_type/metadata.json +++ b/v3/crates/engine/tests/execute/commands/functions/boolean_expression_command_argument/boolean_expression_type/metadata.json @@ -93,7 +93,7 @@ "scalar": { "type": "Int", "comparisonOperators": [ - { "name": "equals", "argumentType": "Int!" } + { "name": "fancy_equals_operator", "argumentType": "Int!" } ], "dataConnectorOperatorMapping": [ { diff --git a/v3/crates/engine/tests/execute/models/select_many/relationship_predicates/common_metadata.json b/v3/crates/engine/tests/execute/models/select_many/relationship_predicates/common_metadata.json index 859aba36550..6092750c0eb 100644 --- a/v3/crates/engine/tests/execute/models/select_many/relationship_predicates/common_metadata.json +++ b/v3/crates/engine/tests/execute/models/select_many/relationship_predicates/common_metadata.json @@ -169,7 +169,15 @@ "booleanExpressionType": "int_bool_exp" } ], - "comparableRelationships": [{ "relationshipName": "Album" }] + "comparableRelationships": [ + { "relationshipName": "Album" }, + { + "relationshipName": "TrackAlbums" + }, + { + "relationshipName": "Genre" + } + ] } }, "logicalOperators": { "enable": true }, @@ -621,7 +629,6 @@ "version": "v1", "kind": "ObjectType" }, - { "kind": "BooleanExpressionType", "version": "v1", @@ -631,7 +638,8 @@ "scalar": { "type": "String", "comparisonOperators": [ - { "name": "_eq", "argumentType": "String!" } + { "name": "_eq", "argumentType": "String!" }, + { "name": "_ilike", "argumentType": "String!" } ], "dataConnectorOperatorMapping": [ { @@ -696,7 +704,7 @@ "booleanExpressionType": "string_bool_exp" } ], - "comparableRelationships": [] + "comparableRelationships": [{ "relationshipName": "Albums" }] } }, "logicalOperators": { "enable": true }, diff --git a/v3/crates/metadata-resolve/src/helpers/argument.rs b/v3/crates/metadata-resolve/src/helpers/argument.rs index 7bec171f50f..64edf60c3a7 100644 --- a/v3/crates/metadata-resolve/src/helpers/argument.rs +++ b/v3/crates/metadata-resolve/src/helpers/argument.rs @@ -1,4 +1,3 @@ -use crate::helpers::model::resolve_ndc_type; use crate::helpers::ndc_validation; use crate::helpers::type_mappings; use crate::helpers::types::{ @@ -23,6 +22,7 @@ use open_dds::data_connector::{ }; use open_dds::models::ModelName; use open_dds::permissions; +use open_dds::relationships::RelationshipName; use open_dds::types::DataConnectorArgumentName; use open_dds::types::{BaseType, CustomTypeName, FieldName, OperatorName, TypeName, TypeReference}; use ref_cast::RefCast; @@ -223,7 +223,7 @@ pub(crate) fn resolve_value_expression_for_argument( })?; // lookup the relevant boolean expression type and get the underlying object type - let (boolean_expression_type, object_type_representation) = + let (boolean_expression_graphql, object_type_representation) = match object_boolean_expression_types.get(base_type) { Some(object_boolean_expression_type) => Ok(( None, @@ -295,7 +295,7 @@ pub(crate) fn resolve_value_expression_for_argument( bool_exp, base_type, object_type_representation, - boolean_expression_type, + boolean_expression_graphql, data_connector_field_mappings, data_connector_link, subgraph, @@ -363,10 +363,9 @@ pub(crate) fn resolve_model_predicate_with_type( }) })?; - // newer boolean expression types have operator_mappings that allow us - // to rename operators, if we have those, we'll need to fetch them - let operator_mapping = - boolean_expression_graphql.map_or(Ok(BTreeMap::new()), |graphql| { + // for newer boolean expressions we already have the information we need + let (resolved_operator, argument_type) = match boolean_expression_graphql { + Some(graphql) => { // lookup this field let comparable_field = graphql.scalar_fields.get(field).ok_or_else(|| { Error::from(TypePredicateError::UnknownFieldInTypePredicate { @@ -376,28 +375,46 @@ pub(crate) fn resolve_model_predicate_with_type( })?; // get any operator mappings - comparable_field + let operator_mappings = comparable_field .operator_mapping .get(&data_connector_link.name) - .cloned() .ok_or_else(|| { Error::from(TypePredicateError::OperatorMappingsNotFound { data_connector_name: data_connector_link.name.clone(), }) - }) - })?; + })?; - let (resolved_operator, argument_type) = resolve_binary_operator_for_type( - operator, - type_name, - &data_connector_link.name, - field, - fields, - scalars, - scalar_type_info.scalar_type, - subgraph, - &operator_mapping, - )?; + // lookup ndc operator name in mappings, falling back to using OperatorName + // when an override has not been specified + let ndc_operator_name = operator_mappings + .get(operator) + .unwrap_or_else(|| DataConnectorOperatorName::ref_cast(operator.inner())); + + // lookup the argument type for this comparison operator + let argument_type = + comparable_field.operators.get(operator).ok_or_else(|| { + Error::from(TypePredicateError::OperatorNotFoundForField { + field_name: field.clone(), + operator_name: operator.clone(), + }) + })?; + + Ok((ndc_operator_name.clone(), argument_type.clone())) + } + None => { + // otherwise we need to infer a lot of it from data_connector_scalar_types info + resolve_binary_operator_for_type( + operator, + type_name, + &data_connector_link.name, + field, + fields, + scalars, + scalar_type_info.scalar_type, + subgraph, + ) + } + }?; let value_expression = match value { open_dds::permissions::ValueExpression::Literal(json_value) => { @@ -412,7 +429,7 @@ pub(crate) fn resolve_model_predicate_with_type( field: field.clone(), field_parent_type: type_name.to_owned(), ndc_column: field_mapping.column.clone(), - operator: resolved_operator.clone(), + operator: resolved_operator, argument_type, value: value_expression, }) @@ -593,30 +610,23 @@ pub(crate) fn resolve_model_predicate_with_type( // if a boolean expression type was provided, we can find the // target boolean expression type by following the appropriate // `comparable_relationship` field - let target_boolean_expression_graphql = boolean_expression_graphql - .and_then(|graphql| { - graphql - .relationship_fields - .get(relationship.relationship_name.as_str()) - }) - .and_then(|comparable_relationship| { - match &comparable_relationship.boolean_expression_type { - Some(target_bool_exp_name) => boolean_expression_types - .objects - .get(target_bool_exp_name), - None => {target_model.filter_expression_type.as_ref().and_then(|met| match met { - models_graphql::ModelExpressionType::BooleanExpressionType(bool_exp) => Some(bool_exp), - models_graphql::ModelExpressionType::ObjectBooleanExpressionType(_) => None}) - }} - } - ) - .and_then(|bool_exp| bool_exp.graphql.as_ref()); + let target_boolean_expression_graphql = + match boolean_expression_graphql { + Some(graphql) => lookup_relationship_in_boolean_expression( + graphql, + type_name, + &relationship.relationship_name, + target_model, + boolean_expression_types, + ), + None => Ok(None), + }?; let target_model_predicate = resolve_model_predicate_with_type( nested_predicate, &target_model.inner.data_type, target_object_type, - target_boolean_expression_graphql, + target_boolean_expression_graphql.as_ref(), data_connector_field_mappings, data_connector_link, subgraph, @@ -723,7 +733,54 @@ pub(crate) fn resolve_model_predicate_with_type( } } -#[allow(clippy::too_many_arguments)] +// if we use a relationship in a predicate, we should be able to find it in our +// `BooleanExpressionType` and use it +fn lookup_relationship_in_boolean_expression( + graphql: &boolean_expressions::BooleanExpressionGraphqlConfig, + type_name: &Qualified, + relationship_name: &RelationshipName, + target_model: &models_graphql::ModelWithGraphql, + boolean_expression_types: &boolean_expressions::BooleanExpressionTypes, +) -> Result, Error> { + // lookup relationship in boolean expression type's + // comparable relationships + let comparable_relationship = graphql + .relationship_fields + .get(relationship_name.as_str()) + .ok_or_else(|| { + Error::from(TypePredicateError::UnknownRelationshipInTypePredicate { + type_name: type_name.clone(), + relationship_name: relationship_name.clone(), + }) + })?; + // lookup the boolean expression type for this comparable + // relationship + // if it is defined, we fetch it from metadata + + match &comparable_relationship.boolean_expression_type { + Some(target_bool_exp_name) => boolean_expression_types + .objects + .get(target_bool_exp_name) + .map(|bool_exp| bool_exp.graphql.clone()) + .ok_or_else(|| { + Error::from(TypePredicateError::BooleanExpressionNotFound { + boolean_expression_name: target_bool_exp_name.clone(), + }) + }), + None => { + // if it is not defined we fall back to the one defined on the model + match &target_model.filter_expression_type { + Some(models_graphql::ModelExpressionType::BooleanExpressionType(bool_exp)) => { + Ok(bool_exp.graphql.clone()) + } + _ => Ok(None), + } + } + } +} + +// this is only used for the older `ObjectBooleanExpressionType` where we +// don't have this information explicitly provided in metadata fn resolve_binary_operator_for_type<'a>( operator: &'a OperatorName, type_name: &'a Qualified, @@ -733,8 +790,7 @@ fn resolve_binary_operator_for_type<'a>( scalars: &'a data_connector_scalar_types::ScalarTypeWithRepresentationInfoMap, ndc_scalar_type: &'a ndc_models::ScalarType, subgraph: &'a str, - operator_mappings: &'a BTreeMap, -) -> Result<(&'a DataConnectorOperatorName, QualifiedTypeReference), Error> { +) -> Result<(DataConnectorOperatorName, QualifiedTypeReference), Error> { let field_definition = fields .get(field_name) .ok_or_else(|| Error::TypePredicateError { @@ -744,10 +800,9 @@ fn resolve_binary_operator_for_type<'a>( }, })?; - // lookup ndc operator name in mappings, falling back to using OperatorName - let ndc_operator_name = operator_mappings - .get(operator) - .unwrap_or_else(|| DataConnectorOperatorName::ref_cast(operator.inner())); + // this function is only used for `ObjectBooleanExpressionType` where we do not have a concept + // of mapping OpenDD operator names to NDC operator names, so we just cast it and yolo + let ndc_operator_name = DataConnectorOperatorName::new(operator.inner().clone()); let comparison_operator_definition = &ndc_scalar_type .comparison_operators @@ -774,7 +829,12 @@ fn resolve_binary_operator_for_type<'a>( )), ndc_models::ComparisonOperatorDefinition::Custom { argument_type } => Ok(( ndc_operator_name, - resolve_ndc_type(data_connector, argument_type, scalars, subgraph)?, + object_boolean_expressions::resolve_ndc_type( + data_connector, + argument_type, + scalars, + subgraph, + )?, )), } } diff --git a/v3/crates/metadata-resolve/src/helpers/mod.rs b/v3/crates/metadata-resolve/src/helpers/mod.rs index 1ea5d93e30e..134be28627e 100644 --- a/v3/crates/metadata-resolve/src/helpers/mod.rs +++ b/v3/crates/metadata-resolve/src/helpers/mod.rs @@ -2,7 +2,6 @@ pub mod argument; pub mod boolean_expression; pub mod http; -pub mod model; pub mod ndc_validation; pub mod type_mappings; pub mod typecheck; diff --git a/v3/crates/metadata-resolve/src/stages/boolean_expressions/graphql.rs b/v3/crates/metadata-resolve/src/stages/boolean_expressions/graphql.rs index 01afef0075d..4ec9f432b32 100644 --- a/v3/crates/metadata-resolve/src/stages/boolean_expressions/graphql.rs +++ b/v3/crates/metadata-resolve/src/stages/boolean_expressions/graphql.rs @@ -6,7 +6,6 @@ pub use super::{ }; use crate::helpers::types::mk_name; use crate::stages::{graphql_config, scalar_boolean_expressions}; -use crate::types::subgraph::mk_qualified_type_reference; use crate::Qualified; use lang_graphql::ast::common::{self as ast}; use open_dds::{ @@ -31,7 +30,6 @@ pub(crate) fn resolve_object_boolean_graphql( scalar_boolean_expressions::ResolvedScalarBooleanExpressionType, >, raw_boolean_expression_types: &super::object::RawBooleanExpressionTypes, - subgraph: &str, graphql_config: &graphql_config::GraphqlConfig, ) -> Result { let boolean_expression_graphql_name = @@ -55,14 +53,6 @@ pub(crate) fn resolve_object_boolean_graphql( { // Generate comparison expression for fields mapped to simple scalar type if let Some(graphql_name) = &scalar_boolean_expression_type.graphql_name { - let mut operators = BTreeMap::new(); - for (op_name, op_definition) in &scalar_boolean_expression_type.comparison_operators - { - operators.insert( - op_name.clone(), - mk_qualified_type_reference(op_definition, subgraph), - ); - } let graphql_type_name = mk_name(graphql_name.as_str()).map(ast::TypeName)?; let operator_mapping = resolve_operator_mapping_for_scalar_type( @@ -70,7 +60,9 @@ pub(crate) fn resolve_object_boolean_graphql( ); // Register scalar comparison field only if it contains non-zero operators. - if !operators.is_empty() + if !scalar_boolean_expression_type + .comparison_operators + .is_empty() || scalar_boolean_expression_type.include_is_null == scalar_boolean_expressions::IncludeIsNull::Yes { @@ -79,7 +71,7 @@ pub(crate) fn resolve_object_boolean_graphql( ComparisonExpressionInfo { object_type_name: Some(comparable_field_type_name.clone()), type_name: graphql_type_name.clone(), - operators: operators.clone(), + operators: scalar_boolean_expression_type.comparison_operators.clone(), operator_mapping, is_null_operator_name: match scalar_boolean_expression_type .include_is_null diff --git a/v3/crates/metadata-resolve/src/stages/boolean_expressions/object.rs b/v3/crates/metadata-resolve/src/stages/boolean_expressions/object.rs index 0e860d555fe..b522f51ec3b 100644 --- a/v3/crates/metadata-resolve/src/stages/boolean_expressions/object.rs +++ b/v3/crates/metadata-resolve/src/stages/boolean_expressions/object.rs @@ -81,7 +81,6 @@ pub(crate) fn resolve_object_boolean_expression_type( &comparable_relationships, scalar_boolean_expression_types, raw_boolean_expression_types, - subgraph, graphql_config, ) }) diff --git a/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/error.rs b/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/error.rs index ffe2bdd69c3..21879dd131f 100644 --- a/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/error.rs +++ b/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/error.rs @@ -1,4 +1,4 @@ -use open_dds::types::{CustomTypeName, TypeName}; +use open_dds::types::CustomTypeName; use open_dds::data_connector::{DataConnectorName, DataConnectorScalarType}; @@ -13,12 +13,6 @@ pub enum DataConnectorScalarTypesError { data_connector: Qualified, scalar_type: DataConnectorScalarType, }, - #[error("conflicting type representations found for data connector {data_connector:}: {old_representation:} and {new_representation:}")] - DataConnectorScalarRepresentationMismatch { - data_connector: Qualified, - old_representation: TypeName, - new_representation: TypeName, - }, #[error("unknown type represented for scalar type {scalar_type:}: {type_name:}")] ScalarTypeUnknownRepresentation { scalar_type: DataConnectorScalarType, diff --git a/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/mod.rs b/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/mod.rs index 6e3d4ae546e..85c022b2706 100644 --- a/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/data_connector_scalar_types/mod.rs @@ -26,10 +26,6 @@ pub fn resolve<'a>( metadata_accessor: &'a open_dds::accessor::MetadataAccessor, data_connectors: &'a data_connectors::DataConnectors, scalar_types: &'a BTreeMap, scalar_types::ScalarTypeRepresentation>, - scalar_boolean_expression_types: &BTreeMap< - Qualified, - scalar_boolean_expressions::ResolvedScalarBooleanExpressionType, - >, existing_graphql_types: &'a BTreeSet, ) -> Result, DataConnectorScalarTypesError> { let mut graphql_types = existing_graphql_types.clone(); @@ -112,50 +108,6 @@ pub fn resolve<'a>( }; } - for scalar_boolean_expression in scalar_boolean_expression_types.values() { - for (data_connector_name, operator_mapping) in - &scalar_boolean_expression.data_connector_operator_mappings - { - let scalar_type_name = &operator_mapping.data_connector_scalar_type; - - let scalars = data_connector_scalars - .get_mut(data_connector_name) - .ok_or_else(|| scalar_boolean_expressions::ScalarBooleanExpressionTypeError::ScalarTypeFromUnknownDataConnector { - scalar_type: scalar_type_name.clone(), - data_connector: data_connector_name.clone(), - })?; - - let scalar_type = scalars.0.get_mut(scalar_type_name).ok_or_else(|| { - scalar_boolean_expressions::ScalarBooleanExpressionTypeError::UnknownScalarTypeInDataConnector { - scalar_type: scalar_type_name.clone(), - data_connector: data_connector_name.clone(), - } - })?; - - validate_type_name( - &scalar_boolean_expression.representation, - &scalar_boolean_expression.name.subgraph, - scalar_types, - scalar_type_name, - )?; - - // we may have multiple `BooleanExpressionType` for the same type, - // we allow it but check their OpenDD types don't conflict - if let Some(existing_representation) = &scalar_type.representation { - if *existing_representation != scalar_boolean_expression.representation { - return Err( - DataConnectorScalarTypesError::DataConnectorScalarRepresentationMismatch { - data_connector: data_connector_name.clone(), - old_representation: existing_representation.clone(), - new_representation: scalar_boolean_expression.representation.clone(), - }, - ); - } - } - scalar_type.representation = Some(scalar_boolean_expression.representation.clone()); - } - } - Ok(DataConnectorWithScalarsOutput { data_connector_scalars, graphql_types, diff --git a/v3/crates/metadata-resolve/src/stages/mod.rs b/v3/crates/metadata-resolve/src/stages/mod.rs index 7d382619bd0..28d3a5a5c08 100644 --- a/v3/crates/metadata-resolve/src/stages/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/mod.rs @@ -62,7 +62,13 @@ pub fn resolve( let scalar_boolean_expressions::ScalarBooleanExpressionsOutput { graphql_types, boolean_expression_scalar_types, - } = scalar_boolean_expressions::resolve(&metadata_accessor, &graphql_types, &data_connectors)?; + } = scalar_boolean_expressions::resolve( + &metadata_accessor, + &graphql_types, + &data_connectors, + &object_types, + &scalar_types, + )?; // Validate `DataConnectorScalarType` metadata. This will soon be deprecated and subsumed by // `BooleanExpressionType` @@ -74,7 +80,6 @@ pub fn resolve( &metadata_accessor, &data_connectors, &scalar_types, - &boolean_expression_scalar_types, &graphql_types, )?; diff --git a/v3/crates/metadata-resolve/src/helpers/model.rs b/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/helpers.rs similarity index 95% rename from v3/crates/metadata-resolve/src/helpers/model.rs rename to v3/crates/metadata-resolve/src/stages/object_boolean_expressions/helpers.rs index 77560c1a23d..d3de69b6dbe 100644 --- a/v3/crates/metadata-resolve/src/helpers/model.rs +++ b/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/helpers.rs @@ -9,7 +9,8 @@ use ndc_models; use open_dds::data_connector::{DataConnectorName, DataConnectorScalarType}; // helper function to resolve ndc types to dds type based on scalar type representations -pub(crate) fn resolve_ndc_type( +// this should only be used when we know the underlying type must be a scalar and not an object +pub fn resolve_ndc_type( data_connector: &Qualified, source_type: &ndc_models::Type, scalars: &data_connector_scalar_types::ScalarTypeWithRepresentationInfoMap, diff --git a/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/mod.rs b/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/mod.rs index 736e9e502a0..753822fffd3 100644 --- a/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/object_boolean_expressions/mod.rs @@ -1,10 +1,11 @@ +mod helpers; pub mod types; use crate::stages::{ boolean_expressions, data_connector_scalar_types, data_connectors, graphql_config, object_types, type_permissions, }; +pub use helpers::resolve_ndc_type; -use crate::helpers::model::resolve_ndc_type; use crate::helpers::types::{mk_name, store_new_graphql_type}; use crate::types::subgraph::Qualified; diff --git a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/error.rs b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/error.rs index f9fc930e579..fba66afdc60 100644 --- a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/error.rs +++ b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/error.rs @@ -1,5 +1,8 @@ use crate::Qualified; -use open_dds::data_connector::{DataConnectorName, DataConnectorScalarType}; +use open_dds::{ + data_connector::{DataConnectorName, DataConnectorScalarType}, + types::{CustomTypeName, OperatorName}, +}; #[derive(Debug, thiserror::Error)] pub enum ScalarBooleanExpressionTypeError { @@ -13,6 +16,12 @@ pub enum ScalarBooleanExpressionTypeError { data_connector: Qualified, scalar_type: DataConnectorScalarType, }, + #[error("cannot find type {custom_type:} when resolving arguments for comparison operator {operator_name:} for {boolean_expression_type:}")] + UnknownCustomTypeInComparisonOperatorArgument { + custom_type: Qualified, + operator_name: OperatorName, + boolean_expression_type: Qualified, + }, #[error( "scalar type representation required for type {scalar_type:} in data connector {data_connector:}" )] diff --git a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/mod.rs b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/mod.rs index 019ded3c0d0..5ff6192f68d 100644 --- a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/mod.rs +++ b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/mod.rs @@ -6,9 +6,9 @@ pub use error::ScalarBooleanExpressionTypeError; use std::collections::{BTreeMap, BTreeSet}; use lang_graphql::ast::common as ast; -use open_dds::boolean_expression::BooleanExpressionOperand; +use open_dds::{boolean_expression::BooleanExpressionOperand, types::CustomTypeName}; -use crate::stages::data_connectors; +use crate::stages::{data_connectors, object_types, scalar_types}; use crate::Qualified; pub use types::{ @@ -19,6 +19,8 @@ pub fn resolve( metadata_accessor: &open_dds::accessor::MetadataAccessor, existing_graphql_types: &BTreeSet, data_connectors: &data_connectors::DataConnectors, + object_types: &object_types::ObjectTypesWithTypeMappings, + scalar_types: &BTreeMap, scalar_types::ScalarTypeRepresentation>, ) -> Result { let mut raw_boolean_expression_types = BTreeMap::new(); @@ -54,6 +56,8 @@ pub fn resolve( &boolean_expression_type.is_null, subgraph, data_connectors, + object_types, + scalar_types, &boolean_expression_type.graphql, )?; diff --git a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/scalar.rs b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/scalar.rs index 0118fde5674..9c5eab30298 100644 --- a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/scalar.rs +++ b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/scalar.rs @@ -1,7 +1,9 @@ use super::error::ScalarBooleanExpressionTypeError; use super::types::{IncludeIsNull, ResolvedScalarBooleanExpressionType}; -use crate::stages::data_connectors; -use crate::Qualified; +use crate::helpers::types::unwrap_qualified_type_name; +use crate::stages::{data_connectors, object_types, scalar_types}; +use crate::types::subgraph::mk_qualified_type_reference; +use crate::{Qualified, QualifiedTypeName}; use open_dds::{ boolean_expression::{ BooleanExpressionIsNull, BooleanExpressionScalarOperand, @@ -18,6 +20,8 @@ pub(crate) fn resolve_scalar_boolean_expression_type( is_null: &BooleanExpressionIsNull, subgraph: &str, data_connectors: &data_connectors::DataConnectors, + object_types: &object_types::ObjectTypesWithTypeMappings, + scalar_types: &BTreeMap, scalar_types::ScalarTypeRepresentation>, graphql: &Option, ) -> Result { let mut data_connector_operator_mappings = BTreeMap::new(); @@ -70,10 +74,28 @@ pub(crate) fn resolve_scalar_boolean_expression_type( let mut resolved_comparison_operators = BTreeMap::new(); for comparison_operator in &scalar_boolean_expression_operand.comparison_operators { - resolved_comparison_operators.insert( - comparison_operator.name.clone(), - comparison_operator.argument_type.clone(), - ); + let qualified_argument_type = + mk_qualified_type_reference(&comparison_operator.argument_type, subgraph); + // if our argument type is a Custom named type, check we know about it + match unwrap_qualified_type_name(&qualified_argument_type) { + QualifiedTypeName::Inbuilt(_) => Ok(()), + QualifiedTypeName::Custom(custom_type_name) => { + if object_types.contains_key(custom_type_name) + || scalar_types.contains_key(custom_type_name) + { + Ok(()) + } else { + Err(ScalarBooleanExpressionTypeError + ::UnknownCustomTypeInComparisonOperatorArgument { + custom_type: custom_type_name.clone(), + operator_name: comparison_operator.name.clone(), + boolean_expression_type: boolean_expression_type_name.clone(), + }) + } + } + }?; + resolved_comparison_operators + .insert(comparison_operator.name.clone(), qualified_argument_type); } let graphql_name = graphql.as_ref().map(|gql| gql.type_name.clone()); diff --git a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/types.rs b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/types.rs index 6888463c361..f4457f804e3 100644 --- a/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/types.rs +++ b/v3/crates/metadata-resolve/src/stages/scalar_boolean_expressions/types.rs @@ -1,5 +1,5 @@ -use crate::types::subgraph::Qualified; -use open_dds::types::{CustomTypeName, GraphQlTypeName, OperatorName, TypeName, TypeReference}; +use crate::types::subgraph::{Qualified, QualifiedTypeReference}; +use open_dds::types::{CustomTypeName, GraphQlTypeName, OperatorName, TypeName}; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, BTreeSet}; @@ -20,7 +20,7 @@ pub struct ResolvedScalarBooleanExpressionType { pub representation: TypeName, /// The list of comparison operators that can used on this scalar type - pub comparison_operators: BTreeMap, + pub comparison_operators: BTreeMap, /// The list of mappings between OpenDD operator names and the names used in the data /// connector schema diff --git a/v3/crates/metadata-resolve/src/types/error.rs b/v3/crates/metadata-resolve/src/types/error.rs index b369e896552..161d092adb2 100644 --- a/v3/crates/metadata-resolve/src/types/error.rs +++ b/v3/crates/metadata-resolve/src/types/error.rs @@ -586,6 +586,10 @@ pub enum TypePredicateError { field_name: FieldName, type_name: Qualified, }, + #[error("boolean expression '{boolean_expression_name:}' not found")] + BooleanExpressionNotFound { + boolean_expression_name: Qualified, + }, #[error( "the source data connector {data_connector:} for type {type_name:} has not been defined" )] @@ -607,7 +611,7 @@ pub enum TypePredicateError { NestedPredicateInTypePredicate { type_name: Qualified, }, - #[error("relationship '{relationship_name:}' used in predicate for type '{type_name:}' does not exist")] + #[error("relationship '{relationship_name:}' is used in predicate but does not exist in comparableRelationships in boolean expression for type '{type_name:}'")] UnknownRelationshipInTypePredicate { relationship_name: RelationshipName, type_name: Qualified, @@ -658,6 +662,11 @@ pub enum TypePredicateError { field_name: FieldName, data_connector_name: Qualified, }, + #[error("Operator {operator_name:} not found for field {field_name:}")] + OperatorNotFoundForField { + field_name: FieldName, + operator_name: OperatorName, + }, #[error( "missing equality operator for source column {ndc_column:} in data connector {data_connector_name:} \ which is mapped to field {field_name:} in type {type_name:}" diff --git a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/basic/resolved.snap b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/basic/resolved.snap index 4059dc104ec..c50a8580091 100644 --- a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/basic/resolved.snap +++ b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/basic/resolved.snap @@ -499,9 +499,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/basic/ comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -514,7 +514,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/basic/ }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -578,9 +578,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/basic/ comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( String, @@ -593,7 +593,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/basic/ }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( String, diff --git a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/no_graphql/resolved.snap b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/no_graphql/resolved.snap index 0cd88bb3e90..86ce8efa223 100644 --- a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/no_graphql/resolved.snap +++ b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/no_graphql/resolved.snap @@ -266,9 +266,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/no_gra comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -281,7 +281,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/no_gra }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -341,9 +341,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/no_gra comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( String, @@ -356,7 +356,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/no_gra }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( String, diff --git a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/metadata.json b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/metadata.json index 0c350f594f0..2990ee1ffde 100644 --- a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/metadata.json +++ b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/metadata.json @@ -60,7 +60,7 @@ "comparisonOperators": [ { "name": "range", - "argumentType": "range" + "argumentType": "int_range" } ], "dataConnectorOperatorMapping": [ @@ -79,7 +79,36 @@ } } }, - + { + "kind": "ObjectType", + "version": "v1", + "definition": { + "name": "int_range", + "fields": [ + { + "name": "start", + "type": "Int!" + }, + { + "name": "end", + "type": "Int!" + } + ], + "graphql": { + "typeName": "IntRange" + }, + "dataConnectorTypeMapping": [ + { + "dataConnectorName": "elastic", + "dataConnectorObjectType": "range", + "fieldMapping": { + "start": { "column": { "name": "gte" } }, + "end": { "column": { "name": "lte" } } + } + } + ] + } + }, { "kind": "BooleanExpressionType", "version": "v1", diff --git a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/resolved.snap b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/resolved.snap index f99454af110..a8ed3235b3b 100644 --- a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/resolved.snap +++ b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/range/resolved.snap @@ -6,6 +6,127 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/range/ ( Metadata { object_types: { + Qualified { + subgraph: "__unknown_namespace", + name: CustomTypeName( + Identifier( + "int_range", + ), + ), + }: ObjectTypeWithRelationships { + object_type: ObjectTypeRepresentation { + fields: { + FieldName( + Identifier( + "start", + ), + ): FieldDefinition { + field_type: QualifiedTypeReference { + underlying_type: Named( + Inbuilt( + Int, + ), + ), + nullable: false, + }, + description: None, + deprecated: None, + field_arguments: {}, + }, + FieldName( + Identifier( + "end", + ), + ): FieldDefinition { + field_type: QualifiedTypeReference { + underlying_type: Named( + Inbuilt( + Int, + ), + ), + nullable: false, + }, + description: None, + deprecated: None, + field_arguments: {}, + }, + }, + global_id_fields: [], + apollo_federation_config: None, + graphql_output_type_name: Some( + TypeName( + Name( + "IntRange", + ), + ), + ), + graphql_input_type_name: None, + description: None, + }, + type_output_permissions: {}, + type_input_permissions: {}, + relationship_fields: {}, + type_mappings: DataConnectorTypeMappingsForObject( + { + Qualified { + subgraph: "__unknown_namespace", + name: DataConnectorName( + Identifier( + "elastic", + ), + ), + }: { + DataConnectorObjectType( + "range", + ): Object { + ndc_object_type_name: DataConnectorObjectType( + "range", + ), + field_mappings: { + FieldName( + Identifier( + "end", + ), + ): FieldMapping { + column: DataConnectorColumnName( + "lte", + ), + column_type: Named { + name: TypeName( + "double", + ), + }, + column_type_representation: Some( + Number, + ), + equal_operators: [], + argument_mappings: {}, + }, + FieldName( + Identifier( + "start", + ), + ): FieldMapping { + column: DataConnectorColumnName( + "gte", + ), + column_type: Named { + name: TypeName( + "double", + ), + }, + column_type_representation: Some( + Number, + ), + equal_operators: [], + argument_mappings: {}, + }, + }, + }, + }, + }, + ), + }, Qualified { subgraph: "__unknown_namespace", name: CustomTypeName( @@ -567,7 +688,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/range/ subgraph: "__unknown_namespace", name: CustomTypeName( Identifier( - "range", + "int_range", ), ), }, @@ -787,7 +908,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/range/ subgraph: "__unknown_namespace", name: CustomTypeName( Identifier( - "range", + "int_range", ), ), }, @@ -940,14 +1061,17 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/range/ comparison_operators: { OperatorName( "range", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Custom( - CustomTypeName( - Identifier( - "range", + Qualified { + subgraph: "__unknown_namespace", + name: CustomTypeName( + Identifier( + "int_range", + ), ), - ), + }, ), ), nullable: true, diff --git a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/two_data_sources/resolved.snap b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/two_data_sources/resolved.snap index 00c808207b6..a362a7f323d 100644 --- a/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/two_data_sources/resolved.snap +++ b/v3/crates/metadata-resolve/tests/passing/boolean_expression_type/two_data_sources/resolved.snap @@ -266,9 +266,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/two_da comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -281,7 +281,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/two_da }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( Int, @@ -365,9 +365,9 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/two_da comparison_operators: { OperatorName( "_in", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: List( - TypeReference { + QualifiedTypeReference { underlying_type: Named( Inbuilt( String, @@ -380,7 +380,7 @@ input_file: crates/metadata-resolve/tests/passing/boolean_expression_type/two_da }, OperatorName( "equals", - ): TypeReference { + ): QualifiedTypeReference { underlying_type: Named( Inbuilt( String,