Add ErrorKind::DeserializeError to specialize ErrorKind::Message (extract::path::ErrorKind)

[vegardgs-ksat]

Motivation

Expose the key and value of the extract::Path extrator for when the serde deserialization fails. This allow custom extractors wrapping the Path extractor to specialize their responses with more context about the failed operation. Currently, all failed deserializations end up as part of the extract::path::ErrorKind::Message variant.

I encountered this situation when attempting to create my own extrator, using it to deserialize uuid::Uuid resources, since I require specializing the response for these error conditions.

//! Manual implementation wrapping `axum::extract::Path` extrator.

use axum::extract::FromRequestParts;
use axum::extract::{path::ErrorKind, rejection::PathRejection};
use axum::http::{request::Parts, StatusCode};
use serde::de::DeserializeOwned;

pub struct Path<T>(pub T);

#[axum::async_trait]
impl<T, S> FromRequestParts<S> for Path<T>
where
    T: DeserializeOwned + Send,
    S: Send + Sync,
{
    type Rejection = (StatusCode, axum::Json<serde_json::Value>);

    async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
        match axum::extract::Path::<T>::from_request_parts(parts, state).await {
            Ok(value) => Ok(Self(value.0)),
            Err(rejection) => match &rejection {
                PathRejection::FailedToDeserializePathParams(f) => match f.kind() {
                    ErrorKind::ParseErrorAtKey { key, value, .. } => Err((
                        StatusCode::BAD_REQUEST,
                        axum::Json(serde_json::json!({
                            "code": "MALFORMED_VALUE",
                            "message": format!("Value '{value}' could not be parsed"),
                            "target": key
                        })),
                    )),
                    
                    // Nominal UUID deserialization issuess occur in ErrorKind::Message
                    
                    _ => Err((
                        StatusCode::BAD_REQUEST,
                        axum::Json(serde_json::json!({
                            "code": "MALFORMED_VALUE",
                            "message": f.body_text(),
                            "target": "",
                        })),
                    )),
                },
                PathRejection::MissingPathParams(..) | _ => {
                    Err((
                        StatusCode::INTERNAL_SERVER_ERROR,
                        axum::Json(serde_json::json!({
                            "message": "An internal server error occurred",
                        })),
                    ))
                }
            },
        }
    }
}

// This is the endpoint handler using the custom Path extractor.
async fn find_resource(Path(resource_id): Path<uuid::Uuid>) -> String {
    todo!()
}

Solution

This commit introduces another extract::path::ErrorKind variant that captures the serde error nominally captured through the serde::de::Error trait impl on PathDeserializeError. We augment the deserialization error with the captured (key, value), allowing extract::Path, and wrapping extractors, to gain programmatic access to the key name, and attempted deserialized value.

This allows me to expand the handling in the wrapping Path extractor.

ErrorKind::DeserializeError { key, value, message } => Err((StatusCode::BAD_REQUEST, axum::Json(serde_json::json!({
  "code": "MALFORMED_VALUE",
  "message": format!("Value '{value}' could not be parsed"),
  "target": key
})),

[mladedav]

This looks good, just a few minor points.

[mladedav]

Do not use debug for value, it would add redundant quotes. You can also that in the new tests.

[vegardgs-ksat]

I was just mirroring the practice established by the other formats for the other variants:

axum/axum/src/extract/path/mod.rs

Lines 330 to 349 in e3bb708

	ErrorKind::ParseErrorAtKey {
	key,
	value,
	expected_type,
	} => write!(
	f,
	"Cannot parse `{key}` with value `{value:?}` to a `{expected_type}`"
	),
	ErrorKind::ParseError {
	value,
	expected_type,
	} => write!(f, "Cannot parse `{value:?}` to a `{expected_type}`"),
	ErrorKind::ParseErrorAtIndex {
	index,
	value,
	expected_type,
	} => write!(
	f,
	"Cannot parse value at index {index} with value `{value:?}` to a `{expected_type}`"
	),

I was a bit perplexed about this as well, and thought it did not make sense to use debug format for value.

Should I change it do non-Debug for this new variant only, or change the other variants as well?

[vegardgs-ksat]

I added an additional commit that changes the format for all variants.

[mladedav]

Thanks. Sorry I didn't notice you just wrote it consistently with the rest, otherwise I wouldn't bring it up here.

[mladedav]

LGTM, @jplatte can you take a look when you have some free time?

[vegardgs-ksat]

Rebased against latest main for an up-to-date PR.

[vegardgs-ksat]

@mladedav anything holding up this PR?

[jplatte]

Yeah, I was holding it up by not reviewing, sorry. I did so now, and the change looks good to me.

One question, and a request though:

• Is ErrorKind::Message still constructed anywhere? If not, would make sense to remove it, I wouldn't want to release this in a patch of 0.7 anyways.
• Could you please add a changelog entry?

[vegardgs-ksat]

* Is `ErrorKind::Message` still constructed anywhere? If not, would make sense to remove it, I wouldn't want to release this in a patch of 0.7 anyways.

It is indirectly constructed through the PathDeserializationError::custom() method, here and here

* Could you please add a changelog entry?

Done! Added it under the "# Unreleased" section

[vegardgs-ksat]

I have a feeling that the failing CI job is unrelated to this PR.

[mladedav]

Yeah, it seems unrelated. I don't know the public api check but when I get some time, I'll try to figure out what's happening and then we'll merge this. Other branches seem to have also failed, it's possible one of our dependencies we're exposing started exposing another dependency we didn't mean to expose.

Sorry the whole thing takes so long.

[mladedav]

Thanks for sticking with this and pushing it through the finish line!