From 9384161b3ccf099ce6fd0cc18af9b07b78bc8048 Mon Sep 17 00:00:00 2001 From: Hatta Zainal Date: Fri, 14 Aug 2026 22:57:50 +0800 Subject: [PATCH] fix(control): classify native error frames authored as a bare SQLSTATE Native error frames carry the stable numeric NodeDB code so the client can rebuild a typed error; a frame that ships ndb_code == 0 collapses on arrival into a generic internal failure, so is_not_found(), is_auth_denied() and is_retriable() all answer wrongly for it. Frames rendered from a classified Error already carried their code. The ones that never held an Error did not: a DDL refusal (DdlError is authored as a SQLSTATE plus a message in ~600 places and has no numeric code to carry), and the session and dispatch guards that reject a request with a literal SQLSTATE. DROP TABLE naming an absent collection therefore reached the client as an internal failure while the identical SELECT arrived typed. Adds sqlstate_code, a table from SQLSTATE to numeric code, used only where the SQLSTATE is the sole classification the server ever produced. It is the inverse of the client-side rule in NodeDbError::from_wire and deliberately so: there every SQLSTATE arrives through one funnel and 23505 cannot be resolved back to a unique violation or a duplicate idempotency key, while here the lookup runs at the site that chose the SQLSTATE. Overloaded SQLSTATEs (53400, 0A000), those with no NodeDB variant (42P07, 42704, 25P02, 3B001) and the deliberately undistinguished credential failures (28P01) map to 0, which is exactly the frame shipped before, so an unmapped SQLSTATE is never worse off. Sites that still hold a classified Error but render a more specific SQLSTATE than the error implies (a plan that cannot be built is 42601, an RLS refusal is 42501) go through error_to_native_with_sqlstate, which keeps the site's SQLSTATE and takes the code from error_classify. The two admission refusals in the auth path pass their code explicitly, since the shared 53400 cannot say whether the database or the tenant pool refused. ILP auth is left alone: every failure there renders as one code and one message so a caller cannot tell a wrong password from an unknown user, and typing them would rebuild the oracle that collapsing removed. --- nodedb-types/src/error/sqlstate.rs | 5 + .../server/native/dispatch/conversion.rs | 85 +++++++++- .../server/native/dispatch/direct_ops.rs | 15 +- .../server/native/dispatch/graph_match.rs | 12 +- .../src/control/server/native/dispatch/mod.rs | 2 +- .../server/native/dispatch/single_task.rs | 4 +- .../src/control/server/native/dispatch/sql.rs | 7 +- .../server/native/dispatch/sql_admin.rs | 3 +- .../server/native/dispatch/sql_loop.rs | 13 +- .../server/native/dispatch/streaming.rs | 2 +- .../server/native/dispatch/transaction.rs | 7 +- .../native/dispatch/transaction_savepoint.rs | 7 +- nodedb/src/control/server/native/mod.rs | 1 + .../src/control/server/native/session/auth.rs | 46 +++-- .../control/server/native/session/request.rs | 23 +-- .../src/control/server/native/session/run.rs | 18 +- .../control/server/native/sqlstate_code.rs | 158 ++++++++++++++++++ .../tests/native_error_code_classification.rs | 68 ++++++++ 18 files changed, 394 insertions(+), 82 deletions(-) create mode 100644 nodedb/src/control/server/native/sqlstate_code.rs diff --git a/nodedb-types/src/error/sqlstate.rs b/nodedb-types/src/error/sqlstate.rs index af330a98a..8f03dcace 100644 --- a/nodedb-types/src/error/sqlstate.rs +++ b/nodedb-types/src/error/sqlstate.rs @@ -90,6 +90,11 @@ pub const TYPE_GUARD_VIOLATION: &str = "23608"; /// `28000` — `invalid_authorization_specification` (no valid credentials) pub const INVALID_AUTHORIZATION: &str = "28000"; +// ── Class 3D — Invalid Catalog Name ────────────────────────────────────────── + +/// `3D000` — `invalid_catalog_name` (the selected database does not exist) +pub const INVALID_CATALOG_NAME: &str = "3D000"; + // ── Class 40 — Transaction Rollback ────────────────────────────────────────── /// `40001` — `serialization_failure` (write conflict; client should retry) diff --git a/nodedb/src/control/server/native/dispatch/conversion.rs b/nodedb/src/control/server/native/dispatch/conversion.rs index 4bf8eeacb..895bedd87 100644 --- a/nodedb/src/control/server/native/dispatch/conversion.rs +++ b/nodedb/src/control/server/native/dispatch/conversion.rs @@ -7,6 +7,7 @@ use nodedb_types::conversion::json_to_value_ref; use nodedb_types::protocol::NativeResponse; use crate::bridge::envelope::Response; +use crate::control::server::native::sqlstate_code::sqlstate_error; use crate::control::server::response_shape::types::ShapedRows; use crate::control::server::shared::ddl::sqlstate::error_code_to_sqlstate; use crate::control::server::shared::ddl::{DdlError, DdlResult}; @@ -52,6 +53,29 @@ pub(crate) fn error_to_native(seq: u64, e: &crate::Error) -> NativeResponse { NativeResponse::error_with_code(seq, code, message, ndb_code) } +/// Convert a Control-Plane error into a native error frame under a SQLSTATE +/// the call site chooses. +/// +/// Same classification as [`error_to_native`] — the numeric code comes from +/// the one `Error` mapping table — but for the guards that render a more +/// specific SQLSTATE than the error's own variant implies: a plan that cannot +/// be built is `42601` to a SQL client whatever its internal cause, and an +/// RLS injection failure is `42501`. Those sites still hold the classified +/// error, so the code must come from it rather than be inferred back out of +/// the SQLSTATE they just chose. +pub(crate) fn error_to_native_with_sqlstate( + seq: u64, + sqlstate: impl Into, + e: &crate::Error, +) -> NativeResponse { + NativeResponse::error_with_code( + seq, + sqlstate, + e.to_string(), + crate::error_classify::classify(e).code().0, + ) +} + /// Convert a `NodeDbError` produced while shaping a response into a /// NativeResponse error frame. /// @@ -93,7 +117,7 @@ pub(crate) fn error_code_to_native( code: Option<&crate::bridge::envelope::ErrorCode>, ) -> NativeResponse { let Some(code) = code else { - return NativeResponse::error(seq, "XX000", "unknown data plane error"); + return sqlstate_error(seq, "XX000", "unknown data plane error"); }; let (_, sqlstate, message) = error_code_to_sqlstate(code); let public = nodedb_types::NodeDbError::from(crate::Error::DataPlane(code.clone())); @@ -108,12 +132,18 @@ pub(crate) fn error_code_to_native( /// row-returning / status / empty result determines the response (a status tag /// becomes a single-column status row, a row result becomes a columns+rows /// frame, an empty result or an empty vec becomes a bare OK). +/// +/// `DdlError` is authored as a SQLSTATE and a message and never holds a +/// classified `Error`, so the numeric code the client rebuilds its typed error +/// from comes from `sqlstate_code`. Without it every DDL refusal — a `DROP +/// TABLE` naming a collection that does not exist, a denied `GRANT` — reaches +/// the client as a generic internal failure. pub(crate) fn ddl_result_to_native( seq: u64, result: Result, DdlError>, ) -> NativeResponse { match result { - Err(DdlError { sqlstate, message }) => NativeResponse::error(seq, sqlstate, message), + Err(DdlError { sqlstate, message }) => sqlstate_error(seq, sqlstate, message), // Unknown pgwire response variants are dropped during translation, so // the first element is the first meaningful result — mirroring the // previous bridge, which returned on the first known variant. @@ -313,6 +343,57 @@ mod tests { ); } + /// A DDL refusal is authored as a SQLSTATE with no `Error` behind it, so + /// its numeric code comes from the SQLSTATE table. Without it the frame + /// ships `ndb_code == 0` and a `DROP TABLE` naming an absent collection + /// arrives as a generic internal failure while the identical `SELECT` + /// arrives typed. + #[test] + fn ddl_refusals_carry_their_numeric_code() { + let response = ddl_result_to_native( + 1, + Err(DdlError { + sqlstate: "42P01".to_owned(), + message: "collection 'missing' does not exist".to_owned(), + }), + ); + + let error = response + .error + .expect("error responses must carry a payload"); + assert_eq!(error.code, "42P01"); + assert_eq!( + error.ndb_code, + nodedb_types::error::ErrorCode::COLLECTION_NOT_FOUND.0 + ); + assert_eq!(error.message, "collection 'missing' does not exist"); + } + + /// A site that renders a more specific SQLSTATE than the error implies + /// must still take the classification from the error rather than from the + /// SQLSTATE it just chose: `42601` is shared by several conditions, while + /// the error in hand names exactly one. + #[test] + fn a_site_chosen_sqlstate_keeps_the_errors_classification() { + let response = error_to_native_with_sqlstate( + 1, + "42601", + &crate::Error::PlanError { + detail: "no such column".to_owned(), + }, + ); + + let error = response + .error + .expect("error responses must carry a payload"); + assert_eq!(error.code, "42601"); + assert_eq!( + error.ndb_code, + nodedb_types::error::ErrorCode::PLAN_ERROR.0, + "the classification must come from the error, not from the SQLSTATE" + ); + } + /// The numeric code is populated for every variant, including the ones /// whose SQLSTATE falls through to `XX000` — otherwise the fix would be a /// per-variant special case rather than one classification. diff --git a/nodedb/src/control/server/native/dispatch/direct_ops.rs b/nodedb/src/control/server/native/dispatch/direct_ops.rs index a3a9588d4..906c9c81f 100644 --- a/nodedb/src/control/server/native/dispatch/direct_ops.rs +++ b/nodedb/src/control/server/native/dispatch/direct_ops.rs @@ -16,7 +16,8 @@ use nodedb_physical::physical_task::{PhysicalTask, PostSetOp}; use super::response::data_plane_response_to_native; use super::single_task::dispatch_single_task; -use super::{DispatchCtx, error_to_native}; +use super::{DispatchCtx, error_to_native, error_to_native_with_sqlstate}; +use crate::control::server::native::sqlstate_code::sqlstate_error; /// Dispatch a direct Data Plane operation by opcode. pub(crate) async fn handle_direct_op( @@ -55,7 +56,7 @@ pub(crate) async fn handle_direct_op( // Per-operation cap enforcement (vector dim, top_k, batch size, etc.). if let Err(e) = super::limits::check_op_limits(ctx.state, fields) { - return NativeResponse::error(seq, "0A000", e.to_string()); + return error_to_native_with_sqlstate(seq, "0A000", &e); } // Quota enforcement — reject before planning or dispatch. @@ -65,7 +66,7 @@ pub(crate) async fn handle_direct_op( let mut plan = match super::plan_builder::build_plan(ctx, op, fields, &collection) { Ok(p) => p, - Err(e) => return NativeResponse::error(seq, "42601", e.to_string()), + Err(e) => return error_to_native_with_sqlstate(seq, "42601", &e), }; // Apply RLS before any special Control-Plane orchestration can observe the plan. @@ -75,7 +76,7 @@ pub(crate) async fn handle_direct_op( &ctx.state.rls, ctx.auth_context(), ) { - return NativeResponse::error(seq, "42501", e.to_string()); + return error_to_native_with_sqlstate(seq, "42501", &e); } // Refuse what column redaction cannot cover (an aggregate over a redacted @@ -86,7 +87,7 @@ pub(crate) async fn handle_direct_op( ctx.auth_context(), &ctx.state.redaction, ) { - return NativeResponse::error(seq, "0A000", e.to_string()); + return error_to_native_with_sqlstate(seq, "0A000", &e); } // Extracted before `plan` is moved/cloned into any of the branches below @@ -107,7 +108,7 @@ pub(crate) async fn handle_direct_op( if let Some(info) = &plan_metering_info && let Err(e) = admit_quota_for_dispatch(ctx.state, &ctx.scope, info) { - return NativeResponse::error(seq, "53400", e.to_string()); + return error_to_native_with_sqlstate(seq, "53400", &e); } // Whether the blanket metering call below (after the block) still needs @@ -328,7 +329,7 @@ pub(crate) async fn handle_direct_op( let task = match authorized_tasks.into_tasks().into_iter().next() { Some(task) => task, None => { - return NativeResponse::error( + return sqlstate_error( seq, "XX000", "authorization returned no task capability", diff --git a/nodedb/src/control/server/native/dispatch/graph_match.rs b/nodedb/src/control/server/native/dispatch/graph_match.rs index 35ec0de31..49f9d92ab 100644 --- a/nodedb/src/control/server/native/dispatch/graph_match.rs +++ b/nodedb/src/control/server/native/dispatch/graph_match.rs @@ -10,7 +10,7 @@ use crate::control::server::shared::quota_admission::admit_quota_for_dispatch; use super::raw_dispatch::dispatch_authorized_single_task; use super::response::data_plane_response_to_native; -use super::{DispatchCtx, error_to_native}; +use super::{DispatchCtx, error_to_native, error_to_native_with_sqlstate}; /// Dispatch a native `GraphMatch` op, unwrapping the DP `{rows, frontier}` /// envelope into a bare rows array before native conversion. @@ -36,7 +36,7 @@ pub(crate) async fn handle_graph_match( let tenant_id = ctx.tenant_id(); if let Err(error) = super::limits::check_op_limits(ctx.state, fields) { - return NativeResponse::error(seq, "0A000", error.to_string()); + return error_to_native_with_sqlstate(seq, "0A000", &error); } if let Err(error) = ctx.state.check_tenant_quota(tenant_id) { return error_to_native(seq, &error); @@ -45,7 +45,7 @@ pub(crate) async fn handle_graph_match( let mut plan = match super::plan_builder::build_plan(ctx, OpCode::GraphMatch, fields, &collection) { Ok(plan) => plan, - Err(error) => return NativeResponse::error(seq, "42601", error.to_string()), + Err(error) => return error_to_native_with_sqlstate(seq, "42601", &error), }; if let Err(error) = crate::control::planner::rls_injection::inject_rls_for_single_plan( tenant_id.as_u64(), @@ -53,7 +53,7 @@ pub(crate) async fn handle_graph_match( &ctx.state.rls, ctx.auth_context(), ) { - return NativeResponse::error(seq, "42501", error.to_string()); + return error_to_native_with_sqlstate(seq, "42501", &error); } // Refuse what column redaction cannot cover: a MATCH returns graph // topology, which the result-path masking hook has no columns to rewrite. @@ -63,7 +63,7 @@ pub(crate) async fn handle_graph_match( ctx.auth_context(), &ctx.state.redaction, ) { - return NativeResponse::error(seq, "0A000", error.to_string()); + return error_to_native_with_sqlstate(seq, "0A000", &error); } // Stamp the active transaction id so MATCH reads resolve this connection's @@ -84,7 +84,7 @@ pub(crate) async fn handle_graph_match( if let Some(info) = &plan_metering_info && let Err(e) = admit_quota_for_dispatch(ctx.state, &ctx.scope, info) { - return NativeResponse::error(seq, "53400", e.to_string()); + return error_to_native_with_sqlstate(seq, "53400", &e); } let _request = ctx.state.tenant_request_guard(tenant_id); let raw = dispatch_authorized_single_task(ctx, tenant_id, vshard_id, plan, txn_id).await; diff --git a/nodedb/src/control/server/native/dispatch/mod.rs b/nodedb/src/control/server/native/dispatch/mod.rs index 2bb6f10ac..279bc151a 100644 --- a/nodedb/src/control/server/native/dispatch/mod.rs +++ b/nodedb/src/control/server/native/dispatch/mod.rs @@ -28,7 +28,7 @@ pub(crate) use admission_op::admission_operation; pub(crate) use auth::{NativeAuthOutcome, handle_auth, handle_ping}; pub(crate) use conversion::{ ddl_result_to_native, error_code_to_native, error_response_to_native, error_to_native, - shape_error_to_native, to_native_columns_rows, + error_to_native_with_sqlstate, shape_error_to_native, to_native_columns_rows, }; pub(crate) use ctx::DispatchCtx; pub(crate) use direct_ops::handle_direct_op; diff --git a/nodedb/src/control/server/native/dispatch/single_task.rs b/nodedb/src/control/server/native/dispatch/single_task.rs index 846f8afb7..5f592a108 100644 --- a/nodedb/src/control/server/native/dispatch/single_task.rs +++ b/nodedb/src/control/server/native/dispatch/single_task.rs @@ -16,7 +16,7 @@ use crate::types::{Lsn, RequestId}; use super::raw_dispatch::dispatch_authorized_single_task; use super::response::data_plane_response_to_native; -use super::{DispatchCtx, error_code_to_native, error_to_native}; +use super::{DispatchCtx, error_code_to_native, error_to_native, error_to_native_with_sqlstate}; /// Dispatch one plan via the gateway (when wired) or the local SPSC path, /// converting the Data-Plane response into a `NativeResponse`. @@ -76,7 +76,7 @@ pub(super) async fn dispatch_single_task( if let Some(info) = &plan_metering_info && let Err(e) = admit_quota_for_dispatch(ctx.state, &ctx.scope, info) { - return NativeResponse::error(seq, "53400", e.to_string()); + return error_to_native_with_sqlstate(seq, "53400", &e); } let task = match route_in_tx_write( diff --git a/nodedb/src/control/server/native/dispatch/sql.rs b/nodedb/src/control/server/native/dispatch/sql.rs index 6871df24c..8aead8a32 100644 --- a/nodedb/src/control/server/native/dispatch/sql.rs +++ b/nodedb/src/control/server/native/dispatch/sql.rs @@ -27,6 +27,7 @@ use super::transaction_savepoint::{ handle_release_savepoint, handle_rollback_to_savepoint, handle_savepoint, }; use super::{DispatchCtx, error_to_native}; +use crate::control::server::native::sqlstate_code::sqlstate_error; /// Handle a SQL statement: transaction control, SET/SHOW, DDL, or DataFusion. /// @@ -110,7 +111,7 @@ async fn handle_sql_inner( } if ctx.sessions.transaction_state(ctx.peer_addr) == TransactionState::Failed { - return resp(NativeResponse::error( + return resp(sqlstate_error( seq, "25P02", "current transaction is aborted, commands ignored until end of transaction block", @@ -361,7 +362,7 @@ async fn execute_planned( match try_open_sql_stream(ctx, seq, &tasks, database_id, Some(&output_schema)).await { Ok(Some(mut stream)) => { let Some(scope) = lease_scope.take() else { - return resp(NativeResponse::error( + return resp(sqlstate_error( seq, "XX000", "internal error: query lease scope missing before SQL stream dispatch", @@ -382,7 +383,7 @@ async fn execute_planned( // after this local owner is dropped. Lazy streams above retain the raw // scope directly in their stream owner. let Some(lease_scope) = lease_scope.take() else { - return resp(NativeResponse::error( + return resp(sqlstate_error( seq, "XX000", "internal error: query lease scope missing before materialized SQL dispatch", diff --git a/nodedb/src/control/server/native/dispatch/sql_admin.rs b/nodedb/src/control/server/native/dispatch/sql_admin.rs index 9f78638b6..ac3df3a72 100644 --- a/nodedb/src/control/server/native/dispatch/sql_admin.rs +++ b/nodedb/src/control/server/native/dispatch/sql_admin.rs @@ -9,6 +9,7 @@ use nodedb_types::protocol::NativeResponse; use nodedb_types::value::Value; use super::{DispatchCtx, error_to_native}; +use crate::control::server::native::sqlstate_code::sqlstate_error; // ─── SET / SHOW / RESET (SQL form) ───────────────────────────────── @@ -36,7 +37,7 @@ pub(super) fn handle_set_sql(ctx: &DispatchCtx<'_>, seq: u64, sql: &str) -> Nati .to_string(), ) } else { - return NativeResponse::error(seq, "42601", "invalid SET syntax"); + return sqlstate_error(seq, "42601", "invalid SET syntax"); }; ctx.sessions.set_parameter(ctx.peer_addr, key, value); diff --git a/nodedb/src/control/server/native/dispatch/sql_loop.rs b/nodedb/src/control/server/native/dispatch/sql_loop.rs index 9700d4025..e1f4483e0 100644 --- a/nodedb/src/control/server/native/dispatch/sql_loop.rs +++ b/nodedb/src/control/server/native/dispatch/sql_loop.rs @@ -32,8 +32,9 @@ use super::sql_dispatch_task::dispatch_task; use super::streaming::SqlOutcome; use super::{ DispatchCtx, error_code_to_native, error_response_to_native, error_to_native, - shape_error_to_native, to_native_columns_rows, + error_to_native_with_sqlstate, shape_error_to_native, to_native_columns_rows, }; +use crate::control::server::native::sqlstate_code::sqlstate_error; /// Wrap a materialized response as a non-streaming [`SqlOutcome`]. #[inline] @@ -69,11 +70,7 @@ pub(super) async fn run_dispatch_loop( for task in tasks { if task.tenant_id != ctx.tenant_id() { - return resp(NativeResponse::error( - seq, - "42501", - "tenant isolation violation", - )); + return resp(sqlstate_error(seq, "42501", "tenant isolation violation")); } // Cloned before `route_in_tx_write` consumes `task`, so a staged @@ -101,7 +98,7 @@ pub(super) async fn run_dispatch_loop( if let Some(info) = &plan_metering_info && let Err(e) = admit_quota_for_dispatch(ctx.state, &ctx.scope, info) { - return resp(NativeResponse::error(seq, "53400", e.to_string())); + return resp(error_to_native_with_sqlstate(seq, "53400", &e)); } // In transaction: route through the protocol-neutral staging gate. @@ -154,7 +151,7 @@ pub(super) async fn run_dispatch_loop( Arc::clone(&plan_lease_scope), ) { - return resp(NativeResponse::error( + return resp(sqlstate_error( seq, "XX000", "internal error: failed to retain descriptor leases for buffered transaction tasks", diff --git a/nodedb/src/control/server/native/dispatch/streaming.rs b/nodedb/src/control/server/native/dispatch/streaming.rs index b3c8bf329..ea67d6c75 100644 --- a/nodedb/src/control/server/native/dispatch/streaming.rs +++ b/nodedb/src/control/server/native/dispatch/streaming.rs @@ -47,7 +47,7 @@ impl SqlOutcome { pub(crate) fn into_response(self) -> NativeResponse { match self { SqlOutcome::Response(r) => *r, - SqlOutcome::Stream(s) => NativeResponse::error( + SqlOutcome::Stream(s) => crate::control::server::native::sqlstate_code::sqlstate_error( s.seq, "XX000", "internal error: SQL stream produced on a non-streaming path", diff --git a/nodedb/src/control/server/native/dispatch/transaction.rs b/nodedb/src/control/server/native/dispatch/transaction.rs index a01e587ac..415cb8be3 100644 --- a/nodedb/src/control/server/native/dispatch/transaction.rs +++ b/nodedb/src/control/server/native/dispatch/transaction.rs @@ -81,7 +81,12 @@ pub(crate) fn handle_begin(ctx: &DispatchCtx<'_>, seq: u64) -> NativeResponse { crate::Error::BadRequest { detail } => detail.clone(), other => other.to_string(), }; - NativeResponse::error(seq, "25P02", message) + NativeResponse::error_with_code( + seq, + "25P02", + message, + crate::error_classify::classify(&e).code().0, + ) } } } diff --git a/nodedb/src/control/server/native/dispatch/transaction_savepoint.rs b/nodedb/src/control/server/native/dispatch/transaction_savepoint.rs index e1eb43113..4a304cd92 100644 --- a/nodedb/src/control/server/native/dispatch/transaction_savepoint.rs +++ b/nodedb/src/control/server/native/dispatch/transaction_savepoint.rs @@ -15,18 +15,17 @@ use crate::control::server::shared::session::savepoint_ops::{self, SavepointErro use super::DispatchCtx; use super::transaction::NativeTxnDp; +use crate::control::server::native::sqlstate_code::sqlstate_error; /// Map a neutral savepoint error to a native error frame. fn savepoint_error_to_native(seq: u64, e: &SavepointError) -> NativeResponse { match e { - SavepointError::NoActiveTransaction => NativeResponse::error( + SavepointError::NoActiveTransaction => sqlstate_error( seq, "25P01", "SAVEPOINT can only be used in transaction blocks", ), - SavepointError::NotFound { message } => { - NativeResponse::error(seq, "3B001", message.clone()) - } + SavepointError::NotFound { message } => sqlstate_error(seq, "3B001", message.clone()), } } diff --git a/nodedb/src/control/server/native/mod.rs b/nodedb/src/control/server/native/mod.rs index 9d423721f..a641e529c 100644 --- a/nodedb/src/control/server/native/mod.rs +++ b/nodedb/src/control/server/native/mod.rs @@ -4,3 +4,4 @@ pub mod codec; pub mod dispatch; pub mod handshake; pub mod session; +pub mod sqlstate_code; diff --git a/nodedb/src/control/server/native/session/auth.rs b/nodedb/src/control/server/native/session/auth.rs index 27e034a82..591972581 100644 --- a/nodedb/src/control/server/native/session/auth.rs +++ b/nodedb/src/control/server/native/session/auth.rs @@ -12,6 +12,8 @@ use crate::control::server::shared::authorization::authorize_database; use super::NativeSession; use super::dispatch; +use crate::control::server::native::dispatch::error_to_native_with_sqlstate; +use crate::control::server::native::sqlstate_code::sqlstate_error; impl NativeSession { /// Handle authentication request. @@ -22,7 +24,7 @@ impl NativeSession { // a client silently swap to a different (database, tenant) scope while // still holding the original scope's connection slots. if self.identity.is_some() || self.connection_permit.is_some() { - return NativeResponse::error( + return sqlstate_error( seq, "0A000", "already authenticated; reconnect to switch identity", @@ -33,11 +35,11 @@ impl NativeSession { RequestFields::Text(f) => match &f.auth { Some(a) => a, None => { - return NativeResponse::error(seq, "28000", "missing 'auth' field"); + return sqlstate_error(seq, "28000", "missing 'auth' field"); } }, _ => { - return NativeResponse::error(seq, "0A000", "unsupported request fields variant"); + return sqlstate_error(seq, "0A000", "unsupported request fields variant"); } }; @@ -65,7 +67,7 @@ impl NativeSession { self.transport, &self.peer_addr.to_string(), ) { - return NativeResponse::error(seq, "28000", format!("{e}")); + return error_to_native_with_sqlstate(seq, "28000", &e); } // Bind the requested database before acquiring any scoped @@ -80,18 +82,14 @@ impl NativeSession { Some(name) => match catalog.get_database_id_by_name(name) { Ok(Some(db_id)) => db_id, Ok(None) => { - return NativeResponse::error( + return sqlstate_error( seq, "3D000", "selected database does not exist", ); } Err(_) => { - return NativeResponse::error( - seq, - "XX000", - "database catalog lookup failed", - ); + return sqlstate_error(seq, "XX000", "database catalog lookup failed"); } }, None => identity @@ -105,18 +103,10 @@ impl NativeSession { match catalog.get_database(db_id) { Ok(Some(_)) => {} Ok(None) => { - return NativeResponse::error( - seq, - "3D000", - "selected database does not exist", - ); + return sqlstate_error(seq, "3D000", "selected database does not exist"); } Err(_) => { - return NativeResponse::error( - seq, - "XX000", - "database catalog lookup failed", - ); + return sqlstate_error(seq, "XX000", "database catalog lookup failed"); } } @@ -124,7 +114,7 @@ impl NativeSession { // database- or tenant-scoped capacity or mutate session state. let audit = ArcAuditEmitter(std::sync::Arc::clone(&self.state.audit)); if authorize_database(&identity, db_id, &audit).is_err() { - return NativeResponse::error(seq, "42501", "permission denied for database"); + return sqlstate_error(seq, "42501", "permission denied for database"); } // Phase 2 admission: acquire per-database and per-tenant permits @@ -134,10 +124,11 @@ impl NativeSession { let db_permit = match self.admission_registry.try_acquire_database(db_id) { Ok(p) => p, Err(e) => { - return NativeResponse::error( + return NativeResponse::error_with_code( seq, nodedb_types::error::sqlstate::QUOTA_EXCEEDED, format!("{e}"), + nodedb_types::error::ErrorCode::DATABASE_QUOTA_EXCEEDED.0, ); } }; @@ -147,10 +138,11 @@ impl NativeSession { Err(e) => { // db_permit is dropped here, releasing the DB slot. drop(db_permit); - return NativeResponse::error( + return NativeResponse::error_with_code( seq, nodedb_types::error::sqlstate::QUOTA_EXCEEDED, format!("{e}"), + nodedb_types::error::ErrorCode::TENANT_QUOTA_EXCEEDED.0, ); } }; @@ -165,7 +157,7 @@ impl NativeSession { // don't leak slots into the per-DB / per-tenant pools. drop(tenant_permit); drop(db_permit); - return NativeResponse::error( + return sqlstate_error( seq, "XX000", "internal error: global admission permit missing during auth assembly", @@ -229,12 +221,12 @@ impl NativeSession { // other auth error (wrong password, lockout, unknown user) stays // collapsed into the generic invalid-password 28P01 so none can be // distinguished from the others. - Err(e @ crate::Error::RateExceeded { .. }) => NativeResponse::error( + Err(e @ crate::Error::RateExceeded { .. }) => error_to_native_with_sqlstate( seq, nodedb_types::error::sqlstate::TOO_MANY_CONNECTIONS, - format!("{e}"), + &e, ), - Err(e) => NativeResponse::error(seq, "28P01", format!("{e}")), + Err(e) => sqlstate_error(seq, "28P01", format!("{e}")), } } } diff --git a/nodedb/src/control/server/native/session/request.rs b/nodedb/src/control/server/native/session/request.rs index 994f16cac..ad81dfadd 100644 --- a/nodedb/src/control/server/native/session/request.rs +++ b/nodedb/src/control/server/native/session/request.rs @@ -8,6 +8,7 @@ use nodedb_types::protocol::{AuthMethod, NativeResponse, OpCode, RequestFields, use super::NativeSession; use super::dispatch::{self, DispatchCtx}; use crate::config::auth::AuthMode; +use crate::control::server::native::sqlstate_code::sqlstate_error; impl NativeSession { /// Route a decoded request to the appropriate handler. @@ -62,7 +63,7 @@ impl NativeSession { let Some(trust_id) = super::super::super::session_auth::configured_trust_identity(&self.state) else { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "28000", "configured trust identity is unavailable", @@ -88,7 +89,7 @@ impl NativeSession { return SqlOutcome::Response(Box::new(auth_response)); } } else { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "28000", "not authenticated. Send Auth request first.", @@ -99,7 +100,7 @@ impl NativeSession { let identity = match self.identity.as_ref() { Some(id) => id, None => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "28000", "not authenticated", @@ -199,7 +200,7 @@ impl NativeSession { let fields = match &req.fields { RequestFields::Text(f) => f, _ => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "0A000", "unsupported request field format for this server version", @@ -213,7 +214,7 @@ impl NativeSession { let sql = match &fields.sql { Some(s) => s.as_str(), None => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'sql' field", @@ -239,7 +240,7 @@ impl NativeSession { dispatch::handle_sql(&ctx, seq, sql, None).await, )); } - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'key' field", @@ -258,7 +259,7 @@ impl NativeSession { dispatch::handle_sql(&ctx, seq, sql, None).await, )); } - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'key' field", @@ -271,7 +272,7 @@ impl NativeSession { let key = match &fields.key { Some(k) => k.as_str(), None => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'key' field", @@ -291,7 +292,7 @@ impl NativeSession { let sql = match &fields.sql { Some(s) => s.as_str(), None => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'sql' field", @@ -380,7 +381,7 @@ impl NativeSession { let sql = match &fields.sql { Some(s) => s.as_str(), None => { - return SqlOutcome::Response(Box::new(NativeResponse::error( + return SqlOutcome::Response(Box::new(sqlstate_error( seq, "42601", "missing 'sql' field", @@ -394,7 +395,7 @@ impl NativeSession { OpCode::Auth | OpCode::Ping | OpCode::Status => unreachable!(), // OpCode is #[non_exhaustive]; future opcodes that reach this // handler before session.rs is updated return a typed error. - _ => NativeResponse::error(seq, "0A000", "opcode not supported by this server version"), + _ => sqlstate_error(seq, "0A000", "opcode not supported by this server version"), }; SqlOutcome::Response(Box::new(response)) diff --git a/nodedb/src/control/server/native/session/run.rs b/nodedb/src/control/server/native/session/run.rs index 712d63b57..90ad7a339 100644 --- a/nodedb/src/control/server/native/session/run.rs +++ b/nodedb/src/control/server/native/session/run.rs @@ -18,6 +18,7 @@ use super::NativeSession; use super::codec::{self, FrameFormat}; use super::dispatch; use super::session_chunk::chunk_large_response; +use crate::control::server::native::sqlstate_code::sqlstate_error; /// Rollback dependencies retained independently from the connection future. /// @@ -187,11 +188,8 @@ impl NativeSession { "session absolute timeout ({}s), closing connection", absolute_timeout_secs ); - let shutdown_resp = NativeResponse::error( - 0, - "57P01", - "session timeout: absolute lifetime exceeded", - ); + let shutdown_resp = + sqlstate_error(0, "57P01", "session timeout: absolute lifetime exceeded"); if let Ok(bytes) = super::codec::encode_response( &shutdown_resp, self.format.unwrap_or(FrameFormat::MessagePack), @@ -224,8 +222,12 @@ impl NativeSession { Ok(None) => return Ok(()), // clean EOF Err(crate::Error::BadRequest { detail }) => { // Send a typed error before closing so the client knows why. - let err_resp = - NativeResponse::error(0, "54000", format!("frame rejected: {detail}")); + let err_resp = NativeResponse::error_with_code( + 0, + "54000", + format!("frame rejected: {detail}"), + nodedb_types::error::ErrorCode::BAD_REQUEST.0, + ); let format = self.format.unwrap_or(FrameFormat::MessagePack); if let Ok(bytes) = codec::encode_response(&err_resp, format) { let _ = codec::write_frame(&mut self.stream, &bytes).await; @@ -248,7 +250,7 @@ impl NativeSession { // Decode and handle. let outcome = match codec::decode_request(&payload, format) { Ok(req) => self.handle_request(req).await, - Err(e) => dispatch::SqlOutcome::Response(Box::new(NativeResponse::error( + Err(e) => dispatch::SqlOutcome::Response(Box::new(sqlstate_error( 0, "42601", format!("{e}"), diff --git a/nodedb/src/control/server/native/sqlstate_code.rs b/nodedb/src/control/server/native/sqlstate_code.rs new file mode 100644 index 000000000..e22ac5196 --- /dev/null +++ b/nodedb/src/control/server/native/sqlstate_code.rs @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Numeric NodeDB codes for native error frames authored as a bare SQLSTATE. +//! +//! A native error frame carries both a SQLSTATE and the stable numeric NodeDB +//! code, and the client rebuilds its typed error from the number: a frame that +//! ships `ndb_code == 0` collapses on arrival into a generic internal failure, +//! so `is_not_found()`, `is_auth_denied()` and `is_retriable()` all answer +//! wrongly for it. +//! +//! Most frames get their number from [`crate::error_classify::classify`], +//! which is the one internal-`Error`-to-public mapping the crate owns. This +//! module exists for the frames that never held an `Error` to classify: a DDL +//! refusal ([`DdlError`](crate::control::server::shared::ddl::DdlError) is +//! authored as a SQLSTATE plus a message, in ~600 places, and has no numeric +//! code to carry), and the session/dispatch guards that reject a request with +//! a literal SQLSTATE and a static message. For those the SQLSTATE *is* the +//! only classification the server ever produced, so reading it back is a +//! lookup rather than a guess. +//! +//! This is the inverse of the client-side rule in +//! `NodeDbError::from_wire`, and deliberately so. There, every SQLSTATE the +//! server can emit arrives through one funnel, so a reverse mapping would have +//! to resolve `23505` into either a unique violation or a duplicate +//! idempotency key with no way to tell them apart. Here the lookup happens at +//! the site that chose the SQLSTATE, and the table only carries SQLSTATEs +//! whose NodeDB classification is unambiguous *whatever* site emitted them. +//! +//! Everything else maps to `0`, which is exactly the frame today's code ships, +//! so an unmapped SQLSTATE is never worse off than before this table existed. +//! Three groups stay unmapped on purpose: +//! +//! - **Overloaded SQLSTATEs.** `53400` is `QUOTA_OVERCOMMIT`, +//! `TENANT_QUOTA_EXCEEDED`, `DATABASE_QUOTA_EXCEEDED` and `SERVER_OVERLOAD`; +//! `0A000` is `SQL_NOT_ENABLED`, `CANNOT_DROP_DEFAULT_DATABASE` and +//! `CANNOT_CLONE_MIRROR`. A caller that knows which one it is passes the +//! code explicitly instead of routing through this table. +//! - **SQLSTATEs with no NodeDB variant.** `42P07` (duplicate table), `42704` +//! (undefined object), `25P02` (aborted transaction), `3B001` (no such +//! savepoint). These need new `ErrorCode`/`ErrorDetails` variants to type at +//! all, which is a public-API change tracked separately. +//! - **SQLSTATEs that are deliberately undistinguished.** Every credential +//! failure renders as `28P01` and every ILP auth failure as a single code +//! with one message, precisely so a caller cannot tell a wrong password from +//! an unknown user. Typing them would rebuild the oracle that collapsing +//! removed. + +use nodedb_types::error::{ErrorCode, sqlstate}; +use nodedb_types::protocol::NativeResponse; + +/// The numeric NodeDB code a bare `sqlstate` classifies to, or `0` when it +/// carries no unambiguous classification. +pub(crate) fn ndb_code_for_sqlstate(sqlstate_str: &str) -> u16 { + let code = match sqlstate_str { + sqlstate::UNDEFINED_TABLE => ErrorCode::COLLECTION_NOT_FOUND, + sqlstate::INVALID_CATALOG_NAME => ErrorCode::DATABASE_NOT_FOUND, + sqlstate::INSUFFICIENT_PRIVILEGE => ErrorCode::AUTHORIZATION_DENIED, + sqlstate::UNDEFINED_FUNCTION => ErrorCode::UNDEFINED_FUNCTION, + // Both a malformed request and a plan that cannot be built render as + // `42601`, so this cannot say which. It does not have to: the two + // differ in which side wrote the bad statement, not in how a client + // must react, and both `BadRequest` and `PlanError` are client errors + // that no caller should retry. + sqlstate::SYNTAX_ERROR => ErrorCode::BAD_REQUEST, + // A cross-shard OCC abort and a retryable refusal both mean "nothing + // applied, retry the whole thing" — the same contract `WriteConflict` + // states, and the classification a retry loop reads. + sqlstate::SERIALIZATION_FAILURE => ErrorCode::WRITE_CONFLICT, + sqlstate::QUERY_CANCELED => ErrorCode::DEADLINE_EXCEEDED, + sqlstate::TOO_MANY_CONNECTIONS => ErrorCode::RATE_EXCEEDED, + sqlstate::INTERNAL_ERROR => ErrorCode::INTERNAL, + _ => return 0, + }; + code.0 +} + +/// Build a native error frame from a bare SQLSTATE, classifying it through +/// [`ndb_code_for_sqlstate`]. +/// +/// Use this wherever a site rejects a request with a literal SQLSTATE and no +/// `Error` value. A site that holds an `Error` must use +/// `error_to_native` / `error_to_native_with_sqlstate` instead: those read the +/// classification the error already carries rather than inferring one. +pub(crate) fn sqlstate_error( + seq: u64, + sqlstate_str: impl Into, + message: impl Into, +) -> NativeResponse { + let sqlstate_str = sqlstate_str.into(); + let ndb_code = ndb_code_for_sqlstate(&sqlstate_str); + NativeResponse::error_with_code(seq, sqlstate_str, message, ndb_code) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn classified_sqlstates_carry_their_code() { + assert_eq!( + ndb_code_for_sqlstate("42P01"), + ErrorCode::COLLECTION_NOT_FOUND.0 + ); + assert_eq!( + ndb_code_for_sqlstate("42501"), + ErrorCode::AUTHORIZATION_DENIED.0 + ); + assert_eq!(ndb_code_for_sqlstate("42601"), ErrorCode::BAD_REQUEST.0); + assert_eq!( + ndb_code_for_sqlstate("3D000"), + ErrorCode::DATABASE_NOT_FOUND.0 + ); + assert_eq!(ndb_code_for_sqlstate("XX000"), ErrorCode::INTERNAL.0); + } + + /// A retry loop reads the numeric code, so the SQLSTATE the server sends + /// precisely to get a transaction retried must not arrive unclassified. + #[test] + fn serialization_failure_stays_retriable() { + let frame = sqlstate_error(1, "40001", "OCC abort"); + let payload = frame.error.expect("error frames carry a payload"); + assert_eq!(payload.ndb_code, ErrorCode::WRITE_CONFLICT.0); + assert!( + nodedb_types::NodeDbError::from_wire(ErrorCode(payload.ndb_code), payload.message) + .is_retriable() + ); + } + + /// An overloaded or unmapped SQLSTATE must fall through to `0` rather than + /// pick a side: `0` is what the frame ships today, so an unknown SQLSTATE + /// is no worse off, while a wrong guess would misreport retriability. + #[test] + fn ambiguous_and_unknown_sqlstates_stay_unclassified() { + // Overloaded across several NodeDB variants. + assert_eq!(ndb_code_for_sqlstate("53400"), 0); + assert_eq!(ndb_code_for_sqlstate("0A000"), 0); + // No NodeDB variant exists to map onto. + assert_eq!(ndb_code_for_sqlstate("42P07"), 0); + assert_eq!(ndb_code_for_sqlstate("42704"), 0); + // Deliberately undistinguished so credential failures stay opaque. + assert_eq!(ndb_code_for_sqlstate("28P01"), 0); + assert_eq!(ndb_code_for_sqlstate("28000"), 0); + // Not a SQLSTATE this server emits. + assert_eq!(ndb_code_for_sqlstate("99999"), 0); + } + + /// An unclassified frame must still reach the client exactly as it does + /// today — same SQLSTATE, same message, `ndb_code == 0` — so adding the + /// table cannot regress a path it does not cover. + #[test] + fn unclassified_frame_is_unchanged() { + let frame = sqlstate_error(7, "42P07", "table 'repro_t' already exists"); + let payload = frame.error.expect("error frames carry a payload"); + assert_eq!(payload.code, "42P07"); + assert_eq!(payload.message, "table 'repro_t' already exists"); + assert_eq!(payload.ndb_code, 0); + } +} diff --git a/nodedb/tests/native_error_code_classification.rs b/nodedb/tests/native_error_code_classification.rs index 88f8c18a5..ed79c30d2 100644 --- a/nodedb/tests/native_error_code_classification.rs +++ b/nodedb/tests/native_error_code_classification.rs @@ -304,3 +304,71 @@ async fn native_permission_denial_carries_authorization_code() { err.message ); } + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn native_ddl_refusal_carries_collection_not_found_code() { + let server = TestServer::start().await; + + let mut stream = native_session(&server).await; + // DDL is refused by the DDL router, whose neutral error is authored as a + // SQLSTATE and a message and never holds a classified `Error` — so this + // frame reached the client with `ndb_code == 0` after the SQL and + // direct-op paths above were already carrying their codes. The same + // statement over the SQL path (`SELECT * FROM ...`) types correctly, so + // only a client that happened to run DDL saw the flattening. + let resp = send_sql(&mut stream, 1, "DROP TABLE native_err_absent_ddl").await; + + assert_eq!( + resp.status, + ResponseStatus::Error, + "dropping an absent collection must be refused" + ); + let err = resp.error.expect("error payload expected"); + assert_eq!( + err.code, + sqlstate::UNDEFINED_TABLE, + "an absent collection must map to its own SQLSTATE, got {}: {}", + err.code, + err.message + ); + assert_eq!( + err.ndb_code, + ErrorCode::COLLECTION_NOT_FOUND.0, + "a DDL refusal must carry the numeric code too, got {} ({})", + err.ndb_code, + err.message + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn native_duplicate_ddl_keeps_its_sqlstate_and_message() { + let server = TestServer::start().await; + + let mut stream = native_session(&server).await; + let create = send_sql(&mut stream, 1, "CREATE COLLECTION native_err_dup").await; + assert_eq!(create.status, ResponseStatus::Ok, "create the target"); + + // `42P07` has no `ErrorDetails` variant to map onto, so it stays + // unclassified rather than being forced onto an approximate one. This + // pins that the fallback is still exactly what shipped before — same + // SQLSTATE, same message, `ndb_code == 0` — so an unmapped SQLSTATE is + // never made worse by the table that classifies the mapped ones. + let resp = send_sql(&mut stream, 2, "CREATE COLLECTION native_err_dup").await; + + assert_eq!( + resp.status, + ResponseStatus::Error, + "creating an existing collection must be refused" + ); + let err = resp.error.expect("error payload expected"); + assert_eq!(err.code, "42P07", "duplicate object keeps its own SQLSTATE"); + assert!( + err.message.contains("native_err_dup"), + "the server's message must survive verbatim, got {}", + err.message + ); + assert_eq!( + err.ndb_code, 0, + "a SQLSTATE with no NodeDB variant must stay unclassified, not be guessed at" + ); +}