fix(backend): resolve cross-domain repo + controller→repo violations before REFACTOR-1 #417
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Precursor for REFACTOR-1 (#407) under Epic #406. Surfaced by AUDIT-2 (#389) as C6.1 + C6.2 Critical fails.
REFACTOR-1 is supposed to be a purely mechanical package move. If the layering violations below are still in the tree when REFACTOR-1 runs, two bad things happen:
shared/ArchitectureTest.javaArchUnit rule (REFACTOR-3 #409) fails on day one, so REFACTOR-1's "tests green" signal is meaningless.Therefore the violations must be resolved before REFACTOR-1's branch is created.
Findings (audit-confirmed, file:line precise)
C6.1 — Controllers injecting repositories directly (2 sites)
backend/src/main/java/org/raddatz/familienarchiv/controller/StatsController.java:18-19PersonRepository,DocumentRepositoryStatsService(or fold into existingDashboardService); controller delegatespersonRepository.count()+documentRepository.count()to it. While there, add@RequirePermission(Permission.READ_ALL)(currently missing — flagged by AUDIT-2 §7).backend/src/main/java/org/raddatz/familienarchiv/controller/AuthE2EController.java:27PasswordResetTokenRepositorygetResetTokenForTest(String email)method toPasswordResetServicegated on@Profile("e2e"); call from controller.C6.2 — Services injecting another domain's repository (11 sites)
service/MassImportService.java:58DocumentRepositoryDocumentServicewrite APIservice/ThumbnailService.java:65DocumentRepositoryDocumentService.findIdsForThumbnailBackfill()+updateThumbnailMetadata()service/ThumbnailBackfillService.java:40DocumentRepositoryservice/ThumbnailAsyncRunner.java:32DocumentRepositoryservice/TranscriptionQueueService.java:23DocumentRepositoryDocumentService.queueProjection(...)service/SegmentationTrainingExportService.java:30-32TranscriptionBlockRepository,AnnotationRepository,DocumentRepositoryservice/TrainingDataExportService.java:31-33TranscriptionBlockRepository,AnnotationRepository,DocumentRepositoryservice/SenderModelService.java:35TranscriptionBlockRepositoryTranscriptionServiceservice/OcrTrainingService.java:40TranscriptionBlockRepositoryservice/TranscriptionService.java:40AnnotationRepositoryAnnotationServiceservice/AnnotationService.java:28TranscriptionBlockRepositoryTranscriptionServiceservice/PasswordResetService.java:33AppUserRepositoryUserService.findByEmail/saveMethod (per violation)
For each row in the tables above:
DocumentRepository→DocumentService,TranscriptionBlockRepository→TranscriptionService,AnnotationRepository→AnnotationService,AppUserRepository→UserService,PasswordResetTokenRepository→PasswordResetService).findIdsForThumbnailBackfill(),queueProjection(...), etc.); write callers get a write helper. Keep the method signature minimal — return what the caller actually consumes../mvnw test. Must stay green after every fix.Adding new service methods is allowed; this issue is a layering fix, not a "change behavior" fix. Do not introduce new abstractions, new DTOs, or new endpoints.
Acceptance criteria
StatsControllerno longer injects any repository (use a new or existing service).AuthE2EControllerno longer injectsPasswordResetTokenRepository../mvnw testis green.--spring.profiles.active=devand diff/v3/api-docs— should be empty).Definition of Done
main, reviewed, and merged before REFACTOR-1's (#407) branch is created.Cross-references
ArchitectureTestrule that codifies these constraints.🏗️ Markus Keller — Senior Application Architect
Observations
This is exactly the right fix to run before REFACTOR-1 — doing the package move with these violations live would have caused the ArchUnit rules to fail on the first commit and obscured what was actually wrong.
Looking at the code, the violations split into three different risk tiers:
Tier 1 — Simple and safe:
StatsControllerandPasswordResetServiceinjectingAppUserRepository. Both are minimal operations (count(),findByEmail). Introducing aStatsService(a 5-line class) and routingPasswordResetServicethroughUserService.findByEmail()is a one-commit job with no behavioral risk.Tier 2 — Careful interface design: The Thumbnail cluster (
ThumbnailService,ThumbnailBackfillService,ThumbnailAsyncRunner) all injectDocumentRepositorydirectly.ThumbnailAsyncRunner.generateAsync()callsdocumentRepository.findById(documentId)which is the canonicalDocumentService.getById()— but the async runner explicitly must not throw on missing documents (it logs and returns). The newDocumentServicemethod needs a nullable/Optional variant (findByIdOrEmpty(UUID)) rather than the throwinggetById(). Get this signature right on the first cut; changing it later requires touching the caller again.Tier 3 — Circular dependency risk:
TranscriptionServiceinjectsAnnotationRepository.AnnotationServiceinjectsTranscriptionBlockRepository. These two services have a mutual read dependency. The fix (read methods on each owning service) is correct, but verify that Spring does not flag a circular bean dependency when both services inject each other. If it does, the path throughTranscriptionBlockQueryService(already in the codebase for similar read queries) is the right escape valve — add the needed method there rather than creating a cycle.AuthE2EController: The fix described in the issue (movegetResetTokenForTesttoPasswordResetServicebehind@Profile("e2e")) is correct. One constraint to watch:@Profile("e2e")on a service method is not how Spring works —@Profilegates bean creation, not individual methods. The right pattern is a separate@Service @Profile("e2e") class PasswordResetTestHelperthat wrapsPasswordResetServiceand exposesgetResetTokenForTest(String email). The controller then injects that helper, and the production bean graph never sees it.Recommendations
DocumentServiceread method asOptional<Document> findById(UUID id)(no-throw variant). Use this inThumbnailAsyncRunner— theisEmpty()guard already exists.TranscriptionService ↔ AnnotationServicemutual dependency: add the needed read methods toTranscriptionBlockQueryServiceinstead. It already exists for this purpose and avoids the circular injection risk.PasswordResetTestHelperas a@Service @Profile("e2e")bean rather than annotating a method on the productionPasswordResetService.StatsControllerfirst (simplest, proves the commit-per-violation discipline), then work through Tier 2, then Tier 3 last when you understand the circular dependency resolution../mvnw dependency:analyzeafter the refactor to verify no unused direct dependencies remain in the affected services.👨💻 Felix Brandt — Senior Fullstack Developer
Observations
The issue spec is unusually precise — file:line citations, an explicit allowlist grep, and a zero-hit acceptance condition. That's the right level of rigor for a mechanical refactor. Good.
Existing tests will break and need updates. I looked at the affected test files:
ThumbnailServiceTestdirectly instantiatesThumbnailService(fileService, s3Client, documentRepository). WhenDocumentRepositoryis removed fromThumbnailServiceand replaced with aDocumentServicemethod, the constructor changes and every test that wires it vianew ThumbnailService(...)orReflectionTestUtilsneeds updating.MassImportServiceTestmocks@Mock DocumentRepository documentRepository— same problem, that@Mockbecomes stale once the injection is removed fromMassImportService.StatsControllerTestmocks@MockitoBean PersonRepository personRepositoryand@MockitoBean DocumentRepository documentRepository. OnceStatsControllerdelegates toStatsService, these@MockitoBeandeclarations switch to@MockitoBean StatsService statsService. The test structure stays the same but the mock target changes.TranscriptionQueueServiceTestandPasswordResetServiceTest— check and update similarly.Per-violation commit discipline. The issue says "commit per violation or per closely-related cluster." The Thumbnail cluster (
ThumbnailService,ThumbnailBackfillService,ThumbnailAsyncRunner) all need the sameDocumentServicemethod to be added, so they should land in one commit. Everything else is independent.ThumbnailServiceis not@RequiredArgsConstructor— it has an explicit constructor. When you addDocumentServiceto its dependencies, update the constructor manually. Don't switch it to@RequiredArgsConstructoras a side-effect of this issue — that's a behavior-neutral change but outside this issue's scope.PasswordResetServicealready injectsAppUserRepository— not a foreign-domain repository.PasswordResetServiceowns password-reset tokens;AppUseris in the user domain. The fix (injectUserServiceinstead) is correct by the layering rule, but verifyUserServicedoesn't already injectPasswordResetTokenRepositorycreating a reverse dependency before you wire it.Recommendations
DocumentServiceread method as a failing test first:DocumentServiceTest#findById_returnsEmpty_whenDocumentDoesNotExist(). Then implement. Then update the Thumbnail service tests to mockDocumentServiceinstead ofDocumentRepository.StatsServiceimplementation is 5 lines: twocount()calls delegated from repository to service. TDD: writeStatsServiceTest#getStats_returnsCountsFromRepositories()as the first failing test../mvnw testand confirm the current suite is green. Record the baseline. Every commit after that must also be green.🔒 Nora "NullX" Steiner — Application Security Engineer
Observations
This issue is primarily an architectural fix, not a security fix — but two specific items have security implications worth naming explicitly.
StatsControllerhas no@RequirePermission(flagged explicitly in the issue body). This endpoint returns document and person counts. By itself that's low sensitivity, but "how many documents exist" is metadata that could confirm whether a family archive is populated — which has minor information-disclosure weight for a private family system. The fix is simple and the issue already calls for it: add@RequirePermission(Permission.READ_ALL). Do this as part of theStatsControllerfix commit, not as a separate concern. Don't defer it.AuthE2EControlleris currently acceptable as-is from a security standpoint, but the proposed refactor to a@Profile("e2e")service is strictly better. Reasons:@Operation(hidden = true)annotation suppresses the endpoint from OpenAPI docs when running bothdevande2eprofiles together. That suppression is correct and must be preserved in the refactored version — add it to the controller method in the new structure too.No new attack surface is introduced by this refactor. Adding service methods that delegate to repositories doesn't change the HTTP surface or authentication requirements. This is a safe structural change.
Verification grep from the acceptance criteria also covers security. The controller grep confirms no repository can be reached by bypassing the service layer. That's the right control.
Recommendations
@RequirePermission(Permission.READ_ALL)goes onStatsController.getStats()in the same commit that introducesStatsService. Don't split this into a separate PR.PasswordResetTestHelper @Profile("e2e")service (or however this is structured), ensure the method that exposes the raw token is not callable from outside thee2eprofile. The existing@Profile("e2e")on the controller class provides this, but if you refactor to a helper bean, the@Profile("e2e")annotation must move to the helper class, not be omitted.StatsControllerTestcurrently hasvoid getStats_returns401_whenUnauthenticated()— this test must be updated to also verifyreturns403_when_user_lacks_READ_ALLonce the@RequirePermissionannotation is added. That's a new test case that should be written first (red) before the annotation is added (green).🧪 Sara Holt — QA Engineer & Test Strategist
Observations
The test impact is higher than the issue body implies. I found these existing test files that directly reference the repositories being removed:
StatsControllerTest@MockitoBean PersonRepository,@MockitoBean DocumentRepositoryThumbnailServiceTestmock(DocumentRepository.class)passed to constructorMassImportServiceTest@Mock DocumentRepositoryThumbnailBackfillServiceTest@Mock DocumentRepository— verifyThumbnailAsyncRunnerTest@Mock DocumentRepositoryTranscriptionQueueServiceTest@Mock DocumentRepository— confirmPasswordResetServiceTest@Mock AppUserRepository— confirmThe acceptance criteria don't mention updating these tests, but each one will fail to compile (or produce a mock that's no longer injected) once the corresponding repository injection is removed.
Strategy for each test:
The fix pattern is consistent: replace
@Mock DocumentRepository documentRepositorywith@Mock DocumentService documentServiceand update thewhen(...).thenReturn(...)stubs to match the new service method signature. This is purely mechanical but must happen for every affected test in the same commit as the production code change — otherwise the commit-level green signal is meaningless.Test coverage for new service methods: Each new method added to
DocumentService,TranscriptionService,AnnotationService, etc. needs at least:ThumbnailAsyncRunnerwhere the empty case has specific behavior: log + return)StatsServiceis new — it needs aStatsServiceTestwith@ExtendWith(MockitoExtension.class)before the class exists (TDD red phase).The acceptance criteria verification greps are excellent and should be run in CI, not just manually. Consider adding them as a Maven Enforcer plugin rule or at minimum as a documented CI check step.
OpenAPI diff verification is also in the acceptance criteria: regenerate the spec and diff. This is a one-command check (
./mvnw clean package -DskipTests && java -jar target/*.jar --spring.profiles.active=dev, then hit/v3/api-docs). I'd add this as an explicit step in the PR description checklist.Recommendations
void getStats_returns403_when_user_lacks_READ_ALL()toStatsControllerTestbefore adding@RequirePermissionto the controller (per TDD discipline).StatsServiceTestbefore creatingStatsService. Two tests needed: counts are delegated correctly, and zero counts return correctly (already implied by the existing controller test shapes).ThumbnailAsyncRunnerTestshould have a test coveringgenerateAsync_skips_and_logs_when_document_not_found— this test may already exist but will need to mockDocumentService.findById()returning empty after the refactor.🚀 Tobias Wendt — DevOps & Platform Engineer
Observations
This is a pure backend refactor with no Docker Compose, CI pipeline, or infrastructure impact. No new services, no new environment variables, no migration files. The deployment footprint is unchanged.
Two operational points worth noting:
OpenAPI spec diff is a required check. The acceptance criteria require regenerating the spec and confirming a zero diff. In the current CI setup, the backend tests run without the
devprofile, so OpenAPI is disabled during CI. This check must be done manually by the implementer and explicitly documented in the PR. Consider adding a dedicated CI step that starts the backend with--spring.profiles.active=dev, hits/v3/api-docs, and diffs against the committedapi-docs.jsonsnapshot — but that's scope for a CI improvement issue, not this one../mvnw testis the only CI gate here. The suite currently runs 92 test files. The refactor will compile-fail several of them (see Sara's comment) if the test updates are not included in each commit. The CI run on the PR branch will catch this, but it's cleaner to run./mvnw testlocally after each commit than to wait for CI to report the failure.No new dependencies are introduced. All the new service methods call existing repository methods — no new Maven dependencies, no new Spring beans beyond possibly
StatsServiceand aPasswordResetTestHelper. Both are trivially small.Recommendations
devprofile. Document this explicitly in the PR as "manually verified:./mvnw clean package -DskipTests && java -jar ... --spring.profiles.active=dev, confirmed/v3/api-docsdiff is empty."🎨 Leonie Voss — UX Designer & Accessibility Strategist
No UI/UX concerns from my angle.
This issue is entirely a backend layering refactor — no Svelte components, no routes, no frontend changes, and no user-visible behavior changes. The
StatsControllerendpoint (/api/stats) feeds the dashboard counts, but the response shape is unchanged (totalPersons,totalDocuments), so the frontend renders identically before and after.The one thing I checked: the OpenAPI spec is required to be unchanged (acceptance criteria item 5). If the spec changes,
npm run generate:apiwould regenerate TypeScript types and potentially affect frontend code. The issue correctly requires a zero-diff verification. That's the only path this refactor could reach the UI layer, and it's already covered.📋 Elicit (Requirements Engineer)
Observations
The issue is exceptionally well-specified. File:line precision, explicit allowlist grep, zero-hit acceptance condition, and a clear DoD including cross-reference comments on #407 and #409. This is implementation-ready as written.
One requirements ambiguity I want to surface before coding starts:
The
PasswordResetServicesituation is slightly different from the other C6.2 violations. The issue categorizes it as "PasswordResetService.java:33injectsAppUserRepository(user domain)." But looking at the code,PasswordResetServicealso injectsPasswordResetTokenRepository— which it legitimately owns. The fix routesAppUserRepositoryaccess throughUserService.findByEmail(). However:UserServicewould then need afindByEmail(String email)method that returns anOptional<AppUser>without throwing — becausePasswordResetService.requestReset()explicitly handles the "user not found" case silently to prevent user enumeration. Confirm that the newUserService.findByEmail()method returnsOptional<AppUser>(not throws) so the caller's silent-fail behavior is preserved.The
TranscriptionService ↔ AnnotationServicemutual read dependency (items 10 and 11 in the C6.2 table) could create a circular Spring bean dependency. The issue doesn't flag this as a known risk. It's worth verifying at the start of that fix rather than discovering it mid-commit.Acceptance criteria completeness check: All five acceptance criteria are measurable and testable. The two greps are exact-match verifiable. The
./mvnw testgate is binary. The OpenAPI diff is verifiable (with the caveat Tobias raised about CI automation). The DoD cross-reference comments are manual but clear. No gaps.Recommendations
Optional<AppUser>return type on the newUserService.findByEmail()before thePasswordResetServicefix, to preserve user-enumeration prevention behavior.TranscriptionService ↔ AnnotationServicepair as the highest-risk fix — do it last, and start by checking for circular dependency in a spike commit before the full fix.PR #420 opened against
main. Eight atomic commits, each green at the test-suite level (1489 → 1503 tests, 0 failures throughout):Both acceptance-criteria greps return zero hits.
./mvnw testis green;./mvnw clean package -DskipTestssucceeds. No DTO or endpoint signatures touched, so the OpenAPI spec is byte-identical tomain(manually verified — automating that diff is its own follow-up per Tobias's comment).Two design notes worth flagging for the reviewer:
@Lazyat the injection point, mirroring the existingSenderModelService.selfpattern:DocumentService ↔ ThumbnailAsyncRunnerandTranscriptionService ↔ AnnotationService.lombok.configwas added withlombok.copyableAnnotations += @Lazyso the annotation reaches the Lombok-generated constructor parameter.TranscriptionBlockQueryServicerather thanTranscriptionService, becauseSenderModelService → TrainingDataExportService → TranscriptionService → SenderModelServicewould otherwise re-introduce a cycle. This matches Markus's recommended escape valve.Closing the loop on #407 with a separate comment now.