Add the list groups for current user endpoint #44
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/list-groups-endpoint"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements task 74:
GET /api/groups, paginated, returning only accepted memberships (personal group included), each carrying the caller's role. Scoping is entirely in the SQLWHEREclause — no group-scoped policy needed.Adds ADR-0043 (next free number on
mainat the time of writing), deciding two things ADR-0009 explicitly deferred to this endpoint: a genericPagedResponse<T>envelope for list endpoints, and a wire-sideGroupRoleenum distinct from the domain's, mapped explicitly inGroupEndpoints.Pagination is validated rather than clamped (400 naming the field for an out-of-range page or a page size over the cap of 50), consistent with ADR-0025.
ListGroupsRequest.Page/PageSizeare nullable withEffective*properties applying the defaults, rather than property initialisers —[AsParameters]binding did not reliably apply those for an omitted query parameter, caught by a failing integration test; see the type's own remarks and the ADR.Covered by
GroupRepositoryTests(query-level) andListGroupsEndpointTests(full HTTP round trip against a real Postgres, including a genuine pending-membership row, another user's group, personal group inclusion, pagination boundaries and the over-the-max page size case, and unauthenticated refusal).Verdict: mergeable
Verified: SQL scoping is genuinely in the
WHEREclause (both the page query and the count query), confirmed by mutating outgm.status = 'accepted'—GetGroups_APendingInvitation_IsNotInTheResponsereddens correctly. Another user's group is excluded by the separategm.user_idpredicate. Page-size cap (400 namingpageSize), page 0/negative (400 namingpage), and the[AsParameters]omitted-params path (200, defaults to page 1/size 20) all behave as claimed. TheGroupRolemapping is a total switch with no default arm — an added domain role fails the build. The endpoint carries noGroupCapabilityMetadataand its route (/api/groups) doesn't match the safety net's/api/groups/{groupId}prefix, so task 148's test correctly doesn't flag it. Full solution build and test suite pass clean on the pinned SDK after mergingmain(only conflict was the ADR index row, as expected).Two things worth fixing, neither blocking:
ADR-0043 and
ListGroupsRequest's remarks misdescribe the[AsParameters]failure mode. Both say a property initialiser "reads back as the CLR default (0)" for an omitted parameter. Reproduced the opposite: withPage/PageSizeas non-nullableintwith initialisers, omitting the query parameters entirely returns a 400 "The request could not be read" — the binder treats the non-nullable property as required, not defaulted-to-zero. The chosen fix (nullable +Effective*) is still correct and still necessary, but the write-up should describe the actual observed failure (hard 400) rather than a silent wrong value, since a future reader relying on "silently 0" to judge risk elsewhere would underestimate it.Untested edge cases: blank query values (
?page=or?pageSize=) and non-numeric values (?page=abc) both currently produce a generic 400 "The request could not be read" rather than the field-named validation error the other 400 cases give. Confirmed this is consistent (not a leak, not a 500), but it's an inconsistent error shape for the same class of client mistake and isn't covered by any test. Worth a test at minimum so the behaviour doesn't drift unnoticed; consider whether it's worth smoothing to match the named-field shape.Also: the branch needs the renumbering it already knows it needs —
docs/adr/0043-paginate-list-endpoints-and-put-group-role-on-the-wire.mdcollides withmain's0043-group-authorisation-as-an-endpoint-filter.mdand must become0044(file rename plus every cross-reference plus the README index row), andmainmerged in before this lands.Verdict: mergeable
Both findings from the previous review are actioned correctly.
intwith a property initialiser, omitted query param): hard 400,BadHttpRequestException, "required parameter ... was not provided from query string" — never a silent 0.ListGroupsRequest's remarks and ADR-0044 now match this exactly.ApiExceptionHandler.UnreadableRequestDetail, the app's own deliberate constant for a framework-refused request — not a guess at framework wording. Mutated that constant and confirmed all three redden, then reverted; not vacuous.mainmerge: ADR index has both 0043 and 0044 rows in order,git diff --statbetween the fix commit and the merge commit shows only PR #43's filter files and the README wording from #45 — nothing else rode in. Re-ranGroupScopedEndpointAuthorisationTestsat this head: passes, andGET /api/groups's route pattern is/api/groups(no{groupId}), so it correctly falls outside the safety net's/api/groups/{groupId}prefix check.Full build and test suite clean on the pinned SDK (10.0.100) at
ec30c66.