Stop casting variables to TEXT on PostgreSQL, MySQL and SQL Server

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.

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.

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"
+        );
+    }
 }

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<RowInputId> {
@@ -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))),

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.
+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.

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 !';

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;

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;

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;

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;

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;

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/<current database>/`.
+/// 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<AppState>,
+    test_files: Vec<SqlTestCase>,
+) {
+    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<SqlTestCase> {
     tests
 }
 
+fn get_database_specific_test_cases(db_type: &str) -> Vec<SqlTestCase> {
+    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<AppState>) -> String {
+    format!("{:?}", app_data.db.info.database_type).to_lowercase()
+}
+
 fn read_sql_tests_in_dir(dir: &str, format: SqlTestFormat) -> Vec<SqlTestCase> {
-    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;