From b0233d38e678583e11a1c986277b8e0df8cb8dae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 22:15:03 -0700 Subject: [PATCH 1/6] fix(api): compute active commitments via uncapped SQL active-only read The dashboard KPI path (fetchCommitmentPurchases) and the inventory commitments/coverage endpoints (fetchCommitmentRecords) read purchase history through a newest-first 1000-row capped fetch (GetAllPurchaseHistory / GetPurchaseHistoryFiltered with Limit 1000) and filtered "active" in Go. Once purchase_history exceeded the cap, the first rows dropped were exactly the oldest still-active 1y/3y commitments, silently understating the active-commitment count, committed-monthly KPI, YTD savings, per-service chart, the renew-next list, and the coverage breakdown, with no error or banner. Reuse the store's SQL active-only primitive instead: extend GetActivePurchaseHistory with the same dual-column account predicate as GetPurchaseHistoryFiltered (cloud_account_id UUIDs OR provider plus external account_id, issues #701/#498/#866) via the shared appendAccountPredicate helper, and route the dashboard KPI path and both inventory endpoints through it. The active filter and account scope now run in SQL with no LIMIT, so the result is bounded by the number of live commitments rather than by all history ever recorded. The Go-side isActiveCommitment gate stays as the status filter. Also fixes a latent Scan failure found while extending the query: GetActivePurchaseHistory selected 17 columns while queryPurchaseHistory scans 21 destinations (the issue-#290 revocation columns were missing), so every call failed at Scan. The pgxmock test now pins the full 21-column SELECT and the absence of a LIMIT clause. Regression tests replicate the real failure shape (1000 newer expired rows plus one older still-active 3y commitment outside the newest-1000 window) for both the dashboard KPI path and the inventory endpoint, and fail against the pre-fix handler code. Closes #1140 --- internal/analytics/collector.go | 3 +- internal/analytics/collector_test.go | 34 +++--- internal/api/handler_dashboard.go | 44 +++---- internal/api/handler_dashboard_test.go | 112 ++++++++++++++---- internal/api/handler_inventory.go | 38 +++--- internal/api/handler_inventory_test.go | 95 +++++++++++++-- .../api/handler_per_account_perms_test.go | 18 ++- internal/api/handler_test.go | 5 +- internal/config/interfaces.go | 18 ++- internal/config/store_postgres.go | 40 +++++-- .../config/store_postgres_pgxmock_test.go | 51 ++++++++ internal/mocks/stores.go | 4 +- internal/server/test_helpers_test.go | 2 +- 13 files changed, 327 insertions(+), 137 deletions(-) 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..ef1a04bd5 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -583,43 +583,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 +629,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..b64a48109 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,68 @@ 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) + // Old capped read: ORDER BY timestamp DESC LIMIT 1000 keeps only the 1000 + // newer (expired) rows; the still-active oldest row is truncated away. + mockStore.On("GetAllPurchaseHistory", ctx, 1000).Return(newestFirst, nil) + // 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 +1199,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 +1243,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 +1311,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 +1335,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..0e9f14fa6 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, 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 @@ -86,19 +88,13 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF // // `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, 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, - }) - } else { - rows, err = h.config.GetAllPurchaseHistory(ctx, config.MaxListLimit) + uuids, externalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, accountID) } + rows, err := h.config.GetActivePurchaseHistory(ctx, asOf, uuids, externalIDsByProvider) if err != nil { return nil, err } @@ -167,7 +163,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, params) if err != nil { return nil, err } @@ -176,7 +173,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..5ea0bfc6d 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,75 @@ 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) + 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: ORDER BY timestamp DESC LIMIT MaxListLimit keeps only + // the newer (expired) rows; the still-active oldest row is truncated away. + mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(newestFirst, nil) + // 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..40222c638 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 } @@ -554,10 +552,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 } @@ -842,7 +838,7 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test 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 @@ -913,7 +909,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 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..ed0466d25 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -131,12 +131,18 @@ type StoreInterface interface { 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) + // 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..d7ec84843 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1687,24 +1687,38 @@ 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. +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..a498733a1 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -896,6 +896,57 @@ 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). 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 } From b4f6e5845dd387b06e868634505aa5810810fda2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 23:44:02 -0700 Subject: [PATCH 2/6] fix(api): scope inventory SQL read to session allowed accounts For a restricted session with no explicit account_id chip, the inventory endpoints (/api/inventory/commitments and /coverage) still called GetActivePurchaseHistory(ctx, asOf, nil, nil), reading every tenant's active commitments and only trimming them in memory via filterPurchaseHistoryByAllowedAccounts. Thread the session into fetchCommitmentRecords and resolve the allowed-account scope through resolveAllowedAccountScope (same issue #956 contract as the dashboard KPI path), including the zero-accessible-accounts sentinel that short-circuits without querying. The in-memory filter stays as defence in depth and still trims an explicit out-of-scope account_id. Regression tests pin the scoped store args for both endpoints and the no-query sentinel path; they fail on the pre-fix handler (unexpected unscoped mock call) and pass post-fix. Addresses CodeRabbit review on PR #1221 (handler_inventory.go). --- internal/api/handler_dashboard.go | 3 +- internal/api/handler_inventory.go | 30 ++++- .../api/handler_per_account_perms_test.go | 106 +++++++++++++++++- 3 files changed, 134 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index ef1a04bd5..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). diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 0e9f14fa6..7be592b11 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -39,7 +39,7 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF } now := time.Now() - purchases, err := h.fetchCommitmentRecords(ctx, now, params) + purchases, err := h.fetchCommitmentRecords(ctx, now, session, params) if err != nil { return nil, err } @@ -86,13 +86,37 @@ 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, asOf time.Time, params map[string]string) ([]config.PurchaseHistoryRecord, 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) + } else { + 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 { @@ -164,7 +188,7 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu // --- covered: active commitments ---------------------------------------- now := time.Now() - purchases, err := h.fetchCommitmentRecords(ctx, now, params) + purchases, err := h.fetchCommitmentRecords(ctx, now, session, params) if err != nil { return nil, err } diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 40222c638..3520cc792 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -801,6 +801,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() @@ -838,11 +844,15 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test Return([]config.RecommendationRecord{recA, recB}, nil) mockStore := new(MockConfigStore) - mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). + // 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), @@ -945,6 +955,100 @@ 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) + 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) { From 6340051b5a69d5ad9505c324416646f894bcd24a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 23:44:18 -0700 Subject: [PATCH 3/6] fix(config): make SQL active-commitment expiry boundary inclusive GetActivePurchaseHistory excluded a commitment expiring exactly at asOf (`> $1`) while the API layer's isActiveCommitment treats that instant as still active (!now.After(expiry)). Align the SQL predicate to `>= $1` so both sides share one boundary definition, and document the inclusive contract in the store and interface docstrings. The pgxmock tests pin the new predicate. Addresses CodeRabbit review on PR #1221 (store_postgres.go). --- internal/config/interfaces.go | 3 ++- internal/config/store_postgres.go | 7 +++++-- internal/config/store_postgres_pgxmock_test.go | 8 +++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index ed0466d25..4f6a79d35 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -130,7 +130,8 @@ 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), + // 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 diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index d7ec84843..2f92b1b0f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1699,11 +1699,14 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([ // 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. +// 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", + "timestamp + make_interval(hours => term * 8760) >= $1", } args := []any{asOf} conds, args = appendAccountPredicate(conds, args, accountIDs, externalIDsByProvider) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index a498733a1..727b8017c 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -902,7 +902,9 @@ func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) { // 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). It also pins +// 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. @@ -914,7 +916,7 @@ func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) { 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$`, + `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) @@ -936,7 +938,7 @@ func TestPGXMock_GetActivePurchaseHistory_AccountScoped(t *testing.T) { 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$`, + `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, From e97f9a2ffeeee451731a7461c8abcad0dd0d174c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 17:16:04 +0200 Subject: [PATCH 4/6] test(api): add mock expectations guard to NoTruncation regression test The TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000 regression test registered GetAllPurchaseHistory with a bare On() call (documentation only -- the point of the test is that the old capped path must NOT be called), but without AssertExpectations the guarantee was one-sided: a future regression restoring the old path would be caught by AssertNotCalled, but the GetActivePurchaseHistory mock could silently never be called without being flagged. Add t.Cleanup(func(){ mockStore.AssertExpectations(t) }) so the mock framework enforces the GetActivePurchaseHistory call, and mark the documentation-only GetAllPurchaseHistory On() with .Maybe() so AssertExpectations does not treat its absence as a failure. --- internal/api/handler_dashboard_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index b64a48109..1ee7bbf7f 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1088,9 +1088,12 @@ func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T) } mockStore := new(MockConfigStore) - // Old capped read: ORDER BY timestamp DESC LIMIT 1000 keeps only the 1000 - // newer (expired) rows; the still-active oldest row is truncated away. - mockStore.On("GetAllPurchaseHistory", ctx, 1000).Return(newestFirst, nil) + 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) From ae756140516ec20856a0746335b465a089d2e99e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 17:46:56 +0200 Subject: [PATCH 5/6] test(api): pin AssertExpectations on new commitment regression tests Apply feedback_mock_assert_expectations to the two new regression tests added in this PR so a future store call slipping into the active-commitment path (or the zero-allowed-accounts short-circuit) cannot pass as a silent false green: - TestHandler_listActiveCommitments_NoTruncationBeyond1000 registers AssertExpectations cleanup, and the documentation-only GetAllPurchaseHistory expectation is now .Maybe() so the cleanup does not fail on the (expected) zero call count. The dashboard's parallel _NoTruncationBeyond1000 already pairs Maybe with cleanup; this brings the inventory test to the same shape. - TestPerAccountPerms_InventoryCommitments_ZeroAllowedAccountsNoQuery registers AssertExpectations on both mockStore and mockAuth so a stray store call (or a missing auth On registration) surfaces as a test failure. No behavior change; tests still pass. --- .gitallowed | 3 +++ internal/api/handler_inventory_test.go | 10 +++++++--- internal/api/handler_per_account_perms_test.go | 4 ++++ 3 files changed, 14 insertions(+), 3 deletions(-) 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/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 5ea0bfc6d..32bea014a 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -711,6 +711,7 @@ func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { 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. @@ -740,9 +741,12 @@ func TestHandler_listActiveCommitments_NoTruncationBeyond1000(t *testing.T) { }) } - // Old capped read: ORDER BY timestamp DESC LIMIT MaxListLimit keeps only - // the newer (expired) rows; the still-active oldest row is truncated away. - mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(newestFirst, nil) + // 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) diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 3520cc792..f7dcf361b 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -1029,6 +1029,10 @@ func TestPerAccountPerms_InventoryCommitments_ZeroAllowedAccountsNoQuery(t *test 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 } From c236d311e767b9576402e78703830f2cd2549b8c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 17:00:19 +0300 Subject: [PATCH 6/6] test(api): positively assert scoped GetActivePurchaseHistory call The AggregatesAllowedSubsetOnly test only asserted the absence of the unscoped GetAllPurchaseHistory call; it could pass if commitment metrics were skipped entirely. Assert the scoped call actually happened (CodeRabbit finding on #1221). --- internal/api/handler_per_account_perms_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index f7dcf361b..7eb59923e 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -528,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