From ca579cea4f4fa6d6719df50200b66af66d32919e Mon Sep 17 00:00:00 2001 From: Nicholas Spangler Date: Mon, 1 Jun 2026 16:04:12 +0000 Subject: [PATCH] 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. --- enterprise/coderd/groups.go | 46 ++++++++++++++++---------- enterprise/coderd/groups_test.go | 56 +++++++++++++++++++------------- 2 files changed, 61 insertions(+), 41 deletions(-) diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 95b238f41a..dd7b9e407e 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -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) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 59335e91c5..ffe8ce2749 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -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") } }) }