From 0bc306ebdcdb88c93b6fd25d43855afc7e4f07fb Mon Sep 17 00:00:00 2001 From: Flavian Desverne Date: Fri, 23 Aug 2024 19:05:48 +0200 Subject: [PATCH] fix(typedsql): mysql should work with `@param` notations (#4985) --- .../sql-schema-connector/src/lib.rs | 7 +- .../src/sql_doc_parser.rs | 34 ++++++++++ .../tests/query_introspection/docs.rs | 66 +++++++++++++++++-- 3 files changed, 99 insertions(+), 8 deletions(-) diff --git a/schema-engine/connectors/sql-schema-connector/src/lib.rs b/schema-engine/connectors/sql-schema-connector/src/lib.rs index f78ac9b60fe..3641eeb9633 100644 --- a/schema-engine/connectors/sql-schema-connector/src/lib.rs +++ b/schema-engine/connectors/sql-schema-connector/src/lib.rs @@ -23,7 +23,7 @@ use migration_pair::MigrationPair; use psl::{datamodel_connector::NativeTypeInstance, parser_database::ScalarType, ValidatedSchema}; use quaint::connector::DescribedQuery; use schema_connector::{migrations_directory::MigrationDirectory, *}; -use sql_doc_parser::parse_sql_doc; +use sql_doc_parser::{parse_sql_doc, sanitize_sql}; use sql_migration::{DropUserDefinedType, DropView, SqlMigration, SqlMigrationStep}; use sql_schema_describer as sql; use std::{future, sync::Arc}; @@ -362,11 +362,12 @@ impl SchemaConnector for SqlSchemaConnector { input: IntrospectSqlQueryInput, ) -> BoxFuture<'_, ConnectorResult> { Box::pin(async move { + let sanitized_sql = sanitize_sql(&input.source); let DescribedQuery { parameters, columns, enum_names, - } = self.flavour.describe_query(&input.source).await?; + } = self.flavour.describe_query(&sanitized_sql).await?; let enum_names = enum_names.unwrap_or_default(); let sql_source = input.source.clone(); let parsed_doc = parse_sql_doc(&sql_source, enum_names.as_slice())?; @@ -397,7 +398,7 @@ impl SchemaConnector for SqlSchemaConnector { Ok(IntrospectSqlQueryOutput { name: input.name, - source: input.source, + source: sanitized_sql, documentation: parsed_doc.description().map(ToOwned::to_owned), parameters, result_columns: columns, diff --git a/schema-engine/connectors/sql-schema-connector/src/sql_doc_parser.rs b/schema-engine/connectors/sql-schema-connector/src/sql_doc_parser.rs index a7f9c4912d2..819621512b3 100644 --- a/schema-engine/connectors/sql-schema-connector/src/sql_doc_parser.rs +++ b/schema-engine/connectors/sql-schema-connector/src/sql_doc_parser.rs @@ -347,6 +347,15 @@ pub(crate) fn parse_sql_doc<'a>(sql: &'a str, enum_names: &'a [String]) -> Conne Ok(parsed_sql) } +/// Mysql-async poorly parses the sql input to support named parameters, which conflicts with our own syntax for overriding query parameters type and nullability. +/// This function removes all single-line comments from the sql input to avoid conflicts. +pub(crate) fn sanitize_sql(sql: &str) -> String { + sql.lines() + .map(|line| line.trim()) + .filter(|line| !line.starts_with("--")) + .join("\n") +} + #[cfg(test)] mod tests { use super::*; @@ -1107,4 +1116,29 @@ SELECT enum FROM "test_introspect_sql"."model" WHERE id = $1;"#, expected.assert_debug_eq(&res); } + + #[test] + fn sanitize_sql_test_1() { + use expect_test::expect; + + let sql = r#" + -- @description This query returns a user by it's id + -- @param {Int} $1:userId valid user identifier + -- @param {String} $2:parentId valid parent identifier + SELECT enum + FROM + "test_introspect_sql"."model" WHERE id = + $1; + "#; + + let expected = expect![[r#" + + SELECT enum + FROM + "test_introspect_sql"."model" WHERE id = + $1; + "#]]; + + expected.assert_eq(&sanitize_sql(sql)); + } } diff --git a/schema-engine/sql-migration-tests/tests/query_introspection/docs.rs b/schema-engine/sql-migration-tests/tests/query_introspection/docs.rs index e7fe7bc630c..33eb9f50161 100644 --- a/schema-engine/sql-migration-tests/tests/query_introspection/docs.rs +++ b/schema-engine/sql-migration-tests/tests/query_introspection/docs.rs @@ -2,13 +2,13 @@ use super::utils::*; use sql_migration_tests::test_api::*; #[test_connector(tags(Postgres))] -fn parses_doc_complex(api: TestApi) { +fn parses_doc_complex_pg(api: TestApi) { api.schema_push(SIMPLE_SCHEMA).send().assert_green(); let expected = expect![[r#" IntrospectSqlQueryOutput { name: "test_1", - source: "\n -- @description some fancy query\n -- @param {Int} $1:myInt some integer\n -- @param {String}$2:myString? some string\n SELECT int FROM model WHERE int = $1 and string = $2;\n ", + source: "\nSELECT int FROM model WHERE int = $1 and string = $2;\n", documentation: Some( "some fancy query", ), @@ -50,6 +50,62 @@ fn parses_doc_complex(api: TestApi) { api.introspect_sql("test_1", sql).send_sync().expect_result(expected) } +#[test_connector(tags(Mysql))] +fn parses_doc_complex_mysql(api: TestApi) { + api.schema_push(SIMPLE_SCHEMA).send().assert_green(); + + let expected = expect![[r#" + IntrospectSqlQueryOutput { + name: "test_1", + source: "\nSELECT `int` FROM `model` WHERE `int` = ? and `string` = ?;\n", + documentation: Some( + "some fancy query", + ), + parameters: [ + IntrospectSqlQueryParameterOutput { + documentation: Some( + "some integer", + ), + name: "myInt", + typ: "int", + nullable: false, + }, + IntrospectSqlQueryParameterOutput { + documentation: Some( + "some string", + ), + name: "myString", + typ: "string", + nullable: true, + }, + ], + result_columns: [ + IntrospectSqlQueryColumnOutput { + name: "int", + typ: "int", + nullable: false, + }, + ], + } + "#]]; + + let sql = r#" + -- @description some fancy query + -- @param {Int} $1:myInt some integer + -- @param {String}$2:myString? some string + SELECT `int` FROM `model` WHERE `int` = ? and `string` = ?; + "#; + + let res = api.introspect_sql("test_1", sql).send_sync(); + + res.expect_result(expected); + + api.query_raw( + &res.output.source, + &[quaint::Value::int32(1), quaint::Value::text("hello")], + ); +} + #[test_connector(tags(Sqlite))] fn parses_doc_no_position(api: TestApi) { api.schema_push(SIMPLE_SCHEMA).send().assert_green(); @@ -57,7 +113,7 @@ fn parses_doc_no_position(api: TestApi) { let expected = expect![[r#" IntrospectSqlQueryOutput { name: "test_1", - source: "\n -- @param {String} :myInt some integer\n SELECT int FROM model WHERE int = :myInt and string = ?;\n ", + source: "\nSELECT int FROM model WHERE int = :myInt and string = ?;\n", documentation: None, parameters: [ IntrospectSqlQueryParameterOutput { @@ -100,7 +156,7 @@ fn parses_doc_no_alias(api: TestApi) { let expected = expect![[r#" IntrospectSqlQueryOutput { name: "test_1", - source: "\n -- @param {String} $2 some string\n SELECT int FROM model WHERE int = $1 and string = $2;\n ", + source: "\nSELECT int FROM model WHERE int = $1 and string = $2;\n", documentation: None, parameters: [ IntrospectSqlQueryParameterOutput { @@ -143,7 +199,7 @@ fn parses_doc_enum_name(api: TestApi) { let expected = expect![[r#" IntrospectSqlQueryOutput { name: "test_1", - source: "\n -- @param {MyFancyEnum} $1\n SELECT * FROM model WHERE id = $1;\n ", + source: "\nSELECT * FROM model WHERE id = $1;\n", documentation: None, parameters: [ IntrospectSqlQueryParameterOutput {