Skip to content

fix(api): fail the dashboard summary on a purchase-history store error #1301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
IAmJSD wants to merge 1 commit into
base: main
Choose a base branch
from
+167 −50
Draft

fix(api): fail the dashboard summary on a purchase-history store error #1301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 35 additions & 22 deletions internal/api/handler_dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,14 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun

totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey)
targetCoverage := h.resolveTargetCoverage(ctx)
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider)
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService, err := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider)
if err != nil {
// Issue #72: purchase history is a hard dependency of the commitment
// KPIs, same as recommendations are of the savings KPIs above. Fail
// the request rather than ship $0 committed / 0 commitments, which
// would be indistinguishable from a real empty portfolio.
return nil, err
}

// Populate CurrentSavings on each per-service bucket from actual
// purchase_history commitments. Services with recommendations but no
Expand Down Expand Up @@ -617,12 +624,13 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco
// calculateCommitmentMetrics aggregates, applying the pre-resolved account
// scope and an optional provider filter. 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.
// pattern). Returns (nil, nil) for an explicit zero-account scope, so the
// caller emits genuinely-zero KPIs without querying, and a wrapped error on a
// store failure (issue #72: a failed read must never be reported as $0).
//
// - 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
// nothing). Returns (nil, nil) 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.
// - otherwise: GetActivePurchaseHistory with the dual-column account predicate
Expand All @@ -633,15 +641,14 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco
// - provider: when non-empty, only purchases for that provider are returned,
// mirroring fetchCommitmentRecords' in-memory provider filter so KPIs match
// the provider chip selection.
func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) {
func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) {
if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 {
return nil, false
return nil, nil
}

purchases, err := h.config.GetActivePurchaseHistory(ctx, asOf, accountUUIDs, accountExternalIDsByProvider)
if err != nil {
logging.Errorf("dashboard: failed to fetch commitment purchases; KPIs will be zeroed: %v", err)
return nil, false
return nil, fmt.Errorf("failed to get active purchase history: %w", err)
}

if provider != "" {
Expand All @@ -654,7 +661,7 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time,
purchases = filtered
}

return purchases, true
return purchases, nil
}

// calculateCommitmentMetrics aggregates active-commitment counts and monthly
Expand All @@ -677,11 +684,13 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time,
// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both
// this KPI path (committedMonthly) and the per-service chart use exactly the
// same gate.
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) {
//
// A store error is returned, never reported as zeroed metrics (issue #72).
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64, err error) {
currentTime := time.Now()
purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider)
if !ok {
return 0, 0, 0, nil
purchases, err := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider)
if err != nil {
return 0, 0, 0, nil, err
}

yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC)
Expand Down Expand Up @@ -717,7 +726,7 @@ func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider strin
ytdSavings += p.EstimatedSavings * float64(elapsedWholeMonths(countFrom, currentTime))
}

return activeCommitments, committedMonthly, ytdSavings, savingsByService
return activeCommitments, committedMonthly, ytdSavings, savingsByService, nil
}

// elapsedWholeMonths returns the number of complete calendar months between
Expand All @@ -736,16 +745,20 @@ func elapsedWholeMonths(from, to time.Time) int {
return months
}

// calculateCurrentCoverage calculates the current coverage percentage.
func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) float64 {
if potentialSavings == 0 {
return 100.0 // No recommendations means 100% coverage
}

// calculateCurrentCoverage returns committed / (committed + potential) as a
// percentage, or nil when both are zero and there is nothing to measure.
//
// It used to return a hard-coded 100% whenever potentialSavings was 0, even
// with nothing committed, so an empty or zeroed commitment half rendered as a
// reassuring "100% coverage, $0 committed" (issue #72). Committed savings with
// no remaining recommendations still compute to 100% through the ratio; only
// the 0/0 case is now reported as unknown (JSON null) rather than invented.
func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) *float64 {
totalPossible := potentialSavings + committedMonthly
if totalPossible == 0 {
return 0
return nil
}

return (committedMonthly / totalPossible) * 100
coverage := (committedMonthly / totalPossible) * 100
return &coverage
}
147 changes: 122 additions & 25 deletions internal/api/handler_dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,26 +852,26 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.NoError(t, err)

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
assert.Equal(t, 0.0, ytdSavings)
assert.Empty(t, savingsByService)
})

t.Run("purchase history error returns zeros", func(t *testing.T) {
// Issue #72: a store error must be returned, not reported as zeroed KPIs.
t.Run("purchase history error is returned, not zeroed", func(t *testing.T) {
mockStore := new(MockConfigStore)
mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(nil, errors.New("db error"))
storeErr := errors.New("db error")
mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(nil, storeErr)

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
assert.Equal(t, 0.0, ytdSavings)
assert.Nil(t, savingsByService)
_, _, _, _, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.Error(t, err)
assert.ErrorIs(t, err, storeErr)
})

t.Run("with active commitments", func(t *testing.T) {
Expand All @@ -892,7 +892,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 100.0, committedMonthly)
Expand All @@ -919,7 +920,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.NoError(t, err)

// Should skip expired commitments
assert.Equal(t, 0, activeCommitments)
Expand All @@ -946,7 +948,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 50.0, committedMonthly)
Expand Down Expand Up @@ -979,7 +982,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})
require.NoError(t, err)

// Only the status="" row counts; the failed row must be excluded.
assert.Equal(t, 1, activeCommitments,
Expand Down Expand Up @@ -1013,7 +1017,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids, nil)
activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics(ctx, "", uuids, nil)
require.NoError(t, err)

assert.Equal(t, 2, activeCommitments)
assert.Equal(t, 250.0, committedMonthly,
Expand All @@ -1040,8 +1045,9 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(
activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics(
ctx, "", []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}})
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments, "external-id-only commitment must be counted")
assert.Equal(t, 175.0, committedMonthly)
Expand Down Expand Up @@ -1103,7 +1109,8 @@ func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T)

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, nil)
activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, nil)
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments,
"the still-active 3y commitment beyond the old 1000-row cap must be counted")
Expand Down Expand Up @@ -1327,27 +1334,94 @@ func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *test
func TestHandler_calculateCurrentCoverage(t *testing.T) {
handler := &Handler{}

t.Run("no potential savings returns 100%", func(t *testing.T) {
t.Run("no potential savings with commitments returns 100%", func(t *testing.T) {
coverage := handler.calculateCurrentCoverage(0.0, 100.0)
assert.Equal(t, 100.0, coverage)
require.NotNil(t, coverage)
assert.Equal(t, 100.0, *coverage)
})

t.Run("no committed monthly", func(t *testing.T) {
coverage := handler.calculateCurrentCoverage(100.0, 0.0)
assert.Equal(t, 0.0, coverage)
require.NotNil(t, coverage)
assert.Equal(t, 0.0, *coverage)
})

t.Run("50% coverage", func(t *testing.T) {
coverage := handler.calculateCurrentCoverage(100.0, 100.0)
assert.Equal(t, 50.0, coverage)
require.NotNil(t, coverage)
assert.Equal(t, 50.0, *coverage)
})

t.Run("both zero returns 0%", func(t *testing.T) {
coverage := handler.calculateCurrentCoverage(0.0, 0.0)
assert.Equal(t, 100.0, coverage)
// Issue #72: with nothing committed and nothing left to buy there is no
// ratio to report. The old code returned a fabricated 100%.
t.Run("both zero returns nil, not 100%", func(t *testing.T) {
assert.Nil(t, handler.calculateCurrentCoverage(0.0, 0.0))
})
}

// TestHandleRequest_DashboardSummary_PurchaseHistoryErrorIs500 is the issue #72
// regression test, through the router so the status code and body are pinned.
// It uses the issue's compounding scenario: no recommendations (potential
// savings 0) plus a purchase-history store error. Before the fix this returned
// 200 with active_commitments 0, committed_monthly 0, ytd_savings 0 and
// current_coverage 100: a fabricated "fully covered" dashboard.
func TestHandleRequest_DashboardSummary_PurchaseHistoryErrorIs500(t *testing.T) {
ctx := context.Background()
mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)

mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil)
mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
mockStore.On("GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return(nil, errors.New("pq: connection refused to db-host-internal"))
t.Cleanup(func() { mockStore.AssertExpectations(t) })

handler := &Handler{
scheduler: mockScheduler,
config: mockStore,
corsAllowedOrigin: "*",
apiKey: "test-key",
}

resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{
Headers: map[string]string{"X-API-Key": "test-key"},
RequestContext: events.LambdaFunctionURLRequestContext{
HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{
Method: "GET",
Path: "/api/dashboard/summary",
},
},
})
require.NoError(t, err)
assert.Equal(t, 500, resp.StatusCode, "a failed purchase-history read must not ship zeroed KPIs as 200: %s", resp.Body)
assert.NotContains(t, resp.Body, "active_commitments", "no KPI payload on failure")
assert.NotContains(t, resp.Body, "db-host-internal", "store error detail must not leak to the client")
}

// TestGetDashboardSummary_NothingToMeasure_CoverageIsNull pins the JSON shape
// of the 0/0 coverage case (issue #72): current_coverage is an explicit null,
// which the dashboard renders as "--", never 100.
func TestGetDashboardSummary_NothingToMeasure_CoverageIsNull(t *testing.T) {
ctx := context.Background()
mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)

mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{auth: mockAuth, scheduler: mockScheduler, config: mockStore}

result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
require.NoError(t, err)
assert.Nil(t, result.CurrentCoverage)

body, err := json.Marshal(result)
require.NoError(t, err)
assert.Contains(t, string(body), `"current_coverage":null`)
}

func TestHandler_getDashboardSummary_Errors(t *testing.T) {
ctx := context.Background()

Expand All @@ -1363,6 +1437,27 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) {
assert.Contains(t, err.Error(), "failed to get recommendations")
})

// Issue #72: purchase history is as much a hard dependency as
// recommendations; a store error must fail the request, not zero the KPIs.
t.Run("purchase history error", func(t *testing.T) {
mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)
storeErr := errors.New("db error")

mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return(nil, storeErr)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{auth: mockAuth, scheduler: mockScheduler, config: mockStore}

result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
require.Error(t, err)
assert.ErrorIs(t, err, storeErr)
assert.Contains(t, err.Error(), "failed to get active purchase history")
assert.Nil(t, result)
})

t.Run("nil global config uses default coverage", func(t *testing.T) {
mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)
Expand Down Expand Up @@ -1857,8 +1952,9 @@ func TestHandler_calculateCommitmentMetrics_RevokedExcluded(t *testing.T) {
t.Cleanup(func() { mockStore.AssertExpectations(t) })

handler := &Handler{config: mockStore}
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(
activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(
ctx, "", nil, map[string][]string{"": {"acct-1"}})
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments,
"revoked commitment must not increment activeCommitments")
Expand Down Expand Up @@ -1889,8 +1985,9 @@ func TestHandler_calculateCommitmentMetrics_ProviderFilter(t *testing.T) {
handler := &Handler{config: mockStore}

// Filter to aws only.
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(
activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(
ctx, "aws", nil, nil)
require.NoError(t, err)

assert.Equal(t, 1, activeCommitments,
"provider filter must restrict activeCommitments to aws rows only")
Expand Down
5 changes: 5 additions & 0 deletions internal/api/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3452,6 +3452,11 @@ components:
current_coverage:
type: number
format: double
nullable: true
description: >-
Committed / (committed + potential) monthly savings, as a
percentage. Null when both are zero, so there is nothing to
measure.
target_coverage:
type: number
format: double
Expand Down
Loading
Loading