From dfab59ba378c75a67dbec578f7ef35c6226b3c93 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Thu, 2 Jul 2026 19:19:16 -0400 Subject: [PATCH] Replace N+1 --- internal/core/repeaters.go | 18 +++---- internal/store/config_profiles.go | 47 ++++++++++++++++++ internal/store/org_profile_names_test.go | 63 ++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 internal/store/org_profile_names_test.go diff --git a/internal/core/repeaters.go b/internal/core/repeaters.go index f5c29c0..9fe84fd 100644 --- a/internal/core/repeaters.go +++ b/internal/core/repeaters.go @@ -69,23 +69,17 @@ func (s *Handlers) pageAddRepeater(w http.ResponseWriter, r *http.Request) { } // setupOrgOptions lists the current user's orgs with their config profile names, -// for the serial-setup config selector. Best-effort: an org whose profiles fail -// to load is included with none. +// for the serial-setup config selector, in a single query. Best-effort: on a +// store error it returns nil (the selector renders with no orgs). func (s *Handlers) setupOrgOptions(r *http.Request) []setupOrgOption { uid := s.Auth.CurrentUserID(r.Context()) - memberships, err := s.Store.ListOrgsForUser(r.Context(), uid) + orgs, err := s.Store.ListOrgProfileNamesForUser(r.Context(), uid) if err != nil { return nil } - out := make([]setupOrgOption, 0, len(memberships)) - for _, m := range memberships { - opt := setupOrgOption{ID: m.Org.ID, Name: m.Org.Name} - if profiles, err := s.Store.ListProfiles(r.Context(), m.Org.ID); err == nil { - for _, p := range profiles { - opt.Profiles = append(opt.Profiles, p.Name) - } - } - out = append(out, opt) + out := make([]setupOrgOption, 0, len(orgs)) + for _, o := range orgs { + out = append(out, setupOrgOption{ID: o.OrgID, Name: o.OrgName, Profiles: o.Profiles}) } return out } diff --git a/internal/store/config_profiles.go b/internal/store/config_profiles.go index 459d6d6..f0f65c2 100644 --- a/internal/store/config_profiles.go +++ b/internal/store/config_profiles.go @@ -126,6 +126,53 @@ func (s *Store) ListProfiles(ctx context.Context, orgID int64) ([]Profile, error return profiles, srows.Err() } +// OrgProfileNames is an org paired with just the names of its config profiles, +// for pickers that need the names but not the full step bodies. +type OrgProfileNames struct { + OrgID int64 + OrgName string + Profiles []string +} + +// ListOrgProfileNamesForUser returns every org the user belongs to together with +// that org's config-profile names, in a single query (avoids the per-org N+1 of +// calling ListProfiles in a loop). Orgs with no profiles are included with an +// empty Profiles slice. Rows are ordered to match ListOrgsForUser (org name, id) +// then profile display order (position, name), and are grouped by org here. +func (s *Store) ListOrgProfileNamesForUser(ctx context.Context, userID int64) ([]OrgProfileNames, error) { + rows, err := s.pool.Query(ctx, ` + SELECT o.id, o.name, p.name + FROM org_members m + JOIN organizations o ON o.id = m.org_id + LEFT JOIN config_profiles p ON p.org_id = o.id + WHERE m.user_id = $1 + ORDER BY lower(o.name), o.id, p.position, p.name`, userID) + if err != nil { + return nil, fmt.Errorf("list org profile names: %w", err) + } + defer rows.Close() + + // Rows for one org are contiguous (ordered by org first), so we can group by + // index without a map — and index-based appends never dangle a pointer. + var out []OrgProfileNames + for rows.Next() { + var orgID int64 + var orgName string + var profileName *string // NULL for an org with no profiles (LEFT JOIN) + if err := rows.Scan(&orgID, &orgName, &profileName); err != nil { + return nil, fmt.Errorf("scan org profile name: %w", err) + } + if len(out) == 0 || out[len(out)-1].OrgID != orgID { + out = append(out, OrgProfileNames{OrgID: orgID, OrgName: orgName}) + } + if profileName != nil { + last := &out[len(out)-1] + last.Profiles = append(last.Profiles, *profileName) + } + } + return out, rows.Err() +} + // ListRegions returns an org's regions ordered (layer, token) — i.e. root to leaf, // the order their tokens appear in a `region def` chain. func (s *Store) ListRegions(ctx context.Context, orgID int64) ([]Region, error) { diff --git a/internal/store/org_profile_names_test.go b/internal/store/org_profile_names_test.go new file mode 100644 index 0000000..0ea198e --- /dev/null +++ b/internal/store/org_profile_names_test.go @@ -0,0 +1,63 @@ +package store + +import "testing" + +// TestListOrgProfileNamesForUser covers the single-query grouping that replaced +// the per-org N+1 in the serial-setup org selector: every org the user belongs to +// is returned once, ordered by org name, each carrying its profile names in +// display order; an org with no profiles still appears (LEFT JOIN) with none. +func TestListOrgProfileNamesForUser(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + owner, err := st.CreateUser(ctx, "npowner", "") + if err != nil { + t.Fatal(err) + } + // Two orgs, deliberately created out of alphabetical order to prove ordering. + beta, err := st.CreateOrg(ctx, "Beta", owner.ID) + if err != nil { + t.Fatal(err) + } + alpha, err := st.CreateOrg(ctx, "Alpha", owner.ID) + if err != nil { + t.Fatal(err) + } + // A third org the user is NOT a member of must never appear. + stranger, err := st.CreateUser(ctx, "stranger", "") + if err != nil { + t.Fatal(err) + } + if _, err := st.CreateOrg(ctx, "Hidden", stranger.ID); err != nil { + t.Fatal(err) + } + + // Alpha gets two profiles (created out of order to prove position ordering); + // Beta gets none. + if err := st.ReplaceOrgConfig(ctx, alpha.ID, []ProfileInput{ + {Name: "ESP32"}, + {Name: "nRF52"}, + }, nil); err != nil { + t.Fatal(err) + } + + got, err := st.ListOrgProfileNamesForUser(ctx, owner.ID) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(got) != 2 { + t.Fatalf("got %d orgs, want 2: %+v", len(got), got) + } + // Ordered by lower(name): Alpha before Beta. + if got[0].OrgID != alpha.ID || got[0].OrgName != "Alpha" { + t.Fatalf("first org = %+v, want Alpha", got[0]) + } + if len(got[0].Profiles) != 2 || got[0].Profiles[0] != "ESP32" || got[0].Profiles[1] != "nRF52" { + t.Fatalf("Alpha profiles = %v, want [ESP32 nRF52]", got[0].Profiles) + } + if got[1].OrgID != beta.ID || got[1].OrgName != "Beta" { + t.Fatalf("second org = %+v, want Beta", got[1]) + } + if len(got[1].Profiles) != 0 { + t.Fatalf("Beta profiles = %v, want none", got[1].Profiles) + } +}