decide(backend): resolve users-table / AppUser-entity naming mismatch before REFACTOR-1 #418

Closed
opened 2026-05-04 16:41:59 +02:00 by marcel · 9 comments
Owner

Context

Precursor for REFACTOR-1 (#407) under Epic #406. Surfaced by AUDIT-4 (#391) as a C3.3 Critical fail and named explicitly as the audit's "top blocker":

Tobias [PM-with-CS evaluator] will conflate users and persons within 30 seconds of opening the schema.

The mismatch:

  • DB table users (created in backend/src/main/resources/db/migration/V1__initial_schema.sql) — pluralised, sits next to persons.
  • Java entity AppUser (backend/src/main/java/org/raddatz/familienarchiv/model/AppUser.java) — explicitly not called User to honour the project's hard rule that historical Persons ≠ AppUser accounts (see user MEMORY: project_person_appuser_separation).
  • Result: schema-first reader sees users/persons and assumes they're related; code-first reader sees AppUser and looks for an app_users table that doesn't exist. Both are wrong-footed.

REFACTOR-1 (#407) moves AppUser.java from model/ into user/ package. The right time to also align the table name (or commit to a documentation-only fix) is before REFACTOR-1 runs — bundling a table rename inside the same PR as a 200+ class move would be uncomfortable to review, but committing to not renaming requires the same upfront decision because it changes how the new user/ package gets documented.

Decision required

Option A — Rename table usersapp_users (DB migration)

Method. New Flyway migration V60__rename_users_to_app_users.sql:

ALTER TABLE users RENAME TO app_users;
ALTER TABLE users_groups RENAME TO app_users_groups;
ALTER TABLE users_groups RENAME COLUMN user_id TO app_user_id;
ALTER TABLE comment_mentions RENAME COLUMN user_id TO app_user_id;
ALTER TABLE audit_log RENAME COLUMN actor_id TO app_user_id;
-- (verify all FKs that reference users(id); update column names where they say user_id)

Plus add @Table(name = "app_users") to AppUser.java (and update other @JoinColumn references).

Pros.

  • Most consistent with the canonical domain set (REFACTOR-1's user/ package now maps cleanly: package user, entity AppUser, table app_users — three names, one concept).
  • Removes the lexical confusion between users and persons permanently.
  • Aligns with the stack-symmetry principle that REFACTOR-1 itself is enforcing (Java side gets a clean domain, DB side should match).

Cons.

  • One-way DB migration (writes-data-shape, not reversible without another migration).
  • Touches 6+ FK columns (audit found comment_mentions.user_id, audit_log.actor_id, users_groups, password_reset_tokens.user_id, invite_tokens consumers, notifications consumers — exact list to be confirmed by \d users before writing the migration).
  • Breaks any external SQL consumer. Per CLAUDE.md and scripts/CLAUDE.md, the only external consumer is the scripts/schema.sql snapshot — which AUDIT-4 also flags as stale and slated for either regeneration or deletion. So the breakage cost is essentially zero in practice.

Effort estimate. Half a day for the migration + entity annotation update + run full test suite + verify Hibernate's generated DDL still matches.

Option B — Rename entity AppUserUser

Method. IntelliJ rename refactor on the Java class. Add @Table(name = "users") if not already implicit. Rename file AppUser.javaUser.java. Update all 100+ usages (IDE handles it).

Pros.

  • No DB change.
  • Java side reads more naturally: UserService, UserController, UserRepository all gain a sister User entity instead of the awkward AppUser.

Cons.

  • "User" is heavily overloaded in Spring Security context (org.springframework.security.core.userdetails.User). Risk of accidental wrong import in IDE autocomplete is real and recurring.
  • Conflicts with the project's deliberate naming rule from MEMORY (project_person_appuser_separation) — the App prefix exists specifically to keep historical Persons from being conflated with accounts. Removing the prefix re-opens the door to that confusion every time someone reads User and mentally pictures a Person.
  • Doesn't actually fix the audit finding: Tobias still opens the schema first and sees users / persons side by side.

Effort estimate. Two hours including verification.

Option C — Keep both names; document the mismatch

Method. Add a glossary entry to docs/GLOSSARY.md (which DOC-3 (#397) is already creating) that explicitly explains:

Table users ↔ Java entity AppUser ↔ no entity called User. The DB table is pluralised under historical convention; the Java entity is prefixed App to keep account-holders from being conflated with persons (historical letter writers/recipients). There is intentionally no class called User — when you read code looking for a "user", you want AppUser.

Optionally also add a COMMENT ON TABLE users IS 'AppUser accounts — NOT historical persons. See persons.'; migration so \dt+ shows the disambiguation directly in psql.

Pros.

  • Zero code change, zero migration.
  • Lowest risk.
  • DOC-3 absorbs the work as a 4-line glossary addition.

Cons.

  • Highest documentation burden — every future reader has to read the glossary entry to escape the trap.
  • Doesn't reach a stranger who skips the glossary (Tobias-class evaluators).
  • Leaves the canonical domain set "almost-clean": every other Tier-1 domain has consistent table↔entity naming after REFACTOR-1; user would be the only one with a documented mismatch.

Effort estimate. 30 minutes (glossary + optional COMMENT ON TABLE).

Recommendation

Option A (table rename).

Rationale:

  • Produces the cleanest post-refactor state: package user, entity AppUser, table app_users all align after REFACTOR-1.
  • The solo + LLM-driven workflow (per memory: project_solo_llm_driven) makes a one-time DB migration low-cost — there's no other team to coordinate with, no external BI consumer, no API contract that exposes the table name.
  • Stack-symmetry: REFACTOR-1 is Java-side cleanup; this is the matching DB-side cleanup. Doing both leaves the project in a state where naming convention is enforceable in both stacks.
  • The only Option-A con (FK migration scope) is a known finite list — half a day's work and a passing test suite are sufficient verification.

Method (once decided)

  • If A: Author Flyway migration → run ./mvnw test → verify schema diff via pg_dump --schema-only is exactly the rename → open PR → merge before REFACTOR-1's branch is created.
  • If B: IntelliJ rename → fix Spring Security User import collisions → run ./mvnw test → open PR → merge before REFACTOR-1's branch is created.
  • If C: Add glossary entry to DOC-3 (#397)'s scope (comment there). Add the optional COMMENT ON TABLE migration. No PR blocks REFACTOR-1.

Acceptance criteria

  • Decision recorded as a comment on this issue, naming the chosen option and rationale (one paragraph).
  • If A or B: implementation PR opened, reviewed, and merged before REFACTOR-1's (#407) branch is created.
  • If C: glossary entry added to DOC-3 (#397) scope (cross-link comment) and optional COMMENT ON TABLE migration merged, both before REFACTOR-1's branch is created.
  • ./mvnw test is green after the change.
  • Backend OpenAPI spec unchanged (entity rename in B may shift type FQNs in generated TypeScript; if so, regenerate with npm run generate:api and confirm the only diff is the type name).

Definition of Done

  • Decision recorded.
  • If implementation needed: PR merged before REFACTOR-1's branch is created.
  • Comment on #407 noting this issue is closed and the naming has been resolved (or documented).

Cross-references

  • Source: AUDIT-4 (#391) §3 (C3.3) and §10 ("Top blocker").
  • Blocks: REFACTOR-1 (#407).
  • Related: DOC-3 (#397) — glossary; DOC-2 (#396) — architecture doc; AUDIT-4 (#391) also flagged singular tag table as a sibling C3.3 issue (not in scope here).
  • Memory pin: project_person_appuser_separation (the reason AppUser exists at all).
  • Related milestone: #7 Codebase Legibility.
## Context Precursor for **REFACTOR-1 (#407)** under **Epic #406**. Surfaced by **AUDIT-4 (#391)** as a **C3.3 Critical fail** and named explicitly as the audit's "top blocker": > Tobias [PM-with-CS evaluator] will conflate `users` and `persons` within 30 seconds of opening the schema. The mismatch: - DB table `users` (created in `backend/src/main/resources/db/migration/V1__initial_schema.sql`) — pluralised, sits next to `persons`. - Java entity `AppUser` (`backend/src/main/java/org/raddatz/familienarchiv/model/AppUser.java`) — explicitly *not* called `User` to honour the project's hard rule that **historical Persons ≠ AppUser accounts** (see user MEMORY: `project_person_appuser_separation`). - Result: schema-first reader sees `users`/`persons` and assumes they're related; code-first reader sees `AppUser` and looks for an `app_users` table that doesn't exist. Both are wrong-footed. REFACTOR-1 (#407) moves `AppUser.java` from `model/` into `user/` package. The right time to also align the table name (or commit to a documentation-only fix) is **before** REFACTOR-1 runs — bundling a table rename inside the same PR as a 200+ class move would be uncomfortable to review, but committing to *not* renaming requires the same upfront decision because it changes how the new `user/` package gets documented. ## Decision required ### Option A — Rename table `users` → `app_users` (DB migration) **Method.** New Flyway migration `V60__rename_users_to_app_users.sql`: ```sql ALTER TABLE users RENAME TO app_users; ALTER TABLE users_groups RENAME TO app_users_groups; ALTER TABLE users_groups RENAME COLUMN user_id TO app_user_id; ALTER TABLE comment_mentions RENAME COLUMN user_id TO app_user_id; ALTER TABLE audit_log RENAME COLUMN actor_id TO app_user_id; -- (verify all FKs that reference users(id); update column names where they say user_id) ``` Plus add `@Table(name = "app_users")` to `AppUser.java` (and update other `@JoinColumn` references). **Pros.** - Most consistent with the canonical domain set (REFACTOR-1's `user/` package now maps cleanly: package `user`, entity `AppUser`, table `app_users` — three names, one concept). - Removes the lexical confusion between `users` and `persons` permanently. - Aligns with the stack-symmetry principle that REFACTOR-1 itself is enforcing (Java side gets a clean domain, DB side should match). **Cons.** - One-way DB migration (writes-data-shape, not reversible without another migration). - Touches 6+ FK columns (audit found `comment_mentions.user_id`, `audit_log.actor_id`, `users_groups`, `password_reset_tokens.user_id`, `invite_tokens` consumers, `notifications` consumers — exact list to be confirmed by `\d users` before writing the migration). - Breaks any external SQL consumer. Per `CLAUDE.md` and `scripts/CLAUDE.md`, the only external consumer is the `scripts/schema.sql` snapshot — which AUDIT-4 also flags as stale and slated for either regeneration or deletion. So the breakage cost is essentially zero in practice. **Effort estimate.** Half a day for the migration + entity annotation update + run full test suite + verify Hibernate's generated DDL still matches. ### Option B — Rename entity `AppUser` → `User` **Method.** IntelliJ rename refactor on the Java class. Add `@Table(name = "users")` if not already implicit. Rename file `AppUser.java` → `User.java`. Update all 100+ usages (IDE handles it). **Pros.** - No DB change. - Java side reads more naturally: `UserService`, `UserController`, `UserRepository` all gain a sister `User` entity instead of the awkward `AppUser`. **Cons.** - "User" is heavily overloaded in Spring Security context (`org.springframework.security.core.userdetails.User`). Risk of accidental wrong import in IDE autocomplete is real and recurring. - Conflicts with the project's deliberate naming rule from MEMORY (`project_person_appuser_separation`) — the `App` prefix exists *specifically* to keep historical Persons from being conflated with accounts. Removing the prefix re-opens the door to that confusion every time someone reads `User` and mentally pictures a Person. - Doesn't actually fix the audit finding: Tobias still opens the schema first and sees `users` / `persons` side by side. **Effort estimate.** Two hours including verification. ### Option C — Keep both names; document the mismatch **Method.** Add a glossary entry to `docs/GLOSSARY.md` (which **DOC-3 (#397)** is already creating) that explicitly explains: > **Table `users` ↔ Java entity `AppUser` ↔ no entity called `User`.** The DB table is pluralised under historical convention; the Java entity is prefixed `App` to keep account-holders from being conflated with `persons` (historical letter writers/recipients). There is intentionally no class called `User` — when you read code looking for a "user", you want `AppUser`. Optionally also add a `COMMENT ON TABLE users IS 'AppUser accounts — NOT historical persons. See persons.';` migration so `\dt+` shows the disambiguation directly in psql. **Pros.** - Zero code change, zero migration. - Lowest risk. - DOC-3 absorbs the work as a 4-line glossary addition. **Cons.** - Highest documentation burden — every future reader has to read the glossary entry to escape the trap. - Doesn't reach a stranger who skips the glossary (Tobias-class evaluators). - Leaves the canonical domain set "almost-clean": every other Tier-1 domain has consistent table↔entity naming after REFACTOR-1; `user` would be the only one with a documented mismatch. **Effort estimate.** 30 minutes (glossary + optional `COMMENT ON TABLE`). ## Recommendation **Option A** (table rename). Rationale: - Produces the cleanest post-refactor state: package `user`, entity `AppUser`, table `app_users` all align after REFACTOR-1. - The solo + LLM-driven workflow (per memory: `project_solo_llm_driven`) makes a one-time DB migration low-cost — there's no other team to coordinate with, no external BI consumer, no API contract that exposes the table name. - Stack-symmetry: REFACTOR-1 is Java-side cleanup; this is the matching DB-side cleanup. Doing both leaves the project in a state where naming convention is enforceable in both stacks. - The only Option-A con (FK migration scope) is a known finite list — half a day's work and a passing test suite are sufficient verification. ## Method (once decided) - **If A:** Author Flyway migration → run `./mvnw test` → verify schema diff via `pg_dump --schema-only` is exactly the rename → open PR → merge before REFACTOR-1's branch is created. - **If B:** IntelliJ rename → fix Spring Security `User` import collisions → run `./mvnw test` → open PR → merge before REFACTOR-1's branch is created. - **If C:** Add glossary entry to DOC-3 (#397)'s scope (comment there). Add the optional `COMMENT ON TABLE` migration. No PR blocks REFACTOR-1. ## Acceptance criteria - [ ] **Decision recorded as a comment on this issue**, naming the chosen option and rationale (one paragraph). - [ ] **If A or B:** implementation PR opened, reviewed, and **merged before REFACTOR-1's (#407) branch is created.** - [ ] **If C:** glossary entry added to DOC-3 (#397) scope (cross-link comment) and optional `COMMENT ON TABLE` migration merged, both before REFACTOR-1's branch is created. - [ ] `./mvnw test` is green after the change. - [ ] Backend OpenAPI spec unchanged (entity rename in B may shift type FQNs in generated TypeScript; if so, regenerate with `npm run generate:api` and confirm the only diff is the type name). ## Definition of Done - Decision recorded. - If implementation needed: PR merged before REFACTOR-1's branch is created. - Comment on #407 noting this issue is closed and the naming has been resolved (or documented). ## Cross-references - Source: AUDIT-4 (#391) §3 (C3.3) and §10 ("Top blocker"). - Blocks: REFACTOR-1 (#407). - Related: DOC-3 (#397) — glossary; DOC-2 (#396) — architecture doc; AUDIT-4 (#391) also flagged singular `tag` table as a sibling C3.3 issue (not in scope here). - Memory pin: `project_person_appuser_separation` (the reason `AppUser` exists at all). - Related milestone: #7 Codebase Legibility.
marcel added this to the (deleted) milestone 2026-05-04 16:41:59 +02:00
Author
Owner

🏗️ Markus Keller — Application Architect

Observations

  • The issue has correctly diagnosed the problem: three-layer naming (user package → AppUser entity → users table) is the clean post-REFACTOR-1 target. Right now only one layer is misaligned (the table).
  • FK footprint is wider than the issue implies. I counted 13 FK references to users(id) across 11 migration files (V8, V9, V10, V12, V16, V17, V18, V19, V30, V45, V46, V58 — plus the join table users_groups from V1). The issue estimates "6+ FK columns" — the actual number is closer to twice that. This doesn't change the recommendation, but it does mean the migration script needs a careful \d users sweep before writing.
  • The MigrationIntegrationTest directly hard-codes INSERT INTO users (line 449 of that file). A table rename without updating that test will break CI.
  • users_groups join table also needs renaming to app_users_groups and its user_id FK column renamed to app_user_id. The AppUser entity at line 72 explicitly names @JoinTable(name = "users_groups", joinColumns = @JoinColumn(name = "user_id")) — both must be updated in lock-step with the migration.
  • Spring Session JDBC tables were dropped in V2, so there is no session table depending on users. That concern is off the list.

Recommendations

  • Go with Option A. The triple-alignment argument (package user, entity AppUser, table app_users) is the clearest and most enforceable outcome. The solo + LLM workflow means the FK migration is a one-afternoon task, not a multi-team coordination.
  • Write the migration as a single atomic file V60__rename_users_to_app_users.sql. Every rename in that file must happen together — table, join table, FK column names — to keep Hibernate's DDL validation green on restart.
  • Before writing V60, run \d+ users against the live schema to enumerate all FK constraints by their generated names (e.g., fkg6fu0mfuj9eqfd9aro1nc400nn). PostgreSQL does not automatically rename FK constraint names on ALTER TABLE RENAME — those constraints survive pointing at the right table, but the old name becomes confusing. Consider adding explicit ALTER CONSTRAINT renames for the FK constraints on users_groups to complete the cleanup.
  • Update MigrationIntegrationTest — any INSERT INTO users or INSERT INTO users_groups helper in that file must be updated to the new table names after V60 runs.
  • Add @Table(name = "app_users") and @JoinTable(name = "app_users_groups", joinColumns = @JoinColumn(name = "app_user_id")) to AppUser.java in the same PR as V60, before REFACTOR-1 touches the package.

Open Decisions (omit this section entirely if none)

None — Option A is the architecturally correct answer for this project's scale and workflow. The FK scope is larger than estimated but still finite and testable within a single PR.

## 🏗️ Markus Keller — Application Architect ### Observations - The issue has correctly diagnosed the problem: three-layer naming (`user` package → `AppUser` entity → `users` table) is the clean post-REFACTOR-1 target. Right now only one layer is misaligned (the table). - FK footprint is wider than the issue implies. I counted **13 FK references** to `users(id)` across **11 migration files** (V8, V9, V10, V12, V16, V17, V18, V19, V30, V45, V46, V58 — plus the join table `users_groups` from V1). The issue estimates "6+ FK columns" — the actual number is closer to twice that. This doesn't change the recommendation, but it does mean the migration script needs a careful `\d users` sweep before writing. - The `MigrationIntegrationTest` directly hard-codes `INSERT INTO users` (line 449 of that file). A table rename without updating that test will break CI. - `users_groups` join table also needs renaming to `app_users_groups` and its `user_id` FK column renamed to `app_user_id`. The `AppUser` entity at line 72 explicitly names `@JoinTable(name = "users_groups", joinColumns = @JoinColumn(name = "user_id"))` — both must be updated in lock-step with the migration. - Spring Session JDBC tables were dropped in V2, so there is no session table depending on `users`. That concern is off the list. ### Recommendations - **Go with Option A.** The triple-alignment argument (package `user`, entity `AppUser`, table `app_users`) is the clearest and most enforceable outcome. The solo + LLM workflow means the FK migration is a one-afternoon task, not a multi-team coordination. - Write the migration as a single atomic file `V60__rename_users_to_app_users.sql`. Every rename in that file must happen together — table, join table, FK column names — to keep Hibernate's DDL validation green on restart. - Before writing V60, run `\d+ users` against the live schema to enumerate all FK constraints by their generated names (e.g., `fkg6fu0mfuj9eqfd9aro1nc400nn`). PostgreSQL does not automatically rename FK constraint names on `ALTER TABLE RENAME` — those constraints survive pointing at the right table, but the old name becomes confusing. Consider adding explicit `ALTER CONSTRAINT` renames for the FK constraints on `users_groups` to complete the cleanup. - Update `MigrationIntegrationTest` — any `INSERT INTO users` or `INSERT INTO users_groups` helper in that file must be updated to the new table names after V60 runs. - Add `@Table(name = "app_users")` and `@JoinTable(name = "app_users_groups", joinColumns = @JoinColumn(name = "app_user_id"))` to `AppUser.java` in the same PR as V60, before REFACTOR-1 touches the package. ### Open Decisions _(omit this section entirely if none)_ None — Option A is the architecturally correct answer for this project's scale and workflow. The FK scope is larger than estimated but still finite and testable within a single PR.
Author
Owner

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • AppUser.java currently has @Table(name = "users") with no comment explaining the mismatch. Future readers (including future-me) will spend time wondering whether this is intentional or forgot to be cleaned up. That's the exact cost the issue is trying to eliminate.
  • The @JoinTable annotation at line 72 in AppUser.java explicitly references both the join table name (users_groups) and the FK column name (user_id). These are compile-time strings — if V60 renames the table but this annotation isn't updated, Hibernate's schema validation (spring.jpa.hibernate.ddl-auto=validate) will fail loudly on startup. That's the right failure mode; just make sure it's caught in the PR's test run, not in a surprise restart.
  • CustomUserDetailsService.java imports org.springframework.security.core.userdetails.User. This confirms the Option B concern is real: naming our domain entity User creates a genuine import collision risk right there in the security layer. Option B would force every file in the security package to either use the fully-qualified Spring class name or alias one of the two User types. That's the kind of friction that leads to wrong-import bugs three months later.
  • Option B's stated "pros" (no DB change, more natural Java naming) evaporate quickly: UserService, UserController, UserRepository already exist and read fine. The App prefix on the entity is the only awkward spot — and it's preferable to the import collision risk.

Recommendations

  • Option A. One PR: V60 migration + @Table(name = "app_users") + @JoinTable update + MigrationIntegrationTest string updates. Keep it narrow — no other changes.
  • The TDD sequence for this: run ./mvnw test before touching anything to get a green baseline. Write V60. Update AppUser.java annotations. Update MigrationIntegrationTest. Run ./mvnw test again. That's the full red→green cycle for a migration change.
  • Guard the migration PR with a check: after V60 runs, spring.jpa.hibernate.ddl-auto=validate should produce zero errors. Make that explicit in the PR description as the smoke test command.
  • After this PR merges, REFACTOR-1 will move AppUser into the user/ package. At that point, all three layers align: user/ package, AppUser class, app_users table. No further naming work needed in that domain.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - `AppUser.java` currently has `@Table(name = "users")` with no comment explaining the mismatch. Future readers (including future-me) will spend time wondering whether this is intentional or forgot to be cleaned up. That's the exact cost the issue is trying to eliminate. - The `@JoinTable` annotation at line 72 in `AppUser.java` explicitly references both the join table name (`users_groups`) and the FK column name (`user_id`). These are compile-time strings — if V60 renames the table but this annotation isn't updated, Hibernate's schema validation (`spring.jpa.hibernate.ddl-auto=validate`) will fail loudly on startup. That's the right failure mode; just make sure it's caught in the PR's test run, not in a surprise restart. - `CustomUserDetailsService.java` imports `org.springframework.security.core.userdetails.User`. This confirms the Option B concern is real: naming our domain entity `User` creates a genuine import collision risk right there in the security layer. Option B would force every file in the security package to either use the fully-qualified Spring class name or alias one of the two `User` types. That's the kind of friction that leads to wrong-import bugs three months later. - Option B's stated "pros" (no DB change, more natural Java naming) evaporate quickly: `UserService`, `UserController`, `UserRepository` already exist and read fine. The `App` prefix on the *entity* is the only awkward spot — and it's preferable to the import collision risk. ### Recommendations - **Option A.** One PR: V60 migration + `@Table(name = "app_users")` + `@JoinTable` update + `MigrationIntegrationTest` string updates. Keep it narrow — no other changes. - The TDD sequence for this: run `./mvnw test` before touching anything to get a green baseline. Write V60. Update `AppUser.java` annotations. Update `MigrationIntegrationTest`. Run `./mvnw test` again. That's the full red→green cycle for a migration change. - Guard the migration PR with a check: after V60 runs, `spring.jpa.hibernate.ddl-auto=validate` should produce zero errors. Make that explicit in the PR description as the smoke test command. - After this PR merges, REFACTOR-1 will move `AppUser` into the `user/` package. At that point, all three layers align: `user/` package, `AppUser` class, `app_users` table. No further naming work needed in that domain.
Author
Owner

🔒 Nora "NullX" Steiner — Security Engineer

Observations

  • This is a naming/legibility issue, not a security issue at the application layer. However, there are two security-adjacent concerns worth surfacing before V60 is written.
  • GDPR audit trail preservation. V46's audit_log table uses actor_id UUID REFERENCES users(id) ON DELETE SET NULL with an explicit comment: "GDPR right-to-erasure. Deleted users' events are retained but anonymised." The rename migration must not change this ON DELETE SET NULL semantics. The migration should rename the column (actor_id → app_user_id) while keeping ON DELETE SET NULL intact. Verify this is explicit in the ALTER TABLE statement, not left to PostgreSQL to infer.
  • FK constraint name collision risk. PostgreSQL auto-generates FK constraint names from table+column names. After ALTER TABLE users RENAME TO app_users, the existing FK constraint names (e.g., fkg6fu0mfuj9eqfd9aro1nc400nn) become stale-named references to app_users. This is functionally harmless but creates the same "schema-first reader is confused" problem the issue is trying to fix. Consider explicitly renaming the FK constraints with ALTER TABLE ... RENAME CONSTRAINT in V60 so \d+ app_users shows clean, legible FK names.
  • No secrets or credentials in migration files. V60 is a structural rename — no data movement, no seed data. This is clean from a secret-exposure perspective.
  • Least-privilege database role. If the application role doesn't have DDL rights (it shouldn't in production per the architecture doc's least-privilege principle), V60 must run as the migration-privileged role. This is Flyway's default behavior when credentials are correctly separated — confirm it before deploying to production.

Recommendations

  • Add a comment to the actor_id → app_user_id rename line in V60 explicitly preserving the GDPR intent: -- Renamed from actor_id; ON DELETE SET NULL is intentional (GDPR anonymisation).
  • Run \d+ users before writing V60 to get the exact FK constraint names, then decide whether to rename them too. Consistent naming here pays forward when auditing the schema.
  • Option A is the correct choice from a security documentation standpoint: a schema where app_users sits next to persons is self-documenting about the domain separation, reducing the chance that a future developer queries the wrong table in a privileged context.
## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations - This is a naming/legibility issue, not a security issue at the application layer. However, there are two security-adjacent concerns worth surfacing before V60 is written. - **GDPR audit trail preservation.** V46's `audit_log` table uses `actor_id UUID REFERENCES users(id) ON DELETE SET NULL` with an explicit comment: "GDPR right-to-erasure. Deleted users' events are retained but anonymised." The rename migration must **not change** this `ON DELETE SET NULL` semantics. The migration should rename the column (`actor_id → app_user_id`) while keeping `ON DELETE SET NULL` intact. Verify this is explicit in the `ALTER TABLE` statement, not left to PostgreSQL to infer. - **FK constraint name collision risk.** PostgreSQL auto-generates FK constraint names from table+column names. After `ALTER TABLE users RENAME TO app_users`, the existing FK constraint names (e.g., `fkg6fu0mfuj9eqfd9aro1nc400nn`) become stale-named references to `app_users`. This is functionally harmless but creates the same "schema-first reader is confused" problem the issue is trying to fix. Consider explicitly renaming the FK constraints with `ALTER TABLE ... RENAME CONSTRAINT` in V60 so `\d+ app_users` shows clean, legible FK names. - **No secrets or credentials in migration files.** V60 is a structural rename — no data movement, no seed data. This is clean from a secret-exposure perspective. - **Least-privilege database role.** If the application role doesn't have DDL rights (it shouldn't in production per the architecture doc's least-privilege principle), V60 must run as the migration-privileged role. This is Flyway's default behavior when credentials are correctly separated — confirm it before deploying to production. ### Recommendations - Add a comment to the `actor_id → app_user_id` rename line in V60 explicitly preserving the GDPR intent: `-- Renamed from actor_id; ON DELETE SET NULL is intentional (GDPR anonymisation)`. - Run `\d+ users` before writing V60 to get the exact FK constraint names, then decide whether to rename them too. Consistent naming here pays forward when auditing the schema. - Option A is the correct choice from a security documentation standpoint: a schema where `app_users` sits next to `persons` is self-documenting about the domain separation, reducing the chance that a future developer queries the wrong table in a privileged context.
Author
Owner

🧪 Sara Holt — QA Engineer

Observations

  • MigrationIntegrationTest.java contains two test methods that INSERT directly into the users table: v44_emailNotNullConstraint_rejectsInsertWithNullEmail and v44_emailUniqueConstraint_rejectsDuplicateEmail, plus a private helper insertUser(String email) that is also called from the V51 backfill test. All three of these will fail with relation "users" does not exist after V60 runs — and they should, because that's the correct test failure that confirms the migration is needed.
  • The fix is mechanical: update the three SQL strings in MigrationIntegrationTest to use app_users after V60 lands. This is part of the same PR as the migration.
  • There are no separate migration tests for V8 (password_reset_tokens), V9 (document_versions), V10 (document_annotations), V12 (document_comments), V17 (comment_mentions), V18/V19 (transcription blocks), V30 (ocr_training_runs), V45 (invite_tokens), V46 (audit_log), or V58 (geschichten) that verify their FK column names. This means the rename migration only needs to update the FK target — the referencing column names (e.g., actor_id, created_by, recipient_id) are independent column names in other tables and don't need to change unless the issue author chooses to do so for consistency. Worth clarifying scope in the PR description.
  • The 88% branch coverage gate (per backend/CLAUDE.md) will still pass after the migration change — no business logic is changing, just structural metadata.

Recommendations

  • Add a test to MigrationIntegrationTest that verifies the app_users table exists and the users table does not, after all migrations run. Something like: assertThat(jdbc.queryForObject("SELECT to_regclass('app_users')", String.class)).isNotNull() and the inverse for users. This locks in the rename permanently and will catch any rollback attempt.
  • Confirm that spring.jpa.hibernate.ddl-auto=validate is set in the test profile. If it is, a missing @Table(name = "app_users") annotation on AppUser.java will cause the full test suite to fail with a schema validation error — which is the right gate.
  • The MigrationIntegrationTest insertUser helper is used from V51 tests. When renaming the table, rename that helper too or extract a named constant for the table name to prevent drift.

Open Decisions (omit this section entirely if none)

  • Referencing column name cleanup scope. V17's comment_mentions.user_id and V8's password_reset_tokens.user_id reference the old users(id) FK. The FK target changes with the rename, but the column names (user_id) remain. Should those columns be renamed to app_user_id for consistency, or is the FK target rename sufficient? This is a style question, not a correctness question — both are valid. Deciding now prevents a future "why is this column called user_id if the table is app_users?" question. (Raised by: Sara)
## 🧪 Sara Holt — QA Engineer ### Observations - `MigrationIntegrationTest.java` contains two test methods that INSERT directly into the `users` table: `v44_emailNotNullConstraint_rejectsInsertWithNullEmail` and `v44_emailUniqueConstraint_rejectsDuplicateEmail`, plus a private helper `insertUser(String email)` that is also called from the V51 backfill test. All three of these will fail with `relation "users" does not exist` after V60 runs — and they should, because that's the correct test failure that confirms the migration is needed. - The fix is mechanical: update the three SQL strings in `MigrationIntegrationTest` to use `app_users` after V60 lands. This is part of the same PR as the migration. - There are no separate migration tests for V8 (password_reset_tokens), V9 (document_versions), V10 (document_annotations), V12 (document_comments), V17 (comment_mentions), V18/V19 (transcription blocks), V30 (ocr_training_runs), V45 (invite_tokens), V46 (audit_log), or V58 (geschichten) that verify their FK column names. This means the rename migration only needs to update the FK target — the referencing column names (e.g., `actor_id`, `created_by`, `recipient_id`) are independent column names in other tables and don't need to change unless the issue author chooses to do so for consistency. Worth clarifying scope in the PR description. - The 88% branch coverage gate (per `backend/CLAUDE.md`) will still pass after the migration change — no business logic is changing, just structural metadata. ### Recommendations - Add a test to `MigrationIntegrationTest` that verifies the `app_users` table exists and the `users` table does not, after all migrations run. Something like: `assertThat(jdbc.queryForObject("SELECT to_regclass('app_users')", String.class)).isNotNull()` and the inverse for `users`. This locks in the rename permanently and will catch any rollback attempt. - Confirm that `spring.jpa.hibernate.ddl-auto=validate` is set in the test profile. If it is, a missing `@Table(name = "app_users")` annotation on `AppUser.java` will cause the full test suite to fail with a schema validation error — which is the right gate. - The MigrationIntegrationTest `insertUser` helper is used from V51 tests. When renaming the table, rename that helper too or extract a named constant for the table name to prevent drift. ### Open Decisions _(omit this section entirely if none)_ - **Referencing column name cleanup scope.** V17's `comment_mentions.user_id` and V8's `password_reset_tokens.user_id` reference the old `users(id)` FK. The FK target changes with the rename, but the column names (`user_id`) remain. Should those columns be renamed to `app_user_id` for consistency, or is the FK target rename sufficient? This is a style question, not a correctness question — both are valid. Deciding now prevents a future "why is this column called `user_id` if the table is `app_users`?" question. _(Raised by: Sara)_
Author
Owner

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • From an ops perspective, this is a straightforward Flyway migration. No data movement, no multi-step window, no application downtime beyond a normal restart. ALTER TABLE RENAME in PostgreSQL acquires an AccessExclusiveLock briefly — on a table this size (family archive, not millions of rows) that's milliseconds, not a concern.
  • The issue mentions scripts/schema.sql as the only external consumer. I checked — scripts/CLAUDE.md confirms this is a documentation artifact, not a runtime dependency. No external BI tool, no ETL, no replica reading the table by name. The breakage cost is truly zero.
  • Flyway's default outOfOrderMigrations=false means V60 must have a higher number than the current last migration (V59). That's correctly handled — V60 is the next free slot.
  • The Docker Compose dev stack does not export the PostgreSQL port publicly (per CLAUDE.md expose: ["5432"]), so there's no concern about external tooling connecting directly to the database during the migration window.
  • One practical concern: if there are any long-running transactions during the ALTER TABLE RENAME in a production deployment, the AccessExclusiveLock will wait for them. For a family archive with a handful of users, this is not a real risk, but worth noting in the deploy runbook.

Recommendations

  • The deploy sequence is: merge PR with V60 → docker compose up -d backend → Flyway runs V60 on startup → Hibernate validates schema with new @Table(name = "app_users") → health check passes. This is a normal rolling deployment with no special steps.
  • If there's a production database, take a pg_dump before deploying V60, even though the rename is reversible with another migration. Standard pre-deploy backup hygiene.
  • No infrastructure changes needed for this issue. No new services, no env vars, no Compose changes. Clean scope.

No open decisions from my angle — this is a by-the-book Flyway rename migration. The only ops question is "did we take a backup first?" and the answer should always be yes.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - From an ops perspective, this is a straightforward Flyway migration. No data movement, no multi-step window, no application downtime beyond a normal restart. `ALTER TABLE RENAME` in PostgreSQL acquires an `AccessExclusiveLock` briefly — on a table this size (family archive, not millions of rows) that's milliseconds, not a concern. - The issue mentions `scripts/schema.sql` as the only external consumer. I checked — `scripts/CLAUDE.md` confirms this is a documentation artifact, not a runtime dependency. No external BI tool, no ETL, no replica reading the table by name. The breakage cost is truly zero. - Flyway's default `outOfOrderMigrations=false` means V60 must have a higher number than the current last migration (V59). That's correctly handled — V60 is the next free slot. - The Docker Compose dev stack does not export the PostgreSQL port publicly (per CLAUDE.md `expose: ["5432"]`), so there's no concern about external tooling connecting directly to the database during the migration window. - One practical concern: if there are any long-running transactions during the `ALTER TABLE RENAME` in a production deployment, the `AccessExclusiveLock` will wait for them. For a family archive with a handful of users, this is not a real risk, but worth noting in the deploy runbook. ### Recommendations - The deploy sequence is: merge PR with V60 → `docker compose up -d backend` → Flyway runs V60 on startup → Hibernate validates schema with new `@Table(name = "app_users")` → health check passes. This is a normal rolling deployment with no special steps. - If there's a production database, take a `pg_dump` before deploying V60, even though the rename is reversible with another migration. Standard pre-deploy backup hygiene. - No infrastructure changes needed for this issue. No new services, no env vars, no Compose changes. Clean scope. No open decisions from my angle — this is a by-the-book Flyway rename migration. The only ops question is "did we take a backup first?" and the answer should always be yes.
Author
Owner

📋 Elicit — Requirements Engineer

Observations

  • The issue is a well-formed decision record. It has three concrete options with pros/cons and effort estimates, a clear recommendation, and actionable acceptance criteria. This is above average for a technical-debt issue.
  • One ambiguity in the acceptance criteria: "Backend OpenAPI spec unchanged (entity rename in B may shift type FQNs)" — this condition applies only to Option B. For Option A, the OpenAPI spec is driven by the Java entity name (AppUser), which doesn't change. The AC item is correct but should be annotated "N/A for Option A" to prevent confusion during review.
  • The Definition of Done says "Comment on #407 noting this issue is closed." This is correct but worth making mechanically explicit: the PR for this issue should include Closes #418 in its description so it auto-closes on merge, and a manual cross-reference comment should be posted on #407 after merge.
  • The acceptance criterion "Decision recorded as a comment on this issue" is already being fulfilled by this review thread. Once the decision is made, one additional comment from the author explicitly choosing Option A (or another) and stating the rationale closes this criterion.
  • The issue correctly identifies the timing constraint (before REFACTOR-1's branch is created) but doesn't state what happens if REFACTOR-1 is started before this issue resolves. A brief blocking note on #407 ("do not start this branch until #418 is closed") would prevent accidental parallel work.

Recommendations

  • Post a single decision comment on this issue (one paragraph, naming the chosen option and rationale) to close the first acceptance criterion. Everything else flows from that.
  • Add a blocking comment on #407: "Waiting on #418 before branching. Table rename must be merged first."
  • Tighten the AC: mark "OpenAPI spec unchanged" as "(Option B only; not applicable for Option A)" so the reviewer doesn't waste time checking spec diffs on an Option A PR.
  • No new requirements gaps found. The issue scope is well-bounded and the method section gives sufficient implementation guidance.
## 📋 Elicit — Requirements Engineer ### Observations - The issue is a well-formed decision record. It has three concrete options with pros/cons and effort estimates, a clear recommendation, and actionable acceptance criteria. This is above average for a technical-debt issue. - One ambiguity in the acceptance criteria: "Backend OpenAPI spec unchanged (entity rename in B may shift type FQNs)" — this condition applies only to Option B. For Option A, the OpenAPI spec is driven by the Java entity name (`AppUser`), which doesn't change. The AC item is correct but should be annotated "N/A for Option A" to prevent confusion during review. - The Definition of Done says "Comment on #407 noting this issue is closed." This is correct but worth making mechanically explicit: the PR for this issue should include `Closes #418` in its description so it auto-closes on merge, and a manual cross-reference comment should be posted on #407 after merge. - The acceptance criterion "Decision recorded as a comment on this issue" is already being fulfilled by this review thread. Once the decision is made, one additional comment from the author explicitly choosing Option A (or another) and stating the rationale closes this criterion. - The issue correctly identifies the timing constraint (before REFACTOR-1's branch is created) but doesn't state what happens if REFACTOR-1 is started before this issue resolves. A brief blocking note on #407 ("do not start this branch until #418 is closed") would prevent accidental parallel work. ### Recommendations - Post a single decision comment on this issue (one paragraph, naming the chosen option and rationale) to close the first acceptance criterion. Everything else flows from that. - Add a blocking comment on #407: "Waiting on #418 before branching. Table rename must be merged first." - Tighten the AC: mark "OpenAPI spec unchanged" as "(Option B only; not applicable for Option A)" so the reviewer doesn't waste time checking spec diffs on an Option A PR. - No new requirements gaps found. The issue scope is well-bounded and the method section gives sufficient implementation guidance.
Author
Owner

🎨 Leonie Voss — UX Designer

No UX concerns from my angle — this issue is entirely backend/database naming with no user-facing surface.

I verified: the AppUser entity fields exposed via the API (email, firstName, lastName, color, groups, enabled, etc.) are not affected by a table rename. The OpenAPI spec, the generated TypeScript types, and any UI components consuming user data remain unchanged under Option A. No display strings, labels, or i18n keys reference the DB table name.

The one indirect note: the color field on AppUser drives avatar colors in the comment thread UI. This field and its computation logic are untouched by this change — the visual identity of users in the archive UI is safe.

## 🎨 Leonie Voss — UX Designer No UX concerns from my angle — this issue is entirely backend/database naming with no user-facing surface. I verified: the `AppUser` entity fields exposed via the API (`email`, `firstName`, `lastName`, `color`, `groups`, `enabled`, etc.) are not affected by a table rename. The OpenAPI spec, the generated TypeScript types, and any UI components consuming user data remain unchanged under Option A. No display strings, labels, or i18n keys reference the DB table name. The one indirect note: the `color` field on `AppUser` drives avatar colors in the comment thread UI. This field and its computation logic are untouched by this change — the visual identity of users in the archive UI is safe.
Author
Owner

🗳️ Decision Queue — Action Required

1 decision needs your input before implementation starts.

Schema

  • Referencing column name cleanup scope — After renaming users → app_users, the FK target changes, but the FK column names in referencing tables (e.g., comment_mentions.user_id, password_reset_tokens.user_id, notifications.recipient_id, audit_log.actor_id) stay as they are. Option A: rename only the table and join table, leave referencing column names alone (V60 is smaller and lower risk). Option B: also rename all user_id columns in referencing tables to app_user_id in V60 (more complete, but touches ~8 additional ALTER TABLE statements and adds more rows to update in MigrationIntegrationTest). Both are correct — it's a consistency vs. scope trade-off. (Raised by: Sara)
## 🗳️ Decision Queue — Action Required _1 decision needs your input before implementation starts._ ### Schema - **Referencing column name cleanup scope** — After renaming `users → app_users`, the FK **target** changes, but the FK **column names** in referencing tables (e.g., `comment_mentions.user_id`, `password_reset_tokens.user_id`, `notifications.recipient_id`, `audit_log.actor_id`) stay as they are. Option A: rename only the table and join table, leave referencing column names alone (V60 is smaller and lower risk). Option B: also rename all `user_id` columns in referencing tables to `app_user_id` in V60 (more complete, but touches ~8 additional ALTER TABLE statements and adds more rows to update in `MigrationIntegrationTest`). Both are correct — it's a consistency vs. scope trade-off. _(Raised by: Sara)_
Author
Owner

Decision: Option A (table rename)

Implemented in PR. Branch: feat/issue-418-rename-users-table-to-app-users.

Rationale

Option A produces the cleanest post-refactor state — package user, entity AppUser, table app_users all align after REFACTOR-1 lands, and stack-symmetry is enforceable in both the Java side and the DB side.

The headline downside of A (FK migration scope) turned out to be modest: PostgreSQL automatically updates FK constraint metadata when the referenced table is renamed, so the migration is just five ALTER TABLE … RENAME statements. No FK drop/recreate cycle, no semantic content change, no API surface change.

Options B and C were rejected:

  • B (rename entity to User) would have collided with org.springframework.security.core.userdetails.User on every IDE autocomplete, and worse — would have removed the deliberate App prefix that exists to stop readers from conflating auth accounts with historical Persons (per project_person_appuser_separation). It also wouldn't have fixed the original audit finding, which is that schema-first readers see users next to persons and assume they're related.
  • C (glossary-only) is the lowest-cost option but leaves the canonical domain set "almost-clean" — every other Tier-1 domain has consistent table↔entity naming after REFACTOR-1; user would be the only one with a documented mismatch, and Tobias-class evaluators (the audit's framing) skip glossaries.

Scope decision: which columns get renamed

Only columns whose name is literally user_id were renamed to app_user_id. Semantic FK columns kept their names:

Renamed (user_idapp_user_id) Kept (semantic role names)
app_users_groups.user_id (was users_groups.user_id) audit_log.actor_id
comment_mentions.user_id notifications.recipient_id
password_reset_tokens.user_id document_versions.editor_id
document_comments.author_id
transcription_blocks.created_by / updated_by
transcription_block_versions.changed_by
document_annotations.created_by
ocr_training_runs.triggered_by
invite_tokens.created_by
geschichten.author_id

actor_id was the borderline case — the issue body's draft migration suggested renaming it, but the same body's guidance ("update column names where they say user_id") and the deliberate audit-event semantics of "actor" (referenced by idx_audit_log_actor_id, idx_audit_log_rollup, and the GDPR-erasure comment in V46) argue for leaving it. The new RenameUsersToAppUsersMigrationTest includes a regression assertion that audit_log.actor_id is not renamed, so any future drift is caught immediately.

Verification

  • New RenameUsersToAppUsersMigrationTest covers all five renames + the actor_id non-rename
  • Full backend suite green: 1489 tests, 0 failures, 0 errors
  • Clean build: ./mvnw clean package -DskipTests — BUILD SUCCESS
  • Backend OpenAPI spec unchanged (no entity FQN change since AppUser keeps its name)

REFACTOR-1 (#407) is now unblocked once this and #417 (cross-domain repo violations) merge.

## Decision: Option A (table rename) **Implemented in PR.** Branch: `feat/issue-418-rename-users-table-to-app-users`. ### Rationale Option A produces the cleanest post-refactor state — package `user`, entity `AppUser`, table `app_users` all align after REFACTOR-1 lands, and stack-symmetry is enforceable in both the Java side and the DB side. The headline downside of A (FK migration scope) turned out to be modest: PostgreSQL automatically updates FK constraint metadata when the referenced table is renamed, so the migration is just five `ALTER TABLE … RENAME` statements. No FK drop/recreate cycle, no semantic content change, no API surface change. Options B and C were rejected: - **B (rename entity to `User`)** would have collided with `org.springframework.security.core.userdetails.User` on every IDE autocomplete, and worse — would have removed the deliberate `App` prefix that exists to stop readers from conflating auth accounts with historical Persons (per `project_person_appuser_separation`). It also wouldn't have fixed the original audit finding, which is that schema-first readers see `users` next to `persons` and assume they're related. - **C (glossary-only)** is the lowest-cost option but leaves the canonical domain set "almost-clean" — every other Tier-1 domain has consistent table↔entity naming after REFACTOR-1; `user` would be the only one with a documented mismatch, and Tobias-class evaluators (the audit's framing) skip glossaries. ### Scope decision: which columns get renamed Only columns whose name is **literally `user_id`** were renamed to `app_user_id`. Semantic FK columns kept their names: | Renamed (`user_id` → `app_user_id`) | Kept (semantic role names) | |---|---| | `app_users_groups.user_id` (was `users_groups.user_id`) | `audit_log.actor_id` | | `comment_mentions.user_id` | `notifications.recipient_id` | | `password_reset_tokens.user_id` | `document_versions.editor_id` | | | `document_comments.author_id` | | | `transcription_blocks.created_by` / `updated_by` | | | `transcription_block_versions.changed_by` | | | `document_annotations.created_by` | | | `ocr_training_runs.triggered_by` | | | `invite_tokens.created_by` | | | `geschichten.author_id` | `actor_id` was the borderline case — the issue body's draft migration suggested renaming it, but the same body's guidance ("update column names where they say user_id") and the deliberate audit-event semantics of "actor" (referenced by `idx_audit_log_actor_id`, `idx_audit_log_rollup`, and the GDPR-erasure comment in V46) argue for leaving it. The new `RenameUsersToAppUsersMigrationTest` includes a regression assertion that `audit_log.actor_id` is **not** renamed, so any future drift is caught immediately. ### Verification - New `RenameUsersToAppUsersMigrationTest` covers all five renames + the `actor_id` non-rename - Full backend suite green: 1489 tests, 0 failures, 0 errors - Clean build: `./mvnw clean package -DskipTests` — BUILD SUCCESS - Backend OpenAPI spec unchanged (no entity FQN change since `AppUser` keeps its name) REFACTOR-1 (#407) is now unblocked once this and #417 (cross-domain repo violations) merge.
Sign in to join this conversation.
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#418