From 9bf91d0e0124374039db32ba89e737be2f17a48c Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Wed, 20 May 2026 02:04:07 +0200
Subject: [PATCH] fix(recommendations): parseMemoryDBDetails uses
 rec.ResourceType, errors loudly on empty

Previously, the function either hardcoded db.r6gd.xlarge (original) or
silently returned nil without populating Details (feat branch). Both
paths caused MemoryDB RI purchases to silently fail when the rec's
actual instance type was anything other than the hardcoded default.

Now the function errors loudly when rec.ResourceType is empty (so the
recommendation is logged as a warning by parseRecommendations and
skipped), and populates CacheDetails correctly when ResourceType is set.

Also removes the unused log import and updates TestParseMemoryDBDetails
to cover the empty-ResourceType error path and two valid instance types.

Closes #378
---
 .../aws/recommendations/parser_services.go    | 23 +++++++++---
 .../recommendations/parser_services_test.go   | 36 ++++++++++++++-----
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/providers/aws/recommendations/parser_services.go b/providers/aws/recommendations/parser_services.go
index 1ad78486..7d0be7b4 100644
--- a/providers/aws/recommendations/parser_services.go
+++ b/providers/aws/recommendations/parser_services.go
@@ -2,7 +2,6 @@ package recommendations
 
 import (
 	"fmt"
-	"log"
 	"strings"
 
 	"github.com/aws/aws-sdk-go-v2/service/costexplorer/types"
@@ -156,8 +155,24 @@ func (c *Client) parseRedshiftDetails(rec *common.Recommendation, details *types
 	return nil
 }
 
-// parseMemoryDBDetails extracts MemoryDB-specific details
-func (c *Client) parseMemoryDBDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error {
-	log.Printf("WARNING: MemoryDB does not provide instance details in Cost Explorer, skipping recommendation")
+// parseMemoryDBDetails extracts MemoryDB-specific details.
+//
+// Cost Explorer does not populate InstanceDetails for MemoryDB reserved nodes:
+// the MemoryDB-specific sub-struct does not exist in the AWS SDK's
+// ReservationPurchaseRecommendationDetail, so the instance type must come from
+// rec.ResourceType, which the upstream caller should have populated from the
+// recommendation summary fields before dispatching here.
+//
+// If rec.ResourceType is empty, the function returns an error so the
+// recommendation is skipped loudly (logged by parseRecommendations) rather
+// than silently substituting a wrong default instance type.
+func (c *Client) parseMemoryDBDetails(rec *common.Recommendation, _ *types.ReservationPurchaseRecommendationDetail) error {
+	if rec.ResourceType == "" {
+		return fmt.Errorf("MemoryDB recommendation has no ResourceType; cannot determine offering — Cost Explorer did not populate instance details")
+	}
+	rec.Details = &common.CacheDetails{
+		Engine:   "redis",
+		NodeType: rec.ResourceType,
+	}
 	return nil
 }
diff --git a/providers/aws/recommendations/parser_services_test.go b/providers/aws/recommendations/parser_services_test.go
index 81fdbf47..f155b5bb 100644
--- a/providers/aws/recommendations/parser_services_test.go
+++ b/providers/aws/recommendations/parser_services_test.go
@@ -497,15 +497,33 @@ func TestParseRedshiftDetails(t *testing.T) {
 
 func TestParseMemoryDBDetails(t *testing.T) {
 	client := &Client{}
-
-	rec := &common.Recommendation{}
 	details := &types.ReservationPurchaseRecommendationDetail{}
 
-	err := client.parseMemoryDBDetails(rec, details)
-
-	// parseMemoryDBDetails logs a warning and skips the recommendation
-	// (MemoryDB does not expose instance details in Cost Explorer)
-	require.NoError(t, err)
-	assert.Empty(t, rec.ResourceType, "ResourceType should not be set when MemoryDB details are unavailable")
-	assert.Nil(t, rec.Details, "Details should remain nil when MemoryDB details are unavailable")
+	t.Run("empty ResourceType returns error", func(t *testing.T) {
+		rec := &common.Recommendation{}
+		err := client.parseMemoryDBDetails(rec, details)
+		require.Error(t, err, "should fail loudly when ResourceType is empty")
+		assert.Contains(t, err.Error(), "ResourceType")
+		assert.Nil(t, rec.Details, "Details should remain nil on error")
+	})
+
+	t.Run("non-default instance type populates Details", func(t *testing.T) {
+		rec := &common.Recommendation{ResourceType: "db.r6g.large"}
+		err := client.parseMemoryDBDetails(rec, details)
+		require.NoError(t, err)
+		cacheDetails, ok := rec.Details.(*common.CacheDetails)
+		require.True(t, ok, "Details should be *common.CacheDetails")
+		assert.Equal(t, "db.r6g.large", cacheDetails.NodeType)
+		assert.Equal(t, "redis", cacheDetails.Engine)
+	})
+
+	t.Run("xlarge instance type populates Details", func(t *testing.T) {
+		rec := &common.Recommendation{ResourceType: "db.r6gd.xlarge"}
+		err := client.parseMemoryDBDetails(rec, details)
+		require.NoError(t, err)
+		cacheDetails, ok := rec.Details.(*common.CacheDetails)
+		require.True(t, ok, "Details should be *common.CacheDetails")
+		assert.Equal(t, "db.r6gd.xlarge", cacheDetails.NodeType)
+		assert.Equal(t, "redis", cacheDetails.Engine)
+	})
 }