Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitallowed
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
3 changes: 2 additions & 1 deletion internal/analytics/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
34 changes: 17 additions & 17 deletions internal/analytics/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand All @@ -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
},
}
Expand Down Expand Up @@ -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
},
}
Expand All @@ -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
},
}
Expand Down
47 changes: 17 additions & 30 deletions internal/api/handler_dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
Loading
Loading