test(backend): add ArchUnit domain boundary enforcement (Rules 1–4) #428

Merged

marcel merged 3 commits from feat/issue-409-archunit into main 2026-05-05 18:08:40 +02:00

Conversation 8 Commits 3 Files Changed 2 +153

marcel commented 2026-05-05 17:17:27 +02:00

Owner

Copy Link

Summary

Closes #409. Adds ArchitectureTest.java with four ArchUnit rules that fail the build if domain boundaries are violated in the future.

• Rule 1 — no @RestController may inject a JpaRepository directly; preserves @RequirePermission AOP enforcement on the service layer
• Rule 2 — @Service classes access only their own domain's repositories, never a foreign domain's (enforced per-domain: document, person, tag, user, dashboard, geschichte, notification, ocr)
• Rule 3 — no @Configuration class (except @SpringBootApplication) may reside in a domain package; keeps infrastructure config auditable in config/ and security/
• Rule 4 — all @Entity classes reside in a named domain package; prevents regression to flat layer-based packaging
• Rule 5 — deferred; TODO comment references #427

Each rule was verified with a deliberate-break round-trip: violation introduced → test RED → violation reverted → test GREEN.

⚠️ Merge order: this branch includes the domain-packaging commits from #407. Merge after #407 and #408 land, or rebase onto the merged main.

Test plan

• ./mvnw test passes: 1514 tests, 0 failures
• Rule 1 RED: injecting TagRepository field into TagController → no_controller_injects_repository_directly fires
• Rule 2 RED: injecting PersonRepository into TagService → services_only_access_own_domain_repositories_tag fires
• Rule 3 RED: adding @Configuration class in document/ package → no_configuration_class_in_domain_packages fires
• Rule 4 RED: adding @Entity class in config/ package → entities_reside_in_domain_packages fires

🤖 Generated with Claude Code

## Summary Closes #409. Adds `ArchitectureTest.java` with four ArchUnit rules that fail the build if domain boundaries are violated in the future. - **Rule 1** — no `@RestController` may inject a `JpaRepository` directly; preserves `@RequirePermission` AOP enforcement on the service layer - **Rule 2** — `@Service` classes access only their own domain's repositories, never a foreign domain's (enforced per-domain: document, person, tag, user, dashboard, geschichte, notification, ocr) - **Rule 3** — no `@Configuration` class (except `@SpringBootApplication`) may reside in a domain package; keeps infrastructure config auditable in `config/` and `security/` - **Rule 4** — all `@Entity` classes reside in a named domain package; prevents regression to flat layer-based packaging - **Rule 5** — deferred; TODO comment references #427 Each rule was verified with a deliberate-break round-trip: violation introduced → test RED → violation reverted → test GREEN. ⚠️ **Merge order**: this branch includes the domain-packaging commits from #407. Merge after #407 and #408 land, or rebase onto the merged main. ## Test plan - [x] `./mvnw test` passes: 1514 tests, 0 failures - [x] Rule 1 RED: injecting `TagRepository` field into `TagController` → `no_controller_injects_repository_directly` fires - [x] Rule 2 RED: injecting `PersonRepository` into `TagService` → `services_only_access_own_domain_repositories_tag` fires - [x] Rule 3 RED: adding `@Configuration` class in `document/` package → `no_configuration_class_in_domain_packages` fires - [x] Rule 4 RED: adding `@Entity` class in `config/` package → `entities_reside_in_domain_packages` fires 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-05-05 17:17:28 +02:00

test(backend): add ArchUnit domain boundary enforcement (Rules 1–4) ... 22ec808b2d

Rules enforced:
- Rule 1: no @RestController may inject a JpaRepository directly (preserves @RequirePermission AOP enforcement)
- Rule 2: @Service classes access only their own domain's repositories, never a foreign domain's
- Rule 3: no @Configuration class (except @SpringBootApplication) in domain packages
- Rule 4: all @Entity classes reside in a domain package

Rule 5 (URL prefix per controller domain) deferred — tracked in #427.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this pull request 2026-05-05 17:17:45 +02:00

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

marcel commented 2026-05-05 17:48:44 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller (@mkeller) — Application Architect

Verdict: ✅ Approved

This is exactly the kind of structural safeguard that pays for itself on the first regression it prevents. ArchUnit enforces what ADRs can only document — and that is the right trade-off. Four rules, clearly justified, verifiably working (RED/GREEN round-trip documented in the PR). Good call.

Blockers

None.

Suggestions

1. importing is missing from Rule 3's domain list (L97–103)

MassImportService lives in ..importing.., but the domain exclusion list in no_configuration_class_in_domain_packages does not include ..importing... The rule currently says: a @Configuration class in importing/ would NOT fire. That is likely a gap — if someone drops a @Configuration there it goes undetected.

// Current
.should().resideInAnyPackage(
    "..document..", "..person..", "..tag..",
    "..geschichte..", "..notification..", "..ocr..",
    "..filestorage..", "..importing..", "..dashboard..", "..audit.."
);

Wait — re-reading: importing IS in the list. Rule 3's should().resideInAnyPackage(...) is a negation (noClasses().that()...should().resideInAnyPackage(...)) — it fires if a @Configuration does reside in those packages. So importing is already covered. Good.

2. Rule 3 word order reads backwards — suggest a comment clarification

The rule reads: no @Configuration (except @SpringBootApplication) should reside in any of the listed packages. The logic is correct (noClasses...should().resideInAnyPackage()), but a new reader might expect the should clause to list the allowed packages (like Rule 4 does). A one-liner noting this is a prohibition, not an allowlist, would save confusion. Minor.

3. importing and audit are omitted from Rule 2's per-domain coverage

Rule 2 generates one @ArchTest field per domain. The importing and audit packages have services (MassImportService, presumably some audit service), but they have no corresponding rule. If MassImportService ever injects a repository from document/, Rule 2 will not catch it. Worth adding entries for at least importing — audit may legitimately cross boundaries as a cross-cutting concern, so that one is debatable.

4. Consider a single parameterized test over 8 near-identical fields (suggestion, not blocker)

Eight static final ArchRule services_only_access_own_domain_repositories_* fields with identical structure signal a missed abstraction. ArchUnit supports @ArchTests collections — a List<ArchRule> helper factory keeps the intent clear and is easier to extend when a new domain is added:

private static List<ArchRule> domainRepositoryRules() {
    return List.of("document", "person", "tag", "user", "dashboard", "geschichte", "notification", "ocr")
        .stream()
        .map(domain -> noClasses()
            .that().areAnnotatedWith(Service.class)
            .and().resideInAPackage(".." + domain + "..")
            .should().dependOnClassesThat(foreignJpaRepositoryFor(domain))
            .as("services_only_access_own_domain_repositories_" + domain))
        .toList();
}

This is a KISS trade-off and the current verbosity is defensible for readability — but worth a discussion when adding domain 9+.

5. Rule 5 deferral is well-reasoned

The TODO comment is precise: it names the problem (inspecting annotation values is brittle), links to the tracking issue (#427), and is in the test file where a future implementer will find it. This is the right way to defer work.

Overall: solid implementation of a long-overdue structural guard. Merge after #407/#408 as noted.

## 🏗️ Markus Keller (@mkeller) — Application Architect **Verdict: ✅ Approved** This is exactly the kind of structural safeguard that pays for itself on the first regression it prevents. ArchUnit enforces what ADRs can only document — and that is the right trade-off. Four rules, clearly justified, verifiably working (RED/GREEN round-trip documented in the PR). Good call. ### Blockers None. ### Suggestions **1. `importing` is missing from Rule 3's domain list (L97–103)** `MassImportService` lives in `..importing..`, but the domain exclusion list in `no_configuration_class_in_domain_packages` does not include `..importing..`. The rule currently says: a `@Configuration` class in `importing/` would NOT fire. That is likely a gap — if someone drops a `@Configuration` there it goes undetected. ```java // Current .should().resideInAnyPackage( "..document..", "..person..", "..tag..", "..geschichte..", "..notification..", "..ocr..", "..filestorage..", "..importing..", "..dashboard..", "..audit.." ); ``` Wait — re-reading: `importing` IS in the list. Rule 3's `should().resideInAnyPackage(...)` is a negation (`noClasses().that()...should().resideInAnyPackage(...)`) — it fires if a `@Configuration` *does* reside in those packages. So `importing` is already covered. Good. **2. Rule 3 word order reads backwards — suggest a comment clarification** The rule reads: *no `@Configuration` (except `@SpringBootApplication`) should reside in any of the listed packages*. The logic is correct (`noClasses...should().resideInAnyPackage()`), but a new reader might expect the `should` clause to list the *allowed* packages (like Rule 4 does). A one-liner noting this is a prohibition, not an allowlist, would save confusion. Minor. **3. `importing` and `audit` are omitted from Rule 2's per-domain coverage** Rule 2 generates one `@ArchTest` field per domain. The `importing` and `audit` packages have services (`MassImportService`, presumably some audit service), but they have no corresponding rule. If `MassImportService` ever injects a repository from `document/`, Rule 2 will not catch it. Worth adding entries for at least `importing` — `audit` may legitimately cross boundaries as a cross-cutting concern, so that one is debatable. **4. Consider a single parameterized test over 8 near-identical fields (suggestion, not blocker)** Eight `static final ArchRule services_only_access_own_domain_repositories_*` fields with identical structure signal a missed abstraction. ArchUnit supports `@ArchTests` collections — a `List<ArchRule>` helper factory keeps the intent clear and is easier to extend when a new domain is added: ```java private static List<ArchRule> domainRepositoryRules() { return List.of("document", "person", "tag", "user", "dashboard", "geschichte", "notification", "ocr") .stream() .map(domain -> noClasses() .that().areAnnotatedWith(Service.class) .and().resideInAPackage(".." + domain + "..") .should().dependOnClassesThat(foreignJpaRepositoryFor(domain)) .as("services_only_access_own_domain_repositories_" + domain)) .toList(); } ``` This is a KISS trade-off and the current verbosity is defensible for readability — but worth a discussion when adding domain 9+. **5. Rule 5 deferral is well-reasoned** The TODO comment is precise: it names the problem (inspecting annotation values is brittle), links to the tracking issue (#427), and is in the test file where a future implementer will find it. This is the right way to defer work. Overall: solid implementation of a long-overdue structural guard. Merge after #407/#408 as noted.

marcel commented 2026-05-05 17:49:05 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

The rules are correct and the comments explain the why — that is exactly what I want to see. But there are two clean-code concerns I cannot let slide quietly, and one pattern that will silently miss violations.

Blockers

1. foreignJpaRepositoryFor() has a subtle string-matching bug that can produce false negatives

ArchitectureTest.java line 118–123:

public boolean test(JavaClass clazz) {
    return clazz.isAssignableTo(JpaRepository.class)
           && !clazz.getPackageName().contains("." + ownDomain);
}

The check getPackageName().contains("." + ownDomain) matches if the string ".document" appears anywhere in the fully-qualified package name. It works for direct subdomain repos (e.g., org.raddatz.familienarchiv.document.DocumentRepository). But it would silently pass (not fire) for a hypothetical future domain whose name contains another domain name as a substring — for example, if someone created document_type or documentmeta domain and the check against "document" accidentally matched it. This is low probability today but the predicate is not as tight as it could be.

A safer check:

return clazz.isAssignableTo(JpaRepository.class)
       && !clazz.getPackageName().matches(".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$");

Or use ArchUnit's built-in resideInAPackage(".."+ownDomain+"..") on the class directly rather than a custom predicate. I'd call this a blocker because it is the correctness foundation of Rule 2.

Suggestions

2. Eight near-identical @ArchTest fields — apply DRY

ArchitectureTest.java lines 36–95: eight static final ArchRule services_only_access_own_domain_repositories_* fields with structurally identical bodies. If a ninth domain is added, it is easy to forget the test. Each field is 6 lines; the whole block is 48 lines of repetition.

ArchUnit's @ArchTests-annotated class or a static factory returning a collection would reduce this to a single call site. The rule descriptions can still be domain-specific via .as("services_only_access_own_domain_repositories_" + domain).

3. Package placement for the test

ArchitectureTest.java lives in org.raddatz.familienarchiv.shared. The production code has no shared package — this is a test-only namespace. That is fine, but naming it architecture (matching the file name) would be more self-documenting:

backend/src/test/java/org/raddatz/familienarchiv/architecture/ArchitectureTest.java

Minor — no behavioral impact, but shared is ambiguous (shared with what?).

4. The DescribedPredicate anonymous inner class could be a lambda

ArchUnit 1.3.0 supports lambdas for DescribedPredicate. The anonymous inner class at lines 114–121 works but is more verbose than necessary:

private static DescribedPredicate<JavaClass> foreignJpaRepositoryFor(String ownDomain) {
    return DescribedPredicate.describe(
        "be a JPA repository from a domain other than " + ownDomain,
        clazz -> clazz.isAssignableTo(JpaRepository.class)
                 && !clazz.getPackageName().contains("." + ownDomain)
    );
}

This is a suggestion only — the anonymous class is valid and readable.

## 👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** The rules are correct and the comments explain the *why* — that is exactly what I want to see. But there are two clean-code concerns I cannot let slide quietly, and one pattern that will silently miss violations. ### Blockers **1. `foreignJpaRepositoryFor()` has a subtle string-matching bug that can produce false negatives** `ArchitectureTest.java` line 118–123: ```java public boolean test(JavaClass clazz) { return clazz.isAssignableTo(JpaRepository.class) && !clazz.getPackageName().contains("." + ownDomain); } ``` The check `getPackageName().contains("." + ownDomain)` matches if the string `".document"` appears anywhere in the fully-qualified package name. It works for direct subdomain repos (e.g., `org.raddatz.familienarchiv.document.DocumentRepository`). But it would silently pass (not fire) for a hypothetical future domain whose name contains another domain name as a substring — for example, if someone created `document_type` or `documentmeta` domain and the check against `"document"` accidentally matched it. This is low probability today but the predicate is not as tight as it could be. A safer check: ```java return clazz.isAssignableTo(JpaRepository.class) && !clazz.getPackageName().matches(".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$"); ``` Or use ArchUnit's built-in `resideInAPackage(".."+ownDomain+"..")` on the class directly rather than a custom predicate. I'd call this a blocker because it is the correctness foundation of Rule 2. ### Suggestions **2. Eight near-identical `@ArchTest` fields — apply DRY** `ArchitectureTest.java` lines 36–95: eight `static final ArchRule services_only_access_own_domain_repositories_*` fields with structurally identical bodies. If a ninth domain is added, it is easy to forget the test. Each field is 6 lines; the whole block is 48 lines of repetition. ArchUnit's `@ArchTests`-annotated class or a static factory returning a collection would reduce this to a single call site. The rule descriptions can still be domain-specific via `.as("services_only_access_own_domain_repositories_" + domain)`. **3. Package placement for the test** `ArchitectureTest.java` lives in `org.raddatz.familienarchiv.shared`. The production code has no `shared` package — this is a test-only namespace. That is fine, but naming it `architecture` (matching the file name) would be more self-documenting: ``` backend/src/test/java/org/raddatz/familienarchiv/architecture/ArchitectureTest.java ``` Minor — no behavioral impact, but `shared` is ambiguous (shared with what?). **4. The `DescribedPredicate` anonymous inner class could be a lambda** ArchUnit 1.3.0 supports lambdas for `DescribedPredicate`. The anonymous inner class at lines 114–121 works but is more verbose than necessary: ```java private static DescribedPredicate<JavaClass> foreignJpaRepositoryFor(String ownDomain) { return DescribedPredicate.describe( "be a JPA repository from a domain other than " + ownDomain, clazz -> clazz.isAssignableTo(JpaRepository.class) && !clazz.getPackageName().contains("." + ownDomain) ); } ``` This is a suggestion only — the anonymous class is valid and readable.

marcel commented 2026-05-05 17:49:13 +02:00

Author

Owner

Copy Link

🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Verdict: ✅ Approved

Two-file PR, both test-scoped. No changes to docker-compose.yml, no new services, no new environment variables, no infrastructure impact whatsoever. My checklist is short.

Blockers

None.

Suggestions

1. ArchUnit version is pinned — good. Check if it is in BOM.

pom.xml line 115: <version>1.3.0</version> is explicitly pinned, which is correct practice. However, Spring Boot 4's parent BOM does not yet manage ArchUnit (it is not a Spring-ecosystem artifact), so a manual version pin is the right call here. Just make sure Renovate (if configured) has visibility into this <version> tag so it will create a bump PR when 1.4.x or 2.x lands. If there is a renovate.json in the repo root, verify it covers com.tngtech.archunit — ArchUnit publishes frequently.

2. CI time impact is negligible

ArchUnit bytecode analysis on a codebase this size typically adds 2–5 seconds to the test run. The PR description says 1514 tests pass — that is well within the <10 seconds unit test budget. No concern there.

3. The <scope>test</scope> on the dependency is correct

No accidental production classpath bloat. ArchUnit only runs during ./mvnw test / ./mvnw package — it will not be in the deployed JAR. Confirmed.

Overall: clean infrastructure footprint. Ship it after the domain PRs land.

## 🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer **Verdict: ✅ Approved** Two-file PR, both test-scoped. No changes to `docker-compose.yml`, no new services, no new environment variables, no infrastructure impact whatsoever. My checklist is short. ### Blockers None. ### Suggestions **1. ArchUnit version is pinned — good. Check if it is in BOM.** `pom.xml` line 115: `<version>1.3.0</version>` is explicitly pinned, which is correct practice. However, Spring Boot 4's parent BOM does not yet manage ArchUnit (it is not a Spring-ecosystem artifact), so a manual version pin is the right call here. Just make sure Renovate (if configured) has visibility into this `<version>` tag so it will create a bump PR when 1.4.x or 2.x lands. If there is a `renovate.json` in the repo root, verify it covers `com.tngtech.archunit` — ArchUnit publishes frequently. **2. CI time impact is negligible** ArchUnit bytecode analysis on a codebase this size typically adds 2–5 seconds to the test run. The PR description says 1514 tests pass — that is well within the `<10 seconds` unit test budget. No concern there. **3. The `<scope>test</scope>` on the dependency is correct** No accidental production classpath bloat. ArchUnit only runs during `./mvnw test` / `./mvnw package` — it will not be in the deployed JAR. Confirmed. Overall: clean infrastructure footprint. Ship it after the domain PRs land.

marcel commented 2026-05-05 17:49:30 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

This PR closes issue #409. I evaluated whether the delivered rules match the stated need (preventing domain boundary regressions) and whether anything relevant was left out or ambiguously specified.

Blockers

None.

Observations

1. Requirement coverage — Rules 1–4 are verifiable and map to stated goals

Each rule has a stated rationale, a test name, and per the PR description was verified with a deliberate RED test. The acceptance criteria from issue #409 appear to be met. Rule 5 is explicitly deferred with a tracked issue reference (#427) — that is a clean deferral, not a silent gap.

2. The importing domain has no Rule 2 coverage

MassImportService lives in ..importing... No services_only_access_own_domain_repositories_importing rule exists. If the business requirement is "no service reaches a foreign repository," then MassImportService is currently ungoverned by Rule 2. This may be intentional (mass import legitimately delegates to other services), but it is an undocumented gap.

Recommendation: Add a comment or a tracking note explaining why importing is excluded from Rule 2. If it is intentional, documenting it prevents a future maintainer from adding the rule unnecessarily — or worse, assuming it was forgotten and adding it when it shouldn't be.

3. The audit domain's omission from Rule 2 is also undocumented

Similarly, cross-cutting concerns like audit logging often need access to multiple domains. If AuditService is intentionally exempt from the domain-isolation constraint, a comment in the test saying so would make that decision visible to future reviewers.

4. Rule 2's per-domain enumeration creates a "new domain tax"

When a new domain is added (e.g., export/, sharing/), a developer must remember to add a new @ArchTest field. There is no mechanism to enforce that the list stays complete. A self-maintaining rule (e.g., derived from scanning all @Service packages at test time) would be more robust — but may be out of scope for this PR. Worth tracking as a follow-up.

5. The merge-order note in the PR description is a process risk

The PR description states: "Merge after #407 and #408 land." This is a manual dependency that could be missed. If Gitea supports blocking merges on PR dependencies, that would be safer than a prose note. If not, this is an acceptable risk for a small team.

Overall: requirements are met for the stated scope. The gaps around importing and audit domain coverage in Rule 2 are worth a clarifying comment at minimum.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** This PR closes issue #409. I evaluated whether the delivered rules match the stated need (preventing domain boundary regressions) and whether anything relevant was left out or ambiguously specified. ### Blockers None. ### Observations **1. Requirement coverage — Rules 1–4 are verifiable and map to stated goals** Each rule has a stated rationale, a test name, and per the PR description was verified with a deliberate RED test. The acceptance criteria from issue #409 appear to be met. Rule 5 is explicitly deferred with a tracked issue reference (#427) — that is a clean deferral, not a silent gap. **2. The `importing` domain has no Rule 2 coverage** `MassImportService` lives in `..importing..`. No `services_only_access_own_domain_repositories_importing` rule exists. If the business requirement is "no service reaches a foreign repository," then `MassImportService` is currently ungoverned by Rule 2. This may be intentional (mass import legitimately delegates to other services), but it is an undocumented gap. **Recommendation**: Add a comment or a tracking note explaining why `importing` is excluded from Rule 2. If it is intentional, documenting it prevents a future maintainer from adding the rule unnecessarily — or worse, assuming it was forgotten and adding it when it shouldn't be. **3. The `audit` domain's omission from Rule 2 is also undocumented** Similarly, cross-cutting concerns like audit logging often need access to multiple domains. If `AuditService` is intentionally exempt from the domain-isolation constraint, a comment in the test saying so would make that decision visible to future reviewers. **4. Rule 2's per-domain enumeration creates a "new domain tax"** When a new domain is added (e.g., `export/`, `sharing/`), a developer must remember to add a new `@ArchTest` field. There is no mechanism to enforce that the list stays complete. A self-maintaining rule (e.g., derived from scanning all `@Service` packages at test time) would be more robust — but may be out of scope for this PR. Worth tracking as a follow-up. **5. The merge-order note in the PR description is a process risk** The PR description states: *"Merge after #407 and #408 land."* This is a manual dependency that could be missed. If Gitea supports blocking merges on PR dependencies, that would be safer than a prose note. If not, this is an acceptable risk for a small team. Overall: requirements are met for the stated scope. The gaps around `importing` and `audit` domain coverage in Rule 2 are worth a clarifying comment at minimum.

marcel commented 2026-05-05 17:49:49 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR is a security control, not just a code quality tool. Rule 1 directly prevents bypassing @RequirePermission AOP enforcement — that is a meaningful defense against a real attack class (privilege escalation via repository injection). I want to confirm the rule is watertight.

Blockers

None. But read the two observations carefully.

Security Analysis

1. Rule 1 correctly targets the @RequirePermission bypass vector

The rationale in the code comment is accurate:

// Security rationale: bypassing the service layer skips @RequirePermission
// AOP checks that are enforced on service methods.

AOP interception via Spring's proxy mechanism only fires when method calls pass through the proxy. A @RestController that injects a JpaRepository directly can read or write data without ever touching the permission check. Rule 1 makes this architectural pattern compile-fail. This is exactly the right layer to enforce it.

2. Rule 1 only catches field-level dependency (dependOnClassesThat) — does it cover indirect access?

noClasses().that().areAnnotatedWith(RestController.class).should().dependOnClassesThat().areAssignableTo(JpaRepository.class) fires when the controller compiles against a JpaRepository subtype — which covers field injection, method parameters, and local variable declarations. It does not cover a controller calling a static method that internally accesses the repo without declaring it as a dependency. That edge case is highly unlikely in a Spring project (Spring repositories are not statics), but worth knowing the scope.

3. Rule 2 provides a second defense layer for cross-domain data leaks

Cross-domain repository access could allow a service to silently read data it has no business reason to access — a form of insecure direct object reference at the service layer. Rule 2 catches this at compile time. Good defense in depth.

4. The foreignJpaRepositoryFor predicate uses string substring matching

!clazz.getPackageName().contains("." + ownDomain) — as Felix also flagged, this is a correctness concern. From a security perspective: if the predicate produces a false negative (incorrectly returns false when it should return true), Rule 2 silently fails to fire on a real violation. A more precise package match (exact segment match rather than substring) would make this a tighter security control.

Recommended fix (same as Felix's):

// Use ArchUnit's built-in package matcher for precision
return clazz.isAssignableTo(JpaRepository.class)
       && !clazz.getPackageName().matches(".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$");

5. No security regression tests needed for this PR

ArchUnit tests are themselves the regression tests. The RED/GREEN verification round-trips documented in the PR description confirm they fire on real violations. No additional security tests are warranted.

Overall security posture: this is a structural control that prevents future developers from accidentally weakening the permission enforcement boundary. It is the right tool at the right layer. The substring matching concern in the predicate is worth fixing — it is the only thing standing between "rule fires" and "rule silently misses a violation."

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR is a security control, not just a code quality tool. Rule 1 directly prevents bypassing `@RequirePermission` AOP enforcement — that is a meaningful defense against a real attack class (privilege escalation via repository injection). I want to confirm the rule is watertight. ### Blockers None. But read the two observations carefully. ### Security Analysis **1. Rule 1 correctly targets the `@RequirePermission` bypass vector** The rationale in the code comment is accurate: ```java // Security rationale: bypassing the service layer skips @RequirePermission // AOP checks that are enforced on service methods. ``` AOP interception via Spring's proxy mechanism only fires when method calls pass through the proxy. A `@RestController` that injects a `JpaRepository` directly can read or write data without ever touching the permission check. Rule 1 makes this architectural pattern compile-fail. This is exactly the right layer to enforce it. **2. Rule 1 only catches field-level dependency (`dependOnClassesThat`) — does it cover indirect access?** `noClasses().that().areAnnotatedWith(RestController.class).should().dependOnClassesThat().areAssignableTo(JpaRepository.class)` fires when the controller *compiles against* a JpaRepository subtype — which covers field injection, method parameters, and local variable declarations. It does not cover a controller calling a static method that internally accesses the repo without declaring it as a dependency. That edge case is highly unlikely in a Spring project (Spring repositories are not statics), but worth knowing the scope. **3. Rule 2 provides a second defense layer for cross-domain data leaks** Cross-domain repository access could allow a service to silently read data it has no business reason to access — a form of insecure direct object reference at the service layer. Rule 2 catches this at compile time. Good defense in depth. **4. The `foreignJpaRepositoryFor` predicate uses string substring matching** `!clazz.getPackageName().contains("." + ownDomain)` — as Felix also flagged, this is a correctness concern. From a security perspective: if the predicate produces a false negative (incorrectly returns `false` when it should return `true`), Rule 2 silently fails to fire on a real violation. A more precise package match (exact segment match rather than substring) would make this a tighter security control. **Recommended fix** (same as Felix's): ```java // Use ArchUnit's built-in package matcher for precision return clazz.isAssignableTo(JpaRepository.class) && !clazz.getPackageName().matches(".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$"); ``` **5. No security regression tests needed for this PR** ArchUnit tests are themselves the regression tests. The RED/GREEN verification round-trips documented in the PR description confirm they fire on real violations. No additional security tests are warranted. **Overall security posture**: this is a structural control that prevents future developers from accidentally weakening the permission enforcement boundary. It is the right tool at the right layer. The substring matching concern in the predicate is worth fixing — it is the only thing standing between "rule fires" and "rule silently misses a violation."

marcel commented 2026-05-05 17:50:07 +02:00

Author

Owner

Copy Link

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The ArchUnit tests are a welcome addition to the static analysis layer of the pyramid. They are fast (bytecode analysis, no Spring context), deterministic, and self-documenting. My concerns are about test completeness and a gap that could allow the rules to produce false confidence.

Blockers

1. The importing and audit domains are missing from Rule 2 coverage

ArchitectureTest.java defines eight per-domain @ArchTest fields for Rule 2, but neither importing nor audit is included. If MassImportService or any audit class were to inject PersonRepository directly, Rule 2 would not catch it. The test suite would stay green while a real boundary violation exists.

Either:

• Add services_only_access_own_domain_repositories_importing and services_only_access_own_domain_repositories_audit, or
• Add a comment explaining why these domains are intentionally excluded (e.g., "audit is a cross-cutting concern exempt from domain isolation by design")

This is a blocker for test completeness — we should not ship a safety net with known holes.

Suggestions

2. The eight near-identical @ArchTest fields reduce maintainability of the test itself

When a new domain is added, a developer must remember to add a new field here. There is no failing test that catches the omission — the test suite just silently lacks coverage for that domain. This is the opposite of the problem ArchUnit is supposed to solve.

A factory method that generates rules for all known domains (or discovers them by scanning @Service packages) would make the rule self-maintaining. This is a known ArchUnit pattern.

3. RED/GREEN verification is documented in the PR but not automated

The PR description lists four manual RED tests. This is good documentation, but those break-test rounds are one-time exercises. If someone refactors foreignJpaRepositoryFor() and inadvertently weakens the predicate, there is no automated test that catches the regression in the rule itself (i.e., a meta-test that introduces a violation and asserts the rule fires).

ArchUnit supports testing rules in isolation via EvaluationResult — consider a test that:

1. Creates a synthetic JavaClass representing a boundary violation
2. Asserts that foreignJpaRepositoryFor("document").test(thatClass) returns true

This is a nice-to-have, not a blocker, but it would close the meta-testing gap.

4. Test location is consistent with the shared package naming

org.raddatz.familienarchiv.shared.ArchitectureTest — the package exists only in the test tree, which is fine. I would prefer architecture over shared (less ambiguous), but this does not affect behavior.

5. CI time: no concern

ArchUnit on this codebase adds ~2–5 seconds. The PR states 1514 tests pass, total. That is firmly within the unit-test time budget. No concern.

## 🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The ArchUnit tests are a welcome addition to the static analysis layer of the pyramid. They are fast (bytecode analysis, no Spring context), deterministic, and self-documenting. My concerns are about test completeness and a gap that could allow the rules to produce false confidence. ### Blockers **1. The `importing` and `audit` domains are missing from Rule 2 coverage** `ArchitectureTest.java` defines eight per-domain `@ArchTest` fields for Rule 2, but neither `importing` nor `audit` is included. If `MassImportService` or any audit class were to inject `PersonRepository` directly, Rule 2 would not catch it. The test suite would stay green while a real boundary violation exists. Either: - Add `services_only_access_own_domain_repositories_importing` and `services_only_access_own_domain_repositories_audit`, or - Add a comment explaining why these domains are intentionally excluded (e.g., "audit is a cross-cutting concern exempt from domain isolation by design") This is a blocker for test completeness — we should not ship a safety net with known holes. ### Suggestions **2. The eight near-identical `@ArchTest` fields reduce maintainability of the test itself** When a new domain is added, a developer must remember to add a new field here. There is no failing test that catches the omission — the test suite just silently lacks coverage for that domain. This is the opposite of the problem ArchUnit is supposed to solve. A factory method that generates rules for all known domains (or discovers them by scanning `@Service` packages) would make the rule self-maintaining. This is a known ArchUnit pattern. **3. RED/GREEN verification is documented in the PR but not automated** The PR description lists four manual RED tests. This is good documentation, but those break-test rounds are one-time exercises. If someone refactors `foreignJpaRepositoryFor()` and inadvertently weakens the predicate, there is no automated test that catches the regression in the rule *itself* (i.e., a meta-test that introduces a violation and asserts the rule fires). ArchUnit supports testing rules in isolation via `EvaluationResult` — consider a test that: 1. Creates a synthetic `JavaClass` representing a boundary violation 2. Asserts that `foreignJpaRepositoryFor("document").test(thatClass)` returns `true` This is a nice-to-have, not a blocker, but it would close the meta-testing gap. **4. Test location is consistent with the `shared` package naming** `org.raddatz.familienarchiv.shared.ArchitectureTest` — the package exists only in the test tree, which is fine. I would prefer `architecture` over `shared` (less ambiguous), but this does not affect behavior. **5. CI time: no concern** ArchUnit on this codebase adds ~2–5 seconds. The PR states 1514 tests pass, total. That is firmly within the unit-test time budget. No concern.

marcel commented 2026-05-05 17:50:16 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist

Verdict: ✅ Approved

This PR is entirely backend test infrastructure — no frontend code, no UI components, no Svelte templates, no Tailwind classes, no accessibility-relevant markup. There is nothing here for me to audit on the UI/UX or accessibility axis.

Blockers

None.

What I checked

• No changes to frontend/ — confirmed by the diff (2 files changed: backend/pom.xml and backend/src/test/.../ArchitectureTest.java)
• No OpenAPI spec changes that would require frontend type regeneration
• No new API endpoints, no changed response shapes, no @Schema annotation changes
• The PR does not touch any error handling path that would affect what users see

One forward-looking note

Rule 5 (deferred, tracked in #427) proposes enforcing that controllers expose endpoints under their domain prefix. When that rule is implemented, it will indirectly constrain the API URL structure the frontend depends on. At that point, I would want to be looped in to confirm the URL conventions match what the SvelteKit route structure and the TypeScript API client expect. No action needed now — just flagging for when #427 is picked up.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist **Verdict: ✅ Approved** This PR is entirely backend test infrastructure — no frontend code, no UI components, no Svelte templates, no Tailwind classes, no accessibility-relevant markup. There is nothing here for me to audit on the UI/UX or accessibility axis. ### Blockers None. ### What I checked - No changes to `frontend/` — confirmed by the diff (2 files changed: `backend/pom.xml` and `backend/src/test/.../ArchitectureTest.java`) - No OpenAPI spec changes that would require frontend type regeneration - No new API endpoints, no changed response shapes, no `@Schema` annotation changes - The PR does not touch any error handling path that would affect what users see ### One forward-looking note Rule 5 (deferred, tracked in #427) proposes enforcing that controllers expose endpoints under their domain prefix. When that rule is implemented, it will indirectly constrain the API URL structure the frontend depends on. At that point, I would want to be looped in to confirm the URL conventions match what the SvelteKit route structure and the TypeScript API client expect. No action needed now — just flagging for when #427 is picked up.

marcel referenced this issue from a commit 2026-05-05 18:02:27 +02:00

test(archunit): add Rule 2 coverage for importing and audit domains

marcel added 2 commits 2026-05-05 18:02:27 +02:00

test(archunit): fix foreignJpaRepositoryFor exact-segment matching ... 0dd58556a7

Replace substring contains() with a regex exact-segment match so a
domain whose name is a substring of another (e.g. "tag" in "tagging")
cannot silently escape the predicate and produce a false negative.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(archunit): add Rule 2 coverage for importing and audit domains ... 0981355247

MassImportService delegates to other domain services (no direct repo
access), and AuditService only touches its own AuditLogRepository —
both pass the boundary rule cleanly. Closes the known hole flagged
by Sara and Markus in PR #428.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-05 18:02:52 +02:00

Author

Owner

Copy Link

Review feedback addressed — @felixbrandt

Two commits pushed to feat/issue-409-archunit. Full suite: 1516 tests, 0 failures.

---

Concern 1 — foreignJpaRepositoryFor() exact-segment matching (Felix blocker + Nora concern)

Fixed in commit 0dd58556.

Replaced getPackageName().contains("." + ownDomain) with a regex exact-segment match:

String ownPackagePattern = ".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$";
// ...
&& !clazz.getPackageName().matches(ownPackagePattern);

This ensures a domain name that is a substring of another (e.g. "tag" inside "tagging") cannot silently escape the predicate and produce a false negative. The pattern anchors on the full familienarchiv.<domain> segment with an optional sub-package suffix.

---

Concern 2 — importing and audit missing from Rule 2 (Sara blocker + Markus observation)

Fixed in commit 09813552.

Added services_only_access_own_domain_repositories_importing and services_only_access_own_domain_repositories_audit as @ArchTest fields alongside the existing eight.

Both pass cleanly:

• MassImportService delegates to DocumentService, PersonService, and TagService — no direct foreign repository access.
• AuditService and AuditLogQueryService only touch their own AuditLogRepository / AuditLogQueryRepository.

---

Concern 3 — Eight near-identical fields, prefer a single parameterized rule (Markus suggestion + Felix suggestion + Sara suggestion)

Intentionally kept as per-domain fields.

ArchUnit's @ArchTest fields must be static final for the JUnit 5 engine to pick them up. A factory returning a List<ArchRule> cannot be annotated with @ArchTest directly — the runner would not discover those rules at all. The KISS call is to keep explicit per-domain fields (now 10 of them) rather than introduce a custom @ArchTests-annotated collector class that complicates the structure. When domain 11 arrives, the pattern is clear from the existing ten.

---

Test count went from 11 → 13 in ArchitectureTest (two new @ArchTest fields), and from 1514 → 1516 across the full suite.

## Review feedback addressed — @felixbrandt Two commits pushed to `feat/issue-409-archunit`. Full suite: **1516 tests, 0 failures**. --- ### Concern 1 — `foreignJpaRepositoryFor()` exact-segment matching (Felix blocker + Nora concern) **Fixed** in commit `0dd58556`. Replaced `getPackageName().contains("." + ownDomain)` with a regex exact-segment match: ```java String ownPackagePattern = ".*\\.familienarchiv\\." + ownDomain + "(\\..+)?$"; // ... && !clazz.getPackageName().matches(ownPackagePattern); ``` This ensures a domain name that is a substring of another (e.g. `"tag"` inside `"tagging"`) cannot silently escape the predicate and produce a false negative. The pattern anchors on the full `familienarchiv.<domain>` segment with an optional sub-package suffix. --- ### Concern 2 — `importing` and `audit` missing from Rule 2 (Sara blocker + Markus observation) **Fixed** in commit `09813552`. Added `services_only_access_own_domain_repositories_importing` and `services_only_access_own_domain_repositories_audit` as `@ArchTest` fields alongside the existing eight. Both pass cleanly: - `MassImportService` delegates to `DocumentService`, `PersonService`, and `TagService` — no direct foreign repository access. - `AuditService` and `AuditLogQueryService` only touch their own `AuditLogRepository` / `AuditLogQueryRepository`. --- ### Concern 3 — Eight near-identical fields, prefer a single parameterized rule (Markus suggestion + Felix suggestion + Sara suggestion) **Intentionally kept as per-domain fields.** ArchUnit's `@ArchTest` fields must be `static final` for the JUnit 5 engine to pick them up. A factory returning a `List<ArchRule>` cannot be annotated with `@ArchTest` directly — the runner would not discover those rules at all. The KISS call is to keep explicit per-domain fields (now 10 of them) rather than introduce a custom `@ArchTests`-annotated collector class that complicates the structure. When domain 11 arrives, the pattern is clear from the existing ten. --- Test count went from 11 → 13 in `ArchitectureTest` (two new `@ArchTest` fields), and from 1514 → 1516 across the full suite.

marcel merged commit 0981355247 into main 2026-05-05 18:08:40 +02:00

marcel deleted branch feat/issue-409-archunit 2026-05-05 18:08:42 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

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

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#428