fix(query): treat modify_column_type.fill_with replacements as bare enum labels - #182
Merged
Merged
Conversation
added 2 commits
August 20, 2026 19:56
…num labels
The schema docs for `modify_column_type.fill_with` showed the replacement
already wrapped in SQL single quotes (`{"cancelled": "'pending'"}`), but
`build_fill_with_updates` binds it with `Expr::val`, which escapes the value
and adds its own quoting. A migration written exactly as documented emitted
`SET "col" = E'\'FIXED\''`, storing the 7-character token `'FIXED'` instead of
the 5-character label `FIXED`, and PostgreSQL rejected it with
`invalid input value for enum`.
Settle on the bare form as the documented contract and keep `Expr::val`
binding, so replacements stay injection-safe:
- Move the fill_with emission out of `modify_column_type/mod.rs` into a new
`modify_column_type/fill_with.rs` (matches the layout already described in
`crates/vespertide-query/AGENTS.md`).
- Add `strip_legacy_outer_quotes`: when a replacement both starts and ends with
a single quote, strip exactly one outer layer, warn once, and proceed, so
existing migration files written against the old docs keep working.
- Document the bare contract on `MigrationAction::ModifyColumnType.fill_with`
and regenerate `schemas/migration.schema.json` (one description line).
- Normalise every collected value through `strip_enum_quotes` in
`collect_enum_fill_with_values`, so the revision prompt cannot reintroduce
quoted values regardless of the injected prompt fn.
Tests cover all three backends: a bare label gets exactly one quote layer, a
pre-quoted label produces byte-identical SQL, and multiple mappings keep the
BTreeMap key ordering.
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
|
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.
Reproduction
schemas/migration.schema.jsondocumentedmodify_column_type.fill_withas taking a pre-quoted replacement:"e.g., `{\"cancelled\": \"'pending'\"}` generates an `UPDATE` before the type change."A migration written exactly as documented ??shrinking a
plan.sheet_policyenum from[FIXED, NEGOTIATION, OVER_500]down to[FIXED, NEGOTIATION]and remapping the removed label:{ "type": "modify_column_type", "table": "plan", "column": "sheet_policy", "new_type": { "kind": "enum", "name": "sheet_policy", "values": ["FIXED", "NEGOTIATION"] }, "fill_with": { "OVER_500": "'FIXED'" } }emitted this on
vespertide log --backend postgres(verified against the real CLI binary with the fix bypassed):The literal's content is the 7-character token
'FIXED', not the 5-character labelFIXED.PostgreSQL rejects it with
invalid input value for enum, and the migration fails.Root cause
crates/vespertide-query/src/sql/modify_column_type/mod.rs:162(pre-change):Expr::valbinds the replacement as a data value, so sea-query escapes it and adds its ownquoting. The implementation therefore expects a bare label, while the schema documentation
showed a pre-quoted one. The two contradicted each other; the existing
test_modify_column_type_with_fill_withtest happened to use a bare value, so the mismatch wasnever caught.
The WHERE side (
.and_where(Expr::col(...).eq(removed_value.as_str())), line 163) is fine ??anunknown-type literal coerces to the enum type in Postgres.
What changed
Contract: BARE. A
fill_withreplacement is a plain enum label with no SQL quotes.Expr::valbinding is kept, so replacements stay injection-safe.crates/vespertide-query/src/sql/modify_column_type/fill_with.rs(new) ??build_fill_with_updatesand
extend_fill_with_updatesmoved out ofmod.rsinto their own module. This is the layoutcrates/vespertide-query/AGENTS.mdalready describes (modify_column_type/=direct / sqlite_rebuild / fill_with),and it keeps
mod.rs(1140 lines) from crossing the 1200-line tier ceiling once the new tests land.Call sites in
direct.rs/sqlite_rebuild.rsare unchanged.strip_legacy_outer_quotes??backward-compatibility path. When a replacement both starts andends with a single quote, exactly one outer layer is stripped, a one-time warning
(
std::sync::Once) goes to stderr, and the build proceeds. Migration files in the wild that followthe old documented form keep working instead of starting to fail.
crates/vespertide-core/src/action/mod.rs??rustdoc onMigrationAction::ModifyColumnType.fill_withnow states the bare contract and mentions the legacy fallback.
schemas/migration.schema.json??regenerated from that rustdoc. One line changed(the
modify_column_type.fill_withdescription);model.schema.jsonandconfig.schema.jsonare byte-identical. Kept deliberately minimal ??see "Overlap with sibling PRs" below.
crates/vespertide-cli/src/commands/revision/prompts/fill_with.rs??every value collected bycollect_enum_fill_with_valuesnow passes throughstrip_enum_quotes, so the interactivevespertide revisionprompt cannot reintroduce a quoted value regardless of which prompt fn isinjected. Previously the guarantee lived only in
prompt_enum_value_bare, one wiring line awayfrom being lost.
Test evidence
New tests in
fill_with.rs, all fanned out across the mandatory{Postgres, MySQL, SQLite}backendtriple per
crates/vespertide-query/AGENTS.md:bare_replacement_gets_exactly_one_quote_layer= 'FIXED'and not'''FIXED'''; 3 snapshotsquoted_replacement_matches_bare_replacement{"OVER_500": "'FIXED'"}produces SQL byte-identical to{"OVER_500": "FIXED"}strip_legacy_outer_quotes_removes_at_most_one_layer''FIXED''??'FIXED','FIXED?뭫nchanged,FIXED'?뭫nchanged,'?뭫nchanged,''??"",FIXED?뭫nchangedmultiple_mappings_are_deterministically_orderedBTreeMapkey order; 3 snapshotsabsent_fill_with_emits_nothingNonecontributes no statementsPlus
test_collect_enum_fill_with_values_strips_quotes_from_prompt_resultincrates/vespertide-cli/src/commands/revision/tests/prompts.rs.Snapshot (Postgres, multi-mapping):
Gates
End-to-end verification against the real CLI
Built
vespertide.exeand ranvespertide log --backend postgreson a scratch project containingthe reproduction above.
Legacy pre-quoted form ??correct SQL, warning emitted once on stderr:
Bare form (the newly documented contract) ??identical SQL, no warning:
Overlap with sibling PRs
Two sibling PRs are in flight on this repo (
add_column.fill_withlowercasing; adata_migrationaction). Both may also touch
schemas/migration.schema.jsonand theMigrationActionrustdoc, sothose edits were kept as small as possible here:
schemas/migration.schema.json: 1 line ??themodify_column_type.fill_withdescription only.add_column.fill_withandmodify_column_nullable.fill_withare untouched.crates/vespertide-core/src/action/mod.rs: 1 hunk ??the doc comment on theModifyColumnType.fill_withfield only. No variant added, removed, or reordered.Changepack
.changepacks/changepack_log_A6HBSMdx7cre8I-RyNhPx.jsondeclaresMinorforvespertide-core,vespertide-query, andvespertide-cli.Minorrather thanPatchis deliberate. Thesemver-checksjob derives its release-type from thePR-introduced descriptor (
.github/workflows/CI.yml:141-155), and that script has noPatchbranch — a
Patch-only descriptor leavesRT="", which makes the action derive strictly from theun-bumped
Cargo.tomlversion. Against the currentmain(which already carries the breakingchanges from #181) that fails, exactly as it did on the first push of this PR before the descriptor
was added. On a
0.xcrate the script mapsMinortorelease-type: major, which is the samesetting under which #181 passed.
The three crates already carry pending
Minorentries in this wave, so the descriptor does notchange the computed version bump (changepacks takes the max per package) — it only makes the
semver-checks gate evaluate this PR under the correct release model.