fix(db): add PRIMARY KEY to group_permissions and promote tbmp UNIQUE to PK (#469) #492

Merged

marcel merged 3 commits from fix/issue-469-group-permissions-pk into main 2026-05-09 15:44:35 +02:00

Conversation 8 Commits 3 Files Changed 6 +112 -3

marcel commented 2026-05-09 15:20:31 +02:00

Owner

Copy Link

Summary

Closes #469.

• V63: Deduplicates any phantom (group_id, permission) rows accumulated since the initial schema (uses ctid comparison, safe on fresh DBs as a no-op)
• V64: Sets NOT NULL on permission column and adds pk_group_permissions PRIMARY KEY (group_id, permission)
• V65: Renames the de-facto PK on transcription_block_mentioned_persons — drops uq_tbmp_block_person, adds pk_tbmp PRIMARY KEY (block_id, person_id) — naming-convention consistency
• UserService.createGroup: Switched to builder and added null-guard for dto.permissions — prevents null propagating into the @ElementCollection after V64 adds NOT NULL

Pre-flight query (run against production before deploying)

SELECT group_id, permission, COUNT(*)
FROM group_permissions
GROUP BY group_id, permission HAVING COUNT(*) > 1;

Expected output: zero rows. V63 handles any duplicates found, but documenting the pre-state is good practice.

Rollback

If V64 needs to be reverted after production deploy:

ALTER TABLE group_permissions DROP CONSTRAINT pk_group_permissions;
ALTER TABLE group_permissions ALTER COLUMN permission DROP NOT NULL;

Test plan

• MigrationIntegrationTest#v64_pk_group_permissions_exists — RED before V64, GREEN after
• MigrationIntegrationTest#v64_permission_column_isNotNullable — RED before V64, GREEN after
• MigrationIntegrationTest#v64_rejectsDuplicateGroupPermission — RED before V64, GREEN after (DataIntegrityViolationException on duplicate insert)
• MigrationIntegrationTest#v65_pk_tbmp_exists — RED before V65, GREEN after
• UserServiceTest#createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet — RED before null-guard fix, GREEN after
• Full backend suite: 1551 tests, 0 failures

## Summary Closes #469. - **V63**: Deduplicates any phantom `(group_id, permission)` rows accumulated since the initial schema (uses `ctid` comparison, safe on fresh DBs as a no-op) - **V64**: Sets `NOT NULL` on `permission` column and adds `pk_group_permissions PRIMARY KEY (group_id, permission)` - **V65**: Renames the de-facto PK on `transcription_block_mentioned_persons` — drops `uq_tbmp_block_person`, adds `pk_tbmp PRIMARY KEY (block_id, person_id)` — naming-convention consistency - **`UserService.createGroup`**: Switched to builder and added null-guard for `dto.permissions` — prevents null propagating into the `@ElementCollection` after V64 adds NOT NULL ## Pre-flight query (run against production before deploying) ```sql SELECT group_id, permission, COUNT(*) FROM group_permissions GROUP BY group_id, permission HAVING COUNT(*) > 1; ``` Expected output: zero rows. V63 handles any duplicates found, but documenting the pre-state is good practice. ## Rollback If V64 needs to be reverted after production deploy: ```sql ALTER TABLE group_permissions DROP CONSTRAINT pk_group_permissions; ALTER TABLE group_permissions ALTER COLUMN permission DROP NOT NULL; ``` ## Test plan - [x] `MigrationIntegrationTest#v64_pk_group_permissions_exists` — RED before V64, GREEN after - [x] `MigrationIntegrationTest#v64_permission_column_isNotNullable` — RED before V64, GREEN after - [x] `MigrationIntegrationTest#v64_rejectsDuplicateGroupPermission` — RED before V64, GREEN after (DataIntegrityViolationException on duplicate insert) - [x] `MigrationIntegrationTest#v65_pk_tbmp_exists` — RED before V65, GREEN after - [x] `UserServiceTest#createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet` — RED before null-guard fix, GREEN after - [x] Full backend suite: 1551 tests, 0 failures

marcel added 2 commits 2026-05-09 15:20:32 +02:00

fix(db): add PRIMARY KEY to group_permissions; promote tbmp UNIQUE to PK ... 3fcdfa85f1

V63 deduplicates any phantom (group_id, permission) rows accumulated since
the initial schema. V64 sets NOT NULL on permission and adds pk_group_permissions.
V65 renames uq_tbmp_block_person to pk_tbmp for naming-convention consistency.
Integration tests confirm each constraint via pg_catalog.pg_constraint. Closes #469 (partial).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(user): use builder in createGroup and guard against null permissions ... eb54a98ea2

Null dto.permissions now produces an empty HashSet instead of propagating null
into the @ElementCollection — prevents a silent NPE after V64 adds NOT NULL.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this pull request 2026-05-09 15:20:41 +02:00

fix(db): add primary key to group_permissions to prevent duplicate grants #469

marcel commented 2026-05-09 15:23:24 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

What I checked

TDD evidence · Naming · Function size · Builder pattern · Guard clauses · Java idioms

Findings

Positive

• createGroup correctly switches from setter-chain to builder — this is the right call and matches the project code style. The null-guard dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>() mirrors the @Builder.Default intent in UserGroup.
• Migration SQL comments explain the why, not the what — exactly what comments should do. V63's ctid note and V64's "future seed can use ON CONFLICT DO NOTHING" note are exactly the right level of documentation.
• v64_rejectsDuplicateGroupPermission uses @Transactional(propagation = NOT_SUPPORTED) correctly — the duplicate PK violation must hit a real commit boundary to fire.

Suggestions (non-blocking)

• v64_rejectsDuplicateGroupPermission does manual cleanup via DELETE statements after the assertion. If assertThatThrownBy itself throws unexpectedly (e.g., the exception is not of the expected type), the cleanup is skipped and the test leaves dirty data in the container. A try-finally block or a dedicated @AfterEach cleanup would be safer:

  try {
      jdbc.update("INSERT INTO group_permissions ...", groupId);
      assertThatThrownBy(() -> ...).isInstanceOf(DataIntegrityViolationException.class);
  } finally {
      jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId);
      jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId);
  }
  

• UserServiceTest uses a fully-qualified org.raddatz.familienarchiv.user.GroupDTO inline — typically a sign of an import collision. Minor style smell; if there's a collision it should show in the import block, not inline.

No blockers. Clean implementation.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### What I checked TDD evidence · Naming · Function size · Builder pattern · Guard clauses · Java idioms ### Findings **Positive** - `createGroup` correctly switches from setter-chain to builder — this is the right call and matches the project code style. The null-guard `dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>()` mirrors the `@Builder.Default` intent in `UserGroup`. - Migration SQL comments explain the *why*, not the *what* — exactly what comments should do. V63's `ctid` note and V64's "future seed can use ON CONFLICT DO NOTHING" note are exactly the right level of documentation. - `v64_rejectsDuplicateGroupPermission` uses `@Transactional(propagation = NOT_SUPPORTED)` correctly — the duplicate PK violation must hit a real commit boundary to fire. **Suggestions (non-blocking)** - `v64_rejectsDuplicateGroupPermission` does manual cleanup via `DELETE` statements after the assertion. If `assertThatThrownBy` itself throws unexpectedly (e.g., the exception is not of the expected type), the cleanup is skipped and the test leaves dirty data in the container. A `try-finally` block or a dedicated `@AfterEach` cleanup would be safer: ```java try { jdbc.update("INSERT INTO group_permissions ...", groupId); assertThatThrownBy(() -> ...).isInstanceOf(DataIntegrityViolationException.class); } finally { jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId); jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId); } ``` - `UserServiceTest` uses a fully-qualified `org.raddatz.familienarchiv.user.GroupDTO` inline — typically a sign of an import collision. Minor style smell; if there's a collision it should show in the import block, not inline. No blockers. Clean implementation.

marcel commented 2026-05-09 15:23:35 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

Constraint placement · Migration sequencing · Module boundaries · Documentation updates

Findings

Strong

This PR does exactly what the architecture principles demand: push integrity to PostgreSQL, not to application code. A PK on (group_id, permission) is atomic and race-condition-free; an application-layer duplicate check is not. The three-step migration sequence (deduplicate → add NOT NULL + PK → promote tbmp UNIQUE to PK) is the correct decomposition — each step is independently safe to run and reviewable.

The V63 dedup migration using ctid is a correct PostgreSQL idiom for removing duplicates while preserving one row when no better discriminant exists. It is a no-op on clean databases, which means the migration is safe regardless of production state.

The UserService.createGroup fix is correctly scoped to the user domain. No boundary leak.

Documentation check

PR contains	Required update
V63/V64/V65: no new tables, columns, or FK relationships — only constraint additions	db-orm.puml and db-relationships.puml — no update needed
No new packages, routes, or Docker services	All other docs — no update needed

Clean.

Operational notes

The PR body includes a pre-flight query and rollback SQL for V64 — this is exactly the right operational discipline. V65 (UNIQUE → PK promotion) involves a DROP CONSTRAINT + ADD CONSTRAINT which in PostgreSQL replaces the backing B-tree. For a family archive table, this is negligible. If transcription_block_mentioned_persons ever reaches significant size, this would need an online approach, but that is not a current concern.

No blockers.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked Constraint placement · Migration sequencing · Module boundaries · Documentation updates ### Findings **Strong** This PR does exactly what the architecture principles demand: **push integrity to PostgreSQL, not to application code**. A PK on `(group_id, permission)` is atomic and race-condition-free; an application-layer duplicate check is not. The three-step migration sequence (deduplicate → add NOT NULL + PK → promote tbmp UNIQUE to PK) is the correct decomposition — each step is independently safe to run and reviewable. The V63 dedup migration using `ctid` is a correct PostgreSQL idiom for removing duplicates while preserving one row when no better discriminant exists. It is a no-op on clean databases, which means the migration is safe regardless of production state. The `UserService.createGroup` fix is correctly scoped to the user domain. No boundary leak. **Documentation check** | PR contains | Required update | |---|---| | V63/V64/V65: no new tables, columns, or FK relationships — only constraint additions | `db-orm.puml` and `db-relationships.puml` — **no update needed** | | No new packages, routes, or Docker services | All other docs — **no update needed** | Clean. **Operational notes** The PR body includes a pre-flight query and rollback SQL for V64 — this is exactly the right operational discipline. V65 (UNIQUE → PK promotion) involves a `DROP CONSTRAINT` + `ADD CONSTRAINT` which in PostgreSQL replaces the backing B-tree. For a family archive table, this is negligible. If `transcription_block_mentioned_persons` ever reaches significant size, this would need an online approach, but that is not a current concern. No blockers.

marcel commented 2026-05-09 15:23:43 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

SQL injection · Input handling · Auth / permission surface · Data exposure · Logging

Findings

No security concerns.

This PR is administrative DDL (primary key and NOT NULL constraints) with a matching service null-guard. The security surface is minimal:

• Migration SQL: Pure DDL (ALTER TABLE, DELETE ... USING). No user-controlled input. ctid is a PostgreSQL system column — not user-controlled, not injectable.
• UserService.createGroup: The null-guard prevents a NullPointerException from propagating into the @ElementCollection. No new input validation surface is introduced; the method was already guarded by @RequirePermission at the controller level.
• No new endpoints, no new error codes, no new logging paths.
• No hardcoded credentials or secrets.
• JPQL usage in the surrounding codebase was not changed; the new JdbcTemplate calls in the test class use parameterized ? placeholders — injection-safe.

The PK constraint on group_permissions actually improves the security posture marginally: it eliminates a class of silent data duplication that could cause inconsistent permission evaluation if the application ever iterated over permissions without deduplicating.

Nothing to flag.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What I checked SQL injection · Input handling · Auth / permission surface · Data exposure · Logging ### Findings **No security concerns.** This PR is administrative DDL (primary key and NOT NULL constraints) with a matching service null-guard. The security surface is minimal: - **Migration SQL**: Pure DDL (`ALTER TABLE`, `DELETE ... USING`). No user-controlled input. `ctid` is a PostgreSQL system column — not user-controlled, not injectable. - **`UserService.createGroup`**: The null-guard prevents a `NullPointerException` from propagating into the `@ElementCollection`. No new input validation surface is introduced; the method was already guarded by `@RequirePermission` at the controller level. - **No new endpoints, no new error codes, no new logging paths.** - **No hardcoded credentials or secrets.** - **JPQL usage** in the surrounding codebase was not changed; the new `JdbcTemplate` calls in the test class use parameterized `?` placeholders — injection-safe. The PK constraint on `group_permissions` actually *improves* the security posture marginally: it eliminates a class of silent data duplication that could cause inconsistent permission evaluation if the application ever iterated over permissions without deduplicating. Nothing to flag.

marcel commented 2026-05-09 15:24:01 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

What I checked

Test pyramid placement · Naming · Coverage gaps · Assertion quality · Test isolation · Flakiness risk

Positive

• Tests are placed correctly: migration schema assertions belong in MigrationIntegrationTest (Testcontainers + real Postgres — not H2). The unit test for createGroup belongs in UserServiceTest with Mockito. Right layer for each concern.
• @Transactional(propagation = NOT_SUPPORTED) on v64_rejectsDuplicateGroupPermission is the correct annotation — without it, the test's own transaction would absorb the duplicate insert and no constraint violation would surface.
• The assertThatThrownBy assertion checks the correct type (DataIntegrityViolationException). Good specificity.
• createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet uses argThat to verify the saved entity's state rather than only checking verify(groupRepository).save(any()). This is meaningful behavior verification.

Concerns

Blocker-level: Test isolation risk in v64_rejectsDuplicateGroupPermission

The cleanup at the end of the test:

jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId);
jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId);

…runs after the assertion. If assertThatThrownBy itself throws an unexpected exception (e.g., a DataIntegrityViolationException that doesn't match, or the first jdbc.update throws), cleanup is skipped. Since this test is NOT_SUPPORTED, there is no transaction to roll back — the data is stranded in the container.

Subsequent tests that run in the same Testcontainers instance may be unaffected (because the group ID is random), but this is a fragile pattern. Wrap in try-finally:

UUID groupId = createUserGroup("DuplicateTestGroup-" + UUID.randomUUID());
try {
    jdbc.update("INSERT INTO group_permissions (group_id, permission) VALUES (?, 'READ_ALL')", groupId);
    assertThatThrownBy(() ->
        jdbc.update("INSERT INTO group_permissions (group_id, permission) VALUES (?, 'READ_ALL')", groupId)
    ).isInstanceOf(DataIntegrityViolationException.class);
} finally {
    jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId);
    jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId);
}

Suggestion: Missing explicit V63 coverage

V63 is the dedup migration. The current tests verify that V64 ran (PK exists, NOT NULL exists, duplicates rejected), but there is no test that V63 actually removed duplicates from a pre-existing dirty state. On a fresh test database there are no duplicates, so V63 is a no-op and is never exercised meaningfully. This is acceptable for a CI-only test environment (duplicates would only exist in production), but it is worth noting that the dedup logic is untested at the migration layer. A comment in the test class documenting this gap would be valuable:

// V63 dedup cannot be regression-tested in CI (fresh DB has no duplicates).
// The pre-flight query in the PR body documents how to verify this manually
// before deploying to production.

Non-blocking: test naming consistency

v64_pk_group_permissions_exists and v65_pk_tbmp_exists use a version-prefixed naming style that is standard for migration tests in this project. Acceptable.

Overall: solid coverage. The try-finally change is the one I'd want before merge.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked Test pyramid placement · Naming · Coverage gaps · Assertion quality · Test isolation · Flakiness risk ### Positive - Tests are placed correctly: migration schema assertions belong in `MigrationIntegrationTest` (Testcontainers + real Postgres — not H2). The unit test for `createGroup` belongs in `UserServiceTest` with Mockito. Right layer for each concern. - `@Transactional(propagation = NOT_SUPPORTED)` on `v64_rejectsDuplicateGroupPermission` is the correct annotation — without it, the test's own transaction would absorb the duplicate insert and no constraint violation would surface. - The `assertThatThrownBy` assertion checks the correct type (`DataIntegrityViolationException`). Good specificity. - `createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet` uses `argThat` to verify the saved entity's state rather than only checking `verify(groupRepository).save(any())`. This is meaningful behavior verification. ### Concerns **Blocker-level: Test isolation risk in `v64_rejectsDuplicateGroupPermission`** The cleanup at the end of the test: ```java jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId); jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId); ``` …runs *after* the assertion. If `assertThatThrownBy` itself throws an unexpected exception (e.g., a `DataIntegrityViolationException` that doesn't match, or the first `jdbc.update` throws), cleanup is skipped. Since this test is `NOT_SUPPORTED`, there is no transaction to roll back — the data is stranded in the container. Subsequent tests that run in the same Testcontainers instance may be unaffected (because the group ID is random), but this is a fragile pattern. Wrap in `try-finally`: ```java UUID groupId = createUserGroup("DuplicateTestGroup-" + UUID.randomUUID()); try { jdbc.update("INSERT INTO group_permissions (group_id, permission) VALUES (?, 'READ_ALL')", groupId); assertThatThrownBy(() -> jdbc.update("INSERT INTO group_permissions (group_id, permission) VALUES (?, 'READ_ALL')", groupId) ).isInstanceOf(DataIntegrityViolationException.class); } finally { jdbc.update("DELETE FROM group_permissions WHERE group_id = ?", groupId); jdbc.update("DELETE FROM user_groups WHERE id = ?", groupId); } ``` **Suggestion: Missing explicit V63 coverage** V63 is the dedup migration. The current tests verify that V64 ran (PK exists, NOT NULL exists, duplicates rejected), but there is no test that V63 actually *removed* duplicates from a pre-existing dirty state. On a fresh test database there are no duplicates, so V63 is a no-op and is never exercised meaningfully. This is acceptable for a CI-only test environment (duplicates would only exist in production), but it is worth noting that the dedup logic is untested at the migration layer. A comment in the test class documenting this gap would be valuable: ```java // V63 dedup cannot be regression-tested in CI (fresh DB has no duplicates). // The pre-flight query in the PR body documents how to verify this manually // before deploying to production. ``` **Non-blocking: test naming consistency** `v64_pk_group_permissions_exists` and `v65_pk_tbmp_exists` use a version-prefixed naming style that is standard for migration tests in this project. Acceptable. Overall: solid coverage. The `try-finally` change is the one I'd want before merge.

marcel commented 2026-05-09 15:24:13 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

Migration deployment safety · Rollback procedure · CI impact · Infrastructure scope

Findings

Strong

The PR description has exactly what I want before a schema-altering migration hits production:

1. Pre-flight query — run this before deploying, confirm zero rows, proceed. Simple, safe.
2. Rollback SQL — if V64 needs reverting after deploy, the commands are documented. This is discipline.

Migration deployment analysis

Migration	Production risk	Notes
V63 dedup	Low	DELETE ... USING ctid — no-op on clean DBs; handles dirty state atomically
V64 NOT NULL + PK	Low-medium	Rewrites the table if nulls exist (they shouldn't after V63); PK creation is fast on small tables
V65 UNIQUE → PK	Low	Drops and re-creates constraint; B-tree rebuild on transcription_block_mentioned_persons; fast on current dataset size

All three run inside Flyway's managed transaction envelope — if any step fails, Flyway marks the migration as failed and the app does not start. This is safe behavior.

No CI changes — this PR has no Gitea Actions workflow modifications, no Compose file changes, no Dockerfile changes.

One note for future migrations

V64's comment documents the ON CONFLICT DO NOTHING pattern for future seed migrations. This is useful institutional knowledge. Consider adding a brief note to docs/infrastructure/ or a CONTRIBUTING.md migration section so the guidance is discoverable outside the migration file itself. Not a blocker — just a suggestion for discoverability.

Ready to ship.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked Migration deployment safety · Rollback procedure · CI impact · Infrastructure scope ### Findings **Strong** The PR description has exactly what I want before a schema-altering migration hits production: 1. **Pre-flight query** — run this before deploying, confirm zero rows, proceed. Simple, safe. 2. **Rollback SQL** — if V64 needs reverting after deploy, the commands are documented. This is discipline. **Migration deployment analysis** | Migration | Production risk | Notes | |---|---|---| | V63 dedup | Low | `DELETE ... USING ctid` — no-op on clean DBs; handles dirty state atomically | | V64 NOT NULL + PK | Low-medium | Rewrites the table if nulls exist (they shouldn't after V63); PK creation is fast on small tables | | V65 UNIQUE → PK | Low | Drops and re-creates constraint; B-tree rebuild on `transcription_block_mentioned_persons`; fast on current dataset size | All three run inside Flyway's managed transaction envelope — if any step fails, Flyway marks the migration as failed and the app does not start. This is safe behavior. **No CI changes** — this PR has no Gitea Actions workflow modifications, no Compose file changes, no Dockerfile changes. **One note for future migrations** V64's comment documents the ON CONFLICT DO NOTHING pattern for future seed migrations. This is useful institutional knowledge. Consider adding a brief note to `docs/infrastructure/` or a `CONTRIBUTING.md` migration section so the guidance is discoverable outside the migration file itself. Not a blocker — just a suggestion for discoverability. Ready to ship.

marcel commented 2026-05-09 15:24:23 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

Requirements traceability · Acceptance criteria coverage · Scope completeness · Issue-to-PR alignment

Findings

Traceability

Issue #469 targets group_permissions data integrity (no PK or UNIQUE constraint since the initial schema). The PR closes #469 and delivers:

Requirement	Delivered
Deduplicate any phantom rows	✅ V63 dedup migration
Add PK constraint to group_permissions	✅ V64 PK + NOT NULL
Prevent future duplicate inserts	✅ PK enforces this at DB layer
Application safe with new NOT NULL	✅ createGroup null-guard

Scope alignment

V65 (tbmp UNIQUE → PK promotion) is not explicitly in the original issue title, but it is a naming-convention cleanup that is logically related (data integrity / constraint naming). It is a low-risk, well-motivated addition that does not expand scope meaningfully.

Acceptance criteria coverage

The PR body's test plan is a de-facto acceptance criteria checklist:

• v64_pk_group_permissions_exists → confirms the PK was created ✅
• v64_permission_column_isNotNullable → confirms NOT NULL was added ✅
• v64_rejectsDuplicateGroupPermission → confirms duplicates are rejected ✅
• v65_pk_tbmp_exists → confirms tbmp naming cleanup ✅
• createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet → confirms app-level safety ✅

One gap to note

The issue presumably existed because earlier migrations lacked proper constraints. It would strengthen the issue backlog if the root-cause note (no PK since V1) were captured as a brief comment on issue #469 so that pattern is documented for future contributors. Not a merge blocker — more a backlog hygiene note.

Requirements fully satisfied.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked Requirements traceability · Acceptance criteria coverage · Scope completeness · Issue-to-PR alignment ### Findings **Traceability** Issue #469 targets `group_permissions` data integrity (no PK or UNIQUE constraint since the initial schema). The PR closes #469 and delivers: | Requirement | Delivered | |---|---| | Deduplicate any phantom rows | ✅ V63 dedup migration | | Add PK constraint to `group_permissions` | ✅ V64 PK + NOT NULL | | Prevent future duplicate inserts | ✅ PK enforces this at DB layer | | Application safe with new NOT NULL | ✅ `createGroup` null-guard | **Scope alignment** V65 (tbmp UNIQUE → PK promotion) is not explicitly in the original issue title, but it is a naming-convention cleanup that is logically related (data integrity / constraint naming). It is a low-risk, well-motivated addition that does not expand scope meaningfully. **Acceptance criteria coverage** The PR body's test plan is a de-facto acceptance criteria checklist: - `v64_pk_group_permissions_exists` → confirms the PK was created ✅ - `v64_permission_column_isNotNullable` → confirms NOT NULL was added ✅ - `v64_rejectsDuplicateGroupPermission` → confirms duplicates are rejected ✅ - `v65_pk_tbmp_exists` → confirms tbmp naming cleanup ✅ - `createGroup_withNullPermissions_savesGroupWithEmptyPermissionSet` → confirms app-level safety ✅ **One gap to note** The issue presumably existed because earlier migrations lacked proper constraints. It would strengthen the issue backlog if the root-cause note (no PK since V1) were captured as a brief comment on issue #469 so that pattern is documented for future contributors. Not a merge blocker — more a backlog hygiene note. Requirements fully satisfied.

marcel commented 2026-05-09 15:24:28 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Verdict: ✅ Approved — No UI changes in scope

What I checked

Frontend changes · Svelte components · Tailwind classes · Accessibility impact · Brand compliance

Findings

This PR is entirely backend: three Flyway SQL migrations and a Java service fix. There are no .svelte files changed, no TypeScript changes, no Tailwind classes, no frontend routes, no API response shape changes.

Nothing in this PR touches the user interface, accessibility, or brand compliance.

One indirect note: V64 adds NOT NULL to group_permissions.permission. If any future UI allows creating groups without selecting permissions, the frontend form should communicate clearly that at least one permission must be chosen (or that an empty permission set is valid — which the createGroup null-guard confirms it is). This is not a concern for this PR, but worth keeping in mind when building the group management UI.

LGTM from my seat.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist **Verdict: ✅ Approved — No UI changes in scope** ### What I checked Frontend changes · Svelte components · Tailwind classes · Accessibility impact · Brand compliance ### Findings This PR is entirely backend: three Flyway SQL migrations and a Java service fix. There are no `.svelte` files changed, no TypeScript changes, no Tailwind classes, no frontend routes, no API response shape changes. Nothing in this PR touches the user interface, accessibility, or brand compliance. **One indirect note**: V64 adds NOT NULL to `group_permissions.permission`. If any future UI allows creating groups without selecting permissions, the frontend form should communicate clearly that at least one permission must be chosen (or that an empty permission set is valid — which the `createGroup` null-guard confirms it is). This is not a concern for this PR, but worth keeping in mind when building the group management UI. LGTM from my seat.

marcel added 1 commit 2026-05-09 15:25:59 +02:00

test(migration): guard cleanup in try-finally to ensure isolation ... bfc3a17676

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-09 15:26:10 +02:00

Author

Owner

Copy Link

Addressed Sara's concern: wrapped the cleanup in v64_rejectsDuplicateGroupPermission in a try-finally block to guarantee data is cleaned up even if the assertion throws unexpectedly. Commit bfc3a176 — all 28 migration integration tests still green.

Addressed Sara's concern: wrapped the cleanup in `v64_rejectsDuplicateGroupPermission` in a `try-finally` block to guarantee data is cleaned up even if the assertion throws unexpectedly. Commit `bfc3a176` — all 28 migration integration tests still green.

marcel merged commit bfc3a17676 into main 2026-05-09 15:44:35 +02:00

marcel deleted branch fix/issue-469-group-permissions-pk 2026-05-09 15:44:38 +02:00

marcel referenced this pull request 2026-05-09 15:56:21 +02:00

fix(a11y): increase PdfControls touch targets to 44×44px (#354) #493

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#492