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

Add axum_extra::extract::Multipart #1692

Merged
merged 6 commits into from
Mar 3, 2023
Merged

Add axum_extra::extract::Multipart #1692

merged 6 commits into from
Mar 3, 2023

Conversation

davidpdrsn
Copy link
Member

It is is similar to axum::extract::Multipart except that it enforces field exclusivity at runtime instead of compile time.

I kinda wish we didn't have to do it honestly. Tracking lifetimes at compile feels much more correct but we've learned that it causes lots of headaches, especially because Field not being 'static.

I'm thinking for 0.7 we can move this into axum but keep it in axum-extra in the meantime.

@jplatte
Copy link
Member

jplatte commented Jan 24, 2023

Are we certain the things people tried to do and ran into compile errors with ultimately worked? I got the impression that most or all of them would have led to runtime panics instead if converted to this API.

@davidpdrsn
Copy link
Member Author

This is mainly for fixing things like #1644 where you wanna stream the contents of the fields somewhere else, perhaps to upload the data somewhere.

The use cases we've heard about where people wanna find a field by name and do things with it would most likely result in runtime errors. But thats probably the wrong way to approach the problem in the first place.

@netthier
Copy link

netthier commented Jan 24, 2023

Are we certain the things people tried to do and ran into compile errors with ultimately worked? I got the impression that most or all of them would have led to runtime panics instead if converted to this API.

I had (and actually still have) a case where I wanted to stream a field of a multipart upload directly to S3 and got a similar problem as the user in the discussion linked by David.
As I only ever held onto one field at a time it shouldn't cause runtime errors with the new solution.

@davidpdrsn
Copy link
Member Author

@jplatte should we merge this?

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Haven't reviewed in detail, but sure.

@davidpdrsn davidpdrsn merged commit aa2cbf6 into main Mar 3, 2023
@davidpdrsn davidpdrsn deleted the multipart-stream branch March 3, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants