From 4e33f52addae8ce6de91fbe5be6aa201dd9f0eb8 Mon Sep 17 00:00:00 2001 From: Marcel Date: Thu, 21 May 2026 10:12:43 +0200 Subject: [PATCH] refactor(import): extract SkipReason enum to replace raw skip-reason strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces MassImportService.SkipReason with all five values — INVALID_FILENAME_PATH_TRAVERSAL, INVALID_PDF_SIGNATURE, FILE_READ_ERROR, ALREADY_EXISTS, S3_UPLOAD_FAILED — making the full set of reasons greppable and type-safe. SkippedFile.reason changes from String to SkipReason; importSingleDocument return type updated accordingly. JSON serialisation is unchanged (Jackson serialises enums by name). All tests updated. Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 24 ++++++++++++------- .../importing/MassImportServiceTest.java | 20 ++++++++-------- 2 files changed, 26 insertions(+), 18 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 64b09294..ccd62649 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -56,9 +56,17 @@ public class MassImportService { public enum State { IDLE, RUNNING, DONE, FAILED } + public enum SkipReason { + INVALID_FILENAME_PATH_TRAVERSAL, + INVALID_PDF_SIGNATURE, + FILE_READ_ERROR, + ALREADY_EXISTS, + S3_UPLOAD_FAILED + } + public record SkippedFile( @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String filename, - @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String reason + @Schema(requiredMode = Schema.RequiredMode.REQUIRED) SkipReason reason ) {} public record ImportStatus( @@ -293,7 +301,7 @@ public class MassImportService { String filename = index.contains(".") ? index : index + ".pdf"; if (!isValidImportFilename(filename)) { log.warn("Skipping import row {}: filename rejected — {}", i, filename); - skippedFiles.add(new SkippedFile(filename, "INVALID_FILENAME_PATH_TRAVERSAL")); + skippedFiles.add(new SkippedFile(filename, SkipReason.INVALID_FILENAME_PATH_TRAVERSAL)); continue; } Optional fileOnDisk = findFileRecursive(filename); @@ -305,17 +313,17 @@ public class MassImportService { try { if (!isPdfMagicBytes(fileOnDisk.get())) { log.warn("Überspringe {}: Datei beginnt nicht mit %PDF-Signatur", filename); - skippedFiles.add(new SkippedFile(filename, "INVALID_PDF_SIGNATURE")); + skippedFiles.add(new SkippedFile(filename, SkipReason.INVALID_PDF_SIGNATURE)); continue; } } catch (IOException e) { log.error("Fehler beim Prüfen der Magic-Bytes für {}", filename, e); - skippedFiles.add(new SkippedFile(filename, "FILE_READ_ERROR")); + skippedFiles.add(new SkippedFile(filename, SkipReason.FILE_READ_ERROR)); continue; } } - Optional skipReason = importSingleDocument(cells, fileOnDisk, filename, index); + Optional skipReason = importSingleDocument(cells, fileOnDisk, filename, index); if (skipReason.isPresent()) { skippedFiles.add(new SkippedFile(filename, skipReason.get())); } else { @@ -364,11 +372,11 @@ public class MassImportService { * @return empty Optional on success; an Optional containing the skip reason on failure/skip. */ @Transactional - protected Optional 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 Optional.of("ALREADY_EXISTS"); + return Optional.of(SkipReason.ALREADY_EXISTS); } String archiveBox = getCell(cells, colBox); @@ -404,7 +412,7 @@ public class MassImportService { status = DocumentStatus.UPLOADED; } catch (Exception e) { log.error("S3 Upload Fehler für {}", file.get().getName(), e); - return Optional.of("S3_UPLOAD_FAILED"); + return Optional.of(SkipReason.S3_UPLOAD_FAILED); } } 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 0fb50a8e..575f568c 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -154,10 +154,10 @@ class MassImportServiceTest { .build(); when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing)); - Optional result = 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"); + assertThat(result).isPresent().contains(MassImportService.SkipReason.ALREADY_EXISTS); } // ─── importSingleDocument — already-exists guard fires before file I/O ───── @@ -179,10 +179,10 @@ class MassImportServiceTest { byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- Files.write(physicalFile, pdfHeader); - Optional result = service.importSingleDocument( + Optional result = service.importSingleDocument( minimalCells("present.pdf"), Optional.of(physicalFile.toFile()), "present.pdf", "present"); - assertThat(result).isPresent().contains("ALREADY_EXISTS"); + assertThat(result).isPresent().contains(MassImportService.SkipReason.ALREADY_EXISTS); verify(s3Client, never()).putObject(any(PutObjectRequest.class), any(RequestBody.class)); verify(documentService, never()).save(any()); } @@ -204,7 +204,7 @@ class MassImportServiceTest { 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")); + .containsExactly(org.assertj.core.groups.Tuple.tuple("upload_fail.pdf", MassImportService.SkipReason.S3_UPLOAD_FAILED)); } @Test @@ -223,7 +223,7 @@ class MassImportServiceTest { assertThat(service.getStatus().skipped()).isEqualTo(1); assertThat(service.getStatus().skippedFiles()) .extracting(MassImportService.SkippedFile::reason) - .containsExactly("ALREADY_EXISTS"); + .containsExactly(MassImportService.SkipReason.ALREADY_EXISTS); } // ─── importSingleDocument — create new document (metadata only) ─────────── @@ -283,11 +283,11 @@ class MassImportServiceTest { doThrow(new RuntimeException("S3 error")) .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class)); - Optional result = 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"); + assertThat(result).isPresent().contains(MassImportService.SkipReason.S3_UPLOAD_FAILED); } // ─── importSingleDocument — sender handling ─────────────────────────────── @@ -539,7 +539,7 @@ class MassImportServiceTest { assertThat(result.processed()).isEqualTo(1); assertThat(result.skippedFiles()) .extracting(MassImportService.SkippedFile::reason) - .containsExactly("INVALID_FILENAME_PATH_TRAVERSAL"); + .containsExactly(MassImportService.SkipReason.INVALID_FILENAME_PATH_TRAVERSAL); } // ─── importSingleDocument — non-blank optional fields ──────────────────── @@ -755,7 +755,7 @@ class MassImportServiceTest { assertThat(spyService.getStatus().skipped()).isEqualTo(1); assertThat(spyService.getStatus().skippedFiles()) .extracting(MassImportService.SkippedFile::reason) - .containsExactly("FILE_READ_ERROR"); + .containsExactly(MassImportService.SkipReason.FILE_READ_ERROR); } // ─── readOds — XXE security regression ───────────────────────────────────