mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix(enterprise/coderd): drop N+1 db queries on groups list endpoint
The groups list handler (GET /api/v2/groups and
GET /api/v2/organizations/{org}/groups) issued per-group
GetGroupMembersByGroupID and GetGroupMembersCountByGroupID queries,
producing 3N+1 database round-trips. Deployments with many groups saw
multi-second or timing-out responses on the Groups page and any page
that loads group data (Users, Org Members).
Replace the per-group loop with a single GetGroupMembersCountByGroupIDs
batch query and pass nil members to db2sdk.Group. The dedicated
paginated /groups/{group}/members endpoint already serves member detail.
This commit is contained in:
+28
-18
@@ -13,6 +13,7 @@ import (
|
||||
agpl "github.com/coder/coder/v2/coderd"
|
||||
"github.com/coder/coder/v2/coderd/audit"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw"
|
||||
@@ -607,26 +608,35 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
// Fetch member counts for all groups in a single query to avoid an
|
||||
// N+1 lookup pattern that makes this endpoint extremely slow on
|
||||
// deployments with many groups.
|
||||
groupIDs := make([]uuid.UUID, len(groups))
|
||||
for i, g := range groups {
|
||||
groupIDs[i] = g.Group.ID
|
||||
}
|
||||
|
||||
// The groups returned above are already authorized via GetGroups
|
||||
// (which uses fetchWithPostFilter). The batch count query requires
|
||||
// system-level read on ResourceGroup, so elevate to system context
|
||||
// the same way /acl/available does.
|
||||
// nolint:gocritic // Auth check already happened in GetGroups above.
|
||||
countRows, err := api.Database.GetGroupMembersCountByGroupIDs(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDsParams{
|
||||
GroupIds: groupIDs,
|
||||
IncludeSystem: false,
|
||||
})
|
||||
if err != nil {
|
||||
httpapi.InternalServerError(rw, err)
|
||||
return
|
||||
}
|
||||
countByGroup := make(map[uuid.UUID]int64, len(countRows))
|
||||
for _, row := range countRows {
|
||||
countByGroup[row.GroupID] = row.MemberCount
|
||||
}
|
||||
|
||||
resp := make([]codersdk.Group, 0, len(groups))
|
||||
for _, group := range groups {
|
||||
members, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
|
||||
GroupID: group.Group.ID,
|
||||
IncludeSystem: false,
|
||||
})
|
||||
if err != nil {
|
||||
httpapi.InternalServerError(rw, err)
|
||||
return
|
||||
}
|
||||
memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{
|
||||
GroupID: group.Group.ID,
|
||||
IncludeSystem: false,
|
||||
})
|
||||
if err != nil {
|
||||
httpapi.InternalServerError(rw, err)
|
||||
return
|
||||
}
|
||||
|
||||
resp = append(resp, db2sdk.Group(group, members, int(memberCount)))
|
||||
resp = append(resp, db2sdk.Group(group, nil, int(countByGroup[group.Group.ID])))
|
||||
}
|
||||
|
||||
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
||||
|
||||
@@ -1013,54 +1013,64 @@ func TestGroups(t *testing.T) {
|
||||
Organization: user.OrganizationID.String(),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
normalizeAllGroups(groups)
|
||||
|
||||
// 'Everyone' group + 2 custom groups.
|
||||
require.ElementsMatch(t, []codersdk.Group{
|
||||
everyoneGroup,
|
||||
group1,
|
||||
group2,
|
||||
}, groups)
|
||||
// The list endpoint no longer populates Members (use the
|
||||
// dedicated /groups/{group}/members endpoint instead), so
|
||||
// compare by ID and verify TotalMemberCount.
|
||||
groupIDs := slice.List(groups, func(g codersdk.Group) uuid.UUID {
|
||||
return g.ID
|
||||
})
|
||||
require.ElementsMatch(t, []uuid.UUID{
|
||||
everyoneGroup.ID,
|
||||
group1.ID,
|
||||
group2.ID,
|
||||
}, groupIDs)
|
||||
for _, g := range groups {
|
||||
require.Empty(t, g.Members, "list endpoint should not populate members")
|
||||
switch g.ID {
|
||||
case group1.ID:
|
||||
require.Equal(t, 2, g.TotalMemberCount)
|
||||
case group2.ID:
|
||||
require.Equal(t, 2, g.TotalMemberCount)
|
||||
default:
|
||||
// Everyone group includes all 6 users (owner + userAdmin + user2-5).
|
||||
require.Equal(t, 6, g.TotalMemberCount)
|
||||
}
|
||||
}
|
||||
|
||||
// Filter by user
|
||||
user5Groups, err := userAdminClient.Groups(ctx, codersdk.GroupArguments{
|
||||
HasMember: user5.Username,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
normalizeAllGroups(user5Groups)
|
||||
user5GroupIDs := slice.List(user5Groups, func(g codersdk.Group) uuid.UUID {
|
||||
return g.ID
|
||||
})
|
||||
// Everyone group and group 2
|
||||
require.ElementsMatch(t, []codersdk.Group{
|
||||
everyoneGroup,
|
||||
group2,
|
||||
}, user5Groups)
|
||||
require.ElementsMatch(t, []uuid.UUID{
|
||||
everyoneGroup.ID,
|
||||
group2.ID,
|
||||
}, user5GroupIDs)
|
||||
|
||||
// Query from the user's perspective
|
||||
user5View, err := user5Client.Groups(ctx, codersdk.GroupArguments{})
|
||||
require.NoError(t, err)
|
||||
normalizeAllGroups(user5View)
|
||||
|
||||
// Org members can read all groups when workspace sharing is not
|
||||
// disabled, but group membership is limited to the requesting user.
|
||||
// disabled.
|
||||
// TODO(geokat): add another test with workspace sharing disabled.
|
||||
require.Len(t, user5View, 3)
|
||||
user5ViewIDs := slice.List(user5View, func(g codersdk.Group) uuid.UUID {
|
||||
return g.ID
|
||||
})
|
||||
|
||||
require.ElementsMatch(t, []uuid.UUID{
|
||||
everyoneGroup.ID,
|
||||
group1.ID,
|
||||
group2.ID,
|
||||
}, user5ViewIDs)
|
||||
// Members are no longer populated in the list response.
|
||||
for _, g := range user5View {
|
||||
if g.ID == everyoneGroup.ID || g.ID == group2.ID {
|
||||
// Only expect the 1 member, themselves.
|
||||
require.Len(t, g.Members, 1)
|
||||
require.Equal(t, user5.ReducedUser.ID, g.Members[0].MinimalUser.ID)
|
||||
continue
|
||||
}
|
||||
|
||||
require.Empty(t, g.Members)
|
||||
require.Empty(t, g.Members, "list endpoint should not populate members")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user