Skip to content

Commit 343c726

Browse files
author
devfive
committed
fix(query): emit fill_with SQL expressions verbatim
`fill_with` is a raw SQL expression slot - whatever the user wrote is spliced into the emitted UPDATE / INSERT ... SELECT via `Expr::cust`. But `add_column` and `modify_column_nullable` ran it through `convert_default_for_backend`, which is written for a column DEFAULT (a single literal or function call it is free to canonicalise). Its PostgreSQL-cast branch split at the FIRST `::`, lower-cased everything after it, and re-joined the halves. A backfill such as (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric was emitted on PostgreSQL with `'api'` / `'monthly_quota'` / `'seat'`: the comparison never matched, so the backfill silently did nothing, and the lower-cased token is not a valid enum label so the cast failed. On MySQL and SQLite the statement was truncated at the split point outright. Changes: - New `sql::fill_with::convert_fill_with_for_backend`. PostgreSQL - the dialect `fill_with` is authored in - always gets the value verbatim. Other backends only rewrite a value that is unambiguously a single simple literal, or a whole-string portable function spelling such as `NOW()`; anything with whitespace, parentheses or composite SQL keywords passes through untouched. - `parse_pg_type_cast` now splits at the LAST *top-level* `::`, skipping operators inside single-quoted literals and inside parentheses, so `CASE WHEN tag = 'a::b' THEN 1 ELSE 2 END::integer` is no longer cut open inside its own string literal. Only the type name is lower-cased; the value is returned byte-for-byte. - `convert_default_for_backend` recurses through cast chains, so `'x'::text::json` nests (`CAST(CAST('x' AS CHAR) AS JSON)` on MySQL) instead of collapsing. - `modify_column_default.backfill` was already interpolated verbatim; locked with a regression test so the defect cannot be copied onto that path.
1 parent e8a091a commit 343c726

18 files changed

Lines changed: 681 additions & 71 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"changes":{"crates/vespertide-query/Cargo.toml":"Patch"},"note":"add_column.fill_with가 사용자 SQL 표현식을 훼손하던 버그 수정: fill_with는 raw SQL 표현식 슬롯이므로 DEFAULT 정규화(convert_default_for_backend)를 태우지 않고 그대로 방출한다. parse_pg_type_cast도 첫 번째 :: 대신 따옴표/괄호를 건너뛴 마지막 top-level :: 에서 분리하도록 수정","date":"2026-08-20T04:00:00.0000000Z"}

crates/vespertide-query/src/sql/add_column.rs

Lines changed: 143 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
use sea_query::{Alias, Expr, Query, Table, TableAlterStatement};
1+
use sea_query::{Alias, Expr, Query, Table, TableAlterStatement};
22

33
use vespertide_core::{ColumnDef, TableDef};
44

5+
use super::fill_with::convert_fill_with_for_backend;
56
use super::helpers::{
67
build_create_enum_type_sql, build_sea_column_def_with_table, build_sqlite_temp_table_create,
78
convert_default_for_backend, normalize_enum_default, normalize_fill_with,
@@ -77,7 +78,7 @@ pub fn build_add_column(
7778
columns_alias.push(alias);
7879
}
7980
let fill_expr = if let Some(fill) = normalize_fill_with(fill_with) {
80-
let converted = convert_default_for_backend(fill, backend);
81+
let converted = convert_fill_with_for_backend(fill, backend);
8182
Expr::cust(normalize_enum_default(&column.r#type, &converted))
8283
} else if let Some(def) = &column.default {
8384
let converted = convert_default_for_backend(&def.to_sql(), backend);
@@ -132,7 +133,7 @@ pub fn build_add_column(
132133

133134
// Backfill with provided value
134135
if let Some(fill) = normalize_fill_with(fill_with) {
135-
let fill = convert_default_for_backend(fill, backend);
136+
let fill = convert_fill_with_for_backend(fill, backend);
136137
let update_stmt = Query::update()
137138
.table(Alias::new(table))
138139
.value(Alias::new(&column.name), Expr::cust(fill))
@@ -159,7 +160,7 @@ pub fn build_add_column(
159160
#[cfg(test)]
160161
mod tests {
161162
use super::*;
162-
use crate::test_support::{joined_sql, joined_sql_semicolon};
163+
use crate::test_support::{backend_tag, joined_sql, joined_sql_semicolon};
163164
use insta::{assert_snapshot, with_settings};
164165
use rstest::rstest;
165166
use vespertide_core::{ColumnType, SimpleColumnType, TableDef};
@@ -701,4 +702,142 @@ mod tests {
701702
assert_snapshot!(sql);
702703
});
703704
}
705+
706+
fn backfill_sql(backend: DatabaseBackend, column: &ColumnDef, fill: &str) -> String {
707+
use crate::test_support::{col_n, table_def};
708+
709+
let current_schema = vec![table_def(
710+
"subscription",
711+
vec![
712+
col_n("id", ColumnType::Simple(SimpleColumnType::Integer), false),
713+
col_n(
714+
"plan_key",
715+
ColumnType::Simple(SimpleColumnType::Text),
716+
false,
717+
),
718+
col_n(
719+
"plan_tag",
720+
ColumnType::Simple(SimpleColumnType::Text),
721+
false,
722+
),
723+
col_n(
724+
"device_os",
725+
ColumnType::Simple(SimpleColumnType::Text),
726+
false,
727+
),
728+
col_n(
729+
"device_family",
730+
ColumnType::Simple(SimpleColumnType::Text),
731+
false,
732+
),
733+
],
734+
vec![],
735+
)];
736+
let queries = build_add_column(
737+
backend,
738+
"subscription",
739+
column,
740+
Some(fill),
741+
&current_schema,
742+
&[],
743+
)
744+
.expect("add_column with fill_with should build");
745+
joined_sql_semicolon(backend, &queries)
746+
}
747+
748+
fn not_null_column(name: &str, r#type: ColumnType) -> ColumnDef {
749+
ColumnDef {
750+
name: name.into(),
751+
r#type,
752+
nullable: false,
753+
default: None,
754+
comment: None,
755+
primary_key: None,
756+
unique: None,
757+
index: None,
758+
foreign_key: None,
759+
}
760+
}
761+
762+
/// Regression: a `fill_with` CASE expression comparing a text-cast column
763+
/// to the uppercase literal `API` and returning `MONTHLY_QUOTA` / `SEAT`,
764+
/// wrapped in parens and cast to an enum type.
765+
///
766+
/// Splitting at the *first* `::` and lower-casing the remainder produced
767+
/// `'api'` / `'monthly_quota'` / `'seat'`: the comparison never matched, so
768+
/// the backfill silently did nothing, and the lower-cased token was not a
769+
/// valid enum label so the cast failed.
770+
#[rstest]
771+
#[case::postgres(DatabaseBackend::Postgres)]
772+
#[case::mysql(DatabaseBackend::MySql)]
773+
#[case::sqlite(DatabaseBackend::Sqlite)]
774+
fn fill_with_enum_cast_case_expression_is_verbatim(#[case] backend: DatabaseBackend) {
775+
use vespertide_core::{ComplexColumnType, EnumValues};
776+
777+
const FILL: &str = "(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric";
778+
779+
let column = not_null_column(
780+
"metric",
781+
ColumnType::Complex(ComplexColumnType::Enum {
782+
name: "billing_metric".into(),
783+
values: EnumValues::String(vec!["MONTHLY_QUOTA".into(), "SEAT".into()]),
784+
}),
785+
);
786+
let sql = backfill_sql(backend, &column, FILL);
787+
788+
assert!(
789+
sql.contains(FILL),
790+
"fill_with must survive byte-for-byte, got: {sql}"
791+
);
792+
793+
with_settings!({ snapshot_suffix => format!("fill_with_enum_cast_verbatim_{}", backend_tag(backend)) }, {
794+
assert_snapshot!(sql);
795+
});
796+
}
797+
798+
/// Regression: uppercase `WINDOWS` sits *before* the first cast operator
799+
/// and survived, while the `ELSE` / `END` keywords *after* it were
800+
/// lower-cased — the observation that pinpointed the first-`::` split.
801+
#[rstest]
802+
#[case::postgres(DatabaseBackend::Postgres)]
803+
#[case::mysql(DatabaseBackend::MySql)]
804+
#[case::sqlite(DatabaseBackend::Sqlite)]
805+
fn fill_with_json_array_case_expression_is_verbatim(#[case] backend: DatabaseBackend) {
806+
const FILL: &str = "CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END";
807+
808+
let column = not_null_column("os_tags", ColumnType::Simple(SimpleColumnType::Json));
809+
let sql = backfill_sql(backend, &column, FILL);
810+
811+
assert!(
812+
sql.contains(FILL),
813+
"fill_with must survive byte-for-byte, got: {sql}"
814+
);
815+
816+
with_settings!({ snapshot_suffix => format!("fill_with_json_array_verbatim_{}", backend_tag(backend)) }, {
817+
assert_snapshot!(sql);
818+
});
819+
}
820+
821+
/// Regression: the comparison literal itself contains a cast operator
822+
/// inside single quotes, followed by a trailing cast to integer. Splitting
823+
/// on the first `::` cut the statement open inside the string literal.
824+
#[rstest]
825+
#[case::postgres(DatabaseBackend::Postgres)]
826+
#[case::mysql(DatabaseBackend::MySql)]
827+
#[case::sqlite(DatabaseBackend::Sqlite)]
828+
fn fill_with_cast_operator_inside_quotes_is_verbatim(#[case] backend: DatabaseBackend) {
829+
const FILL: &str = "CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer";
830+
831+
let column = not_null_column("tier", ColumnType::Simple(SimpleColumnType::Integer));
832+
let sql = backfill_sql(backend, &column, FILL);
833+
834+
assert!(
835+
sql.contains(FILL),
836+
"fill_with must survive byte-for-byte, got: {sql}"
837+
);
838+
839+
with_settings!({ snapshot_suffix => format!("fill_with_quoted_cast_verbatim_{}", backend_tag(backend)) }, {
840+
assert_snapshot!(sql);
841+
});
842+
}
704843
}
Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
//! Backend adaptation for `fill_with` / backfill values.
2+
//!
3+
//! `fill_with` is a **raw SQL expression slot**: whatever the user wrote is
4+
//! spliced into the emitted `UPDATE` / `INSERT ... SELECT` verbatim (via
5+
//! `Expr::cust`). That is a different contract from a column DEFAULT, which
6+
//! [`convert_default_for_backend`] was written for — a single literal or
7+
//! function call it is free to canonicalise.
8+
//!
9+
//! Running an expression through the DEFAULT path corrupted it. The
10+
//! PostgreSQL-cast branch split at the *first* `::`, lower-cased everything
11+
//! after it, and re-joined the halves, so
12+
//!
13+
//! ```sql
14+
//! (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric
15+
//! ```
16+
//!
17+
//! was emitted with `'api'` / `'monthly_quota'` / `'seat'` — the comparison
18+
//! never matched (silent no-op backfill) and the lower-cased token was not a
19+
//! valid enum label, so the cast failed. On MySQL and SQLite the statement was
20+
//! truncated at the split point outright.
21+
//!
22+
//! The rule enforced here: **never mutate user SQL.**
23+
24+
use super::helpers::{
25+
TIMESTAMP_FUNCTION_SPELLINGS, UUID_FUNCTION_SPELLINGS, convert_default_for_backend,
26+
find_last_top_level_cast, matches_any_spelling, quoted_literal_end,
27+
};
28+
use super::types::DatabaseBackend;
29+
30+
/// Keywords that only occur in a *composite* SQL expression. Finding one
31+
/// outside a string literal proves the value is not a lone literal.
32+
const COMPOSITE_SQL_KEYWORDS: [&str; 16] = [
33+
"case", "when", "then", "else", "end", "select", "from", "where", "and", "or", "not",
34+
"between", "in", "like", "union", "join",
35+
];
36+
37+
/// Adapt a `fill_with` / backfill expression for `backend`.
38+
///
39+
/// * PostgreSQL — the dialect `fill_with` is authored in — always receives the
40+
/// value **verbatim**.
41+
/// * Other backends are only allowed to rewrite a value that is unambiguously
42+
/// a single simple literal (or one of the portable function spellings).
43+
/// Anything composite passes through untouched.
44+
#[must_use]
45+
pub(crate) fn convert_fill_with_for_backend(fill: &str, backend: DatabaseBackend) -> String {
46+
if backend == DatabaseBackend::Postgres || !is_simple_literal_fill(fill) {
47+
return fill.to_string();
48+
}
49+
convert_default_for_backend(fill, backend)
50+
}
51+
52+
/// Whether `fill` is safe to hand to [`convert_default_for_backend`], i.e. it
53+
/// is either a whole-string portable function spelling (`NOW()`,
54+
/// `gen_random_uuid()`, …) or a single simple literal / identifier optionally
55+
/// carrying one trailing `::type` cast.
56+
fn is_simple_literal_fill(fill: &str) -> bool {
57+
let trimmed = fill.trim();
58+
if matches_any_spelling(trimmed, &UUID_FUNCTION_SPELLINGS)
59+
|| matches_any_spelling(trimmed, &TIMESTAMP_FUNCTION_SPELLINGS)
60+
{
61+
return true;
62+
}
63+
if contains_composite_keyword(trimmed) {
64+
return false;
65+
}
66+
let value = match find_last_top_level_cast(trimmed) {
67+
Some(split) => trimmed[..split].trim(),
68+
None => trimmed,
69+
};
70+
is_single_sql_atom(value)
71+
}
72+
73+
/// Whether `value` is exactly one complete quoted string literal, or one bare
74+
/// token free of whitespace, parentheses, commas and quotes.
75+
fn is_single_sql_atom(value: &str) -> bool {
76+
if value.is_empty() {
77+
return false;
78+
}
79+
if value.starts_with('\'') {
80+
return quoted_literal_end(value) == Some(value.len());
81+
}
82+
!value
83+
.chars()
84+
.any(|c| c.is_whitespace() || matches!(c, '(' | ')' | ',' | ';' | '\'' | '"'))
85+
}
86+
87+
/// Whether `value` contains a [`COMPOSITE_SQL_KEYWORDS`] entry outside every
88+
/// single-quoted string literal.
89+
///
90+
/// Literal content is skipped because it is data, not syntax: an enum label
91+
/// such as `'not_started'` must not be mistaken for the `NOT` keyword and
92+
/// pushed onto the verbatim path, where MySQL would choke on its `::` cast.
93+
fn contains_composite_keyword(value: &str) -> bool {
94+
let mut rest = value;
95+
loop {
96+
let (outside, next) = match rest.find('\'') {
97+
Some(quote) => {
98+
let after =
99+
quoted_literal_end(&rest[quote..]).map_or(rest.len(), |end| quote + end);
100+
(&rest[..quote], &rest[after..])
101+
}
102+
None => (rest, ""),
103+
};
104+
if outside
105+
.split(|c: char| !c.is_ascii_alphanumeric() && c != '_')
106+
.any(|word| matches_any_spelling(word, &COMPOSITE_SQL_KEYWORDS))
107+
{
108+
return true;
109+
}
110+
if next.is_empty() {
111+
return false;
112+
}
113+
rest = next;
114+
}
115+
}
116+
117+
#[cfg(test)]
118+
mod tests {
119+
use super::*;
120+
use rstest::rstest;
121+
122+
/// The three reported corruptions, at the unit level: every backend must
123+
/// hand back the expression byte-for-byte.
124+
#[rstest]
125+
#[case::enum_cast(
126+
"(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric"
127+
)]
128+
#[case::json_array(
129+
"CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END"
130+
)]
131+
#[case::cast_inside_quotes("CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer")]
132+
fn composite_expressions_survive_verbatim(#[case] fill: &str) {
133+
for backend in [
134+
DatabaseBackend::Postgres,
135+
DatabaseBackend::MySql,
136+
DatabaseBackend::Sqlite,
137+
] {
138+
assert_eq!(
139+
convert_fill_with_for_backend(fill, backend),
140+
fill,
141+
"{backend:?} must not rewrite a fill_with expression"
142+
);
143+
}
144+
}
145+
146+
/// PostgreSQL is the authoring dialect, so even a value the DEFAULT path
147+
/// would canonicalise is emitted exactly as written.
148+
#[rstest]
149+
#[case("NOW()")]
150+
#[case("gen_random_uuid()")]
151+
#[case("'[]'::json")]
152+
#[case("0")]
153+
fn postgres_never_rewrites(#[case] fill: &str) {
154+
assert_eq!(
155+
convert_fill_with_for_backend(fill, DatabaseBackend::Postgres),
156+
fill
157+
);
158+
}
159+
160+
#[rstest]
161+
#[case::now_mysql("NOW()", DatabaseBackend::MySql, "CURRENT_TIMESTAMP")]
162+
#[case::now_sqlite("NOW()", DatabaseBackend::Sqlite, "CURRENT_TIMESTAMP")]
163+
#[case::uuid_mysql("gen_random_uuid()", DatabaseBackend::MySql, "(UUID())")]
164+
#[case::uuid_sqlite(
165+
"gen_random_uuid()",
166+
DatabaseBackend::Sqlite,
167+
"lower(hex(randomblob(16)))"
168+
)]
169+
#[case::json_cast_mysql("'[]'::json", DatabaseBackend::MySql, "CAST('[]' AS JSON)")]
170+
#[case::json_cast_sqlite("'[]'::json", DatabaseBackend::Sqlite, "'[]'")]
171+
#[case::int_cast_mysql("0::integer", DatabaseBackend::MySql, "CAST(0 AS SIGNED)")]
172+
#[case::identifier_cast_sqlite("legacy_id::text", DatabaseBackend::Sqlite, "legacy_id")]
173+
#[case::empty_literal_mysql("''", DatabaseBackend::MySql, "''")]
174+
#[case::plain_number_sqlite("0", DatabaseBackend::Sqlite, "0")]
175+
fn simple_literals_still_convert_cross_backend(
176+
#[case] fill: &str,
177+
#[case] backend: DatabaseBackend,
178+
#[case] expected: &str,
179+
) {
180+
assert_eq!(convert_fill_with_for_backend(fill, backend), expected);
181+
}
182+
183+
#[rstest]
184+
#[case::plain_number("0", true)]
185+
#[case::quoted_literal("'active'", true)]
186+
#[case::quoted_literal_with_space("'in progress'", true)]
187+
#[case::quoted_literal_with_keyword_inside("'not_started'::user_status", true)]
188+
#[case::identifier_cast("legacy_id::text", true)]
189+
#[case::portable_function("NOW()", true)]
190+
#[case::nested_uuid_function("lower(hex(randomblob(16)))", true)]
191+
#[case::empty("", false)]
192+
#[case::whitespace_only(" ", false)]
193+
#[case::function_call("json_build_array('a')", false)]
194+
#[case::concatenation("'a' || 'b'", false)]
195+
#[case::bare_keyword("END", false)]
196+
#[case::case_expression("CASE WHEN a = 1 THEN 'x' ELSE 'y' END", false)]
197+
#[case::parenthesised_cast("(a + b)::integer", false)]
198+
#[case::unterminated_literal("'oops", false)]
199+
fn simple_literal_classification(#[case] fill: &str, #[case] expected: bool) {
200+
assert_eq!(is_simple_literal_fill(fill), expected, "input: {fill}");
201+
}
202+
203+
/// A keyword inside a string literal is data. Without the quote-skipping
204+
/// scan, `'not_started'::user_status` would take the verbatim path and
205+
/// leave an unusable `::` cast in the MySQL statement.
206+
#[test]
207+
fn keyword_inside_string_literal_is_not_syntax() {
208+
assert!(!contains_composite_keyword("'not_started'::user_status"));
209+
assert!(contains_composite_keyword("a IN (1, 2)"));
210+
assert!(!contains_composite_keyword("weekend_total::integer"));
211+
assert!(!contains_composite_keyword("''"));
212+
}
213+
}

0 commit comments

Comments
 (0)