Skip to content

Commit a903d61

Browse files
smkleinsmklein
authored
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.
1 parent 7d33544 commit a903d61

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+291
-391
lines changed

‎Cargo.lock

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ api_identity = { path = "api_identity" }
136136
approx = "0.5.1"
137137
assert_matches = "1.5.0"
138138
assert_cmd = "2.0.12"
139-
async-bb8-diesel = { git = "https://github.com/oxidecomputer/async-bb8-diesel", rev = "da04c087f835a51e0441addb19c5ef4986e1fcf2" }
139+
async-bb8-diesel = { git = "https://github.com/oxidecomputer/async-bb8-diesel", rev = "1446f7e0c1f05f33a0581abd51fa873c7652ab61" }
140140
async-trait = "0.1.73"
141141
atomicwrites = "0.4.1"
142142
authz-macros = { path = "nexus/authz-macros" }

‎nexus/db-queries/src/db/collection_attach.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use super::cte_utils::{
1717
QueryFromClause, QuerySqlType, TableDefaultWhereClause,
1818
};
1919
use super::pool::DbConnection;
20-
use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError};
20+
use async_bb8_diesel::AsyncRunQueryDsl;
2121
use diesel::associations::HasTable;
2222
use diesel::expression::{AsExpression, Expression};
2323
use diesel::helper_types::*;
@@ -26,6 +26,7 @@ use diesel::prelude::*;
2626
use diesel::query_builder::*;
2727
use diesel::query_dsl::methods as query_methods;
2828
use diesel::query_source::Table;
29+
use diesel::result::Error as DieselError;
2930
use diesel::sql_types::{BigInt, Nullable, SingleValue};
3031
use nexus_db_model::DatastoreAttachTargetConfig;
3132
use std::fmt::Debug;
@@ -299,7 +300,7 @@ where
299300

300301
/// Result of [`AttachToCollectionStatement`] when executed asynchronously
301302
pub type AsyncAttachToCollectionResult<ResourceType, C> =
302-
Result<(C, ResourceType), AttachError<ResourceType, C, ConnectionError>>;
303+
Result<(C, ResourceType), AttachError<ResourceType, C, DieselError>>;
303304

304305
/// Errors returned by [`AttachToCollectionStatement`].
305306
#[derive(Debug)]
@@ -998,9 +999,8 @@ mod test {
998999
.set(resource::dsl::collection_id.eq(collection_id)),
9991000
);
10001001

1001-
type TxnError = TransactionError<
1002-
AttachError<Resource, Collection, ConnectionError>,
1003-
>;
1002+
type TxnError =
1003+
TransactionError<AttachError<Resource, Collection, DieselError>>;
10041004
let result = conn
10051005
.transaction_async(|conn| async move {
10061006
attach_query.attach_and_get_result_async(&conn).await.map_err(

‎nexus/db-queries/src/db/collection_detach.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use super::cte_utils::{
1616
QueryFromClause, QuerySqlType,
1717
};
1818
use super::pool::DbConnection;
19-
use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError};
19+
use async_bb8_diesel::AsyncRunQueryDsl;
2020
use diesel::associations::HasTable;
2121
use diesel::expression::{AsExpression, Expression};
2222
use diesel::helper_types::*;
@@ -25,6 +25,7 @@ use diesel::prelude::*;
2525
use diesel::query_builder::*;
2626
use diesel::query_dsl::methods as query_methods;
2727
use diesel::query_source::Table;
28+
use diesel::result::Error as DieselError;
2829
use diesel::sql_types::{Nullable, SingleValue};
2930
use nexus_db_model::DatastoreAttachTargetConfig;
3031
use std::fmt::Debug;
@@ -230,7 +231,7 @@ where
230231

231232
/// Result of [`DetachFromCollectionStatement`] when executed asynchronously
232233
pub type AsyncDetachFromCollectionResult<ResourceType, C> =
233-
Result<ResourceType, DetachError<ResourceType, C, ConnectionError>>;
234+
Result<ResourceType, DetachError<ResourceType, C, DieselError>>;
234235

235236
/// Errors returned by [`DetachFromCollectionStatement`].
236237
#[derive(Debug)]

‎nexus/db-queries/src/db/collection_detach_many.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ use diesel::prelude::*;
2525
use diesel::query_builder::*;
2626
use diesel::query_dsl::methods as query_methods;
2727
use diesel::query_source::Table;
28+
use diesel::result::Error as DieselError;
2829
use diesel::sql_types::{Nullable, SingleValue};
2930
use nexus_db_model::DatastoreAttachTargetConfig;
3031
use std::fmt::Debug;
@@ -241,7 +242,7 @@ where
241242

242243
/// Result of [`DetachManyFromCollectionStatement`] when executed asynchronously
243244
pub type AsyncDetachManyFromCollectionResult<C> =
244-
Result<C, DetachManyError<C, async_bb8_diesel::ConnectionError>>;
245+
Result<C, DetachManyError<C, DieselError>>;
245246

246247
/// Errors returned by [`DetachManyFromCollectionStatement`].
247248
#[derive(Debug)]
@@ -918,9 +919,8 @@ mod test {
918919
.set(resource::dsl::collection_id.eq(Option::<Uuid>::None)),
919920
);
920921

921-
type TxnError = TransactionError<
922-
DetachManyError<Collection, async_bb8_diesel::ConnectionError>,
923-
>;
922+
type TxnError =
923+
TransactionError<DetachManyError<Collection, DieselError>>;
924924
let result = conn
925925
.transaction_async(|conn| async move {
926926
detach_query.detach_and_get_result_async(&conn).await.map_err(

‎nexus/db-queries/src/db/collection_insert.rs

+8-10
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,15 @@
1010
//! 3) inserts the child resource row
1111
1212
use super::pool::DbConnection;
13-
use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError};
13+
use async_bb8_diesel::AsyncRunQueryDsl;
1414
use diesel::associations::HasTable;
1515
use diesel::helper_types::*;
1616
use diesel::pg::Pg;
1717
use diesel::prelude::*;
1818
use diesel::query_builder::*;
1919
use diesel::query_dsl::methods as query_methods;
2020
use diesel::query_source::Table;
21+
use diesel::result::Error as DieselError;
2122
use diesel::sql_types::SingleValue;
2223
use nexus_db_model::DatastoreCollectionConfig;
2324
use std::fmt::Debug;
@@ -170,7 +171,7 @@ pub enum AsyncInsertError {
170171
/// The collection that the query was inserting into does not exist
171172
CollectionNotFound,
172173
/// Other database error
173-
DatabaseError(ConnectionError),
174+
DatabaseError(DieselError),
174175
}
175176

176177
impl<ResourceType, ISR, C> InsertIntoCollectionStatement<ResourceType, ISR, C>
@@ -238,14 +239,11 @@ where
238239

239240
/// Translate from diesel errors into AsyncInsertError, handling the
240241
/// intentional division-by-zero error in the CTE.
241-
fn translate_async_error(err: ConnectionError) -> AsyncInsertError {
242-
match err {
243-
ConnectionError::Query(err)
244-
if Self::error_is_division_by_zero(&err) =>
245-
{
246-
AsyncInsertError::CollectionNotFound
247-
}
248-
other => AsyncInsertError::DatabaseError(other),
242+
fn translate_async_error(err: DieselError) -> AsyncInsertError {
243+
if Self::error_is_division_by_zero(&err) {
244+
AsyncInsertError::CollectionNotFound
245+
} else {
246+
AsyncInsertError::DatabaseError(err)
249247
}
250248
}
251249
}

‎nexus/db-queries/src/db/datastore/address_lot.rs

+12-17
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ use crate::db::error::TransactionError;
1313
use crate::db::model::Name;
1414
use crate::db::model::{AddressLot, AddressLotBlock, AddressLotReservedBlock};
1515
use crate::db::pagination::paginated;
16-
use async_bb8_diesel::{
17-
AsyncConnection, AsyncRunQueryDsl, Connection, ConnectionError,
18-
};
16+
use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, Connection};
1917
use chrono::Utc;
2018
use diesel::result::Error as DieselError;
2119
use diesel::{ExpressionMethods, QueryDsl, SelectableHelper};
@@ -84,15 +82,13 @@ impl DataStore {
8482
})
8583
.await
8684
.map_err(|e| match e {
87-
ConnectionError::Query(DieselError::DatabaseError(_, _)) => {
88-
public_error_from_diesel(
89-
e,
90-
ErrorHandler::Conflict(
91-
ResourceType::AddressLot,
92-
&params.identity.name.as_str(),
93-
),
94-
)
95-
}
85+
DieselError::DatabaseError(_, _) => public_error_from_diesel(
86+
e,
87+
ErrorHandler::Conflict(
88+
ResourceType::AddressLot,
89+
&params.identity.name.as_str(),
90+
),
91+
),
9692
_ => public_error_from_diesel(e, ErrorHandler::Server),
9793
})
9894
}
@@ -151,7 +147,7 @@ impl DataStore {
151147
})
152148
.await
153149
.map_err(|e| match e {
154-
TxnError::Connection(e) => {
150+
TxnError::Database(e) => {
155151
public_error_from_diesel(e, ErrorHandler::Server)
156152
}
157153
TxnError::CustomError(AddressLotDeleteError::LotInUse) => {
@@ -252,11 +248,10 @@ pub(crate) async fn try_reserve_block(
252248
.limit(1)
253249
.first_async::<AddressLotBlock>(conn)
254250
.await
255-
.map_err(|e| match e {
256-
ConnectionError::Query(_) => ReserveBlockTxnError::CustomError(
251+
.map_err(|_e| {
252+
ReserveBlockTxnError::CustomError(
257253
ReserveBlockError::AddressNotInLot,
258-
),
259-
e => e.into(),
254+
)
260255
})?;
261256

262257
// Ensure the address is not already taken.

‎nexus/db-queries/src/db/datastore/db_metadata.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ impl DataStore {
351351
match result {
352352
Ok(()) => Ok(()),
353353
Err(TransactionError::CustomError(())) => panic!("No custom error"),
354-
Err(TransactionError::Connection(e)) => {
354+
Err(TransactionError::Database(e)) => {
355355
Err(public_error_from_diesel(e, ErrorHandler::Server))
356356
}
357357
}

‎nexus/db-queries/src/db/datastore/device_auth.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ impl DataStore {
103103
TxnError::CustomError(TokenGrantError::TooManyRequests) => {
104104
Error::internal_error("unexpectedly found multiple device auth requests for the same user code")
105105
}
106-
TxnError::Connection(e) => {
106+
TxnError::Database(e) => {
107107
public_error_from_diesel(e, ErrorHandler::Server)
108108
}
109109
})

‎nexus/db-queries/src/db/datastore/dns.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,7 @@ impl DataStore {
395395
match result {
396396
Ok(()) => Ok(()),
397397
Err(TransactionError::CustomError(e)) => Err(e),
398-
Err(TransactionError::Connection(e)) => {
398+
Err(TransactionError::Database(e)) => {
399399
Err(public_error_from_diesel(e, ErrorHandler::Server))
400400
}
401401
}

‎nexus/db-queries/src/db/datastore/external_ip.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,9 @@ impl DataStore {
143143
) -> CreateResult<ExternalIp> {
144144
let explicit_ip = data.explicit_ip().is_some();
145145
NextExternalIp::new(data).get_result_async(conn).await.map_err(|e| {
146-
use async_bb8_diesel::ConnectionError::Query;
147146
use diesel::result::Error::NotFound;
148147
match e {
149-
Query(NotFound) => {
148+
NotFound => {
150149
if explicit_ip {
151150
Error::invalid_request(
152151
"Requested external IP address not available",

‎nexus/db-queries/src/db/datastore/ip_pool.rs

+30-34
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use crate::context::OpContext;
1010
use crate::db;
1111
use crate::db::collection_insert::AsyncInsertError;
1212
use crate::db::collection_insert::DatastoreCollection;
13-
use crate::db::error::diesel_result_optional;
1413
use crate::db::error::public_error_from_diesel;
1514
use crate::db::error::ErrorHandler;
1615
use crate::db::fixed_data::silo::INTERNAL_SILO_ID;
@@ -183,18 +182,17 @@ impl DataStore {
183182
opctx.authorize(authz::Action::Delete, authz_pool).await?;
184183

185184
// Verify there are no IP ranges still in this pool
186-
let range = diesel_result_optional(
187-
ip_pool_range::dsl::ip_pool_range
188-
.filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id()))
189-
.filter(ip_pool_range::dsl::time_deleted.is_null())
190-
.select(ip_pool_range::dsl::id)
191-
.limit(1)
192-
.first_async::<Uuid>(
193-
&*self.pool_connection_authorized(opctx).await?,
194-
)
195-
.await,
196-
)
197-
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
185+
let range = ip_pool_range::dsl::ip_pool_range
186+
.filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id()))
187+
.filter(ip_pool_range::dsl::time_deleted.is_null())
188+
.select(ip_pool_range::dsl::id)
189+
.limit(1)
190+
.first_async::<Uuid>(
191+
&*self.pool_connection_authorized(opctx).await?,
192+
)
193+
.await
194+
.optional()
195+
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
198196
if range.is_some() {
199197
return Err(Error::InvalidRequest {
200198
message:
@@ -313,7 +311,6 @@ impl DataStore {
313311
.insert_and_get_result_async(conn)
314312
.await
315313
.map_err(|e| {
316-
use async_bb8_diesel::ConnectionError::Query;
317314
use diesel::result::Error::NotFound;
318315

319316
match e {
@@ -323,7 +320,7 @@ impl DataStore {
323320
lookup_type: LookupType::ById(pool_id),
324321
}
325322
}
326-
AsyncInsertError::DatabaseError(Query(NotFound)) => {
323+
AsyncInsertError::DatabaseError(NotFound) => {
327324
// We've filtered out the IP addresses the client provided,
328325
// i.e., there's some overlap with existing addresses.
329326
Error::invalid_request(
@@ -363,26 +360,25 @@ impl DataStore {
363360
// concurrent inserts of new external IPs from the target range by
364361
// comparing the rcgen.
365362
let conn = self.pool_connection_authorized(opctx).await?;
366-
let range = diesel_result_optional(
367-
dsl::ip_pool_range
368-
.filter(dsl::ip_pool_id.eq(pool_id))
369-
.filter(dsl::first_address.eq(first_net))
370-
.filter(dsl::last_address.eq(last_net))
371-
.filter(dsl::time_deleted.is_null())
372-
.select(IpPoolRange::as_select())
373-
.get_result_async::<IpPoolRange>(&*conn)
374-
.await,
375-
)
376-
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?
377-
.ok_or_else(|| {
378-
Error::invalid_request(
379-
format!(
380-
"The provided range {}-{} does not exist",
381-
first_address, last_address,
363+
let range = dsl::ip_pool_range
364+
.filter(dsl::ip_pool_id.eq(pool_id))
365+
.filter(dsl::first_address.eq(first_net))
366+
.filter(dsl::last_address.eq(last_net))
367+
.filter(dsl::time_deleted.is_null())
368+
.select(IpPoolRange::as_select())
369+
.get_result_async::<IpPoolRange>(&*conn)
370+
.await
371+
.optional()
372+
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?
373+
.ok_or_else(|| {
374+
Error::invalid_request(
375+
format!(
376+
"The provided range {}-{} does not exist",
377+
first_address, last_address,
378+
)
379+
.as_str(),
382380
)
383-
.as_str(),
384-
)
385-
})?;
381+
})?;
386382

387383
// Find external IPs allocated out of this pool and range.
388384
let range_id = range.id;

0 commit comments

Comments
 (0)