From b314d89ff00e492733b8eb300137f745fe561d0d Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Wed, 5 Jun 2024 15:45:17 +0100 Subject: [PATCH] Use newtypes for operator names (#672) ## Description We were using `String` for these, and making some assumptions about NDCs and OpenDD operator names being the same. This uses newtypes and documents where these assumptions are being made. Functional no-op. V3_GIT_ORIGIN_REV_ID: 195e55db6fadf48c956684130e23f647eaa17fb1 --- v3/crates/execute/src/ir/filter.rs | 8 +++++--- v3/crates/execute/src/ir/permissions.rs | 9 ++++++--- .../metadata-resolve/src/helpers/argument.rs | 20 +++++++++++-------- .../src/stages/model_permissions/types.rs | 4 ++-- v3/crates/schema/src/model_filter.rs | 8 ++++++-- v3/crates/schema/src/types.rs | 4 ++-- 6 files changed, 33 insertions(+), 20 deletions(-) diff --git a/v3/crates/execute/src/ir/filter.rs b/v3/crates/execute/src/ir/filter.rs index 1cc7342ef35..a133f2c2e1e 100644 --- a/v3/crates/execute/src/ir/filter.rs +++ b/v3/crates/execute/src/ir/filter.rs @@ -10,7 +10,7 @@ use serde::Serialize; use crate::ir::error; use crate::model_tracking::{count_model, UsagesCounts}; use open_dds::{ - data_connector::DataConnectorColumnName, + data_connector::{DataConnectorColumnName, DataConnectorOperatorName}, types::{CustomTypeName, FieldName}, }; use schema::FilterRelationshipAnnotation; @@ -133,7 +133,9 @@ fn build_filter_expression<'s>( )) => { let FieldMapping { column, .. } = get_field_mapping_of_field_name(type_mappings, object_type, field_name)?; + let mut expressions = Vec::new(); + for (_op_name, op_value) in field.value.as_object()? { match op_value.info.generic { schema::Annotation::Input(InputAnnotation::Model( @@ -271,7 +273,7 @@ fn resolve_filter_object<'s>( /// Generate a binary comparison operator fn build_binary_comparison_expression( - operator: &str, + operator: &DataConnectorOperatorName, column: DataConnectorColumnName, value: &normalized_ast::Value<'_, GDS>, ) -> ndc_models::Expression { @@ -281,7 +283,7 @@ fn build_binary_comparison_expression( path: Vec::new(), field_path: None, }, - operator: operator.to_string(), + operator: operator.0.clone(), value: ndc_models::ComparisonValue::Scalar { value: value.as_json(), }, diff --git a/v3/crates/execute/src/ir/permissions.rs b/v3/crates/execute/src/ir/permissions.rs index d50dcdac749..c4aa1abcb7a 100644 --- a/v3/crates/execute/src/ir/permissions.rs +++ b/v3/crates/execute/src/ir/permissions.rs @@ -4,7 +4,10 @@ use hasura_authn_core::{SessionVariableValue, SessionVariables}; use lang_graphql::normalized_ast; use ndc_models; -use open_dds::{data_connector::DataConnectorColumnName, types::InbuiltType}; +use open_dds::{ + data_connector::{DataConnectorColumnName, DataConnectorOperatorName}, + types::InbuiltType, +}; use crate::ir::error; use crate::model_tracking::{count_model, UsagesCounts}; @@ -167,7 +170,7 @@ pub(crate) fn process_model_predicate<'s>( fn make_permission_binary_boolean_expression( ndc_column: DataConnectorColumnName, argument_type: &QualifiedTypeReference, - operator: &str, + operator: &DataConnectorOperatorName, value_expression: &metadata_resolve::ValueExpression, session_variables: &SessionVariables, usage_counts: &mut UsagesCounts, @@ -184,7 +187,7 @@ fn make_permission_binary_boolean_expression( path: Vec::new(), field_path: None, }, - operator: operator.to_owned(), + operator: operator.0.clone(), value: ndc_models::ComparisonValue::Scalar { value: ndc_expression_value, }, diff --git a/v3/crates/metadata-resolve/src/helpers/argument.rs b/v3/crates/metadata-resolve/src/helpers/argument.rs index b456a1db387..b63f6cae9f4 100644 --- a/v3/crates/metadata-resolve/src/helpers/argument.rs +++ b/v3/crates/metadata-resolve/src/helpers/argument.rs @@ -18,8 +18,9 @@ use crate::types::subgraph::{ArgumentInfo, Qualified, QualifiedBaseType, Qualifi use indexmap::IndexMap; use ndc_models; use open_dds::arguments::ArgumentName; -use open_dds::data_connector::DataConnectorName; -use open_dds::data_connector::DataConnectorObjectType; +use open_dds::data_connector::{ + DataConnectorName, DataConnectorObjectType, DataConnectorOperatorName, +}; use open_dds::models::ModelName; use open_dds::permissions; use open_dds::types::{CustomTypeName, FieldName, OperatorName}; @@ -672,7 +673,7 @@ fn resolve_binary_operator_for_type( scalars: &data_connector_scalar_types::ScalarTypeWithRepresentationInfoMap, ndc_scalar_type: &ndc_models::ScalarType, subgraph: &str, -) -> Result<(String, QualifiedTypeReference), Error> { +) -> Result<(DataConnectorOperatorName, QualifiedTypeReference), Error> { let field_definition = fields .get(field_name) .ok_or_else(|| Error::TypePredicateError { @@ -681,6 +682,7 @@ fn resolve_binary_operator_for_type( type_name: type_name.clone(), }, })?; + let comparison_operator_definition = &ndc_scalar_type .comparison_operators .get(&operator.0) @@ -690,12 +692,14 @@ fn resolve_binary_operator_for_type( operator_name: operator.clone(), }, })?; + match comparison_operator_definition { - ndc_models::ComparisonOperatorDefinition::Equal => { - Ok((operator.0.clone(), field_definition.field_type.clone())) - } + ndc_models::ComparisonOperatorDefinition::Equal => Ok(( + DataConnectorOperatorName(operator.to_string()), + field_definition.field_type.clone(), + )), ndc_models::ComparisonOperatorDefinition::In => Ok(( - operator.0.clone(), + DataConnectorOperatorName(operator.to_string()), QualifiedTypeReference { underlying_type: QualifiedBaseType::List(Box::new( field_definition.field_type.clone(), @@ -704,7 +708,7 @@ fn resolve_binary_operator_for_type( }, )), ndc_models::ComparisonOperatorDefinition::Custom { argument_type } => Ok(( - operator.0.clone(), + DataConnectorOperatorName(operator.to_string()), resolve_ndc_type(data_connector, argument_type, scalars, subgraph)?, )), } diff --git a/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs b/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs index 44b8d3bf89a..c2656575df0 100644 --- a/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs +++ b/v3/crates/metadata-resolve/src/stages/model_permissions/types.rs @@ -3,7 +3,7 @@ use crate::types::error::{Error, RelationshipError}; use crate::types::permission::ValueExpression; use crate::types::subgraph::{deserialize_qualified_btreemap, serialize_qualified_btreemap}; use open_dds::{ - data_connector::DataConnectorColumnName, + data_connector::{DataConnectorColumnName, DataConnectorOperatorName}, models::ModelName, relationships::{RelationshipName, RelationshipType}, types::CustomTypeName, @@ -47,7 +47,7 @@ pub enum ModelPredicate { BinaryFieldComparison { field: FieldName, ndc_column: DataConnectorColumnName, - operator: String, + operator: DataConnectorOperatorName, argument_type: QualifiedTypeReference, value: ValueExpression, }, diff --git a/v3/crates/schema/src/model_filter.rs b/v3/crates/schema/src/model_filter.rs index aad24a7c031..1fcc1ff4b47 100644 --- a/v3/crates/schema/src/model_filter.rs +++ b/v3/crates/schema/src/model_filter.rs @@ -1,6 +1,6 @@ use lang_graphql::ast::common as ast; use lang_graphql::schema::{self as gql_schema, InputField, Namespaced}; -use open_dds::types::CustomTypeName; +use open_dds::{data_connector::DataConnectorOperatorName, types::CustomTypeName}; use std::collections::BTreeMap; use super::types::input_type; @@ -79,6 +79,10 @@ pub fn build_scalar_comparison_input( nullable: true, }; + // this feels a bit loose, we're depending on the fact the ast::Name and + // DataConnectorOperatorName should be the same + let ndc_operator_name = DataConnectorOperatorName(op_name.to_string()); + input_fields.insert( op_name.clone(), builder.allow_all_namespaced( @@ -87,7 +91,7 @@ pub fn build_scalar_comparison_input( None, types::Annotation::Input(types::InputAnnotation::Model( types::ModelInputAnnotation::ComparisonOperation { - operator: op_name.to_string(), + operator: ndc_operator_name, }, )), nullable_input_type, diff --git a/v3/crates/schema/src/types.rs b/v3/crates/schema/src/types.rs index c1a031611d1..aadca16e585 100644 --- a/v3/crates/schema/src/types.rs +++ b/v3/crates/schema/src/types.rs @@ -12,7 +12,7 @@ use std::{ use open_dds::{ arguments::ArgumentName, commands, - data_connector::DataConnectorColumnName, + data_connector::{DataConnectorColumnName, DataConnectorOperatorName}, models, types::{self}, }; @@ -197,7 +197,7 @@ pub enum ModelInputAnnotation { ndc_table_argument: Option, }, ComparisonOperation { - operator: String, + operator: DataConnectorOperatorName, }, IsNullOperation, ModelOrderByExpression,