From f0f9753c42fa8de7e98d06a1ae281469500cb20c Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 22 Apr 2026 23:01:50 +0200 Subject: [PATCH] refactor(backend): split ThumbnailService.generate into stages with distinct logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses @felixbrandt — fix(backend): "the two try blocks in generate() overlap — a save failure logs 'generation failed' even though the thumbnail is already in S3 as an orphan". generate() now orchestrates four stages, each in its own try+log: readSourceImage / encodeThumbnail / uploadToStorage / persistThumbnailMetadata persistThumbnailMetadata emits the distinct "orphaned in storage as " log line so an operator can see database-side failures after the upload completed. The deterministic key ensures the next run overwrites cleanly, so the orphan is self-healing. Also extracts THUMBNAIL_KEY_PREFIX/SUFFIX constants with a comment explaining the deterministic-overwrite contract. Adds test: generate_returnsFailed_whenPersistThrows_butUploadSucceeded. Refs #307 Co-Authored-By: Claude Opus 4.7 --- .../service/ThumbnailService.java | 64 ++++++++++++++++--- .../service/ThumbnailServiceTest.java | 18 ++++++ 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/ThumbnailService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/ThumbnailService.java index 465ff20d..3b4cb48e 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/ThumbnailService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/ThumbnailService.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.io.InputStream; import java.time.LocalDateTime; import java.util.Set; +import java.util.UUID; /** * Generates JPEG thumbnail previews for documents (PDF first-page or scaled-down image) @@ -48,6 +49,13 @@ public class ThumbnailService { private static final Set IMAGE_CONTENT_TYPES = Set.of("image/jpeg", "image/png", "image/tiff"); + // Deterministic S3 key — `thumbnails/{docId}.jpg`. When a document's file is replaced + // the regenerated thumbnail overwrites this same key via PutObject, so we never + // orphan old thumbnails. The URL-level cache buster is the `thumbnail_generated_at` + // timestamp (see /api/documents/{id}/thumbnail ?v= param). + private static final String THUMBNAIL_KEY_PREFIX = "thumbnails/"; + private static final String THUMBNAIL_KEY_SUFFIX = ".jpg"; + private final FileService fileService; private final S3Client s3Client; private final DocumentRepository documentRepository; @@ -74,21 +82,47 @@ public class ThumbnailService { return Outcome.SKIPPED; } - BufferedImage source; + BufferedImage source = readSourceImage(doc, contentType); + if (source == null) return Outcome.FAILED; + + byte[] jpeg = encodeThumbnail(source, doc.getId()); + if (jpeg == null) return Outcome.FAILED; + + String thumbnailKey = thumbnailKeyFor(doc.getId()); + if (!uploadToStorage(thumbnailKey, jpeg, doc.getId())) return Outcome.FAILED; + + return persistThumbnailMetadata(doc, thumbnailKey); + } + + private static String thumbnailKeyFor(UUID documentId) { + return THUMBNAIL_KEY_PREFIX + documentId + THUMBNAIL_KEY_SUFFIX; + } + + private BufferedImage readSourceImage(Document doc, String contentType) { try { - source = PDF_CONTENT_TYPE.equals(contentType) + return PDF_CONTENT_TYPE.equals(contentType) ? renderPdfFirstPage(doc.getFilePath()) : readImage(doc.getFilePath()); } catch (Exception e) { - log.warn("Thumbnail generation failed for doc={} reason={}", + log.warn("Thumbnail source read failed for doc={} reason={}", doc.getId(), e.getMessage()); - return Outcome.FAILED; + return null; } + } + private byte[] encodeThumbnail(BufferedImage source, UUID documentId) { try { BufferedImage scaled = scaleToWidth(source, THUMBNAIL_WIDTH); - byte[] jpeg = encodeJpeg(scaled, JPEG_QUALITY); - String thumbnailKey = "thumbnails/" + doc.getId() + ".jpg"; + return encodeJpeg(scaled, JPEG_QUALITY); + } catch (Exception e) { + log.warn("Thumbnail JPEG encoding failed for doc={} reason={}", + documentId, e.getMessage()); + return null; + } + } + + private boolean uploadToStorage(String thumbnailKey, byte[] jpeg, UUID documentId) { + try { s3Client.putObject( PutObjectRequest.builder() .bucket(bucketName) @@ -96,14 +130,28 @@ public class ThumbnailService { .contentType("image/jpeg") .build(), RequestBody.fromBytes(jpeg)); + return true; + } catch (Exception e) { + log.warn("Thumbnail upload failed for doc={} key={} reason={}", + documentId, thumbnailKey, e.getMessage()); + return false; + } + } + private Outcome persistThumbnailMetadata(Document doc, String thumbnailKey) { + try { doc.setThumbnailKey(thumbnailKey); doc.setThumbnailGeneratedAt(LocalDateTime.now()); documentRepository.save(doc); return Outcome.SUCCESS; } catch (Exception e) { - log.warn("Thumbnail generation failed for doc={} reason={}", - doc.getId(), e.getMessage()); + // Thumbnail is already in S3 but the entity update failed. Because the S3 + // key is deterministic (thumbnails/{docId}.jpg), the next successful run + // — either a re-upload of this document or the admin backfill — will + // overwrite it cleanly. Logging distinctly so an operator tracking + // backfill totals can spot the database-side issue. + log.warn("Thumbnail persist failed for doc={} (orphaned in storage as {}): {}", + doc.getId(), thumbnailKey, e.getMessage()); return Outcome.FAILED; } } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/service/ThumbnailServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/service/ThumbnailServiceTest.java index bf6827af..90b0e8c8 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/ThumbnailServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/ThumbnailServiceTest.java @@ -167,6 +167,24 @@ class ThumbnailServiceTest { verify(documentRepository, never()).save(any()); } + @Test + void generate_returnsFailed_whenPersistThrows_butUploadSucceeded() throws IOException { + // Covers the "orphan thumbnail" edge case: S3 upload succeeded but the + // entity update blew up. We must still return FAILED so the backfill + // tally is honest, without losing the fact that we already put bytes in S3. + Document doc = makeDoc("application/pdf", "documents/letter.pdf"); + when(fileService.downloadFileStream(anyString())) + .thenReturn(new ByteArrayInputStream(createSamplePdf())); + when(documentRepository.save(any())) + .thenThrow(new RuntimeException("constraint violation")); + + ThumbnailService.Outcome outcome = thumbnailService.generate(doc); + + assertThat(outcome).isEqualTo(ThumbnailService.Outcome.FAILED); + verify(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class)); + verify(documentRepository).save(any()); + } + // ─── helpers ────────────────────────────────────────────────────────────── private Document makeDoc(String contentType, String filePath) {