Add the change-member-role endpoint, Owner only #57

Merged

rob merged 2 commits from feat/change-member-role-endpoint into main

2026-08-05 05:27:03 +00:00

rob commented

2026-08-05 05:00:12 +00:00

Owner

Implements task 84: PUT /api/groups/{groupId}/members/{userId}, Owner only.

Refuses to grant Owner through this endpoint (400, role field) — ADR-0052, mirroring ADR-0050's reasoning for invitations. Transfer of ownership (task 132) is the only way to create a second Owner.
Refuses to demote a group's last accepted Owner with 409, closed against concurrent mutual demotion by a single locked SQL statement (WITH ... FOR UPDATE CTE feeding the UPDATE), not a read-then-write check. Proved with genuinely concurrent callers in both the repository tests and the endpoint tests.
Self-demotion is allowed when another Owner remains; changing a pending invitee's stored role is allowed; a personal group needs no special case (its sole membership is covered by the ordinary last-Owner rule).
Editor/Viewer callers get 403; non-member and nonexistent-group both get an identical 404; unauthenticated gets 401.

ADR-0052 records the two decisions and the concurrency design in full.

Took ADR number 0052 — checked against main and the open feat/list-invitations-endpoint PR immediately before pushing; neither claims it.

Implements task 84: `PUT /api/groups/{groupId}/members/{userId}`, Owner only. - Refuses to grant Owner through this endpoint (`400`, `role` field) — ADR-0052, mirroring ADR-0050's reasoning for invitations. Transfer of ownership (task 132) is the only way to create a second Owner. - Refuses to demote a group's last accepted Owner with `409`, closed against concurrent mutual demotion by a single locked SQL statement (`WITH ... FOR UPDATE` CTE feeding the `UPDATE`), not a read-then-write check. Proved with genuinely concurrent callers in both the repository tests and the endpoint tests. - Self-demotion is allowed when another Owner remains; changing a pending invitee's stored role is allowed; a personal group needs no special case (its sole membership is covered by the ordinary last-Owner rule). - Editor/Viewer callers get 403; non-member and nonexistent-group both get an identical 404; unauthenticated gets 401. ADR-0052 records the two decisions and the concurrency design in full. Took ADR number 0052 — checked against `main` and the open `feat/list-invitations-endpoint` PR immediately before pushing; neither claims it.

rob added 1 commit

2026-08-05 05:00:12 +00:00

Add the change-member-role endpoint, Owner only

CI / build (pull_request) Successful in 2m13s

Details

f8ac3984ce

rob reviewed

2026-08-05 05:13:55 +00:00

rob left a comment

No description provided.

Verdict: mergeable The last-Owner invariant holds up under direct attack. `ChangeRoleAsync`'s `WITH locked_owners AS MATERIALIZED (... FOR UPDATE)` locks every accepted-Owner row of the group before the outer `UPDATE`'s guard is evaluated, so a second concurrent demotion blocks on the same rows rather than reading a stale answer, and PostgreSQL's `READ COMMITTED` re-check (EvalPlanQual) on the row it was waiting on correctly drops a now-demoted Owner out of `locked_owners` before the guard re-evaluates. Verified independently: - Ran `ChangeRoleAsync_ConcurrentMutualDemotionOfTheOnlyTwoOwners...` and `PutChangeRole_ConcurrentMutualDemotionOfTheOnlyTwoOwners...` 30 and 20 times respectively against real PostgreSQL (Testcontainers) — 50/50 pass, exactly one Owner survives every time. - Reintroduced a naive check-then-act (`SELECT COUNT` then plain `UPDATE`, no lock) in place of the CTE and reran the repository test 20 times — 20/20 failures, both demotions succeed, group left with zero Owners. The test is not decorative; it genuinely distinguishes the locked write from check-then-act. - Pending-Owner-row guard, self-demotion with a co-Owner present, and the personal-group case (single membership, caught by the ordinary rule, no special case needed) are all covered by both direct tests and reasoned through against ADR-0018/0052 — hold up. - ADR-0052's Owner refusal mirrors ADR-0050's wording and reasoning, and is enforced in `GroupEndpoints.PutChangeMemberRole` before either membership row is read (400, `role` field, `ChangeMemberRoleEndpointTests.PutChangeRole_RoleIsOwner_...` covers it), not merely documented. - Owner-only enforcement, identical 404 for non-member/nonexistent-group, `updated_at` advancing, entities-off-the-wire, and the `OpenApiDocumentTests` path addition all check out against the actual route table and handler. - ADR index (`docs/adr/README.md`) is in numeric order; branch is already even with `main`, no merge needed. - Ran `dotnet test PlaceMark.slnx --no-build` (the whole suite, as CI does) three times with the pinned 10.0.100 SDK — 0 failures across all three runs. The port-collision risk the author avoided by isolating projects is real but pre-existing and out of this PR's diff: `ReadinessAgainstPostgresTests.ReserveAPort()` probes a free TCP port, releases it, then hands it to Testcontainers for a fixed publish binding — an acknowledged TOCTOU (the class's own remarks call it "a strong hint rather than a guarantee"). It predates this branch and didn't reproduce in three full runs here, so the author's call that it's unrelated to this change stands, but it's worth a ticket of its own given it's a known, named source of occasional CI flakiness rather than a one-off. Nothing here blocks merge.

rob added 1 commit

2026-08-05 05:18:55 +00:00

Merge main into feat/change-member-role-endpoint

CI / build (pull_request) Successful in 2m2s

Details

a72b0d3a53

rob commented

2026-08-05 05:19:02 +00:00

Author

Owner

Merged main in — GroupMembershipRepository.cs conflicted with PR #56's ListPendingForUserAsync. Resolved by hand (only the class-level doc comment actually conflicted; every constant and method was auto-merged into complete, non-interleaved blocks — verified by reading the whole file and confirming method/constant counts). Full test suite and dotnet format --verify-no-changes clean under the pinned SDK. This pushes new commits on top of the head the mergeable review covered, so it needs a fresh look.

Merged main in — `GroupMembershipRepository.cs` conflicted with PR #56's `ListPendingForUserAsync`. Resolved by hand (only the class-level doc comment actually conflicted; every constant and method was auto-merged into complete, non-interleaved blocks — verified by reading the whole file and confirming method/constant counts). Full test suite and `dotnet format --verify-no-changes` clean under the pinned SDK. This pushes new commits on top of the head the mergeable review covered, so it needs a fresh look.

rob reviewed

2026-08-05 05:26:41 +00:00

rob left a comment

No description provided.

Verdict: mergeable Re-reviewed the merge commit (`a72b0d3`) only, against both parents (`f8ac398` this branch, `44ce558` main post-PR#56). - Diff scope confirmed narrow: only `GroupMembershipRepository.cs` and its test file appear in both parent diffs; everything else is a pure union of the two PRs' own files. - Read `GroupMembershipRepository.cs` end to end. Diffing the merged file against each parent shows only whole, non-interleaved insertions — the class-doc-comment lines, plus `ListPendingForUserAsync` (with its two SQL constants) inserted intact against the branch-tip parent, and `ChangeRoleAsync` (with its `ChangeRole` CTE constant) inserted intact against the main parent. No fragment of one method appears inside another; `diff` on each method's line range against its source parent is byte-identical. - Method/constant counts reconcile exactly: parent (branch) has 7 methods, parent (main) has 7 methods, 6 shared; merged has 8 = union. Constants: 7 (branch) + 8 (main), 6 shared, merged has 9 = union. - Test file: 24 tests on the branch parent, 23 on main, 17 shared; merged has 30 = exact union, confirmed by name. `ListPendingForUserAsync`'s 6 tests from main and `ChangeRoleAsync`'s 7 tests from this branch (including the concurrent-demotion test) are all present, plus the `DeleteUser` helper main added. Diff against main is a single clean hunk (all `ChangeRoleAsync` tests appended as one block); diff against the branch parent is three clean hunks, all pure additions. - Rebuilt with the pinned 10.0.100 SDK (clean, 0 warnings/errors) and re-ran the concurrent-demotion tests against the merged code specifically, since the file changed since the last check: `ChangeRoleAsync_ConcurrentMutualDemotionOfTheOnlyTwoOwners...` 30/30 and `PutChangeRole_ConcurrentMutualDemotionOfTheOnlyTwoOwners...` 20/20, both passing every time. Also ran `PlaceMark.Infrastructure.Tests` (225 tests) and `PlaceMark.Api.Tests` (442 tests) in full once each — no failures, no interference between the two features' tests. No fragment merge, no dropped test, invariant still holds post-merge. Nothing to act on.

rob merged commit 98e2d7bd2e into main

2026-08-05 05:27:03 +00:00

rob deleted branch feat/change-member-role-endpoint

2026-08-05 05:27:03 +00:00

rob referenced this pull request from a commit

2026-08-05 05:27:05 +00:00

Add the change-member-role endpoint, Owner only (#57)