gengteng / axum-valid

axum-valid is a library that provides data validation extractors for the Axum web framework. It integrates validator, garde and validify, three popular validation crates in the Rust ecosystem, to offer convenient validation and data handling extractors for Axum applications.
MIT License
100 stars 4 forks source link

Add error implementation for `ValidRejection` #3

Closed Tortoaster closed 1 year ago

Tortoaster commented 1 year ago

Hello!

I'm currently working on a Catch type that transforms the Rejection of any FromRequest type into any other type using the From trait, allowing users to do something like this:

async fn bar(Catch(Json(foo), _): Catch<Json<Foo>, MyJsonError>) -> impl IntoResponse {
     // ...
}

My main motivation for this type is to make any parameter return JSON on error, by converting it to a project-specific error type. This solves the problem of JSON endpoints returning non-JSON when something goes wrong, and can also be used to make robust endpoints for other types of output (human-readable, yaml, etc.).

However, when I try to use this type in conjunction with the Valid parameter:

async fn bar(Catch(Valid(Json(foo)), _): Catch<Valid<Json<Foo>>, MyJsonError>) -> impl IntoResponse {
     // ...
}

I get a compile-time error, because I cannot convert a ValidRejection<T> into my own error type in a meaningful way (without losing the information as to what went wrong). Implementing Debug, Display, and Error for ValidRejection would solve this problem, hence this PR.

I know the ValidRejection type already has the option to format its output as JSON, which is great! However, the Catch type can support other types of output, and support all of them within one binary (for instance, returning human-readable format in one handler and JSON in another), so I still think this can be a good addition!

gengteng commented 1 year ago

Thanks! This PR looks good. I am going to merge it.

I noticed that WithRejection from axum-extra provides similar functionality like your Catch. However, implementing Error on ValidRejection directly is also reasonable for consistency with other types.

I have added some related tests just now (at src/test.rs:568-598). It seems this behavior could also be achieved without implementing Debug/Display/Error, but having these interfaces on ValidRejection can still be useful.

Overall nice work, thanks for the contribution! I'm merging it now.

Just a friendly reminder before submitting - it would be great if you could run cargo fmt to format the code and add some unit tests to cover the changes. This will help ensure code quality and consistency.

The modifications will be included in the next version release.

gengteng commented 1 year ago

@Tortoaster

I have an alternative implementation I think may work better:

impl<E: Error + 'static> Error for ValidRejection<E> {

  fn source(&self) -> Option<&(dyn Error + 'static)> {  
    match self {
      ValidRejection::Valid(ve) => Some(ve),
      ValidRejection::Inner(e) => Some(e),
    }
  }
}

I plan to use this version instead of your original implementation. Could you please test that your Catch functionality still works properly with this Error impl?

If it does, then I will merge this in the next release. Let me know if you have any other thoughts! I appreciate you working with me to improve the crate.

Tortoaster commented 1 year ago

Yes that still works fine! I also didn't know about WithRejection, but it does the exact same thing in the exact same way, so I guess that saves some work :p Thanks!

These changes make it easy to convert rejections to a project-specific error type that uses, for instance, thiserror:

#[derive(Debug, Error)]
pub enum AppError {
    #[error("{0}")]
    Valid(#[from] axum_valid::ValidRejection<axum::extract::rejection::JsonRejection>),
}

// impl IntoResponse that returns JSON...

pub async fn foo(
    WithRejection(Valid(Json(bar)), _): WithRejection<Valid<Json<Bar>>, AppError>,
) -> Result<Json<Bar>, AppError> {
    Ok(Json(bar))
}
gengteng commented 1 year ago

Yes that still works fine! I also didn't know about WithRejection, but it does the exact same thing in the exact same way, so I guess that saves some work :p Thanks!

These changes make it easy to convert rejections to a project-specific error type that uses, for instance, thiserror:

#[derive(Debug, Error)]
pub enum AppError {
    #[error("{0}")]
    Valid(#[from] axum_valid::ValidRejection<axum::extract::rejection::JsonRejection>),
}

// impl IntoResponse that returns JSON...

pub async fn foo(
    WithRejection(Valid(Json(bar)), _): WithRejection<Valid<Json<Bar>>, AppError>,
) -> Result<Json<Bar>, AppError> {
    Ok(Json(bar))
}

OK. Your PR has been now released in version 0.5.1.