Skip to content

Commit 2ea7c32

Browse files
authored
Merge pull request #182 from dev-five-git/fix/modify-column-type-fill-with-quoting
fix(query): treat modify_column_type.fill_with replacements as bare enum labels
2 parents e8a091a + a68f8a9 commit 2ea7c32

13 files changed

Lines changed: 284 additions & 46 deletions
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"changes":{"crates/vespertide-core/Cargo.toml":"Minor","crates/vespertide-query/Cargo.toml":"Minor","crates/vespertide-cli/Cargo.toml":"Minor"},"note":"modify_column_type.fill_with 를 bare enum 라벨로 확정 (스키마 문서와 구현 불일치로 enum 축소 마이그레이션이 invalid input value for enum 으로 실패하던 문제). 따옴표가 이미 붙은 기존 값은 한 겹 제거 + 1회 경고로 하위호환 유지","date":"2026-08-20T11:02:39.1090232Z"}

‎crates/vespertide-cli/src/commands/revision/prompts/fill_with.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ pub(in crate::commands::revision) fn prompt_enum_value_bare(
136136

137137
/// Strip SQL single-quotes from an enum value string.
138138
/// `BTreeMap` stores bare enum names; the SQL layer handles quoting via `Expr::val()`.
139+
/// A quoted value gets escaped twice and lands in the column as `'active'`, which
140+
/// `PostgreSQL` rejects with `invalid input value for enum`.
139141
pub(in crate::commands::revision) fn strip_enum_quotes(value: &str) -> String {
140142
value
141143
.trim_start_matches('\'')
@@ -290,6 +292,9 @@ where
290292
/// The original ordering of `remaining_values` is preserved for every entry
291293
/// other than the suggestion (which is hoisted to the top), so non-suggested
292294
/// options remain in a predictable order.
295+
///
296+
/// Every value passes through [`strip_enum_quotes`], so the returned mappings
297+
/// hold bare labels no matter what `enum_prompt_fn` returns.
293298
pub(in crate::commands::revision) fn collect_enum_fill_with_values<E>(
294299
missing: &[EnumFillWithRequired],
295300
enum_prompt_fn: E,
@@ -333,7 +338,7 @@ where
333338
}
334339
let ordered = reorder_with_suggestion(&item.remaining_values, suggestion.as_deref());
335340
let value = enum_prompt_fn(&prompt, &ordered)?;
336-
mappings.insert(removed.clone(), value);
341+
mappings.insert(removed.clone(), strip_enum_quotes(&value));
337342
}
338343
results.push((item.action_index, mappings));
339344
}

‎crates/vespertide-cli/src/commands/revision/tests/prompts.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,25 @@ fn test_collect_enum_fill_with_values_single_removal() {
556556
);
557557
}
558558

559+
#[test]
560+
fn test_collect_enum_fill_with_values_strips_quotes_from_prompt_result() {
561+
use vespertide_planner::EnumFillWithRequired;
562+
563+
let missing = vec![EnumFillWithRequired {
564+
action_index: 0,
565+
table: "plan".to_string(),
566+
column: "sheet_policy".to_string(),
567+
removed_values: vec!["OVER_500".to_string()],
568+
remaining_values: vec!["FIXED".to_string(), "NEGOTIATION".to_string()],
569+
}];
570+
571+
let quoting_enum =
572+
|_prompt: &str, values: &[String]| -> Result<String> { Ok(format!("'{}'", values[0])) };
573+
574+
let collected = collect_enum_fill_with_values(&missing, quoting_enum).unwrap();
575+
assert_eq!(collected[0].1.get("OVER_500"), Some(&"FIXED".to_string()));
576+
}
577+
559578
#[test]
560579
fn test_collect_enum_fill_with_values_multiple_removals() {
561580
use vespertide_planner::EnumFillWithRequired;

‎crates/vespertide-core/src/action/mod.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,12 @@ pub enum MigrationAction {
8484
column: ColumnName,
8585
new_type: ColumnType,
8686
/// Mapping of removed enum values to replacement values for safe enum value removal.
87-
/// e.g., `{"cancelled": "'pending'"}` generates an `UPDATE` before the type change.
87+
/// Both sides are **bare** enum labels — write them exactly as they appear in the enum
88+
/// `values` list, with no surrounding SQL quotes. The SQL generator binds them as data
89+
/// values and adds the quoting itself.
90+
/// e.g., `{"cancelled": "pending"}` generates an `UPDATE` before the type change.
91+
/// A legacy pre-quoted replacement (`"'pending'"`) still works: one outer quote layer is
92+
/// stripped with a warning.
8893
#[serde(default, skip_serializing_if = "Option::is_none")]
8994
fill_with: Option<BTreeMap<String, String>>,
9095
/// Strategy for transforming existing rows that would violate a *narrowed* new type
Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
//! `fill_with` UPDATE emission for `ModifyColumnType`.
2+
//!
3+
//! `fill_with` maps a removed enum label to the surviving label that replaces
4+
//! it. Both sides of the mapping are **bare** labels ??no SQL quoting ??because
5+
//! [`Expr::val`] binds them as data values and the query builder adds exactly
6+
//! one layer of quoting itself.
7+
8+
use std::collections::BTreeMap;
9+
use std::sync::Once;
10+
11+
use sea_query::{Alias, Expr, ExprTrait, Query};
12+
13+
use crate::sql::types::BuiltQuery;
14+
15+
/// Emitted at most once per process by [`strip_legacy_outer_quotes`].
16+
static LEGACY_QUOTE_WARNING: Once = Once::new();
17+
18+
#[expect(
19+
clippy::print_stderr,
20+
reason = "one-time deprecation notice for legacy pre-quoted fill_with values; stderr keeps the emitted SQL on stdout intact"
21+
)]
22+
fn warn_legacy_quoted_replacement(column: &str, removed: &str, replacement: &str, bare: &str) {
23+
LEGACY_QUOTE_WARNING.call_once(|| {
24+
eprintln!(
25+
"vespertide: warning: modify_column_type.fill_with replacement \
26+
{replacement} for {column}.{removed} is wrapped in SQL single \
27+
quotes. fill_with values are bare enum labels; the quotes were \
28+
stripped for compatibility. Rewrite the migration to use {bare}."
29+
);
30+
});
31+
}
32+
33+
/// Backward compatibility for migration files written against the older schema
34+
/// documentation, which showed the replacement already wrapped in SQL single
35+
/// quotes (`{"cancelled": "'pending'"}`).
36+
///
37+
/// Since [`Expr::val`] binds the replacement as a *data value*, a pre-quoted
38+
/// label is escaped into `'''pending'''`, whose content is the 9-character
39+
/// token `'pending'` rather than the 7-character label `pending`. `PostgreSQL`
40+
/// then rejects the UPDATE with `invalid input value for enum`.
41+
///
42+
/// When the value both starts and ends with a single quote, exactly one outer
43+
/// layer is stripped and a one-time warning is emitted. Everything else is
44+
/// passed through untouched.
45+
fn strip_legacy_outer_quotes<'a>(column: &str, removed: &str, replacement: &'a str) -> &'a str {
46+
let Some(bare) = replacement
47+
.strip_prefix('\'')
48+
.and_then(|inner| inner.strip_suffix('\''))
49+
else {
50+
return replacement;
51+
};
52+
53+
warn_legacy_quoted_replacement(column, removed, replacement, bare);
54+
bare
55+
}
56+
57+
/// Build UPDATE statements for `fill_with` mappings (removed enum values ??replacement values).
58+
/// Each entry generates: UPDATE "table" SET "column" = 'replacement' WHERE "column" = '`removed_value`'
59+
///
60+
/// Iteration follows the `BTreeMap` key order, so the emitted statements are
61+
/// deterministic across runs and platforms.
62+
fn build_fill_with_updates(
63+
table: &str,
64+
column: &str,
65+
fill_with: &BTreeMap<String, String>,
66+
) -> Vec<BuiltQuery> {
67+
fill_with
68+
.iter()
69+
.map(|(removed_value, replacement)| {
70+
let replacement = strip_legacy_outer_quotes(column, removed_value, replacement);
71+
let update_stmt = Query::update()
72+
.table(Alias::new(table))
73+
.value(Alias::new(column), Expr::val(replacement))
74+
.and_where(Expr::col(Alias::new(column)).eq(removed_value.as_str()))
75+
.to_owned();
76+
BuiltQuery::Update(Box::new(update_stmt))
77+
})
78+
.collect()
79+
}
80+
81+
/// Conditionally prepend `fill_with` UPDATEs to `queries`.
82+
///
83+
/// Centralises the byte-identical
84+
/// `if let Some(fw) = fill_with { queries.extend(build_fill_with_updates(...)); }`
85+
/// dance that the three `modify_column_type` paths each previously
86+
/// open-coded (`direct::build_postgres_enum_migration`,
87+
/// `direct::build_standard_type_modification`, and
88+
/// `sqlite_rebuild::build_modify_column_type_sqlite_temp_table`). Each
89+
/// callsite now collapses to a single line whose name reads
90+
/// "if a fill_with map exists, prepend its UPDATEs".
91+
pub(super) fn extend_fill_with_updates(
92+
queries: &mut Vec<BuiltQuery>,
93+
table: &str,
94+
column: &str,
95+
fill_with: Option<&BTreeMap<String, String>>,
96+
) {
97+
if let Some(fw) = fill_with {
98+
queries.extend(build_fill_with_updates(table, column, fw));
99+
}
100+
}
101+
102+
#[cfg(test)]
103+
mod tests {
104+
use super::*;
105+
use crate::sql::DatabaseBackend;
106+
use crate::test_support::{backend_tag, joined_sql_semicolon};
107+
use insta::{assert_snapshot, with_settings};
108+
use rstest::rstest;
109+
110+
fn updates(fill_with: &BTreeMap<String, String>, backend: DatabaseBackend) -> String {
111+
let mut queries = Vec::new();
112+
extend_fill_with_updates(&mut queries, "plan", "sheet_policy", Some(fill_with));
113+
joined_sql_semicolon(backend, &queries)
114+
}
115+
116+
fn map(pairs: &[(&str, &str)]) -> BTreeMap<String, String> {
117+
pairs
118+
.iter()
119+
.map(|(k, v)| ((*k).to_string(), (*v).to_string()))
120+
.collect()
121+
}
122+
123+
/// A bare enum label gets exactly one layer of SQL quoting from the query
124+
/// builder ??the enum label reaches the column intact.
125+
#[rstest]
126+
#[case::postgres(DatabaseBackend::Postgres)]
127+
#[case::mysql(DatabaseBackend::MySql)]
128+
#[case::sqlite(DatabaseBackend::Sqlite)]
129+
fn bare_replacement_gets_exactly_one_quote_layer(#[case] backend: DatabaseBackend) {
130+
let sql = updates(&map(&[("OVER_500", "FIXED")]), backend);
131+
132+
assert!(
133+
sql.contains("= 'FIXED'"),
134+
"expected a single quote layer around FIXED, got: {sql}"
135+
);
136+
assert!(
137+
!sql.contains("'''FIXED'''"),
138+
"replacement must not be double-quoted, got: {sql}"
139+
);
140+
141+
with_settings!({ snapshot_path => "../snapshots", snapshot_suffix => format!("fill_with_bare_replacement_{}", backend_tag(backend)) }, {
142+
assert_snapshot!(sql);
143+
});
144+
}
145+
146+
/// Compatibility: a legacy pre-quoted replacement produces the SAME SQL as
147+
/// the bare form, so migrations written against the old documentation keep
148+
/// working.
149+
#[rstest]
150+
#[case::postgres(DatabaseBackend::Postgres)]
151+
#[case::mysql(DatabaseBackend::MySql)]
152+
#[case::sqlite(DatabaseBackend::Sqlite)]
153+
fn quoted_replacement_matches_bare_replacement(#[case] backend: DatabaseBackend) {
154+
let bare = updates(&map(&[("OVER_500", "FIXED")]), backend);
155+
let quoted = updates(&map(&[("OVER_500", "'FIXED'")]), backend);
156+
157+
assert_eq!(quoted, bare);
158+
}
159+
160+
/// Only the outer layer is stripped: a doubly-wrapped value keeps its inner
161+
/// quotes, and a value with a stray quote on one side is left alone.
162+
#[rstest]
163+
#[case::double_wrapped("''FIXED''", "'FIXED'")]
164+
#[case::leading_quote_only("'FIXED", "'FIXED")]
165+
#[case::trailing_quote_only("FIXED'", "FIXED'")]
166+
#[case::lone_quote("'", "'")]
167+
#[case::empty_quotes("''", "")]
168+
#[case::bare("FIXED", "FIXED")]
169+
fn strip_legacy_outer_quotes_removes_at_most_one_layer(
170+
#[case] input: &str,
171+
#[case] expected: &str,
172+
) {
173+
assert_eq!(
174+
strip_legacy_outer_quotes("sheet_policy", "OVER_500", input),
175+
expected
176+
);
177+
}
178+
179+
/// `fill_with` is a `BTreeMap`, so multiple mappings emit in sorted key
180+
/// order regardless of insertion order.
181+
#[rstest]
182+
#[case::postgres(DatabaseBackend::Postgres)]
183+
#[case::mysql(DatabaseBackend::MySql)]
184+
#[case::sqlite(DatabaseBackend::Sqlite)]
185+
fn multiple_mappings_are_deterministically_ordered(#[case] backend: DatabaseBackend) {
186+
let ascending = map(&[
187+
("OVER_500", "FIXED"),
188+
("PER_SHEET", "NEGOTIATION"),
189+
("UNDER_100", "FIXED"),
190+
]);
191+
let descending = map(&[
192+
("UNDER_100", "FIXED"),
193+
("PER_SHEET", "NEGOTIATION"),
194+
("OVER_500", "FIXED"),
195+
]);
196+
let sql = updates(&ascending, backend);
197+
198+
assert_eq!(updates(&descending, backend), sql);
199+
200+
with_settings!({ snapshot_path => "../snapshots", snapshot_suffix => format!("fill_with_multiple_mappings_{}", backend_tag(backend)) }, {
201+
assert_snapshot!(sql);
202+
});
203+
}
204+
205+
/// `None` contributes no statements.
206+
#[test]
207+
fn absent_fill_with_emits_nothing() {
208+
let mut queries = Vec::new();
209+
extend_fill_with_updates(&mut queries, "plan", "sheet_policy", None);
210+
assert!(queries.is_empty());
211+
}
212+
}

‎crates/vespertide-query/src/sql/modify_column_type/mod.rs‎

Lines changed: 3 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
mod direct;
2+
mod fill_with;
23
mod narrowing_preprocess;
34
mod sqlite_rebuild;
45

56
pub use narrowing_preprocess::build_narrowing_preprocess;
67

8+
use fill_with::extend_fill_with_updates;
9+
710
use vespertide_core::NarrowingStrategy;
811

912
/// Combine narrowing pre-processing (when `narrowing_strategy` is set) with
@@ -138,56 +141,13 @@ fn build_pg_alter_with_timezone(
138141

139142
use std::collections::BTreeMap;
140143

141-
use sea_query::{Alias, Expr, ExprTrait, Query};
142-
143144
use vespertide_core::{ColumnType, TableDef};
144145

145146
use self::direct::build_modify_column_type_direct;
146147
use self::sqlite_rebuild::build_modify_column_type_sqlite_temp_table;
147148
use super::types::{BuiltQuery, DatabaseBackend};
148149
use crate::error::QueryError;
149150

150-
/// Build UPDATE statements for `fill_with` mappings (removed enum values → replacement values).
151-
/// Each entry generates: UPDATE "table" SET "column" = 'replacement' WHERE "column" = '`removed_value`'
152-
fn build_fill_with_updates(
153-
table: &str,
154-
column: &str,
155-
fill_with: &BTreeMap<String, String>,
156-
) -> Vec<BuiltQuery> {
157-
fill_with
158-
.iter()
159-
.map(|(removed_value, replacement)| {
160-
let update_stmt = Query::update()
161-
.table(Alias::new(table))
162-
.value(Alias::new(column), Expr::val(replacement.as_str()))
163-
.and_where(Expr::col(Alias::new(column)).eq(removed_value.as_str()))
164-
.to_owned();
165-
BuiltQuery::Update(Box::new(update_stmt))
166-
})
167-
.collect()
168-
}
169-
170-
/// Conditionally prepend `fill_with` UPDATEs to `queries`.
171-
///
172-
/// Centralises the byte-identical
173-
/// `if let Some(fw) = fill_with { queries.extend(build_fill_with_updates(...)); }`
174-
/// dance that the three `modify_column_type` paths each previously
175-
/// open-coded (`direct::build_postgres_enum_migration`,
176-
/// `direct::build_standard_type_modification`, and
177-
/// `sqlite_rebuild::build_modify_column_type_sqlite_temp_table`). Each
178-
/// callsite now collapses to a single line whose name reads
179-
/// "if a fill_with map exists, prepend its UPDATEs".
180-
pub(super) fn extend_fill_with_updates(
181-
queries: &mut Vec<BuiltQuery>,
182-
table: &str,
183-
column: &str,
184-
fill_with: Option<&BTreeMap<String, String>>,
185-
) {
186-
if let Some(fw) = fill_with {
187-
queries.extend(build_fill_with_updates(table, column, fw));
188-
}
189-
}
190-
191151
pub fn build_modify_column_type(
192152
backend: DatabaseBackend,
193153
table: &str,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
source: crates/vespertide-query/src/sql/modify_column_type/fill_with.rs
3+
expression: sql
4+
---
5+
UPDATE `plan` SET `sheet_policy` = 'FIXED' WHERE `sheet_policy` = 'OVER_500'
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
source: crates/vespertide-query/src/sql/modify_column_type/fill_with.rs
3+
expression: sql
4+
---
5+
UPDATE "plan" SET "sheet_policy" = 'FIXED' WHERE "sheet_policy" = 'OVER_500'
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
source: crates/vespertide-query/src/sql/modify_column_type/fill_with.rs
3+
expression: sql
4+
---
5+
UPDATE "plan" SET "sheet_policy" = 'FIXED' WHERE "sheet_policy" = 'OVER_500'
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
source: crates/vespertide-query/src/sql/modify_column_type/fill_with.rs
3+
expression: sql
4+
---
5+
UPDATE `plan` SET `sheet_policy` = 'FIXED' WHERE `sheet_policy` = 'OVER_500';
6+
UPDATE `plan` SET `sheet_policy` = 'NEGOTIATION' WHERE `sheet_policy` = 'PER_SHEET';
7+
UPDATE `plan` SET `sheet_policy` = 'FIXED' WHERE `sheet_policy` = 'UNDER_100'

0 commit comments

Comments
 (0)