diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f24afd39..c1c983813 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## unreleased +- Restored parameter reuse for repeated variables on SQL Server, PostgreSQL, and SQLite, fixing SQL Server and PostgreSQL queries that repeat a variable-dependent expression in `SELECT` and `GROUP BY`. + ## v0.47.0 (2026-10-15) - Redesigned the official website homepage with live SQLPage examples, an interactive database sculpture, flowing section transitions, responsive layouts, and direct links to documentation and deployment guides. - For sites that store their pages in a database, checking for page updates no longer makes other pages wait when the database is busy. In a test, another page loaded in under 1 ms instead of about 250 ms. diff --git a/src/webserver/database/sql.rs b/src/webserver/database/sql.rs index 81f82906d..65c4db86a 100644 --- a/src/webserver/database/sql.rs +++ b/src/webserver/database/sql.rs @@ -681,6 +681,120 @@ mod tests { assert_eq!(query.bindings.len(), 1); } + #[test] + fn repeated_variables_follow_placeholder_style() { + use SupportedDatabase::{Mssql, MySql, Postgres, Sqlite}; + for (db, kind, expected_sql, reuse) in [ + ( + Postgres, + AnyKind::Postgres, + "SELECT $1, $2, $1 FROM t", + true, + ), + (Mssql, AnyKind::Mssql, "SELECT @p1, @p2, @p1 FROM t", true), + ( + Sqlite, + AnyKind::Sqlite, + "SELECT CAST(?1 AS TEXT), CAST(?2 AS TEXT), CAST(?1 AS TEXT) FROM t", + true, + ), + (MySql, AnyKind::MySql, "SELECT ?, ?, ? FROM t", false), + (Mssql, AnyKind::Odbc, "SELECT ?, ?, ? FROM t", false), + ( + Postgres, + AnyKind::Odbc, + "SELECT CAST(? AS TEXT), CAST(? AS TEXT), CAST(? AS TEXT) FROM t", + false, + ), + ( + Sqlite, + AnyKind::Odbc, + "SELECT CAST(? AS TEXT), CAST(? AS TEXT), CAST(? AS TEXT) FROM t", + false, + ), + ] { + let mut info = database(db); + info.kind = kind; + let dialect = dialect::parser_dialect(db); + let FileStatement::Query(Query { + body: QueryBody::Database(query), + .. + }) = parse_sql(&info, dialect.as_ref(), "select $b, $a, $b from t") + .unwrap() + .next() + .unwrap() + else { + panic!("expected database query for {db:?} via {kind:?}"); + }; + assert_eq!(query.sql, expected_sql, "{db:?} via {kind:?}"); + let mut expected_bindings = vec![variable("b"), variable("a")]; + if !reuse { + expected_bindings.push(variable("b")); + } + assert_eq!( + query.bindings.as_ref(), + expected_bindings, + "{db:?} via {kind:?}" + ); + } + } + + #[test] + fn repeated_variables_preserve_lookup_source() { + let query = rewrite_database("select $b, :b, $b, :b from t"); + assert_eq!(query.sql, "SELECT $1, $2, $1, $2 FROM t"); + assert_eq!( + query.bindings.as_ref(), + [ + variable("b"), + SqlPageExpr::Variable(VariableRef { + name: "b".into(), + source: VariableSource::SetOrForm + }), + ] + ); + } + + #[test] + fn mssql_group_by_reuses_variable() { + let FileStatement::Query(Query { + body: QueryBody::Database(query), + .. + }) = one_mssql( + "SELECT n % CAST($b AS int) AS bucket, COUNT(*) AS cnt FROM (VALUES (1),(2),(3),(4),(5),(6)) t(n) GROUP BY n % CAST($b AS int);", + ) + else { + panic!("expected database query"); + }; + assert_eq!( + query.sql, + "SELECT n % CAST(@p1 AS INT) AS bucket, COUNT(*) AS cnt FROM (VALUES (1), (2), (3), (4), (5), (6)) t (n) GROUP BY n % CAST(@p1 AS INT);" + ); + assert_eq!(query.bindings.as_ref(), [variable("b")]); + } + + #[test] + fn reused_variables_preserve_function_binding_order() { + let query = rewrite_database( + "select $b from t where sqlpage.random_string(1) <> $b and sqlpage.random_string(1) <> $b", + ); + assert_eq!(query.sql, "SELECT $1 FROM t WHERE $2 <> $1 AND $3 <> $1"); + assert_eq!( + query.bindings.as_ref(), + [ + variable("b"), + call( + SqlPageFunctionName::random_string, + [SqlPageExpr::Literal(1.into())] + ), + call( + SqlPageFunctionName::random_string, + [SqlPageExpr::Literal(1.into())] + ), + ] + ); + } + #[test] fn effectful_bindings_are_not_deduplicated() { let FileStatement::Query(Query { diff --git a/src/webserver/database/sql/rewrite/database_lowering.rs b/src/webserver/database/sql/rewrite/database_lowering.rs index 23b1278f4..83683d05d 100644 --- a/src/webserver/database/sql/rewrite/database_lowering.rs +++ b/src/webserver/database/sql/rewrite/database_lowering.rs @@ -40,9 +40,21 @@ impl<'a> DatabaseLowerer<'a> { } fn add_binding(&mut self, value: StandaloneExpr) -> SqlExpr { - let sequence = self.bindings.len(); - self.bindings.push(value); - let placeholder = match placeholder_style(self.database.kind) { + let style = placeholder_style(self.database.kind); + // Variable identity includes its lookup source. Calls may have side effects, + // and positional placeholders need a separate argument for each occurrence. + let existing = match (&value, style) { + (SqlPageExpr::Variable(_), PlaceholderStyle::Numbered { .. }) => { + self.bindings.iter().position(|binding| binding == &value) + } + _ => None, + }; + let sequence = existing.unwrap_or_else(|| { + let sequence = self.bindings.len(); + self.bindings.push(value); + sequence + }); + let placeholder = match style { PlaceholderStyle::Numbered { prefix } => format!("{prefix}{}", sequence + 1), PlaceholderStyle::Positional { .. } => format!("${}", sequence + 1), }; diff --git a/tests/end-to-end/official-site.spec.ts b/tests/end-to-end/official-site.spec.ts index b008787a0..20d2554f3 100644 --- a/tests/end-to-end/official-site.spec.ts +++ b/tests/end-to-end/official-site.spec.ts @@ -20,14 +20,22 @@ test("Open documentation", async ({ page }) => { } }); -test("chart", async ({ page }) => { +async function openChartDocumentation(page: Page) { await page.goto("/documentation.sql?component=chart#component"); - await expect(page.getByText("Loading...")).not.toBeVisible(); + // This page initializes every documented chart, which can take several seconds. + await expect(page.locator('[data-pre-init="chart"]')).toHaveCount(0, { + timeout: 15_000, + }); +} + +test("chart", async ({ page }) => { + await openChartDocumentation(page); + await expect(page.getByText("Loading...")).toHaveCount(0); await expect(page.locator(".apexcharts-canvas").first()).toBeVisible(); }); test("chart supports hiding legend", async ({ page }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const expensesChart = page.locator(".card", { has: page.getByRole("heading", { name: "Expenses" }), @@ -55,7 +63,7 @@ const drawnPoints = (card: Locator, series: string) => test("stacked chart draws every series at every x of the chart", async ({ page, }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const powerChart = chartCard(page, "Power draw"); await expect(powerChart.locator(".apexcharts-canvas")).toBeVisible(); @@ -69,7 +77,7 @@ test("stacked chart draws every series at every x of the chart", async ({ test("stacked chart raises a series only where it has a value", async ({ page, }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const powerChart = chartCard(page, "Power draw"); await expect(powerChart.locator(".apexcharts-canvas")).toBeVisible(); @@ -83,7 +91,7 @@ test("stacked chart raises a series only where it has a value", async ({ test("chart draws a yline as a line and a yline_end as a band", async ({ page, }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const temperature = page.locator(".card", { has: page.getByRole("heading", { name: "CPU temperature" }), @@ -103,7 +111,7 @@ test("chart draws a yline as a line and a yline_end as a band", async ({ test("chart draws an xline as a line and an xline_end as a band", async ({ page, }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const latency = page.locator(".card", { has: page.getByRole("heading", { name: "Request latency" }), @@ -123,7 +131,7 @@ test("chart draws an xline as a line and an xline_end as a band", async ({ test("horizontal chart draws a yline down it and an xline across it", async ({ page, }) => { - await page.goto("/documentation.sql?component=chart#component"); + await openChartDocumentation(page); const disks = page.locator(".card", { has: page.getByRole("heading", { name: "Disk usage" }), diff --git a/tests/sql_test_files/data/database-specific/mssql/variable_group_by.sql b/tests/sql_test_files/data/database-specific/mssql/variable_group_by.sql new file mode 100644 index 000000000..fe71aed14 --- /dev/null +++ b/tests/sql_test_files/data/database-specific/mssql/variable_group_by.sql @@ -0,0 +1,11 @@ +-- Repeated variables must use the same parameter in SELECT and GROUP BY. +-- https://github.com/sqlpage/SQLPage/issues/1558 +SET b = '3'; + +SELECT '0:2,1:2,2:2' AS expected, + STRING_AGG(CONCAT(bucket, ':', cnt), ',') WITHIN GROUP (ORDER BY bucket) AS actual +FROM ( + SELECT n % CAST($b AS int) AS bucket, COUNT(*) AS cnt + FROM (VALUES (1), (2), (3), (4), (5), (6)) t(n) + GROUP BY n % CAST($b AS int) +) buckets; diff --git a/tests/sql_test_files/data/parameter_bound_twice_in_a_projection.sql b/tests/sql_test_files/data/parameter_bound_twice_in_a_projection.sql index 66d458ead..892dba3cd 100644 --- a/tests/sql_test_files/data/parameter_bound_twice_in_a_projection.sql +++ b/tests/sql_test_files/data/parameter_bound_twice_in_a_projection.sql @@ -7,3 +7,14 @@ insert into parameter_bound_twice_t (id) values (1); select 'It works !' as expected, case when $x is not null and $x = '1' then 'It works !' else 'fail' end as actual from parameter_bound_twice_t; + +-- Reusing a parameter is scoped to one statement, including SET to NULL. +set x = NULL; +select 'It works !' as expected, + case when $x is null and $x is null then 'It works !' else 'fail' end as actual +from parameter_bound_twice_t; + +set x = '2'; +select 'It works !' as expected, + case when $x = '2' and $x = '2' then 'It works !' else 'fail' end as actual +from parameter_bound_twice_t;