diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b8dd8c6..b6efc455 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ This changelog documents the changes between release versions. ## [Unreleased] - Support filtering and sorting by fields of related collections ([#72](https://github.com/hasura/ndc-mongodb/pull/72)) +- Support for root collection column references ([#75](https://github.com/hasura/ndc-mongodb/pull/75)) +- Fix for databases with field names that begin with a dollar sign, or that contain dots ([#74](https://github.com/hasura/ndc-mongodb/pull/74)) +- Implement column-to-column comparisons within the same collection ([#74](https://github.com/hasura/ndc-mongodb/pull/74)) ## [0.0.6] - 2024-05-01 - Enables logging events from the MongoDB driver by setting the `RUST_LOG` variable ([#67](https://github.com/hasura/ndc-mongodb/pull/67)) diff --git a/crates/integration-tests/src/graphql.rs b/crates/integration-tests/src/graphql.rs index d027b056..9e2ba1e8 100644 --- a/crates/integration-tests/src/graphql.rs +++ b/crates/integration-tests/src/graphql.rs @@ -1,3 +1,5 @@ +use std::collections::BTreeMap; + use reqwest::Client; use serde::{Deserialize, Serialize}; use serde_json::{to_value, Value}; @@ -12,6 +14,8 @@ pub struct GraphQLRequest { operation_name: Option, #[serde(skip_serializing_if = "Option::is_none")] variables: Option, + #[serde(skip_serializing)] + headers: BTreeMap, } impl GraphQLRequest { @@ -20,6 +24,7 @@ impl GraphQLRequest { query, operation_name: Default::default(), variables: Default::default(), + headers: [("x-hasura-role".into(), "admin".into())].into(), } } @@ -33,15 +38,25 @@ impl GraphQLRequest { self } + pub fn headers( + mut self, + headers: impl IntoIterator, + ) -> Self { + self.headers = headers + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect(); + self + } + pub async fn run(&self) -> anyhow::Result { let graphql_url = get_graphql_url()?; let client = Client::new(); - let response = client - .post(graphql_url) - .header("x-hasura-role", "admin") - .json(self) - .send() - .await?; + let mut request_builder = client.post(graphql_url).json(self); + for (key, value) in self.headers.iter() { + request_builder = request_builder.header(key, value); + } + let response = request_builder.send().await?; let graphql_response = response.json().await?; Ok(graphql_response) } diff --git a/crates/integration-tests/src/tests/mod.rs b/crates/integration-tests/src/tests/mod.rs index 74271150..1d008adf 100644 --- a/crates/integration-tests/src/tests/mod.rs +++ b/crates/integration-tests/src/tests/mod.rs @@ -11,4 +11,5 @@ mod basic; mod local_relationship; mod native_mutation; mod native_query; +mod permissions; mod remote_relationship; diff --git a/crates/integration-tests/src/tests/permissions.rs b/crates/integration-tests/src/tests/permissions.rs new file mode 100644 index 00000000..a807e390 --- /dev/null +++ b/crates/integration-tests/src/tests/permissions.rs @@ -0,0 +1,36 @@ +use crate::graphql_query; +use insta::assert_yaml_snapshot; + +#[tokio::test] +async fn filters_results_according_to_configured_permissions() -> anyhow::Result<()> { + assert_yaml_snapshot!( + graphql_query( + r#" + query { + users(order_by: {id: Asc}) { + id + name + email + comments(limit: 5, order_by: {id: Asc}) { + date + email + text + } + } + comments(limit: 5, order_by: {id: Asc}) { + date + email + text + } + } + "# + ) + .headers([ + ("x-hasura-role", "user"), + ("x-hasura-user-id", "59b99db4cfa9a34dcd7885b6"), + ]) + .run() + .await? + ); + Ok(()) +} diff --git a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__permissions__filters_results_according_to_configured_permissions.snap b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__permissions__filters_results_according_to_configured_permissions.snap new file mode 100644 index 00000000..d990e06c --- /dev/null +++ b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__permissions__filters_results_according_to_configured_permissions.snap @@ -0,0 +1,42 @@ +--- +source: crates/integration-tests/src/tests/permissions.rs +expression: "graphql_query(r#\"\n query {\n users(limit: 5) {\n id\n name\n email\n comments(limit: 5) {\n date\n email\n text\n }\n }\n comments(limit: 5) {\n date\n email\n text\n }\n }\n \"#).headers([(\"x-hasura-role\",\n \"user\"),\n (\"x-hasura-user-id\",\n \"59b99db4cfa9a34dcd7885b6\")]).run().await?" +--- +data: + users: + - id: 59b99db4cfa9a34dcd7885b6 + name: Ned Stark + email: sean_bean@gameofthron.es + comments: + - date: "2000-01-21T03:17:04.000000000Z" + email: sean_bean@gameofthron.es + text: Illo nostrum enim sequi doloremque dolore saepe beatae. Iusto alias odit quaerat id dolores. Dolore quaerat accusantium esse voluptatibus. Aspernatur fuga exercitationem explicabo. + - date: "2005-09-24T16:22:38.000000000Z" + email: sean_bean@gameofthron.es + text: Architecto eos eum iste facilis. Sunt aperiam fugit nihil quas. + - date: "1978-10-22T23:49:33.000000000Z" + email: sean_bean@gameofthron.es + text: Aspernatur ullam blanditiis qui dolorum. Magnam minima suscipit esse. Laudantium voluptates incidunt quia saepe. + - date: "2013-08-15T07:24:54.000000000Z" + email: sean_bean@gameofthron.es + text: Ullam error officiis incidunt praesentium debitis. Rerum repudiandae illum reprehenderit aut non. Iusto eum autem veniam eveniet temporibus sed. Accusamus sint sed veritatis eaque. + - date: "2004-12-22T12:53:43.000000000Z" + email: sean_bean@gameofthron.es + text: Ducimus sunt neque sint nesciunt quis vero. Debitis ex non asperiores voluptatem iusto possimus. Doloremque blanditiis consequuntur explicabo placeat commodi repudiandae. + comments: + - date: "2000-01-21T03:17:04.000000000Z" + email: sean_bean@gameofthron.es + text: Illo nostrum enim sequi doloremque dolore saepe beatae. Iusto alias odit quaerat id dolores. Dolore quaerat accusantium esse voluptatibus. Aspernatur fuga exercitationem explicabo. + - date: "2005-09-24T16:22:38.000000000Z" + email: sean_bean@gameofthron.es + text: Architecto eos eum iste facilis. Sunt aperiam fugit nihil quas. + - date: "1978-10-22T23:49:33.000000000Z" + email: sean_bean@gameofthron.es + text: Aspernatur ullam blanditiis qui dolorum. Magnam minima suscipit esse. Laudantium voluptates incidunt quia saepe. + - date: "2013-08-15T07:24:54.000000000Z" + email: sean_bean@gameofthron.es + text: Ullam error officiis incidunt praesentium debitis. Rerum repudiandae illum reprehenderit aut non. Iusto eum autem veniam eveniet temporibus sed. Accusamus sint sed veritatis eaque. + - date: "2004-12-22T12:53:43.000000000Z" + email: sean_bean@gameofthron.es + text: Ducimus sunt neque sint nesciunt quis vero. Debitis ex non asperiores voluptatem iusto possimus. Doloremque blanditiis consequuntur explicabo placeat commodi repudiandae. +errors: ~ diff --git a/crates/mongodb-agent-common/src/mongodb/selection.rs b/crates/mongodb-agent-common/src/mongodb/selection.rs index 56edff9a..cc7d7721 100644 --- a/crates/mongodb-agent-common/src/mongodb/selection.rs +++ b/crates/mongodb-agent-common/src/mongodb/selection.rs @@ -323,6 +323,10 @@ mod tests { let query_plan = plan_for_query_request(&students_config(), query_request)?; + // TODO: MDB-164 This selection illustrates that we end up looking up the relationship + // twice (once with the key `class_students`, and then with the key `class_students_0`). + // This is because the queries on the two relationships have different scope names. The + // query would work with just one lookup. Can we do that optimization? let selection = Selection::from_query_request(&query_plan)?; assert_eq!( Into::::into(selection), @@ -340,7 +344,7 @@ mod tests { "students": { "rows": { "$map": { - "input": { "$getField": { "$literal": "class_students" } }, + "input": { "$getField": { "$literal": "class_students_0" } }, "in": { "student_name": "$$this.student_name" }, diff --git a/crates/mongodb-agent-common/src/query/column_ref.rs b/crates/mongodb-agent-common/src/query/column_ref.rs index fd33829e..2a584724 100644 --- a/crates/mongodb-agent-common/src/query/column_ref.rs +++ b/crates/mongodb-agent-common/src/query/column_ref.rs @@ -1,6 +1,7 @@ use std::{borrow::Cow, iter::once}; use mongodb::bson::{doc, Bson}; +use ndc_query_plan::Scope; use crate::{mongo_query_plan::ComparisonTarget, mongodb::sanitize::is_name_safe}; @@ -49,13 +50,16 @@ fn from_target(column: &ComparisonTarget) -> ColumnRef<'_> { // one element, and we know it does because we start the iterable with `name` from_path(None, name_and_path).unwrap() } - ComparisonTarget::RootCollectionColumn { - name, field_path, .. + ComparisonTarget::ColumnInScope { + name, + field_path, + scope, + .. } => { // "$$ROOT" is not actually a valid match key, but cheating here makes the // implementation much simpler. This match branch produces a ColumnRef::Expression // in all cases. - let init = ColumnRef::MatchKey("$ROOT".into()); + let init = ColumnRef::MatchKey(format!("${}", name_from_scope(scope)).into()); // The None case won't come up if the input to [from_target_helper] has at least // one element, and we know it does because we start the iterable with `name` let col_ref = @@ -69,6 +73,13 @@ fn from_target(column: &ComparisonTarget) -> ColumnRef<'_> { } } +pub fn name_from_scope(scope: &Scope) -> Cow<'_, str> { + match scope { + Scope::Root => "scope_root".into(), + Scope::Named(name) => name.into(), + } +} + fn from_path<'a>( init: Option>, path: impl IntoIterator, @@ -140,6 +151,7 @@ mod tests { use configuration::MongoScalarType; use mongodb::bson::doc; use mongodb_support::BsonScalarType; + use ndc_query_plan::Scope; use pretty_assertions::assert_eq; use crate::mongo_query_plan::{ComparisonTarget, Type}; @@ -255,29 +267,31 @@ mod tests { #[test] fn produces_dot_separated_root_column_reference() -> anyhow::Result<()> { - let target = ComparisonTarget::RootCollectionColumn { + let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["prop1".into(), "prop2".into()]), column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); - let expected = ColumnRef::Expression("$$ROOT.field.prop1.prop2".into()); + let expected = ColumnRef::Expression("$$scope_root.field.prop1.prop2".into()); assert_eq!(actual, expected); Ok(()) } #[test] fn escapes_unsafe_field_name_in_root_column_reference() -> anyhow::Result<()> { - let target = ComparisonTarget::RootCollectionColumn { + let target = ComparisonTarget::ColumnInScope { name: "$field".into(), field_path: Default::default(), column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + scope: Scope::Named("scope_0".into()), }; let actual = ColumnRef::from_comparison_target(&target); let expected = ColumnRef::Expression( doc! { "$getField": { - "input": "$$ROOT", + "input": "$$scope_0", "field": { "$literal": "$field" }, } } @@ -289,16 +303,17 @@ mod tests { #[test] fn escapes_unsafe_nested_property_name_in_root_column_reference() -> anyhow::Result<()> { - let target = ComparisonTarget::RootCollectionColumn { + let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["$unsafe_name".into()]), column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); let expected = ColumnRef::Expression( doc! { "$getField": { - "input": "$$ROOT.field", + "input": "$$scope_root.field", "field": { "$literal": "$unsafe_name" }, } } @@ -311,10 +326,11 @@ mod tests { #[test] fn escapes_multiple_layers_of_nested_property_names_in_root_column_reference( ) -> anyhow::Result<()> { - let target = ComparisonTarget::RootCollectionColumn { + let target = ComparisonTarget::ColumnInScope { name: "$field".into(), field_path: Some(vec!["$unsafe_name1".into(), "$unsafe_name2".into()]), column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); let expected = ColumnRef::Expression( @@ -324,7 +340,7 @@ mod tests { "$getField": { "input": { "$getField": { - "input": "$$ROOT", + "input": "$$scope_root", "field": { "$literal": "$field" }, } }, @@ -342,16 +358,17 @@ mod tests { #[test] fn escapes_unsafe_deeply_nested_property_name_in_root_column_reference() -> anyhow::Result<()> { - let target = ComparisonTarget::RootCollectionColumn { + let target = ComparisonTarget::ColumnInScope { name: "field".into(), field_path: Some(vec!["prop1".into(), "$unsafe_name".into()]), column_type: Type::Scalar(MongoScalarType::Bson(BsonScalarType::String)), + scope: Scope::Root, }; let actual = ColumnRef::from_comparison_target(&target); let expected = ColumnRef::Expression( doc! { "$getField": { - "input": "$$ROOT.field.prop1", + "input": "$$scope_root.field.prop1", "field": { "$literal": "$unsafe_name" }, } } diff --git a/crates/mongodb-agent-common/src/query/make_selector.rs b/crates/mongodb-agent-common/src/query/make_selector.rs index 0aede460..416d4d31 100644 --- a/crates/mongodb-agent-common/src/query/make_selector.rs +++ b/crates/mongodb-agent-common/src/query/make_selector.rs @@ -172,14 +172,21 @@ fn variable_to_mongo_expression( #[cfg(test)] mod tests { use configuration::MongoScalarType; - use mongodb::bson::doc; + use mongodb::bson::{self, bson, doc}; use mongodb_support::BsonScalarType; use ndc_models::UnaryComparisonOperator; + use ndc_query_plan::plan_for_query_request; + use ndc_test_helpers::{ + binop, column_value, path_element, query, query_request, relation_field, root, target, + value, + }; use pretty_assertions::assert_eq; use crate::{ comparison_function::ComparisonFunction, mongo_query_plan::{ComparisonTarget, ComparisonValue, Expression, Type}, + query::pipeline_for_query_request, + test_helpers::{chinook_config, chinook_relationships}, }; use super::make_selector; @@ -284,4 +291,92 @@ mod tests { assert_eq!(selector, expected); Ok(()) } + + #[test] + fn root_column_reference_refereces_column_of_nearest_query() -> anyhow::Result<()> { + let request = query_request() + .collection("Artist") + .query( + query().fields([relation_field!("Albums" => "Albums", query().predicate( + binop( + "_gt", + target!("Milliseconds", relations: [ + path_element("Tracks").predicate( + binop("_eq", target!("Name"), column_value!(root("Title"))) + ), + ]), + value!(30_000), + ) + ))]), + ) + .relationships(chinook_relationships()) + .into(); + + let config = chinook_config(); + let plan = plan_for_query_request(&config, request)?; + let pipeline = pipeline_for_query_request(&config, &plan)?; + + let expected_pipeline = bson!([ + { + "$lookup": { + "from": "Album", + "localField": "ArtistId", + "foreignField": "ArtistId", + "as": "Albums", + "let": { + "scope_root": "$$ROOT", + }, + "pipeline": [ + { + "$lookup": { + "from": "Track", + "localField": "AlbumId", + "foreignField": "AlbumId", + "as": "Tracks", + "let": { + "scope_0": "$$ROOT", + }, + "pipeline": [ + { + "$match": { + "$expr": { "$eq": ["$Name", "$$scope_0.Title"] }, + }, + }, + { + "$replaceWith": { + "Milliseconds": { "$ifNull": ["$Milliseconds", null] } + } + }, + ] + } + }, + { + "$match": { + "Tracks": { + "$elemMatch": { + "Milliseconds": { "$gt": 30_000 } + } + } + } + }, + { + "$replaceWith": { + "Tracks": { "$getField": { "$literal": "Tracks" } } + } + }, + ], + }, + }, + { + "$replaceWith": { + "Albums": { + "rows": [] + } + } + }, + ]); + + assert_eq!(bson::to_bson(&pipeline).unwrap(), expected_pipeline); + Ok(()) + } } diff --git a/crates/mongodb-agent-common/src/query/relations.rs b/crates/mongodb-agent-common/src/query/relations.rs index 4b321a3c..c700a653 100644 --- a/crates/mongodb-agent-common/src/query/relations.rs +++ b/crates/mongodb-agent-common/src/query/relations.rs @@ -2,11 +2,12 @@ use std::collections::BTreeMap; use itertools::Itertools as _; use mongodb::bson::{doc, Bson, Document}; -use ndc_query_plan::VariableSet; +use ndc_query_plan::{Scope, VariableSet}; use crate::mongo_query_plan::{MongoConfiguration, Query, QueryPlan}; use crate::mongodb::sanitize::safe_name; use crate::mongodb::Pipeline; +use crate::query::column_ref::name_from_scope; use crate::{ interface_types::MongoAgentError, mongodb::{sanitize::variable, Stage}, @@ -25,7 +26,11 @@ pub fn pipeline_for_relations( query_plan: &QueryPlan, ) -> Result { let QueryPlan { query, .. } = query_plan; - let Query { relationships, .. } = query; + let Query { + relationships, + scope, + .. + } = query; // Lookup stages perform the join for each relationship, and assign the list of rows or mapping // of aggregate results to a field in the parent document. @@ -49,6 +54,7 @@ pub fn pipeline_for_relations( &relationship.column_mapping, name.to_owned(), lookup_pipeline, + scope.as_ref(), ) }) .try_collect()?; @@ -61,6 +67,7 @@ fn make_lookup_stage( column_mapping: &BTreeMap, r#as: String, lookup_pipeline: Pipeline, + scope: Option<&Scope>, ) -> Result { // If we are mapping a single field in the source collection to a single field in the target // collection then we can use the correlated subquery syntax. @@ -73,9 +80,10 @@ fn make_lookup_stage( target_selector, r#as, lookup_pipeline, + scope, ) } else { - multiple_column_mapping_lookup(from, column_mapping, r#as, lookup_pipeline) + multiple_column_mapping_lookup(from, column_mapping, r#as, lookup_pipeline, scope) } } @@ -86,12 +94,17 @@ fn single_column_mapping_lookup( target_selector: &str, r#as: String, lookup_pipeline: Pipeline, + scope: Option<&Scope>, ) -> Result { Ok(Stage::Lookup { from: Some(from), local_field: Some(safe_name(source_selector)?.into_owned()), foreign_field: Some(safe_name(target_selector)?.into_owned()), - r#let: None, + r#let: scope.map(|scope| { + doc! { + name_from_scope(scope): "$$ROOT" + } + }), pipeline: if lookup_pipeline.is_empty() { None } else { @@ -106,8 +119,9 @@ fn multiple_column_mapping_lookup( column_mapping: &BTreeMap, r#as: String, lookup_pipeline: Pipeline, + scope: Option<&Scope>, ) -> Result { - let let_bindings: Document = column_mapping + let mut let_bindings: Document = column_mapping .keys() .map(|local_field| { Ok(( @@ -117,6 +131,10 @@ fn multiple_column_mapping_lookup( }) .collect::>()?; + if let Some(scope) = scope { + let_bindings.insert(name_from_scope(scope), "$$ROOT"); + } + // Creating an intermediate Vec and sorting it is done just to help with testing. // A stable order for matchers makes it easier to assert equality between actual // and expected pipelines. @@ -208,6 +226,9 @@ mod tests { "from": "students", "localField": "_id", "foreignField": "classId", + "let": { + "scope_root": "$$ROOT", + }, "pipeline": [ { "$replaceWith": { @@ -294,6 +315,9 @@ mod tests { "from": "classes", "localField": "classId", "foreignField": "_id", + "let": { + "scope_root": "$$ROOT", + }, "pipeline": [ { "$replaceWith": { @@ -378,6 +402,7 @@ mod tests { "let": { "v_year": "$year", "v_title": "$title", + "scope_root": "$$ROOT", }, "pipeline": [ { @@ -484,12 +509,18 @@ mod tests { "from": "students", "localField": "_id", "foreignField": "class_id", + "let": { + "scope_root": "$$ROOT", + }, "pipeline": [ { "$lookup": { "from": "assignments", "localField": "_id", "foreignField": "student_id", + "let": { + "scope_0": "$$ROOT", + }, "pipeline": [ { "$replaceWith": { @@ -592,6 +623,9 @@ mod tests { "from": "students", "localField": "_id", "foreignField": "classId", + "let": { + "scope_root": "$$ROOT", + }, "pipeline": [ { "$facet": { @@ -703,6 +737,9 @@ mod tests { "from": "movies", "localField": "movie_id", "foreignField": "_id", + "let": { + "scope_root": "$$ROOT", + }, "pipeline": [ { "$replaceWith": { diff --git a/crates/mongodb-agent-common/src/test_helpers.rs b/crates/mongodb-agent-common/src/test_helpers.rs index 85f61788..d1058709 100644 --- a/crates/mongodb-agent-common/src/test_helpers.rs +++ b/crates/mongodb-agent-common/src/test_helpers.rs @@ -86,6 +86,79 @@ pub fn make_nested_schema() -> MongoConfiguration { }) } +/// Configuration for a MongoDB database with Chinook test data +pub fn chinook_config() -> MongoConfiguration { + MongoConfiguration(Configuration { + collections: [ + collection("Album"), + collection("Artist"), + collection("Genre"), + collection("Track"), + ] + .into(), + object_types: [ + ( + "Album".into(), + object_type([ + ("AlbumId", named_type("Int")), + ("ArtistId", named_type("Int")), + ("Title", named_type("String")), + ]), + ), + ( + "Artist".into(), + object_type([ + ("ArtistId", named_type("Int")), + ("Name", named_type("String")), + ]), + ), + ( + "Genre".into(), + object_type([ + ("GenreId", named_type("Int")), + ("Name", named_type("String")), + ]), + ), + ( + "Track".into(), + object_type([ + ("AlbumId", named_type("Int")), + ("GenreId", named_type("Int")), + ("TrackId", named_type("Int")), + ("Name", named_type("String")), + ("Milliseconds", named_type("Int")), + ]), + ), + ] + .into(), + functions: Default::default(), + procedures: Default::default(), + native_mutations: Default::default(), + native_queries: Default::default(), + options: Default::default(), + }) +} + +pub fn chinook_relationships() -> BTreeMap { + [ + ( + "Albums", + ndc_test_helpers::relationship("Album", [("ArtistId", "ArtistId")]), + ), + ( + "Tracks", + ndc_test_helpers::relationship("Track", [("AlbumId", "AlbumId")]), + ), + ( + "Genre", + ndc_test_helpers::relationship("Genre", [("GenreId", "GenreId")]).object_type(), + ), + ] + .into_iter() + .map(|(name, relationship_builder)| (name.to_string(), relationship_builder.into())) + .collect() +} + /// Configuration for a MongoDB database that resembles MongoDB's sample_mflix test data set. pub fn mflix_config() -> MongoConfiguration { MongoConfiguration(Configuration { diff --git a/crates/ndc-query-plan/src/lib.rs b/crates/ndc-query-plan/src/lib.rs index 032382cb..7ce74bd1 100644 --- a/crates/ndc-query-plan/src/lib.rs +++ b/crates/ndc-query-plan/src/lib.rs @@ -12,6 +12,6 @@ pub use query_plan::{ Aggregate, AggregateFunctionDefinition, ComparisonOperatorDefinition, ComparisonTarget, ComparisonValue, ConnectorTypes, ExistsInCollection, Expression, Field, NestedArray, NestedField, NestedObject, OrderBy, OrderByElement, OrderByTarget, Query, QueryPlan, - Relationship, Relationships, VariableSet, + Relationship, Relationships, Scope, VariableSet, }; 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 883fa0ba..65a68aca 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 @@ -12,7 +12,7 @@ mod tests; use std::collections::VecDeque; -use crate::{self as plan, type_annotated_field, ObjectType, QueryPlan}; +use crate::{self as plan, type_annotated_field, ObjectType, QueryPlan, Scope}; use indexmap::IndexMap; use itertools::Itertools; use ndc::{ExistsInCollection, QueryRequest}; @@ -34,12 +34,13 @@ pub fn plan_for_query_request( let mut plan_state = QueryPlanState::new(context, &request.collection_relationships); let collection_object_type = context.find_collection_object_type(&request.collection)?; - let query = plan_for_query( + let mut query = plan_for_query( &mut plan_state, &collection_object_type, &collection_object_type, request.query, )?; + query.scope = Some(Scope::Root); let unrelated_collections = plan_state.into_unrelated_collections(); @@ -52,6 +53,7 @@ pub fn plan_for_query_request( }) } +/// root_collection_object_type references the collection type of the nearest enclosing [ndc::Query] pub fn plan_for_query( plan_state: &mut QueryPlanState<'_, T>, root_collection_object_type: &plan::ObjectType, @@ -105,6 +107,7 @@ pub fn plan_for_query( offset, predicate, relationships: plan_state.into_relationships(), + scope: None, }) } @@ -511,10 +514,11 @@ fn plan_for_comparison_target( } ndc::ComparisonTarget::RootCollectionColumn { name } => { let column_type = find_object_field(root_collection_object_type, &name)?.clone(); - Ok(plan::ComparisonTarget::RootCollectionColumn { + Ok(plan::ComparisonTarget::ColumnInScope { name, field_path: Default::default(), // TODO: propagate this after ndc-spec update column_type, + scope: plan_state.scope.clone(), }) } } diff --git a/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/query.rs b/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/query.rs index 0f75a3b1..4bad3cac 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/query.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/plan_test_helpers/query.rs @@ -2,6 +2,7 @@ use indexmap::IndexMap; use crate::{ Aggregate, ConnectorTypes, Expression, Field, OrderBy, OrderByElement, Query, Relationships, + Scope, }; #[derive(Clone, Debug, Default)] @@ -14,6 +15,7 @@ pub struct QueryBuilder { order_by: Option>, predicate: Option>, relationships: Relationships, + scope: Option, } #[allow(dead_code)] @@ -32,6 +34,7 @@ impl QueryBuilder { order_by: None, predicate: None, relationships: Default::default(), + scope: None, } } @@ -72,6 +75,11 @@ impl QueryBuilder { self.predicate = Some(expression); self } + + pub fn scope(mut self, scope: Scope) -> Self { + self.scope = Some(scope); + self + } } impl From> for Query { @@ -85,6 +93,7 @@ impl From> for Query { order_by: value.order_by, predicate: value.predicate, relationships: value.relationships, + scope: value.scope, } } } 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 2d90ee6f..5ea76bb0 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 @@ -8,8 +8,9 @@ use ndc::RelationshipArgument; use ndc_models as ndc; use crate::{ - plan_for_query_request::helpers::lookup_relationship, query_plan::UnrelatedJoin, Query, - QueryContext, QueryPlanError, Relationship, + plan_for_query_request::helpers::lookup_relationship, + query_plan::{Scope, UnrelatedJoin}, + Query, QueryContext, QueryPlanError, Relationship, }; use super::unify_relationship_references::unify_relationship_references; @@ -26,15 +27,13 @@ type Result = std::result::Result; pub struct QueryPlanState<'a, T: QueryContext> { pub context: &'a T, pub collection_relationships: &'a BTreeMap, + pub scope: Scope, relationships: BTreeMap>, unrelated_joins: Rc>>>, - counter: Rc>, + relationship_name_counter: Rc>, + scope_name_counter: Rc>, } -// TODO: We may be able to unify relationships that are not identical, but that are compatible. -// For example two relationships that differ only in field selection could be merged into one -// with the union of both field selections. - impl QueryPlanState<'_, T> { pub fn new<'a>( query_context: &'a T, @@ -43,9 +42,11 @@ impl QueryPlanState<'_, T> { QueryPlanState { context: query_context, collection_relationships, + scope: Scope::Root, relationships: Default::default(), unrelated_joins: Rc::new(RefCell::new(Default::default())), - counter: Rc::new(Cell::new(0)), + relationship_name_counter: Rc::new(Cell::new(0)), + scope_name_counter: Rc::new(Cell::new(0)), } } @@ -56,12 +57,19 @@ impl QueryPlanState<'_, T> { QueryPlanState { context: self.context, collection_relationships: self.collection_relationships, + scope: self.scope.clone(), relationships: Default::default(), unrelated_joins: self.unrelated_joins.clone(), - counter: self.counter.clone(), + relationship_name_counter: self.relationship_name_counter.clone(), + scope_name_counter: self.scope_name_counter.clone(), } } + pub fn new_scope(&mut self) { + let name = self.unique_scope_name(); + self.scope = Scope::Named(name) + } + /// Record a relationship reference so that it is added to the list of joins for the query /// plan, and get back an identifier than can be used to access the joined collection. pub fn register_relationship( @@ -94,7 +102,7 @@ impl QueryPlanState<'_, T> { // relationship that we just removed. self.relationships .insert(existing_key, already_registered_relationship); - let key = self.unique_name(ndc_relationship_name); + let key = self.unique_relationship_name(ndc_relationship_name); (key, relationship) } } @@ -121,7 +129,7 @@ impl QueryPlanState<'_, T> { query, }; - let key = self.unique_name(format!("__join_{}", join.target_collection)); + let key = self.unique_relationship_name(format!("__join_{}", join.target_collection)); self.unrelated_joins.borrow_mut().insert(key.clone(), join); // Unlike [Self::register_relationship] this method does not return a reference to the @@ -138,14 +146,24 @@ impl QueryPlanState<'_, T> { self.relationships } + pub fn into_scope(self) -> Scope { + self.scope + } + /// Use this with the top-level plan to get unrelated joins. pub fn into_unrelated_collections(self) -> BTreeMap> { self.unrelated_joins.take() } - fn unique_name(&mut self, name: String) -> String { - let count = self.counter.get(); - self.counter.set(count + 1); + fn unique_relationship_name(&mut self, name: impl std::fmt::Display) -> String { + let count = self.relationship_name_counter.get(); + self.relationship_name_counter.set(count + 1); format!("{name}_{count}") } + + fn unique_scope_name(&mut self) -> String { + let count = self.scope_name_counter.get(); + self.scope_name_counter.set(count + 1); + format!("scope_{count}") + } } diff --git a/crates/ndc-query-plan/src/plan_for_query_request/tests.rs b/crates/ndc-query-plan/src/plan_for_query_request/tests.rs index 69a46b51..e834b186 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/tests.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/tests.rs @@ -246,10 +246,13 @@ fn translates_query_request_relationships() -> Result<(), anyhow::Error> { column_mapping: [("_id".into(), "class_id".into())].into(), relationship_type: RelationshipType::Array, arguments: Default::default(), - query: Default::default(), + query: Query { + scope: Some(plan::Scope::Named("scope_1".into())), + ..Default::default() + }, }, - )] - .into(), + )].into(), + scope: Some(plan::Scope::Named("scope_0".into())), ..Default::default() }, }, @@ -290,6 +293,7 @@ fn translates_query_request_relationships() -> Result<(), anyhow::Error> { )] .into(), ), + scope: Some(plan::Scope::Root), ..Default::default() }, }; @@ -394,12 +398,13 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { }, operator: plan_test_helpers::ComparisonOperator::Equal, value: plan::ComparisonValue::Column { - column: plan::ComparisonTarget::RootCollectionColumn { + column: plan::ComparisonTarget::ColumnInScope { name: "id".into(), field_path: Default::default(), column_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), + scope: plan::Scope::Root, }, }, }, @@ -434,6 +439,7 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { )] .into(), ), + scope: Some(plan::Scope::Root), ..Default::default() }, unrelated_collections: [( @@ -455,8 +461,9 @@ fn translates_root_column_references() -> Result<(), anyhow::Error> { }, operator: plan_test_helpers::ComparisonOperator::Equal, value: plan::ComparisonValue::Column { - column: plan::ComparisonTarget::RootCollectionColumn { + column: plan::ComparisonTarget::ColumnInScope { name: "id".into(), + scope: plan::Scope::Root, column_type: plan::Type::Scalar( plan_test_helpers::ScalarType::Int, ), @@ -533,6 +540,7 @@ fn translates_aggregate_selections() -> Result<(), anyhow::Error> { ] .into(), ), + scope: Some(plan::Scope::Root), ..Default::default() }, arguments: Default::default(), @@ -709,11 +717,13 @@ fn translates_relationships_in_fields_predicates_and_orderings() -> Result<(), a ] .into(), ), + scope: Some(plan::Scope::Named("scope_0".into())), ..Default::default() }, }, )] .into(), + scope: Some(plan::Scope::Root), ..Default::default() }, arguments: Default::default(), @@ -822,6 +832,7 @@ fn translates_nested_fields() -> Result<(), anyhow::Error> { ] .into(), ), + scope: Some(plan::Scope::Root), ..Default::default() }, arguments: Default::default(), @@ -909,11 +920,13 @@ fn translates_predicate_referencing_field_of_related_collection() -> anyhow::Res )] .into(), ), + scope: Some(plan::Scope::Named("scope_0".into())), ..Default::default() }, }, )] .into(), + scope: Some(plan::Scope::Root), ..Default::default() }, arguments: Default::default(), diff --git a/crates/ndc-query-plan/src/plan_for_query_request/type_annotated_field.rs b/crates/ndc-query-plan/src/plan_for_query_request/type_annotated_field.rs index cd8b6a02..fe1b720b 100644 --- a/crates/ndc-query-plan/src/plan_for_query_request/type_annotated_field.rs +++ b/crates/ndc-query-plan/src/plan_for_query_request/type_annotated_field.rs @@ -69,12 +69,16 @@ fn type_annotated_field_helper( .context .find_collection_object_type(&relationship_def.target_collection)?; - let query_plan = plan_for_query( - &mut plan_state.state_for_subquery(), - root_collection_object_type, + let mut subquery_state = plan_state.state_for_subquery(); + subquery_state.new_scope(); + + let mut query_plan = plan_for_query( + &mut subquery_state, + &related_collection_type, &related_collection_type, *query, )?; + query_plan.scope = Some(subquery_state.into_scope()); // It's important to get fields and aggregates from the constructed relationship query // before it is registered because at that point fields and aggregates will be merged 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 def0552b..b011b2ba 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 @@ -97,6 +97,14 @@ where return Err(RelationshipUnificationError::Mismatch(mismatching_fields)); } + let scope = unify_options(a.scope, b.scope, |a, b| { + if a == b { + Ok(a) + } else { + Err(RelationshipUnificationError::Mismatch(vec!["scope"])) + } + })?; + let query = Query { aggregates: unify_aggregates(a.aggregates, b.aggregates)?, fields: unify_fields(a.fields, b.fields)?, @@ -106,6 +114,7 @@ where order_by: a.order_by, predicate: predicate_a, relationships: unify_nested_relationships(a.relationships, b.relationships)?, + scope, }; Ok(query) } diff --git a/crates/ndc-query-plan/src/query_plan.rs b/crates/ndc-query-plan/src/query_plan.rs index e4e10192..323b7f8e 100644 --- a/crates/ndc-query-plan/src/query_plan.rs +++ b/crates/ndc-query-plan/src/query_plan.rs @@ -55,6 +55,11 @@ pub struct Query { /// Relationships referenced by fields and expressions in this query or sub-query. Does not /// include relationships in sub-queries nested under this one. pub relationships: Relationships, + + /// Some relationship references may introduce a named "scope" so that other parts of the query + /// request can reference fields of documents in the related collection. The connector must + /// introduce a variable, or something similar, for such references. + pub scope: Option, } impl Query { @@ -93,6 +98,12 @@ pub struct UnrelatedJoin { pub query: Query, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Scope { + Root, + Named(String), +} + #[derive(Derivative)] #[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] pub enum Aggregate { @@ -227,7 +238,7 @@ impl Expression { Either::Right(iter::empty()) } } - t @ ComparisonTarget::RootCollectionColumn { .. } => Either::Left(iter::once(t)), + t @ ComparisonTarget::ColumnInScope { .. } => Either::Left(iter::once(t)), } } } @@ -299,10 +310,14 @@ pub enum ComparisonTarget { /// fields for the [QueryPlan]. path: Vec, }, - RootCollectionColumn { + ColumnInScope { /// The name of the column name: String, + /// 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>, @@ -314,14 +329,14 @@ impl ComparisonTarget { pub fn column_name(&self) -> &str { match self { ComparisonTarget::Column { name, .. } => name, - ComparisonTarget::RootCollectionColumn { name, .. } => name, + ComparisonTarget::ColumnInScope { name, .. } => name, } } pub fn relationship_path(&self) -> &[String] { match self { ComparisonTarget::Column { path, .. } => path, - ComparisonTarget::RootCollectionColumn { .. } => &[], + ComparisonTarget::ColumnInScope { .. } => &[], } } } @@ -330,7 +345,7 @@ impl ComparisonTarget { pub fn get_column_type(&self) -> &Type { match self { ComparisonTarget::Column { column_type, .. } => column_type, - ComparisonTarget::RootCollectionColumn { column_type, .. } => column_type, + ComparisonTarget::ColumnInScope { column_type, .. } => column_type, } } } diff --git a/fixtures/ddn/sample_mflix/models/Comments.hml b/fixtures/ddn/sample_mflix/models/Comments.hml index a525e184..5e0cba4f 100644 --- a/fixtures/ddn/sample_mflix/models/Comments.hml +++ b/fixtures/ddn/sample_mflix/models/Comments.hml @@ -57,6 +57,15 @@ definition: - movieId - name - text + - role: user + output: + allowedFields: + - id + - date + - email + - movieId + - name + - text --- kind: ObjectBooleanExpressionType @@ -135,4 +144,14 @@ definition: - role: admin select: filter: null - + - role: user + select: + filter: + relationship: + name: user + predicate: + fieldComparison: + field: id + operator: _eq + value: + sessionVariable: x-hasura-user-id diff --git a/fixtures/ddn/sample_mflix/models/Users.hml b/fixtures/ddn/sample_mflix/models/Users.hml index 48f2c1f4..48ba8510 100644 --- a/fixtures/ddn/sample_mflix/models/Users.hml +++ b/fixtures/ddn/sample_mflix/models/Users.hml @@ -45,6 +45,12 @@ definition: - email - name - password + - role: user + output: + allowedFields: + - id + - email + - name --- kind: ObjectBooleanExpressionType @@ -111,4 +117,11 @@ definition: - role: admin select: filter: null - + - role: user + select: + filter: + fieldComparison: + field: id + operator: _eq + value: + sessionVariable: x-hasura-user-id