Skip to content

Commit e49c917

Browse files
ethanndicksonethanndickson
authored
perf: use a single query for notification target lookups (#20574)
Somewhat minor inefficiency in notifications I discovered during a scaletest where I was creating many users. Our `GetUsers` query filter for rbac roles uses the `&&` operator on arrays, which is the intersection of the two arrays. Despite that, we were making seperate DB queries for each role, and then collating the results. I didn't see any other instances of this. The test changes are required as the order of outgoing notifications is now non-deterministic.
1 parent 903c045 commit e49c917

File tree

3 files changed

+45
-45
lines changed

3 files changed

+45
-45
lines changed

‎coderd/templates.go‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,19 +1130,11 @@ func (api *API) convertTemplate(
11301130

11311131
// findTemplateAdmins fetches all users with template admin permission including owners.
11321132
func findTemplateAdmins(ctx context.Context, store database.Store) ([]database.GetUsersRow, error) {
1133-
// Notice: we can't scrape the user information in parallel as pq
1134-
// fails with: unexpected describe rows response: 'D'
1135-
owners, err := store.GetUsers(ctx, database.GetUsersParams{
1136-
RbacRole: []string{codersdk.RoleOwner},
1137-
})
1138-
if err != nil {
1139-
return nil, xerrors.Errorf("get owners: %w", err)
1140-
}
11411133
templateAdmins, err := store.GetUsers(ctx, database.GetUsersParams{
1142-
RbacRole: []string{codersdk.RoleTemplateAdmin},
1134+
RbacRole: []string{codersdk.RoleTemplateAdmin, codersdk.RoleOwner},
11431135
})
11441136
if err != nil {
1145-
return nil, xerrors.Errorf("get template admins: %w", err)
1137+
return nil, xerrors.Errorf("get owners: %w", err)
11461138
}
1147-
return append(owners, templateAdmins...), nil
1139+
return templateAdmins, nil
11481140
}

‎coderd/users.go‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1537,21 +1537,13 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create
15371537

15381538
// findUserAdmins fetches all users with user admin permission including owners.
15391539
func findUserAdmins(ctx context.Context, store database.Store) ([]database.GetUsersRow, error) {
1540-
// Notice: we can't scrape the user information in parallel as pq
1541-
// fails with: unexpected describe rows response: 'D'
1542-
owners, err := store.GetUsers(ctx, database.GetUsersParams{
1543-
RbacRole: []string{codersdk.RoleOwner},
1544-
})
1545-
if err != nil {
1546-
return nil, xerrors.Errorf("get owners: %w", err)
1547-
}
15481540
userAdmins, err := store.GetUsers(ctx, database.GetUsersParams{
1549-
RbacRole: []string{codersdk.RoleUserAdmin},
1541+
RbacRole: []string{codersdk.RoleOwner, codersdk.RoleUserAdmin},
15501542
})
15511543
if err != nil {
1552-
return nil, xerrors.Errorf("get user admins: %w", err)
1544+
return nil, xerrors.Errorf("get owners: %w", err)
15531545
}
1554-
return append(owners, userAdmins...), nil
1546+
return userAdmins, nil
15551547
}
15561548

15571549
func convertUsers(users []database.User, organizationIDsByUserID map[uuid.UUID][]uuid.UUID) []codersdk.User {

‎coderd/users_test.go‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -599,21 +599,28 @@ func TestNotifyDeletedUser(t *testing.T) {
599599
// then
600600
sent := notifyEnq.Sent()
601601
require.Len(t, sent, 5)
602-
// sent[0]: "User admin" account created, "owner" notified
603-
// sent[1]: "Member" account created, "owner" notified
604-
// sent[2]: "Member" account created, "user admin" notified
602+
// Other notifications:
603+
// "User admin" account created, "owner" notified
604+
// "Member" account created, "owner" notified
605+
// "Member" account created, "user admin" notified
605606

606607
// "Member" account deleted, "owner" notified
607-
require.Equal(t, notifications.TemplateUserAccountDeleted, sent[3].TemplateID)
608-
require.Equal(t, firstUser.UserID, sent[3].UserID)
609-
require.Contains(t, sent[3].Targets, member.ID)
610-
require.Equal(t, member.Username, sent[3].Labels["deleted_account_name"])
608+
ownerNotifications := notifyEnq.Sent(func(n *notificationstest.FakeNotification) bool {
609+
return n.TemplateID == notifications.TemplateUserAccountDeleted &&
610+
n.UserID == firstUser.UserID &&
611+
slices.Contains(n.Targets, member.ID) &&
612+
n.Labels["deleted_account_name"] == member.Username
613+
})
614+
require.Len(t, ownerNotifications, 1)
611615

612616
// "Member" account deleted, "user admin" notified
613-
require.Equal(t, notifications.TemplateUserAccountDeleted, sent[4].TemplateID)
614-
require.Equal(t, userAdmin.ID, sent[4].UserID)
615-
require.Contains(t, sent[4].Targets, member.ID)
616-
require.Equal(t, member.Username, sent[4].Labels["deleted_account_name"])
617+
adminNotifications := notifyEnq.Sent(func(n *notificationstest.FakeNotification) bool {
618+
return n.TemplateID == notifications.TemplateUserAccountDeleted &&
619+
n.UserID == userAdmin.ID &&
620+
slices.Contains(n.Targets, member.ID) &&
621+
n.Labels["deleted_account_name"] == member.Username
622+
})
623+
require.Len(t, adminNotifications, 1)
617624
})
618625
}
619626

@@ -960,22 +967,31 @@ func TestNotifyCreatedUser(t *testing.T) {
960967
require.Len(t, sent, 3)
961968

962969
// "User admin" account created, "owner" notified
963-
require.Equal(t, notifications.TemplateUserAccountCreated, sent[0].TemplateID)
964-
require.Equal(t, firstUser.UserID, sent[0].UserID)
965-
require.Contains(t, sent[0].Targets, userAdmin.ID)
966-
require.Equal(t, userAdmin.Username, sent[0].Labels["created_account_name"])
970+
ownerNotifiedAboutUserAdmin := notifyEnq.Sent(func(n *notificationstest.FakeNotification) bool {
971+
return n.TemplateID == notifications.TemplateUserAccountCreated &&
972+
n.UserID == firstUser.UserID &&
973+
slices.Contains(n.Targets, userAdmin.ID) &&
974+
n.Labels["created_account_name"] == userAdmin.Username
975+
})
976+
require.Len(t, ownerNotifiedAboutUserAdmin, 1)
967977

968978
// "Member" account created, "owner" notified
969-
require.Equal(t, notifications.TemplateUserAccountCreated, sent[1].TemplateID)
970-
require.Equal(t, firstUser.UserID, sent[1].UserID)
971-
require.Contains(t, sent[1].Targets, member.ID)
972-
require.Equal(t, member.Username, sent[1].Labels["created_account_name"])
979+
ownerNotifiedAboutMember := notifyEnq.Sent(func(n *notificationstest.FakeNotification) bool {
980+
return n.TemplateID == notifications.TemplateUserAccountCreated &&
981+
n.UserID == firstUser.UserID &&
982+
slices.Contains(n.Targets, member.ID) &&
983+
n.Labels["created_account_name"] == member.Username
984+
})
985+
require.Len(t, ownerNotifiedAboutMember, 1)
973986

974987
// "Member" account created, "user admin" notified
975-
require.Equal(t, notifications.TemplateUserAccountCreated, sent[1].TemplateID)
976-
require.Equal(t, userAdmin.ID, sent[2].UserID)
977-
require.Contains(t, sent[2].Targets, member.ID)
978-
require.Equal(t, member.Username, sent[2].Labels["created_account_name"])
988+
userAdminNotifiedAboutMember := notifyEnq.Sent(func(n *notificationstest.FakeNotification) bool {
989+
return n.TemplateID == notifications.TemplateUserAccountCreated &&
990+
n.UserID == userAdmin.ID &&
991+
slices.Contains(n.Targets, member.ID) &&
992+
n.Labels["created_account_name"] == member.Username
993+
})
994+
require.Len(t, userAdminNotifiedAboutMember, 1)
979995
})
980996
}
981997

0 commit comments

Comments
 (0)