diff --git a/CHANGELOG.md b/CHANGELOG.md index 5144ae1e..2e0d424b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## unreleased + - Removed unnecessary `CAST` around request variables: + - On PostgreSQL, MySQL, SQL Server and DuckDB, variables are now sent without a text cast and the database infers the type from context, which keeps generated SQL readable (`WHERE id = $1` instead of `WHERE id = CAST($1 AS TEXT)`) and fixes cases where the cast was harmful. + - On SQL Server, this fixes `nvarchar` comparisons with non-ASCII characters that were previously mangled by `CAST(... AS VARCHAR)`, and fixes `CONTAINS` and `EXEC` with variables. + - On MySQL/MariaDB, this fixes `LIMIT`/`OFFSET` with variables. + - The cast is retained on SQLite and on ODBC connections to PostgreSQL, SQLite, Oracle, Snowflake and other databases where it is needed for correct comparisons. - AWS Lambda builds and documentation now use the supported Amazon Linux 2023 custom runtime instead of the end-of-life Amazon Linux 2 runtime. Release artifacts include the configuration directory required on Lambda's read-only filesystem. - Added a `toast` component with plain-text or Markdown content, icons, colors, six screen placements, configurable auto-dismiss timing, optional manual dismissal, URL-fragment triggers, and automatic stacking of queued notifications. - `sqlpage.send_mail` now supports rich email bodies. Use `body_html` for a caller-provided HTML alternative, or `body_md` to render Markdown as HTML. Messages retain a plain-text alternative; `body` may be omitted when `body_md` is used, and `body_md` and `body_html` cannot be combined. diff --git a/examples/official-site/extensions-to-sql.md b/examples/official-site/extensions-to-sql.md index 36eb57af..f49d71ad 100644 --- a/examples/official-site/extensions-to-sql.md +++ b/examples/official-site/extensions-to-sql.md @@ -143,6 +143,10 @@ This means `SET` variables always take precedence over request parameters when u Only a single textual value (**string or `NULL`**) is stored. `SET id = 1` will store the string `'1'`, not the number `1`. +Variables are always sent to the database as text. +On SQLite, and on ODBC connections to PostgreSQL, SQLite, Oracle, Snowflake, or other databases, SQLPage wraps variables in an explicit cast to text, because their parameter type handling would otherwise make comparisons unpredictable. +On PostgreSQL, the variable is passed as text, and comparing it to a non-text column requires an explicit cast. +On MySQL, Microsoft SQL Server, and DuckDB, the database converts the variable to the type expected by the surrounding expression. On databases with a strict type system, such as PostgreSQL, if you need a number, you will need to cast your variables: `SELECT * FROM post WHERE id = $id::int`. Complex structures can be stored as json strings. diff --git a/src/webserver/database/sql.rs b/src/webserver/database/sql.rs index 3ceeb050..ef550c17 100644 --- a/src/webserver/database/sql.rs +++ b/src/webserver/database/sql.rs @@ -432,7 +432,7 @@ mod tests { }; assert_eq!(query.bindings.len(), 1); assert!(query.computed_columns.is_empty()); - assert!(query.sql.contains("upper(CAST($1 AS TEXT))")); + assert!(query.sql.contains("upper($1)")); } #[test] @@ -519,10 +519,7 @@ mod tests { else { panic!("expected database query"); }; - assert_eq!( - query.sql, - "WITH c AS (SELECT CAST(? AS CHAR) AS x) SELECT CAST(? AS CHAR) AS y FROM c" - ); + assert_eq!(query.sql, "WITH c AS (SELECT ? AS x) SELECT ? AS y FROM c"); assert_eq!(query.bindings.as_ref(), [variable("a"), variable("b")]); } @@ -630,7 +627,7 @@ mod tests { let statement = parse_sql( &database, &MySqlDialect {}, - "select '@SQLPAGE_TEMP1' as value from t where id = $id", + "select '@SQLPAGE_TEMP1' where id = $id", ) .unwrap() .next() @@ -742,7 +739,7 @@ mod tests { "select coalesce(upper(sqlpage.url_encode($prefix)), sqlpage.url_encode(value)) as result from t" ), DatabaseQuery { - sql: "SELECT value AS \"__sqlpage_input_0\", upper(CAST($1 AS TEXT)) AS \"__sqlpage_input_1\" FROM t".into(), + sql: "SELECT value AS \"__sqlpage_input_0\", upper($1) AS \"__sqlpage_input_1\" FROM t".into(), bindings: Box::new([call(SqlPageFunctionName::url_encode, [variable("prefix")])]), row_input_json: Box::new([false, false]), computed_columns: Box::new([OutputColumn { @@ -764,8 +761,7 @@ mod tests { "select sqlpage.url_encode(value) as encoded from t where sqlpage.url_encode($expected) = 'x'" ), DatabaseQuery { - sql: "SELECT value AS \"__sqlpage_input_0\" FROM t WHERE CAST($1 AS TEXT) = 'x'" - .into(), + sql: "SELECT value AS \"__sqlpage_input_0\" FROM t WHERE $1 = 'x'".into(), bindings: Box::new([call( SqlPageFunctionName::url_encode, [variable("expected")] @@ -804,4 +800,62 @@ mod tests { } ); } + + fn sql_for_dbinfo(info: &DbInfo, sql: &str) -> String { + match parse_sql(info, &PostgreSqlDialect {}, sql).unwrap().next() { + Some(FileStatement::Query(Query { + body: QueryBody::Database(q), + .. + })) => q.sql, + other => panic!("Expected database query for `{sql}`\nGot: {other:?}"), + } + } + + fn sql_for(db: SupportedDatabase, sql: &str) -> String { + sql_for_dbinfo(&database(db), sql) + } + + fn odbc_sql_for(db: SupportedDatabase, sql: &str) -> String { + sql_for_dbinfo( + &DbInfo { + dbms_name: db.display_name().to_owned(), + database_type: db, + kind: AnyKind::Odbc, + }, + sql, + ) + } + + #[test] + fn variables_keep_cast_only_where_typing_is_unpredictable() { + use SupportedDatabase::*; + let src = "SELECT $a"; + assert_eq!(sql_for(Sqlite, src), "SELECT CAST(?1 AS TEXT)"); + assert_eq!(sql_for(Oracle, src), "SELECT CAST(? AS VARCHAR(4000))"); + assert_eq!(sql_for(Snowflake, src), "SELECT CAST(? AS VARCHAR)"); + assert_eq!(sql_for(Generic, src), "SELECT CAST(? AS VARCHAR)"); + assert_eq!(sql_for(Postgres, src), "SELECT $1"); + assert_eq!(sql_for(MySql, src), "SELECT ?"); + assert_eq!(sql_for(Mssql, src), "SELECT @p1"); + assert_eq!(sql_for(Duckdb, src), "SELECT ?"); + } + + #[test] + fn odbc_cast_follows_database() { + use SupportedDatabase::*; + for db in [Postgres, Sqlite] { + assert_eq!(odbc_sql_for(db, "select $a"), "SELECT CAST(? AS TEXT)"); + } + for db in [MySql, Mssql, Duckdb] { + assert_eq!(odbc_sql_for(db, "select $a"), "SELECT ?"); + } + } + + #[test] + fn limit_uses_bare_parameter() { + assert_eq!( + sql_for(SupportedDatabase::Postgres, "select value from t limit $n"), + "SELECT value FROM t LIMIT $1" + ); + } } diff --git a/src/webserver/database/sql/rewrite.rs b/src/webserver/database/sql/rewrite.rs index 5b090f84..819de120 100644 --- a/src/webserver/database/sql/rewrite.rs +++ b/src/webserver/database/sql/rewrite.rs @@ -41,6 +41,7 @@ use crate::webserver::database::sqlpage_expr::{ }; use crate::webserver::database::sqlpage_functions::functions::SqlPageFunctionName; use crate::webserver::database::{DbInfo, SupportedDatabase}; +use sqlx::any::AnyKind; const SQLPAGE_INPUT_PREFIX: &str = "__sqlpage_input_"; @@ -620,7 +621,7 @@ impl QueryRewriter<'_> { PlaceholderStyle::Numbered { prefix } => format!("{prefix}{}", sequence + 1), PlaceholderStyle::Positional { .. } => format!("${}", sequence + 1), }; - cast_placeholder(placeholder, self.database.database_type) + cast_placeholder(placeholder, self.database) } fn add_row_input(&mut self, mut expression: SqlExpr) -> anyhow::Result { @@ -1035,18 +1036,44 @@ fn variable_source(prefix: char) -> VariableSource { } } -/// Wraps a generated placeholder in the backend-specific text cast expected -/// by `SQLPage`'s string-valued binding interface. -fn cast_placeholder(placeholder: String, database: SupportedDatabase) -> SqlExpr { - let data_type = match database { - SupportedDatabase::MySql => DataType::Char(None), - SupportedDatabase::Mssql => DataType::Varchar(Some(CharacterLength::Max)), - SupportedDatabase::Postgres | SupportedDatabase::Sqlite => DataType::Text, - SupportedDatabase::Oracle => DataType::Varchar(Some(CharacterLength::IntegerLength { - length: 4000, - unit: None, - })), - _ => DataType::Varchar(None), +/// Wraps a generated placeholder in the backend-specific text cast when the +/// database cannot reliably infer that the parameter is a string. +/// +/// `SQLPage` always binds parameters as strings. Native `PostgreSQL` (which +/// pins the parameter type to `TEXT` when preparing the statement), `MySQL` +/// and `SQL Server` (which convert the bound string to the type expected by +/// the surrounding expression) do not need the cast, and it can even be +/// harmful: on `SQL Server` the parameter is bound as `NVARCHAR(MAX)`, and +/// casting it to a narrow `VARCHAR` mangles non-ASCII values. `SQLite` +/// needs it to keep text affinity in comparisons with numbers. +/// +/// Through ODBC, the decision follows the database behind the driver, since +/// `SQLPage` knows it from the driver's reported name: +/// - `PostgreSQL` keeps the cast: `psqlodbc` provides no parameter type +/// information, and the server then fails on context-free parameters +/// (`could not determine data type of parameter`). +/// - `SQLite` keeps it for the same affinity reasons as native connections. +/// - `MySQL`, `SQL Server` and `DuckDB` drop it, like their native +/// counterparts: the former two convert the string at execution time, and +/// `DuckDB` defaults untyped parameters to `VARCHAR`. +/// - `Oracle`, `Snowflake` and unknown databases keep it conservatively. +fn cast_placeholder(placeholder: String, database: &DbInfo) -> SqlExpr { + let data_type = match database.kind { + AnyKind::Sqlite => DataType::Text, + AnyKind::Postgres | AnyKind::MySql | AnyKind::Mssql => { + return SqlExpr::value(Value::Placeholder(placeholder)); + } + AnyKind::Odbc => match database.database_type { + SupportedDatabase::Postgres | SupportedDatabase::Sqlite => DataType::Text, + SupportedDatabase::Oracle => DataType::Varchar(Some(CharacterLength::IntegerLength { + length: 4000, + unit: None, + })), + SupportedDatabase::MySql | SupportedDatabase::Mssql | SupportedDatabase::Duckdb => { + return SqlExpr::value(Value::Placeholder(placeholder)); + } + _ => DataType::Varchar(None), + }, }; SqlExpr::Cast { expr: Box::new(SqlExpr::value(Value::Placeholder(placeholder))), diff --git a/tests/sql_test_files/README.md b/tests/sql_test_files/README.md index f28b3722..6e23b124 100644 --- a/tests/sql_test_files/README.md +++ b/tests/sql_test_files/README.md @@ -15,4 +15,13 @@ and the rest of the file name. Files may include `nosqlite`, `nomssql`, Files that only validate data-processing functions should live here. They must return rows with an `actual` column plus either `expected` (exact match) or `expected_contains` (substring match). Tests in this directory are fetched as -JSON and validated row by row. \ No newline at end of file +JSON and validated row by row. + +### `data/database-specific/` + +Files that only work on a single database engine (because they use +engine-specific SQL syntax) live in a subdirectory named after that database +(`sqlite`, `postgres`, `mysql`, `mssql`, `oracle`, `duckdb`, `snowflake`, +`generic`). They are run by a separate test, only when the current database +matches. Unlike the other directories, their file names do not need `_no...` +suffixes to exclude incompatible backends. \ No newline at end of file diff --git a/tests/sql_test_files/data/database-specific/mssql/variable_mssql_sp_executesql.sql b/tests/sql_test_files/data/database-specific/mssql/variable_mssql_sp_executesql.sql new file mode 100644 index 00000000..1efc37f2 --- /dev/null +++ b/tests/sql_test_files/data/database-specific/mssql/variable_mssql_sp_executesql.sql @@ -0,0 +1,5 @@ +-- https://github.com/sqlpage/SQLPage/issues/516 +-- sp_executesql is used instead of CONTAINS: both reject a CAST expression as an +-- argument, but CONTAINS needs a full-text index, which cannot be created on temp tables. +SET x = 'It works !'; +exec sp_executesql N'SELECT @p as actual, @exp as expected', N'@p varchar(100), @exp varchar(100)', @p=$x, @exp='It works !'; diff --git a/tests/sql_test_files/data/database-specific/mssql/variable_unicode.sql b/tests/sql_test_files/data/database-specific/mssql/variable_unicode.sql new file mode 100644 index 00000000..3ed35c24 --- /dev/null +++ b/tests/sql_test_files/data/database-specific/mssql/variable_unicode.sql @@ -0,0 +1,6 @@ +-- MSSQL nvarchar with non-ASCII must not be mangled by CAST to VARCHAR +drop table if exists variable_unicode_t; +create table variable_unicode_t(name nvarchar(100)); +insert into variable_unicode_t values (N'日本語'); +SET x = N'日本語'; +select N'日本語' as expected, name as actual from variable_unicode_t where name = $x; diff --git a/tests/sql_test_files/data/database-specific/mysql/variable_limit_offset.sql b/tests/sql_test_files/data/database-specific/mysql/variable_limit_offset.sql new file mode 100644 index 00000000..663f8a31 --- /dev/null +++ b/tests/sql_test_files/data/database-specific/mysql/variable_limit_offset.sql @@ -0,0 +1,6 @@ +-- https://github.com/sqlpage/SQLPage/issues/1154 +drop table if exists variable_limit_offset_t; +create table variable_limit_offset_t(id int primary key, v varchar(10)); +insert into variable_limit_offset_t values (1,'a'),(2,'It works !'),(3,'c'); +SET lim = 1; SET off = 1; +select 'It works !' as expected, v as actual from variable_limit_offset_t order by id limit $lim offset $off; diff --git a/tests/sql_test_files/data/set_multiple_rows_noduckdb_nogeneric_nomssql_nomysql_nooracle_nopostgres_nosnowflake.sql b/tests/sql_test_files/data/database-specific/sqlite/set_multiple_rows.sql similarity index 100% rename from tests/sql_test_files/data/set_multiple_rows_noduckdb_nogeneric_nomssql_nomysql_nooracle_nopostgres_nosnowflake.sql rename to tests/sql_test_files/data/database-specific/sqlite/set_multiple_rows.sql diff --git a/tests/sql_test_files/data/variable_compared_to_integer_column_nopostgres.sql b/tests/sql_test_files/data/variable_compared_to_integer_column_nopostgres.sql new file mode 100644 index 00000000..487b9af2 --- /dev/null +++ b/tests/sql_test_files/data/variable_compared_to_integer_column_nopostgres.sql @@ -0,0 +1,5 @@ +-- Variable compared to integer column must work without explicit cast +drop table if exists variable_integer_comparison_t; +create table variable_integer_comparison_t(id int primary key, name varchar(100)); +insert into variable_integer_comparison_t values (1, 'It works !'); +select 'It works !' as expected, name as actual from variable_integer_comparison_t where id = $x; diff --git a/tests/sql_test_files/data/variable_compared_to_number_literal_nopostgres.sql b/tests/sql_test_files/data/variable_compared_to_number_literal_nopostgres.sql new file mode 100644 index 00000000..3777dbbc --- /dev/null +++ b/tests/sql_test_files/data/variable_compared_to_number_literal_nopostgres.sql @@ -0,0 +1,2 @@ +-- Variable compared to numeric literal must work (SQLite/ODBC need CAST) +select 'It works !' as expected, case when $x = 1 then 'It works !' else 'fail' end as actual; diff --git a/tests/sql_test_files/data/variable_comparison_without_type_context.sql b/tests/sql_test_files/data/variable_comparison_without_type_context.sql new file mode 100644 index 00000000..1027f834 --- /dev/null +++ b/tests/sql_test_files/data/variable_comparison_without_type_context.sql @@ -0,0 +1,3 @@ +-- Context-free variables (no column or literal) need CAST on SQLite and psqlodbc +SET other = 'other'; +select 'It works !' as expected, 'It works !' as actual where $x <> $other or $x is null; diff --git a/tests/sql_test_files/mod.rs b/tests/sql_test_files/mod.rs index 3335f967..915cc0cc 100644 --- a/tests/sql_test_files/mod.rs +++ b/tests/sql_test_files/mod.rs @@ -8,14 +8,32 @@ use tokio::task::JoinHandle; #[actix_web::test] async fn run_all_sql_test_files() { let app_data = crate::common::make_app_data().await; - let test_files = get_sql_test_cases(); + run_sql_test_cases(&app_data, get_sql_test_cases()).await; +} +/// Runs the SQL test files in `database-specific//`. +/// These files use syntax that only works on a single database engine, so they +/// cannot be part of the generic `run_all_sql_test_files` test. +#[actix_web::test] +async fn run_database_specific_sql_test_files() { + let app_data = crate::common::make_app_data().await; + let db_type = database_type_name(&app_data); + run_sql_test_cases(&app_data, get_database_specific_test_cases(&db_type)).await; +} + +async fn run_sql_test_cases( + app_data: &actix_web::web::Data, + test_files: Vec, +) { + if test_files.is_empty() { + return; + } let (shutdown_tx, shutdown_rx) = oneshot::channel(); let (echo_handle, port) = crate::common::start_echo_server(shutdown_rx); wait_for_echo_server(port).await; for test_file in test_files { - run_sql_test(&test_file, &app_data, &echo_handle, port).await; + run_sql_test(&test_file, app_data, &echo_handle, port).await; } let _ = shutdown_tx.send(()); @@ -63,9 +81,22 @@ fn get_sql_test_cases() -> Vec { tests } +fn get_database_specific_test_cases(db_type: &str) -> Vec { + read_sql_tests_in_dir( + &format!("tests/sql_test_files/data/database-specific/{db_type}"), + SqlTestFormat::Json, + ) +} + +fn database_type_name(app_data: &actix_web::web::Data) -> String { + format!("{:?}", app_data.db.info.database_type).to_lowercase() +} + fn read_sql_tests_in_dir(dir: &str, format: SqlTestFormat) -> Vec { - std::fs::read_dir(dir) - .unwrap() + let Ok(entries) = std::fs::read_dir(dir) else { + return Vec::new(); // no tests in this directory (e.g. no database-specific tests for this database) + }; + entries .filter_map(|e| { let path = e.ok()?.path(); if path.is_dir() || path.extension()? != "sql" { @@ -86,7 +117,7 @@ async fn run_sql_test( let test_file_path = test_file.to_string_lossy().replace('\\', "/"); let stem = test_file.file_stem().unwrap().to_str().unwrap(); - let db_type = format!("{:?}", app_data.db.info.database_type).to_lowercase(); + let db_type = database_type_name(app_data); if stem.contains(&format!("_no{db_type}")) { println!("Skipped {}: {}", test_file.display(), db_type); return;