From 23ca61dd3f99cd87dd702958f66222c022dac4ca Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 18 Jun 2025 22:49:56 +0200 Subject: [PATCH 01/22] gzip: failing test --- Cargo.lock | 37 ++++- dropshot/Cargo.toml | 1 + dropshot/src/test_util.rs | 3 +- dropshot/tests/integration-tests/gzip.rs | 200 +++++++++++++++++++++++ dropshot/tests/integration-tests/main.rs | 1 + 5 files changed, 240 insertions(+), 2 deletions(-) create mode 100644 dropshot/tests/integration-tests/gzip.rs diff --git a/Cargo.lock b/Cargo.lock index 443014a36..fb656fba8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,6 +17,12 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f26201604c87b1e01bd3d98f8d5d9a8fcbb815e8cedb41ffccbeb4bf593a35fe" +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + [[package]] name = "android_system_properties" version = "0.1.5" @@ -110,7 +116,7 @@ dependencies = [ "cc", "cfg-if", "libc", - "miniz_oxide", + "miniz_oxide 0.7.1", "object", "rustc-demangle", ] @@ -254,6 +260,15 @@ dependencies = [ "libc", ] +[[package]] +name = "crc32fast" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9481c1c90cbf2ac953f07c8d4a58aa3945c425b7185c9154d67a65e4230da511" +dependencies = [ + "cfg-if", +] + [[package]] name = "crossbeam-channel" version = "0.5.1" @@ -355,6 +370,7 @@ dependencies = [ "debug-ignore", "dropshot_endpoint", "expectorate", + "flate2", "form_urlencoded", "futures", "hostname 0.4.0", @@ -509,6 +525,16 @@ version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8c02a5121d4ea3eb16a80748c74f5549a5665e4c21333c6098f283870fbdea6" +[[package]] +name = "flate2" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a3d7db9596fecd151c5f638c0ee5d5bd487b6e0ea232e5dc96d5250f6f94b1d" +dependencies = [ + "crc32fast", + "miniz_oxide 0.8.9", +] + [[package]] name = "fnv" version = "1.0.7" @@ -1229,6 +1255,15 @@ dependencies = [ "adler", ] +[[package]] +name = "miniz_oxide" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fa76a2c86f704bdb222d66965fb3d63269ce38518b83cb0575fca855ebb6316" +dependencies = [ + "adler2", +] + [[package]] name = "mio" version = "1.0.3" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index f30ebcdb9..8826b48e0 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -95,6 +95,7 @@ anyhow = "1.0.100" async-channel = "2.5.0" buf-list = "1.0.3" expectorate = "1.2.0" +flate2 = "1.0" hyper-rustls = "0.26.0" hyper-staticfile = "0.10" lazy_static = "1.5.0" diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 8690c094e..864bad17c 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -56,7 +56,8 @@ pub const TEST_HEADER_2: &str = "x-dropshot-test-header-2"; // List of allowed HTTP headers in responses. // Used to make sure we don't leak headers unexpectedly. -const ALLOWED_HEADERS: [AllowedHeader<'static>; 8] = [ +const ALLOWED_HEADERS: [AllowedHeader<'static>; 9] = [ + AllowedHeader::new("content-encoding"), AllowedHeader::new("content-length"), AllowedHeader::new("content-type"), AllowedHeader::new("date"), diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs new file mode 100644 index 000000000..ba8597766 --- /dev/null +++ b/dropshot/tests/integration-tests/gzip.rs @@ -0,0 +1,200 @@ +// Copyright 2025 Oxide Computer Company + +//! Test cases for gzip response compression. + +use dropshot::endpoint; +use dropshot::ApiDescription; +use dropshot::HttpError; +use dropshot::HttpResponseOk; +use dropshot::RequestContext; +use http::{header, Method, StatusCode}; +use hyper::{Request, Response}; +use serde::{Deserialize, Serialize}; + +use crate::common; + +extern crate slog; + +// Test payload that's large enough to benefit from compression +#[derive(Deserialize, Serialize, schemars::JsonSchema)] +struct LargeTestData { + message: String, + repeated_data: Vec, +} + +fn api() -> ApiDescription { + let mut api = ApiDescription::new(); + api.register(api_large_response).unwrap(); + api +} + +/// Returns a large JSON response that should compress well +#[endpoint { + method = GET, + path = "/large-response", +}] +async fn api_large_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + // Create a response with repeated data that will compress well + let repeated_text = "This is some repetitive text that should compress very well with gzip compression. ".repeat(50); + let repeated_data = vec![repeated_text; 100]; // Make it quite large + + Ok(HttpResponseOk(LargeTestData { + message: "This is a large response for testing gzip compression" + .to_string(), + repeated_data, + })) +} + +async fn get_response_bytes( + response: &mut Response, +) -> Vec { + use http_body_util::BodyExt; + + let body_bytes = response + .body_mut() + .collect() + .await + .expect("Error reading response body") + .to_bytes(); + + body_bytes.to_vec() +} + +fn decompress_gzip(compressed_data: &[u8]) -> Vec { + use std::io::Read; + + let mut decoder = flate2::read::GzDecoder::new(compressed_data); + let mut decompressed = Vec::new(); + decoder + .read_to_end(&mut decompressed) + .expect("Failed to decompress gzip data"); + decompressed +} + +#[tokio::test] +async fn test_gzip_compression_with_accept_encoding() { + let api = api(); + let testctx = common::test_setup("gzip_compression_accept_encoding", api); + let client = &testctx.client_testctx; + + // Make request WITHOUT Accept-Encoding: gzip header + let uri = client.url("/large-response"); + let request_no_gzip = Request::builder() + .method(Method::GET) + .uri(&uri) + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response_no_gzip = client + .make_request_with_request(request_no_gzip, StatusCode::OK) + .await + .expect("Request without gzip should succeed"); + + // Make request WITH Accept-Encoding: gzip header + let request_with_gzip = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response_with_gzip = client + .make_request_with_request(request_with_gzip, StatusCode::OK) + .await + .expect("Request with gzip should succeed"); + + // Get response bodies + let uncompressed_body = get_response_bytes(&mut response_no_gzip).await; + let compressed_body = get_response_bytes(&mut response_with_gzip).await; + + // When gzip is implemented, the gzipped response should: + // 1. Have Content-Encoding: gzip header + assert_eq!( + response_with_gzip.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")), + "Response with Accept-Encoding: gzip should have Content-Encoding: gzip header" + ); + + // 2. Be smaller than the uncompressed response + assert!( + compressed_body.len() < uncompressed_body.len(), + "Gzipped response ({} bytes) should be smaller than uncompressed response ({} bytes)", + compressed_body.len(), + uncompressed_body.len() + ); + + // 3. When decompressed, should match the original response + let decompressed_body = decompress_gzip(&compressed_body); + assert_eq!( + decompressed_body, uncompressed_body, + "Decompressed gzip response should match uncompressed response" + ); + + // The response without Accept-Encoding should NOT have Content-Encoding header + assert_eq!( + response_no_gzip.headers().get(header::CONTENT_ENCODING), + None, + "Response without Accept-Encoding: gzip should not have Content-Encoding header" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_gzip_compression_accepts_multiple_encodings() { + let api = api(); + let testctx = + common::test_setup("gzip_compression_multiple_encodings", api); + let client = &testctx.client_testctx; + + // Test that gzip works when client accepts multiple encodings including gzip + let uri = client.url("/large-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "deflate, gzip, br") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request with multiple accept encodings should succeed"); + + // Should still use gzip compression + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")), + "Response should use gzip when it's one of multiple accepted encodings" + ); + + // Verify the response can be decompressed + let compressed_body = get_response_bytes(&mut response).await; + let _decompressed = decompress_gzip(&compressed_body); // Should not panic + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_no_gzip_without_accept_encoding() { + let api = api(); + let testctx = common::test_setup("no_gzip_without_accept", api); + let client = &testctx.client_testctx; + + // Request without any Accept-Encoding header should not get compressed response + let response = client + .make_request_no_body(Method::GET, "/large-response", StatusCode::OK) + .await + .expect("Request without accept encoding should succeed"); + + // Should not have Content-Encoding header + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Response without Accept-Encoding should not be compressed" + ); + + testctx.teardown().await; +} diff --git a/dropshot/tests/integration-tests/main.rs b/dropshot/tests/integration-tests/main.rs index ec49604c1..fe10b99a4 100644 --- a/dropshot/tests/integration-tests/main.rs +++ b/dropshot/tests/integration-tests/main.rs @@ -16,6 +16,7 @@ mod config; mod custom_errors; mod demo; mod detached_shutdown; +mod gzip; mod multipart; mod openapi; mod pagination; From 023c2fe93145f12eca38156364ca4516ecd1bf6a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 20 Jun 2025 00:38:32 +0200 Subject: [PATCH 02/22] make the tests pass by implementing gzip --- dropshot/Cargo.toml | 1 + dropshot/src/compression.rs | 56 +++++++++++++++++++++++++++++++++++++ dropshot/src/lib.rs | 1 + dropshot/src/server.rs | 26 +++++++++++++++-- 4 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 dropshot/src/compression.rs diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 8826b48e0..48ccec165 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -20,6 +20,7 @@ base64 = "0.22.1" bytes = "1" camino = { version = "1.2.0", features = ["serde1"] } debug-ignore = "1.0.5" +flate2 = "1.0" form_urlencoded = "1.2.2" futures = "0.3.31" hostname = "0.4.0" diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs new file mode 100644 index 000000000..8032078dd --- /dev/null +++ b/dropshot/src/compression.rs @@ -0,0 +1,56 @@ +// Copyright 2025 Oxide Computer Company + +//! Response compression support for Dropshot. + +use crate::body::Body; +use bytes::Bytes; +use http::{HeaderMap, HeaderValue, Response}; +use http_body_util::BodyExt; +use std::io::Write; + +/// Checks if the request accepts gzip encoding based on the Accept-Encoding header. +pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { + if let Some(accept_encoding) = headers.get(http::header::ACCEPT_ENCODING) { + if let Ok(encoding_str) = accept_encoding.to_str() { + // Simple check for gzip in the Accept-Encoding header + // This handles cases like "gzip", "gzip, deflate", "deflate, gzip, br", etc. + return encoding_str + .split(',') + .any(|encoding| encoding.trim().eq_ignore_ascii_case("gzip")); + } + } + false +} + +/// Applies gzip compression to a response. +/// This is an async function that consumes the entire body, compresses it, and returns a new response. +pub async fn apply_gzip_compression( + response: Response, +) -> Result, Box> { + let (mut parts, body) = response.into_parts(); + + // Collect the entire body into bytes + let body_bytes = body.collect().await?.to_bytes(); + + // Compress the body using gzip + let mut encoder = flate2::write::GzEncoder::new( + Vec::new(), + flate2::Compression::default(), + ); + encoder.write_all(&body_bytes)?; + let compressed_bytes = encoder.finish()?; + + // Add gzip content-encoding header + parts.headers.insert( + http::header::CONTENT_ENCODING, + HeaderValue::from_static("gzip"), + ); + + // Remove content-length since it will be different after compression + parts.headers.remove(http::header::CONTENT_LENGTH); + + // Create a new body with compressed content + let compressed_body = Body::from(Bytes::from(compressed_bytes)); + + Ok(Response::from_parts(parts, compressed_body)) +} diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 01c6c10a6..b7129ca98 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -866,6 +866,7 @@ mod dtrace; mod api_description; mod body; +mod compression; mod config; mod error; mod error_status_code; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 40f49c77e..797020fe6 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -903,6 +903,10 @@ async fn http_request_handle( let request = request.map(crate::Body::wrap); let method = request.method(); let uri = request.uri(); + + // Store request headers for compression check before moving the request + let request_headers = request.headers().clone(); + let found_version = server.version_policy.request_version(&request, &request_log)?; let lookup_result = server.router.lookup_route( @@ -910,12 +914,13 @@ async fn http_request_handle( uri.path().into(), found_version.as_ref(), )?; + let request_info = RequestInfo::new(&request, remote_addr); let rqctx = RequestContext { server: Arc::clone(&server), - request: RequestInfo::new(&request, remote_addr), + request: request_info, endpoint: lookup_result.endpoint, request_id: request_id.to_string(), - log: request_log, + log: request_log.clone(), }; let handler = lookup_result.handler; @@ -930,7 +935,7 @@ async fn http_request_handle( // Spawn the handler so if we're cancelled, the handler still runs // to completion. let (tx, rx) = oneshot::channel(); - let request_log = rqctx.log.clone(); + let request_log = request_log.clone(); let worker = server.handler_waitgroup_worker.clone(); let handler_task = tokio::spawn(async move { let request_log = rqctx.log.clone(); @@ -981,6 +986,21 @@ async fn http_request_handle( } } }; + + // Apply gzip compression if appropriate + if crate::compression::accepts_gzip_encoding(&request_headers) + && !response.headers().contains_key(http::header::CONTENT_ENCODING) + && !response.headers().contains_key("x-dropshot-disable-compression") + { + response = crate::compression::apply_gzip_compression(response) + .await + .map_err(|e| { + HandlerError::Dropshot(crate::HttpError::for_internal_error( + format!("compression error: {}", e), + )) + })?; + } + response.headers_mut().insert( HEADER_REQUEST_ID, http::header::HeaderValue::from_str(&request_id).unwrap(), From 814b37cef527510b09f9e97e7aad26b050e49eb5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 20 Jun 2025 00:59:06 +0200 Subject: [PATCH 03/22] don't compress if transfer encoding chunked (bad) --- dropshot/src/compression.rs | 29 ++++++++++ dropshot/src/server.rs | 5 +- dropshot/tests/integration-tests/gzip.rs | 55 +++++++++++++++++++ dropshot/tests/integration-tests/streaming.rs | 2 +- 4 files changed, 86 insertions(+), 5 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index 8032078dd..cac390796 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -22,6 +22,35 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { false } +/// Determines if a response should be compressed with gzip. +pub fn should_compress_response( + request_headers: &HeaderMap, + response_headers: &HeaderMap, +) -> bool { + // Don't compress if client doesn't accept gzip + if !accepts_gzip_encoding(request_headers) { + return false; + } + + // Don't compress if already encoded + if response_headers.contains_key(http::header::CONTENT_ENCODING) { + return false; + } + + // Don't compress if explicitly disabled + if response_headers.contains_key("x-dropshot-disable-compression") { + return false; + } + + // Don't compress streaming responses (no content-length suggests streaming) + // This is a heuristic - responses without content-length are likely streaming + if !response_headers.contains_key(http::header::CONTENT_LENGTH) { + return false; + } + + true +} + /// Applies gzip compression to a response. /// This is an async function that consumes the entire body, compresses it, and returns a new response. pub async fn apply_gzip_compression( diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 797020fe6..d04d3d400 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -988,10 +988,7 @@ async fn http_request_handle( }; // Apply gzip compression if appropriate - if crate::compression::accepts_gzip_encoding(&request_headers) - && !response.headers().contains_key(http::header::CONTENT_ENCODING) - && !response.headers().contains_key("x-dropshot-disable-compression") - { + if crate::compression::should_compress_response(&request_headers, response.headers()) { response = crate::compression::apply_gzip_compression(response) .await .map_err(|e| { diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index ba8597766..f94aac37e 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -8,6 +8,7 @@ use dropshot::HttpError; use dropshot::HttpResponseOk; use dropshot::RequestContext; use http::{header, Method, StatusCode}; +use http_body_util::BodyExt; use hyper::{Request, Response}; use serde::{Deserialize, Serialize}; @@ -198,3 +199,57 @@ async fn test_no_gzip_without_accept_encoding() { testctx.teardown().await; } + +#[tokio::test] +async fn test_no_compression_for_streaming_responses() { + // Test that streaming responses are not compressed even when client accepts gzip + let api = crate::streaming::api(); + let testctx = common::test_setup("no_compression_streaming", api); + let client = &testctx.client_testctx; + + // Use custom request with Accept-Encoding header + let uri = client.url("/streaming"); + let request = hyper::Request::builder() + .method(http::Method::GET) + .uri(&uri) + .header(http::header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response = client + .make_request_with_request(request, http::StatusCode::OK) + .await + .expect("Streaming request with gzip accept should succeed"); + + // Debug: print all headers + println!("Response headers:"); + for (name, value) in response.headers().iter() { + println!(" {}: {:?}", name, value); + } + + // Should have chunked transfer encoding (check the way streaming tests do) + let transfer_encoding_header = response.headers().get("transfer-encoding"); + assert!( + transfer_encoding_header.is_some(), + "Streaming response should have transfer-encoding header. Available headers: {:?}", + response.headers().keys().collect::>() + ); + assert_eq!( + "chunked", + transfer_encoding_header.expect("expected transfer-encoding") + ); + + // Should NOT have gzip content encoding even though client accepts it + assert_eq!( + response.headers().get(http::header::CONTENT_ENCODING), + None, + "Streaming response should not be compressed even with Accept-Encoding: gzip" + ); + + // Verify it's actually streaming by consuming a chunk + if let Some(chunk) = response.body_mut().frame().await { + chunk.expect("Should have received chunk without error"); + } + + testctx.teardown().await; +} diff --git a/dropshot/tests/integration-tests/streaming.rs b/dropshot/tests/integration-tests/streaming.rs index 718fd8f2e..c56af265f 100644 --- a/dropshot/tests/integration-tests/streaming.rs +++ b/dropshot/tests/integration-tests/streaming.rs @@ -12,7 +12,7 @@ use crate::common; extern crate slog; -fn api() -> ApiDescription { +pub fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_streaming).unwrap(); api.register(api_not_streaming).unwrap(); From e274044e17c05e4f161bec870a29ea4e5c3dfd37 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 10:50:13 -0500 Subject: [PATCH 04/22] improve streaming detection and streaming test --- dropshot/src/compression.rs | 67 +++++++++++++++++++----- dropshot/src/server.rs | 5 +- dropshot/tests/integration-tests/gzip.rs | 32 ++++------- 3 files changed, 68 insertions(+), 36 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index cac390796..d53e2fc2d 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -6,6 +6,7 @@ use crate::body::Body; use bytes::Bytes; use http::{HeaderMap, HeaderValue, Response}; use http_body_util::BodyExt; +use hyper::body::Body as HttpBody; use std::io::Write; /// Checks if the request accepts gzip encoding based on the Accept-Encoding header. @@ -42,25 +43,57 @@ pub fn should_compress_response( return false; } - // Don't compress streaming responses (no content-length suggests streaming) - // This is a heuristic - responses without content-length are likely streaming - if !response_headers.contains_key(http::header::CONTENT_LENGTH) { - return false; + // Only compress compressible content types (text-based formats) + if let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) + { + if let Ok(ct_str) = content_type.to_str() { + let is_compressible = ct_str.starts_with("application/json") + || ct_str.starts_with("text/") + || ct_str.starts_with("application/xml") + || ct_str.starts_with("application/javascript") + || ct_str.starts_with("application/x-javascript"); + + if !is_compressible { + return false; + } + } } + // TODO: Check body size and only compress if above a threshold (e.g., 1KB) + // This requires reading the body, which we'll do during compression anyway. + true } +/// Minimum size in bytes for a response to be compressed. +/// Responses smaller than this won't benefit from compression and may actually get larger. +const MIN_COMPRESS_SIZE: usize = 1024; // 1KB + /// Applies gzip compression to a response. /// This is an async function that consumes the entire body, compresses it, and returns a new response. +/// If the body is smaller than MIN_COMPRESS_SIZE, compression is skipped. +/// Streaming responses (those without a known size) are not compressed. pub async fn apply_gzip_compression( response: Response, ) -> Result, Box> { let (mut parts, body) = response.into_parts(); + // Check if this is a streaming response (no exact size known) + // If so, don't compress it as buffering would defeat the purpose of streaming + let size_hint = body.size_hint(); + if size_hint.exact().is_none() { + return Ok(Response::from_parts(parts, body)); + } + // Collect the entire body into bytes let body_bytes = body.collect().await?.to_bytes(); + // Don't compress if the body is too small + if body_bytes.len() < MIN_COMPRESS_SIZE { + // Return the original response unchanged + return Ok(Response::from_parts(parts, Body::from(body_bytes))); + } + // Compress the body using gzip let mut encoder = flate2::write::GzEncoder::new( Vec::new(), @@ -69,17 +102,23 @@ pub async fn apply_gzip_compression( encoder.write_all(&body_bytes)?; let compressed_bytes = encoder.finish()?; - // Add gzip content-encoding header - parts.headers.insert( - http::header::CONTENT_ENCODING, - HeaderValue::from_static("gzip"), - ); + // Only use compression if it actually makes the response smaller + if compressed_bytes.len() < body_bytes.len() { + // Add gzip content-encoding header + parts.headers.insert( + http::header::CONTENT_ENCODING, + HeaderValue::from_static("gzip"), + ); - // Remove content-length since it will be different after compression - parts.headers.remove(http::header::CONTENT_LENGTH); + // Remove content-length since it will be different after compression + parts.headers.remove(http::header::CONTENT_LENGTH); - // Create a new body with compressed content - let compressed_body = Body::from(Bytes::from(compressed_bytes)); + // Create a new body with compressed content + let compressed_body = Body::from(Bytes::from(compressed_bytes)); - Ok(Response::from_parts(parts, compressed_body)) + Ok(Response::from_parts(parts, compressed_body)) + } else { + // Compression didn't help, return uncompressed + Ok(Response::from_parts(parts, Body::from(body_bytes))) + } } diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index d04d3d400..883a8a463 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -988,7 +988,10 @@ async fn http_request_handle( }; // Apply gzip compression if appropriate - if crate::compression::should_compress_response(&request_headers, response.headers()) { + if crate::compression::should_compress_response( + &request_headers, + response.headers(), + ) { response = crate::compression::apply_gzip_compression(response) .await .map_err(|e| { diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index f94aac37e..d794d53c3 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -8,7 +8,6 @@ use dropshot::HttpError; use dropshot::HttpResponseOk; use dropshot::RequestContext; use http::{header, Method, StatusCode}; -use http_body_util::BodyExt; use hyper::{Request, Response}; use serde::{Deserialize, Serialize}; @@ -207,7 +206,9 @@ async fn test_no_compression_for_streaming_responses() { let testctx = common::test_setup("no_compression_streaming", api); let client = &testctx.client_testctx; - // Use custom request with Accept-Encoding header + // Make request with Accept-Encoding: gzip header + // Note: We can't use make_request_no_body because it doesn't let us set custom headers + // So we'll use the RequestBuilder pattern used by the client internally let uri = client.url("/streaming"); let request = hyper::Request::builder() .method(http::Method::GET) @@ -221,35 +222,24 @@ async fn test_no_compression_for_streaming_responses() { .await .expect("Streaming request with gzip accept should succeed"); - // Debug: print all headers - println!("Response headers:"); - for (name, value) in response.headers().iter() { - println!(" {}: {:?}", name, value); - } - - // Should have chunked transfer encoding (check the way streaming tests do) + // Should have chunked transfer encoding let transfer_encoding_header = response.headers().get("transfer-encoding"); - assert!( - transfer_encoding_header.is_some(), - "Streaming response should have transfer-encoding header. Available headers: {:?}", - response.headers().keys().collect::>() - ); assert_eq!( - "chunked", - transfer_encoding_header.expect("expected transfer-encoding") + Some(&http::HeaderValue::from_static("chunked")), + transfer_encoding_header, + "Streaming response should have transfer-encoding: chunked" ); - // Should NOT have gzip content encoding even though client accepts it + // Should NOT have gzip content encoding even though client accepts it assert_eq!( response.headers().get(http::header::CONTENT_ENCODING), None, "Streaming response should not be compressed even with Accept-Encoding: gzip" ); - // Verify it's actually streaming by consuming a chunk - if let Some(chunk) = response.body_mut().frame().await { - chunk.expect("Should have received chunk without error"); - } + // Consume the body to verify it works (and to allow teardown to proceed) + let body_bytes = get_response_bytes(&mut response).await; + assert!(!body_bytes.is_empty(), "Streaming response should have content"); testctx.teardown().await; } From 1c4173b2e8e88510045f8f978a56fe6465caee72 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 10:58:06 -0500 Subject: [PATCH 05/22] work on review comments, especially test coverage --- dropshot/src/compression.rs | 126 +++++++++++---- dropshot/src/test_util.rs | 3 +- dropshot/tests/integration-tests/gzip.rs | 189 +++++++++++++++++++++++ 3 files changed, 290 insertions(+), 28 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index d53e2fc2d..ac44f4227 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -6,20 +6,91 @@ use crate::body::Body; use bytes::Bytes; use http::{HeaderMap, HeaderValue, Response}; use http_body_util::BodyExt; -use hyper::body::Body as HttpBody; +use hyper::body::Body as HttpBodyTrait; use std::io::Write; /// Checks if the request accepts gzip encoding based on the Accept-Encoding header. +/// Handles quality values (q parameter) and rejects gzip if q=0. pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { - if let Some(accept_encoding) = headers.get(http::header::ACCEPT_ENCODING) { - if let Ok(encoding_str) = accept_encoding.to_str() { - // Simple check for gzip in the Accept-Encoding header - // This handles cases like "gzip", "gzip, deflate", "deflate, gzip, br", etc. - return encoding_str - .split(',') - .any(|encoding| encoding.trim().eq_ignore_ascii_case("gzip")); + let Some(accept_encoding) = headers.get(http::header::ACCEPT_ENCODING) + else { + return false; + }; + + let Ok(encoding_str) = accept_encoding.to_str() else { + return false; + }; + + // Parse each encoding directive + for directive in encoding_str.split(',') { + let directive = directive.trim(); + + // Split on semicolon to separate encoding from parameters + let mut parts = directive.split(';'); + let encoding = parts.next().unwrap_or("").trim(); + + // Check if this is gzip or * (wildcard) + let is_gzip = encoding.eq_ignore_ascii_case("gzip"); + let is_wildcard = encoding == "*"; + + if !is_gzip && !is_wildcard { + continue; + } + + // Parse quality value if present + let mut quality = 1.0; + for param in parts { + let param = param.trim(); + if let Some(q_value) = param.strip_prefix("q=") { + if let Ok(q) = q_value.parse::() { + quality = q; + } + } + } + + // If quality is 0, this encoding is explicitly rejected + if quality == 0.0 { + if is_gzip { + return false; + } + // If wildcard is rejected, continue checking for explicit gzip + continue; + } + + // Accept gzip if quality > 0 + if is_gzip && quality > 0.0 { + return true; + } + + // Accept wildcard with quality > 0 (but keep looking for explicit gzip) + if is_wildcard && quality > 0.0 { + // Wildcard matches, but continue to see if gzip is explicitly mentioned + // We'll return true after checking all directives + } + } + + // Check if wildcard was present with non-zero quality + for directive in encoding_str.split(',') { + let directive = directive.trim(); + let mut parts = directive.split(';'); + let encoding = parts.next().unwrap_or("").trim(); + + if encoding == "*" { + let mut quality = 1.0; + for param in parts { + let param = param.trim(); + if let Some(q_value) = param.strip_prefix("q=") { + if let Ok(q) = q_value.parse::() { + quality = q; + } + } + } + if quality > 0.0 { + return true; + } } } + false } @@ -44,25 +115,23 @@ pub fn should_compress_response( } // Only compress compressible content types (text-based formats) - if let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) - { - if let Ok(ct_str) = content_type.to_str() { - let is_compressible = ct_str.starts_with("application/json") - || ct_str.starts_with("text/") - || ct_str.starts_with("application/xml") - || ct_str.starts_with("application/javascript") - || ct_str.starts_with("application/x-javascript"); - - if !is_compressible { - return false; - } - } - } + // If there's no content-type or it can't be parsed, don't compress + let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) + else { + return false; + }; - // TODO: Check body size and only compress if above a threshold (e.g., 1KB) - // This requires reading the body, which we'll do during compression anyway. + let Ok(ct_str) = content_type.to_str() else { + return false; + }; + + let is_compressible = ct_str.starts_with("application/json") + || ct_str.starts_with("text/") + || ct_str.starts_with("application/xml") + || ct_str.starts_with("application/javascript") + || ct_str.starts_with("application/x-javascript"); - true + is_compressible } /// Minimum size in bytes for a response to be compressed. @@ -110,8 +179,11 @@ pub async fn apply_gzip_compression( HeaderValue::from_static("gzip"), ); - // Remove content-length since it will be different after compression - parts.headers.remove(http::header::CONTENT_LENGTH); + // Set content-length to the compressed size + parts.headers.insert( + http::header::CONTENT_LENGTH, + HeaderValue::from(compressed_bytes.len()), + ); // Create a new body with compressed content let compressed_body = Body::from(Bytes::from(compressed_bytes)); diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 864bad17c..4335fef95 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -56,12 +56,13 @@ pub const TEST_HEADER_2: &str = "x-dropshot-test-header-2"; // List of allowed HTTP headers in responses. // Used to make sure we don't leak headers unexpectedly. -const ALLOWED_HEADERS: [AllowedHeader<'static>; 9] = [ +const ALLOWED_HEADERS: [AllowedHeader<'static>; 10] = [ AllowedHeader::new("content-encoding"), AllowedHeader::new("content-length"), AllowedHeader::new("content-type"), AllowedHeader::new("date"), AllowedHeader::new("location"), + AllowedHeader::new("x-dropshot-disable-compression"), AllowedHeader::new("x-request-id"), AllowedHeader { name: "transfer-encoding", diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index d794d53c3..0a4cb520d 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -25,6 +25,9 @@ struct LargeTestData { fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_large_response).unwrap(); + api.register(api_image_response).unwrap(); + api.register(api_small_response).unwrap(); + api.register(api_disable_compression_response).unwrap(); api } @@ -47,6 +50,65 @@ async fn api_large_response( })) } +/// Returns a binary response (image) that should not be compressed +#[endpoint { + method = GET, + path = "/image-response", +}] +async fn api_image_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + // Create a fake image response (just random bytes, but large enough) + let image_data = vec![0u8; 2048]; // 2KB of binary data + + Response::builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "image/png") + .body(dropshot::Body::from(image_data)) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + +/// Returns a small JSON response (under 1KB) that should not be compressed +#[endpoint { + method = GET, + path = "/small-response", +}] +async fn api_small_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + // Small response under 1KB + Ok(HttpResponseOk(LargeTestData { + message: "Small response".to_string(), + repeated_data: vec!["short".to_string()], + })) +} + +/// Returns a large response with compression disabled +#[endpoint { + method = GET, + path = "/disable-compression-response", +}] +async fn api_disable_compression_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + // Create a large response + let repeated_text = "This is some repetitive text. ".repeat(100); + let data = LargeTestData { + message: "Large response with compression disabled".to_string(), + repeated_data: vec![repeated_text; 10], + }; + + let json_body = serde_json::to_vec(&data) + .map_err(|e| HttpError::for_internal_error(e.to_string()))?; + + Response::builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "application/json") + .header("x-dropshot-disable-compression", "true") + .body(dropshot::Body::from(json_body)) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + async fn get_response_bytes( response: &mut Response, ) -> Vec { @@ -243,3 +305,130 @@ async fn test_no_compression_for_streaming_responses() { testctx.teardown().await; } + +#[tokio::test] +async fn test_no_compression_for_non_compressible_content_types() { + let api = api(); + let testctx = common::test_setup("no_compression_non_compressible", api); + let client = &testctx.client_testctx; + + // Request an image with Accept-Encoding: gzip + let uri = client.url("/image-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Image request should succeed"); + + // Binary content (images) should NOT be compressed + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Binary content (image/png) should not be compressed even with Accept-Encoding: gzip" + ); + + // Verify content-type is correct + assert_eq!( + response.headers().get(header::CONTENT_TYPE), + Some(&header::HeaderValue::from_static("image/png")), + "Content-Type should be image/png" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_compression_disabled_with_header() { + let api = api(); + let testctx = common::test_setup("compression_disabled_header", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip, but response has disable header + let uri = client.url("/disable-compression-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should NOT be compressed due to x-dropshot-disable-compression header + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Response with x-dropshot-disable-compression header should not be compressed" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_no_compression_below_size_threshold() { + let api = api(); + let testctx = common::test_setup("no_compression_small_response", api); + let client = &testctx.client_testctx; + + // Request a small response (under 1KB) with Accept-Encoding: gzip + let uri = client.url("/small-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Small response request should succeed"); + + // Small responses (under 1KB) should NOT be compressed + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Responses under 1KB should not be compressed" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_reject_gzip_with_quality_zero() { + let api = api(); + let testctx = common::test_setup("reject_gzip_quality_zero", api); + let client = &testctx.client_testctx; + + // Request with gzip explicitly rejected (q=0) + let uri = client.url("/large-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip;q=0, deflate") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should NOT be compressed since gzip has q=0 + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Response should not use gzip when client sets q=0 for gzip" + ); + + testctx.teardown().await; +} From 9c724c28a7dfa11f74d1f971664ad2d5d503d2f6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 11:19:29 -0500 Subject: [PATCH 06/22] try async-compression like tower --- Cargo.lock | 33 ++++++++ dropshot/Cargo.toml | 6 +- dropshot/src/compression.rs | 98 ++++++++++++------------ dropshot/src/server.rs | 9 +-- dropshot/tests/integration-tests/gzip.rs | 23 +++--- 5 files changed, 103 insertions(+), 66 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fb656fba8..93848eb0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,19 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "async-compression" +version = "0.4.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a89bce6054c720275ac2432fbba080a66a2106a44a1b804553930ca6909f4e0" +dependencies = [ + "compression-codecs", + "compression-core", + "futures-core", + "pin-project-lite", + "tokio", +] + [[package]] name = "async-stream" version = "0.3.6" @@ -213,6 +226,23 @@ dependencies = [ "windows-link 0.2.0", ] +[[package]] +name = "compression-codecs" +version = "0.4.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ef8a506ec4b81c460798f572caead636d57d3d7e940f998160f52bd254bf2d23" +dependencies = [ + "compression-core", + "flate2", + "memchr", +] + +[[package]] +name = "compression-core" +version = "0.4.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e47641d3deaf41fb1538ac1f54735925e275eaf3bf4d55c81b137fba797e5cbb" + [[package]] name = "concurrent-queue" version = "2.5.0" @@ -360,6 +390,7 @@ version = "0.16.4" dependencies = [ "anyhow", "async-channel", + "async-compression", "async-stream", "async-trait", "base64", @@ -414,6 +445,7 @@ dependencies = [ "tokio", "tokio-rustls 0.25.0", "tokio-tungstenite", + "tokio-util", "toml", "trybuild", "usdt", @@ -2550,6 +2582,7 @@ checksum = "5427d89453009325de0d8f342c9490009f76e999cb7672d77e46267448f7e6b2" dependencies = [ "bytes", "futures-core", + "futures-io", "futures-sink", "pin-project-lite", "tokio", diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 48ccec165..9f8383e1d 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -14,13 +14,13 @@ categories = ["network-programming", "web-programming::http-server"] workspace = true [dependencies] +async-compression = { version = "0.4", features = ["tokio", "gzip"] } async-stream = "0.3.6" async-trait = "0.1.89" base64 = "0.22.1" bytes = "1" camino = { version = "1.2.0", features = ["serde1"] } debug-ignore = "1.0.5" -flate2 = "1.0" form_urlencoded = "1.2.2" futures = "0.3.31" hostname = "0.4.0" @@ -78,6 +78,10 @@ features = [ "derive" ] version = "1.47" features = [ "full" ] +[dependencies.tokio-util] +version = "0.7" +features = [ "io", "compat" ] + [dependencies.usdt] version = "0.6.0" optional = true diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index ac44f4227..eb01cf9be 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -3,11 +3,11 @@ //! Response compression support for Dropshot. use crate::body::Body; -use bytes::Bytes; +use async_compression::tokio::bufread::GzipEncoder; +use futures::{StreamExt, TryStreamExt}; use http::{HeaderMap, HeaderValue, Response}; -use http_body_util::BodyExt; -use hyper::body::Body as HttpBodyTrait; -use std::io::Write; +use hyper::body::{Body as HttpBodyTrait, Frame}; +use tokio_util::io::{ReaderStream, StreamReader}; /// Checks if the request accepts gzip encoding based on the Accept-Encoding header. /// Handles quality values (q parameter) and rejects gzip if q=0. @@ -136,61 +136,59 @@ pub fn should_compress_response( /// Minimum size in bytes for a response to be compressed. /// Responses smaller than this won't benefit from compression and may actually get larger. -const MIN_COMPRESS_SIZE: usize = 1024; // 1KB +const MIN_COMPRESS_SIZE: u64 = 32; // 32 bytes -/// Applies gzip compression to a response. -/// This is an async function that consumes the entire body, compresses it, and returns a new response. -/// If the body is smaller than MIN_COMPRESS_SIZE, compression is skipped. -/// Streaming responses (those without a known size) are not compressed. -pub async fn apply_gzip_compression( +/// Applies gzip compression to a response using streaming compression. +/// This function wraps the response body in a gzip encoder that compresses data +/// as it's being sent, avoiding the need to buffer the entire response in memory. +/// If the body has a known exact size smaller than MIN_COMPRESS_SIZE, compression is skipped. +pub fn apply_gzip_compression( response: Response, ) -> Result, Box> { let (mut parts, body) = response.into_parts(); - // Check if this is a streaming response (no exact size known) - // If so, don't compress it as buffering would defeat the purpose of streaming + // Check the size hint to see if we should skip compression let size_hint = body.size_hint(); - if size_hint.exact().is_none() { - return Ok(Response::from_parts(parts, body)); + if let Some(exact_size) = size_hint.exact() { + if exact_size < MIN_COMPRESS_SIZE { + // Body is too small, don't compress + return Ok(Response::from_parts(parts, body)); + } } - // Collect the entire body into bytes - let body_bytes = body.collect().await?.to_bytes(); + // Convert body to a stream of data chunks + let data_stream = body.into_data_stream(); - // Don't compress if the body is too small - if body_bytes.len() < MIN_COMPRESS_SIZE { - // Return the original response unchanged - return Ok(Response::from_parts(parts, Body::from(body_bytes))); - } + // Map errors to io::Error so StreamReader can use them + let io_stream = data_stream + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e)); + + // Convert the stream to an AsyncRead using StreamReader + let async_read = StreamReader::new(io_stream); + + // Wrap in a buffered reader and then a GzipEncoder for streaming compression + let gzip_encoder = GzipEncoder::new(tokio::io::BufReader::new(async_read)); + + // Convert the encoder back to a stream using ReaderStream + let compressed_stream = ReaderStream::new(gzip_encoder); - // Compress the body using gzip - let mut encoder = flate2::write::GzEncoder::new( - Vec::new(), - flate2::Compression::default(), + // Convert the stream back to an HTTP body + let compressed_body = Body::wrap(http_body_util::StreamBody::new( + compressed_stream.map(|result| { + result.map(Frame::data).map_err(|e| { + Box::new(e) as Box + }) + }), + )); + + // Add gzip content-encoding header + parts.headers.insert( + http::header::CONTENT_ENCODING, + HeaderValue::from_static("gzip"), ); - encoder.write_all(&body_bytes)?; - let compressed_bytes = encoder.finish()?; - - // Only use compression if it actually makes the response smaller - if compressed_bytes.len() < body_bytes.len() { - // Add gzip content-encoding header - parts.headers.insert( - http::header::CONTENT_ENCODING, - HeaderValue::from_static("gzip"), - ); - - // Set content-length to the compressed size - parts.headers.insert( - http::header::CONTENT_LENGTH, - HeaderValue::from(compressed_bytes.len()), - ); - - // Create a new body with compressed content - let compressed_body = Body::from(Bytes::from(compressed_bytes)); - - Ok(Response::from_parts(parts, compressed_body)) - } else { - // Compression didn't help, return uncompressed - Ok(Response::from_parts(parts, Body::from(body_bytes))) - } + + // Remove content-length since we don't know the compressed size + parts.headers.remove(http::header::CONTENT_LENGTH); + + Ok(Response::from_parts(parts, compressed_body)) } diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 883a8a463..8aa8299b9 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -993,12 +993,11 @@ async fn http_request_handle( response.headers(), ) { response = crate::compression::apply_gzip_compression(response) - .await .map_err(|e| { - HandlerError::Dropshot(crate::HttpError::for_internal_error( - format!("compression error: {}", e), - )) - })?; + HandlerError::Dropshot(crate::HttpError::for_internal_error( + format!("compression error: {}", e), + )) + })?; } response.headers_mut().insert( diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 0a4cb520d..3dccabeec 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -22,6 +22,12 @@ struct LargeTestData { repeated_data: Vec, } +// Tiny test payload for testing size threshold +#[derive(Deserialize, Serialize, schemars::JsonSchema)] +struct TinyData { + x: u8, +} + fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_large_response).unwrap(); @@ -68,19 +74,16 @@ async fn api_image_response( .map_err(|e| HttpError::for_internal_error(e.to_string())) } -/// Returns a small JSON response (under 1KB) that should not be compressed +/// Returns a tiny JSON response (under 32 bytes) that should not be compressed #[endpoint { method = GET, path = "/small-response", }] async fn api_small_response( _rqctx: RequestContext, -) -> Result, HttpError> { - // Small response under 1KB - Ok(HttpResponseOk(LargeTestData { - message: "Small response".to_string(), - repeated_data: vec!["short".to_string()], - })) +) -> Result, HttpError> { + // Tiny response under 32 bytes threshold: {"x":0} is only 7 bytes + Ok(HttpResponseOk(TinyData { x: 0 })) } /// Returns a large response with compression disabled @@ -379,7 +382,7 @@ async fn test_no_compression_below_size_threshold() { let testctx = common::test_setup("no_compression_small_response", api); let client = &testctx.client_testctx; - // Request a small response (under 1KB) with Accept-Encoding: gzip + // Request a tiny response (under 32 bytes) with Accept-Encoding: gzip let uri = client.url("/small-response"); let request = Request::builder() .method(Method::GET) @@ -393,11 +396,11 @@ async fn test_no_compression_below_size_threshold() { .await .expect("Small response request should succeed"); - // Small responses (under 1KB) should NOT be compressed + // Tiny responses (under 32 bytes) should NOT be compressed assert_eq!( response.headers().get(header::CONTENT_ENCODING), None, - "Responses under 1KB should not be compressed" + "Responses under 32 bytes should not be compressed" ); testctx.teardown().await; From abc77de66b852181ca78ef290a7f3e14efa251c0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 11:33:50 -0500 Subject: [PATCH 07/22] address another round of review --- dropshot/src/compression.rs | 92 +++++++- dropshot/src/lib.rs | 1 + dropshot/src/server.rs | 12 +- dropshot/src/test_util.rs | 2 +- dropshot/tests/integration-tests/gzip.rs | 261 +++++++++++++++++++++-- 5 files changed, 337 insertions(+), 31 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index eb01cf9be..a72e54d3f 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -9,6 +9,14 @@ use http::{HeaderMap, HeaderValue, Response}; use hyper::body::{Body as HttpBodyTrait, Frame}; use tokio_util::io::{ReaderStream, StreamReader}; +/// Marker type for disabling compression on a response. +/// Insert this into response extensions to prevent compression: +/// ```ignore +/// response.extensions_mut().insert(NoCompression); +/// ``` +#[derive(Debug, Clone, Copy)] +pub struct NoCompression; + /// Checks if the request accepts gzip encoding based on the Accept-Encoding header. /// Handles quality values (q parameter) and rejects gzip if q=0. pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { @@ -96,9 +104,32 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { /// Determines if a response should be compressed with gzip. pub fn should_compress_response( + request_method: &http::Method, request_headers: &HeaderMap, + response_status: http::StatusCode, response_headers: &HeaderMap, + response_extensions: &http::Extensions, ) -> bool { + // Don't compress responses that must not have a body + // (1xx informational, 204 No Content, 304 Not Modified) + if response_status.is_informational() + || response_status == http::StatusCode::NO_CONTENT + || response_status == http::StatusCode::NOT_MODIFIED + { + return false; + } + + // Don't compress HEAD requests (they have no body) + if request_method == http::Method::HEAD { + return false; + } + + // Don't compress partial content responses (206) + // Compressing already-ranged content changes the meaning for clients + if response_status == http::StatusCode::PARTIAL_CONTENT { + return false; + } + // Don't compress if client doesn't accept gzip if !accepts_gzip_encoding(request_headers) { return false; @@ -109,8 +140,8 @@ pub fn should_compress_response( return false; } - // Don't compress if explicitly disabled - if response_headers.contains_key("x-dropshot-disable-compression") { + // Don't compress if explicitly disabled via extension + if response_extensions.get::().is_some() { return false; } @@ -125,34 +156,47 @@ pub fn should_compress_response( return false; }; + // Don't compress Server-Sent Events (SSE) - these are streaming responses + // where we want low latency, not compression + if ct_str.starts_with("text/event-stream") { + return false; + } + + // Check for standard compressible content types let is_compressible = ct_str.starts_with("application/json") || ct_str.starts_with("text/") || ct_str.starts_with("application/xml") || ct_str.starts_with("application/javascript") || ct_str.starts_with("application/x-javascript"); - is_compressible + // Also check for structured syntax suffixes (+json, +xml) + // This handles media types like application/problem+json, application/hal+json, etc. + // See RFC 6839 for structured syntax suffix registration + let has_compressible_suffix = + ct_str.contains("+json") || ct_str.contains("+xml"); + + is_compressible || has_compressible_suffix } /// Minimum size in bytes for a response to be compressed. /// Responses smaller than this won't benefit from compression and may actually get larger. -const MIN_COMPRESS_SIZE: u64 = 32; // 32 bytes +const MIN_COMPRESS_SIZE: u64 = 512; // 512 bytes /// Applies gzip compression to a response using streaming compression. /// This function wraps the response body in a gzip encoder that compresses data /// as it's being sent, avoiding the need to buffer the entire response in memory. /// If the body has a known exact size smaller than MIN_COMPRESS_SIZE, compression is skipped. -pub fn apply_gzip_compression( - response: Response, -) -> Result, Box> { +/// On any error during compression setup, this function logs the error and returns +/// the original uncompressed response. +pub fn apply_gzip_compression(response: Response) -> Response { let (mut parts, body) = response.into_parts(); // Check the size hint to see if we should skip compression let size_hint = body.size_hint(); if let Some(exact_size) = size_hint.exact() { - if exact_size < MIN_COMPRESS_SIZE { - // Body is too small, don't compress - return Ok(Response::from_parts(parts, body)); + if exact_size == 0 || exact_size < MIN_COMPRESS_SIZE { + // Body is empty or too small, don't compress + return Response::from_parts(parts, body); } } @@ -187,8 +231,34 @@ pub fn apply_gzip_compression( HeaderValue::from_static("gzip"), ); + // Add or update Vary header to include Accept-Encoding + // This is critical for HTTP caching - caches must not serve compressed + // responses to clients that don't accept gzip + if let Some(existing_vary) = parts.headers.get(http::header::VARY) { + // Vary header exists, append Accept-Encoding if not already present + if let Ok(vary_str) = existing_vary.to_str() { + let has_accept_encoding = vary_str + .split(',') + .any(|v| v.trim().eq_ignore_ascii_case("accept-encoding")); + + if !has_accept_encoding { + // Append Accept-Encoding to existing Vary header + let new_vary = format!("{}, Accept-Encoding", vary_str); + if let Ok(new_vary_value) = HeaderValue::from_str(&new_vary) { + parts.headers.insert(http::header::VARY, new_vary_value); + } + } + } + } else { + // No Vary header exists, set it to Accept-Encoding + parts.headers.insert( + http::header::VARY, + HeaderValue::from_static("Accept-Encoding"), + ); + } + // Remove content-length since we don't know the compressed size parts.headers.remove(http::header::CONTENT_LENGTH); - Ok(Response::from_parts(parts, compressed_body)) + Response::from_parts(parts, compressed_body) } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index b7129ca98..2efcb4bd3 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -906,6 +906,7 @@ pub use api_description::TagConfig; pub use api_description::TagDetails; pub use api_description::TagExternalDocs; pub use body::Body; +pub use compression::NoCompression; pub use config::ConfigDropshot; pub use config::ConfigTls; pub use config::HandlerTaskMode; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 8aa8299b9..24c294ee0 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -901,7 +901,7 @@ async fn http_request_handle( // this to take forever. // TODO-correctness: Do we need to dump the body on errors? let request = request.map(crate::Body::wrap); - let method = request.method(); + let method = request.method().clone(); let uri = request.uri(); // Store request headers for compression check before moving the request @@ -989,15 +989,13 @@ async fn http_request_handle( // Apply gzip compression if appropriate if crate::compression::should_compress_response( + &method, &request_headers, + response.status(), response.headers(), + response.extensions(), ) { - response = crate::compression::apply_gzip_compression(response) - .map_err(|e| { - HandlerError::Dropshot(crate::HttpError::for_internal_error( - format!("compression error: {}", e), - )) - })?; + response = crate::compression::apply_gzip_compression(response); } response.headers_mut().insert( diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 4335fef95..d3353aa01 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -62,7 +62,7 @@ const ALLOWED_HEADERS: [AllowedHeader<'static>; 10] = [ AllowedHeader::new("content-type"), AllowedHeader::new("date"), AllowedHeader::new("location"), - AllowedHeader::new("x-dropshot-disable-compression"), + AllowedHeader::new("vary"), AllowedHeader::new("x-request-id"), AllowedHeader { name: "transfer-encoding", diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 3dccabeec..671d041b8 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -34,6 +34,10 @@ fn api() -> ApiDescription { api.register(api_image_response).unwrap(); api.register(api_small_response).unwrap(); api.register(api_disable_compression_response).unwrap(); + api.register(api_json_suffix_response).unwrap(); + api.register(api_xml_suffix_response).unwrap(); + api.register(api_no_content_response).unwrap(); + api.register(api_not_modified_response).unwrap(); api } @@ -74,7 +78,7 @@ async fn api_image_response( .map_err(|e| HttpError::for_internal_error(e.to_string())) } -/// Returns a tiny JSON response (under 32 bytes) that should not be compressed +/// Returns a tiny JSON response (under 512 bytes) that should not be compressed #[endpoint { method = GET, path = "/small-response", @@ -82,7 +86,7 @@ async fn api_image_response( async fn api_small_response( _rqctx: RequestContext, ) -> Result, HttpError> { - // Tiny response under 32 bytes threshold: {"x":0} is only 7 bytes + // Tiny response under 512 bytes threshold: {"x":0} is only 7 bytes Ok(HttpResponseOk(TinyData { x: 0 })) } @@ -104,11 +108,83 @@ async fn api_disable_compression_response( let json_body = serde_json::to_vec(&data) .map_err(|e| HttpError::for_internal_error(e.to_string()))?; - Response::builder() + let mut response = Response::builder() .status(StatusCode::OK) .header(header::CONTENT_TYPE, "application/json") - .header("x-dropshot-disable-compression", "true") .body(dropshot::Body::from(json_body)) + .map_err(|e| HttpError::for_internal_error(e.to_string()))?; + + // Disable compression using the NoCompression extension + response.extensions_mut().insert(dropshot::NoCompression); + + Ok(response) +} + +/// Returns a response with application/problem+json content type +#[endpoint { + method = GET, + path = "/json-suffix-response", +}] +async fn api_json_suffix_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + let data = LargeTestData { + message: "Testing +json suffix".to_string(), + repeated_data: vec!["data".to_string(); 100], + }; + + let json_body = serde_json::to_vec(&data) + .map_err(|e| HttpError::for_internal_error(e.to_string()))?; + + Response::builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "application/problem+json") + .body(dropshot::Body::from(json_body)) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + +/// Returns a response with application/soap+xml content type +#[endpoint { + method = GET, + path = "/xml-suffix-response", +}] +async fn api_xml_suffix_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + let xml_body = "".repeat(100).into_bytes(); + + Response::builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "application/soap+xml") + .body(dropshot::Body::from(xml_body)) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + +/// Returns a 204 No Content response +#[endpoint { + method = GET, + path = "/no-content-response", +}] +async fn api_no_content_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + Response::builder() + .status(StatusCode::NO_CONTENT) + .body(dropshot::Body::empty()) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + +/// Returns a 304 Not Modified response +#[endpoint { + method = GET, + path = "/not-modified-response", +}] +async fn api_not_modified_response( + _rqctx: RequestContext, +) -> Result, HttpError> { + Response::builder() + .status(StatusCode::NOT_MODIFIED) + .body(dropshot::Body::empty()) .map_err(|e| HttpError::for_internal_error(e.to_string())) } @@ -347,12 +423,12 @@ async fn test_no_compression_for_non_compressible_content_types() { } #[tokio::test] -async fn test_compression_disabled_with_header() { +async fn test_compression_disabled_with_extension() { let api = api(); - let testctx = common::test_setup("compression_disabled_header", api); + let testctx = common::test_setup("compression_disabled_extension", api); let client = &testctx.client_testctx; - // Request with Accept-Encoding: gzip, but response has disable header + // Request with Accept-Encoding: gzip, but response has NoCompression extension let uri = client.url("/disable-compression-response"); let request = Request::builder() .method(Method::GET) @@ -366,11 +442,11 @@ async fn test_compression_disabled_with_header() { .await .expect("Request should succeed"); - // Should NOT be compressed due to x-dropshot-disable-compression header + // Should NOT be compressed due to NoCompression extension assert_eq!( response.headers().get(header::CONTENT_ENCODING), None, - "Response with x-dropshot-disable-compression header should not be compressed" + "Response with NoCompression extension should not be compressed" ); testctx.teardown().await; @@ -382,7 +458,7 @@ async fn test_no_compression_below_size_threshold() { let testctx = common::test_setup("no_compression_small_response", api); let client = &testctx.client_testctx; - // Request a tiny response (under 32 bytes) with Accept-Encoding: gzip + // Request a tiny response (under 512 bytes) with Accept-Encoding: gzip let uri = client.url("/small-response"); let request = Request::builder() .method(Method::GET) @@ -396,11 +472,11 @@ async fn test_no_compression_below_size_threshold() { .await .expect("Small response request should succeed"); - // Tiny responses (under 32 bytes) should NOT be compressed + // Tiny responses (under 512 bytes) should NOT be compressed assert_eq!( response.headers().get(header::CONTENT_ENCODING), None, - "Responses under 32 bytes should not be compressed" + "Responses under 512 bytes should not be compressed" ); testctx.teardown().await; @@ -435,3 +511,164 @@ async fn test_reject_gzip_with_quality_zero() { testctx.teardown().await; } + +#[tokio::test] +async fn test_vary_header_is_set() { + let api = api(); + let testctx = common::test_setup("vary_header_set", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip + let uri = client.url("/large-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should have Vary: Accept-Encoding header + assert!( + response.headers().contains_key(header::VARY), + "Response should have Vary header" + ); + + let vary_value = + response.headers().get(header::VARY).unwrap().to_str().unwrap(); + assert!( + vary_value.to_lowercase().contains("accept-encoding"), + "Vary header should include Accept-Encoding, got: {}", + vary_value + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_json_suffix_is_compressed() { + let api = api(); + let testctx = common::test_setup("json_suffix_compressed", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip for application/problem+json + let uri = client.url("/json-suffix-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should be compressed since application/problem+json has +json suffix + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")), + "Response with +json suffix should be compressed" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_xml_suffix_is_compressed() { + let api = api(); + let testctx = common::test_setup("xml_suffix_compressed", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip for application/soap+xml + let uri = client.url("/xml-suffix-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should be compressed since application/soap+xml has +xml suffix + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")), + "Response with +xml suffix should be compressed" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_no_compression_for_204_no_content() { + let api = api(); + let testctx = common::test_setup("no_compression_204", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip for 204 response + let uri = client.url("/no-content-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::NO_CONTENT) + .await + .expect("Request should succeed"); + + // Should NOT be compressed (204 must not have body) + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "204 No Content should not have Content-Encoding header" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_no_compression_for_304_not_modified() { + let api = api(); + let testctx = common::test_setup("no_compression_304", api); + let client = &testctx.client_testctx; + + // Request with Accept-Encoding: gzip for 304 response + let uri = client.url("/not-modified-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::NOT_MODIFIED) + .await + .expect("Request should succeed"); + + // Should NOT be compressed (304 must not have body) + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "304 Not Modified should not have Content-Encoding header" + ); + + testctx.teardown().await; +} + +// Note: HEAD request test is omitted from integration tests because Dropshot +// requires explicit HEAD endpoint registration. The HEAD logic is tested via +// unit tests in should_compress_response. From 6f8ce4160ffa35880eeaef786eb74206b8643267 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 11:48:59 -0500 Subject: [PATCH 08/22] one last review cycle: unit tests --- dropshot/src/compression.rs | 473 +++++++++++++++++++++++++++++++++++- 1 file changed, 471 insertions(+), 2 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index a72e54d3f..f24c06b69 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -186,8 +186,6 @@ const MIN_COMPRESS_SIZE: u64 = 512; // 512 bytes /// This function wraps the response body in a gzip encoder that compresses data /// as it's being sent, avoiding the need to buffer the entire response in memory. /// If the body has a known exact size smaller than MIN_COMPRESS_SIZE, compression is skipped. -/// On any error during compression setup, this function logs the error and returns -/// the original uncompressed response. pub fn apply_gzip_compression(response: Response) -> Response { let (mut parts, body) = response.into_parts(); @@ -244,6 +242,8 @@ pub fn apply_gzip_compression(response: Response) -> Response { if !has_accept_encoding { // Append Accept-Encoding to existing Vary header let new_vary = format!("{}, Accept-Encoding", vary_str); + // Note: If HeaderValue::from_str fails (e.g., malformed header), + // we silently skip updating the Vary header to preserve existing behavior if let Ok(new_vary_value) = HeaderValue::from_str(&new_vary) { parts.headers.insert(http::header::VARY, new_vary_value); } @@ -262,3 +262,472 @@ pub fn apply_gzip_compression(response: Response) -> Response { Response::from_parts(parts, compressed_body) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_accepts_gzip_encoding_basic() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_with_positive_quality() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=0.8"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_rejects_zero_quality() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=0"), + ); + assert!(!accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_wildcard() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("*"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_wildcard_with_quality() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("*;q=0.5"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_wildcard_rejected() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("*;q=0"), + ); + assert!(!accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_multiple_encodings() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("deflate, gzip, br"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_gzip_takes_precedence_over_wildcard() { + // Explicit gzip rejection should override wildcard acceptance + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("*;q=1.0, gzip;q=0"), + ); + assert!(!accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_gzip_acceptance_overrides_wildcard_rejection() { + // Explicit gzip acceptance should work even if wildcard is rejected + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("*;q=0, gzip;q=1.0"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_case_insensitive() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("GZIP"), + ); + assert!(accepts_gzip_encoding(&headers)); + + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("GzIp"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_no_header() { + let headers = HeaderMap::new(); + assert!(!accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_with_spaces() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("deflate , gzip ; q=0.8 , br"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_accepts_gzip_encoding_malformed_quality() { + // If quality parsing fails, should default to 1.0 + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=invalid"), + ); + assert!(accepts_gzip_encoding(&headers)); + } + + #[test] + fn test_should_compress_response_basic() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + let extensions = http::Extensions::new(); + + assert!(should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_head_method() { + let method = http::Method::HEAD; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_no_content() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::NO_CONTENT; + let response_headers = HeaderMap::new(); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_not_modified() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::NOT_MODIFIED; + let response_headers = HeaderMap::new(); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_partial_content() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::PARTIAL_CONTENT; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_no_accept_encoding() { + let method = http::Method::GET; + let request_headers = HeaderMap::new(); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_already_encoded() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + response_headers.insert( + http::header::CONTENT_ENCODING, + HeaderValue::from_static("br"), + ); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_no_compression_extension() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + let mut extensions = http::Extensions::new(); + extensions.insert(NoCompression); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_no_content_type() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let response_headers = HeaderMap::new(); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_sse() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("text/event-stream"), + ); + let extensions = http::Extensions::new(); + + assert!(!should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + )); + } + + #[test] + fn test_should_compress_response_compressible_content_types() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let extensions = http::Extensions::new(); + + // Test various compressible content types + let compressible_types = vec![ + "application/json", + "text/plain", + "text/html", + "text/css", + "application/xml", + "application/javascript", + "application/x-javascript", + "application/problem+json", + "application/hal+json", + "application/soap+xml", + ]; + + for content_type in compressible_types { + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_str(content_type).unwrap(), + ); + + assert!( + should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + ), + "Expected {} to be compressible", + content_type + ); + } + } + + #[test] + fn test_should_compress_response_non_compressible_content_types() { + let method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + let status = http::StatusCode::OK; + let extensions = http::Extensions::new(); + + // Test various non-compressible content types + let non_compressible_types = vec![ + "image/png", + "image/jpeg", + "video/mp4", + "application/pdf", + "application/zip", + "application/gzip", + "application/octet-stream", + ]; + + for content_type in non_compressible_types { + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_str(content_type).unwrap(), + ); + + assert!( + !should_compress_response( + &method, + &request_headers, + status, + &response_headers, + &extensions + ), + "Expected {} to not be compressible", + content_type + ); + } + } +} From 208aea573d0911f871ee9fb3fecfd832feb330d5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 11:59:02 -0500 Subject: [PATCH 09/22] tighten things up --- dropshot/src/compression.rs | 82 ++++++++----------------------------- dropshot/src/server.rs | 3 -- 2 files changed, 17 insertions(+), 68 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index f24c06b69..d29276f11 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -24,28 +24,22 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { else { return false; }; - let Ok(encoding_str) = accept_encoding.to_str() else { return false; }; - // Parse each encoding directive + // First pass: look for explicit gzip directive for directive in encoding_str.split(',') { let directive = directive.trim(); - - // Split on semicolon to separate encoding from parameters let mut parts = directive.split(';'); let encoding = parts.next().unwrap_or("").trim(); - // Check if this is gzip or * (wildcard) let is_gzip = encoding.eq_ignore_ascii_case("gzip"); let is_wildcard = encoding == "*"; - if !is_gzip && !is_wildcard { continue; } - // Parse quality value if present let mut quality = 1.0; for param in parts { let param = param.trim(); @@ -56,28 +50,14 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { } } - // If quality is 0, this encoding is explicitly rejected - if quality == 0.0 { - if is_gzip { - return false; - } - // If wildcard is rejected, continue checking for explicit gzip - continue; - } - - // Accept gzip if quality > 0 - if is_gzip && quality > 0.0 { - return true; - } - - // Accept wildcard with quality > 0 (but keep looking for explicit gzip) - if is_wildcard && quality > 0.0 { - // Wildcard matches, but continue to see if gzip is explicitly mentioned - // We'll return true after checking all directives + match (is_gzip, quality) { + (true, 0.0) => return false, // Explicit gzip rejection + (true, q) if q > 0.0 => return true, // Explicit gzip acceptance + _ => continue, // Wildcard or rejected wildcard } } - // Check if wildcard was present with non-zero quality + // Second pass: check if wildcard accepts gzip (explicit gzip takes precedence) for directive in encoding_str.split(',') { let directive = directive.trim(); let mut parts = directive.split(';'); @@ -110,8 +90,7 @@ pub fn should_compress_response( response_headers: &HeaderMap, response_extensions: &http::Extensions, ) -> bool { - // Don't compress responses that must not have a body - // (1xx informational, 204 No Content, 304 Not Modified) + // Responses that must not have a body per HTTP spec if response_status.is_informational() || response_status == http::StatusCode::NO_CONTENT || response_status == http::StatusCode::NOT_MODIFIED @@ -119,59 +98,49 @@ pub fn should_compress_response( return false; } - // Don't compress HEAD requests (they have no body) + // HEAD responses have no body if request_method == http::Method::HEAD { return false; } - // Don't compress partial content responses (206) - // Compressing already-ranged content changes the meaning for clients + // Compressing partial content changes the meaning for clients if response_status == http::StatusCode::PARTIAL_CONTENT { return false; } - // Don't compress if client doesn't accept gzip if !accepts_gzip_encoding(request_headers) { return false; } - // Don't compress if already encoded if response_headers.contains_key(http::header::CONTENT_ENCODING) { return false; } - // Don't compress if explicitly disabled via extension if response_extensions.get::().is_some() { return false; } - // Only compress compressible content types (text-based formats) - // If there's no content-type or it can't be parsed, don't compress + // Only compress when we know the content type let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) else { return false; }; - let Ok(ct_str) = content_type.to_str() else { return false; }; - // Don't compress Server-Sent Events (SSE) - these are streaming responses - // where we want low latency, not compression + // SSE streams prioritize latency over compression if ct_str.starts_with("text/event-stream") { return false; } - // Check for standard compressible content types let is_compressible = ct_str.starts_with("application/json") || ct_str.starts_with("text/") || ct_str.starts_with("application/xml") || ct_str.starts_with("application/javascript") || ct_str.starts_with("application/x-javascript"); - // Also check for structured syntax suffixes (+json, +xml) - // This handles media types like application/problem+json, application/hal+json, etc. - // See RFC 6839 for structured syntax suffix registration + // RFC 6839 structured syntax suffixes (+json, +xml) let has_compressible_suffix = ct_str.contains("+json") || ct_str.contains("+xml"); @@ -180,7 +149,7 @@ pub fn should_compress_response( /// Minimum size in bytes for a response to be compressed. /// Responses smaller than this won't benefit from compression and may actually get larger. -const MIN_COMPRESS_SIZE: u64 = 512; // 512 bytes +const MIN_COMPRESS_SIZE: u64 = 512; /// Applies gzip compression to a response using streaming compression. /// This function wraps the response body in a gzip encoder that compresses data @@ -189,32 +158,22 @@ const MIN_COMPRESS_SIZE: u64 = 512; // 512 bytes pub fn apply_gzip_compression(response: Response) -> Response { let (mut parts, body) = response.into_parts(); - // Check the size hint to see if we should skip compression let size_hint = body.size_hint(); if let Some(exact_size) = size_hint.exact() { if exact_size == 0 || exact_size < MIN_COMPRESS_SIZE { - // Body is empty or too small, don't compress return Response::from_parts(parts, body); } } - // Convert body to a stream of data chunks + // Transform body into a compressed stream: + // Body -> Stream -> AsyncRead -> GzipEncoder -> Stream -> Body let data_stream = body.into_data_stream(); - - // Map errors to io::Error so StreamReader can use them let io_stream = data_stream .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e)); - - // Convert the stream to an AsyncRead using StreamReader let async_read = StreamReader::new(io_stream); - - // Wrap in a buffered reader and then a GzipEncoder for streaming compression let gzip_encoder = GzipEncoder::new(tokio::io::BufReader::new(async_read)); - - // Convert the encoder back to a stream using ReaderStream let compressed_stream = ReaderStream::new(gzip_encoder); - // Convert the stream back to an HTTP body let compressed_body = Body::wrap(http_body_util::StreamBody::new( compressed_stream.map(|result| { result.map(Frame::data).map_err(|e| { @@ -223,41 +182,34 @@ pub fn apply_gzip_compression(response: Response) -> Response { }), )); - // Add gzip content-encoding header parts.headers.insert( http::header::CONTENT_ENCODING, HeaderValue::from_static("gzip"), ); - // Add or update Vary header to include Accept-Encoding - // This is critical for HTTP caching - caches must not serve compressed + // Vary header is critical for caching - prevents serving compressed // responses to clients that don't accept gzip if let Some(existing_vary) = parts.headers.get(http::header::VARY) { - // Vary header exists, append Accept-Encoding if not already present if let Ok(vary_str) = existing_vary.to_str() { let has_accept_encoding = vary_str .split(',') .any(|v| v.trim().eq_ignore_ascii_case("accept-encoding")); if !has_accept_encoding { - // Append Accept-Encoding to existing Vary header let new_vary = format!("{}, Accept-Encoding", vary_str); - // Note: If HeaderValue::from_str fails (e.g., malformed header), - // we silently skip updating the Vary header to preserve existing behavior + // Silently skip on malformed header to preserve existing behavior if let Ok(new_vary_value) = HeaderValue::from_str(&new_vary) { parts.headers.insert(http::header::VARY, new_vary_value); } } } } else { - // No Vary header exists, set it to Accept-Encoding parts.headers.insert( http::header::VARY, HeaderValue::from_static("Accept-Encoding"), ); } - // Remove content-length since we don't know the compressed size parts.headers.remove(http::header::CONTENT_LENGTH); Response::from_parts(parts, compressed_body) diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 24c294ee0..0f7ef4272 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -903,8 +903,6 @@ async fn http_request_handle( let request = request.map(crate::Body::wrap); let method = request.method().clone(); let uri = request.uri(); - - // Store request headers for compression check before moving the request let request_headers = request.headers().clone(); let found_version = @@ -987,7 +985,6 @@ async fn http_request_handle( } }; - // Apply gzip compression if appropriate if crate::compression::should_compress_response( &method, &request_headers, From 0836813d3850510577147e4ee051f4a284b8349a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 12:11:24 -0500 Subject: [PATCH 10/22] handle multiple quality values, make headers case-insensitive --- dropshot/src/compression.rs | 159 +++++++++++++++++++++++------------- 1 file changed, 102 insertions(+), 57 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index d29276f11..a6d920af0 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -17,68 +17,83 @@ use tokio_util::io::{ReaderStream, StreamReader}; #[derive(Debug, Clone, Copy)] pub struct NoCompression; -/// Checks if the request accepts gzip encoding based on the Accept-Encoding header. -/// Handles quality values (q parameter) and rejects gzip if q=0. -pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { - let Some(accept_encoding) = headers.get(http::header::ACCEPT_ENCODING) - else { - return false; - }; - let Ok(encoding_str) = accept_encoding.to_str() else { - return false; +/// Parses the `Accept-Encoding` header into a list of encodings and their +/// associated quality factors. Returns the encoding names in lowercase for +/// easier comparisons. +fn parse_accept_encoding(header: &HeaderValue) -> Vec<(String, f32)> { + const DEFAULT_QUALITY: f32 = 1.0; + + let Ok(header_value) = header.to_str() else { + return Vec::new(); }; - // First pass: look for explicit gzip directive - for directive in encoding_str.split(',') { - let directive = directive.trim(); - let mut parts = directive.split(';'); - let encoding = parts.next().unwrap_or("").trim(); + header_value + .split(',') + .filter_map(|directive| { + let mut parts = directive.trim().split(';'); + let encoding = parts.next()?.trim(); + if encoding.is_empty() { + return None; + } - let is_gzip = encoding.eq_ignore_ascii_case("gzip"); - let is_wildcard = encoding == "*"; - if !is_gzip && !is_wildcard { - continue; - } + let mut quality = DEFAULT_QUALITY; + for param in parts { + let mut param = param.splitn(2, '='); + let name = param.next()?.trim(); + let value = param.next()?.trim(); - let mut quality = 1.0; - for param in parts { - let param = param.trim(); - if let Some(q_value) = param.strip_prefix("q=") { - if let Ok(q) = q_value.parse::() { - quality = q; + if name.eq_ignore_ascii_case("q") { + if let Ok(parsed) = value.parse::() { + quality = parsed.clamp(0.0, 1.0); + } } } - } - match (is_gzip, quality) { - (true, 0.0) => return false, // Explicit gzip rejection - (true, q) if q > 0.0 => return true, // Explicit gzip acceptance - _ => continue, // Wildcard or rejected wildcard - } - } + Some((encoding.to_ascii_lowercase(), quality)) + }) + .collect() +} - // Second pass: check if wildcard accepts gzip (explicit gzip takes precedence) - for directive in encoding_str.split(',') { - let directive = directive.trim(); - let mut parts = directive.split(';'); - let encoding = parts.next().unwrap_or("").trim(); +/// Checks if the request accepts gzip encoding based on the Accept-Encoding header. +/// Handles quality values (q parameter) using RFC-compliant preference rules. +pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { + let Some(accept_encoding) = headers.get(http::header::ACCEPT_ENCODING) + else { + return false; + }; - if encoding == "*" { - let mut quality = 1.0; - for param in parts { - let param = param.trim(); - if let Some(q_value) = param.strip_prefix("q=") { - if let Ok(q) = q_value.parse::() { - quality = q; - } - } + let mut best_gzip_quality: Option = None; + let mut best_wildcard_quality: Option = None; + + // RFC 9110 §12.5.3 specifies that the most preferred (highest quality) + // representation wins, so we retain the maximum q-value we see for each + // relevant coding. + for (encoding, quality) in parse_accept_encoding(accept_encoding) { + match encoding.as_str() { + "gzip" => { + best_gzip_quality = Some( + best_gzip_quality + .map_or(quality, |current| current.max(quality)), + ); } - if quality > 0.0 { - return true; + "*" => { + best_wildcard_quality = Some( + best_wildcard_quality + .map_or(quality, |current| current.max(quality)), + ); } + _ => {} } } + if let Some(quality) = best_gzip_quality { + return quality > 0.0; + } + + if let Some(quality) = best_wildcard_quality { + return quality > 0.0; + } + false } @@ -129,20 +144,22 @@ pub fn should_compress_response( return false; }; + let ct_lower = ct_str.to_ascii_lowercase(); + // SSE streams prioritize latency over compression - if ct_str.starts_with("text/event-stream") { + if ct_lower.starts_with("text/event-stream") { return false; } - let is_compressible = ct_str.starts_with("application/json") - || ct_str.starts_with("text/") - || ct_str.starts_with("application/xml") - || ct_str.starts_with("application/javascript") - || ct_str.starts_with("application/x-javascript"); + let is_compressible = ct_lower.starts_with("application/json") + || ct_lower.starts_with("text/") + || ct_lower.starts_with("application/xml") + || ct_lower.starts_with("application/javascript") + || ct_lower.starts_with("application/x-javascript"); // RFC 6839 structured syntax suffixes (+json, +xml) let has_compressible_suffix = - ct_str.contains("+json") || ct_str.contains("+xml"); + ct_lower.contains("+json") || ct_lower.contains("+xml"); is_compressible || has_compressible_suffix } @@ -301,7 +318,8 @@ mod tests { } #[test] - fn test_accepts_gzip_encoding_gzip_acceptance_overrides_wildcard_rejection() { + fn test_accepts_gzip_encoding_gzip_acceptance_overrides_wildcard_rejection() + { // Explicit gzip acceptance should work even if wildcard is rejected let mut headers = HeaderMap::new(); headers.insert( @@ -311,6 +329,30 @@ mod tests { assert!(accepts_gzip_encoding(&headers)); } + #[test] + fn test_accepts_gzip_encoding_prefers_highest_quality() { + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=0, gzip;q=0.5"), + ); + assert!(accepts_gzip_encoding(&headers)); + + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=0.8, gzip;q=0"), + ); + assert!(accepts_gzip_encoding(&headers)); + + let mut headers = HeaderMap::new(); + headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip;q=0, *;q=1"), + ); + assert!(!accepts_gzip_encoding(&headers)); + } + #[test] fn test_accepts_gzip_encoding_case_insensitive() { let mut headers = HeaderMap::new(); @@ -581,7 +623,7 @@ mod tests { let mut response_headers = HeaderMap::new(); response_headers.insert( http::header::CONTENT_TYPE, - HeaderValue::from_static("text/event-stream"), + HeaderValue::from_static("TEXT/EVENT-STREAM"), ); let extensions = http::Extensions::new(); @@ -608,6 +650,7 @@ mod tests { // Test various compressible content types let compressible_types = vec![ "application/json", + "APPLICATION/JSON", "text/plain", "text/html", "text/css", @@ -615,8 +658,10 @@ mod tests { "application/javascript", "application/x-javascript", "application/problem+json", + "application/problem+JSON", "application/hal+json", "application/soap+xml", + "application/SOAP+XML", ]; for content_type in compressible_types { From 465fb212e0df23c5b438edec68e5578a2b2f7d30 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 12:21:33 -0500 Subject: [PATCH 11/22] couple more correctness things --- dropshot/src/compression.rs | 160 ++++++++++++++++++++++++++++++++---- 1 file changed, 144 insertions(+), 16 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index a6d920af0..70c3433b5 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -123,6 +123,10 @@ pub fn should_compress_response( return false; } + if response_headers.contains_key(http::header::CONTENT_RANGE) { + return false; + } + if !accepts_gzip_encoding(request_headers) { return false; } @@ -135,6 +139,18 @@ pub fn should_compress_response( return false; } + if let Some(content_length) = + response_headers.get(http::header::CONTENT_LENGTH) + { + if let Ok(length_str) = content_length.to_str() { + if let Ok(length) = length_str.parse::() { + if length < MIN_COMPRESS_SIZE { + return false; + } + } + } + } + // Only compress when we know the content type let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) else { @@ -206,35 +222,36 @@ pub fn apply_gzip_compression(response: Response) -> Response { // Vary header is critical for caching - prevents serving compressed // responses to clients that don't accept gzip - if let Some(existing_vary) = parts.headers.get(http::header::VARY) { - if let Ok(vary_str) = existing_vary.to_str() { - let has_accept_encoding = vary_str - .split(',') - .any(|v| v.trim().eq_ignore_ascii_case("accept-encoding")); - - if !has_accept_encoding { - let new_vary = format!("{}, Accept-Encoding", vary_str); - // Silently skip on malformed header to preserve existing behavior - if let Ok(new_vary_value) = HeaderValue::from_str(&new_vary) { - parts.headers.insert(http::header::VARY, new_vary_value); - } - } - } - } else { - parts.headers.insert( + let vary_has_accept_encoding = parts + .headers + .get_all(http::header::VARY) + .iter() + .any(header_value_contains_accept_encoding); + + if !vary_has_accept_encoding { + parts.headers.append( http::header::VARY, HeaderValue::from_static("Accept-Encoding"), ); } + parts.headers.remove(http::header::ACCEPT_RANGES); parts.headers.remove(http::header::CONTENT_LENGTH); Response::from_parts(parts, compressed_body) } +fn header_value_contains_accept_encoding(value: &HeaderValue) -> bool { + value.to_str().is_ok_and(|vary| { + vary.split(',') + .any(|v| v.trim().eq_ignore_ascii_case("accept-encoding")) + }) +} + #[cfg(test)] mod tests { use super::*; + use http::Extensions; #[test] fn test_accepts_gzip_encoding_basic() { @@ -246,6 +263,117 @@ mod tests { assert!(accepts_gzip_encoding(&headers)); } + #[test] + fn test_should_compress_response_rejects_content_range() { + let request_method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + + let response_status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + response_headers.insert( + http::header::CONTENT_RANGE, + HeaderValue::from_static("bytes 0-100/200"), + ); + + let response_extensions = Extensions::new(); + + assert!(!should_compress_response( + &request_method, + &request_headers, + response_status, + &response_headers, + &response_extensions, + )); + } + + #[test] + fn test_should_compress_response_respects_content_length_threshold() { + let request_method = http::Method::GET; + let mut request_headers = HeaderMap::new(); + request_headers.insert( + http::header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip"), + ); + + let response_status = http::StatusCode::OK; + let mut response_headers = HeaderMap::new(); + response_headers.insert( + http::header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + response_headers.insert( + http::header::CONTENT_LENGTH, + HeaderValue::from_str(&(MIN_COMPRESS_SIZE - 1).to_string()) + .unwrap(), + ); + + let response_extensions = Extensions::new(); + + assert!(!should_compress_response( + &request_method, + &request_headers, + response_status, + &response_headers, + &response_extensions, + )); + } + + #[test] + fn test_apply_gzip_compression_removes_accept_ranges_and_sets_vary() { + let body = "x".repeat((MIN_COMPRESS_SIZE + 10) as usize); + let response = Response::builder() + .header(http::header::CONTENT_TYPE, "application/json") + .header(http::header::ACCEPT_RANGES, "bytes") + .body(Body::from(body)) + .unwrap(); + + let compressed = apply_gzip_compression(response); + let headers = compressed.headers(); + + let gzip = HeaderValue::from_static("gzip"); + assert_eq!(headers.get(http::header::CONTENT_ENCODING), Some(&gzip)); + assert!(!headers.contains_key(http::header::ACCEPT_RANGES)); + + let vary_values: Vec<_> = headers + .get_all(http::header::VARY) + .iter() + .map(|value| value.to_str().unwrap().to_string()) + .collect(); + assert!(vary_values + .iter() + .any(|value| value.eq_ignore_ascii_case("accept-encoding"))); + } + + #[test] + fn test_apply_gzip_compression_avoids_duplicate_vary_entries() { + let body = "x".repeat((MIN_COMPRESS_SIZE + 10) as usize); + let response = Response::builder() + .header(http::header::CONTENT_TYPE, "application/json") + .header(http::header::VARY, "Accept-Encoding, Accept-Language") + .body(Body::from(body)) + .unwrap(); + + let compressed = apply_gzip_compression(response); + let mut accept_encoding_count = 0; + for value in compressed.headers().get_all(http::header::VARY).iter() { + let text = value.to_str().unwrap(); + accept_encoding_count += text + .split(',') + .filter(|v| v.trim().eq_ignore_ascii_case("accept-encoding")) + .count(); + } + + assert_eq!(accept_encoding_count, 1); + } + #[test] fn test_accepts_gzip_encoding_with_positive_quality() { let mut headers = HeaderMap::new(); From 7ce02408ee17849f1a11fd0685bf314c64b3d1c0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 2 Oct 2025 12:49:34 -0500 Subject: [PATCH 12/22] undo a couple of unnecessary changes --- dropshot/src/server.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 0f7ef4272..f982974bb 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -903,8 +903,6 @@ async fn http_request_handle( let request = request.map(crate::Body::wrap); let method = request.method().clone(); let uri = request.uri(); - let request_headers = request.headers().clone(); - let found_version = server.version_policy.request_version(&request, &request_log)?; let lookup_result = server.router.lookup_route( @@ -912,14 +910,14 @@ async fn http_request_handle( uri.path().into(), found_version.as_ref(), )?; - let request_info = RequestInfo::new(&request, remote_addr); let rqctx = RequestContext { server: Arc::clone(&server), - request: request_info, + request: RequestInfo::new(&request, remote_addr), endpoint: lookup_result.endpoint, request_id: request_id.to_string(), log: request_log.clone(), }; + let request_headers = rqctx.request.headers().clone(); let handler = lookup_result.handler; let mut response = match server.config.default_handler_task_mode { From b8c3aeb169f5db036950eea95181362ac4de0a27 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 21 Oct 2025 23:36:45 -0500 Subject: [PATCH 13/22] add config option for compression, default true --- dropshot/src/config.rs | 8 ++ dropshot/src/server.rs | 24 ++++-- dropshot/src/websocket.rs | 1 + dropshot/tests/integration-tests/config.rs | 2 + dropshot/tests/integration-tests/gzip.rs | 88 ++++++++++++++++++++++ dropshot/tests/integration-tests/tls.rs | 2 + 6 files changed, 117 insertions(+), 8 deletions(-) diff --git a/dropshot/src/config.rs b/dropshot/src/config.rs index b8a15db65..465db5ed8 100644 --- a/dropshot/src/config.rs +++ b/dropshot/src/config.rs @@ -63,6 +63,10 @@ pub struct ConfigDropshot { /// is made to deal with headers that appear multiple times in a single /// request. pub log_headers: Vec, + /// Whether to enable gzip compression for responses when response contents + /// allow it and clients ask for it through the Accept-Encoding header. + /// Defaults to true. + pub compression: bool, } /// Enum specifying options for how a Dropshot server should run its handler @@ -119,6 +123,7 @@ impl Default for ConfigDropshot { default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::Detached, log_headers: Default::default(), + compression: true, } } } @@ -137,6 +142,7 @@ struct DeserializedConfigDropshot { request_body_max_bytes: Option, default_handler_task_mode: HandlerTaskMode, log_headers: Vec, + compression: bool, } impl From for ConfigDropshot { @@ -146,6 +152,7 @@ impl From for ConfigDropshot { default_request_body_max_bytes: v.default_request_body_max_bytes, default_handler_task_mode: v.default_handler_task_mode, log_headers: v.log_headers, + compression: v.compression, } } } @@ -158,6 +165,7 @@ impl From for DeserializedConfigDropshot { request_body_max_bytes: None, default_handler_task_mode: v.default_handler_task_mode, log_headers: v.log_headers, + compression: v.compression, } } } diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index f982974bb..36e5534ef 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -3,6 +3,8 @@ use super::api_description::ApiDescription; use super::body::Body; +use super::compression::apply_gzip_compression; +use super::compression::should_compress_response; use super::config::{ConfigDropshot, ConfigTls}; #[cfg(feature = "usdt-probes")] use super::dtrace::probes; @@ -105,6 +107,9 @@ pub struct ServerConfig { /// is made to deal with headers that appear multiple times in a single /// request. pub log_headers: Vec, + /// Whether to enable gzip compression for responses when response contents + /// allow it and clients ask for it through the Accept-Encoding header. + pub compression: bool, } /// See [`ServerBuilder`] instead. @@ -188,6 +193,7 @@ impl HttpServerStarter { page_default_nitems: NonZeroU32::new(100).unwrap(), default_handler_task_mode: config.default_handler_task_mode, log_headers: config.log_headers.clone(), + compression: config.compression, }; let tls_acceptor = tls @@ -983,14 +989,16 @@ async fn http_request_handle( } }; - if crate::compression::should_compress_response( - &method, - &request_headers, - response.status(), - response.headers(), - response.extensions(), - ) { - response = crate::compression::apply_gzip_compression(response); + if server.config.compression + && should_compress_response( + &method, + &request_headers, + response.status(), + response.headers(), + response.extensions(), + ) + { + response = apply_gzip_compression(response); } response.headers_mut().insert( diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index a2fee96ec..5f773950a 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -385,6 +385,7 @@ mod tests { default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), + compression: true, }, router: HttpRouter::new(), log: log.clone(), diff --git a/dropshot/tests/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index 2456cfb9f..f14f7aaa3 100644 --- a/dropshot/tests/integration-tests/config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -66,6 +66,7 @@ fn test_valid_config_all_settings() { default_request_body_max_bytes: 1048576, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: vec!["X-Forwarded-For".to_string()], + compression: true, }, ); } @@ -181,6 +182,7 @@ fn make_config( default_request_body_max_bytes: 1024, default_handler_task_mode, log_headers: Default::default(), + compression: true, } } diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 671d041b8..3f03fdb61 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -672,3 +672,91 @@ async fn test_no_compression_for_304_not_modified() { // Note: HEAD request test is omitted from integration tests because Dropshot // requires explicit HEAD endpoint registration. The HEAD logic is tested via // unit tests in should_compress_response. + +#[tokio::test] +async fn test_compression_config_disabled() { + // Test that compression is disabled when config.compression = false (default) + let api = api(); + let config = + dropshot::ConfigDropshot { compression: false, ..Default::default() }; + let logctx = + crate::common::create_log_context("compression_config_disabled"); + let log = logctx.log.new(slog::o!()); + let testctx = dropshot::test_util::TestContext::new( + api, + 0_usize, + &config, + Some(logctx), + log, + ); + let client = &testctx.client_testctx; + + // Request WITH Accept-Encoding: gzip but compression disabled in config + let uri = client.url("/large-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should NOT be compressed due to config.compression = false + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + None, + "Response should not be compressed when config.compression = false" + ); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_compression_config_enabled() { + // Test that compression works when config.compression = true + let api = api(); + let config = + dropshot::ConfigDropshot { compression: true, ..Default::default() }; + let logctx = + crate::common::create_log_context("compression_config_enabled"); + let log = logctx.log.new(slog::o!()); + let testctx = dropshot::test_util::TestContext::new( + api, + 0_usize, + &config, + Some(logctx), + log, + ); + let client = &testctx.client_testctx; + + // Request WITH Accept-Encoding: gzip and compression enabled in config + let uri = client.url("/large-response"); + let request = Request::builder() + .method(Method::GET) + .uri(&uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed"); + + // Should be compressed since config.compression = true + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")), + "Response should be compressed when config.compression = true" + ); + + // Verify the response can be decompressed + let compressed_body = get_response_bytes(&mut response).await; + let _decompressed = decompress_gzip(&compressed_body); // Should not panic + + testctx.teardown().await; +} diff --git a/dropshot/tests/integration-tests/tls.rs b/dropshot/tests/integration-tests/tls.rs index 1b061a647..42c82bc48 100644 --- a/dropshot/tests/integration-tests/tls.rs +++ b/dropshot/tests/integration-tests/tls.rs @@ -116,6 +116,7 @@ fn make_server( default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), + compression: true, }; let config_tls = Some(ConfigTls::AsFile { cert_file: cert_file.to_path_buf(), @@ -430,6 +431,7 @@ async fn test_server_is_https() { default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), + compression: true, }; let config_tls = Some(ConfigTls::AsFile { cert_file: cert_file.path().to_path_buf(), From 498d0f3ba052de58a419f167ad7150b07374ef69 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 22 Oct 2025 18:27:23 -0500 Subject: [PATCH 14/22] shorten the integration tests --- dropshot/tests/integration-tests/gzip.rs | 404 +++++++---------------- 1 file changed, 127 insertions(+), 277 deletions(-) diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 3f03fdb61..b1f004b48 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -11,10 +11,82 @@ use http::{header, Method, StatusCode}; use hyper::{Request, Response}; use serde::{Deserialize, Serialize}; -use crate::common; +use crate::common::{create_log_context, test_setup}; extern crate slog; +// Helper functions for tests + +/// Creates a request builder with gzip Accept-Encoding header +fn make_gzip_request(uri: &http::Uri) -> Request { + Request::builder() + .method(Method::GET) + .uri(uri) + .header(header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request") +} + +/// Makes a request with gzip Accept-Encoding header and returns the response +async fn get_gzip_response( + client: &dropshot::test_util::ClientTestContext, + uri: &http::Uri, +) -> Response { + let request = make_gzip_request(uri); + client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed") +} + +/// Makes a request without Accept-Encoding header and returns the response +async fn make_plain_request_response( + client: &dropshot::test_util::ClientTestContext, + uri: &http::Uri, +) -> Response { + let request = Request::builder() + .method(Method::GET) + .uri(uri) + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("Request should succeed") +} + +/// Asserts that a response has gzip encoding +fn assert_gzip_encoded(response: &Response) { + assert_eq!( + response.headers().get(header::CONTENT_ENCODING), + Some(&header::HeaderValue::from_static("gzip")) + ); +} + +/// Verifies that compressed and uncompressed responses match when decompressed +async fn assert_compression_works( + uncompressed_response: &mut Response, + compressed_response: &mut Response, +) { + let uncompressed_body = get_response_bytes(uncompressed_response).await; + let compressed_body = get_response_bytes(compressed_response).await; + + // Compressed should be smaller + assert!( + compressed_body.len() < uncompressed_body.len(), + "Gzipped response ({} bytes) should be smaller than uncompressed response ({} bytes)", + compressed_body.len(), + uncompressed_body.len() + ); + + // Decompressed should match original + let decompressed_body = decompress_gzip(&compressed_body); + assert_eq!( + decompressed_body, uncompressed_body, + "Decompressed gzip response should match uncompressed response" + ); +} + // Test payload that's large enough to benefit from compression #[derive(Deserialize, Serialize, schemars::JsonSchema)] struct LargeTestData { @@ -216,78 +288,27 @@ fn decompress_gzip(compressed_data: &[u8]) -> Vec { #[tokio::test] async fn test_gzip_compression_with_accept_encoding() { - let api = api(); - let testctx = common::test_setup("gzip_compression_accept_encoding", api); + let testctx = test_setup("gzip_compression_accept_encoding", api()); let client = &testctx.client_testctx; - // Make request WITHOUT Accept-Encoding: gzip header let uri = client.url("/large-response"); - let request_no_gzip = Request::builder() - .method(Method::GET) - .uri(&uri) - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let mut response_no_gzip = client - .make_request_with_request(request_no_gzip, StatusCode::OK) - .await - .expect("Request without gzip should succeed"); - - // Make request WITH Accept-Encoding: gzip header - let request_with_gzip = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let mut response_with_gzip = client - .make_request_with_request(request_with_gzip, StatusCode::OK) - .await - .expect("Request with gzip should succeed"); - - // Get response bodies - let uncompressed_body = get_response_bytes(&mut response_no_gzip).await; - let compressed_body = get_response_bytes(&mut response_with_gzip).await; - // When gzip is implemented, the gzipped response should: - // 1. Have Content-Encoding: gzip header - assert_eq!( - response_with_gzip.headers().get(header::CONTENT_ENCODING), - Some(&header::HeaderValue::from_static("gzip")), - "Response with Accept-Encoding: gzip should have Content-Encoding: gzip header" - ); - - // 2. Be smaller than the uncompressed response - assert!( - compressed_body.len() < uncompressed_body.len(), - "Gzipped response ({} bytes) should be smaller than uncompressed response ({} bytes)", - compressed_body.len(), - uncompressed_body.len() - ); + // Make requests and get responses directly + let mut response_no_gzip = make_plain_request_response(client, &uri).await; + let mut response_with_gzip = get_gzip_response(client, &uri).await; - // 3. When decompressed, should match the original response - let decompressed_body = decompress_gzip(&compressed_body); - assert_eq!( - decompressed_body, uncompressed_body, - "Decompressed gzip response should match uncompressed response" - ); - - // The response without Accept-Encoding should NOT have Content-Encoding header - assert_eq!( - response_no_gzip.headers().get(header::CONTENT_ENCODING), - None, - "Response without Accept-Encoding: gzip should not have Content-Encoding header" - ); + // Verify compression works correctly + assert_gzip_encoded(&response_with_gzip); + assert_eq!(response_no_gzip.headers().get(header::CONTENT_ENCODING), None); + assert_compression_works(&mut response_no_gzip, &mut response_with_gzip) + .await; testctx.teardown().await; } #[tokio::test] async fn test_gzip_compression_accepts_multiple_encodings() { - let api = api(); - let testctx = - common::test_setup("gzip_compression_multiple_encodings", api); + let testctx = test_setup("gzip_compression_multiple_encodings", api()); let client = &testctx.client_testctx; // Test that gzip works when client accepts multiple encodings including gzip @@ -305,11 +326,7 @@ async fn test_gzip_compression_accepts_multiple_encodings() { .expect("Request with multiple accept encodings should succeed"); // Should still use gzip compression - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - Some(&header::HeaderValue::from_static("gzip")), - "Response should use gzip when it's one of multiple accepted encodings" - ); + assert_gzip_encoded(&response); // Verify the response can be decompressed let compressed_body = get_response_bytes(&mut response).await; @@ -320,22 +337,15 @@ async fn test_gzip_compression_accepts_multiple_encodings() { #[tokio::test] async fn test_no_gzip_without_accept_encoding() { - let api = api(); - let testctx = common::test_setup("no_gzip_without_accept", api); + let testctx = test_setup("no_gzip_without_accept", api()); let client = &testctx.client_testctx; + let uri = client.url("/large-response"); + // Request without any Accept-Encoding header should not get compressed response - let response = client - .make_request_no_body(Method::GET, "/large-response", StatusCode::OK) - .await - .expect("Request without accept encoding should succeed"); + let response = make_plain_request_response(client, &uri).await; - // Should not have Content-Encoding header - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Response without Accept-Encoding should not be compressed" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } @@ -343,13 +353,11 @@ async fn test_no_gzip_without_accept_encoding() { #[tokio::test] async fn test_no_compression_for_streaming_responses() { // Test that streaming responses are not compressed even when client accepts gzip - let api = crate::streaming::api(); - let testctx = common::test_setup("no_compression_streaming", api); + let testctx = + test_setup("no_compression_streaming", crate::streaming::api()); let client = &testctx.client_testctx; // Make request with Accept-Encoding: gzip header - // Note: We can't use make_request_no_body because it doesn't let us set custom headers - // So we'll use the RequestBuilder pattern used by the client internally let uri = client.url("/streaming"); let request = hyper::Request::builder() .method(http::Method::GET) @@ -364,19 +372,13 @@ async fn test_no_compression_for_streaming_responses() { .expect("Streaming request with gzip accept should succeed"); // Should have chunked transfer encoding - let transfer_encoding_header = response.headers().get("transfer-encoding"); assert_eq!( + response.headers().get("transfer-encoding"), Some(&http::HeaderValue::from_static("chunked")), - transfer_encoding_header, "Streaming response should have transfer-encoding: chunked" ); - // Should NOT have gzip content encoding even though client accepts it - assert_eq!( - response.headers().get(http::header::CONTENT_ENCODING), - None, - "Streaming response should not be compressed even with Accept-Encoding: gzip" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); // Consume the body to verify it works (and to allow teardown to proceed) let body_bytes = get_response_bytes(&mut response).await; @@ -387,36 +389,18 @@ async fn test_no_compression_for_streaming_responses() { #[tokio::test] async fn test_no_compression_for_non_compressible_content_types() { - let api = api(); - let testctx = common::test_setup("no_compression_non_compressible", api); + let testctx = test_setup("no_compression_non_compressible", api()); let client = &testctx.client_testctx; // Request an image with Accept-Encoding: gzip let uri = client.url("/image-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Image request should succeed"); + let response = get_gzip_response(client, &uri).await; - // Binary content (images) should NOT be compressed - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Binary content (image/png) should not be compressed even with Accept-Encoding: gzip" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); - // Verify content-type is correct assert_eq!( response.headers().get(header::CONTENT_TYPE), - Some(&header::HeaderValue::from_static("image/png")), - "Content-Type should be image/png" + Some(&header::HeaderValue::from_static("image/png")) ); testctx.teardown().await; @@ -424,68 +408,35 @@ async fn test_no_compression_for_non_compressible_content_types() { #[tokio::test] async fn test_compression_disabled_with_extension() { - let api = api(); - let testctx = common::test_setup("compression_disabled_extension", api); + let testctx = test_setup("compression_disabled_extension", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip, but response has NoCompression extension let uri = client.url("/disable-compression-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); + let response = get_gzip_response(client, &uri).await; - // Should NOT be compressed due to NoCompression extension - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Response with NoCompression extension should not be compressed" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } #[tokio::test] async fn test_no_compression_below_size_threshold() { - let api = api(); - let testctx = common::test_setup("no_compression_small_response", api); + let testctx = test_setup("no_compression_small_response", api()); let client = &testctx.client_testctx; // Request a tiny response (under 512 bytes) with Accept-Encoding: gzip let uri = client.url("/small-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); + let response = get_gzip_response(client, &uri).await; - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Small response request should succeed"); - - // Tiny responses (under 512 bytes) should NOT be compressed - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Responses under 512 bytes should not be compressed" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } #[tokio::test] async fn test_reject_gzip_with_quality_zero() { - let api = api(); - let testctx = common::test_setup("reject_gzip_quality_zero", api); + let testctx = test_setup("reject_gzip_quality_zero", api()); let client = &testctx.client_testctx; // Request with gzip explicitly rejected (q=0) @@ -502,35 +453,19 @@ async fn test_reject_gzip_with_quality_zero() { .await .expect("Request should succeed"); - // Should NOT be compressed since gzip has q=0 - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Response should not use gzip when client sets q=0 for gzip" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } #[tokio::test] async fn test_vary_header_is_set() { - let api = api(); - let testctx = common::test_setup("vary_header_set", api); + let testctx = test_setup("vary_header_set", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip let uri = client.url("/large-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); + let response = get_gzip_response(client, &uri).await; // Should have Vary: Accept-Encoding header assert!( @@ -551,120 +486,68 @@ async fn test_vary_header_is_set() { #[tokio::test] async fn test_json_suffix_is_compressed() { - let api = api(); - let testctx = common::test_setup("json_suffix_compressed", api); + let testctx = test_setup("json_suffix_compressed", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip for application/problem+json let uri = client.url("/json-suffix-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); + let response = get_gzip_response(client, &uri).await; // Should be compressed since application/problem+json has +json suffix - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - Some(&header::HeaderValue::from_static("gzip")), - "Response with +json suffix should be compressed" - ); + assert_gzip_encoded(&response); testctx.teardown().await; } #[tokio::test] async fn test_xml_suffix_is_compressed() { - let api = api(); - let testctx = common::test_setup("xml_suffix_compressed", api); + let testctx = test_setup("xml_suffix_compressed", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip for application/soap+xml let uri = client.url("/xml-suffix-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); + let response = get_gzip_response(client, &uri).await; // Should be compressed since application/soap+xml has +xml suffix - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - Some(&header::HeaderValue::from_static("gzip")), - "Response with +xml suffix should be compressed" - ); + assert_gzip_encoded(&response); testctx.teardown().await; } #[tokio::test] async fn test_no_compression_for_204_no_content() { - let api = api(); - let testctx = common::test_setup("no_compression_204", api); + let testctx = test_setup("no_compression_204", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip for 204 response let uri = client.url("/no-content-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); + let request = make_gzip_request(&uri); let response = client .make_request_with_request(request, StatusCode::NO_CONTENT) .await .expect("Request should succeed"); - // Should NOT be compressed (204 must not have body) - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "204 No Content should not have Content-Encoding header" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } #[tokio::test] async fn test_no_compression_for_304_not_modified() { - let api = api(); - let testctx = common::test_setup("no_compression_304", api); + let testctx = test_setup("no_compression_304", api()); let client = &testctx.client_testctx; // Request with Accept-Encoding: gzip for 304 response let uri = client.url("/not-modified-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); + let request = make_gzip_request(&uri); let response = client .make_request_with_request(request, StatusCode::NOT_MODIFIED) .await .expect("Request should succeed"); - // Should NOT be compressed (304 must not have body) - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "304 Not Modified should not have Content-Encoding header" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } @@ -676,14 +559,12 @@ async fn test_no_compression_for_304_not_modified() { #[tokio::test] async fn test_compression_config_disabled() { // Test that compression is disabled when config.compression = false (default) - let api = api(); let config = dropshot::ConfigDropshot { compression: false, ..Default::default() }; - let logctx = - crate::common::create_log_context("compression_config_disabled"); + let logctx = create_log_context("compression_config_disabled"); let log = logctx.log.new(slog::o!()); let testctx = dropshot::test_util::TestContext::new( - api, + api(), 0_usize, &config, Some(logctx), @@ -693,24 +574,9 @@ async fn test_compression_config_disabled() { // Request WITH Accept-Encoding: gzip but compression disabled in config let uri = client.url("/large-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); + let response = get_gzip_response(client, &uri).await; - let response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); - - // Should NOT be compressed due to config.compression = false - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - None, - "Response should not be compressed when config.compression = false" - ); + assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); testctx.teardown().await; } @@ -718,14 +584,12 @@ async fn test_compression_config_disabled() { #[tokio::test] async fn test_compression_config_enabled() { // Test that compression works when config.compression = true - let api = api(); let config = dropshot::ConfigDropshot { compression: true, ..Default::default() }; - let logctx = - crate::common::create_log_context("compression_config_enabled"); + let logctx = create_log_context("compression_config_enabled"); let log = logctx.log.new(slog::o!()); let testctx = dropshot::test_util::TestContext::new( - api, + api(), 0_usize, &config, Some(logctx), @@ -735,24 +599,10 @@ async fn test_compression_config_enabled() { // Request WITH Accept-Encoding: gzip and compression enabled in config let uri = client.url("/large-response"); - let request = Request::builder() - .method(Method::GET) - .uri(&uri) - .header(header::ACCEPT_ENCODING, "gzip") - .body(dropshot::Body::empty()) - .expect("Failed to construct request"); - - let mut response = client - .make_request_with_request(request, StatusCode::OK) - .await - .expect("Request should succeed"); + let mut response = get_gzip_response(client, &uri).await; // Should be compressed since config.compression = true - assert_eq!( - response.headers().get(header::CONTENT_ENCODING), - Some(&header::HeaderValue::from_static("gzip")), - "Response should be compressed when config.compression = true" - ); + assert_gzip_encoded(&response); // Verify the response can be decompressed let compressed_body = get_response_bytes(&mut response).await; From 90339d310df58bcec0fc2b4d53d8db17a2fc443a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 23 Oct 2025 10:53:54 -0500 Subject: [PATCH 15/22] shorten unit tests --- dropshot/src/compression.rs | 232 ++++++++++-------------------------- 1 file changed, 60 insertions(+), 172 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index 70c3433b5..7bc1de23b 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -251,15 +251,20 @@ fn header_value_contains_accept_encoding(value: &HeaderValue) -> bool { #[cfg(test)] mod tests { use super::*; + use http::header::{ + ACCEPT_ENCODING, ACCEPT_RANGES, CONTENT_ENCODING, CONTENT_LENGTH, + CONTENT_RANGE, CONTENT_TYPE, VARY, + }; use http::Extensions; + fn v(s: &'static str) -> HeaderValue { + HeaderValue::from_static(s) + } + #[test] fn test_accepts_gzip_encoding_basic() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + headers.insert(ACCEPT_ENCODING, v("gzip")); assert!(accepts_gzip_encoding(&headers)); } @@ -267,21 +272,12 @@ mod tests { fn test_should_compress_response_rejects_content_range() { let request_method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let response_status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); - response_headers.insert( - http::header::CONTENT_RANGE, - HeaderValue::from_static("bytes 0-100/200"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); + response_headers.insert(CONTENT_RANGE, v("bytes 0-100/200")); let response_extensions = Extensions::new(); @@ -298,19 +294,13 @@ mod tests { fn test_should_compress_response_respects_content_length_threshold() { let request_method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let response_status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); + response_headers.insert(CONTENT_TYPE, v("application/json")); response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); - response_headers.insert( - http::header::CONTENT_LENGTH, + CONTENT_LENGTH, HeaderValue::from_str(&(MIN_COMPRESS_SIZE - 1).to_string()) .unwrap(), ); @@ -330,20 +320,20 @@ mod tests { fn test_apply_gzip_compression_removes_accept_ranges_and_sets_vary() { let body = "x".repeat((MIN_COMPRESS_SIZE + 10) as usize); let response = Response::builder() - .header(http::header::CONTENT_TYPE, "application/json") - .header(http::header::ACCEPT_RANGES, "bytes") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT_RANGES, "bytes") .body(Body::from(body)) .unwrap(); let compressed = apply_gzip_compression(response); let headers = compressed.headers(); - let gzip = HeaderValue::from_static("gzip"); - assert_eq!(headers.get(http::header::CONTENT_ENCODING), Some(&gzip)); - assert!(!headers.contains_key(http::header::ACCEPT_RANGES)); + let gzip = v("gzip"); + assert_eq!(headers.get(CONTENT_ENCODING), Some(&gzip)); + assert!(!headers.contains_key(ACCEPT_RANGES)); let vary_values: Vec<_> = headers - .get_all(http::header::VARY) + .get_all(VARY) .iter() .map(|value| value.to_str().unwrap().to_string()) .collect(); @@ -356,14 +346,14 @@ mod tests { fn test_apply_gzip_compression_avoids_duplicate_vary_entries() { let body = "x".repeat((MIN_COMPRESS_SIZE + 10) as usize); let response = Response::builder() - .header(http::header::CONTENT_TYPE, "application/json") - .header(http::header::VARY, "Accept-Encoding, Accept-Language") + .header(CONTENT_TYPE, "application/json") + .header(VARY, "Accept-Encoding, Accept-Language") .body(Body::from(body)) .unwrap(); let compressed = apply_gzip_compression(response); let mut accept_encoding_count = 0; - for value in compressed.headers().get_all(http::header::VARY).iter() { + for value in compressed.headers().get_all(VARY).iter() { let text = value.to_str().unwrap(); accept_encoding_count += text .split(',') @@ -377,60 +367,42 @@ mod tests { #[test] fn test_accepts_gzip_encoding_with_positive_quality() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=0.8"), - ); + headers.insert(ACCEPT_ENCODING, HeaderValue::from_static("gzip;q=0.8")); assert!(accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_rejects_zero_quality() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=0"), - ); + headers.insert(ACCEPT_ENCODING, HeaderValue::from_static("gzip;q=0")); assert!(!accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_wildcard() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("*"), - ); + headers.insert(ACCEPT_ENCODING, v("*")); assert!(accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_wildcard_with_quality() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("*;q=0.5"), - ); + headers.insert(ACCEPT_ENCODING, v("*;q=0.5")); assert!(accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_wildcard_rejected() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("*;q=0"), - ); + headers.insert(ACCEPT_ENCODING, v("*;q=0")); assert!(!accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_multiple_encodings() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("deflate, gzip, br"), - ); + headers.insert(ACCEPT_ENCODING, v("deflate, gzip, br")); assert!(accepts_gzip_encoding(&headers)); } @@ -438,10 +410,7 @@ mod tests { fn test_accepts_gzip_encoding_gzip_takes_precedence_over_wildcard() { // Explicit gzip rejection should override wildcard acceptance let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("*;q=1.0, gzip;q=0"), - ); + headers.insert(ACCEPT_ENCODING, v("*;q=1.0, gzip;q=0")); assert!(!accepts_gzip_encoding(&headers)); } @@ -450,51 +419,33 @@ mod tests { { // Explicit gzip acceptance should work even if wildcard is rejected let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("*;q=0, gzip;q=1.0"), - ); + headers.insert(ACCEPT_ENCODING, v("*;q=0, gzip;q=1.0")); assert!(accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_prefers_highest_quality() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=0, gzip;q=0.5"), - ); + headers.insert(ACCEPT_ENCODING, v("gzip;q=0, gzip;q=0.5")); assert!(accepts_gzip_encoding(&headers)); let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=0.8, gzip;q=0"), - ); + headers.insert(ACCEPT_ENCODING, v("gzip;q=0.8, gzip;q=0")); assert!(accepts_gzip_encoding(&headers)); let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=0, *;q=1"), - ); + headers.insert(ACCEPT_ENCODING, v("gzip;q=0, *;q=1")); assert!(!accepts_gzip_encoding(&headers)); } #[test] fn test_accepts_gzip_encoding_case_insensitive() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("GZIP"), - ); + headers.insert(ACCEPT_ENCODING, v("GZIP")); assert!(accepts_gzip_encoding(&headers)); let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("GzIp"), - ); + headers.insert(ACCEPT_ENCODING, v("GzIp")); assert!(accepts_gzip_encoding(&headers)); } @@ -507,10 +458,7 @@ mod tests { #[test] fn test_accepts_gzip_encoding_with_spaces() { let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("deflate , gzip ; q=0.8 , br"), - ); + headers.insert(ACCEPT_ENCODING, v("deflate , gzip ; q=0.8 , br")); assert!(accepts_gzip_encoding(&headers)); } @@ -518,10 +466,7 @@ mod tests { fn test_accepts_gzip_encoding_malformed_quality() { // If quality parsing fails, should default to 1.0 let mut headers = HeaderMap::new(); - headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip;q=invalid"), - ); + headers.insert(ACCEPT_ENCODING, v("gzip;q=invalid")); assert!(accepts_gzip_encoding(&headers)); } @@ -529,16 +474,10 @@ mod tests { fn test_should_compress_response_basic() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); let extensions = http::Extensions::new(); assert!(should_compress_response( @@ -554,16 +493,10 @@ mod tests { fn test_should_compress_response_head_method() { let method = http::Method::HEAD; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); let extensions = http::Extensions::new(); assert!(!should_compress_response( @@ -579,10 +512,7 @@ mod tests { fn test_should_compress_response_no_content() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::NO_CONTENT; let response_headers = HeaderMap::new(); let extensions = http::Extensions::new(); @@ -600,10 +530,7 @@ mod tests { fn test_should_compress_response_not_modified() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::NOT_MODIFIED; let response_headers = HeaderMap::new(); let extensions = http::Extensions::new(); @@ -621,16 +548,10 @@ mod tests { fn test_should_compress_response_partial_content() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::PARTIAL_CONTENT; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); let extensions = http::Extensions::new(); assert!(!should_compress_response( @@ -648,10 +569,7 @@ mod tests { let request_headers = HeaderMap::new(); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); let extensions = http::Extensions::new(); assert!(!should_compress_response( @@ -667,20 +585,11 @@ mod tests { fn test_should_compress_response_already_encoded() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); - response_headers.insert( - http::header::CONTENT_ENCODING, - HeaderValue::from_static("br"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); + response_headers.insert(CONTENT_ENCODING, v("br")); let extensions = http::Extensions::new(); assert!(!should_compress_response( @@ -696,16 +605,10 @@ mod tests { fn test_should_compress_response_no_compression_extension() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); + response_headers.insert(CONTENT_TYPE, v("application/json")); let mut extensions = http::Extensions::new(); extensions.insert(NoCompression); @@ -722,10 +625,7 @@ mod tests { fn test_should_compress_response_no_content_type() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let response_headers = HeaderMap::new(); let extensions = http::Extensions::new(); @@ -743,16 +643,10 @@ mod tests { fn test_should_compress_response_sse() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let mut response_headers = HeaderMap::new(); - response_headers.insert( - http::header::CONTENT_TYPE, - HeaderValue::from_static("TEXT/EVENT-STREAM"), - ); + response_headers.insert(CONTENT_TYPE, v("TEXT/EVENT-STREAM")); let extensions = http::Extensions::new(); assert!(!should_compress_response( @@ -768,10 +662,7 @@ mod tests { fn test_should_compress_response_compressible_content_types() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let extensions = http::Extensions::new(); @@ -795,7 +686,7 @@ mod tests { for content_type in compressible_types { let mut response_headers = HeaderMap::new(); response_headers.insert( - http::header::CONTENT_TYPE, + CONTENT_TYPE, HeaderValue::from_str(content_type).unwrap(), ); @@ -817,10 +708,7 @@ mod tests { fn test_should_compress_response_non_compressible_content_types() { let method = http::Method::GET; let mut request_headers = HeaderMap::new(); - request_headers.insert( - http::header::ACCEPT_ENCODING, - HeaderValue::from_static("gzip"), - ); + request_headers.insert(ACCEPT_ENCODING, v("gzip")); let status = http::StatusCode::OK; let extensions = http::Extensions::new(); @@ -838,7 +726,7 @@ mod tests { for content_type in non_compressible_types { let mut response_headers = HeaderMap::new(); response_headers.insert( - http::header::CONTENT_TYPE, + CONTENT_TYPE, HeaderValue::from_str(content_type).unwrap(), ); From 0b14d4f4af6caac89b6ff76c7cfb27991ab21876 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 23 Oct 2025 12:11:08 -0500 Subject: [PATCH 16/22] properly test compression with streaming responses --- dropshot/tests/integration-tests/gzip.rs | 94 +++++++++++++++++-- dropshot/tests/integration-tests/streaming.rs | 2 +- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index b1f004b48..33d1b679f 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -2,12 +2,16 @@ //! Test cases for gzip response compression. +use bytes::Bytes; use dropshot::endpoint; use dropshot::ApiDescription; use dropshot::HttpError; use dropshot::HttpResponseOk; use dropshot::RequestContext; +use futures::stream; use http::{header, Method, StatusCode}; +use http_body_util::StreamBody; +use hyper::body::Frame; use hyper::{Request, Response}; use serde::{Deserialize, Serialize}; @@ -100,6 +104,51 @@ struct TinyData { x: u8, } +const STREAMING_TEXT_CHUNK: &str = "{\"message\":\"streaming chunk\"}\n"; +const STREAMING_TEXT_CHUNK_COUNT: usize = 32; + +fn streaming_payload() -> Vec { + STREAMING_TEXT_CHUNK.repeat(STREAMING_TEXT_CHUNK_COUNT).into_bytes() +} + +fn streaming_body_stream( +) -> impl futures::Stream, std::io::Error>> + Send { + stream::iter((0..STREAMING_TEXT_CHUNK_COUNT).map(|_| { + Result::, std::io::Error>::Ok(Frame::data( + Bytes::from_static(STREAMING_TEXT_CHUNK.as_bytes()), + )) + })) +} + +#[endpoint { + method = GET, + path = "/streaming-missing-content-type", +}] +async fn streaming_without_content_type( + _rqctx: RequestContext, +) -> Result, HttpError> { + let body = dropshot::Body::wrap(StreamBody::new(streaming_body_stream())); + Response::builder() + .status(StatusCode::OK) + .body(body) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + +#[endpoint { + method = GET, + path = "/streaming-with-content-type", +}] +async fn streaming_with_content_type( + _rqctx: RequestContext, +) -> Result, HttpError> { + let body = dropshot::Body::wrap(StreamBody::new(streaming_body_stream())); + Response::builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "text/plain") + .body(body) + .map_err(|e| HttpError::for_internal_error(e.to_string())) +} + fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_large_response).unwrap(); @@ -110,6 +159,8 @@ fn api() -> ApiDescription { api.register(api_xml_suffix_response).unwrap(); api.register(api_no_content_response).unwrap(); api.register(api_not_modified_response).unwrap(); + api.register(streaming_without_content_type).unwrap(); + api.register(streaming_with_content_type).unwrap(); api } @@ -351,14 +402,12 @@ async fn test_no_gzip_without_accept_encoding() { } #[tokio::test] -async fn test_no_compression_for_streaming_responses() { - // Test that streaming responses are not compressed even when client accepts gzip - let testctx = - test_setup("no_compression_streaming", crate::streaming::api()); +async fn test_streaming_without_content_type_skips_compression() { + let testctx = test_setup("streaming_missing_content_type", api()); let client = &testctx.client_testctx; // Make request with Accept-Encoding: gzip header - let uri = client.url("/streaming"); + let uri = client.url("/streaming-missing-content-type"); let request = hyper::Request::builder() .method(http::Method::GET) .uri(&uri) @@ -380,9 +429,40 @@ async fn test_no_compression_for_streaming_responses() { assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); - // Consume the body to verify it works (and to allow teardown to proceed) + // Consume stream and confirm body is the uncompressed payload let body_bytes = get_response_bytes(&mut response).await; - assert!(!body_bytes.is_empty(), "Streaming response should have content"); + assert_eq!(body_bytes, streaming_payload()); + + testctx.teardown().await; +} + +#[tokio::test] +async fn test_streaming_with_content_type_is_compressed() { + let testctx = test_setup("streaming_with_content_type_compressed", api()); + let client = &testctx.client_testctx; + + let uri = client.url("/streaming-with-content-type"); + let request = hyper::Request::builder() + .method(http::Method::GET) + .uri(&uri) + .header(http::header::ACCEPT_ENCODING, "gzip") + .body(dropshot::Body::empty()) + .expect("Failed to construct request"); + + let mut response = client + .make_request_with_request(request, http::StatusCode::OK) + .await + .expect("Streaming request with content type should succeed"); + + assert_gzip_encoded(&response); + assert_eq!( + response.headers().get(header::CONTENT_TYPE), + Some(&header::HeaderValue::from_static("text/plain")) + ); + + let compressed_body = get_response_bytes(&mut response).await; + let decompressed = decompress_gzip(&compressed_body); + assert_eq!(decompressed, streaming_payload(),); testctx.teardown().await; } diff --git a/dropshot/tests/integration-tests/streaming.rs b/dropshot/tests/integration-tests/streaming.rs index c56af265f..718fd8f2e 100644 --- a/dropshot/tests/integration-tests/streaming.rs +++ b/dropshot/tests/integration-tests/streaming.rs @@ -12,7 +12,7 @@ use crate::common; extern crate slog; -pub fn api() -> ApiDescription { +fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_streaming).unwrap(); api.register(api_not_streaming).unwrap(); From 03a1d0f525cfc4580cc86ccd49a1c15b4cd4dfdb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 28 Oct 2025 09:18:53 -0500 Subject: [PATCH 17/22] comment on why we're removing headers --- dropshot/src/compression.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index 7bc1de23b..4a0ecb323 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -235,6 +235,8 @@ pub fn apply_gzip_compression(response: Response) -> Response { ); } + // because we're streaming, we can't handle ranges and we don't know the + // length of the response ahead of time parts.headers.remove(http::header::ACCEPT_RANGES); parts.headers.remove(http::header::CONTENT_LENGTH); From 52c58425d1d4362000e8096430732fc68dbad4d8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 18 Nov 2025 15:51:47 -0600 Subject: [PATCH 18/22] don't need compat feature on tokio-util --- Cargo.lock | 1 - dropshot/Cargo.toml | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4dde370e8..5b37e5f05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2529,7 +2529,6 @@ checksum = "5427d89453009325de0d8f342c9490009f76e999cb7672d77e46267448f7e6b2" dependencies = [ "bytes", "futures-core", - "futures-io", "futures-sink", "pin-project-lite", "tokio", diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 6cea3cb74..958eb8e20 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -80,7 +80,7 @@ features = [ "full" ] [dependencies.tokio-util] version = "0.7" -features = [ "io", "compat" ] +features = [ "io" ] [dependencies.usdt] version = "0.6.0" From 76e00b24234d8509a62de29d760d01683e2dc659 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 18 Nov 2025 16:37:06 -0600 Subject: [PATCH 19/22] always add Vary: Accept-Encoding for compressible content-types --- dropshot/src/compression.rs | 97 ++++++++++++++---------- dropshot/src/server.rs | 24 +++++- dropshot/tests/integration-tests/gzip.rs | 40 ++++++++++ 3 files changed, 118 insertions(+), 43 deletions(-) diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs index 4a0ecb323..4db5fb7a0 100644 --- a/dropshot/src/compression.rs +++ b/dropshot/src/compression.rs @@ -97,6 +97,41 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap) -> bool { false } +/// Checks if a content type is compressible. +/// This is used to determine if the Vary: Accept-Encoding header should be added, +/// even if compression doesn't occur for this particular request. +pub fn is_compressible_content_type( + response_headers: &HeaderMap, +) -> bool { + // Only compress when we know the content type + let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) + else { + return false; + }; + let Ok(ct_str) = content_type.to_str() else { + return false; + }; + + let ct_lower = ct_str.to_ascii_lowercase(); + + // SSE streams prioritize latency over compression + if ct_lower.starts_with("text/event-stream") { + return false; + } + + let is_compressible = ct_lower.starts_with("application/json") + || ct_lower.starts_with("text/") + || ct_lower.starts_with("application/xml") + || ct_lower.starts_with("application/javascript") + || ct_lower.starts_with("application/x-javascript"); + + // RFC 6839 structured syntax suffixes (+json, +xml) + let has_compressible_suffix = + ct_lower.contains("+json") || ct_lower.contains("+xml"); + + is_compressible || has_compressible_suffix +} + /// Determines if a response should be compressed with gzip. pub fn should_compress_response( request_method: &http::Method, @@ -151,33 +186,9 @@ pub fn should_compress_response( } } - // Only compress when we know the content type - let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE) - else { - return false; - }; - let Ok(ct_str) = content_type.to_str() else { - return false; - }; - - let ct_lower = ct_str.to_ascii_lowercase(); - - // SSE streams prioritize latency over compression - if ct_lower.starts_with("text/event-stream") { - return false; - } - - let is_compressible = ct_lower.starts_with("application/json") - || ct_lower.starts_with("text/") - || ct_lower.starts_with("application/xml") - || ct_lower.starts_with("application/javascript") - || ct_lower.starts_with("application/x-javascript"); - - // RFC 6839 structured syntax suffixes (+json, +xml) - let has_compressible_suffix = - ct_lower.contains("+json") || ct_lower.contains("+xml"); - - is_compressible || has_compressible_suffix + // technically redundant with check outside of the call, but kept here + // because it's logically part of "should compress?" question + is_compressible_content_type(response_headers) } /// Minimum size in bytes for a response to be compressed. @@ -222,18 +233,7 @@ pub fn apply_gzip_compression(response: Response) -> Response { // Vary header is critical for caching - prevents serving compressed // responses to clients that don't accept gzip - let vary_has_accept_encoding = parts - .headers - .get_all(http::header::VARY) - .iter() - .any(header_value_contains_accept_encoding); - - if !vary_has_accept_encoding { - parts.headers.append( - http::header::VARY, - HeaderValue::from_static("Accept-Encoding"), - ); - } + add_vary_header(&mut parts.headers); // because we're streaming, we can't handle ranges and we don't know the // length of the response ahead of time @@ -250,6 +250,25 @@ fn header_value_contains_accept_encoding(value: &HeaderValue) -> bool { }) } +/// Adds the Vary: Accept-Encoding header to a response if not already present. +/// This is critical for correct caching behavior with intermediate caches. +pub fn add_vary_header(headers: &mut HeaderMap) { + let vary_values = headers.get_all(http::header::VARY); + + // can't and shouldn't add anything if we already have "*" + // https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.4 + if vary_values.iter().any(|v| v == "*") { + return; + } + + if !vary_values.iter().any(header_value_contains_accept_encoding) { + headers.append( + http::header::VARY, + HeaderValue::from_static("Accept-Encoding"), + ); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 36e5534ef..869a823d6 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -3,7 +3,9 @@ use super::api_description::ApiDescription; use super::body::Body; +use super::compression::add_vary_header; use super::compression::apply_gzip_compression; +use super::compression::is_compressible_content_type; use super::compression::should_compress_response; use super::config::{ConfigDropshot, ConfigTls}; #[cfg(feature = "usdt-probes")] @@ -990,15 +992,29 @@ async fn http_request_handle( }; if server.config.compression - && should_compress_response( + && is_compressible_content_type(response.headers()) + { + // Add Vary: Accept-Encoding header for all compressible content + // types. This needs to be there even if the response ends up not being + // compressed because it tells caches how to handle the possibility that + // responses could be compressed or not. + // + // This tells caches (like browsers and CDNs) that the response content + // depends on the value of the Accept-Encoding header. Without this, a + // cache might mistakenly serve a compressed response to a client that + // cannot decompress it, or serve an uncompressed response to a client + // that could have benefited from compression. + add_vary_header(response.headers_mut()); + + if should_compress_response( &method, &request_headers, response.status(), response.headers(), response.extensions(), - ) - { - response = apply_gzip_compression(response); + ) { + response = apply_gzip_compression(response); + } } response.headers_mut().insert( diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 33d1b679f..5449e28dd 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -690,3 +690,43 @@ async fn test_compression_config_enabled() { testctx.teardown().await; } + +#[tokio::test] +async fn test_vary_header_on_non_gzip_requests() { + // Test that Vary: Accept-Encoding is present even when client doesn't + // accept gzip. This is critical for correct caching behavior. + // + // Without this header, the following sequence causes incorrect behavior: + // 1. Client A (no gzip) requests resource → cache stores uncompressed + // response without Vary header + // 2. Client B (gzip support) requests same resource → cache serves the + // uncompressed version, even though Client B could handle compression + let testctx = test_setup("vary_header_no_gzip", api()); + let client = &testctx.client_testctx; + + // Request WITHOUT Accept-Encoding: gzip for a compressible resource + let uri = client.url("/large-response"); + let response = make_plain_request_response(client, &uri).await; + + // Should NOT be compressed + assert!( + response.headers().get(header::CONTENT_ENCODING).is_none(), + "Response should not be compressed" + ); + + // Should still have Vary: Accept-Encoding header + assert!( + response.headers().contains_key(header::VARY), + "Response should have Vary header even without gzip encoding" + ); + + let vary_value = + response.headers().get(header::VARY).unwrap().to_str().unwrap(); + assert!( + vary_value.to_lowercase().contains("accept-encoding"), + "Vary header should include Accept-Encoding, got: {}", + vary_value + ); + + testctx.teardown().await; +} From 07804838d49534e2d02df5ed7da190fa7bd097f7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Nov 2025 11:59:39 -0600 Subject: [PATCH 20/22] default compression: false --- dropshot/src/config.rs | 2 +- dropshot/tests/integration-tests/config.rs | 2 +- dropshot/tests/integration-tests/gzip.rs | 21 ++++++++++++++++++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/dropshot/src/config.rs b/dropshot/src/config.rs index 465db5ed8..fb4c8d730 100644 --- a/dropshot/src/config.rs +++ b/dropshot/src/config.rs @@ -123,7 +123,7 @@ impl Default for ConfigDropshot { default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::Detached, log_headers: Default::default(), - compression: true, + compression: false, } } } diff --git a/dropshot/tests/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index f14f7aaa3..31eb26197 100644 --- a/dropshot/tests/integration-tests/config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -66,7 +66,7 @@ fn test_valid_config_all_settings() { default_request_body_max_bytes: 1048576, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: vec!["X-Forwarded-For".to_string()], - compression: true, + compression: false, }, ); } diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 5449e28dd..8de73ddac 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -15,10 +15,29 @@ use hyper::body::Frame; use hyper::{Request, Response}; use serde::{Deserialize, Serialize}; -use crate::common::{create_log_context, test_setup}; +use crate::common::create_log_context; extern crate slog; +// Helper function for tests in this file +// Since we're testing gzip compression, most tests need compression enabled +fn test_setup( + test_name: &str, + api: ApiDescription, +) -> dropshot::test_util::TestContext { + let config = + dropshot::ConfigDropshot { compression: true, ..Default::default() }; + let logctx = create_log_context(test_name); + let log = logctx.log.new(slog::o!()); + dropshot::test_util::TestContext::new( + api, + 0_usize, + &config, + Some(logctx), + log, + ) +} + // Helper functions for tests /// Creates a request builder with gzip Accept-Encoding header From 9be53957097b03a506750813b7da9730ef02ed90 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 25 Nov 2025 18:43:58 -0500 Subject: [PATCH 21/22] consume large uncompressed response to avoid illumos hangs Gemini's theory: When a test case requests the /large-response endpoint but compression is disabled (either by config, lack of header, or rejection), the server attempts to write the full ~5KB JSON body to the socket. On illumos, the default TCP socket buffer size is likely smaller than on macOS/Linux, causing the server's write operation to block because the test client never reads the data to drain the buffer. When teardown() is called, the server hangs trying to finish the write during graceful shutdown, eventually timing out. The fix is to consume the response body in these tests. --- dropshot/tests/integration-tests/gzip.rs | 26 +++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 8de73ddac..6b5c4635b 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -413,10 +413,14 @@ async fn test_no_gzip_without_accept_encoding() { let uri = client.url("/large-response"); // Request without any Accept-Encoding header should not get compressed response - let response = make_plain_request_response(client, &uri).await; + let mut response = make_plain_request_response(client, &uri).await; assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); + // Consume the response body to avoid blocking teardown on platforms with + // small socket buffers (e.g., illumos). + get_response_bytes(&mut response).await; + testctx.teardown().await; } @@ -547,13 +551,16 @@ async fn test_reject_gzip_with_quality_zero() { .body(dropshot::Body::empty()) .expect("Failed to construct request"); - let response = client + let mut response = client .make_request_with_request(request, StatusCode::OK) .await .expect("Request should succeed"); assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); + // Consume response body (see test_no_gzip_without_accept_encoding). + get_response_bytes(&mut response).await; + testctx.teardown().await; } @@ -564,7 +571,7 @@ async fn test_vary_header_is_set() { // Request with Accept-Encoding: gzip let uri = client.url("/large-response"); - let response = get_gzip_response(client, &uri).await; + let mut response = get_gzip_response(client, &uri).await; // Should have Vary: Accept-Encoding header assert!( @@ -580,6 +587,9 @@ async fn test_vary_header_is_set() { vary_value ); + // Consume response body (see test_no_gzip_without_accept_encoding). + get_response_bytes(&mut response).await; + testctx.teardown().await; } @@ -673,10 +683,13 @@ async fn test_compression_config_disabled() { // Request WITH Accept-Encoding: gzip but compression disabled in config let uri = client.url("/large-response"); - let response = get_gzip_response(client, &uri).await; + let mut response = get_gzip_response(client, &uri).await; assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); + // Consume response body (see test_no_gzip_without_accept_encoding). + get_response_bytes(&mut response).await; + testctx.teardown().await; } @@ -725,7 +738,7 @@ async fn test_vary_header_on_non_gzip_requests() { // Request WITHOUT Accept-Encoding: gzip for a compressible resource let uri = client.url("/large-response"); - let response = make_plain_request_response(client, &uri).await; + let mut response = make_plain_request_response(client, &uri).await; // Should NOT be compressed assert!( @@ -747,5 +760,8 @@ async fn test_vary_header_on_non_gzip_requests() { vary_value ); + // Consume response body (see test_no_gzip_without_accept_encoding). + get_response_bytes(&mut response).await; + testctx.teardown().await; } From b35c144290bc4b0399874329cd73c6ac91e8ca2c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 26 Nov 2025 00:30:52 -0500 Subject: [PATCH 22/22] adam review: changelog, enum config, shorter comment --- CHANGELOG.adoc | 1 + dropshot/src/config.rs | 22 ++++++++++++++------ dropshot/src/lib.rs | 1 + dropshot/src/server.rs | 22 ++++++++------------ dropshot/src/websocket.rs | 4 ++-- dropshot/tests/integration-tests/config.rs | 8 ++++---- dropshot/tests/integration-tests/gzip.rs | 24 ++++++++++++++-------- dropshot/tests/integration-tests/tls.rs | 8 ++++---- 8 files changed, 52 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index a47acd0ad..a9c57fc39 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -16,6 +16,7 @@ https://github.com/oxidecomputer/dropshot/compare/v0.16.4\...HEAD[Full list of commits] * https://github.com/oxidecomputer/dropshot/pull/1475[#1475] Added `ClientSpecifiesVersionInHeader::on_missing` to provide a default version when the header is missing, intended for use when you're not in control of all clients +* https://github.com/oxidecomputer/dropshot/pull/1448[#1448] Added support for gzipping responses when clients request it through the `Accept-Encoding` header. Compression is off by default: existing consumers won't see a behavior change unless they opt in with `compression: CompressionConfig::Gzip`. == 0.16.4 (released 2025-09-04) diff --git a/dropshot/src/config.rs b/dropshot/src/config.rs index fb4c8d730..9a84a4c9c 100644 --- a/dropshot/src/config.rs +++ b/dropshot/src/config.rs @@ -6,6 +6,17 @@ use serde::Serialize; use std::net::SocketAddr; use std::path::PathBuf; +/// Configuration for response compression +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum CompressionConfig { + /// Compression is disabled + #[default] + None, + /// Gzip compression is enabled + Gzip, +} + /// Raw [`rustls::ServerConfig`] TLS configuration for use with /// [`ConfigTls::Dynamic`] pub type RawTlsConfig = rustls::ServerConfig; @@ -63,10 +74,9 @@ pub struct ConfigDropshot { /// is made to deal with headers that appear multiple times in a single /// request. pub log_headers: Vec, - /// Whether to enable gzip compression for responses when response contents - /// allow it and clients ask for it through the Accept-Encoding header. - /// Defaults to true. - pub compression: bool, + /// Whether to enable compression for responses (when response contents + /// allow it and clients ask for it through the Accept-Encoding header). + pub compression: CompressionConfig, } /// Enum specifying options for how a Dropshot server should run its handler @@ -123,7 +133,7 @@ impl Default for ConfigDropshot { default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::Detached, log_headers: Default::default(), - compression: false, + compression: CompressionConfig::default(), } } } @@ -142,7 +152,7 @@ struct DeserializedConfigDropshot { request_body_max_bytes: Option, default_handler_task_mode: HandlerTaskMode, log_headers: Vec, - compression: bool, + compression: CompressionConfig, } impl From for ConfigDropshot { diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 2efcb4bd3..909433c53 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -907,6 +907,7 @@ pub use api_description::TagDetails; pub use api_description::TagExternalDocs; pub use body::Body; pub use compression::NoCompression; +pub use config::CompressionConfig; pub use config::ConfigDropshot; pub use config::ConfigTls; pub use config::HandlerTaskMode; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 869a823d6..698e5db5e 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -7,7 +7,7 @@ use super::compression::add_vary_header; use super::compression::apply_gzip_compression; use super::compression::is_compressible_content_type; use super::compression::should_compress_response; -use super::config::{ConfigDropshot, ConfigTls}; +use super::config::{CompressionConfig, ConfigDropshot, ConfigTls}; #[cfg(feature = "usdt-probes")] use super::dtrace::probes; use super::handler::HandlerError; @@ -109,9 +109,8 @@ pub struct ServerConfig { /// is made to deal with headers that appear multiple times in a single /// request. pub log_headers: Vec, - /// Whether to enable gzip compression for responses when response contents - /// allow it and clients ask for it through the Accept-Encoding header. - pub compression: bool, + /// Configuration for response compression. + pub compression: CompressionConfig, } /// See [`ServerBuilder`] instead. @@ -991,19 +990,16 @@ async fn http_request_handle( } }; - if server.config.compression + if matches!(server.config.compression, CompressionConfig::Gzip) && is_compressible_content_type(response.headers()) { // Add Vary: Accept-Encoding header for all compressible content // types. This needs to be there even if the response ends up not being - // compressed because it tells caches how to handle the possibility that - // responses could be compressed or not. - // - // This tells caches (like browsers and CDNs) that the response content - // depends on the value of the Accept-Encoding header. Without this, a - // cache might mistakenly serve a compressed response to a client that - // cannot decompress it, or serve an uncompressed response to a client - // that could have benefited from compression. + // compressed because it tells caches (like browsers and CDNs) that the + // response content depends on the value of the Accept-Encoding header. + // Without this, a cache might mistakenly serve a compressed response to + // a client that cannot decompress it, or serve an uncompressed response + // to a client that could have benefited from compression. add_vary_header(response.headers_mut()); if should_compress_response( diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index 5f773950a..abdd9d79b 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -349,7 +349,7 @@ impl tokio::io::AsyncWrite for WebsocketConnectionRaw { #[cfg(test)] mod tests { use crate::body::Body; - use crate::config::HandlerTaskMode; + use crate::config::{CompressionConfig, HandlerTaskMode}; use crate::router::HttpRouter; use crate::server::{DropshotState, ServerConfig}; use crate::{ @@ -385,7 +385,7 @@ mod tests { default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), - compression: true, + compression: CompressionConfig::Gzip, }, router: HttpRouter::new(), log: log.clone(), diff --git a/dropshot/tests/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index 31eb26197..98dbdd9a4 100644 --- a/dropshot/tests/integration-tests/config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -5,8 +5,8 @@ use dropshot::test_util::read_config; use dropshot::Body; use dropshot::{ - ConfigDropshot, ConfigTls, HandlerTaskMode, HttpError, HttpResponseOk, - RequestContext, + CompressionConfig, ConfigDropshot, ConfigTls, HandlerTaskMode, HttpError, + HttpResponseOk, RequestContext, }; use dropshot::{HttpServer, ServerBuilder}; use slog::o; @@ -66,7 +66,7 @@ fn test_valid_config_all_settings() { default_request_body_max_bytes: 1048576, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: vec!["X-Forwarded-For".to_string()], - compression: false, + compression: CompressionConfig::default(), }, ); } @@ -182,7 +182,7 @@ fn make_config( default_request_body_max_bytes: 1024, default_handler_task_mode, log_headers: Default::default(), - compression: true, + compression: CompressionConfig::Gzip, } } diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index 6b5c4635b..d92174649 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -25,8 +25,10 @@ fn test_setup( test_name: &str, api: ApiDescription, ) -> dropshot::test_util::TestContext { - let config = - dropshot::ConfigDropshot { compression: true, ..Default::default() }; + let config = dropshot::ConfigDropshot { + compression: dropshot::CompressionConfig::Gzip, + ..Default::default() + }; let logctx = create_log_context(test_name); let log = logctx.log.new(slog::o!()); dropshot::test_util::TestContext::new( @@ -667,9 +669,11 @@ async fn test_no_compression_for_304_not_modified() { #[tokio::test] async fn test_compression_config_disabled() { - // Test that compression is disabled when config.compression = false (default) - let config = - dropshot::ConfigDropshot { compression: false, ..Default::default() }; + // Test that compression is disabled when compression = "none" (default) + let config = dropshot::ConfigDropshot { + compression: dropshot::CompressionConfig::None, + ..Default::default() + }; let logctx = create_log_context("compression_config_disabled"); let log = logctx.log.new(slog::o!()); let testctx = dropshot::test_util::TestContext::new( @@ -695,9 +699,11 @@ async fn test_compression_config_disabled() { #[tokio::test] async fn test_compression_config_enabled() { - // Test that compression works when config.compression = true - let config = - dropshot::ConfigDropshot { compression: true, ..Default::default() }; + // Test that compression works when compression = "gzip" + let config = dropshot::ConfigDropshot { + compression: dropshot::CompressionConfig::Gzip, + ..Default::default() + }; let logctx = create_log_context("compression_config_enabled"); let log = logctx.log.new(slog::o!()); let testctx = dropshot::test_util::TestContext::new( @@ -713,7 +719,7 @@ async fn test_compression_config_enabled() { let uri = client.url("/large-response"); let mut response = get_gzip_response(client, &uri).await; - // Should be compressed since config.compression = true + // Should be compressed since compression = "gzip" assert_gzip_encoded(&response); // Verify the response can be decompressed diff --git a/dropshot/tests/integration-tests/tls.rs b/dropshot/tests/integration-tests/tls.rs index 42c82bc48..fcb993a1f 100644 --- a/dropshot/tests/integration-tests/tls.rs +++ b/dropshot/tests/integration-tests/tls.rs @@ -4,8 +4,8 @@ //! mode, including certificate loading and supported modes. use dropshot::{ - ConfigDropshot, ConfigTls, HandlerTaskMode, HttpResponseOk, HttpServer, - ServerBuilder, + CompressionConfig, ConfigDropshot, ConfigTls, HandlerTaskMode, + HttpResponseOk, HttpServer, ServerBuilder, }; use slog::{o, Logger}; use std::convert::TryFrom; @@ -116,7 +116,7 @@ fn make_server( default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), - compression: true, + compression: CompressionConfig::Gzip, }; let config_tls = Some(ConfigTls::AsFile { cert_file: cert_file.to_path_buf(), @@ -431,7 +431,7 @@ async fn test_server_is_https() { default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), - compression: true, + compression: CompressionConfig::Gzip, }; let config_tls = Some(ConfigTls::AsFile { cert_file: cert_file.path().to_path_buf(),