diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 4e76938a..dee210e0 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -505,6 +505,62 @@ impl ThumbnailService { /// `blob_hash` is used to locate the file on disk (dedup-aware). /// If `None`, only the in-memory cache is checked (used for video /// thumbnails where blob_hash is not yet resolved). + /// Identity of the bytes a thumbnail request will serve — the body of its + /// HTTP ETag. + /// + /// Mirrors the tier precedence in [`Self::get_cached_thumbnail`], because + /// an ETag that names a different tier than the one answering is worse + /// than a coarse one: it lets two resources serving different bytes share + /// a validator, and a shared cache may then hand either to either. + /// + /// * An **attached** blob wins, and its own hash is the identity. Nothing + /// else works: uploading a preview does not change the file's content, + /// so a source-keyed ETag would not change either — and with + /// `immutable` set, clients would never revalidate. Worse, a copy + /// inherits the source hash, so an original and a copy carrying + /// *different* uploaded previews would collide on one ETag. + /// * Otherwise a **derived** blob's own hash. Strictly better than + /// source-keying, never worse: the pair is written from the same bytes, + /// and where they can diverge — a sidecar re-rendered while the derived + /// row stays pinned by `ON CONFLICT DO NOTHING` — a source-keyed ETag + /// is wrong too, because the renderer is not part of the key. + /// * Otherwise the source-keyed form, which still identifies a render of + /// known content at a known size and format. + /// + /// Known gap: a legacy `ext-{file_id}.jpg` with no `file_attached_blobs` + /// row yet falls through to the source-keyed form, so those bytes keep + /// today's coarse validator until the import backfills the row. No worse + /// than current behaviour, and it disappears with the migration. + pub async fn thumbnail_content_id( + &self, + file_id: &str, + blob_hash: &str, + size: ThumbnailSize, + format: ThumbnailFormat, + dedup: Option<&DedupService>, + ) -> String { + if let Some(dedup) = dedup { + if let Some(attached) = dedup + .find_attached_blob(file_id, "preview", size.dir_name()) + .await + { + return attached.blob_hash; + } + if let Some(derived) = dedup + .find_derived_blob(blob_hash, "thumbnail", size.dir_name()) + .await + { + return derived.blob_hash; + } + } + format!( + "thumb-{}-{}-{}", + blob_hash, + size.dir_name(), + format.as_str() + ) + } + /// Drain a blob through the dedup stack into memory. /// /// Shared by the attached and derived tiers — the only difference between diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 6fbc0845..9413fe19 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -487,18 +487,22 @@ impl FileHandler { Ok(h) => h, Err(err) => return AppError::from(err).into_response(), }; - let etag = { - let (s, f) = (thumb_size.as_str(), format.as_str()); - let mut e = String::with_capacity(9 + blob_hash.len() + s.len() + f.len()); - e.push_str("\"thumb-"); - e.push_str(&blob_hash); - e.push('-'); - e.push_str(s); - e.push('-'); - e.push_str(f); - e.push('"'); - e - }; + // The identity of the bytes about to be served, resolved through the + // same tier precedence the read path uses — an uploaded preview's own + // hash, else a derived thumbnail's own hash, else the source-keyed + // form. See `ThumbnailService::thumbnail_content_id`. + let etag = format!( + "\"{}\"", + thumbnail_service + .thumbnail_content_id( + &id, + &blob_hash, + thumb_size.into(), + format, + Some(&state.core.dedup_service), + ) + .await + ); if let Some(if_none_match) = headers.get(header::IF_NONE_MATCH) && let Ok(val) = if_none_match.to_str() && (val == etag || val == "*") diff --git a/src/interfaces/nextcloud/preview_handler.rs b/src/interfaces/nextcloud/preview_handler.rs index 385501b8..6c169fe7 100644 --- a/src/interfaces/nextcloud/preview_handler.rs +++ b/src/interfaces/nextcloud/preview_handler.rs @@ -166,16 +166,23 @@ pub async fn handle_preview( .unwrap(); } }; - let etag = { - let s = thumb_size.as_str(); - let mut e = String::with_capacity(9 + blob_hash.len() + s.len()); - e.push_str("\"thumb-"); - e.push_str(&blob_hash); - e.push('-'); - e.push_str(s); - e.push('"'); - e - }; + // Same tier-precedence resolution as the REST endpoint: an uploaded + // preview's own hash, else a derived thumbnail's own hash, else the + // source-keyed form. NC pins JPEG, so that is the format asked for. + let etag = format!( + "\"{}\"", + state + .core + .thumbnail_service + .thumbnail_content_id( + &object_id, + &blob_hash, + thumb_size.into(), + ThumbnailFormat::Jpeg, + Some(&state.core.dedup_service), + ) + .await + ); if let Some(inm) = req.headers().get(header::IF_NONE_MATCH) && let Ok(client_etag) = inm.to_str() && (client_etag == etag || client_etag == "*") diff --git a/tests/api/attached_thumbnail_copy.hurl b/tests/api/attached_thumbnail_copy.hurl index bf4831b1..a14683e5 100644 --- a/tests/api/attached_thumbnail_copy.hurl +++ b/tests/api/attached_thumbnail_copy.hurl @@ -92,6 +92,7 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] rendered_thumb: bytes +rendered_etag: header "ETag" # ───────────────────────────────────────────────────────────── @@ -112,8 +113,33 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] uploaded_thumb: bytes +uploaded_etag: header "ETag" [Asserts] bytes != {{rendered_thumb}} +# The ETag must move with the bytes. It is keyed on the ATTACHED blob's own +# hash, because uploading a preview leaves the file's content — and so a +# source-keyed ETag — unchanged. With `immutable` set, an unchanged +# validator means clients never revalidate and keep the old render for a +# year. +header "ETag" != "{{rendered_etag}}" + + +# A client holding the pre-upload validator must be told to refetch. +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{rendered_etag}} + +HTTP 200 +[Asserts] +header "ETag" == "{{uploaded_etag}}" + + +# ...and the new one revalidates. +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{uploaded_etag}} + +HTTP 304 # ───────────────────────────────────────────────────────────── @@ -141,6 +167,12 @@ HTTP 200 [Asserts] bytes == {{uploaded_thumb}} bytes != {{rendered_thumb}} +# Same bytes, so the same validator — the copy's attachment row points at +# the same blob. This is also what stops the collision a source-keyed ETag +# would allow: the copy inherits the source hash, so if either side later +# gets a DIFFERENT preview the two would serve different bytes under one +# ETag, and a shared cache could hand either to either. +header "ETag" == "{{uploaded_etag}}" # ───────────────────────────────────────────────────────────── @@ -184,6 +216,7 @@ HTTP 200 [Asserts] bytes == {{uploaded_thumb}} bytes != {{rendered_thumb}} +header "ETag" == "{{uploaded_etag}}" # ─────────────────────────────────────────────────────────────