[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