From 8071b1119484c48a2bb5ba1383d18cbdf7a6ab45 Mon Sep 17 00:00:00 2001 From: Gil Mizrahi Date: Fri, 23 Feb 2024 10:29:23 +0200 Subject: [PATCH] fix relationship collection of filter and order by clauses (#323) This fixes a bug with collection_relationships not being populated by relationships constructed from where and/or order by clauses. We fix this by not only collecting relationships from field selection, but also considering relationships introduced by order by and where clauses. V3_GIT_ORIGIN_REV_ID: c612ceed8b3831257ca2c7d23ec9ed23261efedf --- v3/engine/src/execute/query_plan.rs | 1 + v3/engine/src/execute/query_plan/commands.rs | 4 +- .../src/execute/query_plan/model_selection.rs | 4 +- .../src/execute/query_plan/relationships.rs | 70 +++++++++++++++++++ .../src/execute/query_plan/selection_set.rs | 7 +- .../relationships/object/simple/expected.json | 22 ++++++ .../relationships/object/simple/request.gql | 3 + .../relationships/object/simple/expected.json | 22 ++++++ .../relationships/object/simple/request.gql | 5 +- 9 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 v3/engine/src/execute/query_plan/relationships.rs diff --git a/v3/engine/src/execute/query_plan.rs b/v3/engine/src/execute/query_plan.rs index b38e9ac8b12..3f91a50cada 100644 --- a/v3/engine/src/execute/query_plan.rs +++ b/v3/engine/src/execute/query_plan.rs @@ -1,5 +1,6 @@ mod commands; mod model_selection; +mod relationships; pub(crate) mod selection_set; use gql::normalized_ast; diff --git a/v3/engine/src/execute/query_plan/commands.rs b/v3/engine/src/execute/query_plan/commands.rs index 83c24af507a..62ef95dc355 100644 --- a/v3/engine/src/execute/query_plan/commands.rs +++ b/v3/engine/src/execute/query_plan/commands.rs @@ -61,7 +61,7 @@ pub(crate) fn ndc_query_ir<'s, 'ir>( let (query, jl) = ndc_query(&ir.command_info, join_id_counter)?; let mut collection_relationships = BTreeMap::new(); - selection_set::collect_relationships( + selection_set::collect_relationships_from_selection( &ir.command_info.selection, &mut collection_relationships, )?; @@ -96,7 +96,7 @@ pub(crate) fn ndc_mutation_ir<'s, 'ir>( )), }; let mut collection_relationships = BTreeMap::new(); - selection_set::collect_relationships( + selection_set::collect_relationships_from_selection( &ir.command_info.selection, &mut collection_relationships, )?; diff --git a/v3/engine/src/execute/query_plan/model_selection.rs b/v3/engine/src/execute/query_plan/model_selection.rs index 0e6491f5118..ea87cb075ce 100644 --- a/v3/engine/src/execute/query_plan/model_selection.rs +++ b/v3/engine/src/execute/query_plan/model_selection.rs @@ -3,6 +3,7 @@ use ndc_client as ndc; use std::collections::BTreeMap; +use super::relationships; use super::selection_set; use crate::execute::error; use crate::execute::ir::model_selection::ModelSelection; @@ -43,7 +44,8 @@ pub(crate) fn ndc_ir<'s, 'ir>( error::Error, > { let mut collection_relationships = BTreeMap::new(); - selection_set::collect_relationships(&ir.selection, &mut collection_relationships)?; + relationships::collect_relationships(ir, &mut collection_relationships)?; + let (query, join_locations) = ndc_query(ir, join_id_counter)?; let query_request = ndc::models::QueryRequest { query, diff --git a/v3/engine/src/execute/query_plan/relationships.rs b/v3/engine/src/execute/query_plan/relationships.rs new file mode 100644 index 00000000000..2da02af21ed --- /dev/null +++ b/v3/engine/src/execute/query_plan/relationships.rs @@ -0,0 +1,70 @@ +//! NDC query generation from 'ModelSelection' IR for relationships. + +use ndc_client as ndc; +use std::collections::BTreeMap; + +use super::selection_set; +use crate::execute::error; +use crate::execute::ir::model_selection::ModelSelection; +use crate::execute::ir::relationship; +use crate::execute::ir::selection_set::FieldSelection; + +/// collect relationships recursively from IR components containing relationships, +/// and create NDC relationship definitions which will be added to the `relationships` +/// variable. +pub(crate) fn collect_relationships( + ir: &ModelSelection<'_>, + relationships: &mut BTreeMap, +) -> Result<(), error::Error> { + // from selection fields + for field in ir.selection.fields.values() { + match field { + FieldSelection::Column { .. } => (), + FieldSelection::ModelRelationshipLocal { + query, + name, + relationship_info, + } => { + relationships.insert( + name.to_string(), + relationship::process_model_relationship_definition(relationship_info)?, + ); + collect_relationships(query, relationships)?; + } + FieldSelection::CommandRelationshipLocal { + ir, + name, + relationship_info, + } => { + relationships.insert( + name.to_string(), + relationship::process_command_relationship_definition(relationship_info)?, + ); + selection_set::collect_relationships_from_selection( + &ir.command_info.selection, + relationships, + )?; + } + // we ignore remote relationships as we are generating relationship + // definition for one data connector + FieldSelection::ModelRelationshipRemote { .. } => (), + FieldSelection::CommandRelationshipRemote { .. } => (), + }; + } + + // from filter clause + for (name, relationship) in ir.filter_clause.relationships.iter() { + let result = relationship::process_model_relationship_definition(relationship)?; + relationships.insert(name.to_string(), result); + } + + // from order by clause + if let Some(order_by) = &ir.order_by { + for (name, relationship) in order_by.relationships.iter() { + let result = relationship::process_model_relationship_definition(relationship)?; + relationships.insert(name.to_string(), result); + } + }; + + Ok(()) +} diff --git a/v3/engine/src/execute/query_plan/selection_set.rs b/v3/engine/src/execute/query_plan/selection_set.rs index 6030345a1fb..c50d1d06c52 100644 --- a/v3/engine/src/execute/query_plan/selection_set.rs +++ b/v3/engine/src/execute/query_plan/selection_set.rs @@ -4,6 +4,7 @@ use std::collections::{BTreeMap, HashMap}; use super::commands; use super::model_selection; +use super::relationships; use super::ProcessResponseAs; use crate::execute::error; use crate::execute::ir::relationship; @@ -202,7 +203,7 @@ fn make_hasura_phantom_field(field_name: &str) -> String { /// From the fields in `ResultSelectionSet`, collect relationships recursively /// and create NDC relationship definitions -pub(crate) fn collect_relationships( +pub(crate) fn collect_relationships_from_selection( selection: &ResultSelectionSet, relationships: &mut BTreeMap, ) -> Result<(), error::Error> { @@ -218,7 +219,7 @@ pub(crate) fn collect_relationships( name.to_string(), relationship::process_model_relationship_definition(relationship_info)?, ); - collect_relationships(&query.selection, relationships)?; + relationships::collect_relationships(query, relationships)?; } FieldSelection::CommandRelationshipLocal { ir, @@ -229,7 +230,7 @@ pub(crate) fn collect_relationships( name.to_string(), relationship::process_command_relationship_definition(relationship_info)?, ); - collect_relationships(&ir.command_info.selection, relationships)?; + collect_relationships_from_selection(&ir.command_info.selection, relationships)?; } // we ignore remote relationships as we are generating relationship // definition for one data connector diff --git a/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/expected.json b/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/expected.json index 2344503e6ac..fff936fedc3 100644 --- a/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/expected.json +++ b/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/expected.json @@ -21,6 +21,17 @@ } } ], + "TrackOrderByWithoutRelationshipField": [ + { + "Name": "Koyaanisqatsi" + }, + { + "Name": "Quintet for Horn, Violin, 2 Violas, and Cello in E Flat Major, K. 407/386c: III. Allegro" + }, + { + "Name": "L'orfeo, Act 3, Sinfonia (Orchestra)" + } + ], "TrackOrderByWithFilter": [ { "TrackId": 5, @@ -83,6 +94,17 @@ } } ], + "TrackOrderByWithoutRelationshipField": [ + { + "Name": "Koyaanisqatsi" + }, + { + "Name": "Quintet for Horn, Violin, 2 Violas, and Cello in E Flat Major, K. 407/386c: III. Allegro" + }, + { + "Name": "L'orfeo, Act 3, Sinfonia (Orchestra)" + } + ], "TrackOrderByWithFilter": [ { "TrackId": 5, diff --git a/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/request.gql b/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/request.gql index 8c8a23fa1a2..faac555e5af 100644 --- a/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/request.gql +++ b/v3/engine/tests/execute/models/select_many/order_by/relationships/object/simple/request.gql @@ -5,6 +5,9 @@ query MyQuery { Title } } + TrackOrderByWithoutRelationshipField: Track(order_by: [{Album: {ArtistId: Desc}}, {Name: Asc}], limit: 3) { + Name + } TrackOrderByWithFilter: Track( order_by: [{Album: {ArtistId: Asc}}, {TrackId: Desc}] where: {Album: {Artist: {ArtistId: {_eq: 2}}}} diff --git a/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/expected.json b/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/expected.json index cb0d584f1e2..173c2a5bbfb 100644 --- a/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/expected.json +++ b/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/expected.json @@ -24,6 +24,17 @@ } } ], + "TrackWithoutRelationshipField": [ + { + "Name": "Fast As a Shark" + }, + { + "Name": "Restless and Wild" + }, + { + "Name": "Princess of the Dawn" + } + ], "TrackAnd": [ { "AlbumId": 3, @@ -104,6 +115,17 @@ } } ], + "TrackWithoutRelationshipField": [ + { + "Name": "Fast As a Shark" + }, + { + "Name": "Restless and Wild" + }, + { + "Name": "Princess of the Dawn" + } + ], "TrackAnd": [ { "AlbumId": 3, diff --git a/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/request.gql b/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/request.gql index 6f998b08c3b..28d6a8db6f8 100644 --- a/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/request.gql +++ b/v3/engine/tests/execute/models/select_many/where/relationships/object/simple/request.gql @@ -6,6 +6,9 @@ query MyQuery { Title } } + TrackWithoutRelationshipField: Track(where: {Album: {Title: {_eq: "Restless and Wild"}}}) { + Name + } TrackAnd: Track(where: {_and: [{Album: {Title: {_eq: "Restless and Wild"}}}, {AlbumId: {_eq: 3}} ]} ) { AlbumId Name @@ -20,4 +23,4 @@ query MyQuery { Title } } -} \ No newline at end of file +}