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

New Issue

marcel · 2026-05-07T17:27:46+02:00

marcel commented

2026-05-07 17:27:46 +02:00

Context

public.group_permissions is a join table mapping UserGroup → permission strings. Pre-prod audit found the table has no primary key and no UNIQUE constraint — only an FK back to user_groups:

$ docker exec archive-db psql -U archive_user -d family_archive_db -c "
  SELECT t.relname, c.conname, c.contype
  FROM pg_constraint c JOIN pg_class t ON c.conrelid=t.oid
  WHERE t.relname='group_permissions';"

   relname        |          conname          | contype
------------------+---------------------------+---------
 group_permissions| fkaqmvqpbaqnfeg5ixk88k8u6i9| f       (FK)
(1 row)

This means the table can hold duplicate (group_id, permission) rows. The duplicates are harmless at the resolved-permissions level (the application uses Set<String> semantics), but:

Storage bloat — repeated grant/ungrant cycles accumulate phantom rows.
Diagnostic noise — admin tooling that lists permissions per group will show duplicates.
Replication / merge skew — without a unique constraint, any MERGE-style sync code will not be idempotent.

transcription_block_mentioned_persons has the same problem on paper but has a real UNIQUE(block_id, person_id), which acts as a de-facto PK; declaring it explicitly is hygiene only.

Approach

Two-step Flyway migration:

V61__deduplicate_group_permissions.sql

-- Remove duplicates first; group_id + permission must be unique
DELETE FROM group_permissions a
USING group_permissions b
WHERE a.ctid < b.ctid
  AND a.group_id = b.group_id
  AND a.permission = b.permission;

V62__group_permissions_primary_key.sql

ALTER TABLE group_permissions
  ADD CONSTRAINT pk_group_permissions PRIMARY KEY (group_id, permission);

Companion: declare the de-facto PK on the other table

-- V63__transcription_block_mentioned_persons_promote_unique_to_pk.sql
ALTER TABLE transcription_block_mentioned_persons
  DROP CONSTRAINT uq_tbmp_block_person;
ALTER TABLE transcription_block_mentioned_persons
  ADD CONSTRAINT pk_tbmp PRIMARY KEY (block_id, person_id);

(Or skip the rename — UNIQUE behaves the same operationally; the only reason to bother is consistency with our naming convention.)

Critical files

backend/src/main/resources/db/migration/V61__deduplicate_group_permissions.sql — new
backend/src/main/resources/db/migration/V62__group_permissions_primary_key.sql — new
backend/src/main/resources/db/migration/V63__tbmp_promote_unique_to_pk.sql — new (optional, hygiene)
Verify Hibernate/JPA mapping for UserGroup.permissions — likely @ElementCollection. After the PK is in place, the existing mapping should still work, but hibernate-tools or ddl-auto: validate would catch a mismatch.

Verification

Pre-migration check on real data:
```
SELECT group_id, permission, COUNT(*)
FROM group_permissions
GROUP BY group_id, permission HAVING COUNT(*) > 1;
```
Document the output for the migration's smoke trace. Audit's local DB had no duplicates, but production may have accumulated some.
Migration test: V61 + V62 + V63 run cleanly against a fresh DB and against a fixture DB pre-seeded with duplicates.
JPA validate: spring.jpa.hibernate.ddl-auto: validate (transient setting for the test) shows the mapping matches.
Smoke: open /admin/groups/<id>, edit permissions; confirm save works and no duplicate rows appear.

Acceptance criteria

V61 deduplicates group_permissions (no (group_id, permission) pair appears more than once).
V62 adds PRIMARY KEY (group_id, permission).
V63 (optional) promotes the existing UNIQUE on transcription_block_mentioned_persons to a PK.
INSERT ... ON CONFLICT DO NOTHING (or its JPA equivalent) becomes safe to use in admin code.
No regression in admin group-edit smoke.

Effort

XS — 1 hour including the dedup query review.

Risk if not addressed

Phantom row accumulation; non-idempotent permission sync; harder admin diagnostics.

Tracked in audit doc as F-32 (Medium) — new dynamic finding from schema constraint inspection.

## Context `public.group_permissions` is a join table mapping `UserGroup → permission` strings. Pre-prod audit found the table has **no primary key** and **no UNIQUE constraint** — only an FK back to `user_groups`: ``` $ docker exec archive-db psql -U archive_user -d family_archive_db -c " SELECT t.relname, c.conname, c.contype FROM pg_constraint c JOIN pg_class t ON c.conrelid=t.oid WHERE t.relname='group_permissions';" relname | conname | contype ------------------+---------------------------+--------- group_permissions| fkaqmvqpbaqnfeg5ixk88k8u6i9| f (FK) (1 row) ``` This means **the table can hold duplicate `(group_id, permission)` rows**. The duplicates are harmless at the resolved-permissions level (the application uses `Set<String>` semantics), but: 1. **Storage bloat** — repeated grant/ungrant cycles accumulate phantom rows. 2. **Diagnostic noise** — admin tooling that lists permissions per group will show duplicates. 3. **Replication / merge skew** — without a unique constraint, any `MERGE`-style sync code will not be idempotent. `transcription_block_mentioned_persons` has the same problem on paper but has a real `UNIQUE(block_id, person_id)`, which acts as a de-facto PK; declaring it explicitly is hygiene only. ## Approach Two-step Flyway migration: ### V61__deduplicate_group_permissions.sql ```sql -- Remove duplicates first; group_id + permission must be unique DELETE FROM group_permissions a USING group_permissions b WHERE a.ctid < b.ctid AND a.group_id = b.group_id AND a.permission = b.permission; ``` ### V62__group_permissions_primary_key.sql ```sql ALTER TABLE group_permissions ADD CONSTRAINT pk_group_permissions PRIMARY KEY (group_id, permission); ``` ### Companion: declare the de-facto PK on the other table ```sql -- V63__transcription_block_mentioned_persons_promote_unique_to_pk.sql ALTER TABLE transcription_block_mentioned_persons DROP CONSTRAINT uq_tbmp_block_person; ALTER TABLE transcription_block_mentioned_persons ADD CONSTRAINT pk_tbmp PRIMARY KEY (block_id, person_id); ``` (Or skip the rename — UNIQUE behaves the same operationally; the only reason to bother is consistency with our naming convention.) ## Critical files - `backend/src/main/resources/db/migration/V61__deduplicate_group_permissions.sql` — **new** - `backend/src/main/resources/db/migration/V62__group_permissions_primary_key.sql` — **new** - `backend/src/main/resources/db/migration/V63__tbmp_promote_unique_to_pk.sql` — **new** (optional, hygiene) - Verify Hibernate/JPA mapping for `UserGroup.permissions` — likely `@ElementCollection`. After the PK is in place, the existing mapping should still work, but `hibernate-tools` or `ddl-auto: validate` would catch a mismatch. ## Verification 1. **Pre-migration check on real data**: ```sql SELECT group_id, permission, COUNT(*) FROM group_permissions GROUP BY group_id, permission HAVING COUNT(*) > 1; ``` Document the output for the migration's smoke trace. Audit's local DB had no duplicates, but production may have accumulated some. 2. **Migration test**: V61 + V62 + V63 run cleanly against a fresh DB and against a fixture DB pre-seeded with duplicates. 3. **JPA validate**: `spring.jpa.hibernate.ddl-auto: validate` (transient setting for the test) shows the mapping matches. 4. **Smoke**: open `/admin/groups/<id>`, edit permissions; confirm save works and no duplicate rows appear. ## Acceptance criteria - [ ] V61 deduplicates `group_permissions` (no `(group_id, permission)` pair appears more than once). - [ ] V62 adds `PRIMARY KEY (group_id, permission)`. - [ ] V63 (optional) promotes the existing UNIQUE on `transcription_block_mentioned_persons` to a PK. - [ ] `INSERT ... ON CONFLICT DO NOTHING` (or its JPA equivalent) becomes safe to use in admin code. - [ ] No regression in admin group-edit smoke. ## Effort XS — 1 hour including the dedup query review. ## Risk if not addressed Phantom row accumulation; non-idempotent permission sync; harder admin diagnostics. Tracked in audit doc as **F-32** (Medium) — new dynamic finding from schema constraint inspection.

marcel added the P2-medium bug labels 2026-05-07 17:29:36 +02:00

marcel commented

2026-05-07 17:47:57 +02:00

🏛️ Markus Keller (@mkeller) — Application Architect

Observations

Root cause confirmed. V1__initial_schema.sql creates group_permissions with only an FK (fkaqmvqpbaqnfeg5ixk88k8u6i9) and no PK or UNIQUE constraint. This is a schema omission from day one, not a regression — the table has always been structurally incomplete.
JPA mapping is consistent with the constraint gap. UserGroup maps @ElementCollection with @CollectionTable and a Set<String> permissions. Hibernate's @ElementCollection uses DELETE + INSERT per update (not an upsert), so duplicate rows can only accumulate from raw SQL operations like migrations — e.g., V11 (add_annotate_all_permission) and V59 (seed_blog_write) both INSERT INTO group_permissions directly. V59 already guards against duplicates with AND NOT EXISTS (...), which is the right instinct but works only because the dedup logic is present; the constraint would make it redundant.
V59 is the strongest evidence the gap is real. The author knew a bare INSERT could produce duplicates and worked around it in SQL. The right fix is to close the constraint so workarounds are never needed.
transcription_block_mentioned_persons situation. V57 added uq_tbmp_block_person UNIQUE (block_id, person_id). A UNIQUE constraint in PostgreSQL is backed by a B-tree index and behaves as a de-facto PK. Promoting it to a named PK (pk_tbmp) is naming hygiene only — operationally identical. The issue is correct that this is optional.
Ordering in the proposed migration is exactly right. Deduplication must precede the PK constraint addition. Combining them in a single migration (V61+V62 as separate files) is safer than one combined file because Flyway will stop at the first failing migration, giving a clean state for re-run.
Doc impact. No new table, no new column, no new module. The DB diagram (db-orm.puml, db-relationships.puml) does not need updating — the table already exists in the diagram; the PK declaration is internal constraint metadata, not a structural change.

Recommendations

Split V61 and V62 exactly as written. Do not merge them. If production has duplicates, V61 runs standalone and succeeds even if V62 would have failed on a pre-duplicate state.
Add a migration integration test in the pattern of RenameUsersToAppUsersMigrationTest that asserts: (a) no (group_id, permission) pair appears more than once, and (b) a PK named pk_group_permissions exists on group_permissions. The existing migration test harness (Testcontainers + @DataJpaTest) makes this straightforward — it runs all migrations on a real PostgreSQL container on every CI run.
Seed a duplicate-fixture test database for V61 specifically. The dedup SQL uses ctid comparison which is PostgreSQL-specific; verify it behaves correctly when there are 0 duplicates (no-op), 1 duplicate, and N>2 duplicates for the same (group_id, permission).
V63 is worth doing for consistency — the naming convention pk_* vs uq_* signals different semantics to anyone reading the schema. One migration, three lines. Do it.
Consider NOT NULL on permission column. The initial schema has permission character varying(255) with no NOT NULL constraint. A NULL permission would be silently skipped by the Set<String> in Java but would pass the PK if group_id is unique — a NULL permission in a PK is PostgreSQL-valid but semantically wrong. Add ALTER TABLE group_permissions ALTER COLUMN permission SET NOT NULL; as part of V62 or a companion migration.

## 🏛️ Markus Keller (@mkeller) — Application Architect ### Observations - **Root cause confirmed.** `V1__initial_schema.sql` creates `group_permissions` with only an FK (`fkaqmvqpbaqnfeg5ixk88k8u6i9`) and no PK or UNIQUE constraint. This is a schema omission from day one, not a regression — the table has always been structurally incomplete. - **JPA mapping is consistent with the constraint gap.** `UserGroup` maps `@ElementCollection` with `@CollectionTable` and a `Set<String> permissions`. Hibernate's `@ElementCollection` uses `DELETE + INSERT` per update (not an upsert), so duplicate rows can only accumulate from raw SQL operations like migrations — e.g., V11 (`add_annotate_all_permission`) and V59 (`seed_blog_write`) both `INSERT INTO group_permissions` directly. V59 already guards against duplicates with `AND NOT EXISTS (...)`, which is the right instinct but works only because the dedup logic is present; the constraint would make it redundant. - **V59 is the strongest evidence the gap is real.** The author knew a bare INSERT could produce duplicates and worked around it in SQL. The right fix is to close the constraint so workarounds are never needed. - **`transcription_block_mentioned_persons` situation.** V57 added `uq_tbmp_block_person UNIQUE (block_id, person_id)`. A UNIQUE constraint in PostgreSQL is backed by a B-tree index and behaves as a de-facto PK. Promoting it to a named PK (`pk_tbmp`) is naming hygiene only — operationally identical. The issue is correct that this is optional. - **Ordering in the proposed migration is exactly right.** Deduplication must precede the PK constraint addition. Combining them in a single migration (V61+V62 as separate files) is safer than one combined file because Flyway will stop at the first failing migration, giving a clean state for re-run. - **Doc impact.** No new table, no new column, no new module. The DB diagram (`db-orm.puml`, `db-relationships.puml`) does not need updating — the table already exists in the diagram; the PK declaration is internal constraint metadata, not a structural change. ### Recommendations - **Split V61 and V62 exactly as written.** Do not merge them. If production has duplicates, V61 runs standalone and succeeds even if V62 would have failed on a pre-duplicate state. - **Add a migration integration test** in the pattern of `RenameUsersToAppUsersMigrationTest` that asserts: (a) no `(group_id, permission)` pair appears more than once, and (b) a PK named `pk_group_permissions` exists on `group_permissions`. The existing migration test harness (Testcontainers + `@DataJpaTest`) makes this straightforward — it runs all migrations on a real PostgreSQL container on every CI run. - **Seed a duplicate-fixture test database** for V61 specifically. The dedup SQL uses `ctid` comparison which is PostgreSQL-specific; verify it behaves correctly when there are 0 duplicates (no-op), 1 duplicate, and N>2 duplicates for the same `(group_id, permission)`. - **V63 is worth doing for consistency** — the naming convention `pk_*` vs `uq_*` signals different semantics to anyone reading the schema. One migration, three lines. Do it. - **Consider `NOT NULL` on `permission` column.** The initial schema has `permission character varying(255)` with no NOT NULL constraint. A NULL permission would be silently skipped by the `Set<String>` in Java but would pass the PK if `group_id` is unique — a NULL permission in a PK is PostgreSQL-valid but semantically wrong. Add `ALTER TABLE group_permissions ALTER COLUMN permission SET NOT NULL;` as part of V62 or a companion migration.

marcel commented

2026-05-07 17:48:16 +02:00

👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer

Observations

The JPA mapping is sound but has a subtle implication. @ElementCollection with Set<String> means Hibernate always issues a DELETE FROM group_permissions WHERE group_id = ? followed by INSERT statements when setPermissions() is called. This is what updateGroup() in UserService does. Once V62 adds the PK, these DELETEs + INSERTs remain correct because the PK only blocks duplicate rows — it does not change the DELETE+INSERT lifecycle. No JPA code change is needed.
createGroup() has a subtle gap. It calls new UserGroup() + group.setName(...) + group.setPermissions(...) instead of using the @Builder. This bypasses @Builder.Default, meaning permissions could theoretically be null if dto.getPermissions() is null. In practice GroupDTO.permissions is a Set<String> that can be null (no @NotNull), and setPermissions(null) on the entity would set the collection to null. After the PK exists, a null permission field would cause a constraint violation on INSERT — which is actually the right behavior but the error message would be confusing. Better to guard it now: group.setPermissions(dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>()).
updateGroup() has the same null risk. if (dto.getPermissions() != null) group.setPermissions(dto.getPermissions()) — this is correct as a partial-update guard, but dto.getPermissions() could also be an empty set (meaning "remove all permissions"), which is a valid intent and currently works. Good.
No test covers GroupController or UserService.createGroup/updateGroup. UserServiceTest tests user CRUD but has zero coverage of group management methods. GroupController has no test file at all. The migration work is an ideal moment to add these.
V59's INSERT ... WHERE NOT EXISTS is correct but should become unnecessary. After V62, V59's guard is redundant for future migrations. Worth noting in the migration comment that the constraint replaces this pattern.

Recommendations

Write the failing integration test before writing V61/V62/V63. Pattern: @DataJpaTest + Testcontainers, seed a group with duplicate (group_id, permission) rows via JdbcTemplate.update(...), run V61 (or assert post-migration state), then assert zero duplicates remain. Then add V62 and assert the PK constraint exists via information_schema.table_constraints.
Add a UserService unit test for createGroup with null permissions — this is a missing test, not just a migration concern. One test: given dto.permissions is null, createGroup returns a group with empty permissions. Write it red, then fix the null-guard in createGroup(), then go green.
Add @NotNull or at least an empty-set guard to GroupDTO.permissions to prevent null propagating into the DB layer. A @jakarta.validation.constraints.NotNull annotation on GroupDTO.permissions would let Spring MVC reject a null payload before it reaches the service.

Use the builder in createGroup() instead of new UserGroup() + setters, for consistency with the rest of the codebase:

UserGroup group = UserGroup.builder()
    .name(dto.getName())
    .permissions(dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>())
    .build();

## 👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer ### Observations - **The JPA mapping is sound but has a subtle implication.** `@ElementCollection` with `Set<String>` means Hibernate always issues a `DELETE FROM group_permissions WHERE group_id = ?` followed by `INSERT` statements when `setPermissions()` is called. This is what `updateGroup()` in `UserService` does. Once V62 adds the PK, these DELETEs + INSERTs remain correct because the PK only blocks duplicate rows — it does not change the DELETE+INSERT lifecycle. No JPA code change is needed. - **`createGroup()` has a subtle gap.** It calls `new UserGroup()` + `group.setName(...)` + `group.setPermissions(...)` instead of using the `@Builder`. This bypasses `@Builder.Default`, meaning `permissions` could theoretically be `null` if `dto.getPermissions()` is null. In practice `GroupDTO.permissions` is a `Set<String>` that can be null (no `@NotNull`), and `setPermissions(null)` on the entity would set the collection to null. After the PK exists, a null permission field would cause a constraint violation on INSERT — which is actually the right behavior but the error message would be confusing. Better to guard it now: `group.setPermissions(dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>())`. - **`updateGroup()` has the same null risk.** `if (dto.getPermissions() != null) group.setPermissions(dto.getPermissions())` — this is correct as a partial-update guard, but `dto.getPermissions()` could also be an empty set (meaning "remove all permissions"), which is a valid intent and currently works. Good. - **No test covers `GroupController` or `UserService.createGroup/updateGroup`.** `UserServiceTest` tests user CRUD but has zero coverage of group management methods. `GroupController` has no test file at all. The migration work is an ideal moment to add these. - **V59's `INSERT ... WHERE NOT EXISTS` is correct but should become unnecessary.** After V62, V59's guard is redundant for future migrations. Worth noting in the migration comment that the constraint replaces this pattern. ### Recommendations - **Write the failing integration test before writing V61/V62/V63.** Pattern: `@DataJpaTest` + Testcontainers, seed a group with duplicate `(group_id, permission)` rows via `JdbcTemplate.update(...)`, run V61 (or assert post-migration state), then assert zero duplicates remain. Then add V62 and assert the PK constraint exists via `information_schema.table_constraints`. - **Add a `UserService` unit test for `createGroup` with null permissions** — this is a missing test, not just a migration concern. One test: `given dto.permissions is null, createGroup returns a group with empty permissions`. Write it red, then fix the null-guard in `createGroup()`, then go green. - **Add `@NotNull` or at least an empty-set guard to `GroupDTO.permissions`** to prevent null propagating into the DB layer. A `@jakarta.validation.constraints.NotNull` annotation on `GroupDTO.permissions` would let Spring MVC reject a null payload before it reaches the service. - **Use the builder in `createGroup()`** instead of `new UserGroup()` + setters, for consistency with the rest of the codebase: ```java UserGroup group = UserGroup.builder() .name(dto.getName()) .permissions(dto.getPermissions() != null ? dto.getPermissions() : new HashSet<>()) .build(); ```

marcel commented

2026-05-07 17:48:39 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Observations

The missing PK is a data integrity issue, not a direct security vulnerability — but the knock-on effects are security-adjacent. Specifically: duplicate permission grants create ambiguity in admin tooling. If the /admin/groups/<id> UI reads group_permissions via JPA's Set<String>, it de-duplicates automatically (Java Set semantics). But any raw SQL query against the table — audit queries, reporting, backup validation scripts — will see phantoms. An operator might conclude a group has two WRITE_ALL grants and mistakenly delete one, not realizing a second remains.
Permission revocation relies on DELETE semantics. When updateGroup() sets new permissions, Hibernate issues DELETE FROM group_permissions WHERE group_id = ? then re-inserts the desired set. This is safe and idempotent — the PK constraint does not change this path. Good.
V59 demonstrates the existing pattern is aware of the risk. The NOT EXISTS guard in V59 is a manual uniqueness check in SQL. After V62, future data migrations can safely use INSERT ... ON CONFLICT DO NOTHING — which is atomic and race-condition-free. This is an improvement to the security posture of future migrations.
permission character varying(255) with no NOT NULL constraint. A NULL value in the permission column would behave inconsistently: JPA's Set<String> would include it as a null element (or silently skip it, depending on Hibernate's element collection handling), and the PostgreSQL PK would accept it (NULL != NULL in a unique constraint, but in a PK context NULLs are rejected). The net result is a NULL permission that could never be deleted via normal JPA flow. This is a latent integrity bug.
No test asserts the security boundary: unauthorized users cannot call PATCH /api/groups/{id} or POST /api/groups. GroupController carries @RequirePermission(Permission.ADMIN_PERMISSION) at the class level, which is correct. But there is no @WebMvcTest that asserts a 403 is returned when a user with only READ_ALL attempts to modify a group. This is a missing security regression test.

Recommendations

Add NOT NULL to permission in the migration. Include ALTER TABLE group_permissions ALTER COLUMN permission SET NOT NULL; in V62 alongside the PK. A NULL permission has no valid meaning — eliminate it at the schema level.
Write a @WebMvcTest for GroupController that explicitly tests 403 for non-ADMIN_PERMISSION users on all write endpoints (POST /api/groups, PATCH /api/groups/{id}, DELETE /api/groups/{id}). The class-level annotation makes this easy — mock the service, call with a user having only READ_ALL, assert 403. This is a permanent security regression test.

Document the ON CONFLICT DO NOTHING pattern in a comment in V62 itself, so future migration authors know the safe idempotent insert pattern:

-- After this constraint: use INSERT INTO group_permissions ... ON CONFLICT DO NOTHING
-- instead of INSERT ... WHERE NOT EXISTS (...) — it is atomic and race-condition-free.

Verify ddl-auto: validate passes after V61+V62+V63. Spring's JPA validation mode will check that the database schema matches the Hibernate entity mappings. Running spring.jpa.hibernate.ddl-auto=validate in a test profile is a cheap way to catch mapping drift introduced by the PK change.

Open Decisions

Should permission column be constrained to known Permission enum values via a CHECK constraint? A CHECK (permission IN ('READ_ALL', 'WRITE_ALL', 'ADMIN', ...)) would prevent garbage strings from entering the table. Benefit: catches bugs in migrations or admin tooling. Cost: every new Permission enum value requires a companion migration to widen the CHECK. Given that adding permissions is rare and high-stakes, this is a meaningful tradeoff worth an explicit decision.

## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations - **The missing PK is a data integrity issue, not a direct security vulnerability** — but the knock-on effects are security-adjacent. Specifically: duplicate permission grants create ambiguity in admin tooling. If the `/admin/groups/<id>` UI reads `group_permissions` via JPA's `Set<String>`, it de-duplicates automatically (Java Set semantics). But any raw SQL query against the table — audit queries, reporting, backup validation scripts — will see phantoms. An operator might conclude a group has two `WRITE_ALL` grants and mistakenly delete one, not realizing a second remains. - **Permission revocation relies on DELETE semantics.** When `updateGroup()` sets new permissions, Hibernate issues `DELETE FROM group_permissions WHERE group_id = ?` then re-inserts the desired set. This is safe and idempotent — the PK constraint does not change this path. Good. - **V59 demonstrates the existing pattern is aware of the risk.** The `NOT EXISTS` guard in V59 is a manual uniqueness check in SQL. After V62, future data migrations can safely use `INSERT ... ON CONFLICT DO NOTHING` — which is atomic and race-condition-free. This is an improvement to the security posture of future migrations. - **`permission character varying(255)` with no NOT NULL constraint.** A `NULL` value in the `permission` column would behave inconsistently: JPA's `Set<String>` would include it as a null element (or silently skip it, depending on Hibernate's element collection handling), and the PostgreSQL PK would accept it (NULL != NULL in a unique constraint, but in a PK context NULLs are rejected). The net result is a NULL permission that could never be deleted via normal JPA flow. This is a latent integrity bug. - **No test asserts the security boundary: unauthorized users cannot call `PATCH /api/groups/{id}` or `POST /api/groups`.** `GroupController` carries `@RequirePermission(Permission.ADMIN_PERMISSION)` at the class level, which is correct. But there is no `@WebMvcTest` that asserts a `403` is returned when a user with only `READ_ALL` attempts to modify a group. This is a missing security regression test. ### Recommendations - **Add `NOT NULL` to `permission` in the migration.** Include `ALTER TABLE group_permissions ALTER COLUMN permission SET NOT NULL;` in V62 alongside the PK. A NULL permission has no valid meaning — eliminate it at the schema level. - **Write a `@WebMvcTest` for `GroupController` that explicitly tests `403` for non-`ADMIN_PERMISSION` users on all write endpoints** (`POST /api/groups`, `PATCH /api/groups/{id}`, `DELETE /api/groups/{id}`). The class-level annotation makes this easy — mock the service, call with a user having only `READ_ALL`, assert `403`. This is a permanent security regression test. - **Document the `ON CONFLICT DO NOTHING` pattern** in a comment in V62 itself, so future migration authors know the safe idempotent insert pattern: ```sql -- After this constraint: use INSERT INTO group_permissions ... ON CONFLICT DO NOTHING -- instead of INSERT ... WHERE NOT EXISTS (...) — it is atomic and race-condition-free. ``` - **Verify `ddl-auto: validate` passes after V61+V62+V63.** Spring's JPA validation mode will check that the database schema matches the Hibernate entity mappings. Running `spring.jpa.hibernate.ddl-auto=validate` in a test profile is a cheap way to catch mapping drift introduced by the PK change. ### Open Decisions - **Should `permission` column be constrained to known `Permission` enum values via a CHECK constraint?** A `CHECK (permission IN ('READ_ALL', 'WRITE_ALL', 'ADMIN', ...))` would prevent garbage strings from entering the table. Benefit: catches bugs in migrations or admin tooling. Cost: every new `Permission` enum value requires a companion migration to widen the CHECK. Given that adding permissions is rare and high-stakes, this is a meaningful tradeoff worth an explicit decision.

marcel commented

2026-05-07 17:49:02 +02:00

🧪 Sara Holt (@saraholt) — QA Engineer

Observations

Zero test coverage for the affected migration path. There is a well-structured migration test pattern already in the codebase (RenameUsersToAppUsersMigrationTest) using @DataJpaTest + Testcontainers + JdbcTemplate. V61/V62/V63 have no corresponding test class. This means CI cannot currently verify that the dedup query works, that the PK was added, or that the NOT NULL constraint (if added) is in place.
The dedup SQL uses ctid, which is PostgreSQL-specific and correct — but its correctness in edge cases (zero duplicates, one pair of duplicates, three copies of the same row) needs a test fixture to confirm. The issue itself notes "Audit's local DB had no duplicates, but production may have accumulated some." This is exactly the kind of scenario that integration tests with pre-seeded fixtures catch.
V59 (seed_blog_write.sql) was shipped without a migration test confirming that the seed ran correctly and did not create duplicates. The V59 NOT EXISTS guard is a code-review-visible safety mechanism, but there is no test asserting it. This is pre-existing test debt that V62 implicitly fixes — worth noting in the test.
The acceptance criteria are well-structured and testable as written. Each AC maps to a concrete SQL assertion or a Flyway run assertion. This is good issue hygiene.
UserService.createGroup and updateGroup have no unit tests. UserServiceTest covers user lifecycle but skips group management entirely. A permissions regression (e.g., a future refactor that accidentally passes a mutable list instead of a defensive copy to setPermissions) would go undetected.

Recommendations

Create GroupPermissionsMigrationTest alongside V61/V62. Structure:

@DataJpaTest
@AutoConfigureTestDatabase(replace = NONE)
@Import({PostgresContainerConfig.class, FlywayConfig.class})
class GroupPermissionsMigrationTest {
    @Autowired JdbcTemplate jdbc;

    @Test void v62_primaryKeyExists_onGroupPermissions() { ... }
    @Test void v62_noDuplicateGroupPermissionsRemain() { ... }
    @Test void v62_permissionColumnIsNotNullable() { ... }  // if NOT NULL is added
    @Test void v63_primaryKeyExists_onTbmp() { ... }
}

This follows the exact pattern of RenameUsersToAppUsersMigrationTest — same annotations, same JdbcTemplate helpers.

Seed duplicates before running the migration to test V61 in a meaningful state. Use a @Sql script or JdbcTemplate in a @BeforeEach to insert a group with two identical (group_id, 'READ_ALL') rows before asserting that V61 reduced them to one.
Add UserService unit tests for createGroup and updateGroup — at minimum: creating a group with a non-null permission set, creating a group with a null permission set, updating permissions replaces rather than appends, and updating with null permissions is a no-op.
The post-migration smoke AC ("open /admin/groups/<id>, edit permissions, confirm save works") should become a Playwright E2E test step rather than manual verification. A single E2E test that saves a group with a new permission and then re-reads the group to confirm no duplicates in the API response would make this AC permanently automated.

Open Decisions

Should the migration test seed duplicates by bypassing Flyway (raw SQL before migration) or by mocking a pre-V62 DB state? The cleanest approach is to run migrations up to V60, insert duplicates via JdbcTemplate, then apply V61+V62 and assert. Flyway's target property supports this. Worth deciding before implementing the test.

## 🧪 Sara Holt (@saraholt) — QA Engineer ### Observations - **Zero test coverage for the affected migration path.** There is a well-structured migration test pattern already in the codebase (`RenameUsersToAppUsersMigrationTest`) using `@DataJpaTest` + Testcontainers + `JdbcTemplate`. V61/V62/V63 have no corresponding test class. This means CI cannot currently verify that the dedup query works, that the PK was added, or that the `NOT NULL` constraint (if added) is in place. - **The dedup SQL uses `ctid`**, which is PostgreSQL-specific and correct — but its correctness in edge cases (zero duplicates, one pair of duplicates, three copies of the same row) needs a test fixture to confirm. The issue itself notes "Audit's local DB had no duplicates, but production may have accumulated some." This is exactly the kind of scenario that integration tests with pre-seeded fixtures catch. - **V59 (`seed_blog_write.sql`) was shipped without a migration test** confirming that the seed ran correctly and did not create duplicates. The V59 `NOT EXISTS` guard is a code-review-visible safety mechanism, but there is no test asserting it. This is pre-existing test debt that V62 implicitly fixes — worth noting in the test. - **The acceptance criteria are well-structured and testable as written.** Each AC maps to a concrete SQL assertion or a Flyway run assertion. This is good issue hygiene. - **`UserService.createGroup` and `updateGroup` have no unit tests.** `UserServiceTest` covers user lifecycle but skips group management entirely. A permissions regression (e.g., a future refactor that accidentally passes a mutable list instead of a defensive copy to `setPermissions`) would go undetected. ### Recommendations - **Create `GroupPermissionsMigrationTest` alongside V61/V62.** Structure: ```java @DataJpaTest @AutoConfigureTestDatabase(replace = NONE) @Import({PostgresContainerConfig.class, FlywayConfig.class}) class GroupPermissionsMigrationTest { @Autowired JdbcTemplate jdbc; @Test void v62_primaryKeyExists_onGroupPermissions() { ... } @Test void v62_noDuplicateGroupPermissionsRemain() { ... } @Test void v62_permissionColumnIsNotNullable() { ... } // if NOT NULL is added @Test void v63_primaryKeyExists_onTbmp() { ... } } ``` This follows the exact pattern of `RenameUsersToAppUsersMigrationTest` — same annotations, same `JdbcTemplate` helpers. - **Seed duplicates before running the migration** to test V61 in a meaningful state. Use a `@Sql` script or `JdbcTemplate` in a `@BeforeEach` to insert a group with two identical `(group_id, 'READ_ALL')` rows before asserting that V61 reduced them to one. - **Add `UserService` unit tests for `createGroup` and `updateGroup`** — at minimum: creating a group with a non-null permission set, creating a group with a null permission set, updating permissions replaces rather than appends, and updating with null permissions is a no-op. - **The post-migration smoke AC** ("open `/admin/groups/<id>`, edit permissions, confirm save works") should become a Playwright E2E test step rather than manual verification. A single E2E test that saves a group with a new permission and then re-reads the group to confirm no duplicates in the API response would make this AC permanently automated. ### Open Decisions - **Should the migration test seed duplicates by bypassing Flyway (raw SQL before migration) or by mocking a pre-V62 DB state?** The cleanest approach is to run migrations up to V60, insert duplicates via `JdbcTemplate`, then apply V61+V62 and assert. Flyway's `target` property supports this. Worth deciding before implementing the test.

marcel commented

2026-05-07 17:49:30 +02:00

⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Observations

This is a pure schema migration — no Docker, no CI pipeline, no infrastructure change. My scope here is narrow: does the migration run cleanly in CI (Testcontainers), and does the production database require any pre-flight steps before deploying?
Testcontainers CI coverage is already in place. The project uses @DataJpaTest + PostgresContainerConfig with postgres:16-alpine. Every Flyway migration runs in CI on each PR. If V61 or V62 are broken SQL, CI will catch it before it reaches the production database. This is the correct safety net.
Production pre-flight check is important. The issue correctly identifies that the audit's local DB had no duplicates but production may have accumulated some via repeated grant/ungrant cycles. The recommended pre-flight query should be run manually against production before deploying:
```
SELECT group_id, permission, COUNT(*)
FROM group_permissions
GROUP BY group_id, permission HAVING COUNT(*) > 1;
```
This is a non-destructive read — safe to run any time. Document the expected output ("zero rows") in the migration PR description.
The Flyway migration order is safe. V61 deduplicates, V62 adds the PK. If V62 fails for any reason (e.g., production had duplicates that V61 missed due to a ctid edge case), Flyway marks V62 as failed and stops. No partial state. The application continues to start with V61 applied but V62 pending — this is safe since V61 alone does not break anything.
Rollback strategy. Flyway Community (what this project uses) does not support automatic rollback. If V62 needs to be undone after a production deploy, the manual rollback is:
```
ALTER TABLE group_permissions DROP CONSTRAINT pk_group_permissions;
```
This is fast and safe. Worth documenting in the PR for on-call awareness.

Recommendations

Add the pre-flight query to the migration PR description as a required manual step before production deployment. Output should be documented as part of the deploy checklist.
Verify the migration runs against the production DB image version. The Testcontainers config uses postgres:16-alpine. If production is running a different minor version (e.g., 16.3 vs 16.1), ctid-based deduplication is unaffected — ctid behavior has not changed across PostgreSQL 16 minor releases. Still worth confirming the exact production version (SELECT version();) before the deploy.
No docker-compose.yml change needed. This is a schema-only fix. No new service, no new volume, no env var. The migration runs as part of the normal Spring Boot startup sequence.
The backup window. Since production likely runs nightly backups, schedule this deploy immediately after a backup completes so the pre-migration state is captured. The migration itself is milliseconds on a table this size (permissions per group is small), so no downtime window is needed.

Open Decisions

Should the pre-flight query be automated as a pre-deploy CI check (e.g., a Gitea Actions step that runs the query against a production-snapshot DB)? For a project at this scale, manual is sufficient. But if the team ever adds automated production-smoke-test infrastructure, this is a good template query to include.

## ⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer ### Observations - **This is a pure schema migration — no Docker, no CI pipeline, no infrastructure change.** My scope here is narrow: does the migration run cleanly in CI (Testcontainers), and does the production database require any pre-flight steps before deploying? - **Testcontainers CI coverage is already in place.** The project uses `@DataJpaTest` + `PostgresContainerConfig` with `postgres:16-alpine`. Every Flyway migration runs in CI on each PR. If V61 or V62 are broken SQL, CI will catch it before it reaches the production database. This is the correct safety net. - **Production pre-flight check is important.** The issue correctly identifies that the audit's local DB had no duplicates but production may have accumulated some via repeated grant/ungrant cycles. The recommended pre-flight query should be run manually against production before deploying: ```sql SELECT group_id, permission, COUNT(*) FROM group_permissions GROUP BY group_id, permission HAVING COUNT(*) > 1; ``` This is a non-destructive read — safe to run any time. Document the expected output ("zero rows") in the migration PR description. - **The Flyway migration order is safe.** V61 deduplicates, V62 adds the PK. If V62 fails for any reason (e.g., production had duplicates that V61 missed due to a `ctid` edge case), Flyway marks V62 as failed and stops. No partial state. The application continues to start with V61 applied but V62 pending — this is safe since V61 alone does not break anything. - **Rollback strategy.** Flyway Community (what this project uses) does not support automatic rollback. If V62 needs to be undone after a production deploy, the manual rollback is: ```sql ALTER TABLE group_permissions DROP CONSTRAINT pk_group_permissions; ``` This is fast and safe. Worth documenting in the PR for on-call awareness. ### Recommendations - **Add the pre-flight query to the migration PR description** as a required manual step before production deployment. Output should be documented as part of the deploy checklist. - **Verify the migration runs against the production DB image version.** The Testcontainers config uses `postgres:16-alpine`. If production is running a different minor version (e.g., 16.3 vs 16.1), `ctid`-based deduplication is unaffected — `ctid` behavior has not changed across PostgreSQL 16 minor releases. Still worth confirming the exact production version (`SELECT version();`) before the deploy. - **No `docker-compose.yml` change needed.** This is a schema-only fix. No new service, no new volume, no env var. The migration runs as part of the normal Spring Boot startup sequence. - **The backup window.** Since production likely runs nightly backups, schedule this deploy immediately after a backup completes so the pre-migration state is captured. The migration itself is milliseconds on a table this size (permissions per group is small), so no downtime window is needed. ### Open Decisions - **Should the pre-flight query be automated as a pre-deploy CI check** (e.g., a Gitea Actions step that runs the query against a production-snapshot DB)? For a project at this scale, manual is sufficient. But if the team ever adds automated production-smoke-test infrastructure, this is a good template query to include.

marcel commented

2026-05-07 17:49:52 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is exceptionally well-specified for a technical fix. It includes context, evidence (the psql constraint query output), impact analysis (three distinct problem classes), a two-step migration approach, companion SQL, and five acceptance criteria that are directly testable. This is definition-of-ready for implementation.
The acceptance criteria map cleanly to verifiable states, but one is slightly ambiguous: "INSERT ... ON CONFLICT DO NOTHING (or its JPA equivalent) becomes safe to use in admin code." JPA's @ElementCollection does not use ON CONFLICT syntax — it uses DELETE + INSERT. This AC describes a future-safety guarantee for raw SQL migrations, not a change to application code. The wording should clarify the intended scope to avoid a developer checking whether JPA was changed to use ON CONFLICT.
The "optional" framing of V63 creates a risk of indefinite deferral. V63 promotes the UNIQUE constraint on transcription_block_mentioned_persons to a PK. The issue describes it as "hygiene only" and marks it optional. Given that V57 already added the UNIQUE constraint with a rationale comment ("raw-SQL import or concurrent bypass of JPA could"), the case for V63 is consistent with the project's constraint discipline. Calling it optional in the issue means it may never get done.
The "Effort: XS — 1 hour" estimate is correct for the migration files, but underestimates the full scope if the team follows TDD (as project norms require): writing a failing migration test, seeding duplicate fixtures, and adding the companion GroupController security tests adds 1–2 hours.
Non-functional requirements are implicit but not stated. The migration must be backward-compatible (no application code change required) and zero-downtime (add constraint on a small table, no table lock for meaningful duration on PostgreSQL 16). These are satisfied by the proposed approach but worth stating explicitly as constraints.

Recommendations

Rewrite AC-4 for precision: "After V62 is applied, future raw SQL migrations targeting group_permissions may use INSERT INTO group_permissions (...) VALUES (...) ON CONFLICT DO NOTHING as an atomic idempotent pattern. Existing JPA @ElementCollection behavior (DELETE + INSERT) is unchanged."
Promote V63 from optional to a separate should-do item with its own acceptance criterion. Label it P3-low on this issue, or create a follow-up issue immediately so it does not get lost.
Add one explicit non-functional requirement to the issue: "The migration must complete without application downtime. PostgreSQL 16 ALTER TABLE ADD CONSTRAINT PRIMARY KEY on a table with fewer than 100,000 rows acquires a brief ACCESS EXCLUSIVE lock but completes in milliseconds. No maintenance window required."
The verification section is strong. The three-step verification plan (pre-migration SQL check → migration test → JPA validate → smoke) is implementation-ready. Consider adding it explicitly to the acceptance criteria so it is not skipped under time pressure.

## 📋 Elicit — Requirements Engineer ### Observations - **The issue is exceptionally well-specified for a technical fix.** It includes context, evidence (the `psql` constraint query output), impact analysis (three distinct problem classes), a two-step migration approach, companion SQL, and five acceptance criteria that are directly testable. This is definition-of-ready for implementation. - **The acceptance criteria map cleanly to verifiable states**, but one is slightly ambiguous: "INSERT ... ON CONFLICT DO NOTHING (or its JPA equivalent) becomes safe to use in admin code." JPA's `@ElementCollection` does not use `ON CONFLICT` syntax — it uses `DELETE + INSERT`. This AC describes a future-safety guarantee for raw SQL migrations, not a change to application code. The wording should clarify the intended scope to avoid a developer checking whether JPA was changed to use ON CONFLICT. - **The "optional" framing of V63 creates a risk of indefinite deferral.** V63 promotes the UNIQUE constraint on `transcription_block_mentioned_persons` to a PK. The issue describes it as "hygiene only" and marks it optional. Given that V57 already added the UNIQUE constraint with a rationale comment ("raw-SQL import or concurrent bypass of JPA could"), the case for V63 is consistent with the project's constraint discipline. Calling it optional in the issue means it may never get done. - **The "Effort: XS — 1 hour" estimate is correct for the migration files**, but underestimates the full scope if the team follows TDD (as project norms require): writing a failing migration test, seeding duplicate fixtures, and adding the companion `GroupController` security tests adds 1–2 hours. - **Non-functional requirements are implicit but not stated.** The migration must be backward-compatible (no application code change required) and zero-downtime (add constraint on a small table, no table lock for meaningful duration on PostgreSQL 16). These are satisfied by the proposed approach but worth stating explicitly as constraints. ### Recommendations - **Rewrite AC-4 for precision:** "After V62 is applied, future raw SQL migrations targeting `group_permissions` may use `INSERT INTO group_permissions (...) VALUES (...) ON CONFLICT DO NOTHING` as an atomic idempotent pattern. Existing JPA `@ElementCollection` behavior (DELETE + INSERT) is unchanged." - **Promote V63 from optional to a separate should-do item** with its own acceptance criterion. Label it P3-low on this issue, or create a follow-up issue immediately so it does not get lost. - **Add one explicit non-functional requirement to the issue:** "The migration must complete without application downtime. PostgreSQL 16 `ALTER TABLE ADD CONSTRAINT PRIMARY KEY` on a table with fewer than 100,000 rows acquires a brief `ACCESS EXCLUSIVE` lock but completes in milliseconds. No maintenance window required." - **The verification section is strong.** The three-step verification plan (pre-migration SQL check → migration test → JPA validate → smoke) is implementation-ready. Consider adding it explicitly to the acceptance criteria so it is not skipped under time pressure.

marcel commented

2026-05-07 17:50:12 +02:00

🎨 Leonie Voss (@leonievoss) — UI/UX Designer

Observations

This is a database schema fix with no direct UI surface. The admin group editor at /admin/groups/<id> is the only user-facing area affected, and the change is purely a correctness fix — the UI already behaves as if duplicates don't exist (JPA Set<String> semantics deduplicate transparently).
There is an indirect UX concern in the admin tooling. If any admin-facing screen lists permissions per group using a raw SQL query or a debug endpoint rather than the JPA entity, it could currently show duplicate permission entries. After V62, this inconsistency disappears at the data layer. The smoke AC ("open /admin/groups/<id>, edit permissions") should confirm the UI never showed duplicates to begin with, and continues to work after the migration.
No visual regression risk. The permissions editor renders from the Set<String> returned by the API, which deduplicates at the JPA layer regardless of the underlying table state. The migration cannot introduce a new visible state.

Recommendations

Add a brief smoke step to the acceptance criteria from a UX perspective: "After migration, the group permission editor loads, allows adding and removing individual permissions, and the saved state reflects exactly the selected permissions — no phantom duplicates visible in the UI or in the API response body."
Confirm the group editor uses the JPA-backed API response (not a raw SQL view) for permission display. Given that GroupController.getAllGroups() returns userService.getAllGroups() which calls groupRepository.findAll() (JPA), this is already correctly using the Set-deduplicated path. No UI change needed.
No touch-target, contrast, or accessibility concerns on this issue — it is purely a schema migration with no new UI component. This issue is out of scope for UX review beyond the smoke validation above.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Designer ### Observations - **This is a database schema fix with no direct UI surface.** The admin group editor at `/admin/groups/<id>` is the only user-facing area affected, and the change is purely a correctness fix — the UI already behaves as if duplicates don't exist (JPA `Set<String>` semantics deduplicate transparently). - **There is an indirect UX concern in the admin tooling.** If any admin-facing screen lists permissions per group using a raw SQL query or a debug endpoint rather than the JPA entity, it could currently show duplicate permission entries. After V62, this inconsistency disappears at the data layer. The smoke AC ("open `/admin/groups/<id>`, edit permissions") should confirm the UI never showed duplicates to begin with, and continues to work after the migration. - **No visual regression risk.** The permissions editor renders from the `Set<String>` returned by the API, which deduplicates at the JPA layer regardless of the underlying table state. The migration cannot introduce a new visible state. ### Recommendations - **Add a brief smoke step to the acceptance criteria from a UX perspective:** "After migration, the group permission editor loads, allows adding and removing individual permissions, and the saved state reflects exactly the selected permissions — no phantom duplicates visible in the UI or in the API response body." - **Confirm the group editor uses the JPA-backed API response (not a raw SQL view) for permission display.** Given that `GroupController.getAllGroups()` returns `userService.getAllGroups()` which calls `groupRepository.findAll()` (JPA), this is already correctly using the Set-deduplicated path. No UI change needed. - **No touch-target, contrast, or accessibility concerns on this issue** — it is purely a schema migration with no new UI component. This issue is out of scope for UX review beyond the smoke validation above.

marcel commented

2026-05-07 17:50:43 +02:00

🗳️ Decision Queue — Open Items for Human Judgment

Three genuine tradeoffs were raised across the review. No blocking decisions, but each affects the implementation scope.

Theme 1: Constrain `permission` to known enum values via CHECK

Raised by: Nora (Security)

A CHECK (permission IN ('READ_ALL', 'WRITE_ALL', 'ADMIN', 'ADMIN_USER', 'ADMIN_TAG', 'ADMIN_PERMISSION', 'ANNOTATE_ALL', 'BLOG_WRITE')) constraint would prevent garbage strings from entering group_permissions via raw SQL or a future migration bug.

Tradeoff: Every new Permission enum value would require a companion Flyway migration to widen the CHECK — coupling the permission enum and the database schema. Currently, adding a new permission (e.g., V59 adds BLOG_WRITE) requires only the seed migration, not a schema alteration.

Options:

Add the CHECK now — stronger integrity, tighter coupling, slightly more migration work per new permission
Leave unconstrained — current behavior, simpler, relies on application-layer enum validation
Defer — revisit if a garbage-permission bug is ever observed in production

Theme 2: How to seed duplicate fixtures in the migration test

Raised by: Sara (QA)

To meaningfully test V61 (the dedup migration), the test needs a database state with duplicate (group_id, permission) rows before V61 runs. There are two approaches:

Option A — Flyway target version: Run all migrations up to V60, insert duplicates via JdbcTemplate, then apply V61+V62 and assert. Requires Flyway target configuration in the test.

Option B — Raw SQL fixture after all migrations: Run all migrations (V61+V62 applied), then test only the V62 post-state (PK exists, no duplicates). Simpler but does not actually test that V61's dedup query executed correctly — it only tests the end state.

Recommendation from Sara: Option A (test V61 in action, not just the end state).

Theme 3: Automate the production pre-flight check in CI

Raised by: Tobias (DevOps)

The pre-flight duplicate check (SELECT ... GROUP BY ... HAVING COUNT(*) > 1) could be added as a CI step that runs against a production DB snapshot, or kept as a manual deploy checklist item.

Tradeoff: Automating requires a production DB snapshot in CI (adds infrastructure and secrets exposure risk). For this project's scale, manual is sufficient and lower-risk.

Recommendation from Tobias: Keep manual for now; document in the PR description as a required deploy checklist step.

## 🗳️ Decision Queue — Open Items for Human Judgment Three genuine tradeoffs were raised across the review. No blocking decisions, but each affects the implementation scope. --- ### Theme 1: Constrain `permission` to known enum values via CHECK **Raised by:** Nora (Security) A `CHECK (permission IN ('READ_ALL', 'WRITE_ALL', 'ADMIN', 'ADMIN_USER', 'ADMIN_TAG', 'ADMIN_PERMISSION', 'ANNOTATE_ALL', 'BLOG_WRITE'))` constraint would prevent garbage strings from entering `group_permissions` via raw SQL or a future migration bug. **Tradeoff:** Every new `Permission` enum value would require a companion Flyway migration to widen the CHECK — coupling the permission enum and the database schema. Currently, adding a new permission (e.g., V59 adds `BLOG_WRITE`) requires only the seed migration, not a schema alteration. **Options:** - **Add the CHECK now** — stronger integrity, tighter coupling, slightly more migration work per new permission - **Leave unconstrained** — current behavior, simpler, relies on application-layer enum validation - **Defer** — revisit if a garbage-permission bug is ever observed in production --- ### Theme 2: How to seed duplicate fixtures in the migration test **Raised by:** Sara (QA) To meaningfully test V61 (the dedup migration), the test needs a database state with duplicate `(group_id, permission)` rows before V61 runs. There are two approaches: **Option A — Flyway `target` version:** Run all migrations up to V60, insert duplicates via `JdbcTemplate`, then apply V61+V62 and assert. Requires Flyway `target` configuration in the test. **Option B — Raw SQL fixture after all migrations:** Run all migrations (V61+V62 applied), then test only the V62 post-state (PK exists, no duplicates). Simpler but does not actually test that V61's dedup query executed correctly — it only tests the end state. **Recommendation from Sara:** Option A (test V61 in action, not just the end state). --- ### Theme 3: Automate the production pre-flight check in CI **Raised by:** Tobias (DevOps) The pre-flight duplicate check (`SELECT ... GROUP BY ... HAVING COUNT(*) > 1`) could be added as a CI step that runs against a production DB snapshot, or kept as a manual deploy checklist item. **Tradeoff:** Automating requires a production DB snapshot in CI (adds infrastructure and secrets exposure risk). For this project's scale, manual is sufficient and lower-risk. **Recommendation from Tobias:** Keep manual for now; document in the PR description as a required deploy checklist step.

marcel referenced this issue from a commit

2026-05-09 15:20:23 +02:00

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

marcel referenced a pull request that will close this issue

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

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

marcel commented

2026-05-09 15:20:41 +02:00

Implemented in PR #492 (fix/issue-469-group-permissions-pk).

Commits:

3fcdfa85 — V63 (dedup) + V64 (PK + NOT NULL) + V65 (tbmp rename) + 4 migration integration tests
eb54a98e — createGroup builder + null-guard + unit test

All 5 acceptance criteria satisfied. 1551 backend tests green.

Implemented in PR #492 (`fix/issue-469-group-permissions-pk`). **Commits:** - `3fcdfa85` — V63 (dedup) + V64 (PK + NOT NULL) + V65 (tbmp rename) + 4 migration integration tests - `eb54a98e` — `createGroup` builder + null-guard + unit test All 5 acceptance criteria satisfied. 1551 backend tests green.

marcel referenced a pull request that will close this issue

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

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

marcel closed this issue

2026-05-09 15:44:36 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#469

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

Context

Approach

V61__deduplicate_group_permissions.sql

V62__group_permissions_primary_key.sql

Companion: declare the de-facto PK on the other table

Critical files

Verification

Acceptance criteria

Effort

Risk if not addressed

🏛️ Markus Keller (@mkeller) — Application Architect

Observations

Recommendations

👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer

Observations

Recommendations

🔒 Nora "NullX" Steiner — Security Engineer

Observations

Recommendations

Open Decisions

🧪 Sara Holt (@saraholt) — QA Engineer

Observations

Recommendations

Open Decisions

⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

📋 Elicit — Requirements Engineer

Observations

Recommendations

🎨 Leonie Voss (@leonievoss) — UI/UX Designer

Observations

Recommendations

🗳️ Decision Queue — Open Items for Human Judgment

Theme 1: Constrain permission to known enum values via CHECK

Theme 2: How to seed duplicate fixtures in the migration test

Theme 3: Automate the production pre-flight check in CI

Theme 1: Constrain `permission` to known enum values via CHECK