Add the remove-member endpoint, Owner or self #58

Merged

rob merged 5 commits from feat/remove-member-endpoint into main 2026-08-05 06:44:07 +00:00

Conversation 3 Commits 5 Files changed 7 +1158 -30

rob commented 2026-08-05 05:40:37 +00:00

Owner

Copy link

DELETE /api/groups/{groupId}/members/{userId}: an Owner removing another member, or any member leaving on their own.

• Route declares RequireGroupCapability(GroupCapability.Member); the handler makes a second, narrower AuthorizeGroupCapabilityAsync(..., GroupCapability.Owner, ...) check only when the target isn't the caller, reusing the shared 403/404 decision rather than duplicating it. Argued in ADR-0053.
• Last-Owner invariant enforced by RemoveAsync's locked CTE, written byte-for-byte identical to task 84's ChangeRoleAsync over the same predicate, so the two serialise against each other. Verified against task 84's actual implementation (merged forward from main), including a permanent automated test (RemoveAsyncAndChangeRoleAsync_ConcurrentMutualActionOnTheOnlyTwoOwners_AnOwnerAlwaysSurvives) that fires both concurrently.
• Removing a pending invitee revokes the invitation, deliberately. Hard delete of the membership row only; the removed member's places stay with the group (tested explicitly). Personal group's sole owner can never remove themselves (409, same last-Owner rule).

Integration tests cover Owner/self/Editor/Viewer/non-member/nonexistent-group cases plus two same-endpoint and one cross-endpoint concurrency test.

`DELETE /api/groups/{groupId}/members/{userId}`: an Owner removing another member, or any member leaving on their own. - Route declares `RequireGroupCapability(GroupCapability.Member)`; the handler makes a second, narrower `AuthorizeGroupCapabilityAsync(..., GroupCapability.Owner, ...)` check only when the target isn't the caller, reusing the shared 403/404 decision rather than duplicating it. Argued in [ADR-0053](docs/adr/0053-authorise-member-removal-by-target-identity.md). - Last-Owner invariant enforced by `RemoveAsync`'s locked CTE, written byte-for-byte identical to task 84's `ChangeRoleAsync` over the same predicate, so the two serialise against each other. Verified against task 84's actual implementation (merged forward from `main`), including a permanent automated test (`RemoveAsyncAndChangeRoleAsync_ConcurrentMutualActionOnTheOnlyTwoOwners_AnOwnerAlwaysSurvives`) that fires both concurrently. - Removing a pending invitee revokes the invitation, deliberately. Hard delete of the membership row only; the removed member's places stay with the group (tested explicitly). Personal group's sole owner can never remove themselves (409, same last-Owner rule). Integration tests cover Owner/self/Editor/Viewer/non-member/nonexistent-group cases plus two same-endpoint and one cross-endpoint concurrency test.

rob added 3 commits 2026-08-05 05:40:37 +00:00

Add the remove-member endpoint, Owner or self fa8d8ecd9e

Merge remote-tracking branch 'origin/main' into feat/remove-member-endpoint ... 9ba98337b6

# Conflicts:
#	docs/adr/README.md
#	src/PlaceMark.Api/Groups/GroupEndpoints.cs
#	src/PlaceMark.Infrastructure/Groups/GroupMembershipRepository.cs
#	tests/PlaceMark.Infrastructure.Tests/Groups/GroupMembershipRepositoryTests.cs

Update ADR-0053 now the cross-endpoint test is automated 55f58467a4

rob referenced this pull request 2026-08-05 05:43:41 +00:00

Add the promote-to-owner endpoint for group Owners #59

rob reviewed 2026-08-05 05:59:41 +00:00

rob left a comment

Copy link

Verdict: mergeable

Checked both named risks directly, not just by reading:

• Deleted the second AuthorizeGroupCapabilityAsync(..., GroupCapability.Owner, ...) call in DeleteGroupMember. Only this endpoint's own tests caught it (AViewerRemovingSomeoneElse, AnEditorRemovingSomeoneElse both failed); GroupScopedEndpointAuthorisationTests (task 148) stayed green, confirming the structural safety net only sees GroupCapability.Member on this route, as ADR-0053 states. Coverage here rests entirely on DeleteGroupMemberEndpointTests — same posture as the invitation-response routes under ADR-0051, correctly called out rather than left implicit.
• RemoveMember and ChangeRole's locked_owners CTEs are byte-for-byte identical (diffed directly).
• Broke RemoveAsync into a plain read-then-write. With cold/unwarmed connections the race didn't reproduce in isolated runs — ChangeRoleAsync's single round trip reliably wins before RemoveAsync's extra round trip even reads, which masked the defect. Once run in its normal context (full test class, pool warmed by ~40 preceding tests, matching how it actually runs in CI), both RemoveAsync_ConcurrentMutualRemovalOfTheOnlyTwoOwners... and RemoveAsyncAndChangeRoleAsync_ConcurrentMutualActionOnTheOnlyTwoOwners_AnOwnerAlwaysSurvives failed consistently (5/5 runs) against the broken locking. The test is genuine.
• Confirmed a pending Owner row doesn't protect the invariant in either direction — RemoveAsync_ANonOwnerWithAPendingOwnerRowSomehowPresent_TreatsAPendingOwnerAsNotProtecting and the ChangeRoleAsync equivalent both exist and pass, matching the SQL guard's status = 'accepted' qualifier.

Also verified: last Owner can't remove themselves (409, including personal group); removed member's places stay with the group (ADR-0004, tested explicitly); removing a pending invitee revokes the invitation (204); non-member and nonexistent-group both 404 with identical body; ADR-0053's claims about PlaceAuthorizationEndpointExtensions reusing AuthorizeGroupCapabilityAsync and about task 148's exclusion boundary both check out against the code; ADR number 0053 is free and the README index entry is correctly appended; the merge-forward of PR #57 dropped nothing — diffed RemoveAsync/RemoveMember against the pre-merge branch tip and ChangeRoleAsync/ChangeRole against main byte-for-byte, only a local variable rename (existing → removeExisting) came out of the merge.

Build and full test suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100).

Nothing to change.

Verdict: mergeable Checked both named risks directly, not just by reading: - Deleted the second `AuthorizeGroupCapabilityAsync(..., GroupCapability.Owner, ...)` call in `DeleteGroupMember`. Only this endpoint's own tests caught it (`AViewerRemovingSomeoneElse`, `AnEditorRemovingSomeoneElse` both failed); `GroupScopedEndpointAuthorisationTests` (task 148) stayed green, confirming the structural safety net only sees `GroupCapability.Member` on this route, as ADR-0053 states. Coverage here rests entirely on `DeleteGroupMemberEndpointTests` — same posture as the invitation-response routes under ADR-0051, correctly called out rather than left implicit. - `RemoveMember` and `ChangeRole`'s `locked_owners` CTEs are byte-for-byte identical (diffed directly). - Broke `RemoveAsync` into a plain read-then-write. With cold/unwarmed connections the race didn't reproduce in isolated runs — ChangeRoleAsync's single round trip reliably wins before RemoveAsync's extra round trip even reads, which masked the defect. Once run in its normal context (full test class, pool warmed by ~40 preceding tests, matching how it actually runs in CI), both `RemoveAsync_ConcurrentMutualRemovalOfTheOnlyTwoOwners...` and `RemoveAsyncAndChangeRoleAsync_ConcurrentMutualActionOnTheOnlyTwoOwners_AnOwnerAlwaysSurvives` failed consistently (5/5 runs) against the broken locking. The test is genuine. - Confirmed a pending Owner row doesn't protect the invariant in either direction — `RemoveAsync_ANonOwnerWithAPendingOwnerRowSomehowPresent_TreatsAPendingOwnerAsNotProtecting` and the `ChangeRoleAsync` equivalent both exist and pass, matching the SQL guard's `status = 'accepted'` qualifier. Also verified: last Owner can't remove themselves (409, including personal group); removed member's places stay with the group (ADR-0004, tested explicitly); removing a pending invitee revokes the invitation (204); non-member and nonexistent-group both 404 with identical body; ADR-0053's claims about `PlaceAuthorizationEndpointExtensions` reusing `AuthorizeGroupCapabilityAsync` and about task 148's exclusion boundary both check out against the code; ADR number 0053 is free and the README index entry is correctly appended; the merge-forward of PR #57 dropped nothing — diffed `RemoveAsync`/`RemoveMember` against the pre-merge branch tip and `ChangeRoleAsync`/`ChangeRole` against `main` byte-for-byte, only a local variable rename (`existing` → `removeExisting`) came out of the merge. Build and full test suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100). Nothing to change.

rob added 1 commit 2026-08-05 06:13:00 +00:00

Merge main (promote-to-owner) into feat/remove-member-endpoint 76ac7bd48e

rob reviewed 2026-08-05 06:24:55 +00:00

rob left a comment

Copy link

Verdict: changes needed

Re-reviewed the delta 55f5846..76ac7bd.

Merge integrity: clean. Counted SQL constants (11: exactly union of both parents), methods (9 public + RoleColumnValue), and test attributes (45 = 44-method union of both parents' test files, none dropped, plus the one new cross-writer test) directly rather than trusting the description. Read GroupMembershipRepository.cs and GroupEndpoints.cs end to end — both coherent, RemoveMember/RemoveAsync/ChangeRole/ChangeRoleAsync byte-identical to their pre-merge sources. No sign of the interleaved-splice failure mode named in the brief.

The SQL-safety argument is sound — verified independently, not just re-derived from the ADR's own text. Any row in locked_owners is FOR UPDATE-locked and therefore guaranteed to stay an accepted Owner for the statement's duration; PromoteToOwnerAsync can only ever add rows to the true Owner set, never invalidate one already locked. So RemoveMember's guard can be stale by omission only (refuses a removal that was in fact safe) and never wrongly permissive. Worked through both commit orderings by hand; both leave ≥1 Owner. This part of ADR-0053's new section does not overclaim.

The new test does not prove it, and that claim in ADR-0053 does overclaim. RemoveAsync_ConcurrentWithPromoteToOwnerOfADifferentMember_NeverLeavesTheGroupWithZeroOwners promotes an unrelated, unconditionally-promotable accepted member concurrently with the removal. Because PromoteToOwnerAsync always succeeds in this setup regardless of anything RemoveAsync does, the assertion (acceptedOwnerCount >= 1) is satisfied by the promoted member alone no matter how RemoveAsync behaves — correctly guarded, unguarded, or entirely broken.

Confirmed this by deleting RemoveMember's entire last-Owner guard (unconditional DELETE, no CTE, no lock) and running the new test: 8/8 standalone runs green, 5/5 full-class runs green. The three pre-existing RemoveAsync-only tests (TheGroupsLastAcceptedOwner, APersonalGroupsSoleOwner, ANonOwnerWithAPendingOwnerRowSomehowPresent) and the mutual-removal concurrency test did catch the same mutation, isolating the gap to this one test. The same shape exists in ADR-0054's own PromoteToOwnerAsync_ConcurrentWithSelfDemotionOfTheOnlyOwner_NeverLeavesTheGroupWithZeroOwners (not in this PR's diff, but named as the pattern this test "mirrors") — worth knowing even though it's out of scope here.

So: the underlying interaction is genuinely safe, but "proves this rather than resting on the argument alone" (ADR-0053, § A third writer) is not true of the test as written — it would pass unchanged if RemoveAsync's guard were deleted outright. Either soften that sentence to say the test confirms the two operations coexist without throwing/deadlocking rather than that it proves the invariant, or reshape the test so RemoveAsync's own correctness is load-bearing to the assertion (e.g. assert the specific outcome for each commit ordering rather than only the union, or target a scenario — if one exists — where an unguarded RemoveAsync would actually zero the count against a concurrent promotion).

ADR index: 0053 and 0054 both present, correct numeric order. OpenApiDocumentTests.cs's one-line delta is just the new route added to the enumerated list — unremarkable. Build and full suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100).

Verdict: changes needed Re-reviewed the delta 55f5846..76ac7bd. **Merge integrity: clean.** Counted SQL constants (11: exactly union of both parents), methods (9 public + `RoleColumnValue`), and test attributes (45 = 44-method union of both parents' test files, none dropped, plus the one new cross-writer test) directly rather than trusting the description. Read `GroupMembershipRepository.cs` and `GroupEndpoints.cs` end to end — both coherent, `RemoveMember`/`RemoveAsync`/`ChangeRole`/`ChangeRoleAsync` byte-identical to their pre-merge sources. No sign of the interleaved-splice failure mode named in the brief. **The SQL-safety argument is sound** — verified independently, not just re-derived from the ADR's own text. Any row in `locked_owners` is `FOR UPDATE`-locked and therefore guaranteed to stay an accepted Owner for the statement's duration; `PromoteToOwnerAsync` can only ever add rows to the true Owner set, never invalidate one already locked. So `RemoveMember`'s guard can be stale by omission only (refuses a removal that was in fact safe) and never wrongly permissive. Worked through both commit orderings by hand; both leave ≥1 Owner. This part of ADR-0053's new section does not overclaim. **The new test does not prove it, and that claim in ADR-0053 does overclaim.** `RemoveAsync_ConcurrentWithPromoteToOwnerOfADifferentMember_NeverLeavesTheGroupWithZeroOwners` promotes an *unrelated*, unconditionally-promotable accepted member concurrently with the removal. Because `PromoteToOwnerAsync` always succeeds in this setup regardless of anything `RemoveAsync` does, the assertion (`acceptedOwnerCount >= 1`) is satisfied by the promoted member alone no matter how `RemoveAsync` behaves — correctly guarded, unguarded, or entirely broken. Confirmed this by deleting `RemoveMember`'s entire last-Owner guard (unconditional `DELETE`, no CTE, no lock) and running the new test: **8/8 standalone runs green, 5/5 full-class runs green.** The three pre-existing `RemoveAsync`-only tests (`TheGroupsLastAcceptedOwner`, `APersonalGroupsSoleOwner`, `ANonOwnerWithAPendingOwnerRowSomehowPresent`) and the mutual-removal concurrency test did catch the same mutation, isolating the gap to this one test. The same shape exists in ADR-0054's own `PromoteToOwnerAsync_ConcurrentWithSelfDemotionOfTheOnlyOwner_NeverLeavesTheGroupWithZeroOwners` (not in this PR's diff, but named as the pattern this test "mirrors") — worth knowing even though it's out of scope here. So: the underlying interaction is genuinely safe, but "proves this rather than resting on the argument alone" (ADR-0053, § *A third writer*) is not true of the test as written — it would pass unchanged if `RemoveAsync`'s guard were deleted outright. Either soften that sentence to say the test confirms the two operations coexist without throwing/deadlocking rather than that it proves the invariant, or reshape the test so `RemoveAsync`'s own correctness is load-bearing to the assertion (e.g. assert the specific outcome for each commit ordering rather than only the union, or target a scenario — if one exists — where an unguarded `RemoveAsync` would actually zero the count against a concurrent promotion). ADR index: 0053 and 0054 both present, correct numeric order. `OpenApiDocumentTests.cs`'s one-line delta is just the new route added to the enumerated list — unremarkable. Build and full suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100).

rob added 1 commit 2026-08-05 06:35:06 +00:00

Replace the vacuous promote-versus-remove race test 3fd53c8ba8

rob reviewed 2026-08-05 06:43:44 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed the delta 76ac7bd..3fd53c8 — confirmed it touches only GroupMembershipRepositoryTests.cs and ADR-0053, nothing else.

The new test discriminates. Deleted RemoveMember's last-Owner guard entirely (unconditional DELETE, no CTE, no lock) and ran RemoveAsync_ConcurrentWithPromoteToOwnerOfTheSameSoleOwner_AlwaysRefusesAndThePromotionSucceeds: 10/10 standalone failures, 5/5 full-class failures (alongside the same three solo RemoveAsync tests and the mutual-removal concurrency test as before) — matches the author's reported 5/5. On the shipped, guarded code: 10/10 standalone and 8/8 full-class runs green. Restored the file; diffed byte-for-byte against a pre-mutation backup to confirm the restore was exact.

Determinism verified independently, not just re-run. Worked through both lock-acquisition orderings by hand rather than trusting repeated green runs alone:

• If RemoveMember's CTE locks the row first: locked_owners = {ownerId} only, so EXISTS(... <> @UserId) is false regardless of role/status — the guard structurally cannot pass, since there is only one membership row in this group and it's the target. Deletion is refused; the row is unchanged, so PromoteMemberToOwner's WHERE status = 'accepted' still matches once it runs and promotion succeeds.
• If PromoteMemberToOwner's UPDATE takes the row lock first: it commits a no-op role write, RemoveMember's CTE then acquires the lock and (via EvalPlanQual) re-reads the still-owner/accepted row into locked_owners = {ownerId} only — same structural refusal.

Both orderings collapse to the same pair of outcomes because this is a single-row group: nothing either statement can do creates a second distinct accepted-Owner row for locked_owners to find, and nothing deletes the row before promotion's WHERE can match it. The exact-outcome assertions are sound by construction, not merely by luck of repeated runs.

Promoting an already-Owner is genuinely legitimate, and not a new assumption introduced by this test: PromoteToOwnerEndpointTests.PromoteToOwner_AnAlreadyOwner_RespondsWith204Idempotently and GroupMembershipRepositoryTests.PromoteToOwnerAsync_AnAlreadyAcceptedOwner_PromotesIdempotentlyAndAdvancesUpdatedAt already cover it (from PR #59), and PromoteMemberToOwner's own WHERE clause carries no role condition, only status = 'accepted' — so the new test rests on pre-existing, already-tested behaviour rather than a fresh, unverified premise.

ADR-0053's revised wording is accurate and doesn't overclaim in either direction. It states plainly that the different-member scenario has no automated test and explains structurally why no test built on two independent rows could discriminate there — matches what I found in the prior review round. It's equally honest about the new test's own scope (same-row only) and explicitly flags the residual risk: a regression that breaks the different-member case without also breaking the same-row case would ship silently. Nothing here overstates coverage the tests don't actually provide.

Build and full suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100).

Verdict: mergeable Re-reviewed the delta 76ac7bd..3fd53c8 — confirmed it touches only `GroupMembershipRepositoryTests.cs` and ADR-0053, nothing else. **The new test discriminates.** Deleted `RemoveMember`'s last-Owner guard entirely (unconditional `DELETE`, no CTE, no lock) and ran `RemoveAsync_ConcurrentWithPromoteToOwnerOfTheSameSoleOwner_AlwaysRefusesAndThePromotionSucceeds`: 10/10 standalone failures, 5/5 full-class failures (alongside the same three solo `RemoveAsync` tests and the mutual-removal concurrency test as before) — matches the author's reported 5/5. On the shipped, guarded code: 10/10 standalone and 8/8 full-class runs green. Restored the file; diffed byte-for-byte against a pre-mutation backup to confirm the restore was exact. **Determinism verified independently, not just re-run.** Worked through both lock-acquisition orderings by hand rather than trusting repeated green runs alone: - If `RemoveMember`'s CTE locks the row first: `locked_owners` = {ownerId} only, so `EXISTS(... <> @UserId)` is false regardless of role/status — the guard structurally cannot pass, since there is only one membership row in this group and it's the target. Deletion is refused; the row is unchanged, so `PromoteMemberToOwner`'s `WHERE status = 'accepted'` still matches once it runs and promotion succeeds. - If `PromoteMemberToOwner`'s `UPDATE` takes the row lock first: it commits a no-op role write, `RemoveMember`'s CTE then acquires the lock and (via `EvalPlanQual`) re-reads the still-`owner`/`accepted` row into `locked_owners` = {ownerId} only — same structural refusal. Both orderings collapse to the same pair of outcomes because this is a *single-row* group: nothing either statement can do creates a second distinct accepted-Owner row for `locked_owners` to find, and nothing deletes the row before promotion's `WHERE` can match it. The exact-outcome assertions are sound by construction, not merely by luck of repeated runs. **Promoting an already-Owner is genuinely legitimate**, and not a new assumption introduced by this test: `PromoteToOwnerEndpointTests.PromoteToOwner_AnAlreadyOwner_RespondsWith204Idempotently` and `GroupMembershipRepositoryTests.PromoteToOwnerAsync_AnAlreadyAcceptedOwner_PromotesIdempotentlyAndAdvancesUpdatedAt` already cover it (from PR #59), and `PromoteMemberToOwner`'s own `WHERE` clause carries no role condition, only `status = 'accepted'` — so the new test rests on pre-existing, already-tested behaviour rather than a fresh, unverified premise. **ADR-0053's revised wording is accurate and doesn't overclaim in either direction.** It states plainly that the different-member scenario has no automated test and explains structurally why no test built on two independent rows could discriminate there — matches what I found in the prior review round. It's equally honest about the new test's own scope (same-row only) and explicitly flags the residual risk: a regression that breaks the different-member case without also breaking the same-row case would ship silently. Nothing here overstates coverage the tests don't actually provide. Build and full suite (Api, Infrastructure, Architecture) clean on the pinned SDK (10.0.100).

rob merged commit c90a9a41d7 into main 2026-08-05 06:44:07 +00:00

rob deleted branch feat/remove-member-endpoint 2026-08-05 06:44:07 +00:00

rob referenced this pull request from a commit 2026-08-05 06:44:07 +00:00

Add the remove-member endpoint (#58)

rob referenced this pull request 2026-08-05 07:22:16 +00:00

Add the move-place-between-groups endpoint #61

rob referenced this pull request from a commit 2026-08-05 07:29:18 +00:00

Add the move-place-between-groups endpoint (#61)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!58

No description provided.