From 82c933c079d5358dc1d336da2a573dbdfbf0208d Mon Sep 17 00:00:00 2001 From: Nadia Mayor Date: Fri, 10 May 2024 16:44:29 -0300 Subject: [PATCH] Add unssuported matcher logic in NewSplit and tets cases --- engine/grammar/condition.go | 22 ++--- engine/grammar/condition_test.go | 99 +++++++++++++++++++++- engine/grammar/split.go | 28 ++++-- engine/grammar/split_test.go | 141 +++++++++++++++++++++++++++++++ engine/validator/matchers.go | 3 +- 5 files changed, 273 insertions(+), 20 deletions(-) diff --git a/engine/grammar/condition.go b/engine/grammar/condition.go index e55dec5d..a1f9463a 100644 --- a/engine/grammar/condition.go +++ b/engine/grammar/condition.go @@ -18,17 +18,14 @@ type Condition struct { } // NewCondition instantiates a new Condition struct with appropriate wrappers around dtos and returns it. -func NewCondition(cond *dtos.ConditionDTO, ctx *injection.Context, logger logging.LoggerInterface) *Condition { +func NewCondition(cond *dtos.ConditionDTO, ctx *injection.Context, logger logging.LoggerInterface) (*Condition, error) { partitions := make([]Partition, 0) for _, part := range cond.Partitions { partitions = append(partitions, Partition{partitionData: part}) } - matcherObjs := make([]matchers.MatcherInterface, 0) - for _, matcher := range cond.MatcherGroup.Matchers { - m, err := matchers.BuildMatcher(&matcher, ctx, logger) - if err == nil { - matcherObjs = append(matcherObjs, m) - } + matcherObjs, err := processMatchers(cond.MatcherGroup.Matchers, ctx, logger) + if err != nil { + return nil, err } return &Condition{ @@ -37,19 +34,22 @@ func NewCondition(cond *dtos.ConditionDTO, ctx *injection.Context, logger loggin partitions: partitions, label: cond.Label, conditionType: cond.ConditionType, - } + }, nil } -func proccessMatchers(condMatchers []dtos.MatcherDTO, ctx *injection.Context, logger logging.LoggerInterface) []matchers.MatcherInterface { +func processMatchers(condMatchers []dtos.MatcherDTO, ctx *injection.Context, logger logging.LoggerInterface) ([]matchers.MatcherInterface, error) { matcherObjs := make([]matchers.MatcherInterface, 0) for _, matcher := range condMatchers { m, err := matchers.BuildMatcher(&matcher, ctx, logger) if err == nil { matcherObjs = append(matcherObjs, m) - } else if errType, ok := err.(datatypes.UnsupportedMatcherError); ok { - + } else { + if _, ok := err.(datatypes.UnsupportedMatcherError); ok { + return nil, err + } } } + return matcherObjs, nil } // Partition struct with added logic that wraps around a DTO diff --git a/engine/grammar/condition_test.go b/engine/grammar/condition_test.go index 43f38a62..2350fcee 100644 --- a/engine/grammar/condition_test.go +++ b/engine/grammar/condition_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/splitio/go-split-commons/v5/dtos" + "github.com/splitio/go-split-commons/v5/engine/grammar/matchers" "github.com/splitio/go-toolkit/v5/logging" ) @@ -46,7 +47,11 @@ func TestConditionWrapperObject(t *testing.T) { }, } - wrapped := NewCondition(&condition1, nil, logger) + wrapped, err := NewCondition(&condition1, nil, logger) + + if err != nil { + t.Error("err should be nil") + } if wrapped.Label() != "Label1" { t.Error("Label not set properly") @@ -113,7 +118,10 @@ func TestAnotherWrapper(t *testing.T) { }, } - wrapped := NewCondition(&condition1, nil, logger) + wrapped, err := NewCondition(&condition1, nil, logger) + if err != nil { + t.Error("err should be nil") + } if wrapped.Label() != "Label2" { t.Error("Label not set properly") @@ -141,3 +149,90 @@ func TestAnotherWrapper(t *testing.T) { t.Error("CalculateTreatment returned incorrect treatment") } } + +func TestConditionUnsupportedMatcherWrapperObject(t *testing.T) { + logger := logging.NewLogger(&logging.LoggerOptions{}) + condition1 := dtos.ConditionDTO{ + ConditionType: "WHITELIST", + Label: "Label1", + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{ + { + Negate: false, + MatcherType: "UNSUPPORTED", + KeySelector: &dtos.KeySelectorDTO{ + Attribute: nil, + TrafficType: "something", + }, + }, + { + Negate: true, + MatcherType: "ALL_KEYS", + KeySelector: &dtos.KeySelectorDTO{ + Attribute: nil, + TrafficType: "something", + }, + }, + }, + }, + Partitions: []dtos.PartitionDTO{ + { + Size: 75, + Treatment: "on", + }, + { + Size: 25, + Treatment: "off", + }, + }, + } + + _, err := NewCondition(&condition1, nil, logger) + + if err == nil { + t.Error("err should not be nil") + } +} + +func TestConditionMatcherWithNilStringWrapperObject(t *testing.T) { + logger := logging.NewLogger(&logging.LoggerOptions{}) + condition1 := dtos.ConditionDTO{ + ConditionType: "WHITELIST", + Label: "Label1", + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{ + { + Negate: false, + MatcherType: matchers.MatcherTypeStartsWith, + KeySelector: &dtos.KeySelectorDTO{ + Attribute: nil, + TrafficType: "something", + }, + Whitelist: nil, + }, + }, + }, + Partitions: []dtos.PartitionDTO{ + { + Size: 75, + Treatment: "on", + }, + { + Size: 25, + Treatment: "off", + }, + }, + } + + condition, err := NewCondition(&condition1, nil, logger) + + if err != nil { + t.Error("err should be nil") + } + + if len(condition.matchers) != 0 { + t.Error("matchers should be empty") + } +} diff --git a/engine/grammar/split.go b/engine/grammar/split.go index 30a65efd..0eacf729 100644 --- a/engine/grammar/split.go +++ b/engine/grammar/split.go @@ -2,6 +2,8 @@ package grammar import ( "github.com/splitio/go-split-commons/v5/dtos" + "github.com/splitio/go-split-commons/v5/engine/evaluator/impressionlabels" + "github.com/splitio/go-split-commons/v5/engine/grammar/matchers" "github.com/splitio/go-toolkit/v5/injection" "github.com/splitio/go-toolkit/v5/logging" @@ -13,21 +15,35 @@ type Split struct { conditions []*Condition } +var conditionReplacementUnsupportedMatcher []*Condition = []*Condition{{ + conditionType: ConditionTypeWhitelist, + label: impressionlabels.UnsupportedMatcherType, + partitions: []Partition{{partitionData: dtos.PartitionDTO{Treatment: "control", Size: 100}}}, + matchers: []matchers.MatcherInterface{matchers.NewAllKeysMatcher(false)}, +}} + // NewSplit instantiates a new Split object and all it's internal structures mapped to model classes func NewSplit(splitDTO *dtos.SplitDTO, ctx *injection.Context, logger logging.LoggerInterface) *Split { - conditions := make([]*Condition, 0) - for _, cond := range splitDTO.Conditions { - conditions = append(conditions, NewCondition(&cond, ctx, logger)) - } - split := Split{ - conditions: conditions, + conditions: processConditions(splitDTO.Conditions, splitDTO, ctx, logger), splitData: splitDTO, } return &split } +func processConditions(conditions []dtos.ConditionDTO, splitDTO *dtos.SplitDTO, ctx *injection.Context, logger logging.LoggerInterface) []*Condition { + conditionsToReturn := make([]*Condition, 0) + for _, cond := range splitDTO.Conditions { + condition, err := NewCondition(&cond, ctx, logger) + if err != nil { + return conditionReplacementUnsupportedMatcher + } + conditionsToReturn = append(conditionsToReturn, condition) + } + return conditionsToReturn +} + // Name returns the name of the feature func (s *Split) Name() string { return s.splitData.Name diff --git a/engine/grammar/split_test.go b/engine/grammar/split_test.go index 5b054c16..544ae14b 100644 --- a/engine/grammar/split_test.go +++ b/engine/grammar/split_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/splitio/go-split-commons/v5/dtos" + "github.com/splitio/go-split-commons/v5/engine/grammar/matchers" "github.com/splitio/go-toolkit/v5/logging" ) @@ -57,3 +58,143 @@ func TestSplitCreation(t *testing.T) { t.Error("Traffic allocation should be 100") } } + +func TestSplitCreationWithConditionsMatcher(t *testing.T) { + logger := logging.NewLogger(&logging.LoggerOptions{}) + dto := dtos.SplitDTO{ + Algo: 1, + ChangeNumber: 123, + Conditions: []dtos.ConditionDTO{{ + ConditionType: ConditionTypeWhitelist, + Label: "test", + Partitions: []dtos.PartitionDTO{{Treatment: "off", Size: 100}}, + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{{MatcherType: matchers.MatcherTypeAllKeys, Negate: false}}, + }, + }, { + ConditionType: ConditionTypeWhitelist, + Label: "test", + Partitions: []dtos.PartitionDTO{{Treatment: "on", Size: 100}}, + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{{MatcherType: matchers.MatcherTypeAllKeys, Negate: false}}, + }, + }}, + DefaultTreatment: "def", + Killed: false, + Name: "split1", + Seed: 1234, + Status: "ACTIVE", + TrafficAllocation: 100, + TrafficAllocationSeed: 333, + TrafficTypeName: "tt1", + } + split := NewSplit(&dto, nil, logger) + + if split.Algo() != SplitAlgoLegacy { + t.Error("Algo() should return legacy") + } + + if split.ChangeNumber() != 123 { + t.Error("Incorrect changenumber returned") + } + + if split.DefaultTreatment() != "def" { + t.Error("Incorrect default treatment") + } + + if split.Killed() { + t.Error("Split should not be marked as killed") + } + + if split.Seed() != 1234 { + t.Error("Incorrect seed") + } + + if split.Name() != "split1" { + t.Error("Incorrect split name") + } + + if split.Status() != SplitStatusActive { + t.Error("Status should be active") + } + + if split.TrafficAllocation() != 100 { + t.Error("Traffic allocation should be 100") + } + + if len(split.conditions) != 2 { + t.Error("conditions length should be 2") + } +} + +func TestSplitCreationWithUnsupportedMatcher(t *testing.T) { + logger := logging.NewLogger(&logging.LoggerOptions{}) + dto := dtos.SplitDTO{ + Algo: 1, + ChangeNumber: 123, + Conditions: []dtos.ConditionDTO{{ + ConditionType: ConditionTypeWhitelist, + Label: "test", + Partitions: []dtos.PartitionDTO{{Treatment: "control", Size: 100}}, + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{{MatcherType: "unssuported", Negate: false}}, + }, + }, { + ConditionType: ConditionTypeWhitelist, + Label: "test", + Partitions: []dtos.PartitionDTO{{Treatment: "on", Size: 100}}, + MatcherGroup: dtos.MatcherGroupDTO{ + Combiner: "AND", + Matchers: []dtos.MatcherDTO{{MatcherType: matchers.MatcherTypeAllKeys, Negate: false}}, + }, + }}, + DefaultTreatment: "def", + Killed: false, + Name: "split1", + Seed: 1234, + Status: "ACTIVE", + TrafficAllocation: 100, + TrafficAllocationSeed: 333, + TrafficTypeName: "tt1", + } + split := NewSplit(&dto, nil, logger) + + if split.Algo() != SplitAlgoLegacy { + t.Error("Algo() should return legacy") + } + + if split.ChangeNumber() != 123 { + t.Error("Incorrect changenumber returned") + } + + if split.DefaultTreatment() != "def" { + t.Error("Incorrect default treatment") + } + + if split.Killed() { + t.Error("Split should not be marked as killed") + } + + if split.Seed() != 1234 { + t.Error("Incorrect seed") + } + + if split.Name() != "split1" { + t.Error("Incorrect split name") + } + + if split.Status() != SplitStatusActive { + t.Error("Status should be active") + } + + if split.TrafficAllocation() != 100 { + t.Error("Traffic allocation should be 100") + } + + if len(split.conditions) != 1 { + t.Error("conditions length should be 1") + } +} diff --git a/engine/validator/matchers.go b/engine/validator/matchers.go index 3cc49da7..3a804e16 100644 --- a/engine/validator/matchers.go +++ b/engine/validator/matchers.go @@ -6,6 +6,7 @@ import ( "github.com/splitio/go-split-commons/v5/engine/evaluator/impressionlabels" "github.com/splitio/go-split-commons/v5/engine/grammar" "github.com/splitio/go-split-commons/v5/engine/grammar/matchers" + "github.com/splitio/go-split-commons/v5/engine/grammar/matchers/datatypes" "github.com/splitio/go-toolkit/v5/injection" "github.com/splitio/go-toolkit/v5/logging" ) @@ -25,7 +26,7 @@ func shouldOverrideConditions(conditions []dtos.ConditionDTO, logger logging.Log for _, condition := range conditions { for _, matcher := range condition.MatcherGroup.Matchers { _, err := matchers.BuildMatcher(&matcher, &injection.Context{}, logger) - if err != nil { + if _, ok := err.(datatypes.UnsupportedMatcherError); ok { return true } }