From 32389f7dc29ff13040bd0f18540e821b67e802f6 Mon Sep 17 00:00:00 2001 From: zipg Date: Mon, 21 Sep 2026 16:38:47 +0800 Subject: [PATCH] fix(sqlserver): tolerate tinyint foreign key referential actions --- .../tests/live_sqlserver_ddl_fk_display.rs | 87 ++++++++++ .../tests/live_sqlserver_foreign_keys.rs | 92 +++++++++++ crates/dbx-drivers/src/db/sqlserver.rs | 152 ++++++++++++++---- 3 files changed, 304 insertions(+), 27 deletions(-) create mode 100644 crates/dbx-core/tests/live_sqlserver_ddl_fk_display.rs create mode 100644 crates/dbx-core/tests/live_sqlserver_foreign_keys.rs diff --git a/crates/dbx-core/tests/live_sqlserver_ddl_fk_display.rs b/crates/dbx-core/tests/live_sqlserver_ddl_fk_display.rs new file mode 100644 index 0000000000..d89a536224 --- /dev/null +++ b/crates/dbx-core/tests/live_sqlserver_ddl_fk_display.rs @@ -0,0 +1,87 @@ +//! Live regression coverage for the DDL path of a SQL Server table that owns a +//! foreign key: `sys.foreign_keys.delete_referential_action` / +//! `update_referential_action` are `tinyint` and used to be read with +//! `row.get::()`, which panicked inside tiberius and aborted the process +//! (the desktop app crashed on "查看 DDL" and on the hover DDL preview). + +use dbx_core::connection::AppState; +use dbx_core::models::connection::{ConnectionConfig, DatabaseType}; +use dbx_core::query::execute_sql_statement; +use dbx_core::storage::Storage; +use std::sync::Arc; + +fn live_sqlserver_config(id: &str, database: &str) -> ConnectionConfig { + let host = std::env::var("DBX_LIVE_SQLSERVER_HOST").expect("DBX_LIVE_SQLSERVER_HOST"); + let port: u16 = std::env::var("DBX_LIVE_SQLSERVER_PORT") + .expect("DBX_LIVE_SQLSERVER_PORT") + .parse() + .expect("valid DBX_LIVE_SQLSERVER_PORT"); + let username = std::env::var("DBX_LIVE_SQLSERVER_USER").expect("DBX_LIVE_SQLSERVER_USER"); + let password = std::env::var("DBX_LIVE_SQLSERVER_PASSWORD").expect("DBX_LIVE_SQLSERVER_PASSWORD"); + + serde_json::from_value::(serde_json::json!({ + "id": id, + "name": id, + "db_type": DatabaseType::SqlServer, + "host": host, + "port": port, + "username": username, + "password": password, + "database": database, + "connect_timeout_secs": 15, + "query_timeout_secs": 30, + "idle_timeout_secs": 60, + "keepalive_interval_secs": 0 + })) + .expect("live SQL Server config should deserialize") +} + +#[tokio::test] +#[ignore = "requires DBX_LIVE_SQLSERVER_HOST/PORT/USER/PASSWORD pointing at a writable SQL Server"] +async fn live_sqlserver_display_ddl_reads_foreign_key_actions() { + let suffix = uuid::Uuid::new_v4().simple().to_string(); + let connection_id = format!("live-sqlserver-ddl-fk-{suffix}"); + let database = format!("dbx_ddl_fk_{suffix}"); + let dir = std::env::temp_dir().join(format!("dbx-live-sqlserver-ddl-fk-{suffix}")); + std::fs::create_dir_all(&dir).unwrap(); + let storage = Storage::open(&dir.join("storage.db")).await.unwrap(); + let state = Arc::new(AppState::new(storage)); + state.configs.write().await.insert(connection_id.clone(), live_sqlserver_config(&connection_id, "master")); + + let parent = format!("dbx_ddl_parent_{suffix}"); + let child = format!("dbx_ddl_child_{suffix}"); + for sql in [ + format!("CREATE DATABASE [{database}]"), + format!("CREATE TABLE [dbo].[{parent}] ([id] INT NOT NULL CONSTRAINT [PK_{parent}] PRIMARY KEY CLUSTERED ([id] ASC))"), + format!( + "CREATE TABLE [dbo].[{child}] ([id] INT NOT NULL CONSTRAINT [PK_{child}] PRIMARY KEY CLUSTERED ([id] ASC), \ + [parent_id] INT NULL, CONSTRAINT [FK_{child}_parent] FOREIGN KEY ([parent_id]) \ + REFERENCES [dbo].[{parent}] ([id]) ON DELETE CASCADE ON UPDATE NO ACTION)" + ), + ] { + let sql = if sql.starts_with("CREATE TABLE") { format!("USE [{database}]; {sql}") } else { sql }; + execute_sql_statement(&state, &connection_id, "", &sql, None, None) + .await + .unwrap_or_else(|error| panic!("fixture statement failed: {error}")); + } + + // The desktop app calls exactly this path for the DDL tab and the hover preview. + let ddl = dbx_core::schema::get_table_display_ddl_core(&state, &connection_id, &database, "dbo", &child, None) + .await + .expect("display DDL for a table owning a foreign key"); + + let cleanup = execute_sql_statement( + &state, + &connection_id, + "", + &format!("USE [master]; ALTER DATABASE [{database}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; DROP DATABASE [{database}]"), + None, + None, + ) + .await; + + assert!(ddl.contains("FOREIGN KEY ([parent_id])"), "DDL keeps the foreign key: {ddl}"); + assert!(ddl.contains("ON DELETE CASCADE"), "DDL keeps the delete action: {ddl}"); + assert!(!ddl.contains("ON UPDATE"), "NO ACTION stays implicit: {ddl}"); + cleanup.expect("drop the live SQL Server DDL fixture database"); +} diff --git a/crates/dbx-core/tests/live_sqlserver_foreign_keys.rs b/crates/dbx-core/tests/live_sqlserver_foreign_keys.rs new file mode 100644 index 0000000000..1453411ec7 --- /dev/null +++ b/crates/dbx-core/tests/live_sqlserver_foreign_keys.rs @@ -0,0 +1,92 @@ +use std::time::Duration; + +/// `sys.foreign_keys.delete_referential_action` and `update_referential_action` are +/// `tinyint`, which tiberius decodes as `ColumnData::U8`. Reading them with +/// `row.get::()` panicked and aborted the process, so schema comparison crashed +/// on the first table that owned a foreign key. This covers the live read path. +#[tokio::test] +#[ignore = "requires DBX_LIVE_SQLSERVER_HOST/PORT/USER/PASSWORD pointing at a writable SQL Server database"] +async fn live_sqlserver_foreign_key_referential_actions_are_read_from_tinyint_columns() { + let host = std::env::var("DBX_LIVE_SQLSERVER_HOST").expect("DBX_LIVE_SQLSERVER_HOST"); + let port = std::env::var("DBX_LIVE_SQLSERVER_PORT") + .expect("DBX_LIVE_SQLSERVER_PORT") + .parse() + .expect("valid DBX_LIVE_SQLSERVER_PORT"); + let user = std::env::var("DBX_LIVE_SQLSERVER_USER").expect("DBX_LIVE_SQLSERVER_USER"); + let password = std::env::var("DBX_LIVE_SQLSERVER_PASSWORD").expect("DBX_LIVE_SQLSERVER_PASSWORD"); + let database = std::env::var("DBX_LIVE_SQLSERVER_DATABASE").unwrap_or_else(|_| "tempdb".to_string()); + + let mut client = + dbx_core::db::sqlserver::connect(&host, port, &user, &password, Some(&database), None, Duration::from_secs(15)) + .await + .expect("connect SQL Server"); + + let suffix = uuid::Uuid::new_v4().simple().to_string(); + let parent_table = format!("dbx_fk_parent_{suffix}"); + let cascade_child_table = format!("dbx_fk_cascade_child_{suffix}"); + let set_null_child_table = format!("dbx_fk_setnull_child_{suffix}"); + + let cleanup = format!( + "DROP TABLE IF EXISTS [dbo].[{cascade_child_table}];\ + DROP TABLE IF EXISTS [dbo].[{set_null_child_table}];\ + DROP TABLE IF EXISTS [dbo].[{parent_table}];" + ); + let _ = dbx_core::db::sqlserver::execute_batch(&mut client, &cleanup).await; + + let setup = format!( + "CREATE TABLE [dbo].[{parent_table}] (\ + [id] INT NOT NULL CONSTRAINT [PK_{parent_table}] PRIMARY KEY CLUSTERED ([id] ASC)\ + );\ + CREATE TABLE [dbo].[{cascade_child_table}] (\ + [id] INT NOT NULL CONSTRAINT [PK_{cascade_child_table}] PRIMARY KEY CLUSTERED ([id] ASC),\ + [parent_id] INT NULL,\ + CONSTRAINT [FK_{cascade_child_table}_parent] FOREIGN KEY ([parent_id])\ + REFERENCES [dbo].[{parent_table}] ([id]) ON DELETE CASCADE ON UPDATE NO ACTION\ + );\ + CREATE TABLE [dbo].[{set_null_child_table}] (\ + [id] INT NOT NULL CONSTRAINT [PK_{set_null_child_table}] PRIMARY KEY CLUSTERED ([id] ASC),\ + [parent_id] INT NULL,\ + CONSTRAINT [FK_{set_null_child_table}_parent] FOREIGN KEY ([parent_id])\ + REFERENCES [dbo].[{parent_table}] ([id]) ON DELETE SET NULL ON UPDATE SET DEFAULT\ + );" + ); + dbx_core::db::sqlserver::execute_batch(&mut client, &setup).await.expect("create foreign key fixtures"); + + let exercise = async { + let cascade = dbx_core::db::sqlserver::list_foreign_keys(&mut client, "dbo", &cascade_child_table).await?; + assert_eq!(cascade.len(), 1, "expected one foreign key on the cascade child table"); + assert_eq!(cascade[0].ref_table, parent_table); + assert_eq!(cascade[0].ref_column.as_str(), "id"); + assert_eq!(cascade[0].column.as_str(), "parent_id"); + assert_eq!(cascade[0].on_delete.as_deref(), Some("CASCADE")); + assert_eq!(cascade[0].on_update, None, "ON UPDATE NO ACTION is the default and stays unset"); + + let set_null = dbx_core::db::sqlserver::list_foreign_keys(&mut client, "dbo", &set_null_child_table).await?; + assert_eq!(set_null.len(), 1, "expected one foreign key on the set-null child table"); + assert_eq!(set_null[0].on_delete.as_deref(), Some("SET NULL")); + assert_eq!(set_null[0].on_update.as_deref(), Some("SET DEFAULT")); + + let parent = dbx_core::db::sqlserver::list_foreign_keys(&mut client, "dbo", &parent_table).await?; + assert!(parent.is_empty(), "parent table owns no outbound foreign keys"); + + // Mirror the schema comparison loop, which loads every table's metadata and + // DDL one table at a time and used to abort on the first table with a + // foreign key. + let tables = dbx_core::db::sqlserver::list_tables(&mut client, "dbo", None, None, None).await?; + for table in tables.iter().filter(|table| table.table_type.eq_ignore_ascii_case("BASE TABLE")) { + dbx_core::db::sqlserver::list_foreign_keys(&mut client, "dbo", &table.name) + .await + .unwrap_or_else(|error| panic!("list_foreign_keys failed for {}: {error}", table.name)); + dbx_core::schema::build_sqlserver_ddl(&mut client, "dbo", &table.name) + .await + .unwrap_or_else(|error| panic!("build_sqlserver_ddl failed for {}: {error}", table.name)); + } + + Ok::<(), String>(()) + } + .await; + + let cleanup_result = dbx_core::db::sqlserver::execute_batch(&mut client, &cleanup).await; + exercise.expect("read referential actions from tinyint catalog columns"); + cleanup_result.expect("drop foreign key fixtures"); +} diff --git a/crates/dbx-drivers/src/db/sqlserver.rs b/crates/dbx-drivers/src/db/sqlserver.rs index f06e319a99..34f9149f47 100644 --- a/crates/dbx-drivers/src/db/sqlserver.rs +++ b/crates/dbx-drivers/src/db/sqlserver.rs @@ -3015,7 +3015,25 @@ pub async fn list_foreign_keys( schema: &str, table: &str, ) -> Result, String> { - let sql = format!( + let sql = sqlserver_foreign_keys_sql(schema, table); + let stream = client.query(&*sql, &[]).await.map_err(|e| e.to_string())?; + let rows = stream.into_first_result().await.map_err(|e| e.to_string())?; + Ok(rows + .iter() + .map(|row| ForeignKeyInfo { + name: row.get::<&str, _>(0).unwrap_or("").to_string(), + column: row.get::<&str, _>(1).unwrap_or("").to_string(), + ref_schema: Some(row.get::<&str, _>(2).unwrap_or("").to_string()), + ref_table: row.get::<&str, _>(3).unwrap_or("").to_string(), + ref_column: row.get::<&str, _>(4).unwrap_or("").to_string(), + on_update: sqlserver_referential_action_from_row(row, 6), + on_delete: sqlserver_referential_action_from_row(row, 5), + }) + .collect()) +} + +fn sqlserver_foreign_keys_sql(schema: &str, table: &str) -> String { + format!( "SELECT fk.name, c.name, SCHEMA_NAME(rt.schema_id), rt.name, rc.name, \ fk.delete_referential_action, fk.update_referential_action \ FROM sys.foreign_keys fk \ @@ -3027,21 +3045,36 @@ pub async fn list_foreign_keys( ORDER BY fk.name, fkc.constraint_column_id", s = schema.replace('\'', "''"), t = table.replace('\'', "''") - ); - let stream = client.query(&*sql, &[]).await.map_err(|e| e.to_string())?; - let rows = stream.into_first_result().await.map_err(|e| e.to_string())?; - Ok(rows - .iter() - .map(|row| ForeignKeyInfo { - name: row.get::<&str, _>(0).unwrap_or("").to_string(), - column: row.get::<&str, _>(1).unwrap_or("").to_string(), - ref_schema: Some(row.get::<&str, _>(2).unwrap_or("").to_string()), - ref_table: row.get::<&str, _>(3).unwrap_or("").to_string(), - ref_column: row.get::<&str, _>(4).unwrap_or("").to_string(), - on_update: sqlserver_referential_action(row.get::(6).unwrap_or(0)), - on_delete: sqlserver_referential_action(row.get::(5).unwrap_or(0)), - }) - .collect()) + ) +} + +/// `sys.foreign_keys.delete_referential_action` and `update_referential_action` +/// are `tinyint`. tiberius picks the integer width from the received byte length +/// rather than the declared type, so those columns arrive as `ColumnData::U8` and +/// `row.get::` panics on the failed conversion (the whole process aborts +/// under `panic = "abort"`). Read through the narrow integer widths instead. +fn sqlserver_referential_action_from_row(row: &Row, index: usize) -> Option { + sqlserver_referential_action(sqlserver_narrow_i32( + row.try_get::(index), + row.try_get::(index), + row.try_get::(index), + )) +} + +/// Collapses the `int` / `smallint` / `tinyint` reads of one column into a single +/// value. A failed or null conversion falls through to the narrower widths, so +/// `tinyint` columns (decoded as `U8`) are read correctly instead of panicking. +fn sqlserver_narrow_i32( + as_i32: tiberius::Result>, + as_i16: tiberius::Result>, + as_u8: tiberius::Result>, +) -> i32 { + as_i32 + .ok() + .flatten() + .or_else(|| as_i16.ok().flatten().map(i32::from)) + .or_else(|| as_u8.ok().flatten().map(i32::from)) + .unwrap_or(0) } /// sys.foreign_keys referential actions: 0 = NO ACTION (default, omitted), @@ -3806,17 +3839,17 @@ mod tests { restore_sqlserver_spatial_column_types, restore_sqlserver_unsafe_column_types, server_messages_query_result, sqlserver_batch_can_use_execute, sqlserver_bulk_token_row, sqlserver_cell_to_json, sqlserver_columns_sql, sqlserver_completion_assistant_sql, sqlserver_constraints_sql, sqlserver_dml_output_returns_rows, - sqlserver_done_trace_event, sqlserver_filter_definition_error, sqlserver_hidden_schema_names, - sqlserver_indexes_sql, sqlserver_legacy_indexes_sql, sqlserver_legacy_probe, sqlserver_legacy_probe_with_nonce, - sqlserver_legacy_wildcard_metadata_query, sqlserver_list_objects_sql, sqlserver_list_schemas_sql, - sqlserver_list_tables_sql, sqlserver_probe_explicit_alias, sqlserver_query_messages, - sqlserver_query_transport_for_engine_edition, sqlserver_query_transport_for_request, - sqlserver_schema_name_predicate, sqlserver_spatial_marker, sqlserver_split_name_list, - sqlserver_supports_session_database_switch, sqlserver_table_comment_sql, sqlserver_table_objects_sql, - sqlserver_triggers_sql, sqlserver_visible_object_predicate, strip_dbx_sqlserver_row_number_column, - SqlServerDescribedColumn, SqlServerProbeOutputNameOverride, SqlServerQueryTransport, SqlServerRestoredColumn, - SqlServerResultSet, SqlServerSpatialColumn, SqlServerTdsEvent, SQLSERVER_COMPLETION_CONTEXT_SQL, - SQLSERVER_RESULT_TYPE_PROBE_SQL, + sqlserver_done_trace_event, sqlserver_filter_definition_error, sqlserver_foreign_keys_sql, + sqlserver_hidden_schema_names, sqlserver_indexes_sql, sqlserver_legacy_indexes_sql, sqlserver_legacy_probe, + sqlserver_legacy_probe_with_nonce, sqlserver_legacy_wildcard_metadata_query, sqlserver_list_objects_sql, + sqlserver_list_schemas_sql, sqlserver_list_tables_sql, sqlserver_narrow_i32, sqlserver_probe_explicit_alias, + sqlserver_query_messages, sqlserver_query_transport_for_engine_edition, sqlserver_query_transport_for_request, + sqlserver_referential_action, sqlserver_schema_name_predicate, sqlserver_spatial_marker, + sqlserver_split_name_list, sqlserver_supports_session_database_switch, sqlserver_table_comment_sql, + sqlserver_table_objects_sql, sqlserver_triggers_sql, sqlserver_visible_object_predicate, + strip_dbx_sqlserver_row_number_column, SqlServerDescribedColumn, SqlServerProbeOutputNameOverride, + SqlServerQueryTransport, SqlServerRestoredColumn, SqlServerResultSet, SqlServerSpatialColumn, + SqlServerTdsEvent, SQLSERVER_COMPLETION_CONTEXT_SQL, SQLSERVER_RESULT_TYPE_PROBE_SQL, }; use crate::types::{ CompletionAssistantMatchMode, CompletionAssistantObjectKind, CompletionAssistantRequest, QueryResult, @@ -4658,6 +4691,71 @@ mod tests { assert!(!sql.contains("STRING_AGG")); } + #[test] + fn sqlserver_foreign_keys_sql_selects_referential_actions_and_escapes_names() { + let sql = sqlserver_foreign_keys_sql("d'bo", "t'able"); + + assert!(sql.contains("fk.delete_referential_action")); + assert!(sql.contains("fk.update_referential_action")); + assert!(sql.contains("sys.foreign_keys fk")); + assert!(sql.contains("OBJECT_ID('d''bo.t''able')")); + assert!(sql.contains("ORDER BY fk.name, fkc.constraint_column_id")); + } + + #[test] + fn sqlserver_tinyint_referential_actions_decode_as_u8_not_i32() { + // sys.foreign_keys.delete_referential_action / update_referential_action + // are tinyint, and tiberius derives the integer width from the received + // byte length rather than the declared type, so the value arrives as + // ColumnData::U8. + let tinyint = ColumnData::U8(Some(1)); + + assert_eq!(::from_sql(&tinyint).unwrap(), Some(1)); + // row.get::() panics on this conversion error, which aborts the + // release build (`panic = "abort"`), so the reader must tolerate the + // narrow widths instead. + assert!(::from_sql(&tinyint).is_err()); + assert!(::from_sql(&tinyint).is_err()); + } + + #[test] + fn sqlserver_narrow_i32_falls_back_to_tinyint_without_panicking() { + let conversion_error: tiberius::Result> = + ::from_sql(&ColumnData::U8(Some(1))); + assert!(conversion_error.is_err()); + + // A tinyint CASCADE arrives as U8 while the wider reads fail; the value + // must survive instead of aborting the process. + let cascade: tiberius::Result> = ::from_sql(&ColumnData::U8(Some(1))); + let as_u8: tiberius::Result> = ::from_sql(&ColumnData::U8(Some(1))); + assert_eq!(sqlserver_narrow_i32(conversion_error, cascade, as_u8), 1); + } + + #[test] + fn sqlserver_narrow_i32_prefers_wider_widths_and_defaults_to_zero() { + assert_eq!(sqlserver_narrow_i32(Ok(Some(2)), Ok(Some(9)), Ok(Some(9))), 2); + assert_eq!(sqlserver_narrow_i32(Ok(None), Ok(Some(3)), Ok(Some(9))), 3); + // Null or unreadable values fall back to 0, which maps to no action. + assert_eq!(sqlserver_narrow_i32(Ok(None), Ok(None), Ok(None)), 0); + assert_eq!( + sqlserver_narrow_i32( + Err(tiberius::error::Error::Conversion("boom".into())), + Err(tiberius::error::Error::Conversion("boom".into())), + Err(tiberius::error::Error::Conversion("boom".into())) + ), + 0 + ); + } + + #[test] + fn sqlserver_referential_action_maps_tinyint_codes() { + assert_eq!(sqlserver_referential_action(0), None); + assert_eq!(sqlserver_referential_action(1).as_deref(), Some("CASCADE")); + assert_eq!(sqlserver_referential_action(2).as_deref(), Some("SET NULL")); + assert_eq!(sqlserver_referential_action(3).as_deref(), Some("SET DEFAULT")); + assert_eq!(sqlserver_referential_action(4), None); + } + #[test] fn sqlserver_constraints_sql_unions_every_constraint_catalog() { let sql = sqlserver_constraints_sql("d'bo", "t'able");