diff --git a/frontend/src/__tests__/api.test.ts b/frontend/src/__tests__/api.test.ts index ad67db4be..ab0541056 100644 --- a/frontend/src/__tests__/api.test.ts +++ b/frontend/src/__tests__/api.test.ts @@ -709,19 +709,69 @@ describe('Recommendations API', () => { expect(fetchMock).toHaveBeenCalledWith('/api/recommendations', expect.anything()); }); - test('applies filters to query string', async () => { + // Regression test for issue #1089: minSavingsUsd (dollar) and minSavingsPct + // (percentage) must map to distinct query params -- min_savings_usd and + // min_savings_pct -- and must NEVER both map to the same legacy `min_savings` + // key. Conflating them would silently treat a percentage as a dollar amount + // or vice versa. + test('minSavingsUsd maps to min_savings_usd (dollar) query param', async () => { + await getRecommendations({ + provider: 'aws', + service: 'ec2', + region: 'us-east-1', + minSavingsUsd: 100 + }); + + const url = fetchMock.mock.calls[0][0] as string; + expect(url).toContain('provider=aws'); + expect(url).toContain('service=ec2'); + expect(url).toContain('region=us-east-1'); + // Dollar filter uses the distinct usd-suffixed key. + expect(url).toContain('min_savings_usd=100'); + // Must NOT appear as the bare (ambiguous) key or as the pct key. + expect(url).not.toContain('min_savings='); + expect(url).not.toContain('min_savings_pct='); + }); + + test('minSavingsPct maps to min_savings_pct (percentage) query param', async () => { + await getRecommendations({ + minSavingsPct: 30 + }); + + const url = fetchMock.mock.calls[0][0] as string; + // Percentage filter uses the distinct pct-suffixed key. + expect(url).toContain('min_savings_pct=30'); + // Must NOT appear as the bare (ambiguous) key or as the usd key. + expect(url).not.toContain('min_savings='); + expect(url).not.toContain('min_savings_usd='); + }); + + test('usd and pct filters can be combined and remain distinct', async () => { + await getRecommendations({ + minSavingsUsd: 50, + minSavingsPct: 20 + }); + + const url = fetchMock.mock.calls[0][0] as string; + expect(url).toContain('min_savings_usd=50'); + expect(url).toContain('min_savings_pct=20'); + // The same value "50" for usd must not appear on the pct key and vice versa. + expect(url).not.toContain('min_savings_pct=50'); + expect(url).not.toContain('min_savings_usd=20'); + expect(url).not.toContain('min_savings='); + }); + + test('applies other filters to query string', async () => { await getRecommendations({ provider: 'aws', service: 'ec2', region: 'us-east-1', - minSavings: 100 }); const url = fetchMock.mock.calls[0][0] as string; expect(url).toContain('provider=aws'); expect(url).toContain('service=ec2'); expect(url).toContain('region=us-east-1'); - expect(url).toContain('min_savings=100'); }); }); diff --git a/frontend/src/api/recommendations.ts b/frontend/src/api/recommendations.ts index 258623cdd..d7447745f 100644 --- a/frontend/src/api/recommendations.ts +++ b/frontend/src/api/recommendations.ts @@ -13,7 +13,10 @@ export async function getRecommendations(filters: RecommendationFilters = {}): P if (filters.provider) params.set('provider', filters.provider); if (filters.service) params.set('service', filters.service); if (filters.region) params.set('region', filters.region); - if (filters.minSavings) params.set('min_savings', String(filters.minSavings)); + // min_savings_usd: absolute dollar floor (distinct from the CLI --min-savings-pct percentage filter) + if (filters.minSavingsUsd) params.set('min_savings_usd', String(filters.minSavingsUsd)); + // min_savings_pct: percentage floor (mirrors the CLI --min-savings-pct semantics) + if (filters.minSavingsPct) params.set('min_savings_pct', String(filters.minSavingsPct)); if (filters.account_ids && filters.account_ids.length > 0) params.set('account_ids', filters.account_ids.join(',')); const queryString = params.toString(); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 089903d58..6c19fe7ea 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -175,7 +175,24 @@ export interface RecommendationFilters { provider?: Provider | ''; service?: string; region?: string; - minSavings?: number; + /** + * Minimum absolute dollar savings per month. Sent to the API as + * `min_savings_usd`. 0 or absent means no floor. + * + * IMPORTANT: this is a DOLLAR amount, not a percentage. The CLI equivalent + * is `--min-savings-pct` which filters by percentage -- these are two + * distinct concepts. Use `minSavingsPct` to filter by percentage. + */ + minSavingsUsd?: number; + /** + * Minimum effective savings percentage (0-100 scale). Sent to the API as + * `min_savings_pct`. 0 or absent means no floor. + * + * IMPORTANT: this is a PERCENTAGE (0-100), not a dollar amount. The CLI + * `--min-savings-pct` flag uses the same semantics. Use `minSavingsUsd` + * to filter by an absolute dollar floor. + */ + minSavingsPct?: number; account_ids?: string[]; } diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index ab6bfd424..943cc6782 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -12,23 +12,41 @@ import ( "github.com/aws/aws-lambda-go/events" ) -// Recommendations handlers -func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) { - // Require view:recommendations permission - session, err := h.requirePermission(ctx, req, "view", "recommendations") - if err != nil { - return nil, err - } - - // Validate input parameters to prevent injection attacks +// parseRecommendationFilter validates all query parameters and builds the DB +// filter. It centralises the parameter-validation logic so getRecommendations +// stays below the cyclomatic-complexity threshold. +// +// account_ids filter semantics (issue #211): +// +// account_ids param | cloud_account_id row | Result +// -------------------|----------------------|---------------------------- +// absent | any (incl. NULL) | included (no SQL filter) +// non-empty | NULL | excluded (legacy rows are +// | | not "in any account") +// non-empty | matches one of IDs | included +// non-empty | doesn't match | excluded +// non-empty, contains| matches (account is | excluded — enforced by +// a disabled ID | disabled) | filterRecommendationsByAllowedAccounts +// | | below, NOT by the SQL filter +// +// The SQL contract lives in config.buildRecommendationFilter +// (store_postgres_recommendations.go). The disabled-account case is enforced by +// filterRecommendationsByAllowedAccounts after the DB read. +// +// min_savings_usd: absolute dollar floor on monthly savings. +// min_savings_pct: effective savings percentage floor (0-100 scale). +// Both are optional; absent or "0" means no floor. Fractions are rejected (a +// user typing "30%" expects a percentage, not $30.5). +func parseRecommendationFilter(params map[string]string) (config.RecommendationFilter, error) { + // Validate input parameters to prevent injection attacks. if err := validateProvider(params["provider"]); err != nil { - return nil, err + return config.RecommendationFilter{}, err } if err := validateServiceName(params["service"]); err != nil { - return nil, err + return config.RecommendationFilter{}, err } if err := validateRegion(params["region"]); err != nil { - return nil, err + return config.RecommendationFilter{}, err } // parseAccountIDs splits, trims, and UUID-validates the comma-separated @@ -37,33 +55,44 @@ func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunc // MaxAccountIDsPerRequest (200). See validation.go::parseAccountIDs. accountIDs, err := parseAccountIDs(params["account_ids"]) if err != nil { - return nil, NewClientError(400, err.Error()) + return config.RecommendationFilter{}, NewClientError(400, err.Error()) + } + + minSavingsUSD, err := parseMinSavingsParam(params["min_savings_usd"], "min_savings_usd") + if err != nil { + return config.RecommendationFilter{}, err + } + minSavingsPct, err := parseMinSavingsParam(params["min_savings_pct"], "min_savings_pct") + if err != nil { + return config.RecommendationFilter{}, err } + if minSavingsPct < 0 || minSavingsPct > 100 { + return config.RecommendationFilter{}, NewClientError(400, "min_savings_pct must be between 0 and 100") + } + + return config.RecommendationFilter{ + Provider: params["provider"], + Service: params["service"], + Region: params["region"], + AccountIDs: accountIDs, + MinSavingsUSD: minSavingsUSD, + MinSavingsPct: minSavingsPct, + }, nil +} - // account_ids filter semantics (issue #211): - // - // account_ids param | cloud_account_id row | Result - // -------------------|----------------------|---------------------------- - // absent | any (incl. NULL) | included (no SQL filter) - // non-empty | NULL | excluded (legacy rows are - // | | not "in any account") - // non-empty | matches one of IDs | included - // non-empty | doesn't match | excluded - // non-empty, contains| matches (account is | excluded — enforced by - // a disabled ID | disabled) | filterRecommendationsByAllowedAccounts - // | | below, NOT by the SQL filter - // - // The SQL contract lives in config.buildRecommendationFilter - // (store_postgres_recommendations.go). The disabled-account case is - // enforced by filterRecommendationsByAllowedAccounts after the DB read. +// Recommendations handlers +func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) { + // Require view:recommendations permission + session, err := h.requirePermission(ctx, req, "view", "recommendations") + if err != nil { + return nil, err + } - // Translate query string → DB-level filter. ListRecommendations - // pushes these into the WHERE clause so the cache does the pruning. - filter := config.RecommendationFilter{ - Provider: params["provider"], - Service: params["service"], - Region: params["region"], - AccountIDs: accountIDs, + // Translate query string to DB-level filter. ListRecommendations pushes + // these into the WHERE clause so the cache does the pruning. + filter, err := parseRecommendationFilter(params) + if err != nil { + return nil, err } recommendations, err := h.scheduler.ListRecommendations(ctx, filter) diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 942436ddf..8fabe8521 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -454,6 +454,126 @@ func TestBuildRecommendationsResponse_CapacityFields(t *testing.T) { } } +// TestGetRecommendations_MinSavingsFilters is the regression test for issue #1089: +// it asserts that min_savings_usd (dollar floor) and min_savings_pct (percentage +// floor) are distinct parameters with separate semantics and cannot be confused. +// +// Key invariants proven: +// - A dollar value passed as min_savings_usd does NOT conflate with a percentage +// (e.g. min_savings_usd=30 means "$30", not "30%"). +// - A percentage passed as min_savings_pct does NOT conflate with a dollar amount. +// - Both can be specified independently and are applied independently. +// - min_savings_pct validates the range 0-100. +// - Non-numeric values are rejected with 400. +func TestGetRecommendations_MinSavingsFilters(t *testing.T) { + ctx := context.Background() + + t.Run("min_savings_usd filter is wired to RecommendationFilter.MinSavingsUSD not MinSavingsPct", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool { + // Dollar filter must be set; percentage filter must be zero. + return f.MinSavingsUSD == 30 && f.MinSavingsPct == 0 + })).Return([]config.RecommendationRecord{}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + params := map[string]string{"min_savings_usd": "30"} + + result, err := handler.getRecommendations(ctx, req, params) + require.NoError(t, err) + assert.NotNil(t, result) + }) + + t.Run("min_savings_pct filter is wired to RecommendationFilter.MinSavingsPct not MinSavingsUSD", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool { + // Percentage filter must be set; dollar filter must be zero. + return f.MinSavingsPct == 30 && f.MinSavingsUSD == 0 + })).Return([]config.RecommendationRecord{}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + // Sending "30" as min_savings_pct must NOT be treated as $30 by the server. + params := map[string]string{"min_savings_pct": "30"} + + result, err := handler.getRecommendations(ctx, req, params) + require.NoError(t, err) + assert.NotNil(t, result) + }) + + t.Run("both filters can be combined independently", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool { + return f.MinSavingsUSD == 50 && f.MinSavingsPct == 20 + })).Return([]config.RecommendationRecord{}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + params := map[string]string{"min_savings_usd": "50", "min_savings_pct": "20"} + + result, err := handler.getRecommendations(ctx, req, params) + require.NoError(t, err) + assert.NotNil(t, result) + }) + + t.Run("min_savings_pct above 100 returns 400", func(t *testing.T) { + handler := &Handler{apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + params := map[string]string{"min_savings_pct": "101"} + + result, err := handler.getRecommendations(ctx, req, params) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + }) + + t.Run("min_savings_usd with non-numeric value returns 400", func(t *testing.T) { + handler := &Handler{apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + params := map[string]string{"min_savings_usd": "thirty"} + + result, err := handler.getRecommendations(ctx, req, params) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + }) + + t.Run("min_savings_pct with non-numeric value returns 400", func(t *testing.T) { + handler := &Handler{apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + params := map[string]string{"min_savings_pct": "thirty"} + + result, err := handler.getRecommendations(ctx, req, params) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + }) + + t.Run("absent filters pass through zero values in RecommendationFilter", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool { + return f.MinSavingsUSD == 0 && f.MinSavingsPct == 0 + })).Return([]config.RecommendationRecord{}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}} + // No min_savings params at all. + result, err := handler.getRecommendations(ctx, req, map[string]string{}) + require.NoError(t, err) + assert.NotNil(t, result) + }) +} + func TestConfidenceBucketFor(t *testing.T) { cases := []struct { name string diff --git a/internal/api/validation.go b/internal/api/validation.go index 9c5814064..cde323a94 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -6,6 +6,7 @@ import ( "fmt" "net/mail" "regexp" + "strconv" "strings" "unicode" @@ -587,3 +588,26 @@ func decodeBase64Password(encoded string) (string, error) { } return string(decoded), nil } + +// parseMinSavingsParam parses a numeric savings-floor query parameter. +// Returns (0, nil) when the parameter is absent or empty (no floor). +// Returns 400 when the value is present but not a valid non-negative +// integer or float. Fractional values are allowed (e.g. "12.5") since +// savings floors can be sub-dollar amounts. +// +// paramName is included in the error message so callers can distinguish +// min_savings_usd vs min_savings_pct errors in client logs. +func parseMinSavingsParam(raw string, paramName string) (float64, error) { + raw = strings.TrimSpace(raw) + if raw == "" || raw == "0" { + return 0, nil + } + v, err := strconv.ParseFloat(raw, 64) + if err != nil { + return 0, NewClientError(400, fmt.Sprintf("%s must be a non-negative number", paramName)) + } + if v < 0 { + return 0, NewClientError(400, fmt.Sprintf("%s must be non-negative", paramName)) + } + return v, nil +} diff --git a/internal/api/validation_test.go b/internal/api/validation_test.go index 5e07fb396..d3db0a33f 100644 --- a/internal/api/validation_test.go +++ b/internal/api/validation_test.go @@ -6,6 +6,7 @@ import ( "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestValidateRegion(t *testing.T) { @@ -499,3 +500,51 @@ func TestValidateAWSWebIdentityTokenFile(t *testing.T) { }) } } + +// TestParseMinSavingsParam is the regression test for issue #1089. +// It asserts that the savings-floor parsing helper correctly handles absent, +// zero, numeric, and invalid inputs, and that the "usd" and "pct" paths +// produce independent parsing results (preventing unit conflation at parse time). +func TestParseMinSavingsParam(t *testing.T) { + cases := []struct { + name string + raw string + paramName string + wantVal float64 + wantError bool + }{ + // Absent / zero inputs produce a zero floor (no filter). + {"empty string", "", "min_savings_usd", 0, false}, + {"zero string", "0", "min_savings_usd", 0, false}, + {"whitespace only", " ", "min_savings_usd", 0, false}, + + // Valid positive values. + {"positive integer", "30", "min_savings_usd", 30, false}, + {"positive float", "12.5", "min_savings_usd", 12.5, false}, + {"large integer", "10000", "min_savings_usd", 10000, false}, + + // Percentage param uses the same parsing path. + {"pct integer", "20", "min_savings_pct", 20, false}, + {"pct float", "33.3", "min_savings_pct", 33.3, false}, + + // Invalid inputs. + {"non-numeric word", "thirty", "min_savings_usd", 0, true}, + {"negative value", "-5", "min_savings_usd", 0, true}, + {"mixed string", "30abc", "min_savings_usd", 0, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + val, err := parseMinSavingsParam(tc.raw, tc.paramName) + if tc.wantError { + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + } else { + require.NoError(t, err) + assert.InDelta(t, tc.wantVal, val, 0.0001) + } + }) + } +} diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 626ac7e0d..bd5fc2237 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -266,8 +266,13 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) { if len(filter.AccountIDs) > 0 { add("cloud_account_id = ANY($%d)", filter.AccountIDs) } - if filter.MinSavings > 0 { - add("monthly_savings >= $%d", filter.MinSavings) + // MinSavingsUSD is pushed into SQL as a dollar floor on monthly_savings. + // MinSavingsPct is NOT pushed into SQL -- it is applied in-process in + // ListStoredRecommendations after the query, because monthly_savings is + // an absolute dollar column and the percentage requires knowledge of the + // on-demand baseline stored inside the JSONB payload. + if filter.MinSavingsUSD > 0 { + add("monthly_savings >= $%d", filter.MinSavingsUSD) } if filter.ID != "" { add("payload->>'id' = $%d", filter.ID) @@ -278,10 +283,47 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) { return " WHERE " + strings.Join(conds, " AND "), args } +// recEffectiveSavingsPct computes the effective savings percentage for a +// recommendation record. Mirrors the formula used client-side in +// frontend/src/recommendations.ts::effectiveSavingsPct so server-side and +// client-side filtering produce the same results. +// +// Returns (pct, true) when a meaningful percentage can be computed: +// +// amortized_upfront = upfront_cost / (term * 12) +// effective_savings = savings - amortized_upfront +// pct = (effective_savings / on_demand_monthly) * 100 +// +// Returns (0, false) when any required input is absent or invalid (term == 0, +// no on-demand baseline and no monthly_cost, or on-demand == 0). +func recEffectiveSavingsPct(rec *RecommendationRecord) (float64, bool) { + if rec.Term == 0 { + return 0, false + } + hasOnDemand := rec.OnDemandCost != nil && *rec.OnDemandCost > 0 + if !hasOnDemand && rec.MonthlyCost == nil { + return 0, false + } + monthsInTerm := float64(rec.Term * 12) + amortized := rec.UpfrontCost / monthsInTerm + effectiveSavings := rec.Savings - amortized + var onDemand float64 + if hasOnDemand { + onDemand = *rec.OnDemandCost + } else { + onDemand = *rec.MonthlyCost + rec.Savings + amortized + } + if onDemand == 0 { + return 0, false + } + return (effectiveSavings / onDemand) * 100, true +} + // ListStoredRecommendations reads recommendations matching the filter. -// All pushed-down filter conditions are applied in SQL so Postgres (not -// Go) prunes the rows. The payload JSONB is decoded back into the original -// RecommendationRecord. Ordering is left to callers. +// SQL-pushed conditions (Provider, Service, Region, AccountIDs, +// MinSavingsUSD) are applied in SQL so Postgres prunes the rows; the +// MinSavingsPct filter is applied in-process because the on-demand +// baseline lives inside the JSONB payload (not a native column). func (s *PostgresStore) ListStoredRecommendations(ctx context.Context, filter RecommendationFilter) ([]RecommendationRecord, error) { whereClause, args := buildRecommendationFilter(filter) rows, err := s.db.Query(ctx, `SELECT payload FROM recommendations`+whereClause, args...) @@ -300,6 +342,14 @@ func (s *PostgresStore) ListStoredRecommendations(ctx context.Context, filter Re if err := json.Unmarshal(payload, &rec); err != nil { return nil, fmt.Errorf("failed to unmarshal payload: %w", err) } + // Apply the in-process percentage floor. Recs where the percentage + // cannot be computed (missing on-demand baseline) pass through -- we + // only drop a rec when pct is computable AND below the threshold. + if filter.MinSavingsPct > 0 { + if pct, ok := recEffectiveSavingsPct(&rec); ok && pct < filter.MinSavingsPct { + continue + } + } out = append(out, rec) } if err := rows.Err(); err != nil { diff --git a/internal/config/store_postgres_savings_filter_test.go b/internal/config/store_postgres_savings_filter_test.go new file mode 100644 index 000000000..5da97387c --- /dev/null +++ b/internal/config/store_postgres_savings_filter_test.go @@ -0,0 +1,163 @@ +package config + +// Regression tests for issue #1089: min-savings unit disambiguation. +// These tests assert that MinSavingsUSD (dollar floor, pushed to SQL) and +// MinSavingsPct (percentage floor, applied in-process) are distinct fields +// with separate semantics that cannot be conflated. +// +// recEffectiveSavingsPct and buildRecommendationFilter are unexported; the +// white-box package config tests exercise them directly. + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// recEffectiveSavingsPct unit tests +// --------------------------------------------------------------------------- + +func TestRecEffectiveSavingsPct_Config(t *testing.T) { + f64 := func(v float64) *float64 { return &v } + + t.Run("correct pct with on_demand_cost", func(t *testing.T) { + // term=1yr, no upfront, savings=$100/mo, on_demand=$500/mo + // effective = 100 - 0 = 100; pct = (100/500)*100 = 20% + rec := &RecommendationRecord{ + Term: 1, + Savings: 100, + UpfrontCost: 0, + OnDemandCost: f64(500), + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 20.0, pct, 0.001) + }) + + t.Run("amortizes upfront cost across term", func(t *testing.T) { + // term=3yr, upfront=$360, savings=$100/mo, on_demand=$110/mo (approx) + // amortized = 360 / (3*12) = 360/36 = 10 + // effective = 100 - 10 = 90 + // on_demand = 110 (supplied directly) + // pct = (90/110)*100 ~ 81.8% + rec := &RecommendationRecord{ + Term: 3, + Savings: 100, + UpfrontCost: 360, + OnDemandCost: f64(110), + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 81.818, pct, 0.01) + }) + + t.Run("returns false for term zero", func(t *testing.T) { + rec := &RecommendationRecord{Term: 0, Savings: 100, OnDemandCost: f64(500)} + _, ok := recEffectiveSavingsPct(rec) + assert.False(t, ok, "term=0 must not produce a pct") + }) + + t.Run("returns false when no baseline available", func(t *testing.T) { + // Neither on_demand_cost nor monthly_cost: cannot compute denominator. + rec := &RecommendationRecord{Term: 1, Savings: 100} + _, ok := recEffectiveSavingsPct(rec) + assert.False(t, ok) + }) + + t.Run("falls back to monthly_cost reconstruction", func(t *testing.T) { + // monthly_cost=400, savings=100, upfront=0, term=1 + // on_demand reconstructed = 400 + 100 + 0 = 500 + // pct = (100/500)*100 = 20% + monthly := 400.0 + rec := &RecommendationRecord{ + Term: 1, + Savings: 100, + UpfrontCost: 0, + MonthlyCost: &monthly, + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 20.0, pct, 0.001) + }) + + t.Run("returns false when on_demand denominator is zero", func(t *testing.T) { + // on_demand_cost=0 is treated as absent (provider didn't return a + // meaningful baseline), so the function falls back to monthly_cost + // reconstruction. If monthly_cost is also nil, it returns false. + rec := &RecommendationRecord{Term: 1, Savings: 100, OnDemandCost: f64(0)} + _, ok := recEffectiveSavingsPct(rec) + assert.False(t, ok, "on_demand=0 + no monthly_cost must not produce a pct") + }) +} + +// TestRecommendationFilter_UnitDistinction is the central regression test for +// issue #1089. It asserts that the SAME numeric value "30" produces different +// filter behaviour when interpreted as a dollar amount vs. a percentage: +// +// rec.Savings = $100/mo, on_demand = $500/mo => effective_pct = 20% +// min_savings_usd=30 => passes ($100 >= $30) +// min_savings_pct=30 => fails (20% < 30%) +// +// This proves the two filters are truly distinct and cannot be conflated. +func TestRecommendationFilter_UnitDistinction(t *testing.T) { + f64 := func(v float64) *float64 { return &v } + + // A recommendation that saves $100/mo on a $500/mo on-demand baseline + // => effective savings rate = 20%. + rec := &RecommendationRecord{ + Term: 1, + Savings: 100, + UpfrontCost: 0, + OnDemandCost: f64(500), + } + + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + + // The same numeric value "30" means opposite results in each unit. + const threshold = 30.0 + assert.Greater(t, rec.Savings, threshold, + "$100 savings passes $30 dollar floor (USD filter)") + assert.Less(t, pct, threshold, + "20%% savings fails 30%% percentage floor (PCT filter) -- distinct semantics, not conflated") +} + +// --------------------------------------------------------------------------- +// buildRecommendationFilter SQL builder tests +// --------------------------------------------------------------------------- + +func TestBuildRecommendationFilter_MinSavingsUSD(t *testing.T) { + t.Run("MinSavingsUSD zero produces no WHERE clause fragment", func(t *testing.T) { + clause, args := buildRecommendationFilter(RecommendationFilter{MinSavingsUSD: 0}) + assert.Empty(t, clause) + assert.Empty(t, args) + }) + + t.Run("MinSavingsUSD positive includes monthly_savings >= clause", func(t *testing.T) { + clause, args := buildRecommendationFilter(RecommendationFilter{MinSavingsUSD: 50}) + assert.Contains(t, clause, "monthly_savings >= $") + require.Len(t, args, 1) + assert.Equal(t, float64(50), args[0]) + }) + + t.Run("MinSavingsPct zero is never pushed into SQL (no WHERE fragment)", func(t *testing.T) { + // Pct filter is applied in-process, never in SQL. + clause, args := buildRecommendationFilter(RecommendationFilter{MinSavingsPct: 30}) + assert.Empty(t, clause, "MinSavingsPct must not appear in the SQL WHERE clause") + assert.Empty(t, args) + }) + + t.Run("MinSavingsUSD and MinSavingsPct combined: only USD in SQL", func(t *testing.T) { + clause, args := buildRecommendationFilter(RecommendationFilter{ + MinSavingsUSD: 50, + MinSavingsPct: 20, + }) + // Only the dollar floor appears in SQL. + assert.Contains(t, clause, "monthly_savings >= $") + // Exactly one arg (the dollar value); the pct is handled in-process. + require.Len(t, args, 1) + assert.Equal(t, float64(50), args[0]) + }) +} diff --git a/internal/config/types.go b/internal/config/types.go index 2c04a4f76..10eb4bc53 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -443,13 +443,23 @@ type PurchaseSuppression struct { // RecommendationFilter parameterises ListStoredRecommendations and the // handler-facing scheduler.ListRecommendations wrapper. Zero-value fields // mean "no filter"; non-empty AccountIDs restricts to the given IDs. +// +// MinSavingsUSD is a dollar floor: only recommendations whose monthly savings +// are >= MinSavingsUSD are returned. 0 means no floor. +// +// MinSavingsPct is a percentage floor (0–100): only recommendations whose +// effective savings percentage (savings/on-demand*100) meets or exceeds this +// threshold are returned. 0 means no floor. Applied in-process after the DB +// query rather than in SQL (avoids a computed column). These two filters are +// independent and can be combined. type RecommendationFilter struct { - Provider string // "aws" / "azure" / "gcp" / "" (all) - Service string // "" = all services - Region string // "" = all regions - AccountIDs []string // nil/empty = all accounts - MinSavings float64 // 0 = no floor on monthly savings - ID string // "" = all ids; non-empty = exact match on the id column + Provider string // "aws" / "azure" / "gcp" / "" (all) + Service string // "" = all services + Region string // "" = all regions + AccountIDs []string // nil/empty = all accounts + MinSavingsUSD float64 // 0 = no floor on monthly savings dollar amount + MinSavingsPct float64 // 0 = no floor on savings percentage (0–100 scale) + ID string // "" = all ids; non-empty = exact match on the id column } // PurchasePlanFilter parameterises ListPurchasePlans. Zero-value means "no