refactor(backend): ArchUnit Rule 5 — enforce controller @RequestMapping URL prefix per domain #427

New Issue

marcel · 2026-05-05T17:12:51+02:00

marcel commented

2026-05-05 17:12:51 +02:00

Context

Deferred from #409 (ArchUnit domain boundary tests). Rules 1–4 are live. Rule 5 was flagged as potentially brittle and is tracked here separately.

Required rule

Controllers in a given domain package must expose endpoints under that domain's URL prefix. Example: a controller in ..document.. must carry @RequestMapping whose value starts with /api/documents.

// Custom predicate inspecting @RequestMapping annotation value
DescribedPredicate<JavaAnnotation<?>> startsWithDocumentsPrefix =
    new DescribedPredicate<JavaAnnotation<?>>("have value starting with /api/documents") {
        @Override
        public boolean test(JavaAnnotation<?> annotation) {
            Object[] values = (Object[]) annotation.get("value").orElse(new Object[0]);
            return values.length > 0 && values[0].toString().startsWith("/api/documents");
        }
    };

ArchRule rule = classes()
    .that().areAnnotatedWith(RestController.class)
    .and().resideInAPackage("..document..")
    .should().beAnnotatedWith(RequestMapping.class)
    .andShould().beAnnotatedWith(startsWithDocumentsPrefix);
// Repeat for each domain

Why deferred

A custom DescribedPredicate<JavaAnnotation<?>> inspecting annotation string values is ~20 lines per domain and fragile when URL patterns change (e.g., versioning: /api/v2/documents). The team chose TODO + follow-up over a brittle rule on day one.

Acceptance criteria

ArchitectureTest.java is extended with per-domain URL prefix rules (or a loop over a domain→prefix map)
Each rule fires when a controller in domain X uses a wrong prefix
The TODO comment in ArchitectureTest.java is removed and replaced with the live rule
./mvnw test runs the new rules as part of the suite

Dependency

Depends on #409 being merged first.

## Context Deferred from **#409** (ArchUnit domain boundary tests). Rules 1–4 are live. Rule 5 was flagged as potentially brittle and is tracked here separately. ## Required rule Controllers in a given domain package must expose endpoints under that domain's URL prefix. Example: a controller in `..document..` must carry `@RequestMapping` whose value starts with `/api/documents`. ```java // Custom predicate inspecting @RequestMapping annotation value DescribedPredicate<JavaAnnotation<?>> startsWithDocumentsPrefix = new DescribedPredicate<JavaAnnotation<?>>("have value starting with /api/documents") { @Override public boolean test(JavaAnnotation<?> annotation) { Object[] values = (Object[]) annotation.get("value").orElse(new Object[0]); return values.length > 0 && values[0].toString().startsWith("/api/documents"); } }; ArchRule rule = classes() .that().areAnnotatedWith(RestController.class) .and().resideInAPackage("..document..") .should().beAnnotatedWith(RequestMapping.class) .andShould().beAnnotatedWith(startsWithDocumentsPrefix); // Repeat for each domain ``` ## Why deferred A custom `DescribedPredicate<JavaAnnotation<?>>` inspecting annotation string values is ~20 lines per domain and fragile when URL patterns change (e.g., versioning: `/api/v2/documents`). The team chose TODO + follow-up over a brittle rule on day one. ## Acceptance criteria - [ ] `ArchitectureTest.java` is extended with per-domain URL prefix rules (or a loop over a domain→prefix map) - [ ] Each rule fires when a controller in domain X uses a wrong prefix - [ ] The TODO comment in `ArchitectureTest.java` is removed and replaced with the live rule - [ ] `./mvnw test` runs the new rules as part of the suite ## Dependency Depends on #409 being merged first.

marcel added this to the Codebase Legibility milestone 2026-05-05 17:12:51 +02:00

marcel added the P2-medium legibility refactor labels 2026-05-05 17:12:58 +02:00

marcel referenced this issue from a commit

2026-05-05 17:16:46 +02:00

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

marcel referenced this issue

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

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

marcel referenced this issue

2026-05-05 17:17:45 +02:00

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

marcel referenced this issue

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

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

marcel referenced this issue

2026-05-05 17:49:31 +02:00

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

marcel referenced this issue

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

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

marcel commented

2026-05-07 17:48:05 +02:00

🏛️ Markus Keller — Application Architect

Observations

The core design choice in the issue is sound. Asserting that controllers live under their domain's URL prefix is a meaningful architectural boundary — it prevents namespace drift where a document package controller quietly exposes endpoints under /api/users/....
The existing codebase reveals the rule is not universally followed. NotificationController and OcrController use per-method full-path @GetMapping("/api/notifications/...") rather than a class-level @RequestMapping. UserController has @RequestMapping("/api/") (trailing slash, no users segment). TranscriptionQueueController is in ..document.transcription.. but maps to /api/transcription. These are real violations of the invariant the issue is trying to enforce — meaning the rule, once live, will fail immediately without first normalising those controllers.
The proposed implementation design in the issue — one DescribedPredicate per domain — is a maintenance trap. The existing Rule 2 already demonstrates the cost: 10 identical rule declarations, one per domain, each registering a separate @ArchTest field. Adding another 10 for URL prefixes multiplies the surface area.
A Map<String, String> domain→prefix lookup driving a @ParameterizedTest-style loop is the correct pattern here. ArchUnit's @ArchTest does not support parameterised tests directly, but a single @ArchTest field can iterate a map and call rule.check(classes) programmatically, accumulating violations before throwing. This gives one concise declaration with full coverage.
The "brittleness concern" cited as the deferral reason is real but solvable. Brittleness arises from hardcoding string prefixes that will change with versioning. The fix is to make the map the single source of truth and document in a comment that it must be updated whenever a domain is added or its URL prefix changes — this is the same discipline already applied to Flyway migrations and C4 diagrams.
Docs impact: this PR adds no new domain packages, controllers, or routes — no C4 or CLAUDE.md updates are triggered. However, if the implementation normalises NotificationController or OcrController mapping style as a prerequisite, that is a separate concern from the ArchUnit rule itself.

Recommendations

Before implementing Rule 5, normalise the outlier controllers: OcrController and NotificationController should gain a class-level @RequestMapping, and TranscriptionQueueController's /api/transcription prefix should be evaluated (it arguably belongs under /api/documents/transcription). Do this in a separate commit before the ArchUnit rule so the rule passes green from the first run.
Implement Rule 5 as a single @ArchTest driven by a Map rather than 10+ separate field declarations. Keep DOMAIN_TO_PREFIX as a private static constant — it becomes the authoritative registry of all domain→URL mappings in the codebase.
Pair the map constant with a comment explaining the maintenance contract: "Add an entry here whenever a new domain package and its controller are introduced."
Do not attempt to enforce sub-domain controllers (e.g., AnnotationController under ..document.annotation.. mapping to /api/documents/{id}/annotations) through this rule — the prefix check at the top-level domain package boundary is sufficient and avoids exponential complexity.

Open Decisions

TranscriptionQueueController prefix: it currently maps to /api/transcription despite living in ..document.transcription... The rule as specified would require /api/documents/.... Is that the desired API path, or should the domain→prefix map register transcription as a special case with its own prefix? This needs a decision before Rule 5 can go green.
UserController base path: @RequestMapping("/api/") is ambiguous — the trailing slash means the class-level prefix is effectively empty. Should it become @RequestMapping("/api/users") for consistency, or does its mixed /api/users/... + /api/... routing have a specific reason?

## 🏛️ Markus Keller — Application Architect ### Observations - **The core design choice in the issue is sound.** Asserting that controllers live under their domain's URL prefix is a meaningful architectural boundary — it prevents namespace drift where a `document` package controller quietly exposes endpoints under `/api/users/...`. - **The existing codebase reveals the rule is not universally followed.** `NotificationController` and `OcrController` use per-method full-path `@GetMapping("/api/notifications/...")` rather than a class-level `@RequestMapping`. `UserController` has `@RequestMapping("/api/")` (trailing slash, no `users` segment). `TranscriptionQueueController` is in `..document.transcription..` but maps to `/api/transcription`. These are real violations of the invariant the issue is trying to enforce — meaning the rule, once live, will fail immediately without first normalising those controllers. - **The proposed implementation design in the issue — one `DescribedPredicate` per domain — is a maintenance trap.** The existing Rule 2 already demonstrates the cost: 10 identical rule declarations, one per domain, each registering a separate `@ArchTest` field. Adding another 10 for URL prefixes multiplies the surface area. - **A `Map<String, String>` domain→prefix lookup driving a `@ParameterizedTest`-style loop is the correct pattern here.** ArchUnit's `@ArchTest` does not support parameterised tests directly, but a single `@ArchTest` field can iterate a map and call `rule.check(classes)` programmatically, accumulating violations before throwing. This gives one concise declaration with full coverage. - **The "brittleness concern" cited as the deferral reason is real but solvable.** Brittleness arises from hardcoding string prefixes that will change with versioning. The fix is to make the map the single source of truth and document in a comment that it must be updated whenever a domain is added or its URL prefix changes — this is the same discipline already applied to Flyway migrations and C4 diagrams. - **Docs impact:** this PR adds no new domain packages, controllers, or routes — no C4 or `CLAUDE.md` updates are triggered. However, if the implementation normalises `NotificationController` or `OcrController` mapping style as a prerequisite, that is a separate concern from the ArchUnit rule itself. ### Recommendations 1. **Before implementing Rule 5, normalise the outlier controllers:** `OcrController` and `NotificationController` should gain a class-level `@RequestMapping`, and `TranscriptionQueueController`'s `/api/transcription` prefix should be evaluated (it arguably belongs under `/api/documents/transcription`). Do this in a separate commit before the ArchUnit rule so the rule passes green from the first run. 2. **Implement Rule 5 as a single `@ArchTest` driven by a `Map`** rather than 10+ separate field declarations. Keep `DOMAIN_TO_PREFIX` as a private static constant — it becomes the authoritative registry of all domain→URL mappings in the codebase. 3. **Pair the map constant with a comment** explaining the maintenance contract: "Add an entry here whenever a new domain package and its controller are introduced." 4. **Do not attempt to enforce sub-domain controllers** (e.g., `AnnotationController` under `..document.annotation..` mapping to `/api/documents/{id}/annotations`) through this rule — the prefix check at the top-level domain package boundary is sufficient and avoids exponential complexity. ### Open Decisions - **`TranscriptionQueueController` prefix:** it currently maps to `/api/transcription` despite living in `..document.transcription..`. The rule as specified would require `/api/documents/...`. Is that the desired API path, or should the domain→prefix map register `transcription` as a special case with its own prefix? This needs a decision before Rule 5 can go green. - **`UserController` base path:** `@RequestMapping("/api/")` is ambiguous — the trailing slash means the class-level prefix is effectively empty. Should it become `@RequestMapping("/api/users")` for consistency, or does its mixed `/api/users/...` + `/api/...` routing have a specific reason?

marcel commented

2026-05-07 17:48:26 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The ArchUnit DescribedPredicate<JavaAnnotation<?>> API in the issue body is slightly outdated. ArchUnit 1.x (the project uses 1.3.0) provides JavaAnnotation.get("value") returning Optional<Object>, but the cast to Object[] is only safe for multi-value attributes. A class-level @RequestMapping("/api/documents") stores value as a String[] (since Spring 6), not a raw Object[]. The predicate should use (String[]) or iterate with Optional.flatMap. Getting this wrong silently produces a ClassCastException that masks the rule failure.
The foreignJpaRepositoryFor helper in the existing ArchitectureTest uses a regex-based DescribedPredicate<JavaClass>, not one on JavaAnnotation. The new Rule 5 requires a DescribedPredicate<JavaAnnotation<?>>, which is a different generic bound. The two approaches are not composable — the new predicate needs its own factory method with a clear name.
A loop-based implementation avoids the 10-field duplication problem visible in Rules 2+. The Rule 2 block currently has 10 separate @ArchTest fields repeating identical structure. Rule 5 should not repeat this pattern. A single @ArchTest with an internal loop over Map.of(...) entries and ArchRuleDefinition.classes() checked per entry is cleaner and easier to extend.
OcrController and NotificationController both lack class-level @RequestMapping. These controllers use full-path per-method mappings (@GetMapping("/api/ocr/...")). The Rule 5 check needs to handle this: a controller without class-level @RequestMapping should either fail the rule (forcing normalisation) or be explicitly excluded with a comment. Silently passing it would undermine the rule's value.
UserController has @RequestMapping("/api/") — the trailing slash means the class-level path is /api/, not /api/users. The prefix check startsWith("/api/users") would fail for this controller.
The TranscriptionQueueController in ..document.transcription.. maps to /api/transcription, not /api/documents. Rule 5 as spec'd would flag this as a violation.

Recommendations

Use JavaAnnotation.get("value").map(v -> ((String[]) v)[0]) in the predicate body — cast to String[], not Object[], since Spring's @RequestMapping(value = "...") uses String[] semantics.
Name the factory method urlPrefixRuleFor(String domain, String prefix) and keep it private static alongside foreignJpaRepositoryFor. One method per concern, descriptive name.
Make the absence of class-level @RequestMapping a rule failure — a controller without it should not pass the domain-prefix rule. This forces the normalisation of OcrController and NotificationController as a prerequisite (per Markus's recommendation).
Write the test first: add a tiny inner test class or a separate ArchitectureTestRule5Test with a mock JavaClass stubbed to carry a wrong prefix, assert the rule flags it, then implement the predicate to go green.
Do not extract the domain→prefix map to a config file or constants class — keep it in ArchitectureTest.java where it is used. Co-location is the readable choice; the map is a test concern, not a runtime concern.

Open Decisions

None — the technical approach has a clear winner. The only prerequisite is resolving the controller normalisation question (which is Markus's Open Decision, not a code design ambiguity).

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **The ArchUnit `DescribedPredicate<JavaAnnotation<?>>` API in the issue body is slightly outdated.** ArchUnit 1.x (the project uses `1.3.0`) provides `JavaAnnotation.get("value")` returning `Optional<Object>`, but the cast to `Object[]` is only safe for multi-value attributes. A class-level `@RequestMapping("/api/documents")` stores `value` as a `String[]` (since Spring 6), not a raw `Object[]`. The predicate should use `(String[])` or iterate with `Optional.flatMap`. Getting this wrong silently produces a `ClassCastException` that masks the rule failure. - **The `foreignJpaRepositoryFor` helper in the existing `ArchitectureTest` uses a regex-based `DescribedPredicate<JavaClass>`, not one on `JavaAnnotation`. The new Rule 5 requires a `DescribedPredicate<JavaAnnotation<?>>`, which is a different generic bound.** The two approaches are not composable — the new predicate needs its own factory method with a clear name. - **A loop-based implementation avoids the 10-field duplication problem visible in Rules 2+.** The Rule 2 block currently has 10 separate `@ArchTest` fields repeating identical structure. Rule 5 should not repeat this pattern. A single `@ArchTest` with an internal loop over `Map.of(...)` entries and `ArchRuleDefinition.classes()` checked per entry is cleaner and easier to extend. - **`OcrController` and `NotificationController` both lack class-level `@RequestMapping`.** These controllers use full-path per-method mappings (`@GetMapping("/api/ocr/...")`). The Rule 5 check needs to handle this: a controller without class-level `@RequestMapping` should either fail the rule (forcing normalisation) or be explicitly excluded with a comment. Silently passing it would undermine the rule's value. - **`UserController` has `@RequestMapping("/api/")` — the trailing slash means the class-level path is `/api/`, not `/api/users`.** The prefix check `startsWith("/api/users")` would fail for this controller. - **The `TranscriptionQueueController` in `..document.transcription..` maps to `/api/transcription`, not `/api/documents`.** Rule 5 as spec'd would flag this as a violation. ### Recommendations 1. **Use `JavaAnnotation.get("value").map(v -> ((String[]) v)[0])` in the predicate body** — cast to `String[]`, not `Object[]`, since Spring's `@RequestMapping(value = "...")` uses `String[]` semantics. 2. **Name the factory method `urlPrefixRuleFor(String domain, String prefix)`** and keep it private static alongside `foreignJpaRepositoryFor`. One method per concern, descriptive name. 3. **Make the absence of class-level `@RequestMapping` a rule failure** — a controller without it should not pass the domain-prefix rule. This forces the normalisation of `OcrController` and `NotificationController` as a prerequisite (per Markus's recommendation). 4. **Write the test first:** add a tiny inner test class or a separate `ArchitectureTestRule5Test` with a mock `JavaClass` stubbed to carry a wrong prefix, assert the rule flags it, then implement the predicate to go green. 5. **Do not extract the domain→prefix map to a config file or constants class** — keep it in `ArchitectureTest.java` where it is used. Co-location is the readable choice; the map is a test concern, not a runtime concern. ### Open Decisions - None — the technical approach has a clear winner. The only prerequisite is resolving the controller normalisation question (which is Markus's Open Decision, not a code design ambiguity).

marcel commented

2026-05-07 17:48:44 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

This rule is architecturally security-relevant, not just cosmetic. A controller in ..document.. that accidentally exposes endpoints under /api/admin/... could bypass the coarser Spring Security route-based rules that protect the /api/admin/** prefix. Rule 5 prevents that class of misconfiguration structurally.
The permission model already relies on @RequirePermission AOP rather than route-based Spring Security path matchers. This means the URL prefix rule's security value is indirect: it enforces legibility and prevents namespace collisions, but does not directly enforce permission boundaries. The @RequirePermission annotation is the real access gate. However, route namespace collisions can create confusion during security audits — an endpoint appearing in the wrong namespace is harder to audit for correct permission coverage.
No direct security vulnerabilities are introduced or resolved by this issue. It is a correctness and legibility measure for the test suite.
One indirect security benefit: if @RequestMapping is normalised at the class level for OcrController and NotificationController (which this issue may trigger as a prerequisite), the per-method full-path mappings become shorter and more obviously scoped. Long full-path strings like @GetMapping("/api/documents/{documentId}/ocr-status") scattered across a controller are harder to audit for completeness than @GetMapping("/{documentId}/ocr-status") under a class-level @RequestMapping("/api/documents").
OcrController.train and OcrController.segtrain expose powerful endpoints. Their URL paths (/api/ocr/train, /api/ocr/segtrain) are correct for their domain. No cross-namespace issue here.
No new attack surface is opened by the ArchUnit rule itself — it is a compile-time / test-time constraint with no runtime effect.

Recommendations

Document the security rationale in the Rule 5 comment, as was done for Rule 1: "URL prefix isolation prevents a domain controller from accidentally serving endpoints in another domain's protected namespace, which would confuse security audits." One line, committed alongside the rule.
After implementing Rule 5, run a one-time audit: verify that every @RestController annotated with @RequirePermission(Permission.ADMIN) or similar high-privilege annotations lives under the expected URL namespace. The rule catches future drift; the one-time audit catches existing drift.
Do not weaken the rule by excluding OcrController or NotificationController via ignoredClasses. Force normalisation of those controllers instead. An @ArchTest with exclusions is a security smell — exclusions accumulate and the rule loses its enforcement value.

Open Decisions

None from a security perspective. The implementation approach does not create security tradeoffs.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - **This rule is architecturally security-relevant, not just cosmetic.** A controller in `..document..` that accidentally exposes endpoints under `/api/admin/...` could bypass the coarser Spring Security route-based rules that protect the `/api/admin/**` prefix. Rule 5 prevents that class of misconfiguration structurally. - **The permission model already relies on `@RequirePermission` AOP rather than route-based Spring Security path matchers.** This means the URL prefix rule's security value is indirect: it enforces legibility and prevents namespace collisions, but does not directly enforce permission boundaries. The `@RequirePermission` annotation is the real access gate. However, route namespace collisions can create confusion during security audits — an endpoint appearing in the wrong namespace is harder to audit for correct permission coverage. - **No direct security vulnerabilities are introduced or resolved by this issue.** It is a correctness and legibility measure for the test suite. - **One indirect security benefit:** if `@RequestMapping` is normalised at the class level for `OcrController` and `NotificationController` (which this issue may trigger as a prerequisite), the per-method full-path mappings become shorter and more obviously scoped. Long full-path strings like `@GetMapping("/api/documents/{documentId}/ocr-status")` scattered across a controller are harder to audit for completeness than `@GetMapping("/{documentId}/ocr-status")` under a class-level `@RequestMapping("/api/documents")`. - **`OcrController.train` and `OcrController.segtrain` expose powerful endpoints.** Their URL paths (`/api/ocr/train`, `/api/ocr/segtrain`) are correct for their domain. No cross-namespace issue here. - **No new attack surface is opened by the ArchUnit rule itself** — it is a compile-time / test-time constraint with no runtime effect. ### Recommendations 1. **Document the security rationale in the Rule 5 comment**, as was done for Rule 1: *"URL prefix isolation prevents a domain controller from accidentally serving endpoints in another domain's protected namespace, which would confuse security audits."* One line, committed alongside the rule. 2. **After implementing Rule 5, run a one-time audit:** verify that every `@RestController` annotated with `@RequirePermission(Permission.ADMIN)` or similar high-privilege annotations lives under the expected URL namespace. The rule catches future drift; the one-time audit catches existing drift. 3. **Do not weaken the rule by excluding `OcrController` or `NotificationController` via `ignoredClasses`.** Force normalisation of those controllers instead. An `@ArchTest` with exclusions is a security smell — exclusions accumulate and the rule loses its enforcement value. ### Open Decisions - None from a security perspective. The implementation approach does not create security tradeoffs.

marcel commented

2026-05-07 17:49:08 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The acceptance criteria are well-formed and testable. All four criteria are binary pass/fail: the rule exists, the rule fires on wrong prefixes, the TODO is removed, ./mvnw test runs it. No ambiguity.
"Each rule fires when a controller in domain X uses a wrong prefix" — this criterion requires a failing-case test. ArchUnit rules are statically checked against real bytecode, not mocked. To verify the rule would have caught a violation, either: (a) temporarily introduce a wrong-prefix controller in the test source tree to assert the rule catches it, or (b) write a standalone unit test using ArchUnit's ArchRuleEvaluator API to feed synthetic class metadata. Without this, the rule could silently pass even if the predicate has a bug (e.g., wrong cast, wrong annotation attribute name) — the predicate would be evaluated but never trigger because all real controllers already comply.
The existing Rule 2 tests have no violation-scenario coverage either. This is a pre-existing gap in the test suite. Rule 5's implementation is an opportunity to establish the pattern: one rule declaration + one violation-detection test.
The ArchUnit rule runs as a standard JUnit 5 test via @ArchTest + @AnalyzeClasses. It integrates cleanly into the Maven Surefire phase — no additional configuration required. The acceptance criterion "runs as part of the suite" will be satisfied automatically.
CI impact is negligible. ArchUnit analyzes compiled bytecode. The @AnalyzeClasses call is already paying the class-loading cost for Rules 1–4; Rule 5 adds a filter + predicate evaluation on the same loaded class set. No meaningful CI time increase expected.
@AnalyzeClasses(packagesOf = FamilienarchivApplication.class) — this is correct. The class cache is shared across all @ArchTest fields in the class, so loading happens once per test class.
The TODO comment currently in ArchitectureTest.java (lines 128–133) is precise and links back to this issue. Removing it when the rule goes live is a clean, verifiable acceptance criterion.

Recommendations

Add one violation-detection test alongside the new rule. Use ArchUnit's ClassFileImporter to load a synthetic wrong-prefix controller (e.g., a test-scope class annotated with @RestController + @RequestMapping("/api/wrong") in the ..document.. package) and assert rule.check(importedClasses) throws AssertionError. This proves the rule is not vacuously passing.
Follow red/green/refactor strictly: write the violation-detection test first (it should fail because the rule doesn't exist yet), then implement the rule (violation test passes, all real controllers pass because they already comply or have been normalised), then refactor.
Do not suppress the failing violation test with @Disabled. If normalising OcrController/NotificationController is out of scope for this issue, scope the rule to only check controllers that have class-level @RequestMapping (and document that exclusion explicitly) until normalisation is done in a follow-up.
Verify the rule actually fires on TranscriptionQueueController (/api/transcription in ..document.transcription..) before merging. If it does fire, either fix the prefix or add it to the exclusion map with a ticket reference.

Open Decisions

Scope of the rule at merge time: should the rule go live covering all controllers (requiring prerequisite normalisation), or scoped only to controllers that already have class-level @RequestMapping (allowing incremental rollout)? Both are valid; the former is cleaner long-term, the latter avoids blocking this PR on unrelated refactoring. This is a planning decision for the implementer.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - **The acceptance criteria are well-formed and testable.** All four criteria are binary pass/fail: the rule exists, the rule fires on wrong prefixes, the TODO is removed, `./mvnw test` runs it. No ambiguity. - **"Each rule fires when a controller in domain X uses a wrong prefix" — this criterion requires a failing-case test.** ArchUnit rules are statically checked against real bytecode, not mocked. To verify the rule would have caught a violation, either: (a) temporarily introduce a wrong-prefix controller in the test source tree to assert the rule catches it, or (b) write a standalone unit test using ArchUnit's `ArchRuleEvaluator` API to feed synthetic class metadata. Without this, the rule could silently pass even if the predicate has a bug (e.g., wrong cast, wrong annotation attribute name) — the predicate would be evaluated but never trigger because all real controllers already comply. - **The existing Rule 2 tests have no violation-scenario coverage either.** This is a pre-existing gap in the test suite. Rule 5's implementation is an opportunity to establish the pattern: one rule declaration + one violation-detection test. - **The ArchUnit rule runs as a standard JUnit 5 test via `@ArchTest` + `@AnalyzeClasses`.** It integrates cleanly into the Maven Surefire phase — no additional configuration required. The acceptance criterion "runs as part of the suite" will be satisfied automatically. - **CI impact is negligible.** ArchUnit analyzes compiled bytecode. The `@AnalyzeClasses` call is already paying the class-loading cost for Rules 1–4; Rule 5 adds a filter + predicate evaluation on the same loaded class set. No meaningful CI time increase expected. - **`@AnalyzeClasses(packagesOf = FamilienarchivApplication.class)` — this is correct.** The class cache is shared across all `@ArchTest` fields in the class, so loading happens once per test class. - **The TODO comment currently in `ArchitectureTest.java` (lines 128–133) is precise and links back to this issue.** Removing it when the rule goes live is a clean, verifiable acceptance criterion. ### Recommendations 1. **Add one violation-detection test** alongside the new rule. Use ArchUnit's `ClassFileImporter` to load a synthetic wrong-prefix controller (e.g., a test-scope class annotated with `@RestController` + `@RequestMapping("/api/wrong")` in the `..document..` package) and assert `rule.check(importedClasses)` throws `AssertionError`. This proves the rule is not vacuously passing. 2. **Follow red/green/refactor strictly:** write the violation-detection test first (it should fail because the rule doesn't exist yet), then implement the rule (violation test passes, all real controllers pass because they already comply or have been normalised), then refactor. 3. **Do not suppress the failing violation test with `@Disabled`.** If normalising `OcrController`/`NotificationController` is out of scope for this issue, scope the rule to only check controllers that have class-level `@RequestMapping` (and document that exclusion explicitly) until normalisation is done in a follow-up. 4. **Verify the rule actually fires on `TranscriptionQueueController`** (`/api/transcription` in `..document.transcription..`) before merging. If it does fire, either fix the prefix or add it to the exclusion map with a ticket reference. ### Open Decisions - **Scope of the rule at merge time:** should the rule go live covering all controllers (requiring prerequisite normalisation), or scoped only to controllers that already have class-level `@RequestMapping` (allowing incremental rollout)? Both are valid; the former is cleaner long-term, the latter avoids blocking this PR on unrelated refactoring. This is a planning decision for the implementer.

marcel commented

2026-05-07 17:49:28 +02:00

🖥️ Tobias Wendt — DevOps & Platform Engineer

Observations

No infrastructure, CI, or Docker changes are required or implied by this issue. Rule 5 is a pure Java test-code addition with zero runtime surface. The Maven Surefire phase already runs ArchitectureTest; the new rule participates automatically.
CI impact is effectively zero. ArchUnit reuses the loaded class set across all @ArchTest fields within ArchitectureTest via @AnalyzeClasses caching. Adding Rule 5 adds microseconds of predicate evaluation to the existing class-loading cost. No pipeline timing concern.
The archunit-junit5:1.3.0 version in pom.xml is current as of this issue date. No version bump needed.
There is one indirect CI hygiene implication: if the prerequisite controller normalisation (moving OcrController, NotificationController to class-level @RequestMapping) is done in the same PR, it changes the URL routing behavior at runtime. That warrants a smoke test pass before merge to confirm the endpoints are still reachable at their existing paths. The class-level @RequestMapping plus shortened method-level paths must produce the same composed URL as the current full-path per-method annotations — this is a runtime verification concern, not a compile-time one.
The acceptance criterion "runs as part of ./mvnw test" is already satisfied by virtue of being a @ArchTest in the existing @AnalyzeClasses test class. No special CI configuration is needed.

Recommendations

If controller normalisation is included in this PR's scope, add a brief E2E smoke test (or verify via the existing API test files in backend/api_tests/) that the affected endpoints (/api/notifications/..., /api/ocr/...) respond correctly after the @RequestMapping refactor. One curl against the running stack is sufficient — this is not worth a full E2E Playwright run.
No new Docker services, volumes, or CI jobs are needed. Resist any temptation to add infrastructure for what is a test-code change.
Confirm the Maven Surefire plugin is not configured to exclude shared.* tests — check pom.xml for any <excludes> that might accidentally skip ArchitectureTest. (From code inspection, none appear to exist, but worth a quick verify before the PR.)

Open Decisions

None from a DevOps/platform perspective. This is a clean test-code change with no infrastructure implications.

## 🖥️ Tobias Wendt — DevOps & Platform Engineer ### Observations - **No infrastructure, CI, or Docker changes are required or implied by this issue.** Rule 5 is a pure Java test-code addition with zero runtime surface. The Maven Surefire phase already runs `ArchitectureTest`; the new rule participates automatically. - **CI impact is effectively zero.** ArchUnit reuses the loaded class set across all `@ArchTest` fields within `ArchitectureTest` via `@AnalyzeClasses` caching. Adding Rule 5 adds microseconds of predicate evaluation to the existing class-loading cost. No pipeline timing concern. - **The `archunit-junit5:1.3.0` version in `pom.xml` is current as of this issue date.** No version bump needed. - **There is one indirect CI hygiene implication:** if the prerequisite controller normalisation (moving `OcrController`, `NotificationController` to class-level `@RequestMapping`) is done in the same PR, it changes the URL routing behavior at runtime. That warrants a smoke test pass before merge to confirm the endpoints are still reachable at their existing paths. The class-level `@RequestMapping` plus shortened method-level paths must produce the same composed URL as the current full-path per-method annotations — this is a runtime verification concern, not a compile-time one. - **The acceptance criterion "runs as part of `./mvnw test`" is already satisfied by virtue of being a `@ArchTest` in the existing `@AnalyzeClasses` test class.** No special CI configuration is needed. ### Recommendations 1. **If controller normalisation is included in this PR's scope, add a brief E2E smoke test** (or verify via the existing API test files in `backend/api_tests/`) that the affected endpoints (`/api/notifications/...`, `/api/ocr/...`) respond correctly after the `@RequestMapping` refactor. One `curl` against the running stack is sufficient — this is not worth a full E2E Playwright run. 2. **No new Docker services, volumes, or CI jobs are needed.** Resist any temptation to add infrastructure for what is a test-code change. 3. **Confirm the Maven Surefire plugin is not configured to exclude `shared.*` tests** — check `pom.xml` for any `<excludes>` that might accidentally skip `ArchitectureTest`. (From code inspection, none appear to exist, but worth a quick verify before the PR.) ### Open Decisions - None from a DevOps/platform perspective. This is a clean test-code change with no infrastructure implications.

marcel commented

2026-05-07 17:49:51 +02:00

📋 Elicit — Requirements Engineer

Observations

The acceptance criteria are clear and implementation-ready. All four criteria are testable, unambiguous, and scoped. The issue is well-written by brownfield standards: context (deferred from #409), the rule to implement, the why-deferred rationale, and a dependency declaration are all present. Definition of Ready is effectively met.
One acceptance criterion is underspecified: "Each rule fires when a controller in domain X uses a wrong prefix" — this states the desired behaviour but does not specify how the firing is verified (i.e., what the test artifact is). The implementer must decide whether to prove this via a synthetic violation test, a manual inspection, or simply trust the ArchUnit framework. The criterion should specify the verification method to be unambiguous.
The dependency declaration "Depends on #409 being merged first" is correctly stated and is a hard prerequisite — Rule 5 extends ArchitectureTest.java which exists because of #409. No ambiguity there.
The issue does not specify which domains are in scope for Rule 5. The existing Rules 2 cover 10 domains. Does Rule 5 cover all the same domains? Only domains that have controllers? What about cross-domain controllers like OcrController which spans both /api/documents/... and /api/ocr/... paths? This is an implementation ambiguity that will surface during development.
The "brittleness" framing in the Why Deferred section conflates two distinct concerns: (a) the predicate implementation complexity (~20 lines per domain) and (b) future URL versioning (/api/v2/...). These are separate risks. Concern (a) is resolved by the loop approach. Concern (b) is a future risk that should be acknowledged in the rule's comment, not used as a reason to block the rule entirely.
No user-facing requirements are affected. This is a pure developer-tooling / codebase legibility issue with no impact on end users, personas, or user journeys.
NFR coverage: no performance, security, accessibility, i18n, or deployment NFRs apply. The only relevant NFR is maintainability — and the issue explicitly addresses it via the loop-vs-per-domain design discussion.

Recommendations

Tighten acceptance criterion 2: change "Each rule fires when a controller in domain X uses a wrong prefix" to "A synthetic wrong-prefix test class in the domain package causes the rule to fail with an AssertionError (verified by a dedicated unit test)." This makes the criterion unambiguously testable.
Add an explicit scope statement to the issue body: list which domains are covered by Rule 5 (suggest: all domains with at least one @RestController-annotated class). Exclude domains with no controllers to avoid vacuous rules.
Separate the brittleness concern into a future risk note rather than a deferral rationale: "Future risk: if URL versioning (/api/v2/...) is adopted, the prefix map will need updating — this is acceptable maintenance, not brittleness."
The issue correctly does not require i18n, UI, or frontend changes. No scope creep observed.

Open Decisions

Multi-prefix controllers: OcrController currently spans two URL namespaces (/api/documents/{id}/ocr and /api/ocr/...). Should Rule 5 require a single class-level prefix (forcing normalisation or a design decision about which namespace OCR "belongs" to), or should multi-prefix controllers be explicitly excluded with a comment? This is a genuine product/architecture tradeoff requiring a decision before the rule can be finalised.

## 📋 Elicit — Requirements Engineer ### Observations - **The acceptance criteria are clear and implementation-ready.** All four criteria are testable, unambiguous, and scoped. The issue is well-written by brownfield standards: context (deferred from #409), the rule to implement, the why-deferred rationale, and a dependency declaration are all present. Definition of Ready is effectively met. - **One acceptance criterion is underspecified:** "Each rule fires when a controller in domain X uses a wrong prefix" — this states the desired behaviour but does not specify *how* the firing is verified (i.e., what the test artifact is). The implementer must decide whether to prove this via a synthetic violation test, a manual inspection, or simply trust the ArchUnit framework. The criterion should specify the verification method to be unambiguous. - **The dependency declaration "Depends on #409 being merged first" is correctly stated** and is a hard prerequisite — Rule 5 extends `ArchitectureTest.java` which exists because of #409. No ambiguity there. - **The issue does not specify which domains are in scope for Rule 5.** The existing Rules 2 cover 10 domains. Does Rule 5 cover all the same domains? Only domains that have controllers? What about cross-domain controllers like `OcrController` which spans both `/api/documents/...` and `/api/ocr/...` paths? This is an implementation ambiguity that will surface during development. - **The "brittleness" framing in the Why Deferred section conflates two distinct concerns:** (a) the predicate implementation complexity (~20 lines per domain) and (b) future URL versioning (`/api/v2/...`). These are separate risks. Concern (a) is resolved by the loop approach. Concern (b) is a future risk that should be acknowledged in the rule's comment, not used as a reason to block the rule entirely. - **No user-facing requirements are affected.** This is a pure developer-tooling / codebase legibility issue with no impact on end users, personas, or user journeys. - **NFR coverage:** no performance, security, accessibility, i18n, or deployment NFRs apply. The only relevant NFR is maintainability — and the issue explicitly addresses it via the loop-vs-per-domain design discussion. ### Recommendations 1. **Tighten acceptance criterion 2:** change "Each rule fires when a controller in domain X uses a wrong prefix" to "A synthetic wrong-prefix test class in the domain package causes the rule to fail with an `AssertionError` (verified by a dedicated unit test)." This makes the criterion unambiguously testable. 2. **Add an explicit scope statement** to the issue body: list which domains are covered by Rule 5 (suggest: all domains with at least one `@RestController`-annotated class). Exclude domains with no controllers to avoid vacuous rules. 3. **Separate the brittleness concern into a future risk note** rather than a deferral rationale: "Future risk: if URL versioning (`/api/v2/...`) is adopted, the prefix map will need updating — this is acceptable maintenance, not brittleness." 4. **The issue correctly does not require i18n, UI, or frontend changes.** No scope creep observed. ### Open Decisions - **Multi-prefix controllers:** `OcrController` currently spans two URL namespaces (`/api/documents/{id}/ocr` and `/api/ocr/...`). Should Rule 5 require a single class-level prefix (forcing normalisation or a design decision about which namespace OCR "belongs" to), or should multi-prefix controllers be explicitly excluded with a comment? This is a genuine product/architecture tradeoff requiring a decision before the rule can be finalised.

marcel commented

2026-05-07 17:50:09 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Observations

This issue is entirely backend test infrastructure — no UI components, routes, user-visible behaviour, design tokens, or frontend code are affected.

Recommendations

No UI/UX action required for this issue. Proceed without design review.

Open Decisions

None.

## 🎨 Leonie Voss — UI/UX Design Lead ### Observations - This issue is entirely backend test infrastructure — no UI components, routes, user-visible behaviour, design tokens, or frontend code are affected. ### Recommendations - No UI/UX action required for this issue. Proceed without design review. ### Open Decisions - None.

marcel commented

2026-05-07 17:50:38 +02:00

🗳️ Decision Queue — Open Items Requiring Human Judgment

Raised by: Markus (Architect), Elicit (Requirements)

Theme 1: Controller URL Namespace Normalisation (Prerequisite to Rule 5 going green)

Three controllers currently violate the invariant Rule 5 would enforce:

Controller	Package	Current mapping	What Rule 5 would require
`OcrController`	`..ocr..`	Full-path per-method (`/api/documents/{id}/ocr`, `/api/ocr/batch`, ...)	Class-level `@RequestMapping` whose value starts with `/api/ocr`
`NotificationController`	`..notification..`	Full-path per-method (`/api/notifications/...`)	Class-level `@RequestMapping("/api/notifications")`
`UserController`	`..user..`	`@RequestMapping("/api/")` (trailing slash)	`@RequestMapping("/api/users")`
`TranscriptionQueueController`	`..document.transcription..`	`@RequestMapping("/api/transcription")`	Would need to match document domain prefix

Decision needed: Do these get normalised as part of this issue (in a prerequisite commit), or does Rule 5 go live with an explicit exclusion list for outliers (tracked in a follow-up issue)?

Theme 2: OcrController's Dual Namespace

OcrController spans two URL namespaces — it serves both /api/documents/{id}/ocr (document-scoped) and /api/ocr/batch, /api/ocr/train (OCR-domain). A single class-level @RequestMapping cannot cover both.

Decision needed: Should OCR split into two controllers (one under each namespace), or should OcrController be excluded from Rule 5 with a comment explaining the dual-namespace intent?

Theme 3: Scope of Rule 5 at Merge Time (rollout strategy)

Two valid paths:

Full coverage: normalise all outlier controllers first, then Rule 5 covers every @RestController with no exclusions. Cleaner long-term, but requires more work upfront.
Incremental: Rule 5 only fires on controllers that already have a class-level @RequestMapping (others are silently skipped). Smaller PR, but outliers remain unguarded until a follow-up.

Decision needed: Which rollout approach is preferred for this issue?

## 🗳️ Decision Queue — Open Items Requiring Human Judgment Raised by: Markus (Architect), Elicit (Requirements) --- ### Theme 1: Controller URL Namespace Normalisation (Prerequisite to Rule 5 going green) Three controllers currently violate the invariant Rule 5 would enforce: | Controller | Package | Current mapping | What Rule 5 would require | |---|---|---|---| | `OcrController` | `..ocr..` | Full-path per-method (`/api/documents/{id}/ocr`, `/api/ocr/batch`, ...) | Class-level `@RequestMapping` whose value starts with `/api/ocr` | | `NotificationController` | `..notification..` | Full-path per-method (`/api/notifications/...`) | Class-level `@RequestMapping("/api/notifications")` | | `UserController` | `..user..` | `@RequestMapping("/api/")` (trailing slash) | `@RequestMapping("/api/users")` | | `TranscriptionQueueController` | `..document.transcription..` | `@RequestMapping("/api/transcription")` | Would need to match document domain prefix | **Decision needed:** Do these get normalised as part of this issue (in a prerequisite commit), or does Rule 5 go live with an explicit exclusion list for outliers (tracked in a follow-up issue)? --- ### Theme 2: OcrController's Dual Namespace `OcrController` spans two URL namespaces — it serves both `/api/documents/{id}/ocr` (document-scoped) and `/api/ocr/batch`, `/api/ocr/train` (OCR-domain). A single class-level `@RequestMapping` cannot cover both. **Decision needed:** Should OCR split into two controllers (one under each namespace), or should `OcrController` be excluded from Rule 5 with a comment explaining the dual-namespace intent? --- ### Theme 3: Scope of Rule 5 at Merge Time (rollout strategy) Two valid paths: - **Full coverage:** normalise all outlier controllers first, then Rule 5 covers every `@RestController` with no exclusions. Cleaner long-term, but requires more work upfront. - **Incremental:** Rule 5 only fires on controllers that already have a class-level `@RequestMapping` (others are silently skipped). Smaller PR, but outliers remain unguarded until a follow-up. **Decision needed:** Which rollout approach is preferred for this issue?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#427