refactor(common): make DatabaseDetails/CacheDetails pointer-only

cmd/helpers.go

@@ -690,19 +690,30 @@ func adjustSingleRecommendation(rec common.Recommendation, existingMap map[strin
 }
 
 // getEngineFromRecommendation extracts the engine from recommendation details.
+// DatabaseDetails/CacheDetails are always pointers (every producer -- AWS,
+// Azure, the CSV loader, and the JSON codec -- constructs them that way); see
+// pkg/common/service_details_codec.go's package doc for the pointer
+// invariant. The nil-pointer guards are not dead code: `rec.Details == nil`
+// only catches an untyped nil, so a typed nil (a (*common.DatabaseDetails)(nil)
+// stored in the interface) reaches the switch and would panic on the field
+// read. Returning blank matches the sibling helpers (extractDeployment and
+// extractEngine in multi_service_csv.go, rdsEngineDeploymentFromRec in the
+// AWS coverage package), which already guard the same way.
 func getEngineFromRecommendation(rec common.Recommendation) string {
 	if rec.Details == nil {
 		return ""
 	}
 	var engine string
 	switch details := rec.Details.(type) {
-	case common.DatabaseDetails:
-		engine = details.Engine
 	case *common.DatabaseDetails:
-		engine = details.Engine
-	case common.CacheDetails:
+		if details == nil {
+			return ""
+		}
 		engine = details.Engine
 	case *common.CacheDetails:
+		if details == nil {
+			return ""
+		}
 		engine = details.Engine
 	default:
 		return ""

cmd/helpers_test.go

@@ -589,33 +589,24 @@ func TestNormalizeEngineName(t *testing.T) {
 	}
 }
 
+// TestGetEngineFromRecommendation only exercises pointer-typed
+// DatabaseDetails/CacheDetails: every producer (AWS, Azure, the CSV
+// loader, and the JSON codec) constructs them that way, so there is no
+// value-typed case to cover -- see
+// pkg/common/service_details_codec.go's package doc for the invariant.
 func TestGetEngineFromRecommendation(t *testing.T) {
 	tests := []struct {
 		name     string
 		expected string
 		rec      common.Recommendation
 	}{
-		{
-			name: "DatabaseDetails value type",
-			rec: common.Recommendation{
-				Details: common.DatabaseDetails{Engine: "mysql"},
-			},
-			expected: "mysql",
-		},
 		{
 			name: "DatabaseDetails pointer type",
 			rec: common.Recommendation{
 				Details: &common.DatabaseDetails{Engine: "postgresql"},
 			},
 			expected: "postgresql",
 		},
-		{
-			name: "CacheDetails value type",
-			rec: common.Recommendation{
-				Details: common.CacheDetails{Engine: "redis"},
-			},
-			expected: "redis",
-		},
 		{
 			name: "CacheDetails pointer type",
 			rec: common.Recommendation{
@@ -637,6 +628,23 @@ func TestGetEngineFromRecommendation(t *testing.T) {
 			},
 			expected: "",
 		},
+		// Typed nils: rec.Details != nil (the interface holds a type) but the
+		// pointer inside is nil, so the `rec.Details == nil` guard does not
+		// catch it and the field read would panic without the per-case check.
+		{
+			name: "typed nil *DatabaseDetails - returns empty, no panic",
+			rec: common.Recommendation{
+				Details: (*common.DatabaseDetails)(nil),
+			},
+			expected: "",
+		},
+		{
+			name: "typed nil *CacheDetails - returns empty, no panic",
+			rec: common.Recommendation{
+				Details: (*common.CacheDetails)(nil),
+			},
+			expected: "",
+		},
 	}
 
 	for _, tt := range tests {

cmd/main.go

@@ -279,20 +279,19 @@ func effectiveSizingPct(cfg Config) float64 {
 	return cfg.Coverage
 }
 
-// extractEngineLabel returns the engine or platform string from the polymorphic
-// Details field, handling both value and pointer forms. The live parser and the
-// CSV loader store pointers (*DatabaseDetails is required by findOfferingID),
-// while some test callers construct values directly.
+// extractEngineLabel returns the engine or platform string from the
+// polymorphic Details field. DatabaseDetails/CacheDetails are always
+// pointers (every producer constructs them that way; see
+// pkg/common/service_details_codec.go's package doc for the pointer
+// invariant). ComputeDetails is still accepted as a value because the
+// Azure compute client and the GCP compute-engine client both construct
+// it that way.
 func extractEngineLabel(details interface{}) string {
 	switch d := details.(type) {
-	case common.DatabaseDetails:
-		return d.Engine
 	case *common.DatabaseDetails:
 		if d != nil {
 			return d.Engine
 		}
-	case common.CacheDetails:
-		return d.Engine
 	case *common.CacheDetails:
 		if d != nil {
 			return d.Engine

cmd/main_test.go

@@ -111,7 +111,7 @@ func TestGeneratePurchaseID(t *testing.T) {
 				Service:      common.ServiceRDS,
 				ResourceType: "db.t3.micro",
 				Count:        2,
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine:   "mysql",
 					AZConfig: "single-az",
 				},
@@ -141,7 +141,7 @@ func TestGeneratePurchaseID(t *testing.T) {
 				Service:      common.ServiceElastiCache,
 				ResourceType: "cache.r5.large",
 				Count:        1,
-				Details: common.CacheDetails{
+				Details: &common.CacheDetails{
 					Engine: "redis",
 				},
 			},
@@ -157,7 +157,7 @@ func TestGeneratePurchaseID(t *testing.T) {
 				Service:      common.ServiceRDS,
 				ResourceType: "db.m5.xlarge",
 				Count:        3,
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine:   "postgres",
 					AZConfig: "multi-az",
 				},
@@ -373,7 +373,7 @@ func TestGeneratePurchaseIDEdgeCases(t *testing.T) {
 		Service:      common.ServiceRDS,
 		ResourceType: "db.r5b.2xlarge",
 		Count:        10,
-		Details: common.DatabaseDetails{
+		Details: &common.DatabaseDetails{
 			Engine:   "MySQL 8.0",
 			AZConfig: "single-az",
 		},
@@ -414,7 +414,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				ResourceType: "db.r5.large",
 				Count:        3,
 				AccountName:  "Production Account",
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine: "PostgreSQL",
 				},
 			},
@@ -431,7 +431,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				Service:      common.ServiceElastiCache,
 				ResourceType: "cache.r5.xlarge",
 				Count:        5,
-				Details: common.CacheDetails{
+				Details: &common.CacheDetails{
 					Engine: "Redis",
 				},
 			},
@@ -465,7 +465,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				Service:      common.ServiceMemoryDB,
 				ResourceType: "db.r6g.large",
 				Count:        2,
-				Details:      common.CacheDetails{Engine: "redis"},
+				Details:      &common.CacheDetails{Engine: "redis"},
 			},
 			region:   "us-east-1",
 			isDryRun: false,
@@ -523,7 +523,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				ResourceType: "db.r6g.xlarge",
 				Count:        15,
 				AccountName:  "Staging",
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine:   "aurora-mysql",
 					AZConfig: "multi-az",
 				},
@@ -541,7 +541,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				Service:      common.ServiceElastiCache,
 				ResourceType: "cache.m5.large",
 				Count:        1,
-				Details: common.CacheDetails{
+				Details: &common.CacheDetails{
 					Engine: "redis",
 				},
 			},
@@ -558,7 +558,7 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) {
 				Service:      common.ServiceRDS,
 				ResourceType: "db.t3.micro",
 				Count:        20,
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine: "MySQL_8.0_Community",
 				},
 			},
@@ -610,7 +610,7 @@ func TestGeneratePurchaseIDCoverageVariations(t *testing.T) {
 		Service:      common.ServiceRDS,
 		ResourceType: "db.t3.small",
 		Count:        1,
-		Details: common.DatabaseDetails{
+		Details: &common.DatabaseDetails{
 			Engine: "mysql",
 		},
 	}

cmd/multi_service_coverage_test.go

@@ -339,7 +339,7 @@ func TestAdjustRecommendationForExcludedVersions_AdditionalCases(t *testing.T) {
 				ResourceType: "db.t3.small",
 				Count:        10,
 				Region:       "us-east-1",
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine: "mysql",
 				},
 			},
@@ -352,7 +352,7 @@ func TestAdjustRecommendationForExcludedVersions_AdditionalCases(t *testing.T) {
 				ResourceType: "db.t3.small",
 				Count:        10,
 				Region:       "us-east-1",
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine: "mysql",
 				},
 			},
@@ -380,7 +380,7 @@ func TestAdjustRecommendationForExcludedVersions_AdditionalCases(t *testing.T) {
 				ResourceType: "db.t3.small",
 				Count:        10,
 				Region:       "us-east-1",
-				Details: common.DatabaseDetails{
+				Details: &common.DatabaseDetails{
 					Engine: "mysql",
 				},
 			},
@@ -739,7 +739,7 @@ func TestFilterAndAdjustRecommendations_WithEngineVersionFiltering(t *testing.T)
 			ResourceType: "db.t3.small",
 			Count:        5,
 			Region:       "us-east-1",
-			Details: common.DatabaseDetails{
+			Details: &common.DatabaseDetails{
 				Engine: "mysql",
 			},
 		},

cmd/multi_service_csv.go

@@ -385,15 +385,12 @@ func formatNormalizedUnitsOrBlank(rec common.Recommendation) string {
 // operators need to see the deployment alongside the upfront figure to
 // confirm a $X upfront row is for the deployment they expect.
 //
-// Both value and pointer Details are accepted to mirror extractEngine
-// (parser path stores pointers; CSV-loader path constructs values).
+// DatabaseDetails is always a pointer: every producer (AWS, Azure, the CSV
+// loader, and the JSON codec used on the purchase-execution round trip)
+// constructs it that way -- see pkg/common/service_details_codec.go's
+// package doc for the invariant.
 func extractDeployment(rec common.Recommendation) string {
-	switch details := rec.Details.(type) {
-	case *common.DatabaseDetails:
-		if details != nil {
-			return details.AZConfig
-		}
-	case common.DatabaseDetails:
+	if details, ok := rec.Details.(*common.DatabaseDetails); ok && details != nil {
 		return details.AZConfig
 	}
 	return ""
@@ -404,25 +401,21 @@ func extractDeployment(rec common.Recommendation) string {
 // CacheDetails), Platform for EC2 (ComputeDetails), empty for SP and other
 // commitment types that don't carry an engine field.
 //
-// Both value and pointer Details are accepted because the parser stores
-// *DatabaseDetails / *CacheDetails / *ComputeDetails while the CSV-loader
-// path constructs the value forms; the dispatch in generatePurchaseID does
-// the same trick. Without the pointer cases the column silently blanks
-// every row coming from the live parser path.
+// DatabaseDetails/CacheDetails are always pointers (see extractDeployment's
+// godoc for the invariant). ComputeDetails is still accepted as a value
+// because the Azure compute client and the GCP compute-engine client both
+// construct it that way; without that case the column silently blanks every
+// Azure VM and GCP compute row.
 func extractEngine(rec common.Recommendation) string {
 	switch details := rec.Details.(type) {
 	case *common.DatabaseDetails:
 		if details != nil {
 			return details.Engine
 		}
-	case common.DatabaseDetails:
-		return details.Engine
 	case *common.CacheDetails:
 		if details != nil {
 			return details.Engine
 		}
-	case common.CacheDetails:
-		return details.Engine
 	case *common.ComputeDetails:
 		if details != nil {
 			return details.Platform

cmd/multi_service_csv_test.go

@@ -86,7 +86,7 @@ func TestWriteMultiServiceCSVReport(t *testing.T) {
 						EstimatedSavings:  100,
 						SavingsPercentage: 30,
 						Timestamp:         time.Now(),
-						Details: common.DatabaseDetails{
+						Details: &common.DatabaseDetails{
 							Engine:   "mysql",
 							AZConfig: "multi-az",
 						},
@@ -109,7 +109,7 @@ func TestWriteMultiServiceCSVReport(t *testing.T) {
 						ResourceType: "cache.t3.micro",
 						Count:        1,
 						Term:         "1yr",
-						Details: common.CacheDetails{
+						Details: &common.CacheDetails{
 							Engine:   "redis",
 							NodeType: "cache.t3.micro",
 						},
@@ -495,8 +495,9 @@ func TestFormatNormalizedUnitsOrBlank(t *testing.T) {
 // TestExtractDeployment covers the deployment-extraction helper used by
 // the RDS row in the CSV. Single-AZ / Multi-AZ is critical context for
 // pricing verification (Multi-AZ list price is ~2x Single-AZ) so the
-// column should land for every RDS rec regardless of which Details form
-// the upstream path used.
+// column should land for every RDS rec. DatabaseDetails is always a
+// pointer (every producer constructs it that way); no value-typed case
+// is needed.
 func TestExtractDeployment(t *testing.T) {
 	tests := []struct {
 		name string
@@ -505,7 +506,6 @@ func TestExtractDeployment(t *testing.T) {
 	}{
 		{"*DatabaseDetails Single-AZ", common.Recommendation{Details: &common.DatabaseDetails{AZConfig: "single-az"}}, "single-az"},
 		{"*DatabaseDetails Multi-AZ", common.Recommendation{Details: &common.DatabaseDetails{AZConfig: "multi-az"}}, "multi-az"},
-		{"DatabaseDetails (value) Multi-AZ", common.Recommendation{Details: common.DatabaseDetails{AZConfig: "multi-az"}}, "multi-az"},
 		{"DatabaseDetails empty AZConfig", common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}}, ""},
 		// Non-RDS Details → blank (column is RDS-only data).
 		{"CacheDetails -> empty", common.Recommendation{Details: &common.CacheDetails{Engine: "redis"}}, ""},
@@ -523,19 +523,22 @@ func TestExtractDeployment(t *testing.T) {
 // TestExtractEngine covers the four cases the helper dispatches on:
 // DatabaseDetails (RDS engine), CacheDetails (ElastiCache engine),
 // ComputeDetails (EC2 platform), and unset/other Details (blank).
+// DatabaseDetails/CacheDetails are always pointers (every producer
+// constructs them that way); ComputeDetails still accepts a value because
+// the Azure compute client and the GCP compute-engine client both construct
+// it that way.
 func TestExtractEngine(t *testing.T) {
 	tests := []struct {
 		name string
 		rec  common.Recommendation
 		want string
 	}{
-		// Pointer forms — what the live parser actually emits.
 		{"*DatabaseDetails -> Engine", common.Recommendation{Details: &common.DatabaseDetails{Engine: "aurora-postgresql"}}, "aurora-postgresql"},
 		{"*CacheDetails -> Engine", common.Recommendation{Details: &common.CacheDetails{Engine: "redis"}}, "redis"},
 		{"*ComputeDetails -> Platform", common.Recommendation{Details: &common.ComputeDetails{Platform: "Linux/UNIX"}}, "Linux/UNIX"},
-		// Value forms — what the CSV-loader path constructs.
-		{"DatabaseDetails (value) -> Engine", common.Recommendation{Details: common.DatabaseDetails{Engine: "mysql"}}, "mysql"},
-		{"CacheDetails (value) -> Engine", common.Recommendation{Details: common.CacheDetails{Engine: "memcached"}}, "memcached"},
+		// The Azure compute client and GCP's compute-engine client both
+		// still construct ComputeDetails as a value; keep coverage for
+		// that form.
 		{"ComputeDetails (value) -> Platform", common.Recommendation{Details: common.ComputeDetails{Platform: "Windows"}}, "Windows"},
 		// Fallbacks.
 		{"nil Details -> empty", common.Recommendation{}, ""},

cmd/multi_service_engine_versions.go

@@ -426,12 +426,17 @@ func adjustRecommendationForExcludedVersions(rec common.Recommendation, instance
 		return rec
 	}
 
-	// Get the engine name from the recommendation
+	// Get the engine name from the recommendation. DatabaseDetails is
+	// always a pointer (every producer constructs it that way; see
+	// pkg/common/service_details_codec.go's package doc). A typed nil
+	// pointer is treated like a non-RDS rec rather than panicking on the
+	// field read.
 	var recEngine string
 	switch details := rec.Details.(type) {
-	case common.DatabaseDetails:
-		recEngine = details.Engine
 	case *common.DatabaseDetails:
+		if details == nil {
+			return rec
+		}
 		recEngine = details.Engine
 	default:
 		return rec // Not RDS, no engine version filtering