[pbs-devel] [PATCH proxmox 2/2] http: Teach client how to speak deflate
Lukas Wagner
l.wagner at proxmox.com
Wed Mar 27 09:59:55 CET 2024
Hello, thanks for tackling this!
Most of my comments also apply to the first commit.
Regarding the commit message, I think it would be good to
mention the `Accept-Encoding` and `Content-Encoding` headers (e.g
that you set `Accept-Encoding` on the request on decode the response
body based on `Content-Encoding`).
These are both quite well-known and it makes it clearer what these
commits are about.
Thanks for including some tests, that's always good. Of course
it's hard to unit-test this in a more 'realistic' scenario. :)
On 2024-03-26 16:28, Maximiliano Sandoval wrote:
> The Backup Server can speak deflate so we implement that.
>
> Note that the spec [1] allows the server to encode the content multiple
> times with different algorithms.
>
> [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding
>
> Suggested-by: Lukas Wagner <l.wagner at proxmox.com>
> Signed-off-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
> ---
> proxmox-http/src/client/simple.rs | 98 ++++++++++++++++++++++++-------
> 1 file changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/proxmox-http/src/client/simple.rs b/proxmox-http/src/client/simple.rs
> index b33154be..c3afa8d0 100644
> --- a/proxmox-http/src/client/simple.rs
> +++ b/proxmox-http/src/client/simple.rs
> @@ -4,7 +4,7 @@ use std::io::Read;
> #[cfg(all(feature = "client-trait", feature = "proxmox-async"))]
> use std::str::FromStr;
>
> -use flate2::read::GzDecoder;
> +use flate2::read::{DeflateDecoder, GzDecoder};
>
> use futures::*;
> #[cfg(all(feature = "client-trait", feature = "proxmox-async"))]
> @@ -76,7 +76,7 @@ impl Client {
>
> request.headers_mut().insert(
> hyper::header::ACCEPT_ENCODING,
> - HeaderValue::from_static("gzip"),
> + HeaderValue::from_static("gzip, deflate"),
> );
> request
> .headers_mut()
> @@ -149,22 +149,24 @@ impl Client {
> match response {
> Ok(res) => {
> let (mut parts, body) = res.into_parts();
> - let is_gzip_encoded = parts
> - .headers
> - .remove(&hyper::header::CONTENT_ENCODING)
> - .is_some_and(|h| h == "gzip");
> -
> - let buf = hyper::body::to_bytes(body).await?;
> - let new_body = if is_gzip_encoded {
> - let mut gz = GzDecoder::new(&buf[..]);
> - let mut s = String::new();
> - gz.read_to_string(&mut s)?;
> - s
> - } else {
> - String::from_utf8(buf.to_vec())
> - .map_err(|err| format_err!("Error converting HTTP result data: {}", err))?
> + let mut buf = hyper::body::to_bytes(body).await?.to_vec();
> + let content_encoding = parts.headers.remove(&hyper::header::CONTENT_ENCODING);
> +
> + if let Some(content_encoding) = content_encoding {
> + let encodings = content_encoding.to_str()?;
> + for encoding in encodings.rsplit([',', ' ']) {
> + buf = match encoding {
> + "" => buf, // "a, b" splits into ["a", "", "b"].
> + "gzip" => decode_gzip(&buf[..])?,
> + "deflate" => decode_deflate(&buf[..])?,
> + other => anyhow::bail!("Unknown format: {other}"),
`anyhow::bail!` is already in scope, so you can just use `bail!`
> + }
> + }
I would suggest moving the decompression to the `request`
method (maybe as a separate helper function though),
transforming the `Response<Body` into another `Response<Body>`,
with the body decompressed.
Right now, the decompression only happens in `convert_body_to_string`,
which means that this breaks users
- which use the public `Client::request` directly (e.g. the `proxmox-client` crate)
- which use the `HttpClient<Body,Body>` trait impl of `Client`
If the decompression happens directly in `request`, the users for the crate
should not notice any difference, at least from my understanding :)
> };
>
> + let new_body = String::from_utf8(buf)
> + .map_err(|err| format_err!("Error converting HTTP result data: {}", err))?;
> +
> Ok(Response::from_parts(parts, new_body))
> }
> Err(err) => Err(err),
> @@ -267,6 +269,10 @@ impl crate::HttpClient<String, String> for Client {
> mod test {
> use super::*;
>
> + use flate2::write::{DeflateEncoder, GzEncoder};
> + use flate2::Compression;
> + use std::io::Write;
> +
> const BODY: &str = "hello world";
>
> #[tokio::test]
> @@ -288,14 +294,66 @@ mod test {
> assert_eq!(Client::response_body_string(response).await.unwrap(), BODY);
> }
>
> - fn encode_gzip(bytes: &[u8]) -> Result<Vec<u8>, std::io::Error> {
> - use flate2::write::GzEncoder;
> - use flate2::Compression;
> - use std::io::Write;
> + #[tokio::test]
> + async fn test_parse_response_deflate() {
> + let encoded = encode_deflate(BODY.as_bytes()).unwrap();
> + let body = Body::from(encoded);
> +
> + let response = Response::builder()
> + .header(hyper::header::CONTENT_ENCODING, "deflate")
> + .body(body)
> + .unwrap();
> + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY);
> + }
> +
> + #[tokio::test]
> + async fn test_parse_response_deflate_gzip() {
> + let deflate_encoded = encode_deflate(BODY.as_bytes()).unwrap();
> + let gzip_encoded = encode_gzip(&deflate_encoded).unwrap();
> + let body = Body::from(gzip_encoded);
> +
> + let response = Response::builder()
> + .header(hyper::header::CONTENT_ENCODING, "deflate, gzip")
> + .body(body)
> + .unwrap();
> + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY);
>
> + let gzip_encoded = encode_gzip(BODY.as_bytes()).unwrap();
> + let deflate_encoded = encode_deflate(&gzip_encoded).unwrap();
> + let body = Body::from(deflate_encoded);
> +
> + let response = Response::builder()
> + .header(hyper::header::CONTENT_ENCODING, "gzip, deflate")
> + .body(body)
> + .unwrap();
> + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY);
> + }
> +
> + fn encode_deflate(bytes: &[u8]) -> Result<Vec<u8>, std::io::Error> {
> + let mut e = DeflateEncoder::new(Vec::new(), Compression::default());
> + e.write_all(bytes).unwrap();
> +
> + e.finish()
> + }
> +
> + fn encode_gzip(bytes: &[u8]) -> Result<Vec<u8>, std::io::Error> {
> let mut e = GzEncoder::new(Vec::new(), Compression::default());
> e.write_all(bytes).unwrap();
>
> e.finish()
> }
> }
> +
> +fn decode_gzip(buf: &[u8]) -> Result<Vec<u8>, std::io::Error> {
> + let mut dec = GzDecoder::new(buf);
> + let mut v = Vec::new();
> + dec.read_to_end(&mut v)?;
> + Ok(v)
> +}
> +
> +fn decode_deflate(buf: &[u8]) -> Result<Vec<u8>, std::io::Error> {
> + let mut dec = DeflateDecoder::new(buf);
> + let mut v = Vec::new();
> + dec.read_to_end(&mut v)?;
> + Ok(v)
> +}
^ I'd put both of them into the `impl Client` block (as associated static helper functions,
not methods (so no self parameter) - but no hard feelings
--
- Lukas
More information about the pbs-devel
mailing list