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 IntoResponse impl for BytesMut and Chain #767

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

AlisCode
Copy link
Contributor

Adds two IntoResponse implementations for convenience to work on bytes types

Motivation

  • Enlarging the surface of axum's API to work better with the bytes crate
  • In my context, I want my service to output a file and append something to it, bytes::Chain seemed like a good opportunity to do so.

Solution

Two new implementations of IntoResponse :

  • BytesMut which freezes (zero-cost operation) to get a Bytes handle and use its IntoResponse's implementation.
  • Chain<T,U> where T: Buf and U: Buf, which iterates over every collects everything into a Bytes. Sadly the implementation of FromIterator seems to show that it's not exactly zero-cost.

@jplatte

This comment was marked as resolved.

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.

Thanks! I had an idea regarding Chain. Let me know what you think :)

axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member

You can just ignore the cargo deny CI failure. I'll get that fixed at some point 😊

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- **added:** `middleware::from_fn` for creating middleware from async functions.
This previously lived in axum-extra but has been moved to axum ([#719])
- **added:** Implement `IntoResponse` for `bytes::BytesMut` and `bytes::Chain<T, U>` ([#767])
Copy link
Member

Choose a reason for hiding this comment

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

Hm I knew this could come up at some point. This change was actually made in axum-core so it should at least be mentioned in its changelog as well. The question is whether it should be mentioned in axum's as well. The heuristic I've used so far is that only significant changes are mentioned in both. I kinda feel this isn't very significant so should only be in axum-core's changelog.

@jplatte wdyt?

Copy link
Contributor Author

@AlisCode AlisCode Feb 17, 2022

Choose a reason for hiding this comment

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

Didn't notice that there was a changelog specific to subcrates, I believe you're correct, this isn't worth a line in the "main" changelog. I'll move that there :)

Copy link
Member

Choose a reason for hiding this comment

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

I know this won't be very helpful, but I don't care much either way 😅

Copy link
Member

Choose a reason for hiding this comment

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

@jplatte thats a first 😂

@AlisCode
Copy link
Contributor Author

Re-implemented IntoResponse for Chain using your custom body that I renamed to BytesChainBody, that should avoid potential confusion.

Also squashed commits to keep a clean history :)

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.

LGMT! Thanks!

@davidpdrsn
Copy link
Member

Also squashed commits to keep a clean history :)

We squash merge all PRs so you actually don't have to.

@davidpdrsn davidpdrsn merged commit d12494c into tokio-rs:main Feb 17, 2022
davidpdrsn added a commit that referenced this pull request Feb 22, 2022
* Add IntoResponse impl for BytesMut and Chain

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>

* Add CHANGELOG entry

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
davidpdrsn added a commit that referenced this pull request Feb 22, 2022
* Add IntoResponse impl for BytesMut and Chain

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>

* Add CHANGELOG entry

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
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.

3 participants