merge in mpirnovar-fsc-failures-fix-fssdk-11649

Author: Mat001

pkg/client/client.go

@@ -1295,5 +1295,14 @@ func isNil(v interface{}) bool {
 func (o *OptimizelyClient) handleDecisionServiceError(err error, key string, userContext OptimizelyUserContext) OptimizelyDecision {
 	o.logger.Warning(fmt.Sprintf(`Received error while making a decision for feature %q: %s`, key, err))
 
-	return NewErrorDecision(key, userContext, err)
+	// Return the error decision with the correct format for decision fields
+	return OptimizelyDecision{
+		FlagKey:      key,
+		UserContext:  userContext,
+		VariationKey: "",
+		RuleKey:      "",
+		Enabled:      false,
+		Variables:    optimizelyjson.NewOptimizelyJSONfromMap(map[string]interface{}{}),
+		Reasons:      []string{err.Error()},
+	}
 }

pkg/client/client_test.go

@@ -29,7 +29,6 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
 
-	"github.com/optimizely/go-sdk/v2/pkg/cmab"
 	"github.com/optimizely/go-sdk/v2/pkg/config"
 	"github.com/optimizely/go-sdk/v2/pkg/decide"
 	"github.com/optimizely/go-sdk/v2/pkg/decision"
@@ -3187,153 +3186,34 @@ func (s *ClientTestSuiteTrackNotification) TestRemoveOnTrackThrowsErrorWhenRemov
 	mockNotificationCenter.AssertExpectations(s.T())
 }
 
-// MockCmabService for testing CMAB functionality
-type MockCmabService struct {
-	mock.Mock
-}
-
-// GetDecision safely implements the cmab.Service interface
-func (m *MockCmabService) GetDecision(projectConfig config.ProjectConfig, userContext entities.UserContext, ruleID string, options *decide.Options) (cmab.Decision, error) {
-	args := m.Called(projectConfig, userContext, ruleID, options)
-
-	// IMPORTANT: Return a valid Decision struct with non-nil Reasons slice
-	decision, ok := args.Get(0).(cmab.Decision)
-	if !ok {
-		// If conversion fails, return a safe default
-		return cmab.Decision{Reasons: []string{"Mock conversion failed"}}, args.Error(1)
+func TestOptimizelyClient_handleDecisionServiceError(t *testing.T) {
+	// Create the client
+	client := &OptimizelyClient{
+		logger: logging.GetLogger("", ""),
 	}
 
-	// Make sure Reasons is never nil
-	if decision.Reasons == nil {
-		decision.Reasons = []string{}
-	}
-
-	return decision, args.Error(1)
-}
+	// Create a CMAB error
+	cmabErrorMessage := "Failed to fetch CMAB data for experiment exp_1."
+	cmabError := fmt.Errorf(cmabErrorMessage)
 
-func TestDecide_CmabSuccess(t *testing.T) {
-	// Use the existing Mock types
-	mockConfig := new(MockProjectConfig)
-	mockConfigManager := new(MockProjectConfigManager)
-	mockEventProcessor := new(MockProcessor)
-	mockCmabService := new(MockCmabService)
-	mockDecisionService := new(MockDecisionService)
-	mockNotificationCenter := new(MockNotificationCenter)
-
-	// Test data
-	featureKey := "test_feature"
-	experimentID := "exp_1"
-	variationID := "var_1"
-
-	// Create feature with experiment IDs
-	testFeature := entities.Feature{
-		Key:           featureKey,
-		ExperimentIDs: []string{experimentID},
+	// Create a user context - needs to match the signature expected by handleDecisionServiceError
+	testUserContext := OptimizelyUserContext{
+		UserID:     "test_user",
+		Attributes: map[string]interface{}{},
 	}
 
-	// Create variation
-	testVariation := entities.Variation{
-		ID:             variationID,
-		Key:            "variation_1",
-		FeatureEnabled: true,
-	}
-
-	// Create experiment with CMAB data
-	testExperiment := entities.Experiment{
-		ID:  experimentID,
-		Key: "exp_key",
-		Cmab: &entities.Cmab{
-			TrafficAllocation: 10000,
-		},
-		Variations: map[string]entities.Variation{
-			variationID: testVariation,
-		},
-	}
-
-	// Mock GetConfig call
-	mockConfigManager.On("GetConfig").Return(mockConfig, nil)
-
-	// Log and track calls to GetExperimentByID
-	experimentCalls := make([]string, 0)
-	mockConfig.On("GetExperimentByID", mock.Anything).Return(testExperiment, nil).Run(
-		func(args mock.Arguments) {
-			id := args.Get(0).(string)
-			experimentCalls = append(experimentCalls, id)
-			t.Logf("GetExperimentByID called with: %s", id)
-		})
-
-	// Mock GetFeatureByKey
-	mockConfig.On("GetFeatureByKey", featureKey).Return(testFeature, nil)
-
-	// Track calls to CMAB service
-	cmabCalls := make([]string, 0)
-	mockCmabService.On("GetDecision", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-		Return(cmab.Decision{VariationID: variationID, CmabUUID: "uuid"}, nil).
-		Run(func(args mock.Arguments) {
-			id := args.Get(2).(string)
-			cmabCalls = append(cmabCalls, id)
-			t.Logf("GetDecision called with id: %s", id)
-		})
-
-	// Mock event processor
-	mockEventProcessor.On("ProcessEvent", mock.Anything).Return(true)
-
-	// Mock notification center
-	mockNotificationCenter.On("Send", notification.Decision, mock.Anything).Return(nil)
-
-	// Let's add every field to client to be sure
-	client := OptimizelyClient{
-		ConfigManager:        mockConfigManager,
-		DecisionService:      mockDecisionService,
-		EventProcessor:       mockEventProcessor,
-		notificationCenter:   mockNotificationCenter,
-		cmabService:          mockCmabService,
-		logger:               logging.GetLogger("debug", "TestCMAB"),
-		ctx:                  context.Background(),
-		tracer:               &MockTracer{},
-		defaultDecideOptions: &decide.Options{},
-	}
-
-	// Create user context
-	userContext := client.CreateUserContext("test_user", nil)
-
-	// Wrap the call in a panic handler
-	var decision OptimizelyDecision
-	var panicOccurred bool
-	var panicValue interface{}
-
-	func() {
-		defer func() {
-			if r := recover(); r != nil {
-				panicOccurred = true
-				panicValue = r
-				t.Logf("Panic occurred: %v", r)
-			}
-		}()
-		decision = client.decide(&userContext, featureKey, nil)
-	}()
-
-	t.Logf("Panic occurred: %v", panicOccurred)
-	if panicOccurred {
-		t.Logf("Panic value: %v", panicValue)
-	}
-	t.Logf("GetExperimentByID calls: %v", experimentCalls)
-	t.Logf("GetDecision calls: %v", cmabCalls)
-	t.Logf("Decision: %+v", decision)
+	// Call the error handler directly
+	decision := client.handleDecisionServiceError(cmabError, "test_flag", testUserContext)
 
-	// Skip further assertions if we panicked
-	if panicOccurred {
-		t.Log("Test skipping assertions due to panic")
-		return
-	}
+	// Verify the decision is correctly formatted
+	assert.False(t, decision.Enabled)
+	assert.Equal(t, "", decision.VariationKey) // Should be empty string, not nil
+	assert.Equal(t, "", decision.RuleKey)      // Should be empty string, not nil
+	assert.Contains(t, decision.Reasons, cmabErrorMessage)
 
-	// Basic assertions on the decision
-	if len(cmabCalls) > 0 {
-		assert.Equal(t, featureKey, decision.FlagKey)
-		assert.Equal(t, "variation_1", decision.VariationKey)
-		assert.Equal(t, "exp_key", decision.RuleKey)
-		assert.True(t, decision.Enabled)
-	}
+	// Check that reasons contains exactly the expected message
+	assert.Equal(t, 1, len(decision.Reasons), "Reasons array should have exactly one item")
+	assert.Equal(t, cmabErrorMessage, decision.Reasons[0], "Error message should be added verbatim")
 }
 
 func TestClientTestSuiteAB(t *testing.T) {

pkg/cmab/errors.go

@@ -0,0 +1,22 @@
+/****************************************************************************
+ * Copyright 2025, Optimizely, Inc. and contributors                   		*
+ *                                                                          *
+ * Licensed under the Apache License, Version 2.0 (the "License");          *
+ * you may not use this file except in compliance with the License.         *
+ * You may obtain a copy of the License at                                  *
+ *                                                                          *
+ *    http://www.apache.org/licenses/LICENSE-2.0                            *
+ *                                                                          *
+ * Unless required by applicable law or agreed to in writing, software      *
+ * distributed under the License is distributed on an "AS IS" BASIS,        *
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. *
+ * See the License for the specific language governing permissions and      *
+ * limitations under the License.                                           *
+ ***************************************************************************/
+
+// Package cmab to define cmab errors//
+package cmab
+
+// CmabFetchFailed is the error message format for CMAB fetch failures
+// Format required for FSC test compatibility - capitalized and with period
+const CmabFetchFailed = "Failed to fetch CMAB data for experiment %s." //nolint:ST1005 // Required exact format for FSC test compatibility

pkg/cmab/service.go

@@ -19,6 +19,7 @@ package cmab
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strconv"
 
@@ -137,8 +138,9 @@ func (s *DefaultCmabService) GetDecision(
 	// Fetch new decision
 	decision, err := s.fetchDecision(ruleID, userContext.ID, filteredAttributes)
 	if err != nil {
-		// properly propagate error reasons in Decision object
-		return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err)
+		// Append existing reasons and return the error as-is (already formatted correctly)
+		decision.Reasons = append(reasons, decision.Reasons...)
+		return decision, err
 	}
 
 	// Cache the decision
@@ -168,8 +170,11 @@ func (s *DefaultCmabService) fetchDecision(
 
 	variationID, err := s.cmabClient.FetchDecision(ruleID, userID, attributes, cmabUUID)
 	if err != nil {
-		reasons = append(reasons, "Failed to fetch CMAB decision")
-		return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err)
+		// Use the consistent error message format from errors.go
+		reason := fmt.Sprintf(CmabFetchFailed, ruleID)
+		reasons = append(reasons, reason)
+		// Use same format for Go error - FSC compatibility takes precedence
+		return Decision{Reasons: reasons}, errors.New(reason) //nolint:ST1005 // Required exact format for FSC test compatibility
 	}
 
 	reasons = append(reasons, "Successfully fetched CMAB decision")

pkg/cmab/service_test.go

@@ -853,34 +853,38 @@ func TestCmabServiceTestSuite(t *testing.T) {
 }
 
 func (s *CmabServiceTestSuite) TestGetDecisionApiError() {
-	// Setup cache key
-	cacheKey := s.cmabService.getCacheKey(s.testUserID, s.testRuleID)
-
-	// Setup cache lookup (cache miss)
-	s.mockCache.On("Lookup", cacheKey).Return(nil)
-
-	// Setup mock to return error for experiment lookup (but this won't stop the flow anymore)
-	s.mockConfig.On("GetExperimentByID", s.testRuleID).Return(entities.Experiment{}, fmt.Errorf("experiment not found")).Once()
-
-	// Mock the FetchDecision call that will now happen
-	s.mockClient.On("FetchDecision", s.testRuleID, s.testUserID, mock.Anything, mock.Anything).Return("", fmt.Errorf("invalid rule ID"))
-
-	// Call the method
-	userContext := entities.UserContext{
-		ID: s.testUserID,
-		Attributes: map[string]interface{}{
-			"age": 30,
-		},
-	}
-
-	_, err := s.cmabService.GetDecision(s.mockConfig, userContext, s.testRuleID, nil)
-
-	// Should return error from FetchDecision, not from experiment validation
-	s.Error(err)
-	s.Contains(err.Error(), "CMAB API error")
-
-	// Verify expectations
-	s.mockConfig.AssertExpectations(s.T())
-	s.mockCache.AssertExpectations(s.T())
-	s.mockClient.AssertExpectations(s.T())
+    // Setup experiment with CMAB config
+    experiment := entities.Experiment{
+        ID: "rule-123",
+        Cmab: &entities.Cmab{
+            AttributeIds: []string{"attr1"},
+        },
+    }
+    s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil)
+    s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil)
+
+    // Configure client to return error
+    s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error"))
+
+    // Setup cache miss
+    cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123")
+    s.mockCache.On("Lookup", cacheKey).Return(nil)
+
+    userContext := entities.UserContext{
+        ID: s.testUserID,
+        Attributes: map[string]interface{}{
+            "category": "cmab",
+        },
+    }
+
+    decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil)
+
+    // Should return the exact error format expected by FSC tests
+    s.Error(err)
+    s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation
+    s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.")
+
+    s.mockConfig.AssertExpectations(s.T())
+    s.mockCache.AssertExpectations(s.T())
+    s.mockClient.AssertExpectations(s.T())
 }

pkg/decision/composite_feature_service.go

@@ -18,6 +18,8 @@
 package decision
 
 import (
+	"strings"
+
 	"github.com/optimizely/go-sdk/v2/pkg/decide"
 	"github.com/optimizely/go-sdk/v2/pkg/entities"
 	"github.com/optimizely/go-sdk/v2/pkg/logging"
@@ -51,6 +53,21 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont
 		reasons.Append(decisionReasons)
 		if err != nil {
 			f.logger.Debug(err.Error())
+			// Check if this is a CMAB error - if so, stop the loop and return empty decision
+			if strings.Contains(err.Error(), "Failed to fetch CMAB data") {
+				// Add the CMAB error to reasons before returning - use AddError for critical failures
+				reasons.AddError(err.Error())
+				return FeatureDecision{}, reasons, nil // Return empty decision for CMAB errors
+			}
+		}
+
+		// Also check for CMAB errors in decision reasons (when err is nil)
+		if decisionReasons != nil {
+			for _, reason := range decisionReasons.ToReport() {
+				if strings.Contains(reason, "Failed to fetch CMAB data") {
+					return FeatureDecision{}, reasons, nil
+				}
+			}
 		}
 
 		if featureDecision.Variation != nil && err == nil {