From 9183e7380fcc0ed6d186e27b8816c53d10aa880e Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Thu, 18 Jul 2024 17:21:21 -0700 Subject: [PATCH 01/20] wip: parse predicate in ndc-query-plan --- Cargo.lock | 1 + crates/ndc-query-plan/Cargo.toml | 1 + crates/ndc-query-plan/src/lib.rs | 8 +- .../src/plan_for_query_request/mod.rs | 11 +- .../plan_for_arguments.rs | 100 ++++++++++++++++ .../query_plan_error.rs | 25 +++- crates/ndc-query-plan/src/query_plan.rs | 111 ++++++++++++------ 7 files changed, 212 insertions(+), 45 deletions(-) create mode 100644 crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs diff --git a/Cargo.lock b/Cargo.lock index 2be24067..46354404 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1837,6 +1837,7 @@ dependencies = [ "anyhow", "derivative", "enum-iterator", + "indent", "indexmap 2.2.6", "itertools", "lazy_static", diff --git a/crates/ndc-query-plan/Cargo.toml b/crates/ndc-query-plan/Cargo.toml index 7088e5ba..33d4b917 100644 --- a/crates/ndc-query-plan/Cargo.toml +++ b/crates/ndc-query-plan/Cargo.toml @@ -5,6 +5,7 @@ edition = "2021" [dependencies] derivative = "2" +indent = "^0.1" indexmap = { workspace = true } itertools = { workspace = true } ndc-models = { workspace = true } diff --git a/crates/ndc-query-plan/src/lib.rs b/crates/ndc-query-plan/src/lib.rs index 1bfb5e3a..ded90c74 100644 --- a/crates/ndc-query-plan/src/lib.rs +++ b/crates/ndc-query-plan/src/lib.rs @@ -10,9 +10,9 @@ pub use plan_for_query_request::{ type_annotated_field::{type_annotated_field, type_annotated_nested_field}, }; pub use query_plan::{ - Aggregate, AggregateFunctionDefinition, ComparisonOperatorDefinition, ComparisonTarget, - ComparisonValue, ConnectorTypes, ExistsInCollection, Expression, Field, NestedArray, - NestedField, NestedObject, OrderBy, OrderByElement, OrderByTarget, Query, QueryPlan, - Relationship, Relationships, Scope, VariableSet, VariableTypes, + Aggregate, AggregateFunctionDefinition, Argument, Arguments, ComparisonOperatorDefinition, + ComparisonTarget, ComparisonValue, ConnectorTypes, ExistsInCollection, Expression, Field, + NestedArray, NestedField, NestedObject, OrderBy, OrderByElement, OrderByTarget, Query, + QueryPlan, Relationship, Relationships, Scope, VariableSet, VariableTypes, }; pub use type_system::{inline_object_types, ObjectType, 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 594cce4e..7c53e056 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 @@ -1,4 +1,5 @@ mod helpers; +mod plan_for_arguments; pub mod query_context; pub mod query_plan_error; mod query_plan_state; @@ -21,6 +22,7 @@ use query_plan_state::QueryPlanInfo; use self::{ helpers::{find_object_field, find_object_field_path, lookup_relationship}, + plan_for_arguments::plan_for_arguments, query_context::QueryContext, query_plan_error::QueryPlanError, query_plan_state::QueryPlanState, @@ -33,6 +35,7 @@ pub fn plan_for_query_request( request: QueryRequest, ) -> Result> { let mut plan_state = QueryPlanState::new(context, &request.collection_relationships); + let collection_info = context.find_collection(&request.collection)?; let collection_object_type = context.find_collection_object_type(&request.collection)?; let mut query = plan_for_query( @@ -43,6 +46,12 @@ pub fn plan_for_query_request( )?; query.scope = Some(Scope::Root); + let arguments = plan_for_arguments( + &mut plan_state, + &collection_info.arguments, + request.arguments, + )?; + let QueryPlanInfo { unrelated_joins, variable_types, @@ -70,7 +79,7 @@ pub fn plan_for_query_request( Ok(QueryPlan { collection: request.collection, - arguments: request.arguments, + arguments, query, variables, variable_types, diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs new file mode 100644 index 00000000..ea65957e --- /dev/null +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs @@ -0,0 +1,100 @@ +use std::collections::BTreeMap; + +use crate::{self as plan, QueryContext, QueryPlanError}; +use itertools::Itertools as _; +use ndc_models as ndc; + +use super::{plan_for_expression, query_plan_state::QueryPlanState}; + +type Result = std::result::Result; + +fn plan_for_argument( + plan_state: &mut QueryPlanState<'_, T>, + parameter_type: &ndc::Type, + argument: ndc::Argument, +) -> Result> { + match argument { + ndc::Argument::Variable { name } => Ok(plan::Argument::Variable { + name, + argument_type: plan_state.context.ndc_to_plan_type(parameter_type)?, + }), + ndc::Argument::Literal { value } => match parameter_type { + ndc::Type::Predicate { object_type_name } => { + let object_type = plan_state.context.find_object_type(object_type_name)?; + let ndc_expression = serde_json::from_value::(value) + .map_err(QueryPlanError::ErrorParsingPredicate)?; + let expression = + plan_for_expression(plan_state, &object_type, &object_type, ndc_expression)?; + Ok(plan::Argument::Predicate { expression }) + } + t => Ok(plan::Argument::Literal { + value, + argument_type: plan_state.context.ndc_to_plan_type(t)?, + }), + }, + } +} + +pub fn plan_for_arguments( + plan_state: &mut QueryPlanState<'_, T>, + parameters: &BTreeMap, + mut arguments: BTreeMap, +) -> Result>> { + validate_no_excess_arguments(parameters, &arguments)?; + + let (arguments, missing): ( + Vec<(ndc::ArgumentName, ndc::Argument, &ndc::ArgumentInfo)>, + Vec, + ) = parameters + .iter() + .map(|(name, parameter_type)| { + if let Some((name, argument)) = arguments.remove_entry(name) { + Ok((name, argument, parameter_type)) + } else { + Err(name.clone()) + } + }) + .partition_result(); + if !missing.is_empty() { + return Err(QueryPlanError::MissingArguments(missing)); + } + + let (resolved, errors): ( + BTreeMap>, + BTreeMap, + ) = arguments + .into_iter() + .map(|(name, argument, argument_info)| { + match plan_for_argument(plan_state, &argument_info.argument_type, argument) { + Ok(argument) => Ok((name, argument)), + Err(err) => Err((name, err)), + } + }) + .partition_result(); + if !errors.is_empty() { + return Err(QueryPlanError::InvalidArguments(errors)); + } + + Ok(resolved) +} + +pub fn validate_no_excess_arguments( + parameters: &BTreeMap, + arguments: &BTreeMap, +) -> Result<()> { + let excess: Vec = arguments + .iter() + .filter_map(|(name, _)| { + let parameter = parameters.get(name); + match parameter { + Some(_) => None, + None => Some(name.clone()), + } + }) + .collect(); + if !excess.is_empty() { + Err(QueryPlanError::ExcessArguments(excess)) + } else { + Ok(()) + } +} 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 d1f42a0c..07aa395a 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 @@ -1,16 +1,31 @@ +use std::collections::BTreeMap; + +use indent::indent_all_by; use ndc_models as ndc; use thiserror::Error; use super::unify_relationship_references::RelationshipUnificationError; -#[derive(Clone, Debug, Error)] +#[derive(Debug, Error)] pub enum QueryPlanError { + #[error("error parsing predicate: {}", .0)] + ErrorParsingPredicate(#[source] serde_json::Error), + #[error("expected an array at path {}", path.join("."))] ExpectedArray { path: Vec }, #[error("expected an object at path {}", path.join("."))] ExpectedObject { path: Vec }, + #[error("unknown arguments: {}", .0.join(", "))] + ExcessArguments(Vec), + + #[error("some arguments are invalid:\n{}", format_errors(.0))] + InvalidArguments(BTreeMap), + + #[error("missing arguments: {}", .0.join(", "))] + MissingArguments(Vec), + #[error("The connector does not yet support {0}")] NotImplemented(&'static str), @@ -85,3 +100,11 @@ fn in_object_type(type_name: Option<&ndc::ObjectTypeName>) -> String { None => "".to_owned(), } } + +fn format_errors(errors: &BTreeMap) -> String { + errors + .iter() + .map(|(name, error)| format!(" {name}:\n{}", indent_all_by(4, error.to_string()))) + .collect::>() + .join("\n") +} diff --git a/crates/ndc-query-plan/src/query_plan.rs b/crates/ndc-query-plan/src/query_plan.rs index f200c754..57c79656 100644 --- a/crates/ndc-query-plan/src/query_plan.rs +++ b/crates/ndc-query-plan/src/query_plan.rs @@ -4,7 +4,7 @@ use derivative::Derivative; use indexmap::IndexMap; use itertools::Either; use ndc_models::{ - Argument, OrderDirection, RelationshipArgument, RelationshipType, UnaryComparisonOperator, + self as ndc, OrderDirection, RelationshipType, UnaryComparisonOperator, }; use crate::{vec_set::VecSet, Type}; @@ -22,9 +22,9 @@ pub trait ConnectorTypes { PartialEq(bound = "T::ScalarType: PartialEq") )] pub struct QueryPlan { - pub collection: ndc_models::CollectionName, + pub collection: ndc::CollectionName, pub query: Query, - pub arguments: BTreeMap, + pub arguments: BTreeMap>, pub variables: Option>, /// Types for values from the `variables` map as inferred by usages in the query request. It is @@ -44,9 +44,10 @@ impl QueryPlan { } } -pub type Relationships = BTreeMap>; -pub type VariableSet = BTreeMap; -pub type VariableTypes = BTreeMap>>>; +pub type Arguments = BTreeMap>; +pub type Relationships = BTreeMap>; +pub type VariableSet = BTreeMap; +pub type VariableTypes = BTreeMap>>>; #[derive(Derivative)] #[derivative( @@ -56,8 +57,8 @@ pub type VariableTypes = BTreeMap { - pub aggregates: Option>>, - pub fields: Option>>, + pub aggregates: Option>>, + pub fields: Option>>, pub limit: Option, pub aggregates_limit: Option, pub offset: Option, @@ -92,21 +93,53 @@ impl Query { } } +#[derive(Derivative)] +#[derivative( + Clone(bound = ""), + Debug(bound = ""), + PartialEq(bound = "T::ScalarType: PartialEq") +)] +pub enum Argument { + /// The argument is provided by reference to a variable + Variable { name: ndc::VariableName, argument_type: Type }, + /// The argument is provided as a literal value + Literal { value: serde_json::Value, argument_type: Type }, + /// The argument was a literal value that has been parsed as an [Expression] + Predicate { expression: Expression }, +} + #[derive(Derivative)] #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub struct Relationship { - pub column_mapping: BTreeMap, + pub column_mapping: BTreeMap, pub relationship_type: RelationshipType, - pub target_collection: ndc_models::CollectionName, - pub arguments: BTreeMap, + pub target_collection: ndc::CollectionName, + pub arguments: BTreeMap>, pub query: Query, } +#[derive(Derivative)] +#[derivative( + Clone(bound = ""), + Debug(bound = ""), + PartialEq(bound = "T::ScalarType: PartialEq") +)] +pub enum RelationshipArgument { + /// The argument is provided by reference to a variable + Variable { name: ndc::VariableName, argument_type: Type }, + /// The argument is provided as a literal value + Literal { value: serde_json::Value, argument_type: Type }, + // The argument is provided based on a column of the source collection + Column { name: ndc::FieldName, argument_type: Type }, + /// The argument was a literal value that has been parsed as an [Expression] + Predicate { expression: Expression }, +} + #[derive(Derivative)] #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub struct UnrelatedJoin { - pub target_collection: ndc_models::CollectionName, - pub arguments: BTreeMap, + pub target_collection: ndc::CollectionName, + pub arguments: BTreeMap, pub query: Query, } @@ -121,13 +154,13 @@ pub enum Scope { pub enum Aggregate { ColumnCount { /// The column to apply the count aggregate function to - column: ndc_models::FieldName, + column: ndc::FieldName, /// Whether or not only distinct items should be counted distinct: bool, }, SingleColumn { /// The column to apply the aggregation function to - column: ndc_models::FieldName, + column: ndc::FieldName, /// Single column aggregate function name. function: T::AggregateFunction, result_type: Type, @@ -138,7 +171,7 @@ pub enum Aggregate { #[derive(Derivative)] #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub struct NestedObject { - pub fields: IndexMap>, + pub fields: IndexMap>, } #[derive(Derivative)] @@ -158,7 +191,7 @@ pub enum NestedField { #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub enum Field { Column { - column: ndc_models::FieldName, + column: ndc::FieldName, /// When the type of the column is a (possibly-nullable) array or object, /// the caller can request a subset of the complete column data, @@ -172,9 +205,9 @@ pub enum Field { /// The name of the relationship to follow for the subquery - this is the key in the /// [Query] relationships map in this module, it is **not** the key in the /// [ndc::QueryRequest] collection_relationships map. - relationship: ndc_models::RelationshipName, - aggregates: Option>>, - fields: Option>>, + relationship: ndc::RelationshipName, + aggregates: Option>>, + fields: Option>>, }, } @@ -274,34 +307,34 @@ pub struct OrderByElement { pub enum OrderByTarget { Column { /// The name of the column - name: ndc_models::FieldName, + name: ndc::FieldName, /// Path to a nested field within an object column - field_path: Option>, + field_path: Option>, /// Any relationships to traverse to reach this column. These are translated from - /// [ndc_models::OrderByElement] values in the [ndc_models::QueryRequest] to names of relation + /// [ndc::OrderByElement] values in the [ndc::QueryRequest] to names of relation /// fields for the [QueryPlan]. - path: Vec, + path: Vec, }, SingleColumnAggregate { /// The column to apply the aggregation function to - column: ndc_models::FieldName, + column: ndc::FieldName, /// Single column aggregate function name. function: T::AggregateFunction, result_type: Type, /// Any relationships to traverse to reach this aggregate. These are translated from - /// [ndc_models::OrderByElement] values in the [ndc_models::QueryRequest] to names of relation + /// [ndc::OrderByElement] values in the [ndc::QueryRequest] to names of relation /// fields for the [QueryPlan]. - path: Vec, + path: Vec, }, StarCountAggregate { /// Any relationships to traverse to reach this aggregate. These are translated from - /// [ndc_models::OrderByElement] values in the [ndc_models::QueryRequest] to names of relation + /// [ndc::OrderByElement] values in the [ndc::QueryRequest] to names of relation /// fields for the [QueryPlan]. - path: Vec, + path: Vec, }, } @@ -310,42 +343,42 @@ pub enum OrderByTarget { pub enum ComparisonTarget { Column { /// The name of the column - name: ndc_models::FieldName, + name: ndc::FieldName, /// Path to a nested field within an object column - field_path: Option>, + field_path: Option>, 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 + /// [ndc::PathElement] values in the [ndc::QueryRequest] to names of relation /// fields for the [QueryPlan]. - path: Vec, + path: Vec, }, ColumnInScope { /// The name of the column - name: ndc_models::FieldName, + name: ndc::FieldName, /// The named scope that identifies the collection to reference. This corresponds to the /// `scope` field of the [Query] type. scope: Scope, /// Path to a nested field within an object column - field_path: Option>, + field_path: Option>, field_type: Type, }, } impl ComparisonTarget { - pub fn column_name(&self) -> &ndc_models::FieldName { + pub fn column_name(&self) -> &ndc::FieldName { match self { ComparisonTarget::Column { name, .. } => name, ComparisonTarget::ColumnInScope { name, .. } => name, } } - pub fn relationship_path(&self) -> &[ndc_models::RelationshipName] { + pub fn relationship_path(&self) -> &[ndc::RelationshipName] { match self { ComparisonTarget::Column { path, .. } => path, ComparisonTarget::ColumnInScope { .. } => &[], @@ -373,7 +406,7 @@ pub enum ComparisonValue { value_type: Type, }, Variable { - name: ndc_models::VariableName, + name: ndc::VariableName, variable_type: Type, }, } @@ -402,7 +435,7 @@ pub enum ExistsInCollection { Related { /// Key of the relation in the [Query] joins map. Relationships are scoped to the sub-query /// that defines the relation source. - relationship: ndc_models::RelationshipName, + relationship: ndc::RelationshipName, }, Unrelated { /// Key of the relation in the [QueryPlan] joins map. Unrelated collections are not scoped From 816ba0521caa7b5ad0a14fce2e0eb1a2300d3e4c Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Thu, 18 Jul 2024 18:48:51 -0700 Subject: [PATCH 02/20] extend plan_for_arguments to handle relationship arguments --- .../src/mongo_query_plan/mod.rs | 2 + crates/ndc-query-plan/src/lib.rs | 7 +- .../plan_for_arguments.rs | 72 +++++++++++++++++-- 3 files changed, 69 insertions(+), 12 deletions(-) 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 57f54cdc..450de45e 100644 --- a/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs +++ b/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs @@ -99,6 +99,8 @@ fn scalar_type_name(t: &Type) -> Option<&'static str> { } pub type Aggregate = ndc_query_plan::Aggregate; +pub type Argument = ndc_query_plan::Argument; +pub type Arguments = ndc_query_plan::Arguments; pub type ComparisonTarget = ndc_query_plan::ComparisonTarget; pub type ComparisonValue = ndc_query_plan::ComparisonValue; pub type ExistsInCollection = ndc_query_plan::ExistsInCollection; diff --git a/crates/ndc-query-plan/src/lib.rs b/crates/ndc-query-plan/src/lib.rs index ded90c74..2d04a2b5 100644 --- a/crates/ndc-query-plan/src/lib.rs +++ b/crates/ndc-query-plan/src/lib.rs @@ -9,10 +9,5 @@ pub use plan_for_query_request::{ query_plan_error::QueryPlanError, type_annotated_field::{type_annotated_field, type_annotated_nested_field}, }; -pub use query_plan::{ - Aggregate, AggregateFunctionDefinition, Argument, Arguments, ComparisonOperatorDefinition, - ComparisonTarget, ComparisonValue, ConnectorTypes, ExistsInCollection, Expression, Field, - NestedArray, NestedField, NestedObject, OrderBy, OrderByElement, OrderByTarget, Query, - QueryPlan, Relationship, Relationships, Scope, VariableSet, VariableTypes, -}; +pub use query_plan::*; pub use type_system::{inline_object_types, ObjectType, Type}; diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs index ea65957e..2c41ae5e 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs @@ -8,6 +8,29 @@ use super::{plan_for_expression, query_plan_state::QueryPlanState}; type Result = std::result::Result; +/// Convert maps of [ndc::Argument] values to maps of [plan::Argument] +pub fn plan_for_arguments( + plan_state: &mut QueryPlanState<'_, T>, + parameters: &BTreeMap, + arguments: BTreeMap, +) -> Result>> { + plan_for_arguments_generic(plan_state, parameters, arguments, plan_for_argument) +} + +/// Convert maps of [ndc::Argument] values to maps of [plan::Argument] +pub fn plan_for_relationship_arguments( + plan_state: &mut QueryPlanState<'_, T>, + parameters: &BTreeMap, + arguments: BTreeMap, +) -> Result>> { + plan_for_arguments_generic( + plan_state, + parameters, + arguments, + plan_for_relationship_argument, + ) +} + fn plan_for_argument( plan_state: &mut QueryPlanState<'_, T>, parameter_type: &ndc::Type, @@ -35,15 +58,52 @@ fn plan_for_argument( } } -pub fn plan_for_arguments( +fn plan_for_relationship_argument( + plan_state: &mut QueryPlanState<'_, T>, + parameter_type: &ndc::Type, + argument: ndc::RelationshipArgument, +) -> Result> { + match argument { + ndc::RelationshipArgument::Variable { name } => Ok(plan::RelationshipArgument::Variable { + name, + argument_type: plan_state.context.ndc_to_plan_type(parameter_type)?, + }), + ndc::RelationshipArgument::Column { name } => Ok(plan::RelationshipArgument::Column { + name, + argument_type: plan_state.context.ndc_to_plan_type(parameter_type)?, + }), + ndc::RelationshipArgument::Literal { value } => match parameter_type { + ndc::Type::Predicate { object_type_name } => { + let object_type = plan_state.context.find_object_type(object_type_name)?; + let ndc_expression = serde_json::from_value::(value) + .map_err(QueryPlanError::ErrorParsingPredicate)?; + let expression = + plan_for_expression(plan_state, &object_type, &object_type, ndc_expression)?; + Ok(plan::RelationshipArgument::Predicate { expression }) + } + t => Ok(plan::RelationshipArgument::Literal { + value, + argument_type: plan_state.context.ndc_to_plan_type(t)?, + }), + }, + } +} + +/// Convert maps of [ndc::Argument] or [ndc::RelationshipArgument] values to [plan::Argument] or +/// [plan::RelationshipArgument] respectively. +fn plan_for_arguments_generic( plan_state: &mut QueryPlanState<'_, T>, parameters: &BTreeMap, - mut arguments: BTreeMap, -) -> Result>> { + mut arguments: BTreeMap, + convert_argument: F, +) -> Result> +where + F: Fn(&mut QueryPlanState<'_, T>, &ndc::Type, NdcArgument) -> Result, +{ validate_no_excess_arguments(parameters, &arguments)?; let (arguments, missing): ( - Vec<(ndc::ArgumentName, ndc::Argument, &ndc::ArgumentInfo)>, + Vec<(ndc::ArgumentName, NdcArgument, &ndc::ArgumentInfo)>, Vec, ) = parameters .iter() @@ -60,12 +120,12 @@ pub fn plan_for_arguments( } let (resolved, errors): ( - BTreeMap>, + BTreeMap, BTreeMap, ) = arguments .into_iter() .map(|(name, argument, argument_info)| { - match plan_for_argument(plan_state, &argument_info.argument_type, argument) { + match convert_argument(plan_state, &argument_info.argument_type, argument) { Ok(argument) => Ok((name, argument)), Err(err) => Err((name, err)), } From 7392add8edbda8e91a04e625cdf16db1b2224ca6 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Thu, 18 Jul 2024 19:09:01 -0700 Subject: [PATCH 03/20] convert arguments when registering relationships --- .../src/plan_for_query_request/mod.rs | 2 +- .../plan_for_arguments.rs | 31 ++++++++++-- .../plan_test_helpers/relationships.rs | 8 ++-- .../query_plan_state.rs | 48 +++++++++---------- crates/ndc-query-plan/src/query_plan.rs | 33 +++++++++---- 5 files changed, 80 insertions(+), 42 deletions(-) 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 7c53e056..901f1381 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 @@ -689,7 +689,7 @@ fn plan_for_exists( ..Default::default() }; - let join_key = plan_state.register_unrelated_join(collection, arguments, join_query); + let join_key = plan_state.register_unrelated_join(collection, arguments, join_query)?; let in_collection = plan::ExistsInCollection::Unrelated { unrelated_collection: join_key, diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs index 2c41ae5e..35f7f683 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs @@ -14,7 +14,20 @@ pub fn plan_for_arguments( parameters: &BTreeMap, arguments: BTreeMap, ) -> Result>> { - plan_for_arguments_generic(plan_state, parameters, arguments, plan_for_argument) + let arguments = + plan_for_arguments_generic(plan_state, parameters, arguments, plan_for_argument)?; + + for argument in arguments.values() { + if let plan::Argument::Variable { + name, + argument_type, + } = argument + { + plan_state.register_variable_use(name, argument_type.clone()) + } + } + + Ok(arguments) } /// Convert maps of [ndc::Argument] values to maps of [plan::Argument] @@ -23,12 +36,24 @@ pub fn plan_for_relationship_arguments( parameters: &BTreeMap, arguments: BTreeMap, ) -> Result>> { - plan_for_arguments_generic( + let arguments = plan_for_arguments_generic( plan_state, parameters, arguments, plan_for_relationship_argument, - ) + )?; + + for argument in arguments.values() { + if let plan::RelationshipArgument::Variable { + name, + argument_type, + } = argument + { + plan_state.register_variable_use(name, argument_type.clone()) + } + } + + Ok(arguments) } fn plan_for_argument( diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/relationships.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/relationships.rs index 2da3ff53..0ab7cfbd 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/relationships.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/relationships.rs @@ -1,8 +1,8 @@ use std::collections::BTreeMap; -use ndc_models::{RelationshipArgument, RelationshipType}; +use ndc_models::RelationshipType; -use crate::{ConnectorTypes, Field, Relationship}; +use crate::{ConnectorTypes, Field, Relationship, RelationshipArgument}; use super::QueryBuilder; @@ -11,7 +11,7 @@ pub struct RelationshipBuilder { column_mapping: BTreeMap, relationship_type: RelationshipType, target_collection: ndc_models::CollectionName, - arguments: BTreeMap, + arguments: BTreeMap>, query: QueryBuilder, } @@ -63,7 +63,7 @@ impl RelationshipBuilder { pub fn arguments( mut self, - arguments: BTreeMap, + arguments: BTreeMap>, ) -> Self { self.arguments = arguments; self diff --git a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs index a000fdc9..111411dd 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs @@ -4,7 +4,6 @@ use std::{ rc::Rc, }; -use ndc::RelationshipArgument; use ndc_models as ndc; use crate::{ @@ -14,7 +13,10 @@ use crate::{ ConnectorTypes, Query, QueryContext, QueryPlanError, Relationship, Type, }; -use super::unify_relationship_references::unify_relationship_references; +use super::{ + plan_for_arguments::plan_for_relationship_arguments, + unify_relationship_references::unify_relationship_references, +}; type Result = std::result::Result; @@ -79,18 +81,20 @@ impl QueryPlanState<'_, T> { pub fn register_relationship( &mut self, ndc_relationship_name: ndc::RelationshipName, - arguments: BTreeMap, + arguments: BTreeMap, query: Query, ) -> Result { let ndc_relationship = lookup_relationship(self.collection_relationships, &ndc_relationship_name)?; - for argument in arguments.values() { - if let RelationshipArgument::Variable { name } = argument { - // TODO: Is there a way to infer a type here? - self.register_variable_use_of_unknown_type(name) - } - } + let arguments = if !arguments.is_empty() { + let collection = self + .context + .find_collection(&ndc_relationship.target_collection)?; + plan_for_relationship_arguments(self, &collection.arguments, arguments)? + } else { + Default::default() + }; let relationship = Relationship { column_mapping: ndc_relationship.column_mapping.clone(), @@ -131,9 +135,16 @@ impl QueryPlanState<'_, T> { pub fn register_unrelated_join( &mut self, target_collection: ndc::CollectionName, - arguments: BTreeMap, + arguments: BTreeMap, query: Query, - ) -> String { + ) -> Result { + let arguments = if !arguments.is_empty() { + let collection = self.context.find_collection(&target_collection)?; + plan_for_relationship_arguments(self, &collection.arguments, arguments)? + } else { + Default::default() + }; + let join = UnrelatedJoin { target_collection, arguments, @@ -149,7 +160,7 @@ impl QueryPlanState<'_, T> { // borrow map values through a RefCell without keeping a live Ref.) But if that Ref is // still alive the next time [Self::register_unrelated_join] is called then the borrow_mut // call will fail. - key + Ok(key) } /// It's important to call this for every use of a variable encountered when building @@ -159,18 +170,7 @@ impl QueryPlanState<'_, T> { variable_name: &ndc::VariableName, expected_type: Type, ) { - self.register_variable_use_helper(variable_name, Some(expected_type)) - } - - pub fn register_variable_use_of_unknown_type(&mut self, variable_name: &ndc::VariableName) { - self.register_variable_use_helper(variable_name, None) - } - - fn register_variable_use_helper( - &mut self, - variable_name: &ndc::VariableName, - expected_type: Option>, - ) { + // self.register_variable_use_helper(variable_name, Some(expected_type)) let mut type_map = self.variable_types.borrow_mut(); match type_map.get_mut(variable_name) { None => { diff --git a/crates/ndc-query-plan/src/query_plan.rs b/crates/ndc-query-plan/src/query_plan.rs index 57c79656..378e8e09 100644 --- a/crates/ndc-query-plan/src/query_plan.rs +++ b/crates/ndc-query-plan/src/query_plan.rs @@ -3,9 +3,7 @@ use std::{collections::BTreeMap, fmt::Debug, iter}; use derivative::Derivative; use indexmap::IndexMap; use itertools::Either; -use ndc_models::{ - self as ndc, OrderDirection, RelationshipType, UnaryComparisonOperator, -}; +use ndc_models::{self as ndc, OrderDirection, RelationshipType, UnaryComparisonOperator}; use crate::{vec_set::VecSet, Type}; @@ -47,7 +45,7 @@ impl QueryPlan { pub type Arguments = BTreeMap>; pub type Relationships = BTreeMap>; pub type VariableSet = BTreeMap; -pub type VariableTypes = BTreeMap>>>; +pub type VariableTypes = BTreeMap>>; #[derive(Derivative)] #[derivative( @@ -101,9 +99,15 @@ impl Query { )] pub enum Argument { /// The argument is provided by reference to a variable - Variable { name: ndc::VariableName, argument_type: Type }, + Variable { + name: ndc::VariableName, + argument_type: Type, + }, /// The argument is provided as a literal value - Literal { value: serde_json::Value, argument_type: Type }, + Literal { + value: serde_json::Value, + argument_type: Type, + }, /// The argument was a literal value that has been parsed as an [Expression] Predicate { expression: Expression }, } @@ -126,11 +130,20 @@ pub struct Relationship { )] pub enum RelationshipArgument { /// The argument is provided by reference to a variable - Variable { name: ndc::VariableName, argument_type: Type }, + Variable { + name: ndc::VariableName, + argument_type: Type, + }, /// The argument is provided as a literal value - Literal { value: serde_json::Value, argument_type: Type }, + Literal { + value: serde_json::Value, + argument_type: Type, + }, // The argument is provided based on a column of the source collection - Column { name: ndc::FieldName, argument_type: Type }, + Column { + name: ndc::FieldName, + argument_type: Type, + }, /// The argument was a literal value that has been parsed as an [Expression] Predicate { expression: Expression }, } @@ -139,7 +152,7 @@ pub enum RelationshipArgument { #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub struct UnrelatedJoin { pub target_collection: ndc::CollectionName, - pub arguments: BTreeMap, + pub arguments: BTreeMap>, pub query: Query, } From d3321d527f9aae3d68570a4c444e61819ab78c0b Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Thu, 18 Jul 2024 22:14:31 -0700 Subject: [PATCH 04/20] plan_for_mutation_request --- crates/ndc-query-plan/src/lib.rs | 3 + crates/ndc-query-plan/src/mutation_plan.rs | 54 ++++++++++++++ .../src/plan_for_query_request/mod.rs | 2 + .../plan_for_arguments.rs | 63 ++++++++++++---- .../plan_for_mutation_request.rs | 72 +++++++++++++++++++ .../plan_for_query_request/query_context.rs | 6 ++ .../query_plan_error.rs | 3 + .../unify_relationship_references.rs | 63 +++++++++------- 8 files changed, 226 insertions(+), 40 deletions(-) create mode 100644 crates/ndc-query-plan/src/mutation_plan.rs create mode 100644 crates/ndc-query-plan/src/plan_for_query_request/plan_for_mutation_request.rs diff --git a/crates/ndc-query-plan/src/lib.rs b/crates/ndc-query-plan/src/lib.rs index 2d04a2b5..f7b6b1b5 100644 --- a/crates/ndc-query-plan/src/lib.rs +++ b/crates/ndc-query-plan/src/lib.rs @@ -1,12 +1,15 @@ +mod mutation_plan; mod plan_for_query_request; mod query_plan; mod type_system; pub mod vec_set; +pub use mutation_plan::*; pub use plan_for_query_request::{ plan_for_query_request, query_context::QueryContext, query_plan_error::QueryPlanError, + plan_for_mutation_request, type_annotated_field::{type_annotated_field, type_annotated_nested_field}, }; pub use query_plan::*; diff --git a/crates/ndc-query-plan/src/mutation_plan.rs b/crates/ndc-query-plan/src/mutation_plan.rs new file mode 100644 index 00000000..6e0fb694 --- /dev/null +++ b/crates/ndc-query-plan/src/mutation_plan.rs @@ -0,0 +1,54 @@ +use std::collections::BTreeMap; + +use derivative::Derivative; +use ndc_models as ndc; + +use crate::ConnectorTypes; +use crate::{self as plan, Type}; + +#[derive(Derivative)] +#[derivative( + Clone(bound = ""), + Debug(bound = ""), + PartialEq(bound = "T::ScalarType: PartialEq") +)] +pub struct MutationPlan { + /// The mutation operations to perform + pub operations: Vec>, +} + +#[derive(Derivative)] +#[derivative( + Clone(bound = ""), + Debug(bound = ""), + PartialEq(bound = "T::ScalarType: PartialEq") +)] +pub enum MutationOperation { + Procedure { + /// The name of a procedure + name: ndc::ProcedureName, + /// Any named procedure arguments + arguments: BTreeMap>, + /// The fields to return from the result, or null to return everything + fields: Option>, + /// Relationships referenced by fields and expressions in this query or sub-query. Does not + /// include relationships in sub-queries nested under this one. + relationships: plan::Relationships, + }, +} + +#[derive(Derivative)] +#[derivative( + Clone(bound = ""), + Debug(bound = ""), + PartialEq(bound = "T::ScalarType: PartialEq") +)] +pub enum MutationProcedureArgument { + /// The argument is provided as a literal value + Literal { + value: serde_json::Value, + argument_type: Type, + }, + /// The argument was a literal value that has been parsed as an [Expression] + Predicate { expression: plan::Expression }, +} 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 901f1381..4da4fb04 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 @@ -1,5 +1,6 @@ mod helpers; mod plan_for_arguments; +mod plan_for_mutation_request; pub mod query_context; pub mod query_plan_error; mod query_plan_state; @@ -27,6 +28,7 @@ use self::{ query_plan_error::QueryPlanError, query_plan_state::QueryPlanState, }; +pub use self::plan_for_mutation_request::plan_for_mutation_request; type Result = std::result::Result; diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs index 35f7f683..6f485448 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs @@ -30,6 +30,20 @@ pub fn plan_for_arguments( Ok(arguments) } +/// Convert maps of [serde_json::Value] values to maps of [plan::MutationProcedureArgument] +pub fn plan_for_mutation_procedure_arguments( + plan_state: &mut QueryPlanState<'_, T>, + parameters: &BTreeMap, + arguments: BTreeMap, +) -> Result>> { + plan_for_arguments_generic( + plan_state, + parameters, + arguments, + plan_for_mutation_procedure_argument, + ) +} + /// Convert maps of [ndc::Argument] values to maps of [plan::Argument] pub fn plan_for_relationship_arguments( plan_state: &mut QueryPlanState<'_, T>, @@ -67,14 +81,9 @@ fn plan_for_argument( argument_type: plan_state.context.ndc_to_plan_type(parameter_type)?, }), ndc::Argument::Literal { value } => match parameter_type { - ndc::Type::Predicate { object_type_name } => { - let object_type = plan_state.context.find_object_type(object_type_name)?; - let ndc_expression = serde_json::from_value::(value) - .map_err(QueryPlanError::ErrorParsingPredicate)?; - let expression = - plan_for_expression(plan_state, &object_type, &object_type, ndc_expression)?; - Ok(plan::Argument::Predicate { expression }) - } + ndc::Type::Predicate { object_type_name } => Ok(plan::Argument::Predicate { + expression: plan_for_predicate(plan_state, object_type_name, value)?, + }), t => Ok(plan::Argument::Literal { value, argument_type: plan_state.context.ndc_to_plan_type(t)?, @@ -83,6 +92,24 @@ fn plan_for_argument( } } +fn plan_for_mutation_procedure_argument( + plan_state: &mut QueryPlanState<'_, T>, + parameter_type: &ndc::Type, + value: serde_json::Value, +) -> Result> { + match parameter_type { + ndc::Type::Predicate { object_type_name } => { + Ok(plan::MutationProcedureArgument::Predicate { + expression: plan_for_predicate(plan_state, object_type_name, value)?, + }) + } + t => Ok(plan::MutationProcedureArgument::Literal { + value, + argument_type: plan_state.context.ndc_to_plan_type(t)?, + }), + } +} + fn plan_for_relationship_argument( plan_state: &mut QueryPlanState<'_, T>, parameter_type: &ndc::Type, @@ -99,12 +126,9 @@ fn plan_for_relationship_argument( }), ndc::RelationshipArgument::Literal { value } => match parameter_type { ndc::Type::Predicate { object_type_name } => { - let object_type = plan_state.context.find_object_type(object_type_name)?; - let ndc_expression = serde_json::from_value::(value) - .map_err(QueryPlanError::ErrorParsingPredicate)?; - let expression = - plan_for_expression(plan_state, &object_type, &object_type, ndc_expression)?; - Ok(plan::RelationshipArgument::Predicate { expression }) + Ok(plan::RelationshipArgument::Predicate { + expression: plan_for_predicate(plan_state, object_type_name, value)?, + }) } t => Ok(plan::RelationshipArgument::Literal { value, @@ -114,6 +138,17 @@ fn plan_for_relationship_argument( } } +fn plan_for_predicate( + plan_state: &mut QueryPlanState<'_, T>, + object_type_name: &ndc::ObjectTypeName, + value: serde_json::Value, +) -> Result> { + let object_type = plan_state.context.find_object_type(object_type_name)?; + let ndc_expression = serde_json::from_value::(value) + .map_err(QueryPlanError::ErrorParsingPredicate)?; + plan_for_expression(plan_state, &object_type, &object_type, ndc_expression) +} + /// Convert maps of [ndc::Argument] or [ndc::RelationshipArgument] values to [plan::Argument] or /// [plan::RelationshipArgument] respectively. fn plan_for_arguments_generic( diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_for_mutation_request.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_mutation_request.rs new file mode 100644 index 00000000..d644b4f0 --- /dev/null +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_for_mutation_request.rs @@ -0,0 +1,72 @@ +use std::collections::BTreeMap; + +use itertools::Itertools as _; +use ndc_models::{self as ndc, MutationRequest}; + +use crate::{self as plan, type_annotated_nested_field, MutationPlan}; + +use super::{ + plan_for_arguments::plan_for_mutation_procedure_arguments, query_plan_error::QueryPlanError, + query_plan_state::QueryPlanState, QueryContext, +}; + +type Result = std::result::Result; + +pub fn plan_for_mutation_request( + context: &T, + request: MutationRequest, +) -> Result> { + let operations = request + .operations + .into_iter() + .map(|op| plan_for_mutation_operation(context, &request.collection_relationships, op)) + .try_collect()?; + + Ok(MutationPlan { operations }) +} + +fn plan_for_mutation_operation( + context: &T, + collection_relationships: &BTreeMap, + operation: ndc::MutationOperation, +) -> Result> { + match operation { + ndc::MutationOperation::Procedure { + name, + arguments, + fields, + } => { + let mut plan_state = QueryPlanState::new(context, collection_relationships); + + let procedure_info = context.find_procedure(&name)?; + + let arguments = plan_for_mutation_procedure_arguments( + &mut plan_state, + &procedure_info.arguments, + arguments, + )?; + + let fields = fields + .map(|nested_field| { + let result_type = context.ndc_to_plan_type(&procedure_info.result_type)?; + let plan_nested_field = type_annotated_nested_field( + context, + collection_relationships, + &result_type, + nested_field, + )?; + Ok(plan_nested_field) as Result<_> + }) + .transpose()?; + + let relationships = plan_state.into_relationships(); + + Ok(plan::MutationOperation::Procedure { + name, + arguments, + fields, + relationships, + }) + } + } +} diff --git a/crates/ndc-query-plan/src/plan_for_query_request/query_context.rs b/crates/ndc-query-plan/src/plan_for_query_request/query_context.rs index b290e785..64a947e1 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/query_context.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/query_context.rs @@ -119,6 +119,12 @@ pub trait QueryContext: ConnectorTypes { ) } + fn find_procedure(&self, procedure_name: &ndc::ProcedureName) -> Result<&ndc::ProcedureInfo> { + self.procedures() + .get(procedure_name) + .ok_or_else(|| QueryPlanError::UnknownProcedure(procedure_name.to_string())) + } + fn find_scalar_type(scalar_type_name: &ndc::ScalarTypeName) -> Result { Self::lookup_scalar_type(scalar_type_name) .ok_or_else(|| QueryPlanError::UnknownScalarType(scalar_type_name.clone())) 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 07aa395a..29b05782 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 @@ -61,6 +61,9 @@ pub enum QueryPlanError { #[error("Unknown collection, \"{0}\"")] UnknownCollection(String), + #[error("Unknown procedure, \"{0}\"")] + UnknownProcedure(String), + #[error("Unknown relationship, \"{relationship_name}\"{}", at_path(path))] UnknownRelationship { relationship_name: String, diff --git a/crates/ndc-query-plan/src/plan_for_query_request/unify_relationship_references.rs b/crates/ndc-query-plan/src/plan_for_query_request/unify_relationship_references.rs index e83010a8..1d16e70c 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/unify_relationship_references.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/unify_relationship_references.rs @@ -3,37 +3,37 @@ use std::collections::BTreeMap; use indexmap::IndexMap; use itertools::{merge_join_by, EitherOrBoth, Itertools}; -use ndc_models::RelationshipArgument; +use ndc_models as ndc; use thiserror::Error; use crate::{ Aggregate, ConnectorTypes, Expression, Field, NestedArray, NestedField, NestedObject, Query, - Relationship, Relationships, + Relationship, RelationshipArgument, Relationships, }; -#[derive(Clone, Debug, Error)] +#[derive(Debug, Error)] pub enum RelationshipUnificationError { - #[error("relationship arguments mismatch")] + #[error("relationship arguments mismatch\n left: {:?}\n right: {:?}", .a, .b)] ArgumentsMismatch { - a: BTreeMap, - b: BTreeMap, + a: BTreeMap, + b: BTreeMap, }, #[error("relationships select fields with the same name, {field_name}, but that have different types")] - FieldTypeMismatch { field_name: ndc_models::FieldName }, + FieldTypeMismatch { field_name: ndc::FieldName }, #[error("relationships select columns {column_a} and {column_b} with the same field name, {field_name}")] FieldColumnMismatch { - field_name: ndc_models::FieldName, - column_a: ndc_models::FieldName, - column_b: ndc_models::FieldName, + field_name: ndc::FieldName, + column_a: ndc::FieldName, + column_b: ndc::FieldName, }, #[error("relationship references have incompatible configurations: {}", .0.join(", "))] Mismatch(Vec<&'static str>), #[error("relationship references referenced different nested relationships with the same field name, {field_name}")] - RelationshipMismatch { field_name: ndc_models::FieldName }, + RelationshipMismatch { field_name: ndc::FieldName }, } type Result = std::result::Result; @@ -64,17 +64,28 @@ where // TODO: The engine may be set up to avoid a situation where we encounter a mismatch. For now we're // being pessimistic, and if we get an error here we record the two relationships under separate // keys instead of recording one, unified relationship. -fn unify_arguments( - a: BTreeMap, - b: BTreeMap, -) -> Result> { +fn unify_arguments( + a: BTreeMap>, + b: BTreeMap>, +) -> Result>> { if a != b { - Err(RelationshipUnificationError::ArgumentsMismatch { a, b }) + Err(RelationshipUnificationError::ArgumentsMismatch { + a: debuggable_map(a), + b: debuggable_map(b), + }) } else { Ok(a) } } +fn debuggable_map(xs: impl IntoIterator) -> BTreeMap +where + K: Ord, + V: std::fmt::Debug, +{ + xs.into_iter().map(|(k, v)| (k, format!("{v:?}"))).collect() +} + fn unify_query(a: Query, b: Query) -> Result> where T: ConnectorTypes, @@ -120,9 +131,9 @@ where } fn unify_aggregates( - a: Option>>, - b: Option>>, -) -> Result>>> + a: Option>>, + b: Option>>, +) -> Result>>> where T: ConnectorTypes, { @@ -134,9 +145,9 @@ where } fn unify_fields( - a: Option>>, - b: Option>>, -) -> Result>>> + a: Option>>, + b: Option>>, +) -> Result>>> where T: ConnectorTypes, { @@ -144,9 +155,9 @@ where } fn unify_fields_some( - fields_a: IndexMap>, - fields_b: IndexMap>, -) -> Result>> + fields_a: IndexMap>, + fields_b: IndexMap>, +) -> Result>> where T: ConnectorTypes, { @@ -163,7 +174,7 @@ where Ok(fields) } -fn unify_field(field_name: &ndc_models::FieldName, a: Field, b: Field) -> Result> +fn unify_field(field_name: &ndc::FieldName, a: Field, b: Field) -> Result> where T: ConnectorTypes, { From 0970d4f803b5d0277d6eb15de254ed725f7e5300 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 00:40:51 -0700 Subject: [PATCH 05/20] update connector to use MutationPlan --- .../src/mongo_query_plan/mod.rs | 3 + .../src/procedure/error.rs | 19 ++- .../mongodb-agent-common/src/procedure/mod.rs | 56 ++++++--- .../src/query/arguments.rs | 114 ------------------ .../mongodb-agent-common/src/query/foreach.rs | 13 +- crates/mongodb-agent-common/src/query/mod.rs | 1 - .../src/query/native_query.rs | 50 +++++++- .../src/query/query_target.rs | 3 +- crates/mongodb-connector/src/mutation.rs | 51 ++++---- 9 files changed, 131 insertions(+), 179 deletions(-) delete mode 100644 crates/mongodb-agent-common/src/query/arguments.rs 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 450de45e..4f378667 100644 --- a/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs +++ b/crates/mongodb-agent-common/src/mongo_query_plan/mod.rs @@ -106,6 +106,9 @@ pub type ComparisonValue = ndc_query_plan::ComparisonValue; pub type ExistsInCollection = ndc_query_plan::ExistsInCollection; pub type Expression = ndc_query_plan::Expression; pub type Field = ndc_query_plan::Field; +pub type MutationOperation = ndc_query_plan::MutationOperation; +pub type MutationPlan = ndc_query_plan::MutationPlan; +pub type MutationProcedureArgument = ndc_query_plan::MutationProcedureArgument; pub type NestedField = ndc_query_plan::NestedField; pub type NestedArray = ndc_query_plan::NestedArray; pub type NestedObject = ndc_query_plan::NestedObject; diff --git a/crates/mongodb-agent-common/src/procedure/error.rs b/crates/mongodb-agent-common/src/procedure/error.rs index bff2afab..ef447f66 100644 --- a/crates/mongodb-agent-common/src/procedure/error.rs +++ b/crates/mongodb-agent-common/src/procedure/error.rs @@ -1,10 +1,24 @@ use mongodb::bson::Bson; use thiserror::Error; -use crate::query::arguments::ArgumentError; +use crate::{interface_types::MongoAgentError, query::serialization::JsonToBsonError}; #[derive(Debug, Error)] pub enum ProcedureError { + #[error("error parsing argument \"{}\": {}", .argument_name, .error)] + ErrorParsingArgument { + argument_name: String, + #[source] + error: JsonToBsonError, + }, + + #[error("error parsing predicate argument \"{}\": {}", .argument_name, .error)] + ErrorParsingPredicate { + argument_name: String, + #[source] + error: Box, + }, + #[error("error executing mongodb command: {0}")] ExecutionError(#[from] mongodb::error::Error), @@ -16,7 +30,4 @@ pub enum ProcedureError { #[error("object keys must be strings, but got: \"{0}\"")] NonStringKey(Bson), - - #[error("could not resolve arguments: {0}")] - UnresolvableArguments(#[from] ArgumentError), } diff --git a/crates/mongodb-agent-common/src/procedure/mod.rs b/crates/mongodb-agent-common/src/procedure/mod.rs index 9729b071..f509b329 100644 --- a/crates/mongodb-agent-common/src/procedure/mod.rs +++ b/crates/mongodb-agent-common/src/procedure/mod.rs @@ -5,12 +5,15 @@ use std::borrow::Cow; use std::collections::BTreeMap; use configuration::native_mutation::NativeMutation; +use itertools::Itertools as _; +use mongodb::bson::Bson; use mongodb::options::SelectionCriteria; use mongodb::{bson, Database}; -use ndc_models::Argument; +use ndc_models::ArgumentName; -use crate::mongo_query_plan::Type; -use crate::query::arguments::resolve_arguments; +use crate::mongo_query_plan::{MutationProcedureArgument, Type}; +use crate::query::make_selector; +use crate::query::serialization::json_to_bson; pub use self::error::ProcedureError; pub use self::interpolated_command::interpolated_command; @@ -18,9 +21,8 @@ pub use self::interpolated_command::interpolated_command; /// Encapsulates running arbitrary mongodb commands with interpolated arguments #[derive(Clone, Debug)] pub struct Procedure<'a> { - arguments: BTreeMap, + arguments: BTreeMap, command: Cow<'a, bson::Document>, - parameters: Cow<'a, BTreeMap>, result_type: Type, selection_criteria: Option>, } @@ -28,12 +30,11 @@ pub struct Procedure<'a> { impl<'a> Procedure<'a> { pub fn from_native_mutation( native_mutation: &'a NativeMutation, - arguments: BTreeMap, + arguments: BTreeMap, ) -> Self { Procedure { arguments, command: Cow::Borrowed(&native_mutation.command), - parameters: Cow::Borrowed(&native_mutation.arguments), result_type: native_mutation.result_type.clone(), selection_criteria: native_mutation .selection_criteria @@ -47,25 +48,50 @@ impl<'a> Procedure<'a> { database: Database, ) -> Result<(bson::Document, Type), ProcedureError> { let selection_criteria = self.selection_criteria.map(Cow::into_owned); - let command = interpolate(&self.parameters, self.arguments, &self.command)?; + let command = interpolate(self.arguments, &self.command)?; let result = database.run_command(command, selection_criteria).await?; Ok((result, self.result_type)) } pub fn interpolated_command(self) -> Result { - interpolate(&self.parameters, self.arguments, &self.command) + interpolate(self.arguments, &self.command) } } fn interpolate( - parameters: &BTreeMap, - arguments: BTreeMap, + arguments: BTreeMap, command: &bson::Document, ) -> Result { - let arguments = arguments + let bson_arguments = arguments .into_iter() - .map(|(name, value)| (name, Argument::Literal { value })) - .collect(); - let bson_arguments = resolve_arguments(parameters, arguments)?; + .map(|(name, argument)| { + let bson = argument_to_mongodb_expression(&name, argument)?; + Ok((name, bson)) as Result<_, ProcedureError> + }) + .try_collect()?; interpolated_command(command, &bson_arguments) } + +fn argument_to_mongodb_expression( + name: &ArgumentName, + argument: MutationProcedureArgument, +) -> Result { + let bson = match argument { + MutationProcedureArgument::Literal { + value, + argument_type, + } => json_to_bson(&argument_type, value).map_err(|error| { + ProcedureError::ErrorParsingArgument { + argument_name: name.to_string(), + error, + } + })?, + MutationProcedureArgument::Predicate { expression } => make_selector(&expression) + .map_err(|error| ProcedureError::ErrorParsingPredicate { + argument_name: name.to_string(), + error: Box::new(error), + })? + .into(), + }; + Ok(bson) +} diff --git a/crates/mongodb-agent-common/src/query/arguments.rs b/crates/mongodb-agent-common/src/query/arguments.rs deleted file mode 100644 index bd8cdb9a..00000000 --- a/crates/mongodb-agent-common/src/query/arguments.rs +++ /dev/null @@ -1,114 +0,0 @@ -use std::collections::BTreeMap; - -use indent::indent_all_by; -use itertools::Itertools as _; -use mongodb::bson::Bson; -use ndc_models::Argument; -use thiserror::Error; - -use crate::mongo_query_plan::Type; - -use super::{ - query_variable_name::query_variable_name, - serialization::{json_to_bson, JsonToBsonError}, -}; - -#[derive(Debug, Error)] -pub enum ArgumentError { - #[error("unknown variables or arguments: {}", .0.join(", "))] - Excess(Vec), - - #[error("some variables or arguments are invalid:\n{}", format_errors(.0))] - Invalid(BTreeMap), - - #[error("missing variables or arguments: {}", .0.join(", "))] - Missing(Vec), -} - -/// Translate arguments to queries or native queries to BSON according to declared parameter types. -/// -/// Checks that all arguments have been provided, and that no arguments have been given that do not -/// map to declared parameters (no excess arguments). -pub fn resolve_arguments( - parameters: &BTreeMap, - mut arguments: BTreeMap, -) -> Result, ArgumentError> { - validate_no_excess_arguments(parameters, &arguments)?; - - let (arguments, missing): ( - Vec<(ndc_models::ArgumentName, Argument, &Type)>, - Vec, - ) = parameters - .iter() - .map(|(name, parameter_type)| { - if let Some((name, argument)) = arguments.remove_entry(name) { - Ok((name, argument, parameter_type)) - } else { - Err(name.clone()) - } - }) - .partition_result(); - if !missing.is_empty() { - return Err(ArgumentError::Missing(missing)); - } - - let (resolved, errors): ( - BTreeMap, - BTreeMap, - ) = arguments - .into_iter() - .map(|(name, argument, parameter_type)| { - match argument_to_mongodb_expression(&argument, parameter_type) { - Ok(bson) => Ok((name, bson)), - Err(err) => Err((name, err)), - } - }) - .partition_result(); - if !errors.is_empty() { - return Err(ArgumentError::Invalid(errors)); - } - - Ok(resolved) -} - -fn argument_to_mongodb_expression( - argument: &Argument, - parameter_type: &Type, -) -> Result { - match argument { - Argument::Variable { name } => { - let mongodb_var_name = query_variable_name(name, parameter_type); - Ok(format!("$${mongodb_var_name}").into()) - } - Argument::Literal { value } => json_to_bson(parameter_type, value.clone()), - } -} - -pub fn validate_no_excess_arguments( - parameters: &BTreeMap, - arguments: &BTreeMap, -) -> Result<(), ArgumentError> { - let excess: Vec = arguments - .iter() - .filter_map(|(name, _)| { - let parameter = parameters.get(name); - match parameter { - Some(_) => None, - None => Some(name.clone()), - } - }) - .collect(); - if !excess.is_empty() { - Err(ArgumentError::Excess(excess)) - } else { - Ok(()) - } -} - -fn format_errors(errors: &BTreeMap) -> String { - errors - .iter() - .map(|(name, error)| format!(" {name}:\n{}", indent_all_by(4, error.to_string()))) - .collect::>() - .join("\n") -} diff --git a/crates/mongodb-agent-common/src/query/foreach.rs b/crates/mongodb-agent-common/src/query/foreach.rs index 00bf3596..29f0fcc6 100644 --- a/crates/mongodb-agent-common/src/query/foreach.rs +++ b/crates/mongodb-agent-common/src/query/foreach.rs @@ -1,5 +1,4 @@ use anyhow::anyhow; -use configuration::MongoScalarType; use itertools::Itertools as _; use mongodb::bson::{self, doc, Bson}; use ndc_query_plan::VariableSet; @@ -94,15 +93,11 @@ fn variable_sets_to_bson( fn variable_to_bson<'a>( name: &'a ndc_models::VariableName, value: &'a serde_json::Value, - variable_types: impl IntoIterator> + 'a, + variable_types: impl IntoIterator + 'a, ) -> impl Iterator> + 'a { - variable_types.into_iter().map(|t| { - let resolved_type = match t { - None => &Type::Scalar(MongoScalarType::ExtendedJSON), - Some(t) => t, - }; - let variable_name = query_variable_name(name, resolved_type); - let bson_value = json_to_bson(resolved_type, value.clone()) + variable_types.into_iter().map(|variable_type| { + let variable_name = query_variable_name(name, variable_type); + let bson_value = json_to_bson(variable_type, value.clone()) .map_err(|e| MongoAgentError::BadQuery(anyhow!(e)))?; Ok((variable_name, bson_value)) }) diff --git a/crates/mongodb-agent-common/src/query/mod.rs b/crates/mongodb-agent-common/src/query/mod.rs index 5c4e5dca..f9297a07 100644 --- a/crates/mongodb-agent-common/src/query/mod.rs +++ b/crates/mongodb-agent-common/src/query/mod.rs @@ -1,4 +1,3 @@ -pub mod arguments; mod column_ref; mod constants; mod execute_query_request; diff --git a/crates/mongodb-agent-common/src/query/native_query.rs b/crates/mongodb-agent-common/src/query/native_query.rs index 7b976b4f..946b5eea 100644 --- a/crates/mongodb-agent-common/src/query/native_query.rs +++ b/crates/mongodb-agent-common/src/query/native_query.rs @@ -2,16 +2,20 @@ use std::collections::BTreeMap; use configuration::native_query::NativeQuery; use itertools::Itertools as _; -use ndc_models::Argument; +use mongodb::bson::Bson; +use ndc_models::ArgumentName; use crate::{ interface_types::MongoAgentError, - mongo_query_plan::{MongoConfiguration, QueryPlan}, + mongo_query_plan::{Argument, MongoConfiguration, QueryPlan}, mongodb::{Pipeline, Stage}, procedure::{interpolated_command, ProcedureError}, }; -use super::{arguments::resolve_arguments, query_target::QueryTarget}; +use super::{ + make_selector, query_target::QueryTarget, query_variable_name::query_variable_name, + serialization::json_to_bson, +}; /// Returns either the pipeline defined by a native query with variable bindings for arguments, or /// an empty pipeline if the query request target is not a native query @@ -33,8 +37,13 @@ fn make_pipeline( native_query: &NativeQuery, arguments: &BTreeMap, ) -> Result { - let bson_arguments = resolve_arguments(&native_query.arguments, arguments.clone()) - .map_err(ProcedureError::UnresolvableArguments)?; + let bson_arguments = arguments + .iter() + .map(|(name, argument)| { + let bson = argument_to_mongodb_expression(name, argument.clone())?; + Ok((name.clone(), bson)) as Result<_, MongoAgentError> + }) + .try_collect()?; // Replace argument placeholders with resolved expressions, convert document list to // a `Pipeline` value @@ -48,6 +57,37 @@ fn make_pipeline( Ok(Pipeline::new(stages)) } +fn argument_to_mongodb_expression( + name: &ArgumentName, + argument: Argument, +) -> Result { + let bson = match argument { + Argument::Literal { + value, + argument_type, + } => json_to_bson(&argument_type, value).map_err(|error| { + ProcedureError::ErrorParsingArgument { + argument_name: name.to_string(), + error, + } + })?, + Argument::Variable { + name, + argument_type, + } => { + let mongodb_var_name = query_variable_name(&name, &argument_type); + format!("$${mongodb_var_name}").into() + } + Argument::Predicate { expression } => make_selector(&expression) + .map_err(|error| ProcedureError::ErrorParsingPredicate { + argument_name: name.to_string(), + error: Box::new(error), + })? + .into(), + }; + Ok(bson) +} + #[cfg(test)] mod tests { use configuration::{ diff --git a/crates/mongodb-agent-common/src/query/query_target.rs b/crates/mongodb-agent-common/src/query/query_target.rs index b48fa7c3..6100333b 100644 --- a/crates/mongodb-agent-common/src/query/query_target.rs +++ b/crates/mongodb-agent-common/src/query/query_target.rs @@ -1,9 +1,8 @@ use std::{collections::BTreeMap, fmt::Display}; use configuration::native_query::NativeQuery; -use ndc_models::Argument; -use crate::mongo_query_plan::{MongoConfiguration, QueryPlan}; +use crate::mongo_query_plan::{Argument, MongoConfiguration, QueryPlan}; #[derive(Clone, Debug)] pub enum QueryTarget<'a> { diff --git a/crates/mongodb-connector/src/mutation.rs b/crates/mongodb-connector/src/mutation.rs index 9f710812..e517dbb4 100644 --- a/crates/mongodb-connector/src/mutation.rs +++ b/crates/mongodb-connector/src/mutation.rs @@ -5,19 +5,19 @@ use mongodb::{ Database, }; use mongodb_agent_common::{ - mongo_query_plan::MongoConfiguration, + mongo_query_plan::{ + Field, MongoConfiguration, MutationOperation, MutationPlan, NestedArray, NestedField, + NestedObject, + }, procedure::Procedure, query::{response::type_for_nested_field, serialization::bson_to_json}, state::ConnectorState, }; -use ndc_query_plan::type_annotated_nested_field; +use ndc_query_plan::plan_for_mutation_request; use ndc_sdk::{ connector::MutationError, json_response::JsonResponse, - models::{ - self as ndc, MutationOperation, MutationOperationResults, MutationRequest, - MutationResponse, NestedField, NestedObject, - }, + models::{MutationOperationResults, MutationRequest, MutationResponse}, }; use crate::error_mapping::error_response; @@ -28,16 +28,16 @@ pub async fn handle_mutation_request( mutation_request: MutationRequest, ) -> Result, MutationError> { tracing::debug!(?config, mutation_request = %serde_json::to_string(&mutation_request).unwrap(), "executing mutation"); + let mutation_plan = plan_for_mutation_request(config, mutation_request).map_err(|err| { + MutationError::UnprocessableContent(error_response(format!( + "error processing mutation request: {}", + err + ))) + })?; let database = state.database(); - let jobs = look_up_procedures(config, &mutation_request)?; + let jobs = look_up_procedures(config, &mutation_plan)?; let operation_results = try_join_all(jobs.into_iter().map(|(procedure, requested_fields)| { - execute_procedure( - config, - &mutation_request, - database.clone(), - procedure, - requested_fields, - ) + execute_procedure(config, database.clone(), procedure, requested_fields) })) .await?; Ok(JsonResponse::Value(MutationResponse { operation_results })) @@ -47,9 +47,9 @@ pub async fn handle_mutation_request( /// arguments and requested fields. Returns an error if any procedures cannot be found. fn look_up_procedures<'a, 'b>( config: &'a MongoConfiguration, - mutation_request: &'b MutationRequest, + mutation_plan: &'b MutationPlan, ) -> Result, Option<&'b NestedField>)>, MutationError> { - let (procedures, not_found): (Vec<_>, Vec) = mutation_request + let (procedures, not_found): (Vec<_>, Vec) = mutation_plan .operations .iter() .map(|operation| match operation { @@ -57,6 +57,7 @@ fn look_up_procedures<'a, 'b>( name, arguments, fields, + relationships: _, } => { let native_mutation = config.native_mutations().get(name); let procedure = native_mutation @@ -83,7 +84,6 @@ fn look_up_procedures<'a, 'b>( async fn execute_procedure( config: &MongoConfiguration, - mutation_request: &MutationRequest, database: Database, procedure: Procedure<'_>, requested_fields: Option<&NestedField>, @@ -96,14 +96,7 @@ async fn execute_procedure( let rewritten_result = rewrite_response(requested_fields, result.into())?; let requested_result_type = if let Some(fields) = requested_fields { - let plan_field = type_annotated_nested_field( - config, - &mutation_request.collection_relationships, - &result_type, - fields.clone(), - ) - .map_err(|err| MutationError::UnprocessableContent(error_response(err.to_string())))?; - type_for_nested_field(&[], &result_type, &plan_field) + type_for_nested_field(&[], &result_type, fields) .map_err(|err| MutationError::UnprocessableContent(error_response(err.to_string())))? } else { result_type @@ -155,10 +148,10 @@ fn rewrite_doc( .iter() .map(|(name, field)| { let field_value = match field { - ndc::Field::Column { + Field::Column { column, + column_type: _, fields, - arguments: _, } => { let orig_value = doc.remove(column.as_str()).ok_or_else(|| { MutationError::UnprocessableContent(error_response(format!( @@ -167,7 +160,7 @@ fn rewrite_doc( })?; rewrite_response(fields.as_ref(), orig_value) } - ndc::Field::Relationship { .. } => Err(MutationError::UnsupportedOperation( + Field::Relationship { .. } => Err(MutationError::UnsupportedOperation( error_response("The MongoDB connector does not support relationship references in mutations" .to_owned()), )), @@ -178,7 +171,7 @@ fn rewrite_doc( .try_collect() } -fn rewrite_array(fields: &ndc::NestedArray, values: Vec) -> Result, MutationError> { +fn rewrite_array(fields: &NestedArray, values: Vec) -> Result, MutationError> { let nested = &fields.fields; values .into_iter() From cb8ff98171ba47f352b43b8e2c499a60ae92c84d Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 01:03:27 -0700 Subject: [PATCH 06/20] update tests --- .../arguments_to_mongodb_expressions.rs | 48 +++++++++++++ .../src/procedure/interpolated_command.rs | 67 ++++++++++--------- .../mongodb-agent-common/src/procedure/mod.rs | 39 +---------- 3 files changed, 88 insertions(+), 66 deletions(-) create mode 100644 crates/mongodb-agent-common/src/procedure/arguments_to_mongodb_expressions.rs diff --git a/crates/mongodb-agent-common/src/procedure/arguments_to_mongodb_expressions.rs b/crates/mongodb-agent-common/src/procedure/arguments_to_mongodb_expressions.rs new file mode 100644 index 00000000..17485885 --- /dev/null +++ b/crates/mongodb-agent-common/src/procedure/arguments_to_mongodb_expressions.rs @@ -0,0 +1,48 @@ +use std::collections::BTreeMap; + +use itertools::Itertools as _; +use mongodb::bson::Bson; +use ndc_models as ndc; + +use crate::{ + mongo_query_plan::MutationProcedureArgument, + query::{make_selector, serialization::json_to_bson}, +}; + +use super::ProcedureError; + +pub fn arguments_to_mongodb_expressions( + arguments: BTreeMap, +) -> Result, ProcedureError> { + arguments + .into_iter() + .map(|(name, argument)| { + let bson = argument_to_mongodb_expression(&name, argument)?; + Ok((name, bson)) as Result<_, ProcedureError> + }) + .try_collect() +} + +fn argument_to_mongodb_expression( + name: &ndc::ArgumentName, + argument: MutationProcedureArgument, +) -> Result { + let bson = match argument { + MutationProcedureArgument::Literal { + value, + argument_type, + } => json_to_bson(&argument_type, value).map_err(|error| { + ProcedureError::ErrorParsingArgument { + argument_name: name.to_string(), + error, + } + })?, + MutationProcedureArgument::Predicate { expression } => make_selector(&expression) + .map_err(|error| ProcedureError::ErrorParsingPredicate { + argument_name: name.to_string(), + error: Box::new(error), + })? + .into(), + }; + Ok(bson) +} diff --git a/crates/mongodb-agent-common/src/procedure/interpolated_command.rs b/crates/mongodb-agent-common/src/procedure/interpolated_command.rs index f04dde4c..d586940d 100644 --- a/crates/mongodb-agent-common/src/procedure/interpolated_command.rs +++ b/crates/mongodb-agent-common/src/procedure/interpolated_command.rs @@ -150,13 +150,13 @@ mod tests { use configuration::{native_mutation::NativeMutation, MongoScalarType}; use mongodb::bson::doc; use mongodb_support::BsonScalarType as S; - use ndc_models::Argument; + use ndc_query_plan::MutationProcedureArgument; use pretty_assertions::assert_eq; use serde_json::json; use crate::{ mongo_query_plan::{ObjectType, Type}, - query::arguments::resolve_arguments, + procedure::arguments_to_mongodb_expressions::arguments_to_mongodb_expressions, }; use super::*; @@ -188,18 +188,24 @@ mod tests { }; let input_arguments = [ - ("id".into(), Argument::Literal { value: json!(1001) }), + ( + "id".into(), + MutationProcedureArgument::Literal { + value: json!(1001), + argument_type: Type::Scalar(MongoScalarType::Bson(S::Int)), + }, + ), ( "name".into(), - Argument::Literal { + MutationProcedureArgument::Literal { value: json!("Regina Spektor"), + argument_type: Type::Scalar(MongoScalarType::Bson(S::String)), }, ), ] - .into_iter() - .collect(); + .into(); - let arguments = resolve_arguments(&native_mutation.arguments, input_arguments)?; + let arguments = arguments_to_mongodb_expressions(input_arguments)?; let command = interpolated_command(&native_mutation.command, &arguments)?; assert_eq!( @@ -217,29 +223,27 @@ mod tests { #[test] fn interpolates_array_argument() -> anyhow::Result<()> { + let documents_type = Type::ArrayOf(Box::new(Type::Object(ObjectType { + name: Some("ArtistInput".into()), + fields: [ + ( + "ArtistId".into(), + Type::Scalar(MongoScalarType::Bson(S::Int)), + ), + ( + "Name".into(), + Type::Scalar(MongoScalarType::Bson(S::String)), + ), + ] + .into(), + }))); + let native_mutation = NativeMutation { result_type: Type::Object(ObjectType { name: Some("InsertArtist".into()), fields: [("ok".into(), Type::Scalar(MongoScalarType::Bson(S::Bool)))].into(), }), - arguments: [( - "documents".into(), - Type::ArrayOf(Box::new(Type::Object(ObjectType { - name: Some("ArtistInput".into()), - fields: [ - ( - "ArtistId".into(), - Type::Scalar(MongoScalarType::Bson(S::Int)), - ), - ( - "Name".into(), - Type::Scalar(MongoScalarType::Bson(S::String)), - ), - ] - .into(), - }))), - )] - .into(), + arguments: [("documents".into(), documents_type.clone())].into(), command: doc! { "insert": "Artist", "documents": "{{ documents }}", @@ -250,17 +254,18 @@ mod tests { let input_arguments = [( "documents".into(), - Argument::Literal { + MutationProcedureArgument::Literal { value: json!([ { "ArtistId": 1001, "Name": "Regina Spektor" } , { "ArtistId": 1002, "Name": "Ok Go" } , ]), + argument_type: documents_type, }, )] .into_iter() .collect(); - let arguments = resolve_arguments(&native_mutation.arguments, input_arguments)?; + let arguments = arguments_to_mongodb_expressions(input_arguments)?; let command = interpolated_command(&native_mutation.command, &arguments)?; assert_eq!( @@ -311,21 +316,23 @@ mod tests { let input_arguments = [ ( "prefix".into(), - Argument::Literal { + MutationProcedureArgument::Literal { value: json!("current"), + argument_type: Type::Scalar(MongoScalarType::Bson(S::String)), }, ), ( "basename".into(), - Argument::Literal { + MutationProcedureArgument::Literal { value: json!("some-coll"), + argument_type: Type::Scalar(MongoScalarType::Bson(S::String)), }, ), ] .into_iter() .collect(); - let arguments = resolve_arguments(&native_mutation.arguments, input_arguments)?; + let arguments = arguments_to_mongodb_expressions(input_arguments)?; let command = interpolated_command(&native_mutation.command, &arguments)?; assert_eq!( diff --git a/crates/mongodb-agent-common/src/procedure/mod.rs b/crates/mongodb-agent-common/src/procedure/mod.rs index f509b329..e700efa8 100644 --- a/crates/mongodb-agent-common/src/procedure/mod.rs +++ b/crates/mongodb-agent-common/src/procedure/mod.rs @@ -1,19 +1,16 @@ +mod arguments_to_mongodb_expressions; mod error; mod interpolated_command; use std::borrow::Cow; use std::collections::BTreeMap; +use arguments_to_mongodb_expressions::arguments_to_mongodb_expressions; use configuration::native_mutation::NativeMutation; -use itertools::Itertools as _; -use mongodb::bson::Bson; use mongodb::options::SelectionCriteria; use mongodb::{bson, Database}; -use ndc_models::ArgumentName; use crate::mongo_query_plan::{MutationProcedureArgument, Type}; -use crate::query::make_selector; -use crate::query::serialization::json_to_bson; pub use self::error::ProcedureError; pub use self::interpolated_command::interpolated_command; @@ -62,36 +59,6 @@ fn interpolate( arguments: BTreeMap, command: &bson::Document, ) -> Result { - let bson_arguments = arguments - .into_iter() - .map(|(name, argument)| { - let bson = argument_to_mongodb_expression(&name, argument)?; - Ok((name, bson)) as Result<_, ProcedureError> - }) - .try_collect()?; + let bson_arguments = arguments_to_mongodb_expressions(arguments)?; interpolated_command(command, &bson_arguments) } - -fn argument_to_mongodb_expression( - name: &ArgumentName, - argument: MutationProcedureArgument, -) -> Result { - let bson = match argument { - MutationProcedureArgument::Literal { - value, - argument_type, - } => json_to_bson(&argument_type, value).map_err(|error| { - ProcedureError::ErrorParsingArgument { - argument_name: name.to_string(), - error, - } - })?, - MutationProcedureArgument::Predicate { expression } => make_selector(&expression) - .map_err(|error| ProcedureError::ErrorParsingPredicate { - argument_name: name.to_string(), - error: Box::new(error), - })? - .into(), - }; - Ok(bson) -} From b646ec6fa8c2649b3317cadb5d6616c4e672e190 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 01:15:07 -0700 Subject: [PATCH 07/20] allow predicate arguments in native mutation and query definitions --- crates/configuration/src/schema/mod.rs | 10 ++++++++++ .../src/interface_types/mongo_agent_error.rs | 10 +++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/configuration/src/schema/mod.rs b/crates/configuration/src/schema/mod.rs index 465fe724..3476e75f 100644 --- a/crates/configuration/src/schema/mod.rs +++ b/crates/configuration/src/schema/mod.rs @@ -34,6 +34,12 @@ pub enum Type { ArrayOf(Box), /// A nullable form of any of the other types Nullable(Box), + /// A predicate type for a given object type + #[serde(rename_all = "camelCase")] + Predicate { + /// The object type name + object_type_name: ndc_models::ObjectTypeName, + }, } impl Type { @@ -42,6 +48,7 @@ impl Type { Type::ExtendedJSON => Type::ExtendedJSON, Type::Scalar(s) => Type::Scalar(s), Type::Object(o) => Type::Object(o), + Type::Predicate { object_type_name } => Type::Predicate { object_type_name }, Type::ArrayOf(a) => Type::ArrayOf(Box::new((*a).normalize_type())), Type::Nullable(n) => match *n { Type::ExtendedJSON => Type::ExtendedJSON, @@ -84,6 +91,9 @@ impl From for ndc_models::Type { Type::Nullable(t) => ndc_models::Type::Nullable { underlying_type: Box::new(map_normalized_type(*t)), }, + Type::Predicate { object_type_name } => { + ndc_models::Type::Predicate { object_type_name } + } } } map_normalized_type(t.normalize_type()) diff --git a/crates/mongodb-agent-common/src/interface_types/mongo_agent_error.rs b/crates/mongodb-agent-common/src/interface_types/mongo_agent_error.rs index 40b1dff1..667e30c5 100644 --- a/crates/mongodb-agent-common/src/interface_types/mongo_agent_error.rs +++ b/crates/mongodb-agent-common/src/interface_types/mongo_agent_error.rs @@ -126,13 +126,13 @@ pub enum ErrorResponseType { MutationPermissionCheckFailure, } -impl ToString for ErrorResponseType { - fn to_string(&self) -> String { +impl std::fmt::Display for ErrorResponseType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::UncaughtError => String::from("uncaught-error"), - Self::MutationConstraintViolation => String::from("mutation-constraint-violation"), + Self::UncaughtError => f.write_str("uncaught-error"), + Self::MutationConstraintViolation => f.write_str("mutation-constraint-violation"), Self::MutationPermissionCheckFailure => { - String::from("mutation-permission-check-failure") + f.write_str("mutation-permission-check-failure") } } } From 52e1d6d2eb26a4c18f31c485d54eeaef1aa7229f Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 01:35:00 -0700 Subject: [PATCH 08/20] example native mutation using predicate argument --- .../native_mutations/update_track_prices.json | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json diff --git a/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json new file mode 100644 index 00000000..5e3783fb --- /dev/null +++ b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json @@ -0,0 +1,45 @@ +{ + "name": "updateTrackPrices", + "description": "Update unit price of every track that matches predicate", + "resultType": { + "object": "UpdateTrackPrices" + }, + "arguments": { + "newPrice": { + "type": { + "scalar": "decimal" + } + }, + "where": { + "type": { + "predicate": { "objectTypeName": "Track" } + } + } + }, + "objectTypes": { + "UpdateTrackPrices": { + "fields": { + "ok": { + "type": { + "scalar": "double" + } + }, + "n": { + "type": { + "scalar": "int" + } + } + } + } + }, + "command": { + "update": "Track", + "updates": [{ + "q": "{{ where }}", + "u": { + "UnitPrice": "{{ newPrice }}" + }, + "multi": true + }] + } +} From 8186d02efe5e31d8e64ab078816ff8bfac64e543 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 12:35:25 -0700 Subject: [PATCH 09/20] add Parameter type to catch value vs predicate distinction --- crates/configuration/src/lib.rs | 2 + crates/configuration/src/native_mutation.rs | 12 +++--- crates/configuration/src/native_query.rs | 12 ++---- crates/configuration/src/parameter.rs | 38 +++++++++++++++++++ .../query_plan_error.rs | 6 +-- crates/ndc-query-plan/src/type_system.rs | 2 +- 6 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 crates/configuration/src/parameter.rs diff --git a/crates/configuration/src/lib.rs b/crates/configuration/src/lib.rs index c9c2f971..83ba02ab 100644 --- a/crates/configuration/src/lib.rs +++ b/crates/configuration/src/lib.rs @@ -3,6 +3,7 @@ mod directory; mod mongo_scalar_type; pub mod native_mutation; pub mod native_query; +mod parameter; pub mod schema; pub mod serialized; mod with_name; @@ -16,3 +17,4 @@ pub use crate::directory::write_schema_directory; pub use crate::mongo_scalar_type::MongoScalarType; pub use crate::serialized::Schema; pub use crate::with_name::{WithName, WithNameRef}; +pub use parameter::Parameter; diff --git a/crates/configuration/src/native_mutation.rs b/crates/configuration/src/native_mutation.rs index 436673f2..f53fca97 100644 --- a/crates/configuration/src/native_mutation.rs +++ b/crates/configuration/src/native_mutation.rs @@ -6,7 +6,7 @@ use ndc_models as ndc; use ndc_query_plan as plan; use plan::{inline_object_types, QueryPlanError}; -use crate::{serialized, MongoScalarType}; +use crate::{serialized, MongoScalarType, Parameter}; /// Internal representation of Native Mutations. For doc comments see /// [crate::serialized::NativeMutation] @@ -17,7 +17,7 @@ use crate::{serialized, MongoScalarType}; #[derive(Clone, Debug)] pub struct NativeMutation { pub result_type: plan::Type, - pub arguments: BTreeMap>, + pub arguments: BTreeMap, pub command: bson::Document, pub selection_criteria: Option, pub description: Option, @@ -28,17 +28,15 @@ impl NativeMutation { object_types: &BTreeMap, input: serialized::NativeMutation, ) -> Result { + // TODO: convert predicate arguments to the appropriate argument enum variant instead of + // sending them through [inline_object_types] let arguments = input .arguments .into_iter() .map(|(name, object_field)| { Ok(( name, - inline_object_types( - object_types, - &object_field.r#type.into(), - MongoScalarType::lookup_scalar_type, - )?, + Parameter::from_object_field(object_types, object_field)?, )) as Result<_, QueryPlanError> }) .try_collect()?; diff --git a/crates/configuration/src/native_query.rs b/crates/configuration/src/native_query.rs index 3eea44a2..8582fbca 100644 --- a/crates/configuration/src/native_query.rs +++ b/crates/configuration/src/native_query.rs @@ -4,11 +4,11 @@ use itertools::Itertools as _; use mongodb::bson; use ndc_models as ndc; use ndc_query_plan as plan; -use plan::{inline_object_types, QueryPlanError}; +use plan::QueryPlanError; use schemars::JsonSchema; use serde::Deserialize; -use crate::{serialized, MongoScalarType}; +use crate::{serialized, Parameter}; /// Internal representation of Native Queries. For doc comments see /// [crate::serialized::NativeQuery] @@ -20,7 +20,7 @@ use crate::{serialized, MongoScalarType}; pub struct NativeQuery { pub representation: NativeQueryRepresentation, pub input_collection: Option, - pub arguments: BTreeMap>, + pub arguments: BTreeMap, pub result_document_type: ndc::ObjectTypeName, pub pipeline: Vec, pub description: Option, @@ -37,11 +37,7 @@ impl NativeQuery { .map(|(name, object_field)| { Ok(( name, - inline_object_types( - object_types, - &object_field.r#type.into(), - MongoScalarType::lookup_scalar_type, - )?, + Parameter::from_object_field(object_types, object_field)?, )) as Result<_, QueryPlanError> }) .try_collect()?; diff --git a/crates/configuration/src/parameter.rs b/crates/configuration/src/parameter.rs new file mode 100644 index 00000000..215e4206 --- /dev/null +++ b/crates/configuration/src/parameter.rs @@ -0,0 +1,38 @@ +use std::collections::BTreeMap; + +use ndc_models as ndc; +use ndc_query_plan::{self as plan, inline_object_types, QueryPlanError}; + +use crate::{schema, MongoScalarType}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Parameter { + Value { + parameter_type: plan::Type, + }, + Predicate { + object_type_name: ndc::ObjectTypeName, + }, +} + +impl Parameter { + pub fn from_object_field( + object_types: &BTreeMap, + field: schema::ObjectField, + ) -> Result { + let parameter = match field.r#type { + schema::Type::Predicate { object_type_name } => { + Parameter::Predicate { object_type_name } + } + t => { + let parameter_type = inline_object_types( + object_types, + &t.into(), + MongoScalarType::lookup_scalar_type, + )?; + Parameter::Value { parameter_type } + } + }; + Ok(parameter) + } +} 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 29b05782..e0d0ffc0 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 @@ -26,9 +26,6 @@ pub enum QueryPlanError { #[error("missing arguments: {}", .0.join(", "))] MissingArguments(Vec), - #[error("The connector does not yet support {0}")] - NotImplemented(&'static str), - #[error("{0}")] RelationshipUnification(#[from] RelationshipUnificationError), @@ -38,6 +35,9 @@ pub enum QueryPlanError { #[error("{0}")] TypeMismatch(String), + #[error("found predicate argument in a value-only context")] + UnexpectedPredicate, + #[error("Unknown comparison operator, \"{0}\"")] UnknownComparisonOperator(ndc::ComparisonOperatorName), diff --git a/crates/ndc-query-plan/src/type_system.rs b/crates/ndc-query-plan/src/type_system.rs index 36c0824a..5d67904e 100644 --- a/crates/ndc-query-plan/src/type_system.rs +++ b/crates/ndc-query-plan/src/type_system.rs @@ -60,7 +60,7 @@ pub fn inline_object_types( element_type, lookup_scalar_type, )?)), - ndc::Type::Predicate { .. } => Err(QueryPlanError::NotImplemented("predicate types"))?, + ndc::Type::Predicate { .. } => Err(QueryPlanError::UnexpectedPredicate)?, }; Ok(plan_type) } From 2ab47fb329bf0a8500391c51c791f2601d62ecd5 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 12:37:51 -0700 Subject: [PATCH 10/20] actually it looks like native mutations and queries don't need parameter defs --- crates/configuration/src/lib.rs | 2 -- crates/configuration/src/native_mutation.rs | 18 +--------- crates/configuration/src/native_query.rs | 18 ++-------- crates/configuration/src/parameter.rs | 38 --------------------- 4 files changed, 3 insertions(+), 73 deletions(-) delete mode 100644 crates/configuration/src/parameter.rs diff --git a/crates/configuration/src/lib.rs b/crates/configuration/src/lib.rs index 83ba02ab..c9c2f971 100644 --- a/crates/configuration/src/lib.rs +++ b/crates/configuration/src/lib.rs @@ -3,7 +3,6 @@ mod directory; mod mongo_scalar_type; pub mod native_mutation; pub mod native_query; -mod parameter; pub mod schema; pub mod serialized; mod with_name; @@ -17,4 +16,3 @@ pub use crate::directory::write_schema_directory; pub use crate::mongo_scalar_type::MongoScalarType; pub use crate::serialized::Schema; pub use crate::with_name::{WithName, WithNameRef}; -pub use parameter::Parameter; diff --git a/crates/configuration/src/native_mutation.rs b/crates/configuration/src/native_mutation.rs index f53fca97..0f10c827 100644 --- a/crates/configuration/src/native_mutation.rs +++ b/crates/configuration/src/native_mutation.rs @@ -1,12 +1,11 @@ use std::collections::BTreeMap; -use itertools::Itertools as _; use mongodb::{bson, options::SelectionCriteria}; use ndc_models as ndc; use ndc_query_plan as plan; use plan::{inline_object_types, QueryPlanError}; -use crate::{serialized, MongoScalarType, Parameter}; +use crate::{serialized, MongoScalarType}; /// Internal representation of Native Mutations. For doc comments see /// [crate::serialized::NativeMutation] @@ -17,7 +16,6 @@ use crate::{serialized, MongoScalarType, Parameter}; #[derive(Clone, Debug)] pub struct NativeMutation { pub result_type: plan::Type, - pub arguments: BTreeMap, pub command: bson::Document, pub selection_criteria: Option, pub description: Option, @@ -28,19 +26,6 @@ impl NativeMutation { object_types: &BTreeMap, input: serialized::NativeMutation, ) -> Result { - // TODO: convert predicate arguments to the appropriate argument enum variant instead of - // sending them through [inline_object_types] - let arguments = input - .arguments - .into_iter() - .map(|(name, object_field)| { - Ok(( - name, - Parameter::from_object_field(object_types, object_field)?, - )) as Result<_, QueryPlanError> - }) - .try_collect()?; - let result_type = inline_object_types( object_types, &input.result_type.into(), @@ -49,7 +34,6 @@ impl NativeMutation { Ok(NativeMutation { result_type, - arguments, command: input.command, selection_criteria: input.selection_criteria, description: input.description, diff --git a/crates/configuration/src/native_query.rs b/crates/configuration/src/native_query.rs index 8582fbca..e8986bb6 100644 --- a/crates/configuration/src/native_query.rs +++ b/crates/configuration/src/native_query.rs @@ -1,6 +1,5 @@ use std::collections::BTreeMap; -use itertools::Itertools as _; use mongodb::bson; use ndc_models as ndc; use ndc_query_plan as plan; @@ -8,7 +7,7 @@ use plan::QueryPlanError; use schemars::JsonSchema; use serde::Deserialize; -use crate::{serialized, Parameter}; +use crate::serialized; /// Internal representation of Native Queries. For doc comments see /// [crate::serialized::NativeQuery] @@ -20,7 +19,6 @@ use crate::{serialized, Parameter}; pub struct NativeQuery { pub representation: NativeQueryRepresentation, pub input_collection: Option, - pub arguments: BTreeMap, pub result_document_type: ndc::ObjectTypeName, pub pipeline: Vec, pub description: Option, @@ -28,24 +26,12 @@ pub struct NativeQuery { impl NativeQuery { pub fn from_serialized( - object_types: &BTreeMap, + _object_types: &BTreeMap, input: serialized::NativeQuery, ) -> Result { - let arguments = input - .arguments - .into_iter() - .map(|(name, object_field)| { - Ok(( - name, - Parameter::from_object_field(object_types, object_field)?, - )) as Result<_, QueryPlanError> - }) - .try_collect()?; - Ok(NativeQuery { representation: input.representation, input_collection: input.input_collection, - arguments, result_document_type: input.result_document_type, pipeline: input.pipeline, description: input.description, diff --git a/crates/configuration/src/parameter.rs b/crates/configuration/src/parameter.rs deleted file mode 100644 index 215e4206..00000000 --- a/crates/configuration/src/parameter.rs +++ /dev/null @@ -1,38 +0,0 @@ -use std::collections::BTreeMap; - -use ndc_models as ndc; -use ndc_query_plan::{self as plan, inline_object_types, QueryPlanError}; - -use crate::{schema, MongoScalarType}; - -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum Parameter { - Value { - parameter_type: plan::Type, - }, - Predicate { - object_type_name: ndc::ObjectTypeName, - }, -} - -impl Parameter { - pub fn from_object_field( - object_types: &BTreeMap, - field: schema::ObjectField, - ) -> Result { - let parameter = match field.r#type { - schema::Type::Predicate { object_type_name } => { - Parameter::Predicate { object_type_name } - } - t => { - let parameter_type = inline_object_types( - object_types, - &t.into(), - MongoScalarType::lookup_scalar_type, - )?; - Parameter::Value { parameter_type } - } - }; - Ok(parameter) - } -} From 8de65ccefc6dd86dfc2ddce9df19dc8361a02e60 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 12:39:03 -0700 Subject: [PATCH 11/20] update tests --- .../src/procedure/interpolated_command.rs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/crates/mongodb-agent-common/src/procedure/interpolated_command.rs b/crates/mongodb-agent-common/src/procedure/interpolated_command.rs index d586940d..0761156a 100644 --- a/crates/mongodb-agent-common/src/procedure/interpolated_command.rs +++ b/crates/mongodb-agent-common/src/procedure/interpolated_command.rs @@ -168,14 +168,6 @@ mod tests { name: Some("InsertArtist".into()), fields: [("ok".into(), Type::Scalar(MongoScalarType::Bson(S::Bool)))].into(), }), - arguments: [ - ("id".into(), Type::Scalar(MongoScalarType::Bson(S::Int))), - ( - "name".into(), - Type::Scalar(MongoScalarType::Bson(S::String)), - ), - ] - .into(), command: doc! { "insert": "Artist", "documents": [{ @@ -243,7 +235,6 @@ mod tests { name: Some("InsertArtist".into()), fields: [("ok".into(), Type::Scalar(MongoScalarType::Bson(S::Bool)))].into(), }), - arguments: [("documents".into(), documents_type.clone())].into(), command: doc! { "insert": "Artist", "documents": "{{ documents }}", @@ -294,17 +285,6 @@ mod tests { name: Some("Insert".into()), fields: [("ok".into(), Type::Scalar(MongoScalarType::Bson(S::Bool)))].into(), }), - arguments: [ - ( - "prefix".into(), - Type::Scalar(MongoScalarType::Bson(S::String)), - ), - ( - "basename".into(), - Type::Scalar(MongoScalarType::Bson(S::String)), - ), - ] - .into(), command: doc! { "insert": "{{prefix}}-{{basename}}", "empty": "", From 14a1e4292ec4e2355be10b10644f9099b93eb0e4 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 12:44:48 -0700 Subject: [PATCH 12/20] track new native mutation in metadata --- .../native_mutations/update_track_prices.json | 18 +----------- .../hasura/chinook/metadata/chinook-types.hml | 4 +-- fixtures/hasura/chinook/metadata/chinook.hml | 16 +++++++++- .../metadata/commands/UpdateTrackPrices.hml | 29 +++++++++++++++++++ .../chinook/metadata/models/Invoice.hml | 2 +- .../chinook/metadata/models/InvoiceLine.hml | 2 +- .../hasura/chinook/metadata/models/Track.hml | 2 +- 7 files changed, 50 insertions(+), 23 deletions(-) create mode 100644 fixtures/hasura/chinook/metadata/commands/UpdateTrackPrices.hml diff --git a/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json index 5e3783fb..d5cb22da 100644 --- a/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json +++ b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json @@ -2,7 +2,7 @@ "name": "updateTrackPrices", "description": "Update unit price of every track that matches predicate", "resultType": { - "object": "UpdateTrackPrices" + "object": "InsertArtist" }, "arguments": { "newPrice": { @@ -16,22 +16,6 @@ } } }, - "objectTypes": { - "UpdateTrackPrices": { - "fields": { - "ok": { - "type": { - "scalar": "double" - } - }, - "n": { - "type": { - "scalar": "int" - } - } - } - } - }, "command": { "update": "Track", "updates": [{ diff --git a/fixtures/hasura/chinook/metadata/chinook-types.hml b/fixtures/hasura/chinook/metadata/chinook-types.hml index 8a8c6de0..4847339b 100644 --- a/fixtures/hasura/chinook/metadata/chinook-types.hml +++ b/fixtures/hasura/chinook/metadata/chinook-types.hml @@ -48,7 +48,7 @@ definition: kind: ScalarType version: v1 definition: - name: Decimal + name: Chinook_Decimal graphql: typeName: Chinook_Decimal @@ -58,7 +58,7 @@ version: v1 definition: dataConnectorName: chinook dataConnectorScalarType: Decimal - representation: Decimal + representation: Chinook_Decimal graphql: comparisonExpressionTypeName: Chinook_DecimalComparisonExp diff --git a/fixtures/hasura/chinook/metadata/chinook.hml b/fixtures/hasura/chinook/metadata/chinook.hml index 86f633b4..f627567e 100644 --- a/fixtures/hasura/chinook/metadata/chinook.hml +++ b/fixtures/hasura/chinook/metadata/chinook.hml @@ -1033,8 +1033,22 @@ definition: result_type: type: named name: InsertArtist + - name: updateTrackPrices + description: Update unit price of every track that matches predicate + arguments: + newPrice: + type: + type: named + name: Decimal + where: + type: + type: predicate + object_type_name: Track + result_type: + type: named + name: InsertArtist capabilities: - version: 0.1.4 + version: 0.1.5 capabilities: query: aggregates: {} diff --git a/fixtures/hasura/chinook/metadata/commands/UpdateTrackPrices.hml b/fixtures/hasura/chinook/metadata/commands/UpdateTrackPrices.hml new file mode 100644 index 00000000..4c6917dc --- /dev/null +++ b/fixtures/hasura/chinook/metadata/commands/UpdateTrackPrices.hml @@ -0,0 +1,29 @@ +--- +kind: Command +version: v1 +definition: + name: UpdateTrackPrices + outputType: InsertArtist! + arguments: + - name: newPrice + type: Chinook_Decimal! + - name: where + type: TrackBoolExp! + source: + dataConnectorName: chinook + dataConnectorCommand: + procedure: updateTrackPrices + graphql: + rootFieldName: chinook_updateTrackPrices + rootFieldKind: Mutation + description: Update unit price of every track that matches predicate + +--- +kind: CommandPermissions +version: v1 +definition: + commandName: UpdateTrackPrices + permissions: + - role: admin + allowExecution: true + diff --git a/fixtures/hasura/chinook/metadata/models/Invoice.hml b/fixtures/hasura/chinook/metadata/models/Invoice.hml index 8cd0391a..59f0f67f 100644 --- a/fixtures/hasura/chinook/metadata/models/Invoice.hml +++ b/fixtures/hasura/chinook/metadata/models/Invoice.hml @@ -23,7 +23,7 @@ definition: - name: invoiceId type: Int! - name: total - type: Decimal! + type: Chinook_Decimal! graphql: typeName: Invoice inputTypeName: InvoiceInput diff --git a/fixtures/hasura/chinook/metadata/models/InvoiceLine.hml b/fixtures/hasura/chinook/metadata/models/InvoiceLine.hml index 19d790c9..8f6d8792 100644 --- a/fixtures/hasura/chinook/metadata/models/InvoiceLine.hml +++ b/fixtures/hasura/chinook/metadata/models/InvoiceLine.hml @@ -15,7 +15,7 @@ definition: - name: trackId type: Int! - name: unitPrice - type: Decimal! + type: Chinook_Decimal! graphql: typeName: InvoiceLine inputTypeName: InvoiceLineInput diff --git a/fixtures/hasura/chinook/metadata/models/Track.hml b/fixtures/hasura/chinook/metadata/models/Track.hml index 3910420c..9ac5889e 100644 --- a/fixtures/hasura/chinook/metadata/models/Track.hml +++ b/fixtures/hasura/chinook/metadata/models/Track.hml @@ -23,7 +23,7 @@ definition: - name: trackId type: Int! - name: unitPrice - type: Decimal! + type: Chinook_Decimal! graphql: typeName: Track inputTypeName: TrackInput From d93e630da377eb36100115e1084d1b557fa458f8 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 13:42:34 -0700 Subject: [PATCH 13/20] integration test --- .../src/tests/native_mutation.rs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/integration-tests/src/tests/native_mutation.rs b/crates/integration-tests/src/tests/native_mutation.rs index 6a7574b4..8577d41e 100644 --- a/crates/integration-tests/src/tests/native_mutation.rs +++ b/crates/integration-tests/src/tests/native_mutation.rs @@ -57,3 +57,36 @@ async fn updates_with_native_mutation() -> anyhow::Result<()> { ); Ok(()) } + +#[tokio::test] +async fn accepts_predicate_argument() -> anyhow::Result<()> { + assert_yaml_snapshot!( + graphql_query( + r#" + mutation { + chinook_updateTrackPrices(newPrice: "11.99", where: {albumId: {_eq: 3}}) { + n + ok + } + } + "# + ) + .run() + .await? + ); + assert_yaml_snapshot!( + graphql_query( + r#" + query { + track(where: {albumId: {_eq: 3}}, order_by: {id: Asc}) { + name + unitPrice + } + } + "# + ) + .run() + .await? + ); + Ok(()) +} From e2f29944bd1c663a720fd5d4d6103e559fb10bad Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 19 Jul 2024 14:18:13 -0700 Subject: [PATCH 14/20] update to latest engine --- flake.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flake.lock b/flake.lock index 6192f37f..37094039 100644 --- a/flake.lock +++ b/flake.lock @@ -119,11 +119,11 @@ "graphql-engine-source": { "flake": false, "locked": { - "lastModified": 1717090976, - "narHash": "sha256-NUjY32Ec+pdYBXgfE0xtqfquTBJqoQqEKs4tV0jt+S0=", + "lastModified": 1721396058, + "narHash": "sha256-IqoYVvmd6a0fHbwYLR2Gm03Rd9u6GyGxZxC3juiUF50=", "owner": "hasura", "repo": "graphql-engine", - "rev": "11e1e02d59c9eede27a6c69765232f0273f03585", + "rev": "4244b0d87b6b08cee37723f4e30259d714dad332", "type": "github" }, "original": { From 9113165e7f99a53fc528267acd6f858c02dcf6a5 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Thu, 1 Aug 2024 10:39:01 -0700 Subject: [PATCH 15/20] update engine to get fix for permissions test regression --- flake.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flake.lock b/flake.lock index 37094039..44e7f7ac 100644 --- a/flake.lock +++ b/flake.lock @@ -119,11 +119,11 @@ "graphql-engine-source": { "flake": false, "locked": { - "lastModified": 1721396058, - "narHash": "sha256-IqoYVvmd6a0fHbwYLR2Gm03Rd9u6GyGxZxC3juiUF50=", + "lastModified": 1722615509, + "narHash": "sha256-LH10Tc/UWZ1uwxrw4tohmqR/uzVi53jHnr+ziuxJi8I=", "owner": "hasura", "repo": "graphql-engine", - "rev": "4244b0d87b6b08cee37723f4e30259d714dad332", + "rev": "03c85f69857ef556e9bb26f8b92e9e47317991a3", "type": "github" }, "original": { From b7a4102d04c726f0ea26fd6c27934e7461ca49c1 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 2 Aug 2024 10:00:24 -0700 Subject: [PATCH 16/20] test should verify that mutation updated something --- .../src/tests/native_mutation.rs | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/crates/integration-tests/src/tests/native_mutation.rs b/crates/integration-tests/src/tests/native_mutation.rs index 8577d41e..f12a9c27 100644 --- a/crates/integration-tests/src/tests/native_mutation.rs +++ b/crates/integration-tests/src/tests/native_mutation.rs @@ -60,20 +60,27 @@ async fn updates_with_native_mutation() -> anyhow::Result<()> { #[tokio::test] async fn accepts_predicate_argument() -> anyhow::Result<()> { - assert_yaml_snapshot!( - graphql_query( - r#" - mutation { - chinook_updateTrackPrices(newPrice: "11.99", where: {albumId: {_eq: 3}}) { - n - ok - } - } - "# - ) - .run() - .await? + let mutation_resp = graphql_query( + r#" + mutation { + chinook_updateTrackPrices(newPrice: "11.99", where: {albumId: {_eq: 3}}) { + n + ok + } + } + "#, + ) + .run() + .await?; + assert_eq!( + mutation_resp, + GraphQLResponse { + data: json!({ "chinook_updateTrackPrices": { "n": 3, "ok": 1 } }), + errors: None, + }, + "mutation with predicate argument succeeded, and updated a non-zero number of documents", ); + assert_yaml_snapshot!( graphql_query( r#" From f993cd99bac7b237cc1344aaadfd608a0f791342 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 2 Aug 2024 10:13:52 -0700 Subject: [PATCH 17/20] fix update document in native mutation --- .../connector/chinook/native_mutations/update_track_prices.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json index d5cb22da..5cbb8c2a 100644 --- a/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json +++ b/fixtures/hasura/chinook/connector/chinook/native_mutations/update_track_prices.json @@ -21,7 +21,7 @@ "updates": [{ "q": "{{ where }}", "u": { - "UnitPrice": "{{ newPrice }}" + "$set": { "UnitPrice": "{{ newPrice }}" } }, "multi": true }] From c86c9b7aa20a7a4033c9bf40b2311e71ed5a331f Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 2 Aug 2024 13:20:19 -0700 Subject: [PATCH 18/20] fix test --- Cargo.lock | 46 +++++++++++++ crates/integration-tests/Cargo.toml | 1 + crates/integration-tests/src/lib.rs | 2 + .../src/tests/native_mutation.rs | 64 +++++++++++-------- crates/integration-tests/src/validators.rs | 22 +++++++ 5 files changed, 110 insertions(+), 25 deletions(-) create mode 100644 crates/integration-tests/src/validators.rs diff --git a/Cargo.lock b/Cargo.lock index 46354404..21a6d05b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -109,6 +109,17 @@ version = "1.0.86" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b3d1d046238990b9cf5bcde22a3fb3584ee5cf65fb2765f454ed428c7a0063da" +[[package]] +name = "assert_json" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e0550d5b3aaf86bc467a65dda46146b51a62b72929fe6a22a8a9348eff8e822b" +dependencies = [ + "codespan-reporting", + "serde_json", + "thiserror", +] + [[package]] name = "async-stream" version = "0.3.5" @@ -404,6 +415,16 @@ version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4b82cf0babdbd58558212896d1a4272303a57bdb245c2bf1147185fb45640e70" +[[package]] +name = "codespan-reporting" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3538270d33cc669650c4b093848450d380def10c331d38c768e34cac80576e6e" +dependencies = [ + "termcolor", + "unicode-width", +] + [[package]] name = "colorchoice" version = "1.0.1" @@ -1424,6 +1445,7 @@ name = "integration-tests" version = "0.1.0" dependencies = [ "anyhow", + "assert_json", "insta", "ndc-models", "ndc-test-helpers", @@ -3195,6 +3217,15 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "termcolor" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06794f8f6c5c898b3275aebefa6b8a1cb24cd2c6c79397ab15774837a0bc5755" +dependencies = [ + "winapi-util", +] + [[package]] name = "termtree" version = "0.4.1" @@ -3702,6 +3733,12 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e4259d9d4425d9f0661581b804cb85fe66a4c631cadd8f490d1c13a35d5d9291" +[[package]] +name = "unicode-width" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0336d538f7abc86d282a4189614dfaa90810dfc2c6f6427eaf88e16311dd225d" + [[package]] name = "unsafe-libyaml" version = "0.2.11" @@ -3915,6 +3952,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" +[[package]] +name = "winapi-util" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4d4cc384e1e73b93bafa6fb4f1df8c41695c8a91cf9c4c64358067d15a7b6c6b" +dependencies = [ + "windows-sys 0.52.0", +] + [[package]] name = "winapi-x86_64-pc-windows-gnu" version = "0.4.0" diff --git a/crates/integration-tests/Cargo.toml b/crates/integration-tests/Cargo.toml index f8e9a380..2b885f49 100644 --- a/crates/integration-tests/Cargo.toml +++ b/crates/integration-tests/Cargo.toml @@ -11,6 +11,7 @@ ndc-models = { workspace = true } ndc-test-helpers = { path = "../ndc-test-helpers" } anyhow = "1" +assert_json = "^0.1" insta = { version = "^1.38", features = ["yaml"] } reqwest = { version = "^0.12.4", features = ["json"] } serde = { version = "1", features = ["derive"] } diff --git a/crates/integration-tests/src/lib.rs b/crates/integration-tests/src/lib.rs index 9044753e..72d02205 100644 --- a/crates/integration-tests/src/lib.rs +++ b/crates/integration-tests/src/lib.rs @@ -8,6 +8,7 @@ mod tests; mod connector; mod graphql; +mod validators; use std::env; @@ -16,6 +17,7 @@ use url::Url; pub use self::connector::{run_connector_query, ConnectorQueryRequest}; pub use self::graphql::{graphql_query, GraphQLRequest, GraphQLResponse}; +pub use self::validators::*; const CONNECTOR_URL: &str = "CONNECTOR_URL"; const ENGINE_GRAPHQL_URL: &str = "ENGINE_GRAPHQL_URL"; diff --git a/crates/integration-tests/src/tests/native_mutation.rs b/crates/integration-tests/src/tests/native_mutation.rs index f12a9c27..2dea14ac 100644 --- a/crates/integration-tests/src/tests/native_mutation.rs +++ b/crates/integration-tests/src/tests/native_mutation.rs @@ -1,4 +1,5 @@ -use crate::{graphql_query, GraphQLResponse}; +use crate::{graphql_query, non_empty_array, GraphQLResponse}; +use assert_json::{assert_json, validators}; use insta::assert_yaml_snapshot; use serde_json::json; @@ -60,40 +61,53 @@ async fn updates_with_native_mutation() -> anyhow::Result<()> { #[tokio::test] async fn accepts_predicate_argument() -> anyhow::Result<()> { + let album_id = 3; + let mutation_resp = graphql_query( r#" - mutation { - chinook_updateTrackPrices(newPrice: "11.99", where: {albumId: {_eq: 3}}) { + mutation($albumId: Int!) { + chinook_updateTrackPrices(newPrice: "11.99", where: {albumId: {_eq: $albumId}}) { n ok } } "#, ) + .variables(json!({ "albumId": album_id })) .run() .await?; - assert_eq!( - mutation_resp, - GraphQLResponse { - data: json!({ "chinook_updateTrackPrices": { "n": 3, "ok": 1 } }), - errors: None, - }, - "mutation with predicate argument succeeded, and updated a non-zero number of documents", - ); - assert_yaml_snapshot!( - graphql_query( - r#" - query { - track(where: {albumId: {_eq: 3}}, order_by: {id: Asc}) { - name - unitPrice - } - } - "# - ) - .run() - .await? - ); + assert_eq!(mutation_resp.errors, None); + assert_json!(mutation_resp.data, { + "chinook_updateTrackPrices": { + "ok": 1.0, + "n": validators::i64(|n| if n > &0 { + Ok(()) + } else { + Err("expected number of updated documents to be non-zero".to_string()) + }) + } + }); + + let tracks_resp = graphql_query( + r#" + query($albumId: Int!) { + track(where: {albumId: {_eq: $albumId}}, order_by: {id: Asc}) { + name + unitPrice + } + } + "#, + ) + .variables(json!({ "albumId": album_id })) + .run() + .await?; + + assert_json!(tracks_resp.data, { + "track": non_empty_array().and(validators::array_for_each(validators::object([ + ("unitPrice".to_string(), Box::new(validators::eq("11.99")) as Box) + ].into()))) + }); + Ok(()) } diff --git a/crates/integration-tests/src/validators.rs b/crates/integration-tests/src/validators.rs new file mode 100644 index 00000000..4bba2793 --- /dev/null +++ b/crates/integration-tests/src/validators.rs @@ -0,0 +1,22 @@ +use assert_json::{Error, Validator}; +use serde_json::Value; + +pub fn non_empty_array() -> NonEmptyArrayValidator { + NonEmptyArrayValidator +} + +pub struct NonEmptyArrayValidator; + +impl Validator for NonEmptyArrayValidator { + fn validate<'a>(&self, value: &'a Value) -> Result<(), Error<'a>> { + if let Value::Array(xs) = value { + if xs.is_empty() { + Err(Error::InvalidValue(value, "non-empty array".to_string())) + } else { + Ok(()) + } + } else { + Err(Error::InvalidType(value, "array".to_string())) + } + } +} From b09c8ae7f551f8bed422630f76567048473d4d0e Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 2 Aug 2024 13:22:23 -0700 Subject: [PATCH 19/20] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f728716b..8e4b6b66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ This changelog documents the changes between release versions. ## [Unreleased] +- Accept predicate arguments in native mutations and native queries ([#92](https://github.com/hasura/ndc-mongodb/pull/92)) ## [1.0.0] - 2024-07-09 From d9042e94e408f82f76bee35d97884fbe8ad97c25 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Fri, 2 Aug 2024 14:02:55 -0700 Subject: [PATCH 20/20] remove commented-out code --- .../src/plan_for_query_request/query_plan_state.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs index 111411dd..d82e5183 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/query_plan_state.rs @@ -170,7 +170,6 @@ impl QueryPlanState<'_, T> { variable_name: &ndc::VariableName, expected_type: Type, ) { - // self.register_variable_use_helper(variable_name, Some(expected_type)) let mut type_map = self.variable_types.borrow_mut(); match type_map.get_mut(variable_name) { None => {