-
Notifications
You must be signed in to change notification settings - Fork 168
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
Invalid transformation to http::Response<Vec<u8>> for non utf8 response bodies #701
Comments
Thanks for reporting this. Looking at that code, it's just wrong regardless of content type. Instead of checking content type, that code should unconditionally call |
Make sense to me. Wdyt of #702? |
It would be great to have the happy path transformation too (TryFrom). Which would be the best way to define the possible error? (E.g io::Error) Would it be possible to extend the #[derive(Debug)]
pub enum Error {
/// A response was successfully received but had status code >= 400.
/// Values are (status_code, Response).
Status(u16, Response),
/// There was an error making the request or receiving the response.
Transport(Transport),
#[cfg(features = ...)]
/// There was an error while transforming the request to http::Request.
ConvertError(String),
} |
When using protobuffers over HTTP the library silently transforms the protobuf data into an invalid
http::Response<Vec<u8>>
. Decoding protobuf crates like Prost fail to decode the generatedVec<u8>
.It seems the issue is because the crate is trying to transform the body into a String and then to Vec: https://github.com/algesten/ureq/blob/main/src/http_interop.rs#L132
When dealing with binary data (
"content-type": "application/x-protobuf"
), it shouldn't be converted into a String. https://github.com/algesten/ureq/blob/main/src/http_interop.rs#L132We have been able to fix the issue by manually converting the internal reader into a Vec.
What do you think of adding a content type check before transforming the Reader into a String?
We are able to contribute with the decided solution.
The text was updated successfully, but these errors were encountered: