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

Remove ContentLengthLimit #1400

Merged
merged 15 commits into from Sep 24, 2022
Merged

Remove ContentLengthLimit #1400

merged 15 commits into from Sep 24, 2022

Conversation

nylonicious
Copy link
Contributor

Closes #1399

Before I continue I wanted to ask if the implementation should live in the Bytes extractor, and if so should we also check the method like ContentLengthLimit impl used to? If the impl should live in in the Bytes extractor what with rejections, should I just extend it with the required fields or maybe add one variant to BytesRejection named something like BodyLimitRejection and store more detailed rejection inside that variant?

@davidpdrsn
Copy link
Member

davidpdrsn commented Sep 22, 2022

Before I continue I wanted to ask if the implementation should live in the Bytes extractor, and if so should we also check the method like ContentLengthLimit impl used to?

Yes I think that would be good to add as well. I think that should be its own PR though.

If the impl should live in in the Bytes extractor what with rejections, should I just extend it with the required fields or maybe add one variant to BytesRejection named something like BodyLimitRejection and store more detailed rejection inside that variant?

Hm I didn't really consider that. I suppose yes, we would have to add the rejection variants from ContentLengthLimitRejection to BytesRejection, or perhaps FailedToBufferBody.

axum-core/src/extract/request_parts.rs Outdated Show resolved Hide resolved
axum/src/extract/multipart.rs Outdated Show resolved Hide resolved
@nylonicious nylonicious changed the title feat: replace ContentLengthLimit with DefaultBodyLimit feat: remove ContentLengthLimit Sep 23, 2022
@nylonicious
Copy link
Contributor Author

I couldn't find any other nice way of adding RequestBodyLimitLayer to key value store.

@nylonicious nylonicious marked this pull request as ready for review September 23, 2022 17:28
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Looks good!

Wanna update the changelog as well? Then we can merge it 😊

axum/CHANGELOG.md Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn enabled auto-merge (squash) September 24, 2022 11:25
@davidpdrsn davidpdrsn enabled auto-merge (squash) September 24, 2022 11:26
@davidpdrsn davidpdrsn changed the title feat: remove ContentLengthLimit Remove ContentLengthLimit Sep 24, 2022
@davidpdrsn davidpdrsn merged commit 896ffc5 into tokio-rs:main Sep 24, 2022
@nylonicious nylonicious deleted the body-limit-1399 branch September 30, 2022 07:28
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.

Remove ContentLengthLimit and make DefaultBodyLimit check Content-Length
2 participants