Closed olix0r closed 2 years ago
It looks like my editor ran rustfmt which modified some whitespace. I can back out those changes if desired.
@seanmonstar @carllerche I'm curious if there objections to adding these accessors. I looked through the PR history but didn't spot any discussion that touched on this.
cc @davidpdrsn
It's worth pointing out that by adding an accessor like this, we are then stuck with keeping the internal shape the same forever, since it's essentially opening up as public fields. This differs from into_parts
, which could move from a changed structure into the Parts
shape.
@seanmonstar that's a fair point.
in favor of this: it would allow a function that takes &http::Request<_>
to access the &Parts
.
Maybe an alternative would be to add a PartsRef<'_>
that holds only a reference the headers/extensions/uri? This seems awkward for other reasons (we would probably have to drop the _mut
variant).
I don't feel strongly about this, but it's something I've run into more than once; so I wanted to start a discussion.
Closing for now, as this will probably ossify the API more than we want. Happy to reopen in the future, though.
Can this be re-opened? Of course it's a tradeoff, but it doesn't seem like anybody was strongly against committing to Request
holding request::Parts
/ Response
holding response::Parts
. It would simplify the implementation of RequestExt
that we just added in axum, and also future-proof it for any additional Parts
request / response bits.
Ive been recently thinking we could make the size of Request
smaller if we possibly boxed some fields. I still don't see a reason to commit to holding a Parts
internally. It's the same as just making all the fields public.
Well, the reason would be that it's useful for downstream projects, as shown twice now. Maybe a solution could be to (in the next breaking release) make the Parts
types as opaque as Request
/ Response
in terms of fields, and provide accessors as well? Then a change like boxing fields could be done across all types at the same time and parts()
and parts_mut()
could exist too.
I have a case where I need to modify both extensions and headers, something like:
fn foo<T>(request: &mut Request<T>) {
// Use `request.extensions_mut()` and `request.headers_mut()`.
}
The function above does not need to access body, so I see a chance to optimize the binary size, like this:
fn foo<T>(request: &mut Request<T>) {
fn inner(extensions: &mut Extensions, headers: &mut HeaderMap) {
// Use `extensions` and `headers`.
}
inner(request.extensions_mut(), request.headers_mut())
}
But the compiler will complain about borrowing request
twice. I could use a trait object to solve this:
trait ErasedRequest {
fn extensions_mut(&mut self) -> &mut Extensions;
fn headers_mut(&mut self) -> &mut HeaderMap;
}
impl<T> ErasedRequest for Request<T> {
fn extensions_mut(&mut self) -> &mut Extensions {
self.extensions_mut()
}
fn headers_mut(&mut self) -> &mut HeaderMap {
self.headers_mut()
}
}
fn foo<T>(request: &mut Request<T>) {
fn inner(request: &mut dyn ErasedRequest) {
// Use `request.extensions_mut()` and `request.headers_mut()`.
}
inner(request)
}
But I think it is a bit too much. If I can get access to the parts
reference, the problem should be easy to solve:
fn foo<T>(request: &mut Request<T>) {
fn inner(parts: &mut Parts) {
// Use `parts.extensions` and `parts.headers`.
}
inner(request.parts_mut())
}
Hi @seanmonstar, How about an alternative design like this:
pub struct SplitPartsMut<'a, T> {
pub method: &'a mut Method,
pub uri: &'a mut Uri,
pub version: &'a mut Version,
pub headers: &'a mut HeaderMap<HeaderValue>,
pub extensions: &'a mut Extensions,
pub body: &'a mut T,
_priv: (),
}
impl<T> Request<T> {
pub fn split_parts_mut(&mut self) -> SplitPartsMut<'_, T> {
SplitPartsMut { ... }
}
}
or
pub struct PartsMut<'a> {
pub method: &'a mut Method,
pub uri: &'a mut Uri,
pub version: &'a mut Version,
pub headers: &'a mut HeaderMap<HeaderValue>,
pub extensions: &'a mut Extensions,
_priv: (),
}
impl<T> Request<T> {
pub fn split_parts_mut(&mut self) -> (PartsMut<'_>, &mut T) {
(PartsMut { ... }, &mut self.body)
}
}
In this way, we can allow user to hold mutable references to different parts without exposing the concrete implementation.
http::Request::into_parts()
is the only way to access a request'sParts
structure.It can be convenient to write utility code that takes a
&Parts
or&mut Parts
, i.e., so there's no need to parameterize the utility on body type if it can't be used.This change introduces accessors for the
Parts
structure so callers need not useRequest::into_parts
andRequest::from_parts
to access theParts
structure.In service of https://github.com/tower-rs/tower-http/issues/154