diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index f95dfc281..227d97dcc 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -157,10 +157,11 @@ describe('Recommendations Module', () => { }); describe('loadRecommendations', () => { - test('fetches recommendations with provider/account_ids hints (Bundle B)', async () => { - // After Bundle B, only provider + account_ids are sent to the API as - // hints; service/region/numeric filters are pure client-side via - // applyColumnFilters. The legacy DOM filter inputs are gone. + afterEach(() => (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({})); + + test('fetches with provider/account_ids when no column filter is set', async () => { + // No column filters active: service and region are absent from the + // API call (undefined); provider + account_ids carry the only hints. (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [], @@ -172,7 +173,63 @@ describe('Recommendations Module', () => { expect(api.getRecommendations).toHaveBeenCalledWith({ provider: 'all', account_ids: undefined, + service: undefined, + region: undefined, + }); + }); + + test('pushes single-value service filter to API params (issue #162)', async () => { + // A single-value service column filter must be forwarded to the API + // so the backend applies it in SQL, saving bandwidth on large tenants. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + service: { kind: 'set', values: ['ec2'] }, + }); + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, + recommendations: [], + regions: [], + }); + + await loadRecommendations(); + + expect(api.getRecommendations).toHaveBeenCalledWith( + expect.objectContaining({ service: 'ec2' }), + ); + }); + + test('pushes single-value region filter to API params (issue #162)', async () => { + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + region: { kind: 'set', values: ['us-east-1'] }, + }); + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, + recommendations: [], + regions: [], }); + + await loadRecommendations(); + + expect(api.getRecommendations).toHaveBeenCalledWith( + expect.objectContaining({ region: 'us-east-1' }), + ); + }); + + test('does not push multi-value service filter to API (stays client-side)', async () => { + // When the user has selected multiple services the server only supports + // equality (single-value); fall back to client-side filtering. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + service: { kind: 'set', values: ['ec2', 'rds'] }, + }); + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, + recommendations: [], + regions: [], + }); + + await loadRecommendations(); + + const call = (api.getRecommendations as jest.Mock).mock.calls[0][0]; + expect(call.service).toBeUndefined(); }); test('renders recommendations summary', async () => { @@ -4971,11 +5028,16 @@ describe('Monthly Cost + Effective % column rendering', () => { }); (state.getRecommendations as jest.Mock).mockReturnValue([nullRec]); // Apply a filter that would match $0 if NaN were treated as 0. - // Use mockReturnValueOnce so the override doesn't leak into subsequent tests. - (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValueOnce({ + // loadRecommendations now calls getRecommendationsColumnFilters once for + // pushdown extraction before the render path calls it again (issue #162), + // so we need mockReturnValue rather than mockReturnValueOnce. The mock is + // explicitly reset to {} afterward so subsequent tests are unaffected + // (jest.clearAllMocks does not reset mockReturnValue). + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ on_demand_monthly: { kind: 'expr', expr: '0' }, }); await loadRecommendations(); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); // Null-monthly_cost row should be filtered out; tbody should have no visible recommendation rows. const rows = document.querySelectorAll('tbody tr.recommendation-row'); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 9b9f2c5d3..a75566e20 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -195,8 +195,10 @@ export function resetCachedGlobalConfig(): void { // populateRecommendationsAccountFilter / populateRegionFilter / the legacy // service-filter helpers were removed in Bundle B — those DOM elements are // gone from index.html. Provider / Account values now drive an API re-fetch -// via the column-header popovers (see openColumnPopover wiring); Service / -// Region filtering is purely client-side via applyColumnFilters. +// via the column-header popovers (see openColumnPopover wiring). Service and +// Region single-value categorical commits also trigger a re-fetch (issue +// #162) so the backend WHERE clause prunes rows before they hit the wire; +// multi-value and numeric column filters remain purely client-side. /** * Get the recommendations currently loaded in the purchase modal, @@ -488,13 +490,31 @@ export async function loadRecommendations(): Promise { } try { - // Provider + account_ids are still sent to the API as hints so the - // backend stays bounded for big multi-cloud tenants. Service / Region / - // numeric filters are pure client-side via applyColumnFilters. + // Provider + account_ids are sent to the API as push-down hints so the + // backend stays bounded for big multi-cloud tenants. Service and Region + // single-value categorical filters are also pushed down (issue #162): + // when the user has selected exactly one value in the service or region + // column popover the API receives that value and the backend applies the + // WHERE clause before returning rows, saving bandwidth on large tenants. + // Multi-value or absent service/region filters fall through unchanged and + // are applied client-side by applyColumnFilters below. const accountIDs = state.getCurrentAccountIDs(); + const columnFilters = state.getRecommendationsColumnFilters(); + const serviceFilter = columnFilters['service']; + const regionFilter = columnFilters['region']; + const pushedService = + serviceFilter?.kind === 'set' && serviceFilter.values.length === 1 + ? serviceFilter.values[0] + : undefined; + const pushedRegion = + regionFilter?.kind === 'set' && regionFilter.values.length === 1 + ? regionFilter.values[0] + : undefined; const filters: api.RecommendationFilters = { provider: state.getCurrentProvider(), account_ids: accountIDs.length > 0 ? accountIDs : undefined, + service: pushedService, + region: pushedRegion, }; const [data, accounts, cfgResponse] = await Promise.all([ @@ -1967,6 +1987,34 @@ function categoricalDisplayLabel( return value; } +// Columns whose single-value categorical filter is pushed down to the server +// (issue #162). When a commit on one of these columns resolves to exactly one +// selected value, loadRecommendations() re-fetches from the API with that +// value as a query param; the backend applies it in the WHERE clause before +// returning rows. For multi-value or cleared filters on these columns the +// filter is still applied client-side so the UX stays consistent. +// 'account' drives account_ids which is already a server-side hint; include +// it here so clearing the account filter triggers a re-fetch like the others. +const SERVER_PUSHDOWN_COLUMNS: ReadonlySet = new Set([ + 'service', 'region', 'account', +]); + +// commitAndRefresh is called by every categorical popover commit. For +// server-pushdown columns it fires loadRecommendations() (re-fetch) so the +// backend can prune rows before they hit the wire; for all other columns it +// calls rerenderRecommendations() (client-side filter, no extra API call). +// +// The re-fetch path is intentionally async/void: the popover commit returns +// immediately and the table skeleton renders while the fetch is in flight, +// matching the existing provider/account-change flow. +function commitAndRefresh(column: state.RecommendationsColumnId): void { + if (SERVER_PUSHDOWN_COLUMNS.has(column)) { + void loadRecommendations(); + } else { + rerenderRecommendations(); + } +} + // Build the popover DOM for a given column. Categorical: checkbox list with // (All) tri-state + Clear footer. Numeric: free-text expression input with // inline error and Clear footer. @@ -2131,6 +2179,9 @@ function buildPopoverContent( // allow-list so unchecking the last value reaches the same // zero-row state as the (All) checkbox and the Clear button, // rather than snapping back to "all checked". + // For server-pushdown columns (service / region / account) a single-value + // selection triggers a re-fetch so the backend prunes rows before they + // hit the wire (issue #162). All other columns stay client-side only. const commit = (): void => { const selected: string[] = []; checkboxes.forEach((cb, value) => { if (cb.checked) selected.push(value); }); @@ -2142,7 +2193,7 @@ function buildPopoverContent( saveColumnFilters(state.getRecommendationsColumnFilters()); updateAllTriState(); updateSPTriState(); - rerenderRecommendations(); + commitAndRefresh(column); }; // (All) and Clear use commitAll(target) directly: @@ -2161,7 +2212,7 @@ function buildPopoverContent( saveColumnFilters(state.getRecommendationsColumnFilters()); updateAllTriState(); updateSPTriState(); - rerenderRecommendations(); + commitAndRefresh(column); }; // Expose commitAll to the Clear button handler outside this else-branch. commitAllRef = commitAll; @@ -2214,7 +2265,8 @@ function buildPopoverContent( // Issue #700: call commitAllRef(false) (the same as commitAll(false) // inside the categorical branch) so updateAllTriState() is invoked and // the (All) checkbox reflects the cleared state. commitAllRef also - // calls rerenderRecommendations() internally. + // calls commitAndRefresh(column) internally, which re-fetches for + // server-pushdown columns (issue #162) or re-renders for the others. commitAllRef?.(false); } }); diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 1d82e3ee8..f9a4792ab 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -43,6 +43,82 @@ func TestHandler_getRecommendations(t *testing.T) { assert.Equal(t, float64(0), result.Summary.TotalMonthlySavings) } +// TestGetRecommendations_PushdownFilters verifies that service, region, and +// account_id query params are translated into the RecommendationFilter that +// reaches ListRecommendations (issue #162). The mock is registered with the +// exact filter shape expected so testify's mock.AssertExpectations confirms +// the backend does not silently drop any of the three values. +func TestGetRecommendations_PushdownFilters(t *testing.T) { + ctx := context.Background() + validUUID := "12345678-1234-1234-1234-123456789abc" + + t.Run("service and region params reach the filter", func(t *testing.T) { + mockScheduler := new(MockScheduler) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + mockScheduler.On( + "ListRecommendations", ctx, + config.RecommendationFilter{Provider: "aws", Service: "ec2", Region: "us-east-1"}, + ).Return([]config.RecommendationRecord{}, nil) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + _, err := handler.getRecommendations(ctx, req, map[string]string{ + "provider": "aws", + "service": "ec2", + "region": "us-east-1", + }) + require.NoError(t, err) + }) + + t.Run("account_id param reaches the filter", func(t *testing.T) { + mockScheduler := new(MockScheduler) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + mockScheduler.On( + "ListRecommendations", ctx, + config.RecommendationFilter{AccountIDs: []string{validUUID}}, + ).Return([]config.RecommendationRecord{}, nil) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + _, err := handler.getRecommendations(ctx, req, map[string]string{ + "account_ids": validUUID, + }) + require.NoError(t, err) + }) + + t.Run("invalid service slug returns 400", func(t *testing.T) { + handler := &Handler{apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + _, err := handler.getRecommendations(ctx, req, map[string]string{ + "service": "EC2 Reserved!", // uppercase + space + bang are invalid + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T", err) + assert.Equal(t, 400, ce.code) + }) + + t.Run("invalid region name returns 400", func(t *testing.T) { + handler := &Handler{apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + _, err := handler.getRecommendations(ctx, req, map[string]string{ + "region": "US EAST 1!", // spaces and bang are invalid + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T", err) + assert.Equal(t, 400, ce.code) + }) +} + func TestHandler_getRecommendationsFreshness(t *testing.T) { ctx := context.Background()