Simplify Diesel Error management (#4210)

Depends on oxidecomputer/async-bb8-diesel#54 As of #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.
oxidecomputer · Oct 12, 2023 · a903d61 · a903d61
1 parent 7d33544
commit a903d61
Show file tree

Hide file tree

Showing 41 changed files with 291 additions and 391 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -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<ResourceType, C> =
-    Result<(C, ResourceType), AttachError<ResourceType, C, ConnectionError>>;
+    Result<(C, ResourceType), AttachError<ResourceType, C, DieselError>>;
 
 /// Errors returned by [`AttachToCollectionStatement`].
 #[derive(Debug)]
@@ -998,9 +999,8 @@ mod test {
                 .set(resource::dsl::collection_id.eq(collection_id)),
         );
 
-        type TxnError = TransactionError<
-            AttachError<Resource, Collection, ConnectionError>,
-        >;
+        type TxnError =
+            TransactionError<AttachError<Resource, Collection, DieselError>>;
         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
@@ -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<ResourceType, C> =
-    Result<ResourceType, DetachError<ResourceType, C, ConnectionError>>;
+    Result<ResourceType, DetachError<ResourceType, C, DieselError>>;
 
 /// 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
@@ -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<C> =
-    Result<C, DetachManyError<C, async_bb8_diesel::ConnectionError>>;
+    Result<C, DetachManyError<C, DieselError>>;
 
 /// Errors returned by [`DetachManyFromCollectionStatement`].
 #[derive(Debug)]
@@ -918,9 +919,8 @@ mod test {
                 .set(resource::dsl::collection_id.eq(Option::<Uuid>::None)),
         );
 
-        type TxnError = TransactionError<
-            DetachManyError<Collection, async_bb8_diesel::ConnectionError>,
-        >;
+        type TxnError =
+            TransactionError<DetachManyError<Collection, DieselError>>;
         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
@@ -10,14 +10,15 @@
 //! 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;
 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<ResourceType, ISR, C> InsertIntoCollectionStatement<ResourceType, ISR, C>
@@ -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
@@ -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,
-                            &params.identity.name.as_str(),
-                        ),
-                    )
-                }
+                DieselError::DatabaseError(_, _) => public_error_from_diesel(
+                    e,
+                    ErrorHandler::Conflict(
+                        ResourceType::AddressLot,
+                        &params.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::<AddressLotBlock>(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
@@ -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
@@ -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
@@ -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
@@ -143,10 +143,9 @@ impl DataStore {
     ) -> CreateResult<ExternalIp> {
         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
@@ -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::<Uuid>(
-                    &*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::<Uuid>(
+                &*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::<IpPoolRange>(&*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::<IpPoolRange>(&*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;