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/2] 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 900627ba..1a9af681 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 960be35c..e7f6cc45 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 ca2f495d..36b05a4a 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 d83531b3..35d8bdb3 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 e58799f4..90ef707d 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 3fdc4ed5..5f765df1 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 047dafec..db941f94 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 eff74f43..58906460 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 3cce623d..ce57a7e5 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 0df9e89d..eb0cfd84 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 d02ac600..7cd33a79 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 de8d0334..4f1f7476 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 150f448e..f2c979c7 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 ecf2ba8c..b4717487 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 cd02e0cf..65707d8e 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 6743539e..3742e2e9 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 caf518f6..5ed1115d 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 21e51d69..dbfe7bf3 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 00000000..913b505d --- /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 00000000..1669f993 --- /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 fd1c7e84..013eb918 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/2] 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 ce57a7e5..572a33b3 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 5ed1115d..0ec25175 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