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

Implement IntoResponse for MultipartError #1861

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Mar 17, 2023

So you can return MultipartError from handlers without having to
downcast the errors yourself.

@AcrylicShrimp what do you think about this?

Fixes #1851

So you can return `MultipartError` from handlers without having to
downcast the errors yourself.

Fixes #1851
@AcrylicShrimp
Copy link

Great! What a nice work. Your implementation looks fine to me. Let's note that this kind of handling may not work(or make no sense) in future, because of changes of actual error structures. But I think it's not big deal.

@davidpdrsn
Copy link
Member Author

Let's note that this kind of handling may not work(or make no sense) in future, because of changes of actual error structures. But I think it's not big deal.

That's always the case and why multer is a private dependency of axum. Then we can change things without breakage.

multer::Error::UnknownField { .. }
| multer::Error::IncompleteFieldData { .. }
| multer::Error::IncompleteHeaders
| multer::Error::ReadHeaderFailed(..)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this multer::Error::ReadHeaderFailed is always thrown by the client side. How do you think? Is it OK to response as 400?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like if the headers cannot be parsed then that's a client issue. The inner error is httparse::Error

| multer::Error::DecodeHeaderValue { .. }
| multer::Error::NoMultipart
| multer::Error::IncompleteStream => StatusCode::BAD_REQUEST,
multer::Error::FieldSizeExceeded { .. } | multer::Error::StreamSizeExceeded { .. } => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of error will never thrown, because axum does not support constraints of multer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we still need an exhaustive match.

Copy link

@AcrylicShrimp AcrylicShrimp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for hard working!

@davidpdrsn davidpdrsn enabled auto-merge (squash) March 21, 2023 08:13
@davidpdrsn davidpdrsn merged commit 03e8bc7 into main Mar 21, 2023
@davidpdrsn davidpdrsn deleted the david/multipart-error-into-response branch March 21, 2023 08:24
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

Successfully merging this pull request may close these issues.

Implement IntoResponse for MultipartError
3 participants