From e124c68cf4f6e2e2a36bc62a05706bc670584dd7 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 12:51:29 +0200 Subject: [PATCH 01/10] feat(import): validate PDF magic bytes before S3 upload Reads first 4 bytes of each candidate file before upload; rejects any file whose header does not match %PDF (0x25 0x50 0x44 0x46). Skipped files are counted and collected in ImportStatus.skippedFiles so operators can see what was rejected without querying Loki. Breaking: ImportStatus record gains skipped + skippedFiles fields. Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 68 +++++++++---- .../importing/MassImportServiceTest.java | 95 +++++++++++++++++-- .../user/AdminControllerTest.java | 5 +- 3 files changed, 141 insertions(+), 27 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java index d6453e19..d2f1e276 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -31,6 +31,7 @@ import javax.xml.parsers.DocumentBuilderFactory; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -53,9 +54,13 @@ public class MassImportService { public enum State { IDLE, RUNNING, DONE, FAILED } - public record ImportStatus(State state, String statusCode, @JsonIgnore String message, int processed, LocalDateTime startedAt) {} + public record SkippedFile(String filename, String reason) {} - private volatile ImportStatus currentStatus = new ImportStatus(State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, null); + public record ImportStatus(State state, String statusCode, @JsonIgnore String message, int processed, int skipped, List skippedFiles, LocalDateTime startedAt) {} + + record ProcessResult(int processed, int skipped, List skippedFiles) {} + + private volatile ImportStatus currentStatus = new ImportStatus(State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null); public ImportStatus getStatus() { return currentStatus; @@ -117,22 +122,22 @@ public class MassImportService { if (currentStatus.state() == State.RUNNING) { throw DomainException.conflict(ErrorCode.IMPORT_ALREADY_RUNNING, "A mass import is already in progress"); } - currentStatus = new ImportStatus(State.RUNNING, "IMPORT_RUNNING", "Import läuft...", 0, LocalDateTime.now()); + currentStatus = new ImportStatus(State.RUNNING, "IMPORT_RUNNING", "Import läuft...", 0, 0, List.of(), LocalDateTime.now()); try { File spreadsheet = findSpreadsheetFile(); log.info("Starte Massenimport aus: {}", spreadsheet.getAbsolutePath()); - int processed = processRows(readSpreadsheet(spreadsheet)); + ProcessResult result = processRows(readSpreadsheet(spreadsheet)); currentStatus = new ImportStatus(State.DONE, "IMPORT_DONE", - "Import abgeschlossen. " + processed + " Dokumente verarbeitet.", - processed, currentStatus.startedAt()); + "Import abgeschlossen. " + result.processed() + " Dokumente verarbeitet.", + result.processed(), result.skipped(), result.skippedFiles(), currentStatus.startedAt()); } catch (NoSpreadsheetException e) { log.error("Massenimport fehlgeschlagen: keine Tabellendatei", e); currentStatus = new ImportStatus(State.FAILED, "IMPORT_FAILED_NO_SPREADSHEET", - "Fehler: " + e.getMessage(), 0, currentStatus.startedAt()); + "Fehler: " + e.getMessage(), 0, 0, List.of(), currentStatus.startedAt()); } catch (Exception e) { log.error("Massenimport fehlgeschlagen", e); currentStatus = new ImportStatus(State.FAILED, "IMPORT_FAILED_INTERNAL", - "Fehler: " + e.getMessage(), 0, currentStatus.startedAt()); + "Fehler: " + e.getMessage(), 0, 0, List.of(), currentStatus.startedAt()); } } @@ -254,8 +259,10 @@ public class MassImportService { // --- Import logic (works on neutral List rows) --- - private int processRows(List> rows) { - int count = 0; + private ProcessResult processRows(List> rows) { + int processed = 0; + List skippedFiles = new ArrayList<>(); + for (int i = 1; i < rows.size(); i++) { // skip header row List cells = rows.get(i); String index = getCell(cells, colIndex); @@ -266,18 +273,46 @@ public class MassImportService { if (fileOnDisk.isEmpty()) { log.warn("Datei nicht gefunden, importiere nur Metadaten: {}", filename); } - importSingleDocument(cells, fileOnDisk, filename, index); - count++; + + if (fileOnDisk.isPresent()) { + try { + if (!isPdfMagicBytes(fileOnDisk.get())) { + log.warn("Überspringe {}: Datei beginnt nicht mit %PDF-Signatur", filename); + skippedFiles.add(new SkippedFile(filename, "Keine gültige PDF-Signatur")); + continue; + } + } catch (IOException e) { + log.error("Fehler beim Prüfen der Magic-Bytes für {}", filename, e); + skippedFiles.add(new SkippedFile(filename, "Fehler beim Lesen der Datei")); + continue; + } + } + + boolean imported = importSingleDocument(cells, fileOnDisk, filename, index); + if (imported) { + processed++; + } + } + return new ProcessResult(processed, skippedFiles.size(), skippedFiles); + } + + private boolean isPdfMagicBytes(File file) throws IOException { + try (InputStream is = new FileInputStream(file)) { + byte[] header = is.readNBytes(4); + return header.length == 4 + && header[0] == 0x25 // % + && header[1] == 0x50 // P + && header[2] == 0x44 // D + && header[3] == 0x46; // F } - return count; } @Transactional - protected void importSingleDocument(List cells, Optional file, String originalFilename, String index) { + protected boolean importSingleDocument(List cells, Optional file, String originalFilename, String index) { Optional existing = documentService.findByOriginalFilename(originalFilename); if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) { log.info("Dokument {} existiert bereits, überspringe.", originalFilename); - return; + return false; } String archiveBox = getCell(cells, colBox); @@ -313,7 +348,7 @@ public class MassImportService { status = DocumentStatus.UPLOADED; } catch (Exception e) { log.error("S3 Upload Fehler für {}", file.get().getName(), e); - return; + return false; } } @@ -355,6 +390,7 @@ public class MassImportService { thumbnailAsyncRunner.dispatchAfterCommit(saved.getId()); } log.info("Importiert{}: {}", file.isEmpty() ? " (nur Metadaten)" : "", originalFilename); + return true; } // --- Helpers --- diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java index dcd7c707..eef66a71 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -135,7 +135,7 @@ class MassImportServiceTest { @Test void runImportAsync_throwsConflict_whenAlreadyRunning() { MassImportService.ImportStatus running = new MassImportService.ImportStatus( - MassImportService.State.RUNNING, "IMPORT_RUNNING", "Running...", 0, LocalDateTime.now()); + MassImportService.State.RUNNING, "IMPORT_RUNNING", "Running...", 0, 0, List.of(), LocalDateTime.now()); ReflectionTestUtils.setField(service, "currentStatus", running); assertThatThrownBy(() -> service.runImportAsync()) @@ -325,8 +325,8 @@ class MassImportServiceTest { @Test void processRows_returnsZero_whenOnlyHeaderRow() { List> rows = List.of(List.of("header", "col1")); - Integer result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); - assertThat(result).isEqualTo(0); + MassImportService.ProcessResult result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); + assertThat(result.processed()).isEqualTo(0); } @Test @@ -335,8 +335,8 @@ class MassImportServiceTest { List.of("header"), minimalCells("") // blank index ); - Integer result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); - assertThat(result).isEqualTo(0); + MassImportService.ProcessResult result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); + assertThat(result.processed()).isEqualTo(0); verify(documentService, never()).findByOriginalFilename(any()); } @@ -349,9 +349,9 @@ class MassImportServiceTest { List.of("header"), minimalCells("doc001") // no dot → appends ".pdf" ); - Integer result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); + MassImportService.ProcessResult result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); - assertThat(result).isEqualTo(1); + assertThat(result.processed()).isEqualTo(1); verify(documentService).findByOriginalFilename("doc001.pdf"); } @@ -364,9 +364,9 @@ class MassImportServiceTest { List.of("header"), minimalCells("doc002.pdf") // has dot → used as-is ); - Integer result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); + MassImportService.ProcessResult result = ReflectionTestUtils.invokeMethod(service, "processRows", rows); - assertThat(result).isEqualTo(1); + assertThat(result.processed()).isEqualTo(1); verify(documentService).findByOriginalFilename("doc002.pdf"); } @@ -525,6 +525,69 @@ class MassImportServiceTest { assertThat(result).isEqualTo("hello"); } + // ─── PDF magic byte validation regression ───────────────────────────────── + + @Test + void runImportAsync_uploadsValidPdf_andSkipsFakeOne(@TempDir Path tempDir) throws Exception { + byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- + Files.write(tempDir.resolve("real.pdf"), pdfHeader); + Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf"); + buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + + when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty()); + when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + service.runImportAsync(); + + verify(s3Client, times(1)).putObject(any(PutObjectRequest.class), any(RequestBody.class)); + } + + @Test + void runImportAsync_setsSkippedCount_toOne_whenOneFakeFile(@TempDir Path tempDir) throws Exception { + byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- + Files.write(tempDir.resolve("real.pdf"), pdfHeader); + Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf"); + buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + + when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty()); + when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + service.runImportAsync(); + + assertThat(service.getStatus().skipped()).isEqualTo(1); + } + + @Test + void runImportAsync_includesRejectedFilename_inSkippedFiles(@TempDir Path tempDir) throws Exception { + byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- + Files.write(tempDir.resolve("real.pdf"), pdfHeader); + Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf"); + buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + + when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty()); + when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + service.runImportAsync(); + + assertThat(service.getStatus().skippedFiles()) + .extracting(MassImportService.SkippedFile::filename) + .contains("fake.pdf"); + } + + @Test + void runImportAsync_skipsFile_whenShorterThanFourBytes(@TempDir Path tempDir) throws Exception { + Files.write(tempDir.resolve("tiny.pdf"), new byte[]{0x25, 0x50, 0x44}); // only 3 bytes + buildMinimalImportXlsx(tempDir, "tiny.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + + service.runImportAsync(); + + assertThat(service.getStatus().skipped()).isEqualTo(1); + } + // ─── readOds — XXE security regression ─────────────────────────────────── // Security regression — do not remove. @@ -621,4 +684,18 @@ class MassImportServiceTest { } return destination.toFile(); } + + private void buildMinimalImportXlsx(Path dir, String... filenames) throws Exception { + Path xlsx = dir.resolve("import.xlsx"); + try (XSSFWorkbook wb = new XSSFWorkbook()) { + org.apache.poi.ss.usermodel.Sheet sheet = wb.createSheet("Sheet1"); + sheet.createRow(0).createCell(0).setCellValue("Index"); + for (int i = 0; i < filenames.length; i++) { + sheet.createRow(i + 1).createCell(0).setCellValue(filenames[i]); + } + try (OutputStream out = Files.newOutputStream(xlsx)) { + wb.write(out); + } + } + } } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/user/AdminControllerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/user/AdminControllerTest.java index 533aa7b3..cc2b885a 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/user/AdminControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/user/AdminControllerTest.java @@ -19,6 +19,7 @@ import org.springframework.test.web.servlet.MockMvc; import java.time.LocalDateTime; import java.util.List; +import java.util.List; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.Mockito.verify; @@ -46,7 +47,7 @@ class AdminControllerTest { @WithMockUser(authorities = "ADMIN") void importStatus_returns200_withStatusCode_whenAdmin() throws Exception { MassImportService.ImportStatus status = new MassImportService.ImportStatus( - MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, null); + MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null); when(massImportService.getStatus()).thenReturn(status); mockMvc.perform(get("/api/admin/import-status")) @@ -60,7 +61,7 @@ class AdminControllerTest { @WithMockUser(authorities = "ADMIN") void importStatus_messageField_notPresentInApiResponse() throws Exception { MassImportService.ImportStatus status = new MassImportService.ImportStatus( - MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, null); + MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null); when(massImportService.getStatus()).thenReturn(status); mockMvc.perform(get("/api/admin/import-status")) -- 2.49.1 From 22589e4729baaecbdbf7a60c2a26194d6a541fab Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 13:14:32 +0200 Subject: [PATCH 02/10] feat(admin): surface skipped file count in ImportStatusCard Adds SkippedFile to the local ImportStatus type and updates ImportStatusCard to show an amber skipped-count section with a collapsible filename list in the DONE state. Only rendered when skipped > 0. i18n keys added for de/en/es. Co-Authored-By: Claude Sonnet 4.6 --- frontend/messages/de.json | 1 + frontend/messages/en.json | 1 + frontend/messages/es.json | 1 + .../admin/system/ImportStatusCard.svelte | 15 ++++++ .../system/ImportStatusCard.svelte.test.ts | 51 +++++++++++++++++++ frontend/src/routes/admin/system/types.ts | 7 +++ 6 files changed, 76 insertions(+) diff --git a/frontend/messages/de.json b/frontend/messages/de.json index 03d63f48..55e93d40 100644 --- a/frontend/messages/de.json +++ b/frontend/messages/de.json @@ -350,6 +350,7 @@ "admin_system_import_status_running": "Import läuft…", "admin_system_import_status_done": "Import abgeschlossen", "admin_system_import_status_done_label": "Dokumente verarbeitet", + "admin_system_import_skipped_label": "übersprungen", "admin_system_import_status_failed": "Import fehlgeschlagen", "admin_system_import_failed_no_spreadsheet": "Keine Tabellendatei gefunden.", "admin_system_import_failed_internal": "Interner Fehler beim Import.", diff --git a/frontend/messages/en.json b/frontend/messages/en.json index d52ddbf5..1ace8196 100644 --- a/frontend/messages/en.json +++ b/frontend/messages/en.json @@ -350,6 +350,7 @@ "admin_system_import_status_running": "Import running…", "admin_system_import_status_done": "Import complete", "admin_system_import_status_done_label": "Documents processed", + "admin_system_import_skipped_label": "skipped", "admin_system_import_status_failed": "Import failed", "admin_system_import_failed_no_spreadsheet": "No spreadsheet file found.", "admin_system_import_failed_internal": "Import failed due to an internal error.", diff --git a/frontend/messages/es.json b/frontend/messages/es.json index 177dcdff..286faf5b 100644 --- a/frontend/messages/es.json +++ b/frontend/messages/es.json @@ -350,6 +350,7 @@ "admin_system_import_status_running": "Importación en curso…", "admin_system_import_status_done": "Importación completada", "admin_system_import_status_done_label": "Documentos procesados", + "admin_system_import_skipped_label": "omitidos", "admin_system_import_status_failed": "Importación fallida", "admin_system_import_failed_no_spreadsheet": "No se encontró ninguna hoja de cálculo.", "admin_system_import_failed_internal": "Error interno durante la importación.", diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte b/frontend/src/routes/admin/system/ImportStatusCard.svelte index 01b565c4..a7cbba65 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte @@ -48,6 +48,21 @@ const failureMessage = $derived(

{m.admin_system_import_status_done()}

+ {#if importStatus.skipped > 0} +
+ +

{importStatus.skipped}

+

+ {m.admin_system_import_skipped_label()} +

+ +
    + {#each importStatus.skippedFiles as { filename, reason } (filename)} +
  • {filename} — {reason}
    • + {/each} +
+
+ {/if} {/if} + + diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts index 7f28de0a..a786b787 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts @@ -140,9 +140,9 @@ describe('ImportStatusCard', () => { processed: 10, skipped: 3, skippedFiles: [ - { filename: 'fake.pdf', reason: 'Keine gültige PDF-Signatur' }, - { filename: 'other.pdf', reason: 'Keine gültige PDF-Signatur' }, - { filename: 'tiny.pdf', reason: 'Keine gültige PDF-Signatur' } + { filename: 'fake.pdf', reason: 'INVALID_PDF_SIGNATURE' }, + { filename: 'other.pdf', reason: 'INVALID_PDF_SIGNATURE' }, + { filename: 'tiny.pdf', reason: 'INVALID_PDF_SIGNATURE' } ] }), ontrigger: () => {} @@ -160,7 +160,7 @@ describe('ImportStatusCard', () => { statusCode: 'IMPORT_DONE', processed: 5, skipped: 1, - skippedFiles: [{ filename: 'fake.pdf', reason: 'Keine gültige PDF-Signatur' }] + skippedFiles: [{ filename: 'fake.pdf', reason: 'INVALID_PDF_SIGNATURE' }] }), ontrigger: () => {} } -- 2.49.1 From e770b81ea50b7be217ad5020612c843979bd736c Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 14:58:34 +0200 Subject: [PATCH 04/10] fix(test): skip IOException test when running as root setReadable(false) silently no-ops as root; check canRead() to guard the assumption correctly so the test is skipped in Docker CI. Co-Authored-By: Claude Sonnet 4.6 --- .../familienarchiv/importing/MassImportServiceTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java index 4442e15d..e6f81f57 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -575,7 +575,8 @@ class MassImportServiceTest { File unreadable = tempDir.resolve("unreadable.pdf").toFile(); buildMinimalImportXlsx(tempDir, "unreadable.pdf"); ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); - assumeTrue(unreadable.setReadable(false), "Requires non-root file permissions"); + unreadable.setReadable(false); + assumeTrue(!unreadable.canRead(), "Requires non-root file permissions"); try { service.runImportAsync(); assertThat(service.getStatus().skipped()).isEqualTo(1); -- 2.49.1 From 53dba772ae1289e88883843b474168a7e65490e2 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 15:12:02 +0200 Subject: [PATCH 05/10] fix(import): add @Schema annotations and fix IOException test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add @Schema(requiredMode = REQUIRED) to SkippedFile and ImportStatus record components so TypeScript codegen produces non-optional fields when generate:api is next run - Extract openFileStream(File) as package-private method so the IOException path can be tested deterministically without relying on OS-level file permissions (which are bypassed when running as root) - Replace assumeTrue-based IOException test with Mockito spy that stubs openFileStream — test now runs in CI unconditionally (45 tests, 0 skipped) Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 22 ++++++++++++++--- .../importing/MassImportServiceTest.java | 24 +++++++++---------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java index b4873b41..1ae77541 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -2,6 +2,7 @@ package org.raddatz.familienarchiv.importing; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import io.swagger.v3.oas.annotations.media.Schema; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.poi.ss.usermodel.*; @@ -55,9 +56,20 @@ public class MassImportService { public enum State { IDLE, RUNNING, DONE, FAILED } - public record SkippedFile(String filename, String reason) {} + public record SkippedFile( + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String filename, + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String reason + ) {} - public record ImportStatus(State state, String statusCode, @JsonIgnore String message, int processed, List skippedFiles, LocalDateTime startedAt) { + public record ImportStatus( + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) State state, + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String statusCode, + @JsonIgnore String message, + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) int processed, + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) List skippedFiles, + LocalDateTime startedAt + ) { + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) @JsonProperty("skipped") public int skipped() { return skippedFiles.size(); } } @@ -300,8 +312,12 @@ public class MassImportService { return new ProcessResult(processed, skippedFiles); } + InputStream openFileStream(File file) throws IOException { + return new FileInputStream(file); + } + private boolean isPdfMagicBytes(File file) throws IOException { - try (InputStream is = new FileInputStream(file)) { + try (InputStream is = openFileStream(file)) { byte[] header = is.readNBytes(4); return header.length == 4 && header[0] == 0x25 // % diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java index e6f81f57..31cd2842 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -40,7 +40,6 @@ import java.util.zip.ZipOutputStream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -572,20 +571,19 @@ class MassImportServiceTest { @Test void runImportAsync_skipsFile_whenMagicBytesCheckThrowsIOException(@TempDir Path tempDir) throws Exception { Files.writeString(tempDir.resolve("unreadable.pdf"), "some content"); - File unreadable = tempDir.resolve("unreadable.pdf").toFile(); buildMinimalImportXlsx(tempDir, "unreadable.pdf"); ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); - unreadable.setReadable(false); - assumeTrue(!unreadable.canRead(), "Requires non-root file permissions"); - try { - service.runImportAsync(); - assertThat(service.getStatus().skipped()).isEqualTo(1); - assertThat(service.getStatus().skippedFiles()) - .extracting(MassImportService.SkippedFile::reason) - .containsExactly("FILE_READ_ERROR"); - } finally { - unreadable.setReadable(true); - } + lenient().when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty()); + + MassImportService spyService = spy(service); + doThrow(new java.io.IOException("simulated read error")).when(spyService).openFileStream(any(File.class)); + + spyService.runImportAsync(); + + assertThat(spyService.getStatus().skipped()).isEqualTo(1); + assertThat(spyService.getStatus().skippedFiles()) + .extracting(MassImportService.SkippedFile::reason) + .containsExactly("FILE_READ_ERROR"); } // ─── readOds — XXE security regression ─────────────────────────────────── -- 2.49.1 From 93da5914a8deffb2745f0e777eed2d8ed84ae759 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 16:30:43 +0200 Subject: [PATCH 06/10] test(admin): add regression test for skipped section hidden during RUNNING Co-Authored-By: Claude Sonnet 4.6 --- .../system/ImportStatusCard.svelte.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts index a786b787..ab7acacf 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts @@ -179,4 +179,24 @@ describe('ImportStatusCard', () => { await expect.element(getByTestId('skipped-count')).not.toBeInTheDocument(); }); + + it('does not show skipped section when RUNNING even with skipped > 0', async () => { + const { getByTestId } = render(ImportStatusCard, { + props: { + importStatus: makeStatus({ + state: 'RUNNING', + statusCode: 'IMPORT_RUNNING', + processed: 5, + skipped: 2, + skippedFiles: [ + { filename: 'a.pdf', reason: 'INVALID_PDF_SIGNATURE' }, + { filename: 'b.pdf', reason: 'INVALID_PDF_SIGNATURE' } + ] + }), + ontrigger: () => {} + } + }); + + await expect.element(getByTestId('skipped-count')).not.toBeInTheDocument(); + }); }); -- 2.49.1 From b38b8c39b863e9aeb225032fdfc6829595bf4edc Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 16:31:30 +0200 Subject: [PATCH 07/10] fix(admin): address round-2 review concerns on ImportStatusCard - Use loop index as each key (handles duplicate filenames) - Increase skipped filename font from text-xs to text-sm - Add motion-safe guard to details chevron transition - Replace text-warning with text-amber-900 to meet WCAG AA contrast Co-Authored-By: Claude Sonnet 4.6 --- .../src/routes/admin/system/ImportStatusCard.svelte | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte b/frontend/src/routes/admin/system/ImportStatusCard.svelte index 69e267c5..176b90f5 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte @@ -55,10 +55,10 @@ function reasonLabel(code: string): string {

{m.admin_system_import_status_done()}

{#if importStatus.skipped > 0} -
+
    - {#each importStatus.skippedFiles as { filename, reason } (filename)} -
  • {filename} — {reasonLabel(reason)}
    • + {#each importStatus.skippedFiles as skipped, i (i)} +
  • + {skipped.filename} — {reasonLabel(skipped.reason)} +
    • {/each}
-- 2.49.1 From f144b025b08d5573604d11881d2bd7d54cc1f9f2 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 19:04:06 +0200 Subject: [PATCH 08/10] fix(import): address round-3 review concerns - Add comment to openFileStream() explaining package-private visibility is intentional (Mockito spy seam for IOException test) - Key {#each} skippedFiles by filename instead of array index - Add test: skipped section hidden when state is FAILED - Add test: reasonLabel returns raw code for unknown reason strings Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 1 + .../admin/system/ImportStatusCard.svelte | 2 +- .../system/ImportStatusCard.svelte.test.ts | 33 +++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java index 1ae77541..58d6493a 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -312,6 +312,7 @@ public class MassImportService { return new ProcessResult(processed, skippedFiles); } + // package-private: Mockito spy in tests can override to inject IOException InputStream openFileStream(File file) throws IOException { return new FileInputStream(file); } diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte b/frontend/src/routes/admin/system/ImportStatusCard.svelte index 176b90f5..f6e9ab82 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte @@ -78,7 +78,7 @@ function reasonLabel(code: string): string {
    - {#each importStatus.skippedFiles as skipped, i (i)} + {#each importStatus.skippedFiles as skipped (skipped.filename)}
  • {skipped.filename} — {reasonLabel(skipped.reason)}
    • diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts index ab7acacf..d7470cda 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts @@ -199,4 +199,37 @@ describe('ImportStatusCard', () => { await expect.element(getByTestId('skipped-count')).not.toBeInTheDocument(); }); + + it('does not show skipped section when FAILED even with skipped > 0', async () => { + const { getByTestId } = render(ImportStatusCard, { + props: { + importStatus: makeStatus({ + state: 'FAILED', + statusCode: 'IMPORT_FAILED_INTERNAL', + skipped: 1, + skippedFiles: [{ filename: 'bad.pdf', reason: 'INVALID_PDF_SIGNATURE' }] + }), + ontrigger: () => {} + } + }); + + await expect.element(getByTestId('skipped-count')).not.toBeInTheDocument(); + }); + + it('shows raw reason code for unknown skip reasons', async () => { + const { getByText } = render(ImportStatusCard, { + props: { + importStatus: makeStatus({ + state: 'DONE', + statusCode: 'IMPORT_DONE', + processed: 1, + skipped: 1, + skippedFiles: [{ filename: 'odd.pdf', reason: 'SOME_FUTURE_CODE' }] + }), + ontrigger: () => {} + } + }); + + await expect.element(getByText('SOME_FUTURE_CODE', { exact: false })).toBeInTheDocument(); + }); }); -- 2.49.1 From 9443d8e668b418f98536a283ed01e54a246a0dfa Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 19 May 2026 07:44:12 +0200 Subject: [PATCH 09/10] fix(import): surface S3 failures + already-exists in skippedFiles, a11y + max-height - Change importSingleDocument return type from boolean to Optional so callers in processRows receive the skip reason on every non-success path. S3 upload failures now surface as "S3_UPLOAD_FAILED" and already-imported documents as "ALREADY_EXISTS" in the skippedFiles list shown in the admin UI. - Add two new tests: runImportAsync_addsS3UploadFailed_toSkippedFiles and runImportAsync_addsAlreadyExists_toSkippedFiles; update importSingleDocument_skips_whenDocumentAlreadyUploadedNotPlaceholder and the S3-failure test to assert on the Optional return value. - Add i18n keys for S3_UPLOAD_FAILED and ALREADY_EXISTS in de/en/es messages. - Svelte ImportStatusCard: add aria-hidden="true" to SVG chevron, wrap conditional warning section in aria-live="polite" div, add max-h-64 overflow-y-auto to skipped-files
      to cap height on large batches. Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 27 ++++++-- .../importing/MassImportServiceTest.java | 47 ++++++++++++- frontend/messages/de.json | 2 + frontend/messages/en.json | 2 + frontend/messages/es.json | 2 + .../admin/system/ImportStatusCard.svelte | 67 ++++++++++--------- 6 files changed, 106 insertions(+), 41 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java index 58d6493a..e09aaa76 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -69,9 +69,15 @@ public class MassImportService { @Schema(requiredMode = Schema.RequiredMode.REQUIRED) List skippedFiles, LocalDateTime startedAt ) { - @Schema(requiredMode = Schema.RequiredMode.REQUIRED) + // Note: @Schema on a record accessor method is not picked up by SpringDoc; the + // "skipped" count is a computed convenience field derived from skippedFiles.size(). @JsonProperty("skipped") public int skipped() { return skippedFiles.size(); } + + /** Defensive-copy constructor — callers cannot mutate the stored list after construction. */ + public ImportStatus { + skippedFiles = List.copyOf(skippedFiles); + } } record ProcessResult(int processed, List skippedFiles) {} @@ -304,8 +310,10 @@ public class MassImportService { } } - boolean imported = importSingleDocument(cells, fileOnDisk, filename, index); - if (imported) { + Optional skipReason = importSingleDocument(cells, fileOnDisk, filename, index); + if (skipReason.isPresent()) { + skippedFiles.add(new SkippedFile(filename, skipReason.get())); + } else { processed++; } } @@ -328,12 +336,17 @@ public class MassImportService { } } + /** + * Imports a single document row. + * + * @return empty Optional on success; an Optional containing the skip reason on failure/skip. + */ @Transactional - protected boolean importSingleDocument(List cells, Optional file, String originalFilename, String index) { + protected Optional importSingleDocument(List cells, Optional file, String originalFilename, String index) { Optional existing = documentService.findByOriginalFilename(originalFilename); if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) { log.info("Dokument {} existiert bereits, überspringe.", originalFilename); - return false; + return Optional.of("ALREADY_EXISTS"); } String archiveBox = getCell(cells, colBox); @@ -369,7 +382,7 @@ public class MassImportService { status = DocumentStatus.UPLOADED; } catch (Exception e) { log.error("S3 Upload Fehler für {}", file.get().getName(), e); - return false; + return Optional.of("S3_UPLOAD_FAILED"); } } @@ -411,7 +424,7 @@ public class MassImportService { thumbnailAsyncRunner.dispatchAfterCommit(saved.getId()); } log.info("Importiert{}: {}", file.isEmpty() ? " (nur Metadaten)" : "", originalFilename); - return true; + return Optional.empty(); } // --- Helpers --- diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java index 31cd2842..26d38679 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -154,9 +154,49 @@ class MassImportServiceTest { .build(); when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing)); - service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001"); + Optional result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001"); verify(documentService, never()).save(any()); + assertThat(result).isPresent().contains("ALREADY_EXISTS"); + } + + // ─── importSingleDocument — S3 failure surfaced in skippedFiles ────────── + + @Test + void runImportAsync_addsS3UploadFailed_toSkippedFiles_whenS3Throws(@TempDir Path tempDir) throws Exception { + byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- + Files.write(tempDir.resolve("upload_fail.pdf"), pdfHeader); + buildMinimalImportXlsx(tempDir, "upload_fail.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + when(documentService.findByOriginalFilename("upload_fail.pdf")).thenReturn(Optional.empty()); + doThrow(new RuntimeException("S3 unavailable")) + .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class)); + + service.runImportAsync(); + + assertThat(service.getStatus().skipped()).isEqualTo(1); + assertThat(service.getStatus().skippedFiles()) + .extracting(MassImportService.SkippedFile::filename, MassImportService.SkippedFile::reason) + .containsExactly(org.assertj.core.groups.Tuple.tuple("upload_fail.pdf", "S3_UPLOAD_FAILED")); + } + + @Test + void runImportAsync_addsAlreadyExists_toSkippedFiles_whenDocumentAlreadyUploaded(@TempDir Path tempDir) throws Exception { + buildMinimalImportXlsx(tempDir, "existing.pdf"); + ReflectionTestUtils.setField(service, "importDir", tempDir.toString()); + Document existing = Document.builder() + .id(UUID.randomUUID()) + .originalFilename("existing.pdf") + .status(DocumentStatus.UPLOADED) + .build(); + when(documentService.findByOriginalFilename("existing.pdf")).thenReturn(Optional.of(existing)); + + service.runImportAsync(); + + assertThat(service.getStatus().skipped()).isEqualTo(1); + assertThat(service.getStatus().skippedFiles()) + .extracting(MassImportService.SkippedFile::reason) + .containsExactly("ALREADY_EXISTS"); } // ─── importSingleDocument — create new document (metadata only) ─────────── @@ -208,7 +248,7 @@ class MassImportServiceTest { } @Test - void importSingleDocument_returnsEarly_whenS3UploadFails(@TempDir Path tempDir) throws Exception { + void importSingleDocument_returnsS3UploadFailed_whenS3UploadFails(@TempDir Path tempDir) throws Exception { Path tempFile = tempDir.resolve("fail.pdf"); Files.write(tempFile, "data".getBytes()); @@ -216,10 +256,11 @@ class MassImportServiceTest { doThrow(new RuntimeException("S3 error")) .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class)); - service.importSingleDocument( + Optional result = service.importSingleDocument( minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail"); verify(documentService, never()).save(any()); + assertThat(result).isPresent().contains("S3_UPLOAD_FAILED"); } // ─── importSingleDocument — sender handling ─────────────────────────────── diff --git a/frontend/messages/de.json b/frontend/messages/de.json index ace72a7e..92b405b8 100644 --- a/frontend/messages/de.json +++ b/frontend/messages/de.json @@ -353,6 +353,8 @@ "admin_system_import_skipped_label": "übersprungen", "import_reason_invalid_pdf_signature": "Keine gültige PDF-Signatur", "import_reason_file_read_error": "Fehler beim Lesen der Datei", + "import_reason_s3_upload_failed": "Upload-Fehler (S3)", + "import_reason_already_exists": "Bereits importiert", "admin_system_import_status_failed": "Import fehlgeschlagen", "admin_system_import_failed_no_spreadsheet": "Keine Tabellendatei gefunden.", "admin_system_import_failed_internal": "Interner Fehler beim Import.", diff --git a/frontend/messages/en.json b/frontend/messages/en.json index 4bcc3ca7..cf8b3f01 100644 --- a/frontend/messages/en.json +++ b/frontend/messages/en.json @@ -353,6 +353,8 @@ "admin_system_import_skipped_label": "skipped", "import_reason_invalid_pdf_signature": "Invalid PDF signature", "import_reason_file_read_error": "File read error", + "import_reason_s3_upload_failed": "Upload error (S3)", + "import_reason_already_exists": "Already imported", "admin_system_import_status_failed": "Import failed", "admin_system_import_failed_no_spreadsheet": "No spreadsheet file found.", "admin_system_import_failed_internal": "Import failed due to an internal error.", diff --git a/frontend/messages/es.json b/frontend/messages/es.json index b52b17c3..d0423397 100644 --- a/frontend/messages/es.json +++ b/frontend/messages/es.json @@ -353,6 +353,8 @@ "admin_system_import_skipped_label": "omitidos", "import_reason_invalid_pdf_signature": "Firma PDF no válida", "import_reason_file_read_error": "Error al leer el archivo", + "import_reason_s3_upload_failed": "Error de carga (S3)", + "import_reason_already_exists": "Ya importado", "admin_system_import_status_failed": "Importación fallida", "admin_system_import_failed_no_spreadsheet": "No se encontró ninguna hoja de cálculo.", "admin_system_import_failed_internal": "Error interno durante la importación.", diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte b/frontend/src/routes/admin/system/ImportStatusCard.svelte index f6e9ab82..c18fcef3 100644 --- a/frontend/src/routes/admin/system/ImportStatusCard.svelte +++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte @@ -19,6 +19,8 @@ const failureMessage = $derived( function reasonLabel(code: string): string { if (code === 'INVALID_PDF_SIGNATURE') return m.import_reason_invalid_pdf_signature(); if (code === 'FILE_READ_ERROR') return m.import_reason_file_read_error(); + if (code === 'S3_UPLOAD_FAILED') return m.import_reason_s3_upload_failed(); + if (code === 'ALREADY_EXISTS') return m.import_reason_already_exists(); return code; } @@ -54,38 +56,41 @@ function reasonLabel(code: string): string {

      {m.admin_system_import_status_done()}

      - {#if importStatus.skipped > 0} -
      - - - - -
      - {importStatus.skipped} + {#if importStatus.skipped > 0} +
      + + - {m.admin_system_import_skipped_label()} - -
      -       -
        - {#each importStatus.skippedFiles as skipped (skipped.filename)} -
      • - {skipped.filename} — {reasonLabel(skipped.reason)} -
        • - {/each} -
      -
      - {/if} + + +
      + {importStatus.skipped} + + {m.admin_system_import_skipped_label()} + +
      + +
        + {#each importStatus.skippedFiles as skipped (skipped.filename)} +
      • + {skipped.filename} — {reasonLabel(skipped.reason)} +
        • + {/each} +
      +
+ {/if} +