From eab1f6344bbf815a75323d398ec8bb8c27890490 Mon Sep 17 00:00:00 2001 From: Melody Ruch <396921921@qq.com> Date: Sat, 5 Sep 2026 22:15:59 +0800 Subject: [PATCH] fix(diff): default missing VARCHAR length for MySQL-family targets --- crates/dbx-core/src/schema_diff.rs | 258 +++++++++++++++++- .../dbx-core/src/sql_dialect/ddl_profile.rs | 21 ++ .../dbx-core/src/sql_dialect/type_rewrite.rs | 70 ++++- 3 files changed, 335 insertions(+), 14 deletions(-) diff --git a/crates/dbx-core/src/schema_diff.rs b/crates/dbx-core/src/schema_diff.rs index a3b07f622a..81e73339ac 100644 --- a/crates/dbx-core/src/schema_diff.rs +++ b/crates/dbx-core/src/schema_diff.rs @@ -219,17 +219,92 @@ pub struct FieldMapping { pub custom_params: Option, } +/// Canonicalizes a handful of ANSI-SQL type synonyms that name the same +/// underlying type across dialects (e.g. Postgres/Kingbase report the base +/// type as `character varying` via `format_type()`, while the field-mapping +/// UI's type catalog lists the shorter `varchar`). Without this, a user +/// mapping configured against one spelling silently never matches a column +/// reported under the other. +fn canonical_type_name(base: &str) -> std::borrow::Cow<'_, str> { + match base.trim().to_ascii_uppercase().as_str() { + "CHARACTER VARYING" => std::borrow::Cow::Borrowed("VARCHAR"), + "CHARACTER" => std::borrow::Cow::Borrowed("CHAR"), + _ => std::borrow::Cow::Owned(base.trim().to_ascii_uppercase()), + } +} + +/// Finds the mapping for `base_type`, preferring an exact (case-insensitive) +/// match over an alias match. The field-mapping panel auto-generates one row +/// per catalog type — `char`, `character`, `varchar` and `character varying` +/// commonly coexist as separate rows with independently chosen targets — so +/// treating aliases as interchangeable on the first hit alone would let an +/// earlier row (e.g. `char`) silently shadow a later exact row (e.g. +/// `character`) that shares the same canonical name. Alias matching is only +/// a fallback for when no row exactly names the reported type. +fn find_mapping<'a>(mappings: &'a [FieldMapping], base_type: &str) -> Option<&'a FieldMapping> { + mappings + .iter() + .find(|m| m.source_type.eq_ignore_ascii_case(base_type)) + .or_else(|| mappings.iter().find(|m| canonical_type_name(&m.source_type) == canonical_type_name(base_type))) +} + +/// Splices a driver-reported column length back into its type string when +/// the type name itself omits it. Postgres/Kingbase can report a varying +/// column as bare `character varying` (no length) via `format_type()` while +/// still exposing the real length separately as `character_maximum_length`. +/// Without this, the cross-dialect rewrite below has no way to tell "no +/// length was ever declared" (safe to default) apart from "the length just +/// isn't embedded in this dialect's type string" (issue #8011) — silently +/// falling back to a generic default in the latter case would replace a +/// known-correct length with a possibly wrong one. +fn with_known_length(source_type: &str, character_maximum_length: Option) -> String { + let trimmed = source_type.trim(); + if trimmed.contains('(') { + return trimmed.to_string(); + } + // `character_maximum_length` is populated by several drivers for types + // where it does NOT mean "declared length in this position" — MySQL's + // information_schema fills it in for TEXT/BLOB family columns (e.g. + // TEXT -> 65535), and Oracle's DATA_LENGTH is filled in for every + // column, including DATE (byte length, e.g. 7) and NUMBER. Splicing + // those in verbatim is wrong twice over: `TEXT(65535)` is silently + // *reinterpreted* as MEDIUMTEXT by MySQL (real DB verified), and + // `DATE(7)` sent to a MySQL target is a straight syntax error. + // + // CHAR/CHARACTER/NCHAR belong on this whitelist alongside the VARCHAR + // family, unlike in type_rewrite's *default*-to-255 list: that list + // invents a length out of thin air (where CHAR must be excluded — a + // bare CHAR is already valid MySQL, meaning CHAR(1)), whereas this + // function only *restores* a length the driver already reported + // separately. MySQL's own information_schema does this for CHAR too + // (DATA_TYPE="char", CHARACTER_MAXIMUM_LENGTH=10 for a CHAR(10) column, + // real DB verified) — excluding CHAR here would silently truncate a + // real CHAR(10) column down to CHAR(1). Mirrors the same whitelist + // `columnDDLDataType` uses in agents/drivers/kingbase-go/kingbase_metadata.go. + let base_upper = trimmed.to_ascii_uppercase(); + if !matches!( + base_upper.as_str(), + "VARCHAR" | "CHARACTER VARYING" | "NVARCHAR" | "CHAR" | "CHARACTER" | "NCHAR" | "VARCHAR2" | "NVARCHAR2" + ) { + return trimmed.to_string(); + } + match character_maximum_length { + Some(len) if len > 0 => format!("{trimmed}({len})"), + _ => trimmed.to_string(), + } +} + impl FieldMapping { pub fn apply<'a>(mappings: &'a [FieldMapping], source_type: &str) -> Option<&'a str> { let base_type = source_type.split('(').next().unwrap_or(source_type).trim(); - mappings.iter().find(|m| m.source_type.eq_ignore_ascii_case(base_type)).map(|m| m.target_type.as_str()) + find_mapping(mappings, base_type).map(|m| m.target_type.as_str()) } pub fn apply_with_params(mappings: &[FieldMapping], source_type: &str, target_kind: DialectKind) -> Option { let trimmed = source_type.trim(); let base_type = trimmed.split('(').next().unwrap_or(trimmed); let source_params = &trimmed[base_type.len()..]; - let matched = mappings.iter().find(|m| m.source_type.eq_ignore_ascii_case(base_type))?; + let matched = find_mapping(mappings, base_type)?; let result = match matched.param_strategy { ParamStrategy::Strip => Some(matched.target_type.clone()), @@ -4727,11 +4802,12 @@ fn generate_create_table_sql( let profile = profile_for(db_type); // Type rewrite: user mappings → profile type_map → DialectKind matrix → normalize. // Call sites must not branch on individual DatabaseType values. - let map_type = |source_type: &str| -> String { - if let Some(user_target) = FieldMapping::apply_with_params(field_mappings, source_type, target_dialect) { + let map_type = |col: &ColumnInfo| -> String { + let source_type = with_known_length(&col.data_type, col.character_maximum_length); + if let Some(user_target) = FieldMapping::apply_with_params(field_mappings, &source_type, target_dialect) { return user_target; } - rewrite_column_type(source_type, db_type, source_dialect) + rewrite_column_type(&source_type, db_type, source_dialect) }; let table = qualified_name(name, db_type, schema); @@ -4746,7 +4822,7 @@ fn generate_create_table_sql( continue; }; let col_name = quote_id(&col.name, db_type); - let mapped_type = map_type(&col.data_type); + let mapped_type = map_type(col); if db_type == DatabaseType::SqlServer { col_defs.push(sqlserver_column_definition(col, &mapped_type, source_dialect, schema)); if col.is_primary_key { @@ -5182,12 +5258,13 @@ fn generate_schema_sync_sql_inner( // explicitly by the comparison plan instead of appending an invalid clause. let cascade = if cascade_delete && db_type != DatabaseType::SqlServer { " CASCADE" } else { "" }; - let map_type = |source_type: &str| -> String { + let map_type = |col: &ColumnInfo| -> String { let tgt = DialectKind::from_database_type(db_type); - if let Some(user_target) = FieldMapping::apply_with_params(field_mappings, source_type, tgt) { + let source_type = with_known_length(&col.data_type, col.character_maximum_length); + if let Some(user_target) = FieldMapping::apply_with_params(field_mappings, &source_type, tgt) { return user_target; } - rewrite_column_type(source_type, db_type, source_dialect) + rewrite_column_type(&source_type, db_type, source_dialect) }; let is_same_dialect = source_dialect.map(|source| DialectKind::from_database_type(db_type) == source).unwrap_or(false); @@ -5359,7 +5436,7 @@ fn generate_schema_sync_sql_inner( if let Some(columns) = &diff.columns { let convert_col = - |col: &ColumnInfo| -> ColumnInfo { ColumnInfo { data_type: map_type(&col.data_type), ..col.clone() } }; + |col: &ColumnInfo| -> ColumnInfo { ColumnInfo { data_type: map_type(col), ..col.clone() } }; for column in columns { match column.diff_type.as_str() { "added" => { @@ -12968,6 +13045,167 @@ mod tests { ); } + #[test] + fn field_mapping_matches_character_varying_alias() { + // Kingbase/Postgres report a varchar column's base type as + // `character varying` (via format_type()), not `varchar`. A user who + // configures a mapping using the shorter, more common `varchar` + // spelling must still match it (issue #8011) — previously an exact + // string comparison meant such a mapping silently never fired + // against the real `character varying` column, dropping the user's + // chosen param strategy. + let mappings = vec![FieldMapping { + source_type: "varchar".into(), + target_type: "varchar".into(), + param_strategy: ParamStrategy::Custom, + custom_params: Some("255".to_string()), + }]; + let result = FieldMapping::apply_with_params(&mappings, "character varying", DialectKind::Mysql); + assert_eq!(result, Some("varchar(255)".to_string())); + + let result = FieldMapping::apply_with_params(&mappings, "character varying(50)", DialectKind::Mysql); + assert_eq!(result, Some("varchar(255)".to_string())); + + assert_eq!(FieldMapping::apply(&mappings, "character varying"), Some("varchar")); + } + + #[test] + fn field_mapping_exact_match_is_not_shadowed_by_an_alias() { + // char/character/varchar/character varying commonly coexist as + // separate auto-generated rows with independently chosen targets. + // An exact match must win over an alias match picked up from an + // earlier, unrelated row that merely shares the same canonical name + // (issue #8011 review: aliasing broke this without a two-pass find). + let mappings = vec![ + FieldMapping { + source_type: "char".into(), + target_type: "binary".into(), + param_strategy: ParamStrategy::Preserve, + custom_params: None, + }, + FieldMapping { + source_type: "character".into(), + target_type: "text".into(), + param_strategy: ParamStrategy::Preserve, + custom_params: None, + }, + ]; + assert_eq!(FieldMapping::apply(&mappings, "character"), Some("text"), "exact row must win over the char alias"); + assert_eq!( + FieldMapping::apply(&mappings, "char"), + Some("binary"), + "exact row must win over the character alias" + ); + } + + #[test] + fn with_known_length_splices_reported_length_into_bare_type() { + assert_eq!(with_known_length("varchar", Some(1000)), "varchar(1000)"); + assert_eq!(with_known_length("character varying", None), "character varying"); + assert_eq!(with_known_length("varchar(50)", Some(1000)), "varchar(50)", "explicit params are never overridden"); + assert_eq!(with_known_length("varchar", Some(0)), "varchar", "non-positive length is ignored"); + assert_eq!(with_known_length("varchar", Some(-1)), "varchar", "negative length (unbounded marker) is ignored"); + } + + #[test] + fn with_known_length_ignores_non_length_bearing_types() { + // `character_maximum_length` is populated by several drivers for + // columns where it does not mean "declared length here" — MySQL's + // information_schema fills it in for TEXT/BLOB (byte capacity, e.g. + // TEXT -> 65535), and Oracle's DATA_LENGTH is filled in for every + // column, DATE and NUMBER included (issue #8011 review round 2). + // Splicing those in would silently reinterpret the type (MySQL turns + // `TEXT(65535)` into MEDIUMTEXT) or produce invalid DDL (`DATE(7)`). + assert_eq!(with_known_length("text", Some(65535)), "text"); + assert_eq!(with_known_length("date", Some(7)), "date"); + assert_eq!(with_known_length("number", Some(22)), "number"); + } + + #[test] + fn with_known_length_restores_a_real_char_length() { + // Unlike type_rewrite's *default*-to-255 list (which must exclude + // CHAR — a bare CHAR is already valid, meaning CHAR(1)), this + // function *restores* a length the driver already knows: MySQL's + // information_schema reports a CHAR(10) column as DATA_TYPE="char" + // with CHARACTER_MAXIMUM_LENGTH=10 (real DB verified). Using + // length 1 here would make this assertion pass even with CHAR + // wrongly excluded — CHAR(1) and bare CHAR mean the same thing — so + // this deliberately uses a length where truncation would show up + // (issue #8011 review round 3). + assert_eq!(with_known_length("char", Some(10)), "char(10)"); + assert_eq!(with_known_length("character", Some(10)), "character(10)"); + assert_eq!(with_known_length("char", None), "char", "no known length means no invented one either"); + } + + #[test] + fn mysql_same_dialect_add_column_keeps_text_type_when_length_metadata_is_present() { + // MySQL's own information_schema reports a real character_maximum_length + // for TEXT (65535) even though COLUMN_TYPE never carries it in + // parentheses. Splicing it in verbatim would have this same-dialect + // ADD COLUMN silently reinterpreted as MEDIUMTEXT by the server (real + // MySQL 8.4.6 verified) instead of staying TEXT. + let mut source_col = column("notes", "text", None); + source_col.character_maximum_length = Some(65535); + let diff = ColumnDiff { + diff_type: "added".to_string(), + name: "notes".to_string(), + source: Some(source_col), + target: None, + changes: vec![], + add_position: None, + }; + let sql = gen_sql(wrap_table_diff("t", vec![diff]), DatabaseType::Mysql, Some(DialectKind::Mysql)); + assert!(sql.contains("text"), "expected the column to stay TEXT: {sql}"); + assert!(!sql.contains("(65535)"), "must not splice TEXT's byte capacity in as a length: {sql}"); + } + + #[test] + fn oracle_to_mysql_date_column_does_not_gain_an_invalid_length() { + // Oracle's DATA_LENGTH is populated for every column, DATE included + // (byte length, e.g. 7) — not just character types. Splicing it in + // would send `DATE(7)` to MySQL, which is a syntax error there (real + // MySQL 8.4.6 verified); the pre-fix behavior of passing `DATE` + // through untouched was already correct. + let mut source_col = column("created_on", "DATE", None); + source_col.character_maximum_length = Some(7); + let diff = ColumnDiff { + diff_type: "added".to_string(), + name: "created_on".to_string(), + source: Some(source_col), + target: None, + changes: vec![], + add_position: None, + }; + let sql = gen_sql(wrap_table_diff("t", vec![diff]), DatabaseType::Mysql, Some(DialectKind::Oracle)); + assert!(!sql.contains("DATE(7)"), "must not turn a valid DATE column into invalid DDL: {sql}"); + } + + #[test] + fn cross_dialect_add_column_uses_known_character_maximum_length_not_a_generic_default() { + // Kingbase's MySQL-compatible introspection can report a bare + // `varchar`/`character varying` (no length in the type string) + // while still exposing the real length via `character_maximum_length` + // on the same ColumnInfo. Silently defaulting to 255 in that case + // would trade a loud syntax error for a quiet wrong-schema bug + // (issue #8011 review). + let mut source_col = column("bio", "character varying", None); + source_col.character_maximum_length = Some(1000); + let diff = ColumnDiff { + diff_type: "added".to_string(), + name: "bio".to_string(), + source: Some(source_col), + target: None, + changes: vec![], + add_position: None, + }; + let sql = gen_sql(wrap_table_diff("t", vec![diff]), DatabaseType::Mysql, Some(DialectKind::Postgres)); + assert!(sql.contains("(1000)"), "expected the real reported length to be preserved: {sql}"); + assert!( + !sql.contains("(255)"), + "must not silently fall back to the generic default when the real length is known: {sql}" + ); + } + #[test] fn postgres_creates_sequences_before_tables_that_reference_them() { let table_diff = TableDiff { diff --git a/crates/dbx-core/src/sql_dialect/ddl_profile.rs b/crates/dbx-core/src/sql_dialect/ddl_profile.rs index dcdbcab718..cf092ef658 100644 --- a/crates/dbx-core/src/sql_dialect/ddl_profile.rs +++ b/crates/dbx-core/src/sql_dialect/ddl_profile.rs @@ -103,6 +103,11 @@ pub struct DdlDialectProfile { pub auto_inc: AutoIncSyntax, /// When false, display widths like `INT(11)` are stripped if no type_map hit. pub supports_display_width: bool, + /// Whether this target's VARCHAR/CHARACTER VARYING/NVARCHAR syntax rejects a + /// missing length (e.g. `VARCHAR` with no length is a MySQL syntax error). + /// Distinct from `supports_display_width`: that flag is about whether the + /// target *prints* MySQL-style `INT(11)` widths, not whether VARCHAR needs one. + pub requires_explicit_varchar_length: bool, /// Cap for length params when applying `{}` templates (e.g. Access TEXT max 255). pub max_varchar_len: Option, /// MySQL-style inline `COMMENT '...'` on columns. @@ -314,6 +319,7 @@ fn mysql_family(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::Backtick, auto_inc: AutoIncSyntax::Suffix(" AUTO_INCREMENT"), supports_display_width: true, + requires_explicit_varchar_length: true, max_varchar_len: Some(65_535), inline_column_comment: true, foreign_keys_inline_in_create: false, @@ -359,6 +365,7 @@ fn postgres_family(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::DoubleQuote, auto_inc: AutoIncSyntax::PostgresSequence, supports_display_width: false, + requires_explicit_varchar_length: false, max_varchar_len: None, inline_column_comment: false, foreign_keys_inline_in_create: false, @@ -404,6 +411,10 @@ fn oracle_family(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::UnquotedUpper, auto_inc: AutoIncSyntax::Suffix(" GENERATED BY DEFAULT AS IDENTITY"), supports_display_width: false, + // Oracle's VARCHAR2 also rejects a missing length in real DDL, but this + // is left false (unverified, no Oracle instance in scope for this + // change) rather than asserting untested cross-dialect behavior. + requires_explicit_varchar_length: false, max_varchar_len: Some(4000), inline_column_comment: false, foreign_keys_inline_in_create: false, @@ -450,6 +461,7 @@ fn sqlserver_family(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::Brackets, auto_inc: AutoIncSyntax::Suffix(" IDENTITY(1,1)"), supports_display_width: false, + requires_explicit_varchar_length: false, max_varchar_len: None, inline_column_comment: false, foreign_keys_inline_in_create: false, @@ -495,6 +507,7 @@ fn sqlite_family(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::DoubleQuote, auto_inc: AutoIncSyntax::None, supports_display_width: false, + requires_explicit_varchar_length: false, max_varchar_len: None, inline_column_comment: false, foreign_keys_inline_in_create: true, @@ -540,6 +553,7 @@ fn access_profile() -> DdlDialectProfile { quote: QuoteStyle::DoubleQuote, auto_inc: AutoIncSyntax::ReplaceTypeWith("COUNTER"), supports_display_width: false, + requires_explicit_varchar_length: false, max_varchar_len: Some(255), inline_column_comment: false, foreign_keys_inline_in_create: false, @@ -585,6 +599,7 @@ fn h2_profile() -> DdlDialectProfile { quote: QuoteStyle::DoubleQuote, auto_inc: AutoIncSyntax::None, supports_display_width: false, + requires_explicit_varchar_length: false, max_varchar_len: None, inline_column_comment: false, foreign_keys_inline_in_create: false, @@ -630,6 +645,12 @@ fn conservative_ansi(db: DatabaseType) -> DdlDialectProfile { quote: QuoteStyle::DoubleQuote, auto_inc: AutoIncSyntax::None, supports_display_width: false, + // This fallback also covers DB2 and Informix, which reject a bare + // VARCHAR with no length — but it's shared with dialects that don't + // (Snowflake, BigQuery, DuckDB, Trino, ...), so it can't be flipped + // true here without a dedicated DB2/Informix profile and DB + // verification. Tracked as a known gap, not fixed by this flag. + requires_explicit_varchar_length: false, max_varchar_len: None, inline_column_comment: false, foreign_keys_inline_in_create: false, diff --git a/crates/dbx-core/src/sql_dialect/type_rewrite.rs b/crates/dbx-core/src/sql_dialect/type_rewrite.rs index 68d11a1adf..271fa471a8 100644 --- a/crates/dbx-core/src/sql_dialect/type_rewrite.rs +++ b/crates/dbx-core/src/sql_dialect/type_rewrite.rs @@ -343,10 +343,33 @@ pub fn rewrite_column_type(source_type: &str, target: DatabaseType, source_diale // Dialect-kind matrix if let Some(src) = source_dialect { let target_kind = DialectKind::from_database_type(target); - let matrix = TypeMappingMatrix::for_dialects(src, target_kind); - let (converted, _) = matrix.convert_type(source_type); - if converted != source_type { - return strip_display_width_if_needed(&converted, &profile); + if src != target_kind { + let matrix = TypeMappingMatrix::for_dialects(src, target_kind); + let (converted, _) = matrix.convert_type(source_type); + if converted != source_type { + return strip_display_width_if_needed(&converted, &profile); + } + // No matrix rule for this cross-dialect pair, and it's a + // *varying*-length string type reported without a length — e.g. + // Postgres/Kingbase's unbounded `character varying` via + // format_type(). A bare passthrough would be invalid DDL for a + // target that requires an explicit VARCHAR length (MySQL and its + // family: VARCHAR/CHARACTER VARYING with no length is a syntax + // error there). Default to 255, matching the same fallback + // apply_template() already uses when a mapped type's length is + // missing. CHAR/CHARACTER are deliberately excluded: MySQL + // accepts a bare CHAR as CHAR(1), so defaulting it to 255 would + // silently change valid output into a different, wrong length. + // VARCHAR2/NVARCHAR2 are also excluded: they aren't valid MySQL + // type names at all, with or without a length, so defaulting + // their length fixes nothing. + if params.is_none() + && profile.requires_explicit_varchar_length + && profile.max_varchar_len.is_some() + && matches!(base.as_str(), "VARCHAR" | "CHARACTER VARYING" | "NVARCHAR") + { + return format!("{}(255)", source_type.trim()); + } } } @@ -509,6 +532,45 @@ mod tests { assert_eq!(t, "int(11)"); } + #[test] + fn mysql_defaults_missing_length_on_cross_dialect_varchar() { + // Postgres/Kingbase report an unbounded varchar column as a bare + // `character varying` (no length) via format_type(). MySQL's + // VARCHAR/CHARACTER VARYING requires an explicit length, so passing + // that through verbatim is a syntax error (issue #8011). + let source = Some(DialectKind::Postgres); + assert_eq!(rewrite_column_type("character varying", DatabaseType::Mysql, source), "character varying(255)"); + assert_eq!(rewrite_column_type("varchar", DatabaseType::Mysql, source), "varchar(255)"); + } + + #[test] + fn mysql_leaves_bare_char_untouched() { + // Unlike VARCHAR, a bare CHAR/CHARACTER is already valid MySQL + // syntax (implicitly CHAR(1)); defaulting it to 255 would silently + // change correct output into a different, wrong length. + let source = Some(DialectKind::Postgres); + assert_eq!(rewrite_column_type("character", DatabaseType::Mysql, source), "character"); + assert_eq!(rewrite_column_type("char", DatabaseType::Mysql, source), "char"); + } + + #[test] + fn mysql_keeps_explicit_length_on_cross_dialect_varchar() { + let source = Some(DialectKind::Postgres); + assert_eq!( + rewrite_column_type("character varying(120)", DatabaseType::Mysql, source), + "character varying(120)" + ); + } + + #[test] + fn mysql_same_dialect_varchar_is_untouched_regardless_of_length() { + // Same-dialect metadata (or unknown source dialect) must never be + // rewritten — only a genuine cross-dialect gap should trigger the + // missing-length default. + assert_eq!(rewrite_column_type("varchar(255)", DatabaseType::Mysql, Some(DialectKind::Mysql)), "varchar(255)"); + assert_eq!(rewrite_column_type("varchar(255)", DatabaseType::Mysql, None), "varchar(255)"); + } + #[test] fn normalize_len_params_strips_oracle_char_unit() { assert_eq!(normalize_len_params("(6 CHAR)"), "(6)");