diff --git a/clients/python/src/objectstore_client/metadata.py b/clients/python/src/objectstore_client/metadata.py index 0867f7b1..ce4f50b3 100644 --- a/clients/python/src/objectstore_client/metadata.py +++ b/clients/python/src/objectstore_client/metadata.py @@ -14,6 +14,7 @@ HEADER_TIME_EXPIRES = "x-sn-time-expires" HEADER_ORIGIN = "x-sn-origin" HEADER_FILENAME = "x-sn-filename" +HEADER_SIZE = "x-sn-size" HEADER_META_PREFIX = "x-snme-" @@ -70,6 +71,17 @@ class Metadata: prompting browsers and download tools to save the file under this name. """ + size: int | None + """ + The size of the complete stored object in bytes. + + This is always the size of the whole object, even when only a range of it was + requested. For a compressed object it is the compressed size, matching the bytes on + the wire. + + This field is computed by the server, it cannot be set by clients. + """ + custom: dict[str, str] @classmethod @@ -81,6 +93,7 @@ def from_headers(cls, headers: Mapping[str, str]) -> Metadata: time_expires = None origin = None filename = None + size = None custom_metadata = {} for k, v in headers.items(): @@ -98,6 +111,8 @@ def from_headers(cls, headers: Mapping[str, str]) -> Metadata: origin = v elif k == HEADER_FILENAME: filename = v + elif k == HEADER_SIZE: + size = int(v) elif k.startswith(HEADER_META_PREFIX): custom_metadata[k[len(HEADER_META_PREFIX) :]] = v @@ -109,6 +124,7 @@ def from_headers(cls, headers: Mapping[str, str]) -> Metadata: time_expires=time_expires, origin=origin, filename=filename, + size=size, custom=custom_metadata, ) diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 42b28890..49b13309 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -185,12 +185,15 @@ def test_head(server_url: str) -> None: session = client.session(test_usecase, org=42, project=1337) - object_key = session.put(b"test data", origin="203.0.113.42") + payload = b"test data" + object_key = session.put(payload, origin="203.0.113.42", compression="none") metadata = session.head(object_key) assert metadata is not None assert metadata.time_created is not None assert metadata.origin == "203.0.113.42" + # `size` reports the stored bytes, which match the payload only without compression. + assert metadata.size == len(payload) session.delete(object_key) @@ -741,7 +744,7 @@ def test_presigned_head_succeeds(server_url: str) -> None: ) status, _ = _fetch(url, method="HEAD") - assert status == 204 + assert status == 200 def test_presigned_case_insensitive_method(server_url: str) -> None: diff --git a/objectstore-server/src/endpoints/batch.rs b/objectstore-server/src/endpoints/batch.rs index 8cbee6d2..b7aa57c5 100644 --- a/objectstore-server/src/endpoints/batch.rs +++ b/objectstore-server/src/endpoints/batch.rs @@ -191,7 +191,7 @@ async fn convert_to_part( idx, &key, "head", - StatusCode::NO_CONTENT, + StatusCode::OK, None, Bytes::new(), Some(headers), diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index a08b6620..fa93d462 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -88,25 +88,27 @@ async fn object_get( }; let stream = state.meter_stream(stream, &context); - let metadata_headers = metadata.to_headers("").map_err(ServiceError::from)?; + let mut metadata_headers = metadata.to_headers("").map_err(ServiceError::from)?; let mut response = match content_range { Some(ref content_range) => { - let mut resp = ( + metadata_headers.insert( + http::header::CONTENT_LENGTH, + content_range.len_to_header_value(), + ); + metadata_headers.insert(http::header::CONTENT_RANGE, content_range.to_header_value()); + + ( StatusCode::PARTIAL_CONTENT, metadata_headers, Body::from_stream(stream), ) - .into_response(); - let headers = resp.headers_mut(); - headers.insert( - http::header::CONTENT_LENGTH, - content_range.len_to_header_value(), - ); - headers.insert(http::header::CONTENT_RANGE, content_range.to_header_value()); - resp + .into_response() + } + None => { + insert_content_length(&mut metadata_headers, &metadata); + (StatusCode::OK, metadata_headers, Body::from_stream(stream)).into_response() } - None => (StatusCode::OK, metadata_headers, Body::from_stream(stream)).into_response(), }; insert_content_disposition(&mut response, &metadata); @@ -120,14 +122,29 @@ async fn object_head(service: AuthAwareService, Xt(id): Xt) -> ApiResu return Ok(StatusCode::NOT_FOUND.into_response()); }; - let headers = metadata.to_headers("").map_err(ServiceError::from)?; + let mut headers = metadata.to_headers("").map_err(ServiceError::from)?; + insert_content_length(&mut headers, &metadata); - let mut response = (StatusCode::NO_CONTENT, headers).into_response(); + let mut response = (StatusCode::OK, headers).into_response(); insert_content_disposition(&mut response, &metadata); insert_accept_ranges(&mut response); Ok(response) } +/// Inserts a `Content-Length` header covering the complete object. +/// +/// Only valid for responses whose body is the whole object, and for `HEAD` responses, which +/// describe what a `GET` would have returned. Ranged responses announce the length of the range +/// instead and must not use this. +fn insert_content_length(headers: &mut HeaderMap, metadata: &Metadata) { + if let Some(size) = metadata.size { + headers.insert( + http::header::CONTENT_LENGTH, + http::HeaderValue::from(size as u64), + ); + } +} + fn insert_content_disposition(response: &mut Response, metadata: &Metadata) { if let Some(val) = metadata .filename diff --git a/objectstore-server/tests/objects.rs b/objectstore-server/tests/objects.rs index d7f0b00d..212f18eb 100644 --- a/objectstore-server/tests/objects.rs +++ b/objectstore-server/tests/objects.rs @@ -43,7 +43,7 @@ async fn filename_produces_content_disposition() -> Result<()> { .head(server.url("/v1/objects/test/org=1/cd-key")) .send() .await?; - assert_eq!(resp.status(), reqwest::StatusCode::NO_CONTENT); + assert_eq!(resp.status(), reqwest::StatusCode::OK); assert_eq!(resp.headers().get("x-sn-filename").unwrap(), "report.pdf"); assert_eq!( resp.headers().get("content-disposition").unwrap(), diff --git a/objectstore-server/tests/presigned.rs b/objectstore-server/tests/presigned.rs index 743b8971..81e920f6 100644 --- a/objectstore-server/tests/presigned.rs +++ b/objectstore-server/tests/presigned.rs @@ -144,7 +144,7 @@ async fn presigned_head_succeeds() -> Result<()> { .send() .await?; - assert_eq!(resp.status(), reqwest::StatusCode::NO_CONTENT); + assert_eq!(resp.status(), reqwest::StatusCode::OK); Ok(()) } diff --git a/objectstore-server/tests/range_requests.rs b/objectstore-server/tests/range_requests.rs index ca5afd4d..b2d30a3d 100644 --- a/objectstore-server/tests/range_requests.rs +++ b/objectstore-server/tests/range_requests.rs @@ -66,7 +66,7 @@ async fn head_returns_accept_ranges() -> Result<()> { .send() .await?; - assert_eq!(resp.status(), reqwest::StatusCode::NO_CONTENT); + assert_eq!(resp.status(), reqwest::StatusCode::OK); assert_eq!( resp.headers().get("accept-ranges").unwrap().to_str()?, "bytes" @@ -74,6 +74,72 @@ async fn head_returns_accept_ranges() -> Result<()> { Ok(()) } +/// `HEAD` announces the object size, so clients can learn it without a download. +#[tokio::test] +async fn head_reports_size() -> Result<()> { + let (server, key) = setup().await; + let client = reqwest::Client::new(); + + let resp = client + .head(server.url(&format!("/v1/objects/test/org=1/{key}"))) + .send() + .await?; + + assert_eq!(resp.status(), reqwest::StatusCode::OK); + assert_eq!( + resp.headers().get("content-length").unwrap().to_str()?, + "22" + ); + assert_eq!(resp.headers().get("x-sn-size").unwrap().to_str()?, "22"); + Ok(()) +} + +/// A full `GET` announces the same length as `HEAD`, rather than falling back to chunked. +#[tokio::test] +async fn full_get_reports_size() -> Result<()> { + let (server, key) = setup().await; + let client = reqwest::Client::new(); + + let resp = client + .get(server.url(&format!("/v1/objects/test/org=1/{key}"))) + .send() + .await?; + + assert_eq!(resp.status(), reqwest::StatusCode::OK); + assert_eq!( + resp.headers().get("content-length").unwrap().to_str()?, + "22" + ); + assert_eq!(resp.headers().get("x-sn-size").unwrap().to_str()?, "22"); + + let body = resp.text().await?; + assert_eq!(body, "Hello, Range Requests!"); + Ok(()) +} + +/// On a ranged response `Content-Length` describes the range, while `x-sn-size` stays the size of +/// the complete object. +#[tokio::test] +async fn range_content_length_covers_only_the_range() -> 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); + assert_eq!(resp.headers().get("content-length").unwrap().to_str()?, "5"); + assert_eq!( + resp.headers().get("content-range").unwrap().to_str()?, + "bytes 0-4/22" + ); + assert_eq!(resp.headers().get("x-sn-size").unwrap().to_str()?, "22"); + Ok(()) +} + #[tokio::test] async fn range_prefix_returns_206() -> Result<()> { let (server, key) = setup().await; diff --git a/objectstore-service/src/backend/s3_compatible.rs b/objectstore-service/src/backend/s3_compatible.rs index bc0efafd..8644a0c7 100644 --- a/objectstore-service/src/backend/s3_compatible.rs +++ b/objectstore-service/src/backend/s3_compatible.rs @@ -4,9 +4,9 @@ use std::time::SystemTime; use std::{fmt, io}; use futures_util::{StreamExt, TryStreamExt}; -use objectstore_types::metadata::Metadata; +use objectstore_types::metadata::{HEADER_SIZE, Metadata}; use objectstore_types::range::{ByteRange, ContentRange}; -use reqwest::header::HeaderMap; +use reqwest::header::{HeaderMap, HeaderName}; use reqwest::{Body, IntoUrl, Method, RequestBuilder, Response, StatusCode}; use super::extensions::{ResponseExt, SendTraced}; @@ -125,6 +125,13 @@ fn metadata_to_gcs_headers( prefix: &str, ) -> Result { let mut headers = metadata.to_headers(prefix)?; + + // The size is derived from the native `Content-Length` on every read, so it must not be + // persisted: metadata updates rewrite *all* stored metadata, and a stored `x-sn-size` key + // is rejected by the GCS JSON backend when it deserializes the object. + let size = HeaderName::try_from(format!("{prefix}{HEADER_SIZE}"))?; + headers.remove(&size); + // GCS custom-time for lifecycle expiration if let Some(expires_at) = metadata.time_expires { let expires_at = humantime::format_rfc3339_seconds(expires_at); @@ -217,8 +224,20 @@ where metadata.size = Some(range.total as usize); Some(range) } else { - if let Some(len) = response.content_length() { - metadata.size = Some(len as usize); + // NB: Read the header rather than `Response::content_length`, which reports the + // length of the decoded body and is therefore always zero for a HEAD response. + let size = headers + .get(reqwest::header::CONTENT_LENGTH) + .and_then(|value| value.to_str().ok()) + .map(|value| value.parse::()) + .transpose() + .map_err(|cause| Error::Generic { + context: "S3: failed to parse Content-Length from object response".to_string(), + cause: Some(Box::new(cause)), + })?; + + if let Some(size) = size { + metadata.size = Some(size); } else { objectstore_log::warn!("S3: 200 response missing Content-Length header"); } @@ -403,6 +422,19 @@ mod tests { }) } + #[test] + fn metadata_to_gcs_headers_omits_size() { + let metadata = Metadata { + size: Some(4096), + ..Default::default() + }; + + let headers = metadata_to_gcs_headers(&metadata, GCS_CUSTOM_PREFIX).unwrap(); + + // Persisting the size would store a key that the GCS JSON backend rejects on read. + assert!(headers.get("x-goog-meta-x-sn-size").is_none()); + } + #[test] fn metadata_to_gcs_headers_uses_time_expires() { let expires = SystemTime::now() + Duration::from_hours(1); @@ -429,6 +461,25 @@ mod tests { Ok(()) } + #[tokio::test] + #[ignore = "MinIO does not support streaming bodies (requires Content-Length)"] + async fn test_get_metadata_reports_size() -> Result<()> { + let backend = create_test_backend(); + let id = make_id(); + let payload = "hello, world"; + + backend + .put_object(&id, &Metadata::default(), stream::single(payload)) + .await?; + + // The size must come from the `Content-Length` header, not from the (empty) body of + // the HEAD response. + let metadata = backend.get_metadata(&id).await?.expect("object exists"); + assert_eq!(metadata.size, Some(payload.len())); + + Ok(()) + } + #[tokio::test] #[ignore = "MinIO does not support streaming bodies (requires Content-Length)"] async fn test_ttl_immediate() -> Result<()> { diff --git a/objectstore-types/src/metadata.rs b/objectstore-types/src/metadata.rs index 290b5130..b25436ae 100644 --- a/objectstore-types/src/metadata.rs +++ b/objectstore-types/src/metadata.rs @@ -33,6 +33,7 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::fmt; +use std::num::ParseIntError; use std::str::FromStr; use std::time::{Duration, SystemTime}; @@ -50,6 +51,8 @@ pub const HEADER_TIME_EXPIRES: &str = "x-sn-time-expires"; pub const HEADER_ORIGIN: &str = "x-sn-origin"; /// The custom HTTP header that contains the filename of the object. pub const HEADER_FILENAME: &str = "x-sn-filename"; +/// The custom HTTP header that contains the size of the stored object in bytes. +pub const HEADER_SIZE: &str = "x-sn-size"; /// The prefix for custom HTTP headers containing custom per-object metadata. pub const HEADER_META_PREFIX: &str = "x-snme-"; @@ -82,6 +85,9 @@ pub enum Error { /// The creation time is invalid. #[error("invalid creation time")] CreationTime(#[from] humantime::TimestampError), + /// The object size is not a valid byte count. + #[error("invalid object size")] + Size(#[from] ParseIntError), /// An internal consistency invariant on the metadata was violated. #[error("invariant violation: {0}")] Invariant(&'static str), @@ -305,10 +311,11 @@ pub struct Metadata { #[serde(skip_serializing_if = "Option::is_none")] pub filename: Option, - /// Size of the data in bytes, if known. + /// Size of the stored data in bytes, if known (header: `x-sn-size`). /// - /// Not transmitted via HTTP headers; set by backends when the object is - /// stored or retrieved. + /// Read-only. This is the size of the complete object, even when only a range of it is + /// being returned. It describes the stored bytes, so for a compressed object it is the + /// compressed size. #[serde(skip_serializing_if = "Option::is_none")] pub size: Option, @@ -421,6 +428,10 @@ impl Metadata { HEADER_FILENAME => { metadata.filename = Some(value.to_str()?.to_owned()); } + HEADER_SIZE if !skip_read_only => { + let size = value.to_str()?; + metadata.size = Some(size.parse()?); + } _ => { // customer-provided metadata if let Some(name) = name.strip_prefix(HEADER_META_PREFIX) { @@ -449,7 +460,7 @@ impl Metadata { expiration_policy, time_created, time_expires, - size: _, + size, custom, } = self; @@ -484,6 +495,10 @@ impl Metadata { let name = HeaderName::try_from(format!("{prefix}{HEADER_FILENAME}"))?; headers.append(name, filename.parse()?); } + if let Some(size) = size { + let name = HeaderName::try_from(format!("{prefix}{HEADER_SIZE}"))?; + headers.append(name, size.to_string().parse()?); + } // customer-provided metadata for (key, value) in custom { @@ -920,15 +935,47 @@ mod tests { } #[test] - fn size_not_included_in_headers() { + fn size_roundtrips_through_headers() { + let metadata = Metadata { + size: Some(42), + ..Default::default() + }; + + let headers = metadata.to_headers("").unwrap(); + assert_eq!(headers.get(HEADER_SIZE).unwrap(), "42"); + assert_eq!(Metadata::from_headers(&headers, "").unwrap().size, Some(42)); + } + + #[test] + fn size_is_prefixed_in_headers() { let metadata = Metadata { size: Some(42), ..Default::default() }; let headers = metadata.to_headers("x-goog-meta-").unwrap(); - let has_size_header = headers.keys().any(|k| k.as_str().contains("size")); - assert!(!has_size_header); + assert_eq!(headers.get("x-goog-meta-x-sn-size").unwrap(), "42"); + } + + #[test] + fn from_insert_headers_ignores_size() { + // Size is materialized by the server; a client-supplied value must never be trusted. + let mut headers = HeaderMap::new(); + headers.insert(HEADER_SIZE, "9999".parse().unwrap()); + + let metadata = Metadata::from_insert_headers(&headers, "").unwrap(); + assert!(metadata.size.is_none()); + } + + #[test] + fn from_headers_rejects_malformed_size() { + let mut headers = HeaderMap::new(); + headers.insert(HEADER_SIZE, "not-a-number".parse().unwrap()); + + assert!(matches!( + Metadata::from_headers(&headers, ""), + Err(Error::Size(_)) + )); } #[test] diff --git a/objectstore-types/src/range.rs b/objectstore-types/src/range.rs index a6e23576..bd1ea8a4 100644 --- a/objectstore-types/src/range.rs +++ b/objectstore-types/src/range.rs @@ -184,7 +184,7 @@ impl ContentRange { /// Formats the length of this range for a `Content-Length` response header. pub fn len_to_header_value(&self) -> HeaderValue { - HeaderValue::from_str(&self.len().to_string()).expect("always a valid header value") + HeaderValue::from(self.len()) } /// Parses the total from an unsatisfiable `Content-Range` response header value.