From 343c7268fa23b448de375c3859977ae806c87754 Mon Sep 17 00:00:00 2001 From: devfive Date: Thu, 20 Aug 2026 20:08:32 +0900 Subject: [PATCH 1/2] 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. --- .../changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json | 1 + crates/vespertide-query/src/sql/add_column.rs | 147 +++++++++++- crates/vespertide-query/src/sql/fill_with.rs | 213 ++++++++++++++++++ crates/vespertide-query/src/sql/helpers.rs | 182 ++++++++++----- crates/vespertide-query/src/sql/mod.rs | 1 + .../src/sql/modify_column_default.rs | 44 ++++ .../src/sql/modify_column_nullable.rs | 37 ++- ...@fill_with_quoted_cast_verbatim_mysql.snap | 7 + ...ll_with_quoted_cast_verbatim_postgres.snap | 7 + ...fill_with_quoted_cast_verbatim_sqlite.snap | 8 + ...im@fill_with_enum_cast_verbatim_mysql.snap | 8 + ...fill_with_enum_cast_verbatim_postgres.snap | 8 + ...m@fill_with_enum_cast_verbatim_sqlite.snap | 8 + ...m@fill_with_json_array_verbatim_mysql.snap | 7 + ...ill_with_json_array_verbatim_postgres.snap | 7 + ...@fill_with_json_array_verbatim_sqlite.snap | 8 + ...o_current_timestamp@postgres_fill_now.snap | 2 +- .../vespertide-query/src/sql/tests/helpers.rs | 57 +++++ 18 files changed, 681 insertions(+), 71 deletions(-) create mode 100644 .changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json create mode 100644 crates/vespertide-query/src/sql/fill_with.rs create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_mysql.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_postgres.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_sqlite.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_mysql.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_postgres.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_sqlite.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_mysql.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_postgres.snap create mode 100644 crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_sqlite.snap diff --git a/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json b/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json new file mode 100644 index 00000000..854d6f18 --- /dev/null +++ b/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json @@ -0,0 +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"} \ No newline at end of file diff --git a/crates/vespertide-query/src/sql/add_column.rs b/crates/vespertide-query/src/sql/add_column.rs index ab67d3f2..0650bc84 100644 --- a/crates/vespertide-query/src/sql/add_column.rs +++ b/crates/vespertide-query/src/sql/add_column.rs @@ -1,7 +1,8 @@ -use sea_query::{Alias, Expr, Query, Table, TableAlterStatement}; +use sea_query::{Alias, Expr, Query, Table, TableAlterStatement}; use vespertide_core::{ColumnDef, TableDef}; +use super::fill_with::convert_fill_with_for_backend; use super::helpers::{ build_create_enum_type_sql, build_sea_column_def_with_table, build_sqlite_temp_table_create, convert_default_for_backend, normalize_enum_default, normalize_fill_with, @@ -77,7 +78,7 @@ pub fn build_add_column( columns_alias.push(alias); } let fill_expr = if let Some(fill) = normalize_fill_with(fill_with) { - let converted = convert_default_for_backend(fill, backend); + let converted = convert_fill_with_for_backend(fill, backend); Expr::cust(normalize_enum_default(&column.r#type, &converted)) } else if let Some(def) = &column.default { let converted = convert_default_for_backend(&def.to_sql(), backend); @@ -132,7 +133,7 @@ pub fn build_add_column( // Backfill with provided value if let Some(fill) = normalize_fill_with(fill_with) { - let fill = convert_default_for_backend(fill, backend); + let fill = convert_fill_with_for_backend(fill, backend); let update_stmt = Query::update() .table(Alias::new(table)) .value(Alias::new(&column.name), Expr::cust(fill)) @@ -159,7 +160,7 @@ pub fn build_add_column( #[cfg(test)] mod tests { use super::*; - use crate::test_support::{joined_sql, joined_sql_semicolon}; + use crate::test_support::{backend_tag, joined_sql, joined_sql_semicolon}; use insta::{assert_snapshot, with_settings}; use rstest::rstest; use vespertide_core::{ColumnType, SimpleColumnType, TableDef}; @@ -701,4 +702,142 @@ mod tests { assert_snapshot!(sql); }); } + + fn backfill_sql(backend: DatabaseBackend, column: &ColumnDef, fill: &str) -> String { + use crate::test_support::{col_n, table_def}; + + let current_schema = vec![table_def( + "subscription", + vec![ + col_n("id", ColumnType::Simple(SimpleColumnType::Integer), false), + col_n( + "plan_key", + ColumnType::Simple(SimpleColumnType::Text), + false, + ), + col_n( + "plan_tag", + ColumnType::Simple(SimpleColumnType::Text), + false, + ), + col_n( + "device_os", + ColumnType::Simple(SimpleColumnType::Text), + false, + ), + col_n( + "device_family", + ColumnType::Simple(SimpleColumnType::Text), + false, + ), + ], + vec![], + )]; + let queries = build_add_column( + backend, + "subscription", + column, + Some(fill), + ¤t_schema, + &[], + ) + .expect("add_column with fill_with should build"); + joined_sql_semicolon(backend, &queries) + } + + fn not_null_column(name: &str, r#type: ColumnType) -> ColumnDef { + ColumnDef { + name: name.into(), + r#type, + nullable: false, + default: None, + comment: None, + primary_key: None, + unique: None, + index: None, + foreign_key: None, + } + } + + /// Regression: a `fill_with` CASE expression comparing a text-cast column + /// to the uppercase literal `API` and returning `MONTHLY_QUOTA` / `SEAT`, + /// wrapped in parens and cast to an enum type. + /// + /// Splitting at the *first* `::` and lower-casing the remainder produced + /// `'api'` / `'monthly_quota'` / `'seat'`: the comparison never matched, so + /// the backfill silently did nothing, and the lower-cased token was not a + /// valid enum label so the cast failed. + #[rstest] + #[case::postgres(DatabaseBackend::Postgres)] + #[case::mysql(DatabaseBackend::MySql)] + #[case::sqlite(DatabaseBackend::Sqlite)] + fn fill_with_enum_cast_case_expression_is_verbatim(#[case] backend: DatabaseBackend) { + use vespertide_core::{ComplexColumnType, EnumValues}; + + const FILL: &str = "(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric"; + + let column = not_null_column( + "metric", + ColumnType::Complex(ComplexColumnType::Enum { + name: "billing_metric".into(), + values: EnumValues::String(vec!["MONTHLY_QUOTA".into(), "SEAT".into()]), + }), + ); + let sql = backfill_sql(backend, &column, FILL); + + assert!( + sql.contains(FILL), + "fill_with must survive byte-for-byte, got: {sql}" + ); + + with_settings!({ snapshot_suffix => format!("fill_with_enum_cast_verbatim_{}", backend_tag(backend)) }, { + assert_snapshot!(sql); + }); + } + + /// Regression: uppercase `WINDOWS` sits *before* the first cast operator + /// and survived, while the `ELSE` / `END` keywords *after* it were + /// lower-cased — the observation that pinpointed the first-`::` split. + #[rstest] + #[case::postgres(DatabaseBackend::Postgres)] + #[case::mysql(DatabaseBackend::MySql)] + #[case::sqlite(DatabaseBackend::Sqlite)] + fn fill_with_json_array_case_expression_is_verbatim(#[case] backend: DatabaseBackend) { + const FILL: &str = "CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END"; + + let column = not_null_column("os_tags", ColumnType::Simple(SimpleColumnType::Json)); + let sql = backfill_sql(backend, &column, FILL); + + assert!( + sql.contains(FILL), + "fill_with must survive byte-for-byte, got: {sql}" + ); + + with_settings!({ snapshot_suffix => format!("fill_with_json_array_verbatim_{}", backend_tag(backend)) }, { + assert_snapshot!(sql); + }); + } + + /// Regression: the comparison literal itself contains a cast operator + /// inside single quotes, followed by a trailing cast to integer. Splitting + /// on the first `::` cut the statement open inside the string literal. + #[rstest] + #[case::postgres(DatabaseBackend::Postgres)] + #[case::mysql(DatabaseBackend::MySql)] + #[case::sqlite(DatabaseBackend::Sqlite)] + fn fill_with_cast_operator_inside_quotes_is_verbatim(#[case] backend: DatabaseBackend) { + const FILL: &str = "CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer"; + + let column = not_null_column("tier", ColumnType::Simple(SimpleColumnType::Integer)); + let sql = backfill_sql(backend, &column, FILL); + + assert!( + sql.contains(FILL), + "fill_with must survive byte-for-byte, got: {sql}" + ); + + with_settings!({ snapshot_suffix => format!("fill_with_quoted_cast_verbatim_{}", backend_tag(backend)) }, { + assert_snapshot!(sql); + }); + } } diff --git a/crates/vespertide-query/src/sql/fill_with.rs b/crates/vespertide-query/src/sql/fill_with.rs new file mode 100644 index 00000000..e007fda5 --- /dev/null +++ b/crates/vespertide-query/src/sql/fill_with.rs @@ -0,0 +1,213 @@ +//! Backend adaptation for `fill_with` / backfill values. +//! +//! `fill_with` is a **raw SQL expression slot**: whatever the user wrote is +//! spliced into the emitted `UPDATE` / `INSERT ... SELECT` verbatim (via +//! `Expr::cust`). That is a different contract from a column DEFAULT, which +//! [`convert_default_for_backend`] was written for — a single literal or +//! function call it is free to canonicalise. +//! +//! Running an expression through the DEFAULT path corrupted it. The +//! PostgreSQL-cast branch split at the *first* `::`, lower-cased everything +//! after it, and re-joined the halves, so +//! +//! ```sql +//! (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric +//! ``` +//! +//! was emitted with `'api'` / `'monthly_quota'` / `'seat'` — the comparison +//! never matched (silent no-op backfill) and the lower-cased token was not a +//! valid enum label, so the cast failed. On MySQL and SQLite the statement was +//! truncated at the split point outright. +//! +//! The rule enforced here: **never mutate user SQL.** + +use super::helpers::{ + TIMESTAMP_FUNCTION_SPELLINGS, UUID_FUNCTION_SPELLINGS, convert_default_for_backend, + find_last_top_level_cast, matches_any_spelling, quoted_literal_end, +}; +use super::types::DatabaseBackend; + +/// Keywords that only occur in a *composite* SQL expression. Finding one +/// outside a string literal proves the value is not a lone literal. +const COMPOSITE_SQL_KEYWORDS: [&str; 16] = [ + "case", "when", "then", "else", "end", "select", "from", "where", "and", "or", "not", + "between", "in", "like", "union", "join", +]; + +/// Adapt a `fill_with` / backfill expression for `backend`. +/// +/// * PostgreSQL — the dialect `fill_with` is authored in — always receives the +/// value **verbatim**. +/// * Other backends are only allowed to rewrite a value that is unambiguously +/// a single simple literal (or one of the portable function spellings). +/// Anything composite passes through untouched. +#[must_use] +pub(crate) fn convert_fill_with_for_backend(fill: &str, backend: DatabaseBackend) -> String { + if backend == DatabaseBackend::Postgres || !is_simple_literal_fill(fill) { + return fill.to_string(); + } + convert_default_for_backend(fill, backend) +} + +/// Whether `fill` is safe to hand to [`convert_default_for_backend`], i.e. it +/// is either a whole-string portable function spelling (`NOW()`, +/// `gen_random_uuid()`, …) or a single simple literal / identifier optionally +/// carrying one trailing `::type` cast. +fn is_simple_literal_fill(fill: &str) -> bool { + let trimmed = fill.trim(); + if matches_any_spelling(trimmed, &UUID_FUNCTION_SPELLINGS) + || matches_any_spelling(trimmed, &TIMESTAMP_FUNCTION_SPELLINGS) + { + return true; + } + if contains_composite_keyword(trimmed) { + return false; + } + let value = match find_last_top_level_cast(trimmed) { + Some(split) => trimmed[..split].trim(), + None => trimmed, + }; + is_single_sql_atom(value) +} + +/// Whether `value` is exactly one complete quoted string literal, or one bare +/// token free of whitespace, parentheses, commas and quotes. +fn is_single_sql_atom(value: &str) -> bool { + if value.is_empty() { + return false; + } + if value.starts_with('\'') { + return quoted_literal_end(value) == Some(value.len()); + } + !value + .chars() + .any(|c| c.is_whitespace() || matches!(c, '(' | ')' | ',' | ';' | '\'' | '"')) +} + +/// Whether `value` contains a [`COMPOSITE_SQL_KEYWORDS`] entry outside every +/// single-quoted string literal. +/// +/// Literal content is skipped because it is data, not syntax: an enum label +/// such as `'not_started'` must not be mistaken for the `NOT` keyword and +/// pushed onto the verbatim path, where MySQL would choke on its `::` cast. +fn contains_composite_keyword(value: &str) -> bool { + let mut rest = value; + loop { + let (outside, next) = match rest.find('\'') { + Some(quote) => { + let after = + quoted_literal_end(&rest[quote..]).map_or(rest.len(), |end| quote + end); + (&rest[..quote], &rest[after..]) + } + None => (rest, ""), + }; + if outside + .split(|c: char| !c.is_ascii_alphanumeric() && c != '_') + .any(|word| matches_any_spelling(word, &COMPOSITE_SQL_KEYWORDS)) + { + return true; + } + if next.is_empty() { + return false; + } + rest = next; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + /// The three reported corruptions, at the unit level: every backend must + /// hand back the expression byte-for-byte. + #[rstest] + #[case::enum_cast( + "(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric" + )] + #[case::json_array( + "CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END" + )] + #[case::cast_inside_quotes("CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer")] + fn composite_expressions_survive_verbatim(#[case] fill: &str) { + for backend in [ + DatabaseBackend::Postgres, + DatabaseBackend::MySql, + DatabaseBackend::Sqlite, + ] { + assert_eq!( + convert_fill_with_for_backend(fill, backend), + fill, + "{backend:?} must not rewrite a fill_with expression" + ); + } + } + + /// PostgreSQL is the authoring dialect, so even a value the DEFAULT path + /// would canonicalise is emitted exactly as written. + #[rstest] + #[case("NOW()")] + #[case("gen_random_uuid()")] + #[case("'[]'::json")] + #[case("0")] + fn postgres_never_rewrites(#[case] fill: &str) { + assert_eq!( + convert_fill_with_for_backend(fill, DatabaseBackend::Postgres), + fill + ); + } + + #[rstest] + #[case::now_mysql("NOW()", DatabaseBackend::MySql, "CURRENT_TIMESTAMP")] + #[case::now_sqlite("NOW()", DatabaseBackend::Sqlite, "CURRENT_TIMESTAMP")] + #[case::uuid_mysql("gen_random_uuid()", DatabaseBackend::MySql, "(UUID())")] + #[case::uuid_sqlite( + "gen_random_uuid()", + DatabaseBackend::Sqlite, + "lower(hex(randomblob(16)))" + )] + #[case::json_cast_mysql("'[]'::json", DatabaseBackend::MySql, "CAST('[]' AS JSON)")] + #[case::json_cast_sqlite("'[]'::json", DatabaseBackend::Sqlite, "'[]'")] + #[case::int_cast_mysql("0::integer", DatabaseBackend::MySql, "CAST(0 AS SIGNED)")] + #[case::identifier_cast_sqlite("legacy_id::text", DatabaseBackend::Sqlite, "legacy_id")] + #[case::empty_literal_mysql("''", DatabaseBackend::MySql, "''")] + #[case::plain_number_sqlite("0", DatabaseBackend::Sqlite, "0")] + fn simple_literals_still_convert_cross_backend( + #[case] fill: &str, + #[case] backend: DatabaseBackend, + #[case] expected: &str, + ) { + assert_eq!(convert_fill_with_for_backend(fill, backend), expected); + } + + #[rstest] + #[case::plain_number("0", true)] + #[case::quoted_literal("'active'", true)] + #[case::quoted_literal_with_space("'in progress'", true)] + #[case::quoted_literal_with_keyword_inside("'not_started'::user_status", true)] + #[case::identifier_cast("legacy_id::text", true)] + #[case::portable_function("NOW()", true)] + #[case::nested_uuid_function("lower(hex(randomblob(16)))", true)] + #[case::empty("", false)] + #[case::whitespace_only(" ", false)] + #[case::function_call("json_build_array('a')", false)] + #[case::concatenation("'a' || 'b'", false)] + #[case::bare_keyword("END", false)] + #[case::case_expression("CASE WHEN a = 1 THEN 'x' ELSE 'y' END", false)] + #[case::parenthesised_cast("(a + b)::integer", false)] + #[case::unterminated_literal("'oops", false)] + fn simple_literal_classification(#[case] fill: &str, #[case] expected: bool) { + assert_eq!(is_simple_literal_fill(fill), expected, "input: {fill}"); + } + + /// A keyword inside a string literal is data. Without the quote-skipping + /// scan, `'not_started'::user_status` would take the verbatim path and + /// leave an unusable `::` cast in the MySQL statement. + #[test] + fn keyword_inside_string_literal_is_not_syntax() { + assert!(!contains_composite_keyword("'not_started'::user_status")); + assert!(contains_composite_keyword("a IN (1, 2)")); + assert!(!contains_composite_keyword("weekend_total::integer")); + assert!(!contains_composite_keyword("''")); + } +} diff --git a/crates/vespertide-query/src/sql/helpers.rs b/crates/vespertide-query/src/sql/helpers.rs index 5173c846..6adc9fbb 100644 --- a/crates/vespertide-query/src/sql/helpers.rs +++ b/crates/vespertide-query/src/sql/helpers.rs @@ -223,15 +223,37 @@ pub(crate) fn to_sea_fk_action(action: &ReferenceAction) -> ForeignKeyAction { } } +/// Function spellings meaning "generate a UUID". Matched against the **whole** +/// input, case-insensitively, so they can never rewrite part of a larger +/// expression. +pub(super) const UUID_FUNCTION_SPELLINGS: [&str; 3] = + ["gen_random_uuid()", "uuid()", "lower(hex(randomblob(16)))"]; + +/// Function spellings meaning "current timestamp". Same whole-input matching +/// rule as [`UUID_FUNCTION_SPELLINGS`]. +pub(super) const TIMESTAMP_FUNCTION_SPELLINGS: [&str; 4] = [ + "current_timestamp()", + "now()", + "current_timestamp", + "getdate()", +]; + +/// Whole-string, case-insensitive membership test. +/// +/// Uses `eq_ignore_ascii_case` rather than `to_lowercase()` so no `String` is +/// allocated per call, mirroring the convention `needs_quoting` uses below. +pub(super) fn matches_any_spelling(value: &str, spellings: &[&str]) -> bool { + spellings.iter().any(|s| value.eq_ignore_ascii_case(s)) +} + /// Convert a default value string to the appropriate backend-specific expression +/// +/// This is for a column **DEFAULT** — a single literal or function call the +/// generator is free to canonicalise. It is *not* safe for a raw SQL +/// expression slot such as `fill_with`; see +/// [`super::fill_with::convert_fill_with_for_backend`]. pub(crate) fn convert_default_for_backend(default: &str, backend: DatabaseBackend) -> String { - // UUID generation functions (case-insensitive match against ASCII literals - // — avoids the per-call `String` allocation that `to_lowercase()` would - // incur, mirroring the convention `needs_quoting` already uses below). - if default.eq_ignore_ascii_case("gen_random_uuid()") - || default.eq_ignore_ascii_case("uuid()") - || default.eq_ignore_ascii_case("lower(hex(randomblob(16)))") - { + if matches_any_spelling(default, &UUID_FUNCTION_SPELLINGS) { return match backend { DatabaseBackend::Postgres => "gen_random_uuid()".to_string(), DatabaseBackend::MySql => "(UUID())".to_string(), @@ -239,71 +261,123 @@ pub(crate) fn convert_default_for_backend(default: &str, backend: DatabaseBacken }; } - // Timestamp functions (case-insensitive) - if default.eq_ignore_ascii_case("current_timestamp()") - || default.eq_ignore_ascii_case("now()") - || default.eq_ignore_ascii_case("current_timestamp") - || default.eq_ignore_ascii_case("getdate()") - { + if matches_any_spelling(default, &TIMESTAMP_FUNCTION_SPELLINGS) { return "CURRENT_TIMESTAMP".to_string(); } // PostgreSQL-style type casts: 'value'::type or expr::type if let Some((value, cast_type)) = parse_pg_type_cast(default) { - return convert_type_cast(value, &cast_type, backend); + return convert_cast_chain(value, &cast_type, backend); } default.to_string() } -/// Parse a PostgreSQL-style type cast expression (e.g., `'[]'::json`, `0::boolean`) -/// Returns `(value, type)` if parsed, or None if not a type cast. +/// End (exclusive byte index) of the single-quoted SQL string literal starting +/// at the beginning of `value`, or `None` when the literal is never closed. /// -/// The value borrows `expr` (both arms return a contiguous slice of the -/// input — quotes included for the quoted arm); only `cast_type` is owned -/// because of the `to_lowercase()` normalisation. -pub(super) fn parse_pg_type_cast(expr: &str) -> Option<(&str, String)> { - let trimmed = expr.trim(); - - // Handle quoted values: 'value'::type - if let Some(after_open) = trimmed.strip_prefix('\'') { - // Find the closing quote (handle escaped quotes '') - let mut chars = after_open.char_indices().peekable(); - while let Some((i, ch)) = chars.next() { - if ch == '\'' { - // Check for escaped quote '' - if chars.next_if(|(_, next)| *next == '\'').is_some() { - continue; - } - // Found closing quote - let value_end = i + ch.len_utf8(); // index in `after_open` - let rest = after_open.get(value_end..)?; - if let Some(stripped) = rest.strip_prefix("::") { - let cast_type = stripped.trim().to_lowercase(); - if !cast_type.is_empty() { - // Opening quote + verbatim content (incl. doubled - // quotes) + closing quote is exactly the contiguous - // input slice `trimmed[..i + 2]` — no allocation. - let value = trimmed.get(..i + 1 + ch.len_utf8())?; - return Some((value, cast_type)); - } - } - return None; +/// `''` inside a literal is the SQL escape for one embedded quote, not a +/// terminator. Scanning bytes is sound because every byte we compare is ASCII, +/// and ASCII bytes never occur inside a multi-byte UTF-8 sequence — so the +/// returned index is always a `char` boundary. +pub(super) fn quoted_literal_end(value: &str) -> Option { + let bytes = value.as_bytes(); + debug_assert_eq!( + bytes.first(), + Some(&b'\''), + "caller must pass a string starting with a single quote" + ); + let mut i = 1; + while i < bytes.len() { + if bytes[i] == b'\'' { + if bytes.get(i + 1) == Some(&b'\'') { + i += 2; + continue; } + return Some(i + 1); } - return None; + i += 1; } + None +} - // Handle unquoted values: expr::type (e.g., 0::boolean, NULL::json) - if let Some((value, cast_type)) = trimmed.split_once("::") { - let value = value.trim(); - let cast_type = cast_type.trim().to_lowercase(); - if !value.is_empty() && !cast_type.is_empty() { - return Some((value, cast_type)); +/// Byte offset of the **last top-level** `::` cast operator in `expr`. +/// +/// Top-level means outside every single-quoted string literal *and* outside +/// every parenthesised group. Both properties matter: +/// +/// * Taking the **last** operator makes a cast chain (`'x'::text::json`) peel +/// from the outside in, instead of treating `text::json` as one type name. +/// * Skipping quoted and nested occurrences stops +/// `CASE WHEN tag = 'a::b' THEN 1 ELSE 2 END::integer` from being split +/// inside its own string literal — the defect that let a `fill_with` +/// expression be silently truncated and case-folded. +/// +/// Returns `None` when there is no top-level cast, or when a string literal is +/// left unterminated (at that point syntax cannot be told from data). +pub(super) fn find_last_top_level_cast(expr: &str) -> Option { + let bytes = expr.as_bytes(); + let mut depth: usize = 0; + let mut last = None; + let mut i = 0; + while i < bytes.len() { + match bytes[i] { + // `bytes[i]` is ASCII here, so `i` is a `char` boundary and the + // slice cannot panic. + b'\'' => { + i += quoted_literal_end(&expr[i..])?; + continue; + } + b'(' => depth += 1, + b')' => depth = depth.saturating_sub(1), + b':' if depth == 0 && bytes.get(i + 1) == Some(&b':') => { + last = Some(i); + i += 2; + continue; + } + _ => {} } + i += 1; } + last +} - None +/// Parse a PostgreSQL-style type cast expression (e.g., `'[]'::json`, `0::boolean`) +/// Returns `(value, type)` if parsed, or None if not a type cast. +/// +/// The split happens at the last top-level `::` (see +/// [`find_last_top_level_cast`]), so `'x'::text::json` yields +/// `("'x'::text", "json")` and a `::` that only appears inside a string +/// literal is not a split point at all. +/// +/// The value borrows `expr` (a contiguous slice of the input, quotes +/// included); only `cast_type` is owned because of the `to_lowercase()` +/// normalisation. **Only the type name is lower-cased — the value is returned +/// byte-for-byte**, so no caller can mangle user SQL through this function. +pub(super) fn parse_pg_type_cast(expr: &str) -> Option<(&str, String)> { + let trimmed = expr.trim(); + let split = find_last_top_level_cast(trimmed)?; + // `split` and `split + 2` index the two ASCII `:` bytes, so both slices + // land on `char` boundaries. + let value = trimmed[..split].trim(); + let cast_type = trimmed[split + 2..].trim().to_lowercase(); + if value.is_empty() || cast_type.is_empty() { + return None; + } + Some((value, cast_type)) +} + +/// Convert a possibly *chained* `PostgreSQL` cast to backend syntax. +/// +/// Recurses so `'x'::text::json` nests properly: MySQL emits +/// `CAST(CAST('x' AS CHAR) AS JSON)` and SQLite strips every level rather than +/// leaving a stray `::text` behind. +fn convert_cast_chain(value: &str, cast_type: &str, backend: DatabaseBackend) -> String { + let inner = match parse_pg_type_cast(value) { + Some((inner_value, inner_cast)) => convert_cast_chain(inner_value, &inner_cast, backend), + None => value.to_string(), + }; + convert_type_cast(&inner, cast_type, backend) } /// Map `PostgreSQL` type name to `MySQL` CAST target type diff --git a/crates/vespertide-query/src/sql/mod.rs b/crates/vespertide-query/src/sql/mod.rs index 61fa3876..10acfa93 100644 --- a/crates/vespertide-query/src/sql/mod.rs +++ b/crates/vespertide-query/src/sql/mod.rs @@ -3,6 +3,7 @@ pub mod add_constraint; pub mod create_table; pub mod delete_column; pub mod delete_table; +pub(crate) mod fill_with; pub mod helpers; pub mod modify_column_comment; pub mod modify_column_default; diff --git a/crates/vespertide-query/src/sql/modify_column_default.rs b/crates/vespertide-query/src/sql/modify_column_default.rs index e7326858..0e4b7983 100644 --- a/crates/vespertide-query/src/sql/modify_column_default.rs +++ b/crates/vespertide-query/src/sql/modify_column_default.rs @@ -512,4 +512,48 @@ mod tests { assert!(sql.contains("status")); assert!(sql.contains("'active'")); } + + /// `backfill` is a raw SQL expression slot, so it is interpolated + /// verbatim. This locks that contract against the `fill_with` defect + /// (first-`::` split + `to_lowercase`) ever being copied onto this path. + #[rstest] + #[case::postgres(DatabaseBackend::Postgres)] + #[case::mysql(DatabaseBackend::MySql)] + #[case::sqlite(DatabaseBackend::Sqlite)] + fn build_modify_column_default_backfill_expression_is_verbatim( + #[case] backend: DatabaseBackend, + ) { + const BACKFILL: &str = "(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric"; + + let schema = vec![table_def( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer), false), + col( + "plan_key", + ColumnType::Simple(SimpleColumnType::Text), + false, + ), + col("metric", ColumnType::Simple(SimpleColumnType::Text), false), + ], + vec![], + )]; + + let queries = build_modify_column_default( + backend, + "users", + "metric", + None, + Some(BACKFILL), + &schema, + &[], + ) + .expect("backfill path should succeed"); + let sql = joined_sql(backend, &queries); + + assert!( + sql.contains(BACKFILL), + "backfill must survive byte-for-byte, got: {sql}" + ); + } } diff --git a/crates/vespertide-query/src/sql/modify_column_nullable.rs b/crates/vespertide-query/src/sql/modify_column_nullable.rs index fc1d7300..04a8aa3e 100644 --- a/crates/vespertide-query/src/sql/modify_column_nullable.rs +++ b/crates/vespertide-query/src/sql/modify_column_nullable.rs @@ -1,8 +1,9 @@ use vespertide_core::TableDef; +use super::fill_with::convert_fill_with_for_backend; use super::helpers::{ build_mysql_modify_column_with, build_pg_alter_column_sql, build_sqlite_modify_column_with, - convert_default_for_backend, normalize_fill_with, quote_ident, + normalize_fill_with, quote_ident, }; use super::types::{BuiltQuery, DatabaseBackend, RawSql}; use crate::error::QueryError; @@ -34,7 +35,7 @@ pub fn build_modify_column_nullable( } // If changing to NOT NULL, first update existing NULL values if fill_with is provided else if !nullable && let Some(fill_value) = normalize_fill_with(fill_with) { - let fill_value = convert_default_for_backend(fill_value, backend); + let fill_value = convert_fill_with_for_backend(fill_value, backend); let quoted_table = quote_ident(table, backend); let quoted_column = quote_ident(column, backend); let update_sql = format!( @@ -246,7 +247,13 @@ mod tests { }); } - /// Test `fill_with` containing `NOW()` should be converted to `CURRENT_TIMESTAMP` for all backends + /// Test `fill_with` containing `NOW()`. + /// + /// `fill_with` is a raw SQL expression slot, so PostgreSQL — the dialect + /// it is authored in — now emits it verbatim. MySQL and SQLite still get + /// `CURRENT_TIMESTAMP` because `NOW()` is not a SQLite function; that + /// rewrite is safe only because the spelling is matched against the whole + /// value, never a fragment of a larger expression. #[rstest] #[case::postgres_fill_now(DatabaseBackend::Postgres)] #[case::mysql_fill_now(DatabaseBackend::MySql)] @@ -279,15 +286,21 @@ mod tests { let queries = result.unwrap(); let sql = joined_sql(backend, &queries); - // NOW() should be converted to CURRENT_TIMESTAMP for all backends - assert!( - !sql.contains("NOW()"), - "SQL should not contain NOW(), got: {sql}" - ); - assert!( - sql.contains("CURRENT_TIMESTAMP"), - "SQL should contain CURRENT_TIMESTAMP, got: {sql}" - ); + if backend == DatabaseBackend::Postgres { + assert!( + sql.contains("NOW()"), + "PostgreSQL must emit fill_with verbatim, got: {sql}" + ); + } else { + assert!( + !sql.contains("NOW()"), + "SQL should not contain NOW(), got: {sql}" + ); + assert!( + sql.contains("CURRENT_TIMESTAMP"), + "SQL should contain CURRENT_TIMESTAMP, got: {sql}" + ); + } let suffix = format!("{}_fill_now", backend_tag(backend)); diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_mysql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_mysql.snap new file mode 100644 index 00000000..759dc50c --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_mysql.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +ALTER TABLE `subscription` ADD COLUMN `tier` int; +UPDATE `subscription` SET `tier` = CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer; +ALTER TABLE `subscription` MODIFY COLUMN `tier` int NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_postgres.snap new file mode 100644 index 00000000..36867ec5 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_postgres.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +ALTER TABLE "subscription" ADD COLUMN "tier" integer; +UPDATE "subscription" SET "tier" = CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer; +ALTER TABLE "subscription" ALTER COLUMN "tier" TYPE integer, ALTER COLUMN "tier" SET NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_sqlite.snap new file mode 100644 index 00000000..3769bf5b --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_cast_operator_inside_quotes_is_verbatim@fill_with_quoted_cast_verbatim_sqlite.snap @@ -0,0 +1,8 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +CREATE TABLE "subscription_temp" ( "id" integer NOT NULL, "plan_key" text NOT NULL, "plan_tag" text NOT NULL, "device_os" text NOT NULL, "device_family" text NOT NULL, "tier" integer NOT NULL ); +INSERT INTO "subscription_temp" ("id", "plan_key", "plan_tag", "device_os", "device_family", "tier") SELECT "id", "plan_key", "plan_tag", "device_os", "device_family", CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer AS "tier" FROM "subscription"; +DROP TABLE "subscription"; +ALTER TABLE "subscription_temp" RENAME TO "subscription" diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_mysql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_mysql.snap new file mode 100644 index 00000000..24147432 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_mysql.snap @@ -0,0 +1,8 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +; +ALTER TABLE `subscription` ADD COLUMN `metric` ENUM('MONTHLY_QUOTA', 'SEAT'); +UPDATE `subscription` SET `metric` = (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric; +ALTER TABLE `subscription` MODIFY COLUMN `metric` ENUM('MONTHLY_QUOTA', 'SEAT') NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_postgres.snap new file mode 100644 index 00000000..49097479 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_postgres.snap @@ -0,0 +1,8 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +CREATE TYPE "subscription_billing_metric" AS ENUM ('MONTHLY_QUOTA', 'SEAT'); +ALTER TABLE "subscription" ADD COLUMN "metric" subscription_billing_metric; +UPDATE "subscription" SET "metric" = (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric; +ALTER TABLE "subscription" ALTER COLUMN "metric" TYPE subscription_billing_metric, ALTER COLUMN "metric" SET NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_sqlite.snap new file mode 100644 index 00000000..cdfc53e6 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_enum_cast_case_expression_is_verbatim@fill_with_enum_cast_verbatim_sqlite.snap @@ -0,0 +1,8 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +CREATE TABLE "subscription_temp" ( "id" integer NOT NULL, "plan_key" text NOT NULL, "plan_tag" text NOT NULL, "device_os" text NOT NULL, "device_family" text NOT NULL, "metric" enum_text NOT NULL , CONSTRAINT "chk_subscription__metric" CHECK ("metric" IN ('MONTHLY_QUOTA', 'SEAT'))); +INSERT INTO "subscription_temp" ("id", "plan_key", "plan_tag", "device_os", "device_family", "metric") SELECT "id", "plan_key", "plan_tag", "device_os", "device_family", (CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric AS "metric" FROM "subscription"; +DROP TABLE "subscription"; +ALTER TABLE "subscription_temp" RENAME TO "subscription" diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_mysql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_mysql.snap new file mode 100644 index 00000000..14f1dee2 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_mysql.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +ALTER TABLE `subscription` ADD COLUMN `os_tags` json; +UPDATE `subscription` SET `os_tags` = CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END; +ALTER TABLE `subscription` MODIFY COLUMN `os_tags` json NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_postgres.snap new file mode 100644 index 00000000..8aaf3b50 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_postgres.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +ALTER TABLE "subscription" ADD COLUMN "os_tags" json; +UPDATE "subscription" SET "os_tags" = CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END; +ALTER TABLE "subscription" ALTER COLUMN "os_tags" TYPE json, ALTER COLUMN "os_tags" SET NOT NULL diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_sqlite.snap new file mode 100644 index 00000000..4115b829 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__add_column__tests__fill_with_json_array_case_expression_is_verbatim@fill_with_json_array_verbatim_sqlite.snap @@ -0,0 +1,8 @@ +--- +source: crates/vespertide-query/src/sql/add_column.rs +expression: sql +--- +CREATE TABLE "subscription_temp" ( "id" integer NOT NULL, "plan_key" text NOT NULL, "plan_tag" text NOT NULL, "device_os" text NOT NULL, "device_family" text NOT NULL, "os_tags" json_text NOT NULL ); +INSERT INTO "subscription_temp" ("id", "plan_key", "plan_tag", "device_os", "device_family", "os_tags") SELECT "id", "plan_key", "plan_tag", "device_os", "device_family", CASE WHEN device_os = 'win' THEN json_build_array('WINDOWS', device_family::text) ELSE '[]'::json END AS "os_tags" FROM "subscription"; +DROP TABLE "subscription"; +ALTER TABLE "subscription_temp" RENAME TO "subscription" diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__modify_column_nullable__tests__fill_with_now_converted_to_current_timestamp@postgres_fill_now.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__modify_column_nullable__tests__fill_with_now_converted_to_current_timestamp@postgres_fill_now.snap index 81223662..afc73857 100644 --- a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__modify_column_nullable__tests__fill_with_now_converted_to_current_timestamp@postgres_fill_now.snap +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__modify_column_nullable__tests__fill_with_now_converted_to_current_timestamp@postgres_fill_now.snap @@ -2,5 +2,5 @@ source: crates/vespertide-query/src/sql/modify_column_nullable.rs expression: sql --- -UPDATE "orders" SET "paid_at" = CURRENT_TIMESTAMP WHERE "paid_at" IS NULL +UPDATE "orders" SET "paid_at" = NOW() WHERE "paid_at" IS NULL ALTER TABLE "orders" ALTER COLUMN "paid_at" SET NOT NULL diff --git a/crates/vespertide-query/src/sql/tests/helpers.rs b/crates/vespertide-query/src/sql/tests/helpers.rs index 88aae126..49b1848d 100644 --- a/crates/vespertide-query/src/sql/tests/helpers.rs +++ b/crates/vespertide-query/src/sql/tests/helpers.rs @@ -284,6 +284,8 @@ fn test_parse_pg_type_cast_no_cast() { assert!(parse_pg_type_cast("42").is_none()); assert!(parse_pg_type_cast("NOW()").is_none()); assert!(parse_pg_type_cast("CURRENT_TIMESTAMP").is_none()); + assert!(parse_pg_type_cast("::json").is_none()); + assert!(parse_pg_type_cast("0::").is_none()); } #[test] @@ -347,6 +349,61 @@ fn test_parse_pg_type_cast_unterminated_quote() { assert!(parse_pg_type_cast("'no close quote::json").is_none()); } +/// The split point is the LAST *top-level* `::`: not one inside a string +/// literal, and not one nested in parentheses. Splitting at the first +/// occurrence truncated `fill_with` expressions mid-literal. +#[rstest] +#[case::cast_operator_inside_literal( + "CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END::integer", + "CASE WHEN plan_tag = 'legacy::v1' THEN 1 ELSE 2 END", + "integer" +)] +#[case::cast_chain("'x'::text::json", "'x'::text", "json")] +#[case::cast_nested_in_parens( + "(CASE WHEN plan_key::text = 'API' THEN 'A' ELSE 'B' END)::billing_metric", + "(CASE WHEN plan_key::text = 'API' THEN 'A' ELSE 'B' END)", + "billing_metric" +)] +#[case::escaped_quote_before_cast("'it''s::not'::text", "'it''s::not'", "text")] +fn test_parse_pg_type_cast_splits_at_last_top_level_operator( + #[case] expr: &str, + #[case] expected_value: &str, + #[case] expected_type: &str, +) { + let (value, cast_type) = parse_pg_type_cast(expr).expect("expression carries a trailing cast"); + assert_eq!(value, expected_value); + assert_eq!(cast_type, expected_type); +} + +/// A `::` that only ever appears inside a string literal or inside an +/// unclosed paren group is not a cast at all. +#[rstest] +#[case::only_inside_literal("'legacy::v1'")] +#[case::only_inside_parens("(a::text)")] +#[case::unbalanced_open_paren("(a::text")] +fn test_parse_pg_type_cast_ignores_non_top_level_operators(#[case] expr: &str) { + assert!( + parse_pg_type_cast(expr).is_none(), + "no top-level cast in: {expr}" + ); +} + +/// Chained casts must nest, not collapse: MySQL wraps each level and SQLite +/// strips every level rather than leaving a stray `::text` behind. +#[rstest] +#[case::postgres(DatabaseBackend::Postgres, "'x'::text::json")] +#[case::mysql(DatabaseBackend::MySql, "CAST(CAST('x' AS CHAR) AS JSON)")] +#[case::sqlite(DatabaseBackend::Sqlite, "'x'")] +fn test_convert_default_for_backend_cast_chain( + #[case] backend: DatabaseBackend, + #[case] expected: &str, +) { + assert_eq!( + convert_default_for_backend("'x'::text::json", backend), + expected + ); +} + #[rstest] #[case::numeric("'0.5'::numeric", DatabaseBackend::MySql, "CAST('0.5' AS DECIMAL)")] #[case::decimal("'1.23'::decimal", DatabaseBackend::MySql, "CAST('1.23' AS DECIMAL)")] From abc0548e0c0b41afe31a02a451395b77b3ee57c8 Mon Sep 17 00:00:00 2001 From: devfive Date: Thu, 20 Aug 2026 20:25:48 +0900 Subject: [PATCH 2/2] fix(query): make the fill_with scanner iterator-driven and strip a stray BOM The hand-rolled byte cursors in quoted_literal_end / ind_last_top_level_cast / contains_composite_keyword advanced with i += 1 / i += 2 / quote + end. cargo-mutants turns each of those into -= or *=, which stops the cursor advancing and hangs the scan, so the mutants surfaced as TIMEOUT (plus one genuine MISSED on i *= 2). Drive the scan from ytes().enumerate() instead: the cursor is monotonic by construction, so the whole non-termination mutant class disappears rather than needing a mutants.toml exclusion. Toggling in_quote on every ' handles the SQL '' escape for free, which removes quoted_literal_end entirely. contains_composite_keyword no longer needs its own quote-skipping loop: with the short label-like keywords (nd, or, ot, in, like) dropped from the list, a plain whole-word split keeps 'not_started'::user_status and 'in progress' on the convertible path. Also strips a UTF-8 BOM accidentally written into add_column.rs, which made cargo-mutants skip that file wholesale, and raises the changepack to Minor: CI derives the cargo-semver-checks release-type only from Major/Minor, so a Patch descriptor left the gate deriving from the (unbumped) Cargo.toml version and failing on the API removals already queued from #181. --- .../changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json | 2 +- crates/vespertide-query/src/sql/add_column.rs | 2 +- crates/vespertide-query/src/sql/fill_with.rs | 75 +++++++++-------- crates/vespertide-query/src/sql/helpers.rs | 82 +++++++++---------- 4 files changed, 78 insertions(+), 83 deletions(-) diff --git a/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json b/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json index 854d6f18..e1323954 100644 --- a/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json +++ b/.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json @@ -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"} \ No newline at end of file +{"changes":{"crates/vespertide-query/Cargo.toml":"Minor"},"note":"add_column.fill_with가 사용자 SQL 표현식을 훼손하던 버그 수정: fill_with는 raw SQL 표현식 슬롯이므로 DEFAULT 정규화(convert_default_for_backend)를 태우지 않고 그대로 방출한다. parse_pg_type_cast도 첫 번째 :: 대신 따옴표/괄호를 건너뛴 마지막 top-level :: 에서 분리하도록 수정. 동작 변경: PostgreSQL은 fill_with를 항상 원문 그대로 방출하므로 NOW()가 더 이상 CURRENT_TIMESTAMP로 치환되지 않는다(MySQL/SQLite는 유지)","date":"2026-08-20T04:00:00.0000000Z"} \ No newline at end of file diff --git a/crates/vespertide-query/src/sql/add_column.rs b/crates/vespertide-query/src/sql/add_column.rs index 0650bc84..2f78b9d1 100644 --- a/crates/vespertide-query/src/sql/add_column.rs +++ b/crates/vespertide-query/src/sql/add_column.rs @@ -1,4 +1,4 @@ -use sea_query::{Alias, Expr, Query, Table, TableAlterStatement}; +use sea_query::{Alias, Expr, Query, Table, TableAlterStatement}; use vespertide_core::{ColumnDef, TableDef}; diff --git a/crates/vespertide-query/src/sql/fill_with.rs b/crates/vespertide-query/src/sql/fill_with.rs index e007fda5..c4c6b217 100644 --- a/crates/vespertide-query/src/sql/fill_with.rs +++ b/crates/vespertide-query/src/sql/fill_with.rs @@ -23,15 +23,17 @@ use super::helpers::{ TIMESTAMP_FUNCTION_SPELLINGS, UUID_FUNCTION_SPELLINGS, convert_default_for_backend, - find_last_top_level_cast, matches_any_spelling, quoted_literal_end, + find_last_top_level_cast, matches_any_spelling, }; use super::types::DatabaseBackend; -/// Keywords that only occur in a *composite* SQL expression. Finding one -/// outside a string literal proves the value is not a lone literal. -const COMPOSITE_SQL_KEYWORDS: [&str; 16] = [ - "case", "when", "then", "else", "end", "select", "from", "where", "and", "or", "not", - "between", "in", "like", "union", "join", +/// Keywords that only occur in a *composite* SQL expression. +/// +/// Deliberately excludes the short, label-like keywords (`and`, `or`, `not`, +/// `in`, `like`): an enum label such as `'in progress'` must stay on the +/// convertible path, or MySQL would receive its `::` cast verbatim. +const COMPOSITE_SQL_KEYWORDS: [&str; 11] = [ + "case", "when", "then", "else", "end", "select", "from", "where", "union", "join", "between", ]; /// Adapt a `fill_with` / backfill expression for `backend`. @@ -77,41 +79,38 @@ fn is_single_sql_atom(value: &str) -> bool { return false; } if value.starts_with('\'') { - return quoted_literal_end(value) == Some(value.len()); + return is_one_complete_quoted_literal(value); } !value .chars() .any(|c| c.is_whitespace() || matches!(c, '(' | ')' | ',' | ';' | '\'' | '"')) } -/// Whether `value` contains a [`COMPOSITE_SQL_KEYWORDS`] entry outside every -/// single-quoted string literal. +/// Whether `value` is a single quoted literal with nothing trailing it. /// -/// Literal content is skipped because it is data, not syntax: an enum label -/// such as `'not_started'` must not be mistaken for the `NOT` keyword and -/// pushed onto the verbatim path, where MySQL would choke on its `::` cast. -fn contains_composite_keyword(value: &str) -> bool { - let mut rest = value; - loop { - let (outside, next) = match rest.find('\'') { - Some(quote) => { - let after = - quoted_literal_end(&rest[quote..]).map_or(rest.len(), |end| quote + end); - (&rest[..quote], &rest[after..]) - } - None => (rest, ""), - }; - if outside - .split(|c: char| !c.is_ascii_alphanumeric() && c != '_') - .any(|word| matches_any_spelling(word, &COMPOSITE_SQL_KEYWORDS)) - { - return true; - } - if next.is_empty() { +/// Any byte outside the literal disqualifies it, so `'a' || 'b'` is rejected, +/// while the `''` escape in `'it''s'` keeps the literal open — the pair closes +/// and immediately reopens it. +fn is_one_complete_quoted_literal(value: &str) -> bool { + let mut in_quote = false; + for byte in value.bytes() { + if byte == b'\'' { + in_quote = !in_quote; + } else if !in_quote { return false; } - rest = next; } + !in_quote +} + +/// Whether `value` contains a [`COMPOSITE_SQL_KEYWORDS`] entry as a whole word. +/// +/// Splitting on non-identifier bytes keeps `weekend_total` distinct from `end`, +/// which is what lets a lone identifier stay on the convertible path. +fn contains_composite_keyword(value: &str) -> bool { + value + .split(|c: char| !c.is_ascii_alphanumeric() && c != '_') + .any(|word| matches_any_spelling(word, &COMPOSITE_SQL_KEYWORDS)) } #[cfg(test)] @@ -200,13 +199,17 @@ mod tests { assert_eq!(is_simple_literal_fill(fill), expected, "input: {fill}"); } - /// A keyword inside a string literal is data. Without the quote-skipping - /// scan, `'not_started'::user_status` would take the verbatim path and - /// leave an unusable `::` cast in the MySQL statement. + /// Whole-word matching is what keeps a lone identifier convertible: an + /// enum label like `not_started` or a column like `weekend_total` must not + /// be read as a keyword and pushed onto the verbatim path, where MySQL + /// would choke on the trailing `::` cast. #[test] - fn keyword_inside_string_literal_is_not_syntax() { + fn composite_keywords_match_whole_words_only() { + assert!(contains_composite_keyword( + "CASE WHEN a = 1 THEN 'x' ELSE 'y' END" + )); + assert!(contains_composite_keyword("SELECT 1 FROM t")); assert!(!contains_composite_keyword("'not_started'::user_status")); - assert!(contains_composite_keyword("a IN (1, 2)")); assert!(!contains_composite_keyword("weekend_total::integer")); assert!(!contains_composite_keyword("''")); } diff --git a/crates/vespertide-query/src/sql/helpers.rs b/crates/vespertide-query/src/sql/helpers.rs index 6adc9fbb..1906c911 100644 --- a/crates/vespertide-query/src/sql/helpers.rs +++ b/crates/vespertide-query/src/sql/helpers.rs @@ -273,34 +273,6 @@ pub(crate) fn convert_default_for_backend(default: &str, backend: DatabaseBacken default.to_string() } -/// End (exclusive byte index) of the single-quoted SQL string literal starting -/// at the beginning of `value`, or `None` when the literal is never closed. -/// -/// `''` inside a literal is the SQL escape for one embedded quote, not a -/// terminator. Scanning bytes is sound because every byte we compare is ASCII, -/// and ASCII bytes never occur inside a multi-byte UTF-8 sequence — so the -/// returned index is always a `char` boundary. -pub(super) fn quoted_literal_end(value: &str) -> Option { - let bytes = value.as_bytes(); - debug_assert_eq!( - bytes.first(), - Some(&b'\''), - "caller must pass a string starting with a single quote" - ); - let mut i = 1; - while i < bytes.len() { - if bytes[i] == b'\'' { - if bytes.get(i + 1) == Some(&b'\'') { - i += 2; - continue; - } - return Some(i + 1); - } - i += 1; - } - None -} - /// Byte offset of the **last top-level** `::` cast operator in `expr`. /// /// Top-level means outside every single-quoted string literal *and* outside @@ -315,31 +287,51 @@ pub(super) fn quoted_literal_end(value: &str) -> Option { /// /// Returns `None` when there is no top-level cast, or when a string literal is /// left unterminated (at that point syntax cannot be told from data). +/// +/// Toggling `in_quote` on every `'` also handles the SQL `''` escape for free: +/// the pair closes and immediately reopens the literal, so its content stays +/// quoted. Driving the scan from `bytes().enumerate()` keeps the cursor +/// monotonic by construction — there is no hand-rolled index arithmetic that +/// could stall the loop. Comparing bytes is sound because every byte matched +/// here is ASCII, which never occurs inside a multi-byte UTF-8 sequence, so a +/// returned index is always a `char` boundary. pub(super) fn find_last_top_level_cast(expr: &str) -> Option { - let bytes = expr.as_bytes(); + let mut in_quote = false; let mut depth: usize = 0; + let mut pending_colon: Option = None; let mut last = None; - let mut i = 0; - while i < bytes.len() { - match bytes[i] { - // `bytes[i]` is ASCII here, so `i` is a `char` boundary and the - // slice cannot panic. + + for (index, byte) in expr.bytes().enumerate() { + if in_quote { + if byte == b'\'' { + in_quote = false; + } + pending_colon = None; + continue; + } + match byte { b'\'' => { - i += quoted_literal_end(&expr[i..])?; - continue; + in_quote = true; + pending_colon = None; } - b'(' => depth += 1, - b')' => depth = depth.saturating_sub(1), - b':' if depth == 0 && bytes.get(i + 1) == Some(&b':') => { - last = Some(i); - i += 2; - continue; + b'(' => { + depth += 1; + pending_colon = None; } - _ => {} + b')' => { + depth = depth.saturating_sub(1); + pending_colon = None; + } + b':' => match pending_colon.take() { + Some(start) if depth == 0 => last = Some(start), + Some(_) => {} + None => pending_colon = Some(index), + }, + _ => pending_colon = None, } - i += 1; } - last + + if in_quote { None } else { last } } /// Parse a PostgreSQL-style type cast expression (e.g., `'[]'::json`, `0::boolean`)