From be5048c63b8049dfb57cab40b4187167e2eac62f Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Tue, 18 Nov 2025 12:17:43 -0600 Subject: [PATCH] Make host counts optional in "list labels" API (#35831) **Related issue:** Resolves #35376 # Details This PR updates the "list labels" (`GET /labels`) API by adding an optional `include_host_counts` parameter, which defaults to `true`. If explicitly set to `false`, the underlying db code will skip doing an expensive subquery which returns the number of hosts that are members of each label. The UI will now default to setting this to `false` in its calls, because: 1. This is an N+1 query pattern which scales poorly as the # of labels and hosts increases (see associated ticket as well as https://github.com/fleetdm/fleet/issues/4890) 1. _We don't use this data anywhere._ At least no where I could find in the front end or back end (besides a test specifically for this functionality). So we're doing this work for nothing. Since this is a public API we can't just [drop the functionality entirely](https://github.com/fleetdm/fleet/pull/35763) as that would be a breaking change. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [X] QA'd all new/changed functionality manually The only place that I could find that lists host counts for labels is the Packs UI, which uses a different endpoint and database method (`GET /targets` and `SearchLabels()` --- changes/35763-change-host-counts-default | 1 + frontend/services/entities/labels.ts | 13 +++++++--- server/fleet/service.go | 2 +- server/mock/service/service_mock.go | 6 ++--- server/service/labels.go | 20 ++++++++++++--- server/service/labels_test.go | 31 ++++++++++++++++++++++-- 6 files changed, 60 insertions(+), 13 deletions(-) create mode 100644 changes/35763-change-host-counts-default diff --git a/changes/35763-change-host-counts-default b/changes/35763-change-host-counts-default new file mode 100644 index 0000000000..40dea671cc --- /dev/null +++ b/changes/35763-change-host-counts-default @@ -0,0 +1 @@ +- Made returning host_count optional in the "List Labels" API, to improve performance. \ No newline at end of file diff --git a/frontend/services/entities/labels.ts b/frontend/services/entities/labels.ts index 6799601305..0d7ef536f4 100644 --- a/frontend/services/entities/labels.ts +++ b/frontend/services/entities/labels.ts @@ -7,6 +7,7 @@ import { IDynamicLabelFormData } from "pages/labels/components/DynamicLabelForm/ import { IManualLabelFormData } from "pages/labels/components/ManualLabelForm/ManualLabelForm"; import { IHost } from "interfaces/host"; import { INewLabelFormData } from "pages/labels/NewLabelPage/NewLabelPage"; +import { buildQueryStringFromParams } from "utilities/url"; export interface ILabelsResponse { labels: ILabel[]; @@ -109,12 +110,18 @@ export default { return sendRequest("DELETE", path); }, - // TODO: confirm this still works - loadAll: async (): Promise => { + loadAll: async (includeHostCounts = false): Promise => { const { LABELS } = endpoints; + const queryStringParams = { + include_host_counts: includeHostCounts, + }; + + const queryString = buildQueryStringFromParams(queryStringParams); + const path = `${LABELS}?${queryString}`; + try { - const response = await sendRequest("GET", LABELS); + const response = await sendRequest("GET", path); return Promise.resolve({ labels: helpers.formatLabelResponse(response) }); } catch (error) { console.error(error); diff --git a/server/fleet/service.go b/server/fleet/service.go index 1d4aecf562..5fb511b5c1 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -261,7 +261,7 @@ type Service interface { NewLabel(ctx context.Context, p LabelPayload) (label *Label, hostIDs []uint, err error) ModifyLabel(ctx context.Context, id uint, payload ModifyLabelPayload) (*Label, []uint, error) - ListLabels(ctx context.Context, opt ListOptions) (labels []*Label, err error) + ListLabels(ctx context.Context, opt ListOptions, includeHostCounts bool) (labels []*Label, err error) LabelsSummary(ctx context.Context) (labels []*LabelSummary, err error) GetLabel(ctx context.Context, id uint) (label *Label, hostIDs []uint, err error) diff --git a/server/mock/service/service_mock.go b/server/mock/service/service_mock.go index fb1bb1fb5e..c84203dded 100644 --- a/server/mock/service/service_mock.go +++ b/server/mock/service/service_mock.go @@ -148,7 +148,7 @@ type NewLabelFunc func(ctx context.Context, p fleet.LabelPayload) (label *fleet. type ModifyLabelFunc func(ctx context.Context, id uint, payload fleet.ModifyLabelPayload) (*fleet.Label, []uint, error) -type ListLabelsFunc func(ctx context.Context, opt fleet.ListOptions) (labels []*fleet.Label, err error) +type ListLabelsFunc func(ctx context.Context, opt fleet.ListOptions, includeHostCounts bool) (labels []*fleet.Label, err error) type LabelsSummaryFunc func(ctx context.Context) (labels []*fleet.LabelSummary, err error) @@ -2557,11 +2557,11 @@ func (s *Service) ModifyLabel(ctx context.Context, id uint, payload fleet.Modify return s.ModifyLabelFunc(ctx, id, payload) } -func (s *Service) ListLabels(ctx context.Context, opt fleet.ListOptions) (labels []*fleet.Label, err error) { +func (s *Service) ListLabels(ctx context.Context, opt fleet.ListOptions, includeHostCounts bool) (labels []*fleet.Label, err error) { s.mu.Lock() s.ListLabelsFuncInvoked = true s.mu.Unlock() - return s.ListLabelsFunc(ctx, opt) + return s.ListLabelsFunc(ctx, opt, includeHostCounts) } func (s *Service) LabelsSummary(ctx context.Context) (labels []*fleet.LabelSummary, err error) { diff --git a/server/service/labels.go b/server/service/labels.go index b366dd42f3..1d20f9940b 100644 --- a/server/service/labels.go +++ b/server/service/labels.go @@ -271,7 +271,8 @@ func (svc *Service) GetLabel(ctx context.Context, id uint) (*fleet.Label, []uint //////////////////////////////////////////////////////////////////////////////// type listLabelsRequest struct { - ListOptions fleet.ListOptions `url:"list_options"` + ListOptions fleet.ListOptions `url:"list_options"` + IncludeHostCounts *bool `query:"include_host_counts,optional"` } type listLabelsResponse struct { @@ -284,7 +285,12 @@ func (r listLabelsResponse) Error() error { return r.Err } func listLabelsEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (fleet.Errorer, error) { req := request.(*listLabelsRequest) - labels, err := svc.ListLabels(ctx, req.ListOptions) + includeHostCounts := true + if req.IncludeHostCounts != nil { + includeHostCounts = *req.IncludeHostCounts + } + + labels, err := svc.ListLabels(ctx, req.ListOptions, includeHostCounts) if err != nil { return listLabelsResponse{Err: err}, nil } @@ -300,15 +306,21 @@ func listLabelsEndpoint(ctx context.Context, request interface{}, svc fleet.Serv return resp, nil } -func (svc *Service) ListLabels(ctx context.Context, opt fleet.ListOptions) ([]*fleet.Label, error) { +func (svc *Service) ListLabels(ctx context.Context, opt fleet.ListOptions, includeHostCounts bool) ([]*fleet.Label, error) { if err := svc.authz.Authorize(ctx, &fleet.Label{}, fleet.ActionRead); err != nil { return nil, err } + + filter := fleet.TeamFilter{} vc, ok := viewer.FromContext(ctx) if !ok { return nil, fleet.ErrNoContext } - filter := fleet.TeamFilter{User: vc.User, IncludeObserver: true} + + // Default to including host counts. + if includeHostCounts { + filter = fleet.TeamFilter{User: vc.User, IncludeObserver: true} + } // TODO(mna): ListLabels doesn't currently return the hostIDs members of the // label, the quick approach would be an N+1 queries endpoint. Leaving like diff --git a/server/service/labels_test.go b/server/service/labels_test.go index c30e1dbc85..9473435ae1 100644 --- a/server/service/labels_test.go +++ b/server/service/labels_test.go @@ -114,7 +114,7 @@ func TestLabelsAuth(t *testing.T) { _, err = svc.GetLabelSpec(ctx, "abc") checkAuthErr(t, tt.shouldFailRead, err) - _, err = svc.ListLabels(ctx, fleet.ListOptions{}) + _, err = svc.ListLabels(ctx, fleet.ListOptions{}, true) checkAuthErr(t, tt.shouldFailRead, err) _, err = svc.LabelsSummary((ctx)) @@ -132,6 +132,33 @@ func TestLabelsAuth(t *testing.T) { } } +func TestListLabelsHostCountOptions(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + user := &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)} + ctx = viewer.NewContext(ctx, viewer.Viewer{User: user}) + + ds.ListLabelsFunc = func(ctx context.Context, filter fleet.TeamFilter, opts fleet.ListOptions) ([]*fleet.Label, error) { + // Expect the team filter to be empty, meaning no host counts requested + require.Nil(t, filter.User) + return nil, nil + } + + // Test explicitly setting include_host_counts to false + _, err := svc.ListLabels(ctx, fleet.ListOptions{}, false) + require.NoError(t, err) + + ds.ListLabelsFunc = func(ctx context.Context, filter fleet.TeamFilter, opts fleet.ListOptions) ([]*fleet.Label, error) { + // Expect the team filter to be empty, meaning no host counts requested + require.Equal(t, filter.User, user) + return nil, nil + } + + // Test explicitly setting include_host_counts to true + _, err = svc.ListLabels(ctx, fleet.ListOptions{}, true) + require.NoError(t, err) +} + func TestLabelsWithDS(t *testing.T) { ds := mysql.CreateMySQLDS(t) @@ -171,7 +198,7 @@ func testLabelsListLabels(t *testing.T, ds *mysql.Datastore) { svc, ctx := newTestService(t, ds, nil, nil) require.NoError(t, ds.MigrateData(context.Background())) - labels, err := svc.ListLabels(test.UserContext(ctx, test.UserAdmin), fleet.ListOptions{Page: 0, PerPage: 1000}) + labels, err := svc.ListLabels(test.UserContext(ctx, test.UserAdmin), fleet.ListOptions{Page: 0, PerPage: 1000}, true) require.NoError(t, err) require.Len(t, labels, 8)