From a903d61bcc8813f0ef4fbe974f469ff4619e1cc0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Oct 2023 18:31:55 -0700 Subject: [PATCH] Simplify Diesel Error management (#4210) Depends on https://github.com/oxidecomputer/async-bb8-diesel/pull/54 As of https://github.com/oxidecomputer/omicron/pull/4140 , we check out connections before issuing queries to the underlying database. This means that when we receive errors from the database, they are not overloaded as "connection checkout" OR "database" errors - they are now always database errors. --- Cargo.lock | 2 +- Cargo.toml | 2 +- nexus/db-queries/src/db/collection_attach.rs | 10 +- nexus/db-queries/src/db/collection_detach.rs | 5 +- .../src/db/collection_detach_many.rs | 8 +- nexus/db-queries/src/db/collection_insert.rs | 18 ++- .../src/db/datastore/address_lot.rs | 29 ++--- .../src/db/datastore/db_metadata.rs | 2 +- .../src/db/datastore/device_auth.rs | 2 +- nexus/db-queries/src/db/datastore/dns.rs | 2 +- .../src/db/datastore/external_ip.rs | 3 +- nexus/db-queries/src/db/datastore/ip_pool.rs | 64 +++++------ nexus/db-queries/src/db/datastore/mod.rs | 13 +-- .../src/db/datastore/network_interface.rs | 3 +- nexus/db-queries/src/db/datastore/project.rs | 24 ++-- nexus/db-queries/src/db/datastore/rack.rs | 7 +- .../src/db/datastore/region_snapshot.rs | 2 +- nexus/db-queries/src/db/datastore/role.rs | 2 +- nexus/db-queries/src/db/datastore/silo.rs | 24 ++-- .../db-queries/src/db/datastore/silo_group.rs | 5 +- nexus/db-queries/src/db/datastore/sled.rs | 2 +- nexus/db-queries/src/db/datastore/snapshot.rs | 16 +-- .../src/db/datastore/switch_interface.rs | 24 ++-- .../src/db/datastore/switch_port.rs | 104 +++++++----------- nexus/db-queries/src/db/datastore/update.rs | 2 +- nexus/db-queries/src/db/datastore/volume.rs | 8 +- nexus/db-queries/src/db/datastore/vpc.rs | 49 ++++----- nexus/db-queries/src/db/error.rs | 79 ++++--------- nexus/db-queries/src/db/explain.rs | 7 +- nexus/db-queries/src/db/pool.rs | 4 +- .../db-queries/src/db/queries/external_ip.rs | 3 +- .../src/db/queries/network_interface.rs | 96 +++++++--------- .../src/db/queries/region_allocation.rs | 3 +- nexus/db-queries/src/db/queries/vpc_subnet.rs | 20 ++-- nexus/db-queries/src/db/true_or_cast_error.rs | 9 +- nexus/db-queries/src/db/update_and_check.rs | 3 +- nexus/src/app/sagas/disk_create.rs | 5 +- nexus/src/app/sagas/instance_create.rs | 4 +- nexus/src/app/sagas/project_create.rs | 5 +- nexus/src/app/sagas/snapshot_create.rs | 6 +- nexus/src/app/sagas/vpc_create.rs | 6 +- 41 files changed, 291 insertions(+), 391 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d58ba77133..d5a90f7f85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -298,7 +298,7 @@ checksum = "9b34d609dfbaf33d6889b2b7106d3ca345eacad44200913df5ba02bfd31d2ba9" [[package]] name = "async-bb8-diesel" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/async-bb8-diesel?rev=da04c087f835a51e0441addb19c5ef4986e1fcf2#da04c087f835a51e0441addb19c5ef4986e1fcf2" +source = "git+https://github.com/oxidecomputer/async-bb8-diesel?rev=1446f7e0c1f05f33a0581abd51fa873c7652ab61#1446f7e0c1f05f33a0581abd51fa873c7652ab61" dependencies = [ "async-trait", "bb8", diff --git a/Cargo.toml b/Cargo.toml index 832b8663e6..7521bb4d45 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -136,7 +136,7 @@ api_identity = { path = "api_identity" } approx = "0.5.1" assert_matches = "1.5.0" assert_cmd = "2.0.12" -async-bb8-diesel = { git = "https://github.com/oxidecomputer/async-bb8-diesel", rev = "da04c087f835a51e0441addb19c5ef4986e1fcf2" } +async-bb8-diesel = { git = "https://github.com/oxidecomputer/async-bb8-diesel", rev = "1446f7e0c1f05f33a0581abd51fa873c7652ab61" } async-trait = "0.1.73" atomicwrites = "0.4.1" authz-macros = { path = "nexus/authz-macros" } diff --git a/nexus/db-queries/src/db/collection_attach.rs b/nexus/db-queries/src/db/collection_attach.rs index 40ec659bf9..ea4d9d5beb 100644 --- a/nexus/db-queries/src/db/collection_attach.rs +++ b/nexus/db-queries/src/db/collection_attach.rs @@ -17,7 +17,7 @@ use super::cte_utils::{ QueryFromClause, QuerySqlType, TableDefaultWhereClause, }; use super::pool::DbConnection; -use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::AsyncRunQueryDsl; use diesel::associations::HasTable; use diesel::expression::{AsExpression, Expression}; use diesel::helper_types::*; @@ -26,6 +26,7 @@ use diesel::prelude::*; use diesel::query_builder::*; use diesel::query_dsl::methods as query_methods; use diesel::query_source::Table; +use diesel::result::Error as DieselError; use diesel::sql_types::{BigInt, Nullable, SingleValue}; use nexus_db_model::DatastoreAttachTargetConfig; use std::fmt::Debug; @@ -299,7 +300,7 @@ where /// Result of [`AttachToCollectionStatement`] when executed asynchronously pub type AsyncAttachToCollectionResult = - Result<(C, ResourceType), AttachError>; + Result<(C, ResourceType), AttachError>; /// Errors returned by [`AttachToCollectionStatement`]. #[derive(Debug)] @@ -998,9 +999,8 @@ mod test { .set(resource::dsl::collection_id.eq(collection_id)), ); - type TxnError = TransactionError< - AttachError, - >; + type TxnError = + TransactionError>; let result = conn .transaction_async(|conn| async move { attach_query.attach_and_get_result_async(&conn).await.map_err( diff --git a/nexus/db-queries/src/db/collection_detach.rs b/nexus/db-queries/src/db/collection_detach.rs index df157040e6..03e09d41ca 100644 --- a/nexus/db-queries/src/db/collection_detach.rs +++ b/nexus/db-queries/src/db/collection_detach.rs @@ -16,7 +16,7 @@ use super::cte_utils::{ QueryFromClause, QuerySqlType, }; use super::pool::DbConnection; -use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::AsyncRunQueryDsl; use diesel::associations::HasTable; use diesel::expression::{AsExpression, Expression}; use diesel::helper_types::*; @@ -25,6 +25,7 @@ use diesel::prelude::*; use diesel::query_builder::*; use diesel::query_dsl::methods as query_methods; use diesel::query_source::Table; +use diesel::result::Error as DieselError; use diesel::sql_types::{Nullable, SingleValue}; use nexus_db_model::DatastoreAttachTargetConfig; use std::fmt::Debug; @@ -230,7 +231,7 @@ where /// Result of [`DetachFromCollectionStatement`] when executed asynchronously pub type AsyncDetachFromCollectionResult = - Result>; + Result>; /// Errors returned by [`DetachFromCollectionStatement`]. #[derive(Debug)] diff --git a/nexus/db-queries/src/db/collection_detach_many.rs b/nexus/db-queries/src/db/collection_detach_many.rs index 0b65c404c5..8df6d4aed4 100644 --- a/nexus/db-queries/src/db/collection_detach_many.rs +++ b/nexus/db-queries/src/db/collection_detach_many.rs @@ -25,6 +25,7 @@ use diesel::prelude::*; use diesel::query_builder::*; use diesel::query_dsl::methods as query_methods; use diesel::query_source::Table; +use diesel::result::Error as DieselError; use diesel::sql_types::{Nullable, SingleValue}; use nexus_db_model::DatastoreAttachTargetConfig; use std::fmt::Debug; @@ -241,7 +242,7 @@ where /// Result of [`DetachManyFromCollectionStatement`] when executed asynchronously pub type AsyncDetachManyFromCollectionResult = - Result>; + Result>; /// Errors returned by [`DetachManyFromCollectionStatement`]. #[derive(Debug)] @@ -918,9 +919,8 @@ mod test { .set(resource::dsl::collection_id.eq(Option::::None)), ); - type TxnError = TransactionError< - DetachManyError, - >; + type TxnError = + TransactionError>; let result = conn .transaction_async(|conn| async move { detach_query.detach_and_get_result_async(&conn).await.map_err( diff --git a/nexus/db-queries/src/db/collection_insert.rs b/nexus/db-queries/src/db/collection_insert.rs index 993f16e048..b295f0574d 100644 --- a/nexus/db-queries/src/db/collection_insert.rs +++ b/nexus/db-queries/src/db/collection_insert.rs @@ -10,7 +10,7 @@ //! 3) inserts the child resource row use super::pool::DbConnection; -use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::AsyncRunQueryDsl; use diesel::associations::HasTable; use diesel::helper_types::*; use diesel::pg::Pg; @@ -18,6 +18,7 @@ use diesel::prelude::*; use diesel::query_builder::*; use diesel::query_dsl::methods as query_methods; use diesel::query_source::Table; +use diesel::result::Error as DieselError; use diesel::sql_types::SingleValue; use nexus_db_model::DatastoreCollectionConfig; use std::fmt::Debug; @@ -170,7 +171,7 @@ pub enum AsyncInsertError { /// The collection that the query was inserting into does not exist CollectionNotFound, /// Other database error - DatabaseError(ConnectionError), + DatabaseError(DieselError), } impl InsertIntoCollectionStatement @@ -238,14 +239,11 @@ where /// Translate from diesel errors into AsyncInsertError, handling the /// intentional division-by-zero error in the CTE. - fn translate_async_error(err: ConnectionError) -> AsyncInsertError { - match err { - ConnectionError::Query(err) - if Self::error_is_division_by_zero(&err) => - { - AsyncInsertError::CollectionNotFound - } - other => AsyncInsertError::DatabaseError(other), + fn translate_async_error(err: DieselError) -> AsyncInsertError { + if Self::error_is_division_by_zero(&err) { + AsyncInsertError::CollectionNotFound + } else { + AsyncInsertError::DatabaseError(err) } } } diff --git a/nexus/db-queries/src/db/datastore/address_lot.rs b/nexus/db-queries/src/db/datastore/address_lot.rs index 9d264dbf6b..97dfb59eba 100644 --- a/nexus/db-queries/src/db/datastore/address_lot.rs +++ b/nexus/db-queries/src/db/datastore/address_lot.rs @@ -13,9 +13,7 @@ use crate::db::error::TransactionError; use crate::db::model::Name; use crate::db::model::{AddressLot, AddressLotBlock, AddressLotReservedBlock}; use crate::db::pagination::paginated; -use async_bb8_diesel::{ - AsyncConnection, AsyncRunQueryDsl, Connection, ConnectionError, -}; +use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, Connection}; use chrono::Utc; use diesel::result::Error as DieselError; use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; @@ -84,15 +82,13 @@ impl DataStore { }) .await .map_err(|e| match e { - ConnectionError::Query(DieselError::DatabaseError(_, _)) => { - public_error_from_diesel( - e, - ErrorHandler::Conflict( - ResourceType::AddressLot, - ¶ms.identity.name.as_str(), - ), - ) - } + DieselError::DatabaseError(_, _) => public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::AddressLot, + ¶ms.identity.name.as_str(), + ), + ), _ => public_error_from_diesel(e, ErrorHandler::Server), }) } @@ -151,7 +147,7 @@ impl DataStore { }) .await .map_err(|e| match e { - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } TxnError::CustomError(AddressLotDeleteError::LotInUse) => { @@ -252,11 +248,10 @@ pub(crate) async fn try_reserve_block( .limit(1) .first_async::(conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => ReserveBlockTxnError::CustomError( + .map_err(|_e| { + ReserveBlockTxnError::CustomError( ReserveBlockError::AddressNotInLot, - ), - e => e.into(), + ) })?; // Ensure the address is not already taken. diff --git a/nexus/db-queries/src/db/datastore/db_metadata.rs b/nexus/db-queries/src/db/datastore/db_metadata.rs index 181b3c1798..9e4e8b1a48 100644 --- a/nexus/db-queries/src/db/datastore/db_metadata.rs +++ b/nexus/db-queries/src/db/datastore/db_metadata.rs @@ -351,7 +351,7 @@ impl DataStore { match result { Ok(()) => Ok(()), Err(TransactionError::CustomError(())) => panic!("No custom error"), - Err(TransactionError::Connection(e)) => { + Err(TransactionError::Database(e)) => { Err(public_error_from_diesel(e, ErrorHandler::Server)) } } diff --git a/nexus/db-queries/src/db/datastore/device_auth.rs b/nexus/db-queries/src/db/datastore/device_auth.rs index e084834833..e1facb43f6 100644 --- a/nexus/db-queries/src/db/datastore/device_auth.rs +++ b/nexus/db-queries/src/db/datastore/device_auth.rs @@ -103,7 +103,7 @@ impl DataStore { TxnError::CustomError(TokenGrantError::TooManyRequests) => { Error::internal_error("unexpectedly found multiple device auth requests for the same user code") } - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/dns.rs b/nexus/db-queries/src/db/datastore/dns.rs index d9704594b1..f7ad97593e 100644 --- a/nexus/db-queries/src/db/datastore/dns.rs +++ b/nexus/db-queries/src/db/datastore/dns.rs @@ -395,7 +395,7 @@ impl DataStore { match result { Ok(()) => Ok(()), Err(TransactionError::CustomError(e)) => Err(e), - Err(TransactionError::Connection(e)) => { + Err(TransactionError::Database(e)) => { Err(public_error_from_diesel(e, ErrorHandler::Server)) } } diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index 268b284a0a..e663130a84 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -143,10 +143,9 @@ impl DataStore { ) -> CreateResult { let explicit_ip = data.explicit_ip().is_some(); NextExternalIp::new(data).get_result_async(conn).await.map_err(|e| { - use async_bb8_diesel::ConnectionError::Query; use diesel::result::Error::NotFound; match e { - Query(NotFound) => { + NotFound => { if explicit_ip { Error::invalid_request( "Requested external IP address not available", diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index bd3148f2f7..fb300ef833 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -10,7 +10,6 @@ use crate::context::OpContext; use crate::db; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; -use crate::db::error::diesel_result_optional; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::fixed_data::silo::INTERNAL_SILO_ID; @@ -183,18 +182,17 @@ impl DataStore { opctx.authorize(authz::Action::Delete, authz_pool).await?; // Verify there are no IP ranges still in this pool - let range = diesel_result_optional( - ip_pool_range::dsl::ip_pool_range - .filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id())) - .filter(ip_pool_range::dsl::time_deleted.is_null()) - .select(ip_pool_range::dsl::id) - .limit(1) - .first_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + let range = ip_pool_range::dsl::ip_pool_range + .filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id())) + .filter(ip_pool_range::dsl::time_deleted.is_null()) + .select(ip_pool_range::dsl::id) + .limit(1) + .first_async::( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if range.is_some() { return Err(Error::InvalidRequest { message: @@ -313,7 +311,6 @@ impl DataStore { .insert_and_get_result_async(conn) .await .map_err(|e| { - use async_bb8_diesel::ConnectionError::Query; use diesel::result::Error::NotFound; match e { @@ -323,7 +320,7 @@ impl DataStore { lookup_type: LookupType::ById(pool_id), } } - AsyncInsertError::DatabaseError(Query(NotFound)) => { + AsyncInsertError::DatabaseError(NotFound) => { // We've filtered out the IP addresses the client provided, // i.e., there's some overlap with existing addresses. Error::invalid_request( @@ -363,26 +360,25 @@ impl DataStore { // concurrent inserts of new external IPs from the target range by // comparing the rcgen. let conn = self.pool_connection_authorized(opctx).await?; - let range = diesel_result_optional( - dsl::ip_pool_range - .filter(dsl::ip_pool_id.eq(pool_id)) - .filter(dsl::first_address.eq(first_net)) - .filter(dsl::last_address.eq(last_net)) - .filter(dsl::time_deleted.is_null()) - .select(IpPoolRange::as_select()) - .get_result_async::(&*conn) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? - .ok_or_else(|| { - Error::invalid_request( - format!( - "The provided range {}-{} does not exist", - first_address, last_address, + let range = dsl::ip_pool_range + .filter(dsl::ip_pool_id.eq(pool_id)) + .filter(dsl::first_address.eq(first_net)) + .filter(dsl::last_address.eq(last_net)) + .filter(dsl::time_deleted.is_null()) + .select(IpPoolRange::as_select()) + .get_result_async::(&*conn) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .ok_or_else(|| { + Error::invalid_request( + format!( + "The provided range {}-{} does not exist", + first_address, last_address, + ) + .as_str(), ) - .as_str(), - ) - })?; + })?; // Find external IPs allocated out of this pool and range. let range_id = range.id; diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index b1f3203c60..7d5e32cad9 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -1670,7 +1670,6 @@ mod test { async fn test_external_ip_check_constraints() { use crate::db::model::IpKind; use crate::db::schema::external_ip::dsl; - use async_bb8_diesel::ConnectionError::Query; use diesel::result::DatabaseErrorKind::CheckViolation; use diesel::result::Error::DatabaseError; @@ -1756,10 +1755,10 @@ mod test { assert!( matches!( err, - Query(DatabaseError( + DatabaseError( CheckViolation, _ - )) + ) ), "Expected a CHECK violation when inserting a \ Floating IP record with NULL name and/or description", @@ -1805,10 +1804,10 @@ mod test { assert!( matches!( err, - Query(DatabaseError( + DatabaseError( CheckViolation, _ - )) + ) ), "Expected a CHECK violation when inserting an \ Ephemeral Service IP", @@ -1836,10 +1835,10 @@ mod test { assert!( matches!( err, - Query(DatabaseError( + DatabaseError( CheckViolation, _ - )) + ) ), "Expected a CHECK violation when inserting a \ {:?} IP record with non-NULL name, description, \ diff --git a/nexus/db-queries/src/db/datastore/network_interface.rs b/nexus/db-queries/src/db/datastore/network_interface.rs index 3d7b8afa71..4a46b23529 100644 --- a/nexus/db-queries/src/db/datastore/network_interface.rs +++ b/nexus/db-queries/src/db/datastore/network_interface.rs @@ -29,6 +29,7 @@ use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; +use diesel::result::Error as DieselError; use omicron_common::api::external; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::DeleteResult; @@ -463,7 +464,7 @@ impl DataStore { #[derive(Debug)] enum NetworkInterfaceUpdateError { InstanceNotStopped, - FailedToUnsetPrimary(async_bb8_diesel::ConnectionError), + FailedToUnsetPrimary(DieselError), } type TxnError = TransactionError; diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index 0285679cd5..c447b5bf98 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -11,7 +11,6 @@ use crate::context::OpContext; use crate::db; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; -use crate::db::error::diesel_result_optional; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::error::TransactionError; @@ -60,16 +59,15 @@ macro_rules! generate_fn_to_ensure_none_in_project { ) -> DeleteResult { use db::schema::$i; - let maybe_label = diesel_result_optional( - $i::dsl::$i - .filter($i::dsl::project_id.eq(authz_project.id())) - .filter($i::dsl::time_deleted.is_null()) - .select($i::dsl::$label) - .limit(1) - .first_async::<$label_ty>(&*self.pool_connection_authorized(opctx).await?) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + let maybe_label = $i::dsl::$i + .filter($i::dsl::project_id.eq(authz_project.id())) + .filter($i::dsl::time_deleted.is_null()) + .select($i::dsl::$label) + .limit(1) + .first_async::<$label_ty>(&*self.pool_connection_authorized(opctx).await?) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if let Some(label) = maybe_label { let object = stringify!($i).replace('_', " "); @@ -193,7 +191,7 @@ impl DataStore { .await .map_err(|e| match e { TransactionError::CustomError(e) => e, - TransactionError::Connection(e) => { + TransactionError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } })?; @@ -270,7 +268,7 @@ impl DataStore { .await .map_err(|e| match e { TxnError::CustomError(e) => e, - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } })?; diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 1be3e1ee4c..f5f7524aab 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -30,6 +30,7 @@ use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; +use diesel::result::Error as DieselError; use diesel::upsert::excluded; use nexus_db_model::DnsGroup; use nexus_db_model::DnsZone; @@ -79,7 +80,7 @@ enum RackInitError { AddingNic(Error), ServiceInsert(Error), DatasetInsert { err: AsyncInsertError, zpool_id: Uuid }, - RackUpdate { err: async_bb8_diesel::ConnectionError, rack_id: Uuid }, + RackUpdate { err: DieselError, rack_id: Uuid }, DnsSerialization(Error), Silo(Error), RoleAssignment(Error), @@ -137,7 +138,7 @@ impl From for Error { err )) } - TxnError::Connection(e) => { + TxnError::Database(e) => { Error::internal_error(&format!("Transaction error: {}", e)) } } @@ -631,7 +632,7 @@ impl DataStore { .await .map_err(|error: TxnError| match error { TransactionError::CustomError(err) => err, - TransactionError::Connection(e) => { + TransactionError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/region_snapshot.rs b/nexus/db-queries/src/db/datastore/region_snapshot.rs index 148cfe4812..3d328a6206 100644 --- a/nexus/db-queries/src/db/datastore/region_snapshot.rs +++ b/nexus/db-queries/src/db/datastore/region_snapshot.rs @@ -10,8 +10,8 @@ use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::model::RegionSnapshot; use async_bb8_diesel::AsyncRunQueryDsl; -use async_bb8_diesel::OptionalExtension; use diesel::prelude::*; +use diesel::OptionalExtension; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::LookupResult; diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index f1198c239b..b2ad441475 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -280,7 +280,7 @@ impl DataStore { .await .map_err(|e| match e { TransactionError::CustomError(e) => e, - TransactionError::Connection(e) => { + TransactionError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 5e909b84c4..ec3658c067 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -10,7 +10,6 @@ use crate::authz; use crate::context::OpContext; use crate::db; use crate::db::datastore::RunnableQuery; -use crate::db::error::diesel_result_optional; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::error::TransactionError; @@ -261,7 +260,7 @@ impl DataStore { .await .map_err(|e| match e { TransactionError::CustomError(e) => e, - TransactionError::Connection(e) => { + TransactionError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) @@ -338,16 +337,15 @@ impl DataStore { // Make sure there are no projects present within this silo. let id = authz_silo.id(); let rcgen = db_silo.rcgen; - let project_found = diesel_result_optional( - project::dsl::project - .filter(project::dsl::silo_id.eq(id)) - .filter(project::dsl::time_deleted.is_null()) - .select(project::dsl::id) - .limit(1) - .first_async::(&*conn) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + let project_found = project::dsl::project + .filter(project::dsl::silo_id.eq(id)) + .filter(project::dsl::time_deleted.is_null()) + .select(project::dsl::id) + .limit(1) + .first_async::(&*conn) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if project_found.is_some() { return Err(Error::InvalidRequest { @@ -395,7 +393,7 @@ impl DataStore { .await .map_err(|e| match e { TxnError::CustomError(e) => e, - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } })?; diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index d13986bb2d..46f4aae7c9 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -15,8 +15,8 @@ use crate::db::error::TransactionError; use crate::db::model::SiloGroup; use crate::db::model::SiloGroupMembership; use crate::db::pagination::paginated; +use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; -use async_bb8_diesel::{AsyncConnection, OptionalExtension}; use chrono::Utc; use diesel::prelude::*; use omicron_common::api::external::CreateResult; @@ -237,8 +237,7 @@ impl DataStore { "group {0} still has memberships", id )), - - TxnError::Connection(error) => { + TxnError::Database(error) => { public_error_from_diesel(error, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index ec6cca0071..a52d1b7772 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -183,7 +183,7 @@ impl DataStore { "No sleds can fit the requested instance", ) } - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/snapshot.rs b/nexus/db-queries/src/db/datastore/snapshot.rs index 29fbb38e88..59fb00c84d 100644 --- a/nexus/db-queries/src/db/datastore/snapshot.rs +++ b/nexus/db-queries/src/db/datastore/snapshot.rs @@ -22,10 +22,9 @@ use crate::db::update_and_check::UpdateAndCheck; use crate::db::TransactionError; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; -use async_bb8_diesel::ConnectionError; use chrono::Utc; use diesel::prelude::*; -use diesel::result::Error as DieselError; +use diesel::OptionalExtension; use nexus_types::identity::Resource; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::CreateResult; @@ -101,7 +100,7 @@ impl DataStore { // does not match, but a project and name that does, return // ObjectAlreadyExists here. - let existing_snapshot_id: Option = match dsl::snapshot + let existing_snapshot_id: Option = dsl::snapshot .filter(dsl::time_deleted.is_null()) .filter(dsl::name.eq(snapshot.name().to_string())) .filter(dsl::project_id.eq(snapshot.project_id)) @@ -109,13 +108,7 @@ impl DataStore { .limit(1) .first_async(&conn) .await - { - Ok(v) => Ok(Some(v)), - Err(ConnectionError::Query(DieselError::NotFound)) => { - Ok(None) - } - Err(e) => Err(e), - }?; + .optional()?; if let Some(existing_snapshot_id) = existing_snapshot_id { if existing_snapshot_id != snapshot.id() { @@ -161,8 +154,7 @@ impl DataStore { } }, }, - - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } })?; diff --git a/nexus/db-queries/src/db/datastore/switch_interface.rs b/nexus/db-queries/src/db/datastore/switch_interface.rs index 498064ce37..88cff50471 100644 --- a/nexus/db-queries/src/db/datastore/switch_interface.rs +++ b/nexus/db-queries/src/db/datastore/switch_interface.rs @@ -14,7 +14,7 @@ use crate::db::error::ErrorHandler; use crate::db::error::TransactionError; use crate::db::model::LoopbackAddress; use crate::db::pagination::paginated; -use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl}; use diesel::result::Error as DieselError; use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; use ipnetwork::IpNetwork; @@ -65,8 +65,8 @@ impl DataStore { LoopbackAddressCreateError::ReserveBlock(err), ) } - ReserveBlockTxnError::Connection(err) => { - TxnError::Connection(err) + ReserveBlockTxnError::Database(err) => { + TxnError::Database(err) } })?; @@ -103,16 +103,14 @@ impl DataStore { ReserveBlockError::AddressNotInLot, ), ) => Error::invalid_request("address not in lot"), - TxnError::Connection(e) => match e { - ConnectionError::Query(DieselError::DatabaseError(_, _)) => { - public_error_from_diesel( - e, - ErrorHandler::Conflict( - ResourceType::LoopbackAddress, - &format!("lo {}", inet), - ), - ) - } + TxnError::Database(e) => match e { + DieselError::DatabaseError(_, _) => public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::LoopbackAddress, + &format!("lo {}", inet), + ), + ), _ => public_error_from_diesel(e, ErrorHandler::Server), }, }) diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index 940fedb473..45be594be6 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -20,7 +20,7 @@ use crate::db::model::{ SwitchVlanInterfaceConfig, }; use crate::db::pagination::paginated; -use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl}; use diesel::result::Error as DieselError; use diesel::{ ExpressionMethods, JoinOnDsl, NullableExpressionMethods, QueryDsl, @@ -279,11 +279,10 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => TxnError::CustomError( + .map_err(|_| { + TxnError::CustomError( SwitchPortSettingsCreateError::BgpAnnounceSetNotFound, - ), - e => e.into(), + ) })? } }; @@ -300,12 +299,11 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => TxnError::CustomError( + .map_err(|_| + TxnError::CustomError( SwitchPortSettingsCreateError::BgpConfigNotFound, - ), - e => e.into(), - })? + ) + )? } }; @@ -341,14 +339,11 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => { - TxnError::CustomError( - SwitchPortSettingsCreateError::AddressLotNotFound, - ) - } - e => e.into() - })? + .map_err(|_| + TxnError::CustomError( + SwitchPortSettingsCreateError::AddressLotNotFound, + ) + )? } }; // TODO: Reduce DB round trips needed for reserving ip blocks @@ -369,7 +364,7 @@ impl DataStore { SwitchPortSettingsCreateError::ReserveBlock(err) ) } - ReserveBlockTxnError::Connection(err) => TxnError::Connection(err), + ReserveBlockTxnError::Database(err) => TxnError::Database(err), })?; address_config.push(SwitchPortAddressConfig::new( @@ -416,10 +411,8 @@ impl DataStore { ReserveBlockError::AddressNotInLot ) ) => Error::invalid_request("address not in lot"), - TxnError::Connection(e) => match e { - ConnectionError::Query( - DieselError::DatabaseError(_, _), - ) => public_error_from_diesel( + TxnError::Database(e) => match e { + DieselError::DatabaseError(_, _) => public_error_from_diesel( e, ErrorHandler::Conflict( ResourceType::SwitchPortSettings, @@ -467,12 +460,11 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => TxnError::CustomError( + .map_err(|_| + TxnError::CustomError( SwitchPortSettingsDeleteError::SwitchPortSettingsNotFound, - ), - e => e.into() - })? + ) + )? } }; @@ -599,10 +591,8 @@ impl DataStore { SwitchPortSettingsDeleteError::SwitchPortSettingsNotFound) => { Error::invalid_request("port settings not found") } - TxnError::Connection(e) => match e { - ConnectionError::Query( - DieselError::DatabaseError(_, _), - ) => { + TxnError::Database(e) => match e { + DieselError::DatabaseError(_, _) => { let name = match ¶ms.port_settings { Some(name_or_id) => name_or_id.to_string(), None => String::new(), @@ -676,11 +666,10 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => TxnError::CustomError( + .map_err(|_| { + TxnError::CustomError( SwitchPortSettingsGetError::NotFound(name.clone()) - ), - e => e.into() + ) })? } }; @@ -804,10 +793,8 @@ impl DataStore { SwitchPortSettingsGetError::NotFound(name)) => { Error::not_found_by_name(ResourceType::SwitchPortSettings, &name) } - TxnError::Connection(e) => match e { - ConnectionError::Query( - DieselError::DatabaseError(_, _), - ) => { + TxnError::Database(e) => match e { + DieselError::DatabaseError(_, _) => { let name = name_or_id.to_string(); public_error_from_diesel( e, @@ -855,11 +842,8 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => TxnError::CustomError( - SwitchPortCreateError::RackNotFound, - ), - e => e.into(), + .map_err(|_| { + TxnError::CustomError(SwitchPortCreateError::RackNotFound) })?; // insert switch port @@ -878,19 +862,14 @@ impl DataStore { TxnError::CustomError(SwitchPortCreateError::RackNotFound) => { Error::invalid_request("rack not found") } - TxnError::Connection(e) => match e { - ConnectionError::Query(DieselError::DatabaseError(_, _)) => { - public_error_from_diesel( - e, - ErrorHandler::Conflict( - ResourceType::SwitchPort, - &format!( - "{}/{}/{}", - rack_id, &switch_location, &port, - ), - ), - ) - } + TxnError::Database(e) => match e { + DieselError::DatabaseError(_, _) => public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::SwitchPort, + &format!("{}/{}/{}", rack_id, &switch_location, &port,), + ), + ), _ => public_error_from_diesel(e, ErrorHandler::Server), }, }) @@ -929,11 +908,8 @@ impl DataStore { .limit(1) .first_async::(&conn) .await - .map_err(|e| match e { - ConnectionError::Query(_) => { - TxnError::CustomError(SwitchPortDeleteError::NotFound) - } - e => e.into(), + .map_err(|_| { + TxnError::CustomError(SwitchPortDeleteError::NotFound) })?; if port.port_settings_id.is_some() { @@ -958,7 +934,7 @@ impl DataStore { TxnError::CustomError(SwitchPortDeleteError::ActiveSettings) => { Error::invalid_request("must clear port settings first") } - TxnError::Connection(e) => { + TxnError::Database(e) => { public_error_from_diesel(e, ErrorHandler::Server) } }) diff --git a/nexus/db-queries/src/db/datastore/update.rs b/nexus/db-queries/src/db/datastore/update.rs index 5a3e3b27e4..8b1eecb781 100644 --- a/nexus/db-queries/src/db/datastore/update.rs +++ b/nexus/db-queries/src/db/datastore/update.rs @@ -164,7 +164,7 @@ impl DataStore { .await .map_err(|e| match e { TransactionError::CustomError(e) => e, - TransactionError::Connection(e) => public_error_from_diesel( + TransactionError::Database(e) => public_error_from_diesel( e, ErrorHandler::Conflict( ResourceType::ComponentUpdate, diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index b97b8451cf..38e3875036 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -16,9 +16,9 @@ use crate::db::model::RegionSnapshot; use crate::db::model::Volume; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; -use async_bb8_diesel::OptionalExtension; use chrono::Utc; use diesel::prelude::*; +use diesel::OptionalExtension; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; @@ -336,7 +336,7 @@ impl DataStore { .await .map_err(|e| match e { TxnError::CustomError(VolumeGetError::DieselError(e)) => { - public_error_from_diesel(e.into(), ErrorHandler::Server) + public_error_from_diesel(e, ErrorHandler::Server) } _ => { @@ -757,7 +757,7 @@ impl DataStore { .map_err(|e| match e { TxnError::CustomError( DecreaseCrucibleResourcesError::DieselError(e), - ) => public_error_from_diesel(e.into(), ErrorHandler::Server), + ) => public_error_from_diesel(e, ErrorHandler::Server), _ => { Error::internal_error(&format!("Transaction error: {}", e)) @@ -955,7 +955,7 @@ impl DataStore { TxnError::CustomError( RemoveReadOnlyParentError::DieselError(e), ) => public_error_from_diesel( - e.into(), + e, ErrorHandler::Server, ), diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index af7ea93456..46c3d2504e 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -10,7 +10,6 @@ use crate::context::OpContext; use crate::db; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; -use crate::db::error::diesel_result_optional; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::error::TransactionError; @@ -389,19 +388,18 @@ impl DataStore { // but we can't have NICs be a child of both tables at this point, and // we need to prevent VPC Subnets from being deleted while they have // NICs in them as well. - if diesel_result_optional( - vpc_subnet::dsl::vpc_subnet - .filter(vpc_subnet::dsl::vpc_id.eq(authz_vpc.id())) - .filter(vpc_subnet::dsl::time_deleted.is_null()) - .select(vpc_subnet::dsl::id) - .limit(1) - .first_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? - .is_some() + if vpc_subnet::dsl::vpc_subnet + .filter(vpc_subnet::dsl::vpc_id.eq(authz_vpc.id())) + .filter(vpc_subnet::dsl::time_deleted.is_null()) + .select(vpc_subnet::dsl::id) + .limit(1) + .first_async::( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .is_some() { return Err(Error::InvalidRequest { message: String::from( @@ -556,7 +554,7 @@ impl DataStore { TxnError::CustomError( FirewallUpdateError::CollectionNotFound, ) => Error::not_found_by_id(ResourceType::Vpc, &authz_vpc.id()), - TxnError::Connection(e) => public_error_from_diesel( + TxnError::Database(e) => public_error_from_diesel( e, ErrorHandler::NotFoundByResource(authz_vpc), ), @@ -700,17 +698,16 @@ impl DataStore { let conn = self.pool_connection_authorized(opctx).await?; // Verify there are no child network interfaces in this VPC Subnet - if diesel_result_optional( - network_interface::dsl::network_interface - .filter(network_interface::dsl::subnet_id.eq(authz_subnet.id())) - .filter(network_interface::dsl::time_deleted.is_null()) - .select(network_interface::dsl::id) - .limit(1) - .first_async::(&*conn) - .await, - ) - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? - .is_some() + if network_interface::dsl::network_interface + .filter(network_interface::dsl::subnet_id.eq(authz_subnet.id())) + .filter(network_interface::dsl::time_deleted.is_null()) + .select(network_interface::dsl::id) + .limit(1) + .first_async::(&*conn) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .is_some() { return Err(Error::InvalidRequest { message: String::from( diff --git a/nexus/db-queries/src/db/error.rs b/nexus/db-queries/src/db/error.rs index f7402bb8c7..cbe2b0a71f 100644 --- a/nexus/db-queries/src/db/error.rs +++ b/nexus/db-queries/src/db/error.rs @@ -4,7 +4,6 @@ //! Error handling and conversions. -use async_bb8_diesel::ConnectionError; use diesel::result::DatabaseErrorInformation; use diesel::result::DatabaseErrorKind as DieselErrorKind; use diesel::result::Error as DieselError; @@ -25,16 +24,8 @@ pub enum TransactionError { /// /// This error covers failure due to accessing the DB pool or errors /// propagated from the DB itself. - #[error("Connection error: {0}")] - Connection(#[from] async_bb8_diesel::ConnectionError), -} - -// Maps a "diesel error" into a "pool error", which -// is already contained within the error type. -impl From for TransactionError { - fn from(err: DieselError) -> Self { - Self::Connection(ConnectionError::Query(err)) - } + #[error("Database error: {0}")] + Database(#[from] DieselError), } impl From for TransactionError { @@ -50,8 +41,9 @@ impl TransactionError { /// [1]: https://www.cockroachlabs.com/docs/v23.1/transaction-retry-error-reference#client-side-retry-handling pub fn retry_transaction(&self) -> bool { match &self { - TransactionError::Connection(ConnectionError::Query( - DieselError::DatabaseError(kind, boxed_error_information), + Self::Database(DieselError::DatabaseError( + kind, + boxed_error_information, )) => match kind { DieselErrorKind::SerializationFailure => { return boxed_error_information @@ -93,19 +85,6 @@ fn format_database_error( rv } -/// Like [`diesel::result::OptionalExtension::optional`]. This turns Ok(v) -/// into Ok(Some(v)), Err("NotFound") into Ok(None), and leave all other values -/// unchanged. -pub fn diesel_result_optional( - result: Result, -) -> Result, ConnectionError> { - match result { - Ok(v) => Ok(Some(v)), - Err(ConnectionError::Query(DieselError::NotFound)) => Ok(None), - Err(e) => Err(e), - } -} - /// Allows the caller to handle user-facing errors, and provide additional /// context which may be used to populate more informative errors. /// @@ -142,41 +121,27 @@ pub enum ErrorHandler<'a> { /// [`ErrorHandler`] may be used to add additional handlers for the error /// being returned. pub fn public_error_from_diesel( - error: ConnectionError, + error: DieselError, handler: ErrorHandler<'_>, ) -> PublicError { - match error { - ConnectionError::Connection(error) => PublicError::unavail(&format!( - "Failed to access connection pool: {}", + match handler { + ErrorHandler::NotFoundByResource(resource) => { + public_error_from_diesel_lookup( + error, + resource.resource_type(), + resource.lookup_type(), + ) + } + ErrorHandler::NotFoundByLookup(resource_type, lookup_type) => { + public_error_from_diesel_lookup(error, resource_type, &lookup_type) + } + ErrorHandler::Conflict(resource_type, object_name) => { + public_error_from_diesel_create(error, resource_type, object_name) + } + ErrorHandler::Server => PublicError::internal_error(&format!( + "unexpected database error: {:#}", error )), - ConnectionError::Query(error) => match handler { - ErrorHandler::NotFoundByResource(resource) => { - public_error_from_diesel_lookup( - error, - resource.resource_type(), - resource.lookup_type(), - ) - } - ErrorHandler::NotFoundByLookup(resource_type, lookup_type) => { - public_error_from_diesel_lookup( - error, - resource_type, - &lookup_type, - ) - } - ErrorHandler::Conflict(resource_type, object_name) => { - public_error_from_diesel_create( - error, - resource_type, - object_name, - ) - } - ErrorHandler::Server => PublicError::internal_error(&format!( - "unexpected database error: {:#}", - error - )), - }, } } diff --git a/nexus/db-queries/src/db/explain.rs b/nexus/db-queries/src/db/explain.rs index fc8098b876..3de5b4f280 100644 --- a/nexus/db-queries/src/db/explain.rs +++ b/nexus/db-queries/src/db/explain.rs @@ -5,11 +5,12 @@ //! Utility allowing Diesel to EXPLAIN queries. use super::pool::DbConnection; -use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError}; +use async_bb8_diesel::AsyncRunQueryDsl; use async_trait::async_trait; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; +use diesel::result::Error as DieselError; /// A wrapper around a runnable Diesel query, which EXPLAINs what it is doing. /// @@ -49,7 +50,7 @@ pub trait ExplainableAsync { async fn explain_async( self, conn: &async_bb8_diesel::Connection, - ) -> Result; + ) -> Result; } #[async_trait] @@ -65,7 +66,7 @@ where async fn explain_async( self, conn: &async_bb8_diesel::Connection, - ) -> Result { + ) -> Result { Ok(ExplainStatement { query: self } .get_results_async::(conn) .await? diff --git a/nexus/db-queries/src/db/pool.rs b/nexus/db-queries/src/db/pool.rs index 6311121bd1..73c95f4e91 100644 --- a/nexus/db-queries/src/db/pool.rs +++ b/nexus/db-queries/src/db/pool.rs @@ -99,7 +99,9 @@ impl CustomizeConnection, ConnectionError> &self, conn: &mut Connection, ) -> Result<(), ConnectionError> { - conn.batch_execute_async(DISALLOW_FULL_TABLE_SCAN_SQL).await + conn.batch_execute_async(DISALLOW_FULL_TABLE_SCAN_SQL) + .await + .map_err(|e| e.into()) } } diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 18360e1045..cf182e080d 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -20,6 +20,7 @@ use diesel::query_builder::AstPass; use diesel::query_builder::Query; use diesel::query_builder::QueryFragment; use diesel::query_builder::QueryId; +use diesel::result::Error as DieselError; use diesel::sql_types; use diesel::Column; use diesel::Expression; @@ -42,7 +43,7 @@ const REALLOCATION_WITH_DIFFERENT_IP_SENTINEL: &'static str = "Reallocation of IP with different value"; /// Translates a generic pool error to an external error. -pub fn from_diesel(e: async_bb8_diesel::ConnectionError) -> external::Error { +pub fn from_diesel(e: DieselError) -> external::Error { use crate::db::error; let sentinels = [REALLOCATION_WITH_DIFFERENT_IP_SENTINEL]; diff --git a/nexus/db-queries/src/db/queries/network_interface.rs b/nexus/db-queries/src/db/queries/network_interface.rs index 877daad9e3..bac2610b41 100644 --- a/nexus/db-queries/src/db/queries/network_interface.rs +++ b/nexus/db-queries/src/db/queries/network_interface.rs @@ -17,6 +17,7 @@ use diesel::prelude::Column; use diesel::query_builder::AstPass; use diesel::query_builder::QueryFragment; use diesel::query_builder::QueryId; +use diesel::result::Error as DieselError; use diesel::sql_types; use diesel::Insertable; use diesel::QueryResult; @@ -126,16 +127,14 @@ impl InsertError { /// address exhaustion or an attempt to attach an interface to an instance /// that is already associated with another VPC. pub fn from_diesel( - e: async_bb8_diesel::ConnectionError, + e: DieselError, interface: &IncompleteNetworkInterface, ) -> Self { use crate::db::error; - use async_bb8_diesel::ConnectionError; - use diesel::result::Error; match e { // Catch the specific errors designed to communicate the failures we // want to distinguish - ConnectionError::Query(Error::DatabaseError(_, _)) => { + DieselError::DatabaseError(_, _) => { decode_database_error(e, interface) } // Any other error at all is a bug @@ -223,13 +222,11 @@ impl InsertError { /// As such, it naturally is extremely tightly coupled to the database itself, /// including the software version and our schema. fn decode_database_error( - err: async_bb8_diesel::ConnectionError, + err: DieselError, interface: &IncompleteNetworkInterface, ) -> InsertError { use crate::db::error; - use async_bb8_diesel::ConnectionError; use diesel::result::DatabaseErrorKind; - use diesel::result::Error; // Error message generated when we attempt to insert an interface in a // different VPC from the interface(s) already associated with the instance @@ -292,10 +289,10 @@ fn decode_database_error( // If the address allocation subquery fails, we'll attempt to insert // NULL for the `ip` column. This checks that the non-NULL constraint on // that colum has been violated. - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::NotNullViolation, - ref info, - )) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { + info, + ) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { InsertError::NoAvailableIpAddresses } @@ -303,29 +300,27 @@ fn decode_database_error( // `push_ensure_unique_vpc_expression` subquery, which generates a // UUID parsing error if the resource (e.g. instance) we want to attach // to is already associated with another VPC. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => + { InsertError::ResourceSpansMultipleVpcs(interface.parent_id) } // This checks the constraint on the interface slot numbers, used to // limit total number of interfaces per resource to a maximum number. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::CheckViolation, - ref info, - )) if info.message() == NO_SLOTS_AVAILABLE_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::CheckViolation, info) + if info.message() == NO_SLOTS_AVAILABLE_ERROR_MESSAGE => + { InsertError::NoSlotsAvailable } // If the MAC allocation subquery fails, we'll attempt to insert NULL // for the `mac` column. This checks that the non-NULL constraint on // that column has been violated. - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::NotNullViolation, - ref info, - )) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { + info, + ) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { InsertError::NoMacAddrressesAvailable } @@ -333,39 +328,36 @@ fn decode_database_error( // `push_ensure_unique_vpc_subnet_expression` subquery, which generates // a UUID parsing error if the resource has another interface in the VPC // Subnet of the one we're trying to insert. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == NON_UNIQUE_VPC_SUBNET_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == NON_UNIQUE_VPC_SUBNET_ERROR_MESSAGE => + { InsertError::NonUniqueVpcSubnets } // This catches the UUID-cast failure intentionally introduced by // `push_instance_state_verification_subquery`, which verifies that // the instance is actually stopped when running this query. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == INSTANCE_BAD_STATE_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == INSTANCE_BAD_STATE_ERROR_MESSAGE => + { assert_eq!(interface.kind, NetworkInterfaceKind::Instance); InsertError::InstanceMustBeStopped(interface.parent_id) } // This catches the UUID-cast failure intentionally introduced by // `push_instance_state_verification_subquery`, which verifies that // the instance doesn't even exist when running this query. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == NO_INSTANCE_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == NO_INSTANCE_ERROR_MESSAGE => + { assert_eq!(interface.kind, NetworkInterfaceKind::Instance); InsertError::InstanceNotFound(interface.parent_id) } // This path looks specifically at constraint names. - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::UniqueViolation, ref info, - )) => match info.constraint_name() { + ) => match info.constraint_name() { // Constraint violated if a user-requested IP address has // already been assigned within the same VPC Subnet. Some(constraint) if constraint == IP_NOT_AVAILABLE_CONSTRAINT => { @@ -1550,17 +1542,12 @@ impl DeleteError { /// can generate, specifically the intentional errors that indicate that /// either the instance is still running, or that the instance has one or /// more secondary interfaces. - pub fn from_diesel( - e: async_bb8_diesel::ConnectionError, - query: &DeleteQuery, - ) -> Self { + pub fn from_diesel(e: DieselError, query: &DeleteQuery) -> Self { use crate::db::error; - use async_bb8_diesel::ConnectionError; - use diesel::result::Error; match e { // Catch the specific errors designed to communicate the failures we // want to distinguish - ConnectionError::Query(Error::DatabaseError(_, _)) => { + DieselError::DatabaseError(_, _) => { decode_delete_network_interface_database_error( e, query.parent_id, @@ -1608,13 +1595,11 @@ impl DeleteError { /// As such, it naturally is extremely tightly coupled to the database itself, /// including the software version and our schema. fn decode_delete_network_interface_database_error( - err: async_bb8_diesel::ConnectionError, + err: DieselError, parent_id: Uuid, ) -> DeleteError { use crate::db::error; - use async_bb8_diesel::ConnectionError; use diesel::result::DatabaseErrorKind; - use diesel::result::Error; // Error message generated when we're attempting to delete a primary // interface, and that instance also has one or more secondary interfaces @@ -1627,29 +1612,26 @@ fn decode_delete_network_interface_database_error( // first CTE, which generates a UUID parsing error if we're trying to // delete the primary interface, and the instance also has one or more // secondaries. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == HAS_SECONDARIES_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, ref info) + if info.message() == HAS_SECONDARIES_ERROR_MESSAGE => + { DeleteError::SecondariesExist(parent_id) } // This catches the UUID-cast failure intentionally introduced by // `push_instance_state_verification_subquery`, which verifies that // the instance can be worked on when running this query. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == INSTANCE_BAD_STATE_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, ref info) + if info.message() == INSTANCE_BAD_STATE_ERROR_MESSAGE => + { DeleteError::InstanceBadState(parent_id) } // This catches the UUID-cast failure intentionally introduced by // `push_instance_state_verification_subquery`, which verifies that // the instance doesn't even exist when running this query. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) if info.message() == NO_INSTANCE_ERROR_MESSAGE => { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, ref info) + if info.message() == NO_INSTANCE_ERROR_MESSAGE => + { DeleteError::InstanceNotFound(parent_id) } diff --git a/nexus/db-queries/src/db/queries/region_allocation.rs b/nexus/db-queries/src/db/queries/region_allocation.rs index 7f7b2ea9bf..a080af4c37 100644 --- a/nexus/db-queries/src/db/queries/region_allocation.rs +++ b/nexus/db-queries/src/db/queries/region_allocation.rs @@ -14,6 +14,7 @@ use crate::db::true_or_cast_error::{matches_sentinel, TrueOrCastError}; use db_macros::Subquery; use diesel::pg::Pg; use diesel::query_builder::{AstPass, Query, QueryFragment, QueryId}; +use diesel::result::Error as DieselError; use diesel::PgBinaryExpressionMethods; use diesel::{ sql_types, BoolExpressionMethods, Column, CombineDsl, ExpressionMethods, @@ -36,7 +37,7 @@ const NOT_ENOUGH_UNIQUE_ZPOOLS_SENTINEL: &'static str = /// Translates a generic pool error to an external error based /// on messages which may be emitted during region provisioning. -pub fn from_diesel(e: async_bb8_diesel::ConnectionError) -> external::Error { +pub fn from_diesel(e: DieselError) -> external::Error { use crate::db::error; let sentinels = [ diff --git a/nexus/db-queries/src/db/queries/vpc_subnet.rs b/nexus/db-queries/src/db/queries/vpc_subnet.rs index bbb229da1e..9ddec32080 100644 --- a/nexus/db-queries/src/db/queries/vpc_subnet.rs +++ b/nexus/db-queries/src/db/queries/vpc_subnet.rs @@ -11,6 +11,7 @@ use chrono::{DateTime, Utc}; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; +use diesel::result::Error as DieselError; use diesel::sql_types; use omicron_common::api::external; use ref_cast::RefCast; @@ -28,14 +29,9 @@ pub enum SubnetError { impl SubnetError { /// Construct a `SubnetError` from a Diesel error, catching the desired /// cases and building useful errors. - pub fn from_diesel( - e: async_bb8_diesel::ConnectionError, - subnet: &VpcSubnet, - ) -> Self { + pub fn from_diesel(e: DieselError, subnet: &VpcSubnet) -> Self { use crate::db::error; - use async_bb8_diesel::ConnectionError; use diesel::result::DatabaseErrorKind; - use diesel::result::Error; const IPV4_OVERLAP_ERROR_MESSAGE: &str = r#"null value in column "ipv4_block" violates not-null constraint"#; const IPV6_OVERLAP_ERROR_MESSAGE: &str = @@ -43,26 +39,26 @@ impl SubnetError { const NAME_CONFLICT_CONSTRAINT: &str = "vpc_subnet_vpc_id_name_key"; match e { // Attempt to insert overlapping IPv4 subnet - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::NotNullViolation, ref info, - )) if info.message() == IPV4_OVERLAP_ERROR_MESSAGE => { + ) if info.message() == IPV4_OVERLAP_ERROR_MESSAGE => { SubnetError::OverlappingIpRange(subnet.ipv4_block.0 .0.into()) } // Attempt to insert overlapping IPv6 subnet - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::NotNullViolation, ref info, - )) if info.message() == IPV6_OVERLAP_ERROR_MESSAGE => { + ) if info.message() == IPV6_OVERLAP_ERROR_MESSAGE => { SubnetError::OverlappingIpRange(subnet.ipv6_block.0 .0.into()) } // Conflicting name for the subnet within a VPC - ConnectionError::Query(Error::DatabaseError( + DieselError::DatabaseError( DatabaseErrorKind::UniqueViolation, ref info, - )) if info.constraint_name() == Some(NAME_CONFLICT_CONSTRAINT) => { + ) if info.constraint_name() == Some(NAME_CONFLICT_CONSTRAINT) => { SubnetError::External(error::public_error_from_diesel( e, error::ErrorHandler::Conflict( diff --git a/nexus/db-queries/src/db/true_or_cast_error.rs b/nexus/db-queries/src/db/true_or_cast_error.rs index e04d865182..6d7b2a1dbd 100644 --- a/nexus/db-queries/src/db/true_or_cast_error.rs +++ b/nexus/db-queries/src/db/true_or_cast_error.rs @@ -9,6 +9,7 @@ use diesel::pg::Pg; use diesel::query_builder::AstPass; use diesel::query_builder::QueryFragment; use diesel::query_builder::QueryId; +use diesel::result::Error as DieselError; use diesel::Expression; use diesel::SelectableExpression; @@ -77,10 +78,9 @@ where /// Returns one of the sentinels if it matches the expected value from /// a [`TrueOrCastError`]. pub fn matches_sentinel( - e: &async_bb8_diesel::ConnectionError, + e: &DieselError, sentinels: &[&'static str], ) -> Option<&'static str> { - use async_bb8_diesel::ConnectionError; use diesel::result::DatabaseErrorKind; use diesel::result::Error; @@ -93,10 +93,7 @@ pub fn matches_sentinel( match e { // Catch the specific errors designed to communicate the failures we // want to distinguish. - ConnectionError::Query(Error::DatabaseError( - DatabaseErrorKind::Unknown, - ref info, - )) => { + Error::DatabaseError(DatabaseErrorKind::Unknown, info) => { for sentinel in sentinels { if info.message() == bool_parse_error(sentinel) { return Some(sentinel); diff --git a/nexus/db-queries/src/db/update_and_check.rs b/nexus/db-queries/src/db/update_and_check.rs index 96cb3e4c79..d6bf14c083 100644 --- a/nexus/db-queries/src/db/update_and_check.rs +++ b/nexus/db-queries/src/db/update_and_check.rs @@ -12,6 +12,7 @@ use diesel::prelude::*; use diesel::query_builder::*; use diesel::query_dsl::methods::LoadQuery; use diesel::query_source::Table; +use diesel::result::Error as DieselError; use diesel::sql_types::Nullable; use diesel::QuerySource; use std::marker::PhantomData; @@ -156,7 +157,7 @@ where pub async fn execute_and_check( self, conn: &async_bb8_diesel::Connection, - ) -> Result, async_bb8_diesel::ConnectionError> + ) -> Result, DieselError> where // We require this bound to ensure that "Self" is runnable as query. Self: LoadQuery<'static, DbConnection, (Option, Option, Q)>, diff --git a/nexus/src/app/sagas/disk_create.rs b/nexus/src/app/sagas/disk_create.rs index 275c8738cc..fe403a7d41 100644 --- a/nexus/src/app/sagas/disk_create.rs +++ b/nexus/src/app/sagas/disk_create.rs @@ -832,9 +832,10 @@ pub(crate) mod test { }; use async_bb8_diesel::{ AsyncConnection, AsyncRunQueryDsl, AsyncSimpleConnection, - OptionalExtension, }; - use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; + use diesel::{ + ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper, + }; use dropshot::test_util::ClientTestContext; use nexus_db_queries::context::OpContext; use nexus_db_queries::{authn::saga::Serialized, db::datastore::DataStore}; diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 6fc93ce8db..2762ecaff3 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -1372,10 +1372,10 @@ pub mod test { }; use async_bb8_diesel::{ AsyncConnection, AsyncRunQueryDsl, AsyncSimpleConnection, - OptionalExtension, }; use diesel::{ - BoolExpressionMethods, ExpressionMethods, QueryDsl, SelectableHelper, + BoolExpressionMethods, ExpressionMethods, OptionalExtension, QueryDsl, + SelectableHelper, }; use dropshot::test_util::ClientTestContext; use nexus_db_queries::authn::saga::Serialized; diff --git a/nexus/src/app/sagas/project_create.rs b/nexus/src/app/sagas/project_create.rs index 1cbf9070ee..135e20ff06 100644 --- a/nexus/src/app/sagas/project_create.rs +++ b/nexus/src/app/sagas/project_create.rs @@ -159,9 +159,10 @@ mod test { }; use async_bb8_diesel::{ AsyncConnection, AsyncRunQueryDsl, AsyncSimpleConnection, - OptionalExtension, }; - use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; + use diesel::{ + ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper, + }; use nexus_db_queries::{ authn::saga::Serialized, authz, context::OpContext, db::datastore::DataStore, diff --git a/nexus/src/app/sagas/snapshot_create.rs b/nexus/src/app/sagas/snapshot_create.rs index 9c8a33fb17..0b3c5c99d7 100644 --- a/nexus/src/app/sagas/snapshot_create.rs +++ b/nexus/src/app/sagas/snapshot_create.rs @@ -1568,8 +1568,10 @@ mod test { use crate::app::sagas::test_helpers; use crate::app::test_interfaces::TestInterfaces; use crate::external_api::shared::IpRange; - use async_bb8_diesel::{AsyncRunQueryDsl, OptionalExtension}; - use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; + use async_bb8_diesel::AsyncRunQueryDsl; + use diesel::{ + ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper, + }; use dropshot::test_util::ClientTestContext; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; diff --git a/nexus/src/app/sagas/vpc_create.rs b/nexus/src/app/sagas/vpc_create.rs index 85eed6616d..4b5bedf41e 100644 --- a/nexus/src/app/sagas/vpc_create.rs +++ b/nexus/src/app/sagas/vpc_create.rs @@ -445,8 +445,10 @@ pub(crate) mod test { app::saga::create_saga_dag, app::sagas::vpc_create::Params, app::sagas::vpc_create::SagaVpcCreate, external_api::params, }; - use async_bb8_diesel::{AsyncRunQueryDsl, OptionalExtension}; - use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; + use async_bb8_diesel::AsyncRunQueryDsl; + use diesel::{ + ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper, + }; use dropshot::test_util::ClientTestContext; use nexus_db_queries::{ authn::saga::Serialized, authz, context::OpContext,