diff --git a/.gitallowed b/.gitallowed index 8769f9bb8..6ff918dde 100644 --- a/.gitallowed +++ b/.gitallowed @@ -39,6 +39,9 @@ # Descending-step fixture added in #956 tests (store_postgres_pgxmock_test.go, # handler_analytics_test.go, handler_history_test.go, handler_dashboard_test.go). 999988887777 +# Repeating-pair-block fixture used in handler_analytics_test.go, +# handler_inventory_test.go, store_postgres_pgxmock_test.go. +111122223333 # UUID-shaped account ID used in handler_accounts_test.go (synthetic, not a # real subscription/account). diff --git a/internal/analytics/collector.go b/internal/analytics/collector.go index 6f2a5d684..2ea7d5c6d 100644 --- a/internal/analytics/collector.go +++ b/internal/analytics/collector.go @@ -91,7 +91,8 @@ func (c *Collector) Collect(ctx context.Context) error { // bounded by the number of live commitments rather than by all history ever // recorded. This avoids silently truncating older-but-still-active 1y/3y // commitments the way a single capped all-history page did. - purchases, err := c.configStore.GetActivePurchaseHistory(ctx, now) + // nil/nil account scope: the collector aggregates across all accounts. + purchases, err := c.configStore.GetActivePurchaseHistory(ctx, now, nil, nil) if err != nil { return fmt.Errorf("failed to get active purchase history: %w", err) } diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 5c7019925..07a824760 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -121,7 +121,7 @@ func (m *mockAnalyticsStore) Close() error { type mockConfigStore struct { getPurchaseHistoryFunc func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) getAllPurchaseHistoryFunc func(ctx context.Context, limit int) ([]config.PurchaseHistoryRecord, error) - getActivePurchaseHistoryFunc func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) + getActivePurchaseHistoryFunc func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) getPurchaseHistoryByPurchaseIDFunc func(ctx context.Context, purchaseID string) (*config.PurchaseHistoryRecord, error) markPurchaseRevokedFunc func(ctx context.Context, purchaseID string, revokedAt time.Time, revokedVia string, supportCaseID string) error } @@ -220,9 +220,9 @@ func (m *mockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return nil, nil } -func (m *mockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { +func (m *mockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { if m.getActivePurchaseHistoryFunc != nil { - return m.getActivePurchaseHistoryFunc(ctx, asOf) + return m.getActivePurchaseHistoryFunc(ctx, asOf, accountIDs, externalIDsByProvider) } return nil, nil } @@ -474,7 +474,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("returns error when GetAllPurchaseHistory fails", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return nil, errors.New("database connection failed") }, } @@ -487,7 +487,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("handles empty purchase history", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{}, nil }, } @@ -503,7 +503,7 @@ func TestCollectorCollect(t *testing.T) { Term: 1, EstimatedSavings: 100, UpfrontCost: 500, } cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{expired}, nil }, } @@ -514,7 +514,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("processes active purchases and creates snapshots", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 720, 1000)}, nil }, } @@ -532,7 +532,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("aggregates multiple purchases for same bucket", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{ activeRecord("aws", "rds", "us-east-1", 1, 100, 500), activeRecord("aws", "rds", "us-east-1", 1, 200, 1000), @@ -547,7 +547,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("creates separate snapshots per region and provider", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{ activeRecord("aws", "rds", "us-east-1", 1, 100, 500), activeRecord("aws", "rds", "us-west-2", 1, 150, 700), @@ -562,7 +562,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("sets SavingsPlan commitment type for SavingsPlans service", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "SavingsPlans", "us-east-1", 1, 500, 2000)}, nil }, } @@ -578,7 +578,7 @@ func TestCollectorCollect(t *testing.T) { }, } cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 100, 500)}, nil }, } @@ -591,7 +591,7 @@ func TestCollectorCollect(t *testing.T) { store := &mockAnalyticsStore{} const monthlySavings, upfront = 720.0, 8760.0 cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, monthlySavings, upfront)}, nil }, } @@ -608,7 +608,7 @@ func TestCollectorCollect(t *testing.T) { good := activeRecord("aws", "rds", "us-east-1", 1, 100, 500) badTerm := activeRecord("aws", "rds", "us-east-1", 0, 999, 999) // Term==0 -> +Inf commitment pre-fix cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{good, badTerm}, nil }, } @@ -625,7 +625,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("H1: a negative Term is also skipped", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", -1, 100, 500)}, nil }, } @@ -640,7 +640,7 @@ func TestCollectorCollect(t *testing.T) { noCost := activeRecord("aws", "ec2", "us-east-1", 1, 100, 500) // MonthlyCost nil cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{withCost, noCost}, nil }, } @@ -671,7 +671,7 @@ func TestCollectorCollect(t *testing.T) { tenantB.AccountID = tenantA.AccountID cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{tenantA, tenantB}, nil }, } @@ -689,7 +689,7 @@ func TestCollectorCollect(t *testing.T) { t.Run("ctx cancellation is terminal and surfaces an error", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ - getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 100, 500)}, nil }, } diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 7d2a56e71..8f2ce75f9 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -125,7 +125,8 @@ func (h *Handler) resolveDashboardAccountScope(ctx context.Context, params map[s // resolveAllowedAccountScope resolves a restricted session's allowed_accounts // into the dual-column purchase-history filter inputs so dashboard commitment -// metrics never include accounts the session can't access (issue #956). It lists +// metrics and the inventory endpoints (fetchCommitmentRecords) never include +// accounts the session can't access (issue #956). It lists // the cloud accounts, keeps those the session matches via auth.MatchesAccount, // and resolves their UUIDs through resolveAccountFilterIDs (same code path as an // explicit filter). @@ -583,43 +584,29 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco return byService } -// fetchCommitmentPurchases loads the purchase_history rows that calculateCommitmentMetrics -// aggregates, applying the pre-resolved account scope. Extracted to keep the -// parent under the cyclomatic limit (mirrors the appendAccountPredicate / -// accountMatchesFilters pattern). Returns ok=false on a store error or an -// explicit zero-account scope so the caller emits zeroed KPIs without querying. +// fetchCommitmentPurchases loads the active purchase_history rows that +// calculateCommitmentMetrics aggregates, applying the pre-resolved account +// scope. Extracted to keep the parent under the cyclomatic limit (mirrors the +// appendAccountPredicate / accountMatchesFilters pattern). Returns ok=false on +// a store error or an explicit zero-account scope so the caller emits zeroed +// KPIs without querying. // // - non-nil but empty accountUUIDs (no external groups): explicit "scoped to // zero accounts" sentinel (a restricted session whose allowed_accounts match // nothing). Returns (nil, false) WITHOUT querying so a scoped user never sees // all-accounts data (issue #956). A nil/empty filter sent to the store would // match every row (no WHERE clause), so this must short-circuit. -// - either accountUUIDs or accountExternalIDsByProvider non-empty: -// GetPurchaseHistoryFiltered with the dual-column predicate. -// - both nil: GetAllPurchaseHistory (no account filter — unrestricted session). -func (h *Handler) fetchCommitmentPurchases(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) { - const fetchLimit = 1000 - +// - otherwise: GetActivePurchaseHistory with the dual-column account predicate +// (empty scope means all accounts). The active filter runs in SQL with no +// row cap, so older-but-still-active 1y/3y commitments cannot be silently +// dropped the way a newest-first LIMIT 1000 page dropped them (issue #1140); +// the result is bounded by the number of live commitments. +func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) { if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { return nil, false } - var ( - purchases []config.PurchaseHistoryRecord - err error - ) - if len(accountUUIDs) > 0 || len(accountExternalIDsByProvider) > 0 { - // Account filter: dual-column match so NULL-cloud_account_id rows are - // counted via their external id. - purchases, err = h.config.GetPurchaseHistoryFiltered(ctx, config.PurchaseHistoryFilter{ - AccountIDs: accountUUIDs, - ExternalIDsByProvider: accountExternalIDsByProvider, - Limit: fetchLimit, - }) - } else { - // No account filter: fetch across all accounts. - purchases, err = h.config.GetAllPurchaseHistory(ctx, fetchLimit) - } + purchases, err := h.config.GetActivePurchaseHistory(ctx, asOf, accountUUIDs, accountExternalIDsByProvider) if err != nil { // Log error but don't fail the dashboard request. return nil, false @@ -643,12 +630,12 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, accountUUIDs []s // this KPI path (committedMonthly) and the per-service chart use exactly the // same gate. func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { - purchases, ok := h.fetchCommitmentPurchases(ctx, accountUUIDs, accountExternalIDsByProvider) + currentTime := time.Now() + purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, accountUUIDs, accountExternalIDsByProvider) if !ok { return 0, 0, 0, nil } - currentTime := time.Now() yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC) // Derive the per-service breakdown from the shared primitive so the diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index c6c2872fb..1ee7bbf7f 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "testing" "time" @@ -63,8 +64,9 @@ func TestHandler_getDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - // No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory. - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // No account_id / account_ids filter → calculateCommitmentMetrics fetches the + // uncapped active set across all accounts via GetActivePurchaseHistory. + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -125,7 +127,7 @@ func TestHandler_getDashboardSummary_PerAccountCoverageScalesSavings(t *testing. mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{ Provider: "aws", Service: "rds", Enabled: true, Coverage: 100, }, nil) @@ -168,7 +170,7 @@ func TestHandler_getDashboardSummary_ZeroCoverageInServiceConfigFallsThroughToFu mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) // Global ServiceConfig has Coverage=0 (zero-value — operator never set it). mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{ Provider: "aws", Service: "rds", Enabled: true, Coverage: 0, @@ -843,7 +845,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { t.Run("no purchase history", func(t *testing.T) { mockStore := new(MockConfigStore) - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return([]config.PurchaseHistoryRecord{}, nil) handler := &Handler{config: mockStore} @@ -857,7 +859,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { t.Run("purchase history error returns zeros", func(t *testing.T) { mockStore := new(MockConfigStore) - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(nil, errors.New("db error")) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(nil, errors.New("db error")) handler := &Handler{config: mockStore} @@ -883,7 +885,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(purchases, nil) handler := &Handler{config: mockStore} @@ -910,7 +912,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(purchases, nil) handler := &Handler{config: mockStore} @@ -937,7 +939,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(purchases, nil) handler := &Handler{config: mockStore} @@ -970,7 +972,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { // Status "" = completed DB row — must be counted }, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(purchases, nil) handler := &Handler{config: mockStore} @@ -983,10 +985,10 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { "failed commitment's savings must not appear in committedMonthly") }) - // Multi-account scope: a UUID-filtered request must use GetPurchaseHistoryFiltered + // Multi-account scope: a UUID-filtered request must use GetActivePurchaseHistory // scoped to the supplied cloud_account_id UUIDs. Rows from account C must NOT // appear when the filter contains only A and B. - t.Run("multi-account UUID filter routes to GetPurchaseHistoryFiltered", func(t *testing.T) { + t.Run("multi-account UUID filter routes to GetActivePurchaseHistory", func(t *testing.T) { mockStore := new(MockConfigStore) purchaseTime := time.Now().AddDate(0, -2, 0) @@ -1000,7 +1002,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { {CloudAccountID: &accountBID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 150.0}, } uuids := []string{accountAID, accountBID} - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: uuids, Limit: 1000}). + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), uuids, map[string][]string(nil)). Return(purchasesAB, nil) // GetPurchaseHistory must not be called — no On registration so it // would panic if accidentally invoked. @@ -1027,11 +1029,10 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { externalOnly := []config.PurchaseHistoryRecord{ {AccountID: "999988887777", Timestamp: purchaseTime, Term: 1, EstimatedSavings: 175.0}, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ - AccountIDs: []string{"bbbbbbbb-1111-2222-3333-444444444444"}, - ExternalIDsByProvider: map[string][]string{"aws": {"999988887777"}}, - Limit: 1000, - }).Return(externalOnly, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string{"bbbbbbbb-1111-2222-3333-444444444444"}, + map[string][]string{"aws": {"999988887777"}}, + ).Return(externalOnly, nil) t.Cleanup(func() { mockStore.AssertExpectations(t) }) handler := &Handler{config: mockStore} @@ -1044,6 +1045,71 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }) } +// TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000 is the +// regression test for issue #1140 (review finding PERF-01): the dashboard KPI +// path used a newest-first 1000-row capped GetAllPurchaseHistory read and +// filtered "active" in Go, so once purchase_history exceeded 1000 rows the +// FIRST rows dropped were exactly the oldest still-active 1y/3y commitments, +// silently understating ActiveCommitments / CommittedMonthly. +// +// The data shape replicates the real failure: 1000 newer-but-expired rows plus +// one older still-active 3-year commitment that falls outside the newest-1000 +// window. The mocks emulate the store contract for each read: the capped +// all-history page returns only the newest 1000 rows (active row truncated +// away), while GetActivePurchaseHistory returns the complete SQL-filtered +// active set with no cap. Pre-fix the handler took the capped path and +// reported 0 active commitments; post-fix it must report the old commitment. +func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T) { + ctx := context.Background() + now := time.Now() + + // Oldest row: 3-year commitment purchased ~2.5 years ago, still active. + oldActive := config.PurchaseHistoryRecord{ + AccountID: "111122223333", + PurchaseID: "p-old-active", + Provider: "aws", + Service: "ec2", + Term: 3, + Timestamp: now.AddDate(0, -30, 0), + EstimatedSavings: 120.0, + } + // 1000 newer rows: 1-year commitments purchased ~2 years ago, all expired. + newestFirst := make([]config.PurchaseHistoryRecord, 0, 1000) + for i := 999; i >= 0; i-- { + newestFirst = append(newestFirst, config.PurchaseHistoryRecord{ + AccountID: "111122223333", + PurchaseID: fmt.Sprintf("p-expired-%04d", i), + Provider: "aws", + Service: "ec2", + Term: 1, + Timestamp: now.AddDate(-2, 0, 0).Add(time.Duration(i) * time.Minute), + EstimatedSavings: 10.0, + }) + } + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + // Old capped read (documentation-only): ORDER BY timestamp DESC LIMIT 1000 + // keeps only the 1000 newer (expired) rows; the still-active oldest row is + // truncated away. Marked Maybe() so AssertExpectations does not require it + // to be called — the whole point of the test is that it must NOT be called. + mockStore.On("GetAllPurchaseHistory", ctx, 1000).Return(newestFirst, nil).Maybe() + // SQL active-only read: no cap, returns exactly the live commitments. + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). + Return([]config.PurchaseHistoryRecord{oldActive}, nil) + + handler := &Handler{config: mockStore} + + activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, nil) + + assert.Equal(t, 1, activeCommitments, + "the still-active 3y commitment beyond the old 1000-row cap must be counted") + assert.Equal(t, 120.0, committedMonthly, + "CommittedMonthly must include the still-active commitment dropped by the old capped read") + assert.InDelta(t, 120.0, savingsByService["ec2"], 0.001) + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) +} + // TestAggregateActiveCommitmentsPerService covers the core primitive used by // both the KPI total and the per-service chart CurrentSavings. func TestAggregateActiveCommitmentsPerService(t *testing.T) { @@ -1136,8 +1202,8 @@ func TestHandler_getDashboardSummary_CurrentSavingsPopulated(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) // No account_id / account_ids filter, so calculateCommitmentMetrics fetches - // across all accounts via GetAllPurchaseHistory. - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil) + // across all accounts via GetActivePurchaseHistory (uncapped, active-only). + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return(purchases, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -1180,8 +1246,9 @@ func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return( []config.RecommendationRecord{{Service: "EC2", Savings: 400.0}}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - // No account filter, so the no-filter fetch path (GetAllPurchaseHistory) runs. - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil) + // No account filter, so the all-accounts fetch path (GetActivePurchaseHistory + // with an empty scope) runs. + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return(purchases, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -1247,7 +1314,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) mockStore.On("GetGlobalConfig", ctx).Return(nil, nil) - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -1271,7 +1338,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index f9bb622f2..7be592b11 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -38,7 +38,8 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF return nil, err } - purchases, err := h.fetchCommitmentRecords(ctx, params) + now := time.Now() + purchases, err := h.fetchCommitmentRecords(ctx, now, session, params) if err != nil { return nil, err } @@ -49,7 +50,6 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF } nameByID := h.resolveAccountNamesByID(ctx) - now := time.Now() commitments := make([]InventoryCommitment, 0, len(purchases)) for _, p := range purchases { @@ -70,12 +70,14 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF return InventoryCommitmentsResponse{Commitments: commitments}, nil } -// fetchCommitmentRecords reads purchase history from the store, honoring -// optional `account_id` and `provider` query params the same way -// fetchPurchaseHistory does for /api/history. Limit defaults to -// MaxListLimit — commitments are a strict subset of purchase history (we -// drop expired rows before returning) so a high cap is appropriate; an -// over-truncation here would silently hide rows the user is entitled to. +// fetchCommitmentRecords reads the active purchase_history rows from the +// store, honouring optional `account_id` and `provider` query params the same +// way fetchPurchaseHistory does for /api/history. The read goes through +// GetActivePurchaseHistory so the active filter runs in SQL with no row cap: +// a newest-first LIMIT page dropped exactly the oldest still-active 1y/3y +// commitments once history exceeded the cap, silently hiding rows the user is +// entitled to (issue #1140). The result is bounded by the number of live +// commitments, not by all history ever recorded. // // The singular `account_id` (a top-bar chip cloud_accounts UUID for current // callers, or a raw external number for legacy ones) is resolved to the @@ -84,21 +86,39 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF // #701/#498/#866 bug was that passing the UUID to the single-column // GetPurchaseHistory (account_id) matched nothing. // +// When no explicit `account_id` is supplied, a restricted session's +// allowed_accounts scope is pushed into the SQL read via +// resolveAllowedAccountScope, the same contract as the dashboard KPI path +// (issue #956): without it the store would read every tenant's active +// commitments and only trim them in memory afterwards. A restricted session +// whose allowed_accounts match no account resolves to the non-nil-but-empty +// sentinel and short-circuits to an empty result without querying (an empty +// scope sent to the store would match ALL rows, not none). Unrestricted / +// admin sessions resolve to a nil scope and keep the all-accounts read. The +// in-memory filterPurchaseHistoryByAllowedAccounts in the callers still runs +// afterwards; it is what trims an explicit account_id outside the session's +// scope and serves as defence in depth for the no-param path. +// // `provider` filtering is applied in-memory after the store read so the // record set is small enough that a post-read filter has negligible cost. -func (h *Handler) fetchCommitmentRecords(ctx context.Context, params map[string]string) ([]config.PurchaseHistoryRecord, error) { - var rows []config.PurchaseHistoryRecord - var err error +func (h *Handler) fetchCommitmentRecords(ctx context.Context, asOf time.Time, session *Session, params map[string]string) ([]config.PurchaseHistoryRecord, error) { + var uuids []string + var externalIDsByProvider map[string][]string if accountID := params["account_id"]; accountID != "" { - uuids, externalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) - rows, err = h.config.GetPurchaseHistoryFiltered(ctx, config.PurchaseHistoryFilter{ - AccountIDs: uuids, - ExternalIDsByProvider: externalIDsByProvider, - Limit: config.MaxListLimit, - }) + uuids, externalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, accountID) } else { - rows, err = h.config.GetAllPurchaseHistory(ctx, config.MaxListLimit) + var err error + uuids, externalIDsByProvider, err = h.resolveAllowedAccountScope(ctx, session) + if err != nil { + return nil, err + } + // Non-nil empty scope: restricted session with zero accessible + // accounts. Querying with it would match every row, so short-circuit. + if uuids != nil && len(uuids) == 0 && len(externalIDsByProvider) == 0 { + return nil, nil + } } + rows, err := h.config.GetActivePurchaseHistory(ctx, asOf, uuids, externalIDsByProvider) if err != nil { return nil, err } @@ -167,7 +187,8 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu } // --- covered: active commitments ---------------------------------------- - purchases, err := h.fetchCommitmentRecords(ctx, params) + now := time.Now() + purchases, err := h.fetchCommitmentRecords(ctx, now, session, params) if err != nil { return nil, err } @@ -176,7 +197,6 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu return nil, err } - now := time.Now() // coveredByKey accumulates the effective covered monthly spend by // "provider:service". A commitment's covered monthly is its recurring // MonthlyCost plus the amortized upfront, so an all-upfront commitment diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 7cc81a961..32bea014a 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -2,12 +2,14 @@ package api import ( "context" + "fmt" "testing" "time" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -35,7 +37,7 @@ func TestHandler_listActiveCommitments_Empty(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{}, nil } @@ -92,7 +94,7 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { }, } - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) // Use realistic fixtures: CloudAccount.ID is a UUID; CloudAccount.ExternalID is // the provider external ID (e.g. AWS account number) that matches // PurchaseHistoryRecord.AccountID. The name lookup in resolveAccountNamesByID @@ -131,8 +133,9 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { } // TestHandler_listActiveCommitments_AccountFilter verifies the account_id -// query param routes through the dual-column GetPurchaseHistoryFiltered -// instead of GetAllPurchaseHistory, and the response respects the filter. +// query param routes through the dual-column account scope of +// GetActivePurchaseHistory instead of an unscoped read, and the response +// respects the filter. // "acc-1" is a known cloud_accounts UUID (no external id in this fixture), so // it is matched on cloud_account_id only (issue #701/#498/#866). func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { @@ -154,7 +157,7 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { }, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{"acc-1"}, Limit: config.MaxListLimit}).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{"acc-1"}, map[string][]string(nil)).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil } @@ -206,7 +209,7 @@ func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { }, } - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil } @@ -267,7 +270,7 @@ func TestHandler_listActiveCommitments_SortedByExpiry(t *testing.T) { }, } - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil } @@ -443,7 +446,7 @@ func TestHandler_getCoverageBreakdown_Integration(t *testing.T) { {Provider: "aws", Service: "ec2", Savings: 350.0}, } - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{}, nil } @@ -530,7 +533,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { {Provider: "azure", Service: "compute", Savings: 999.0, CloudAccountID: strPtr("acc-1")}, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{"acc-1"}, Limit: config.MaxListLimit}).Return(acc1Purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{"acc-1"}, map[string][]string(nil)).Return(acc1Purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{ {ID: "acc-1", Name: "Account One"}, @@ -643,7 +646,7 @@ func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { "dashboard must count the active Azure commitment (EstimatedSavings)") // --- Consistency leg 2: the Coverage tab must reflect the same commitment --- - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{}, nil } @@ -688,3 +691,79 @@ func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { require.NotNil(t, azure.OverallCoveragePct) assert.InDelta(t, 100.0, *azure.OverallCoveragePct, 0.001) } + +// TestHandler_listActiveCommitments_NoTruncationBeyond1000 is the regression +// test for issue #1140 (review finding PERF-01) on the inventory endpoint: +// /api/inventory/commitments used a newest-first MaxListLimit-capped +// GetAllPurchaseHistory read and filtered "active" in Go, so once +// purchase_history exceeded the cap, the FIRST rows dropped were exactly the +// oldest still-active 1y/3y commitments and they silently vanished from the +// renew-next list (and from /api/inventory/coverage, which shares +// fetchCommitmentRecords). +// +// The data shape replicates the real failure: 1000 newer-but-expired rows plus +// one older still-active 3-year commitment outside the newest-1000 window. The +// mocks emulate the store contract for each read: the capped all-history page +// returns only the newest 1000 rows (active row truncated away), while +// GetActivePurchaseHistory returns the complete SQL-filtered active set with +// no cap. Pre-fix the handler took the capped path and returned an empty list; +// post-fix the old commitment must be present. +func TestHandler_listActiveCommitments_NoTruncationBeyond1000(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + now := time.Now() + + // Oldest row: 3-year commitment purchased ~2.5 years ago, still active. + oldActive := config.PurchaseHistoryRecord{ + AccountID: "111122223333", + PurchaseID: "p-old-active", + Provider: "aws", + Service: "ec2", + Term: 3, + Count: 1, + Timestamp: now.AddDate(0, -30, 0), + MonthlyCost: float64Ptr(80.0), + EstimatedSavings: 120.0, + } + // 1000 newer rows: 1-year commitments purchased ~2 years ago, all expired. + newestFirst := make([]config.PurchaseHistoryRecord, 0, config.MaxListLimit) + for i := config.MaxListLimit - 1; i >= 0; i-- { + newestFirst = append(newestFirst, config.PurchaseHistoryRecord{ + AccountID: "111122223333", + PurchaseID: fmt.Sprintf("p-expired-%04d", i), + Provider: "aws", + Service: "ec2", + Term: 1, + Count: 1, + Timestamp: now.AddDate(-2, 0, 0).Add(time.Duration(i) * time.Minute), + EstimatedSavings: 10.0, + }) + } + + // Old capped read (documentation-only): ORDER BY timestamp DESC LIMIT + // MaxListLimit kept only the newer (expired) rows; the still-active oldest + // row was truncated away. Maybe() so AssertExpectations does not require it + // to be called — the whole point of the test is that it must NOT be called + // post-fix. AssertNotCalled below pins that explicitly. + mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(newestFirst, nil).Maybe() + // SQL active-only read: no cap, returns exactly the live commitments. + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). + Return([]config.PurchaseHistoryRecord{oldActive}, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{}, nil + } + + mockAuth, req := adminInventoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.listActiveCommitments(ctx, req, map[string]string{}) + require.NoError(t, err) + + resp := result.(InventoryCommitmentsResponse) + require.Len(t, resp.Commitments, 1, + "the still-active 3y commitment beyond the old row cap must be listed") + assert.Equal(t, "111122223333:p-old-active", resp.Commitments[0].ID) + assert.Equal(t, 120.0, resp.Commitments[0].EstimatedSavings) + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) +} diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index e70c1947b..7eb59923e 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -503,12 +503,10 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return((*config.ServiceConfig)(nil), nil) // Issue #956: a restricted session with no explicit account filter scopes the // commitment metrics to its allowed_accounts (permsAccA), so the handler calls - // GetPurchaseHistoryFiltered scoped to account A — NOT GetAllPurchaseHistory. + // GetActivePurchaseHistory scoped to account A — NOT an unscoped all-accounts read. // permsAccA has no ExternalID, so only the UUID half of the filter is set. - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ - AccountIDs: []string{permsAccA}, - Limit: 1000, - }).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string{permsAccA}, map[string][]string(nil)).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } @@ -530,6 +528,10 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing // Issue #956 regression: the all-accounts fast path must NOT be taken for a // restricted session, or commitment KPIs would leak other accounts' data. mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) + // Positively assert the scoped read happened: without this the test could + // pass if commitment metrics were skipped entirely. + mockStore.AssertCalled(t, "GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string{permsAccA}, map[string][]string(nil)) } // TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts is @@ -554,10 +556,8 @@ func TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts( mockStore := new(MockConfigStore) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) - mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ - AccountIDs: []string{permsAccA}, - Limit: 1000, - }).Return(accountARows, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string{permsAccA}, map[string][]string(nil)).Return(accountARows, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } @@ -805,6 +805,12 @@ func TestPerAccountPerms_PlannedPurchase_CrossAccountPlanRejected404(t *testing. // getCoverageBreakdown's recommendations leg, the account-B savings (200) // pollute onDemandByKey and the aws:ec2 on_demand_monthly in the response rises // from 200 to 400, lowering the coverage% from 50% to 33%. +// +// The commitments leg must also scope the SQL read itself: with no explicit +// account_id, fetchCommitmentRecords resolves the session's allowed_accounts +// (permsAccA) and passes them to GetActivePurchaseHistory, NOT an unscoped +// nil/nil read that pulls every tenant's rows into memory first (issue #956 +// contract, PR #1221 review). The mock expectation pins the scoped args. func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *testing.T) { ctx := context.Background() @@ -842,11 +848,15 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test Return([]config.RecommendationRecord{recA, recB}, nil) mockStore := new(MockConfigStore) - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit). + // Scoped session, no account_id param: the store read must already be + // scoped to the allowed account's UUID (permsAccA has no ExternalID in the + // fixture, so only the UUID half of the dual-column filter is set). + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{permsAccA}, map[string][]string(nil)). Return([]config.PurchaseHistoryRecord{purchaseA}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } + t.Cleanup(func() { mockStore.AssertExpectations(t) }) handler := &Handler{ auth: scopedAuthMock(ctx), @@ -913,7 +923,7 @@ func TestPerAccountPerms_CoverageBreakdown_AdminSeesAll(t *testing.T) { Return([]config.RecommendationRecord{recA, recB}, nil) mockStore := new(MockConfigStore) - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit). + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). Return([]config.PurchaseHistoryRecord{purchaseA}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil @@ -949,6 +959,104 @@ func TestPerAccountPerms_CoverageBreakdown_AdminSeesAll(t *testing.T) { "admin coverage = 200/(200+400) * 100 = 33.3%%") } +// ─── 9b. GET /inventory/commitments ────────────────────────────────────────── + +// TestPerAccountPerms_InventoryCommitments_ScopedSQLRead asserts that a +// restricted session with no explicit account_id scopes the active-commitments +// SQL read itself to its allowed_accounts: fetchCommitmentRecords must call +// GetActivePurchaseHistory with the resolved allowed-account scope +// ([permsAccA], nil; the fixture account has no ExternalID), never the +// unscoped nil/nil read that pulls every tenant's active commitments into +// memory before filterPurchaseHistoryByAllowedAccounts trims them (issue #956 +// contract, PR #1221 review). +// +// Regression: pre-fix the handler called GetActivePurchaseHistory(ctx, asOf, +// nil, nil) for this exact request shape; the pinned mock args make that call +// panic the test. +func TestPerAccountPerms_InventoryCommitments_ScopedSQLRead(t *testing.T) { + ctx := context.Background() + + now := time.Now() + purchaseA := config.PurchaseHistoryRecord{ + AccountID: permsAccA, + PurchaseID: "p-inv-a", + Provider: "aws", + Service: "ec2", + Timestamp: now.AddDate(0, -6, 0), + Term: 1, + Count: 1, + MonthlyCost: float64Ptr(80.0), + EstimatedSavings: 120.0, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{permsAccA}, map[string][]string(nil)). + Return([]config.PurchaseHistoryRecord{purchaseA}, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + } + + result, err := handler.listActiveCommitments(ctx, scopedReq(), map[string]string{}) + require.NoError(t, err) + + resp, ok := result.(InventoryCommitmentsResponse) + require.True(t, ok) + require.Len(t, resp.Commitments, 1, + "scoped user must see exactly the allowed account's commitment") + assert.Equal(t, permsAccA+":p-inv-a", resp.Commitments[0].ID) +} + +// TestPerAccountPerms_InventoryCommitments_ZeroAllowedAccountsNoQuery asserts +// the zero-account sentinel: a restricted session whose allowed_accounts match +// no cloud account must get an empty commitments list WITHOUT the store being +// queried at all: an empty scope passed to GetActivePurchaseHistory would +// emit no account predicate and match every tenant's rows (the same +// short-circuit fetchCommitmentPurchases applies on the dashboard path). +func TestPerAccountPerms_InventoryCommitments_ZeroAllowedAccountsNoQuery(t *testing.T) { + ctx := context.Background() + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, permsScopedToken).Return(&Session{ + UserID: permsScopedUserID, + Email: "scoped@example.com", + }, nil) + mockAuth.On("HasPermissionAPI", ctx, permsScopedUserID, mock.Anything, mock.Anything).Return(true, nil) + // Allowed entry matches neither fixture account, so the resolved scope is + // the non-nil-but-empty sentinel. + mockAuth.On("GetAllowedAccountsAPI", ctx, permsScopedUserID). + Return([]string{"dddddddd-dddd-4ddd-dddd-dddddddddddd"}, nil) + + mockStore := new(MockConfigStore) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + }) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: mockAuth, + config: mockStore, + } + + result, err := handler.listActiveCommitments(ctx, scopedReq(), map[string]string{}) + require.NoError(t, err) + + resp, ok := result.(InventoryCommitmentsResponse) + require.True(t, ok) + assert.Empty(t, resp.Commitments, + "zero accessible accounts must yield an empty list, not all-accounts data") + mockStore.AssertNotCalled(t, "GetActivePurchaseHistory", + mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + // TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds is the paired // positive case confirming the filter passes for account-A plans. func TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds(t *testing.T) { diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index a3449592d..bd60503e3 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -920,8 +920,9 @@ func TestHandler_HandleRequest_GetDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - // No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory. - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // No account_id / account_ids filter → calculateCommitmentMetrics fetches the + // uncapped active set across all accounts via GetActivePurchaseHistory. + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) handler := &Handler{ scheduler: mockScheduler, diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 1c1067ef8..4f6a79d35 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -130,13 +130,20 @@ type StoreInterface interface { GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error) GetAllPurchaseHistory(ctx context.Context, limit int) ([]PurchaseHistoryRecord, error) // GetActivePurchaseHistory returns every purchase_history row whose commitment - // is still within its term at asOf (term > 0 AND timestamp + term years > asOf), - // across all accounts, newest-first. Unlike GetAllPurchaseHistory it is not - // row-capped: the analytics collector needs the complete active set, and - // filtering expired commitments in SQL keeps the result bounded by the number - // of live commitments rather than by all history ever recorded (so it cannot - // silently truncate older-but-still-active 1y/3y commitments). - GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]PurchaseHistoryRecord, error) + // is still within its term at asOf (term > 0 AND timestamp + term years >= asOf; + // the expiry boundary is inclusive, matching the API layer's isActiveCommitment), + // newest-first, optionally scoped to a set of accounts. The account scope uses + // the same dual-column predicate as GetPurchaseHistoryFiltered: accountIDs + // match cloud_account_id (cloud_accounts UUIDs) and externalIDsByProvider + // matches account_id per provider, OR'd together so rows carrying only one + // identifier are still returned (issues #701/#498/#866). Both empty means all + // accounts. Unlike GetAllPurchaseHistory it is not row-capped: the analytics + // collector, dashboard KPIs, and inventory endpoints need the complete active + // set, and filtering expired commitments in SQL keeps the result bounded by + // the number of live commitments rather than by all history ever recorded (so + // it cannot silently truncate older-but-still-active 1y/3y commitments the + // way a newest-first capped page does, issue #1140). + GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]PurchaseHistoryRecord, error) // GetPurchaseHistoryFiltered reads purchase_history rows matching the // PurchaseHistoryFilter, newest-first, capped at filter.Limit. Each field is // applied independently and only when populated (see PurchaseHistoryFilter). diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 6f56d7552..2f92b1b0f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1687,24 +1687,41 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([ } // GetActivePurchaseHistory retrieves every purchase_history row still within its -// commitment term at asOf, across all accounts. The active filter is pushed into -// SQL so the result is bounded by the number of live commitments (not by all -// history ever recorded), which is what the analytics collector needs and avoids -// silently truncating older-but-still-active 1y/3y commitments. term*8760 hours -// matches the collector's HoursPerYear (365*24) so the SQL and Go term windows -// agree. -func (s *PostgresStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]PurchaseHistoryRecord, error) { - query := ` +// commitment term at asOf, optionally scoped to a set of accounts via the same +// dual-column predicate as GetPurchaseHistoryFiltered (appendAccountPredicate): +// both accountIDs (cloud_accounts UUIDs matched on cloud_account_id) and +// externalIDsByProvider (provider-scoped external numbers matched on +// account_id) are applied with OR semantics so rows carrying only one +// identifier are still returned (issues #701/#498/#866). Both empty means all +// accounts. The active filter is pushed into SQL so the result is bounded by +// the number of live commitments (not by all history ever recorded), which is +// what the analytics collector, dashboard KPIs, and inventory endpoints need: +// it cannot silently truncate older-but-still-active 1y/3y commitments the way +// a newest-first LIMIT page does (issue #1140). term*8760 hours matches the +// collector's HoursPerYear and the API layer's commitmentExpiry (both 365*24) +// so the SQL and Go term windows agree. The expiry comparison is inclusive +// (expiry >= asOf): a commitment expiring exactly at asOf is still active, +// matching the API layer's isActiveCommitment (!now.After(expiry)) so the SQL +// result set and the Go-side active checks share one boundary definition. +func (s *PostgresStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]PurchaseHistoryRecord, error) { + conds := []string{ + "term > 0", + "timestamp + make_interval(hours => term * 8760) >= $1", + } + args := []any{asOf} + conds, args = appendAccountPredicate(conds, args, accountIDs, externalIDsByProvider) + + query := fmt.Sprintf(` SELECT account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, - estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id + estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, + revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history - WHERE term > 0 - AND timestamp + make_interval(hours => term * 8760) > $1 + WHERE %s ORDER BY timestamp DESC - ` + `, strings.Join(conds, " AND ")) - return s.queryPurchaseHistory(ctx, query, asOf) + return s.queryPurchaseHistory(ctx, query, args...) } // appendAccountPredicate pulls the dual-column account-predicate arg-building diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index cebe69d08..727b8017c 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -896,6 +896,59 @@ func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } +// ─── GetActivePurchaseHistory (issue #1140) ────────────────────────────────── + +// TestPGXMock_GetActivePurchaseHistory_Unscoped asserts the emitted SQL pushes +// the active filter into the WHERE clause with NO LIMIT (the `$` anchor after +// ORDER BY proves no trailing LIMIT clause), so the result is bounded by the +// number of live commitments and a newest-first row cap can never silently +// drop the oldest still-active 1y/3y commitments (issue #1140). The pinned +// `>= $1` expiry comparison is inclusive so a commitment expiring exactly at +// asOf stays active, matching the API layer's isActiveCommitment. It also pins +// the full 21-column SELECT including the issue-#290 revocation columns: +// before this fix the query selected only 17 columns while +// queryPurchaseHistory scans 21 destinations, so every call failed at Scan. +func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "acct-1")...) + mock.ExpectQuery( + `SELECT account_id, purchase_id, .*revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 ORDER BY timestamp DESC$`, + ).WithArgs(now).WillReturnRows(rows) + + records, err := store.GetActivePurchaseHistory(ctx, now, nil, nil) + require.NoError(t, err) + require.Len(t, records, 1) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetActivePurchaseHistory_AccountScoped asserts the dual-column +// account predicate (same shape as GetPurchaseHistoryFiltered, issues +// #701/#498/#866) composes with the active filter, again with no LIMIT, so the +// dashboard KPI path and the inventory endpoints get the complete active set +// for the selected account scope (issue #1140). +func TestPGXMock_GetActivePurchaseHistory_AccountScoped(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "111122223333")...) + mock.ExpectQuery( + `FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) ORDER BY timestamp DESC$`, + ).WithArgs(now, []string{"acct-uuid-1"}, "aws", []string{"111122223333"}).WillReturnRows(rows) + + records, err := store.GetActivePurchaseHistory(ctx, now, + []string{"acct-uuid-1"}, map[string][]string{"aws": {"111122223333"}}) + require.NoError(t, err) + require.Len(t, records, 1) + assert.Equal(t, "111122223333", records[0].AccountID) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // TestPGXMock_GetPurchaseHistoryFiltered_PartialFilters asserts that // supplying only a subset of filters (here: provider + start, no account ids, // no end) emits exactly two AND clauses and binds the right positional diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 9339192ae..e419d0680 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -369,8 +369,8 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) } // GetActivePurchaseHistory mocks the GetActivePurchaseHistory operation -func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { - args := m.Called(ctx, asOf) +func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, asOf, accountIDs, externalIDsByProvider) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 3f4301321..cc162650b 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -103,7 +103,7 @@ func (m *mockConfigStoreForHealth) GetAllPurchaseHistory(ctx context.Context, li return nil, nil } -func (m *mockConfigStoreForHealth) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { +func (m *mockConfigStoreForHealth) GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { return nil, nil }