Implement OptionalFromRequest for the Json extractor

[infiniteregrets]

Motivation

I want to receive requests with an empty body when using the Json extractor without the request getting rejected. I saw that OptionalFromRequest has been implemented for Query and Path and I think this is quite useful to have as it allows me to do something like:

pub async fn create(
    State(account_service): State<Arc<AccountService>>,
    Path(name): Path<String>,
    WithRejection(payload, _): WithRejection<Option<Json<CreateRequest>>, ServiceError>,
) -> Result<impl IntoResponse, ServiceError> {
...

    let Json(payload) = payload.unwrap_or_default();
...
}

Solution

Implement OptionalFromRequest for the Json extractor.

I have tested that this works locally for myself. I don't know if this is absolutely the right way/per standards of how something like this should be approached for axum. However very quick change, so I thought I'd open a PR. Please correct me if I am wrong anywhere, I'll be happy to make any changes/close the PR if not in plans!

[Turbo87]

given the various ways in which None could be interpreted, I'm wondering whether this causes more confusion than it solves. it might be better to use Result<Json, _> as the extractor and explicitly handle the corresponding error cases 🤔

[Turbo87]

if the header says that the content-type is JSON, but the body is empty, shouldn't that be an error?

[infiniteregrets]

yeah that makes sense, I added a new error, is that the right way to approach this?

[Turbo87]

what rejection is the non-optional extractor using in this case? I assume JsonSyntaxError? would it be possible to reuse that rejection?

[infiniteregrets]

I looked into that one, I am not quite sure what Error is supposed to be here in JsonSyntaxError(Error) ? Is it really just specific to serde?

[Turbo87]

I guess we should always call Self::from_bytes() even if bytes.is_empty(), because if the header was set then we expect valid JSON and an empty body is not valid JSON 🤔

[infiniteregrets]

do i need to create a deserializer and then use serde_path_to_error::deserialize to use that error variant?

[infiniteregrets]

I guess we should always call Self::from_bytes() even if bytes.is_empty(), because if the header was set then we expect valid JSON and an empty body is not valid JSON 🤔

do you mean something like this right in the block where we check if the body is empty:?

match Self::from_bytes(&bytes) {
  Ok(_) => unreachable!("JSON body can only be empty here"),
  Err(err) => Err(err),
}               

[Turbo87]

this causes a mismatching JSON body to return an error instead of None, is that intentional?

[infiniteregrets]

is this in reference to the previous CR comment? by mismatch do you mean an empty body here?

[jplatte]

I like it but I think only a request with no content-type header and an empty body should result in None. Would that fit your use case, @infiniteregrets?

[infiniteregrets]

thanks for the review!

@jplatte that does fit my use case here!

[jplatte]

Yeah, this looks good to me. Thanks!

[jplatte]

Could you add a changelog entry for this?

[infiniteregrets]

yup!

[Turbo87]

I guess the new behavior should also be documented in the doc comment?

[jplatte]

Ah, yeah.