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)