From 3cd63c85fdb0305b5faebef75c462fa3eb6850d3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 23:48:16 +0200 Subject: [PATCH 1/4] feat(db): nullable MonthlyCost on PurchaseHistoryRecord (closes #255) Migration 000063 drops NOT NULL on purchase_history.monthly_cost so rows from providers without a monthly recurring breakdown (Azure/GCP all-upfront) can store NULL instead of a misleading 0.0. - PurchaseHistoryRecord.MonthlyCost: float64 -> *float64 - InventoryCommitment.MonthlyCost: float64 -> *float64 - All scan loops use sql.NullFloat64; nil DB value -> nil pointer - execution.go: remove derefFloat64; pass pointer straight through - handler_inventory.go: skip nil MonthlyCost in coverage aggregation - handler_history.go: sumRecommendationMonthlyCostPtr returns nil when every rec has nil MonthlyCost (renders as -- not $0.00) - Frontend: monthly_cost typed number|null; inventory cell renders -- - All test fixtures and assertions updated to use *float64 (pf helper) --- frontend/src/api/types.ts | 3 ++- frontend/src/inventory.ts | 2 +- frontend/src/types.ts | 2 +- internal/api/handler_history.go | 27 ++++++++++--------- internal/api/handler_history_test.go | 9 ++++--- internal/api/handler_inventory.go | 4 ++- internal/api/handler_inventory_test.go | 11 ++++---- .../api/handler_per_account_perms_test.go | 4 +-- internal/api/types.go | 7 +++-- internal/config/store_postgres.go | 10 ++++++- .../config/store_postgres_additional_test.go | 8 +++++- .../store_postgres_comprehensive_test.go | 13 ++++++--- .../config/store_postgres_coverage_test.go | 2 +- internal/config/store_postgres_db_test.go | 2 +- internal/config/store_postgres_test.go | 2 +- internal/config/store_postgres_unit_test.go | 4 +++ internal/config/types.go | 12 +++++++-- internal/config/types_test.go | 6 +++-- ...ase_history_monthly_cost_nullable.down.sql | 8 ++++++ ...chase_history_monthly_cost_nullable.up.sql | 14 ++++++++++ internal/purchase/execution.go | 13 +-------- 21 files changed, 110 insertions(+), 53 deletions(-) create mode 100644 internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.down.sql create mode 100644 internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.up.sql diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 900627ba2..1a9af681d 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -676,7 +676,8 @@ export interface InventoryCommitment { start_date: string; end_date: string; upfront_cost: number; - monthly_cost: number; + /** null when the provider API did not return a monthly recurring breakdown; "$X.XX" when populated, "—" when null. */ + monthly_cost: number | null; estimated_savings: number; /** * Always "active" today — the backend filters expired rows before diff --git a/frontend/src/inventory.ts b/frontend/src/inventory.ts index 960be35c6..e7f6cc458 100644 --- a/frontend/src/inventory.ts +++ b/frontend/src/inventory.ts @@ -236,7 +236,7 @@ function buildCommitmentRow(c: api.InventoryCommitment): HTMLTableRowElement { appendCell(tr, String(c.count)); appendCell(tr, `${c.term_years}y`); appendCell(tr, c.payment_option ?? ''); - appendCell(tr, formatCurrency(c.monthly_cost)); + appendCell(tr, c.monthly_cost != null ? formatCurrency(c.monthly_cost) : '—'); appendCell(tr, formatCurrency(c.estimated_savings)); appendCell(tr, formatDate(c.end_date)); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index ca2f495d8..36b05a4ad 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -187,7 +187,7 @@ export interface HistoryPurchase { term: number; payment?: string; upfront_cost: number; - monthly_cost?: number; + monthly_cost?: number | null; estimated_savings: number; account_id?: string; plan_name?: string; diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d83531b37..35d8bdb3f 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -380,9 +380,7 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.Payment = r.Payment row.UpfrontCost = r.UpfrontCost row.EstimatedSavings = r.Savings - if r.MonthlyCost != nil { - row.MonthlyCost = *r.MonthlyCost - } + row.MonthlyCost = r.MonthlyCost return } row.Region = "multiple" @@ -390,7 +388,7 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.Service = collapseRecommendationService(recs) row.Term = collapseRecommendationTerm(recs) row.Payment = collapseRecommendationPayment(recs) - row.MonthlyCost = sumRecommendationMonthlyCost(recs) + row.MonthlyCost = sumRecommendationMonthlyCostPtr(recs) row.UpfrontCost = exec.TotalUpfrontCost row.EstimatedSavings = exec.EstimatedSavings } @@ -486,21 +484,24 @@ func collapseRecommendationAccount(recs []config.RecommendationRecord) string { return first } -// sumRecommendationMonthlyCost adds up the per-rec MonthlyCost values in a -// multi-rec execution so the Approval queue's Monthly Cost cell shows the -// committed recurring spend for the full basket. Nil per-rec entries -// contribute 0 (the provider API did not return a monthly breakdown for -// that rec) — the same treatment as the single-rec branch, which only -// copies MonthlyCost when non-nil and otherwise leaves the row's field at -// the zero value. -func sumRecommendationMonthlyCost(recs []config.RecommendationRecord) float64 { +// sumRecommendationMonthlyCostPtr sums the per-rec MonthlyCost values for a +// multi-rec execution row in the Approval queue. Nil entries (provider API +// did not return a monthly breakdown) are skipped. Returns nil when every rec +// has a nil MonthlyCost so the row renders as "—" rather than "$0.00". +// Returns a pointer to the accumulated total otherwise. +func sumRecommendationMonthlyCostPtr(recs []config.RecommendationRecord) *float64 { var total float64 + anyNonNil := false for _, r := range recs { if r.MonthlyCost != nil { total += *r.MonthlyCost + anyNonNil = true } } - return total + if !anyNonNil { + return nil + } + return &total } // MaxHistoryDateRangeDays caps the inclusive start/end window the History diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index e58799f41..90ef707d3 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -478,7 +478,8 @@ func TestHandler_getHistory_InProgressRowMapsRecFields(t *testing.T) { assert.Equal(t, 1, row.Count) assert.Equal(t, 0.0, row.UpfrontCost, "upfront must come from the rec") assert.Equal(t, 1.2333, row.EstimatedSavings, "savings must come from the rec") - assert.Equal(t, 2.117, row.MonthlyCost, "monthly cost must come from the rec") + require.NotNil(t, row.MonthlyCost, "monthly cost must come from the rec") + assert.InDelta(t, 2.117, *row.MonthlyCost, 1e-9, "monthly cost must come from the rec") assert.Equal(t, "no-upfront", row.Payment, "payment must come from the rec, not be left blank (#733)") assert.NotEmpty(t, row.StatusDescription, "in-progress rows must carry a human-readable status description, not render as a finished purchase") @@ -1177,7 +1178,8 @@ func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { assert.Equal(t, accountID, row.AccountID, "Account must fall back to rec.CloudAccountID when exec.CloudAccountID is nil (#733)") assert.Equal(t, "all-upfront", row.Payment, "Payment must be copied from the single rec (#733)") - assert.Equal(t, 7.5, row.MonthlyCost, "MonthlyCost must come from the rec") + require.NotNil(t, row.MonthlyCost, "MonthlyCost must come from the rec") + assert.InDelta(t, 7.5, *row.MonthlyCost, 1e-9, "MonthlyCost must come from the rec") }) t.Run("multi-rec uniform pending row collapses Account + Payment, sums MonthlyCost", func(t *testing.T) { @@ -1214,7 +1216,8 @@ func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { assert.Equal(t, accountID, row.AccountID, "Account must collapse to the shared rec value (#733)") assert.Equal(t, "no-upfront", row.Payment, "Payment must collapse to the shared rec value (#733)") - assert.InDelta(t, 7.5, row.MonthlyCost, 1e-9, "MonthlyCost must sum across recs (#733)") + require.NotNil(t, row.MonthlyCost, "MonthlyCost must sum across recs (#733)") + assert.InDelta(t, 7.5, *row.MonthlyCost, 1e-9, "MonthlyCost must sum across recs (#733)") }) t.Run("multi-rec heterogeneous Payment collapses to empty for honest dash fallback", func(t *testing.T) { diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 3fdc4ed53..5f765df18 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -171,7 +171,9 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu if !isActiveCommitment(p, now) { continue } - coveredByKey[p.Provider+":"+p.Service] += p.MonthlyCost + if p.MonthlyCost != nil { + coveredByKey[p.Provider+":"+p.Service] += *p.MonthlyCost + } } // --- on-demand gap: recommendations ------------------------------------- diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 047dafec9..db941f945 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -74,7 +74,7 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { Term: 1, Payment: "no-upfront", Timestamp: now.AddDate(0, -6, 0), - MonthlyCost: 100.0, + MonthlyCost: float64Ptr(100.0), EstimatedSavings: 30.0, }, // Expired: bought 2 years ago, 1-year term. @@ -87,7 +87,7 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { Count: 1, Term: 1, Timestamp: now.AddDate(-2, 0, 0), - MonthlyCost: 50.0, + MonthlyCost: float64Ptr(50.0), EstimatedSavings: 15.0, }, } @@ -117,7 +117,8 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { assert.Equal(t, 2, row.Count) assert.Equal(t, 1, row.TermYears) assert.Equal(t, "no-upfront", row.PaymentOption) - assert.Equal(t, 100.0, row.MonthlyCost) + require.NotNil(t, row.MonthlyCost) + assert.Equal(t, 100.0, *row.MonthlyCost) assert.Equal(t, 30.0, row.EstimatedSavings) assert.Equal(t, "active", row.Status) assert.False(t, row.StartDate.IsZero()) @@ -141,7 +142,7 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { Timestamp: now.AddDate(0, -3, 0), Term: 1, Count: 1, - MonthlyCost: 80.0, + MonthlyCost: float64Ptr(80.0), EstimatedSavings: 20.0, }, } @@ -426,7 +427,7 @@ func TestHandler_getCoverageBreakdown_Integration(t *testing.T) { Service: "ec2", Timestamp: now.AddDate(-1, 0, 1), // active: 1y term started ~1y ago Term: 1, - MonthlyCost: 150.0, + MonthlyCost: float64Ptr(150.0), }, } recs := []config.RecommendationRecord{ diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index eff74f43f..589064608 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -750,7 +750,7 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test Service: "ec2", Timestamp: now.AddDate(0, -6, 0), Term: 1, - MonthlyCost: 200.0, + MonthlyCost: float64Ptr(200.0), } // Recommendation for account A: on-demand gap the scoped user is allowed to see. @@ -830,7 +830,7 @@ func TestPerAccountPerms_CoverageBreakdown_AdminSeesAll(t *testing.T) { Service: "ec2", Timestamp: now.AddDate(0, -6, 0), Term: 1, - MonthlyCost: 200.0, + MonthlyCost: float64Ptr(200.0), } recA := config.RecommendationRecord{ Provider: "aws", Service: "ec2", diff --git a/internal/api/types.go b/internal/api/types.go index 3cce623d2..ce57a7e53 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -508,8 +508,11 @@ type InventoryCommitment struct { PaymentOption string `json:"payment_option,omitempty"` StartDate time.Time `json:"start_date"` EndDate time.Time `json:"end_date"` - UpfrontCost float64 `json:"upfront_cost"` - MonthlyCost float64 `json:"monthly_cost"` + UpfrontCost float64 `json:"upfront_cost"` + // MonthlyCost is nil when the source purchase_history row has a NULL + // monthly_cost (provider did not return a monthly breakdown). The + // frontend renders "—" for nil and "$X.XX" when non-nil. + MonthlyCost *float64 `json:"monthly_cost"` EstimatedSavings float64 `json:"estimated_savings"` Status string `json:"status"` } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 0df9e89df..eb0cfd842 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1444,6 +1444,7 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, for rows.Next() { var record PurchaseHistoryRecord var planID, planName, cloudAccountID sql.NullString + var monthlyCost sql.NullFloat64 err := rows.Scan( &record.AccountID, @@ -1457,7 +1458,7 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, &record.Term, &record.Payment, &record.UpfrontCost, - &record.MonthlyCost, + &monthlyCost, &record.EstimatedSavings, &planID, &planName, @@ -1468,6 +1469,13 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, return nil, fmt.Errorf("failed to scan purchase history: %w", err) } + // Handle nullable monthly_cost: nil means "provider did not return a + // monthly breakdown"; 0.0 means "explicitly $0 recurring charge". + if monthlyCost.Valid { + v := monthlyCost.Float64 + record.MonthlyCost = &v + } + // Handle nullable strings if planID.Valid { record.PlanID = planID.String diff --git a/internal/config/store_postgres_additional_test.go b/internal/config/store_postgres_additional_test.go index d02ac6008..7cd33a791 100644 --- a/internal/config/store_postgres_additional_test.go +++ b/internal/config/store_postgres_additional_test.go @@ -124,6 +124,7 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st records := make([]PurchaseHistoryRecord, 0) for rows.Next() { var record PurchaseHistoryRecord + var monthlyCost sql.NullFloat64 var planID, planName sql.NullString err := rows.Scan( @@ -138,7 +139,7 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st &record.Term, &record.Payment, &record.UpfrontCost, - &record.MonthlyCost, + &monthlyCost, &record.EstimatedSavings, &planID, &planName, @@ -148,6 +149,11 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st return nil, err } + if monthlyCost.Valid { + v := monthlyCost.Float64 + record.MonthlyCost = &v + } + if planID.Valid { record.PlanID = planID.String } diff --git a/internal/config/store_postgres_comprehensive_test.go b/internal/config/store_postgres_comprehensive_test.go index de8d03341..4f1f74760 100644 --- a/internal/config/store_postgres_comprehensive_test.go +++ b/internal/config/store_postgres_comprehensive_test.go @@ -413,6 +413,7 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query for rows.Next() { var record PurchaseHistoryRecord var planID, planName sql.NullString + var monthlyCost sql.NullFloat64 err := rows.Scan( &record.AccountID, @@ -426,7 +427,7 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query &record.Term, &record.Payment, &record.UpfrontCost, - &record.MonthlyCost, + &monthlyCost, &record.EstimatedSavings, &planID, &planName, @@ -436,6 +437,11 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query return nil, err } + if monthlyCost.Valid { + v := monthlyCost.Float64 + record.MonthlyCost = &v + } + if planID.Valid { record.PlanID = planID.String } @@ -1277,7 +1283,7 @@ func TestSavePurchaseHistory_Success(t *testing.T) { Term: 3, Payment: "all-upfront", UpfrontCost: 2250.00, - MonthlyCost: 0, + MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "plan-123", PlanName: "Production RDS Plan", @@ -1973,7 +1979,8 @@ func TestQueryPurchaseHistory_AllFieldsPresent(t *testing.T) { assert.Equal(t, 3, record.Term) assert.Equal(t, "all-upfront", record.Payment) assert.Equal(t, 3000.0, record.UpfrontCost) - assert.Equal(t, 0.0, record.MonthlyCost) + require.NotNil(t, record.MonthlyCost, "monthly_cost 0.0 from DB must scan as non-nil pointer") + assert.Equal(t, 0.0, *record.MonthlyCost) assert.Equal(t, 600.0, record.EstimatedSavings) assert.Equal(t, "search-plan", record.PlanID) assert.Equal(t, "OpenSearch Production", record.PlanName) diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index 150f448ee..f2c979c78 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -492,7 +492,7 @@ func TestPostgresStore_SavePurchaseHistory_NilDB(t *testing.T) { Term: 3, Payment: "all-upfront", UpfrontCost: 2250.00, - MonthlyCost: 0, + MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "plan-123", PlanName: "RDS Plan", diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index ecf2ba8cc..b4717487c 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -709,7 +709,7 @@ func TestPostgresStoreDB_PurchaseHistory(t *testing.T) { Term: 3, Payment: "all-upfront", UpfrontCost: 2250.00, - MonthlyCost: 0, + MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "", PlanName: "Test Plan", diff --git a/internal/config/store_postgres_test.go b/internal/config/store_postgres_test.go index cd02e0cf7..65707d8e1 100644 --- a/internal/config/store_postgres_test.go +++ b/internal/config/store_postgres_test.go @@ -351,7 +351,7 @@ func TestPostgresStore_PurchaseHistory(t *testing.T) { Term: 3, Payment: "all-upfront", UpfrontCost: 2250.00, - MonthlyCost: 0, + MonthlyCost: pf(0), EstimatedSavings: 450.00, // PlanID intentionally left empty since it needs to be a valid UUID PlanName: "Test Plan", diff --git a/internal/config/store_postgres_unit_test.go b/internal/config/store_postgres_unit_test.go index 6743539e1..3742e2e9e 100644 --- a/internal/config/store_postgres_unit_test.go +++ b/internal/config/store_postgres_unit_test.go @@ -8,6 +8,10 @@ import ( "github.com/stretchr/testify/assert" ) +// pf returns a pointer to the given float64 value. Used in test struct +// literals where a *float64 field must be initialised from a constant. +func pf(v float64) *float64 { return &v } + // TestTimeFromTTL tests the timeFromTTL helper function func TestTimeFromTTL(t *testing.T) { tests := []struct { diff --git a/internal/config/types.go b/internal/config/types.go index caf518f68..5ed1115d9 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -461,8 +461,16 @@ type PurchaseHistoryRecord struct { Count int `json:"count" dynamodbav:"count"` Term int `json:"term" dynamodbav:"term"` Payment string `json:"payment" dynamodbav:"payment"` - UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` - MonthlyCost float64 `json:"monthly_cost" dynamodbav:"monthly_cost"` + UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` + // MonthlyCost is nil when the provider API did not return a monthly + // recurring breakdown for this commitment (e.g. Azure/GCP all-upfront + // where no recurring charge exists at the commitment layer). The frontend + // renders "—" for nil, "$X.XX" when populated. Aggregations must skip nil + // entries rather than treating them as $0 to avoid distorting totals. + // Migration 000063 dropped the NOT NULL constraint so new rows can carry + // NULL; existing rows with 0.0 are preserved as-is (those are real zeros + // from AWS all-upfront commitments). + MonthlyCost *float64 `json:"monthly_cost" dynamodbav:"monthly_cost"` EstimatedSavings float64 `json:"estimated_savings" dynamodbav:"estimated_savings"` PlanID string `json:"plan_id,omitempty" dynamodbav:"plan_id,omitempty"` PlanName string `json:"plan_name,omitempty" dynamodbav:"plan_name,omitempty"` diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 21e51d69a..dbfe7bf34 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -5,6 +5,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRampSchedule_GetCurrentCoverage(t *testing.T) { @@ -335,7 +336,7 @@ func TestPurchaseHistoryRecord_Fields(t *testing.T) { Term: 1, Payment: "no-upfront", UpfrontCost: 0, - MonthlyCost: 150.00, + MonthlyCost: pf(150.00), EstimatedSavings: 50.00, PlanID: "plan-123", PlanName: "EC2 Production Plan", @@ -353,7 +354,8 @@ func TestPurchaseHistoryRecord_Fields(t *testing.T) { assert.Equal(t, 1, rec.Term) assert.Equal(t, "no-upfront", rec.Payment) assert.Equal(t, float64(0), rec.UpfrontCost) - assert.Equal(t, 150.00, rec.MonthlyCost) + require.NotNil(t, rec.MonthlyCost) + assert.Equal(t, 150.00, *rec.MonthlyCost) assert.Equal(t, 50.00, rec.EstimatedSavings) assert.Equal(t, "plan-123", rec.PlanID) assert.Equal(t, "EC2 Production Plan", rec.PlanName) diff --git a/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.down.sql b/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.down.sql new file mode 100644 index 000000000..913b505d1 --- /dev/null +++ b/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.down.sql @@ -0,0 +1,8 @@ +-- Reverse 000063: restore NOT NULL on purchase_history.monthly_cost +-- +-- Coerces any existing NULLs to 0.0 before re-adding the constraint so the +-- rollback never fails on rows written after the up migration applied. + +UPDATE purchase_history SET monthly_cost = 0.0 WHERE monthly_cost IS NULL; + +ALTER TABLE purchase_history ALTER COLUMN monthly_cost SET NOT NULL; diff --git a/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.up.sql b/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.up.sql new file mode 100644 index 000000000..1669f9933 --- /dev/null +++ b/internal/database/postgres/migrations/000063_purchase_history_monthly_cost_nullable.up.sql @@ -0,0 +1,14 @@ +-- Migration 000063: allow NULL in purchase_history.monthly_cost +-- +-- Prior to this migration, monthly_cost had a NOT NULL constraint that forced +-- execution.go to collapse nil (provider API did not return a monthly breakdown) +-- into 0.0, making "no data" indistinguishable from "explicitly $0 recurring +-- charge" (e.g. AWS all-upfront commitments). Dropping the constraint lets new +-- writes carry NULL for Azure/GCP recommendations where the monthly breakdown +-- is absent, preserving the semantic distinction for the History UI. +-- +-- Existing 0.0 rows are NOT updated: those represent real $0 recurring charges +-- (typically AWS all-upfront RIs/SPs) and the conversion was lossless for them. +-- Only new writes from providers where monthly_cost was nil can now store NULL. + +ALTER TABLE purchase_history ALTER COLUMN monthly_cost DROP NOT NULL; diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index fd1c7e84b..013eb9187 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -637,7 +637,7 @@ func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.Purchase Term: rec.Term, Payment: rec.Payment, UpfrontCost: result.Cost, - MonthlyCost: derefFloat64(rec.MonthlyCost), + MonthlyCost: rec.MonthlyCost, EstimatedSavings: rec.Savings, PlanID: exec.PlanID, PlanName: plan.Name, @@ -969,17 +969,6 @@ func (m *Manager) getAWSAccountID(ctx context.Context) string { return "unknown" } -// derefFloat64 safely dereferences a *float64, returning 0 for nil. -// Used when copying from RecommendationRecord.MonthlyCost (*float64) -// into PurchaseHistoryRecord.MonthlyCost (float64), where nil means -// "not provided" and maps to 0 in the history table. -func derefFloat64(p *float64) float64 { - if p == nil { - return 0 - } - return *p -} - // applyEngineFallback backfills the Engine field on DB / Cache Details // from the persisted RecommendationRecord.Engine column when the decoded // Details left Engine empty. Legacy rows (persisted before #453) carry From 634ff44999f95820a6299cae1ba519ca8c1f7782 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 19:27:06 +0200 Subject: [PATCH 2/4] style(api): apply gofmt alignment to InventoryCommitment and related types --- internal/api/types.go | 32 ++++++++++++++++---------------- internal/config/types.go | 36 ++++++++++++++++++------------------ 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/internal/api/types.go b/internal/api/types.go index ce57a7e53..572a33b31 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -496,25 +496,25 @@ type ServiceSavings struct { // The field stays in the response shape so a future "expiring soon" // sub-state has a slot without a breaking API change. type InventoryCommitment struct { - ID string `json:"id"` - Provider string `json:"provider"` - AccountID string `json:"account_id"` - AccountName string `json:"account_name,omitempty"` - Service string `json:"service"` - ResourceType string `json:"resource_type,omitempty"` - Region string `json:"region"` - Count int `json:"count"` - TermYears int `json:"term_years"` - PaymentOption string `json:"payment_option,omitempty"` - StartDate time.Time `json:"start_date"` - EndDate time.Time `json:"end_date"` - UpfrontCost float64 `json:"upfront_cost"` + ID string `json:"id"` + Provider string `json:"provider"` + AccountID string `json:"account_id"` + AccountName string `json:"account_name,omitempty"` + Service string `json:"service"` + ResourceType string `json:"resource_type,omitempty"` + Region string `json:"region"` + Count int `json:"count"` + TermYears int `json:"term_years"` + PaymentOption string `json:"payment_option,omitempty"` + StartDate time.Time `json:"start_date"` + EndDate time.Time `json:"end_date"` + UpfrontCost float64 `json:"upfront_cost"` // MonthlyCost is nil when the source purchase_history row has a NULL // monthly_cost (provider did not return a monthly breakdown). The // frontend renders "—" for nil and "$X.XX" when non-nil. - MonthlyCost *float64 `json:"monthly_cost"` - EstimatedSavings float64 `json:"estimated_savings"` - Status string `json:"status"` + MonthlyCost *float64 `json:"monthly_cost"` + EstimatedSavings float64 `json:"estimated_savings"` + Status string `json:"status"` } // InventoryCommitmentsResponse is the envelope returned by diff --git a/internal/config/types.go b/internal/config/types.go index 5ed1115d9..0ec251759 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -451,17 +451,17 @@ type RIUtilizationCacheEntry struct { // layer never writes it (tag `dynamodbav:"-"` keeps it out of persistence), // and the API layer populates it as "completed" or "pending" before returning. type PurchaseHistoryRecord struct { - AccountID string `json:"account_id" dynamodbav:"account_id"` - PurchaseID string `json:"purchase_id" dynamodbav:"purchase_id"` - Timestamp time.Time `json:"timestamp" dynamodbav:"timestamp"` - Provider string `json:"provider" dynamodbav:"provider"` - Service string `json:"service" dynamodbav:"service"` - Region string `json:"region" dynamodbav:"region"` - ResourceType string `json:"resource_type" dynamodbav:"resource_type"` - Count int `json:"count" dynamodbav:"count"` - Term int `json:"term" dynamodbav:"term"` - Payment string `json:"payment" dynamodbav:"payment"` - UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` + AccountID string `json:"account_id" dynamodbav:"account_id"` + PurchaseID string `json:"purchase_id" dynamodbav:"purchase_id"` + Timestamp time.Time `json:"timestamp" dynamodbav:"timestamp"` + Provider string `json:"provider" dynamodbav:"provider"` + Service string `json:"service" dynamodbav:"service"` + Region string `json:"region" dynamodbav:"region"` + ResourceType string `json:"resource_type" dynamodbav:"resource_type"` + Count int `json:"count" dynamodbav:"count"` + Term int `json:"term" dynamodbav:"term"` + Payment string `json:"payment" dynamodbav:"payment"` + UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` // MonthlyCost is nil when the provider API did not return a monthly // recurring breakdown for this commitment (e.g. Azure/GCP all-upfront // where no recurring charge exists at the commitment layer). The frontend @@ -470,13 +470,13 @@ type PurchaseHistoryRecord struct { // Migration 000063 dropped the NOT NULL constraint so new rows can carry // NULL; existing rows with 0.0 are preserved as-is (those are real zeros // from AWS all-upfront commitments). - MonthlyCost *float64 `json:"monthly_cost" dynamodbav:"monthly_cost"` - EstimatedSavings float64 `json:"estimated_savings" dynamodbav:"estimated_savings"` - PlanID string `json:"plan_id,omitempty" dynamodbav:"plan_id,omitempty"` - PlanName string `json:"plan_name,omitempty" dynamodbav:"plan_name,omitempty"` - RampStep int `json:"ramp_step,omitempty" dynamodbav:"ramp_step,omitempty"` - CloudAccountID *string `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"` - Status string `json:"status,omitempty" dynamodbav:"-"` + MonthlyCost *float64 `json:"monthly_cost" dynamodbav:"monthly_cost"` + EstimatedSavings float64 `json:"estimated_savings" dynamodbav:"estimated_savings"` + PlanID string `json:"plan_id,omitempty" dynamodbav:"plan_id,omitempty"` + PlanName string `json:"plan_name,omitempty" dynamodbav:"plan_name,omitempty"` + RampStep int `json:"ramp_step,omitempty" dynamodbav:"ramp_step,omitempty"` + CloudAccountID *string `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"` + Status string `json:"status,omitempty" dynamodbav:"-"` // Approver holds the email address the approval request was sent to (or // would have been, if SES failed). Set only on pending rows, so the // History UI can show "awaiting approval from " and the user knows From eccdc636b0177d976e5d5187f049320afa7210f4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 3 Jun 2026 12:47:06 +0200 Subject: [PATCH 3/4] fix(api/tests): wrap MonthlyCost literals in float64Ptr after nullable refactor The PurchaseHistoryRecord.MonthlyCost type changed to *float64 in 3cd63c85f but four test literals in handler_inventory_test.go (lines 186, 196, 499, 508) still assigned untyped float constants, breaking go vet with: cannot use 80.0 (untyped float constant 80) as *float64 value in struct literal Switch them to float64Ptr(...) consistent with the rest of the file (lines 77, 90, 145, 430 already use the helper). --- internal/api/handler_inventory_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index db941f945..c0db0ae00 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -183,7 +183,7 @@ func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { Timestamp: now.AddDate(0, -3, 0), Term: 1, Count: 1, - MonthlyCost: 80.0, + MonthlyCost: float64Ptr(80.0), }, { AccountID: "acc-1", @@ -193,7 +193,7 @@ func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { Timestamp: now.AddDate(0, -3, 0), Term: 1, Count: 2, - MonthlyCost: 120.0, + MonthlyCost: float64Ptr(120.0), }, } @@ -496,7 +496,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { Service: "ec2", Timestamp: now.AddDate(0, -3, 0), Term: 1, - MonthlyCost: 200.0, + MonthlyCost: float64Ptr(200.0), }, { AccountID: "acc-1", @@ -505,7 +505,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { Service: "compute", Timestamp: now.AddDate(0, -3, 0), Term: 1, - MonthlyCost: 999.0, // must be dropped by the provider=aws chip + MonthlyCost: float64Ptr(999.0), // must be dropped by the provider=aws chip }, } From f05dce0e01647cd0e1479439b2b1f274af9a8a9d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 3 Jun 2026 14:52:31 +0200 Subject: [PATCH 4/4] docs(db): clarify GCP commitments are monthly + nil round-trip test Address CodeRabbit review on PR #848: - F1 (types.go): remove the misleading "GCP all-upfront" example from the MonthlyCost field comment. In this repo GCP CUD commitments are always monthly-billed (per #709, #717, #718, #829), so the only provider that can legitimately produce a NULL recurring breakdown at the commitment layer is Azure all-upfront. Comment now mentions only Azure and explicitly notes that GCP always populates MonthlyCost. - F2 (store_postgres_db_test.go): add a "MonthlyCost nil round-trip" subtest that inserts a PurchaseHistoryRecord with MonthlyCost: nil and asserts it reads back as nil. This is the contract migration 000063 was meant to establish (NULL -> nil, distinct from 0.0 -> &0), and the frontend renders the dash glyph vs "$0.00" based on it. Also strengthens the existing 0.0 subtest to assert the pointer is non-nil with *MonthlyCost == 0.0, so both sides of the contract are pinned end-to-end. No production code changes; comment + test only. --- internal/config/store_postgres_db_test.go | 40 +++++++++++++++++++++++ internal/config/types.go | 15 +++++---- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index b4717487c..3561d5ef1 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -731,11 +731,51 @@ func TestPostgresStoreDB_PurchaseHistory(t *testing.T) { assert.Equal(t, "rds", h.Service) assert.Equal(t, 3, h.Count) assert.Equal(t, "Test Plan", h.PlanName) + require.NotNil(t, h.MonthlyCost, "expected non-nil MonthlyCost for explicit 0.0 row") + assert.Equal(t, 0.0, *h.MonthlyCost) } } assert.True(t, found, "Expected purchase record not found") }) + t.Run("MonthlyCost nil round-trip", func(t *testing.T) { + // Regression guard for migration 000063: a row inserted with + // MonthlyCost: nil must round-trip back as nil (NULL -> nil), not 0.0. + // This is the contract the slice's main change established, and the + // frontend depends on it to render a dash instead of "$0.00" for Azure + // all-upfront commitments where no recurring charge exists. + record := &PurchaseHistoryRecord{ + AccountID: "123456789012", + PurchaseID: "purchase-nil-cost", + Timestamp: time.Now().Truncate(time.Microsecond), + Provider: "azure", + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D4s_v3", + Count: 1, + Term: 1, + Payment: "all-upfront", + UpfrontCost: 1200.00, + MonthlyCost: nil, + EstimatedSavings: 240.00, + } + + err := store.SavePurchaseHistory(ctx, record) + require.NoError(t, err) + + history, err := store.GetPurchaseHistory(ctx, "123456789012", 10) + require.NoError(t, err) + + found := false + for _, h := range history { + if h.PurchaseID == "purchase-nil-cost" { + found = true + assert.Nil(t, h.MonthlyCost, "expected nil MonthlyCost to round-trip as nil (NULL -> nil)") + } + } + assert.True(t, found, "Expected nil-MonthlyCost record not found") + }) + t.Run("SavePurchaseHistory with empty optional fields", func(t *testing.T) { record := &PurchaseHistoryRecord{ AccountID: "empty-account", diff --git a/internal/config/types.go b/internal/config/types.go index 0ec251759..e257ace3a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -463,13 +463,14 @@ type PurchaseHistoryRecord struct { Payment string `json:"payment" dynamodbav:"payment"` UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` // MonthlyCost is nil when the provider API did not return a monthly - // recurring breakdown for this commitment (e.g. Azure/GCP all-upfront - // where no recurring charge exists at the commitment layer). The frontend - // renders "—" for nil, "$X.XX" when populated. Aggregations must skip nil - // entries rather than treating them as $0 to avoid distorting totals. - // Migration 000063 dropped the NOT NULL constraint so new rows can carry - // NULL; existing rows with 0.0 are preserved as-is (those are real zeros - // from AWS all-upfront commitments). + // recurring breakdown for this commitment (e.g. Azure all-upfront where + // no recurring charge exists at the commitment layer). GCP commitments + // are monthly-billed in this repo, so they always populate MonthlyCost. + // The frontend renders "—" for nil, "$X.XX" when populated. Aggregations + // must skip nil entries rather than treating them as $0 to avoid + // distorting totals. Migration 000063 dropped the NOT NULL constraint so + // new rows can carry NULL; existing rows with 0.0 are preserved as-is + // (those are real zeros from AWS all-upfront commitments). MonthlyCost *float64 `json:"monthly_cost" dynamodbav:"monthly_cost"` EstimatedSavings float64 `json:"estimated_savings" dynamodbav:"estimated_savings"` PlanID string `json:"plan_id,omitempty" dynamodbav:"plan_id,omitempty"`