feat(response): preserve URL when converting Response to http::Response#2798
feat(response): preserve URL when converting Response to http::Response#27980x676e67 wants to merge 5 commits into
Response to http::Response#2798Conversation
|
If this suggestion is adopted, to be honest, I’m not sure how the ResponseExt API should be designed to align with most people’s usage habits — things like: /// Extension trait for http::Response objects
///
/// Provides methods to extract URL information from HTTP responses
pub trait ResponseExt {
/// Extracts the URL associated with this response
fn url(&self) -> Option<&Url>;
/// Extracts a mutable reference to the URL associated with this response
fn url_mut(&mut self) -> Option<&mut Url>;
/// Takes the URL from the response, consuming it and returning the URL
fn take_url(&mut self) -> Option<Url>;
} |
| /// Extension trait for http::Response objects | ||
| /// | ||
| /// Provides methods to extract URL information from HTTP responses | ||
| pub trait ResponseExt { |
There was a problem hiding this comment.
I'm thinking it might be better to leave this off. Is there a need for it that I'm overlooking?
There was a problem hiding this comment.
I did run into some scenarios where I needed to convert a reqwest::Response into an http::Response while preserving the original URL. That said, as you pointed out, it's always possible to manually clone the URL using the url() method on reqwest::Response. While that approach is a bit more verbose, the only benefit of this change would be to avoid one additional clone, which probably isn't significant in practice.
So I agree with your suggestion—this functionality isn't strictly necessary at the moment. I'm fine with closing this PR, and if a stronger need arises in the future, we can revisit it then.
There was a problem hiding this comment.
Well, I don't mean we need to close the whole thing... I actually have for a while thought we should just store the url in the extensions instead of directly on the struct...
There was a problem hiding this comment.
Alternatively, it's still possible to get the URL with an implementation like this:
impl From<Response> for (Url, http::Response<Body>) {
fn from(r: Response) -> (Url, http::Response<Body>) {
let (parts, body) = r.res.into_parts();
let body = Body::wrap(body);
let response = http::Response::from_parts(parts, body);
(*r.url, response)
}
}|
I would find this useful. My use-case is adding some instrumentation to the response body stream in a I'd like to do this, but currently the URL is lost: let (parts, body) = http::Response::from(res).into_parts();
let wrapped_body = MyWrapperStream::wrap(http_body_util::BodyDataStream::new(body));
Response::from(http::Response::from_parts(
parts,
Body::wrap_stream(wrapped_body),
))Currently I'm doing this: let url = res.url().clone();
let (parts, body) = http::Response::from(res).into_parts();
let mut new_res = http::Response::builder()
.status(parts.status)
.version(parts.version)
.url(url);
if let Some(headers) = new_res.headers_mut() {
*headers = parts.headers;
}
if let Some(extensions) = new_res.extensions_mut() {
extensions.extend(parts.extensions);
}
let wrapped_body = MyWrapperStream::wrap(http_body_util::BodyDataStream::new(body));
new_res
.body(Body::wrap_stream(wrapped_body))
.map_err(reqwest_middleware::Error::middleware)
.map(Response::from)Which is fine, it's just one extra clone and a bit more verbose. Or maybe there's already a better way to do it that I'm missing? |
Add URL preservation in the From for http::Response conversion by
inserting the response URL as a ResponseUrl extension. This ensures that the original
URL information is not lost during the conversion and can be accessed later.
This change maintains consistency with the inverse conversion (Fromhttp::Response
for Response) which expects the ResponseUrl extension to be present.