Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow returning custom error types in Result #92

Open
JonahLund opened this issue Oct 20, 2023 · 6 comments
Open

Allow returning custom error types in Result #92

JonahLund opened this issue Oct 20, 2023 · 6 comments

Comments

@JonahLund
Copy link

Currently it does not work to return a custom error type that implements IntoResponse, it expects the error type to be viz::Error

Example

use std::net::SocketAddr;
use viz::{IntoResponse, Request, Result, Router, Server, ServiceMaker};

#[derive(Debug, thiserror::Error)]
#[error("{message}")]
pub struct MyError {
    message: String,
}

impl IntoResponse for MyError {
    fn into_response(self) -> viz::Response {
        viz::Response::builder()
            .body(viz::Body::from(self.message.to_string()))
            .unwrap()
    }
}

async fn index(_: Request) -> Result<&'static str, MyError> {
    Ok("Hello Viz")
}

#[tokio::main]
async fn main() -> Result<()> {
    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    println!("listening on {addr}");

    let app = Router::new().get("/", index);

    if let Err(err) = Server::bind(&addr).serve(ServiceMaker::from(app)).await {
        println!("{err}");
    }

    Ok(())
}
error[E0271]: expected `impl Future<Output = Result<&str, MyError>>` to be a future that resolves to `Result<_, Error>`, but it resolves to `Result<&str, MyError>`
   --> src/main.rs:27:38
    |
27  |     let app = Router::new().get("/", index);
    |                             ---      ^^^^^ expected `Result<_, Error>`, found `Result<&str, MyError>`
    |                             |
    |                             required by a bound introduced by this call
    |
    = note: expected enum `Result<_, viz::Error>`
               found enum `Result<&'static str, MyError>`

Version 0.4.17

@fundon
Copy link
Member

fundon commented Oct 21, 2023

Thanks for the feedback.

It's not currently supported, an async function should return Result<T, Error> in Viz,

But we can make some adjustments:

  • Implement from MyError for Error
impl From<MyError> for Error {
    fn from(value: MyError) -> Self {
        Error::Responder(value.into_response())
    }
}

async fn index(_: Request) -> Result<&'static str> {
    Err(MyError {
        message: "my error".to_string(),
    })?;
    Ok("Hello Viz")
}
  • Also, we can add our main logic in index function and add index_wrapped for router.
async fn index_wrapped(req: Request) -> Result<&'static str> {
    Ok(index(req).await?)
}

let app = Router::new().get("/", index_wrapped);
  • Maybe we need add a function: map_custom_error in HandlerExt

poc

    let app = Router::new().get("/", index.map_custom_error(IntoResponse::into_response));

@JonahLund
Copy link
Author

JonahLund commented Oct 21, 2023

Yes that is indeed a possible solution
However in the case that you would want full customization of the possible error messages, having your own error type that implements From<viz::Error> aswell as From<viz::PayloadError> can be very beneficial since it allows you to do things like this:

// custom data structure representing the error body
#[derive(Debug, thiserror::Error)]
pub struct MyError {
  message: String,
}

impl From<viz::Error> for MyError {
  fn from(value: viz::Error) -> Self {
    // return whatever makes sense for viz::Error
    Self {
      message: "Something went wrong. Please try again later".into()
    }   
  }
}

impl From<viz::PayloadError> for MyError {
  fn from(value: viz::PayloadError) -> Self {
    Self {
      message: value.to_string(),
    }
  }
}

impl IntoResponse for MyError { .. }

type ApiResult<T, E = MyError> = Result<T, E>;

async fn index(mut req: viz::Request) -> ApiResult<&'static str> {
  // since MyError implements From<viz::PayloadError> we can use '?'
  let form = req.form::<RegisterForm>().await?;

  Ok("Success")
}

Which gives you a lot of flexibility over your errors

@fundon
Copy link
Member

fundon commented Oct 21, 2023

Good idea. 👍

A lot of code needs to be redesigned(handler, router). I will try it. And PR is welcome!

@fundon
Copy link
Member

fundon commented Jan 1, 2024

I think we can add a trait TryHandler for this, likes TryFuture.

@JonahLund
Copy link
Author

Axum's approach is to allow anything that implements IntoResponse to be returned from a handler, so it doesn't treat Result::Ok or Result::Err differently, they just have to implement IntoResponse, which provides maximum flexibility

@fundon
Copy link
Member

fundon commented Jan 2, 2024

https://viz.rs/en/0.4.x/concepts/error-handling

  1. Why not just return Response?

Since in a real-world scenario, you need to determine regular errors in files, IO, DB, etc., returning Result is the most appropriate. It is also possible to use ? operator to return an error early and respond to the client.

  1. Why not just return impl IntoResponse?

Although the IntoResponse feature has been implemented for Result, there is a special case if T = Result<R>, in which fatten is not stable, the result cannot be tied yet, so it cannot be returned properly.

I think, the compromise solution is to add a type Error; on Handler and return Result<Self::Output, Self::Error>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants