refactor: Simplify range request code and consolidate tests

Reduce test boilerplate by consolidating 20 unit tests into 7 with
identical coverage. Remove redundant integration test already covered
by unit tests. Use HeaderValue::from_static, delegate to Display impl,
and pass ByteRange directly instead of an intermediate HeaderMap.

Author: lcian

objectstore-server/src/endpoints/objects.rs

@@ -100,17 +100,21 @@ async fn object_get(
 
     let stream = state.meter_stream(stream, &context);
     let metadata_headers = metadata.to_headers("").map_err(ServiceError::from)?;
+    let is_partial = !content_range.is_full();
 
-    let status = if content_range.is_full() {
-        StatusCode::OK
-    } else {
+    let status = if is_partial {
         StatusCode::PARTIAL_CONTENT
+    } else {
+        StatusCode::OK
     };
 
     let mut response = (status, metadata_headers, Body::from_stream(stream)).into_response();
     let resp_headers = response.headers_mut();
-    resp_headers.insert(http::header::ACCEPT_RANGES, "bytes".parse().unwrap());
-    if !content_range.is_full() {
+    resp_headers.insert(
+        http::header::ACCEPT_RANGES,
+        http::header::HeaderValue::from_static("bytes"),
+    );
+    if is_partial {
         resp_headers.insert(
             http::header::CONTENT_LENGTH,
             content_range.len().to_string().parse().unwrap(),

objectstore-server/tests/range_requests.rs

@@ -240,20 +240,3 @@ async fn full_range_returns_200() -> Result<()> {
     assert_eq!(body, "Hello, Range Requests!");
     Ok(())
 }
-
-#[tokio::test]
-async fn case_insensitive_range_unit() -> Result<()> {
-    let (server, key) = setup().await;
-    let client = reqwest::Client::new();
-
-    let resp = client
-        .get(server.url(&format!("/v1/objects/test/org=1/{key}")))
-        .header("range", "Bytes=0-4")
-        .send()
-        .await?;
-
-    assert_eq!(resp.status(), reqwest::StatusCode::PARTIAL_CONTENT);
-    let body = resp.text().await?;
-    assert_eq!(body, "Hello");
-    Ok(())
-}

objectstore-service/src/backend/s3_compatible.rs

@@ -164,13 +164,13 @@ where
         &self,
         method: Method,
         id: &ObjectId,
-        extra_headers: Option<reqwest::header::HeaderMap>,
+        range: Option<ByteRange>,
     ) -> Result<Option<(Metadata, reqwest::Response)>> {
         let object_url = self.object_url(id);
 
         let mut builder = self.request(method, &object_url).await?;
-        if let Some(headers) = extra_headers {
-            builder = builder.headers(headers);
+        if let Some(r) = range {
+            builder = builder.header(reqwest::header::RANGE, r.to_header_value());
         }
         let response = builder.send().await.map_err(|cause| Error::Reqwest {
             context: "S3: failed to send request".to_string(),
@@ -308,15 +308,7 @@ impl<T: TokenProvider> Backend for S3CompatibleBackend<T> {
     async fn get_object(&self, id: &ObjectId, range: Option<ByteRange>) -> Result<GetResponse> {
         objectstore_log::debug!("Reading from s3_compatible backend");
 
-        let extra_headers = range.map(|r| {
-            let mut headers = reqwest::header::HeaderMap::new();
-            headers.insert(reqwest::header::RANGE, r.to_header_value());
-            headers
-        });
-
-        let Some((metadata, response)) =
-            self.request_object(Method::GET, id, extra_headers).await?
-        else {
+        let Some((metadata, response)) = self.request_object(Method::GET, id, range).await? else {
             return Ok(None);
         };
 

objectstore-types/src/range.rs

@@ -26,8 +26,6 @@ impl ByteRange {
             ByteRange::From(s) => format!("bytes={s}-"),
             ByteRange::Last(n) => format!("bytes=-{n}"),
         };
-        // SAFETY: the format only contains ASCII digits, hyphens, and the
-        // literal prefix "bytes=", which are all valid header value bytes.
         HeaderValue::from_str(&s).expect("ByteRange always produces a valid header value")
     }
 
@@ -168,10 +166,8 @@ impl ContentRange {
     /// The returned value is always valid ASCII and can be inserted directly
     /// into an HTTP header map.
     pub fn to_header_value(&self) -> HeaderValue {
-        let s = format!("bytes {}-{}/{}", self.start, self.end, self.total);
-        // SAFETY: the format only contains ASCII digits, spaces, hyphens,
-        // and slashes, which are all valid header value bytes.
-        HeaderValue::from_str(&s).expect("ContentRange always produces a valid header value")
+        HeaderValue::from_str(&self.to_string())
+            .expect("ContentRange always produces a valid header value")
     }
 }
 
@@ -212,33 +208,14 @@ mod tests {
     use super::*;
 
     #[test]
-    fn parse_from_to() {
+    fn parse_valid_ranges() {
         assert_eq!(
             ByteRange::try_from("bytes=0-499"),
             Ok(ByteRange::Inclusive(0, 499))
         );
-    }
-
-    #[test]
-    fn parse_from() {
         assert_eq!(ByteRange::try_from("bytes=500-"), Ok(ByteRange::From(500)));
-    }
-
-    #[test]
-    fn parse_suffix() {
         assert_eq!(ByteRange::try_from("bytes=-100"), Ok(ByteRange::Last(100)));
-    }
-
-    #[test]
-    fn parse_rejects_multi_range() {
-        assert_eq!(
-            ByteRange::try_from("bytes=0-10, 20-30"),
-            Err(RangeError::MultiRangeNotSupported)
-        );
-    }
-
-    #[test]
-    fn parse_case_insensitive() {
+        // Case insensitive
         assert_eq!(
             ByteRange::try_from("Bytes=0-499"),
             Ok(ByteRange::Inclusive(0, 499))
@@ -247,86 +224,33 @@ mod tests {
     }
 
     #[test]
-    fn parse_returns_unknown_unit_for_non_bytes() {
-        assert_eq!(ByteRange::try_from("items=0-10"), Err(RangeError::UnknownUnit));
-    }
-
-    #[test]
-    fn parse_rejects_inverted_range() {
+    fn parse_invalid_ranges() {
         assert_eq!(
-            ByteRange::try_from("bytes=500-100"),
-            Err(RangeError::InvalidRange)
+            ByteRange::try_from("bytes=0-10, 20-30"),
+            Err(RangeError::MultiRangeNotSupported)
         );
-    }
-
-    #[test]
-    fn parse_rejects_zero_suffix() {
-        assert_eq!(ByteRange::try_from("bytes=-0"), Err(RangeError::InvalidRange));
-    }
-
-    #[test]
-    fn resolve_from_to() {
-        let range = ByteRange::Inclusive(0, 499).resolve(1000);
         assert_eq!(
-            range,
-            Some(ContentRange {
-                start: 0,
-                end: 499,
-                total: 1000
-            })
+            ByteRange::try_from("items=0-10"),
+            Err(RangeError::UnknownUnit)
         );
-    }
-
-    #[test]
-    fn resolve_from_to_clamped() {
-        let range = ByteRange::Inclusive(0, 9999).resolve(500);
         assert_eq!(
-            range,
-            Some(ContentRange {
-                start: 0,
-                end: 499,
-                total: 500
-            })
-        );
-    }
-
-    #[test]
-    fn resolve_from() {
-        let range = ByteRange::From(500).resolve(1000);
-        assert_eq!(
-            range,
-            Some(ContentRange {
-                start: 500,
-                end: 999,
-                total: 1000
-            })
+            ByteRange::try_from("bytes=500-100"),
+            Err(RangeError::InvalidRange)
         );
-    }
-
-    #[test]
-    fn resolve_suffix() {
-        let range = ByteRange::Last(100).resolve(1000);
         assert_eq!(
-            range,
-            Some(ContentRange {
-                start: 900,
-                end: 999,
-                total: 1000
-            })
+            ByteRange::try_from("bytes=-0"),
+            Err(RangeError::InvalidRange)
         );
     }
 
     #[test]
-    fn resolve_suffix_larger_than_total() {
-        let range = ByteRange::Last(2000).resolve(1000);
-        assert_eq!(
-            range,
-            Some(ContentRange {
-                start: 0,
-                end: 999,
-                total: 1000
-            })
-        );
+    fn resolve_satisfiable() {
+        let cr = |start, end, total| Some(ContentRange { start, end, total });
+        assert_eq!(ByteRange::Inclusive(0, 499).resolve(1000), cr(0, 499, 1000));
+        assert_eq!(ByteRange::Inclusive(0, 9999).resolve(500), cr(0, 499, 500));
+        assert_eq!(ByteRange::From(500).resolve(1000), cr(500, 999, 1000));
+        assert_eq!(ByteRange::Last(100).resolve(1000), cr(900, 999, 1000));
+        assert_eq!(ByteRange::Last(2000).resolve(1000), cr(0, 999, 1000));
     }
 
     #[test]
@@ -337,60 +261,54 @@ mod tests {
     }
 
     #[test]
-    fn content_range_full() {
-        let cr = ContentRange::full(1000);
-        assert_eq!(cr.start, 0);
-        assert_eq!(cr.end, 999);
-        assert_eq!(cr.total, 1000);
-        assert_eq!(cr.len(), 1000);
-        assert!(cr.is_full());
-        assert_eq!(cr.to_header_value(), "bytes 0-999/1000");
-    }
+    fn content_range_properties() {
+        let full = ContentRange::full(1000);
+        assert_eq!(
+            full,
+            ContentRange {
+                start: 0,
+                end: 999,
+                total: 1000
+            }
+        );
+        assert_eq!(full.len(), 1000);
+        assert!(full.is_full());
 
-    #[test]
-    fn content_range_partial_is_not_full() {
-        let cr = ContentRange {
+        let partial = ContentRange {
             start: 0,
             end: 499,
             total: 1000,
         };
-        assert!(!cr.is_full());
-        assert_eq!(cr.len(), 500);
-    }
+        assert_eq!(partial.len(), 500);
+        assert!(!partial.is_full());
 
-    #[test]
-    fn content_range_full_zero_bytes() {
-        let cr = ContentRange::full(0);
-        assert_eq!(cr.len(), 0);
-        assert!(cr.is_full());
+        let zero = ContentRange::full(0);
+        assert_eq!(zero.len(), 0);
+        assert!(zero.is_full());
     }
 
     #[test]
-    fn byte_range_to_header_value() {
+    fn header_value_roundtrips() {
         assert_eq!(
             ByteRange::Inclusive(0, 499).to_header_value(),
             "bytes=0-499"
         );
         assert_eq!(ByteRange::From(500).to_header_value(), "bytes=500-");
         assert_eq!(ByteRange::Last(100).to_header_value(), "bytes=-100");
-    }
 
-    #[test]
-    fn content_range_parse() {
-        assert_eq!(
-            ContentRange::parse("bytes 0-499/1234"),
-            Some(ContentRange {
-                start: 0,
-                end: 499,
-                total: 1234
-            })
-        );
+        let cr = ContentRange {
+            start: 0,
+            end: 499,
+            total: 1234,
+        };
+        assert_eq!(cr.to_header_value(), "bytes 0-499/1234");
+        assert_eq!(ContentRange::parse("bytes 0-499/1234"), Some(cr));
         assert_eq!(ContentRange::parse("bytes */1234"), None);
         assert_eq!(ContentRange::parse("invalid"), None);
     }
 
     #[test]
-    fn content_range_parse_unsatisfiable_total() {
+    fn parse_unsatisfiable_total() {
         assert_eq!(
             ContentRange::parse_unsatisfiable_total("bytes */1234"),
             Some(1234)