From 8951915ee85dde7f5521a2bd1504fe7d70a3ef6b Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Thu, 22 Aug 2019 21:06:21 +0500 Subject: [PATCH 1/6] getFeatureVariableString API implemented. --- optimizely/client/client.go | 111 +++++++++ optimizely/client/client_test.go | 232 ++++++++++++++++++ .../config/datafileprojectconfig/config.go | 15 +- .../entities/entities.go | 32 ++- .../mappers/experiment.go | 1 + .../datafileprojectconfig/mappers/feature.go | 17 +- .../mappers/feature_test.go | 29 ++- optimizely/entities/experiment.go | 5 + optimizely/event/processor_test.go | 2 +- optimizely/interface.go | 2 + 10 files changed, 434 insertions(+), 12 deletions(-) diff --git a/optimizely/client/client.go b/optimizely/client/client.go index c0e5c7e7e..e271d3ee4 100644 --- a/optimizely/client/client.go +++ b/optimizely/client/client.go @@ -22,6 +22,7 @@ import ( "fmt" "reflect" "runtime/debug" + "strconv" "github.com/optimizely/go-sdk/optimizely/event" @@ -78,6 +79,7 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit userID := userContext.ID logger.Debug(fmt.Sprintf(`Evaluating feature "%s" for user "%s".`, featureKey, userID)) featureDecision, err := o.decisionService.GetFeatureDecision(featureDecisionContext, userContext) + if err != nil { logger.Error("Received an error while computing feature decision", err) return result, err @@ -133,6 +135,115 @@ func (o *OptimizelyClient) GetEnabledFeatures(userContext entities.UserContext) return enabledFeatures, nil } +// GetFeatureVariableString returns string feature variable value +func (o *OptimizelyClient) GetFeatureVariableString(featureKey string, variableKey string, userContext entities.UserContext) (value string, err error) { + val, err := o.getFeatureVariable("", featureKey, variableKey, userContext) + if err == nil { + switch val.(type) { + case string: + return val.(string), err + default: + break + } + } + return "", err +} + +func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) { + if !o.isValid { + errorMessage := "Optimizely instance is not valid. Failing getFeatureVariable." + err := errors.New(errorMessage) + logger.Error(errorMessage, nil) + return nil, err + } + + defer func() { + if r := recover(); r != nil { + errorMessage := fmt.Sprintf(`Optimizely SDK is panicking with the error "%s"`, string(debug.Stack())) + err = errors.New(errorMessage) + logger.Error(errorMessage, err) + } + }() + + projectConfig := o.configManager.GetConfig() + + if reflect.ValueOf(projectConfig).IsNil() { + return nil, fmt.Errorf("project config is null") + } + + featureFlag, err := projectConfig.GetFeatureFlagByKey(featureKey) + if err != nil { + logger.Error("Error retrieving feature flag", err) + return nil, err + } + variable, err := featureFlag.GetVariable(variableKey) + if err != nil { + logger.Error("Error retrieving variable", err) + return nil, err + } + + var featureValue = variable.DefaultValue + + feature, err := projectConfig.GetFeatureByKey(featureKey) + if err != nil { + logger.Error("Error retrieving feature", err) + return nil, err + } + featureDecisionContext := decision.FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: projectConfig, + } + + featureDecision, err := o.decisionService.GetFeatureDecision(featureDecisionContext, userContext) + if err == nil { + for _, v := range featureDecision.Variation.Variables { + if v.ID == variable.ID && featureDecision.Variation.FeatureEnabled { + featureValue = v.Value + break + } + } + } + + var typeName = "" + var valueParsed interface{} + switch valueType.(type) { + case string: + typeName = "string" + valueParsed = featureValue + break + case int: + typeName = "integer" + convertedValue, err := strconv.Atoi(featureValue) + if err == nil { + valueParsed = convertedValue + } + break + case float64: + typeName = "double" + convertedValue, err := strconv.ParseFloat(featureValue, 64) + if err == nil { + valueParsed = convertedValue + } + break + case bool: + typeName = "boolean" + convertedValue, err := strconv.ParseBool(featureValue) + if err == nil { + valueParsed = convertedValue + } + break + default: + break + } + + if valueParsed == nil || variable.Type != typeName { + return nil, fmt.Errorf("Variable value for key %s is invalid or wrong type", variableKey) + } + + // @TODO(yasir): send decision notification + return valueParsed, nil +} + // Close closes the Optimizely instance and stops any ongoing tasks from its children components func (o *OptimizelyClient) Close() { o.cancelFunc() diff --git a/optimizely/client/client_test.go b/optimizely/client/client_test.go index e282b7125..f05b2e865 100644 --- a/optimizely/client/client_test.go +++ b/optimizely/client/client_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/optimizely/go-sdk/optimizely" + datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/decision" "github.com/optimizely/go-sdk/optimizely/entities" "github.com/stretchr/testify/mock" @@ -43,6 +44,11 @@ func (c *MockProjectConfig) GetFeatureList() []entities.Feature { return args.Get(0).([]entities.Feature) } +func (c *MockProjectConfig) GetFeatureFlagByKey(featureKey string) (datafileEntities.FeatureFlag, error) { + args := c.Called(featureKey) + return args.Get(0).(datafileEntities.FeatureFlag), args.Error(1) +} + type MockProjectConfigManager struct { mock.Mock } @@ -297,3 +303,229 @@ func TestGetEnabledFeaturesPanic(t *testing.T) { assert.Empty(t, result) assert.True(t, assert.Error(t, err)) } + +func TestGetFeatureVariableStringWithValidValue(t *testing.T) { + testFeatureKey := "test_feature_key" + testFeatureFlagKey := "test_feature_flag_key" + testVariableValue := "teststring" + testUserContext := entities.UserContext{ID: "test_user_1"} + testVariationVariable := datafileEntities.VariationVariable{ + ID: "1", + Value: testVariableValue, + } + testVariable := datafileEntities.Variable{ + DefaultValue: "default", + ID: "1", + Key: "test_feature_flag_key", + Type: "string"} + testVariation := getTestVariationWithFeatureVariable(true, testVariationVariable) + testExperiment := entities.Experiment{ + ID: "111111", + Variations: map[string]entities.Variation{"22222": testVariation}, + } + testFeature := getTestFeature(testFeatureKey, testExperiment) + testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) + mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfigManager := new(MockProjectConfigManager) + mockConfigManager.On("GetConfig").Return(mockConfig) + + testDecisionContext := decision.FeatureDecisionContext{ + Feature: &testFeature, + ProjectConfig: mockConfig, + } + + expectedFeatureDecision := getTestFeatureDecision(testExperiment, testVariation, true) + mockDecisionService := new(MockDecisionService) + mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, nil) + + client := OptimizelyClient{ + configManager: mockConfigManager, + decisionService: mockDecisionService, + isValid: true, + } + result, _ := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + assert.Equal(t, testVariableValue, result) + mockConfig.AssertExpectations(t) + mockConfigManager.AssertExpectations(t) + mockDecisionService.AssertExpectations(t) +} + +func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { + testFeatureKey := "test_feature_key" + testFeatureFlagKey := "test_feature_flag_key" + testVariableValue := "true" + testUserContext := entities.UserContext{ID: "test_user_1"} + testVariationVariable := datafileEntities.VariationVariable{ + ID: "1", + Value: testVariableValue, + } + testVariable := datafileEntities.Variable{ + DefaultValue: "default", + ID: "1", + Key: "test_feature_flag_key", + Type: "boolean"} + testVariation := getTestVariationWithFeatureVariable(true, testVariationVariable) + testExperiment := entities.Experiment{ + ID: "111111", + Variations: map[string]entities.Variation{"22222": testVariation}, + } + testFeature := getTestFeature(testFeatureKey, testExperiment) + testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) + mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfigManager := new(MockProjectConfigManager) + mockConfigManager.On("GetConfig").Return(mockConfig) + + testDecisionContext := decision.FeatureDecisionContext{ + Feature: &testFeature, + ProjectConfig: mockConfig, + } + + expectedFeatureDecision := getTestFeatureDecision(testExperiment, testVariation, true) + mockDecisionService := new(MockDecisionService) + mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, nil) + + client := OptimizelyClient{ + configManager: mockConfigManager, + decisionService: mockDecisionService, + isValid: true, + } + result, err := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + assert.Equal(t, "", result) + assert.NotNil(t, err) + mockConfig.AssertExpectations(t) + mockConfigManager.AssertExpectations(t) + mockDecisionService.AssertExpectations(t) +} + +func TestGetFeatureVariableStringReturnsDefaultValueIfFeatureNotEnabled(t *testing.T) { + testFeatureKey := "test_feature_key" + testFeatureFlagKey := "test_feature_flag_key" + testVariableValue := "teststring" + testUserContext := entities.UserContext{ID: "test_user_1"} + testVariationVariable := datafileEntities.VariationVariable{ + ID: "1", + Value: testVariableValue, + } + testVariable := datafileEntities.Variable{ + DefaultValue: "defaultString", + ID: "1", + Key: "test_feature_flag_key", + Type: "string"} + testVariation := getTestVariationWithFeatureVariable(false, testVariationVariable) + testExperiment := entities.Experiment{ + ID: "111111", + Variations: map[string]entities.Variation{"22222": testVariation}, + } + testFeature := getTestFeature(testFeatureKey, testExperiment) + testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) + mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfigManager := new(MockProjectConfigManager) + mockConfigManager.On("GetConfig").Return(mockConfig) + + testDecisionContext := decision.FeatureDecisionContext{ + Feature: &testFeature, + ProjectConfig: mockConfig, + } + + expectedFeatureDecision := getTestFeatureDecision(testExperiment, testVariation, true) + mockDecisionService := new(MockDecisionService) + mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, nil) + + client := OptimizelyClient{ + configManager: mockConfigManager, + decisionService: mockDecisionService, + isValid: true, + } + result, err := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + assert.Equal(t, "defaultString", result) + assert.Nil(t, err) + mockConfig.AssertExpectations(t) + mockConfigManager.AssertExpectations(t) + mockDecisionService.AssertExpectations(t) +} + +func TestGetFeatureVariableErrorCases(t *testing.T) { + testUserContext := entities.UserContext{ID: "test_user_1"} + + mockConfigManager := new(MockProjectConfigManager) + mockDecisionService := new(MockDecisionService) + + client := OptimizelyClient{ + configManager: mockConfigManager, + decisionService: mockDecisionService, + isValid: false, + } + _, err1 := client.GetFeatureVariableString("test_feature_key", "test_variable_key", testUserContext) + assert.Error(t, err1) + mockConfigManager.AssertNotCalled(t, "GetFeatureFlagByKey") + mockConfigManager.AssertNotCalled(t, "GetFeatureByKey") + mockDecisionService.AssertNotCalled(t, "GetFeatureDecision") +} + +func TestGetFeatureVariableStringPanic(t *testing.T) { + testUserContext := entities.UserContext{ID: "test_user_1"} + testFeatureKey := "test_feature_key" + testVariableKey := "test_variable_key" + + mockConfigManager := new(MockProjectConfigManager) + mockDecisionService := new(MockDecisionService) + + client := OptimizelyClient{ + configManager: mockConfigManager, + decisionService: mockDecisionService, + isValid: true, + } + + // returning an error object will cause the Client to panic + mockConfigManager.On("GetFeatureByKey", testFeatureKey, testUserContext).Return(errors.New("failure")) + + // ensure that the client calms back down and recovers + result, err := client.GetFeatureVariableString(testFeatureKey, testVariableKey, testUserContext) + assert.Equal(t, "", result) + assert.True(t, assert.Error(t, err)) +} + +// Helper Methods +func getTestFeatureDecision(experiment entities.Experiment, variation entities.Variation, decisionMade bool) decision.FeatureDecision { + return decision.FeatureDecision{ + Experiment: experiment, + Variation: variation, + Decision: decision.Decision{ + DecisionMade: decisionMade, + }, + } +} + +func getTestVariationWithFeatureVariable(featureEnabled bool, featureVariable datafileEntities.VariationVariable) entities.Variation { + return entities.Variation{ + ID: "22222", + Key: "22222", + FeatureEnabled: featureEnabled, + Variables: []datafileEntities.VariationVariable{featureVariable}, + } +} + +func getTestFeatureFlag(featureFlagKey string, variable datafileEntities.Variable) datafileEntities.FeatureFlag { + return datafileEntities.FeatureFlag{ + ID: "21111", + Key: featureFlagKey, + RolloutID: "41111", + ExperimentIDs: []string{"31111", "31112"}, + Variables: []datafileEntities.Variable{variable}, + } +} + +func getMockConfig(featureKey string, feature entities.Feature, featureFlag datafileEntities.FeatureFlag) *MockProjectConfig { + mockConfig := new(MockProjectConfig) + mockConfig.On("GetFeatureByKey", featureKey).Return(feature, nil) + mockConfig.On("GetFeatureFlagByKey", featureKey).Return(featureFlag, nil) + return mockConfig +} + +func getTestFeature(featureKey string, experiment entities.Experiment) entities.Feature { + return entities.Feature{ + ID: "22222", + Key: featureKey, + FeatureExperiments: []entities.Experiment{experiment}, + } +} diff --git a/optimizely/config/datafileprojectconfig/config.go b/optimizely/config/datafileprojectconfig/config.go index 8a38e5165..35447a3f6 100644 --- a/optimizely/config/datafileprojectconfig/config.go +++ b/optimizely/config/datafileprojectconfig/config.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" + datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/mappers" "github.com/optimizely/go-sdk/optimizely/entities" "github.com/optimizely/go-sdk/optimizely/logging" @@ -39,6 +40,7 @@ type DatafileProjectConfig struct { experimentKeyToIDMap map[string]string experimentMap map[string]entities.Experiment featureMap map[string]entities.Feature + featureFlagMap map[string]datafileEntities.FeatureFlag groupMap map[string]entities.Group projectID string revision string @@ -94,7 +96,8 @@ func NewDatafileProjectConfig(jsonDatafile []byte) (*DatafileProjectConfig, erro experimentMap: experimentMap, experimentKeyToIDMap: experimentKeyMap, rolloutMap: rolloutMap, - featureMap: mappers.MapFeatureFlags(datafile.FeatureFlags, rolloutMap, experimentMap), + featureMap: mappers.MapFeatures(datafile.FeatureFlags, rolloutMap, experimentMap), + featureFlagMap: mappers.MapFeatureFlags(datafile.FeatureFlags), } logger.Info("Datafile is valid.") @@ -121,6 +124,16 @@ func (c DatafileProjectConfig) GetFeatureByKey(featureKey string) (entities.Feat return entities.Feature{}, errors.New(errMessage) } +// GetFeatureFlagByKey returns the featureflag with the given key +func (c DatafileProjectConfig) GetFeatureFlagByKey(featureKey string) (datafileEntities.FeatureFlag, error) { + if feature, ok := c.featureFlagMap[featureKey]; ok { + return feature, nil + } + + errMessage := fmt.Sprintf("FeatureFlag with key %s not found", featureKey) + return datafileEntities.FeatureFlag{}, errors.New(errMessage) +} + // GetAttributeByKey returns the attribute with the given key func (c DatafileProjectConfig) GetAttributeByKey(key string) (entities.Attribute, error) { if attributeID, ok := c.attributeKeyToIDMap[key]; ok { diff --git a/optimizely/config/datafileprojectconfig/entities/entities.go b/optimizely/config/datafileprojectconfig/entities/entities.go index 1a16c7986..6eb7d286f 100644 --- a/optimizely/config/datafileprojectconfig/entities/entities.go +++ b/optimizely/config/datafileprojectconfig/entities/entities.go @@ -16,6 +16,10 @@ package entities +import ( + "fmt" +) + // Audience represents an Audience object from the Optimizely datafile type Audience struct { ID string `json:"id"` @@ -51,6 +55,20 @@ type FeatureFlag struct { Variables []Variable `json:"variables"` } +// GetVariable returns variable for key from feature flag +func (f *FeatureFlag) GetVariable(key string) (Variable, error) { + var variable Variable + var err = fmt.Errorf("Variable with key %s not found", key) + for _, v := range f.Variables { + if v.Key == key { + variable = v + err = nil + break + } + } + return variable, err +} + // Variable represents a Variable object from the Optimizely datafile type Variable struct { DefaultValue string `json:"defaultValue"` @@ -67,10 +85,16 @@ type trafficAllocation struct { // Variation represents an experiment variation from the Optimizely datafile type Variation struct { - ID string `json:"id"` - // @TODO(mng): include variables - Key string `json:"key"` - FeatureEnabled bool `json:"featureEnabled"` + ID string `json:"id"` + Variables []VariationVariable `json:"variables"` + Key string `json:"key"` + FeatureEnabled bool `json:"featureEnabled"` +} + +// VariationVariable represents a Variable object from the Variation +type VariationVariable struct { + ID string `json:"id"` + Value string `json:"value"` } // Event represents an event from the Optimizely datafile diff --git a/optimizely/config/datafileprojectconfig/mappers/experiment.go b/optimizely/config/datafileprojectconfig/mappers/experiment.go index 5dd553a1e..662802912 100644 --- a/optimizely/config/datafileprojectconfig/mappers/experiment.go +++ b/optimizely/config/datafileprojectconfig/mappers/experiment.go @@ -43,6 +43,7 @@ func mapVariation(rawVariation datafileEntities.Variation) entities.Variation { ID: rawVariation.ID, Key: rawVariation.Key, FeatureEnabled: rawVariation.FeatureEnabled, + Variables: rawVariation.Variables, } return variation } diff --git a/optimizely/config/datafileprojectconfig/mappers/feature.go b/optimizely/config/datafileprojectconfig/mappers/feature.go index 978b1c71c..84c02eff9 100644 --- a/optimizely/config/datafileprojectconfig/mappers/feature.go +++ b/optimizely/config/datafileprojectconfig/mappers/feature.go @@ -21,11 +21,8 @@ import ( "github.com/optimizely/go-sdk/optimizely/entities" ) -// MapFeatureFlags maps the raw datafile feature flag entities to SDK Feature entities -func MapFeatureFlags( - featureFlags []datafileEntities.FeatureFlag, - rolloutMap map[string]entities.Rollout, - experimentMap map[string]entities.Experiment, +// MapFeatures maps the raw datafile feature flag entities to SDK Feature entities +func MapFeatures(featureFlags []datafileEntities.FeatureFlag, rolloutMap map[string]entities.Rollout, experimentMap map[string]entities.Experiment, ) map[string]entities.Feature { featureMap := make(map[string]entities.Feature) @@ -49,3 +46,13 @@ func MapFeatureFlags( } return featureMap } + +// MapFeatureFlags maps the raw datafile feature flag entities to their keys +func MapFeatureFlags(featureFlags []datafileEntities.FeatureFlag) map[string]datafileEntities.FeatureFlag { + + featureFlagsMap := make(map[string]datafileEntities.FeatureFlag) + for _, featureFlag := range featureFlags { + featureFlagsMap[featureFlag.Key] = featureFlag + } + return featureFlagsMap +} diff --git a/optimizely/config/datafileprojectconfig/mappers/feature_test.go b/optimizely/config/datafileprojectconfig/mappers/feature_test.go index 831eac1e9..cd43509d5 100644 --- a/optimizely/config/datafileprojectconfig/mappers/feature_test.go +++ b/optimizely/config/datafileprojectconfig/mappers/feature_test.go @@ -25,6 +25,33 @@ import ( "github.com/stretchr/testify/assert" ) +func TestMapFeatureFlags(t *testing.T) { + const testFeatureFlagString = `{ + "id": "21111", + "key": "test_feature_21111", + "rolloutId": "41111", + "experimentIds": ["31111", "31112"] + }` + + var rawFeatureFlag datafileEntities.FeatureFlag + json.Unmarshal([]byte(testFeatureFlagString), &rawFeatureFlag) + + rawFeatureFlags := []datafileEntities.FeatureFlag{rawFeatureFlag} + featureFlagsMap := MapFeatureFlags(rawFeatureFlags) + assert.Equal(t, len(featureFlagsMap), 1) + + expectedFeatureFlagsMap := map[string]datafileEntities.FeatureFlag{ + "test_feature_21111": datafileEntities.FeatureFlag{ + ID: "21111", + Key: "test_feature_21111", + RolloutID: "41111", + ExperimentIDs: []string{"31111", "31112"}, + }, + } + + assert.Equal(t, expectedFeatureFlagsMap, featureFlagsMap) +} + func TestMapFeatures(t *testing.T) { const testFeatureFlagString = `{ "id": "21111", @@ -47,7 +74,7 @@ func TestMapFeatures(t *testing.T) { "31111": experiment31111, "31112": experiment31112, } - featureMap := MapFeatureFlags(rawFeatureFlags, rolloutMap, experimentMap) + featureMap := MapFeatures(rawFeatureFlags, rolloutMap, experimentMap) expectedFeatureMap := map[string]entities.Feature{ "test_feature_21111": entities.Feature{ ID: "21111", diff --git a/optimizely/entities/experiment.go b/optimizely/entities/experiment.go index c402fb62b..48db7b82b 100644 --- a/optimizely/entities/experiment.go +++ b/optimizely/entities/experiment.go @@ -16,9 +16,14 @@ package entities +import ( + datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" +) + // Variation represents a variation in the experiment type Variation struct { ID string + Variables []datafileEntities.VariationVariable Key string FeatureEnabled bool } diff --git a/optimizely/event/processor_test.go b/optimizely/event/processor_test.go index 2a2ea2e28..5c53a08a9 100644 --- a/optimizely/event/processor_test.go +++ b/optimizely/event/processor_test.go @@ -16,7 +16,7 @@ func TestDefaultEventProcessor_ProcessImpression(t *testing.T) { assert.Equal(t, 1, processor.EventsCount()) - time.Sleep(200 * time.Millisecond) + time.Sleep(2000 * time.Millisecond) assert.NotNil(t, processor.Ticker) diff --git a/optimizely/interface.go b/optimizely/interface.go index 9dc985e9a..734a5bb56 100644 --- a/optimizely/interface.go +++ b/optimizely/interface.go @@ -17,6 +17,7 @@ package optimizely import ( + datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/entities" ) @@ -32,6 +33,7 @@ type ProjectConfig interface { GetEventByKey(string) (entities.Event, error) GetExperimentByKey(string) (entities.Experiment, error) GetFeatureByKey(string) (entities.Feature, error) + GetFeatureFlagByKey(string) (datafileEntities.FeatureFlag, error) GetFeatureList() []entities.Feature GetGroupByID(string) (entities.Group, error) GetProjectID() string From 1acdb79f460b7aaf989a86e756e868b4e62b8dff Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 23 Aug 2019 00:16:03 +0500 Subject: [PATCH 2/6] Addressed Recommended changes. --- optimizely/client/client.go | 99 ++++++++----------- optimizely/client/client_test.go | 60 +++++------ .../config/datafileprojectconfig/config.go | 24 +++-- .../entities/entities.go | 18 ---- .../mappers/experiment.go | 6 +- .../datafileprojectconfig/mappers/feature.go | 20 ++-- .../mappers/feature_test.go | 37 ++----- optimizely/entities/experiment.go | 12 ++- optimizely/entities/feature.go | 9 ++ optimizely/interface.go | 3 +- 10 files changed, 118 insertions(+), 170 deletions(-) diff --git a/optimizely/client/client.go b/optimizely/client/client.go index e271d3ee4..e5e076f60 100644 --- a/optimizely/client/client.go +++ b/optimizely/client/client.go @@ -46,12 +46,6 @@ type OptimizelyClient struct { // IsFeatureEnabled returns true if the feature is enabled for the given user func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entities.UserContext) (result bool, err error) { - if !o.isValid { - errorMessage := "Optimizely instance is not valid. Failing IsFeatureEnabled." - err := errors.New(errorMessage) - logger.Error(errorMessage, nil) - return false, err - } defer func() { if r := recover(); r != nil { @@ -61,11 +55,11 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit } }() - projectConfig := o.configManager.GetConfig() - - if reflect.ValueOf(projectConfig).IsNil() { - return false, fmt.Errorf("project config is null") + if isValid, err := o.isValidClient("IsFeatureEnabled"); !isValid { + return false, err } + + projectConfig := o.configManager.GetConfig() feature, err := projectConfig.GetFeatureByKey(featureKey) if err != nil { logger.Error("Error retrieving feature", err) @@ -102,12 +96,6 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit // GetEnabledFeatures returns an array containing the keys of all features in the project that are enabled for the given user. func (o *OptimizelyClient) GetEnabledFeatures(userContext entities.UserContext) (enabledFeatures []string, err error) { - if !o.isValid { - errorMessage := "Optimizely instance is not valid. Failing GetEnabledFeatures." - err := errors.New(errorMessage) - logger.Error(errorMessage, nil) - return enabledFeatures, err - } defer func() { if r := recover(); r != nil { @@ -117,12 +105,11 @@ func (o *OptimizelyClient) GetEnabledFeatures(userContext entities.UserContext) } }() - projectConfig := o.configManager.GetConfig() - - if reflect.ValueOf(projectConfig).IsNil() { - return enabledFeatures, fmt.Errorf("project config is null") + if isValid, err := o.isValidClient("GetEnabledFeatures"); !isValid { + return enabledFeatures, err } + projectConfig := o.configManager.GetConfig() featureList := projectConfig.GetFeatureList() for _, feature := range featureList { isEnabled, _ := o.IsFeatureEnabled(feature.Key, userContext) @@ -137,25 +124,14 @@ func (o *OptimizelyClient) GetEnabledFeatures(userContext entities.UserContext) // GetFeatureVariableString returns string feature variable value func (o *OptimizelyClient) GetFeatureVariableString(featureKey string, variableKey string, userContext entities.UserContext) (value string, err error) { - val, err := o.getFeatureVariable("", featureKey, variableKey, userContext) - if err == nil { - switch val.(type) { - case string: - return val.(string), err - default: - break - } + val, err := o.getFeatureVariable("string", featureKey, variableKey, userContext) + if returnValue, ok := val.(string); ok { + value = returnValue } - return "", err + return value, err } -func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) { - if !o.isValid { - errorMessage := "Optimizely instance is not valid. Failing getFeatureVariable." - err := errors.New(errorMessage) - logger.Error(errorMessage, nil) - return nil, err - } +func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) { defer func() { if r := recover(); r != nil { @@ -165,18 +141,19 @@ func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey } }() - projectConfig := o.configManager.GetConfig() - - if reflect.ValueOf(projectConfig).IsNil() { - return nil, fmt.Errorf("project config is null") + if isValid, err := o.isValidClient("getFeatureVariable"); !isValid { + return nil, err } - featureFlag, err := projectConfig.GetFeatureFlagByKey(featureKey) + projectConfig := o.configManager.GetConfig() + + feature, err := projectConfig.GetFeatureByKey(featureKey) if err != nil { - logger.Error("Error retrieving feature flag", err) + logger.Error("Error retrieving feature", err) return nil, err } - variable, err := featureFlag.GetVariable(variableKey) + + variable, err := projectConfig.GetVariableByKey(featureKey, variableKey) if err != nil { logger.Error("Error retrieving variable", err) return nil, err @@ -184,11 +161,6 @@ func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey var featureValue = variable.DefaultValue - feature, err := projectConfig.GetFeatureByKey(featureKey) - if err != nil { - logger.Error("Error retrieving feature", err) - return nil, err - } featureDecisionContext := decision.FeatureDecisionContext{ Feature: &feature, ProjectConfig: projectConfig, @@ -204,29 +176,24 @@ func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey } } - var typeName = "" var valueParsed interface{} - switch valueType.(type) { - case string: - typeName = "string" + switch valueType { + case "string": valueParsed = featureValue break - case int: - typeName = "integer" + case "integer": convertedValue, err := strconv.Atoi(featureValue) if err == nil { valueParsed = convertedValue } break - case float64: - typeName = "double" + case "double": convertedValue, err := strconv.ParseFloat(featureValue, 64) if err == nil { valueParsed = convertedValue } break - case bool: - typeName = "boolean" + case "boolean": convertedValue, err := strconv.ParseBool(featureValue) if err == nil { valueParsed = convertedValue @@ -236,7 +203,7 @@ func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey break } - if valueParsed == nil || variable.Type != typeName { + if valueParsed == nil || variable.Type != valueType { return nil, fmt.Errorf("Variable value for key %s is invalid or wrong type", variableKey) } @@ -244,6 +211,20 @@ func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey return valueParsed, nil } +func (o *OptimizelyClient) isValidClient(methodName string) (result bool, err error) { + if !o.isValid { + errorMessage := fmt.Sprintf("Optimizely instance is not valid. Failing %s.", methodName) + err := errors.New(errorMessage) + logger.Error(errorMessage, nil) + return false, err + } + + if reflect.ValueOf(o.configManager.GetConfig()).IsNil() { + return false, fmt.Errorf("project config is null") + } + return true, nil +} + // Close closes the Optimizely instance and stops any ongoing tasks from its children components func (o *OptimizelyClient) Close() { o.cancelFunc() diff --git a/optimizely/client/client_test.go b/optimizely/client/client_test.go index f05b2e865..5d9fd98b2 100644 --- a/optimizely/client/client_test.go +++ b/optimizely/client/client_test.go @@ -23,7 +23,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/optimizely/go-sdk/optimizely" - datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/decision" "github.com/optimizely/go-sdk/optimizely/entities" "github.com/stretchr/testify/mock" @@ -44,9 +43,9 @@ func (c *MockProjectConfig) GetFeatureList() []entities.Feature { return args.Get(0).([]entities.Feature) } -func (c *MockProjectConfig) GetFeatureFlagByKey(featureKey string) (datafileEntities.FeatureFlag, error) { - args := c.Called(featureKey) - return args.Get(0).(datafileEntities.FeatureFlag), args.Error(1) +func (c *MockProjectConfig) GetVariableByKey(featureKey string, variableKey string) (entities.Variable, error) { + args := c.Called(featureKey, variableKey) + return args.Get(0).(entities.Variable), args.Error(1) } type MockProjectConfigManager struct { @@ -306,14 +305,14 @@ func TestGetEnabledFeaturesPanic(t *testing.T) { func TestGetFeatureVariableStringWithValidValue(t *testing.T) { testFeatureKey := "test_feature_key" - testFeatureFlagKey := "test_feature_flag_key" + testVariableKey := "test_feature_flag_key" testVariableValue := "teststring" testUserContext := entities.UserContext{ID: "test_user_1"} - testVariationVariable := datafileEntities.VariationVariable{ + testVariationVariable := entities.VariationVariable{ ID: "1", Value: testVariableValue, } - testVariable := datafileEntities.Variable{ + testVariable := entities.Variable{ DefaultValue: "default", ID: "1", Key: "test_feature_flag_key", @@ -324,8 +323,7 @@ func TestGetFeatureVariableStringWithValidValue(t *testing.T) { Variations: map[string]entities.Variation{"22222": testVariation}, } testFeature := getTestFeature(testFeatureKey, testExperiment) - testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) - mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfig := getMockConfig(testFeatureKey, testVariableKey, testFeature, testVariable) mockConfigManager := new(MockProjectConfigManager) mockConfigManager.On("GetConfig").Return(mockConfig) @@ -343,7 +341,7 @@ func TestGetFeatureVariableStringWithValidValue(t *testing.T) { decisionService: mockDecisionService, isValid: true, } - result, _ := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + result, _ := client.GetFeatureVariableString(testFeatureKey, testVariableKey, testUserContext) assert.Equal(t, testVariableValue, result) mockConfig.AssertExpectations(t) mockConfigManager.AssertExpectations(t) @@ -352,14 +350,14 @@ func TestGetFeatureVariableStringWithValidValue(t *testing.T) { func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { testFeatureKey := "test_feature_key" - testFeatureFlagKey := "test_feature_flag_key" + testVariableKey := "test_feature_flag_key" testVariableValue := "true" testUserContext := entities.UserContext{ID: "test_user_1"} - testVariationVariable := datafileEntities.VariationVariable{ + testVariationVariable := entities.VariationVariable{ ID: "1", Value: testVariableValue, } - testVariable := datafileEntities.Variable{ + testVariable := entities.Variable{ DefaultValue: "default", ID: "1", Key: "test_feature_flag_key", @@ -370,8 +368,7 @@ func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { Variations: map[string]entities.Variation{"22222": testVariation}, } testFeature := getTestFeature(testFeatureKey, testExperiment) - testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) - mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfig := getMockConfig(testFeatureKey, testVariableKey, testFeature, testVariable) mockConfigManager := new(MockProjectConfigManager) mockConfigManager.On("GetConfig").Return(mockConfig) @@ -389,7 +386,7 @@ func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { decisionService: mockDecisionService, isValid: true, } - result, err := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + result, err := client.GetFeatureVariableString(testFeatureKey, testVariableKey, testUserContext) assert.Equal(t, "", result) assert.NotNil(t, err) mockConfig.AssertExpectations(t) @@ -399,14 +396,14 @@ func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { func TestGetFeatureVariableStringReturnsDefaultValueIfFeatureNotEnabled(t *testing.T) { testFeatureKey := "test_feature_key" - testFeatureFlagKey := "test_feature_flag_key" + testVariableKey := "test_feature_flag_key" testVariableValue := "teststring" testUserContext := entities.UserContext{ID: "test_user_1"} - testVariationVariable := datafileEntities.VariationVariable{ + testVariationVariable := entities.VariationVariable{ ID: "1", Value: testVariableValue, } - testVariable := datafileEntities.Variable{ + testVariable := entities.Variable{ DefaultValue: "defaultString", ID: "1", Key: "test_feature_flag_key", @@ -417,8 +414,7 @@ func TestGetFeatureVariableStringReturnsDefaultValueIfFeatureNotEnabled(t *testi Variations: map[string]entities.Variation{"22222": testVariation}, } testFeature := getTestFeature(testFeatureKey, testExperiment) - testFeatureFlag := getTestFeatureFlag(testFeatureFlagKey, testVariable) - mockConfig := getMockConfig(testFeatureKey, testFeature, testFeatureFlag) + mockConfig := getMockConfig(testFeatureKey, testVariableKey, testFeature, testVariable) mockConfigManager := new(MockProjectConfigManager) mockConfigManager.On("GetConfig").Return(mockConfig) @@ -436,7 +432,7 @@ func TestGetFeatureVariableStringReturnsDefaultValueIfFeatureNotEnabled(t *testi decisionService: mockDecisionService, isValid: true, } - result, err := client.GetFeatureVariableString(testFeatureKey, testFeatureFlagKey, testUserContext) + result, err := client.GetFeatureVariableString(testFeatureKey, testVariableKey, testUserContext) assert.Equal(t, "defaultString", result) assert.Nil(t, err) mockConfig.AssertExpectations(t) @@ -457,8 +453,8 @@ func TestGetFeatureVariableErrorCases(t *testing.T) { } _, err1 := client.GetFeatureVariableString("test_feature_key", "test_variable_key", testUserContext) assert.Error(t, err1) - mockConfigManager.AssertNotCalled(t, "GetFeatureFlagByKey") mockConfigManager.AssertNotCalled(t, "GetFeatureByKey") + mockConfigManager.AssertNotCalled(t, "GetVariableByKey") mockDecisionService.AssertNotCalled(t, "GetFeatureDecision") } @@ -496,29 +492,19 @@ func getTestFeatureDecision(experiment entities.Experiment, variation entities.V } } -func getTestVariationWithFeatureVariable(featureEnabled bool, featureVariable datafileEntities.VariationVariable) entities.Variation { +func getTestVariationWithFeatureVariable(featureEnabled bool, variable entities.VariationVariable) entities.Variation { return entities.Variation{ ID: "22222", Key: "22222", FeatureEnabled: featureEnabled, - Variables: []datafileEntities.VariationVariable{featureVariable}, - } -} - -func getTestFeatureFlag(featureFlagKey string, variable datafileEntities.Variable) datafileEntities.FeatureFlag { - return datafileEntities.FeatureFlag{ - ID: "21111", - Key: featureFlagKey, - RolloutID: "41111", - ExperimentIDs: []string{"31111", "31112"}, - Variables: []datafileEntities.Variable{variable}, + Variables: []entities.VariationVariable{variable}, } } -func getMockConfig(featureKey string, feature entities.Feature, featureFlag datafileEntities.FeatureFlag) *MockProjectConfig { +func getMockConfig(featureKey string, variableKey string, feature entities.Feature, variable entities.Variable) *MockProjectConfig { mockConfig := new(MockProjectConfig) mockConfig.On("GetFeatureByKey", featureKey).Return(feature, nil) - mockConfig.On("GetFeatureFlagByKey", featureKey).Return(featureFlag, nil) + mockConfig.On("GetVariableByKey", featureKey, variableKey).Return(variable, nil) return mockConfig } diff --git a/optimizely/config/datafileprojectconfig/config.go b/optimizely/config/datafileprojectconfig/config.go index 35447a3f6..af60a2935 100644 --- a/optimizely/config/datafileprojectconfig/config.go +++ b/optimizely/config/datafileprojectconfig/config.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" - datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/mappers" "github.com/optimizely/go-sdk/optimizely/entities" "github.com/optimizely/go-sdk/optimizely/logging" @@ -40,7 +39,6 @@ type DatafileProjectConfig struct { experimentKeyToIDMap map[string]string experimentMap map[string]entities.Experiment featureMap map[string]entities.Feature - featureFlagMap map[string]datafileEntities.FeatureFlag groupMap map[string]entities.Group projectID string revision string @@ -97,7 +95,6 @@ func NewDatafileProjectConfig(jsonDatafile []byte) (*DatafileProjectConfig, erro experimentKeyToIDMap: experimentKeyMap, rolloutMap: rolloutMap, featureMap: mappers.MapFeatures(datafile.FeatureFlags, rolloutMap, experimentMap), - featureFlagMap: mappers.MapFeatureFlags(datafile.FeatureFlags), } logger.Info("Datafile is valid.") @@ -124,14 +121,21 @@ func (c DatafileProjectConfig) GetFeatureByKey(featureKey string) (entities.Feat return entities.Feature{}, errors.New(errMessage) } -// GetFeatureFlagByKey returns the featureflag with the given key -func (c DatafileProjectConfig) GetFeatureFlagByKey(featureKey string) (datafileEntities.FeatureFlag, error) { - if feature, ok := c.featureFlagMap[featureKey]; ok { - return feature, nil - } +// GetVariableByKey returns the featureVariable with the given key +func (c DatafileProjectConfig) GetVariableByKey(featureKey string, variableKey string) (entities.Variable, error) { - errMessage := fmt.Sprintf("FeatureFlag with key %s not found", featureKey) - return datafileEntities.FeatureFlag{}, errors.New(errMessage) + var variable entities.Variable + var err = fmt.Errorf("Variable with key %s not found", featureKey) + if feature, ok := c.featureMap[featureKey]; ok { + for _, v := range feature.Variables { + if v.Key == variableKey { + variable = v + err = nil + break + } + } + } + return variable, err } // GetAttributeByKey returns the attribute with the given key diff --git a/optimizely/config/datafileprojectconfig/entities/entities.go b/optimizely/config/datafileprojectconfig/entities/entities.go index 6eb7d286f..304d6a2ee 100644 --- a/optimizely/config/datafileprojectconfig/entities/entities.go +++ b/optimizely/config/datafileprojectconfig/entities/entities.go @@ -16,10 +16,6 @@ package entities -import ( - "fmt" -) - // Audience represents an Audience object from the Optimizely datafile type Audience struct { ID string `json:"id"` @@ -55,20 +51,6 @@ type FeatureFlag struct { Variables []Variable `json:"variables"` } -// GetVariable returns variable for key from feature flag -func (f *FeatureFlag) GetVariable(key string) (Variable, error) { - var variable Variable - var err = fmt.Errorf("Variable with key %s not found", key) - for _, v := range f.Variables { - if v.Key == key { - variable = v - err = nil - break - } - } - return variable, err -} - // Variable represents a Variable object from the Optimizely datafile type Variable struct { DefaultValue string `json:"defaultValue"` diff --git a/optimizely/config/datafileprojectconfig/mappers/experiment.go b/optimizely/config/datafileprojectconfig/mappers/experiment.go index 662802912..243abbe68 100644 --- a/optimizely/config/datafileprojectconfig/mappers/experiment.go +++ b/optimizely/config/datafileprojectconfig/mappers/experiment.go @@ -43,8 +43,12 @@ func mapVariation(rawVariation datafileEntities.Variation) entities.Variation { ID: rawVariation.ID, Key: rawVariation.Key, FeatureEnabled: rawVariation.FeatureEnabled, - Variables: rawVariation.Variables, } + + for _, variable := range rawVariation.Variables { + variation.Variables = append(variation.Variables, entities.VariationVariable{ID: variable.ID, Value: variable.Value}) + } + return variation } diff --git a/optimizely/config/datafileprojectconfig/mappers/feature.go b/optimizely/config/datafileprojectconfig/mappers/feature.go index 84c02eff9..1987522f1 100644 --- a/optimizely/config/datafileprojectconfig/mappers/feature.go +++ b/optimizely/config/datafileprojectconfig/mappers/feature.go @@ -41,18 +41,18 @@ func MapFeatures(featureFlags []datafileEntities.FeatureFlag, rolloutMap map[str } } + var variables = []entities.Variable{} + for _, variable := range featureFlag.Variables { + variables = append(variables, entities.Variable{ + DefaultValue: variable.DefaultValue, + ID: variable.ID, + Key: variable.Key, + Type: variable.Type}) + } + feature.FeatureExperiments = featureExperiments + feature.Variables = variables featureMap[featureFlag.Key] = feature } return featureMap } - -// MapFeatureFlags maps the raw datafile feature flag entities to their keys -func MapFeatureFlags(featureFlags []datafileEntities.FeatureFlag) map[string]datafileEntities.FeatureFlag { - - featureFlagsMap := make(map[string]datafileEntities.FeatureFlag) - for _, featureFlag := range featureFlags { - featureFlagsMap[featureFlag.Key] = featureFlag - } - return featureFlagsMap -} diff --git a/optimizely/config/datafileprojectconfig/mappers/feature_test.go b/optimizely/config/datafileprojectconfig/mappers/feature_test.go index cd43509d5..44f8c5b81 100644 --- a/optimizely/config/datafileprojectconfig/mappers/feature_test.go +++ b/optimizely/config/datafileprojectconfig/mappers/feature_test.go @@ -25,39 +25,13 @@ import ( "github.com/stretchr/testify/assert" ) -func TestMapFeatureFlags(t *testing.T) { - const testFeatureFlagString = `{ - "id": "21111", - "key": "test_feature_21111", - "rolloutId": "41111", - "experimentIds": ["31111", "31112"] - }` - - var rawFeatureFlag datafileEntities.FeatureFlag - json.Unmarshal([]byte(testFeatureFlagString), &rawFeatureFlag) - - rawFeatureFlags := []datafileEntities.FeatureFlag{rawFeatureFlag} - featureFlagsMap := MapFeatureFlags(rawFeatureFlags) - assert.Equal(t, len(featureFlagsMap), 1) - - expectedFeatureFlagsMap := map[string]datafileEntities.FeatureFlag{ - "test_feature_21111": datafileEntities.FeatureFlag{ - ID: "21111", - Key: "test_feature_21111", - RolloutID: "41111", - ExperimentIDs: []string{"31111", "31112"}, - }, - } - - assert.Equal(t, expectedFeatureFlagsMap, featureFlagsMap) -} - func TestMapFeatures(t *testing.T) { const testFeatureFlagString = `{ "id": "21111", "key": "test_feature_21111", "rolloutId": "41111", - "experimentIds": ["31111", "31112"] + "experimentIds": ["31111", "31112"], + "variables": [{"defaultValue":"1","id":"1","key":"test","type":"integer"}] }` var rawFeatureFlag datafileEntities.FeatureFlag @@ -75,12 +49,19 @@ func TestMapFeatures(t *testing.T) { "31112": experiment31112, } featureMap := MapFeatures(rawFeatureFlags, rolloutMap, experimentMap) + variable := entities.Variable{ + ID: "1", + DefaultValue: "1", + Key: "test", + Type: "integer", + } expectedFeatureMap := map[string]entities.Feature{ "test_feature_21111": entities.Feature{ ID: "21111", Key: "test_feature_21111", Rollout: rollout, FeatureExperiments: []entities.Experiment{experiment31111, experiment31112}, + Variables: []entities.Variable{variable}, }, } diff --git a/optimizely/entities/experiment.go b/optimizely/entities/experiment.go index 48db7b82b..979119e8e 100644 --- a/optimizely/entities/experiment.go +++ b/optimizely/entities/experiment.go @@ -16,14 +16,10 @@ package entities -import ( - datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" -) - // Variation represents a variation in the experiment type Variation struct { ID string - Variables []datafileEntities.VariationVariable + Variables []VariationVariable Key string FeatureEnabled bool } @@ -45,3 +41,9 @@ type Range struct { EntityID string EndOfRange int } + +// VariationVariable represents a Variable object from the Variation +type VariationVariable struct { + ID string + Value string +} diff --git a/optimizely/entities/feature.go b/optimizely/entities/feature.go index 8e4654956..131d88f16 100644 --- a/optimizely/entities/feature.go +++ b/optimizely/entities/feature.go @@ -22,6 +22,7 @@ type Feature struct { Key string FeatureExperiments []Experiment Rollout Rollout + Variables []Variable } // Rollout represents a feature rollout @@ -29,3 +30,11 @@ type Rollout struct { ID string Experiments []Experiment } + +// Variable represents a feature variable +type Variable struct { + DefaultValue string + ID string + Key string + Type string +} diff --git a/optimizely/interface.go b/optimizely/interface.go index 734a5bb56..7bee0f66a 100644 --- a/optimizely/interface.go +++ b/optimizely/interface.go @@ -17,7 +17,6 @@ package optimizely import ( - datafileEntities "github.com/optimizely/go-sdk/optimizely/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/optimizely/entities" ) @@ -33,7 +32,7 @@ type ProjectConfig interface { GetEventByKey(string) (entities.Event, error) GetExperimentByKey(string) (entities.Experiment, error) GetFeatureByKey(string) (entities.Feature, error) - GetFeatureFlagByKey(string) (datafileEntities.FeatureFlag, error) + GetVariableByKey(featureKey string, variableKey string) (entities.Variable, error) GetFeatureList() []entities.Feature GetGroupByID(string) (entities.Group, error) GetProjectID() string From deff23bac21c5fbf4d494ce4b9c4f72b1ddc6158 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 23 Aug 2019 11:52:37 +0500 Subject: [PATCH 3/6] Recommended changes made. --- optimizely/client/client.go | 7 ++----- optimizely/client/client_test.go | 11 +++++++---- .../datafileprojectconfig/mappers/experiment.go | 2 +- optimizely/entities/experiment.go | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/optimizely/client/client.go b/optimizely/client/client.go index e5e076f60..83a214630 100644 --- a/optimizely/client/client.go +++ b/optimizely/client/client.go @@ -168,11 +168,8 @@ func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey strin featureDecision, err := o.decisionService.GetFeatureDecision(featureDecisionContext, userContext) if err == nil { - for _, v := range featureDecision.Variation.Variables { - if v.ID == variable.ID && featureDecision.Variation.FeatureEnabled { - featureValue = v.Value - break - } + if v, ok := featureDecision.Variation.Variables[variable.ID]; ok && featureDecision.Variation.FeatureEnabled { + featureValue = v.Value } } diff --git a/optimizely/client/client_test.go b/optimizely/client/client_test.go index 5d9fd98b2..d467f1ead 100644 --- a/optimizely/client/client_test.go +++ b/optimizely/client/client_test.go @@ -316,7 +316,8 @@ func TestGetFeatureVariableStringWithValidValue(t *testing.T) { DefaultValue: "default", ID: "1", Key: "test_feature_flag_key", - Type: "string"} + Type: "string", + } testVariation := getTestVariationWithFeatureVariable(true, testVariationVariable) testExperiment := entities.Experiment{ ID: "111111", @@ -361,7 +362,8 @@ func TestGetFeatureVariableStringWithInvalidValueType(t *testing.T) { DefaultValue: "default", ID: "1", Key: "test_feature_flag_key", - Type: "boolean"} + Type: "boolean", + } testVariation := getTestVariationWithFeatureVariable(true, testVariationVariable) testExperiment := entities.Experiment{ ID: "111111", @@ -407,7 +409,8 @@ func TestGetFeatureVariableStringReturnsDefaultValueIfFeatureNotEnabled(t *testi DefaultValue: "defaultString", ID: "1", Key: "test_feature_flag_key", - Type: "string"} + Type: "string", + } testVariation := getTestVariationWithFeatureVariable(false, testVariationVariable) testExperiment := entities.Experiment{ ID: "111111", @@ -497,7 +500,7 @@ func getTestVariationWithFeatureVariable(featureEnabled bool, variable entities. ID: "22222", Key: "22222", FeatureEnabled: featureEnabled, - Variables: []entities.VariationVariable{variable}, + Variables: map[string]entities.VariationVariable{variable.ID: variable}, } } diff --git a/optimizely/config/datafileprojectconfig/mappers/experiment.go b/optimizely/config/datafileprojectconfig/mappers/experiment.go index 243abbe68..54911490a 100644 --- a/optimizely/config/datafileprojectconfig/mappers/experiment.go +++ b/optimizely/config/datafileprojectconfig/mappers/experiment.go @@ -46,7 +46,7 @@ func mapVariation(rawVariation datafileEntities.Variation) entities.Variation { } for _, variable := range rawVariation.Variables { - variation.Variables = append(variation.Variables, entities.VariationVariable{ID: variable.ID, Value: variable.Value}) + variation.Variables[variable.ID] = entities.VariationVariable{ID: variable.ID, Value: variable.Value} } return variation diff --git a/optimizely/entities/experiment.go b/optimizely/entities/experiment.go index 979119e8e..e5a10313a 100644 --- a/optimizely/entities/experiment.go +++ b/optimizely/entities/experiment.go @@ -19,7 +19,7 @@ package entities // Variation represents a variation in the experiment type Variation struct { ID string - Variables []VariationVariable + Variables map[string]VariationVariable Key string FeatureEnabled bool } From 6a60c9aee5eb6fb63d0a0b1d9bc82dbef10ec775 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 23 Aug 2019 12:23:13 +0500 Subject: [PATCH 4/6] bug fixes. --- .../config/datafileprojectconfig/mappers/experiment.go | 1 + .../datafileprojectconfig/mappers/experiment_test.go | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/optimizely/config/datafileprojectconfig/mappers/experiment.go b/optimizely/config/datafileprojectconfig/mappers/experiment.go index 54911490a..fcfb63f55 100644 --- a/optimizely/config/datafileprojectconfig/mappers/experiment.go +++ b/optimizely/config/datafileprojectconfig/mappers/experiment.go @@ -45,6 +45,7 @@ func mapVariation(rawVariation datafileEntities.Variation) entities.Variation { FeatureEnabled: rawVariation.FeatureEnabled, } + variation.Variables = make(map[string]entities.VariationVariable) for _, variable := range rawVariation.Variables { variation.Variables[variable.ID] = entities.VariationVariable{ID: variable.ID, Value: variable.Value} } diff --git a/optimizely/config/datafileprojectconfig/mappers/experiment_test.go b/optimizely/config/datafileprojectconfig/mappers/experiment_test.go index 1927948b8..6ed6ca42a 100644 --- a/optimizely/config/datafileprojectconfig/mappers/experiment_test.go +++ b/optimizely/config/datafileprojectconfig/mappers/experiment_test.go @@ -34,12 +34,14 @@ func TestMapExperiments(t *testing.T) { { "id": "21111", "key": "variation_1", - "featureEnabled": true + "featureEnabled": true, + "variables": [{"id":"1","value":"1"}] }, { "id": "21112", "key": "variation_2", - "featureEnabled": false + "featureEnabled": false, + "variables": [{"id":"2","value":"2"}] } ], "trafficAllocation": [ @@ -71,11 +73,13 @@ func TestMapExperiments(t *testing.T) { Variations: map[string]entities.Variation{ "21111": { ID: "21111", + Variables: map[string]entities.VariationVariable{"1": entities.VariationVariable{ID: "1", Value: "1"}}, Key: "variation_1", FeatureEnabled: true, }, "21112": { ID: "21112", + Variables: map[string]entities.VariationVariable{"2": entities.VariationVariable{ID: "2", Value: "2"}}, Key: "variation_2", FeatureEnabled: false, }, From 75e592f52244de5894b6263303dd15b7d53ce8e7 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Sat, 24 Aug 2019 01:54:13 +0500 Subject: [PATCH 5/6] Addressed review. --- optimizely/client/client.go | 51 ++++++++++--------------------------- 1 file changed, 13 insertions(+), 38 deletions(-) diff --git a/optimizely/client/client.go b/optimizely/client/client.go index 83a214630..539372f4e 100644 --- a/optimizely/client/client.go +++ b/optimizely/client/client.go @@ -22,7 +22,6 @@ import ( "fmt" "reflect" "runtime/debug" - "strconv" "github.com/optimizely/go-sdk/optimizely/event" @@ -124,14 +123,17 @@ func (o *OptimizelyClient) GetEnabledFeatures(userContext entities.UserContext) // GetFeatureVariableString returns string feature variable value func (o *OptimizelyClient) GetFeatureVariableString(featureKey string, variableKey string, userContext entities.UserContext) (value string, err error) { - val, err := o.getFeatureVariable("string", featureKey, variableKey, userContext) - if returnValue, ok := val.(string); ok { - value = returnValue + value, valueType, err := o.getFeatureVariable(featureKey, variableKey, userContext) + if err != nil { + return "", err + } + if valueType != "string" { + return "", fmt.Errorf("Variable value for key %s is wrong type", variableKey) } return value, err } -func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) { +func (o *OptimizelyClient) getFeatureVariable(featureKey string, variableKey string, userContext entities.UserContext) (value string, valueType string, err error) { defer func() { if r := recover(); r != nil { @@ -142,7 +144,7 @@ func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey strin }() if isValid, err := o.isValidClient("getFeatureVariable"); !isValid { - return nil, err + return "", "", err } projectConfig := o.configManager.GetConfig() @@ -150,13 +152,13 @@ func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey strin feature, err := projectConfig.GetFeatureByKey(featureKey) if err != nil { logger.Error("Error retrieving feature", err) - return nil, err + return "", "", err } variable, err := projectConfig.GetVariableByKey(featureKey, variableKey) if err != nil { logger.Error("Error retrieving variable", err) - return nil, err + return "", "", err } var featureValue = variable.DefaultValue @@ -173,39 +175,12 @@ func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey strin } } - var valueParsed interface{} - switch valueType { - case "string": - valueParsed = featureValue - break - case "integer": - convertedValue, err := strconv.Atoi(featureValue) - if err == nil { - valueParsed = convertedValue - } - break - case "double": - convertedValue, err := strconv.ParseFloat(featureValue, 64) - if err == nil { - valueParsed = convertedValue - } - break - case "boolean": - convertedValue, err := strconv.ParseBool(featureValue) - if err == nil { - valueParsed = convertedValue - } - break - default: - break - } - - if valueParsed == nil || variable.Type != valueType { - return nil, fmt.Errorf("Variable value for key %s is invalid or wrong type", variableKey) + if featureValue == "" { + return "", "", fmt.Errorf("Variable value for key %s is invalid", variableKey) } // @TODO(yasir): send decision notification - return valueParsed, nil + return featureValue, variable.Type, nil } func (o *OptimizelyClient) isValidClient(methodName string) (result bool, err error) { From 8ff52ba4a11f5ef9f04b8f71b97c1ab00f02e664 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Sat, 24 Aug 2019 02:46:40 +0500 Subject: [PATCH 6/6] Addressed review. --- optimizely/client/client.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/optimizely/client/client.go b/optimizely/client/client.go index 539372f4e..770b436a6 100644 --- a/optimizely/client/client.go +++ b/optimizely/client/client.go @@ -175,10 +175,6 @@ func (o *OptimizelyClient) getFeatureVariable(featureKey string, variableKey str } } - if featureValue == "" { - return "", "", fmt.Errorf("Variable value for key %s is invalid", variableKey) - } - // @TODO(yasir): send decision notification return featureValue, variable.Type, nil }