mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix: drop N+1 db query on template ACL available (#25465)
Fixes [PLAT-149](https://linear.app/codercom/issue/PLAT-149/template-permissions-search-is-extremely-slow-with-many-groups). `/acl/available` ran a db query per group. A deployment with >5,000 groups made this route extremely slow.
This commit is contained in:
@@ -3476,6 +3476,15 @@ func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg databas
|
|||||||
return memberCount, nil
|
return memberCount, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (q *querier) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) {
|
||||||
|
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceGroup); err != nil {
|
||||||
|
// Ideally we would check read access on each group ID, but that would be N queries.
|
||||||
|
// So this function is really only usable by admins.
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
return q.db.GetGroupMembersCountByGroupIDs(ctx, arg)
|
||||||
|
}
|
||||||
|
|
||||||
func (q *querier) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
func (q *querier) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
||||||
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err == nil {
|
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err == nil {
|
||||||
// Optimize this query for system users as it is used in telemetry.
|
// Optimize this query for system users as it is used in telemetry.
|
||||||
|
|||||||
@@ -1820,6 +1820,18 @@ func (s *MethodTestSuite) TestGroup() {
|
|||||||
check.Args(arg).Asserts(g, policy.ActionRead)
|
check.Args(arg).Asserts(g, policy.ActionRead)
|
||||||
}))
|
}))
|
||||||
|
|
||||||
|
s.Run("GetGroupMembersCountByGroupIDs", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||||
|
g1 := testutil.Fake(s.T(), faker, database.Group{})
|
||||||
|
g2 := testutil.Fake(s.T(), faker, database.Group{})
|
||||||
|
arg := database.GetGroupMembersCountByGroupIDsParams{GroupIds: []uuid.UUID{g1.ID, g2.ID}, IncludeSystem: false}
|
||||||
|
rows := []database.GetGroupMembersCountByGroupIDsRow{
|
||||||
|
{GroupID: g1.ID, MemberCount: 1},
|
||||||
|
{GroupID: g2.ID, MemberCount: 2},
|
||||||
|
}
|
||||||
|
dbm.EXPECT().GetGroupMembersCountByGroupIDs(gomock.Any(), arg).Return(rows, nil).AnyTimes()
|
||||||
|
check.Args(arg).Asserts(rbac.ResourceGroup, policy.ActionRead).Returns(rows)
|
||||||
|
}))
|
||||||
|
|
||||||
s.Run("GetGroupMembers", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
|
s.Run("GetGroupMembers", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
|
||||||
dbm.EXPECT().GetGroupMembers(gomock.Any(), false).Return([]database.GroupMember{}, nil).AnyTimes()
|
dbm.EXPECT().GetGroupMembers(gomock.Any(), false).Return([]database.GroupMember{}, nil).AnyTimes()
|
||||||
check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead)
|
check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead)
|
||||||
|
|||||||
@@ -1945,6 +1945,14 @@ func (m queryMetricsStore) GetGroupMembersCountByGroupID(ctx context.Context, ar
|
|||||||
return r0, r1
|
return r0, r1
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (m queryMetricsStore) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) {
|
||||||
|
start := time.Now()
|
||||||
|
r0, r1 := m.s.GetGroupMembersCountByGroupIDs(ctx, arg)
|
||||||
|
m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupIDs").Observe(time.Since(start).Seconds())
|
||||||
|
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetGroupMembersCountByGroupIDs").Inc()
|
||||||
|
return r0, r1
|
||||||
|
}
|
||||||
|
|
||||||
func (m queryMetricsStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
func (m queryMetricsStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
||||||
start := time.Now()
|
start := time.Now()
|
||||||
r0, r1 := m.s.GetGroups(ctx, arg)
|
r0, r1 := m.s.GetGroups(ctx, arg)
|
||||||
|
|||||||
@@ -3616,6 +3616,21 @@ func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(ctx, arg any) *go
|
|||||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), ctx, arg)
|
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), ctx, arg)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// GetGroupMembersCountByGroupIDs mocks base method.
|
||||||
|
func (m *MockStore) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) {
|
||||||
|
m.ctrl.T.Helper()
|
||||||
|
ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupIDs", ctx, arg)
|
||||||
|
ret0, _ := ret[0].([]database.GetGroupMembersCountByGroupIDsRow)
|
||||||
|
ret1, _ := ret[1].(error)
|
||||||
|
return ret0, ret1
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetGroupMembersCountByGroupIDs indicates an expected call of GetGroupMembersCountByGroupIDs.
|
||||||
|
func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupIDs(ctx, arg any) *gomock.Call {
|
||||||
|
mr.mock.ctrl.T.Helper()
|
||||||
|
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupIDs", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupIDs), ctx, arg)
|
||||||
|
}
|
||||||
|
|
||||||
// GetGroups mocks base method.
|
// GetGroups mocks base method.
|
||||||
func (m *MockStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
func (m *MockStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) {
|
||||||
m.ctrl.T.Helper()
|
m.ctrl.T.Helper()
|
||||||
|
|||||||
@@ -473,6 +473,12 @@ type sqlcQuerier interface {
|
|||||||
// count even if the caller does not have read access to ResourceGroupMember.
|
// count even if the caller does not have read access to ResourceGroupMember.
|
||||||
// They only need ResourceGroup read access.
|
// They only need ResourceGroup read access.
|
||||||
GetGroupMembersCountByGroupID(ctx context.Context, arg GetGroupMembersCountByGroupIDParams) (int64, error)
|
GetGroupMembersCountByGroupID(ctx context.Context, arg GetGroupMembersCountByGroupIDParams) (int64, error)
|
||||||
|
// Returns the total member count for each of the given group IDs in a
|
||||||
|
// single query. Used to avoid N+1 lookups when listing many groups. Like
|
||||||
|
// GetGroupMembersCountByGroupID, the count is returned even when the
|
||||||
|
// caller does not have read access to individual group members.
|
||||||
|
GetGroupMembersCountByGroupIDs(ctx context.Context, arg GetGroupMembersCountByGroupIDsParams) ([]GetGroupMembersCountByGroupIDsRow, error)
|
||||||
|
// A limit of 0 means "no limit".
|
||||||
GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error)
|
GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error)
|
||||||
GetHealthSettings(ctx context.Context) (string, error)
|
GetHealthSettings(ctx context.Context) (string, error)
|
||||||
GetInboxNotificationByID(ctx context.Context, id uuid.UUID) (InboxNotification, error)
|
GetInboxNotificationByID(ctx context.Context, id uuid.UUID) (InboxNotification, error)
|
||||||
|
|||||||
@@ -12689,6 +12689,56 @@ func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg GetG
|
|||||||
return count, err
|
return count, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const getGroupMembersCountByGroupIDs = `-- name: GetGroupMembersCountByGroupIDs :many
|
||||||
|
SELECT
|
||||||
|
group_id,
|
||||||
|
COUNT(*) AS member_count
|
||||||
|
FROM group_members_expanded
|
||||||
|
WHERE group_id = ANY($1 :: uuid[])
|
||||||
|
AND CASE
|
||||||
|
WHEN $2::bool THEN TRUE
|
||||||
|
ELSE user_is_system = false
|
||||||
|
END
|
||||||
|
GROUP BY group_id
|
||||||
|
`
|
||||||
|
|
||||||
|
type GetGroupMembersCountByGroupIDsParams struct {
|
||||||
|
GroupIds []uuid.UUID `db:"group_ids" json:"group_ids"`
|
||||||
|
IncludeSystem bool `db:"include_system" json:"include_system"`
|
||||||
|
}
|
||||||
|
|
||||||
|
type GetGroupMembersCountByGroupIDsRow struct {
|
||||||
|
GroupID uuid.UUID `db:"group_id" json:"group_id"`
|
||||||
|
MemberCount int64 `db:"member_count" json:"member_count"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// Returns the total member count for each of the given group IDs in a
|
||||||
|
// single query. Used to avoid N+1 lookups when listing many groups. Like
|
||||||
|
// GetGroupMembersCountByGroupID, the count is returned even when the
|
||||||
|
// caller does not have read access to individual group members.
|
||||||
|
func (q *sqlQuerier) GetGroupMembersCountByGroupIDs(ctx context.Context, arg GetGroupMembersCountByGroupIDsParams) ([]GetGroupMembersCountByGroupIDsRow, error) {
|
||||||
|
rows, err := q.db.QueryContext(ctx, getGroupMembersCountByGroupIDs, pq.Array(arg.GroupIds), arg.IncludeSystem)
|
||||||
|
if err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
defer rows.Close()
|
||||||
|
var items []GetGroupMembersCountByGroupIDsRow
|
||||||
|
for rows.Next() {
|
||||||
|
var i GetGroupMembersCountByGroupIDsRow
|
||||||
|
if err := rows.Scan(&i.GroupID, &i.MemberCount); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
items = append(items, i)
|
||||||
|
}
|
||||||
|
if err := rows.Close(); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
if err := rows.Err(); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
return items, nil
|
||||||
|
}
|
||||||
|
|
||||||
const insertGroupMember = `-- name: InsertGroupMember :exec
|
const insertGroupMember = `-- name: InsertGroupMember :exec
|
||||||
INSERT INTO
|
INSERT INTO
|
||||||
group_members (user_id, group_id)
|
group_members (user_id, group_id)
|
||||||
@@ -12906,6 +12956,14 @@ WHERE
|
|||||||
groups.id = ANY($4)
|
groups.id = ANY($4)
|
||||||
ELSE true
|
ELSE true
|
||||||
END
|
END
|
||||||
|
-- Filter by group name or display name (substring, case-insensitive).
|
||||||
|
AND CASE WHEN $5 :: text != '' THEN (
|
||||||
|
groups.name ILIKE concat('%', $5, '%')
|
||||||
|
OR groups.display_name ILIKE concat('%', $5, '%')
|
||||||
|
)
|
||||||
|
ELSE true
|
||||||
|
END
|
||||||
|
LIMIT NULLIF($6 :: int, 0)
|
||||||
`
|
`
|
||||||
|
|
||||||
type GetGroupsParams struct {
|
type GetGroupsParams struct {
|
||||||
@@ -12913,6 +12971,8 @@ type GetGroupsParams struct {
|
|||||||
HasMemberID uuid.UUID `db:"has_member_id" json:"has_member_id"`
|
HasMemberID uuid.UUID `db:"has_member_id" json:"has_member_id"`
|
||||||
GroupNames []string `db:"group_names" json:"group_names"`
|
GroupNames []string `db:"group_names" json:"group_names"`
|
||||||
GroupIds []uuid.UUID `db:"group_ids" json:"group_ids"`
|
GroupIds []uuid.UUID `db:"group_ids" json:"group_ids"`
|
||||||
|
Search string `db:"search" json:"search"`
|
||||||
|
LimitOpt int32 `db:"limit_opt" json:"limit_opt"`
|
||||||
}
|
}
|
||||||
|
|
||||||
type GetGroupsRow struct {
|
type GetGroupsRow struct {
|
||||||
@@ -12921,12 +12981,15 @@ type GetGroupsRow struct {
|
|||||||
OrganizationDisplayName string `db:"organization_display_name" json:"organization_display_name"`
|
OrganizationDisplayName string `db:"organization_display_name" json:"organization_display_name"`
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A limit of 0 means "no limit".
|
||||||
func (q *sqlQuerier) GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) {
|
func (q *sqlQuerier) GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) {
|
||||||
rows, err := q.db.QueryContext(ctx, getGroups,
|
rows, err := q.db.QueryContext(ctx, getGroups,
|
||||||
arg.OrganizationID,
|
arg.OrganizationID,
|
||||||
arg.HasMemberID,
|
arg.HasMemberID,
|
||||||
pq.Array(arg.GroupNames),
|
pq.Array(arg.GroupNames),
|
||||||
pq.Array(arg.GroupIds),
|
pq.Array(arg.GroupIds),
|
||||||
|
arg.Search,
|
||||||
|
arg.LimitOpt,
|
||||||
)
|
)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
|
|||||||
@@ -142,6 +142,22 @@ WHERE group_id = @group_id
|
|||||||
user_is_system = false
|
user_is_system = false
|
||||||
END;
|
END;
|
||||||
|
|
||||||
|
-- name: GetGroupMembersCountByGroupIDs :many
|
||||||
|
-- Returns the total member count for each of the given group IDs in a
|
||||||
|
-- single query. Used to avoid N+1 lookups when listing many groups. Like
|
||||||
|
-- GetGroupMembersCountByGroupID, the count is returned even when the
|
||||||
|
-- caller does not have read access to individual group members.
|
||||||
|
SELECT
|
||||||
|
group_id,
|
||||||
|
COUNT(*) AS member_count
|
||||||
|
FROM group_members_expanded
|
||||||
|
WHERE group_id = ANY(@group_ids :: uuid[])
|
||||||
|
AND CASE
|
||||||
|
WHEN @include_system::bool THEN TRUE
|
||||||
|
ELSE user_is_system = false
|
||||||
|
END
|
||||||
|
GROUP BY group_id;
|
||||||
|
|
||||||
-- InsertUserGroupsByID adds a user to all provided groups, if they exist.
|
-- InsertUserGroupsByID adds a user to all provided groups, if they exist.
|
||||||
-- name: InsertUserGroupsByID :many
|
-- name: InsertUserGroupsByID :many
|
||||||
WITH groups AS (
|
WITH groups AS (
|
||||||
|
|||||||
@@ -78,6 +78,15 @@ WHERE
|
|||||||
groups.id = ANY(@group_ids)
|
groups.id = ANY(@group_ids)
|
||||||
ELSE true
|
ELSE true
|
||||||
END
|
END
|
||||||
|
-- Filter by group name or display name (substring, case-insensitive).
|
||||||
|
AND CASE WHEN @search :: text != '' THEN (
|
||||||
|
groups.name ILIKE concat('%', @search, '%')
|
||||||
|
OR groups.display_name ILIKE concat('%', @search, '%')
|
||||||
|
)
|
||||||
|
ELSE true
|
||||||
|
END
|
||||||
|
-- A limit of 0 means "no limit".
|
||||||
|
LIMIT NULLIF(@limit_opt :: int, 0)
|
||||||
;
|
;
|
||||||
|
|
||||||
-- name: InsertGroup :one
|
-- name: InsertGroup :one
|
||||||
|
|||||||
+13
-3
@@ -375,9 +375,19 @@ func (c *Client) UpdateTemplateACL(ctx context.Context, templateID uuid.UUID, re
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// TemplateACLAvailable returns available users + groups that can be assigned template perms
|
// TemplateACLAvailable returns available users + groups that can be assigned
|
||||||
func (c *Client) TemplateACLAvailable(ctx context.Context, templateID uuid.UUID) (ACLAvailable, error) {
|
// template perms. The optional req controls the q/limit/offset query
|
||||||
res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/acl/available", templateID), nil)
|
// parameters applied server-side; pass codersdk.UsersRequest{} when no
|
||||||
|
// filtering is desired.
|
||||||
|
func (c *Client) TemplateACLAvailable(ctx context.Context, templateID uuid.UUID, req UsersRequest) (ACLAvailable, error) {
|
||||||
|
res, err := c.Request(
|
||||||
|
ctx,
|
||||||
|
http.MethodGet,
|
||||||
|
fmt.Sprintf("/api/v2/templates/%s/acl/available", templateID),
|
||||||
|
nil,
|
||||||
|
req.Pagination.asRequestOption(),
|
||||||
|
req.asRequestOption(),
|
||||||
|
)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return ACLAvailable{}, err
|
return ACLAvailable{}, err
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -35,7 +35,9 @@ func TestDynamicParametersOwnerGroups(t *testing.T) {
|
|||||||
_, noGroupUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
|
_, noGroupUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
|
||||||
|
|
||||||
// Create the group to be asserted
|
// Create the group to be asserted
|
||||||
group := coderdtest.CreateGroup(t, ownerClient, owner.OrganizationID, "bloob", templateAdminUser)
|
// Make the group name something after "Everyone" when sorted alphabetically.
|
||||||
|
// The test wants to check that `Everyone` is the default, which is the first alphabetical group in the test.
|
||||||
|
group := coderdtest.CreateGroup(t, ownerClient, owner.OrganizationID, "zebra", templateAdminUser)
|
||||||
|
|
||||||
dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/groups/main.tf")
|
dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/groups/main.tf")
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import (
|
|||||||
"golang.org/x/xerrors"
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
"cdr.dev/slog/v3"
|
"cdr.dev/slog/v3"
|
||||||
|
agpl "github.com/coder/coder/v2/coderd"
|
||||||
"github.com/coder/coder/v2/coderd/audit"
|
"github.com/coder/coder/v2/coderd/audit"
|
||||||
"github.com/coder/coder/v2/coderd/database"
|
"github.com/coder/coder/v2/coderd/database"
|
||||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||||
@@ -17,6 +18,7 @@ import (
|
|||||||
"github.com/coder/coder/v2/coderd/httpmw"
|
"github.com/coder/coder/v2/coderd/httpmw"
|
||||||
"github.com/coder/coder/v2/coderd/rbac/acl"
|
"github.com/coder/coder/v2/coderd/rbac/acl"
|
||||||
"github.com/coder/coder/v2/coderd/rbac/policy"
|
"github.com/coder/coder/v2/coderd/rbac/policy"
|
||||||
|
"github.com/coder/coder/v2/coderd/searchquery"
|
||||||
"github.com/coder/coder/v2/coderd/util/slice"
|
"github.com/coder/coder/v2/coderd/util/slice"
|
||||||
"github.com/coder/coder/v2/codersdk"
|
"github.com/coder/coder/v2/codersdk"
|
||||||
)
|
)
|
||||||
@@ -50,39 +52,63 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Apply the same q/limit semantics to groups as the users half of this response.
|
||||||
|
// The query semantics are defined for the users, which is awkward. But we can
|
||||||
|
// just reuse the search part of the query which is a fuzzy match.
|
||||||
|
userFilter, verr := searchquery.Users(r.URL.Query().Get("q"))
|
||||||
|
if len(verr) > 0 {
|
||||||
|
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||||
|
Message: "Invalid user search query.",
|
||||||
|
Validations: verr,
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
groupPagination, ok := agpl.ParsePagination(rw, r)
|
||||||
|
if !ok {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
// Perm check is the template update check.
|
// Perm check is the template update check.
|
||||||
// nolint:gocritic
|
// nolint:gocritic
|
||||||
groups, err := api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{
|
groups, err := api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{
|
||||||
OrganizationID: template.OrganizationID,
|
OrganizationID: template.OrganizationID,
|
||||||
|
Search: userFilter.Search,
|
||||||
|
// #nosec G115 - Pagination limits are small and fit in int32
|
||||||
|
LimitOpt: int32(groupPagination.Limit),
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
httpapi.InternalServerError(rw, err)
|
httpapi.InternalServerError(rw, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Fetch member counts for all groups in a single query to avoid an
|
||||||
|
// N+1 lookup pattern that was making this endpoint extremely slow on
|
||||||
|
// deployments with many groups. The per-group member lists are
|
||||||
|
// intentionally not populated here: callers of this endpoint only
|
||||||
|
// surface total_member_count (see Group.TotalMemberCount, which is
|
||||||
|
// already documented as the canonical value).
|
||||||
|
groupIDs := make([]uuid.UUID, len(groups))
|
||||||
|
for i, g := range groups {
|
||||||
|
groupIDs[i] = g.Group.ID
|
||||||
|
}
|
||||||
|
|
||||||
|
// nolint:gocritic // Same justification as the GetGroups call 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
|
||||||
|
}
|
||||||
|
|
||||||
sdkGroups := make([]codersdk.Group, 0, len(groups))
|
sdkGroups := make([]codersdk.Group, 0, len(groups))
|
||||||
for _, group := range groups {
|
for _, group := range groups {
|
||||||
// nolint:gocritic
|
sdkGroups = append(sdkGroups, db2sdk.Group(group, nil, int(countByGroup[group.Group.ID])))
|
||||||
members, err := api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{
|
|
||||||
GroupID: group.Group.ID,
|
|
||||||
IncludeSystem: false,
|
|
||||||
})
|
|
||||||
if err != nil {
|
|
||||||
httpapi.InternalServerError(rw, err)
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
// nolint:gocritic
|
|
||||||
memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDParams{
|
|
||||||
GroupID: group.Group.ID,
|
|
||||||
IncludeSystem: false,
|
|
||||||
})
|
|
||||||
if err != nil {
|
|
||||||
httpapi.InternalServerError(rw, err)
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
sdkGroups = append(sdkGroups, db2sdk.Group(group, members, int(memberCount)))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
httpapi.Write(ctx, rw, http.StatusOK, codersdk.ACLAvailable{
|
httpapi.Write(ctx, rw, http.StatusOK, codersdk.ACLAvailable{
|
||||||
|
|||||||
@@ -1241,6 +1241,119 @@ func TestTemplateACL(t *testing.T) {
|
|||||||
})
|
})
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
// Regression test for PLAT-149. Previously this endpoint did an N+1
|
||||||
|
// fetch of every group's members and member count. Verify that the
|
||||||
|
// member count is returned correctly for many groups, and that the
|
||||||
|
// per-group members list is no longer populated (callers should rely
|
||||||
|
// on TotalMemberCount).
|
||||||
|
t.Run("AvailableReturnsGroupMemberCounts", func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{
|
||||||
|
Features: license.Features{
|
||||||
|
codersdk.FeatureTemplateRBAC: 1,
|
||||||
|
},
|
||||||
|
}})
|
||||||
|
admin, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin())
|
||||||
|
|
||||||
|
// Create a couple of users we can stuff into groups.
|
||||||
|
_, alice := coderdtest.CreateAnotherUser(t, client, user.OrganizationID)
|
||||||
|
_, bob := coderdtest.CreateAnotherUser(t, client, user.OrganizationID)
|
||||||
|
_, carol := coderdtest.CreateAnotherUser(t, client, user.OrganizationID)
|
||||||
|
|
||||||
|
// emptyGroup: zero non-system members.
|
||||||
|
// singleGroup: alice only.
|
||||||
|
// fullGroup: alice + bob + carol.
|
||||||
|
emptyGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "empty-group")
|
||||||
|
singleGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "single-group", alice)
|
||||||
|
fullGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "full-group", alice, bob, carol)
|
||||||
|
|
||||||
|
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
|
||||||
|
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
|
||||||
|
|
||||||
|
ctx := testutil.Context(t, testutil.WaitLong)
|
||||||
|
|
||||||
|
available, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
wantCounts := map[uuid.UUID]int{
|
||||||
|
emptyGroup.ID: 0,
|
||||||
|
singleGroup.ID: 1,
|
||||||
|
fullGroup.ID: 3,
|
||||||
|
}
|
||||||
|
|
||||||
|
found := map[uuid.UUID]bool{}
|
||||||
|
for _, group := range available.Groups {
|
||||||
|
if want, ok := wantCounts[group.ID]; ok {
|
||||||
|
found[group.ID] = true
|
||||||
|
require.Equal(t, want, group.TotalMemberCount,
|
||||||
|
"unexpected total_member_count for group %q", group.Name)
|
||||||
|
require.Empty(t, group.Members,
|
||||||
|
"members must not be populated by the available endpoint for group %q", group.Name)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
for id := range wantCounts {
|
||||||
|
require.True(t, found[id], "group %s missing from available response", id)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
// Companion to the AvailableReturnsGroupMemberCounts test above. Verifies
|
||||||
|
// that the q query parameter applies a server-side substring filter on
|
||||||
|
// group name / display_name, and that limit caps the number of groups
|
||||||
|
// returned. The autocomplete sends both on each keystroke; before
|
||||||
|
// PLAT-149 both were ignored for groups.
|
||||||
|
t.Run("AvailableHonorsGroupSearchAndLimit", func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{
|
||||||
|
Features: license.Features{
|
||||||
|
codersdk.FeatureTemplateRBAC: 1,
|
||||||
|
},
|
||||||
|
}})
|
||||||
|
admin, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin())
|
||||||
|
|
||||||
|
// Create a handful of groups with predictable names so we can
|
||||||
|
// pin assertions to specific substrings.
|
||||||
|
engAlpha := coderdtest.CreateGroup(t, admin, user.OrganizationID, "engineering-alpha")
|
||||||
|
engBeta := coderdtest.CreateGroup(t, admin, user.OrganizationID, "engineering-beta")
|
||||||
|
design := coderdtest.CreateGroup(t, admin, user.OrganizationID, "design")
|
||||||
|
sales := coderdtest.CreateGroup(t, admin, user.OrganizationID, "sales")
|
||||||
|
|
||||||
|
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
|
||||||
|
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
|
||||||
|
|
||||||
|
ctx := testutil.Context(t, testutil.WaitLong)
|
||||||
|
|
||||||
|
groupIDs := func(available codersdk.ACLAvailable) []uuid.UUID {
|
||||||
|
ids := make([]uuid.UUID, 0, len(available.Groups))
|
||||||
|
for _, g := range available.Groups {
|
||||||
|
ids = append(ids, g.ID)
|
||||||
|
}
|
||||||
|
return ids
|
||||||
|
}
|
||||||
|
|
||||||
|
// q filters by group name / display_name substring.
|
||||||
|
filtered, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{
|
||||||
|
SearchQuery: "engineering",
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
got := groupIDs(filtered)
|
||||||
|
require.ElementsMatch(t, []uuid.UUID{engAlpha.ID, engBeta.ID}, got,
|
||||||
|
"q=engineering should return only engineering-* groups, got %v", got)
|
||||||
|
require.NotContains(t, got, design.ID)
|
||||||
|
require.NotContains(t, got, sales.ID)
|
||||||
|
|
||||||
|
// limit caps the number of groups returned. With 4 user-created
|
||||||
|
// groups plus the implicit Everyone group, asking for 2 must
|
||||||
|
// return at most 2 groups.
|
||||||
|
limited, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{
|
||||||
|
Pagination: codersdk.Pagination{Limit: 2},
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.Len(t, limited.Groups, 2,
|
||||||
|
"limit=2 should cap groups to 2, got %d", len(limited.Groups))
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestUpdateTemplateACL(t *testing.T) {
|
func TestUpdateTemplateACL(t *testing.T) {
|
||||||
@@ -1626,7 +1739,7 @@ func TestUpdateTemplateACL(t *testing.T) {
|
|||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
||||||
// Should be able to see user 3
|
// Should be able to see user 3
|
||||||
available, err := client2.TemplateACLAvailable(ctx, template.ID)
|
available, err := client2.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{})
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
userFound := false
|
userFound := false
|
||||||
for _, avail := range available.Users {
|
for _, avail := range available.Users {
|
||||||
|
|||||||
Reference in New Issue
Block a user