Merge branch 'main' into fix/config-list-env-1383

Author: lovasoa

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.
@@ -15,6 +20,7 @@
  - Screen readers now announce the title of the modal component instead of an unnamed dialog.
  - `sqlpage.request_body` and `sqlpage.request_body_base64` now return NULL when the request has no body. A body that cannot be read, such as one exceeding the payload limit, is now reported as an error instead of being silently replaced with an empty body.
  - List-valued configuration options, including OIDC paths and trusted audiences, can now be set through environment variables as space-separated lists.
+ - Datagrid rows with an icon or image no longer display an unnecessary en-dash placeholder, and an explicitly empty description remains empty.
  - Charts can display reference lines. A row with a `yline` is drawn as a line across the chart at that value of the y axis, with the row's `label` and `color` for its text and its color. Reference lines are rows, so a chart can have as many of them as the query returns. A line follows its axis, so on a `horizontal` bar chart a `yline` is drawn down the chart rather than across it. They are not added to the total of a `stacked` chart, and are not filled in an `area` chart.
 
 ## v0.45

docs/screenshots/datagrid-issue-1384-after.png

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

docs/screenshots/datagrid-issue-1384-before.png

@@ -42,6 +42,9 @@
                         {{/if}}
                         {{#if description}}
                             {{description}}
+                        {{else if (eq description "")}}
+                        {{else if icon}}
+                        {{else if image_url}}
                         {{else}}
                             –
                         {{/if}}

examples/official-site/extensions-to-sql.md

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

sqlpage/templates/datagrid.handlebars

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

src/webserver/database/sql.rs

@@ -0,0 +1,4 @@
+SELECT 'datagrid' AS component;
+SELECT 'Facebook' AS title, 'brand-facebook' AS icon;
+SELECT 'Empty' AS title, '' AS description;
+SELECT 'Missing' AS title;

src/webserver/database/sql/rewrite.rs

@@ -40,6 +40,20 @@ async fn test_concurrent_requests() {
     }
 }
 
+#[actix_web::test]
+async fn test_datagrid_description_presence_controls_placeholder() {
+    let resp = req_path("/tests/components/datagrid_icon_only.sql")
+        .await
+        .unwrap();
+    assert_eq!(resp.status(), StatusCode::OK);
+    let body = String::from_utf8(test::read_body(resp).await.to_vec()).unwrap();
+    assert!(body.contains("Facebook"), "{body}");
+    assert!(body.contains("Empty"), "{body}");
+    assert!(body.contains("Missing"), "{body}");
+    assert!(body.contains("<svg"), "{body}");
+    assert_eq!(body.matches('–').count(), 1, "{body}");
+}
+
 #[actix_web::test]
 async fn test_routing_with_db_fs() {
     let mut config = test_config();

tests/components/datagrid_icon_only.sql

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