fix(composite services): Don't use decision returned from inner service when it returns an error (#138)

Summary:

- In CompositeExperimentService and CompositeFeatureService GetDecision methods, when the inner service returns an error, skip that decision and move to the next inner service
- Return an error from CompositeExperimentService GetDecision when no decision was made

Test Plan:

Updated & added unit tests

Author: mjc1283

pkg/decision/composite_experiment_service.go

@@ -18,9 +18,15 @@
 package decision
 
 import (
+	"errors"
+	"fmt"
+
 	"github.com/optimizely/go-sdk/pkg/entities"
+	"github.com/optimizely/go-sdk/pkg/logging"
 )
 
+var ceLogger = logging.GetLogger("CompositeExperimentService")
+
 // CompositeExperimentService bridges together the various experiment decision services that ship by default with the SDK
 type CompositeExperimentService struct {
 	experimentServices []ExperimentService
@@ -47,10 +53,14 @@ func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisi
 
 	// Run through the various decision services until we get a decision
 	for _, experimentService := range s.experimentServices {
-		if decision, err := experimentService.GetDecision(decisionContext, userContext); decision.Variation != nil {
+		decision, err := experimentService.GetDecision(decisionContext, userContext)
+		if err != nil {
+			ceLogger.Debug(fmt.Sprintf("%v", err))
+		}
+		if decision.Variation != nil && err == nil {
 			return decision, err
 		}
 	}
 
-	return experimentDecision, nil
+	return experimentDecision, errors.New("no decision was made")
 }

pkg/decision/composite_experiment_service_test.go

@@ -17,6 +17,7 @@
 package decision
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
@@ -109,12 +110,46 @@ func (s *CompositeExperimentTestSuite) TestGetDecisionNoDecisionsMade() {
 	}
 	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext)
 
-	s.NoError(err)
+	s.Error(err)
 	s.Equal(expectedExperimentDecision2, decision)
 	s.mockExperimentService.AssertExpectations(s.T())
 	s.mockExperimentService2.AssertExpectations(s.T())
 }
 
+func (s *CompositeExperimentTestSuite) TestGetDecisionReturnsError() {
+	// Assert that we continue to the next inner service when an inner service GetDecision returns an error
+	testUserContext := entities.UserContext{
+		ID: "test_user_1",
+	}
+
+	testDecisionContext := ExperimentDecisionContext{
+		Experiment:    &testExp1114,
+		ProjectConfig: s.mockConfig,
+	}
+
+	shouldBeIgnoredDecision := ExperimentDecision{
+		Variation: &testExp1114Var2225,
+	}
+	s.mockExperimentService.On("GetDecision", testDecisionContext, testUserContext).Return(shouldBeIgnoredDecision, errors.New("Error making decision"))
+
+	expectedDecision := ExperimentDecision{
+		Variation: &testExp1114Var2226,
+	}
+	s.mockExperimentService2.On("GetDecision", testDecisionContext, testUserContext).Return(expectedDecision, nil)
+
+	compositeExperimentService := &CompositeExperimentService{
+		experimentServices: []ExperimentService{
+			s.mockExperimentService,
+			s.mockExperimentService2,
+		},
+	}
+	decision, err := compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
+	s.Equal(expectedDecision, decision)
+	s.NoError(err)
+	s.mockExperimentService.AssertExpectations(s.T())
+	s.mockExperimentService2.AssertExpectations(s.T())
+}
+
 func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentService() {
 	// Assert that the service is instantiated with the correct child services in the right order
 	compositeExperimentService := NewCompositeExperimentService()

pkg/decision/composite_feature_service.go

@@ -18,6 +18,7 @@
 package decision
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/optimizely/go-sdk/pkg/entities"
@@ -45,10 +46,13 @@ func NewCompositeFeatureService(compositeExperimentService ExperimentService) *C
 func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) {
 	for _, featureDecisionService := range f.featureServices {
 		featureDecision, err := featureDecisionService.GetDecision(decisionContext, userContext)
-		if featureDecision.Variation != nil {
+		if err != nil {
+			cfLogger.Debug(fmt.Sprintf("%v", err))
+		}
+		if featureDecision.Variation != nil && err == nil {
 			return featureDecision, err
 		}
 	}
 
-	return FeatureDecision{}, fmt.Errorf("no decision was made for feature %s", decisionContext.Feature.Key)
+	return FeatureDecision{}, errors.New("no decision was made")
 }

pkg/decision/composite_feature_service_test.go

@@ -17,6 +17,7 @@
 package decision
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/optimizely/go-sdk/pkg/decision/reasons"
@@ -98,6 +99,35 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionFallthrough() {
 	s.mockFeatureService2.AssertExpectations(s.T())
 }
 
+func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsError() {
+	// test that we move onto the next decision service if an inner service returns an error
+	testUserContext := entities.UserContext{
+		ID: "test_user_1",
+	}
+
+	shouldBeIgnoredDecision := FeatureDecision{
+		Variation: &testExp1113Var2223,
+	}
+	s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext).Return(shouldBeIgnoredDecision, errors.New("Error making decision"))
+
+	expectedDecision := FeatureDecision{
+		Variation: &testExp1113Var2224,
+	}
+	s.mockFeatureService2.On("GetDecision", s.testFeatureDecisionContext, testUserContext).Return(expectedDecision, nil)
+
+	compositeFeatureService := &CompositeFeatureService{
+		featureServices: []FeatureService{
+			s.mockFeatureService,
+			s.mockFeatureService2,
+		},
+	}
+	decision, err := compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext)
+	s.Equal(expectedDecision, decision)
+	s.NoError(err)
+	s.mockFeatureService.AssertExpectations(s.T())
+	s.mockFeatureService2.AssertExpectations(s.T())
+}
+
 func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() {
 	// Assert that the service is instantiated with the correct child services in the right order
 	compositeExperimentService := NewCompositeExperimentService()