From be6c2e17d32d34b86a7330d053b5325127d6b061 Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Fri, 14 Aug 2026 11:03:12 -0400 Subject: [PATCH 1/5] refactor: extract SSL-mode migration into a path-based shared module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves stale_postgres_ssl_mode_replacement, migrate_connection_ssl_mode_in_place, and their tests out of commands.rs into a new connection_migrations module. The orchestration function (migrate_postgres_ssl_mode_spelling) is split into a path-based core (migrate_postgres_ssl_mode_spelling_at_path, no AppHandle dependency) plus a thin AppHandle wrapper left in commands.rs that just resolves the path and invalidates the connection cache. Per .rules/rust.md rule 1 (extract pure helpers into dedicated sibling modules) and to avoid a one-directional dependency between commands.rs (6300+ lines) and mcp/mod.rs — this is prep for wiring the migration into the --mcp server process (issue #639), which has no AppHandle. That wiring is a separate follow-up commit; this one is a pure extraction with no behavior change, except the return type improves from Result<()> to Result so callers can tell "nothing needed migrating" apart from "migrated and saved." --- src-tauri/src/commands.rs | 268 ++----------------- src-tauri/src/connection_migrations.rs | 134 ++++++++++ src-tauri/src/connection_migrations_tests.rs | 228 ++++++++++++++++ src-tauri/src/lib.rs | 3 + 4 files changed, 381 insertions(+), 252 deletions(-) create mode 100644 src-tauri/src/connection_migrations.rs create mode 100644 src-tauri/src/connection_migrations_tests.rs 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..41a97e87 --- /dev/null +++ b/src-tauri/src/connection_migrations.rs @@ -0,0 +1,134 @@ +//! 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 a follow-up commit, writes) the same `connections.json` +//! but has no Tauri context. + +use std::collections::HashMap; +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 +} + +/// 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, in a follow-up commit, 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)` when nothing needed migrating — a strict improvement +/// over the previous `Result<()>`, which couldn't distinguish the two. +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 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) = 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 + } + + 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..4c6f9d23 --- /dev/null +++ b/src-tauri/src/connection_migrations_tests.rs @@ -0,0 +1,228 @@ +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use tempfile::TempDir; + + use crate::connection_migrations::{ + 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")); + } + + // --- migrate_postgres_ssl_mode_spelling_at_path: path-based core --- + // + // These exercise the file-level behavior (missing file, no-op file, + // save-and-report-true) without touching the driver registry — 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. A connection with driver == "postgres" is used here + // specifically because the function skips the registry lookup for it + // (line: `if driver_id == "postgres" ... continue`), so these tests + // don't need a registered driver. + + #[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 be2d7a80..9d12af71 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)] From 44eeb5b756376179dd324ef83e5d97222c69bf1d Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Fri, 14 Aug 2026 11:05:34 -0400 Subject: [PATCH 2/5] fix: guard the SSL-mode migration against a concurrent-write race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding this migration to the --mcp process (next commit) makes it the first write path that process exercises against connections.json — previously read-only there. MCP clients spawn tabularis --mcp as an independent subprocess that can run at the same time as the (single-instance) GUI app, and this repo's connection persistence layer has no file locking anywhere. Without a guard, the GUI saving an unrelated edit (e.g. a rename) at the same moment this migration is mid read-modify-write could be silently discarded: the migration reads before the GUI's edit lands, computes its rewrite on stale data, then overwrites. Compares the file's raw content immediately before writing; if it changed since this function's own initial read, skips the write for this run rather than clobbering the concurrent edit. Safe to skip 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. Content comparison (not mtime) is used because some filesystems have coarse (1-second) mtime resolution that wouldn't reliably detect two writes landing within the same second. Considered and rejected: real OS-level file locking (the correct fix, but would need to touch ~20 existing write call sites across commands.rs to achieve actual mutual exclusion, not just this one path — worth its own issue); an atomic temp-file-plus-rename write (fixes torn writes on a crash, not the lost-update race this guards against); and refusing TLS at connection time instead of migrating the saved value (sidesteps the race entirely but changes behavior, not just structure — a bigger conversation than this fix). A compare-and-swap on file content is proportionate to how narrow and self-limiting the actual risk is: at most 2 concurrent writers, one-time and self-terminating once migrated, and the MCP call site only runs once at process startup, not per request. --- src-tauri/src/connection_migrations.rs | 58 +++++++++++++++++--- src-tauri/src/connection_migrations_tests.rs | 11 +++- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src-tauri/src/connection_migrations.rs b/src-tauri/src/connection_migrations.rs index 41a97e87..6568c1b3 100644 --- a/src-tauri/src/connection_migrations.rs +++ b/src-tauri/src/connection_migrations.rs @@ -2,10 +2,11 @@ //! 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 a follow-up commit, writes) the same `connections.json` -//! but has no Tauri context. +//! 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; @@ -75,6 +76,22 @@ pub(crate) fn migrate_connection_ssl_mode_in_place( 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 @@ -83,18 +100,35 @@ pub(crate) fn migrate_connection_ssl_mode_in_place( /// /// 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, in a follow-up commit, the -/// standalone `--mcp` server process, which has no Tauri context and no -/// cache to invalidate. +/// 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)` when nothing needed migrating — a strict improvement -/// over the previous `Result<()>`, which couldn't distinguish the two. +/// 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::load_connections_file(conn_path)?; // Resolve each distinct non-builtin driver id's dialect once, not once @@ -125,6 +159,16 @@ pub async fn migrate_postgres_ssl_mode_spelling_at_path(conn_path: &Path) -> Res 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 diff --git a/src-tauri/src/connection_migrations_tests.rs b/src-tauri/src/connection_migrations_tests.rs index 4c6f9d23..514b443d 100644 --- a/src-tauri/src/connection_migrations_tests.rs +++ b/src-tauri/src/connection_migrations_tests.rs @@ -5,8 +5,8 @@ mod tests { use tempfile::TempDir; use crate::connection_migrations::{ - migrate_connection_ssl_mode_in_place, migrate_postgres_ssl_mode_spelling_at_path, - stale_postgres_ssl_mode_replacement, + 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}; @@ -178,6 +178,13 @@ mod tests { 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 the file-level behavior (missing file, no-op file, From c7058ae87edeba01fbb39a2cd0e6afa6f60edbee Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Fri, 14 Aug 2026 11:07:21 -0400 Subject: [PATCH 3/5] fix: run the SSL-mode migration from the --mcp server process (closes #639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit register_drivers_for_mcp() never called any connection migration — not the new SSL one, and not the pre-existing SSH one either — so a connection saved with a stale ssl_mode value before #614's dropdown fix shipped stayed silently cleartext forever when accessed via `tabularis --mcp`, even after the GUI-side fix landed, as long as it was never reopened in the GUI to trigger get_connections's own migration call. Appended as the last step of register_drivers_for_mcp(), after plugin loading completes — the migration resolves each connection's driver dialect through the same driver registry, so it needs plugin drivers already registered, same ordering constraint the GUI path already has via its .setup() block_on. Errors are swallowed the same way the GUI call site already does (.ok()): migration failure must never block MCP server startup. migrate_ssh_connections has this same gap but is meaningfully more complex — it touches a separate ssh_connections.json file and OS keychain credential migration. Left as a follow-up decision, not fixed here. --- src-tauri/src/mcp/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) 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, From b1ccc641670ac2431c8fe4164b904b9dbd90cf0f Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Fri, 14 Aug 2026 11:41:49 -0400 Subject: [PATCH 4/5] refactor: avoid a redundant file read in the SSL-mode migration guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit migrate_postgres_ssl_mode_spelling_at_path read connections.json twice before comparing content — once directly for content_before, then again inside persistence::load_connections_file. Not a correctness bug (traced every ordering: a concurrent edit landing in the gap between the two reads still gets caught by the final content_before vs content_now comparison, since content_before was captured first and content_now last), just an unnecessary extra syscall and a slightly wider window than needed. Splits load_connections_file's parsing logic into a new persistence::parse_connections_file(&str), so a caller that already has the file's content in hand can parse it without a second read. load_connections_file itself is unchanged in behavior — still reads then delegates to the new function — so none of its ~20 other call sites need to change. Found during PR review (#643). --- src-tauri/src/connection_migrations.rs | 2 +- src-tauri/src/persistence.rs | 31 +++++++++++++++++--------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/connection_migrations.rs b/src-tauri/src/connection_migrations.rs index 6568c1b3..429eee57 100644 --- a/src-tauri/src/connection_migrations.rs +++ b/src-tauri/src/connection_migrations.rs @@ -129,7 +129,7 @@ pub async fn migrate_postgres_ssl_mode_spelling_at_path(conn_path: &Path) -> Res } let content_before = fs::read_to_string(conn_path).map_err(|e| e.to_string())?; - let mut conn_file = persistence::load_connections_file(conn_path)?; + 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 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)?; From 75ddc0e1e77f08dd44ef7c29d4ba2f14e6b45099 Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Fri, 14 Aug 2026 12:06:14 -0400 Subject: [PATCH 5/5] docs: fix a test comment that overstated coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claimed a "save-and-report-true" case existed for migrate_postgres_ssl_mode_spelling_at_path; only the two no-op paths are actually tested. Corrects the comment to say what's covered and, more usefully, what isn't and why (the write-success path and the concurrent- change skip branch both need a driver registered under a non-"postgres" id, which needs either a live plugin process or a 62-method DatabaseDriver mock — judged disproportionate, not an oversight). Found by the Kilo Code review bot on PR #643. --- src-tauri/src/connection_migrations_tests.rs | 22 +++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/connection_migrations_tests.rs b/src-tauri/src/connection_migrations_tests.rs index 514b443d..0893ab79 100644 --- a/src-tauri/src/connection_migrations_tests.rs +++ b/src-tauri/src/connection_migrations_tests.rs @@ -187,14 +187,20 @@ mod tests { // --- migrate_postgres_ssl_mode_spelling_at_path: path-based core --- // - // These exercise the file-level behavior (missing file, no-op file, - // save-and-report-true) without touching the driver registry — 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. A connection with driver == "postgres" is used here - // specifically because the function skips the registry lookup for it - // (line: `if driver_id == "postgres" ... continue`), so these tests - // don't need a registered driver. + // 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() {