From 9fcbbd6f900f743bb135c66c5331765727bd3857 Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Wed, 30 Oct 2019 18:14:44 -0700 Subject: [PATCH 1/3] fix(decision): Make sure that we support audienceIds in the absence of audienceConditions. --- .../mappers/condition_trees.go | 4 +++- .../mappers/condition_trees_test.go | 16 ++++++++++++++++ .../datafileprojectconfig/mappers/experiment.go | 8 +++++++- pkg/decision/evaluator/matchers/exact.go | 6 +++--- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/pkg/config/datafileprojectconfig/mappers/condition_trees.go b/pkg/config/datafileprojectconfig/mappers/condition_trees.go index e7f24176c..2668a6361 100644 --- a/pkg/config/datafileprojectconfig/mappers/condition_trees.go +++ b/pkg/config/datafileprojectconfig/mappers/condition_trees.go @@ -108,7 +108,9 @@ func buildAudienceConditionTree(conditions interface{}) (conditionTree *entities value := reflect.ValueOf(conditions) visited := make(map[interface{}]bool) - conditionTree = &entities.TreeNode{} + conditionTree = &entities.TreeNode{ + Operator: "or", + } var populateConditions func(v reflect.Value, root *entities.TreeNode) populateConditions = func(v reflect.Value, root *entities.TreeNode) { diff --git a/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go b/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go index 7493a23fb..e2c63f601 100644 --- a/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go +++ b/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go @@ -70,6 +70,22 @@ func TestBuildAudienceConditionTreeSimpleAudienceCondition(t *testing.T) { assert.Equal(t, expectedConditionTree, conditionTree) } +func TestBuildAudienceConditionTreeNoOperators(t *testing.T) { + conditions := []string{"123"} + expectedConditionTree := &entities.TreeNode{ + Operator: "or", + Nodes: []*entities.TreeNode{ + { + Item: "123", + }, + }, + } + + conditionTree, err := buildAudienceConditionTree(conditions) + assert.Equal(t, expectedConditionTree, conditionTree) + assert.NoError(t, err) +} + func TestBuildConditionTreeUsingDatafileAudienceConditions(t *testing.T) { audience := datafileConfig.Audience{ diff --git a/pkg/config/datafileprojectconfig/mappers/experiment.go b/pkg/config/datafileprojectconfig/mappers/experiment.go index 8649b958d..520a675e7 100644 --- a/pkg/config/datafileprojectconfig/mappers/experiment.go +++ b/pkg/config/datafileprojectconfig/mappers/experiment.go @@ -56,7 +56,13 @@ func mapVariation(rawVariation datafileEntities.Variation) entities.Variation { // Maps the raw experiment entity from the datafile into an SDK Experiment entity func mapExperiment(rawExperiment datafileEntities.Experiment) entities.Experiment { - audienceConditionTree, err := buildAudienceConditionTree(rawExperiment.AudienceConditions) + var audienceConditionTree *entities.TreeNode + var err error + if rawExperiment.AudienceConditions == nil && len(rawExperiment.AudienceIds) > 0 { + audienceConditionTree, err = buildAudienceConditionTree(rawExperiment.AudienceIds) + } else { + audienceConditionTree, err = buildAudienceConditionTree(rawExperiment.AudienceConditions) + } if err != nil { // @TODO: handle error func() {}() // cheat the linters diff --git a/pkg/decision/evaluator/matchers/exact.go b/pkg/decision/evaluator/matchers/exact.go index bf307c08c..097bc8c73 100644 --- a/pkg/decision/evaluator/matchers/exact.go +++ b/pkg/decision/evaluator/matchers/exact.go @@ -34,7 +34,7 @@ func (m ExactMatcher) Match(user entities.UserContext) (bool, error) { if stringValue, ok := m.Condition.Value.(string); ok { attributeValue, err := user.GetStringAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return stringValue == attributeValue, nil } @@ -42,7 +42,7 @@ func (m ExactMatcher) Match(user entities.UserContext) (bool, error) { if boolValue, ok := m.Condition.Value.(bool); ok { attributeValue, err := user.GetBoolAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return boolValue == attributeValue, nil } @@ -50,7 +50,7 @@ func (m ExactMatcher) Match(user entities.UserContext) (bool, error) { if floatValue, ok := utils.ToFloat(m.Condition.Value); ok { attributeValue, err := user.GetFloatAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return floatValue == attributeValue, nil } From 09d02ed91f483259167ad9a0a4a2d8a3c74a95fa Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Wed, 30 Oct 2019 21:27:21 -0700 Subject: [PATCH 2/3] fix: more bugs and unit tests. --- .../datafileprojectconfig/entities/entities.go | 2 +- .../mappers/condition_trees_test.go | 5 +++-- .../datafileprojectconfig/mappers/experiment.go | 2 +- pkg/decision/evaluator/matchers/exact_test.go | 16 ++++++++-------- pkg/decision/evaluator/matchers/gt.go | 2 +- pkg/decision/evaluator/matchers/gt_test.go | 8 ++++---- pkg/decision/evaluator/matchers/lt.go | 2 +- pkg/decision/evaluator/matchers/lt_test.go | 8 ++++---- pkg/decision/evaluator/matchers/substring.go | 2 +- .../evaluator/matchers/substring_test.go | 4 ++-- 10 files changed, 26 insertions(+), 25 deletions(-) diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 5790bc8b8..ff5ba5caf 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -42,7 +42,7 @@ type Experiment struct { TrafficAllocation []trafficAllocation `json:"trafficAllocation"` AudienceIds []string `json:"audienceIds"` ForcedVariations map[string]string `json:"forcedVariations"` - AudienceConditions interface{} `json:"audienceConditions"` + AudienceConditions []interface{} `json:"audienceConditions"` } // FeatureFlag represents a FeatureFlag object from the Optimizely datafile diff --git a/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go b/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go index e2c63f601..45740372d 100644 --- a/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go +++ b/pkg/config/datafileprojectconfig/mappers/condition_trees_test.go @@ -30,8 +30,9 @@ func TestBuildAudienceConditionTreeEmpty(t *testing.T) { json.Unmarshal([]byte(conditionString), &conditions) conditionTree, err := buildAudienceConditionTree(conditions) - assert.NotNil(t, err) - assert.Equal(t, (*entities.TreeNode)(nil), conditionTree) + expectedTree := &entities.TreeNode{Operator: "or"} + assert.NoError(t, err) + assert.Equal(t, expectedTree, conditionTree) } func TestBuildAudienceConditionTreeSimpleAudienceCondition(t *testing.T) { diff --git a/pkg/config/datafileprojectconfig/mappers/experiment.go b/pkg/config/datafileprojectconfig/mappers/experiment.go index 520a675e7..d586d0205 100644 --- a/pkg/config/datafileprojectconfig/mappers/experiment.go +++ b/pkg/config/datafileprojectconfig/mappers/experiment.go @@ -60,7 +60,7 @@ func mapExperiment(rawExperiment datafileEntities.Experiment) entities.Experimen var err error if rawExperiment.AudienceConditions == nil && len(rawExperiment.AudienceIds) > 0 { audienceConditionTree, err = buildAudienceConditionTree(rawExperiment.AudienceIds) - } else { + } else if len(rawExperiment.AudienceConditions) > 0 { audienceConditionTree, err = buildAudienceConditionTree(rawExperiment.AudienceConditions) } if err != nil { diff --git a/pkg/decision/evaluator/matchers/exact_test.go b/pkg/decision/evaluator/matchers/exact_test.go index c5b5c9779..c1622ecb7 100644 --- a/pkg/decision/evaluator/matchers/exact_test.go +++ b/pkg/decision/evaluator/matchers/exact_test.go @@ -54,7 +54,7 @@ func TestExactMatcherString(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "string_not_foo": "foo", @@ -62,7 +62,7 @@ func TestExactMatcherString(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } func TestExactMatcherBool(t *testing.T) { @@ -95,7 +95,7 @@ func TestExactMatcherBool(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "not_bool_true": true, @@ -103,7 +103,7 @@ func TestExactMatcherBool(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } func TestExactMatcherInt(t *testing.T) { @@ -147,7 +147,7 @@ func TestExactMatcherInt(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "int_43": 42, @@ -155,7 +155,7 @@ func TestExactMatcherInt(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } func TestExactMatcherFloat(t *testing.T) { @@ -188,7 +188,7 @@ func TestExactMatcherFloat(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "float_4_3": 4.2, @@ -196,5 +196,5 @@ func TestExactMatcherFloat(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } diff --git a/pkg/decision/evaluator/matchers/gt.go b/pkg/decision/evaluator/matchers/gt.go index 04832d44b..0332ef9d5 100644 --- a/pkg/decision/evaluator/matchers/gt.go +++ b/pkg/decision/evaluator/matchers/gt.go @@ -35,7 +35,7 @@ func (m GtMatcher) Match(user entities.UserContext) (bool, error) { if floatValue, ok := utils.ToFloat(m.Condition.Value); ok { attributeValue, err := user.GetFloatAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return floatValue < attributeValue, nil } diff --git a/pkg/decision/evaluator/matchers/gt_test.go b/pkg/decision/evaluator/matchers/gt_test.go index 5b38ed21f..6674e005b 100644 --- a/pkg/decision/evaluator/matchers/gt_test.go +++ b/pkg/decision/evaluator/matchers/gt_test.go @@ -76,7 +76,7 @@ func TestGtMatcherInt(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "int_43": 42, @@ -84,7 +84,7 @@ func TestGtMatcherInt(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } func TestGtMatcherFloat(t *testing.T) { @@ -127,7 +127,7 @@ func TestGtMatcherFloat(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "float_4_3": 4.2, @@ -135,5 +135,5 @@ func TestGtMatcherFloat(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } diff --git a/pkg/decision/evaluator/matchers/lt.go b/pkg/decision/evaluator/matchers/lt.go index ad47eddaf..a0e27d8f1 100644 --- a/pkg/decision/evaluator/matchers/lt.go +++ b/pkg/decision/evaluator/matchers/lt.go @@ -35,7 +35,7 @@ func (m LtMatcher) Match(user entities.UserContext) (bool, error) { if floatValue, ok := utils.ToFloat(m.Condition.Value); ok { attributeValue, err := user.GetFloatAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return floatValue > attributeValue, nil } diff --git a/pkg/decision/evaluator/matchers/lt_test.go b/pkg/decision/evaluator/matchers/lt_test.go index 4ed042979..d4fa56593 100644 --- a/pkg/decision/evaluator/matchers/lt_test.go +++ b/pkg/decision/evaluator/matchers/lt_test.go @@ -76,7 +76,7 @@ func TestLtMatcherInt(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "int_43": 42, @@ -84,7 +84,7 @@ func TestLtMatcherInt(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } func TestLtMatcherFloat(t *testing.T) { @@ -127,7 +127,7 @@ func TestLtMatcherFloat(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "float_4_3": 4.2, @@ -135,5 +135,5 @@ func TestLtMatcherFloat(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } diff --git a/pkg/decision/evaluator/matchers/substring.go b/pkg/decision/evaluator/matchers/substring.go index f25241f02..e8051700f 100644 --- a/pkg/decision/evaluator/matchers/substring.go +++ b/pkg/decision/evaluator/matchers/substring.go @@ -35,7 +35,7 @@ func (m SubstringMatcher) Match(user entities.UserContext) (bool, error) { if stringValue, ok := m.Condition.Value.(string); ok { attributeValue, err := user.GetStringAttribute(m.Condition.Name) if err != nil { - return false, err + return false, nil } return strings.Contains(attributeValue, stringValue), nil } diff --git a/pkg/decision/evaluator/matchers/substring_test.go b/pkg/decision/evaluator/matchers/substring_test.go index 623f842fa..eed9fd557 100644 --- a/pkg/decision/evaluator/matchers/substring_test.go +++ b/pkg/decision/evaluator/matchers/substring_test.go @@ -55,7 +55,7 @@ func TestSubstringMatcher(t *testing.T) { assert.NoError(t, err) assert.False(t, result) - // Test error case + // Test attribute not found user = entities.UserContext{ Attributes: map[string]interface{}{ "not_string_foo": "foo", @@ -63,5 +63,5 @@ func TestSubstringMatcher(t *testing.T) { } _, err = matcher.Match(user) - assert.Error(t, err) + assert.NoError(t, err) } From 11546e9d6f843ba11f3dafe8b934e1fb58fed28b Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Thu, 31 Oct 2019 11:36:03 -0700 Subject: [PATCH 3/3] fix: Test coverage. --- .../mappers/condition_trees.go | 4 +-- .../mappers/experiment_test.go | 30 ++++++++++++++++++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/pkg/config/datafileprojectconfig/mappers/condition_trees.go b/pkg/config/datafileprojectconfig/mappers/condition_trees.go index 2668a6361..46eb10191 100644 --- a/pkg/config/datafileprojectconfig/mappers/condition_trees.go +++ b/pkg/config/datafileprojectconfig/mappers/condition_trees.go @@ -108,9 +108,7 @@ func buildAudienceConditionTree(conditions interface{}) (conditionTree *entities value := reflect.ValueOf(conditions) visited := make(map[interface{}]bool) - conditionTree = &entities.TreeNode{ - Operator: "or", - } + conditionTree = &entities.TreeNode{ Operator: "or" } var populateConditions func(v reflect.Value, root *entities.TreeNode) populateConditions = func(v reflect.Value, root *entities.TreeNode) { diff --git a/pkg/config/datafileprojectconfig/mappers/experiment_test.go b/pkg/config/datafileprojectconfig/mappers/experiment_test.go index 0c5c73956..d978627f6 100644 --- a/pkg/config/datafileprojectconfig/mappers/experiment_test.go +++ b/pkg/config/datafileprojectconfig/mappers/experiment_test.go @@ -19,7 +19,7 @@ package mappers import ( "testing" - "github.com/json-iterator/go" + jsoniter "github.com/json-iterator/go" datafileEntities "github.com/optimizely/go-sdk/pkg/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/pkg/entities" "github.com/stretchr/testify/assert" @@ -113,3 +113,31 @@ func TestMapExperiments(t *testing.T) { assert.Equal(t, expectedExperiments, experiments) assert.Equal(t, expectedExperimentKeyMap, experimentKeyMap) } + +func TestMapExperimentsAudienceIdsOnly(t *testing.T) { + var rawExperiment datafileEntities.Experiment + rawExperiment.AudienceIds = []string{"11111", "11112"} + rawExperiment.Key = "test_experiment_1" + rawExperiment.ID = "22222" + + expectedExperiment := entities.Experiment{ + AudienceIds: rawExperiment.AudienceIds, + ID: rawExperiment.ID, + Key: rawExperiment.Key, + AudienceConditionTree: &entities.TreeNode{ + Operator: "or", + Nodes: []*entities.TreeNode{ + { + Operator: "", + Item: "11111", + }, + { + Operator: "", + Item: "11112", + }, + }, + }, + } + experiments, _ := MapExperiments([]datafileEntities.Experiment{rawExperiment}) + assert.Equal(t, expectedExperiment.AudienceConditionTree, experiments[rawExperiment.ID].AudienceConditionTree) +}