fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408)

internal/api/handler_purchases.go

@@ -3638,11 +3638,17 @@ func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord
 		if err != nil {
 			return "", err
 		}
-		tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s",
+		// Single-AZ and Multi-AZ RDS buys of the same class are different
+		// purchases and must not dedupe into one (issue #408).
+		azConfig, err := r.DetailsAZConfig()
+		if err != nil {
+			return "", err
+		}
+		tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s|%s",
 			strings.ToLower(strings.TrimSpace(r.Provider)),
 			r.Service, r.Region, r.ResourceType, r.Engine, acct,
 			r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)), az,
-			ci.Platform, ci.Tenancy))
+			ci.Platform, ci.Tenancy, azConfig))
 	}
 	sort.Strings(tuples)
 	h := sha256.New()

internal/api/purchase_azconfig_test.go

@@ -0,0 +1,88 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Real RDS Details shape as persisted by the scheduler (issue #408).
+func rdsDetails(azConfig string) json.RawMessage {
+	return json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `","instance_class":"db.r5.large"}`)
+}
+
+func rdsAZConfigRec(azConfig string, upfront float64) config.RecommendationRecord {
+	return config.RecommendationRecord{
+		ID: "rec-rds-" + azConfig, Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large",
+		Engine: "mysql", Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, Savings: 10,
+		Details: rdsDetails(azConfig),
+	}
+}
+
+// Stored Single-AZ and Multi-AZ rows of the same class must both load (no
+// "more than one row for identity key" refusal) and the request, which carries
+// az_config only in Details, is priced from its own variant's row.
+func TestPriceRecommendationsFromStore_PricesFromRequestedAZConfig(t *testing.T) {
+	for _, want := range []struct {
+		azConfig string
+		upfront  float64
+	}{
+		{"single-az", 100},
+		{"multi-az", 300},
+	} {
+		t.Run(want.azConfig, func(t *testing.T) {
+			store := new(MockConfigStore)
+			t.Cleanup(func() { store.AssertExpectations(t) })
+			expectStoredRecs(store, rdsAZConfigRec("single-az", 100), rdsAZConfigRec("multi-az", 300))
+			h := &Handler{config: store}
+			req := []config.RecommendationRecord{{
+				Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql",
+				Term: 1, Payment: "all-upfront", Count: 1, Details: rdsDetails(want.azConfig),
+			}}
+			require.NoError(t, h.priceRecommendationsFromStore(context.Background(), req))
+			assert.Equal(t, want.upfront, req[0].UpfrontCost)
+			got, err := req[0].DetailsAZConfig()
+			require.NoError(t, err)
+			assert.Equal(t, want.azConfig, got)
+		})
+	}
+}
+
+// A request that does not name its az_config cannot be priced off an arbitrary
+// RDS variant: it is refused with a 409.
+func TestPriceRecommendationsFromStore_RequestWithoutAZConfigRefusedForRDS(t *testing.T) {
+	store := new(MockConfigStore)
+	t.Cleanup(func() { store.AssertExpectations(t) })
+	expectStoredRecs(store, rdsAZConfigRec("multi-az", 300))
+	h := &Handler{config: store}
+	req := []config.RecommendationRecord{{
+		Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql",
+		Term: 1, Payment: "all-upfront", Count: 1,
+	}}
+	err := h.priceRecommendationsFromStore(context.Background(), req)
+	require.Error(t, err)
+	ce, isClient := IsClientError(err)
+	require.True(t, isClient)
+	assert.Equal(t, 409, ce.code)
+}
+
+func TestPurchaseIdempotencyKey_DiffersByAZConfig(t *testing.T) {
+	t.Parallel()
+	key := func(azConfig string) string {
+		return mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{rdsAZConfigRec(azConfig, 1)}, 100)
+	}
+	assert.NotEqual(t, key("single-az"), key("multi-az"),
+		"a Multi-AZ submit right after a Single-AZ one must not dedupe into it")
+	assert.Equal(t, key("multi-az"), key("multi-az"))
+}
+
+func TestDescribeRec_IncludesAZConfig(t *testing.T) {
+	r := rdsAZConfigRec("multi-az", 1)
+	assert.Contains(t, describeRec(&r), `az_config="multi-az"`)
+	bare := config.RecommendationRecord{Provider: "aws", Service: "rds"}
+	assert.NotContains(t, describeRec(&bare), "az_config=")
+}

internal/api/purchase_pricing.go

@@ -363,5 +363,8 @@ func describeRec(rec *config.RecommendationRecord) string {
 	if ci, err := rec.ComputeIdentity(); err == nil && !ci.IsZero() {
 		desc += fmt.Sprintf(" platform=%q tenancy=%q", ci.Platform, ci.Tenancy)
 	}
+	if azConfig, err := rec.DetailsAZConfig(); err == nil && azConfig != "" {
+		desc += fmt.Sprintf(" az_config=%q", azConfig)
+	}
 	return desc
 }

internal/config/recommendation_identity.go

@@ -41,10 +41,33 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) {
 	return ComputeIdentity{Platform: d.Platform, Tenancy: d.Tenancy}, nil
 }
 
-// RecIdentityKey is the tuple a price is a function of: the same eleven
+// DetailsAZConfig reads the database AZ configuration (RDS "single-az" /
+// "multi-az", Azure SQL "zoneRedundant" / "none") from the rec's Details
+// blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different
+// offerings with different prices, yet share every other identity field
+// (issue #408). It mirrors the generated az_config column of migration 000154
+// (COALESCE(payload->'details'->>'az_config', ”)) and is pinned to it by the
+// shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a
+// missing key and a null value all yield "". Only DatabaseDetails carries the
+// key. A non-string value is an error.
+func (r *RecommendationRecord) DetailsAZConfig() (string, error) {
+	if len(r.Details) == 0 {
+		return "", nil
+	}
+	var d struct {
+		AZConfig string `json:"az_config"`
+	}
+	if err := json.Unmarshal(r.Details, &d); err != nil {
+		return "", fmt.Errorf("decode az_config of %s/%s details: %w", r.Region, r.ResourceType, err)
+	}
+	return d.AZConfig, nil
+}
+
+// RecIdentityKey is the tuple a price is a function of: the same twelve
 // components (the ninth is the zonal EC2 availability zone, "" for every
 // other rec, issue #792; the tenth and eleventh are the EC2 platform and
-// tenancy read from Details, "" when absent, issue #806) behind
+// tenancy read from Details, "" when absent, issue #806; the twelfth is the
+// database az_config read from Details, "" when absent, issue #408) behind
 // RecommendationRecord.ID, with a nil
 // and an empty CloudAccountID collapsed together (as purchaseConstraintSets
 // and the store's account_key both do). Provider and payment are lowercased
@@ -54,14 +77,18 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	azConfig, err := rec.DetailsAZConfig()
+	if err != nil {
+		return "", err
+	}
 	account := ""
 	if rec.CloudAccountID != nil {
 		account = *rec.CloudAccountID
 	}
 	return strings.Join([]string{
 		strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType,
 		rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment),
-		rec.AvailabilityZone, ci.Platform, ci.Tenancy,
+		rec.AvailabilityZone, ci.Platform, ci.Tenancy, azConfig,
 	}, "\x1f"), nil
 }
 

internal/config/recommendation_identity_test.go

@@ -45,3 +45,59 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) {
 	_, err = bad.ComputeIdentity()
 	assert.Error(t, err)
 }
+
+// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000154,
+// internal/database/postgres/migrations) so the Go reader and the generated
+// az_config column cannot diverge (issue #408). Details here is the details
+// blob itself.
+func TestDetailsAZConfig_Vectors(t *testing.T) {
+	vectors := []struct {
+		name, details, azConfig string
+	}{
+		{"details-absent", ``, ""},
+		{"details-null", `null`, ""},
+		{"details-empty", `{}`, ""},
+		{"az-config-null", `{"az_config":null}`, ""},
+		{"empty-string", `{"az_config":""}`, ""},
+		{"rds-single-az", `{"engine":"mysql","az_config":"single-az","instance_class":"db.r5.large"}`, "single-az"},
+		{"rds-multi-az", `{"engine":"mysql","az_config":"multi-az","instance_class":"db.r5.large"}`, "multi-az"},
+		{"azure-zone-redundant", `{"engine":"sqlserver","az_config":"zoneRedundant"}`, "zoneRedundant"},
+		{"verbatim-spaces", `{"az_config":" Multi-AZ "}`, " Multi-AZ "},
+		{"compute-details", `{"platform":"Linux/UNIX","tenancy":"default","scope":"Region"}`, ""},
+	}
+	for _, v := range vectors {
+		t.Run(v.name, func(t *testing.T) {
+			rec := RecommendationRecord{Details: json.RawMessage(v.details)}
+			got, err := rec.DetailsAZConfig()
+			require.NoError(t, err)
+			assert.Equal(t, v.azConfig, got)
+		})
+	}
+}
+
+func TestDetailsAZConfig_MalformedFailsLoud(t *testing.T) {
+	bad := RecommendationRecord{Details: json.RawMessage(`{"az_config":5}`)}
+	_, err := bad.DetailsAZConfig()
+	require.Error(t, err)
+	_, err = RecIdentityKey(&bad)
+	require.Error(t, err, "the identity key must not silently drop an unreadable az_config")
+}
+
+// Two otherwise identical RDS rows that differ only by az_config have
+// different identity keys; the same az_config yields the same key.
+func TestRecIdentityKey_DiffersByAZConfig(t *testing.T) {
+	rds := func(azConfig string) *RecommendationRecord {
+		return &RecommendationRecord{
+			Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql",
+			Term: 1, Payment: "all-upfront", Details: json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `"}`),
+		}
+	}
+	key := func(r *RecommendationRecord) string {
+		k, err := RecIdentityKey(r)
+		require.NoError(t, err)
+		return k
+	}
+	assert.NotEqual(t, key(rds("single-az")), key(rds("multi-az")))
+	assert.NotEqual(t, key(rds("single-az")), key(rds("")))
+	assert.Equal(t, key(rds("multi-az")), key(rds("multi-az")))
+}

internal/config/store_postgres_recommendations.go

@@ -25,7 +25,8 @@ const azurePricingMigrationVersion = 106
 
 // UpsertRecommendations is the incremental write path: it upserts each row
 // by natural key (cloud_account_id, provider, service, region,
-// resource_type, engine, term, payment_option, availability_zone, platform, tenancy) and then evicts stale rows for the
+// resource_type, engine, term, payment_option, availability_zone, platform,
+// tenancy, az_config) and then evicts stale rows for the
 // set of (provider, account) pairs that successfully collected in this run.
 // Pairs whose collection failed are NOT in successfulCollects and their
 // stale rows stay — callers see older data with a banner rather than a
@@ -35,8 +36,9 @@ const azurePricingMigrationVersion = 106
 // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration
 // 000109 added availability_zone, a GENERATED column derived from payload that
 // is never written here. Migration 000111 added the platform and tenancy
-// generated columns the same way; the upsert arbiter is the 11-column
-// recommendations_natural_key_az_tp_idx (issues #792, #806).
+// generated columns the same way, and migration 000154 the az_config one; the
+// upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx
+// (issues #792, #806, #408).
 //
 // The eviction predicate uses (provider, account_key) IN (unnest($2, $3))
 // where account_key matches the generated column on the table — nil
@@ -191,7 +193,7 @@ func insertRecommendationsBatch(ctx context.Context, tx pgx.Tx, collectedAt time
 		     resource_type, engine, payload, upfront_cost, monthly_savings,
 		     term, payment_option)
 		VALUES %s
-		ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy)
+		ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config)
 		DO UPDATE SET
 		    payload         = EXCLUDED.payload,
 		    upfront_cost    = EXCLUDED.upfront_cost,