diff --git a/src/client.rs b/src/client.rs index 9fe7e9c..db7bbfd 100644 --- a/src/client.rs +++ b/src/client.rs @@ -31,6 +31,36 @@ use crate::models::ConnectionParams; static POOLS: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); +/// Format a `tokio_postgres::Error` the way the built-in driver does: for a +/// server-side `DbError` (syntax errors, constraint violations, etc.) surface +/// the real severity/message instead of the generic `Kind::Db` "db error" +/// string that `tokio_postgres::Error`'s own `Display` impl produces. +pub(crate) fn format_pg_error(e: &tokio_postgres::Error) -> String { + if let Some(db) = e.as_db_error() { + let brief = format!("{}: {}", db.severity(), db.message()); + let detail = format!("{e:#?}"); + format!("{brief}\n\n{detail}") + } else { + e.to_string() + } +} + +/// Format a `pool.get()` failure. The pool's own connection-establishment +/// handshake (bad database name, bad password, ...) surfaces as a +/// `tokio_postgres::Error` wrapped in `PoolError::Backend` — the exact same +/// `Kind::Db`/"db error" pitfall `format_pg_error` exists to avoid, just one +/// layer deeper. `PostCreateHook` wraps the same shape via `HookError`. +pub(crate) fn format_pool_error(e: &deadpool_postgres::PoolError) -> String { + use deadpool_postgres::HookError; + match e { + deadpool_postgres::PoolError::Backend(pg_err) => format_pg_error(pg_err), + deadpool_postgres::PoolError::PostCreateHook(HookError::Backend(pg_err)) => { + format_pg_error(pg_err) + } + other => other.to_string(), + } +} + /// Build a connection pool from the given params and verify connectivity /// by acquiring one client and running `SELECT 1`. pub async fn test_connection(params: &ConnectionParams) -> Result<(), String> { @@ -38,11 +68,11 @@ pub async fn test_connection(params: &ConnectionParams) -> Result<(), String> { let client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pool_error(&e)))?; client .query_one("SELECT 1", &[]) .await - .map_err(|e| format!("Query failed: {e}"))?; + .map_err(|e| format_pg_error(&e))?; Ok(()) } @@ -58,11 +88,11 @@ pub async fn query_strings( let client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pool_error(&e)))?; let rows = client .query(query, query_params) .await - .map_err(|e| format!("Query failed: {e}"))?; + .map_err(|e| format_pg_error(&e))?; let results = rows .iter() @@ -81,11 +111,11 @@ pub async fn query_rows( let client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pool_error(&e)))?; client .query(query, query_params) .await - .map_err(|e| format!("Query failed: {e}")) + .map_err(|e| format_pg_error(&e)) } /// Execute a statement with explicit per-placeholder wire types, pinned via @@ -101,17 +131,17 @@ pub async fn execute_typed( let client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pool_error(&e)))?; let types: Vec = typed_params.iter().map(|(_, t)| t.clone()).collect(); let stmt = client .prepare_typed(query, &types) .await - .map_err(|e| format!("Prepare failed: {e}"))?; + .map_err(|e| format_pg_error(&e))?; let values: Vec<&(dyn ToSql + Sync)> = typed_params.iter().map(|(v, _)| *v).collect(); client .execute(&stmt, &values) .await - .map_err(|e| format!("Execute failed: {e}")) + .map_err(|e| format_pg_error(&e)) } /// Run a SELECT with explicit per-placeholder wire types (same rationale as @@ -125,17 +155,17 @@ pub async fn query_typed( let client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pool_error(&e)))?; let types: Vec = typed_params.iter().map(|(_, t)| t.clone()).collect(); let stmt = client .prepare_typed(query, &types) .await - .map_err(|e| format!("Prepare failed: {e}"))?; + .map_err(|e| format_pg_error(&e))?; let values: Vec<&(dyn ToSql + Sync)> = typed_params.iter().map(|(v, _)| *v).collect(); client .query(&stmt, &values) .await - .map_err(|e| format!("Query failed: {e}")) + .map_err(|e| format_pg_error(&e)) } /// Fetch data types for every column in a table as a name -> type map. @@ -363,9 +393,11 @@ async fn build_pool(params: &ConnectionParams) -> Result { /// Format a startup-script execution failure so the surfaced error clearly /// names the startup script as the cause, instead of reading like a bad host -/// or wrong credentials. -fn startup_script_error(err: impl std::fmt::Display) -> String { - format!("Startup script failed: {err}") +/// or wrong credentials. Uses `format_pg_error` so a `DbError` (the common +/// case — a typo in the script) surfaces its real message instead of the +/// generic "db error" fallback. +fn startup_script_error(err: tokio_postgres::Error) -> String { + format!("Startup script failed: {}", format_pg_error(&err)) } /// Build the `post_create` hook that runs the startup script on every new @@ -404,7 +436,7 @@ where let (mut client, connection) = pg_config .connect(tls) .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", format_pg_error(&e)))?; let driver = tokio::spawn(async move { let _ = connection.await; }); diff --git a/src/handlers/blob.rs b/src/handlers/blob.rs index 25d355a..4583a86 100644 --- a/src/handlers/blob.rs +++ b/src/handlers/blob.rs @@ -96,7 +96,8 @@ async fn fetch_blob_bytes( let rows = client::query_typed(conn_params, &query, &typed_params).await?; let row = rows.first().ok_or_else(|| "Row not found".to_string())?; - row.try_get::<_, Vec>(0).map_err(|e| e.to_string()) + row.try_get::<_, Vec>(0) + .map_err(|e| client::format_pg_error(&e)) } /// Sanity-check `file_path` before spending a DB round-trip on a write that's diff --git a/src/handlers/query.rs b/src/handlers/query.rs index 5eeacc9..0a0228d 100644 --- a/src/handlers/query.rs +++ b/src/handlers/query.rs @@ -50,13 +50,23 @@ pub async fn execute_query_batch(id: Value, params: &Value) -> Value { }; let pg_client = match pool.get().await { Ok(c) => c, - Err(e) => return error_response(id, -32603, &format!("Connection failed: {e}")), + Err(e) => { + return error_response( + id, + -32603, + &format!("Connection failed: {}", client::format_pool_error(&e)), + ) + } }; if let Some(s) = schema { let set_path = format!("SET search_path TO \"{}\"", s.replace('"', "\"\"")); if let Err(e) = pg_client.batch_execute(&set_path).await { - return error_response(id, -32603, &format!("Failed to set search_path: {e}")); + return error_response( + id, + -32603, + &format!("Failed to set search_path: {}", client::format_pg_error(&e)), + ); } } @@ -128,7 +138,7 @@ async fn exec_query( let pg_client = pool .get() .await - .map_err(|e| format!("Connection failed: {e}"))?; + .map_err(|e| format!("Connection failed: {}", client::format_pool_error(&e)))?; // Set search_path if schema is specified if let Some(s) = schema { @@ -136,7 +146,7 @@ async fn exec_query( pg_client .batch_execute(&set_path) .await - .map_err(|e| format!("Failed to set search_path: {e}"))?; + .map_err(|e| format!("Failed to set search_path: {}", client::format_pg_error(&e)))?; } exec_query_on_client(&pg_client, query, limit, page).await @@ -154,7 +164,7 @@ async fn exec_query_on_client( let affected = pg_client .execute(query, &[]) .await - .map_err(|e| format!("{e}"))?; + .map_err(|e| client::format_pg_error(&e))?; return Ok(json!({ "columns": [], "rows": [], @@ -177,7 +187,7 @@ async fn exec_query_on_client( let rows = pg_client .query(&final_query, &[]) .await - .map_err(|e| format!("{e}"))?; + .map_err(|e| client::format_pg_error(&e))?; if rows.is_empty() { // Get columns from the statement if possible diff --git a/tests/live_db.rs b/tests/live_db.rs index ba0249b..c81c736 100644 --- a/tests/live_db.rs +++ b/tests/live_db.rs @@ -147,6 +147,36 @@ fn execute_query_returns_rows_from_live_database() { assert_eq!(rows[0][0], json!(1)); } +// Coverage for #66: `tokio_postgres::Error`'s own `Display` impl prints the +// generic "db error" string for any server-side error (its `Kind::Db` arm), +// throwing away the real message in the wrapped `DbError`. `exec_query_on_client` +// previously stringified the error with `format!("{e}")` directly instead of +// checking `as_db_error()` first, so every query error (a syntax error, a +// missing column, a constraint violation) surfaced as the unhelpful literal +// "db error" — see issue #66. +#[test] +fn query_syntax_error_surfaces_the_real_postgres_message_not_generic_db_error() { + let mut plugin = Plugin::spawn(); + let response = plugin.call( + "execute_query", + json!({ "params": conn_params(), "query": "select foo" }), + ); + let error = response + .get("error") + .and_then(|e| e.get("message")) + .and_then(Value::as_str) + .expect("an invalid query must produce a JSON-RPC error"); + assert_ne!( + error, "db error", + "error message must surface the real PostgreSQL error, not the generic \ + tokio_postgres::Error::Display fallback" + ); + assert!( + error.contains("foo"), + "error message should mention the offending identifier, got: {error}" + ); +} + #[test] fn insert_record_persists_a_row() { let mut plugin = Plugin::spawn(); @@ -310,6 +340,15 @@ fn broken_startup_script_fails_fast_with_clear_attribution() { error.starts_with("Startup script failed:"), "error should be clearly attributed to the startup script, got: {error}" ); + // Coverage for #66: startup_script_error previously stringified the + // tokio_postgres::Error directly, so a DbError (a syntax error in the + // script, the common case) collapsed to the generic "db error" instead + // of the real PostgreSQL message. + assert!( + !error.contains("db error") && error.contains("syntax error"), + "error should surface the real PostgreSQL syntax error, not the generic \ + tokio_postgres::Error::Display fallback, got: {error}" + ); } // Coverage for #43: build_pool never called cfg.ssl_mode(...), so @@ -331,3 +370,47 @@ fn ssl_mode_require_fails_against_a_server_without_tls() { "ssl_mode=require must fail against a server with no TLS, not silently connect over plaintext" ); } + +// Coverage for #66: the connection-establishment handshake itself (bad +// database name, bad password) surfaces as a tokio_postgres::Error wrapped +// in deadpool_postgres::PoolError::Backend, from pool.get() — the same +// Kind::Db/"db error" pitfall as query execution, just one layer deeper. +// Every "Connection failed: {e}" call site previously stringified the +// PoolError directly instead of unwrapping to the inner DbError. +#[test] +fn connecting_to_a_nonexistent_database_surfaces_the_real_postgres_message() { + let mut plugin = Plugin::spawn(); + let mut params = conn_params(); + params["database"] = json!("this_database_does_not_exist_xyz"); + + let response = plugin.call("test_connection", json!({ "params": params })); + let error = response + .get("error") + .and_then(|e| e.get("message")) + .and_then(Value::as_str) + .expect("connecting to a nonexistent database must produce a JSON-RPC error"); + assert!( + !error.contains("db error") && error.contains("does not exist"), + "error should surface the real PostgreSQL message, not the generic \ + tokio_postgres::Error::Display fallback, got: {error}" + ); +} + +#[test] +fn connecting_with_a_wrong_password_surfaces_the_real_postgres_message() { + let mut plugin = Plugin::spawn(); + let mut params = conn_params(); + params["password"] = json!("definitely_wrong_password"); + + let response = plugin.call("test_connection", json!({ "params": params })); + let error = response + .get("error") + .and_then(|e| e.get("message")) + .and_then(Value::as_str) + .expect("a wrong password must produce a JSON-RPC error"); + assert!( + !error.contains("db error") && error.contains("password authentication failed"), + "error should surface the real PostgreSQL message, not the generic \ + tokio_postgres::Error::Display fallback, got: {error}" + ); +}