marcel/familienarchiv
1
0
Fork 0
Code Issues 107 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor(import): extract SkipReason enum to replace raw skip-reason strings

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-05-21 10:12:43 +02:00
parent 890f014bb3
commit 4e33f52add
2 changed files with 26 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
View File

@@ -56,9 +56,17 @@ public class MassImportService {
public enum State { IDLE, RUNNING, DONE, FAILED } 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( public record SkippedFile(
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) String filename, @Schema(requiredMode = Schema.RequiredMode.REQUIRED) String filename,
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) String reason @Schema(requiredMode = Schema.RequiredMode.REQUIRED) SkipReason reason
) {} ) {}
public record ImportStatus( public record ImportStatus(
@@ -293,7 +301,7 @@ public class MassImportService {
String filename = index.contains(".") ? index : index + ".pdf"; String filename = index.contains(".") ? index : index + ".pdf";
if (!isValidImportFilename(filename)) { if (!isValidImportFilename(filename)) {
log.warn("Skipping import row {}: filename rejected — {}", i, 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; continue;
} }
Optional<File> fileOnDisk = findFileRecursive(filename); Optional<File> fileOnDisk = findFileRecursive(filename);
@@ -305,17 +313,17 @@ public class MassImportService {
try { try {
if (!isPdfMagicBytes(fileOnDisk.get())) { if (!isPdfMagicBytes(fileOnDisk.get())) {
log.warn("Überspringe {}: Datei beginnt nicht mit %PDF-Signatur", filename); 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; continue;
} }
} catch (IOException e) { } catch (IOException e) {
log.error("Fehler beim Prüfen der Magic-Bytes für {}", filename, 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; continue;
} }
} }
Optional<String> skipReason = importSingleDocument(cells, fileOnDisk, filename, index); Optional<SkipReason> skipReason = importSingleDocument(cells, fileOnDisk, filename, index);
if (skipReason.isPresent()) { if (skipReason.isPresent()) {
skippedFiles.add(new SkippedFile(filename, skipReason.get())); skippedFiles.add(new SkippedFile(filename, skipReason.get()));
} else { } else {
@@ -364,11 +372,11 @@ public class MassImportService {
* @return empty Optional on success; an Optional containing the skip reason on failure/skip. * @return empty Optional on success; an Optional containing the skip reason on failure/skip.
*/ */
@Transactional @Transactional
protected Optional<String> importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) { protected Optional<SkipReason> importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) {
Optional<Document> existing = documentService.findByOriginalFilename(originalFilename); Optional<Document> existing = documentService.findByOriginalFilename(originalFilename);
if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) { if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) {
log.info("Dokument {} existiert bereits, überspringe.", originalFilename); log.info("Dokument {} existiert bereits, überspringe.", originalFilename);
return Optional.of("ALREADY_EXISTS"); return Optional.of(SkipReason.ALREADY_EXISTS);
} }
String archiveBox = getCell(cells, colBox); String archiveBox = getCell(cells, colBox);
@@ -404,7 +412,7 @@ public class MassImportService {
status = DocumentStatus.UPLOADED; status = DocumentStatus.UPLOADED;
} catch (Exception e) { } catch (Exception e) {
log.error("S3 Upload Fehler für {}", file.get().getName(), 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);
} }
} }

20
backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
View File

@@ -154,10 +154,10 @@ class MassImportServiceTest {
.build(); .build();
when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing)); when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing));
Optional<String> result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001"); Optional<MassImportService.SkipReason> result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001");
verify(documentService, never()).save(any()); 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 ───── // ─── importSingleDocument — already-exists guard fires before file I/O ─────
@@ -179,10 +179,10 @@ class MassImportServiceTest {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF- byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(physicalFile, pdfHeader); Files.write(physicalFile, pdfHeader);
Optional<String> result = service.importSingleDocument( Optional<MassImportService.SkipReason> result = service.importSingleDocument(
minimalCells("present.pdf"), Optional.of(physicalFile.toFile()), "present.pdf", "present"); 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(s3Client, never()).putObject(any(PutObjectRequest.class), any(RequestBody.class));
verify(documentService, never()).save(any()); verify(documentService, never()).save(any());
} }
@@ -204,7 +204,7 @@ class MassImportServiceTest {
assertThat(service.getStatus().skipped()).isEqualTo(1); assertThat(service.getStatus().skipped()).isEqualTo(1);
assertThat(service.getStatus().skippedFiles()) assertThat(service.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::filename, MassImportService.SkippedFile::reason) .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 @Test
@@ -223,7 +223,7 @@ class MassImportServiceTest {
assertThat(service.getStatus().skipped()).isEqualTo(1); assertThat(service.getStatus().skipped()).isEqualTo(1);
assertThat(service.getStatus().skippedFiles()) assertThat(service.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::reason) .extracting(MassImportService.SkippedFile::reason)
.containsExactly("ALREADY_EXISTS"); .containsExactly(MassImportService.SkipReason.ALREADY_EXISTS);
} }
// ─── importSingleDocument — create new document (metadata only) ─────────── // ─── importSingleDocument — create new document (metadata only) ───────────
@@ -283,11 +283,11 @@ class MassImportServiceTest {
doThrow(new RuntimeException("S3 error")) doThrow(new RuntimeException("S3 error"))
.when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class)); .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class));
Optional<String> result = service.importSingleDocument( Optional<MassImportService.SkipReason> result = service.importSingleDocument(
minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail"); minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail");
verify(documentService, never()).save(any()); verify(documentService, never()).save(any());
assertThat(result).isPresent().contains("S3_UPLOAD_FAILED"); assertThat(result).isPresent().contains(MassImportService.SkipReason.S3_UPLOAD_FAILED);
} }
// ─── importSingleDocument — sender handling ─────────────────────────────── // ─── importSingleDocument — sender handling ───────────────────────────────
@@ -539,7 +539,7 @@ class MassImportServiceTest {
assertThat(result.processed()).isEqualTo(1); assertThat(result.processed()).isEqualTo(1);
assertThat(result.skippedFiles()) assertThat(result.skippedFiles())
.extracting(MassImportService.SkippedFile::reason) .extracting(MassImportService.SkippedFile::reason)
.containsExactly("INVALID_FILENAME_PATH_TRAVERSAL"); .containsExactly(MassImportService.SkipReason.INVALID_FILENAME_PATH_TRAVERSAL);
} }
// ─── importSingleDocument — non-blank optional fields ──────────────────── // ─── importSingleDocument — non-blank optional fields ────────────────────
@@ -755,7 +755,7 @@ class MassImportServiceTest {
assertThat(spyService.getStatus().skipped()).isEqualTo(1); assertThat(spyService.getStatus().skipped()).isEqualTo(1);
assertThat(spyService.getStatus().skippedFiles()) assertThat(spyService.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::reason) .extracting(MassImportService.SkippedFile::reason)
.containsExactly("FILE_READ_ERROR"); .containsExactly(MassImportService.SkipReason.FILE_READ_ERROR);
} }
// ─── readOds — XXE security regression ─────────────────────────────────── // ─── readOds — XXE security regression ───────────────────────────────────