fix(backend): resolve cross-domain repo + controller→repo violations (#417) #420
Reference in New Issue
Block a user
Delete Branch "feat/issue-417-resolve-layering-violations"
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?
Closes #417. Precursor for REFACTOR-1 (#407) — must merge before #407's branch is created so REFACTOR-3 (#409)'s ArchUnit rule has a clean tree to validate.
Summary
./mvnw testCommits (atomic, one cluster per commit)
refactor(stats): introduce StatsService and require READ_ALL—StatsController's two repo injections move into a 5-lineStatsServicethat delegates toPersonService.count/DocumentService.count. Adds the missing@RequirePermission(Permission.READ_ALL)flagged by AUDIT-2 §7.refactor(auth): route password reset through service layer + e2e helper—PasswordResetServiceswapsAppUserRepositoryforUserService.findByEmailOptional(new no-throw sibling) +UserService.save.AuthE2EControllerno longer touchesPasswordResetTokenRepository; a new@Service @Profile("e2e") PasswordResetTestHelperwrapsPasswordResetService.findLatestActiveTokenForEmail(also new).refactor(thumbnail): route document access through DocumentService— Thumbnail trio (Service, Backfill, AsyncRunner) dropDocumentRepositoryfor newDocumentService.findById(Optional)/findForThumbnailBackfill/updateThumbnailMetadata.@Lazyon the existingDocumentService → ThumbnailAsyncRunnerfield breaks the resulting cycle.lombok.configadds@LazytocopyableAnnotations.refactor(transcription-queue): route through DocumentService projections— Four queue-projection delegations onDocumentService(findSegmentationQueue,findTranscriptionQueue,findReadyToReadQueue,findWeeklyStats).refactor(import): route MassImportService through DocumentService—findByOriginalFilename+savedelegations.refactor(training-export): route export services through owning services— Both export services drop their three foreign repos. Block reads go through expandedTranscriptionBlockQueryService(chosen overTranscriptionServiceto keep theSenderModelService → TrainingDataExportService → …chain cycle-free); annotation reads throughAnnotationService.findById(new); document reads through the existingDocumentService.findById.refactor(ocr-training): route SenderModelService and OcrTrainingService through TranscriptionBlockQueryService—countandcountManualKurrentBlocksByPersonadded to the query service.refactor(transcription/annotation): break mutual repo dependency—TranscriptionService.deleteByAnnotationIdandAnnotationService.deleteById/deleteAllById(all writes) replace the mutual repo touchpoints.@LazyonAnnotationService's newTranscriptionServicefield breaks the two-bean cycle.Verification
The two zero-hit acceptance greps from the issue body — both return zero matches against
mainafter this branch:./mvnw testis green at every commit boundary../mvnw clean package -DskipTestssucceeds.OpenAPI diff
No DTO, controller, or endpoint signatures changed. The OpenAPI spec is byte-identical to
main's — verified by manually generating with--spring.profiles.active=dev(per Tobias's note this CI step isn't automated yet).Test plan
./mvnw clean package -DskipTestsbuilds the JAR.main(manual — to confirm before merge).🤖 Generated with Claude Code
- PasswordResetService injects UserService instead of AppUserRepository. - New UserService.findByEmailOptional preserves the silent-fail behaviour of the old findByEmail-returning-Optional path; the existing throwing findByEmail is unchanged. - New PasswordResetService.findLatestActiveTokenForEmail exposes the latest active reset token without leaking the repository upward. - New @Profile("e2e") PasswordResetTestHelper wraps that read so the AuthE2EController no longer touches PasswordResetTokenRepository directly. Profile guard moves from the controller-only annotation to also cover the helper bean, so the production graph never instantiates either. Refs #417 (C6.1 violation #2 + C6.2 violation #12). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>🏛️ Markus Keller (@mkeller) — Senior Application Architect
Verdict: ✅ Approved
This PR is the structural work I've been waiting for. Every domain boundary violation is now repaired, and the approach is consistent throughout. The layering rule (
Controller → Service → Repository → DB) is fully enforced.What's Done Right
The @Lazy cycle-breaking pattern is correct. The
DocumentService ↔ ThumbnailAsyncRunnercycle and theAnnotationService ↔ TranscriptionServicecycle are both genuine mutual dependencies caused by the layering migration itself.@Lazyon the back-edge is the right Spring idiom, and wiring it intolombok.configviacopyableAnnotationsis exactly how to make it work with@RequiredArgsConstructor. The code comments explain the why, which is what ADRs are for when they're too small for a full document.The
TranscriptionBlockQueryServicechoice for the export chain is architecturally sound. RoutingSenderModelService → TrainingDataExportServicethroughTranscriptionBlockQueryServiceinstead ofTranscriptionServiceavoids aSenderModelService → TranscriptionService → SenderModelServicecycle that would require another@Lazy. The commit description explains this reasoning explicitly — good.StatsServiceis exactly right. Two-line service, delegates to owning services. Simple, clean, and testable. Adding@RequirePermission(READ_ALL)toStatsControlleras part of this commit is the correct place to apply it (AUDIT-2 §7 was a real gap).PasswordResetTestHelper @Profile("e2e")is the right pattern for test-only infrastructure. A@Servicethat only loads in thee2eprofile, wrapping the productionPasswordResetService, is cleaner than putting test-only logic in the controller itself.Concerns
DocumentServiceis growing into a pass-through for its own repository. The new delegation methods (save(),findById(),findByOriginalFilename(),findForThumbnailBackfill(),updateThumbnailMetadata(), and four queue projections) are all thin wrappers. This is the correct solution given the layering constraint, but the service is now doing two distinct things: orchestrating complex document workflows and providing access to simple repository reads for other services. A future step (out of scope here) would be consolidating the queue projections onto aDocumentQueryServiceanalogous toTranscriptionBlockQueryService. Not a blocker — that's #407's ArchUnit guard's job to track.UserService.save()is a very generic public method. Exposing a baresave()on a service risks becoming a general-purpose back-door. The caller isPasswordResetService.resetPassword(), which is the right use case. Worth keeping an eye on to ensure it doesn't get pulled into other contexts where a more specific method name would communicate intent better.Summary
The acceptance criteria (0 cross-domain repo injections in services, 0 repo injections in controllers) are met. The architecture is cleaner than before. The two concerns above are follow-up opportunities, not blockers for this PR.
👨💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: ✅ Approved
Clean refactor. All production changes have corresponding test updates in the same commit. Nothing left unmocked, nothing left dangling.
What's Done Right
Every mock is updated alongside the production change. No commit leaves the suite with a test that mocks
DocumentRepositorywhile the production code callsDocumentService. That's the right discipline — red/green stays clean at every commit boundary.makeService()inTrainingDataExportServiceTestis an elegant adapter. The problem:@DataJpaTestgives you real JPA repos, but the service now needs service dependencies. The solution — create mocks that transparently delegate to the real repos viathenAnswer(inv -> realRepo.method(...))— keeps the integration test's database fidelity while honoring the new layering. Pragmatic and correct.DocumentService.findById()alongsidegetById()is intentional and named correctly.findById()returnsOptional<Document>for callers that handle absence themselves (ThumbnailAsyncRunner, export services).getById()throws for callers where absence is an error. This is consistent with Spring Data's own conventions.TranscriptionService.deleteByAnnotationId()is@Transactional. Correct — it's a write.AnnotationService.deleteById/deleteAllByIdare also@Transactional. No write method is missing its boundary.Suggestions (non-blocking)
makeService()javadoc mentions the issue number ("after #417's layering refactor"). Per our convention, issue references belong in commit messages and PR descriptions, not code comments — they rot as the codebase evolves. The why of the adapter is the important part; the fix number is redundant context. Consider trimming that sentence.DocumentService.save()is named identically to the Spring Data method. This is fine functionally, but callers readingdocumentService.save(doc)inMassImportServicehave no idea this is a simple delegation. A more intentional name likepersistDocument(doc)orsaveImportedDocument(doc)would make the intent visible. Not a blocker — the consistency with the repository method name is also an argument for keeping it.PasswordResetServicestill uses field-level@Autowired(pre-existing, not introduced here). Not a blocker for this PR, but worth a cleanup issue since we standardize on@RequiredArgsConstructor+finalfields.Summary
38 files touched, 1503 green tests, zero architectural regressions. The TDD discipline is evident — every new method (e.g.
UserService.findByEmailOptional,UserService.save) has dedicated test coverage inUserServiceTest. Solid work.🔒 Nora "NullX" Steiner (@nullx) — Application Security Engineer
Verdict: ✅ Approved
This PR closes a real security gap and introduces no new attack surface. The architectural changes actually improve the security posture.
Security Fix: Missing Authorization on
/api/statsThis is the most important change in the PR from a security perspective.
Before this PR,
StatsControllerinjectedPersonRepositoryandDocumentRepositorydirectly and had no@RequirePermissionannotation. The/api/statsendpoint was reachable by any authenticated user regardless of permission group — or, depending on the Spring Security configuration, potentially unauthenticated users.After this PR:
The
@RequirePermission(READ_ALL)is enforced byPermissionAspectvia AOP — it's not a scattered if-statement, it's declarative and auditable. This is a blocker fix that was correct to include here.PasswordResetTestHelper @Profile("e2e")— No Production ExposureThe helper bean is gated by
@Profile("e2e"). Spring only registers it when thee2eprofile is active. TheAuthE2EControllerthat calls it is also@Profile("e2e"). Neither bean is reachable in production or dev.The production
PasswordResetService.findLatestActiveTokenForEmail()is a new public method — but it only returns a token string, which is already stored in the database and protected by the existing token expiry andisUsedflag logic. No credential exposure.UserService.save()— Controlled SurfaceThe new
save()method is callable from any service that can injectUserService. The current caller (PasswordResetService) uses it correctly — saving after a password hash update. No bypass of password hashing: the encoder call happens inPasswordResetService.resetPassword()beforeuserService.save()is called. No raw password persisted.Watch: if
UserService.save()gets called from a path that doesn't hash the password first, that's a vulnerability. The pattern is safe today with the single caller. It would be safer as a named method (updatePassword(user)orsaveWithHashedPassword(user)) to make the intent explicit and prevent misuse. Noting as a smell, not a confirmed vuln.No New Attack Surface
@Lazyproxies are Spring infrastructure — no security implicationslombok.configchange only affects code generation — no runtime impactRecommendation
Add a
@WebMvcTest-based test forStatsControllerthat verifies the 403 response for a user with onlyWRITE_ALL(notREAD_ALL). The test suite has 1503 passing tests but I don't see explicit permission-boundary coverage for the new@RequirePermissionannotation on/api/stats. The bug existed; the fix is right; the regression test is missing.🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Test count: 1489 → 1503 (+14). Mocks updated alongside every production change — no commit leaves the suite in a broken state. The fundamentals are solid. One concern.
What's Right
All mock seams are updated correctly. Every service that switched from a
*Repositorydependency to a service dependency has its test updated in the same PR.TranscriptionQueueServiceTest,OcrTrainingServiceTest,MassImportServiceTest,ThumbnailServiceTest,TranscriptionServiceTest,TranscriptionServiceGuidedTest— all updated. Nothing left testing the old contract.makeService()inTrainingDataExportServiceTestis the correct approach. The test was a@DataJpaTestintegration test, meaning it has a real Postgres schema via Testcontainers. The service now needs service dependencies, not repo dependencies. The adapter pattern (mock(XService.class)withthenAnswer(inv -> realRepo.method(...))) keeps the database fidelity of the integration test while respecting the new layering. This is the right trade-off.New
UserServicetests are clean.findByEmailOptional_returnsEmpty_whenMissing,findByEmailOptional_returnsUser_whenFound,save_delegatesToRepository— three tests, three behaviors, one assertion each. Well-named, Arrange-Act-Assert structure, factory-built data.StatsServicehas dedicated test coverage — I can see from the PR description that./mvnw testis 1503 green. The new service is exercised.Concern: Missing Permission Boundary Test for
/api/statsThe
@RequirePermission(READ_ALL)annotation was added toStatsControlleras part of this PR. This is a security fix (AUDIT-2 §7). Security fixes must be accompanied by a regression test that would have caught the original bug.There is no
StatsControllerTestvisible in this diff. The existing test suite does not include a test that verifies:Without this test, the
@RequirePermissionannotation can be accidentally removed in a future refactor and no test will catch it. The bug existed; the fix is correct; the regression test is the missing piece.This is a suggestion, not a blocker — the overall direction is correct and the test count is green. But I'd like to see a follow-up issue for the
StatsControllerpermission test before #407's ArchUnit guard is merged.Minor Notes
@Disabledtests: none introduced — ✅Thread.sleep()in tests: none — ✅The test hygiene is good. One follow-up item on the security regression test.
🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes. Backend-only refactoring. I'm checking what matters from my side.
What I Checked
lombok.configaddition —lombok.copyableAnnotations += org.springframework.context.annotation.Lazyis a Lombok code-generation config change, not a runtime config. It tells Lombok to propagate@Lazyfrom the field to the generated constructor parameter when using@RequiredArgsConstructor. No impact on the Docker image, build time, or runtime behavior. Clean.Dependency graph unchanged — No new
pom.xmldependencies. The build adds no new JAR files to the image. Docker layer cache for the Maven dependencies layer is unaffected../mvnw clean package -DskipTestsbuilds the JAR — PR description confirms this. The artifact is buildable.No new application profiles —
PasswordResetTestHelperis@Profile("e2e"). Thee2eprofile is already in use. No new profile configuration needed in CI or production.OpenAPI spec unchanged — Confirmed in the PR description. No
generate:apirun needed before merge. The frontend TypeScript types don't need regeneration.One Note on CI
The PR description notes that OpenAPI re-generation is still a manual step ("CI step isn't automated yet"). The PR has a
[ ]checkbox for this. That's a known gap from before this PR, not introduced here. Worth a separate issue to automate the spec diff as a CI gate — prevents future PRs from accidentally breaking the API contract without noticing. Not a blocker here.LGTM from my side.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing against the stated acceptance criteria from issue #417.
Acceptance Criteria Traceability
StatsControllerandAuthE2EControllercleaned./mvnw testMissing Test-Marker (⚠️ Unchecked Item)
The PR's test plan has one unchecked item:
This is the only outstanding deliverable. The PR description is transparent about it being manual. The claim that the spec is "byte-identical to main's" is based on manual verification during PR prep, not an automated gate. This should be completed before merge — the PR description acknowledges it.
Scope Compliance
The PR is strictly scoped to layering violation cleanup (issue #417). It introduces:
The
@RequirePermission(READ_ALL)addition toStatsControlleris in-scope (AUDIT-2 §7 was cited as a prerequisite fix). ThePasswordResetTestHelperextraction is in-scope (required by theAuthE2EControllerlayering fix).Sequence Dependency Is Well-Documented
The PR description explicitly states: "must merge before #407's branch is created so REFACTOR-3 (#409)'s ArchUnit rule has a clean tree to validate." This is the right kind of dependency documentation — the merge order is clear, the reason is stated, and it can be acted on without re-reading the issue thread.
Verdict
The acceptance criteria are met. The one open item (OpenAPI diff) is clearly flagged in the test plan and requires manual completion before merge. Everything else is traceable, verified, and in scope.
🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This PR contains no frontend changes — no Svelte components, no routes, no Tailwind classes, no i18n strings, no accessibility-relevant markup.
I reviewed the full diff to confirm: all 38 changed files are backend Java source files and test files. The changes are a backend service-layer refactoring with no user-visible impact.
What I verified:
.sveltefiles changedfrontend/directory changesmain)errors.tsor Paraglide translation key additionsNothing to flag from a UI, UX, accessibility, or brand perspective. LGTM.
Review follow-up —
89e9a245Concerns addressed
Felix (@felixbrandt) —
makeService()javadoc mentions issue number ✅Trimmed the
#417's layering refactorreference from themakeService()javadoc inTrainingDataExportServiceTest. The why (keeping real-database fidelity without pulling the full service trees into scope) is preserved; the issue number is gone.Commit:
refactor(test): remove issue reference from makeService javadocConcerns already resolved in the original commits
Nora (@nullx) + Sara (@saraholt) — Missing
StatsControllerTestwith 403 permission boundary testStatsControllerTestwas already added in commitd5e0e969(refactor(stats): introduce StatsService and require READ_ALL). It covers all four cases: 401 unauthenticated, 403 withoutREAD_ALL, 200 with correct counts, 200 with zero counts.Concerns deferred (non-blocking, follow-up issues)
DocumentServicegrowing into a pass-through (Markus) — tracked as follow-up for #407UserService.save()generic name (Nora) — smell noted, no confirmed vuln; follow-up cleanupPasswordResetServicefield@Autowired(Felix) — pre-existing; separate cleanup issue🤖 Generated with Claude Code