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

Form extractor should return 422 Unprocessable Entity for deserialization errors #1680

Closed
1 task done
connec opened this issue Jan 6, 2023 · 1 comment · Fixed by #1683
Closed
1 task done

Form extractor should return 422 Unprocessable Entity for deserialization errors #1680

connec opened this issue Jan 6, 2023 · 1 comment · Fixed by #1683
Labels
A-axum C-enhancement Category: A PR with an enhancement E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@connec
Copy link
Contributor

connec commented Jan 6, 2023

  • I have looked for existing issues (including closed) about this

Feature Request

Motivation

I noticed this when upgrading a project from axum 0.5 to 0.6, which led to some failing tests of invalid form data: deserialization errors from the Form extractor now convert into 400 Bad Request responses, when previously they were 422 Unprocessable Entity responses. Per the semantics discussed in #1378 it seems like 422 Unprocessable Entity would be the more appropriate response for syntactically valid form data that could not be deserialized.

Proposal

From doing some digging around, it looks like this change occurred to fix #1378. At that time, the rejection for a Form deserialization error was also FailedToDeserializeQueryString:

composite_rejection! {
/// Rejection used for [`Form`](super::Form).
///
/// Contains one variant for each way the [`Form`](super::Form) extractor
/// can fail.
pub enum FormRejection {
InvalidFormContentType,
FailedToDeserializeQueryString,
BytesRejection,
}
}

Later, in #1496, Form gets a dedicated FailedToDeserializeForm variant:

composite_rejection! {
/// Rejection used for [`Form`](super::Form).
///
/// Contains one variant for each way the [`Form`](super::Form) extractor
/// can fail.
pub enum FormRejection {
InvalidFormContentType,
FailedToDeserializeForm,
BytesRejection,
}
}

However, this rejection is still set to return a 400 Bad Request:

define_rejection! {
#[status = BAD_REQUEST]
#[body = "Failed to deserialize form"]
/// Rejection type used if the [`Form`](super::Form) extractor is unable to
/// deserialize the form into the target type.
pub struct FailedToDeserializeForm(Error);
}

Since the Form and Query extractors now have dedicated rejection types, it seems we could simply update the status for FailedToDeserializeForm to UNPROCESSABLE_ENTITY without disturbing the original fix in #1378. I'd be happy to make that PR if it's desirable and I'm not missing anything. This would be a breaking change (assuming the rejection statuses are covered by the versioning policy, and this isn't considered a bug).

@davidpdrsn
Copy link
Member

That makes sense! Its just an oversight. We don't consider changing rejection statuses breaking changes so you're very welcome to submit a PR.

@davidpdrsn davidpdrsn added C-enhancement Category: A PR with an enhancement E-easy Call for participation: Experience needed to fix: Easy / not much A-axum E-help-wanted Call for participation: Help is requested to fix this issue. labels Jan 6, 2023
connec added a commit to connec/axum that referenced this issue Jan 6, 2023
davidpdrsn added a commit that referenced this issue Jan 7, 2023
Deserialization errors for `Form` previously always used `400 Bad
Request`. However that should apply for `GET` requests. `POST` etc.
should use `422`.

Fixes #1680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-enhancement Category: A PR with an enhancement E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
2 participants