fix(query): emit fill_with SQL expressions verbatim - #184
Merged
Conversation
`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.
Changepacksvespertide@0.2.1 → 0.2.2 - crates/vespertide/Cargo.tomlPatch
vespertide-cli@0.2.1 → 0.3.0 - crates/vespertide-cli/Cargo.tomlMinor
vespertide-config@0.2.1 → 0.3.0 - crates/vespertide-config/Cargo.tomlMinor
vespertide-core@0.2.1 → 0.3.0 - crates/vespertide-core/Cargo.tomlMinor
vespertide-exporter@0.2.1 → 0.3.0 - crates/vespertide-exporter/Cargo.tomlMinor
vespertide-loader@0.2.1 → 0.2.2 - crates/vespertide-loader/Cargo.tomlPatch
vespertide-lsp@0.2.1 → 0.2.2 - crates/vespertide-lsp/Cargo.tomlPatch
vespertide-macro@0.2.1 → 0.2.2 - crates/vespertide-macro/Cargo.tomlPatch
vespertide-naming@0.2.1 → 0.3.0 - crates/vespertide-naming/Cargo.tomlMinor
vespertide-planner@0.2.1 → 0.3.0 - crates/vespertide-planner/Cargo.tomlMinor
vespertide-query@0.2.1 → 0.3.0 - crates/vespertide-query/Cargo.tomlMinor
|
…ray 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
add_column.fill_withsilently corrupted user-supplied SQL expressions. The migration reported success while writing wrong data ??or no data at all.fill_withis a raw SQL expression slot: whatever the user wrote is spliced into the emittedUPDATE/INSERT ... SELECTviaExpr::cust. But it was being run throughconvert_default_for_backend, which exists to normalise a column DEFAULT (a single literal or function call it is free to canonicalise). The two contracts are incompatible.Reproduction
A scratch project with an
add_columnaction carrying a backfill expression, driven through the real CLI (vespertide log). Identical command, identical input,HEAD(e8a091a) vs this branch:Input (
migrations/0002_backfill.vespertide.json):{ "type": "add_column", "table": "subscription", "column": { "name": "metric", "nullable": false, "type": { "kind": "enum", "name": "billing_metric", "values": ["MONTHLY_QUOTA", "SEAT"] } }, "fill_with": "(CASE WHEN plan_key::text = 'API' THEN 'MONTHLY_QUOTA' ELSE 'SEAT' END)::billing_metric" }Before
On PostgreSQL the three uppercase literals are lower-cased, so
plan_key::text = 'api'never matches (silent no-op backfill) and'monthly_quota'is not a validbilling_metriclabel, so the cast fails. On MySQL/SQLite the statement is truncated outright.The second reported case pinpoints the cause ??
WINDOWSsits before the first cast operator and survives, whileELSE/ENDafter it do not:After
Byte-for-byte on every backend.
Root cause
crates/vespertide-query/src/sql/add_column.rsfedfill_withtoconvert_default_for_backendat both fill sites ??L80 (SQLite temp-table rebuild) and L135 (the backfillUPDATE).crates/vespertide-query/src/sql/modify_column_nullable.rsL37 did the same.Inside
crates/vespertide-query/src/sql/helpers.rs:convert_default_for_backend(L227) ??parse_pg_type_cast(L265) ??convert_type_cast(L328).parse_pg_type_cast's non-quoted branch (L298) usedsplit_once("::"), i.e. the first occurrence, and L300 appliedto_lowercase()to everything that followed.convert_type_cast(L331) re-joinedvalue+::+ the lower-cased tail on PostgreSQL, so the mangled body became the emitted SQL. On MySQL/SQLite the tail was discarded entirely (CAST(value AS TYPE)/ barevalue), which is where the truncation came from.Both properties of the split were wrong: it picked the first operator instead of the last, and it was blind to
::occurring inside a single-quoted literal or inside parentheses.What changed
1.
fill_withis never rewritten ??newcrates/vespertide-query/src/sql/fill_with.rs:convert_fill_with_for_backend(fill, backend)fill_withis authored in ??returns the value verbatim;convert_default_for_backendis not called at all.NOW(),gen_random_uuid(), ??, or a single simple literal / identifier optionally carrying one trailing::typecast. Whitespace, parentheses, commas, quotes or composite SQL keywords (CASE/WHEN/THEN/ELSE/END/?? ??verbatim passthrough.'not_started'::user_statusis still recognised as a literal and still converts (it is data, not theNOTkeyword).Wired into
add_column.rs(both fill sites) andmodify_column_nullable.rs. Column DEFAULT handling is untouched ??convert_default_for_backendkeeps its old behaviour for defaults.2.
parse_pg_type_castfixed at the source (helpers.rs):find_last_top_level_castsplits at the last top-level::, skipping operators inside single-quoted literals (''escape aware) and inside parentheses. An unterminated literal returnsNonerather than guessing. The scan is driven bybytes().enumerate()so the cursor is monotonic by construction — no hand-rolled index arithmetic that a mutant could stall into an infinite loop. Togglingin_quoteon every'handles the''escape for free, so no separate escape branch (orquoted_literal_endhelper) is needed.'[]'::json,'it''s'::text, Unicode literals) is preserved ??all its existing tests and the two proptests still pass unchanged.convert_default_for_backendnow recurses through cast chains, so'x'::text::jsonnests correctly (CAST(CAST('x' AS CHAR) AS JSON)on MySQL,'x'on SQLite) instead of leaving a stray::textbehind ??required to avoid a regression from first-split to last-split.3. Other backfill-carrying actions
modify_column_default.backfillwas already interpolated verbatim (modify_column_default.rsL85). No change needed; locked with a regression test so this defect cannot be copied onto that path.modify_column_type.fill_withis aBTreeMap<removed_value, replacement>of enum values emitted throughExpr::val??a value map, not a raw SQL slot ??so it never went throughconvert_default_for_backendand is out of scope here (it is the subject of the sibling quoting PR).Deliberate behaviour changes
fill_withis now fully verbatim.fill_with: "NOW()"staysNOW()instead of becomingCURRENT_TIMESTAMP. Both are valid PostgreSQL and semantically identical;test_fill_with_now_converted_to_current_timestampand itspostgres_fill_nowsnapshot were updated. MySQL/SQLite still getCURRENT_TIMESTAMP, becauseNOW()is not a SQLite function and that rewrite is a whole-string match that cannot touch a fragment of a larger expression.fill_withverbatim, PostgreSQL syntax included. If the user writes PG-only SQL and targets MySQL, the migration now fails loudly at parse time instead of silently writing wrong data ??strictly better than the previous truncated-but-executable statement.Test evidence
New tests:
add_column.rs??three verbatim-survival regression tests 횞 3 backends (9 snapshots), one per reported case:fill_with_enum_cast_case_expression_is_verbatim??text-cast column compared to uppercaseAPI, returningMONTHLY_QUOTA/SEAT, wrapped in parens and cast to an enum type.fill_with_json_array_case_expression_is_verbatim??JSON array containing uppercaseWINDOWSplus a text-cast column, with anELSEbranch casting'[]'tojson.fill_with_cast_operator_inside_quotes_is_verbatim??comparison literal'legacy::v1'contains a cast operator inside single quotes, followed by a trailing::integer.fill_with.rs??33 unit cases covering the verbatim rule, the PostgreSQL passthrough, cross-backend conversion of simple literals, atom classification, and the quote-aware keyword scan.sql/tests/helpers.rs??parse_pg_type_castlast-top-level-split cases (quoted operator, cast chain, parenthesised cast, escaped quote), non-top-level rejection, empty-side rejection, and cast-chain conversion across all 3 backends.modify_column_default.rs??backfill-expression verbatim regression 횞 3 backends.Every pre-existing
convert_default_for_backend/parse_pg_type_castDEFAULT test is green and unmodified, including both Unicode proptests.Scope
Confined to
crates/vespertide-query/src/sql/.schemas/migration.schema.jsonand theMigrationActionenum are untouched, andmodify_column_typeis untouched, so this stays independently reviewable alongside the two sibling PRs (modify_column_type.fill_withdouble-quoting, and thedata_migrationaction).One release-process file is added:
.changepacks/changepack_log_Qp7rvN2xKdLm9aBcT4wZs.json(Minorbump forvespertide-query), per the documented pre-merge changepack step. The random suffix keeps it conflict-free with the sibling PRs.Minorrather thanPatchbecause CI derives thecargo-semver-checksrelease-type only fromMajor/Minor— aPatchdescriptor leaves it empty, so the gate falls back to the (still un-bumped)Cargo.tomlversion and trips over the public-API removals already queued from #181. On a 0.x crateMinoris also the correct tier for the PostgreSQLfill_withbehaviour change described above.Follow-up commit in this PR
The first CI run surfaced two problems, both fixed in
abc0548:cargo-mutantsflagged the byte scanner'si += 1/i += 2/quote + endcursor advances: mutating them to-=/*=stalls the scan into an infinite loop (5 TIMEOUTs, plus one genuine MISSED oni *= 2). Rather than adding amutants.tomlexclusion — the established route for this class, see E18 — the scanners were rewritten on top ofbytes().enumerate(), which removes the whole non-termination mutant class and the MISSED one along with it.quoted_literal_enddisappeared entirely;contains_composite_keywordlost its quote-skipping loop by dropping the short, label-like keywords (and,or,not,in,like) so a plain whole-word split keeps'not_started'::user_statusand'in progress'convertible.add_column.rs, which madecargo-mutantsskip that file wholesale (Diff content doesn't match source file). Removed; the file is now covered by the mutation gate.Adjacent issue found, not fixed here
normalize_enum_default(helpers.rs) quotes a value when the column is an enum andneeds_quotingreturns true.needs_quotingbails on parentheses, so every expression in this PR is safe ??but a paren-lessCASE ... ENDfill on an enum column would still be wrapped in quotes and stored as a literal string. That is a quoting bug, i.e. the sibling PR's theme, so it is flagged here rather than fixed, to keep the three PRs independent.