Closed gliderkite closed 4 years ago
Please make a concrete suggestions how a more flexible API would look like if we remove that bound. Also keep in mind that every use case that is supported today should be supported in the future, that includes using a custom error type as return type of the transaction callback.
I'm closing this issue now, because it is currently not actionable for the diesel team. If you have any concrete suggestions we can talk about reopening this issue.
I wasn't aware of this policy in Diesel
(this was the first issue I opened); in fact I see many open tickets that are not actionable today but still open, for example https://github.com/diesel-rs/diesel/issues/2084, that by chance redirected me to this comment, that could be applied here as well https://github.com/diesel-rs/diesel/issues/399#issuecomment-603491154. I'll reopen this issue in case I'll find the time to think to an alternative, and leave to you the job to redirect other people opening similar issues to this closed one.
I probably should explain why I closed this issue a bit more in detail. The corresponding code for the transaction function is here , to show what we are talking about. We have the following constraints there:
This leave us in my point of view with exactly two choices:
From<diesel::result::Error>
bound there.std::error::Error
or something like that.That means in the end both solutions would require a trait bound on the user provided error type. So in the general case non of those solutions will solve your problem.
in fact I see many open tickets that are not actionable today but still open, for example #2084,
I consider this issue as actionable, see #2257.
I'll reopen this issue in case I'll find the time to think to an alternative, and leave to you the job to redirect other people opening similar issues to this closed one.
To make it clear again: Closing a issue does not mean the discussion here is done, just that I do not see what could be done immediately to solve the issue. Basically all of the open issues should contain something a potential contributor could pick up and work on. (I know that is not necessarily the case for some older issue)
As a potential workaround for your concrete issue: You could just do your own error type that wraps diesel::result::Error
and the third party error type into a common type. So something like
enum MyError<E> {
DieselError(diesel::result::Error),
LibraryError(E)
}
impl<E> From<diesel::result::Error> for MyError<E> {
fn from(e: diesel::result::Error) -> Self {
MyError::DieselError(e)
}
}
I'm facing the same issue, I'm using http-api-problem which provides an RFC7807 compliant error type in my web application, and naturally I want my to be able to return these errors from within a transaction.
This seems like a very reasonable use case to me, so I think the current API is very ununergonomic. I can suggest some possible solutions on the diesel side, otherwise creating all these wrappers means a lot of boilerplate code
pub enum TransactionError<E> {
DieselError(diesel::result::Error),
UserError(E)
}
fn transaction<T, E, F>(&self, f: F) -> Result<T, TransactionError<E>>
where
F: FnOnce() -> Result<T, E>,
fn transaction<T, E, F, M>(&self, f: F, m: M) -> Result<T, E>
where
F: FnOnce() -> Result<T, E>,
M: FnOnce(diesel::result::Error) -> E
@jacob-pro Both of your proposals suffer from the same issue. If you want to handle any error inside of the transaction closure using the ?
operator you need to have a From<diesel::result::Error>
impl for the return type to convert any query error to your custom result type. Any possible solution need to provide this feature as this is basically mandatory for normal rust error handling code.
That written: You solution 1 is basically the implemented api. Just define that enum on your own and add a corresponding From
impl.
Btw: Calling a specific API "unergonomic" just because it does not fit your special use case is not a good way to start any discussion, specially if you did not even try to include some concrete code why it is not working for your usecase.
I'm answering https://github.com/diesel-rs/diesel/pull/3102#issuecomment-1086290359 here because I think this is a more appropriate place for it (as reference for others who may have the same issue).
So my usecase is when building an API (using actix), I want to return a standardised
http_api_problem
error (which is defined in an external crate) from each of the route handlers.Defining the enum in my own crate does work, its just that then I have this boilerplate code that I either: (a) have to copy and paste everywhere, or (b) create a very tiny library just containing the code I have included in this PR, it would in my opinion be preferable to have this as part of the diesel API itself...
So this is a valid concern, but it the usecase I described the same problem exists even without transactions, i.e. say I want to perform just a simple database query, I already have to have my own function trait for converting the disel error to the
http_api_problem
error in the firstplace since I can't implement From.
Specifically about:
i.e. say I want to perform just a simple database query, I already have to have my own function trait for converting the disel error to the http_api_problem error in the firstplace since I can't implement From.
I think a better interface than https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/error.rs to solve this issue would be to do this:
pub struct CalpolError {
// For instance, but could also be an enum of all the things that could go wrong, or box dyn stuff...
api_error: ApiError
}
pub fn internal_server_error<E: std::fmt::Display>(prefix: &str, error: E) -> CalpolError {
let builder = ApiError::builder(StatusCode::INTERNAL_SERVER_ERROR);
CalpolError {
api_error: if cfg!(debug_assertions) {
builder.message(format!("{}: {}", prefix, error)).finish()
} else {
builder.finish()
}
}
}
impl From<actix_utils::real_ip::RealIpAddressError> for CalpolError {
fn from(e: actix_utils::real_ip::RealIpAddressError) -> CalpolError {
internal_server_error("RealIpAddressError", self)
}
}
impl From<CalpolError> for ApiError {
fn form(e: CalpolError) -> ApiError {
e.api_error
}
}
You would then only need to update your response_mapper
(https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/mod.rs#L29-L37) into:
pub fn response_mapper<T, E>(response: Result<T, E>) -> Result<HttpResponse, ApiError>
where
T: Serialize,
E: Into<ApiError>,
{
response
.map(|value| HttpResponse::Ok().json(value))
.map_err(|e| e.into())
}
and you could then use your CalpolError
type everywhere else in your app instead of ApiError
, which would:
map_api_error()
boilerplate across your project, since thanks to the From
implementations you could just use the ?
.ApiError
or store something else inside? Maybe you'll want to report some of the errors to an error reporting system such as Sentry before sending error 500 to the end-user?)From
you need.While this may seem complex at first sight because you introduce your own type, I'm pretty confident that you'll find this to be a more comfortable design in the end.
then I have this boilerplate code that I either: (a) have to copy and paste everywhere, or (b) create a very tiny library just containing the code I have included in this PR, it would in my opinion be preferable to have this as part of the diesel API itself...
This code: https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/error.rs
that you're using to solve 3. already generates more boilerplate (especially through all the map_api_error
calls it enforces) than the above proposed design.
In conclusion, I believe introducing the code proposed in #3102 would generally ease using a design that isn't optimal instead of encouraging users to use a more appropriate rust-idiomatic design, so I would conclude we should not do that (apart from the typo fix ^^).
@Ten0 this is a really good idea! Thank you!!
Feel free to close the associated PR :)
I am working on a crate that makes use of an Error type defined in a dependency library, which I use as return type of a method that needs to be run as part of a transaction, such as for example:
This results in the following error when compiling:
AFAIK it's it not possible for me to create the
From<Error>
implementation required by this method, because themy_lib::Error
type is defined in another crate. And I have to create my ownTransactionError
dummy type just to map themy_lib::Error
(and map it back at the end of the transaction).Is there a strong reason to have this Trait bound in place? Would it be possible to remove it and make the API more flexible?