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

refactor(backend): add ArchUnit test enforcing domain boundaries #409

New Issue
Closed
opened 2026-05-04 16:14:51 +02:00 by marcel · 9 comments
marcel commented 2026-05-04 16:14:51 +02:00
Owner
Copy Link

Context

Part of Epic #406 — Big-bang restructure. This is REFACTOR-3: add an ArchUnit test that fails the build if anyone violates the layering or cross-domain rules in the future. Without this, the structure that REFACTOR-1 (#407) achieves will silently rot.

Per the Legibility Rubric, this addresses C6.1, C6.2 (Critical) — structurally.

Required rules

The ArchUnit test class lives at backend/src/test/java/.../shared/ArchitectureTest.java and asserts:

Rule 1 — No controller injects a repository directly

ArchRule rule = noClasses()
    .that().areAnnotatedWith(RestController.class)
    .should().dependOnClassesThat().areAssignableTo(JpaRepository.class);

Rule 2 — No service depends on another domain's repository

// For each Tier-1 domain package <D>:
//   Classes in package "<...>.<D>.." may only access repositories in "<...>.<D>.."
//   (and never repositories in any other domain's package)
ArchRule rule = noClasses()
    .that().resideInAPackage("..document..")
    .should().dependOnClassesThat().areAssignableTo(JpaRepository.class)
        .and().resideOutsideOfPackage("..document..");
// Repeat for person, tag, user, geschichte, notification, ocr

Rule 3 — Cross-cutting code only in shared/

ArchRule rule = classes()
    .that().areAssignableTo(SecurityConfig.class).or().areAssignableTo(AsyncConfig.class)
    .should().resideInAPackage("..shared.config..").orShould().resideInAPackage("..shared..");

Rule 4 — Entities live in their domain package, not in a shared model/

ArchRule rule = classes()
    .that().areAnnotatedWith(Entity.class)
    .should().resideInAnyPackage(
        "..document..", "..person..", "..tag..", "..user..",
        "..geschichte..", "..notification..", "..ocr..", "..shared.audit.."
    );

Rule 5 — Controllers expose endpoints under their domain prefix

A weaker check (this one is harder to enforce mechanically — accept a TODO if too brittle):

ArchRule rule = classes()
    .that().areAnnotatedWith(RestController.class).and().resideInAPackage("..document..")
    .should().beAnnotatedWith(RequestMapping.class)
    // and the value of @RequestMapping starts with "/api/documents"
    // (custom predicate)

Maven dependency

Add to backend/pom.xml:

<dependency>
    <groupId>com.tngtech.archunit</groupId>
    <artifactId>archunit-junit5</artifactId>
    <version>1.3.0</version>
    <scope>test</scope>
</dependency>

Acceptance criteria

  • ArchitectureTest exists at backend/src/test/java/.../shared/ArchitectureTest.java
  • Rules 1, 2, 3, 4 are implemented and pass on the current domain-based codebase (post REFACTOR-1 / #407)
  • Rule 5 is either implemented or has a documented TODO with a follow-up issue
  • ./mvnw test runs ArchitectureTest as part of the suite
  • Deliberately introducing a violation (e.g., add @Autowired DocumentRepository docRepo in PersonService) makes the test FAIL
  • PR opened and merged

Dependency

Hard dependency on REFACTOR-1 (#407) — the rules assert the post-refactor structure.

Definition of Done

PR merged. Closing comment lists each rule with a one-line "what it prevents."

Dispatch

Best done by an agent comfortable with ArchUnit DSL. Verify each rule by intentionally breaking the code, confirming the test fails, then reverting.

## Context Part of **Epic #406** — Big-bang restructure. This is **REFACTOR-3**: add an ArchUnit test that fails the build if anyone violates the layering or cross-domain rules in the future. Without this, the structure that REFACTOR-1 (#407) achieves will silently rot. Per the Legibility Rubric, this addresses **C6.1, C6.2 (Critical) — structurally**. ## Required rules The ArchUnit test class lives at `backend/src/test/java/.../shared/ArchitectureTest.java` and asserts: ### Rule 1 — No controller injects a repository directly ```java ArchRule rule = noClasses() .that().areAnnotatedWith(RestController.class) .should().dependOnClassesThat().areAssignableTo(JpaRepository.class); ``` ### Rule 2 — No service depends on another domain's repository ```java // For each Tier-1 domain package <D>: // Classes in package "<...>.<D>.." may only access repositories in "<...>.<D>.." // (and never repositories in any other domain's package) ArchRule rule = noClasses() .that().resideInAPackage("..document..") .should().dependOnClassesThat().areAssignableTo(JpaRepository.class) .and().resideOutsideOfPackage("..document.."); // Repeat for person, tag, user, geschichte, notification, ocr ``` ### Rule 3 — Cross-cutting code only in shared/ ```java ArchRule rule = classes() .that().areAssignableTo(SecurityConfig.class).or().areAssignableTo(AsyncConfig.class) .should().resideInAPackage("..shared.config..").orShould().resideInAPackage("..shared.."); ``` ### Rule 4 — Entities live in their domain package, not in a shared model/ ```java ArchRule rule = classes() .that().areAnnotatedWith(Entity.class) .should().resideInAnyPackage( "..document..", "..person..", "..tag..", "..user..", "..geschichte..", "..notification..", "..ocr..", "..shared.audit.." ); ``` ### Rule 5 — Controllers expose endpoints under their domain prefix A weaker check (this one is harder to enforce mechanically — accept a TODO if too brittle): ```java ArchRule rule = classes() .that().areAnnotatedWith(RestController.class).and().resideInAPackage("..document..") .should().beAnnotatedWith(RequestMapping.class) // and the value of @RequestMapping starts with "/api/documents" // (custom predicate) ``` ## Maven dependency Add to `backend/pom.xml`: ```xml <dependency> <groupId>com.tngtech.archunit</groupId> <artifactId>archunit-junit5</artifactId> <version>1.3.0</version> <scope>test</scope> </dependency> ``` ## Acceptance criteria - [ ] `ArchitectureTest` exists at `backend/src/test/java/.../shared/ArchitectureTest.java` - [ ] Rules 1, 2, 3, 4 are implemented and pass on the current domain-based codebase (post REFACTOR-1 / #407) - [ ] Rule 5 is either implemented or has a documented TODO with a follow-up issue - [ ] `./mvnw test` runs `ArchitectureTest` as part of the suite - [ ] Deliberately introducing a violation (e.g., add `@Autowired DocumentRepository docRepo` in PersonService) makes the test FAIL - [ ] PR opened and merged ## Dependency **Hard dependency** on REFACTOR-1 (#407) — the rules assert the post-refactor structure. ## Definition of Done PR merged. Closing comment lists each rule with a one-line "what it prevents." ## Dispatch Best done by an agent comfortable with ArchUnit DSL. Verify each rule by intentionally breaking the code, confirming the test fails, then reverting.
marcel added this to the (deleted) milestone 2026-05-04 16:14:51 +02:00
marcel added the P2-mediumlegibilityrefactor labels 2026-05-04 16:16:07 +02:00
marcel commented 2026-05-04 21:04:11 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Senior Application Architect

Observations

  • The rules as written are correct and cover the key failure modes. ArchUnit 1.3.0 is the current release — the version is right.
  • Rule 2 will immediately find a real violation: StatsController in controller/StatsController.java currently injects PersonRepository and DocumentRepository directly. That's a Rule 1 hit (controller → repository), not a Rule 2 hit, but it's a real pre-existing violation. The issue says rules "pass on the current domain-based codebase (post REFACTOR-1 / #407)". That assumption needs verifying — the current pre-REFACTOR-1 codebase already has this violation. Make sure the rule is only enforced after #407 lands.
  • The package structure is still flat layer-based (controller/, service/, repository/) rather than domain-based. The ArchUnit rule text uses ..document.., ..person.. etc., which won't match anything in the current structure — all repositories live in org.raddatz.familienarchiv.repository. Rule 2 as written assumes the post-REFACTOR-1 domain-based package layout. This must be an explicit hard dependency: these rules are non-compilable gibberish until #407 ships.
  • Rule 3 targets SecurityConfig and AsyncConfig residing in ..shared.config.. or ..shared... Those currently live in config/. Again, only valid post-REFACTOR-1.
  • Rule 4 targets entity packages like ..document.., ..person.. etc. Currently all entities are in model/. Same hard dependency on #407.
  • Rule 5 (URL prefix enforcement) is correctly flagged as potentially brittle — a custom ArchUnit predicate inspecting annotation string values is doable but fragile when URL patterns change. The suggested approach of accepting a TODO is the right call for a first pass.

Recommendations

  • Make the hard dependency explicit in the acceptance criteria: these tests must not be merged before #407 is merged. A CI pipeline that merges #409 first and then #407 would have a full red suite before the refactor even happens.
  • Consider wrapping the rules in a @Tag("architecture") annotation so they can be skipped selectively until the post-REFACTOR-1 branch is in play: ./mvnw test -Dgroups=architecture vs. ./mvnw test -DexcludedGroups=architecture.
  • For Rule 2, also add a rule that ..repository.. (post-refactor) packages may only be accessed by services in the same domain package — not just the inverse. Belt-and-suspenders.
  • The ArchitectureTest.java should live in the shared package of the test tree: backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java. The issue specifies this correctly — just confirming the path aligns with the post-REFACTOR-1 shared/ package that will be created by #407.
  • The closing comment requirement (one-line "what it prevents" per rule) is good hygiene — keep it in the DoD.
## 🏗️ Markus Keller — Senior Application Architect ### Observations - The rules as written are correct and cover the key failure modes. ArchUnit 1.3.0 is the current release — the version is right. - **Rule 2 will immediately find a real violation**: `StatsController` in `controller/StatsController.java` currently injects `PersonRepository` and `DocumentRepository` directly. That's a Rule 1 hit (controller → repository), not a Rule 2 hit, but it's a real pre-existing violation. The issue says rules "pass on the current domain-based codebase (post REFACTOR-1 / #407)". That assumption needs verifying — the current pre-REFACTOR-1 codebase already has this violation. Make sure the rule is only enforced *after* #407 lands. - The package structure is still **flat layer-based** (`controller/`, `service/`, `repository/`) rather than domain-based. The ArchUnit rule text uses `..document..`, `..person..` etc., which **won't match anything in the current structure** — all repositories live in `org.raddatz.familienarchiv.repository`. Rule 2 as written assumes the post-REFACTOR-1 domain-based package layout. This must be an explicit hard dependency: these rules are non-compilable gibberish until #407 ships. - Rule 3 targets `SecurityConfig` and `AsyncConfig` residing in `..shared.config..` or `..shared..`. Those currently live in `config/`. Again, only valid post-REFACTOR-1. - Rule 4 targets entity packages like `..document..`, `..person..` etc. Currently all entities are in `model/`. Same hard dependency on #407. - Rule 5 (URL prefix enforcement) is correctly flagged as potentially brittle — a custom ArchUnit predicate inspecting annotation string values is doable but fragile when URL patterns change. The suggested approach of accepting a TODO is the right call for a first pass. ### Recommendations - Make the hard dependency explicit in the acceptance criteria: **these tests must not be merged before #407 is merged**. A CI pipeline that merges #409 first and then #407 would have a full red suite before the refactor even happens. - Consider wrapping the rules in a `@Tag("architecture")` annotation so they can be skipped selectively until the post-REFACTOR-1 branch is in play: `./mvnw test -Dgroups=architecture` vs. `./mvnw test -DexcludedGroups=architecture`. - For Rule 2, also add a rule that `..repository..` (post-refactor) packages may only be accessed by services in the same domain package — not just the inverse. Belt-and-suspenders. - The `ArchitectureTest.java` should live in the `shared` package of the *test* tree: `backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java`. The issue specifies this correctly — just confirming the path aligns with the post-REFACTOR-1 `shared/` package that will be created by #407. - The closing comment requirement (one-line "what it prevents" per rule) is good hygiene — keep it in the DoD.
marcel commented 2026-05-04 21:04:29 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • The implementation is pure Java — no frontend or Python work involved. Clean scope.
  • The ArchUnit DSL shown in the issue is idiomatic and will work. One subtle point on Rule 2: the DSL as written uses noClasses().that().resideInAPackage("..document..").should().dependOnClassesThat().areAssignableTo(JpaRepository.class).and().resideOutsideOfPackage("..document..") — this is correct ArchUnit chaining. Confirm the .and() there actually chains the .resideOutsideOfPackage() onto the repository predicate, not onto the "no classes" clause. In some ArchUnit versions, chaining on should() clauses behaves differently from chaining on that() clauses. Test by deliberately adding a PersonRepository import in a document service and verifying the rule fires.
  • TDD cycle: The acceptance criteria explicitly require a deliberate-violation round-trip (inject a bad dependency, confirm test fails, revert). This is exactly the red-phase verification I'd want. Follow the issue's own DoD here — don't skip the deliberate break.
  • The test class name ArchitectureTest is fine and follows the existing naming convention (e.g., ApplicationContextTest, RelationshipServiceTest).
  • The issue mentions @AnalyzeClasses — ArchUnit requires this annotation on the test class to specify which packages to scan. The issue's code snippets don't show this, but it's mandatory. Example:
@AnalyzeClasses(packagesOf = FamilienarchivApplication.class)
class ArchitectureTest {
    ...
}

Without it, ArchUnit scans nothing and every rule trivially passes (false green).

Recommendations

  • Add @AnalyzeClasses(packagesOf = FamilienarchivApplication.class) to the test class — this is the most common ArchUnit mistake that causes silent false-green rules.
  • For Rule 5 (URL prefix check), if implementing the custom predicate, use DescribedPredicate<JavaAnnotation<?>> to inspect the @RequestMapping value array and check startsWith("/api/documents"). This is ~20 lines of ArchUnit custom predicate code. If it feels brittle, the TODO approach is fine — just file a follow-up issue at the time of writing the TODO so it doesn't evaporate.
  • Test method names should follow the project's should_<behavior>_when_<condition> convention: should_fail_when_controller_injects_repository_directly, should_fail_when_service_accesses_other_domain_repository, etc.
  • Since ArchUnit JUnit 5 tests run as standard @Test methods, they integrate with ./mvnw test automatically with no extra config needed beyond the dependency.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The implementation is pure Java — no frontend or Python work involved. Clean scope. - The ArchUnit DSL shown in the issue is idiomatic and will work. One subtle point on Rule 2: the DSL as written uses `noClasses().that().resideInAPackage("..document..").should().dependOnClassesThat().areAssignableTo(JpaRepository.class).and().resideOutsideOfPackage("..document..")` — this is **correct ArchUnit chaining**. Confirm the `.and()` there actually chains the `.resideOutsideOfPackage()` onto the repository predicate, not onto the "no classes" clause. In some ArchUnit versions, chaining on `should()` clauses behaves differently from chaining on `that()` clauses. Test by deliberately adding a `PersonRepository` import in a `document` service and verifying the rule fires. - **TDD cycle**: The acceptance criteria explicitly require a deliberate-violation round-trip (inject a bad dependency, confirm test fails, revert). This is exactly the red-phase verification I'd want. Follow the issue's own DoD here — don't skip the deliberate break. - The test class name `ArchitectureTest` is fine and follows the existing naming convention (e.g., `ApplicationContextTest`, `RelationshipServiceTest`). - The issue mentions `@AnalyzeClasses` — ArchUnit requires this annotation on the test class to specify which packages to scan. The issue's code snippets don't show this, but it's mandatory. Example: ```java @AnalyzeClasses(packagesOf = FamilienarchivApplication.class) class ArchitectureTest { ... } ``` Without it, ArchUnit scans nothing and every rule trivially passes (false green). ### Recommendations - Add `@AnalyzeClasses(packagesOf = FamilienarchivApplication.class)` to the test class — this is the most common ArchUnit mistake that causes silent false-green rules. - For Rule 5 (URL prefix check), if implementing the custom predicate, use `DescribedPredicate<JavaAnnotation<?>>` to inspect the `@RequestMapping` value array and check `startsWith("/api/documents")`. This is ~20 lines of ArchUnit custom predicate code. If it feels brittle, the TODO approach is fine — just file a follow-up issue at the time of writing the TODO so it doesn't evaporate. - Test method names should follow the project's `should_<behavior>_when_<condition>` convention: `should_fail_when_controller_injects_repository_directly`, `should_fail_when_service_accesses_other_domain_repository`, etc. - Since `ArchUnit JUnit 5` tests run as standard `@Test` methods, they integrate with `./mvnw test` automatically with no extra config needed beyond the dependency.
marcel commented 2026-05-04 21:04:50 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

  • This is a meta-test issue — the deliverable is the test class. That makes the acceptance criteria especially important: they must be unambiguous and self-verifying.
  • The AC includes "Deliberately introducing a violation makes the test FAIL." This is exactly right. I'd expand the deliberate-break verification: test one violation per rule, not just one overall. Four rules = four deliberate breaks, four confirmations.
  • There is no existing ArchitectureTest.java in the test tree (confirmed via grep). The shared/ test package doesn't exist yet — it will be created by this issue. The path backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java is correct per the issue spec.
  • ArchUnit tests are deterministic and stateless — no database, no Spring context, no async. They are pure Java bytecode analysis. This means they run in milliseconds and fit in the unit test layer. No Testcontainers, no @SpringBootTest.
  • One coverage gap: Rule 2 covers service-to-foreign-repository. There's no explicit rule preventing controllers calling foreign services (e.g., PersonController calling DocumentService). That would be a different kind of boundary leak. Raising as an observation for consideration, not a blocker.

Recommendations

  • Structure the test class with one @ArchTest-annotated field per rule. ArchUnit JUnit 5 supports static @ArchTest ArchRule fields that run as individual test cases — this gives per-rule failure messages in CI rather than one monolithic test failure:
@AnalyzeClasses(packagesOf = FamilienarchivApplication.class)
class ArchitectureTest {
    @ArchTest
    static final ArchRule no_controller_injects_repository = noClasses()...;

    @ArchTest
    static final ArchRule services_only_access_own_domain_repositories = ...;
}

This produces named failures like "no_controller_injects_repository" in the test report — far easier to diagnose than a single ArchitectureTest failure.

  • The DoD requires a closing comment listing each rule with a one-line "what it prevents." That doubles as acceptance evidence — include the deliberate-break results in the closing comment as a mini test report (e.g., "Rule 1: confirmed red when StatsController injects DocumentRepository directly").
  • CI time impact: ArchUnit bytecode analysis on a small project like this runs in well under 2 seconds. Zero CI time concern.
## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - This is a meta-test issue — the deliverable *is* the test class. That makes the acceptance criteria especially important: they must be unambiguous and self-verifying. - The AC includes "Deliberately introducing a violation makes the test FAIL." This is exactly right. I'd expand the deliberate-break verification: test one violation per rule, not just one overall. Four rules = four deliberate breaks, four confirmations. - There is no existing `ArchitectureTest.java` in the test tree (confirmed via grep). The `shared/` test package doesn't exist yet — it will be created by this issue. The path `backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java` is correct per the issue spec. - **ArchUnit tests are deterministic and stateless** — no database, no Spring context, no async. They are pure Java bytecode analysis. This means they run in milliseconds and fit in the unit test layer. No Testcontainers, no `@SpringBootTest`. - One coverage gap: Rule 2 covers service-to-foreign-repository. There's no explicit rule preventing **controllers calling foreign services** (e.g., `PersonController` calling `DocumentService`). That would be a different kind of boundary leak. Raising as an observation for consideration, not a blocker. ### Recommendations - Structure the test class with one `@ArchTest`-annotated field per rule. ArchUnit JUnit 5 supports static `@ArchTest ArchRule` fields that run as individual test cases — this gives per-rule failure messages in CI rather than one monolithic test failure: ```java @AnalyzeClasses(packagesOf = FamilienarchivApplication.class) class ArchitectureTest { @ArchTest static final ArchRule no_controller_injects_repository = noClasses()...; @ArchTest static final ArchRule services_only_access_own_domain_repositories = ...; } ``` This produces named failures like `"no_controller_injects_repository"` in the test report — far easier to diagnose than a single `ArchitectureTest` failure. - The DoD requires a closing comment listing each rule with a one-line "what it prevents." That doubles as acceptance evidence — include the deliberate-break results in the closing comment as a mini test report (e.g., "Rule 1: confirmed red when `StatsController` injects `DocumentRepository` directly"). - **CI time impact**: ArchUnit bytecode analysis on a small project like this runs in well under 2 seconds. Zero CI time concern.
marcel commented 2026-05-04 21:05:14 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

  • No direct security surface here — this is a structural enforcement test with no auth, no input validation, no data exposure.
  • Indirect security value: Rule 1 (no controller → repository) prevents a class of bugs where a controller accidentally bypasses the service layer and its @RequirePermission checks. If a controller calls a repository directly, the permission aspect (AOP) is not in the call path. This rule is therefore a structural enforcement of the authorization model.
  • The issue's Rule 3 (cross-cutting code in shared/) is also indirectly security-relevant: SecurityConfig must stay in a predictable, auditable location. Scattering security configuration across domain packages would make security audits significantly harder.
  • No concerns about the ArchUnit dependency itself — it is a test-scope library with no runtime footprint, no external calls, and no attack surface.

Recommendations

  • In the ArchitectureTest.java class, add a comment on Rule 1 explaining the security rationale, not just the architecture rationale:
// Rule 1: Controllers must never inject repositories directly.
// Security rationale: bypassing the service layer skips @RequirePermission
// AOP checks that are enforced on service methods.
@ArchTest
static final ArchRule no_controller_injects_repository = ...;

This makes the test self-documenting for future security audits.

  • Consider adding a Rule 6 (not in the issue) at some future point: @RequirePermission must appear on all controller methods that are not in a permit-all list. ArchUnit can enforce this. It would close the gap where a new controller method is added without any permission annotation. Flag for a follow-up issue rather than scope-creeping this one.
  • The deliberate-break verification in the AC would also serve as proof that the authorization layer cannot be bypassed via the forbidden code path — worth noting in the PR description.
## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - No direct security surface here — this is a structural enforcement test with no auth, no input validation, no data exposure. - Indirect security value: Rule 1 (no controller → repository) prevents a class of bugs where a controller accidentally bypasses the service layer and its `@RequirePermission` checks. If a controller calls a repository directly, the permission aspect (AOP) is not in the call path. This rule is therefore a **structural enforcement of the authorization model**. - The issue's Rule 3 (cross-cutting code in `shared/`) is also indirectly security-relevant: `SecurityConfig` must stay in a predictable, auditable location. Scattering security configuration across domain packages would make security audits significantly harder. - No concerns about the ArchUnit dependency itself — it is a test-scope library with no runtime footprint, no external calls, and no attack surface. ### Recommendations - In the `ArchitectureTest.java` class, add a comment on Rule 1 explaining the security rationale, not just the architecture rationale: ```java // Rule 1: Controllers must never inject repositories directly. // Security rationale: bypassing the service layer skips @RequirePermission // AOP checks that are enforced on service methods. @ArchTest static final ArchRule no_controller_injects_repository = ...; ``` This makes the test self-documenting for future security audits. - Consider adding a Rule 6 (not in the issue) at some future point: `@RequirePermission` must appear on all controller methods that are not in a permit-all list. ArchUnit can enforce this. It would close the gap where a new controller method is added without any permission annotation. Flag for a follow-up issue rather than scope-creeping this one. - The deliberate-break verification in the AC would also serve as proof that the authorization layer cannot be bypassed via the forbidden code path — worth noting in the PR description.
marcel commented 2026-05-04 21:05:40 +02:00
Author
Owner
Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • archunit-junit5:1.3.0 is a test-scoped Maven dependency. Zero Docker Compose, infrastructure, or runtime impact. Build pipeline impact: negligible (ArchUnit analysis on this codebase will run in under 2 seconds).
  • No new services, no new ports, no new volumes. Nothing to configure in Compose files.
  • The dependency integrates transparently with ./mvnw test — Gitea Actions CI picks it up with no workflow changes needed.
  • One thing to confirm: the Gitea Actions CI workflow for the backend actually runs ./mvnw test (not just ./mvnw clean package -DskipTests). If the CI workflow currently skips tests to speed up builds, ArchitectureTest will never run in CI. The AC says "runs as part of the suite" — verify this is true in the current workflow YAML.

Recommendations

  • After merging, verify in Gitea Actions that the pipeline output shows ArchitectureTest in the test results count (e.g., Tests run: 47, Failures: 0 → should become Tests run: 51, Failures: 0 if 4 rules are added as separate @ArchTest fields).
  • No CI workflow changes should be needed. If the workflow currently skips tests, that should be fixed separately — but it's outside scope here.
  • No version concern with 1.3.0 — ArchUnit follows semantic versioning and has a stable API. Renovate will handle future patch bumps automatically.
  • No concerns from my angle on deployment or infrastructure. This is a clean test-only addition.
## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - `archunit-junit5:1.3.0` is a test-scoped Maven dependency. Zero Docker Compose, infrastructure, or runtime impact. Build pipeline impact: negligible (ArchUnit analysis on this codebase will run in under 2 seconds). - No new services, no new ports, no new volumes. Nothing to configure in Compose files. - The dependency integrates transparently with `./mvnw test` — Gitea Actions CI picks it up with no workflow changes needed. - One thing to confirm: the Gitea Actions CI workflow for the backend actually runs `./mvnw test` (not just `./mvnw clean package -DskipTests`). If the CI workflow currently skips tests to speed up builds, `ArchitectureTest` will never run in CI. The AC says "runs as part of the suite" — verify this is true in the current workflow YAML. ### Recommendations - After merging, verify in Gitea Actions that the pipeline output shows `ArchitectureTest` in the test results count (e.g., `Tests run: 47, Failures: 0` → should become `Tests run: 51, Failures: 0` if 4 rules are added as separate `@ArchTest` fields). - No CI workflow changes should be needed. If the workflow currently skips tests, that should be fixed separately — but it's outside scope here. - No version concern with `1.3.0` — ArchUnit follows semantic versioning and has a stable API. Renovate will handle future patch bumps automatically. - No concerns from my angle on deployment or infrastructure. This is a clean test-only addition.
marcel commented 2026-05-04 21:06:06 +02:00
Author
Owner
Copy Link

📋 Elicit — Requirements Engineer

Observations

  • The acceptance criteria are well-formed and testable. Each criterion is observable and verifiable. The DoD (closing comment with per-rule explanations) adds a useful audit trail.
  • One ambiguity in AC bullet 2: "Rules 1, 2, 3, 4 are implemented and pass on the current domain-based codebase (post REFACTOR-1 / #407)." This is under-specified. It says "post REFACTOR-1" but the hard dependency is listed separately. There are two interpretations: (a) these rules are written against the post-refactor package structure, so they can only be verified after #407 is merged — or (b) they should pass on the codebase at the time this issue is worked. These are different preconditions. Given the current codebase is still flat-packaged and StatsController already violates Rule 1, interpretation (a) is clearly intended. The AC should say this explicitly.
  • AC gap: There is no criterion confirming that Rule 5 is either implemented or the follow-up issue is created. "Rule 5 is either implemented or has a documented TODO with a follow-up issue" is stated but doesn't appear in the checkbox list — easy to overlook.
  • The issue is well-labeled (P2-medium, legibility, refactor) and correctly milestoned (Codebase Legibility). No label issues.

Recommendations

  • Rewrite AC bullet 2 to be unambiguous:
    • Before: "Rules 1, 2, 3, 4 are implemented and pass on the current domain-based codebase (post REFACTOR-1 / #407)"
    • After: "Rules 1, 2, 3, 4 are implemented and pass on the post-#407 package structure. This issue must not be merged before #407."
  • Add an explicit AC checkbox: [ ] If Rule 5 is not implemented, a follow-up Gitea issue is created and linked here.
  • The "Dispatch" note ("best done by an agent comfortable with ArchUnit DSL") is good agent guidance. The deliberate-break verification round-trip is a non-negotiable quality signal — keep it in the DoD.
  • No scope creep concerns. This issue is tightly scoped and correctly sized (S/M).
## 📋 Elicit — Requirements Engineer ### Observations - The acceptance criteria are well-formed and testable. Each criterion is observable and verifiable. The DoD (closing comment with per-rule explanations) adds a useful audit trail. - **One ambiguity in AC bullet 2**: "Rules 1, 2, 3, 4 are implemented and **pass on the current domain-based codebase (post REFACTOR-1 / #407)**." This is under-specified. It says "post REFACTOR-1" but the hard dependency is listed separately. There are two interpretations: (a) these rules are written against the post-refactor package structure, so they can only be verified after #407 is merged — or (b) they should pass on the codebase at the time this issue is worked. These are different preconditions. Given the current codebase is still flat-packaged and `StatsController` already violates Rule 1, interpretation (a) is clearly intended. The AC should say this explicitly. - **AC gap**: There is no criterion confirming that Rule 5 is either implemented or the follow-up issue is created. "Rule 5 is either implemented or has a documented TODO with a follow-up issue" is stated but doesn't appear in the checkbox list — easy to overlook. - The issue is well-labeled (P2-medium, legibility, refactor) and correctly milestoned (Codebase Legibility). No label issues. ### Recommendations - Rewrite AC bullet 2 to be unambiguous: - **Before**: "Rules 1, 2, 3, 4 are implemented and pass on the current domain-based codebase (post REFACTOR-1 / #407)" - **After**: "Rules 1, 2, 3, 4 are implemented and pass on the **post-#407 package structure**. This issue **must not be merged before #407**." - Add an explicit AC checkbox: `[ ] If Rule 5 is not implemented, a follow-up Gitea issue is created and linked here.` - The "Dispatch" note ("best done by an agent comfortable with ArchUnit DSL") is good agent guidance. The deliberate-break verification round-trip is a non-negotiable quality signal — keep it in the DoD. - No scope creep concerns. This issue is tightly scoped and correctly sized (S/M).
marcel commented 2026-05-04 21:06:34 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

No concerns from my angle. This issue is purely a backend test infrastructure task with no user-facing UI components, no frontend changes, and no accessibility surface. I checked: there are no new Svelte components, no new API endpoints, and no changes to any route that would affect the UI.

The only thing I'd note for completeness: the ArchitectureTest closing comment (required by the DoD) should use plain English descriptions that non-architect team members can also read. Avoid opaque shorthand like "prevents C6.1/C6.2 violations" without explaining what that means in one plain sentence per rule.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist No concerns from my angle. This issue is purely a backend test infrastructure task with no user-facing UI components, no frontend changes, and no accessibility surface. I checked: there are no new Svelte components, no new API endpoints, and no changes to any route that would affect the UI. The only thing I'd note for completeness: the `ArchitectureTest` closing comment (required by the DoD) should use plain English descriptions that non-architect team members can also read. Avoid opaque shorthand like "prevents C6.1/C6.2 violations" without explaining what that means in one plain sentence per rule.
marcel commented 2026-05-04 21:07:10 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Merge Order / Dependency

  • Enforce the hard dependency on #407 before merging #409 — Rules 2, 3, and 4 reference domain-based package paths (..document.., ..person.., ..shared..) that don't exist in the current flat-layer structure. The rules will either fail to compile or trivially pass green on the pre-refactor codebase. Options: (a) implement the rules now on a branch that is rebased on top of #407's branch — merge together, (b) implement now with all rules @Disabled and a @Tag("architecture") guard, enabled only after #407 merges. Option (a) is cleaner and avoids a guarded-but-inactive test class landing in main. (Raised by: Markus, Elicit)

Rule 5 — URL Prefix Enforcement

  • Implement Rule 5 or file a follow-up issue — the issue says "accept a TODO if too brittle," but doesn't require a linked follow-up issue. If Rule 5 is skipped now, it should become its own Gitea issue (linked from the TODO comment) so it doesn't silently disappear. The options: (a) implement the custom DescribedPredicate now (~20 lines), giving full enforcement from day one, or (b) write a TODO comment in ArchitectureTest.java and open a follow-up issue immediately. Either is acceptable — the key is that the work is tracked. (Raised by: Felix, Elicit)
## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Merge Order / Dependency - **Enforce the hard dependency on #407 before merging #409** — Rules 2, 3, and 4 reference domain-based package paths (`..document..`, `..person..`, `..shared..`) that don't exist in the current flat-layer structure. The rules will either fail to compile or trivially pass green on the pre-refactor codebase. Options: (a) implement the rules now on a branch that is rebased on top of #407's branch — merge together, (b) implement now with all rules `@Disabled` and a `@Tag("architecture")` guard, enabled only after #407 merges. Option (a) is cleaner and avoids a guarded-but-inactive test class landing in main. _(Raised by: Markus, Elicit)_ ### Rule 5 — URL Prefix Enforcement - **Implement Rule 5 or file a follow-up issue** — the issue says "accept a TODO if too brittle," but doesn't require a linked follow-up issue. If Rule 5 is skipped now, it should become its own Gitea issue (linked from the TODO comment) so it doesn't silently disappear. The options: (a) implement the custom `DescribedPredicate` now (~20 lines), giving full enforcement from day one, or (b) write a TODO comment in `ArchitectureTest.java` and open a follow-up issue immediately. Either is acceptable — the key is that the work is tracked. _(Raised by: Felix, Elicit)_
marcel commented 2026-05-05 17:17:45 +02:00
Author
Owner
Copy Link

Implementation complete — PR #428

All four rules are live in backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java.

What each rule prevents

Rule @ArchTest field What it prevents
1 no_controller_injects_repository_directly A @RestController bypassing the service layer to call a repository directly — which would skip @RequirePermission AOP checks and expose raw data access without authorization
2 services_only_access_own_domain_repositories_{domain} A @Service in domain X reaching into domain Y's repository — cross-domain coupling that should go through domain Y's service API instead
3 no_configuration_class_in_domain_packages A @Configuration class (infrastructure/framework wiring) being scattered into a domain package instead of staying in config/ or security/ where it can be audited
4 entities_reside_in_domain_packages An @Entity class being placed outside a domain package — regression to flat layer-based packaging that was deliberately removed in #407

Rule 5 status

Deferred. TODO comment in ArchitectureTest.java references #427, which contains the spec, the DescribedPredicate implementation sketch, and the acceptance criteria.

Deliberate-break evidence

Each rule was verified with a violation round-trip before committing:

  • Rule 1 RED: added private final TagRepository tagRepository field to TagController → no_controller_injects_repository_directly fired
  • Rule 2 RED: added private final PersonRepository personRepository field to TagService → services_only_access_own_domain_repositories_tag fired
  • Rule 3 RED: created DocumentSomeConfig.java with @Configuration in document/ → no_configuration_class_in_domain_packages fired
  • Rule 4 RED: created SomeEntity.java with @Entity in config/ → entities_reside_in_domain_packages fired

Test count

Baseline: 1503 tests. After: 1514 tests (+11 ArchUnit rules), 0 failures.

Commits

  • 22ec808b — test(backend): add ArchUnit domain boundary enforcement (Rules 1–4)
## Implementation complete — PR #428 All four rules are live in `backend/src/test/java/org/raddatz/familienarchiv/shared/ArchitectureTest.java`. ### What each rule prevents | Rule | `@ArchTest` field | What it prevents | |------|-------------------|-----------------| | 1 | `no_controller_injects_repository_directly` | A `@RestController` bypassing the service layer to call a repository directly — which would skip `@RequirePermission` AOP checks and expose raw data access without authorization | | 2 | `services_only_access_own_domain_repositories_{domain}` | A `@Service` in domain X reaching into domain Y's repository — cross-domain coupling that should go through domain Y's service API instead | | 3 | `no_configuration_class_in_domain_packages` | A `@Configuration` class (infrastructure/framework wiring) being scattered into a domain package instead of staying in `config/` or `security/` where it can be audited | | 4 | `entities_reside_in_domain_packages` | An `@Entity` class being placed outside a domain package — regression to flat layer-based packaging that was deliberately removed in #407 | ### Rule 5 status Deferred. TODO comment in `ArchitectureTest.java` references **#427**, which contains the spec, the DescribedPredicate implementation sketch, and the acceptance criteria. ### Deliberate-break evidence Each rule was verified with a violation round-trip before committing: - **Rule 1** RED: added `private final TagRepository tagRepository` field to `TagController` → `no_controller_injects_repository_directly` fired - **Rule 2** RED: added `private final PersonRepository personRepository` field to `TagService` → `services_only_access_own_domain_repositories_tag` fired - **Rule 3** RED: created `DocumentSomeConfig.java` with `@Configuration` in `document/` → `no_configuration_class_in_domain_packages` fired - **Rule 4** RED: created `SomeEntity.java` with `@Entity` in `config/` → `entities_reside_in_domain_packages` fired ### Test count Baseline: 1503 tests. After: **1514 tests** (+11 ArchUnit rules), 0 failures. ### Commits - `22ec808b` — `test(backend): add ArchUnit domain boundary enforcement (Rules 1–4)`
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label P2-medium legibility refactor
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#409
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?