From aa1ad79e27612b8c51e50b76d67ee934c0c6b1ab Mon Sep 17 00:00:00 2001 From: Flavian Desverne Date: Tue, 20 Sep 2022 16:00:58 +0200 Subject: [PATCH 1/2] fix: order by self-relation --- .../order_by_dependent.rs | 107 ++++++ .../sql-query-connector/src/join_utils.rs | 20 +- .../sql-query-connector/src/ordering.rs | 310 +++++++++--------- .../src/query_builder/read.rs | 6 +- 4 files changed, 276 insertions(+), 167 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs index 4bbeaf436575..3c50094840e8 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs @@ -452,4 +452,111 @@ mod order_by_dependent { Ok(()) } + + fn schema_self_rel() -> String { + let schema = indoc! { + r#"model Parent { + #id(id, Int, @id) + + resource Resource @relation("Resource", fields: [resourceId], references: [id]) + resourceId Int @unique + } + + model Resource { + #id(id, Int, @id) + + dependsOnId Int? + dependsOn Resource? @relation("DependsOn", fields: [dependsOnId], references: [id]) + + dependedOn Resource[] @relation("DependsOn") + parent Parent? @relation("Resource") + } + "# + }; + + schema.to_owned() + } + + // Regression test for: https://github.com/prisma/prisma/issues/12003 + #[connector_test(schema(schema_self_rel))] + async fn self_relation_works(runner: Runner) -> TestResult<()> { + run_query!( + &runner, + r#"mutation { + createOneParent( + data: { + id: 1 + resource: { + create: { + id: 1 + dependsOn: { create: { id: 2, dependsOn: { create: { id: 3 } } } } + } + } + } + ) { + id + } + } + "# + ); + run_query!( + &runner, + r#"mutation { + createOneParent( + data: { + id: 2 + resource: { + create: { + id: 4 + dependsOn: { create: { id: 5, dependsOn: { create: { id: 6 } } } } + } + } + } + ) { + id + } + } + "# + ); + + insta::assert_snapshot!( + run_query!(&runner, r#"{ + findManyParent(orderBy: { resource: { dependsOn: { id: asc } } }) { + id + resource { dependsOn { id } } + } + }"#), + @r###"{"data":{"findManyParent":[{"id":1,"resource":{"dependsOn":{"id":2}}},{"id":2,"resource":{"dependsOn":{"id":5}}}]}}"### + ); + insta::assert_snapshot!( + run_query!(&runner, r#"{ + findManyParent(orderBy: { resource: { dependsOn: { id: desc } } }) { + id + resource { dependsOn { id } } + } + }"#), + @r###"{"data":{"findManyParent":[{"id":2,"resource":{"dependsOn":{"id":5}}},{"id":1,"resource":{"dependsOn":{"id":2}}}]}}"### + ); + + insta::assert_snapshot!( + run_query!(&runner, r#"{ + findManyParent(orderBy: { resource: { dependsOn: { dependsOn: { id: asc } } } }) { + id + resource { dependsOn { dependsOn { id } } } + } + }"#), + @r###"{"data":{"findManyParent":[{"id":1,"resource":{"dependsOn":{"dependsOn":{"id":3}}}},{"id":2,"resource":{"dependsOn":{"dependsOn":{"id":6}}}}]}}"### + ); + insta::assert_snapshot!( + run_query!(&runner, r#"{ + findManyParent(orderBy: { resource: { dependsOn: { dependsOn: { id: desc } } } }) { + id + resource { dependsOn { dependsOn { id } } } + } + }"#), + @r###"{"data":{"findManyParent":[{"id":2,"resource":{"dependsOn":{"dependsOn":{"id":6}}}},{"id":1,"resource":{"dependsOn":{"dependsOn":{"id":3}}}}]}}"### + ); + + Ok(()) + } } diff --git a/query-engine/connectors/sql-query-connector/src/join_utils.rs b/query-engine/connectors/sql-query-connector/src/join_utils.rs index 3b2f33efc3f0..2e939c3fca50 100644 --- a/query-engine/connectors/sql-query-connector/src/join_utils.rs +++ b/query-engine/connectors/sql-query-connector/src/join_utils.rs @@ -216,7 +216,11 @@ fn compute_aggr_join_m2m( } } -pub fn compute_one2m_join(base_model: &ModelRef, rf: &RelationFieldRef, join_prefix: &str) -> AliasedJoin { +pub fn compute_one2m_join( + rf: &RelationFieldRef, + join_prefix: &str, + previous_join: Option<&AliasedJoin>, +) -> AliasedJoin { let (left_fields, right_fields) = if rf.is_inlined_on_enclosing_model() { (rf.scalar_fields(), rf.referenced_fields()) } else { @@ -226,13 +230,6 @@ pub fn compute_one2m_join(base_model: &ModelRef, rf: &RelationFieldRef, join_pre ) }; - // `rf` is always the relation field on the left model in the join (parent). - let left_table_alias = if rf.model().name != base_model.name { - Some(format!("{}_{}", join_prefix, &rf.model().name)) - } else { - None - }; - let right_table_alias = format!("{}_{}", join_prefix, &rf.related_model().name); let related_model = rf.related_model(); @@ -240,10 +237,9 @@ pub fn compute_one2m_join(base_model: &ModelRef, rf: &RelationFieldRef, join_pre let on_conditions: Vec = pairs .map(|(a, b)| { - let a_col = if let Some(alias) = left_table_alias.clone() { - Column::from((alias, a.db_name().to_owned())) - } else { - a.as_column() + let a_col = match previous_join { + Some(prev_join) => Column::from((prev_join.alias.to_owned(), a.db_name().to_owned())), + None => a.as_column(), }; let b_col = Column::from((right_table_alias.clone(), b.db_name().to_owned())); diff --git a/query-engine/connectors/sql-query-connector/src/ordering.rs b/query-engine/connectors/sql-query-connector/src/ordering.rs index a21be0b1d171..c8b672414270 100644 --- a/query-engine/connectors/sql-query-connector/src/ordering.rs +++ b/query-engine/connectors/sql-query-connector/src/ordering.rs @@ -17,172 +17,178 @@ pub struct OrderByDefinition { pub(crate) joins: Vec, } -/// Builds all expressions for an `ORDER BY` clause based on the query arguments. -pub fn build( - query_arguments: &QueryArguments, - base_model: &ModelRef, // The model the ordering will start from -) -> Vec { - let needs_reversed_order = query_arguments.needs_reversed_order(); - - // The index is used to differentiate potentially separate relations to the same model. - query_arguments - .order_by - .iter() - .enumerate() - .map(|(index, order_by)| match order_by { - OrderBy::Scalar(order_by) => build_order_scalar(order_by, base_model, needs_reversed_order, index), - OrderBy::ScalarAggregation(order_by) => build_order_aggr_scalar(order_by, needs_reversed_order), - OrderBy::ToManyAggregation(order_by) => { - build_order_aggr_rel(order_by, base_model, needs_reversed_order, index) - } - OrderBy::Relevance(order_by) => build_order_relevance(order_by, needs_reversed_order), - }) - .collect_vec() +#[derive(Debug, Default)] +pub struct OrderByBuilder { + // Used to generate unique join alias + join_counter: usize, } -fn build_order_scalar( - order_by: &OrderByScalar, - base_model: &ModelRef, - needs_reversed_order: bool, - index: usize, -) -> OrderByDefinition { - let (joins, order_column) = compute_joins_scalar(order_by, index, base_model); - let order: Option = Some(into_order( - &order_by.sort_order, - order_by.nulls_order.as_ref(), - needs_reversed_order, - )); - let order_definition: OrderDefinition = (order_column.clone().into(), order); - - OrderByDefinition { - order_column: order_column.into(), - order_definition, - joins, +impl OrderByBuilder { + /// Builds all expressions for an `ORDER BY` clause based on the query arguments. + pub fn build(&mut self, query_arguments: &QueryArguments) -> Vec { + let needs_reversed_order = query_arguments.needs_reversed_order(); + + // The index is used to differentiate potentially separate relations to the same model. + query_arguments + .order_by + .iter() + .map(|order_by| match order_by { + OrderBy::Scalar(order_by) => self.build_order_scalar(order_by, needs_reversed_order), + OrderBy::ScalarAggregation(order_by) => self.build_order_aggr_scalar(order_by, needs_reversed_order), + OrderBy::ToManyAggregation(order_by) => self.build_order_aggr_rel(order_by, needs_reversed_order), + OrderBy::Relevance(order_by) => self.build_order_relevance(order_by, needs_reversed_order), + }) + .collect_vec() + } + + fn build_order_scalar(&mut self, order_by: &OrderByScalar, needs_reversed_order: bool) -> OrderByDefinition { + let (joins, order_column) = self.compute_joins_scalar(order_by); + let order: Option = Some(into_order( + &order_by.sort_order, + order_by.nulls_order.as_ref(), + needs_reversed_order, + )); + let order_definition: OrderDefinition = (order_column.clone().into(), order); + + OrderByDefinition { + order_column: order_column.into(), + order_definition, + joins, + } } -} -fn build_order_relevance(order_by: &OrderByRelevance, needs_reversed_order: bool) -> OrderByDefinition { - let columns: Vec = order_by.fields.iter().map(|sf| sf.as_column().into()).collect(); - let order_column: Expression = text_search_relevance(&columns, order_by.search.clone()).into(); - let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); - let order_definition: OrderDefinition = (order_column.clone(), order); + fn build_order_relevance(&mut self, order_by: &OrderByRelevance, needs_reversed_order: bool) -> OrderByDefinition { + let columns: Vec = order_by.fields.iter().map(|sf| sf.as_column().into()).collect(); + let order_column: Expression = text_search_relevance(&columns, order_by.search.clone()).into(); + let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); + let order_definition: OrderDefinition = (order_column.clone(), order); - OrderByDefinition { - order_column, - order_definition, - joins: vec![], + OrderByDefinition { + order_column, + order_definition, + joins: vec![], + } } -} -fn build_order_aggr_scalar(order_by: &OrderByScalarAggregation, needs_reversed_order: bool) -> OrderByDefinition { - let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); - let order_column = order_by.field.as_column(); - let order_definition: OrderDefinition = match order_by.sort_aggregation { - SortAggregation::Count => (count(order_column.clone()).into(), order), - SortAggregation::Avg => (avg(order_column.clone()).into(), order), - SortAggregation::Sum => (sum(order_column.clone()).into(), order), - SortAggregation::Min => (min(order_column.clone()).into(), order), - SortAggregation::Max => (max(order_column.clone()).into(), order), - }; - - OrderByDefinition { - order_column: order_column.into(), - order_definition, - joins: vec![], + fn build_order_aggr_scalar( + &mut self, + order_by: &OrderByScalarAggregation, + needs_reversed_order: bool, + ) -> OrderByDefinition { + let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); + let order_column = order_by.field.as_column(); + let order_definition: OrderDefinition = match order_by.sort_aggregation { + SortAggregation::Count => (count(order_column.clone()).into(), order), + SortAggregation::Avg => (avg(order_column.clone()).into(), order), + SortAggregation::Sum => (sum(order_column.clone()).into(), order), + SortAggregation::Min => (min(order_column.clone()).into(), order), + SortAggregation::Max => (max(order_column.clone()).into(), order), + }; + + OrderByDefinition { + order_column: order_column.into(), + order_definition, + joins: vec![], + } } -} -fn build_order_aggr_rel( - order_by: &OrderByToManyAggregation, - base_model: &ModelRef, - needs_reversed_order: bool, - index: usize, -) -> OrderByDefinition { - let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); - let (joins, order_column) = compute_joins_aggregation(order_by, index, base_model); - let order_definition: OrderDefinition = match order_by.sort_aggregation { - SortAggregation::Count => { - let exprs: Vec = vec![order_column.clone().into(), Value::integer(0).into()]; - - // We coalesce the order by expr to 0 so that if there's no relation, - // `COALESCE(NULL, 0)` will return `0`, thus preserving the order - (coalesce(exprs).into(), order) + fn build_order_aggr_rel( + &mut self, + order_by: &OrderByToManyAggregation, + needs_reversed_order: bool, + ) -> OrderByDefinition { + let order: Option = Some(into_order(&order_by.sort_order, None, needs_reversed_order)); + let (joins, order_column) = self.compute_joins_aggregation(order_by); + let order_definition: OrderDefinition = match order_by.sort_aggregation { + SortAggregation::Count => { + let exprs: Vec = vec![order_column.clone().into(), Value::integer(0).into()]; + + // We coalesce the order by expr to 0 so that if there's no relation, + // `COALESCE(NULL, 0)` will return `0`, thus preserving the order + (coalesce(exprs).into(), order) + } + _ => unreachable!("Order by relation aggregation other than count are not supported"), + }; + + OrderByDefinition { + order_column: order_column.into(), + order_definition, + joins, + } + } + + fn compute_joins_aggregation( + &mut self, + order_by: &OrderByToManyAggregation, + ) -> (Vec, Column<'static>) { + let (last_hop, rest_hops) = order_by + .path + .split_last() + .expect("An order by relation aggregation has to have at least one hop"); + + // Unwraps are safe because the SQL connector doesn't yet support any other type of orderBy hop but the relation hop. + let mut joins = vec![]; + for (i, hop) in rest_hops.iter().enumerate() { + let previous_join = if i > 0 { joins.get(i - 1) } else { None }; + + let join = compute_one2m_join(hop.into_relation_hop().unwrap(), &self.join_prefix(), previous_join); + + joins.push(join); } - _ => unreachable!("Order by relation aggregation other than count are not supported"), - }; - OrderByDefinition { - order_column: order_column.into(), - order_definition, - joins, + let aggregation_type = match order_by.sort_aggregation { + SortAggregation::Count => AggregationType::Count, + _ => unreachable!("Order by relation aggregation other than count are not supported"), + }; + + // We perform the aggregation on the last join + let last_aggr_join = compute_aggr_join( + last_hop.into_relation_hop().unwrap(), + aggregation_type, + None, + ORDER_AGGREGATOR_ALIAS, + &self.join_prefix(), + joins.last(), + ); + + // This is the final column identifier to be used for the scalar field to order by. + // `{last_join_alias}.{ORDER_AGGREGATOR_ALIAS}` + let order_by_column = Column::from((last_aggr_join.alias.to_owned(), ORDER_AGGREGATOR_ALIAS.to_owned())); + + joins.push(last_aggr_join); + + (joins, order_by_column) } -} -pub fn compute_joins_aggregation( - order_by: &OrderByToManyAggregation, - order_by_index: usize, - base_model: &ModelRef, -) -> (Vec, Column<'static>) { - let join_prefix = format!("{}{}", ORDER_JOIN_PREFIX, order_by_index); - let (last_hop, rest_hops) = order_by - .path - .split_last() - .expect("An order by relation aggregation has to have at least one hop"); - - // Unwraps are safe because the SQL connector doesn't yet support any other type of orderBy hop but the relation hop. - let mut joins = rest_hops - .iter() - .map(|hop| compute_one2m_join(base_model, hop.into_relation_hop().unwrap(), join_prefix.as_str())) - .collect_vec(); - - let aggregation_type = match order_by.sort_aggregation { - SortAggregation::Count => AggregationType::Count, - _ => unreachable!("Order by relation aggregation other than count are not supported"), - }; - - // We perform the aggregation on the last join - let last_aggr_join = compute_aggr_join( - last_hop.into_relation_hop().unwrap(), - aggregation_type, - None, - ORDER_AGGREGATOR_ALIAS, - join_prefix.as_str(), - joins.last(), - ); - - // This is the final column identifier to be used for the scalar field to order by. - // `{last_join_alias}.{ORDER_AGGREGATOR_ALIAS}` - let order_by_column = Column::from((last_aggr_join.alias.to_owned(), ORDER_AGGREGATOR_ALIAS.to_owned())); - - joins.push(last_aggr_join); - - (joins, order_by_column) -} + pub fn compute_joins_scalar(&mut self, order_by: &OrderByScalar) -> (Vec, Column<'static>) { + let mut joins = vec![]; -pub fn compute_joins_scalar( - order_by: &OrderByScalar, - order_by_index: usize, - base_model: &ModelRef, -) -> (Vec, Column<'static>) { - let join_prefix = format!("{}{}", ORDER_JOIN_PREFIX, order_by_index); - let joins = order_by - .path - .iter() - .map(|hop| compute_one2m_join(base_model, hop.into_relation_hop().unwrap(), join_prefix.as_str())) - .collect_vec(); - - // This is the final column identifier to be used for the scalar field to order by. - // - If we order by a scalar field on the base model, we simply use the model's scalar field. eg: - // `{modelTable}.{field}` - // - If we order by some relations, we use the alias used for the last join, e.g. - // `{join_alias}.{field}` - let order_by_column = if let Some(last_join) = joins.last() { - Column::from((last_join.alias.to_owned(), order_by.field.db_name().to_owned())) - } else { - order_by.field.as_column() - }; - - (joins, order_by_column) + for (i, hop) in order_by.path.iter().enumerate() { + let previous_join = if i > 0 { joins.get(i - 1) } else { None }; + let join = compute_one2m_join(hop.into_relation_hop().unwrap(), &self.join_prefix(), previous_join); + + joins.push(join); + } + + // This is the final column identifier to be used for the scalar field to order by. + // - If we order by a scalar field on the base model, we simply use the model's scalar field. eg: + // `{modelTable}.{field}` + // - If we order by some relations, we use the alias used for the last join, e.g. + // `{join_alias}.{field}` + let order_by_column = if let Some(last_join) = joins.last() { + Column::from((last_join.alias.to_owned(), order_by.field.db_name().to_owned())) + } else { + order_by.field.as_column() + }; + + (joins, order_by_column) + } + + fn join_prefix(&mut self) -> String { + self.join_counter += 1; + + format!("{}{}", ORDER_JOIN_PREFIX, self.join_counter) + } } pub fn into_order(prisma_order: &SortOrder, nulls_order: Option<&NullsOrder>, reverse: bool) -> Order { diff --git a/query-engine/connectors/sql-query-connector/src/query_builder/read.rs b/query-engine/connectors/sql-query-connector/src/query_builder/read.rs index 2febe154c049..2a2fe48c93fb 100644 --- a/query-engine/connectors/sql-query-connector/src/query_builder/read.rs +++ b/query-engine/connectors/sql-query-connector/src/query_builder/read.rs @@ -1,6 +1,6 @@ use crate::{ - cursor_condition, filter_conversion::AliasedCondition, model_extensions::*, nested_aggregations, ordering, - sql_trace::SqlTraceComment, + cursor_condition, filter_conversion::AliasedCondition, model_extensions::*, nested_aggregations, + ordering::OrderByBuilder, sql_trace::SqlTraceComment, }; use connector_interface::{filter::Filter, AggregationSelection, QueryArguments, RelAggregationSelection}; use itertools::Itertools; @@ -58,7 +58,7 @@ impl SelectDefinition for QueryArguments { aggr_selections: &[RelAggregationSelection], trace_id: Option, ) -> (Select<'static>, Vec>) { - let order_by_definitions = ordering::build(&self, &model); + let order_by_definitions = OrderByBuilder::default().build(&self); let (table_opt, cursor_condition) = cursor_condition::build(&self, &model, &order_by_definitions); let aggregation_joins = nested_aggregations::build(aggr_selections); From 51b54daf8d572269b91bdde0d9b03aa2c8684d52 Mon Sep 17 00:00:00 2001 From: Flavian Desverne Date: Tue, 20 Sep 2022 17:41:21 +0200 Subject: [PATCH 2/2] fix test for mongo & mssql --- .../tests/queries/order_and_pagination/order_by_dependent.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs index 3c50094840e8..4c456dd4a72d 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/order_and_pagination/order_by_dependent.rs @@ -458,7 +458,7 @@ mod order_by_dependent { r#"model Parent { #id(id, Int, @id) - resource Resource @relation("Resource", fields: [resourceId], references: [id]) + resource Resource @relation("Resource", fields: [resourceId], references: [id], onUpdate: NoAction, onDelete: NoAction) resourceId Int @unique } @@ -466,7 +466,7 @@ mod order_by_dependent { #id(id, Int, @id) dependsOnId Int? - dependsOn Resource? @relation("DependsOn", fields: [dependsOnId], references: [id]) + dependsOn Resource? @relation("DependsOn", fields: [dependsOnId], references: [id], onUpdate: NoAction, onDelete: NoAction) dependedOn Resource[] @relation("DependsOn") parent Parent? @relation("Resource")