diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 7d50ed4b..b94dfeed 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -1569,116 +1569,30 @@ async fn migrate_ssh_connections(app: &AppHandle) -> Result<(), S } // ==================== PostgreSQL Plugin SSL Mode Migration ==================== - -/// The SSL mode dropdown used to branch on `driver === "postgres"` literally -/// (issue #614), so a postgres-dialect driver with a different id (e.g. the -/// standalone PostgreSQL plugin, id `"postgresql"`) offered MySQL-style -/// underscored `ssl_mode` values instead of Postgres-style hyphenated ones. -/// The plugin's own TLS check only recognizes the hyphenated spelling, so a -/// connection saved with the wrong-family value connects in cleartext with no -/// error. Fixing the dropdown only stops *new* saves from getting the wrong -/// value — this rewrites values already persisted before the fix shipped. -/// -/// Maps a stale MySQL-style `ssl_mode` value to its Postgres-style -/// equivalent. Returns `None` for a value that isn't one of the stale -/// MySQL-style spellings (including values already correct, or driver- -/// specific values like ClickHouse's `"disable"`/`"require"`, which happen to -/// already be spelled correctly in both families and need no rewrite). -fn stale_postgres_ssl_mode_replacement(value: &str) -> Option<&'static str> { - match value { - "disabled" => Some("disable"), - "preferred" => Some("prefer"), - "required" => Some("require"), - "verify_ca" => Some("verify-ca"), - "verify_identity" => Some("verify-full"), - _ => None, - } -} - -/// Rewrites `conn.params.ssl_mode` in place if `conn`'s driver resolves (via -/// `dialects`) to the postgres SQL dialect and its stored value is a stale -/// MySQL-style spelling. Builtin `"postgres"` connections are excluded — -/// their own dropdown was always correct, so nothing there needs migrating. -/// A driver whose manifest doesn't declare `sql_dialect` (`None`, or absent -/// from the map because it never resolved) is treated as NOT postgres — -/// deliberately not defaulted, unlike the splitter's own historical -/// postgres-default, because this migration must distinguish "explicitly -/// postgres" from "unspecified" to avoid rewriting a driver's SSL value -/// based on a guess. -/// Pure and synchronous so it can be exercised directly in tests without a -/// live driver registry. Returns whether a rewrite happened. -fn migrate_connection_ssl_mode_in_place( - conn: &mut SavedConnection, - dialects: &HashMap>, -) -> bool { - if conn.params.driver == "postgres" { - return false; - } - let is_postgres_dialect = dialects - .get(&conn.params.driver) - .copied() - .flatten() - .is_some_and(|d| d == crate::drivers::driver_trait::SqlDialect::Postgres); - if !is_postgres_dialect { - return false; - } - let Some(stale) = conn.params.ssl_mode.as_deref() else { - return false; - }; - let Some(replacement) = stale_postgres_ssl_mode_replacement(stale) else { - return false; - }; - conn.params.ssl_mode = Some(replacement.to_string()); - true -} +// +// The shared, path-based migration logic lives in `connection_migrations` +// (also callable from the standalone `--mcp` server process, which has no +// `AppHandle`). This is a thin wrapper: resolve the path, run the shared +// core, and invalidate the connection cache only if it actually rewrote +// something. /// Migrates already-persisted `ssl_mode` values on postgres-dialect /// connections (driver id other than the builtin `"postgres"`) from the /// stale MySQL-style spelling to the Postgres-style spelling the plugin /// actually understands. Idempotent — a no-op once every affected -/// connection has been rewritten. +/// connection has been rewritten. See +/// `connection_migrations::migrate_postgres_ssl_mode_spelling_at_path` for +/// the full rationale, including the concurrency guard against a race with +/// the `--mcp` process's own call to the same function. async fn migrate_postgres_ssl_mode_spelling(app: &AppHandle) -> Result<(), String> { let conn_path = get_config_path(app)?; - if !conn_path.exists() { - return Ok(()); // Nothing to migrate - } - - let mut conn_file = persistence::load_connections_file(&conn_path)?; - - // Resolve each distinct non-builtin driver id's dialect once, not once - // per connection — the registry lookup is async and connections commonly - // share a driver. - let mut dialects: HashMap> = - HashMap::new(); - for conn in &conn_file.connections { - let driver_id = &conn.params.driver; - if driver_id == "postgres" || dialects.contains_key(driver_id) { - continue; // builtin driver's own dropdown was always correct - } - if let Some(driver) = crate::drivers::registry::get_driver(driver_id).await { - dialects.insert( - driver_id.clone(), - driver.manifest().capabilities.sql_dialect, - ); - } - } - - let mut migrated_count = 0usize; - for conn in conn_file.connections.iter_mut() { - if migrate_connection_ssl_mode_in_place(conn, &dialects) { - migrated_count += 1; - } - } - - if migrated_count == 0 { - return Ok(()); // No migration needed + let migrated = + crate::connection_migrations::migrate_postgres_ssl_mode_spelling_at_path(&conn_path) + .await?; + if migrated { + app.state::>() + .invalidate(); } - - eprintln!( - "[Migration] Rewriting stale ssl_mode spelling on {} postgres-dialect connection(s)", - migrated_count - ); - save_connections_and_invalidate(app, &conn_path, &conn_file)?; Ok(()) } @@ -2581,156 +2495,6 @@ mod tests { } } - #[test] - fn stale_postgres_ssl_mode_replacement_maps_every_mysql_style_value() { - assert_eq!(stale_postgres_ssl_mode_replacement("disabled"), Some("disable")); - assert_eq!(stale_postgres_ssl_mode_replacement("preferred"), Some("prefer")); - assert_eq!(stale_postgres_ssl_mode_replacement("required"), Some("require")); - assert_eq!(stale_postgres_ssl_mode_replacement("verify_ca"), Some("verify-ca")); - assert_eq!( - stale_postgres_ssl_mode_replacement("verify_identity"), - Some("verify-full"), - ); - } - - #[test] - fn stale_postgres_ssl_mode_replacement_leaves_already_correct_values_alone() { - for already_correct in - ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"] - { - assert_eq!( - stale_postgres_ssl_mode_replacement(already_correct), - None, - "{already_correct} should not be rewritten", - ); - } - } - - #[test] - fn stale_postgres_ssl_mode_replacement_ignores_unrecognized_values() { - assert_eq!(stale_postgres_ssl_mode_replacement(""), None); - assert_eq!(stale_postgres_ssl_mode_replacement("not-a-real-mode"), None); - } - - fn saved_connection(driver: &str, ssl_mode: Option<&str>) -> SavedConnection { - SavedConnection { - id: "conn-1".to_string(), - name: "test".to_string(), - params: ConnectionParams { - driver: driver.to_string(), - ssl_mode: ssl_mode.map(str::to_string), - ..base_params() - }, - group_id: None, - sort_order: None, - detect_json_in_text_columns: None, - appearance: None, - tag_ids: None, - environment: None, - } - } - - #[test] - fn migrate_connection_ssl_mode_rewrites_a_plugin_postgres_connection() { - let mut dialects = HashMap::new(); - dialects.insert( - "postgresql".to_string(), - Some(crate::drivers::driver_trait::SqlDialect::Postgres), - ); - let mut conn = saved_connection("postgresql", Some("required")); - - let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); - - assert!(rewrote); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); - } - - #[test] - fn migrate_connection_ssl_mode_leaves_a_mysql_connection_alone() { - // "required" is the CORRECT spelling for mysql — this is the case - // that makes the migration driver-aware rather than a blanket - // string-remap. A dialect map that (incorrectly) resolved mysql to - // Postgres would also demonstrate the bug this guards against, so - // this test exercises the real decision, not just the dialect map. - let mut dialects = HashMap::new(); - dialects.insert( - "mysql".to_string(), - Some(crate::drivers::driver_trait::SqlDialect::Mysql), - ); - let mut conn = saved_connection("mysql", Some("required")); - - let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); - - assert!(!rewrote); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); - } - - #[test] - fn migrate_connection_ssl_mode_leaves_the_builtin_postgres_driver_alone() { - // The builtin driver's own dropdown was always correct — even if it - // somehow ended up with a stale value, this migration is scoped to - // plugin-driven connections only (driver id != "postgres"). - let mut dialects = HashMap::new(); - dialects.insert( - "postgres".to_string(), - Some(crate::drivers::driver_trait::SqlDialect::Postgres), - ); - let mut conn = saved_connection("postgres", Some("required")); - - let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); - - assert!(!rewrote); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); - } - - #[test] - fn migrate_connection_ssl_mode_leaves_an_unresolved_driver_alone() { - // No entry in `dialects` (e.g. the driver failed to resolve from the - // registry) must not be treated as postgres-dialect by default. - let dialects = HashMap::new(); - let mut conn = saved_connection("postgresql", Some("required")); - - let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); - - assert!(!rewrote); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); - } - - #[test] - fn migrate_connection_ssl_mode_leaves_a_resolved_driver_with_no_declared_dialect_alone() { - // A driver that resolves from the registry but whose manifest omits - // `sql_dialect` entirely (e.g. the Oracle plugin, which sets - // supports_ssl but declares no dialect) must be treated as NOT - // postgres-dialect — `None`, not defaulted to `Some(Postgres)`. - // Getting this wrong would rewrite that driver's legitimately-spelled - // SSL value based on a guess, exactly the bug this test guards - // against. - let mut dialects = HashMap::new(); - dialects.insert("oracle".to_string(), None); - let mut conn = saved_connection("oracle", Some("required")); - - let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); - - assert!(!rewrote); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); - } - - #[test] - fn migrate_connection_ssl_mode_is_idempotent() { - let mut dialects = HashMap::new(); - dialects.insert( - "postgresql".to_string(), - Some(crate::drivers::driver_trait::SqlDialect::Postgres), - ); - let mut conn = saved_connection("postgresql", Some("required")); - - assert!(migrate_connection_ssl_mode_in_place(&mut conn, &dialects)); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); - // Second pass: the value is already correct, nothing to rewrite. - assert!(!migrate_connection_ssl_mode_in_place(&mut conn, &dialects)); - assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); - } - #[test] fn persisted_params_never_contain_the_connection_uri() { let sentinel = "mongodb+srv://fixture-user:fixture-password@cluster.example.invalid/app"; diff --git a/src-tauri/src/connection_migrations.rs b/src-tauri/src/connection_migrations.rs new file mode 100644 index 00000000..429eee57 --- /dev/null +++ b/src-tauri/src/connection_migrations.rs @@ -0,0 +1,178 @@ +//! Migrations for already-persisted connection data — rewriting values on +//! disk that a previous version of the app saved incorrectly. Each migration +//! is path-based (no `AppHandle` dependency) so it can run from both the GUI +//! process's Tauri commands and the standalone `--mcp` server process, which +//! reads and (as of the SSL-mode migration below) writes the same +//! `connections.json` but has no Tauri context. + +use std::collections::HashMap; +use std::fs; +use std::path::Path; + +use crate::drivers::driver_trait::SqlDialect; +use crate::drivers::registry as driver_registry; +use crate::models::SavedConnection; +use crate::persistence; + +/// The SSL mode dropdown used to branch on `driver === "postgres"` literally +/// (issue #614), so a postgres-dialect driver with a different id (e.g. the +/// standalone PostgreSQL plugin, id `"postgresql"`) offered MySQL-style +/// underscored `ssl_mode` values instead of Postgres-style hyphenated ones. +/// The plugin's own TLS check only recognizes the hyphenated spelling, so a +/// connection saved with the wrong-family value connects in cleartext with no +/// error. Fixing the dropdown only stops *new* saves from getting the wrong +/// value — this rewrites values already persisted before the fix shipped. +/// +/// Maps a stale MySQL-style `ssl_mode` value to its Postgres-style +/// equivalent. Returns `None` for a value that isn't one of the stale +/// MySQL-style spellings (including values already correct, or driver- +/// specific values like ClickHouse's `"disable"`/`"require"`, which happen to +/// already be spelled correctly in both families and need no rewrite). +pub(crate) fn stale_postgres_ssl_mode_replacement(value: &str) -> Option<&'static str> { + match value { + "disabled" => Some("disable"), + "preferred" => Some("prefer"), + "required" => Some("require"), + "verify_ca" => Some("verify-ca"), + "verify_identity" => Some("verify-full"), + _ => None, + } +} + +/// Rewrites `conn.params.ssl_mode` in place if `conn`'s driver resolves (via +/// `dialects`) to the postgres SQL dialect and its stored value is a stale +/// MySQL-style spelling. Builtin `"postgres"` connections are excluded — +/// their own dropdown was always correct, so nothing there needs migrating. +/// A driver whose manifest doesn't declare `sql_dialect` (`None`, or absent +/// from the map because it never resolved) is treated as NOT postgres — +/// deliberately not defaulted, unlike the splitter's own historical +/// postgres-default, because this migration must distinguish "explicitly +/// postgres" from "unspecified" to avoid rewriting a driver's SSL value +/// based on a guess. +/// Pure and synchronous so it can be exercised directly in tests without a +/// live driver registry. Returns whether a rewrite happened. +pub(crate) fn migrate_connection_ssl_mode_in_place( + conn: &mut SavedConnection, + dialects: &HashMap>, +) -> bool { + if conn.params.driver == "postgres" { + return false; + } + let is_postgres_dialect = dialects + .get(&conn.params.driver) + .copied() + .flatten() + .is_some_and(|d| d == SqlDialect::Postgres); + if !is_postgres_dialect { + return false; + } + let Some(stale) = conn.params.ssl_mode.as_deref() else { + return false; + }; + let Some(replacement) = stale_postgres_ssl_mode_replacement(stale) else { + return false; + }; + conn.params.ssl_mode = Some(replacement.to_string()); + true +} + +/// True if `connections.json` changed since `content_before` was captured — +/// i.e. another process wrote a concurrent edit while this migration was +/// computing its own rewrite. Pure and synchronous so the concurrency guard +/// itself can be tested without touching the driver registry or the +/// filesystem beyond what the caller already read. +/// +/// Content comparison (not mtime) is used deliberately: some filesystems +/// have coarse (1-second) mtime resolution, which wouldn't reliably detect +/// two writes landing within the same second. +pub(crate) fn connections_file_changed_concurrently( + content_before: &str, + content_now: &str, +) -> bool { + content_before != content_now +} + +/// Migrates already-persisted `ssl_mode` values on postgres-dialect +/// connections (driver id other than the builtin `"postgres"`) from the +/// stale MySQL-style spelling to the Postgres-style spelling the plugin +/// actually understands. Idempotent — a no-op once every affected +/// connection has been rewritten. +/// +/// Path-based, no `AppHandle` — callable from both the GUI process (wrapped +/// by `commands::migrate_postgres_ssl_mode_spelling`, which additionally +/// invalidates the connection cache) and the standalone `--mcp` server +/// process, which has no Tauri context and no cache to invalidate. +/// +/// Returns `Ok(true)` only once the rewrite has actually been committed to +/// disk — `Ok(false)` covers both "nothing needed migrating" and "a +/// migration was computed but skipped because the file changed concurrently" +/// (see the concurrency note below). +/// +/// # Concurrency +/// +/// This is the first write path the `--mcp` process exercises against +/// `connections.json` — previously read-only there. The GUI app enforces +/// single-instance, but MCP clients (Claude Desktop, Cursor, etc.) spawn +/// `tabularis --mcp` as an independent subprocess that can run at the same +/// time as the GUI, and this repo's connection persistence layer has no file +/// locking anywhere. To avoid silently clobbering a concurrent GUI edit made +/// while this function is mid read-modify-write, the file's raw content is +/// compared immediately before writing (`connections_file_changed_concurrently`); +/// if it changed since this function's own read, the migration is skipped +/// for this run rather than overwritten. This is safe because the migration +/// is idempotent and self-terminating — the next process to load +/// connections (GUI reopen, or MCP's next startup) will see the still-stale +/// value and retry. +pub async fn migrate_postgres_ssl_mode_spelling_at_path(conn_path: &Path) -> Result { + if !conn_path.exists() { + return Ok(false); // Nothing to migrate + } + + let content_before = fs::read_to_string(conn_path).map_err(|e| e.to_string())?; + let mut conn_file = persistence::parse_connections_file(&content_before)?; + + // Resolve each distinct non-builtin driver id's dialect once, not once + // per connection — the registry lookup is async and connections commonly + // share a driver. + let mut dialects: HashMap> = HashMap::new(); + for conn in &conn_file.connections { + let driver_id = &conn.params.driver; + if driver_id == "postgres" || dialects.contains_key(driver_id) { + continue; // builtin driver's own dropdown was always correct + } + if let Some(driver) = driver_registry::get_driver(driver_id).await { + dialects.insert( + driver_id.clone(), + driver.manifest().capabilities.sql_dialect, + ); + } + } + + let mut migrated_count = 0usize; + for conn in conn_file.connections.iter_mut() { + if migrate_connection_ssl_mode_in_place(conn, &dialects) { + migrated_count += 1; + } + } + + if migrated_count == 0 { + return Ok(false); // No migration needed + } + + // Bail if another process (e.g. the GUI) saved a concurrent edit while + // we were computing the migration above — don't blindly overwrite it. + let content_now = fs::read_to_string(conn_path).map_err(|e| e.to_string())?; + if connections_file_changed_concurrently(&content_before, &content_now) { + eprintln!( + "[Migration] connections.json changed concurrently — skipping this run, will retry next launch" + ); + return Ok(false); + } + + eprintln!( + "[Migration] Rewriting stale ssl_mode spelling on {} postgres-dialect connection(s)", + migrated_count + ); + persistence::save_connections_file(conn_path, &conn_file)?; + Ok(true) +} diff --git a/src-tauri/src/connection_migrations_tests.rs b/src-tauri/src/connection_migrations_tests.rs new file mode 100644 index 00000000..0893ab79 --- /dev/null +++ b/src-tauri/src/connection_migrations_tests.rs @@ -0,0 +1,241 @@ +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use tempfile::TempDir; + + use crate::connection_migrations::{ + connections_file_changed_concurrently, migrate_connection_ssl_mode_in_place, + migrate_postgres_ssl_mode_spelling_at_path, stale_postgres_ssl_mode_replacement, + }; + use crate::drivers::driver_trait::SqlDialect; + use crate::models::{ConnectionParams, ConnectionsFile, DatabaseSelection, SavedConnection}; + use crate::persistence; + + fn base_params() -> ConnectionParams { + ConnectionParams { + driver: "mysql".to_string(), + host: Some("localhost".to_string()), + port: Some(3306), + username: Some("root".to_string()), + database: DatabaseSelection::Single("testdb".to_string()), + ..Default::default() + } + } + + fn saved_connection(driver: &str, ssl_mode: Option<&str>) -> SavedConnection { + SavedConnection { + id: "conn-1".to_string(), + name: "test".to_string(), + params: ConnectionParams { + driver: driver.to_string(), + ssl_mode: ssl_mode.map(str::to_string), + ..base_params() + }, + group_id: None, + sort_order: None, + detect_json_in_text_columns: None, + appearance: None, + tag_ids: None, + environment: None, + } + } + + #[test] + fn stale_postgres_ssl_mode_replacement_maps_every_mysql_style_value() { + assert_eq!( + stale_postgres_ssl_mode_replacement("disabled"), + Some("disable") + ); + assert_eq!( + stale_postgres_ssl_mode_replacement("preferred"), + Some("prefer") + ); + assert_eq!( + stale_postgres_ssl_mode_replacement("required"), + Some("require") + ); + assert_eq!( + stale_postgres_ssl_mode_replacement("verify_ca"), + Some("verify-ca") + ); + assert_eq!( + stale_postgres_ssl_mode_replacement("verify_identity"), + Some("verify-full"), + ); + } + + #[test] + fn stale_postgres_ssl_mode_replacement_leaves_already_correct_values_alone() { + for already_correct in [ + "disable", + "allow", + "prefer", + "require", + "verify-ca", + "verify-full", + ] { + assert_eq!( + stale_postgres_ssl_mode_replacement(already_correct), + None, + "{already_correct} should not be rewritten", + ); + } + } + + #[test] + fn stale_postgres_ssl_mode_replacement_ignores_unrecognized_values() { + assert_eq!(stale_postgres_ssl_mode_replacement(""), None); + assert_eq!(stale_postgres_ssl_mode_replacement("not-a-real-mode"), None); + } + + #[test] + fn migrate_connection_ssl_mode_rewrites_a_plugin_postgres_connection() { + let mut dialects = HashMap::new(); + dialects.insert("postgresql".to_string(), Some(SqlDialect::Postgres)); + let mut conn = saved_connection("postgresql", Some("required")); + + let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); + + assert!(rewrote); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); + } + + #[test] + fn migrate_connection_ssl_mode_leaves_a_mysql_connection_alone() { + // "required" is the CORRECT spelling for mysql — this is the case + // that makes the migration driver-aware rather than a blanket + // string-remap. A dialect map that (incorrectly) resolved mysql to + // Postgres would also demonstrate the bug this guards against, so + // this test exercises the real decision, not just the dialect map. + let mut dialects = HashMap::new(); + dialects.insert("mysql".to_string(), Some(SqlDialect::Mysql)); + let mut conn = saved_connection("mysql", Some("required")); + + let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); + + assert!(!rewrote); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); + } + + #[test] + fn migrate_connection_ssl_mode_leaves_the_builtin_postgres_driver_alone() { + // The builtin driver's own dropdown was always correct — even if it + // somehow ended up with a stale value, this migration is scoped to + // plugin-driven connections only (driver id != "postgres"). + let mut dialects = HashMap::new(); + dialects.insert("postgres".to_string(), Some(SqlDialect::Postgres)); + let mut conn = saved_connection("postgres", Some("required")); + + let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); + + assert!(!rewrote); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); + } + + #[test] + fn migrate_connection_ssl_mode_leaves_an_unresolved_driver_alone() { + // No entry in `dialects` (e.g. the driver failed to resolve from the + // registry) must not be treated as postgres-dialect by default. + let dialects = HashMap::new(); + let mut conn = saved_connection("postgresql", Some("required")); + + let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); + + assert!(!rewrote); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); + } + + #[test] + fn migrate_connection_ssl_mode_leaves_a_resolved_driver_with_no_declared_dialect_alone() { + // A driver that resolves from the registry but whose manifest omits + // `sql_dialect` entirely (e.g. the Oracle plugin, which sets + // supports_ssl but declares no dialect) must be treated as NOT + // postgres-dialect — `None`, not defaulted to `Some(Postgres)`. + // Getting this wrong would rewrite that driver's legitimately-spelled + // SSL value based on a guess, exactly the bug this test guards + // against. + let mut dialects = HashMap::new(); + dialects.insert("oracle".to_string(), None); + let mut conn = saved_connection("oracle", Some("required")); + + let rewrote = migrate_connection_ssl_mode_in_place(&mut conn, &dialects); + + assert!(!rewrote); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("required")); + } + + #[test] + fn migrate_connection_ssl_mode_is_idempotent() { + let mut dialects = HashMap::new(); + dialects.insert("postgresql".to_string(), Some(SqlDialect::Postgres)); + let mut conn = saved_connection("postgresql", Some("required")); + + assert!(migrate_connection_ssl_mode_in_place(&mut conn, &dialects)); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); + // Second pass: the value is already correct, nothing to rewrite. + assert!(!migrate_connection_ssl_mode_in_place(&mut conn, &dialects)); + assert_eq!(conn.params.ssl_mode.as_deref(), Some("require")); + } + + #[test] + fn connections_file_changed_concurrently_detects_any_byte_difference() { + assert!(!connections_file_changed_concurrently("{}", "{}")); + assert!(connections_file_changed_concurrently("{}", "{ }")); + assert!(connections_file_changed_concurrently("", "{}")); + } + + // --- migrate_postgres_ssl_mode_spelling_at_path: path-based core --- + // + // These exercise only the two no-op paths (missing file; a file whose + // one connection is builtin "postgres", always skipped) without a + // registered driver — the dialect-resolution decision itself is already + // fully covered above by migrate_connection_ssl_mode_in_place's tests, + // which take a `dialects` map directly. + // + // NOT covered here: the actual rewrite-and-save success path (Ok(true)) + // and the concurrent-change skip branch inside this function. Both + // require a driver registered under a non-"postgres" id resolving to + // Postgres dialect, which needs either a live plugin process or a full + // DatabaseDriver mock (62 required methods, only 5 with default bodies) + // — judged disproportionate for this test. The pure concurrency-guard + // logic itself (`connections_file_changed_concurrently`) is fully + // covered above in isolation. + + #[tokio::test] + async fn migrate_postgres_ssl_mode_spelling_at_path_is_a_noop_for_a_missing_file() { + let dir = TempDir::new().expect("create temp dir"); + let path = dir.path().join("connections.json"); + + let migrated = migrate_postgres_ssl_mode_spelling_at_path(&path) + .await + .expect("missing file is not an error"); + + assert!(!migrated); + } + + #[tokio::test] + async fn migrate_postgres_ssl_mode_spelling_at_path_is_a_noop_when_nothing_needs_migrating() { + let dir = TempDir::new().expect("create temp dir"); + let path = dir.path().join("connections.json"); + let file = ConnectionsFile { + groups: Vec::new(), + // Builtin "postgres" is always skipped, regardless of ssl_mode. + connections: vec![saved_connection("postgres", Some("required"))], + tags: Vec::new(), + }; + persistence::save_connections_file(&path, &file).expect("seed fixture"); + let content_before = std::fs::read_to_string(&path).expect("read fixture"); + + let migrated = migrate_postgres_ssl_mode_spelling_at_path(&path) + .await + .expect("no migration needed is not an error"); + + assert!(!migrated); + let content_after = std::fs::read_to_string(&path).expect("read after"); + assert_eq!( + content_before, content_after, + "a no-op run must not rewrite the file at all" + ); + } +} diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index cd73c3e9..8e874d2c 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -27,6 +27,9 @@ pub mod config; pub mod connection_cache; #[cfg(test)] pub mod connection_cache_tests; +pub mod connection_migrations; +#[cfg(test)] +pub mod connection_migrations_tests; pub mod connection_tags; pub mod connection_window; #[cfg(test)] diff --git a/src-tauri/src/mcp/mod.rs b/src-tauri/src/mcp/mod.rs index b6dc8714..3d31180b 100644 --- a/src-tauri/src/mcp/mod.rs +++ b/src-tauri/src/mcp/mod.rs @@ -321,6 +321,16 @@ async fn register_drivers_for_mcp() { let plugin_configs = app_config.plugins.unwrap_or_default(); let enabled_ids = app_config.active_external_drivers; plugins::manager::load_plugins_with_configs(plugin_configs, enabled_ids.as_deref()).await; + + // Must run after driver registration above — the migration resolves each + // connection's driver dialect via the same registry, so it needs plugin + // drivers already loaded. This is the standalone MCP process's own entry + // point for a migration that previously only ran from the GUI process's + // Tauri commands (issue #639) — never blocks server startup on failure. + let conn_path = paths::resolve_connections_path(&paths::get_app_config_dir()); + crate::connection_migrations::migrate_postgres_ssl_mode_spelling_at_path(&conn_path) + .await + .ok(); } /// Resolve the driver for an MCP-known connection. Returns the connection, diff --git a/src-tauri/src/persistence.rs b/src-tauri/src/persistence.rs index 5a910bdc..10b8df31 100644 --- a/src-tauri/src/persistence.rs +++ b/src-tauri/src/persistence.rs @@ -2,22 +2,20 @@ use crate::models::{ConnectionGroup, ConnectionsFile, SavedConnection}; use std::fs; use std::path::Path; -/// Load connections file (raw, no keychain reads). -/// Supports both old format (array of connections) and new format (with groups). -/// Use `load_connections` or `load_connections_with_passwords` when passwords are needed. -pub fn load_connections_file(path: &Path) -> Result { - if !path.exists() { - return Ok(ConnectionsFile::default()); - } - let content = fs::read_to_string(path).map_err(|e| e.to_string())?; - +/// Parses connections file content already read from disk. Supports both +/// the old format (a bare array of connections) and the new format (an +/// object with groups/tags). Split out from `load_connections_file` so a +/// caller that already has the file's content in hand (e.g. to compare +/// against a later read, as `connection_migrations` does) can parse it +/// without triggering a second `fs::read_to_string`. +pub fn parse_connections_file(content: &str) -> Result { // Try parsing as the new format first - if let Ok(file) = serde_json::from_str::(&content) { + if let Ok(file) = serde_json::from_str::(content) { return Ok(file); } // Fall back to old format (array of connections) - let connections: Vec = serde_json::from_str(&content) + let connections: Vec = serde_json::from_str(content) .map_err(|_| "Failed to parse connections file".to_string())?; Ok(ConnectionsFile { @@ -27,6 +25,17 @@ pub fn load_connections_file(path: &Path) -> Result { }) } +/// Load connections file (raw, no keychain reads). +/// Supports both old format (array of connections) and new format (with groups). +/// Use `load_connections` or `load_connections_with_passwords` when passwords are needed. +pub fn load_connections_file(path: &Path) -> Result { + if !path.exists() { + return Ok(ConnectionsFile::default()); + } + let content = fs::read_to_string(path).map_err(|e| e.to_string())?; + parse_connections_file(&content) +} + /// Load connections list (raw, no keychain reads) — for listing UI. pub fn load_connections(path: &Path) -> Result, String> { let file = load_connections_file(path)?;