From 516a0a381458d6821c3fd0a6e5f8c66be43162ad Mon Sep 17 00:00:00 2001 From: Marcel Date: Thu, 28 May 2026 16:16:26 +0200 Subject: [PATCH] refactor(person): single source of truth for generation bounds (#689) Markus flagged the 0/10 range was duplicated across five sites (DB CHECK, both importers, DTO @Min/@Max, dropdown range). New PersonGeneration.MIN_GENERATION / MAX_GENERATION constants are now the canonical Java source; the DTO annotations and both importer guards reference them. The V70 SQL CHECK comment now points at the Java constants so future widening updates one Java class plus one SQL literal (Flyway forbids rewriting the migration in place). Refs #689 Co-Authored-By: Claude Opus 4.7 --- .../importing/PersonRegisterImporter.java | 11 +++++------ .../importing/PersonTreeImporter.java | 12 +++++------- .../familienarchiv/person/PersonGeneration.java | 16 ++++++++++++++++ .../familienarchiv/person/PersonUpdateDTO.java | 8 ++++---- .../migration/V70__add_generation_to_persons.sql | 12 +++++++----- 5 files changed, 37 insertions(+), 22 deletions(-) create mode 100644 backend/src/main/java/org/raddatz/familienarchiv/person/PersonGeneration.java diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonRegisterImporter.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonRegisterImporter.java index 1058938b..490694c5 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonRegisterImporter.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonRegisterImporter.java @@ -2,6 +2,7 @@ package org.raddatz.familienarchiv.importing; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.raddatz.familienarchiv.person.PersonGeneration; import org.raddatz.familienarchiv.person.PersonService; import org.raddatz.familienarchiv.person.PersonType; import org.raddatz.familienarchiv.person.PersonUpsertCommand; @@ -33,8 +34,6 @@ public class PersonRegisterImporter { // carry an inline note. Out-of-range values are caught by the post-parse // range guard, not by the regex. private static final Pattern GENERATION_PATTERN = Pattern.compile("^\\s*G?\\s*(-?\\d+)"); - private static final int GENERATION_MIN = 0; - private static final int GENERATION_MAX = 10; private final PersonService personService; @@ -68,16 +67,16 @@ public class PersonRegisterImporter { /** * Parses an optional {@code G n} generation cell. Returns null for blanks, - * non-matching strings, and any value outside {@value #GENERATION_MIN}..{@value #GENERATION_MAX} - * (mirroring the V70 CHECK). Out-of-range values log a WARN but never abort - * the batch — REQ-IMP-001. + * non-matching strings, and any value outside the {@link PersonGeneration} + * bounds (mirroring the V70 CHECK). Out-of-range values log a WARN but + * never abort the batch — REQ-IMP-001. */ static Integer parseGeneration(String raw, String personId) { if (raw == null || raw.isBlank()) return null; Matcher m = GENERATION_PATTERN.matcher(raw); if (!m.find()) return null; int parsed = Integer.parseInt(m.group(1)); - if (parsed < GENERATION_MIN || parsed > GENERATION_MAX) { + if (parsed < PersonGeneration.MIN_GENERATION || parsed > PersonGeneration.MAX_GENERATION) { log.warn("Skipping out-of-range generation '{}' for row {}", raw, personId); return null; } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonTreeImporter.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonTreeImporter.java index 56eab828..df402050 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonTreeImporter.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/PersonTreeImporter.java @@ -7,6 +7,7 @@ import lombok.extern.slf4j.Slf4j; import org.raddatz.familienarchiv.exception.DomainException; import org.raddatz.familienarchiv.exception.ErrorCode; import org.raddatz.familienarchiv.person.Person; +import org.raddatz.familienarchiv.person.PersonGeneration; import org.raddatz.familienarchiv.person.PersonService; import org.raddatz.familienarchiv.person.PersonType; import org.raddatz.familienarchiv.person.PersonUpsertCommand; @@ -87,24 +88,21 @@ public class PersonTreeImporter { } /** - * Returns the JSON {@code generation} value if present and in - * {@value #GENERATION_MIN}..{@value #GENERATION_MAX}; null otherwise. Out-of-range - * values log a WARN but never abort the batch — mirrors the register-importer + * Returns the JSON {@code generation} value if present and within the + * {@link PersonGeneration} bounds; null otherwise. Out-of-range values + * log a WARN but never abort the batch — mirrors the register-importer * skip-and-warn policy. */ private static Integer generationOrNull(JsonNode node, String personId) { Integer raw = intOrNull(node, "generation"); if (raw == null) return null; - if (raw < GENERATION_MIN || raw > GENERATION_MAX) { + if (raw < PersonGeneration.MIN_GENERATION || raw > PersonGeneration.MAX_GENERATION) { log.warn("Skipping out-of-range generation '{}' for person {}", raw, personId); return null; } return raw; } - private static final int GENERATION_MIN = 0; - private static final int GENERATION_MAX = 10; - private int createRelationships(JsonNode relationships, Map idByRowId) { int created = 0; for (JsonNode node : relationships) { diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonGeneration.java b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonGeneration.java new file mode 100644 index 00000000..cea036cd --- /dev/null +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonGeneration.java @@ -0,0 +1,16 @@ +package org.raddatz.familienarchiv.person; + +/** + * Single source of truth for the {@code persons.generation} value range. + * The DB CHECK in V70, the {@code PersonUpdateDTO} Bean Validation annotations, + * and the canonical importers all reference these constants so a future widening + * (e.g. accepting {@code G −1} ancestors) happens in one place. Mirror this file + * by hand in the V70 migration comment when adjusting bounds. + */ +public final class PersonGeneration { + + public static final int MIN_GENERATION = 0; + public static final int MAX_GENERATION = 10; + + private PersonGeneration() {} +} diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonUpdateDTO.java b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonUpdateDTO.java index a462943c..a3cd9aca 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonUpdateDTO.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonUpdateDTO.java @@ -23,9 +23,9 @@ public class PersonUpdateDTO { private String notes; private Integer birthYear; private Integer deathYear; - // Mirror of the persons.generation CHECK constraint (V70). - // null clears the field; 0..10 is the allowlist of valid indices. - @Min(0) - @Max(10) + // Mirror of the persons.generation CHECK constraint (V70). Bounds live in + // PersonGeneration so DB, DTO, and importer all read from one place. + @Min(PersonGeneration.MIN_GENERATION) + @Max(PersonGeneration.MAX_GENERATION) private Integer generation; } diff --git a/backend/src/main/resources/db/migration/V70__add_generation_to_persons.sql b/backend/src/main/resources/db/migration/V70__add_generation_to_persons.sql index acb39b30..5693720b 100644 --- a/backend/src/main/resources/db/migration/V70__add_generation_to_persons.sql +++ b/backend/src/main/resources/db/migration/V70__add_generation_to_persons.sql @@ -10,11 +10,13 @@ ALTER TABLE persons ADD COLUMN generation SMALLINT; --- Allowlist of valid generation indices. The upper bound is intentionally --- loose — current data tops out at G 5, but a future G 6 → G 10 widening --- needs no migration. A G −1 ancestor would require a separate one-shot --- shift migration (out of scope here; the layout's normalise step already --- handles negative seeds at render time). +-- Allowlist of valid generation indices. The 0..10 bounds mirror +-- PersonGeneration.MIN_GENERATION / MAX_GENERATION in Java — keep the +-- two in sync (the DTO @Min/@Max and both importer range guards read from +-- those Java constants). Current data tops out at G 5, but a future G 6 → +-- G 10 widening needs no migration. A G −1 ancestor would require a +-- separate one-shot shift migration (out of scope here; the layout's +-- normalise step already handles negative seeds at render time). ALTER TABLE persons ADD CONSTRAINT chk_generation_range CHECK (generation IS NULL OR generation BETWEEN 0 AND 10);