This came up when I was talking to @smklein about #326, and then I ran into head-on while trying to figure out appropriate return types for the session-related datastore methods.
Right now the datastore methods return *Result wrappers:
|
/** Result of a create operation for the specified type */ |
|
pub type CreateResult<T> = Result<T, Error>; |
|
/** Result of a delete operation for the specified type */ |
|
pub type DeleteResult = Result<(), Error>; |
|
/** Result of a list operation that returns an ObjectStream */ |
|
pub type ListResult<T> = Result<ObjectStream<T>, Error>; |
|
/** Result of a list operation that returns a vector */ |
|
pub type ListResultVec<T> = Result<Vec<T>, Error>; |
|
/** Result of a lookup operation for the specified type */ |
|
pub type LookupResult<T> = Result<T, Error>; |
|
/** Result of an update operation for the specified type */ |
|
pub type UpdateResult<T> = Result<T, Error>; |
with user-facing Errors in the error slot:
|
pub enum Error { |
|
/** An object needed as part of this operation was not found. */ |
|
#[error("Object (of type {lookup_type:?}) not found: {type_name}")] |
|
ObjectNotFound { type_name: ResourceType, lookup_type: LookupType }, |
|
/** An object already exists with the specified name or identifier. */ |
|
#[error("Object (of type {type_name:?}) already exists: {object_name}")] |
|
ObjectAlreadyExists { type_name: ResourceType, object_name: String }, |
|
/** |
|
* The request was well-formed, but the operation cannot be completed given |
|
* the current state of the system. |
|
*/ |
|
#[error("Invalid Request: {message}")] |
|
InvalidRequest { message: String }, |
|
/** The specified input field is not valid. */ |
|
#[error("Invalid Value: {label}, {message}")] |
|
InvalidValue { label: String, message: String }, |
|
/** The system encountered an unhandled operational error. */ |
|
#[error("Internal Error: {message}")] |
|
InternalError { message: String }, |
|
/** The system (or part of it) is unavailable. */ |
|
#[error("Service Unavailable: {message}")] |
|
ServiceUnavailable { message: String }, |
|
} |
We generally propagate these up from datastore methods to the response with ? in nexus.rs and the route handlers. This means the database query methods are essentially deciding the external-facing meaning of their errors, which doesn't make sense because the same error could have different meanings in different contexts. For example, when we look up a session and don't find it, that's not going to become a 404, that's probably going to be a 401. We can return a 404 from datastore.session_fetch and convert it in the cookie auth middleware (essentially what we're doing already), but as @davepacheco pointed out here, that makes it easy to accidentally bubble up an error to the user that we don't want them to see.
We got a big architectural improvement by separating the database models from the external views of those models (which currently live in common/external for sharing purposes, but will move into Nexus once we have client generation across the board). I think we should have a separate Error enum for datastore functions, which are then converted explicitly into external errors at least one level higher in the chain. Concretely, this should mostly just mean moving these map_errs:
|
pub async fn organization_fetch( |
|
&self, |
|
name: &Name, |
|
) -> LookupResult<Organization> { |
|
use db::schema::organization::dsl; |
|
dsl::organization |
|
.filter(dsl::time_deleted.is_null()) |
|
.filter(dsl::name.eq(name.clone())) |
|
.select(Organization::as_select()) |
|
.first_async::<Organization>(self.pool()) |
|
.await |
|
.map_err(|e| { |
|
public_error_from_diesel_pool( |
|
e, |
|
ResourceType::Organization, |
|
LookupType::ByName(name.as_str().to_owned()), |
|
) |
|
}) |
|
} |
up into nexus.rs
|
pub async fn organization_fetch( |
|
&self, |
|
name: &Name, |
|
) -> LookupResult<db::model::Organization> { |
|
self.db_datastore.organization_fetch(name).await |
|
} |
The fact that many of these methods do so little always felt like a code smell, like we hadn't quite figured out what the Nexus service layer is for. Maybe this is why.
This came up when I was talking to @smklein about #326, and then I ran into head-on while trying to figure out appropriate return types for the session-related datastore methods.
Right now the datastore methods return
*Resultwrappers:omicron/common/src/api/external/mod.rs
Lines 40 to 51 in 4466bee
with user-facing
Errors in the error slot:omicron/common/src/api/external/error.rs
Lines 29 to 51 in 4466bee
We generally propagate these up from datastore methods to the response with
?innexus.rsand the route handlers. This means the database query methods are essentially deciding the external-facing meaning of their errors, which doesn't make sense because the same error could have different meanings in different contexts. For example, when we look up a session and don't find it, that's not going to become a 404, that's probably going to be a 401. We can return a 404 fromdatastore.session_fetchand convert it in the cookie auth middleware (essentially what we're doing already), but as @davepacheco pointed out here, that makes it easy to accidentally bubble up an error to the user that we don't want them to see.We got a big architectural improvement by separating the database models from the external views of those models (which currently live in common/external for sharing purposes, but will move into Nexus once we have client generation across the board). I think we should have a separate
Errorenum for datastore functions, which are then converted explicitly into external errors at least one level higher in the chain. Concretely, this should mostly just mean moving thesemap_errs:omicron/nexus/src/db/datastore.rs
Lines 153 to 171 in 4466bee
up into
nexus.rsomicron/nexus/src/nexus.rs
Lines 404 to 409 in 4466bee
The fact that many of these methods do so little always felt like a code smell, like we hadn't quite figured out what the
Nexusservice layer is for. Maybe this is why.