From 7ffdce190417b709ebc01e57d5e7d8d777f79ca6 Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sat, 5 Sep 2026 22:15:39 -0700 Subject: [PATCH 1/2] refactor(mcp)!: drive Engine transactions with the RAII Transaction guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Engine::execute_in_transaction` drove the session with the unguarded `begin_transaction_unguarded` / `commit_unguarded` / `rollback_unguarded` trio and discharged the pairing obligation by hand: a `catch_unwind` around the closure plus a three-arm match that rolled back before resuming an unwind. That works, but every exit path has to be named, and the closure received `&Engine` — so transactional code could reach the raw connection and issue statements around the transaction it was supposed to be inside. Hold `hyperdb_api::Transaction` instead. The helper takes `&mut self` (the guard borrows the connection exclusively) and hands the closure an `EngineTransaction` view exposing the three operations the call sites actually use: `execute_command`, `create_table_in`, and `connection()` for `ArrowInserter`. Commit and the error-path rollback stay explicit, so the `tracing::warn!` on a failed rollback is preserved; the panic path is now the guard's `Drop`, which makes `catch_unwind` redundant and covers exit paths a hand-written match cannot enumerate. Two call sites held a `ScopedSearchPath` across the transaction. That guard borrows `&Engine` for its whole lifetime and cannot coexist with the transaction's exclusive borrow of the same connection, so add `Engine::with_search_path(alias, f)` — the closure form, which sequences the set/restore around `f` rather than holding a borrow across it. It restores on the `Ok`, `Err`, and panic paths, matching the guard's `Drop`. `scoped_search_path` is unchanged and still preferred where `&Engine` suffices. The `&mut Engine` requirement propagates to the seven ingest entry points and to `merge_via_temp_table`, which now passes the engine into its `replace_load` closure instead of having callers capture it alongside the `&mut` borrow. `HyperMcpServer::with_engine` hands out `&mut Engine`, which costs nothing: the engine already lives behind an exclusive `Arc>>`, so no caller was ever sharing it. `create_table_in`'s validation and identifier quoting move into a shared `create_table_statements` builder so the `&Engine` and transaction-scoped paths cannot drift. Tests: `execute_in_transaction_commit_outlives_its_transaction` forces the committed row to survive a *later* transaction's rollback, so replacing the explicit commit with a bare drop fails it; and `execute_in_transaction_never_leaks_an_open_transaction` walks commit -> error -> panic -> commit on one engine, catching any path that leaves a `BEGIN` open. Four tests cover `with_search_path` restoring on success, error, panic, and the `None` no-routing case. Refs #72 BREAKING CHANGE: `hyperdb-mcp` is published to crates.io, and this reshapes its library surface: `Engine::execute_in_transaction` takes `&mut self` and yields `EngineTransaction` rather than `&Engine`, the seven ingest entry points and `merge_via_temp_table` take `&mut Engine`, and `Engine::with_search_path` is new. The `missing_docs` allow reason on `src/lib.rs` claimed the crate is not published; corrected in passing. What the guard does *not* cover: the panic still propagates, exactly as the previous `resume_unwind` did, so `with_engine`'s `MutexGuard` is dropped mid-unwind and poisons the engine mutex. `ensure_engine` then fails every later tool call with "Lock poisoned" and nothing calls `clear_poison()`. That is pre-existing, not a regression, but the docs asserted otherwise, so `DEVELOPMENT.md` now scopes the claim to the SQL session and flags mutex recovery as an open design question. Also corrects a false claim this change introduced: `MIGRATING-0.3.md` and `docs/TRANSACTIONS.md` said nothing outside `hyperdb-api`'s tests and examples calls the unguarded methods. `KvStore::{pop, set_batch, set_batch_if_absent}` and the three `AsyncKvStore` equivalents do. They are not migrable by the same trick — they hold `&Connection`, so the guard's `&mut self` would have to be threaded out through `Connection::kv_store()` — and the async three cannot be fixed by any guard, since Rust has no async `Drop`. Both documents now name them as holdouts and describe the residual panic and cancellation windows. `Transaction::drop` now logs its implicit rollback (`warn!` on failure, `debug!` on success), restoring the observability the removed `catch_unwind` provided on the panic path. --- MIGRATING-0.3.md | 8 +- docs/TRANSACTIONS.md | 21 +- hyperdb-api/CHANGELOG.md | 8 + hyperdb-api/src/transaction.rs | 17 +- hyperdb-mcp/CHANGELOG.md | 29 +- hyperdb-mcp/DEVELOPMENT.md | 23 +- hyperdb-mcp/examples/demo.rs | 4 +- hyperdb-mcp/src/engine.rs | 356 ++++++++++++++----- hyperdb-mcp/src/ingest.rs | 45 +-- hyperdb-mcp/src/ingest_arrow.rs | 50 +-- hyperdb-mcp/src/lakehouse.rs | 8 +- hyperdb-mcp/src/lib.rs | 2 +- hyperdb-mcp/src/server.rs | 41 ++- hyperdb-mcp/tests/export_tests.rs | 16 +- hyperdb-mcp/tests/ingest_arrow_tests.rs | 76 ++-- hyperdb-mcp/tests/ingest_tests.rs | 127 ++++--- hyperdb-mcp/tests/integration_tests.rs | 16 +- hyperdb-mcp/tests/lakehouse_tests.rs | 16 +- hyperdb-mcp/tests/per_tool_database_tests.rs | 150 +++++++- hyperdb-mcp/tests/transaction_tests.rs | 156 ++++++-- 20 files changed, 811 insertions(+), 358 deletions(-) diff --git a/MIGRATING-0.3.md b/MIGRATING-0.3.md index 6500e812..d4b88d7c 100644 --- a/MIGRATING-0.3.md +++ b/MIGRATING-0.3.md @@ -252,7 +252,13 @@ Callers that hold a pooled connection (`deadpool::managed::Object Transaction<'conn> { impl Drop for Transaction<'_> { fn drop(&mut self) { if !self.completed { - // Best-effort rollback; ignore errors during drop. + // Best-effort rollback: a `Drop` cannot report failure, so log + // instead of swallowing silently. This is the only trace of an + // implicit rollback — on the panic path there is no `Err` for the + // caller to inspect. // Hyper produces a WARNING (not error) if no active transaction. - let _ = self.connection.rollback_unguarded(); + if let Err(e) = self.connection.rollback_unguarded() { + tracing::warn!( + error = %e, + "Transaction dropped without commit/rollback and the implicit ROLLBACK failed \ + — the transaction may still be open on this connection" + ); + } else { + tracing::debug!( + "Transaction dropped without commit/rollback — issued implicit ROLLBACK" + ); + } } } } diff --git a/hyperdb-mcp/CHANGELOG.md b/hyperdb-mcp/CHANGELOG.md index 96cca2a2..42791ca2 100644 --- a/hyperdb-mcp/CHANGELOG.md +++ b/hyperdb-mcp/CHANGELOG.md @@ -121,13 +121,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/). `parquet` 58.x pinned `thrift ^0.17`, and `parquet` 59 dropped thrift entirely. -- `Engine::execute_in_transaction` now calls `hyperdb-api`'s `*_unguarded` - transaction methods instead of the deprecated `begin_transaction` / `commit` - / `rollback`, which 1.0.0 removed. No behavior change: the helper still takes - `&self`, so the RAII guard remains unavailable to it, and it still rolls back - before resuming an unwind. The `#[allow(deprecated)]` it needed is gone. - Moving to the guard still waits on - [issue #72](https://github.com/tableau/hyper-api-rust/issues/72). +- **BREAKING:** `Engine::execute_in_transaction` now holds `hyperdb-api`'s RAII + `Transaction` guard instead of driving the session with the unguarded + `begin`/`commit`/`rollback` methods + ([#72](https://github.com/tableau/hyper-api-rust/issues/72)). The helper + takes `&mut self` and its closure receives an `EngineTransaction` view + rather than `&Engine`, so transactional work can no longer reach around the + transaction to the raw connection. Rollback on the panic path is now the + guard's `Drop` rather than a hand-written `catch_unwind`; commit and + error-path rollback are unchanged, including the `tracing::warn!` on a + failed rollback. The panic itself still propagates, exactly as the previous + `resume_unwind` did, so a panicking tool call still poisons the server's + engine mutex: the guard cleans up the SQL session, not the process state. The ingest entry points (`ingest_json`, `ingest_csv`, + `ingest_csv_file`, `ingest_json_file`, `ingest_parquet_file`, + `ingest_arrow_ipc_file`, `ingest_iceberg_table`) and + `merge_via_temp_table` take `&mut Engine` to match; `merge_via_temp_table` + passes the engine to its `replace_load` closure instead of having callers + capture it. +- **New** `Engine::with_search_path(alias, f)` — the closure form of + `scoped_search_path`, for callers that need `&mut Engine` inside the scope. + It restores the primary database on the `Ok`, `Err`, and panic paths, the + same as the guard's `Drop`. `scoped_search_path` is unchanged and still + preferred where `&Engine` suffices. - **BREAKING:** the minimum supported Rust version is now **1.88**, up from 1.81, and the crate is compiled with **edition 2024**. 1.88 is the version Red Hat Enterprise Linux 9.7 ships as `rust-toolset`. diff --git a/hyperdb-mcp/DEVELOPMENT.md b/hyperdb-mcp/DEVELOPMENT.md index c4f79bb1..3c91b0cb 100644 --- a/hyperdb-mcp/DEVELOPMENT.md +++ b/hyperdb-mcp/DEVELOPMENT.md @@ -101,16 +101,21 @@ Every ingest function (`ingest_json`, `ingest_csv`, `ingest_parquet_file`, `inge Three edges to this guarantee, all documented in `src/engine.rs`: 1. **DDL auto-commits.** Hyper commits `CREATE TABLE` / `DROP TABLE` immediately, regardless of the surrounding transaction. In `replace` mode the original table is already gone by the time INSERTs start, so a failed replace-mode ingest leaves an empty table rather than restoring the original. Append mode is fully atomic because it issues DDL only when the target doesn't exist and, when it does, no data is lost on failure. -2. **Panic safety.** `execute_in_transaction` wraps the closure in - `catch_unwind(AssertUnwindSafe(...))`, issues a best-effort ROLLBACK on - panic, and `resume_unwind`s the original payload. Without this, a panic - inside the closure (unwrap on None, indexing OOB, arithmetic overflow) would - leave an open transaction and every subsequent tool call would hit - "transaction already in progress" — classified as `InternalError`, not - `ConnectionLost`, so the reconnect path at `with_engine` would not rescue it - and the engine would stay wedged until restart. Tested via - `execute_in_transaction_rolls_back_on_panic` in +2. **Panic safety, at the SQL level only.** `execute_in_transaction` holds a + `hyperdb_api::Transaction` RAII guard and hands the closure an + `EngineTransaction` view of it. The guard's `Drop` issues the ROLLBACK, so a + panic inside the closure (unwrap on None, indexing OOB, arithmetic overflow) + rolls back as the unwind passes through — no `catch_unwind` needed, and no + exit path can forget. That is the whole of what the guard buys: the + *session* is left clean, so no later statement hits "transaction already in + progress" on a rolled-back transaction. Tested via + `execute_in_transaction_rolls_back_on_panic` and + `execute_in_transaction_never_leaks_an_open_transaction` in `tests/transaction_tests.rs`. + + It does **not** keep a panicking tool call from wedging the server, and neither did the `catch_unwind` it replaced — both re-raise the panic once the rollback is done. + `with_engine` holds a `std::sync::MutexGuard` across the closure, so the unwind poisons `Arc>>`; `ensure_engine` then returns `InternalError "Lock poisoned"` for every subsequent tool call, and nothing calls `clear_poison()`. The engine is unusable for the process lifetime. + Recovering from poisoning — most plausibly by having `ensure_engine` drop the engine and re-spawn, as it already does for `ConnectionLost` — is a live design question, deliberately out of scope for issue #72. Note that the two tests above drive `TestEngine` directly and never cross `with_engine`, so they cannot observe the poisoning. 3. **Post-error wire-protocol quirk.** After a mid-transaction Hyper-level error (e.g. a NOT NULL violation on INSERT), the first SELECT after rollback may return an empty result set due to residual bytes on the connection. Retrying the query once restores normal behavior; the rollback itself is always correct. The `query_resilient` helper in `tests/transaction_tests.rs` is the robust pattern. --- diff --git a/hyperdb-mcp/examples/demo.rs b/hyperdb-mcp/examples/demo.rs index e0311cf0..e1151063 100644 --- a/hyperdb-mcp/examples/demo.rs +++ b/hyperdb-mcp/examples/demo.rs @@ -229,7 +229,7 @@ fn main() -> Result<(), Box> { // ── Step 1: spin up engine ───────────────────────────────────────── section("Step 1 · Launch the engine (local database)"); - let engine = Engine::new(None)?; + let mut engine = Engine::new(None)?; println!(" Ephemeral DB: {}", engine.ephemeral_path().display()); println!(" Log dir: {}", engine.log_dir().display()); println!(" hyperd is running: {}", engine.is_running()); @@ -243,7 +243,7 @@ fn main() -> Result<(), Box> { merge_key: None, target_db: None, }; - let ingest_result = ingest_csv_file(&engine, csv_path.to_str().unwrap(), &ingest_opts)?; + let ingest_result = ingest_csv_file(&mut engine, csv_path.to_str().unwrap(), &ingest_opts)?; println!( " Ingested {} rows into `coder_stats`.", ingest_result.rows diff --git a/hyperdb-mcp/src/engine.rs b/hyperdb-mcp/src/engine.rs index 6955116b..f653159a 100644 --- a/hyperdb-mcp/src/engine.rs +++ b/hyperdb-mcp/src/engine.rs @@ -54,7 +54,7 @@ use crate::error::{ErrorCode, McpError}; use crate::schema::ColumnSchema; use hyperdb_api::{ Catalog, Connection, CopyTableReport, CreateMode, HyperProcess, Parameters, SqlType, - escape_sql_path, + Transaction, escape_sql_path, }; use serde_json::{Value, json}; use std::path::{Path, PathBuf}; @@ -239,6 +239,167 @@ impl Drop for ScopedSearchPath<'_> { } } +/// Builds the DDL for [`Engine::create_table_in`] and +/// [`EngineTransaction::create_table_in`] — the statements to run, in order. +/// +/// Kept separate from execution so both the plain `&Engine` path and the +/// transaction-scoped path share one definition of the schema validation and +/// identifier quoting. Yields a `DROP TABLE IF EXISTS` first when `replace`, +/// then the `CREATE TABLE IF NOT EXISTS`. +/// +/// # Errors +/// +/// - [`ErrorCode::EmptyData`] if `columns` is empty. +/// - [`ErrorCode::SchemaMismatch`] if a column's `hyper_type` is not +/// resolvable by [`crate::schema::map_hyper_type`]. +fn create_table_statements( + table_name: &str, + columns: &[ColumnSchema], + replace: bool, + target_db: Option<&str>, +) -> Result, McpError> { + if columns.is_empty() { + return Err(McpError::new( + ErrorCode::EmptyData, + "No columns to create table from", + )); + } + for col in columns { + if crate::schema::map_hyper_type(&col.hyper_type).is_none() { + return Err(McpError::new( + ErrorCode::SchemaMismatch, + format!( + "Unknown type '{}' for column '{}'", + col.hyper_type, col.name + ), + )); + } + } + + let quoted_table = match target_db { + Some(db) => { + let esc_db = db.replace('"', "\"\""); + let esc_tbl = table_name.replace('"', "\"\""); + format!("\"{esc_db}\".\"public\".\"{esc_tbl}\"") + } + None => format!("\"{}\"", table_name.replace('"', "\"\"")), + }; + + let col_defs: Vec = columns + .iter() + .map(|c| { + let nullable = if c.nullable { "" } else { " NOT NULL" }; + format!( + "\"{}\" {}{}", + c.name.replace('"', "\"\""), + c.hyper_type, + nullable + ) + }) + .collect(); + + let mut statements = Vec::with_capacity(2); + if replace { + statements.push(format!("DROP TABLE IF EXISTS {quoted_table}")); + } + statements.push(format!( + "CREATE TABLE IF NOT EXISTS {} ({})", + quoted_table, + col_defs.join(", ") + )); + Ok(statements) +} + +/// The transaction-scoped view of an [`Engine`], handed to +/// [`Engine::execute_in_transaction`] closures. +/// +/// Wraps [`hyperdb_api::Transaction`] — the RAII guard that issues +/// `ROLLBACK` on drop unless committed — and re-exposes the engine +/// operations that transactional callers need, translating +/// [`hyperdb_api::Error`] into [`McpError`] as the equivalent `&Engine` +/// methods do. +/// +/// Closures receive this rather than `&Engine` for a reason: the guard +/// holds `&mut Connection`, so the borrow checker will not let the same +/// connection be driven around the transaction. That statically rules out +/// the "statement escaped the transaction" bug class that the previous +/// `&self` + `*_unguarded` shape could only address by convention. +#[derive(Debug)] +pub struct EngineTransaction<'conn> { + txn: Transaction<'conn>, +} + +impl EngineTransaction<'_> { + /// Commits the transaction. + /// + /// # Errors + /// + /// Propagates the server's `COMMIT` failure as an [`McpError`]. + pub fn commit(self) -> Result<(), McpError> { + self.txn.commit().map_err(McpError::from) + } + + /// Rolls the transaction back explicitly. + /// + /// Dropping the guard rolls back too; this exists so callers can + /// observe (and log) a rollback failure. + /// + /// # Errors + /// + /// Propagates the server's `ROLLBACK` failure as an [`McpError`]. + pub fn rollback(self) -> Result<(), McpError> { + self.txn.rollback().map_err(McpError::from) + } + + /// Executes a DDL/DML command inside the transaction. Returns the + /// affected row count. + /// + /// The transaction-scoped counterpart of [`Engine::execute_command`], + /// with the same error conversion. + /// + /// # Errors + /// + /// Converts any [`hyperdb_api::Error`] into an [`McpError`] — SQL + /// syntax errors, constraint violations, and connection loss all + /// surface here. + pub fn execute_command(&self, sql: &str) -> Result { + self.txn.execute_command(sql).map_err(McpError::from) + } + + /// Creates a table inside the transaction, optionally in a + /// non-primary database. + /// + /// The transaction-scoped counterpart of [`Engine::create_table_in`]; + /// see that method for the `replace` semantics and the note on Hyper + /// auto-committing DDL. + /// + /// # Errors + /// + /// Same as [`Engine::create_table_in`]. + pub fn create_table_in( + &self, + table_name: &str, + columns: &[ColumnSchema], + replace: bool, + target_db: Option<&str>, + ) -> Result<(), McpError> { + for sql in create_table_statements(table_name, columns, replace, target_db)? { + self.execute_command(&sql)?; + } + Ok(()) + } + + /// The connection this transaction is running on, for the APIs that + /// take a `&Connection` directly (e.g. [`hyperdb_api::ArrowInserter`]). + /// + /// Work issued through this reference lands inside the transaction — + /// it is the same session the guard holds open. + #[must_use] + pub fn connection(&self) -> &Connection { + self.txn.connection() + } +} + /// - The connection is *bound* to the ephemeral primary at /// [`Self::ephemeral_path`]. Unqualified SQL routes here. /// - When [`Self::persistent_path`] is `Some`, the server attaches that @@ -640,6 +801,74 @@ impl Engine { }) } + /// Runs `f` with the schema search path pointed at `alias`, restoring + /// the primary database afterwards. A `None` alias runs `f` unchanged. + /// + /// The closure form of [`Self::scoped_search_path`], for callers that + /// need `&mut Engine` inside the scope — [`Self::execute_in_transaction`] + /// being the motivating case. The `ScopedSearchPath` guard borrows the + /// engine immutably for its whole lifetime, so it cannot coexist with + /// the transaction guard's exclusive borrow of the same connection; + /// this helper sequences the set/restore around `f` instead of holding + /// a borrow across it. Prefer the guard when `&Engine` suffices. + /// + /// Restoration is unconditional: it runs on the `Ok` path, the `Err` + /// path, and while unwinding from a panic, matching + /// [`ScopedSearchPath`]'s `Drop`. A failed restore is logged, not + /// surfaced — the engine mutex serializes tool calls, so a stale path + /// only survives until the next call sets its own. + /// + /// # Errors + /// + /// Returns the `SET schema_search_path` failure if the path cannot be + /// pointed at `alias`, in which case `f` never runs. Otherwise returns + /// whatever `f` returns. + /// + /// Does **not** nest: the restore target is always + /// [`Self::primary_db_name`], not the search path in effect on entry, so a + /// nested call restores to the primary database rather than to the outer + /// alias. [`Self::scoped_search_path`] has the same limitation. No call + /// site nests today; reading the session's current `schema_search_path` + /// would be the fix if one ever needs to. + /// + /// # Panics + /// + /// Does not introduce new panic sites. A panic inside `f` is caught + /// only long enough to restore the search path, then re-raised via + /// [`std::panic::resume_unwind`] with its payload intact. + pub fn with_search_path(&mut self, alias: Option<&str>, f: F) -> Result + where + F: FnOnce(&mut Engine) -> Result, + { + let Some(alias) = alias else { + return f(self); + }; + let restore_to = self.primary_db_name(); + let set_sql = format!("SET schema_search_path = '{}'", alias.replace('\'', "''")); + self.execute_command(&set_sql)?; + + // `AssertUnwindSafe` is sound for the same reason it is in + // `ScopedSearchPath`'s `Drop`: the only state that outlives the + // unwind is a session variable we are about to overwrite anyway. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))); + + let restore_sql = format!( + "SET schema_search_path = '{}'", + restore_to.replace('\'', "''") + ); + if let Err(e) = self.execute_command(&restore_sql) { + tracing::warn!( + error = %e.message, + "failed to restore schema_search_path — next tool call may route incorrectly" + ); + } + + match result { + Ok(inner) => inner, + Err(panic_payload) => std::panic::resume_unwind(panic_payload), + } + } + /// Directory where `hyperd` writes its log files. The MCP binary should /// also drop its own client-side log here so debugging starts in one /// place. @@ -828,49 +1057,41 @@ impl Engine { /// /// # Panics /// - /// Does not introduce new panic sites. If `f` panics, the transaction - /// is rolled back (best-effort) and the original panic is re-raised - /// via [`std::panic::resume_unwind`], preserving the panic payload. - // Uses the `*_unguarded` transaction methods rather than the RAII guard, - // because this helper takes `&self` and the guard needs `&mut self`. - // Moving to the guard requires reshaping `Engine`'s locking model — see - // issue #72 for two implementation paths (wrap the connection in a - // `Mutex` vs. introduce an `EngineTransaction` guard) and the closure call - // sites that need updating. Until then the pairing obligation the - // `*_unguarded` docs describe is discharged by the `catch_unwind` below, - // which rolls back before resuming any unwind. - pub fn execute_in_transaction(&self, f: F) -> Result + /// Does not introduce new panic sites. If `f` panics, the guard's + /// `Drop` rolls the transaction back as the unwind passes through + /// and the original panic continues to propagate untouched. + pub fn execute_in_transaction(&mut self, f: F) -> Result where - F: FnOnce(&Engine) -> Result, + F: FnOnce(&EngineTransaction<'_>) -> Result, { - self.connection - .begin_transaction_unguarded() - .map_err(McpError::from)?; + let txn = EngineTransaction { + txn: self.connection.transaction().map_err(McpError::from)?, + }; tracing::debug!("tx: BEGIN issued"); - // `catch_unwind` wraps the closure so a panic (unwrap on None, - // indexing OOB, arithmetic overflow, …) doesn't leave an open - // transaction on the connection. Without this, the next tool - // call would hit "transaction already in progress" and the - // server's ConnectionLost auto-reconnect would *not* recover - // because the connection is live; the engine would stay wedged - // until restart. `AssertUnwindSafe` is correct here: we hold - // the transaction open for the closure's duration, and we - // always issue a rollback before resuming the panic, so no - // logical invariant survives into the panicking stack. - let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))); - match result { - Ok(Ok(val)) => { + // No `catch_unwind` here: the guard's `Drop` issues the rollback, + // so a panic in `f` (unwrap on None, indexing OOB, arithmetic + // overflow, …) unwinds through this frame and rolls back on the + // way out. That is the whole point of holding the RAII guard — + // it discharges the pairing obligation on *every* exit path, + // including the ones a hand-written match cannot name. Leaving a + // transaction open would wedge the session: the next tool call + // fails with "transaction already in progress" on a connection + // that is otherwise healthy, so the server's ConnectionLost + // auto-reconnect would not recover it. + match f(&txn) { + Ok(val) => { tracing::debug!("tx: closure returned Ok, issuing COMMIT"); - self.connection.commit_unguarded().map_err(McpError::from)?; + txn.commit()?; Ok(val) } - Ok(Err(e)) => { + Err(e) => { tracing::debug!(err = %e, "tx: closure returned Err, issuing ROLLBACK"); - if let Err(rb_err) = self.connection.rollback_unguarded() { - // Rollback itself failed — log it but keep the original - // error as the primary cause. A failed rollback usually - // means the transaction was already aborted by the server, - // which is fine (nothing to unwind). + // Rolling back explicitly rather than leaning on `Drop` + // so a rollback failure can be logged. The original error + // stays the primary cause either way — a failed rollback + // usually means the server already aborted the + // transaction, which is what we wanted anyway. + if let Err(rb_err) = txn.rollback() { tracing::warn!( "rollback after error failed (original error preserved): {}", rb_err @@ -880,15 +1101,6 @@ impl Engine { } Err(e) } - Err(panic_payload) => { - tracing::error!("tx: closure panicked, issuing ROLLBACK before resuming unwind"); - // Best-effort rollback. If it fails, the connection is - // unusable — but we're about to panic anyway, and - // `HyperMcpServer::with_engine` will drop the engine - // when the panic surfaces as a poisoned tokio task. - let _ = self.connection.rollback_unguarded(); - std::panic::resume_unwind(panic_payload) - } } } @@ -1017,59 +1229,11 @@ impl Engine { replace: bool, target_db: Option<&str>, ) -> Result<(), McpError> { - if columns.is_empty() { - return Err(McpError::new( - ErrorCode::EmptyData, - "No columns to create table from", - )); - } - for col in columns { - if crate::schema::map_hyper_type(&col.hyper_type).is_none() { - return Err(McpError::new( - ErrorCode::SchemaMismatch, - format!( - "Unknown type '{}' for column '{}'", - col.hyper_type, col.name - ), - )); - } - } - - let quoted_table = match target_db { - Some(db) => { - let esc_db = db.replace('"', "\"\""); - let esc_tbl = table_name.replace('"', "\"\""); - format!("\"{esc_db}\".\"public\".\"{esc_tbl}\"") - } - None => format!("\"{}\"", table_name.replace('"', "\"\"")), - }; - if replace { + for sql in create_table_statements(table_name, columns, replace, target_db)? { self.connection - .execute_command(&format!("DROP TABLE IF EXISTS {quoted_table}")) + .execute_command(&sql) .map_err(McpError::from)?; } - - let col_defs: Vec = columns - .iter() - .map(|c| { - let nullable = if c.nullable { "" } else { " NOT NULL" }; - format!( - "\"{}\" {}{}", - c.name.replace('"', "\"\""), - c.hyper_type, - nullable - ) - }) - .collect(); - - let create_sql = format!( - "CREATE TABLE IF NOT EXISTS {} ({})", - quoted_table, - col_defs.join(", ") - ); - self.connection - .execute_command(&create_sql) - .map_err(McpError::from)?; Ok(()) } diff --git a/hyperdb-mcp/src/ingest.rs b/hyperdb-mcp/src/ingest.rs index 23688815..285aa7b4 100644 --- a/hyperdb-mcp/src/ingest.rs +++ b/hyperdb-mcp/src/ingest.rs @@ -13,9 +13,10 @@ //! # Atomicity //! //! Every ingest function wraps its `INSERT` / `COPY` work inside a single -//! transaction via [`Engine::execute_in_transaction`]. If any row fails to -//! insert, all prior inserts from the same call are rolled back, so a failed -//! ingest leaves zero additional rows behind. +//! transaction via [`Engine::execute_in_transaction`], which holds an RAII +//! guard for the duration. If any row fails to insert, all prior inserts from +//! the same call are rolled back, so a failed ingest leaves zero additional +//! rows behind — and the guard's `Drop` covers the panic path too. //! //! Note that Hyper auto-commits DDL (`DROP TABLE`, `CREATE TABLE`) regardless //! of the surrounding transaction. In `replace` mode, this means the original @@ -295,12 +296,12 @@ impl Drop for TempTableGuard<'_> { /// INSERT statements. The temp table is dropped before the error /// propagates. pub fn merge_via_temp_table( - engine: &Engine, + engine: &mut Engine, opts: &IngestOptions, replace_load: F, ) -> Result where - F: FnOnce(&IngestOptions) -> Result, + F: FnOnce(&mut Engine, &IngestOptions) -> Result, { // Belt-and-suspenders contract check. Every per-format ingest only // calls this helper when `opts.mode == "merge"`; if a future @@ -373,7 +374,7 @@ where merge_key: None, target_db: opts.target_db.clone(), }; - let tmp_result = replace_load(&tmp_opts)?; + let tmp_result = replace_load(engine, &tmp_opts)?; // Arm the cleanup guard immediately after the load so any later // failure (or panic) drops the temp table on unwind. The guard @@ -616,12 +617,12 @@ fn types_compatible(a: &str, b: &str) -> bool { /// [`Engine::create_table`], the per-row `INSERT` statements, or /// transaction commit/rollback failures. pub fn ingest_json( - engine: &Engine, + engine: &mut Engine, json_str: &str, opts: &IngestOptions, ) -> Result { if opts.mode == "merge" { - return merge_via_temp_table(engine, opts, |tmp_opts| { + return merge_via_temp_table(engine, opts, |engine, tmp_opts| { ingest_json(engine, json_str, tmp_opts) }); } @@ -649,8 +650,8 @@ pub fn ingest_json( // zero side effects. let is_replace = opts.mode != "append"; let qualified = qualified_table(opts); - let row_count = engine.execute_in_transaction(|engine| { - engine.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; + let row_count = engine.execute_in_transaction(|txn| { + txn.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; let mut row_count = 0u64; let col_names: Vec = columns.iter().map(|c| format!("\"{}\"", c.name)).collect(); for obj in &array { @@ -671,7 +672,7 @@ pub fn ingest_json( col_names.join(", "), values.join(", ") ); - engine.execute_command(&sql)?; + txn.execute_command(&sql)?; row_count += 1; } Ok(row_count) @@ -718,12 +719,12 @@ pub fn ingest_json( /// or the `COPY FROM` statement (SQL errors, schema mismatches, /// connection loss). pub fn ingest_csv( - engine: &Engine, + engine: &mut Engine, csv_text: &str, opts: &IngestOptions, ) -> Result { if opts.mode == "merge" { - return merge_via_temp_table(engine, opts, |tmp_opts| { + return merge_via_temp_table(engine, opts, |engine, tmp_opts| { ingest_csv(engine, csv_text, tmp_opts) }); } @@ -786,9 +787,9 @@ pub fn ingest_csv( // Create table + COPY inside one transaction so that a COPY failure also // unwinds the table creation. let is_replace = opts.mode != "append"; - let row_count = engine.execute_in_transaction(|engine| { - engine.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; - engine.execute_command(©_sql) + let row_count = engine.execute_in_transaction(|txn| { + txn.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; + txn.execute_command(©_sql) }); // `temp_path` (TempPath) auto-deletes the file when dropped at end of scope. @@ -835,12 +836,12 @@ pub fn ingest_csv( /// - Propagates any transaction error from [`Engine::create_table`] /// or the `COPY FROM` statement. pub fn ingest_csv_file( - engine: &Engine, + engine: &mut Engine, path: &str, opts: &IngestOptions, ) -> Result { if opts.mode == "merge" { - return merge_via_temp_table(engine, opts, |tmp_opts| { + return merge_via_temp_table(engine, opts, |engine, tmp_opts| { ingest_csv_file(engine, path, tmp_opts) }); } @@ -880,9 +881,9 @@ pub fn ingest_csv_file( ); let is_replace = opts.mode != "append"; - let row_count = engine.execute_in_transaction(|engine| { - engine.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; - engine.execute_command(©_sql) + let row_count = engine.execute_in_transaction(|txn| { + txn.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; + txn.execute_command(©_sql) })?; let elapsed = timer.elapsed_ms(); @@ -1039,7 +1040,7 @@ pub async fn ingest_csv_file_async( /// JSON / JSONL) and from [`ingest_json`] (schema inference, /// transaction failures, etc.). pub fn ingest_json_file( - engine: &Engine, + engine: &mut Engine, path: &str, opts: &IngestOptions, ) -> Result { diff --git a/hyperdb-mcp/src/ingest_arrow.rs b/hyperdb-mcp/src/ingest_arrow.rs index 844d6acd..65f21aed 100644 --- a/hyperdb-mcp/src/ingest_arrow.rs +++ b/hyperdb-mcp/src/ingest_arrow.rs @@ -351,12 +351,12 @@ async fn count_rows_async(conn: &AsyncConnection, table: &str) -> Result Result { if opts.mode == "merge" { - return crate::ingest::merge_via_temp_table(engine, opts, |tmp_opts| { + return crate::ingest::merge_via_temp_table(engine, opts, |engine, tmp_opts| { ingest_parquet_file(engine, path, tmp_opts) }); } @@ -383,12 +383,12 @@ pub fn ingest_parquet_file( // (Hyper treats all DDL that way), so wrapping it in `execute_in_transaction` // no longer buys us rollback — but it still gives us a clean error // path that runs the transaction prelude + drops if needed. - let affected = engine.execute_in_transaction(|engine| { + let affected = engine.execute_in_transaction(|txn| { if is_replace { let qualified = crate::ingest::qualified_table(opts); - engine.execute_command(&format!("DROP TABLE IF EXISTS {qualified}"))?; + txn.execute_command(&format!("DROP TABLE IF EXISTS {qualified}"))?; } - engine.execute_command(&sql) + txn.execute_command(&sql) })?; // Row count: `CREATE TABLE AS` reports 0 affected, so for replace mode @@ -653,12 +653,12 @@ fn read_arrow_ipc_file(path: &str) -> Result<(Vec, Vec Result { if opts.mode == "merge" { - return crate::ingest::merge_via_temp_table(engine, opts, |tmp_opts| { + return crate::ingest::merge_via_temp_table(engine, opts, |engine, tmp_opts| { ingest_arrow_ipc_file(engine, path, tmp_opts) }); } @@ -679,25 +679,25 @@ pub fn ingest_arrow_ipc_file( let is_replace = opts.mode != "append"; // Arrow IPC uses the binary COPY protocol which resolves table names via // the search path. When targeting a non-primary database, temporarily - // redirect the search path for the duration of the transaction. - let _search_guard = if let Some(ref db) = opts.target_db { - Some(engine.scoped_search_path(db)?) - } else { - None - }; - let row_count = engine.execute_in_transaction(|engine| { - engine.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; - - // Stream RecordBatches through the binary COPY protocol. Each - // batch is written to an IPC Stream segment internally — no - // text encoding, no per-row SQL. - let mut inserter = - hyperdb_api::ArrowInserter::from_table(engine.connection(), opts.table.as_str()) + // redirect the search path for the duration of the transaction. The + // closure form rather than the `ScopedSearchPath` guard because the + // transaction below needs `&mut Engine`, which the guard's immutable + // borrow would block. + let row_count = engine.with_search_path(opts.target_db.as_deref(), |engine| { + engine.execute_in_transaction(|txn| { + txn.create_table_in(&opts.table, &columns, is_replace, opts.target_db.as_deref())?; + + // Stream RecordBatches through the binary COPY protocol. Each + // batch is written to an IPC Stream segment internally — no + // text encoding, no per-row SQL. + let mut inserter = + hyperdb_api::ArrowInserter::from_table(txn.connection(), opts.table.as_str()) + .map_err(McpError::from)?; + inserter + .insert_batches(batches.iter()) .map_err(McpError::from)?; - inserter - .insert_batches(batches.iter()) - .map_err(McpError::from)?; - inserter.execute().map_err(McpError::from) + inserter.execute().map_err(McpError::from) + }) })?; let elapsed = timer.elapsed_ms(); diff --git a/hyperdb-mcp/src/lakehouse.rs b/hyperdb-mcp/src/lakehouse.rs index cabddc81..72a35957 100644 --- a/hyperdb-mcp/src/lakehouse.rs +++ b/hyperdb-mcp/src/lakehouse.rs @@ -179,7 +179,7 @@ fn count_rows(engine: &Engine, table: &str) -> Result { /// - Returns [`ErrorCode::InternalError`] if the post-ingest /// `COUNT(*)` cannot be read back (bubbled from `count_rows`). pub fn ingest_iceberg_table( - engine: &Engine, + engine: &mut Engine, path: &str, opts: &IcebergIngestOptions, ) -> Result { @@ -193,12 +193,12 @@ pub fn ingest_iceberg_table( // the transaction to avoid the post-CTAS wire-state quirk that // truncates the returned count — see `ingest_parquet_file` for the // long version. - let affected = engine.execute_in_transaction(|engine| { + let affected = engine.execute_in_transaction(|txn| { if is_replace { let quoted_table = format!("\"{}\"", opts.table.replace('"', "\"\"")); - engine.execute_command(&format!("DROP TABLE IF EXISTS {quoted_table}"))?; + txn.execute_command(&format!("DROP TABLE IF EXISTS {quoted_table}"))?; } - engine.execute_command(&sql) + txn.execute_command(&sql) })?; let row_count = if is_replace { diff --git a/hyperdb-mcp/src/lib.rs b/hyperdb-mcp/src/lib.rs index 56958a59..a8f3ebfa 100644 --- a/hyperdb-mcp/src/lib.rs +++ b/hyperdb-mcp/src/lib.rs @@ -3,7 +3,7 @@ #![allow( missing_docs, - reason = "MCP server binary crate; not published to crates.io. Tool-level docs are surfaced via the MCP protocol, not rustdoc." + reason = "Primarily an MCP server binary; the library target exists to support it and is not a documented API surface. Tool-level docs are surfaced via the MCP protocol, not rustdoc." )] //! MCP (Model Context Protocol) server that exposes the Hyper columnar database diff --git a/hyperdb-mcp/src/server.rs b/hyperdb-mcp/src/server.rs index ce162d25..405747f9 100644 --- a/hyperdb-mcp/src/server.rs +++ b/hyperdb-mcp/src/server.rs @@ -1622,11 +1622,16 @@ impl HyperMcpServer { /// heals itself. fn with_engine(&self, f: F) -> Result where - F: FnOnce(&Engine) -> Result, + F: FnOnce(&mut Engine) -> Result, { let (result, daemon_health_port, connection_lost) = { let mut guard = self.ensure_engine()?; - let engine = guard.as_ref().expect("ensure_engine guarantees Some"); + // `&mut` because transactional paths need + // `Engine::execute_in_transaction`, whose RAII guard borrows the + // connection exclusively. Handing it out costs nothing: the + // engine already lives behind this exclusive `Mutex`, so no + // caller was ever sharing it concurrently. + let engine = guard.as_mut().expect("ensure_engine guarantees Some"); let daemon_health_port = engine.daemon_health_port(); // Bootstrap the catalog exactly once per engine. Intentionally // runs *inside* `with_engine` (not `ensure_engine`) so the @@ -2779,12 +2784,13 @@ impl HyperMcpServer { // require_writable=true ensures non-primary aliases must be writable. // Held for the entire batch (and transaction, if multi-statement). let target_db = self.resolve_db(engine, params.database.as_deref(), None, true)?; - let _search_guard = match target_db { - Some(ref alias) => Some(engine.scoped_search_path(alias)?), - None => None, - }; let total_timer = crate::stats::StatsTimer::start(); + // The closure form of the search-path scope rather than the + // `ScopedSearchPath` guard: the multi-statement branch needs + // `&mut Engine` for the transaction guard, which the guard's + // immutable borrow of the engine would block. let (per_statement, affected_total, operation): (Vec, u64, &'static str) = + engine.with_search_path(target_db.as_deref(), |engine| { if params.sql.len() == 1 { // Singletons skip BEGIN/COMMIT — same auto-commit behavior // as the pre-batch `execute` tool, and DDL singletons stay @@ -2792,7 +2798,7 @@ impl HyperMcpServer { let stmt = ¶ms.sql[0]; let t = crate::stats::StatsTimer::start(); let affected = engine.execute_command(stmt)?; - ( + Ok(( vec![json!({ "sql": Self::fmt_sql(stmt), "affected_rows": affected, @@ -2800,15 +2806,15 @@ impl HyperMcpServer { })], affected, "command", - ) + )) } else { let stmts = ¶ms.sql; - let (results, total) = engine.execute_in_transaction(|engine| { + let (results, total) = engine.execute_in_transaction(|txn| { let mut out = Vec::with_capacity(stmts.len()); let mut total: u64 = 0; for (idx, stmt) in stmts.iter().enumerate() { let t = crate::stats::StatsTimer::start(); - let affected = engine.execute_command(stmt).map_err(|e| { + let affected = txn.execute_command(stmt).map_err(|e| { // Preserve the original error's code AND its // suggestion (e.g. Hyper's "did you mean // ?") — append the rollback context @@ -2844,8 +2850,9 @@ impl HyperMcpServer { } Ok((out, total)) })?; - (results, total, "transaction") - }; + Ok((results, total, "transaction")) + } + })?; let elapsed = total_timer.elapsed_ms(); // Reconcile only when the batch contains a statement that // could have changed the set of tables (CREATE / DROP / @@ -4442,7 +4449,7 @@ impl HyperMcpServer { pub fn resource_body_for_uri(&self, uri: &str) -> Result, McpError> { if uri == "hyper://workspace" { return self - .with_engine(super::engine::Engine::status) + .with_engine(|engine| engine.status()) .map(|v| Some(ResourceBody::Json(v))); } if uri == "hyper://tables" { @@ -4584,7 +4591,7 @@ impl HyperMcpServer { "hyper://readme".to_string(), "hyper://schema/kv".to_string(), ]; - if let Ok(tables) = self.with_engine(super::engine::Engine::describe_tables) { + if let Ok(tables) = self.with_engine(|engine| engine.describe_tables()) { // `describe_tables` already filters out `_hyperdb_*` meta- // tables via `is_internal_table`, so any table we see here // is user-visible. @@ -4614,9 +4621,9 @@ impl HyperMcpServer { /// orient itself in a single resource read without first calling /// `status` and `describe` tools. fn build_readme_body(&self) -> Result { - let status = self.with_engine(super::engine::Engine::status)?; + let status = self.with_engine(|engine| engine.status())?; let tables = self - .with_engine(super::engine::Engine::describe_tables) + .with_engine(|engine| engine.describe_tables()) .unwrap_or_default(); let has_persistent = status @@ -5059,7 +5066,7 @@ Full SQL reference: https://developer.salesforce.com/docs/data/data-cloud-query- .no_annotation(), ]; - if let Ok(tables) = self.with_engine(super::engine::Engine::describe_tables) { + if let Ok(tables) = self.with_engine(|engine| engine.describe_tables()) { // `describe_tables` already excludes `_hyperdb_*` meta- // tables (see `is_internal_table`), so the resource // catalog only surfaces user-visible tables. diff --git a/hyperdb-mcp/tests/export_tests.rs b/hyperdb-mcp/tests/export_tests.rs index b3bfaed5..2a3979e3 100644 --- a/hyperdb-mcp/tests/export_tests.rs +++ b/hyperdb-mcp/tests/export_tests.rs @@ -378,7 +378,7 @@ fn export_overwrite_true_replaces_existing_file() { fn iceberg_export_round_trips_through_load_iceberg() { use hyperdb_mcp::lakehouse::{IcebergIngestOptions, ingest_iceberg_table}; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); setup_test_table(&te); let dir = tempfile::tempdir().unwrap(); @@ -416,7 +416,7 @@ fn iceberg_export_round_trips_through_load_iceberg() { metadata_filename: None, version_as_of: None, }; - let ingest_result = ingest_iceberg_table(&te.engine, iceberg_str, &ingest_opts).unwrap(); + let ingest_result = ingest_iceberg_table(&mut te.engine, iceberg_str, &ingest_opts).unwrap(); assert_eq!(ingest_result.rows, 2); // The reported schema must list all three source columns. The initial @@ -473,7 +473,7 @@ fn iceberg_export_round_trips_through_load_iceberg() { /// its place. #[test] fn iceberg_export_overwrite_replaces_directory() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); setup_test_table(&te); let dir = tempfile::tempdir().unwrap(); @@ -510,7 +510,7 @@ fn iceberg_export_overwrite_replaces_directory() { // 2-row Iceberg table was replaced, not augmented. use hyperdb_mcp::lakehouse::{IcebergIngestOptions, ingest_iceberg_table}; let ingest = ingest_iceberg_table( - &te.engine, + &mut te.engine, iceberg_str, &IcebergIngestOptions { table: "t".into(), @@ -571,7 +571,7 @@ fn parquet_export_round_trips_through_load_file() { use hyperdb_mcp::ingest::IngestOptions; use hyperdb_mcp::ingest_arrow::ingest_parquet_file; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // A table whose columns include every type the old JSON-based // exporter would have degraded: NUMERIC with scale, DATE, plus // plain INT/TEXT/DOUBLE. The round-trip must preserve all of them. @@ -618,7 +618,7 @@ fn parquet_export_round_trips_through_load_file() { // Reload through our own parquet loader. let ingest_result = ingest_parquet_file( - &te.engine, + &mut te.engine, path_str, &IngestOptions { table: "pq_export_reloaded".into(), @@ -688,7 +688,7 @@ fn arrow_ipc_export_round_trips_through_load_file() { use hyperdb_mcp::ingest::IngestOptions; use hyperdb_mcp::ingest_arrow::ingest_arrow_ipc_file; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); setup_test_table(&te); let dir = tempfile::tempdir().unwrap(); @@ -715,7 +715,7 @@ fn arrow_ipc_export_round_trips_through_load_file() { // Stream vs File sub-format, so an export producing Stream bytes // round-trips without extra conversion. let ingest_result = ingest_arrow_ipc_file( - &te.engine, + &mut te.engine, path_str, &IngestOptions { table: "arrow_reloaded".into(), diff --git a/hyperdb-mcp/tests/ingest_arrow_tests.rs b/hyperdb-mcp/tests/ingest_arrow_tests.rs index f6f0fcd8..ac7a58c0 100644 --- a/hyperdb-mcp/tests/ingest_arrow_tests.rs +++ b/hyperdb-mcp/tests/ingest_arrow_tests.rs @@ -105,7 +105,7 @@ fn create_test_arrow_ipc(path: &str) { /// back to verify all rows loaded and NULL values are preserved. #[test] fn ingest_parquet() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test.parquet"); let path_str = path.to_str().unwrap(); @@ -118,7 +118,7 @@ fn ingest_parquet() { merge_key: None, target_db: None, }; - let result = ingest_parquet_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_parquet_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 3); let rows = te @@ -152,7 +152,7 @@ fn ingest_parquet_decimal128_not_null_preserves_values() { use parquet::arrow::ArrowWriter; use std::fs::File; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("decimal.parquet"); let path_str = path.to_str().unwrap(); @@ -185,7 +185,7 @@ fn ingest_parquet_decimal128_not_null_preserves_values() { merge_key: None, target_db: None, }; - let result = ingest_parquet_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_parquet_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 3); // The inferred target schema must preserve precision and scale so the @@ -227,7 +227,7 @@ fn ingest_parquet_reports_accurate_row_count_above_131072() { const ROW_COUNT: usize = 200_000; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("big.parquet"); let path_str = path.to_str().unwrap(); @@ -249,7 +249,7 @@ fn ingest_parquet_reports_accurate_row_count_above_131072() { merge_key: None, target_db: None, }; - let result = ingest_parquet_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_parquet_file(&mut te.engine, path_str, &opts).unwrap(); // The bug would have reported 200000 & 0x1FFFF = 68928. assert_eq!( @@ -270,7 +270,7 @@ fn ingest_parquet_reports_accurate_row_count_above_131072() { /// `INSERT INTO ... SELECT * FROM external(...)` branch of the native path. #[test] fn ingest_parquet_append_adds_to_existing_rows() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("append.parquet"); let path_str = path.to_str().unwrap(); @@ -283,7 +283,7 @@ fn ingest_parquet_append_adds_to_existing_rows() { merge_key: None, target_db: None, }; - let r1 = ingest_parquet_file(&te.engine, path_str, &opts_replace).unwrap(); + let r1 = ingest_parquet_file(&mut te.engine, path_str, &opts_replace).unwrap(); assert_eq!(r1.rows, 3); let opts_append = IngestOptions { @@ -293,7 +293,7 @@ fn ingest_parquet_append_adds_to_existing_rows() { merge_key: None, target_db: None, }; - let r2 = ingest_parquet_file(&te.engine, path_str, &opts_append).unwrap(); + let r2 = ingest_parquet_file(&mut te.engine, path_str, &opts_append).unwrap(); assert_eq!(r2.rows, 3); let rows = te @@ -310,7 +310,7 @@ fn ingest_parquet_append_adds_to_existing_rows() { /// override. #[test] fn ingest_parquet_applies_schema_override() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("override.parquet"); let path_str = path.to_str().unwrap(); @@ -325,7 +325,7 @@ fn ingest_parquet_applies_schema_override() { merge_key: None, target_db: None, }; - let result = ingest_parquet_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_parquet_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 3); let id_col = result @@ -353,7 +353,7 @@ fn ingest_parquet_applies_schema_override() { /// back to verify the row count and that the exact schema was preserved. #[test] fn ingest_arrow_ipc() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test.arrow"); let path_str = path.to_str().unwrap(); @@ -366,7 +366,7 @@ fn ingest_arrow_ipc() { merge_key: None, target_db: None, }; - let result = ingest_arrow_ipc_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = te @@ -391,7 +391,7 @@ fn ingest_arrow_ipc_decimal128_not_null_preserves_values() { use arrow::record_batch::RecordBatch; use std::fs::File; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("decimal.arrow"); let path_str = path.to_str().unwrap(); @@ -421,7 +421,7 @@ fn ingest_arrow_ipc_decimal128_not_null_preserves_values() { merge_key: None, target_db: None, }; - let result = ingest_arrow_ipc_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 3); let amount_col = result @@ -447,7 +447,7 @@ fn ingest_arrow_ipc_decimal128_not_null_preserves_values() { /// `ingest_arrow_ipc_file`. #[test] fn ingest_arrow_ipc_append_adds_to_existing_rows() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("append.arrow"); let path_str = path.to_str().unwrap(); @@ -460,7 +460,7 @@ fn ingest_arrow_ipc_append_adds_to_existing_rows() { merge_key: None, target_db: None, }; - let r1 = ingest_arrow_ipc_file(&te.engine, path_str, &opts_replace).unwrap(); + let r1 = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts_replace).unwrap(); assert_eq!(r1.rows, 2); let opts_append = IngestOptions { @@ -470,7 +470,7 @@ fn ingest_arrow_ipc_append_adds_to_existing_rows() { merge_key: None, target_db: None, }; - let r2 = ingest_arrow_ipc_file(&te.engine, path_str, &opts_append).unwrap(); + let r2 = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts_append).unwrap(); assert_eq!(r2.rows, 2); let rows = te @@ -485,7 +485,7 @@ fn ingest_arrow_ipc_append_adds_to_existing_rows() { /// The embedded Arrow schema is authoritative on this path. #[test] fn ingest_arrow_ipc_rejects_schema_override() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("override.arrow"); let path_str = path.to_str().unwrap(); @@ -500,7 +500,7 @@ fn ingest_arrow_ipc_rejects_schema_override() { merge_key: None, target_db: None, }; - let Err(err) = ingest_arrow_ipc_file(&te.engine, path_str, &opts) else { + let Err(err) = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts) else { panic!("override on IPC should be rejected") }; let msg = err.to_string(); @@ -623,7 +623,7 @@ fn ingest_arrow_ipc_accepts_stream_format() { use arrow::record_batch::RecordBatch; use std::fs::File; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("stream.arrow"); let path_str = path.to_str().unwrap(); @@ -654,7 +654,7 @@ fn ingest_arrow_ipc_accepts_stream_format() { merge_key: None, target_db: None, }; - let result = ingest_arrow_ipc_file(&te.engine, path_str, &opts).unwrap(); + let result = ingest_arrow_ipc_file(&mut te.engine, path_str, &opts).unwrap(); assert_eq!(result.rows, 3); let rows = te @@ -671,7 +671,7 @@ fn ingest_arrow_ipc_accepts_stream_format() { /// 3 rows, merge file has 2 overlapping + 1 new → final 4. #[test] fn ingest_parquet_merge_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); // Initial: id=1,2,3 with names Alice/Bob/(NULL). @@ -684,8 +684,12 @@ fn ingest_parquet_merge_basic() { merge_key: None, target_db: None, }; - let r1 = - ingest_parquet_file(&te.engine, initial_path.to_str().unwrap(), &opts_replace).unwrap(); + let r1 = ingest_parquet_file( + &mut te.engine, + initial_path.to_str().unwrap(), + &opts_replace, + ) + .unwrap(); assert_eq!(r1.rows, 3); // Merge file: id=2 (Bob → "Bob Updated"), id=4 (new "Dave"). @@ -719,7 +723,7 @@ fn ingest_parquet_merge_basic() { merge_key: Some(vec!["id".into()]), target_db: None, }; - ingest_parquet_file(&te.engine, merge_path.to_str().unwrap(), &opts_merge).unwrap(); + ingest_parquet_file(&mut te.engine, merge_path.to_str().unwrap(), &opts_merge).unwrap(); let rows = te .engine @@ -737,7 +741,7 @@ fn ingest_parquet_merge_basic() { /// column types. #[test] fn ingest_arrow_ipc_merge_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); // Initial: x=10,20 / y=1.5,2.5 @@ -750,8 +754,12 @@ fn ingest_arrow_ipc_merge_basic() { merge_key: None, target_db: None, }; - let r1 = - ingest_arrow_ipc_file(&te.engine, initial_path.to_str().unwrap(), &opts_replace).unwrap(); + let r1 = ingest_arrow_ipc_file( + &mut te.engine, + initial_path.to_str().unwrap(), + &opts_replace, + ) + .unwrap(); assert_eq!(r1.rows, 2); // Merge: x=20 (update y=2.5→9.9), x=30 (new) @@ -785,7 +793,7 @@ fn ingest_arrow_ipc_merge_basic() { merge_key: Some(vec!["x".into()]), target_db: None, }; - ingest_arrow_ipc_file(&te.engine, merge_path.to_str().unwrap(), &opts_merge).unwrap(); + ingest_arrow_ipc_file(&mut te.engine, merge_path.to_str().unwrap(), &opts_merge).unwrap(); let rows = te .engine @@ -816,7 +824,7 @@ fn ingest_arrow_ipc_merge_basic() { /// have already run `DROP TABLE IF EXISTS` + `CREATE TABLE AS`. #[test] fn ingest_parquet_null_type_column_fails_before_issuing_sql() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("nulltype.parquet"); let path_str = path.to_str().unwrap(); @@ -829,7 +837,7 @@ fn ingest_parquet_null_type_column_fails_before_issuing_sql() { merge_key: None, target_db: None, }; - let err = ingest_parquet_file(&te.engine, path_str, &opts) + let err = ingest_parquet_file(&mut te.engine, path_str, &opts) .expect_err("a physical NullType column must be rejected"); assert_eq!(err.code, hyperdb_mcp::error::ErrorCode::UnsupportedFormat); @@ -871,7 +879,7 @@ fn ingest_parquet_null_type_column_fails_before_issuing_sql() { /// rather than a plausible-looking SQL statement and a 42804. #[test] fn ingest_parquet_null_type_column_rejected_even_with_schema_override() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("nulltype_override.parquet"); let path_str = path.to_str().unwrap(); @@ -889,7 +897,7 @@ fn ingest_parquet_null_type_column_rejected_even_with_schema_override() { merge_key: None, target_db: None, }; - let err = ingest_parquet_file(&te.engine, path_str, &opts) + let err = ingest_parquet_file(&mut te.engine, path_str, &opts) .expect_err("a schema override must not appear to fix a NullType column"); assert_eq!(err.code, hyperdb_mcp::error::ErrorCode::UnsupportedFormat); } diff --git a/hyperdb-mcp/tests/ingest_tests.rs b/hyperdb-mcp/tests/ingest_tests.rs index 67d1e737..53fff9a4 100644 --- a/hyperdb-mcp/tests/ingest_tests.rs +++ b/hyperdb-mcp/tests/ingest_tests.rs @@ -17,7 +17,7 @@ use tempfile::TempPath; /// inserted with correct column values and ordering. #[test] fn ingest_json_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data = r#"[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]"#; let opts = IngestOptions { table: "users".into(), @@ -26,7 +26,7 @@ fn ingest_json_basic() { merge_key: None, target_db: None, }; - let result = ingest_json(&te.engine, data, &opts).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = te @@ -41,7 +41,7 @@ fn ingest_json_basic() { /// First ingest creates the table with 1 row, second ingest appends 1 more. #[test] fn ingest_json_append_mode() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data1 = r#"[{"id": 1}]"#; let data2 = r#"[{"id": 2}]"#; let opts_replace = IngestOptions { @@ -58,8 +58,8 @@ fn ingest_json_append_mode() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, data1, &opts_replace).unwrap(); - ingest_json(&te.engine, data2, &opts_append).unwrap(); + ingest_json(&mut te.engine, data1, &opts_replace).unwrap(); + ingest_json(&mut te.engine, data2, &opts_append).unwrap(); let count: i64 = te .engine @@ -74,7 +74,7 @@ fn ingest_json_append_mode() { /// correctly loads both data rows. #[test] fn ingest_csv_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let csv_text = "id,name,score\n1,Alice,95.5\n2,Bob,88.0\n"; let opts = IngestOptions { table: "scores".into(), @@ -83,7 +83,7 @@ fn ingest_csv_basic() { merge_key: None, target_db: None, }; - let result = ingest_csv(&te.engine, csv_text, &opts).unwrap(); + let result = ingest_csv(&mut te.engine, csv_text, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = te @@ -98,7 +98,7 @@ fn ingest_csv_basic() { /// declared by the override rather than being inferred as TEXT. #[test] fn ingest_json_with_schema_override() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data = r#"[{"amount": "123.45"}]"#; let mut schema = serde_json::Map::new(); schema.insert( @@ -112,7 +112,7 @@ fn ingest_json_with_schema_override() { merge_key: None, target_db: None, }; - let result = ingest_json(&te.engine, data, &opts).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 1); } @@ -120,7 +120,7 @@ fn ingest_json_with_schema_override() { /// silently creating a table with no columns. #[test] fn ingest_json_empty_returns_error() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data = "[]"; let opts = IngestOptions { table: "empty".into(), @@ -129,7 +129,7 @@ fn ingest_json_empty_returns_error() { merge_key: None, target_db: None, }; - let result = ingest_json(&te.engine, data, &opts); + let result = ingest_json(&mut te.engine, data, &opts); assert!(result.is_err()); } @@ -160,7 +160,7 @@ fn tmp_with_ext(ext: &str, content: &[u8]) -> (String, TempPath) { /// of objects — the format produced by typical REST API snapshots. #[test] fn ingest_json_file_loads_json_array() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let (path, _keep) = tmp_with_ext( "json", b"[{\"id\":1,\"name\":\"Alice\"},{\"id\":2,\"name\":\"Bob\"},{\"id\":3,\"name\":\"Carol\"}]", @@ -172,7 +172,7 @@ fn ingest_json_file_loads_json_array() { merge_key: None, target_db: None, }; - let result = ingest_json_file(&te.engine, &path, &opts).unwrap(); + let result = ingest_json_file(&mut te.engine, &path, &opts).unwrap(); assert_eq!(result.rows, 3); assert_eq!(result.stats.file_format.as_deref(), Some("json")); assert_eq!(result.stats.operation, "load_file"); @@ -190,7 +190,7 @@ fn ingest_json_file_loads_json_array() { /// are tolerated so real-world log files load without preprocessing. #[test] fn ingest_json_file_loads_jsonl() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let jsonl = b"{\"k\":\"start\",\"n\":1}\n\ \n\ {\"k\":\"progress\",\"n\":2}\n\ @@ -203,7 +203,7 @@ fn ingest_json_file_loads_jsonl() { merge_key: None, target_db: None, }; - let result = ingest_json_file(&te.engine, &path, &opts).unwrap(); + let result = ingest_json_file(&mut te.engine, &path, &opts).unwrap(); assert_eq!(result.rows, 3, "blank lines are skipped, data rows count 3"); assert_eq!(result.stats.file_format.as_deref(), Some("jsonl")); @@ -220,7 +220,7 @@ fn ingest_json_file_loads_jsonl() { /// the offending line number, not a cryptic byte offset. #[test] fn ingest_json_file_reports_bad_jsonl_line() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let bad = b"{\"id\":1}\n\ {\"id\":2}\n\ not-json\n"; @@ -232,7 +232,7 @@ fn ingest_json_file_reports_bad_jsonl_line() { merge_key: None, target_db: None, }; - let Err(err) = ingest_json_file(&te.engine, &path, &opts) else { + let Err(err) = ingest_json_file(&mut te.engine, &path, &opts) else { panic!("expected malformed JSONL to error") }; assert_eq!(err.code, hyperdb_mcp::error::ErrorCode::SchemaMismatch); @@ -249,7 +249,7 @@ fn ingest_json_file_reports_bad_jsonl_line() { /// users rely on when filtering with `WHERE col IS NULL`. #[test] fn ingest_csv_empty_cells_become_null() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Rows 1/3 have age set, row 2 leaves age empty. The empty cell // should land as SQL NULL, not an empty-string zero or a parse // error on the numeric column. @@ -261,7 +261,7 @@ fn ingest_csv_empty_cells_become_null() { merge_key: None, target_db: None, }; - let result = ingest_csv(&te.engine, csv_text, &opts).unwrap(); + let result = ingest_csv(&mut te.engine, csv_text, &opts).unwrap(); assert_eq!(result.rows, 3); let nulls: i64 = te @@ -381,7 +381,7 @@ fn detect_file_format_defaults_to_csv_when_unreadable() { /// this whole fix set was designed to remove. #[test] fn ingest_json_file_handles_log_extension_via_content_sniff() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Construct the payload through `detect_file_format` so the test // mirrors the production dispatch path rather than hard-coding a // specific ingest function. @@ -400,7 +400,7 @@ fn ingest_json_file_handles_log_extension_via_content_sniff() { merge_key: None, target_db: None, }; - let result = ingest_json_file(&te.engine, &path, &opts).unwrap(); + let result = ingest_json_file(&mut te.engine, &path, &opts).unwrap(); assert_eq!(result.rows, 2); assert_eq!(result.stats.file_format.as_deref(), Some("jsonl")); } @@ -410,7 +410,7 @@ fn ingest_json_file_handles_log_extension_via_content_sniff() { /// defensive `OR col IS NULL` clause. #[test] fn ingest_csv_file_empty_cells_become_null() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let (path, _keep) = tmp_with_ext("csv", b"code,label\nAAA,first\n,middle\nBBB,\n"); let opts = IngestOptions { table: "lookup".into(), @@ -419,7 +419,7 @@ fn ingest_csv_file_empty_cells_become_null() { merge_key: None, target_db: None, }; - let result = ingest_csv_file(&te.engine, &path, &opts).unwrap(); + let result = ingest_csv_file(&mut te.engine, &path, &opts).unwrap(); assert_eq!(result.rows, 3); let code_nulls: i64 = te @@ -549,7 +549,7 @@ fn extract_json_path_multi_level() { /// End-to-end: extract from Splunk-shaped wrapper and ingest into Hyper. #[test] fn extract_json_path_then_ingest() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let inner = serde_json::json!({ "status": "success", "query_result": { @@ -573,7 +573,7 @@ fn extract_json_path_then_ingest() { merge_key: None, target_db: None, }; - let result = ingest_json(&te.engine, &extracted, &opts).unwrap(); + let result = ingest_json(&mut te.engine, &extracted, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = te @@ -593,7 +593,7 @@ fn extract_json_path_then_ingest() { /// Final shape: 4 rows; updated rows show the new values. #[test] fn ingest_json_merge_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let initial = r#"[ {"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}, @@ -612,7 +612,7 @@ fn ingest_json_merge_basic() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, initial, &opts_replace).unwrap(); + ingest_json(&mut te.engine, initial, &opts_replace).unwrap(); let opts_merge = IngestOptions { table: "users".into(), @@ -621,7 +621,7 @@ fn ingest_json_merge_basic() { merge_key: Some(vec!["id".into()]), target_db: None, }; - let merge_result = ingest_json(&te.engine, updates, &opts_merge).unwrap(); + let merge_result = ingest_json(&mut te.engine, updates, &opts_merge).unwrap(); // No new columns in this merge → schema_changed must remain false so // the server handler skips the resource-list-changed broadcast. assert!( @@ -649,7 +649,7 @@ fn ingest_json_merge_basic() { /// and old rows have `host = NULL`. #[test] fn ingest_json_merge_adds_new_column() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let initial = r#"[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]"#; let with_host = r#"[ {"id": 2, "name": "Bob", "host": "host-2"}, @@ -663,7 +663,7 @@ fn ingest_json_merge_adds_new_column() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, initial, &opts_replace).unwrap(); + ingest_json(&mut te.engine, initial, &opts_replace).unwrap(); let opts_merge = IngestOptions { table: "t".into(), @@ -672,7 +672,7 @@ fn ingest_json_merge_adds_new_column() { merge_key: Some(vec!["id".into()]), target_db: None, }; - let merge_result = ingest_json(&te.engine, with_host, &opts_merge).unwrap(); + let merge_result = ingest_json(&mut te.engine, with_host, &opts_merge).unwrap(); // ALTER TABLE fired → schema_changed must be true so the server // handler issues a resource-list-changed broadcast and clients // re-fetch their schema cache. @@ -704,7 +704,7 @@ fn ingest_json_merge_adds_new_column() { /// table becomes the target and rows are loaded as-if by replace. #[test] fn ingest_json_merge_target_does_not_exist() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data = r#"[{"id": 1, "name": "Alice"}]"#; let opts_merge = IngestOptions { table: "fresh".into(), @@ -713,7 +713,7 @@ fn ingest_json_merge_target_does_not_exist() { merge_key: Some(vec!["id".into()]), target_db: None, }; - let result = ingest_json(&te.engine, data, &opts_merge).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts_merge).unwrap(); assert_eq!(result.rows, 1); // Target was just created from scratch via the rename short-circuit; // by definition this is a "shape changed" event, so notify clients. @@ -735,7 +735,7 @@ fn ingest_json_merge_target_does_not_exist() { /// at the tool boundary). #[test] fn ingest_json_merge_missing_key_param() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let data = r#"[{"id": 1}]"#; let opts = IngestOptions { table: "t".into(), @@ -744,7 +744,7 @@ fn ingest_json_merge_missing_key_param() { merge_key: None, target_db: None, }; - let err = ingest_json(&te.engine, data, &opts).unwrap_err(); + let err = ingest_json(&mut te.engine, data, &opts).unwrap_err(); assert!( err.message.to_lowercase().contains("merge_key"), "error must mention merge_key; got: {}", @@ -758,7 +758,7 @@ fn ingest_json_merge_missing_key_param() { /// untouched. #[test] fn ingest_json_merge_key_not_in_target() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Target has only `id`. Merge attempts to key on `not_a_col`. let opts_replace = IngestOptions { table: "t".into(), @@ -767,7 +767,7 @@ fn ingest_json_merge_key_not_in_target() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, r#"[{"id": 1}]"#, &opts_replace).unwrap(); + ingest_json(&mut te.engine, r#"[{"id": 1}]"#, &opts_replace).unwrap(); let opts_merge = IngestOptions { table: "t".into(), @@ -776,7 +776,12 @@ fn ingest_json_merge_key_not_in_target() { merge_key: Some(vec!["not_a_col".into()]), target_db: None, }; - let err = ingest_json(&te.engine, r#"[{"id": 2, "not_a_col": "x"}]"#, &opts_merge).unwrap_err(); + let err = ingest_json( + &mut te.engine, + r#"[{"id": 2, "not_a_col": "x"}]"#, + &opts_merge, + ) + .unwrap_err(); assert!( err.message.contains("not_a_col"), "error must name the missing column; got: {}", @@ -799,7 +804,7 @@ fn ingest_json_merge_key_not_in_target() { /// practice. Reject with a clear error rather than silently coercing. #[test] fn ingest_json_merge_key_type_mismatch() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Force `id` to BIGINT explicitly. let mut so = serde_json::Map::new(); so.insert("id".into(), serde_json::json!("BIGINT")); @@ -810,7 +815,7 @@ fn ingest_json_merge_key_type_mismatch() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, r#"[{"id": 1, "name": "a"}]"#, &opts_replace).unwrap(); + ingest_json(&mut te.engine, r#"[{"id": 1, "name": "a"}]"#, &opts_replace).unwrap(); // Incoming has id as quoted string, no override → inferred TEXT. let opts_merge = IngestOptions { @@ -820,7 +825,8 @@ fn ingest_json_merge_key_type_mismatch() { merge_key: Some(vec!["id".into()]), target_db: None, }; - let err = ingest_json(&te.engine, r#"[{"id": "1", "name": "a"}]"#, &opts_merge).unwrap_err(); + let err = + ingest_json(&mut te.engine, r#"[{"id": "1", "name": "a"}]"#, &opts_merge).unwrap_err(); assert!( err.message.to_lowercase().contains("type mismatch"), "error must mention type mismatch; got: {}", @@ -832,7 +838,7 @@ fn ingest_json_merge_key_type_mismatch() { /// We still reject because silently coercing risks data loss. #[test] fn ingest_json_merge_existing_column_type_mismatch() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Target has score as DOUBLE PRECISION. let mut so = serde_json::Map::new(); so.insert("score".into(), serde_json::json!("DOUBLE PRECISION")); @@ -843,7 +849,12 @@ fn ingest_json_merge_existing_column_type_mismatch() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, r#"[{"id": 1, "score": 99.5}]"#, &opts_replace).unwrap(); + ingest_json( + &mut te.engine, + r#"[{"id": 1, "score": 99.5}]"#, + &opts_replace, + ) + .unwrap(); // Incoming has score as quoted text. let opts_merge = IngestOptions { @@ -854,7 +865,7 @@ fn ingest_json_merge_existing_column_type_mismatch() { target_db: None, }; let err = ingest_json( - &te.engine, + &mut te.engine, r#"[{"id": 1, "score": "not a number"}]"#, &opts_merge, ) @@ -871,7 +882,7 @@ fn ingest_json_merge_existing_column_type_mismatch() { /// describe/list experience over time. #[test] fn ingest_json_merge_no_orphan_tmp_on_failure() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Force a key-not-in-target failure (cheap, deterministic). let opts_replace = IngestOptions { table: "t".into(), @@ -880,7 +891,7 @@ fn ingest_json_merge_no_orphan_tmp_on_failure() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, r#"[{"id": 1}]"#, &opts_replace).unwrap(); + ingest_json(&mut te.engine, r#"[{"id": 1}]"#, &opts_replace).unwrap(); let opts_merge = IngestOptions { table: "t".into(), @@ -889,7 +900,11 @@ fn ingest_json_merge_no_orphan_tmp_on_failure() { merge_key: Some(vec!["bogus_key".into()]), target_db: None, }; - let _ = ingest_json(&te.engine, r#"[{"id": 2, "bogus_key": "x"}]"#, &opts_merge); + let _ = ingest_json( + &mut te.engine, + r#"[{"id": 2, "bogus_key": "x"}]"#, + &opts_merge, + ); // Look for any leftover `__hyperdb_merge_*` table. let table_rows = te @@ -923,7 +938,7 @@ fn ingest_json_merge_no_orphan_tmp_on_failure() { /// non-matching key tuples insert as new rows. #[test] fn ingest_json_merge_multi_key() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Initial: 4 rows keyed by (region, year). let initial = r#"[ {"region": "us", "year": 2025, "amount": 100}, @@ -938,7 +953,7 @@ fn ingest_json_merge_multi_key() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, initial, &opts_replace).unwrap(); + ingest_json(&mut te.engine, initial, &opts_replace).unwrap(); // Merge: us/2026 updates (amount→999); eu/2027 is new; us/2025 stays untouched. let updates = r#"[ @@ -952,7 +967,7 @@ fn ingest_json_merge_multi_key() { merge_key: Some(vec!["region".into(), "year".into()]), target_db: None, }; - ingest_json(&te.engine, updates, &opts_merge).unwrap(); + ingest_json(&mut te.engine, updates, &opts_merge).unwrap(); let rows = te .engine @@ -984,7 +999,7 @@ fn ingest_json_merge_multi_key() { /// the alias behavior so future refactors don't regress it. #[test] fn ingest_json_merge_type_canonicalization_does_not_false_reject() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Force the target's `id` column to be created from the `"INT"` user // string. After `CREATE TABLE`, Hyper canonicalizes to `"INTEGER"` @@ -998,7 +1013,7 @@ fn ingest_json_merge_type_canonicalization_does_not_false_reject() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, r#"[{"id": 1, "name": "a"}]"#, &opts_replace).unwrap(); + ingest_json(&mut te.engine, r#"[{"id": 1, "name": "a"}]"#, &opts_replace).unwrap(); // Sanity: confirm the catalog canonicalized to INTEGER. (If this // assertion ever fails, Hyper's behavior changed, and the test @@ -1025,7 +1040,7 @@ fn ingest_json_merge_type_canonicalization_does_not_false_reject() { target_db: None, }; let result = ingest_json( - &te.engine, + &mut te.engine, r#"[{"id": 1, "name": "updated"}, {"id": 2, "name": "new"}]"#, &opts_merge, ); @@ -1054,7 +1069,7 @@ fn ingest_json_merge_type_canonicalization_does_not_false_reject() { /// overlapping (update) + 1 new (insert) → final 4. #[test] fn ingest_csv_merge_basic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let initial = "id,name\n1,Alice\n2,Bob\n3,Carol\n"; let updates = "id,name\n2,Bob Jr.\n3,Carol Updated\n4,Dave\n"; @@ -1065,7 +1080,7 @@ fn ingest_csv_merge_basic() { merge_key: None, target_db: None, }; - ingest_csv(&te.engine, initial, &opts_replace).unwrap(); + ingest_csv(&mut te.engine, initial, &opts_replace).unwrap(); let opts_merge = IngestOptions { table: "users_csv".into(), @@ -1074,7 +1089,7 @@ fn ingest_csv_merge_basic() { merge_key: Some(vec!["id".into()]), target_db: None, }; - let merge_result = ingest_csv(&te.engine, updates, &opts_merge).unwrap(); + let merge_result = ingest_csv(&mut te.engine, updates, &opts_merge).unwrap(); assert!( !merge_result.stats.schema_changed, "row-only merge must leave schema_changed false" diff --git a/hyperdb-mcp/tests/integration_tests.rs b/hyperdb-mcp/tests/integration_tests.rs index defab9cd..2d0ed08e 100644 --- a/hyperdb-mcp/tests/integration_tests.rs +++ b/hyperdb-mcp/tests/integration_tests.rs @@ -14,7 +14,7 @@ use hyperdb_mcp::ingest::{IngestOptions, ingest_csv, ingest_json}; /// GROUP BY aggregation. Verifies multi-table workspace queries work end-to-end. #[test] fn full_pipeline_json_to_query() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let orders = r#"[ {"order_id": 1, "customer_id": 1, "amount": 100.50}, @@ -28,7 +28,7 @@ fn full_pipeline_json_to_query() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, orders, &opts).unwrap(); + ingest_json(&mut te.engine, orders, &opts).unwrap(); let customers = r#"[ {"customer_id": 1, "name": "Alice"}, @@ -41,7 +41,7 @@ fn full_pipeline_json_to_query() { merge_key: None, target_db: None, }; - ingest_json(&te.engine, customers, &opts).unwrap(); + ingest_json(&mut te.engine, customers, &opts).unwrap(); let rows = te.engine.execute_query_to_json( "SELECT c.name, SUM(o.amount) as total FROM orders o JOIN customers c ON o.customer_id = c.customer_id GROUP BY c.name ORDER BY total DESC" @@ -55,7 +55,7 @@ fn full_pipeline_json_to_query() { /// pipeline that the `query_file` MCP tool relies on. #[test] fn full_pipeline_csv_ingest_and_export() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let csv_data = "product,quantity,price\nWidget,100,9.99\nGadget,50,19.99\n"; let opts = IngestOptions { @@ -65,7 +65,7 @@ fn full_pipeline_csv_ingest_and_export() { merge_key: None, target_db: None, }; - ingest_csv(&te.engine, csv_data, &opts).unwrap(); + ingest_csv(&mut te.engine, csv_data, &opts).unwrap(); let dir = tempfile::tempdir().unwrap(); let export_path = dir.path().join("export.csv"); @@ -135,7 +135,7 @@ fn status_reports_workspace_info() { /// final count should be 3. #[test] fn append_mode_accumulates_data() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let batch1 = r#"[{"v": 1}, {"v": 2}]"#; let batch2 = r#"[{"v": 3}]"#; @@ -155,8 +155,8 @@ fn append_mode_accumulates_data() { target_db: None, }; - ingest_json(&te.engine, batch1, &opts_replace).unwrap(); - ingest_json(&te.engine, batch2, &opts_append).unwrap(); + ingest_json(&mut te.engine, batch1, &opts_replace).unwrap(); + ingest_json(&mut te.engine, batch2, &opts_append).unwrap(); let rows = te .engine diff --git a/hyperdb-mcp/tests/lakehouse_tests.rs b/hyperdb-mcp/tests/lakehouse_tests.rs index c273fe9e..be62113a 100644 --- a/hyperdb-mcp/tests/lakehouse_tests.rs +++ b/hyperdb-mcp/tests/lakehouse_tests.rs @@ -22,14 +22,14 @@ use tempfile::TempDir; /// sending an unchecked path string into `external(...)`. #[test] fn ingest_iceberg_missing_path_errors_cleanly() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IcebergIngestOptions { table: "t".into(), mode: "replace".into(), metadata_filename: None, version_as_of: None, }; - let Err(err) = ingest_iceberg_table(&te.engine, "/nonexistent/iceberg/path", &opts) else { + let Err(err) = ingest_iceberg_table(&mut te.engine, "/nonexistent/iceberg/path", &opts) else { panic!("missing path should fail") }; let msg = err.to_string(); @@ -44,7 +44,7 @@ fn ingest_iceberg_missing_path_errors_cleanly() { /// missing `metadata/`. #[test] fn ingest_iceberg_file_instead_of_directory_errors_cleanly() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = TempDir::new().unwrap(); let file_path = dir.path().join("not-a-dir.txt"); std::fs::write(&file_path, b"hello").unwrap(); @@ -55,7 +55,7 @@ fn ingest_iceberg_file_instead_of_directory_errors_cleanly() { metadata_filename: None, version_as_of: None, }; - let Err(err) = ingest_iceberg_table(&te.engine, file_path.to_str().unwrap(), &opts) else { + let Err(err) = ingest_iceberg_table(&mut te.engine, file_path.to_str().unwrap(), &opts) else { panic!("file path should fail") }; let msg = err.to_string(); @@ -71,7 +71,7 @@ fn ingest_iceberg_file_instead_of_directory_errors_cleanly() { /// exercises the full tx-wrapped execute path end-to-end. #[test] fn ingest_iceberg_empty_directory_surfaces_hyperd_error() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let dir = TempDir::new().unwrap(); let opts = IcebergIngestOptions { @@ -80,7 +80,7 @@ fn ingest_iceberg_empty_directory_surfaces_hyperd_error() { metadata_filename: None, version_as_of: None, }; - let Err(err) = ingest_iceberg_table(&te.engine, dir.path().to_str().unwrap(), &opts) else { + let Err(err) = ingest_iceberg_table(&mut te.engine, dir.path().to_str().unwrap(), &opts) else { panic!("empty directory should fail") }; // We don't pin the exact wording — that's hyperd's to define. Just @@ -111,14 +111,14 @@ fn ingest_iceberg_empty_directory_surfaces_hyperd_error() { /// non-existent table, so we just check the outer error is path-related. #[test] fn ingest_iceberg_unknown_mode_treated_as_replace() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IcebergIngestOptions { table: "t".into(), mode: "blahblah".into(), metadata_filename: None, version_as_of: None, }; - let Err(err) = ingest_iceberg_table(&te.engine, "/nonexistent/x", &opts) else { + let Err(err) = ingest_iceberg_table(&mut te.engine, "/nonexistent/x", &opts) else { panic!("should fail on path") }; assert!(err.to_string().to_lowercase().contains("not exist")); diff --git a/hyperdb-mcp/tests/per_tool_database_tests.rs b/hyperdb-mcp/tests/per_tool_database_tests.rs index bd9de890..dd13bb4b 100644 --- a/hyperdb-mcp/tests/per_tool_database_tests.rs +++ b/hyperdb-mcp/tests/per_tool_database_tests.rs @@ -117,7 +117,7 @@ fn resolve_target_db_persistent_errors_in_ephemeral_only() { /// into the persistent attachment, not the primary. #[test] fn ingest_json_with_persistent_target_lands_in_persistent() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IngestOptions { table: "persisted_data".into(), mode: "replace".into(), @@ -126,7 +126,7 @@ fn ingest_json_with_persistent_target_lands_in_persistent() { target_db: Some("persistent".into()), }; let data = r#"[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]"#; - let result = ingest_json(&te.engine, data, &opts).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 2); // Visible via fully-qualified SQL pointing at persistent. @@ -153,7 +153,7 @@ fn ingest_json_with_persistent_target_lands_in_persistent() { /// FROM target table into the persistent attachment. #[test] fn ingest_csv_with_persistent_target_lands_in_persistent() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IngestOptions { table: "csv_target".into(), mode: "replace".into(), @@ -162,7 +162,7 @@ fn ingest_csv_with_persistent_target_lands_in_persistent() { target_db: Some("persistent".into()), }; let data = "id,name\n1,Alice\n2,Bob\n"; - let result = ingest_csv(&te.engine, data, &opts).unwrap(); + let result = ingest_csv(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = te @@ -181,7 +181,7 @@ fn ingest_to_persistent_survives_engine_recreate() { let path_str = path.to_str().unwrap().to_string(); { - let engine = Engine::new_no_daemon(Some(path_str.clone())).unwrap(); + let mut engine = Engine::new_no_daemon(Some(path_str.clone())).unwrap(); let opts = IngestOptions { table: "library".into(), mode: "replace".into(), @@ -190,7 +190,7 @@ fn ingest_to_persistent_survives_engine_recreate() { target_db: Some("persistent".into()), }; let data = r#"[{"id": 1, "title": "Dune"}]"#; - ingest_json(&engine, data, &opts).unwrap(); + ingest_json(&mut engine, data, &opts).unwrap(); } // Reopen and verify the table is still there. @@ -206,7 +206,7 @@ fn ingest_to_persistent_survives_engine_recreate() { /// (ephemeral) database — backward-compat invariant. #[test] fn ingest_with_no_target_db_lands_in_primary() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IngestOptions { table: "scratch".into(), mode: "replace".into(), @@ -215,7 +215,7 @@ fn ingest_with_no_target_db_lands_in_primary() { target_db: None, }; let data = r#"[{"x": 1}]"#; - ingest_json(&te.engine, data, &opts).unwrap(); + ingest_json(&mut te.engine, data, &opts).unwrap(); // Visible as unqualified table in the primary. let rows = te @@ -295,7 +295,7 @@ fn describe_table_in_unknown_returns_table_not_found() { /// persistent table. #[test] fn sample_table_in_persistent_returns_rows() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IngestOptions { table: "samples".into(), mode: "replace".into(), @@ -303,7 +303,12 @@ fn sample_table_in_persistent_returns_rows() { merge_key: None, target_db: Some("persistent".into()), }; - ingest_json(&te.engine, r#"[{"id": 1}, {"id": 2}, {"id": 3}]"#, &opts).unwrap(); + ingest_json( + &mut te.engine, + r#"[{"id": 1}, {"id": 2}, {"id": 3}]"#, + &opts, + ) + .unwrap(); let sample = te .engine @@ -363,6 +368,110 @@ fn scoped_search_path_redirects_and_restores() { assert_eq!(rows.len(), 2, "search path restored to primary (2 rows)"); } +/// Builds the same two-database fixture `scoped_search_path_redirects_and_restores` +/// uses: `persistent.public.t` holds one row, primary `t` holds two. +fn two_db_fixture() -> TestEngine { + let te = TestEngine::new_ephemeral(); + te.engine + .execute_command("CREATE TABLE \"persistent\".\"public\".\"t\" (x INT)") + .unwrap(); + te.engine + .execute_command("INSERT INTO \"persistent\".\"public\".\"t\" VALUES (42)") + .unwrap(); + te.engine.execute_command("CREATE TABLE t (x INT)").unwrap(); + te.engine + .execute_command("INSERT INTO t VALUES (1), (2)") + .unwrap(); + te +} + +fn unqualified_t_row_count(te: &TestEngine) -> usize { + te.engine + .execute_query_to_json("SELECT * FROM t ORDER BY x") + .unwrap() + .len() +} + +/// `with_search_path` is the closure form used by the transactional paths, +/// where `ScopedSearchPath`'s immutable borrow of the engine would block +/// the transaction guard. It must redirect and restore exactly like the +/// guard does — including when the closure fails or panics, which is the +/// part a plain "set, call, restore" sequence would get wrong. +#[test] +fn with_search_path_redirects_and_restores_on_success() { + let mut te = two_db_fixture(); + + let seen = te + .engine + .with_search_path(Some("persistent"), |engine| { + Ok(engine + .execute_query_to_json("SELECT * FROM t ORDER BY x") + .unwrap() + .len()) + }) + .unwrap(); + + assert_eq!(seen, 1, "inside the scope, `t` resolves to persistent"); + assert_eq!( + unqualified_t_row_count(&te), + 2, + "search path restored to primary" + ); +} + +#[test] +fn with_search_path_restores_after_closure_error() { + use hyperdb_mcp::error::{ErrorCode, McpError}; + let mut te = two_db_fixture(); + + let result: Result<(), McpError> = te.engine.with_search_path(Some("persistent"), |_| { + Err(McpError::new(ErrorCode::InternalError, "simulated failure")) + }); + assert!(result.is_err()); + + assert_eq!( + unqualified_t_row_count(&te), + 2, + "search path restored even though the closure failed" + ); +} + +#[test] +fn with_search_path_restores_after_closure_panic() { + let mut te = two_db_fixture(); + + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + te.engine + .with_search_path::<_, ()>(Some("persistent"), |_| panic!("simulated closure bug")) + })); + assert!(outcome.is_err(), "panic should propagate out"); + + assert_eq!( + unqualified_t_row_count(&te), + 2, + "search path restored while unwinding from the panic" + ); +} + +/// `None` is the no-routing case every unqualified tool call takes: the +/// closure runs with the primary still selected and no `SET` is issued. +#[test] +fn with_search_path_none_leaves_primary_selected() { + let mut te = two_db_fixture(); + + let seen = te + .engine + .with_search_path(None, |engine| { + Ok(engine + .execute_query_to_json("SELECT * FROM t ORDER BY x") + .unwrap() + .len()) + }) + .unwrap(); + + assert_eq!(seen, 2, "no alias means the primary stays selected"); +} + // --- Case-insensitive PERSISTENT_ALIAS matching ---------------------------- /// `"Persistent"`, `"PERSISTENT"`, `"persistent"` all resolve to the @@ -402,7 +511,7 @@ fn resolve_target_db_persistent_uppercase_errors_in_ephemeral_only() { /// works against a non-primary database. #[test] fn merge_into_persistent_creates_table_when_missing() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let opts = IngestOptions { table: "merged_persist".into(), mode: "merge".into(), @@ -411,7 +520,7 @@ fn merge_into_persistent_creates_table_when_missing() { target_db: Some("persistent".into()), }; let data = r#"[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]"#; - let result = ingest_json(&te.engine, data, &opts).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 2); assert!( result.stats.schema_changed, @@ -432,7 +541,7 @@ fn merge_into_persistent_creates_table_when_missing() { /// matching rows are replaced, unmatched rows are appended. #[test] fn merge_into_persistent_replaces_matching_and_appends_new() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Seed target with two rows. let seed_opts = IngestOptions { table: "merge_target".into(), @@ -442,7 +551,7 @@ fn merge_into_persistent_replaces_matching_and_appends_new() { target_db: Some("persistent".into()), }; ingest_json( - &te.engine, + &mut te.engine, r#"[{"id": 1, "name": "old1"}, {"id": 2, "name": "old2"}]"#, &seed_opts, ) @@ -457,7 +566,7 @@ fn merge_into_persistent_replaces_matching_and_appends_new() { target_db: Some("persistent".into()), }; let result = ingest_json( - &te.engine, + &mut te.engine, r#"[{"id": 1, "name": "new1"}, {"id": 3, "name": "new3"}]"#, &merge_opts, ) @@ -481,7 +590,7 @@ fn merge_into_persistent_replaces_matching_and_appends_new() { /// shows up in the post-merge schema with NULL for pre-existing rows. #[test] fn merge_into_persistent_alters_when_incoming_has_new_column() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); let seed_opts = IngestOptions { table: "widens".into(), mode: "replace".into(), @@ -489,7 +598,12 @@ fn merge_into_persistent_alters_when_incoming_has_new_column() { merge_key: None, target_db: Some("persistent".into()), }; - ingest_json(&te.engine, r#"[{"id": 1, "name": "Alice"}]"#, &seed_opts).unwrap(); + ingest_json( + &mut te.engine, + r#"[{"id": 1, "name": "Alice"}]"#, + &seed_opts, + ) + .unwrap(); let merge_opts = IngestOptions { table: "widens".into(), @@ -499,7 +613,7 @@ fn merge_into_persistent_alters_when_incoming_has_new_column() { target_db: Some("persistent".into()), }; let result = ingest_json( - &te.engine, + &mut te.engine, r#"[{"id": 2, "name": "Bob", "email": "bob@x.com"}]"#, &merge_opts, ) diff --git a/hyperdb-mcp/tests/transaction_tests.rs b/hyperdb-mcp/tests/transaction_tests.rs index 32bfe726..c4cd1904 100644 --- a/hyperdb-mcp/tests/transaction_tests.rs +++ b/hyperdb-mcp/tests/transaction_tests.rs @@ -30,14 +30,14 @@ fn query_resilient(engine: &Engine, sql: &str) -> Vec { #[test] fn execute_in_transaction_rolls_back_on_error() { use hyperdb_mcp::error::{ErrorCode, McpError}; - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE direct (v INT)") .unwrap(); - let result: Result<(), McpError> = te.engine.execute_in_transaction(|engine| { - engine.execute_command("INSERT INTO direct VALUES (1)")?; - engine.execute_command("INSERT INTO direct VALUES (2)")?; + let result: Result<(), McpError> = te.engine.execute_in_transaction(|txn| { + txn.execute_command("INSERT INTO direct VALUES (1)")?; + txn.execute_command("INSERT INTO direct VALUES (2)")?; Err(McpError::new(ErrorCode::InternalError, "simulated failure")) }); assert!(result.is_err()); @@ -53,15 +53,15 @@ fn execute_in_transaction_rolls_back_on_error() { /// Sanity check: successful commits stick. #[test] fn execute_in_transaction_commits_on_success() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE direct (v INT)") .unwrap(); te.engine - .execute_in_transaction(|engine| { - engine.execute_command("INSERT INTO direct VALUES (10)")?; - engine.execute_command("INSERT INTO direct VALUES (20)")?; + .execute_in_transaction(|txn| { + txn.execute_command("INSERT INTO direct VALUES (10)")?; + txn.execute_command("INSERT INTO direct VALUES (20)")?; Ok(()) }) .unwrap(); @@ -74,14 +74,102 @@ fn execute_in_transaction_commits_on_success() { assert_eq!(count, 2); } +/// The regression test for the RAII migration: an `Ok` closure must +/// **commit**, not silently roll back when the guard drops. +/// +/// `execute_in_transaction_commits_on_success` reads the rows back on the +/// same session that wrote them, which an uncommitted-but-still-open +/// transaction would also satisfy. This test closes that hole by forcing +/// the write to be durable past the end of its own transaction: a second, +/// independent transaction rolls back, and the first batch must survive. +/// Swap the `txn.commit()?` in `execute_in_transaction` for a bare drop and +/// this fails while the simpler test still passes. +#[test] +fn execute_in_transaction_commit_outlives_its_transaction() { + use hyperdb_mcp::error::{ErrorCode, McpError}; + let mut te = TestEngine::new_ephemeral(); + te.engine + .execute_command("CREATE TABLE direct (v INT)") + .unwrap(); + + te.engine + .execute_in_transaction(|txn| txn.execute_command("INSERT INTO direct VALUES (10)")) + .unwrap(); + + // A second transaction that aborts. If the first one had never + // committed, its row would be discarded here along with this one. + let result: Result<(), McpError> = te.engine.execute_in_transaction(|txn| { + txn.execute_command("INSERT INTO direct VALUES (20)")?; + Err(McpError::new(ErrorCode::InternalError, "simulated failure")) + }); + assert!(result.is_err()); + + let rows = query_resilient(&te.engine, "SELECT v FROM direct ORDER BY v"); + assert_eq!(rows.len(), 1, "committed row must survive a later rollback"); + assert_eq!(rows[0]["v"].as_i64().unwrap(), 10); +} + +/// Every exit path must leave the session with no transaction open. +/// +/// The guard's whole job is discharging the `BEGIN`/`COMMIT`-or-`ROLLBACK` +/// pairing obligation. If any path leaked one, the *next* `BEGIN` would +/// fail with "transaction already in progress" on a connection that is +/// otherwise healthy — and the server's `ConnectionLost` auto-reconnect +/// would not rescue it, because the connection is live. This walks +/// commit → error → panic → commit on one engine and asserts each +/// subsequent transaction still opens. +#[test] +fn execute_in_transaction_never_leaks_an_open_transaction() { + use hyperdb_mcp::error::{ErrorCode, McpError}; + let mut te = TestEngine::new_ephemeral(); + te.engine + .execute_command("CREATE TABLE direct (v INT)") + .unwrap(); + + // 1. Commit path. + te.engine + .execute_in_transaction(|txn| txn.execute_command("INSERT INTO direct VALUES (1)")) + .expect("commit path"); + + // 2. Error path — must not wedge the session for the next BEGIN. + let err: Result<(), McpError> = te.engine.execute_in_transaction(|txn| { + txn.execute_command("INSERT INTO direct VALUES (2)")?; + Err(McpError::new(ErrorCode::InternalError, "simulated failure")) + }); + assert!(err.is_err()); + + // 3. Panic path — rollback happens in `Drop` as the unwind passes. + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + te.engine.execute_in_transaction::<_, ()>(|txn| { + txn.execute_command("INSERT INTO direct VALUES (3)")?; + panic!("simulated closure bug"); + }) + })); + assert!(outcome.is_err(), "panic should propagate out"); + + // 4. If any of the three leaked a `BEGIN`, this one fails. + te.engine + .execute_in_transaction(|txn| txn.execute_command("INSERT INTO direct VALUES (4)")) + .expect("session must still accept BEGIN after commit/error/panic"); + + let rows = query_resilient(&te.engine, "SELECT v FROM direct ORDER BY v"); + let values: Vec = rows.iter().map(|r| r["v"].as_i64().unwrap()).collect(); + assert_eq!( + values, + vec![1, 4], + "only the two committed rows survive; the error and panic paths rolled back" + ); +} + /// A panic inside the transaction closure (e.g. an unwrap on None, array /// indexing OOB, arithmetic overflow) must not leave an open transaction -/// on the connection. Without the `catch_unwind` guard in -/// `execute_in_transaction`, the next operation would hit "transaction -/// already in progress" and the engine would be wedged until restart. +/// on the connection. The RAII guard rolls back from its `Drop` as the +/// unwind passes through `execute_in_transaction`; without that, the next +/// operation would hit "transaction already in progress" and the engine +/// would be wedged until restart. #[test] fn execute_in_transaction_rolls_back_on_panic() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE direct (v INT)") .unwrap(); @@ -90,8 +178,8 @@ fn execute_in_transaction_rolls_back_on_panic() { // std::panic boundary lets the test assert the engine is still // usable afterwards without aborting the whole test binary. let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - te.engine.execute_in_transaction::<_, ()>(|engine| { - engine.execute_command("INSERT INTO direct VALUES (1)")?; + te.engine.execute_in_transaction::<_, ()>(|txn| { + txn.execute_command("INSERT INTO direct VALUES (1)")?; // Simulate a programmer error mid-transaction — any panic // will do. Using `panic!` directly keeps clippy from // second-guessing a synthetic `.unwrap()` on a literal. @@ -117,7 +205,7 @@ fn execute_in_transaction_rolls_back_on_panic() { /// (missing the non-null key, which our INSERT emits as NULL). #[test] fn failed_json_ingest_rolls_back_partial_inserts() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE t (id INT NOT NULL, name TEXT)") @@ -137,7 +225,7 @@ fn failed_json_ingest_rolls_back_partial_inserts() { target_db: None, }; - let result = ingest_json(&te.engine, data, &opts); + let result = ingest_json(&mut te.engine, data, &opts); assert!(result.is_err(), "ingest should fail on NOT NULL violation"); // Crucially: the first two rows must have been rolled back. If @@ -154,7 +242,7 @@ fn failed_json_ingest_rolls_back_partial_inserts() { /// INSERTs atomically — no observable intermediate state. #[test] fn successful_replace_commits_atomically() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); // Pre-populate the table with data that a replace-mode ingest should overwrite. te.engine @@ -175,7 +263,7 @@ fn successful_replace_commits_atomically() { merge_key: None, target_db: None, }; - let result = ingest_json(&te.engine, data, &opts).unwrap(); + let result = ingest_json(&mut te.engine, data, &opts).unwrap(); assert_eq!(result.rows, 2); let rows = query_resilient(&te.engine, "SELECT COUNT(*) as cnt FROM t"); @@ -196,15 +284,15 @@ fn successful_replace_commits_atomically() { /// at the engine level — the MCP handler is a thin wrapper around this. #[test] fn batched_upsert_inserts_when_row_missing() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE settings (key TEXT NOT NULL, value TEXT NOT NULL)") .unwrap(); te.engine - .execute_in_transaction(|engine| { - engine.execute_command("UPDATE settings SET value = 'dark' WHERE key = 'theme'")?; - engine.execute_command( + .execute_in_transaction(|txn| { + txn.execute_command("UPDATE settings SET value = 'dark' WHERE key = 'theme'")?; + txn.execute_command( "INSERT INTO settings (key, value) SELECT 'theme', 'dark' \ WHERE NOT EXISTS (SELECT 1 FROM settings WHERE key = 'theme')", )?; @@ -219,7 +307,7 @@ fn batched_upsert_inserts_when_row_missing() { #[test] fn batched_upsert_updates_when_row_exists() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE settings (key TEXT NOT NULL, value TEXT NOT NULL)") .unwrap(); @@ -228,9 +316,9 @@ fn batched_upsert_updates_when_row_exists() { .unwrap(); te.engine - .execute_in_transaction(|engine| { - engine.execute_command("UPDATE settings SET value = 'dark' WHERE key = 'theme'")?; - engine.execute_command( + .execute_in_transaction(|txn| { + txn.execute_command("UPDATE settings SET value = 'dark' WHERE key = 'theme'")?; + txn.execute_command( "INSERT INTO settings (key, value) SELECT 'theme', 'dark' \ WHERE NOT EXISTS (SELECT 1 FROM settings WHERE key = 'theme')", )?; @@ -255,7 +343,7 @@ fn batched_upsert_updates_when_row_exists() { /// one fails — leaving the table in a state the user never asked for. #[test] fn batched_multi_table_mutation_rolls_back_on_second_failure() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE orders (id INT NOT NULL, customer_id INT)") .unwrap(); @@ -267,14 +355,12 @@ fn batched_multi_table_mutation_rolls_back_on_second_failure() { .unwrap(); use hyperdb_mcp::error::McpError; - let result: Result<(), McpError> = te.engine.execute_in_transaction(|engine| { - engine.execute_command("INSERT INTO orders (id, customer_id) VALUES (1001, 42)")?; + let result: Result<(), McpError> = te.engine.execute_in_transaction(|txn| { + txn.execute_command("INSERT INTO orders (id, customer_id) VALUES (1001, 42)")?; // This second statement violates NOT NULL on `id` because we // omit it — the entire batch must roll back. - engine.execute_command("INSERT INTO orders (customer_id) VALUES (42)")?; - engine.execute_command( - "UPDATE customers SET total_orders = total_orders + 2 WHERE id = 42", - )?; + txn.execute_command("INSERT INTO orders (customer_id) VALUES (42)")?; + txn.execute_command("UPDATE customers SET total_orders = total_orders + 2 WHERE id = 42")?; Ok(()) }); assert!(result.is_err(), "batch should fail on NOT NULL violation"); @@ -305,7 +391,7 @@ fn batched_multi_table_mutation_rolls_back_on_second_failure() { /// partially populated. #[test] fn failed_replace_leaves_empty_table_not_partial() { - let te = TestEngine::new_ephemeral(); + let mut te = TestEngine::new_ephemeral(); te.engine .execute_command("CREATE TABLE t (id INT, name TEXT)") @@ -331,7 +417,7 @@ fn failed_replace_leaves_empty_table_not_partial() { target_db: None, }; - let result = ingest_json(&te.engine, data, &opts); + let result = ingest_json(&mut te.engine, data, &opts); assert!(result.is_err(), "ingest should fail on type cast error"); // The pre-failure INSERT was rolled back — the table exists but has From 1897bb8c1e18c68cb1a8e1d1f1d69b8f76b2fbb5 Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sat, 5 Sep 2026 22:30:21 -0700 Subject: [PATCH 2/2] refactor(core)!: flatten internal client::Error to the canonical M-ERRORS shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies to `hyperdb_api_core::client::Error` the shape `hyperdb_api::Error` already took in #70: a flat enum with one variant per failure mode, matched directly, per the Microsoft Pragmatic Rust Guidelines (M-ERRORS-CANONICAL-STRUCTS, M-ERRORS-AVOID-WRAPPING-AND-AS-DYN). Was a struct carrying `kind: ErrorKind` plus a `Box` cause channel, which forced a two-level match and type-erased the cause. No effect on `hyperdb-api`'s public API, though `hyperdb-api-core` is itself published, so this is a breaking change to a published crate. `client::Error` and `client::ErrorKind` are not re-exported from `hyperdb-api` — `hyperdb-api/src/lib.rs` says so explicitly at the `Notice` re-export — and the only in-tree consumer outside `hyperdb-api-core` is the `From for hyperdb_api::Error` impl. `hyperdb-api-core` is published for dependency resolution only and its README and CHANGELOG both document it as internal with no semver promise. Information preserved: - `Connection`, `Cancelled`, and `Closed` carry `{ message, sqlstate }`; `Query` carries `{ message, sqlstate, detail, hint }`. Those are exactly the variants that could hold a SQLSTATE before, so `sqlstate()`, `detail()`, `hint()`, and `message()` return what they always did on those variants — which matters because the public mapping reads `sqlstate` for all four. - Narrower on the four single-string variants, though: `Authentication`, `FeatureNotSupported`, `Timeout`, and `Other` fold any `detail` into the message and discard `hint` and `sqlstate`, where `new_with_details` stored all three on any kind. Nothing observes the loss — the public mapping already discarded both on the arms these feed — so this is a narrowing of what is stored, not of what any caller can read. - `Error::with_cause` had zero call sites, so removing the `Box` channel drops no information that was ever populated. The only other producer, `Error::io`, stored the same text as both `message` and `cause` and `Display` printed both — an I/O failure rendered as "refused: refused". That duplication is now gone. - gRPC is the one place that picks a variant at runtime from a wire code, so `grpc/error.rs` keeps a private `Variant` discriminator with a `build` method. Variants without a `detail` field fold it into the message rather than dropping it, matching what the old `Display` rendered. Constructors: every variant has a snake_case one taking `impl Into` (`config`, `conversion`, and `cancelled` are new). `Error::new`, `with_cause`, and `new_with_details` are gone, along with `ErrorKind` and `kind()`. `closed()` and `timeout()` now take a message instead of supplying a canned one, and `io(io::Error)` became `from_io` so `io` could be the message-taking constructor like its peers. The public mapping is now a variant-to-variant match with no wildcard arm. `client::Error` is deliberately *not* `#[non_exhaustive]`: the attribute would buy forward-compatible matching that an internal type with a single in-workspace consumer has no use for, at the cost of the compile-time exhaustiveness check on the one match that must give every variant a public meaning. A variant added upstream should break that build rather than silently degrade to `Error::Internal`. Refs #75 BREAKING CHANGE: `hyperdb-api-core` is published to crates.io, so although `client::Error` is internal and never re-exported, this breaks that crate's API: `client::ErrorKind` is deleted; `Error` goes from struct to enum; `Error::new`, `with_cause`, `new_with_details`, and `kind()` are removed; and `Error::io` is resignatured (the message-taking constructor now owns that name, and the `io::Error` one is `from_io`). Both removals fail loudly — `io::Error` does not implement `Into`, and a missing `ErrorKind` is a hard resolution error — so no downstream can silently misbehave. `Display` text changes for I/O-origin connection errors, which do reach the public `hyperdb_api::Error`: `"refused: refused"` becomes `"refused"`. Intended, but string-matching on the message will notice. --- Cargo.lock | 1 + MIGRATING-0.3.md | 5 +- hyperdb-api-core/CHANGELOG.md | 57 +++ hyperdb-api-core/Cargo.toml | 1 + hyperdb-api-core/src/client/async_client.rs | 20 +- .../src/client/async_connection.rs | 7 +- hyperdb-api-core/src/client/client.rs | 19 +- hyperdb-api-core/src/client/connection.rs | 22 +- hyperdb-api-core/src/client/error.rs | 449 ++++++++++++------ .../src/client/grpc/authenticated_client.rs | 134 ++---- hyperdb-api-core/src/client/grpc/client.rs | 38 +- hyperdb-api-core/src/client/grpc/error.rs | 214 ++++++--- hyperdb-api-core/src/client/grpc/executor.rs | 22 +- hyperdb-api-core/src/client/grpc/result.rs | 17 +- hyperdb-api-core/src/client/mod.rs | 2 +- hyperdb-api-core/src/client/row.rs | 6 +- hyperdb-api-core/src/client/tls.rs | 37 +- hyperdb-api/src/error.rs | 134 +++--- hyperdb-api/src/lib.rs | 5 +- hyperdb-compile-check/Cargo.lock | 1 + hyperdb-mcp/src/error.rs | 2 +- 21 files changed, 726 insertions(+), 467 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 04bfe7b6..92ee3835 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1828,6 +1828,7 @@ dependencies = [ "sha2 0.11.0", "socket2", "tempfile", + "thiserror 2.0.20", "tokio", "tokio-rustls", "tonic", diff --git a/MIGRATING-0.3.md b/MIGRATING-0.3.md index d4b88d7c..edc67a19 100644 --- a/MIGRATING-0.3.md +++ b/MIGRATING-0.3.md @@ -19,7 +19,7 @@ The public `hyperdb_api::Error` type was redesigned into a flat enum per the [Mi | `Error::new(msg)` | Constructor deleted. Use a specific variant or `Error::internal(msg)` (see below). | | `Error::with_cause(msg, e)` | Constructor deleted. For an `io::Error` cause use `Error::connection_with_io(msg, e)`; otherwise fold the cause into a message string. | | `Error::kind() -> Option` | Method deleted. Match directly on the enum. | -| `pub use ... ErrorKind` from `hyperdb_api` | Re-export removed. The `ErrorKind` type is internal to `hyperdb-api-core` and not part of `hyperdb-api`'s public surface. | +| `pub use ... ErrorKind` from `hyperdb_api` | Re-export removed, and the type itself is gone: `hyperdb-api-core`'s internal `client::Error` was later flattened the same way, so there is no `ErrorKind` anywhere in the workspace. | ### What's new @@ -184,7 +184,8 @@ if let Error::Server { sqlstate: Some(code), detail, hint, .. } = &err { ### Notes for downstream crate authors -- The `From for hyperdb_api::Error` impl is exhaustive over `client::ErrorKind`. Adding a kind to `client::Error` will break this build until a mapping is added. This is intended. +- The `From for hyperdb_api::Error` impl is a variant-to-variant match: `client::Error` is a flat enum too ([#75](https://github.com/tableau/hyper-api-rust/issues/75)). Unlike the public enum it feeds, `client::Error` is deliberately **not** `#[non_exhaustive]`, so that match has no wildcard arm and stays exhaustive. + A variant added upstream therefore breaks this build until it is given a deliberate public mapping, rather than silently degrading to `Error::Internal`. That check is the reason for the asymmetry: `client::Error` is internal and has exactly one consumer, shipped from the same workspace, so it gains nothing from forward-compatibility and would lose the compiler's help where it matters most. - `Error::Connection { source }` carries an `Option`. The wire-protocol layer in `hyperdb-api-core` does not preserve typed causes through its boundary, so `source` is `None` for errors that originated there. Direct callers in `hyperdb-api` who construct `Error::connection_with_io` *do* preserve the typed source. - The `Error::Internal { .. }` variant is a deliberate catch-all for invariant violations. New code should reach for a domain variant first. diff --git a/hyperdb-api-core/CHANGELOG.md b/hyperdb-api-core/CHANGELOG.md index 68936f2e..3a8e177b 100644 --- a/hyperdb-api-core/CHANGELOG.md +++ b/hyperdb-api-core/CHANGELOG.md @@ -29,6 +29,52 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Changed +- **BREAKING:** `client::Error` is now a flat enum — one + variant per failure mode, matched directly — rather than a struct with a + `kind` discriminator and a `Box` cause channel + ([#75](https://github.com/tableau/hyper-api-rust/issues/75)). This applies + the shape `hyperdb_api::Error` already adopted in + [#70](https://github.com/tableau/hyper-api-rust/issues/70) per the + [Microsoft Pragmatic Rust Guidelines](https://microsoft.github.io/rust-guidelines/) + (M-ERRORS-CANONICAL-STRUCTS, M-ERRORS-AVOID-WRAPPING-AND-AS-DYN). + + **No effect on `hyperdb-api`'s public API** — `client::Error` and + `client::ErrorKind` were never re-exported from it, and this crate is + documented above as internal. + + - `client::ErrorKind` is **removed**, along with `Error::kind()`, + `Error::new()`, `Error::with_cause()`, and `Error::new_with_details()`. + - Every variant has a snake_case constructor taking `impl Into`: + `connection`, `authentication`, `query`, `protocol`, `io`, `config`, + `timeout`, `cancelled`, `closed`, `conversion`, `feature_not_supported`, + `other`. `Error::closed()` and `Error::timeout()` previously took no + arguments and supplied a canned message; they now take one. + - `Error::io(err: io::Error)` became `Error::from_io(err: io::Error)`, so + the name `io` could be the message-taking constructor like its peers. + `impl From` is unchanged. + - `Connection`, `Cancelled`, and `Closed` carry `{ message, sqlstate }`; + `Query` carries `{ message, sqlstate, detail, hint }`. Those are exactly + the variants that could hold a SQLSTATE before, so `sqlstate()`, + `detail()`, `hint()`, and `message()` return what they used to **on + those variants**. + - The enum is deliberately **not** `#[non_exhaustive]`, unlike the public + `hyperdb_api::Error`. This type is internal with a single in-workspace + consumer, so it gains nothing from forward-compatible matching, while + staying exhaustive keeps the compile-time check on + `From for hyperdb_api::Error` — a new variant must be + given a deliberate public mapping instead of silently degrading to + `Error::Internal`. + - **Narrower diagnostics on four variants.** `Authentication`, + `FeatureNotSupported`, `Timeout`, and `Other` are single-string, so the + gRPC error path folds any server `detail` into the message and + **discards `hint` and `sqlstate`** on those four; the old + `new_with_details` stored all three regardless of kind. No caller + observes the loss — the public `hyperdb_api::Error` mapping already + discarded `hint` and `sqlstate` on the arms these feed — but the stored + data is genuinely narrower, not merely reshaped. + - `Error::with_cause` had no call sites, so dropping the `Box` channel + loses no information. `source()` now returns `None` for I/O errors; the + public `hyperdb_api::Error` mapping already discarded that cause. - The all-binary `Bind` path now sends a **single** parameter format code rather than one per parameter. The PostgreSQL protocol broadcasts a lone format code across every parameter, so this is wire-compatible and drops the @@ -56,6 +102,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- An `Error` built from an `io::Error` no longer renders its message twice. + `Error::io` stored the same text as both `message` and `cause`, and + `Display` printed both, so an I/O failure surfaced as `"refused: refused"`. + Dropping the cause channel removed the duplication. + + This **changes the `Display` text** of I/O-origin connection errors, and + those reach the public `hyperdb_api::Error` (its `Connection` variant is + built from this message). The new text is the intended one, but anything + matching on the error *string* rather than the variant will see + `"refused"` where it saw `"refused: refused"`. + - **`AuthenticatedGrpcClient::get_table_labels` and `get_column_labels` now report Arrow failures instead of returning a partial map.** Both iterated record batches with `if let Ok(batch) = batch_result`, so a decode failure diff --git a/hyperdb-api-core/Cargo.toml b/hyperdb-api-core/Cargo.toml index 5b47275d..3d3e7734 100644 --- a/hyperdb-api-core/Cargo.toml +++ b/hyperdb-api-core/Cargo.toml @@ -23,6 +23,7 @@ salesforce-auth = ["dep:hyperdb-api-salesforce", "dep:arrow"] bytes = { workspace = true } byteorder = { workspace = true } memchr = { workspace = true } +thiserror = { workspace = true } tracing = { workspace = true } socket2 = { workspace = true } diff --git a/hyperdb-api-core/src/client/async_client.rs b/hyperdb-api-core/src/client/async_client.rs index e97572de..e3aa0401 100644 --- a/hyperdb-api-core/src/client/async_client.rs +++ b/hyperdb-api-core/src/client/async_client.rs @@ -439,7 +439,7 @@ impl AsyncClient { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; } #[cfg(unix)] @@ -466,7 +466,7 @@ impl AsyncClient { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; } #[cfg(windows)] @@ -499,10 +499,10 @@ impl AsyncClient { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; - file.flush().map_err(Error::io)?; + file.flush().map_err(Error::from_io)?; } } @@ -608,8 +608,8 @@ impl AsyncClient { let mut buf = BytesMut::with_capacity(16); frontend::cancel_request(self.process_id, self.secret_key, &mut buf); - stream.write_all(&buf).map_err(Error::io)?; - stream.flush().map_err(Error::io)?; + stream.write_all(&buf).map_err(Error::from_io)?; + stream.flush().map_err(Error::from_io)?; } #[cfg(unix)] ConnectionEndpoint::DomainSocket { directory, name } => { @@ -630,8 +630,8 @@ impl AsyncClient { let mut buf = BytesMut::with_capacity(16); frontend::cancel_request(self.process_id, self.secret_key, &mut buf); - stream.write_all(&buf).map_err(Error::io)?; - stream.flush().map_err(Error::io)?; + stream.write_all(&buf).map_err(Error::from_io)?; + stream.flush().map_err(Error::from_io)?; } #[cfg(windows)] ConnectionEndpoint::NamedPipe { host, name } => { @@ -655,8 +655,8 @@ impl AsyncClient { let mut buf = BytesMut::with_capacity(16); frontend::cancel_request(self.process_id, self.secret_key, &mut buf); - file.write_all(&buf).map_err(Error::io)?; - file.flush().map_err(Error::io)?; + file.write_all(&buf).map_err(Error::from_io)?; + file.flush().map_err(Error::from_io)?; } } diff --git a/hyperdb-api-core/src/client/async_connection.rs b/hyperdb-api-core/src/client/async_connection.rs index d6aa3295..319ad559 100644 --- a/hyperdb-api-core/src/client/async_connection.rs +++ b/hyperdb-api-core/src/client/async_connection.rs @@ -102,8 +102,7 @@ where /// connection before any bytes hit the wire. pub(crate) fn ensure_healthy(&self) -> Result<()> { if self.desynchronized { - return Err(Error::new( - super::error::ErrorKind::Connection, + return Err(Error::connection( "connection is desynchronized from the server and cannot be reused; \ discard it and open a new one", )); @@ -526,7 +525,7 @@ where /// (server closed the connection). pub async fn read_message(&mut self) -> Result { loop { - if let Some(msg) = Message::parse(&mut self.read_buf).map_err(Error::io)? { + if let Some(msg) = Message::parse(&mut self.read_buf).map_err(Error::from_io)? { return Ok(msg); } @@ -542,7 +541,7 @@ where if n == 0 { self.read_buf.truncate(prev_len); warn!(target: "hyperdb_api", "connection-closed"); - return Err(Error::closed()); + return Err(Error::closed("connection closed")); } self.read_buf.truncate(prev_len + n); } diff --git a/hyperdb-api-core/src/client/client.rs b/hyperdb-api-core/src/client/client.rs index 6d59adf4..44b8f5f1 100644 --- a/hyperdb-api-core/src/client/client.rs +++ b/hyperdb-api-core/src/client/client.rs @@ -70,7 +70,7 @@ use super::cancel::Cancellable; use super::config::Config; use super::connection::{RawConnection, parse_error_response}; use super::endpoint::ConnectionEndpoint; -use super::error::{Error, ErrorKind, Result}; +use super::error::{Error, Result}; use super::prepare; use super::row::{Row, StreamRow}; use super::statement::ParamFormat; @@ -572,10 +572,10 @@ impl Client { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; - stream.flush().map_err(Error::io)?; + stream.flush().map_err(Error::from_io)?; } #[cfg(unix)] ConnectionEndpoint::DomainSocket { directory, name } => { @@ -602,10 +602,10 @@ impl Client { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; - stream.flush().map_err(Error::io)?; + stream.flush().map_err(Error::from_io)?; } #[cfg(windows)] ConnectionEndpoint::NamedPipe { host, name } => { @@ -636,10 +636,10 @@ impl Client { error = %e, "query-cancel-send-failed" ); - Error::io(e) + Error::from_io(e) })?; - file.flush().map_err(Error::io)?; + file.flush().map_err(Error::from_io)?; } } @@ -1185,7 +1185,7 @@ impl Client { /// /// # Errors /// - /// - Returns [`ErrorKind::Query`] if `query` (trimmed) does not + /// - Returns [`Error::Query`] if `query` (trimmed) does not /// start with `COPY` (defense-in-depth check against non-COPY /// statements). /// - Returns [`Error`] (connection) if the connection mutex is @@ -1195,8 +1195,7 @@ impl Client { pub fn copy_in_raw(&self, query: &str) -> Result> { // Defense-in-depth: reject queries that don't look like COPY statements if !query.trim_start().to_ascii_uppercase().starts_with("COPY") { - return Err(Error::new( - ErrorKind::Query, + return Err(Error::query( "copy_in_raw() requires a COPY statement. \ The query must start with 'COPY'.", )); diff --git a/hyperdb-api-core/src/client/connection.rs b/hyperdb-api-core/src/client/connection.rs index 75e587d0..a87c1aa7 100644 --- a/hyperdb-api-core/src/client/connection.rs +++ b/hyperdb-api-core/src/client/connection.rs @@ -144,7 +144,7 @@ where !self.desynchronized } - /// Fast-fails with an explicit [`ErrorKind::Connection`] error if the + /// Fast-fails with an explicit [`Error::Connection`] error if the /// wire has fallen out of sync with the server, before any bytes are /// written to the stream. Called from the entry point of every public /// method that initiates a new server request — simple queries, @@ -155,8 +155,7 @@ where /// *next* unrelated operation. pub(crate) fn ensure_healthy(&self) -> Result<()> { if self.desynchronized { - return Err(Error::new( - crate::client::error::ErrorKind::Connection, + return Err(Error::connection( "connection is desynchronized from the server and cannot be reused; \ discard it and open a new one", )); @@ -579,7 +578,7 @@ where /// (server closed the connection). pub fn read_message(&mut self) -> Result { loop { - if let Some(msg) = Message::parse(&mut self.read_buf).map_err(Error::io)? { + if let Some(msg) = Message::parse(&mut self.read_buf).map_err(Error::from_io)? { return Ok(msg); } @@ -604,7 +603,7 @@ where if n == 0 { self.read_buf.truncate(prev_len); warn!(target: "hyperdb_api", "connection-closed"); - return Err(Error::closed()); + return Err(Error::closed("connection closed")); } self.read_buf.truncate(prev_len + n); } @@ -1180,12 +1179,9 @@ where } Message::CopyData(body) if in_copy_out => { let chunk = body.data(); - writer.write_all(chunk).map_err(|e| { - Error::new( - super::error::ErrorKind::Io, - format!("Failed to write COPY data: {e}"), - ) - })?; + writer + .write_all(chunk) + .map_err(|e| Error::io(format!("Failed to write COPY data: {e}")))?; total_bytes += chunk.len() as u64; } Message::CopyDone => { @@ -1280,7 +1276,7 @@ mod tests { conn.desynchronized = true; assert!(!conn.is_healthy()); let err = conn.ensure_healthy().expect_err("must fail-fast"); - assert_eq!(err.kind(), crate::client::error::ErrorKind::Connection); + assert!(matches!(err, Error::Connection { .. })); assert!( err.to_string().to_lowercase().contains("desynchron"), "error message should mention desynchronization; got: {err}", @@ -1319,7 +1315,7 @@ mod tests { let Err(err) = conn.simple_query("SELECT 1") else { panic!("desynced simple_query must fail-fast") }; - assert_eq!(err.kind(), crate::client::error::ErrorKind::Connection); + assert!(matches!(err, Error::Connection { .. })); assert!(err.to_string().to_lowercase().contains("desynchron")); } } diff --git a/hyperdb-api-core/src/client/error.rs b/hyperdb-api-core/src/client/error.rs index 1f45b28f..d51d7954 100644 --- a/hyperdb-api-core/src/client/error.rs +++ b/hyperdb-api-core/src/client/error.rs @@ -2,190 +2,278 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT //! Error types for the Hyper client. +//! +//! [`Error`] is a flat enum: one variant per failure mode, matched +//! directly, with no `kind()` discriminator and no `Box` +//! cause channel. That is the shape the [Microsoft Pragmatic Rust +//! Guidelines][msrg] call for (M-ERRORS-CANONICAL-STRUCTS, +//! M-ERRORS-AVOID-WRAPPING-AND-AS-DYN), and it mirrors the public +//! `hyperdb_api::Error` this type feeds. +//! +//! This type is **internal**. It is not re-exported from `hyperdb-api`; +//! callers of the public API match on `hyperdb_api::Error` instead, which +//! `From` produces. +//! +//! [msrg]: https://microsoft.github.io/rust-guidelines/ -use std::error::Error as StdError; -use std::fmt; use std::io; -/// The error type for Hyper client operations. -#[derive(Debug)] -pub struct Error { - kind: ErrorKind, - message: String, - cause: Option>, - /// SQLSTATE error code (for query errors) - sqlstate_code: Option, - /// Additional detail about the error - detail: Option, - /// Hint for resolving the error - hint: Option, -} +use thiserror::Error as ThisError; -/// The kind of error that occurred. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ErrorKind { +/// The error type for Hyper client operations. +/// +/// Variants that can carry a server-supplied SQLSTATE expose it as a +/// field, so callers match on it structurally rather than scraping the +/// message. The remaining variants are single-string: whatever context +/// exists is already rendered into that string by the constructor. +/// +/// Deliberately **not** `#[non_exhaustive]`. That attribute buys +/// forward-compatibility for downstream matches, which this type has no +/// use for: it is internal, not re-exported, and `hyperdb-api` is its only +/// consumer and ships from this same workspace in lockstep. What it would +/// cost is the compile-time exhaustiveness check on +/// `From for hyperdb_api::Error` — the one place a new +/// variant must be given a public mapping. Better that adding a variant +/// breaks that build than silently degrades to `Error::Internal`. +#[derive(Debug, ThisError)] +pub enum Error { /// Connection failed. - Connection, + #[error("{message}")] + Connection { + /// Human-readable description of the failure. + message: String, + /// SQLSTATE, when the failure arrived from the server (gRPC + /// reports connection-class SQLSTATEs in the `08xxx` family). + sqlstate: Option, + }, + /// Authentication failed. - Authentication, + #[error("{0}")] + Authentication(String), + /// Query execution failed. - Query, + /// + /// The one variant that carries the server's full diagnostic + /// payload — `DETAIL` and `HINT` are surfaced verbatim by the public + /// `hyperdb_api::Error::Server` variant this maps to. + #[error("{message}{}", render_detail(message, detail.as_deref()))] + Query { + /// The primary error message. + message: String, + /// SQLSTATE code, when the server supplied one. + sqlstate: Option, + /// The server's `DETAIL` field. + detail: Option, + /// The server's `HINT` field. + hint: Option, + }, + /// Invalid response from server. - Protocol, + #[error("{0}")] + Protocol(String), + /// I/O error. - Io, + #[error("{0}")] + Io(String), + /// Configuration error. - Config, + #[error("{0}")] + Config(String), + /// Operation timed out. - Timeout, + #[error("{0}")] + Timeout(String), + /// Operation was cancelled. - Cancelled, + #[error("{message}")] + Cancelled { + /// Human-readable description of the cancellation. + message: String, + /// SQLSTATE, typically `57014` (`query_canceled`). + sqlstate: Option, + }, + /// The connection was closed. - Closed, + #[error("{message}")] + Closed { + /// Human-readable description. + message: String, + /// SQLSTATE, when the server supplied one. + sqlstate: Option, + }, + /// Type conversion error. - Conversion, + #[error("{0}")] + Conversion(String), + /// Feature not supported by this connection type. - FeatureNotSupported, + #[error("{0}")] + FeatureNotSupported(String), + /// Other error. - Other, + #[error("{0}")] + Other(String), +} + +/// Renders the `": {detail}"` suffix for [`Error::Query`]'s `Display`, +/// suppressing it when `message` already contains the detail text. +/// +/// gRPC's `decode_error_info` builds `"{primary}: {customer_detail}"` as +/// the message and *also* reports `customer_detail` separately, so without +/// this guard the detail would print twice. +fn render_detail(message: &str, detail: Option<&str>) -> String { + match detail { + Some(detail) if !message.contains(detail) => format!(": {detail}"), + _ => String::new(), + } } impl Error { - /// Creates a new error with the given kind and message. - pub fn new(kind: ErrorKind, message: impl Into) -> Self { - Error { - kind, + // Constructors. Every variant has one taking `impl Into`; + // the variants with a SQLSTATE field default it to `None` here and + // are built with struct literals where a code is available. + + /// Creates a connection error with no SQLSTATE. + pub fn connection(message: impl Into) -> Self { + Error::Connection { message: message.into(), - cause: None, - sqlstate_code: None, - detail: None, - hint: None, + sqlstate: None, } } - /// Creates a new error with a cause. - pub fn with_cause(kind: ErrorKind, message: impl Into, cause: E) -> Self - where - E: Into>, - { - Error { - kind, - message: message.into(), - cause: Some(cause.into()), - sqlstate_code: None, - detail: None, - hint: None, - } + /// Creates an authentication error. + pub fn authentication(message: impl Into) -> Self { + Error::Authentication(message.into()) } - /// Creates a new error with additional details (SQLSTATE, detail, hint). - /// - /// This is primarily used for gRPC errors that carry structured error information. - pub fn new_with_details( - kind: ErrorKind, - message: impl Into, - detail: Option, - hint: Option, - sqlstate: Option, - ) -> Self { - Error { - kind, + /// Creates a query error with no SQLSTATE, detail, or hint. + pub fn query(message: impl Into) -> Self { + Error::Query { message: message.into(), - cause: None, - sqlstate_code: sqlstate, - detail, - hint, + sqlstate: None, + detail: None, + hint: None, } } - /// Returns the error kind. - #[must_use] - pub fn kind(&self) -> ErrorKind { - self.kind + /// Creates a protocol error. + pub fn protocol(message: impl Into) -> Self { + Error::Protocol(message.into()) } - /// Returns the error message. - #[must_use] - pub fn message(&self) -> &str { - &self.message + /// Creates an I/O error from a message. + /// + /// Prefer [`Error::from_io`] when an [`io::Error`] is in hand. + pub fn io(message: impl Into) -> Self { + Error::Io(message.into()) } - /// Returns the error detail, if available. - #[must_use] - pub fn detail(&self) -> Option<&str> { - self.detail.as_deref() + /// Creates a configuration error. + pub fn config(message: impl Into) -> Self { + Error::Config(message.into()) } - /// Returns the error hint, if available. - #[must_use] - pub fn hint(&self) -> Option<&str> { - self.hint.as_deref() + /// Creates a timeout error. + pub fn timeout(message: impl Into) -> Self { + Error::Timeout(message.into()) } - // Convenience constructors - - /// Creates a connection error. - pub fn connection(message: impl Into) -> Self { - Self::new(ErrorKind::Connection, message) + /// Creates a cancellation error with no SQLSTATE. + pub fn cancelled(message: impl Into) -> Self { + Error::Cancelled { + message: message.into(), + sqlstate: None, + } } - /// Creates an authentication error. - pub fn authentication(message: impl Into) -> Self { - Self::new(ErrorKind::Authentication, message) + /// Creates a closed-connection error with no SQLSTATE. + pub fn closed(message: impl Into) -> Self { + Error::Closed { + message: message.into(), + sqlstate: None, + } } - /// Creates a query error. - pub fn query(message: impl Into) -> Self { - Self::new(ErrorKind::Query, message) + /// Creates a type-conversion error. + pub fn conversion(message: impl Into) -> Self { + Error::Conversion(message.into()) } - /// Creates a protocol error. - pub fn protocol(message: impl Into) -> Self { - Self::new(ErrorKind::Protocol, message) + /// Creates a "feature not supported" error. + /// + /// Used when an operation is not available on a particular connection + /// type (e.g. write operations on gRPC connections). + pub fn feature_not_supported(message: impl Into) -> Self { + Error::FeatureNotSupported(message.into()) } - /// Creates a closed connection error. - #[must_use] - pub fn closed() -> Self { - Self::new(ErrorKind::Closed, "connection closed") + /// Creates a generic "other" error. + pub fn other(message: impl Into) -> Self { + Error::Other(message.into()) } - /// Creates a timeout error. - #[must_use] - pub fn timeout() -> Self { - Self::new(ErrorKind::Timeout, "operation timed out") - } + // Convenience constructors for the common shapes. - /// Creates an error from an I/O error. + /// Creates an I/O error from an [`io::Error`]. + /// + /// Takes the error by value so it can be used point-free as + /// `.map_err(Error::from_io)`. + #[expect( + clippy::needless_pass_by_value, + reason = "call-site ergonomics: consumed as a `map_err` function reference" + )] #[must_use] - pub fn io(err: io::Error) -> Self { - Self::with_cause(ErrorKind::Io, err.to_string(), err) + pub fn from_io(err: io::Error) -> Self { + Error::Io(err.to_string()) } /// Creates an error from a database error response. #[must_use] pub fn db(severity: &str, code: &str, message: &str) -> Self { - Error { - kind: ErrorKind::Query, + Error::Query { message: format!("{severity}: {message} ({code})"), - cause: None, - sqlstate_code: Some(code.to_string()), + sqlstate: Some(code.to_string()), detail: None, hint: None, } } - /// Creates a "feature not supported" error. - /// - /// Used when an operation is not available on a particular connection type - /// (e.g., write operations on gRPC connections). - pub fn feature_not_supported(message: impl Into) -> Self { - Self::new(ErrorKind::FeatureNotSupported, message) + /// Returns the error message, without any `DETAIL` suffix that + /// `Display` would append. + #[must_use] + pub fn message(&self) -> &str { + match self { + Error::Connection { message, .. } + | Error::Query { message, .. } + | Error::Cancelled { message, .. } + | Error::Closed { message, .. } => message, + Error::Authentication(message) + | Error::Protocol(message) + | Error::Io(message) + | Error::Config(message) + | Error::Timeout(message) + | Error::Conversion(message) + | Error::FeatureNotSupported(message) + | Error::Other(message) => message, + } } - /// Creates a generic "other" error. - pub fn other(message: impl Into) -> Self { - Self::new(ErrorKind::Other, message) + /// Returns the error detail, if available. + #[must_use] + pub fn detail(&self) -> Option<&str> { + match self { + Error::Query { detail, .. } => detail.as_deref(), + _ => None, + } + } + + /// Returns the error hint, if available. + #[must_use] + pub fn hint(&self) -> Option<&str> { + match self { + Error::Query { hint, .. } => hint.as_deref(), + _ => None, + } } /// Extracts the `PostgreSQL` SQLSTATE code from the error, if present. @@ -193,25 +281,32 @@ impl Error { /// SQLSTATE codes are 5-character codes that identify error conditions. /// See: /// + /// For [`Error::Query`] with no stored code, falls back to scraping the + /// trailing `(CODE)` that Hyper appends to wire error messages. + /// /// # Example /// /// ``` - /// use hyperdb_api_core::client::{Error, ErrorKind}; + /// use hyperdb_api_core::client::Error; /// /// let err = Error::db("ERROR", "42P04", "database already exists"); /// assert_eq!(err.sqlstate(), Some("42P04")); /// ``` #[must_use] pub fn sqlstate(&self) -> Option<&str> { - // First check if we have a stored SQLSTATE code - if let Some(ref code) = self.sqlstate_code { - return Some(code); - } - // Fall back to extracting from message for backwards compatibility - if self.kind == ErrorKind::Query { - extract_sqlstate(&self.message) - } else { - None + match self { + Error::Connection { sqlstate, .. } + | Error::Cancelled { sqlstate, .. } + | Error::Closed { sqlstate, .. } => sqlstate.as_deref(), + Error::Query { + sqlstate, message, .. + } => match sqlstate { + Some(code) => Some(code), + // Backwards compatibility: older paths encode the code in + // the message rather than storing it. + None => extract_sqlstate(message), + }, + _ => None, } } } @@ -235,30 +330,9 @@ fn extract_sqlstate(message: &str) -> Option<&str> { } } -impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.message)?; - if let Some(ref detail) = self.detail - && !self.message.contains(detail) - { - write!(f, ": {detail}")?; - } - if let Some(ref cause) = self.cause { - write!(f, ": {cause}")?; - } - Ok(()) - } -} - -impl StdError for Error { - fn source(&self) -> Option<&(dyn StdError + 'static)> { - self.cause.as_ref().map(|e| &**e as &dyn std::error::Error) - } -} - impl From for Error { fn from(err: io::Error) -> Self { - Error::io(err) + Error::from_io(err) } } @@ -290,14 +364,77 @@ mod tests { #[test] fn test_sqlstate_non_query_error() { - // Non-query errors should not have SQLSTATE + // Non-query errors carry no SQLSTATE unless one was supplied. let err = Error::connection("connection failed"); assert_eq!(err.sqlstate(), None); - let err = Error::timeout(); + let err = Error::timeout("operation timed out"); assert_eq!(err.sqlstate(), None); } + /// The gRPC path builds `Connection` / `Cancelled` / `Closed` with a + /// server-supplied SQLSTATE; `sqlstate()` must surface it, because the + /// public `hyperdb_api::Error` mapping forwards it to callers. + #[test] + fn test_sqlstate_on_non_query_variants() { + let err = Error::Cancelled { + message: "query canceled".to_string(), + sqlstate: Some("57014".to_string()), + }; + assert_eq!(err.sqlstate(), Some("57014")); + + let err = Error::Connection { + message: "connection failure".to_string(), + sqlstate: Some("08006".to_string()), + }; + assert_eq!(err.sqlstate(), Some("08006")); + + let err = Error::Closed { + message: "closed".to_string(), + sqlstate: Some("08003".to_string()), + }; + assert_eq!(err.sqlstate(), Some("08003")); + } + + /// `Display` appends `DETAIL` only when the message doesn't already + /// carry it — gRPC folds the detail into the message *and* reports it + /// separately, and printing it twice was the bug this guard prevents. + #[test] + fn test_display_detail_suffix() { + let err = Error::Query { + message: "column not found".to_string(), + sqlstate: None, + detail: Some("column \"foo\" does not exist".to_string()), + hint: None, + }; + assert_eq!( + err.to_string(), + "column not found: column \"foo\" does not exist" + ); + + let err = Error::Query { + message: "column not found: column \"foo\" does not exist".to_string(), + sqlstate: None, + detail: Some("column \"foo\" does not exist".to_string()), + hint: None, + }; + assert_eq!( + err.to_string(), + "column not found: column \"foo\" does not exist", + "detail already present in the message must not be repeated" + ); + } + + /// An `io::Error` renders exactly once. The previous struct-shaped + /// error stored the same text as both `message` and `cause`, so + /// `Display` emitted it twice. + #[test] + fn test_io_error_renders_once() { + let io_err = io::Error::new(io::ErrorKind::ConnectionRefused, "refused"); + let err = Error::from(io_err); + assert_eq!(err.to_string(), "refused"); + } + #[test] fn test_extract_sqlstate_edge_cases() { // Valid SQLSTATE diff --git a/hyperdb-api-core/src/client/grpc/authenticated_client.rs b/hyperdb-api-core/src/client/grpc/authenticated_client.rs index 62fcc0c2..3914ba84 100644 --- a/hyperdb-api-core/src/client/grpc/authenticated_client.rs +++ b/hyperdb-api-core/src/client/grpc/authenticated_client.rs @@ -34,7 +34,7 @@ use tracing::{debug, info, warn}; use std::collections::HashMap; -use crate::client::error::{Error, ErrorKind, Result}; +use crate::client::error::{Error, Result}; use hyperdb_api_salesforce::{DataCloudToken, SharedTokenProvider}; use super::error::from_grpc_status; @@ -167,9 +167,9 @@ impl AuthenticatedGrpcClient { /// # Errors /// /// Propagates any error from `Self::ensure_connected`: Salesforce - /// auth failures (surfaced as [`ErrorKind::Authentication`]), - /// invalid tenant URL (surfaced as [`ErrorKind::Config`]), or - /// gRPC transport setup failures ([`ErrorKind::Connection`]). + /// auth failures (surfaced as [`Error::Authentication`]), + /// invalid tenant URL (surfaced as [`Error::Config`]), or + /// gRPC transport setup failures ([`Error::Connection`]). pub async fn connect( token_provider: SharedTokenProvider, dataspace: Option, @@ -398,7 +398,7 @@ impl AuthenticatedGrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Authentication`] if the DC JWT cannot be + /// - Returns [`Error::Authentication`] if the DC JWT cannot be /// refreshed through the underlying Salesforce token provider /// (including after the auth-retry budget is exhausted). /// - Propagates any error from @@ -461,10 +461,7 @@ impl AuthenticatedGrpcClient { } Err(last_error.unwrap_or_else(|| { - Error::new( - ErrorKind::Authentication, - "Parameterized query failed after token refresh", - ) + Error::authentication("Parameterized query failed after token refresh") })) } @@ -472,7 +469,7 @@ impl AuthenticatedGrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Authentication`] if every retry attempt + /// - Returns [`Error::Authentication`] if every retry attempt /// still surfaces an auth error after forcing a token refresh. /// - Propagates any other [`Error`] from the underlying gRPC /// executor (SQL errors, transport failures). @@ -522,21 +519,16 @@ impl AuthenticatedGrpcClient { } } - Err(last_error.unwrap_or_else(|| { - Error::new( - ErrorKind::Authentication, - "Query failed after token refresh", - ) - })) + Err(last_error.unwrap_or_else(|| Error::authentication("Query failed after token refresh"))) } /// Forces a token refresh, even if the current token is still valid. /// /// # Errors /// - /// - Returns [`ErrorKind::Authentication`] if + /// - Returns [`Error::Authentication`] if /// [`SharedTokenProvider::force_refresh`] fails. - /// - Returns [`ErrorKind::Config`] or [`ErrorKind::Connection`] + /// - Returns [`Error::Config`] or [`Error::Connection`] /// if the fresh tenant URL is invalid or the gRPC channel /// cannot be rebuilt. pub async fn refresh_token(&mut self) -> Result<()> { @@ -584,7 +576,7 @@ impl AuthenticatedGrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Other`] if every retry attempt still + /// - Returns [`Error::Other`] if every retry attempt still /// fails with an auth error after forcing a token refresh. /// - Propagates any error from /// [`GrpcClient::cancel_query`](super::GrpcClient::cancel_query) (transport failure, `tonic::Status`). @@ -617,10 +609,7 @@ impl AuthenticatedGrpcClient { } } - Err(Error::new( - ErrorKind::Other, - "Cancel failed after DC JWT refresh", - )) + Err(Error::other("Cancel failed after DC JWT refresh")) } #[expect( @@ -844,21 +833,13 @@ impl AuthenticatedGrpcClient { return Ok(Vec::new()); } - let reader = StreamReader::try_new(Cursor::new(arrow_data), None).map_err(|e| { - Error::new( - ErrorKind::Protocol, - format!("Failed to parse Arrow data: {e}"), - ) - })?; + let reader = StreamReader::try_new(Cursor::new(arrow_data), None) + .map_err(|e| Error::protocol(format!("Failed to parse Arrow data: {e}")))?; let mut values = Vec::new(); for batch_result in reader { - let batch = batch_result.map_err(|e| { - Error::new( - ErrorKind::Protocol, - format!("Failed to read Arrow batch: {e}"), - ) - })?; + let batch = batch_result + .map_err(|e| Error::protocol(format!("Failed to read Arrow batch: {e}")))?; if let Some(arr) = batch .column(column_idx) @@ -887,21 +868,13 @@ impl AuthenticatedGrpcClient { return Ok(Vec::new()); } - let reader = StreamReader::try_new(Cursor::new(arrow_data), None).map_err(|e| { - Error::new( - ErrorKind::Protocol, - format!("Failed to parse Arrow data: {e}"), - ) - })?; + let reader = StreamReader::try_new(Cursor::new(arrow_data), None) + .map_err(|e| Error::protocol(format!("Failed to parse Arrow data: {e}")))?; let mut tables = Vec::new(); for batch_result in reader { - let batch = batch_result.map_err(|e| { - Error::new( - ErrorKind::Protocol, - format!("Failed to read Arrow batch: {e}"), - ) - })?; + let batch = batch_result + .map_err(|e| Error::protocol(format!("Failed to read Arrow batch: {e}")))?; let schema_col = batch .column(0) @@ -947,7 +920,7 @@ impl AuthenticatedGrpcClient { /// # Errors /// /// Propagates any error from [`Self::execute_query`]. Returns an - /// Arrow IPC parse error (wrapped as [`ErrorKind::Other`]) when + /// Arrow IPC parse error (wrapped as [`Error::Other`]) when /// the result payload cannot be read as a `StreamReader`. pub async fn get_table_labels( &mut self, @@ -982,7 +955,7 @@ impl AuthenticatedGrpcClient { /// # Errors /// /// Propagates any error from [`Self::execute_query`]. Returns an - /// Arrow IPC parse error (wrapped as [`ErrorKind::Other`]) when + /// Arrow IPC parse error (wrapped as [`Error::Other`]) when /// the result payload cannot be read as a `StreamReader`. pub async fn get_column_labels( &mut self, @@ -1048,12 +1021,11 @@ impl AuthenticatedGrpcClient { return Ok(()); } - let token = self.token_provider.get_token().await.map_err(|e| { - Error::new( - ErrorKind::Authentication, - format!("Failed to get DC JWT: {e}"), - ) - })?; + let token = self + .token_provider + .get_token() + .await + .map_err(|e| Error::authentication(format!("Failed to get DC JWT: {e}")))?; self.connect_to_tenant(&token).await?; self.current_token = Some(token); @@ -1069,12 +1041,11 @@ impl AuthenticatedGrpcClient { async fn force_refresh_and_reconnect(&mut self) -> Result<()> { info!("Refreshing DC JWT"); - let token = self.token_provider.refresh_token().await.map_err(|e| { - Error::new( - ErrorKind::Authentication, - format!("Failed to refresh DC JWT: {e}"), - ) - })?; + let token = self + .token_provider + .refresh_token() + .await + .map_err(|e| Error::authentication(format!("Failed to refresh DC JWT: {e}")))?; self.connect_to_tenant(&token).await?; self.current_token = Some(token); @@ -1088,13 +1059,13 @@ impl AuthenticatedGrpcClient { let tenant_url = token.tenant_url(); let hostname = tenant_url .host_str() - .ok_or_else(|| Error::new(ErrorKind::Config, "No hostname in tenant URL"))?; + .ok_or_else(|| Error::config("No hostname in tenant URL"))?; let grpc_endpoint = format!("https://{hostname}:443"); info!(endpoint = %grpc_endpoint, "Connecting to Data Cloud"); let endpoint = Endpoint::from_shared(grpc_endpoint.clone()) - .map_err(|e| Error::new(ErrorKind::Config, format!("Invalid gRPC endpoint: {e}")))?; + .map_err(|e| Error::config(format!("Invalid gRPC endpoint: {e}")))?; let endpoint = endpoint .connect_timeout(self.connect_timeout) @@ -1104,14 +1075,12 @@ impl AuthenticatedGrpcClient { let tls_config = tonic::transport::ClientTlsConfig::new().with_enabled_roots(); let endpoint = endpoint .tls_config(tls_config) - .map_err(|e| Error::new(ErrorKind::Config, format!("TLS configuration error: {e}")))?; + .map_err(|e| Error::config(format!("TLS configuration error: {e}")))?; - let channel = endpoint.connect().await.map_err(|e| { - Error::new( - ErrorKind::Connection, - format!("Failed to connect to {grpc_endpoint}: {e}"), - ) - })?; + let channel = endpoint + .connect() + .await + .map_err(|e| Error::connection(format!("Failed to connect to {grpc_endpoint}: {e}")))?; self.channel = Some(channel); debug!("gRPC channel established"); @@ -1148,12 +1117,12 @@ impl AuthenticatedGrpcClient { let channel = self .channel .as_ref() - .ok_or_else(|| Error::new(ErrorKind::Connection, "Not connected"))?; + .ok_or_else(|| Error::connection("Not connected"))?; let token = self .current_token .as_ref() - .ok_or_else(|| Error::new(ErrorKind::Authentication, "No token available"))?; + .ok_or_else(|| Error::authentication("No token available"))?; let params = params.into(); debug!( @@ -1167,12 +1136,7 @@ impl AuthenticatedGrpcClient { // Build the lakehouse name let lakehouse = token .lakehouse_name(self.dataspace.as_deref()) - .map_err(|e| { - Error::new( - ErrorKind::Authentication, - format!("Failed to get lakehouse name: {e}"), - ) - })?; + .map_err(|e| Error::authentication(format!("Failed to get lakehouse name: {e}")))?; // Build query parameter let query_param = QueryParam { @@ -1239,12 +1203,12 @@ impl AuthenticatedGrpcClient { let channel = self .channel .as_ref() - .ok_or_else(|| Error::new(ErrorKind::Connection, "Not connected"))?; + .ok_or_else(|| Error::connection("Not connected"))?; let token = self .current_token .as_ref() - .ok_or_else(|| Error::new(ErrorKind::Authentication, "No token available"))?; + .ok_or_else(|| Error::authentication("No token available"))?; debug!(query_id = %query_id, "Cancelling query"); @@ -1263,14 +1227,14 @@ impl AuthenticatedGrpcClient { token .bearer_token() .parse() - .map_err(|_| Error::new(ErrorKind::Authentication, "Invalid token format"))?, + .map_err(|_| Error::authentication("Invalid token format"))?, ); request.metadata_mut().insert( "audience", token .tenant_url_str() .parse() - .map_err(|_| Error::new(ErrorKind::Config, "Invalid tenant URL"))?, + .map_err(|_| Error::config("Invalid tenant URL"))?, ); let mut client = HyperServiceClient::new(channel.clone()) @@ -1293,7 +1257,7 @@ impl AuthenticatedGrpcClient { /// Broader substring matches (e.g. "token", "expired") are intentionally /// avoided to prevent spurious retries on unrelated errors. fn is_auth_error(error: &Error) -> bool { - if matches!(error.kind(), ErrorKind::Authentication) { + if matches!(error, Error::Authentication(_)) { return true; } @@ -1331,14 +1295,14 @@ impl AuthenticatedGrpcClientSync { /// /// # Errors /// - /// - Returns [`ErrorKind::Other`] if a current-thread Tokio + /// - Returns [`Error::Other`] if a current-thread Tokio /// runtime cannot be built. /// - Propagates any error from [`AuthenticatedGrpcClient::connect`]. pub fn connect(token_provider: SharedTokenProvider, dataspace: Option) -> Result { let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() - .map_err(|e| Error::new(ErrorKind::Other, format!("Failed to create runtime: {e}")))?; + .map_err(|e| Error::other(format!("Failed to create runtime: {e}")))?; let inner = runtime.block_on(AuthenticatedGrpcClient::connect(token_provider, dataspace))?; diff --git a/hyperdb-api-core/src/client/grpc/client.rs b/hyperdb-api-core/src/client/grpc/client.rs index c8f56c4a..82d0eaf4 100644 --- a/hyperdb-api-core/src/client/grpc/client.rs +++ b/hyperdb-api-core/src/client/grpc/client.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use tonic::transport::{Channel, Endpoint}; use tracing::{debug, info, warn}; -use crate::client::error::{Error, ErrorKind, Result}; +use crate::client::error::{Error, Result}; use super::config::GrpcConfig; use super::error::from_grpc_status; @@ -88,15 +88,15 @@ impl GrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Config`] if `config.endpoint` is not a + /// - Returns [`Error::Config`] if `config.endpoint` is not a /// well-formed URI, or if TLS configuration fails. - /// - Returns [`ErrorKind::Connection`] if the gRPC transport + /// - Returns [`Error::Connection`] if the gRPC transport /// cannot establish a channel to the endpoint. pub async fn connect(config: GrpcConfig) -> Result { info!(endpoint = %config.endpoint, "Connecting to Hyper via gRPC"); let endpoint = Endpoint::from_shared(config.endpoint.clone()) - .map_err(|e| Error::new(ErrorKind::Config, format!("Invalid gRPC endpoint: {e}")))?; + .map_err(|e| Error::config(format!("Invalid gRPC endpoint: {e}")))?; // Configure timeouts let endpoint = endpoint @@ -108,9 +108,9 @@ impl GrpcClient { // Use system root certificates for TLS validation let tls_config = tonic::transport::ClientTlsConfig::new().with_enabled_roots(); - endpoint.tls_config(tls_config).map_err(|e| { - Error::new(ErrorKind::Config, format!("TLS configuration error: {e}")) - })? + endpoint + .tls_config(tls_config) + .map_err(|e| Error::config(format!("TLS configuration error: {e}")))? } else { endpoint }; @@ -118,10 +118,9 @@ impl GrpcClient { // Connect let channel = endpoint.connect().await.map_err(|e| { debug!("gRPC connection error details: {:?}", e); - Error::new( - ErrorKind::Connection, - format!("Failed to connect to gRPC endpoint: {e} (details: {e:?})"), - ) + Error::connection(format!( + "Failed to connect to gRPC endpoint: {e} (details: {e:?})" + )) })?; debug!("gRPC channel established"); @@ -303,7 +302,7 @@ impl GrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Protocol`] if the server returns no + /// - Returns [`Error::Protocol`] if the server returns no /// result chunks and does not signal completion. /// - Propagates any error from the underlying /// `GrpcQueryExecutor` — auth failure, transport error, or @@ -381,7 +380,7 @@ impl GrpcClient { } if final_result.chunks.is_empty() && !final_result.is_complete { - return Err(Error::new(ErrorKind::Protocol, "No result from query")); + return Err(Error::protocol("No result from query")); } Ok(final_result) @@ -393,7 +392,7 @@ impl GrpcClient { /// /// # Errors /// - /// - Returns [`ErrorKind::Protocol`] if the server returns no + /// - Returns [`Error::Protocol`] if the server returns no /// result chunks and does not signal completion. /// - Propagates any error from the underlying /// `GrpcQueryExecutor` — auth failure, transport error, or @@ -463,7 +462,7 @@ impl GrpcClient { } if final_result.chunks.is_empty() && !final_result.is_complete { - return Err(Error::new(ErrorKind::Protocol, "No result from query")); + return Err(Error::protocol("No result from query")); } Ok(final_result) @@ -800,7 +799,7 @@ impl GrpcClientSync { /// /// # Errors /// - /// - Returns [`ErrorKind::Other`] if a current-thread Tokio + /// - Returns [`Error::Other`] if a current-thread Tokio /// runtime cannot be built. /// - Propagates any error from [`GrpcClient::connect`] (invalid /// endpoint, TLS configuration failure, or transport setup @@ -809,12 +808,7 @@ impl GrpcClientSync { let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() - .map_err(|e| { - Error::new( - ErrorKind::Other, - format!("Failed to create Tokio runtime: {e}"), - ) - })?; + .map_err(|e| Error::other(format!("Failed to create Tokio runtime: {e}")))?; let inner = runtime.block_on(GrpcClient::connect(config))?; diff --git a/hyperdb-api-core/src/client/grpc/error.rs b/hyperdb-api-core/src/client/grpc/error.rs index 604814cb..5d50c3a4 100644 --- a/hyperdb-api-core/src/client/grpc/error.rs +++ b/hyperdb-api-core/src/client/grpc/error.rs @@ -10,7 +10,76 @@ use std::fmt; use tonic::Status; -use crate::client::error::{Error, ErrorKind}; +use crate::client::error::Error; + +/// Which [`Error`] variant a gRPC status code or SQLSTATE maps to. +/// +/// gRPC is the one place in the crate where the variant is chosen at +/// runtime from a wire code rather than being known at the call site, so +/// the decision needs a name it can be carried around under. It stays +/// private to this module — [`Error`] itself is flat and has no `kind`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Variant { + Authentication, + Cancelled, + Connection, + FeatureNotSupported, + Other, + Query, + Timeout, +} + +impl Variant { + /// Builds the corresponding [`Error`], attaching the server's + /// diagnostics to the variants that carry them. + /// + /// `Query`, `Connection`, and `Cancelled` have fields for the pieces + /// they can use. The rest have only a message, so any `detail` is + /// folded into it rather than dropped — that keeps the rendered text + /// identical to what the previous struct-shaped error produced. + fn build( + self, + message: String, + detail: Option, + hint: Option, + sqlstate: Option, + ) -> Error { + match self { + Variant::Query => Error::Query { + message, + sqlstate, + detail, + hint, + }, + Variant::Connection => Error::Connection { + message: fold_detail(message, detail.as_deref()), + sqlstate, + }, + Variant::Cancelled => Error::Cancelled { + message: fold_detail(message, detail.as_deref()), + sqlstate, + }, + Variant::Authentication => { + Error::authentication(fold_detail(message, detail.as_deref())) + } + Variant::FeatureNotSupported => { + Error::feature_not_supported(fold_detail(message, detail.as_deref())) + } + Variant::Timeout => Error::timeout(fold_detail(message, detail.as_deref())), + Variant::Other => Error::other(fold_detail(message, detail.as_deref())), + } + } +} + +/// Appends `": {detail}"` unless `message` already contains it, matching +/// what `Error`'s `Display` does for the variants that keep a `detail` +/// field. +fn fold_detail(message: String, detail: Option<&str>) -> String { + match detail { + Some(detail) if !message.contains(detail) => format!("{message}: {detail}"), + _ => message, + } +} /// gRPC-specific error information. /// @@ -54,8 +123,7 @@ impl std::error::Error for GrpcError {} pub(super) fn from_grpc_status(status: Status) -> Error { // First, try to parse structured error details (ErrorInfo proto) if let Some(error_info) = parse_error_info(&status) { - return Error::new_with_details( - grpc_code_to_error_kind(status.code()), + return grpc_code_to_variant(status.code()).build( error_info.message, error_info.detail, error_info.hint, @@ -69,7 +137,7 @@ pub(super) fn from_grpc_status(status: Status) -> Error { } // Last resort: use the raw gRPC error message - Error::new(grpc_code_to_error_kind(status.code()), status.message()) + grpc_code_to_variant(status.code()).build(status.message().to_string(), None, None, None) } /// Attempts to parse `ErrorInfo` from the gRPC status details. @@ -216,18 +284,12 @@ fn parse_xml_error(message: &str) -> Option { (None, None) => message.to_string(), }; - // Determine error kind from SQLSTATE - let kind = sqlstate + // Determine the variant from SQLSTATE + let variant = sqlstate .as_ref() - .map_or(ErrorKind::Query, |s| sqlstate_to_error_kind(s)); - - Some(Error::new_with_details( - kind, - error_message, - detail, - hint, - sqlstate, - )) + .map_or(Variant::Query, |s| sqlstate_to_variant(s)); + + Some(variant.build(error_message, detail, hint, sqlstate)) } /// Extracts content from an XML tag like `content`. @@ -241,42 +303,42 @@ fn extract_xml_tag(text: &str, tag: &str) -> Option { Some(text[start..end].to_string()) } -/// Converts gRPC status code to `ErrorKind`. -fn grpc_code_to_error_kind(code: tonic::Code) -> ErrorKind { +/// Converts a gRPC status code to the [`Error`] variant it maps to. +fn grpc_code_to_variant(code: tonic::Code) -> Variant { match code { - tonic::Code::Ok => ErrorKind::Other, // Shouldn't happen for errors - tonic::Code::Cancelled => ErrorKind::Cancelled, - tonic::Code::Unknown => ErrorKind::Query, - tonic::Code::InvalidArgument => ErrorKind::Query, - tonic::Code::DeadlineExceeded => ErrorKind::Timeout, - tonic::Code::NotFound => ErrorKind::Query, - tonic::Code::AlreadyExists => ErrorKind::Query, - tonic::Code::PermissionDenied => ErrorKind::Authentication, - tonic::Code::ResourceExhausted => ErrorKind::Query, - tonic::Code::FailedPrecondition => ErrorKind::Query, - tonic::Code::Aborted => ErrorKind::Query, - tonic::Code::OutOfRange => ErrorKind::Query, - tonic::Code::Unimplemented => ErrorKind::FeatureNotSupported, - tonic::Code::Internal => ErrorKind::Query, - tonic::Code::Unavailable => ErrorKind::Connection, - tonic::Code::DataLoss => ErrorKind::Query, - tonic::Code::Unauthenticated => ErrorKind::Authentication, + tonic::Code::Ok => Variant::Other, // Shouldn't happen for errors + tonic::Code::Cancelled => Variant::Cancelled, + tonic::Code::Unknown => Variant::Query, + tonic::Code::InvalidArgument => Variant::Query, + tonic::Code::DeadlineExceeded => Variant::Timeout, + tonic::Code::NotFound => Variant::Query, + tonic::Code::AlreadyExists => Variant::Query, + tonic::Code::PermissionDenied => Variant::Authentication, + tonic::Code::ResourceExhausted => Variant::Query, + tonic::Code::FailedPrecondition => Variant::Query, + tonic::Code::Aborted => Variant::Query, + tonic::Code::OutOfRange => Variant::Query, + tonic::Code::Unimplemented => Variant::FeatureNotSupported, + tonic::Code::Internal => Variant::Query, + tonic::Code::Unavailable => Variant::Connection, + tonic::Code::DataLoss => Variant::Query, + tonic::Code::Unauthenticated => Variant::Authentication, } } -/// Converts SQLSTATE code to `ErrorKind`. -fn sqlstate_to_error_kind(sqlstate: &str) -> ErrorKind { +/// Converts a SQLSTATE code to the [`Error`] variant it maps to. +fn sqlstate_to_variant(sqlstate: &str) -> Variant { match sqlstate { // Query canceled - "57014" => ErrorKind::Cancelled, + "57014" => Variant::Cancelled, // Authentication errors (28xxx) - s if s.starts_with("28") => ErrorKind::Authentication, + s if s.starts_with("28") => Variant::Authentication, // Connection errors (08xxx) - s if s.starts_with("08") => ErrorKind::Connection, + s if s.starts_with("08") => Variant::Connection, // Feature not supported (0A000) - "0A000" => ErrorKind::FeatureNotSupported, + "0A000" => Variant::FeatureNotSupported, // Everything else is a query error - _ => ErrorKind::Query, + _ => Variant::Query, } } @@ -306,17 +368,63 @@ mod tests { #[test] fn test_grpc_code_mapping() { - assert!(matches!( - grpc_code_to_error_kind(tonic::Code::Cancelled), - ErrorKind::Cancelled - )); - assert!(matches!( - grpc_code_to_error_kind(tonic::Code::Unauthenticated), - ErrorKind::Authentication - )); - assert!(matches!( - grpc_code_to_error_kind(tonic::Code::Unavailable), - ErrorKind::Connection - )); + assert_eq!( + grpc_code_to_variant(tonic::Code::Cancelled), + Variant::Cancelled + ); + assert_eq!( + grpc_code_to_variant(tonic::Code::Unauthenticated), + Variant::Authentication + ); + assert_eq!( + grpc_code_to_variant(tonic::Code::Unavailable), + Variant::Connection + ); + } + + /// A SQLSTATE-selected variant must keep the code on the variants that + /// have a field for it — the public `hyperdb_api::Error` mapping reads + /// `sqlstate()` for `Cancelled` and `Connection`, not just `Query`. + #[test] + fn test_sqlstate_survives_variant_selection() { + let err = sqlstate_to_variant("57014").build( + "canceled".to_string(), + None, + None, + Some("57014".to_string()), + ); + assert!(matches!(err, Error::Cancelled { .. })); + assert_eq!(err.sqlstate(), Some("57014")); + + let err = sqlstate_to_variant("08006").build( + "connection failure".to_string(), + None, + None, + Some("08006".to_string()), + ); + assert!(matches!(err, Error::Connection { .. })); + assert_eq!(err.sqlstate(), Some("08006")); + } + + /// The variants with no `detail` field must fold it into the message + /// rather than dropping it, so the rendered text is unchanged. + #[test] + fn test_detail_folded_into_message_when_no_field() { + let err = Variant::Timeout.build( + "deadline exceeded".to_string(), + Some("waited 30s".to_string()), + None, + None, + ); + assert_eq!(err.to_string(), "deadline exceeded: waited 30s"); + + // Already contained → not repeated. + let err = Variant::Timeout.build( + "deadline exceeded: waited 30s".to_string(), + Some("waited 30s".to_string()), + None, + None, + ); + assert_eq!(err.to_string(), "deadline exceeded: waited 30s"); } } diff --git a/hyperdb-api-core/src/client/grpc/executor.rs b/hyperdb-api-core/src/client/grpc/executor.rs index 372e10f2..2214a398 100644 --- a/hyperdb-api-core/src/client/grpc/executor.rs +++ b/hyperdb-api-core/src/client/grpc/executor.rs @@ -48,7 +48,7 @@ use bytes::Bytes; use tonic::Streaming; use tracing::{debug, trace, warn}; -use crate::client::error::{Error, ErrorKind, Result}; +use crate::client::error::{Error, Result}; use super::error::from_grpc_status; use super::proto::hyper_service::query_param::TransferMode; @@ -254,9 +254,10 @@ where /// instead of buffering the entire inline response. async fn read_initial_results(&mut self) -> Result<()> { let response = { - let stream = self.execute_stream.as_mut().ok_or_else(|| { - Error::new(ErrorKind::Protocol, "ExecuteQuery stream not initialized") - })?; + let stream = self + .execute_stream + .as_mut() + .ok_or_else(|| Error::protocol("ExecuteQuery stream not initialized"))?; stream.message().await.map_err(from_grpc_status)? }; @@ -428,7 +429,7 @@ where let query_id = self .query_id .clone() - .ok_or_else(|| Error::new(ErrorKind::Protocol, "No query ID for status request"))?; + .ok_or_else(|| Error::protocol("No query ID for status request"))?; debug!(query_id = %query_id, "Requesting query status"); @@ -470,7 +471,7 @@ where let stream = self .query_info_stream .as_mut() - .ok_or_else(|| Error::new(ErrorKind::Protocol, "QueryInfo stream not initialized"))?; + .ok_or_else(|| Error::protocol("QueryInfo stream not initialized"))?; if let Some(info) = stream.message().await.map_err(from_grpc_status)? { match info.content { @@ -531,7 +532,7 @@ where let query_id = self .query_id .clone() - .ok_or_else(|| Error::new(ErrorKind::Protocol, "No query ID for result request"))?; + .ok_or_else(|| Error::protocol("No query ID for result request"))?; debug!( query_id = %query_id, @@ -582,9 +583,10 @@ where async fn read_results(&mut self) -> Result<()> { loop { let result = { - let stream = self.query_result_stream.as_mut().ok_or_else(|| { - Error::new(ErrorKind::Protocol, "QueryResult stream not initialized") - })?; + let stream = self + .query_result_stream + .as_mut() + .ok_or_else(|| Error::protocol("QueryResult stream not initialized"))?; stream.message().await.map_err(from_grpc_status)? }; diff --git a/hyperdb-api-core/src/client/grpc/result.rs b/hyperdb-api-core/src/client/grpc/result.rs index 905a43f9..0d596413 100644 --- a/hyperdb-api-core/src/client/grpc/result.rs +++ b/hyperdb-api-core/src/client/grpc/result.rs @@ -10,7 +10,7 @@ use std::collections::VecDeque; use bytes::{Bytes, BytesMut}; -use crate::client::error::{Error, ErrorKind, Result}; +use crate::client::error::{Error, Result}; use super::proto::{QueryResultSchema, SqlType}; @@ -304,18 +304,14 @@ pub(super) fn sql_type_to_hyper_type(sql_type: &SqlType) -> Result Err(Error::new(ErrorKind::Conversion, "Unspecified SQL type")), + TypeTag::HyperUnspecified => Err(Error::conversion("Unspecified SQL type")), TypeTag::HyperBool => Ok(HyperSqlType::Bool), TypeTag::HyperSmallInt => Ok(HyperSqlType::SmallInt), TypeTag::HyperInt => Ok(HyperSqlType::Int), @@ -356,10 +352,7 @@ pub(super) fn sql_type_to_hyper_type(sql_type: &SqlType) -> Result Ok(HyperSqlType::Geography), TypeTag::HyperArrayOfFloat => { // Array types are not directly supported in crate::types::SqlType - Err(Error::new( - ErrorKind::Conversion, - "Array types not yet supported", - )) + Err(Error::conversion("Array types not yet supported")) } } } diff --git a/hyperdb-api-core/src/client/mod.rs b/hyperdb-api-core/src/client/mod.rs index 9899c60e..4bbf4322 100644 --- a/hyperdb-api-core/src/client/mod.rs +++ b/hyperdb-api-core/src/client/mod.rs @@ -306,7 +306,7 @@ pub use cancel::Cancellable; pub use client::{Client, CopyInWriter, QueryStream}; pub use config::Config; pub use endpoint::ConnectionEndpoint; -pub use error::{Error, ErrorKind, Result}; +pub use error::{Error, Result}; pub use notice::{Notice, NoticeReceiver}; pub use prepare::{OwnedPreparedStatement, PreparedStatement, SqlParam}; pub use row::{BatchRow, FromBinaryValue, Row, StreamRow}; diff --git a/hyperdb-api-core/src/client/row.rs b/hyperdb-api-core/src/client/row.rs index 04d9c697..d0e76821 100644 --- a/hyperdb-api-core/src/client/row.rs +++ b/hyperdb-api-core/src/client/row.rs @@ -42,7 +42,7 @@ use std::sync::Arc; use crate::protocol::message::backend::DataRowBody; use crate::types::FromHyperBinary; -use super::error::{Error, ErrorKind, Result}; +use super::error::{Error, Result}; use super::statement::Column; // ============================================================================= @@ -292,13 +292,13 @@ impl Row { /// /// # Errors /// - /// Returns [`ErrorKind::Query`] with the column name in the message + /// Returns [`Error::Query`] with the column name in the message /// if no column matches. pub fn column_index(&self, name: &str) -> Result { self.columns .iter() .position(|c| c.name() == name) - .ok_or_else(|| Error::new(ErrorKind::Query, format!("column not found: {name}"))) + .ok_or_else(|| Error::query(format!("column not found: {name}"))) } } diff --git a/hyperdb-api-core/src/client/tls.rs b/hyperdb-api-core/src/client/tls.rs index da856acc..356d7118 100644 --- a/hyperdb-api-core/src/client/tls.rs +++ b/hyperdb-api-core/src/client/tls.rs @@ -184,13 +184,13 @@ pub mod rustls_impl { use tokio_rustls::TlsConnector; use tokio_rustls::rustls::{ClientConfig, RootCertStore}; - use crate::client::error::{Error, ErrorKind, Result}; + use crate::client::error::{Error, Result}; /// Creates a TLS connector from the configuration. /// /// # Errors /// - /// Returns [`ErrorKind::Config`] when: + /// Returns [`Error::Config`] when: /// - The CA cert path is set but cannot be opened or the PEM bytes /// cannot be parsed / added to the root store. /// - The client cert / key path is set but cannot be opened, the @@ -214,20 +214,20 @@ pub mod rustls_impl { // Add custom CA certificate if provided if let Some(ref ca_path) = config.ca_cert_path { let certs = CertificateDer::pem_file_iter(ca_path) - .map_err(|e| Error::new(ErrorKind::Config, format!("failed to read CA cert: {e}")))? + .map_err(|e| Error::config(format!("failed to read CA cert: {e}")))? .collect::, _>>() - .map_err(|e| Error::new(ErrorKind::Config, format!("invalid CA cert: {e}")))?; + .map_err(|e| Error::config(format!("invalid CA cert: {e}")))?; for cert in certs { - root_store.add(cert).map_err(|e| { - Error::new(ErrorKind::Config, format!("failed to add CA cert: {e}")) - })?; + root_store + .add(cert) + .map_err(|e| Error::config(format!("failed to add CA cert: {e}")))?; } } let provider = Arc::new(rustls::crypto::ring::default_provider()); let builder = ClientConfig::builder_with_provider(provider) .with_safe_default_protocol_versions() - .map_err(|e| Error::new(ErrorKind::Config, format!("TLS protocol config error: {e}")))? + .map_err(|e| Error::config(format!("TLS protocol config error: {e}")))? .with_root_certificates(root_store); let client_config = if config.has_client_cert() { @@ -236,24 +236,19 @@ pub mod rustls_impl { let key_path = config.client_key_path.as_ref().unwrap(); let certs = CertificateDer::pem_file_iter(cert_path) - .map_err(|e| { - Error::new( - ErrorKind::Config, - format!("failed to read client cert: {e}"), - ) - })? + .map_err(|e| Error::config(format!("failed to read client cert: {e}")))? .collect::, _>>() - .map_err(|e| Error::new(ErrorKind::Config, format!("invalid client cert: {e}")))?; + .map_err(|e| Error::config(format!("invalid client cert: {e}")))?; // `from_pem_file` returns `Error::NoItemsFound` when the file is // syntactically valid PEM but contains no private-key section, so // we don't need a separate "no private key found" branch. let key = PrivateKeyDer::from_pem_file(key_path) - .map_err(|e| Error::new(ErrorKind::Config, format!("invalid client key: {e}")))?; + .map_err(|e| Error::config(format!("invalid client key: {e}")))?; builder .with_client_auth_cert(certs, key) - .map_err(|e| Error::new(ErrorKind::Config, format!("invalid client auth: {e}")))? + .map_err(|e| Error::config(format!("invalid client auth: {e}")))? } else { builder.with_no_client_auth() }; @@ -268,9 +263,9 @@ pub mod rustls_impl { /// /// # Errors /// - /// - Returns [`ErrorKind::Config`] if `server_name` is not a + /// - Returns [`Error::Config`] if `server_name` is not a /// valid DNS name or IP literal accepted by `rustls`. - /// - Returns [`ErrorKind::Connection`] if the TLS handshake with + /// - Returns [`Error::Connection`] if the TLS handshake with /// the peer fails (certificate rejected, protocol error, I/O /// failure). pub async fn wrap_stream( @@ -279,12 +274,12 @@ pub mod rustls_impl { server_name: &str, ) -> Result { let domain = rustls::pki_types::ServerName::try_from(server_name.to_string()) - .map_err(|_| Error::new(ErrorKind::Config, "invalid server name"))?; + .map_err(|_| Error::config("invalid server name"))?; connector .connect(domain, stream) .await - .map_err(|e| Error::new(ErrorKind::Connection, format!("TLS handshake failed: {e}"))) + .map_err(|e| Error::connection(format!("TLS handshake failed: {e}"))) } } diff --git a/hyperdb-api/src/error.rs b/hyperdb-api/src/error.rs index 0ed0e007..a40d701e 100644 --- a/hyperdb-api/src/error.rs +++ b/hyperdb-api/src/error.rs @@ -475,76 +475,80 @@ impl Error { } } -// Internal mapping: `client::Error` → public `Error`. The mapping is -// exhaustive over `client::ErrorKind` (verified to NOT be -// `#[non_exhaustive]`); adding a kind in `hyperdb-api-core` will break -// this build until the mapping is updated, which is intended. +// Internal mapping: `client::Error` → public `Error`. Both types are now +// flat enums, so this is a variant-to-variant match. `client::Error` is +// `#[non_exhaustive]`, so the wildcard arm is required; it routes any +// future variant to `Internal` rather than failing the build. // -// `chain = err.to_string()` walks the inner error's full Display chain -// (message + cause + detail). We use it for tuple variants whose -// `Display` is just `": {0}"`, where embedding the chain into -// the single string field gives the caller the full picture. +// `chain = err.to_string()` renders the inner error's `Display`, which +// folds in the `DETAIL` suffix where one applies. We use it for tuple +// variants whose `Display` is just `": {0}"`, where embedding the +// whole rendering into the single string field gives the caller the full +// picture. // // For the `Server` variant we use the *un-chained* `message` and pass // `detail`/`hint` separately; the `Server` `Display` impl re-appends // "DETAIL: ..." and "HINT: ..." lines from those fields, so using // `chain` would duplicate the detail text. // -// SQLSTATE: `client::Error::sqlstate()` may return `Some` for any -// kind. After Follow-up C, the flat enum carries `sqlstate` on -// `Server`, `Connection`, `Closed`, and `Cancelled` so callers can -// match on it programmatically (e.g. SQLSTATE 57014 `query_canceled` -// arrives via Cancelled and is now exposed structurally). Other -// variants still drop SQLSTATE — folded into the message via `chain`. +// SQLSTATE: the flat enum carries `sqlstate` on `Server`, `Connection`, +// `Closed`, and `Cancelled` so callers can match on it programmatically +// (e.g. SQLSTATE 57014 `query_canceled` arrives via Cancelled and is +// exposed structurally). Those are exactly the `client::Error` variants +// that carry one, so nothing is dropped in transit. impl From for Error { fn from(err: hyperdb_api_core::client::Error) -> Self { - use hyperdb_api_core::client::ErrorKind as CoreKind; + use hyperdb_api_core::client::Error as CoreError; let chain = err.to_string(); - let kind = err.kind(); - let sqlstate = err.sqlstate().map(str::to_string); - let detail = err.detail().map(str::to_string); - let hint = err.hint().map(str::to_string); - let message = err.message().to_string(); - - match kind { - CoreKind::Connection => Error::Connection { + + match err { + CoreError::Connection { sqlstate, .. } => Error::Connection { message: chain, source: None, sqlstate, }, - CoreKind::Authentication => Error::Authentication(chain), - // Use unchained `message` here: detail/hint are passed as + CoreError::Authentication(_) => Error::Authentication(chain), + // Use the unchained `message` here: detail/hint are passed as // separate fields and the `Server` Display impl re-renders // them. Using `chain` would duplicate detail text. - CoreKind::Query => Error::Server { + CoreError::Query { + message, + sqlstate, + detail, + hint, + } => Error::Server { sqlstate, message, detail, hint, }, - CoreKind::Protocol => Error::Protocol(chain), + CoreError::Protocol(_) => Error::Protocol(chain), // Wire-level I/O failures are reported as Connection errors // (the underlying io::Error is type-erased in core, so we // cannot recover it as a typed `source` here). - CoreKind::Io => Error::Connection { + CoreError::Io(_) => Error::Connection { message: chain, source: None, - sqlstate, + sqlstate: None, }, - CoreKind::Config => Error::Config(chain), - CoreKind::Timeout => Error::Timeout(chain), - CoreKind::Cancelled => Error::Cancelled { + CoreError::Config(_) => Error::Config(chain), + CoreError::Timeout(_) => Error::Timeout(chain), + CoreError::Cancelled { sqlstate, .. } => Error::Cancelled { message: chain, sqlstate, }, - CoreKind::Closed => Error::Closed { + CoreError::Closed { sqlstate, .. } => Error::Closed { message: chain, sqlstate, }, - CoreKind::Conversion => Error::Conversion(chain), - CoreKind::FeatureNotSupported => Error::FeatureNotSupported(chain), - CoreKind::Other => Error::Internal { message: chain }, + CoreError::Conversion(_) => Error::Conversion(chain), + CoreError::FeatureNotSupported(_) => Error::FeatureNotSupported(chain), + CoreError::Other(_) => Error::Internal { message: chain }, + // No wildcard arm on purpose: `client::Error` is not + // `#[non_exhaustive]`, so this match is exhaustive and a + // variant added upstream fails to compile here until it is + // given a deliberate public mapping. } } } @@ -567,7 +571,7 @@ pub type Result = std::result::Result; #[cfg(test)] mod tests { use super::*; - use hyperdb_api_core::client::{Error as CoreError, ErrorKind as CoreKind}; + use hyperdb_api_core::client::Error as CoreError; #[test] fn server_display_includes_sqlstate_detail_and_hint() { @@ -603,13 +607,12 @@ mod tests { // appends ": {detail}" inline. The flat-Error mapping must // not also add "\nDETAIL: {detail}" — that would duplicate the // text. We verify by counting occurrences. - let core = CoreError::new_with_details( - CoreKind::Query, - "duplicate key value", - Some("Key (id)=(42) already exists.".to_string()), - Some("Choose a different key.".to_string()), - Some("23505".to_string()), - ); + let core = CoreError::Query { + message: "duplicate key value".to_string(), + sqlstate: Some("23505".to_string()), + detail: Some("Key (id)=(42) already exists.".to_string()), + hint: Some("Choose a different key.".to_string()), + }; let public: Error = core.into(); let s = public.to_string(); // The detail text should appear exactly once in the rendered @@ -623,29 +626,36 @@ mod tests { } #[test] - fn from_client_error_exhaustive_over_kinds() { - // Smoke test: every ErrorKind maps cleanly with no panic. - // (Compilation already enforces exhaustiveness.) - for kind in [ - CoreKind::Connection, - CoreKind::Authentication, - CoreKind::Query, - CoreKind::Protocol, - CoreKind::Io, - CoreKind::Config, - CoreKind::Timeout, - CoreKind::Cancelled, - CoreKind::Closed, - CoreKind::Conversion, - CoreKind::FeatureNotSupported, - CoreKind::Other, + fn from_client_error_maps_every_variant() { + // Smoke test: every `client::Error` variant maps cleanly with no + // panic and without losing the message. + // + // This list is hand-written and nothing forces it to stay + // complete. The guarantee that a new variant gets a deliberate + // mapping comes from the `From` impl instead: `client::Error` is + // not `#[non_exhaustive]` and that match has no wildcard, so + // adding a variant upstream breaks the build there. Extend this + // list when that happens. + for core in [ + CoreError::connection("test message"), + CoreError::authentication("test message"), + CoreError::query("test message"), + CoreError::protocol("test message"), + CoreError::io("test message"), + CoreError::config("test message"), + CoreError::timeout("test message"), + CoreError::cancelled("test message"), + CoreError::closed("test message"), + CoreError::conversion("test message"), + CoreError::feature_not_supported("test message"), + CoreError::other("test message"), ] { - let core = CoreError::new(kind, "test message"); + let rendered = format!("{core:?}"); let public: Error = core.into(); // Each variant's Display must include the message text. assert!( public.to_string().contains("test message"), - "{kind:?} mapping lost the message: {public}", + "{rendered} mapping lost the message: {public}", ); } } diff --git a/hyperdb-api/src/lib.rs b/hyperdb-api/src/lib.rs index 0cd0a2e0..77f3f6b1 100644 --- a/hyperdb-api/src/lib.rs +++ b/hyperdb-api/src/lib.rs @@ -217,8 +217,9 @@ pub use connection_builder::ConnectionBuilder; pub use error::{ColumnErrorKind, Error, Result}; pub use params::{ParamFormat, ToSqlParam}; pub use prepared::PreparedStatement; -// Re-export Notice for callback registrants. ErrorKind is intentionally -// NOT re-exported — callers match directly on the flat `Error` enum. +// Re-export Notice for callback registrants. `hyperdb-api-core`'s +// `client::Error` is intentionally NOT re-exported — callers match +// directly on the flat `Error` enum this crate defines. pub use async_transaction::AsyncTransaction; pub use hyperdb_api_core::client::{Notice, NoticeReceiver}; pub use inserter::{ChunkSender, ColumnMapping, InsertChunk, Inserter, IntoValue, MappedInserter}; diff --git a/hyperdb-compile-check/Cargo.lock b/hyperdb-compile-check/Cargo.lock index 08cb5f34..2632381e 100644 --- a/hyperdb-compile-check/Cargo.lock +++ b/hyperdb-compile-check/Cargo.lock @@ -901,6 +901,7 @@ dependencies = [ "serde_json", "sha2", "socket2", + "thiserror 2.0.20", "tokio", "tokio-rustls", "tonic", diff --git a/hyperdb-mcp/src/error.rs b/hyperdb-mcp/src/error.rs index b556d4e6..4c29d41f 100644 --- a/hyperdb-mcp/src/error.rs +++ b/hyperdb-mcp/src/error.rs @@ -287,7 +287,7 @@ impl From for McpError { /// `POST_ERROR_DRAIN_CAP` budget without reaching `ReadyForQuery` or /// hits an I/O error mid-drain. Subsequent operations on that /// connection fast-fail with an -/// `ErrorKind::Connection` whose message contains `"desynchronized"`. +/// `Error::Connection` whose message contains `"desynchronized"`. /// The socket is technically still open but the wire state is corrupt /// and the only valid recovery is the same as #1: discard the /// connection and reconnect. Recognizing the signal here is what