fix(analytics): label snapshot commitment types from provider and service

internal/analytics/collector.go

@@ -158,7 +158,7 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR
 		}
 		activePurchases++
 
-		commitmentType := commitmentTypeFor(p.Service)
+		commitmentType := commitmentTypeFor(p.Provider, p.Service)
 		key := aggKey(p, commitmentType)
 
 		agg := serviceMap[key]
@@ -198,15 +198,6 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR
 	return serviceMap, activePurchases, skippedBadTerm, nil
 }
 
-// commitmentTypeFor maps a service to its commitment_type. SavingsPlans is the
-// only Savings Plan service today; everything else is a Reserved Instance.
-func commitmentTypeFor(service string) string {
-	if service == "SavingsPlans" {
-		return "SavingsPlan"
-	}
-	return "RI"
-}
-
 // buildSnapshots converts the aggregation map into snapshot rows. total_usage is
 // nil when no contributing row carried a recurring cost; coverage_percentage is
 // nil because purchase_history carries no on-demand baseline to derive it from

internal/analytics/collector_test.go

@@ -648,7 +648,7 @@ func TestCollectorCollect(t *testing.T) {
 		assert.Equal(t, "123456789012", s.AccountID)
 		assert.Equal(t, "aws", s.Provider)
 		assert.Equal(t, "rds", s.Service)
-		assert.Equal(t, "RI", s.CommitmentType)
+		assert.Equal(t, string(common.CommitmentReservedInstance), s.CommitmentType)
 		assert.Greater(t, s.TotalSavings, 0.0)
 		require.NotNil(t, s.TotalCommitment)
 		assert.Greater(t, *s.TotalCommitment, 0.0)
@@ -684,16 +684,35 @@ func TestCollectorCollect(t *testing.T) {
 		assert.Len(t, store.savedSnapshots, 3)
 	})
 
-	t.Run("sets SavingsPlan commitment type for SavingsPlans service", func(t *testing.T) {
+	// #339 (A06-019): the type comes from (provider, service). Purchase history
+	// stores Savings Plans as "savingsplans" or a per-plan-type slug, never the
+	// "SavingsPlans" the old mapper matched, and GCP purchases are CUDs.
+	t.Run("derives commitment type from provider and service", func(t *testing.T) {
 		store := &mockAnalyticsStore{}
 		cfgStore := &mockConfigStore{
 			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
+				return []config.PurchaseHistoryRecord{
+					activeRecord("aws", "savingsplans", "us-east-1", 1, 500, 2000),
+					activeRecord("aws", "savings-plans-compute", "us-east-1", 1, 500, 2000),
+					activeRecord("gcp", "compute", "us-central1", 1, 300, 0),
+					activeRecord("azure", "compute", "eastus", 1, 200, 900),
+					activeRecord("aws", "ec2", "us-east-1", 1, 100, 400),
+				}, nil
 			},
 		}
 		require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background()))
-		require.Len(t, store.savedSnapshots, 1)
-		assert.Equal(t, "SavingsPlan", store.savedSnapshots[0].CommitmentType)
+		got := make(map[string]string, len(store.savedSnapshots))
+		for _, s := range store.savedSnapshots {
+			got[s.Provider+"/"+s.Service] = s.CommitmentType
+			require.NoError(t, validateCommitmentType(s.CommitmentType), "the store CHECK must admit what the collector emits")
+		}
+		assert.Equal(t, map[string]string{
+			"aws/savingsplans":          string(common.CommitmentSavingsPlan),
+			"aws/savings-plans-compute": string(common.CommitmentSavingsPlan),
+			"gcp/compute":               string(common.CommitmentCUD),
+			"azure/compute":             string(common.CommitmentReservedInstance),
+			"aws/ec2":                   string(common.CommitmentReservedInstance),
+		}, got)
 	})
 
 	t.Run("returns error when BulkInsertSnapshots fails", func(t *testing.T) {

internal/analytics/commitment_type.go

@@ -0,0 +1,62 @@
+package analytics
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+)
+
+// Legacy commitment_type literals written before #339. The savings_snapshots
+// CHECK still admits them (migration 000145) so a not-yet-replaced replica can
+// keep writing during a rolling deploy; the collector never emits them.
+const (
+	legacyCommitmentRI          = "RI"
+	legacyCommitmentSavingsPlan = "SavingsPlan"
+)
+
+// allowedCommitmentTypes mirrors the savings_snapshots valid_commitment_type
+// CHECK constraint (migration 000145): the pkg/common vocabulary plus the two
+// legacy literals.
+var allowedCommitmentTypes = map[string]bool{
+	string(common.CommitmentReservedInstance): true,
+	string(common.CommitmentSavingsPlan):      true,
+	string(common.CommitmentCUD):              true,
+	string(common.CommitmentReservedCapacity): true,
+	legacyCommitmentRI:                        true,
+	legacyCommitmentSavingsPlan:               true,
+}
+
+// commitmentTypeFor maps a purchase's (provider, service) to its
+// commitment_type in the pkg/common vocabulary. GCP commitments are
+// committed-use discounts, any Savings Plans service slug is a savings plan,
+// and everything else (AWS and Azure reservations) is a reserved instance.
+func commitmentTypeFor(provider, service string) string {
+	switch {
+	case strings.EqualFold(provider, string(common.ProviderGCP)):
+		return string(common.CommitmentCUD)
+	case isSavingsPlanService(service):
+		return string(common.CommitmentSavingsPlan)
+	default:
+		return string(common.CommitmentReservedInstance)
+	}
+}
+
+// isSavingsPlanService recognizes every Savings Plans service slug persisted in
+// purchase_history.service: the umbrella "savingsplans", the four per-plan-type
+// slugs, and the pre-#85 "savings-plans" spelling.
+func isSavingsPlanService(service string) bool {
+	s := strings.ToLower(service)
+	return common.IsSavingsPlan(common.ServiceType(s)) || s == "savings-plans"
+}
+
+// validateCommitmentType rejects a commitment_type that the savings_snapshots
+// table CHECK constraint would reject. Extracted so the guard is unit-testable
+// directly (the COPY path acquires a real pooled connection that pgxmock can't
+// stand in for, so an end-to-end test can only prove the acquire-failure path).
+func validateCommitmentType(commitmentType string) error {
+	if !allowedCommitmentTypes[commitmentType] {
+		return fmt.Errorf("invalid commitment_type %q", commitmentType)
+	}
+	return nil
+}

internal/analytics/commitment_type_db_test.go

@@ -0,0 +1,63 @@
+//go:build integration
+
+package analytics
+
+import (
+	"context"
+	"path/filepath"
+	"runtime"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestCollector_GCPCommittedUse_DB is the #339 (A06-019) end-to-end check: the
+// collector writes active GCP, AWS Savings Plans and AWS RI purchases through
+// the real Postgres store, and each snapshot lands with its pkg/common
+// commitment_type. Before the fix all three were stored as "RI".
+func TestCollector_GCPCommittedUse_DB(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+	defer cancel()
+
+	container := testhelpers.RequirePostgresContainer(ctx, t)
+	t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) })
+	_, file, _, _ := runtime.Caller(0)
+	migrationsPath := filepath.Join(filepath.Dir(file), "..", "database", "postgres", "migrations")
+	require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), migrationsPath, "", ""))
+
+	store := NewPostgresAnalyticsStore(container.DB)
+	cfgStore := &mockConfigStore{
+		getActivePurchaseHistoryFunc: func(context.Context, time.Time, []string, map[string][]string) ([]config.PurchaseHistoryRecord, error) {
+			return []config.PurchaseHistoryRecord{
+				activeRecord("gcp", "compute", "us-central1", 1, 300, 1200),
+				activeRecord("aws", "savingsplans", "us-east-1", 1, 500, 2000),
+				activeRecord("aws", "ec2", "us-east-1", 1, 100, 400),
+			}, nil
+		},
+	}
+	collector, err := NewCollector(CollectorConfig{AnalyticsStore: store}, cfgStore)
+	require.NoError(t, err)
+	require.NoError(t, collector.Collect(ctx))
+
+	rows, err := container.DB.Pool().Query(ctx, `SELECT provider, service, commitment_type FROM savings_snapshots`)
+	require.NoError(t, err)
+	defer rows.Close()
+	got := map[string]string{}
+	for rows.Next() {
+		var provider, service, commitmentType string
+		require.NoError(t, rows.Scan(&provider, &service, &commitmentType))
+		got[provider+"/"+service] = commitmentType
+	}
+	require.NoError(t, rows.Err())
+	assert.Equal(t, map[string]string{
+		"gcp/compute":      string(common.CommitmentCUD),
+		"aws/savingsplans": string(common.CommitmentSavingsPlan),
+		"aws/ec2":          string(common.CommitmentReservedInstance),
+	}, got)
+}

internal/analytics/postgres_analytics.go

@@ -140,17 +140,6 @@ func (s *PostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *Sav
 	return nil
 }
 
-// validateCommitmentType rejects a commitment_type that the savings_snapshots
-// table CHECK constraint would reject. Extracted so the guard is unit-testable
-// directly (the COPY path acquires a real pooled connection that pgxmock can't
-// stand in for, so an end-to-end test can only prove the acquire-failure path).
-func validateCommitmentType(commitmentType string) error {
-	if commitmentType != "RI" && commitmentType != "SavingsPlan" {
-		return fmt.Errorf("invalid commitment_type %q (want RI or SavingsPlan)", commitmentType)
-	}
-	return nil
-}
-
 // BulkInsertSnapshots inserts multiple snapshots efficiently via COPY.
 func (s *PostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapshots []SavingsSnapshot) error {
 	if len(snapshots) == 0 {

internal/analytics/postgres_analytics_mock_test.go

@@ -176,27 +176,30 @@ func TestUUIDGeneration(t *testing.T) {
 	})
 }
 
-// TestCommitmentTypeLogic verifies commitment type determination.
+// TestCommitmentTypeLogic pins the (provider, service) -> commitment_type map
+// (#339): every Savings Plans slug is a savings plan, GCP is a CUD, and the
+// rest are reserved instances.
 func TestCommitmentTypeLogic(t *testing.T) {
-	t.Run("SavingsPlans service gets SavingsPlan type", func(t *testing.T) {
-		service := "SavingsPlans"
-		commitmentType := "RI"
-		if service == "SavingsPlans" {
-			commitmentType = "SavingsPlan"
-		}
-		assert.Equal(t, "SavingsPlan", commitmentType)
-	})
-
-	t.Run("other services get RI type", func(t *testing.T) {
-		services := []string{"rds", "elasticache", "opensearch", "ec2"}
-		for _, service := range services {
-			commitmentType := "RI"
-			if service == "SavingsPlans" {
-				commitmentType = "SavingsPlan"
-			}
-			assert.Equal(t, "RI", commitmentType)
-		}
-	})
+	cases := []struct{ provider, service, want string }{
+		{"aws", "savingsplans", "savings-plan"},
+		{"aws", "savings-plans", "savings-plan"},
+		{"aws", "SavingsPlans", "savings-plan"},
+		{"aws", "savings-plans-compute", "savings-plan"},
+		{"aws", "savings-plans-ec2instance", "savings-plan"},
+		{"aws", "savings-plans-sagemaker", "savings-plan"},
+		{"aws", "savings-plans-database", "savings-plan"},
+		{"aws", "ec2", "reserved-instance"},
+		{"aws", "rds", "reserved-instance"},
+		{"aws", "elasticache", "reserved-instance"},
+		{"aws", "opensearch", "reserved-instance"},
+		{"azure", "compute", "reserved-instance"},
+		{"azure", "database", "reserved-instance"},
+		{"gcp", "compute", "committed-use"},
+		{"GCP", "cloudsql", "committed-use"},
+	}
+	for _, tc := range cases {
+		assert.Equal(t, tc.want, commitmentTypeFor(tc.provider, tc.service), "%s/%s", tc.provider, tc.service)
+	}
 }
 
 // TestBulkInsertEmptySlice verifies empty slice handling.

internal/analytics/postgres_analytics_test.go

@@ -250,6 +250,11 @@ func TestValidateCommitmentType(t *testing.T) {
 	t.Run("accepts SavingsPlan", func(t *testing.T) {
 		assert.NoError(t, validateCommitmentType("SavingsPlan"))
 	})
+	t.Run("accepts the pkg/common vocabulary", func(t *testing.T) {
+		for _, v := range []string{"reserved-instance", "savings-plan", "committed-use", "reserved-capacity"} {
+			assert.NoError(t, validateCommitmentType(v), v)
+		}
+	})
 	t.Run("rejects an unknown value", func(t *testing.T) {
 		err := validateCommitmentType("bogus")
 		require.Error(t, err)

internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.down.sql

@@ -0,0 +1,18 @@
+-- Revert 000145 (issue #339): restore the two-value commitment_type CHECK.
+--
+-- Rows are mapped back to the only labels the old collector could emit:
+-- savings-plan becomes 'SavingsPlan' and everything else 'RI'. This is lossy on
+-- purpose. GCP committed-use rows go back to 'RI', which is what the pre-#339
+-- code wrote for them.
+
+ALTER TABLE savings_snapshots DROP CONSTRAINT IF EXISTS valid_commitment_type;
+
+UPDATE savings_snapshots
+SET commitment_type = CASE
+        WHEN commitment_type = 'savings-plan' THEN 'SavingsPlan'
+        ELSE 'RI'
+    END
+WHERE commitment_type NOT IN ('RI', 'SavingsPlan');
+
+ALTER TABLE savings_snapshots
+    ADD CONSTRAINT valid_commitment_type CHECK (commitment_type IN ('RI', 'SavingsPlan'));

internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.up.sql

@@ -0,0 +1,45 @@
+-- Issue #339 (audit finding A06-019): savings_snapshots.commitment_type uses
+-- the pkg/common CommitmentType vocabulary.
+--
+-- The collector labeled every commitment "RI" except a service literally named
+-- "SavingsPlans", which purchase_history never stores (Savings Plans are
+-- "savingsplans" or a "savings-plans-<type>" slug). So GCP committed-use
+-- discounts and AWS Savings Plans were both stored as "RI", and the CHECK only
+-- admitted the two literals that mapper could emit. The collector now derives
+-- the type from (provider, service) as one of the pkg/common values:
+-- reserved-instance, savings-plan, committed-use, reserved-capacity.
+--
+-- The CHECK keeps admitting the legacy 'RI' / 'SavingsPlan' literals so a
+-- replica still on the old binary can write during a rolling deploy without
+-- failing its collection run. The collector never emits them, and the backfill
+-- below rewrites every existing legacy row.
+--
+-- The whole file runs in one transaction (golang-migrate), and ALTER TABLE
+-- holds an ACCESS EXCLUSIVE lock on savings_snapshots and its partitions until
+-- commit. A concurrent collector insert waits for the commit and is then
+-- checked against the new constraint, which admits both vocabularies, so it
+-- cannot fail because of this migration. All partitions are created with
+-- PARTITION OF, so they inherit the constraint from the parent, and DROP/ADD on
+-- the parent applies to every partition.
+
+ALTER TABLE savings_snapshots DROP CONSTRAINT IF EXISTS valid_commitment_type;
+
+-- Relabel existing rows from (provider, service), as the collector now does.
+UPDATE savings_snapshots
+SET commitment_type = CASE
+        WHEN commitment_type = 'SavingsPlan' THEN 'savings-plan'
+        WHEN lower(provider) = 'gcp' THEN 'committed-use'
+        WHEN lower(service) IN (
+            'savingsplans', 'savings-plans',
+            'savings-plans-compute', 'savings-plans-ec2instance',
+            'savings-plans-sagemaker', 'savings-plans-database'
+        ) THEN 'savings-plan'
+        ELSE 'reserved-instance'
+    END
+WHERE commitment_type IN ('RI', 'SavingsPlan');
+
+ALTER TABLE savings_snapshots
+    ADD CONSTRAINT valid_commitment_type CHECK (commitment_type IN (
+        'reserved-instance', 'savings-plan', 'committed-use', 'reserved-capacity',
+        'RI', 'SavingsPlan'
+    ));