refactor: domain-based package layout (#407) #421
Reference in New Issue
Block a user
Delete Branch "feat/issue-407-domain-packaging"
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 #407
Summary
controller/,service/,repository/,model/,dto/) to package-by-domainDataInitializerrenamed toUserDataInitializerand moved touser/New structure
Test plan
./mvnw test→ 1503 tests, all green./mvnw clean package -DskipTests→ BUILD SUCCESS🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
This is exactly the refactoring I have been waiting for. Package-by-domain is the prerequisite for everything we want to do next: extract OCR into its own module, enforce inter-domain contracts, and eventually isolate deployments if the need arises. Layer-first packaging was invisible coupling — a single feature change touched 3+ unrelated packages. That is now fixed.
What I Checked
Domain boundaries are correctly established. Each domain owns its controller, service, repository, entities, and DTOs in one package. I verified
document/,person/,person/relationship/,user/,ocr/,geschichte/,tag/,dashboard/,notification/,importing/,filestorage/,audit/,config/,exception/,security/.Cross-domain imports are clean.
grep -r "import org.raddatz.familienarchiv.controller.",service.,repository.,model.on the new main sources finds nothing. No stale layer-level imports survived the migration.Sub-domains are correctly nested.
document/annotation/,document/comment/,document/transcription/, andperson/relationship/are nested under their parent domains rather than being promoted to the root. This preserves the ownership hierarchy.All 1503 tests pass. Since the only change is import paths and package declarations (pure structural rename), this is expected — and it is the right proof that nothing behaviorally changed.
Blockers
None.
Suggestions
Clean up stale empty directories. The old layer packages are gone in code but their empty directories remain on disk:
src/main/java/.../controller/,service/,repository/,model/,dto/, and a stale top-levelrelationship/dto/. Git does not track empty directories, so they will vanish for new clones — but on anyone's existing working tree they sit there, silently implying there is more content to come. A follow-up commit withgit clean -fdor explicitrmdirwould finish the job.CLAUDE.mdstill listsExcelServiceunderimporting/.ExcelServicewas deleted in an earlier commit (fa60c5be). Both the rootCLAUDE.mdandbackend/CLAUDE.mdstill sayimporting/ # MassImportService, ExcelServiceand the services table still has anExcelServicerow. This is pre-existing doc debt but the documentation update commit in this PR is the right moment to fix it — it was missed..agent/PLAN.md,.agent/current-plan.md,.claude/settings.json,.claude/worktrees/,.claude/personas/req_engineer.md, and.claude/scheduled_tasks.lockare included in this PR. These are LLM-agent artifacts (import pipeline planning notes, Claude Code settings, agent worktree records) that have no relationship to the domain packaging refactoring. They should be in a separate branch or added to.gitignore. Shipping them in this PR contaminates the clean refactor narrative.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean structural refactoring. Thirteen atomic commits, one domain per commit — this is how a large package migration should be done. The commit history reads as a clear audit trail and would make bisecting trivial if something ever broke.
What I Checked
Package declarations match file locations. Spot-checked
DocumentService.java(package org.raddatz.familienarchiv.document),SecurityConfig.java(package org.raddatz.familienarchiv.security),UserService.java(package org.raddatz.familienarchiv.user),RelationshipService.java(package org.raddatz.familienarchiv.person.relationship) — all correct.No old layer-based imports survive. Grepped the entire new source tree for
import org.raddatz.familienarchiv.controller.,service.,repository.,model.— zero hits. The migration is complete at the import level.Test packages match source packages.
PersonServiceTestis inperson/,DocumentServiceTestis indocument/,AnnotationControllerTestis indocument/annotation/. Co-location of test and source gives the right local navigation experience.Dead code from old locations is gone.
model/DocumentSort.javawas a duplicate that lived in the oldmodel/package — the diff confirms it was deleted. The canonical version is now indocument/DocumentSort.java.No behavior changes. 1503 tests pass. Correct — a pure rename/move should be invisible to tests.
Blockers
None.
Suggestions
Empty legacy directories remain on disk.
src/main/java/.../controller/,service/,repository/,model/,dto/are empty but still present. Git does not track empty directories, so on a fresh clone they disappear — but on any existing working tree they confuse navigation. Worth a follow-uprmdirpass.Unrelated LLM-agent files are included.
.agent/PLAN.mdis an import pipeline planning document..agent/current-plan.md,.claude/settings.json, and.claude/worktrees/entries are Claude Code artifacts. These do not belong in a domain packaging PR. Per our atomic commit rule, one logical change per commit — this PR bundles the structural refactor with unrelated planning files.CLAUDE.mdstill referencesExcelServiceunderimporting/and in the services table. Since this PR has a docs commit (docs: update package structure docs to reflect domain-based layout), this is the right place to remove that stale entry.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Pure structural refactoring — no security controls were modified, bypassed, or weakened. My review focused on confirming that the move preserved every security boundary exactly as-is.
What I Checked
SecurityConfigis still isolated in its own package. Moved tosecurity/SecurityConfig.javawith correct package declarationpackage org.raddatz.familienarchiv.security. The centralization principle (one file to audit for all auth rules) is maintained.@RequirePermissionAOP is intact.PermissionAspectand thePermissionenum are insecurity/. Controllers that use@RequirePermissionstill import fromorg.raddatz.familienarchiv.security.Permission— no magic string regressions introduced.CustomUserDetailsServicecorrectly lives underuser/.SecurityConfigdepends on it via theuser/package import — cross-package dependency is clean and in the right direction (security depends on user, not the other way around).No
@CrossOriginor@RequestMapping-without-method annotations introduced. This is a rename operation, not a feature change — no new endpoint annotations were added.Parameterized JPQL queries are unchanged. Repository files were moved, not modified — no query string concatenation was introduced during the migration.
GlobalExceptionHandleris inexception/. Still properly isolated. DomainException error codes reach the frontend unchanged.Blockers
None. The security surface is identical before and after.
Observations (non-blocking)
The
.agent/PLAN.mdincluded in this PR describes deletingExcelServiceand simplifying the import path. That deletion happened in a prior commit (fa60c5be). I have no concerns about the security implications of that prior removal (removing an upload path reduces attack surface), but the planning document's presence in this PR is noise — it describes work that was done before this refactoring began.🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
1503 tests all green after moving 190+ Java source files is the proof I need. The test suite is the regression safety net here, and it held. Let me detail what I audited.
What I Checked
Test files co-located with their sources.
PersonServiceTest→person/,PersonServiceIntegrationTest→person/,DocumentServiceTest→document/,AnnotationControllerTest→document/annotation/,AnnotationServiceTest→document/annotation/,CommentControllerTest→document/comment/. This is the correct pattern: test files live in the same domain package as the classes they test.Test package declarations are correct. Verified
package org.raddatz.familienarchiv.personinPersonServiceTest.javaandpackage org.raddatz.familienarchiv.documentinDocumentServiceTest.java. Not just file locations — the package declarations match.Integration test infrastructure is unchanged.
MigrationIntegrationTest.javaandPostgresContainerConfig.javaremain at the root test level, which is correct — they are cross-domain infrastructure tests, not domain-specific.ApplicationContextTest.javastill at root. This is the smoke test that verifies the full context loads. After a package reorganization, this is the critical test. It is present and (per the PR description) passes.Blockers
None.
Suggestions
Stale empty test directories remain.
src/test/java/.../controller/,service/,model/,dto/are empty but still present on disk. Also a stale top-levelrelationship/test directory. These are cosmetic but confuse navigation — a developer might wonder if there should be tests there. Follow-up cleanup is worth doing.The PR description says "1503 tests, all green" — it would be stronger to include the CI run link. For a 2563-file diff, seeing the Gitea CI badge or a linked run gives reviewers confidence without having to clone and run locally. Not a blocker, just a workflow suggestion for large refactoring PRs.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a pure Java package rename. No Docker Compose changes, no CI workflow changes, no environment variables touched, no infrastructure affected. The Spring Boot JAR is assembled from whatever packages exist — it does not care about package names, only that
@SpringBootApplicationcomponent scanning finds the beans. Let me confirm the key infrastructure facts.What I Checked
FamilienarchivApplication.javais still at the root of the base package. The main entry point is inorg.raddatz.familienarchiv, which is the scan root. Spring will find all@Component,@Service,@Repository,@Controllerbeans in all sub-packages automatically. No@ComponentScanannotations need updating.No Flyway migration files were changed. The
db/migration/directory is untouched — schema is version-controlled separately from code structure. No migration risk.Docker Compose and Dockerfile are unchanged. No build paths, no JAR names, no exposed ports changed. The
./mvnw clean packagestep produces the same artifact.Build succeeds with
-DskipTestsper PR description. The JAR builds correctly.One Note
The
.claude/settings.jsonand.claude/worktrees/files included in this PR are Claude Code IDE artifacts. They have no deployment or infrastructure impact, but if these accumulate in the main branch they will eventually cause confusion. I'd recommend adding.claude/worktrees/and.agent/to.gitignoreso agent-generated files don't end up committed. Low priority but worth tracking.🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Verdict: ✅ Approved
No frontend files were changed. No Svelte components, no routes, no Tailwind classes, no i18n keys, no TypeScript types. This is a pure backend package restructuring — the API surface and response shapes are identical, so there is no user-visible change and no UI/accessibility impact.
I verified:
frontend/directory: zero changes in this PRLGTM from a UI/UX perspective. Nothing to review here.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved (with one documentation finding)
Operating in Brownfield mode — reviewing whether this PR faithfully delivers issue #407 and whether the delivered state leaves requirements artifacts in good health.
Does This PR Deliver Issue #407?
The issue title is "domain-based package layout" (#407). The PR delivers:
DataInitializer→UserDataInitializerrename (better intent-revealing name)The stated acceptance criteria in the PR description are met.
Documentation Debt Found (Non-blocking)
The documentation update commit (
docs: update package structure docs to reflect domain-based layout) introduced one inaccuracy that should be tracked:ExcelServiceappears in two places in CLAUDE.md but no longer exists in the codebase:CLAUDE.mdline:importing/ # MassImportService, ExcelServiceCLAUDE.mdservices table row:ExcelService | Lower-level spreadsheet parsingbackend/CLAUDE.mdline:importing/ # MassImportService, ExcelServiceExcelService.javawas deleted in commitfa60c5be(prior to this branch). The documentation update in this PR had the opportunity to correct this but did not. A reader following CLAUDE.md would expectExcelServiceto exist and spend time searching for it.Recommendation: Either fix in this PR (preferred, since there's already a docs commit here) or open a new issue to track the correction. If left unfixed, it creates a misleading authoritative spec for new contributors.
Scope Observation
The PR diff includes
.agent/PLAN.mdand.agent/current-plan.md— LLM-generated planning documents for an import pipeline overhaul. These are requirements artifacts for a different issue (import pipeline alignment, not domain packaging). Their presence here blurs the change boundary. For the purposes of issue #407, they are out of scope and should be on a separate branch or excluded via.gitignore.Review concerns addressed
Three concerns raised by reviewers have been resolved in three follow-up commits:
1. Stale
ExcelServicereferences in CLAUDE.md (Markus, Felix, Elicit)Removed from:
CLAUDE.md— package tree line and services table rowbackend/CLAUDE.md— package tree line→
c0d8704ddocs: remove stale ExcelService from CLAUDE.md2.
.agent/PLAN.mdand.agent/current-plan.mdremoved (Markus, Felix, Nora, Elicit)These import pipeline planning docs were unrelated to the domain packaging refactor.
→
823735b0chore: remove .agent planning docs from branch3.
.claude/worktrees/agent-*and.claude/scheduled_tasks.lockremoved (Markus, Felix, Tobias)19 runtime worktree records and the lock file were deleted from the branch.
→
cab9f1dbchore: remove runtime agent artifacts from branch4.
.gitignoreupdated (Tobias)Added
.agent/,.claude/worktrees/, and.claude/scheduled_tasks.lockso these don't land in future branches.→
22165c23chore: gitignore .agent/, .claude/worktrees/, scheduled_tasks.lockThe
.claude/personas/req_engineer.md,.claude/skills/deliver-issue/SKILL.md, and.claude/settings.jsonwere intentionally kept — they are project infrastructure files, not runtime artifacts.