From f53e2a0e0a11e712d4f78d05d1b166ed827c4275 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Thu, 3 Oct 2024 19:15:45 +0530 Subject: [PATCH] Update `ndc-sdk` version to `v1.6.0` (#151) ### What ### How --------- Co-authored-by: py Co-authored-by: pranshi06 <85474619+pranshi06@users.noreply.github.com> --- Cargo.lock | 134 +++++---------- Cargo.toml | 10 +- crates/cli/Cargo.toml | 3 - crates/cli/tests/update_tests.rs | 19 +-- crates/configuration/Cargo.toml | 1 + crates/configuration/src/version1.rs | 56 +++--- crates/ndc-sqlserver/Cargo.toml | 1 + crates/ndc-sqlserver/src/capabilities.rs | 33 ++++ crates/ndc-sqlserver/src/connector.rs | 148 ++++++++-------- crates/ndc-sqlserver/src/error/convert.rs | 36 ++++ crates/ndc-sqlserver/src/error/mod.rs | 4 + crates/ndc-sqlserver/src/error/record.rs | 37 ++++ crates/ndc-sqlserver/src/explain.rs | 69 +++----- crates/ndc-sqlserver/src/lib.rs | 2 + crates/ndc-sqlserver/src/mutation.rs | 58 +++---- crates/ndc-sqlserver/src/query.rs | 60 +++---- crates/ndc-sqlserver/src/schema.rs | 159 ++++++++++-------- crates/ndc-sqlserver/tests/common/database.rs | 7 +- .../tests/common/fresh_deployments.rs | 5 +- crates/ndc-sqlserver/tests/common/helpers.rs | 2 +- crates/ndc-sqlserver/tests/explain_tests.rs | 8 +- crates/ndc-sqlserver/tests/mutation_tests.rs | 2 +- crates/ndc-sqlserver/tests/ndc_tests.rs | 3 + ...procedure_without_providing_arguments.snap | 6 +- crates/query-engine/execution/Cargo.toml | 1 + crates/query-engine/execution/src/lib.rs | 1 + crates/query-engine/execution/src/query.rs | 98 +++++------ crates/query-engine/metadata/Cargo.toml | 2 + .../metadata/src/metadata/database.rs | 18 +- .../metadata/src/metadata/native_queries.rs | 27 +-- crates/query-engine/sql/Cargo.toml | 1 + .../sql/src/sql/execution_plan.rs | 4 +- .../translation/src/translation/helpers.rs | 74 ++++---- .../src/translation/mutation/mod.rs | 48 ++++-- .../translation/mutation/stored_procedures.rs | 14 +- .../src/translation/query/aggregates.rs | 25 ++- .../src/translation/query/filtering.rs | 41 +++-- .../translation/src/translation/query/mod.rs | 4 +- .../src/translation/query/relationships.rs | 27 +-- .../translation/src/translation/query/root.rs | 15 +- .../src/translation/query/sorting.rs | 38 +++-- .../translation/src/translation/values.rs | 5 +- static/configuration.json | 36 ++-- 43 files changed, 720 insertions(+), 622 deletions(-) create mode 100644 crates/ndc-sqlserver/src/capabilities.rs create mode 100644 crates/ndc-sqlserver/src/error/convert.rs create mode 100644 crates/ndc-sqlserver/src/error/mod.rs create mode 100644 crates/ndc-sqlserver/src/error/record.rs diff --git a/Cargo.lock b/Cargo.lock index f09a48cd..dd6d4010 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -250,12 +250,6 @@ dependencies = [ "rustc-demangle", ] -[[package]] -name = "base64" -version = "0.13.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" - [[package]] name = "base64" version = "0.21.5" @@ -318,16 +312,6 @@ dependencies = [ "regex-automata 0.1.10", ] -[[package]] -name = "build-data" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eda20fcece9c23f3c3f4c2751a8a5ca9491c05fa7a69920af65953c3b39b7ce4" -dependencies = [ - "chrono", - "safe-regex", -] - [[package]] name = "bumpalo" version = "3.14.0" @@ -1235,32 +1219,22 @@ dependencies = [ [[package]] name = "ndc-models" -version = "0.1.2" -source = "git+https://github.com/hasura/ndc-spec.git?tag=v0.1.2#6e7d12a31787d5f618099a42ddc0bea786438c00" -dependencies = [ - "indexmap 2.2.6", - "schemars", - "serde", - "serde_json", - "serde_with", -] - -[[package]] -name = "ndc-models" -version = "0.1.2" -source = "git+https://github.com/hasura/ndc-spec.git?rev=c59f824ff95e6a376c34f85816e80164bc1f3894#c59f824ff95e6a376c34f85816e80164bc1f3894" +version = "0.1.6" +source = "git+https://github.com/hasura/ndc-spec.git?tag=v0.1.6#d1be19e9cdd86ac7b6ad003ff82b7e5b4e96b84f" dependencies = [ "indexmap 2.2.6", + "ref-cast", "schemars", "serde", "serde_json", "serde_with", + "smol_str", ] [[package]] name = "ndc-sdk" -version = "0.1.0" -source = "git+https://github.com/hasura/ndc-sdk-rs.git?rev=a273a01efccfc71ef3341cf5f357b2c9ae2d109f#a273a01efccfc71ef3341cf5f357b2c9ae2d109f" +version = "0.4.0" +source = "git+https://github.com/hasura/ndc-sdk-rs.git?tag=v0.4.0#665509f7d3b47ce4f014fc23f817a3599ba13933" dependencies = [ "async-trait", "axum", @@ -1269,7 +1243,8 @@ dependencies = [ "clap", "http", "mime", - "ndc-models 0.1.2 (git+https://github.com/hasura/ndc-spec.git?tag=v0.1.2)", + "ndc-models", + "ndc-test", "opentelemetry", "opentelemetry-http", "opentelemetry-otlp", @@ -1306,6 +1281,7 @@ dependencies = [ "prometheus", "query-engine-execution", "query-engine-metadata", + "query-engine-metrics", "query-engine-sql", "query-engine-translation", "reqwest", @@ -1325,7 +1301,6 @@ name = "ndc-sqlserver-cli" version = "0.2.1" dependencies = [ "anyhow", - "build-data", "clap", "ndc-sqlserver-configuration", "schemars", @@ -1343,6 +1318,7 @@ version = "0.2.1" dependencies = [ "bb8", "bb8-tiberius", + "ndc-models", "prometheus", "query-engine-metadata", "query-engine-metrics", @@ -1355,19 +1331,20 @@ dependencies = [ [[package]] name = "ndc-test" -version = "0.1.2" -source = "git+https://github.com/hasura/ndc-spec.git?rev=c59f824ff95e6a376c34f85816e80164bc1f3894#c59f824ff95e6a376c34f85816e80164bc1f3894" +version = "0.1.6" +source = "git+https://github.com/hasura/ndc-spec.git?tag=v0.1.6#d1be19e9cdd86ac7b6ad003ff82b7e5b4e96b84f" dependencies = [ "async-trait", "clap", "colorful", "indexmap 2.2.6", - "ndc-models 0.1.2 (git+https://github.com/hasura/ndc-spec.git?rev=c59f824ff95e6a376c34f85816e80164bc1f3894)", + "ndc-models", "rand 0.8.5", "reqwest", "semver", "serde", "serde_json", + "smol_str", "thiserror", "tokio", "url", @@ -1741,6 +1718,7 @@ dependencies = [ "bb8", "bb8-tiberius", "bytes", + "ndc-models", "prometheus", "query-engine-metrics", "query-engine-sql", @@ -1757,8 +1735,10 @@ dependencies = [ name = "query-engine-metadata" version = "0.2.1" dependencies = [ + "ndc-models", "schemars", "serde", + "smol_str", ] [[package]] @@ -1772,6 +1752,7 @@ dependencies = [ name = "query-engine-sql" version = "0.2.1" dependencies = [ + "ndc-models", "serde_json", ] @@ -1877,6 +1858,26 @@ dependencies = [ "bitflags 1.3.2", ] +[[package]] +name = "ref-cast" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ccf0a6f84d5f1d581da8b41b47ec8600871962f2a528115b542b362d4b744931" +dependencies = [ + "ref-cast-impl", +] + +[[package]] +name = "ref-cast-impl" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bcc303e793d3734489387d205e9b186fac9c6cfacedd98cbb2e8a5943595f3e6" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.58", +] + [[package]] name = "regex" version = "1.10.2" @@ -2137,53 +2138,6 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1ad4cc8da4ef723ed60bced201181d83791ad433213d8c24efffda1eec85d741" -[[package]] -name = "safe-proc-macro2" -version = "1.0.67" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fd85be67db87168aa3c13fd0da99f48f2ab005dccad5af5626138dc1df20eb6" -dependencies = [ - "unicode-ident", -] - -[[package]] -name = "safe-quote" -version = "1.0.15" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77e530f7831f3feafcd5f1aae406ac205dd998436b4007c8e80f03eca78a88f7" -dependencies = [ - "safe-proc-macro2", -] - -[[package]] -name = "safe-regex" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5194fafa3cb9da89e0cab6dffa1f3fdded586bd6396d12be11b4cae0c7ee45c2" -dependencies = [ - "safe-regex-macro", -] - -[[package]] -name = "safe-regex-compiler" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e822ae1e61251bcfd698317c237cf83f7c57161a5dc24ee609a85697f1ed15b3" -dependencies = [ - "safe-proc-macro2", - "safe-quote", -] - -[[package]] -name = "safe-regex-macro" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2768de7e6ef19f59c5fd3c3ac207ef12b68a49f95e3172d67e4a04cfd992ca06" -dependencies = [ - "safe-proc-macro2", - "safe-regex-compiler", -] - [[package]] name = "schannel" version = "0.1.22" @@ -2332,15 +2286,17 @@ dependencies = [ [[package]] name = "serde_with" -version = "2.3.3" +version = "3.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07ff71d2c147a7b57362cead5e22f772cd52f6ab31cfcd9edcd7f6aeb2a0afbe" +checksum = "0ad483d2ab0149d5a5ebcd9972a3852711e0153d863bf5a5d0391d28883c4a20" dependencies = [ - "base64 0.13.1", + "base64 0.22.0", "chrono", "hex", "indexmap 1.9.3", + "indexmap 2.2.6", "serde", + "serde_derive", "serde_json", "serde_with_macros", "time", @@ -2348,9 +2304,9 @@ dependencies = [ [[package]] name = "serde_with_macros" -version = "2.3.3" +version = "3.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "881b6f881b17d13214e5d494c939ebab463d01264ce1811e9d4ac3a882e7695f" +checksum = "65569b702f41443e8bc8bbb1c5779bd0450bbe723b56198980e80ec45780bce2" dependencies = [ "darling", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index b8a53724..ee3a1540 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,9 @@ unused_async = "allow" [workspace.dependencies] # ndc-models was using version 0.1.2 but we needed rustls fix -ndc-models = { git = "https://github.com/hasura/ndc-spec.git", rev = "c59f824ff95e6a376c34f85816e80164bc1f3894" } -ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs.git", rev = "a273a01efccfc71ef3341cf5f357b2c9ae2d109f", default-features = false, features = ["rustls"]} -# ndc-test was using version 0.1.2 but we needed rustls fix -ndc-test = { git = "https://github.com/hasura/ndc-spec.git", rev = "c59f824ff95e6a376c34f85816e80164bc1f3894", default-features = false, features = ["rustls"] } + +ndc-models = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.6" } +ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs.git", tag = "v0.4.0" } +ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.6" } + +smol_str = "0.1" diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 9805b41f..df4df560 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -21,8 +21,5 @@ tokio = { version = "1.37.0", features = ["full"] } [dev-dependencies] tempfile = "3.10.1" -[build-dependencies] -build-data = "0.2.1" - [package.metadata.cargo-machete] ignored = ["build_data"] # apparently cargo-machete doesn't find dependencies used by build scripts diff --git a/crates/cli/tests/update_tests.rs b/crates/cli/tests/update_tests.rs index 62d098af..13cbe381 100644 --- a/crates/cli/tests/update_tests.rs +++ b/crates/cli/tests/update_tests.rs @@ -46,16 +46,15 @@ async fn test_update_configuration() -> anyhow::Result<()> { let contents = fs::read_to_string(configuration_file_path).await?; common::assert_ends_with_newline(&contents); let output: RawConfiguration = serde_json::from_str(&contents)?; - match output { - configuration::RawConfiguration { - mssql_connection_string, - metadata, - .. - } => { - assert_eq!(mssql_connection_string, connection_uri); - let some_table_metadata = metadata.tables.0.get("Artist"); - assert!(some_table_metadata.is_some()); - } + let configuration::RawConfiguration { + mssql_connection_string, + metadata, + .. + } = output; + { + assert_eq!(mssql_connection_string, connection_uri); + let some_table_metadata = metadata.tables.0.get("Artist"); + assert!(some_table_metadata.is_some()); } Ok(()) diff --git a/crates/configuration/Cargo.toml b/crates/configuration/Cargo.toml index accd1115..7c722d33 100644 --- a/crates/configuration/Cargo.toml +++ b/crates/configuration/Cargo.toml @@ -7,6 +7,7 @@ edition.workspace = true workspace = true [dependencies] +ndc-models = { workspace = true } query-engine-metadata = { path = "../query-engine/metadata" } query-engine-metrics = { path = "../query-engine/metrics" } diff --git a/crates/configuration/src/version1.rs b/crates/configuration/src/version1.rs index bbb52bd0..43f90e0b 100644 --- a/crates/configuration/src/version1.rs +++ b/crates/configuration/src/version1.rs @@ -6,6 +6,7 @@ use crate::error::Error; use crate::secret::Secret; use crate::{uri, ConnectionUri}; +use ndc_models::{AggregateFunctionName, CollectionName, ComparisonOperatorName, FieldName}; use query_engine_metadata::metadata; use query_engine_metadata::metadata::stored_procedures::{ StoredProcedureArgumentInfo, StoredProcedureInfo, StoredProcedures, @@ -326,12 +327,12 @@ fn get_aggregate_functions(type_names: &Vec) -> database::AggregateFun // taken from https://learn.microsoft.com/en-us/sql/t-sql/functions/aggregate-functions-transact-sql?view=sql-server-ver16 fn get_aggregate_functions_for_type( type_name: &database::ScalarType, -) -> BTreeMap { +) -> BTreeMap { let mut aggregate_functions = BTreeMap::new(); if !NOT_APPROX_COUNTABLE.contains(&type_name.0.as_str()) { aggregate_functions.insert( - "APPROX_COUNT_DISTINCT".to_string(), + AggregateFunctionName::new("APPROX_COUNT_DISTINCT".to_string().into()), database::AggregateFunction { return_type: metadata::ScalarType("bigint".to_string()), }, @@ -340,7 +341,7 @@ fn get_aggregate_functions_for_type( if !NOT_COUNTABLE.contains(&type_name.0.as_str()) { aggregate_functions.insert( - "COUNT".to_string(), + AggregateFunctionName::new("COUNT".to_string().into()), database::AggregateFunction { return_type: metadata::ScalarType("int".to_string()), }, @@ -355,13 +356,13 @@ fn get_aggregate_functions_for_type( || type_name.0.as_str() == "uniqueidentifier") { aggregate_functions.insert( - "MIN".to_string(), + AggregateFunctionName::new("MIN".to_string().into()), database::AggregateFunction { return_type: type_name.clone(), }, ); aggregate_functions.insert( - "MAX".to_string(), + AggregateFunctionName::new("MAX".to_string().into()), database::AggregateFunction { return_type: type_name.clone(), }, @@ -373,25 +374,25 @@ fn get_aggregate_functions_for_type( || APPROX_NUMERICS.contains(&type_name.0.as_str())) { aggregate_functions.insert( - "STDEV".to_string(), + AggregateFunctionName::new("STDEV".to_string().into()), database::AggregateFunction { return_type: database::ScalarType("float".to_string()), }, ); aggregate_functions.insert( - "STDEVP".to_string(), + AggregateFunctionName::new("STDEVP".to_string().into()), database::AggregateFunction { return_type: database::ScalarType("float".to_string()), }, ); aggregate_functions.insert( - "VAR".to_string(), + AggregateFunctionName::new("VAR".to_string().into()), database::AggregateFunction { return_type: database::ScalarType("float".to_string()), }, ); aggregate_functions.insert( - "VARP".to_string(), + AggregateFunctionName::new("VARP".to_string().into()), database::AggregateFunction { return_type: database::ScalarType("float".to_string()), }, @@ -411,13 +412,13 @@ fn get_aggregate_functions_for_type( _ => None, } { aggregate_functions.insert( - "AVG".to_string(), + AggregateFunctionName::new("AVG".to_string().into()), database::AggregateFunction { return_type: metadata::ScalarType(precise_return_type.to_string()), }, ); aggregate_functions.insert( - "SUM".to_string(), + AggregateFunctionName::new("SUM".to_string().into()), database::AggregateFunction { return_type: metadata::ScalarType(precise_return_type.to_string()), }, @@ -425,7 +426,7 @@ fn get_aggregate_functions_for_type( }; aggregate_functions.insert( - "COUNT_BIG".to_string(), + AggregateFunctionName::new("COUNT_BIG".to_string().into()), database::AggregateFunction { return_type: metadata::ScalarType("bigint".to_string()), }, @@ -454,12 +455,12 @@ fn get_comparison_operators(type_names: &Vec) -> database::ComparisonO // categories taken from https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-types-transact-sql fn get_comparison_operators_for_type( type_name: &database::ScalarType, -) -> BTreeMap { +) -> BTreeMap { let mut comparison_operators = BTreeMap::new(); // in ndc-spec, all things can be `==` comparison_operators.insert( - "_eq".to_string(), + ComparisonOperatorName::new("_eq".to_string().into()), database::ComparisonOperator { operator_name: "=".to_string(), argument_type: type_name.clone(), @@ -468,7 +469,7 @@ fn get_comparison_operators_for_type( ); comparison_operators.insert( - "_in".to_string(), + ComparisonOperatorName::new("_in".to_string().into()), database::ComparisonOperator { operator_name: "IN".to_string(), argument_type: type_name.clone(), @@ -481,7 +482,7 @@ fn get_comparison_operators_for_type( || UNICODE_CHARACTER_STRINGS.contains(&type_name.0.as_str()) { comparison_operators.insert( - "_like".to_string(), + ComparisonOperatorName::new("_like".to_string().into()), database::ComparisonOperator { operator_name: "LIKE".to_string(), argument_type: type_name.clone(), @@ -489,7 +490,7 @@ fn get_comparison_operators_for_type( }, ); comparison_operators.insert( - "_nlike".to_string(), + ComparisonOperatorName::new("_nlike".to_string().into()), database::ComparisonOperator { operator_name: "NOT LIKE".to_string(), argument_type: type_name.clone(), @@ -502,7 +503,7 @@ fn get_comparison_operators_for_type( // https://learn.microsoft.com/en-us/sql/t-sql/language-elements/comparison-operators-transact-sql?view=sql-server-ver16 if !CANNOT_COMPARE.contains(&type_name.0.as_str()) { comparison_operators.insert( - "_neq".to_string(), + ComparisonOperatorName::new("_neq".to_string().into()), database::ComparisonOperator { operator_name: "!=".to_string(), argument_type: type_name.clone(), @@ -510,7 +511,7 @@ fn get_comparison_operators_for_type( }, ); comparison_operators.insert( - "_lt".to_string(), + ComparisonOperatorName::new("_lt".to_string().into()), database::ComparisonOperator { operator_name: "<".to_string(), argument_type: type_name.clone(), @@ -518,7 +519,7 @@ fn get_comparison_operators_for_type( }, ); comparison_operators.insert( - "_gt".to_string(), + ComparisonOperatorName::new("_gt".to_string().into()), database::ComparisonOperator { operator_name: ">".to_string(), argument_type: type_name.clone(), @@ -527,7 +528,7 @@ fn get_comparison_operators_for_type( ); comparison_operators.insert( - "_gte".to_string(), + ComparisonOperatorName::new("_gte".to_string().into()), database::ComparisonOperator { operator_name: ">=".to_string(), argument_type: type_name.clone(), @@ -535,7 +536,7 @@ fn get_comparison_operators_for_type( }, ); comparison_operators.insert( - "_lte".to_string(), + ComparisonOperatorName::new("_lte".to_string().into()), database::ComparisonOperator { operator_name: "<=".to_string(), argument_type: type_name.clone(), @@ -560,7 +561,7 @@ fn get_tables_info( for introspection_column in introspection_table.joined_sys_column { let column_name = introspection_column.name.clone(); let (column, new_foreign_relations) = get_column_info(introspection_column); - columns.insert(column_name, column); + columns.insert(FieldName::new(column_name.into()), column); foreign_relations_inner.extend(new_foreign_relations); } @@ -575,7 +576,7 @@ fn get_tables_info( ), }; - tables.insert(table_name, table_info); + tables.insert(CollectionName::new(table_name.into()), table_info); } database::TablesInfo(tables) @@ -586,7 +587,10 @@ fn get_foreign_relation( foreign_key: introspection::IntrospectionForeignKeyColumn, ) -> database::ForeignRelation { let mut column_mapping = BTreeMap::new(); - column_mapping.insert(local_column, foreign_key.joined_referenced_column_name); + column_mapping.insert( + local_column.into(), + foreign_key.joined_referenced_column_name.into(), + ); database::ForeignRelation { foreign_table: foreign_key.joined_referenced_table_name, @@ -606,7 +610,7 @@ fn get_uniqueness_constraints( .columns .iter() .fold(BTreeSet::new(), |mut set, part| { - set.insert(part.name.clone()); + set.insert(part.name.clone().into()); set }); diff --git a/crates/ndc-sqlserver/Cargo.toml b/crates/ndc-sqlserver/Cargo.toml index 96dac1e6..cc73304d 100644 --- a/crates/ndc-sqlserver/Cargo.toml +++ b/crates/ndc-sqlserver/Cargo.toml @@ -23,6 +23,7 @@ query-engine-execution = { path = "../query-engine/execution" } query-engine-metadata = {path = "../query-engine/metadata"} query-engine-sql = { path = "../query-engine/sql" } query-engine-translation = { path = "../query-engine/translation" } +query-engine-metrics = { path = "../query-engine/metrics" } ndc-sqlserver-configuration = { path = "../configuration" } diff --git a/crates/ndc-sqlserver/src/capabilities.rs b/crates/ndc-sqlserver/src/capabilities.rs new file mode 100644 index 00000000..91a42fa0 --- /dev/null +++ b/crates/ndc-sqlserver/src/capabilities.rs @@ -0,0 +1,33 @@ +//! `/capabilities` endpoint for the connector. + +use ndc_sdk::models; + +/// Get the connector's capabilities. +/// +/// This function implements the [capabilities endpoint](https://hasura.github.io/ndc-spec/specification/capabilities.html) +/// from the NDC specification. +pub fn get_capabilities() -> models::Capabilities { + models::Capabilities { + query: models::QueryCapabilities { + aggregates: Some(models::LeafCapability {}), + variables: Some(models::LeafCapability {}), + explain: Some(models::LeafCapability {}), + exists: models::ExistsCapabilities { + nested_collections: Some(models::LeafCapability {}), + }, + nested_fields: models::NestedFieldCapabilities { + filter_by: Some(models::LeafCapability {}), + order_by: Some(models::LeafCapability {}), + aggregates: None, + }, + }, + mutation: models::MutationCapabilities { + transactional: Some(models::LeafCapability {}), + explain: Some(models::LeafCapability {}), + }, + relationships: Some(models::RelationshipCapabilities { + relation_comparisons: Some(models::LeafCapability {}), + order_by_aggregate: Some(models::LeafCapability {}), + }), + } +} diff --git a/crates/ndc-sqlserver/src/connector.rs b/crates/ndc-sqlserver/src/connector.rs index 8a34d962..f1e23f0e 100644 --- a/crates/ndc-sqlserver/src/connector.rs +++ b/crates/ndc-sqlserver/src/connector.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use async_trait::async_trait; use configuration::environment::Environment; use ndc_sdk::connector; +use ndc_sdk::connector::Result; use ndc_sdk::json_response::JsonResponse; use ndc_sdk::models; use tokio::fs; @@ -43,19 +44,14 @@ impl connector::ConnectorSetup for SQLServerSetu async fn parse_configuration( &self, configuration_dir: impl AsRef + Send, - ) -> Result<::Configuration, connector::ParseError> - { + ) -> Result<::Configuration> { let configuration_file = configuration_dir .as_ref() .join(configuration::CONFIGURATION_FILENAME); let configuration_file_contents = - fs::read_to_string(&configuration_file) - .await - .map_err(|err| { - connector::ParseError::Other( - format!("{}: {}", &configuration_file.display(), err).into(), - ) - })?; + fs::read_to_string(&configuration_file).await.map_err(|_| { + connector::ParseError::CouldNotFindConfiguration(configuration_file.clone()) + })?; let configuration: configuration::RawConfiguration = serde_json::from_str(&configuration_file_contents).map_err(|error| { connector::ParseError::ParseError(connector::LocatedError { @@ -66,7 +62,7 @@ impl connector::ConnectorSetup for SQLServerSetu }) })?; - configuration::validate_raw_configuration( + let configuration = configuration::validate_raw_configuration( &configuration_file, configuration, &self.environment, @@ -107,15 +103,20 @@ impl connector::ConnectorSetup for SQLServerSetu } configuration::Error::IoError(inner) => connector::ParseError::IoError(inner), configuration::Error::IoErrorButStringified(inner) => { - connector::ParseError::Other(inner.into()) + std::io::Error::new(std::io::ErrorKind::Other, inner).into() } configuration::Error::ConnectionPoolError(inner) => { - connector::ParseError::Other(inner.into()) + std::io::Error::new(std::io::ErrorKind::Other, inner.to_string()).into() } configuration::Error::StoredProcedureIntrospectionError(inner) => { - connector::ParseError::Other(inner.into()) + connector::ParseError::from(std::io::Error::new( + std::io::ErrorKind::Other, + inner.to_string(), + )) } - }) + })?; + + Ok(configuration) } /// Initialize the connector's in-memory state. @@ -129,12 +130,11 @@ impl connector::ConnectorSetup for SQLServerSetu &self, configuration: &::Configuration, metrics: &mut prometheus::Registry, - ) -> Result<::State, connector::InitializationError> - { + ) -> Result<::State> { configuration::create_state(configuration, metrics) .await .map(Arc::new) - .map_err(|err| connector::InitializationError::Other(err.into())) + .map_err(connector::ErrorResponse::from_error) } } @@ -151,51 +151,18 @@ impl connector::Connector for SQLServer { /// query metrics which cannot be updated directly, e.g. /// the number of idle connections in a connection pool /// can be polled but not updated directly. - fn fetch_metrics( - _configuration: &Self::Configuration, - _state: &Self::State, - ) -> Result<(), connector::FetchMetricsError> { + fn fetch_metrics(_configuration: &Self::Configuration, _state: &Self::State) -> Result<()> { // We'd call something `update_pool_metrics` here ideally, see SQLServer NDC Ok(()) } - /// Check the health of the connector. - /// - /// For example, this function should check that the connector - /// is able to reach its data source over the network. - async fn health_check( - _configuration: &Self::Configuration, - state: &Self::State, - ) -> Result<(), connector::HealthError> { - health_check_connect(state) - .await - .map_err(connector::HealthError::Other) - } - /// Get the connector's capabilities. /// /// This function implements the [capabilities endpoint](https://hasura.github.io/ndc-spec/specification/capabilities.html) /// from the NDC specification. - async fn get_capabilities() -> JsonResponse { - JsonResponse::Value(models::CapabilitiesResponse { - version: "0.1.2".into(), - capabilities: models::Capabilities { - query: models::QueryCapabilities { - aggregates: Some(models::LeafCapability {}), - variables: Some(models::LeafCapability {}), - explain: Some(models::LeafCapability {}), - }, - mutation: models::MutationCapabilities { - transactional: Some(models::LeafCapability {}), - explain: Some(models::LeafCapability {}), - }, - relationships: Some(models::RelationshipCapabilities { - relation_comparisons: Some(models::LeafCapability {}), - order_by_aggregate: Some(models::LeafCapability {}), - }), - }, - }) + async fn get_capabilities() -> models::Capabilities { + crate::capabilities::get_capabilities() } /// Get the connector's schema. @@ -204,8 +171,21 @@ impl connector::Connector for SQLServer { /// from the NDC specification. async fn get_schema( configuration: &Self::Configuration, - ) -> Result, connector::SchemaError> { - schema::get_schema(configuration).map(Into::into) + ) -> Result> { + schema::get_schema(configuration) + .map_err(|err| { + tracing::error!( + meta.signal_type = "log", + event.domain = "ndc", + event.name = "Schema error", + name = "Schema error", + body = %err, + error = true, + "Schema error", + ); + err + }) + .map(Into::into) } /// Explain a query by creating an execution plan @@ -216,7 +196,7 @@ impl connector::Connector for SQLServer { configuration: &Self::Configuration, state: &Self::State, query_request: models::QueryRequest, - ) -> Result, connector::ExplainError> { + ) -> Result> { explain::explain(configuration, state, query_request) .await .map(JsonResponse::Value) @@ -226,7 +206,7 @@ impl connector::Connector for SQLServer { _configuration: &Self::Configuration, _state: &Self::State, _mutation_request: models::MutationRequest, - ) -> Result, connector::ExplainError> { + ) -> Result> { //TODO(PY): Implement mutation explain todo!("mutation explain is currently not implemented") } @@ -239,8 +219,21 @@ impl connector::Connector for SQLServer { configuration: &Self::Configuration, state: &Self::State, request: models::MutationRequest, - ) -> Result, connector::MutationError> { - mutation::mutation(configuration, state, request).await + ) -> Result> { + mutation::mutation(configuration, state, request) + .await + .map_err(|err| { + tracing::error!( + meta.signal_type = "log", + event.domain = "ndc", + event.name = "Mutation error", + name = "Mutation error", + body = %err, + error = true, + "Mutation error", + ); + err + }) } /// Execute a query @@ -251,27 +244,20 @@ impl connector::Connector for SQLServer { configuration: &Self::Configuration, state: &Self::State, query_request: models::QueryRequest, - ) -> Result, connector::QueryError> { - query::query(configuration, state, query_request).await - } -} - -// let's connect to our sql server and get the party started -async fn health_check_connect( - state: &configuration::State, -) -> Result<(), Box> { - let mut connection = state.mssql_pool.get().await?; - let select = tiberius::Query::new("SELECT 1"); - - let stream = select.query(&mut connection).await?; - let Some(row) = stream.into_row().await? else { - return Err("No results returned from health check query".into()); - }; - - // check we got a valid result - if row.get(0) != Some(1) { - return Err("Health check query returned invalid results".into()); + ) -> Result> { + query::query(configuration, state, query_request) + .await + .map_err(|err| { + tracing::error!( + meta.signal_type = "log", + event.domain = "ndc", + event.name = "Query error", + name = "Query error", + body = %err, + error = true, + "Query error", + ); + err + }) } - - Ok(()) } diff --git a/crates/ndc-sqlserver/src/error/convert.rs b/crates/ndc-sqlserver/src/error/convert.rs new file mode 100644 index 00000000..35cbfbe4 --- /dev/null +++ b/crates/ndc-sqlserver/src/error/convert.rs @@ -0,0 +1,36 @@ +//! Functions to convert between internal error types and the error types exposed by ndc-sdk. + +use ndc_sdk::connector::{self, ErrorResponse}; + +/// Convert an error from [query_engine_execution] to [ErrorResponse]. +pub fn execution_error_to_response(error: query_engine_execution::error::Error) -> ErrorResponse { + use query_engine_execution::error::*; + match error { + Error::Query(query_error) => { + connector::QueryError::new_invalid_request(&query_error.to_string()).into() + } + Error::Mutation(mutation_error) => { + connector::MutationError::new_invalid_request(&mutation_error.to_string()).into() + } + Error::ConnectionPool(connection_pool_error) => { + connector::QueryError::new_unprocessable_content(&connection_pool_error.to_string()) + .into() + } + Error::TiberiusError(tiberius_error) => { + connector::QueryError::new_unprocessable_content(&tiberius_error.to_string()).into() + } + } +} + +/// Convert an error from [query_engine_translation] to [connector::QueryError]. +pub fn translation_error_to_response( + error: &query_engine_translation::translation::error::Error, +) -> ErrorResponse { + use query_engine_translation::translation::error::*; + match error { + Error::CapabilityNotSupported(_) | Error::NotImplementedYet(_) => { + connector::QueryError::new_unsupported_operation(&error.to_string()).into() + } + _ => connector::QueryError::new_invalid_request(&error.to_string()).into(), + } +} diff --git a/crates/ndc-sqlserver/src/error/mod.rs b/crates/ndc-sqlserver/src/error/mod.rs new file mode 100644 index 00000000..9177ba88 --- /dev/null +++ b/crates/ndc-sqlserver/src/error/mod.rs @@ -0,0 +1,4 @@ +//! Tools for working with error types. + +pub mod convert; +pub mod record; diff --git a/crates/ndc-sqlserver/src/error/record.rs b/crates/ndc-sqlserver/src/error/record.rs new file mode 100644 index 00000000..abc5ae8e --- /dev/null +++ b/crates/ndc-sqlserver/src/error/record.rs @@ -0,0 +1,37 @@ +//! Record information about errors in traces and metrics. + +use query_engine_metrics::metrics; + +/// Record an execution error in the current trace, and increment a counter. +pub fn execution_error(error: &query_engine_execution::error::Error, metrics: &metrics::Metrics) { + use query_engine_execution::error::*; + // TODO(PY): fix the correct error category + match error { + Error::Query(_) + | Error::Mutation(_) + | Error::ConnectionPool(_) + | Error::TiberiusError(_) => { + metrics.error_metrics.record_invalid_request(); + } + } +} + +/// Record a translation error in the current trace, and increment a counter. +pub fn translation_error( + error: &query_engine_translation::translation::error::Error, + metrics: &metrics::Metrics, +) { + use query_engine_translation::translation::error::*; + tracing::error!("{}", error); + match error { + Error::CapabilityNotSupported(_) => { + metrics.error_metrics.record_unsupported_capability(); + } + Error::NotImplementedYet(_) => { + metrics.error_metrics.record_unsupported_feature(); + } + _ => { + metrics.error_metrics.record_invalid_request(); + } + } +} diff --git a/crates/ndc-sqlserver/src/explain.rs b/crates/ndc-sqlserver/src/explain.rs index 70672143..67826ba1 100644 --- a/crates/ndc-sqlserver/src/explain.rs +++ b/crates/ndc-sqlserver/src/explain.rs @@ -10,11 +10,13 @@ use tracing::{info_span, Instrument}; use ndc_sdk::connector; use ndc_sdk::models; use ndc_sqlserver_configuration as configuration; -use query_engine_execution::error; use query_engine_execution::query; use query_engine_sql::sql; use query_engine_translation::translation; +use crate::error::convert; +use crate::error::record; + /// Explain a query by creating an execution plan /// /// This function implements the [explain endpoint](https://hasura.github.io/ndc-spec/specification/explain.html) @@ -23,7 +25,7 @@ pub async fn explain( configuration: &configuration::Configuration, state: &configuration::State, query_request: models::QueryRequest, -) -> Result { +) -> Result { async move { tracing::info!( query_request_json = serde_json::to_string(&query_request).unwrap(), @@ -31,36 +33,26 @@ pub async fn explain( ); // Compile the query. - let plan = async { plan_query(configuration, state, query_request) } - .instrument(info_span!("Plan query")) - .await?; + let plan = async { + plan_query(configuration, state, query_request).map_err(|err| { + record::translation_error(&err, &state.metrics); + convert::translation_error_to_response(&err) + }) + } + .instrument(info_span!("Plan query")) + .await?; // Execute an explain query. - let (query, plan) = query::explain(&state.mssql_pool, plan) - .instrument(info_span!("Explain query")) - .await - .map_err(|err| match err { - error::Error::Query(err) => { - tracing::error!("{}", err); - - connector::ExplainError::Other(err.to_string().into()) - } - error::Error::ConnectionPool(err) => { - tracing::error!("{}", err); - - connector::ExplainError::Other(err.to_string().into()) - } - error::Error::TiberiusError(err) => { - tracing::error!("{}", err); - - connector::ExplainError::Other(err.to_string().into()) - } - error::Error::Mutation(err) => { - tracing::error!("{}", err); - - connector::ExplainError::Other(err.to_string().into()) - } - })?; + let (query, plan) = async { + query::explain(&state.mssql_pool, plan) + .await + .map_err(|err| { + record::execution_error(&err, &state.metrics); + convert::execution_error_to_response(err) + }) + } + .instrument(info_span!("Explain query")) + .await?; state.metrics.record_successful_explain(); @@ -79,21 +71,8 @@ fn plan_query( configuration: &configuration::Configuration, state: &configuration::State, query_request: models::QueryRequest, -) -> Result { +) -> Result { let timer = state.metrics.time_query_plan(); - let result = - translation::query::translate(&configuration.metadata, query_request).map_err(|err| { - tracing::error!("{}", err); - match err { - translation::error::Error::CapabilityNotSupported(_) => { - state.metrics.error_metrics.record_unsupported_capability(); - connector::ExplainError::UnsupportedOperation(err.to_string()) - } - _ => { - state.metrics.error_metrics.record_invalid_request(); - connector::ExplainError::InvalidRequest(err.to_string()) - } - } - }); + let result = translation::query::translate(&configuration.metadata, query_request); timer.complete_with(result) } diff --git a/crates/ndc-sqlserver/src/lib.rs b/crates/ndc-sqlserver/src/lib.rs index a623d20d..8d6537dd 100644 --- a/crates/ndc-sqlserver/src/lib.rs +++ b/crates/ndc-sqlserver/src/lib.rs @@ -1,4 +1,6 @@ +pub mod capabilities; pub mod connector; +pub mod error; pub mod explain; pub mod mutation; pub mod query; diff --git a/crates/ndc-sqlserver/src/mutation.rs b/crates/ndc-sqlserver/src/mutation.rs index 93ba527e..e83f9647 100644 --- a/crates/ndc-sqlserver/src/mutation.rs +++ b/crates/ndc-sqlserver/src/mutation.rs @@ -5,13 +5,15 @@ use ndc_sdk::{connector, json_response::JsonResponse, models}; use ndc_sqlserver_configuration as configuration; -use query_engine_execution::error; use query_engine_execution::mutation; use query_engine_sql::sql; use query_engine_translation::translation; use tracing::info_span; use tracing::Instrument; +use crate::error::convert; +use crate::error::record; + /// Execute a mutation /// /// This function implements the [mutation endpoint](https://hasura.github.io/ndc-spec/specification/mutations/index.html) @@ -20,7 +22,7 @@ pub async fn mutation( configuration: &configuration::Configuration, state: &configuration::State, request: models::MutationRequest, -) -> Result, connector::MutationError> { +) -> Result, connector::ErrorResponse> { let timer = state.metrics.time_mutation_total(); let result = async move { @@ -28,15 +30,22 @@ pub async fn mutation( request_json = serde_json::to_string(&request).unwrap(), request = ?request ); - let plan = async { plan_mutation(configuration, state, request) } - .instrument(info_span!("Execute Mutation")) - .await?; + let plan = async { + plan_mutation(configuration, state, request).map_err(|err| { + record::translation_error(&err, &state.metrics); + convert::translation_error_to_response(&err) + }) + } + .instrument(info_span!("Execute Mutation")) + .await?; let result = async { - execute_mutation_plan(state, plan) - .instrument(info_span!("Execute mutation")) - .await + execute_mutation_plan(state, plan).await.map_err(|err| { + record::execution_error(&err, &state.metrics); + convert::execution_error_to_response(err) + }) } + .instrument(info_span!("Execute mutation")) .await?; Ok(result) @@ -53,18 +62,9 @@ fn plan_mutation( configuration: &configuration::Configuration, state: &configuration::State, mutation_request: models::MutationRequest, -) -> Result { +) -> Result { let timer = state.metrics.time_mutation_plan(); - let result = translation::mutation::translate(&configuration.metadata, mutation_request) - .map_err(|err| { - tracing::error!("{}", err); - match err { - translation::error::Error::NotSupported(_) => { - connector::MutationError::UnsupportedOperation(err.to_string()) - } - _ => connector::MutationError::InvalidRequest(err.to_string()), - } - }); + let result = translation::mutation::translate(&configuration.metadata, mutation_request); timer.complete_with(result) } @@ -72,26 +72,8 @@ fn plan_mutation( async fn execute_mutation_plan( state: &configuration::State, plan: sql::execution_plan::MutationExecutionPlan, -) -> Result, connector::MutationError> { +) -> Result, query_engine_execution::error::Error> { mutation::execute_mutations(&state.mssql_pool, &state.metrics, plan) .await .map(JsonResponse::Serialized) - .map_err(|err| match err { - error::Error::Query(err) => { - tracing::error!("{}", err); - connector::MutationError::Other(err.into()) - } - error::Error::ConnectionPool(err) => { - tracing::error!("{}", err); - connector::MutationError::Other(err.into()) - } - error::Error::TiberiusError(err) => { - tracing::error!("{}", err); - connector::MutationError::Other(err.into()) - } - error::Error::Mutation(err) => { - tracing::error!("{}", err); - connector::MutationError::Other(err.into()) - } - }) } diff --git a/crates/ndc-sqlserver/src/query.rs b/crates/ndc-sqlserver/src/query.rs index 6ab6ef08..31490c26 100644 --- a/crates/ndc-sqlserver/src/query.rs +++ b/crates/ndc-sqlserver/src/query.rs @@ -2,11 +2,12 @@ //! See the Hasura //! [Native Data Connector Specification](https://hasura.github.io/ndc-spec/specification/queries/index.html) //! for further details. +use crate::error::convert; +use crate::error::record; use ndc_sdk::connector; use ndc_sdk::json_response::JsonResponse; use ndc_sdk::models; use ndc_sqlserver_configuration as configuration; -use query_engine_execution::error; use query_engine_execution::query; use query_engine_sql::sql; use query_engine_translation::translation; @@ -20,21 +21,31 @@ pub async fn query( configuration: &configuration::Configuration, state: &configuration::State, query_request: models::QueryRequest, -) -> Result, connector::QueryError> { +) -> Result, connector::ErrorResponse> { tracing::info!("{}", serde_json::to_string(&query_request).unwrap()); tracing::info!("{:?}", query_request); let timer = state.metrics.time_query_total(); let result = async move { // Plan the query - let plan = async { plan_query(configuration, state, query_request) } - .instrument(info_span!("Plan query")) - .await?; + let plan = async { + plan_query(configuration, state, query_request).map_err(|err| { + record::translation_error(&err, &state.metrics); + convert::translation_error_to_response(&err) + }) + } + .instrument(info_span!("Plan query")) + .await?; // Execute the query. - let result = execute_query(state, plan) - .instrument(info_span!("Execute query")) - .await?; + let result = async { + execute_query(state, plan).await.map_err(|err| { + record::execution_error(&err, &state.metrics); + convert::execution_error_to_response(err) + }) + } + .instrument(info_span!("Execute query")) + .await?; // assuming query succeeded, increment counter state.metrics.record_successful_query(); @@ -52,44 +63,17 @@ fn plan_query( configuration: &configuration::Configuration, state: &configuration::State, query_request: models::QueryRequest, -) -> Result { +) -> Result { let timer = state.metrics.time_query_plan(); - let result = - translation::query::translate(&configuration.metadata, query_request).map_err(|err| { - tracing::error!("{}", err); - match err { - translation::error::Error::NotSupported(_) => { - connector::QueryError::UnsupportedOperation(err.to_string()) - } - _ => connector::QueryError::InvalidRequest(err.to_string()), - } - }); + let result = translation::query::translate(&configuration.metadata, query_request); timer.complete_with(result) } async fn execute_query( state: &configuration::State, plan: sql::execution_plan::QueryExecutionPlan, -) -> Result, connector::QueryError> { +) -> Result, query_engine_execution::error::Error> { query::mssql_execute_query_plan(&state.mssql_pool, &state.metrics, plan) .await .map(JsonResponse::Serialized) - .map_err(|err| match err { - error::Error::Query(err) => { - tracing::error!("{}", err); - connector::QueryError::Other(err.into()) - } - error::Error::ConnectionPool(err) => { - tracing::error!("{}", err); - connector::QueryError::Other(err.into()) - } - error::Error::TiberiusError(err) => { - tracing::error!("{}", err); - connector::QueryError::Other(err.into()) - } - error::Error::Mutation(err) => { - tracing::error!("{}", err); - connector::QueryError::Other(err.into()) - } - }) } diff --git a/crates/ndc-sqlserver/src/schema.rs b/crates/ndc-sqlserver/src/schema.rs index a5f3ec0d..a322a029 100644 --- a/crates/ndc-sqlserver/src/schema.rs +++ b/crates/ndc-sqlserver/src/schema.rs @@ -7,6 +7,10 @@ use std::collections::BTreeMap; use ndc_sdk::connector; use ndc_sdk::models; +use ndc_sdk::models::ArgumentName; +use ndc_sdk::models::ObjectTypeName; +use ndc_sdk::models::ProcedureName; +use ndc_sdk::models::ScalarTypeName; use ndc_sdk::models::TypeRepresentation; use query_engine_metadata::metadata; @@ -24,9 +28,13 @@ enum SchemaError { fn scalar_type_to_type(type_name: String, nullability: &metadata::Nullable) -> models::Type { match nullability { - metadata::Nullable::NonNullable => models::Type::Named { name: type_name }, + metadata::Nullable::NonNullable => models::Type::Named { + name: type_name.into(), + }, metadata::Nullable::Nullable => models::Type::Nullable { - underlying_type: Box::new(models::Type::Named { name: type_name }), + underlying_type: Box::new(models::Type::Named { + name: type_name.into(), + }), }, } } @@ -41,8 +49,8 @@ fn column_to_type(column: &metadata::ColumnInfo) -> models::Type { /// to the `columns` field. fn get_native_queries_schema( native_queries: &query_engine_metadata::metadata::NativeQueries, - object_types: &mut BTreeMap, -) -> Result, connector::SchemaError> { + object_types: &mut BTreeMap, +) -> Vec { let mut read_only_native_queries = Vec::new(); native_queries.0.iter().for_each(|(name, info)| { @@ -54,21 +62,22 @@ fn get_native_queries_schema( models::ObjectField { description: column.description.clone(), r#type: column_to_type(column), + arguments: BTreeMap::new(), }, ) })), }; - object_types.insert(name.clone(), native_query_object_type); + object_types.insert(name.clone().into(), native_query_object_type); let native_query_collection_info = models::CollectionInfo { - name: name.clone(), + name: name.as_str().into(), description: info.description.clone(), arguments: info .arguments .iter() .map(|(name, column_info)| { ( - name.clone(), + name.as_str().into(), models::ArgumentInfo { description: column_info.description.clone(), argument_type: column_to_type(column_info), @@ -76,14 +85,14 @@ fn get_native_queries_schema( ) }) .collect(), - collection_type: name.clone(), + collection_type: name.as_str().into(), uniqueness_constraints: BTreeMap::new(), foreign_keys: BTreeMap::new(), }; read_only_native_queries.push(native_query_collection_info); }); - Ok(read_only_native_queries) + read_only_native_queries } /// Build a `ProcedureInfo` type from the given parameters. @@ -94,15 +103,15 @@ fn get_native_queries_schema( /// in the schema). So, this function creates that object type, optionally adds that scalar type, /// and then returns a `ProcedureInfo` that points to the correct object type. fn make_procedure_type( - name: String, + name: ProcedureName, description: Option, - arguments: BTreeMap, + arguments: BTreeMap, result_type: models::Type, - object_types: &mut BTreeMap, - scalar_types: &mut BTreeMap, + object_types: &mut BTreeMap, + scalar_types: &mut BTreeMap, ) -> models::ProcedureInfo { let mut fields = BTreeMap::new(); - let object_type_name = format!("{name}_response"); + let object_type_name: models::ObjectTypeName = format!("{name}_response").into(); // If int doesn't exist anywhere else in the schema, we need to add it here. However, a user // can't filter or aggregate based on the affected rows of a procedure, so we don't need to add @@ -110,7 +119,7 @@ fn make_procedure_type( // schema and has already been added, it will also already contain these functions and // operators. scalar_types - .entry("int".to_string()) + .entry("int".into()) .or_insert(models::ScalarType { aggregate_functions: BTreeMap::new(), comparison_operators: BTreeMap::new(), @@ -118,22 +127,22 @@ fn make_procedure_type( }); fields.insert( - "affected_rows".to_string(), + "affected_rows".into(), models::ObjectField { description: Some("The number of rows affected by the mutation".to_string()), - r#type: models::Type::Named { - name: "int".to_string(), - }, + r#type: models::Type::Named { name: "int".into() }, + arguments: BTreeMap::new(), }, ); fields.insert( - "returning".to_string(), + "returning".into(), models::ObjectField { description: Some("Data from rows affected by the mutation".to_string()), r#type: models::Type::Array { element_type: Box::from(result_type), }, + arguments: BTreeMap::new(), }, ); @@ -150,7 +159,7 @@ fn make_procedure_type( description, arguments, result_type: models::Type::Named { - name: object_type_name, + name: object_type_name.into(), }, } } @@ -166,9 +175,9 @@ fn make_procedure_type( /// contain the fields specified in the `columns`. fn get_native_mutations_schema( native_mutations_metadata: &query_engine_metadata::metadata::NativeMutations, - object_types: &mut BTreeMap, - scalar_types: &mut BTreeMap, -) -> Result, connector::SchemaError> { + object_types: &mut BTreeMap, + scalar_types: &mut BTreeMap, +) -> Result, connector::ErrorResponse> { let mut native_mutations = Vec::new(); native_mutations_metadata.0.iter().for_each(|(name, info)| { @@ -180,11 +189,13 @@ fn get_native_mutations_schema( models::ObjectField { description: column.column_info.description.clone(), r#type: column_to_type(&column.column_info), + arguments: BTreeMap::new(), }, ) })), }; - object_types.insert(name.clone(), native_query_object_type); + //TODO(PY): check if converting ProcedureName to ObjectTypeName is correct + object_types.insert(name.to_string().into(), native_query_object_type); let procedure_info = make_procedure_type( name.clone(), @@ -201,7 +212,10 @@ fn get_native_mutations_schema( ) }) .collect(), - models::Type::Named { name: name.clone() }, + // TODO(PY): check if this is correct + models::Type::Named { + name: name.to_string().into(), + }, object_types, scalar_types, ); @@ -226,18 +240,18 @@ fn get_stored_procedure_argument( fn get_stored_procedures_schema( stored_procedures: &StoredProcedures, - object_types: &mut BTreeMap, -) -> Result, connector::SchemaError> { + object_types: &mut BTreeMap, +) -> Result, connector::ErrorResponse> { let mut stored_procedures_schema = Vec::new(); for (proc_name, proc_info) in stored_procedures.0.iter() { if let Some(returns) = &proc_info.returns { - let proc_args: BTreeMap = proc_info + let proc_args: BTreeMap = proc_info .arguments .iter() .map(|(arg_name, arg_info)| { ( - arg_name.clone(), + arg_name.clone().into(), get_stored_procedure_argument(arg_info.clone()), ) }) @@ -246,31 +260,32 @@ fn get_stored_procedures_schema( description: proc_info.description.clone(), fields: BTreeMap::from_iter(returns.iter().map(|(column_name, column)| { ( - column_name.clone(), + column_name.clone().into(), models::ObjectField { description: column.description.clone(), r#type: column_to_type(column), + arguments: BTreeMap::new(), }, ) })), }; let object_type_name = format!("{proc_name}_response"); if object_types - .insert(object_type_name.clone(), stored_proc_object_type) + .insert(object_type_name.clone().into(), stored_proc_object_type) .is_some() { - return Err(connector::SchemaError::Other(Box::new( + return Err(connector::ErrorResponse::from_error(Box::new( SchemaError::DuplicateObject(object_type_name), ))); }; let stored_proc_schema = models::ProcedureInfo { - name: proc_name.to_string(), + name: proc_name.to_string().into(), description: proc_info.description.clone(), arguments: proc_args, result_type: models::Type::Array { element_type: Box::new(models::Type::Named { - name: object_type_name, + name: object_type_name.into(), }), }, }; @@ -286,13 +301,13 @@ fn get_stored_procedures_schema( /// from the NDC specification. pub fn get_schema( configuration::Configuration { metadata, .. }: &configuration::Configuration, -) -> Result { - let mut scalar_types: BTreeMap = +) -> Result { + let mut scalar_types: BTreeMap = configuration::occurring_scalar_types(metadata) .iter() .map(|scalar_type| { ( - scalar_type.0.clone(), + scalar_type.0.clone().into(), models::ScalarType { // TODO(PY): Add representation for beta representation: None, @@ -307,7 +322,7 @@ pub fn get_schema( function_name.clone(), models::AggregateFunctionDefinition { result_type: models::Type::Named { - name: function_definition.return_type.0.clone(), + name: function_definition.return_type.0.clone().into(), }, }, ) @@ -332,7 +347,7 @@ pub fn get_schema( metadata::OperatorKind::Custom => { models::ComparisonOperatorDefinition::Custom { argument_type: models::Type::Named { - name: op_def.argument_type.0.clone(), + name: op_def.argument_type.0.clone().into(), }, } } @@ -353,7 +368,8 @@ pub fn get_schema( name: table_name.clone(), description: table.description.clone(), arguments: BTreeMap::new(), - collection_type: table_name.clone(), + // TODO(PY): check if converting CollectionName to ObjectTypeName is correct + collection_type: table_name.to_string().into(), uniqueness_constraints: table .uniqueness_constraints .0 @@ -384,7 +400,7 @@ pub fn get_schema( ( constraint_name.clone(), models::ForeignKeyConstraint { - foreign_collection: foreign_table.clone(), + foreign_collection: foreign_table.clone().into(), column_mapping: column_mapping.clone(), }, ) @@ -399,20 +415,21 @@ pub fn get_schema( description: table.description.clone(), fields: BTreeMap::from_iter(table.columns.values().map(|column| { ( - column.name.clone(), + column.name.clone().into(), models::ObjectField { description: column.description.clone(), r#type: column_to_type(column), + arguments: BTreeMap::new(), }, ) })), }; - (table_name.clone(), object_type) + (table_name.as_str().into(), object_type) })); let mut object_types = table_types; - let native_queries = get_native_queries_schema(&metadata.native_queries, &mut object_types)?; + let native_queries = get_native_queries_schema(&metadata.native_queries, &mut object_types); let native_mutations = get_native_mutations_schema( &metadata.native_mutations, @@ -474,14 +491,14 @@ mod tests { let mut columns = BTreeMap::new(); columns.insert( - "id".to_owned(), + "id".to_owned().into(), NativeMutationColumnInfo { column_info: id_col_info, cast_as: None, }, ); columns.insert( - "name".to_owned(), + "name".to_owned().into(), NativeMutationColumnInfo { column_info: name_col_info, cast_as: None, @@ -498,7 +515,7 @@ mod tests { let mut native_mutations = BTreeMap::new(); native_mutations.insert( - "insert_user_native_mutation".to_string(), + "insert_user_native_mutation".to_string().into(), native_mutation_info, ); @@ -512,7 +529,7 @@ mod tests { .unwrap(); let expected_mutation_procedure_info = ProcedureInfo { - name: "insert_user_native_mutation".to_string(), + name: "insert_user_native_mutation".to_string().into(), description: None, arguments: BTreeMap::new(), result_type: ndc_sdk::models::Type::Named { @@ -528,29 +545,32 @@ mod tests { let expected_object_field_id = ObjectField { description: None, r#type: models::Type::Named { - name: "int".to_string(), + name: "int".to_string().into(), }, + arguments: BTreeMap::new(), }; let expected_object_field_name = ObjectField { description: None, r#type: models::Type::Named { - name: "varchar".to_string(), + name: "varchar".to_string().into(), }, + arguments: BTreeMap::new(), }; let expected_object_field_affected_rows = ObjectField { description: Some("The number of rows affected by the mutation".into()), r#type: models::Type::Named { - name: "int".to_string(), + name: "int".to_string().into(), }, + arguments: BTreeMap::new(), }; let expected_native_mutation_object_type = ObjectType { description: None, fields: BTreeMap::from([ - ("id".to_owned(), expected_object_field_id), - ("name".to_owned(), expected_object_field_name), + ("id".to_owned().into(), expected_object_field_id), + ("name".to_owned().into(), expected_object_field_name), ]), }; @@ -561,26 +581,30 @@ mod tests { name: "insert_user_native_mutation".into(), }), }, + arguments: BTreeMap::new(), }; let expected_native_mutation_response_object_type = ObjectType { description: Some("Responses from the 'insert_user_native_mutation' procedure".into()), fields: BTreeMap::from([ ( - "affected_rows".to_owned(), + "affected_rows".to_owned().into(), expected_object_field_affected_rows, ), - ("returning".to_owned(), expected_object_field_returning), + ( + "returning".to_owned().into(), + expected_object_field_returning, + ), ]), }; - let mut expected_object_types = BTreeMap::new(); + let mut expected_object_types: BTreeMap = BTreeMap::new(); expected_object_types.insert( "insert_user_native_mutation".into(), expected_native_mutation_object_type, ); expected_object_types.insert( - "insert_user_native_mutation_response".to_string(), + "insert_user_native_mutation_response".into(), expected_native_mutation_response_object_type, ); @@ -678,22 +702,22 @@ mod tests { let mut expected_args = BTreeMap::new(); expected_args.insert( - "CustomerId".to_string(), + "CustomerId".to_string().into(), models::ArgumentInfo { description: None, argument_type: models::Type::Named { - name: "int".to_string(), + name: "int".to_string().into(), }, }, ); expected_args.insert( - "Phone".to_string(), + "Phone".to_string().into(), models::ArgumentInfo { description: None, argument_type: models::Type::Nullable { underlying_type: Box::new(models::Type::Named { - name: "varchar".to_string(), + name: "varchar".to_string().into(), }), }, }, @@ -701,7 +725,9 @@ mod tests { let expected_result_type = models::Type::Array { element_type: Box::new(models::Type::Named { - name: "GetCustomerDetailsWithTotalPurchases_response".to_string(), + name: "GetCustomerDetailsWithTotalPurchases_response" + .to_string() + .into(), }), }; @@ -709,7 +735,7 @@ mod tests { assert_eq!( proc_schema, &ProcedureInfo { - name: "GetCustomerDetailsWithTotalPurchases".to_string(), + name: "GetCustomerDetailsWithTotalPurchases".to_string().into(), description: None, arguments: expected_args, result_type: expected_result_type @@ -719,12 +745,13 @@ mod tests { let mut expected_obj_type_fields = BTreeMap::new(); expected_obj_type_fields.insert( - "result".to_string(), + "result".to_string().into(), models::ObjectField { description: None, r#type: (models::Type::Named { - name: "int".to_string(), + name: "int".to_string().into(), }), + arguments: BTreeMap::new(), }, ); diff --git a/crates/ndc-sqlserver/tests/common/database.rs b/crates/ndc-sqlserver/tests/common/database.rs index 1e0657e3..b9f96ae6 100644 --- a/crates/ndc-sqlserver/tests/common/database.rs +++ b/crates/ndc-sqlserver/tests/common/database.rs @@ -91,11 +91,8 @@ pub async fn drop_database(db_name: &str, connection_uri: String) -> Result<(), let drop_db_sql = format!("USE master; ALTER DATABASE \"{db_name}\" SET SINGLE_USER WITH ROLLBACK IMMEDIATE; DROP DATABASE \"{db_name}\" "); // we don't mind if this fails - match connection.simple_query(drop_db_sql).await { - Err(e) => { - println!("Dropping DB {} failed with error: {}", db_name, e); - } - Ok(_) => {} + if let Err(e) = connection.simple_query(drop_db_sql).await { + println!("Dropping DB {} failed with error: {}", db_name, e); } Ok(()) } diff --git a/crates/ndc-sqlserver/tests/common/fresh_deployments.rs b/crates/ndc-sqlserver/tests/common/fresh_deployments.rs index 370ff131..afd087a5 100644 --- a/crates/ndc-sqlserver/tests/common/fresh_deployments.rs +++ b/crates/ndc-sqlserver/tests/common/fresh_deployments.rs @@ -96,9 +96,8 @@ impl Drop for FreshDeployment { tokio::runtime::Handle::current().block_on(async move { let drop_db_result = database::drop_database(&db_name, admin_connection_uri).await; - match drop_db_result { - Err(e) => println!("Error while dropping the temp db: {e}"), - Ok(()) => {} + if let Err(e) = drop_db_result { + println!("Error while dropping the temp db: {e}") } configuration::delete_ndc_metadata(&ndc_metadata_path) diff --git a/crates/ndc-sqlserver/tests/common/helpers.rs b/crates/ndc-sqlserver/tests/common/helpers.rs index 89d6fadb..8db1add4 100644 --- a/crates/ndc-sqlserver/tests/common/helpers.rs +++ b/crates/ndc-sqlserver/tests/common/helpers.rs @@ -165,7 +165,7 @@ async fn make_request serde::Deserialize<'a>>( /// Used to check the output of EXPLAIN. We use this method instead of /// snapshot testing because small details (like cost) can change from /// run to run rendering the output unstable. -pub fn is_contained_in_lines(keywords: Vec<&str>, lines: String) { +pub fn is_contained_in_lines(keywords: &[&str], lines: &str) { tracing::info!("expected keywords: {:?}\nlines: {}", keywords, lines,); assert!(keywords.iter().all(|&s| lines.contains(s))); } diff --git a/crates/ndc-sqlserver/tests/explain_tests.rs b/crates/ndc-sqlserver/tests/explain_tests.rs index d0e4e14f..dbd6166f 100644 --- a/crates/ndc-sqlserver/tests/explain_tests.rs +++ b/crates/ndc-sqlserver/tests/explain_tests.rs @@ -5,21 +5,21 @@ pub mod common; #[tokio::test] async fn select_by_pk() { let result = run_explain("select_by_pk").await; - is_contained_in_lines(vec!["Clustered", "Index", "Seek"], result.details.plan); + is_contained_in_lines(&["Clustered", "Index", "Seek"], &result.details.plan); insta::assert_snapshot!(result.details.query); } #[tokio::test] async fn select_where_variable() { let result = run_explain("select_where_variable").await; - is_contained_in_lines(vec!["Clustered", "Index", "Scan"], result.details.plan); + is_contained_in_lines(&["Clustered", "Index", "Scan"], &result.details.plan); insta::assert_snapshot!(result.details.query); } #[tokio::test] async fn select_where_name_nilike() { let result = run_explain("select_where_name_like").await; - let keywords = vec!["Compute", "Scalar"]; - is_contained_in_lines(keywords, result.details.plan); + let keywords = &["Compute", "Scalar"]; + is_contained_in_lines(keywords, &result.details.plan); insta::assert_snapshot!(result.details.query); } diff --git a/crates/ndc-sqlserver/tests/mutation_tests.rs b/crates/ndc-sqlserver/tests/mutation_tests.rs index 41ebd8a7..3dbd1837 100644 --- a/crates/ndc-sqlserver/tests/mutation_tests.rs +++ b/crates/ndc-sqlserver/tests/mutation_tests.rs @@ -64,7 +64,7 @@ mod native_mutations { let _ = run_mutation_fail( "fail_insert_artist_and_return_id", fresh_deployment.connection_uri.clone(), - StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::UNPROCESSABLE_ENTITY, ) .await; diff --git a/crates/ndc-sqlserver/tests/ndc_tests.rs b/crates/ndc-sqlserver/tests/ndc_tests.rs index 850bef5f..c4dc0ff4 100644 --- a/crates/ndc-sqlserver/tests/ndc_tests.rs +++ b/crates/ndc-sqlserver/tests/ndc_tests.rs @@ -36,6 +36,9 @@ mod ndc_tests { ndc_test::test_connector( &ndc_test::configuration::TestConfiguration { + options: ndc_test::configuration::TestOptions { + validate_responses: true, + }, seed: None, snapshots_dir: None, gen_config: Default::default(), diff --git a/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_without_providing_arguments.snap b/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_without_providing_arguments.snap index 8aefe864..46513afe 100644 --- a/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_without_providing_arguments.snap +++ b/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_without_providing_arguments.snap @@ -3,8 +3,6 @@ source: crates/ndc-sqlserver/tests/mutation_tests.rs expression: result --- { - "message": "Invalid request", - "details": { - "detail": "Argument 'CustomerId' not found." - } + "message": "Argument 'CustomerId' not found.", + "details": null } diff --git a/crates/query-engine/execution/Cargo.toml b/crates/query-engine/execution/Cargo.toml index 21077217..7d69166e 100644 --- a/crates/query-engine/execution/Cargo.toml +++ b/crates/query-engine/execution/Cargo.toml @@ -7,6 +7,7 @@ edition.workspace = true workspace = true [dependencies] +ndc-models = { workspace = true } query-engine-sql = { path = "../sql" } query-engine-translation = { path = "../translation" } query-engine-metrics = { path = "../metrics" } diff --git a/crates/query-engine/execution/src/lib.rs b/crates/query-engine/execution/src/lib.rs index bd36ebd9..5e60fbf3 100644 --- a/crates/query-engine/execution/src/lib.rs +++ b/crates/query-engine/execution/src/lib.rs @@ -3,5 +3,6 @@ pub mod error; pub mod helpers; +pub mod metrics; pub mod mutation; pub mod query; diff --git a/crates/query-engine/execution/src/query.rs b/crates/query-engine/execution/src/query.rs index a7bc5501..47d49171 100644 --- a/crates/query-engine/execution/src/query.rs +++ b/crates/query-engine/execution/src/query.rs @@ -74,7 +74,7 @@ async fn execute_queries( pub(crate) async fn execute_query( connection: &mut bb8::PooledConnection<'_, bb8_tiberius::ConnectionManager>, query: &sql::string::SQL, - variables: &BTreeMap, + variables: &BTreeMap, buffer: &mut (impl BufMut + Send), ) -> Result<(), Error> { let query_text = query.sql.as_str(); @@ -88,33 +88,35 @@ pub(crate) async fn execute_query( mssql_query.bind(string); Ok(()) } - sql::string::Param::Variable(var) => match variables.get(&var) { - Some(value) => match value { - serde_json::Value::String(s) => { - mssql_query.bind(s); - Ok(()) - } - serde_json::Value::Number(n) => { - mssql_query.bind(n.as_f64()); - Ok(()) - } - serde_json::Value::Bool(b) => { - mssql_query.bind(*b); - Ok(()) - } - // this is a problem - we don't know the type of the value! - serde_json::Value::Null => Err(Error::Query( - "null variable not currently supported".to_string(), - )), - serde_json::Value::Array(_array) => Err(Error::Query( - "array variable not currently supported".to_string(), - )), - serde_json::Value::Object(_object) => Err(Error::Query( - "object variable not currently supported".to_string(), - )), - }, - None => Err(Error::Query(format!("Variable not found '{}'", var))), - }, + sql::string::Param::Variable(var) => { + match variables.get::(&var.clone().into()) { + Some(value) => match value { + serde_json::Value::String(s) => { + mssql_query.bind(s); + Ok(()) + } + serde_json::Value::Number(n) => { + mssql_query.bind(n.as_f64()); + Ok(()) + } + serde_json::Value::Bool(b) => { + mssql_query.bind(*b); + Ok(()) + } + // this is a problem - we don't know the type of the value! + serde_json::Value::Null => Err(Error::Query( + "null variable not currently supported".to_string(), + )), + serde_json::Value::Array(_array) => Err(Error::Query( + "array variable not currently supported".to_string(), + )), + serde_json::Value::Object(_object) => Err(Error::Query( + "object variable not currently supported".to_string(), + )), + }, + None => Err(Error::Query(format!("Variable not found '{}'", var))), + } + } }? } @@ -185,7 +187,7 @@ pub async fn explain( fn get_query_text( query: &sql::string::SQL, - variables: Option>>, + variables: Option>>, ) -> Result { let empty_map = BTreeMap::new(); let variable_sets = variables.unwrap_or_default(); @@ -199,24 +201,26 @@ fn get_query_text( .try_fold(String::new(), |str, (i, param)| { let type_name: String = match param { sql::string::Param::String(_string) => Ok("VARCHAR(MAX)".to_string()), - sql::string::Param::Variable(var) => match &variables.get(var) { - Some(value) => match value { - serde_json::Value::String(_s) => Ok("VARCHAR(MAX)".to_string()), - serde_json::Value::Number(_n) => Ok("NUMERIC".to_string()), - serde_json::Value::Bool(_b) => Ok("TINYINT".to_string()), - // this is a problem - we don't know the type of the value! - serde_json::Value::Null => Err(Error::Query( - "null variable not currently supported".to_string(), - )), - serde_json::Value::Array(_array) => Err(Error::Query( - "array variable not currently supported".to_string(), - )), - serde_json::Value::Object(_object) => Err(Error::Query( - "object variable not currently supported".to_string(), - )), - }, - None => Err(Error::Query(format!("Variable not found '{}'", var))), - }, + sql::string::Param::Variable(var) => { + match &variables.get::(&var.clone().into()) { + Some(value) => match value { + serde_json::Value::String(_s) => Ok("VARCHAR(MAX)".to_string()), + serde_json::Value::Number(_n) => Ok("NUMERIC".to_string()), + serde_json::Value::Bool(_b) => Ok("TINYINT".to_string()), + // this is a problem - we don't know the type of the value! + serde_json::Value::Null => Err(Error::Query( + "null variable not currently supported".to_string(), + )), + serde_json::Value::Array(_array) => Err(Error::Query( + "array variable not currently supported".to_string(), + )), + serde_json::Value::Object(_object) => Err(Error::Query( + "object variable not currently supported".to_string(), + )), + }, + None => Err(Error::Query(format!("Variable not found '{}'", var))), + } + } }?; Ok(format!("{} DECLARE @P{} {}; ", str, i + 1, type_name)) diff --git a/crates/query-engine/metadata/Cargo.toml b/crates/query-engine/metadata/Cargo.toml index c517863b..9b75bb93 100644 --- a/crates/query-engine/metadata/Cargo.toml +++ b/crates/query-engine/metadata/Cargo.toml @@ -8,5 +8,7 @@ edition.workspace = true workspace = true [dependencies] +ndc-models = { workspace = true } schemars = { version = "0.8.16", features = ["smol_str"] } serde = { version = "1.0.198", features = ["derive"] } +smol_str = { workspace = true } diff --git a/crates/query-engine/metadata/src/metadata/database.rs b/crates/query-engine/metadata/src/metadata/database.rs index 66b9fa0f..547422e6 100644 --- a/crates/query-engine/metadata/src/metadata/database.rs +++ b/crates/query-engine/metadata/src/metadata/database.rs @@ -1,5 +1,7 @@ //! Metadata information regarding the database and tracked information. +use models::{AggregateFunctionName, CollectionName, ComparisonOperatorName, FieldName}; +use ndc_models as models; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, BTreeSet}; @@ -22,7 +24,9 @@ pub enum Type { /// Not all of these are supported for every type. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] -pub struct ComparisonOperators(pub BTreeMap>); +pub struct ComparisonOperators( + pub BTreeMap>, +); /// Represents a postgres binary comparison operator #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -45,7 +49,7 @@ pub enum OperatorKind { /// Mapping from a "table" name to its information. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] -pub struct TablesInfo(pub BTreeMap); +pub struct TablesInfo(pub BTreeMap); /// Information about a database table (or any other kind of relation). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -53,7 +57,7 @@ pub struct TablesInfo(pub BTreeMap); pub struct TableInfo { pub schema_name: String, pub table_name: String, - pub columns: BTreeMap, + pub columns: BTreeMap, #[serde(default)] pub uniqueness_constraints: UniquenessConstraints, #[serde(default)] @@ -106,7 +110,7 @@ pub struct UniquenessConstraints(pub BTreeMap); /// The set of columns that make up a uniqueness constraint. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] -pub struct UniquenessConstraint(pub BTreeSet); +pub struct UniquenessConstraint(pub BTreeSet); /// A mapping from the name of a foreign key constraint to its value. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, JsonSchema)] @@ -118,13 +122,15 @@ pub struct ForeignRelations(pub BTreeMap); #[serde(rename_all = "camelCase")] pub struct ForeignRelation { pub foreign_table: String, - pub column_mapping: BTreeMap, + pub column_mapping: BTreeMap, } /// All supported aggregate functions, grouped by type. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] -pub struct AggregateFunctions(pub BTreeMap>); +pub struct AggregateFunctions( + pub BTreeMap>, +); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] diff --git a/crates/query-engine/metadata/src/metadata/native_queries.rs b/crates/query-engine/metadata/src/metadata/native_queries.rs index 0495bb6d..15223bb0 100644 --- a/crates/query-engine/metadata/src/metadata/native_queries.rs +++ b/crates/query-engine/metadata/src/metadata/native_queries.rs @@ -2,6 +2,7 @@ use super::database::*; +use ndc_models::{ArgumentName, FieldName, ProcedureName}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -17,7 +18,7 @@ pub struct NativeQueries(pub BTreeMap); /// tracked as mutations. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] -pub struct NativeMutations(pub BTreeMap); +pub struct NativeMutations(pub BTreeMap); /// Information about a Native Query. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -26,10 +27,10 @@ pub struct NativeQueryInfo { /** SQL expression to use for the Native Query. We can interpolate values using `{{variable_name}}` syntax, such as `SELECT * FROM authors WHERE name = {{author_name}}` */ pub sql: NativeQuerySql, /** Columns returned by the Native Query */ - pub columns: BTreeMap, + pub columns: BTreeMap, #[serde(default)] /** Names and types of arguments that can be passed to this Native Query */ - pub arguments: BTreeMap, + pub arguments: BTreeMap, #[serde(default)] pub description: Option, } @@ -41,10 +42,10 @@ pub struct NativeMutationInfo { /** SQL expression to use for the Native Query. We can interpolate values using `{{variable_name}}` syntax, such as `SELECT * FROM authors WHERE name = {{author_name}}` */ pub sql: NativeQuerySql, /** Columns returned by the Native Query */ - pub columns: BTreeMap, + pub columns: BTreeMap, #[serde(default)] /** Names and types of arguments that can be passed to this Native Query */ - pub arguments: BTreeMap, + pub arguments: BTreeMap, #[serde(default)] pub description: Option, } @@ -55,7 +56,7 @@ pub enum NativeQueryPart { /// A raw text part Text(String), /// A parameter - Parameter(String), + Parameter(smol_str::SmolStr), } /// A Native Query SQL after parsing. @@ -131,10 +132,10 @@ pub fn parse_native_query(string: &str) -> Vec { None => vec![NativeQueryPart::Text(part.to_string())], Some((var, text)) => { if text.is_empty() { - vec![NativeQueryPart::Parameter(var.to_string())] + vec![NativeQueryPart::Parameter(var.into())] } else { vec![ - NativeQueryPart::Parameter(var.to_string()), + NativeQueryPart::Parameter(var.into()), NativeQueryPart::Text(text.to_string()), ] } @@ -162,7 +163,7 @@ mod tests { parse_native_query("select * from t where {{name}} = name"), vec![ NativeQueryPart::Text("select * from t where ".to_string()), - NativeQueryPart::Parameter("name".to_string()), + NativeQueryPart::Parameter("name".into()), NativeQueryPart::Text(" = name".to_string()), ] ); @@ -174,11 +175,11 @@ mod tests { parse_native_query("select * from t where id = {{id}} and {{name}} = {{other_name}}"), vec![ NativeQueryPart::Text("select * from t where id = ".to_string()), - NativeQueryPart::Parameter("id".to_string()), + NativeQueryPart::Parameter("id".into()), NativeQueryPart::Text(" and ".to_string()), - NativeQueryPart::Parameter("name".to_string()), + NativeQueryPart::Parameter("name".into()), NativeQueryPart::Text(" = ".to_string()), - NativeQueryPart::Parameter("other_name".to_string()), + NativeQueryPart::Parameter("other_name".into()), ] ); } @@ -189,7 +190,7 @@ mod tests { parse_native_query("select * from t where {{name}} = '{name}'"), vec![ NativeQueryPart::Text("select * from t where ".to_string()), - NativeQueryPart::Parameter("name".to_string()), + NativeQueryPart::Parameter("name".into()), NativeQueryPart::Text(" = '{name}'".to_string()), ] ); diff --git a/crates/query-engine/sql/Cargo.toml b/crates/query-engine/sql/Cargo.toml index 0237c774..1720fe1c 100644 --- a/crates/query-engine/sql/Cargo.toml +++ b/crates/query-engine/sql/Cargo.toml @@ -8,4 +8,5 @@ edition.workspace = true workspace = true [dependencies] +ndc-models = { workspace = true } serde_json = "1.0.116" diff --git a/crates/query-engine/sql/src/sql/execution_plan.rs b/crates/query-engine/sql/src/sql/execution_plan.rs index 1bae2f43..8dafb132 100644 --- a/crates/query-engine/sql/src/sql/execution_plan.rs +++ b/crates/query-engine/sql/src/sql/execution_plan.rs @@ -66,7 +66,7 @@ pub struct MutationExecutionPlan { #[derive(Debug)] /// Definition of a query execution plan to be run against the database. pub struct QueryExecutionPlan { - pub variables: Option>>, + pub variables: Option>>, pub root_field: String, /// The query. pub query: sql::ast::Select, @@ -95,7 +95,7 @@ pub fn explain_to_sql(explain: &sql::ast::Explain) -> sql::string::SQL { /// A simple execution plan with only a root field and a query. pub fn simple_exec_plan( - variables: Option>>, + variables: Option>>, root_field: String, query: sql::ast::Select, ) -> QueryExecutionPlan { diff --git a/crates/query-engine/translation/src/translation/helpers.rs b/crates/query-engine/translation/src/translation/helpers.rs index e667dacc..b37678af 100644 --- a/crates/query-engine/translation/src/translation/helpers.rs +++ b/crates/query-engine/translation/src/translation/helpers.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; -use ndc_sdk::models; +use ndc_sdk::models::{self, ArgumentName}; use super::error::Error; use crate::translation::values; @@ -13,7 +13,7 @@ use query_engine_sql::sql; /// Static information from the query and metadata. pub struct Env<'a> { metadata: &'a metadata::Metadata, - relationships: BTreeMap, + relationships: BTreeMap, } #[derive(Debug)] @@ -48,7 +48,7 @@ impl NativeQueries { #[derive(Debug)] pub struct MutationOperation { pub name: String, - pub arguments: BTreeMap, + pub arguments: BTreeMap, pub fields: Option, pub kind: MutationOperationKind, } @@ -63,7 +63,7 @@ pub enum MutationOperationKind { /// Information we store about a native query call. pub struct NativeQueryInfo { pub info: metadata::NativeQueryInfo, - pub arguments: BTreeMap, + pub arguments: BTreeMap, pub alias: sql::ast::TableAlias, } @@ -151,8 +151,8 @@ pub enum CollectionOrProcedureInfo { /// in the parameterized SQL statement and returns /// a SQL statement that can be run in the DB. pub fn generate_native_query_sql( - type_arguments: &BTreeMap, - native_query_arguments: &BTreeMap, + type_arguments: &BTreeMap, + native_query_arguments: &BTreeMap, native_query_sql: &NativeQuerySql, ) -> Result, Error> { native_query_sql @@ -162,18 +162,18 @@ pub fn generate_native_query_sql( metadata::NativeQueryPart::Text(text) => Ok(sql::ast::RawSql::RawText(text.clone())), metadata::NativeQueryPart::Parameter(param) => { let typ = match type_arguments.get(param) { - None => Err(Error::ArgumentNotFound(param.clone())), + None => Err(Error::ArgumentNotFound(param.to_string())), Some(argument) => Ok(argument.r#type.clone()), }?; - let exp = match native_query_arguments.get(param) { - None => Err(Error::ArgumentNotFound(param.clone())), + let exp = match native_query_arguments.get(param.as_str()) { + None => Err(Error::ArgumentNotFound(param.to_string())), Some(argument) => match argument { models::Argument::Literal { value } => { values::translate_json_value(value, &typ) } models::Argument::Variable { name } => { - Ok(values::translate_variable(name.clone(), &typ)) + Ok(values::translate_variable(name, &typ)) } }, }?; @@ -187,7 +187,7 @@ impl<'a> Env<'a> { /// Create a new Env by supplying the metadata and relationships. pub fn new( metadata: &'a metadata::Metadata, - relationships: BTreeMap, + relationships: BTreeMap, ) -> Env { Env { metadata, @@ -228,13 +228,13 @@ impl<'a> Env<'a> { /// Lookup a collection's information in the metadata. pub fn lookup_collection( &self, - collection_name: &str, + collection_name: &models::CollectionName, ) -> Result { let table = self .metadata .tables .0 - .get(collection_name) + .get(collection_name.as_str()) .map(|t| CollectionInfo::Table { name: collection_name.to_string(), info: t.clone(), @@ -243,20 +243,20 @@ impl<'a> Env<'a> { match table { Some(table) => Ok(CollectionOrProcedureInfo::Collection(table)), None => { - let proc_maybe = self.lookup_procedure(collection_name); + let proc_maybe = self.lookup_procedure(collection_name.as_str()); match proc_maybe { Some(proc_info) => Ok(CollectionOrProcedureInfo::Procedure(proc_info)), None => { - let native_query = - self.metadata - .native_queries - .0 - .get(collection_name) - .map(|nq| CollectionInfo::NativeQuery { - name: collection_name.to_string(), - info: nq.clone(), - }); + let native_query = self + .metadata + .native_queries + .0 + .get(collection_name.as_str()) + .map(|nq| CollectionInfo::NativeQuery { + name: collection_name.to_string(), + info: nq.clone(), + }); // FIXME(KC): THis is terrible. Please refactor this. match native_query { Some(native_query) => { @@ -266,7 +266,7 @@ impl<'a> Env<'a> { .metadata .native_mutations .0 - .get(collection_name) + .get(collection_name.as_str()) .map(|nq| CollectionInfo::NativeMutation { name: collection_name.to_string(), info: nq.clone(), @@ -280,9 +280,12 @@ impl<'a> Env<'a> { } } - pub fn lookup_relationship(&self, name: &str) -> Result<&models::Relationship, Error> { + pub fn lookup_relationship( + &self, + name: &models::RelationshipName, + ) -> Result<&models::Relationship, Error> { self.relationships - .get(name) + .get(name.as_str()) .ok_or(Error::RelationshipNotFound(name.to_string())) } @@ -290,28 +293,28 @@ impl<'a> Env<'a> { pub fn lookup_comparison_operator( &self, scalar_type: &metadata::ScalarType, - name: &String, + name: &models::ComparisonOperatorName, ) -> Result<&'a metadata::ComparisonOperator, Error> { self.metadata .comparison_operators .0 .get(scalar_type) - .and_then(|ops| ops.get(name)) + .and_then(|ops| ops.get(name.as_str())) .ok_or(Error::OperatorNotFound { - operator_name: name.clone(), + operator_name: name.to_string(), type_name: scalar_type.clone(), }) } } impl CollectionOrProcedureInfo { - pub fn lookup_column(&self, column_name: &str) -> Result { + pub fn lookup_column(&self, column_name: &models::FieldName) -> Result { match &self { CollectionOrProcedureInfo::Collection(collection_info) => { - collection_info.lookup_column(column_name) + collection_info.lookup_column(column_name.as_str()) } CollectionOrProcedureInfo::Procedure(procedure_info) => { - procedure_info.lookup_column(column_name) + procedure_info.lookup_column(column_name.as_str()) } } } @@ -408,7 +411,7 @@ impl State { &mut self, name: &str, info: metadata::NativeQueryInfo, - arguments: BTreeMap, + arguments: BTreeMap, ) -> sql::ast::TableReference { let alias = self.make_native_query_table_alias(name); self.native_queries.native_queries.push(NativeQueryInfo { @@ -465,10 +468,7 @@ impl State { /// Create a table alias for order by target part. /// Provide an index and a source table name (to disambiguate the table being queried), /// and get an alias. - pub fn make_order_path_part_table_alias( - &mut self, - table_name: &String, - ) -> sql::ast::TableAlias { + pub fn make_order_path_part_table_alias(&mut self, table_name: &str) -> sql::ast::TableAlias { self.make_table_alias(format!("ORDER_PART_{}", table_name)) } diff --git a/crates/query-engine/translation/src/translation/mutation/mod.rs b/crates/query-engine/translation/src/translation/mutation/mod.rs index 6414f3c0..e562e931 100644 --- a/crates/query-engine/translation/src/translation/mutation/mod.rs +++ b/crates/query-engine/translation/src/translation/mutation/mod.rs @@ -61,24 +61,25 @@ fn translate_mutation_operation( fields, } => { let procedure_info: ProcedureInfo = env - .lookup_procedure(&name) - .ok_or_else(|| Error::ProcedureNotFound(name.clone()))?; + .lookup_procedure(name.as_str()) + .ok_or_else(|| Error::ProcedureNotFound(name.to_string()))?; let mutation_operation_kind = match procedure_info { ProcedureInfo::NativeMutation { info, .. } => { MutationOperationKind::NativeMutation(NativeMutationInfo { - name: name.clone(), + name: name.to_string(), info, }) } ProcedureInfo::StoredProcedure { name, info } => { - MutationOperationKind::StoredProcedure(super::helpers::StoredProcedureInfo { - name: name.clone(), + let stored_procedure_info = super::helpers::StoredProcedureInfo { + name: name.to_string(), info, - }) + }; + MutationOperationKind::StoredProcedure(stored_procedure_info) } }; Ok(MutationOperation { - name, + name: name.to_string(), arguments, fields, kind: mutation_operation_kind, @@ -97,25 +98,39 @@ pub fn parse_procedure_fields( fields: Option, ) -> Result< ( - (String, Option>), // Contains "affected_rows" - (String, Option>), // Contains "returning" + ( + models::FieldName, + Option>, + ), // Contains "affected_rows" + ( + models::FieldName, + Option>, + ), // Contains "returning" ), Error, > { match fields { Some(models::NestedField::Object(models::NestedObject { fields })) => { - let mut affected_rows = ("affected_rows".to_string(), None); - let mut returning = ("returning".to_string(), None); + let mut affected_rows = ("affected_rows".into(), None); + let mut returning = ("returning".into(), None); for (alias, field) in fields { match field { - models::Field::Column { column, fields: _ } if column == "affected_rows" => { + models::Field::Column { + column, + fields: _, + arguments: _, + } if column.as_str() == "affected_rows" => { affected_rows = ( alias.clone(), Some(indexmap!(alias => models::Aggregate::StarCount {})), ); } - models::Field::Column { column, fields } if column == "returning" => { + models::Field::Column { + column, + fields, + arguments: _, + } if column.as_str() == "returning" => { returning = match fields { Some(nested_fields) => match nested_fields { models::NestedField::Object(models::NestedObject { .. }) => { @@ -312,7 +327,8 @@ fn generate_mutation_execution_plan( alias: json_response_cte_alias.clone(), }; - let procedure_info = env.lookup_collection(&native_mutation_info.name)?; + let procedure_info = + env.lookup_collection(&native_mutation_info.name.clone().into())?; let select_set = query::translate_query( env, @@ -333,11 +349,11 @@ fn generate_mutation_execution_plan( ), ( state.make_table_alias("rows".to_string()), - sql::helpers::make_column_alias(returning_alias), + sql::helpers::make_column_alias(returning_alias.to_string()), ), vec![], state.make_table_alias("aggregates".to_string()), - make_column_alias(affected_rows.0), + make_column_alias(affected_rows.0.to_string()), select_set, ); diff --git a/crates/query-engine/translation/src/translation/mutation/stored_procedures.rs b/crates/query-engine/translation/src/translation/mutation/stored_procedures.rs index 0e60ea0e..4d1cc0e0 100644 --- a/crates/query-engine/translation/src/translation/mutation/stored_procedures.rs +++ b/crates/query-engine/translation/src/translation/mutation/stored_procedures.rs @@ -21,7 +21,7 @@ use query_engine_sql::sql::{ fn parse_stored_procedure_fields( fields: Option, -) -> Result>, Error> { +) -> Result>, Error> { match fields { None => Ok(None), Some(models::NestedField::Object(_)) => Err(Error::UnexpectedStructure( @@ -38,13 +38,14 @@ fn parse_stored_procedure_fields( fn get_all_procedure_fields( proc_fields: BTreeMap, -) -> IndexMap { +) -> IndexMap { let mut fields = IndexMap::new(); for (proc_field_name, proc_field_col_info) in proc_fields { fields.insert( - proc_field_name, + proc_field_name.into(), models::Field::Column { - column: proc_field_col_info.name, + arguments: BTreeMap::new(), + column: proc_field_col_info.name.into(), fields: None, }, ); @@ -57,7 +58,7 @@ pub(crate) fn generate_execution_plan( state: &mut State, stored_proc_info: StoredProcedureInfo, requested_fields: Option, - provided_args: &BTreeMap, + provided_args: &BTreeMap, ) -> Result { // Compute the fields that need to be returned. let parsed_fields = parse_stored_procedure_fields(requested_fields)?.unwrap_or( @@ -72,7 +73,8 @@ pub(crate) fn generate_execution_plan( // Process the arguments provided and convert it into // an `Expression` for (arg_name, arg_info) in stored_proc_info.info.arguments { - let arg_val = provided_args.get(&arg_name); + let arg_val: Option<&serde_json::Value> = + provided_args.get::(&arg_name.clone().into()); match arg_val { Some(arg_val) if *arg_val != serde_json::Value::Null => { diff --git a/crates/query-engine/translation/src/translation/query/aggregates.rs b/crates/query-engine/translation/src/translation/query/aggregates.rs index 3be06b5a..dedded1f 100644 --- a/crates/query-engine/translation/src/translation/query/aggregates.rs +++ b/crates/query-engine/translation/src/translation/query/aggregates.rs @@ -10,14 +10,16 @@ use query_engine_sql::sql; /// Translate any aggregates we should include in the query into our SQL AST. pub fn translate( table: &sql::ast::TableReference, - aggregates: &IndexMap, + aggregates: &IndexMap, ) -> Result, Error> { aggregates .into_iter() .map(|(alias, aggregation)| { let expression = match aggregation { - models::Aggregate::ColumnCount { column, distinct } => { - let count_column_alias = sql::helpers::make_column_alias(column.clone()); + models::Aggregate::ColumnCount { + column, distinct, .. + } => { + let count_column_alias = sql::helpers::make_column_alias(column.to_string()); if *distinct { sql::ast::Expression::Count(sql::ast::CountType::Distinct( sql::ast::ColumnReference::AliasedColumn { @@ -34,23 +36,27 @@ pub fn translate( )) } } - models::Aggregate::SingleColumn { column, function } => { + models::Aggregate::SingleColumn { + column, + function, + field_path: _, + } => { let column_ref_expression = sql::ast::Expression::ColumnReference( sql::ast::ColumnReference::AliasedColumn { table: table.clone(), - column: sql::helpers::make_column_alias(column.clone()), + column: sql::helpers::make_column_alias(column.to_string()), }, ); match function.as_str() { "SUM" | "AVG" => sql::ast::Expression::FunctionCall { - function: sql::ast::Function::Unknown(function.clone()), + function: sql::ast::Function::Unknown(function.to_string()), args: vec![sql::ast::Expression::Cast { expression: Box::new(column_ref_expression), r#type: sql::ast::ScalarType("BIGINT".to_string()), }], }, _ => sql::ast::Expression::FunctionCall { - function: sql::ast::Function::Unknown(function.clone()), + function: sql::ast::Function::Unknown(function.to_string()), args: vec![column_ref_expression], }, } @@ -59,7 +65,10 @@ pub fn translate( sql::ast::Expression::Count(sql::ast::CountType::Star) } }; - Ok((sql::helpers::make_column_alias(alias.clone()), expression)) + Ok(( + sql::helpers::make_column_alias(alias.to_string()), + expression, + )) }) .collect::, Error>>() } diff --git a/crates/query-engine/translation/src/translation/query/filtering.rs b/crates/query-engine/translation/src/translation/query/filtering.rs index cf2a9bca..1ee2ad3e 100644 --- a/crates/query-engine/translation/src/translation/query/filtering.rs +++ b/crates/query-engine/translation/src/translation/query/filtering.rs @@ -338,12 +338,16 @@ fn translate_comparison_target( column: &models::ComparisonTarget, ) -> Result<(sql::ast::Expression, Vec), Error> { match column { - models::ComparisonTarget::Column { name, path } => { + models::ComparisonTarget::Column { + name, + path, + field_path: _, + } => { let (table_ref, joins) = translate_comparison_pathelements(env, state, root_and_current_tables, path)?; // get the unrelated table information from the metadata. - let collection_info = env.lookup_collection(&table_ref.name)?; + let collection_info = env.lookup_collection(&table_ref.name.into())?; let ColumnInfo { name, .. } = collection_info.lookup_column(name)?; Ok(( @@ -356,10 +360,13 @@ fn translate_comparison_target( } // Compare a column from the root table. - models::ComparisonTarget::RootCollectionColumn { name } => { + models::ComparisonTarget::RootCollectionColumn { + name, + field_path: _, + } => { let RootAndCurrentTables { root_table, .. } = root_and_current_tables; // get the unrelated table information from the metadata. - let collection_info = env.lookup_collection(&root_table.name)?; + let collection_info = env.lookup_collection(&root_table.name.clone().into())?; // find the requested column in the tables columns. let ColumnInfo { name, .. } = collection_info.lookup_column(name)?; @@ -391,7 +398,7 @@ fn translate_comparison_value( Ok((values::translate_json_value(json_value, typ)?, vec![])) } models::ComparisonValue::Variable { name: var } => { - Ok((values::translate_variable(var.clone(), typ), vec![])) + Ok((values::translate_variable(var, typ), vec![])) } } } @@ -407,6 +414,11 @@ pub fn translate_exists_in_collection( predicate: &models::Expression, ) -> Result { match in_collection { + models::ExistsInCollection::NestedCollection { + column_name: _, + arguments: _, + field_path: _, + } => todo!("Not implemented"), models::ExistsInCollection::Unrelated { collection, arguments, @@ -418,7 +430,7 @@ pub fn translate_exists_in_collection( }, )?; - let table_alias = state.make_table_alias(collection.clone()); + let table_alias = state.make_table_alias(collection.to_string()); // create a from clause and get a reference of inner query. let (table, from_clause) = root::make_from_clause_and_reference( @@ -477,7 +489,7 @@ pub fn translate_exists_in_collection( }, )?; - let table_alias = state.make_table_alias(relationship.target_collection.clone()); + let table_alias = state.make_table_alias(relationship.target_collection.to_string()); // create a from clause and get a reference of inner query. let (table, from_clause) = root::make_from_clause_and_reference( @@ -540,16 +552,23 @@ fn get_comparison_target_type( column: &models::ComparisonTarget, ) -> Result { match column { - models::ComparisonTarget::RootCollectionColumn { name } => { + models::ComparisonTarget::RootCollectionColumn { + name, + field_path: _, + } => { let column = env - .lookup_collection(&root_and_current_tables.root_table.name)? + .lookup_collection(&root_and_current_tables.root_table.name.clone().into())? .lookup_column(name)?; Ok(column.r#type) } - models::ComparisonTarget::Column { name, path } => match path.last() { + models::ComparisonTarget::Column { + name, + path, + field_path: _, + } => match path.last() { None => { let column = env - .lookup_collection(&root_and_current_tables.current_table.name)? + .lookup_collection(&root_and_current_tables.current_table.name.clone().into())? .lookup_column(name)?; Ok(column.r#type) } diff --git a/crates/query-engine/translation/src/translation/query/mod.rs b/crates/query-engine/translation/src/translation/query/mod.rs index a377f12b..61efcff9 100644 --- a/crates/query-engine/translation/src/translation/query/mod.rs +++ b/crates/query-engine/translation/src/translation/query/mod.rs @@ -22,7 +22,7 @@ pub fn translate( ) -> Result { let env = Env::new(metadata, query_request.collection_relationships); let mut state = State::new(); - let table_alias = state.make_table_alias(query_request.collection.clone()); + let table_alias = state.make_table_alias(query_request.collection.to_string()); let (current_table, from_clause) = root::make_from_clause_and_reference( &query_request.collection, &query_request.arguments, @@ -62,7 +62,7 @@ pub fn translate( Ok(sql::execution_plan::simple_exec_plan( query_request.variables, - query_request.collection, + query_request.collection.to_string(), json_select, )) } diff --git a/crates/query-engine/translation/src/translation/query/relationships.rs b/crates/query-engine/translation/src/translation/query/relationships.rs index bc1bbbbe..49b22492 100644 --- a/crates/query-engine/translation/src/translation/query/relationships.rs +++ b/crates/query-engine/translation/src/translation/query/relationships.rs @@ -13,7 +13,7 @@ pub struct JoinFieldInfo { pub table_alias: sql::ast::TableAlias, pub column_alias: sql::ast::ColumnAlias, pub relationship_name: String, - pub arguments: BTreeMap, + pub arguments: BTreeMap, pub query: models::Query, } @@ -29,13 +29,13 @@ pub fn translate_joins( join_fields .into_iter() .map(|join_field| { - let relationship = env.lookup_relationship(&join_field.relationship_name)?; + let relationship = env.lookup_relationship(&join_field.relationship_name.into())?; let arguments = make_relationship_arguments(MakeRelationshipArguments { caller_arguments: join_field.arguments, relationship_arguments: relationship.arguments.clone(), })?; - let table_alias = state.make_table_alias(relationship.target_collection.clone()); + let table_alias = state.make_table_alias(relationship.target_collection.to_string()); // create a from clause and get a reference of inner query. let (target_collection, from_clause) = root::make_from_clause_and_reference( @@ -46,7 +46,8 @@ pub fn translate_joins( &table_alias, )?; - let target_collection_info = env.lookup_collection(&target_collection.name)?; + let target_collection_info = + env.lookup_collection(&target_collection.name.clone().into())?; // process inner query and get the SELECTs for the 'rows' and 'aggregates' fields. let select_set = super::translate_query( @@ -154,7 +155,7 @@ pub fn translate_column_mapping( expr: sql::ast::Expression, relationship: &models::Relationship, ) -> Result { - let table_info = env.lookup_collection(¤t_table.name)?; + let table_info = env.lookup_collection(¤t_table.name.clone().into())?; let target_collection_info = env.lookup_collection(&relationship.target_collection)?; @@ -192,8 +193,8 @@ pub fn translate_column_mapping( #[derive(Debug)] /// Used in `make_relationship_arguments()` below. pub struct MakeRelationshipArguments { - pub relationship_arguments: BTreeMap, - pub caller_arguments: BTreeMap, + pub relationship_arguments: BTreeMap, + pub caller_arguments: BTreeMap, } /// Combine the caller arguments and the relationship arguments into a single map. @@ -202,20 +203,20 @@ pub struct MakeRelationshipArguments { /// and throw an error on the column case. Will be fixed in the future. pub fn make_relationship_arguments( arguments: MakeRelationshipArguments, -) -> Result, Error> { +) -> Result, Error> { // these are arguments defined in the relationship definition. - let relationship_arguments: BTreeMap = arguments + let relationship_arguments: BTreeMap = arguments .relationship_arguments .into_iter() .map(|(key, argument)| Ok((key, relationship_argument_to_argument(argument)?))) - .collect::, Error>>()?; + .collect::, Error>>()?; // these are arguments defined when calling the relationship. - let caller_arguments: BTreeMap = arguments + let caller_arguments: BTreeMap = arguments .caller_arguments .into_iter() .map(|(key, argument)| Ok((key, relationship_argument_to_argument(argument)?))) - .collect::, Error>>()?; + .collect::, Error>>()?; let mut arguments = relationship_arguments; @@ -225,7 +226,7 @@ pub fn make_relationship_arguments( for (key, value) in caller_arguments { match arguments.insert(key.clone(), value) { None => Ok(()), - Some(_) => Err(Error::RelationshipArgumentWasOverriden(key)), + Some(_) => Err(Error::RelationshipArgumentWasOverriden(key.to_string())), }?; } diff --git a/crates/query-engine/translation/src/translation/query/root.rs b/crates/query-engine/translation/src/translation/query/root.rs index 6df53af2..db4546d2 100644 --- a/crates/query-engine/translation/src/translation/query/root.rs +++ b/crates/query-engine/translation/src/translation/query/root.rs @@ -190,7 +190,7 @@ pub fn translate_rows_query( Ok(sql::helpers::make_column( current_table.reference.clone(), column_info.name.clone(), - sql::helpers::make_column_alias(alias), + sql::helpers::make_column_alias(alias.to_string()), )) } models::Field::Relationship { @@ -198,8 +198,9 @@ pub fn translate_rows_query( relationship, arguments, } => { - let table_alias = state.make_relationship_table_alias(&alias); - let column_alias = sql::helpers::make_column_alias(alias); + let table_alias = + state.make_relationship_table_alias(&alias.clone().into()); + let column_alias = sql::helpers::make_column_alias(alias.to_string()); let json_column_alias = sql::helpers::make_json_column_alias(); let column_name = sql::ast::ColumnReference::AliasedColumn { table: sql::ast::TableReference::AliasedTable(table_alias.clone()), @@ -208,7 +209,7 @@ pub fn translate_rows_query( join_fields.push(relationships::JoinFieldInfo { table_alias, column_alias: column_alias.clone(), - relationship_name: relationship, + relationship_name: relationship.to_string(), arguments, query: *query, }); @@ -346,8 +347,8 @@ fn translate_query_part( /// Create a from clause from a collection name and its reference. pub fn make_from_clause_and_reference( - collection_name: &str, - arguments: &BTreeMap, + collection_name: &models::CollectionName, + arguments: &BTreeMap, env: &Env, state: &mut State, collection_alias: &sql::ast::TableAlias, @@ -371,7 +372,7 @@ fn make_from_clause( state: &mut State, current_table_alias: &sql::ast::TableAlias, collection_info: &CollectionOrProcedureInfo, - arguments: &BTreeMap, + arguments: &BTreeMap, ) -> Result { match &collection_info { CollectionOrProcedureInfo::Collection(collection_info) => match collection_info { diff --git a/crates/query-engine/translation/src/translation/query/sorting.rs b/crates/query-engine/translation/src/translation/query/sorting.rs index 7523446a..9fb3cce9 100644 --- a/crates/query-engine/translation/src/translation/query/sorting.rs +++ b/crates/query-engine/translation/src/translation/query/sorting.rs @@ -87,7 +87,11 @@ pub fn translate_order_by( .iter() .map(|order_by| { let target = match &order_by.target { - models::OrderByTarget::Column { name, path } => translate_order_by_target( + models::OrderByTarget::Column { + name, + path, + field_path: _, + } => translate_order_by_target( env, state, root_and_current_tables, @@ -100,6 +104,7 @@ pub fn translate_order_by( column, function, path, + field_path: _, } => translate_order_by_target( env, state, @@ -184,7 +189,7 @@ fn translate_order_by_star_count_aggregate( let relationship = env.lookup_relationship(&path_element.relationship)?; let target_collection_alias = - state.make_table_alias(relationship.target_collection.clone()); + state.make_table_alias(relationship.target_collection.to_string()); let (table, from_clause) = from_for_path_element( env, @@ -229,17 +234,17 @@ fn translate_order_by_target( env: &Env, state: &mut State, root_and_current_tables: &RootAndCurrentTables, - (column, path): (&str, &Vec), + (column, path): (&models::FieldName, &Vec), // we expect function to be derived derived from the schema we publish by v3-engine, // so no sql injection shenanigans should be possible. - function: Option, + function: Option, joins: &mut Vec, ) -> Result { let column_or_relationship_select = translate_order_by_target_for_column( env, state, root_and_current_tables, - column, + column.as_str(), path, function, )?; @@ -297,7 +302,7 @@ fn translate_order_by_target_for_column( root_and_current_tables: &RootAndCurrentTables, column_name: &str, path: &[models::PathElement], - function: Option, + function: Option, ) -> Result { // We want to build a select query where "Track" is the root table, and "Artist"."Name" // is the column we need for the order by. Our query will look like this: @@ -341,8 +346,9 @@ fn translate_order_by_target_for_column( if path.is_empty() { // if there were no relationship columns, we don't need to build a query, just return the column. - let table = env.lookup_collection(&root_and_current_tables.current_table.name)?; - let selected_column = table.lookup_column(column_name)?; + let table = + env.lookup_collection(&root_and_current_tables.current_table.name.clone().into())?; + let selected_column = table.lookup_column(&column_name.into())?; let selected_column_name = sql::ast::ColumnReference::AliasedColumn { table: root_and_current_tables.current_table.reference.clone(), @@ -356,8 +362,8 @@ fn translate_order_by_target_for_column( // for the order by, and build a select of all the joins to select from. else { // order by columns - let table = env.lookup_collection(&last_table.name)?; - let selected_column = table.lookup_column(column_name)?; + let table = env.lookup_collection(&last_table.name.into())?; + let selected_column = table.lookup_column(&column_name.into())?; let selected_column_name = sql::ast::ColumnReference::AliasedColumn { table: last_table.reference, @@ -371,7 +377,7 @@ fn translate_order_by_target_for_column( let selected_column_expr = match function { None => sql::ast::Expression::ColumnReference(selected_column_name.clone()), Some(func) => sql::ast::Expression::FunctionCall { - function: sql::ast::Function::Unknown(func), + function: sql::ast::Function::Unknown(func.to_string()), args: vec![sql::ast::Expression::ColumnReference( selected_column_name.clone(), )], @@ -420,7 +426,7 @@ fn process_path_element_for_order_by_target_for_column( root_and_current_tables: &RootAndCurrentTables, target_column_name: &str, path: &[models::PathElement], - aggregate_function_for_arrays: &Option, + aggregate_function_for_arrays: &Option, // to get the information about this path element we need to select from the relevant table // and join with the previous table. We add a new join to this list of joins. joins: &mut Vec, @@ -439,7 +445,7 @@ fn process_path_element_for_order_by_target_for_column( }?; let target_collection_alias = - state.make_order_path_part_table_alias(&relationship.target_collection); + state.make_order_path_part_table_alias(relationship.target_collection.as_str()); let (table, from_clause) = from_for_path_element( env, @@ -458,7 +464,7 @@ fn process_path_element_for_order_by_target_for_column( .column_mapping .keys() .map(|source_col| { - let collection = env.lookup_collection(&table.name)?; + let collection = env.lookup_collection(&table.name.clone().into())?; let selected_column = collection.lookup_column(source_col)?; // we are going to deliberately use the table column name and not an alias we get from // the query request because this is internal to the sorting mechanism. @@ -479,7 +485,7 @@ fn process_path_element_for_order_by_target_for_column( } None => { let target_collection = env.lookup_collection(&relationship.target_collection)?; - let selected_column = target_collection.lookup_column(target_column_name)?; + let selected_column = target_collection.lookup_column(&target_column_name.into())?; // we are going to deliberately use the table column name and not an alias we get from // the query request because this is internal to the sorting mechanism. let selected_column_alias = @@ -529,7 +535,7 @@ fn from_for_path_element( state: &mut State, relationship: &models::Relationship, target_collection_alias: &sql::ast::TableAlias, - arguments: &std::collections::BTreeMap, + arguments: &std::collections::BTreeMap, ) -> Result<(TableNameAndReference, sql::ast::From), Error> { let arguments = relationships::make_relationship_arguments(relationships::MakeRelationshipArguments { diff --git a/crates/query-engine/translation/src/translation/values.rs b/crates/query-engine/translation/src/translation/values.rs index 69477668..963c8d38 100644 --- a/crates/query-engine/translation/src/translation/values.rs +++ b/crates/query-engine/translation/src/translation/values.rs @@ -2,6 +2,7 @@ use crate::translation::error::Error; +use ndc_sdk::models; use query_engine_metadata::metadata::database; use query_engine_sql::sql; use sql::ast::{Expression, Value}; @@ -40,10 +41,10 @@ pub fn translate_json_value( /// Convert a variable into a SQL value. pub fn translate_variable( - variable: String, + variable: &models::VariableName, scalar_type: &database::ScalarType, ) -> sql::ast::Expression { - let exp = Expression::Value(Value::Variable(variable)); + let exp = Expression::Value(Value::Variable(variable.to_string())); sql::ast::Expression::Cast { expression: Box::new(exp), diff --git a/static/configuration.json b/static/configuration.json index 7cf07dde..9d5d4cf8 100644 --- a/static/configuration.json +++ b/static/configuration.json @@ -2728,24 +2728,24 @@ } }, "returns": { - "CustomerId": { - "name": "CustomerId", - "type": "int", - "nullable": "nonNullable", - "description": null - }, - "Phone": { - "name": "Phone", - "type": "varchar", - "nullable": "nonNullable", - "description": null - }, - "TotalPurchases": { - "name": "TotalPurchases", - "type": "int", - "nullable": "nonNullable", - "description": null - } + "CustomerId": { + "name": "CustomerId", + "type": "int", + "nullable": "nonNullable", + "description": null + }, + "Phone": { + "name": "Phone", + "type": "varchar", + "nullable": "nonNullable", + "description": null + }, + "TotalPurchases": { + "name": "TotalPurchases", + "type": "int", + "nullable": "nonNullable", + "description": null + } }, "description": null },