Merge pull request #35 from davejohnston/FFM-1219

[FFM-1219]: Fix how segment rules are processed

evaluation/feature.go

@@ -6,7 +6,7 @@ import (
 	"reflect"
 	"strconv"
 
-	"github.com/labstack/gommon/log"
+	"github.com/drone/ff-golang-server-sdk/log"
 
 	"github.com/drone/ff-golang-server-sdk/types"
 )
@@ -88,7 +88,7 @@ func (c Clauses) Evaluate(target *Target, segments Segments) bool {
 		// operator should be evaluated based on type of attribute
 		op, err := target.GetOperator(clause.Attribute)
 		if err != nil {
-			fmt.Print(err)
+			log.Warn(err)
 		}
 		if !clause.Evaluate(target, segments, op) {
 			return false

evaluation/segment.go

@@ -1,6 +1,10 @@
 package evaluation
 
-import "strings"
+import (
+	"strings"
+
+	"github.com/drone/ff-golang-server-sdk/log"
+)
 
 // StrSlice helper type used for string slice operations
 type StrSlice []string
@@ -25,6 +29,32 @@ func (slice StrSlice) ContainsSensitive(s string) bool {
 	return false
 }
 
+// SegmentRules is a set of clauses to determine if a target should be included in the segment.
+type SegmentRules Clauses
+
+// Evaluate SegmentRules.  This determines if a segment rule is being used to include a target with
+// the segment.  SegmentRules are similar to ServingRules except a ServingRule can contain multiple clauses
+// but a Segment rule only contains one clause.
+//
+func (c SegmentRules) Evaluate(target *Target, segments Segments) bool {
+	// OR operation
+	for _, clause := range c {
+		// operator should be evaluated based on type of attribute
+		op, err := target.GetOperator(clause.Attribute)
+		if err != nil {
+			log.Warn(err)
+			continue
+		}
+		if clause.Evaluate(target, segments, op) {
+			return true
+		}
+
+		// continue on next rule
+	}
+	// it means that there was no matching rule
+	return false
+}
+
 // Segment object used in feature flag evaluation.
 // Examples: beta users, premium customers
 type Segment struct {
@@ -41,24 +71,33 @@ type Segment struct {
 	Included StrSlice
 
 	// An array of rules that can cause a user to be included in this segment.
-	Rules   Clauses
+	Rules   SegmentRules
 	Tags    []Tag
 	Version int64
 }
 
 // Evaluate segment based on target input
 func (s Segment) Evaluate(target *Target) bool {
+
+	// is target excluded from segment via the exclude list
+	if s.Excluded.ContainsSensitive(target.Identifier) {
+		log.Debugf("target %s excluded from segment %s via exclude list\n", target.Identifier, s.Identifier)
+		return false
+	}
+
+	// is target included from segment via the include list
 	if s.Included.ContainsSensitive(target.Identifier) {
+		log.Debugf("target %s included in segment %s via include list\n", target.Identifier, s.Identifier)
 		return true
 	}
 
+	// is target included in the segment via the clauses
 	if s.Rules.Evaluate(target, nil) {
+		log.Debugf("target %s included in segment %s via rules\n", target.Identifier, s.Identifier)
 		return true
 	}
 
-	if s.Excluded.ContainsSensitive(target.Identifier) {
-		return true
-	}
+	log.Debugf("No rules to include target %s in segment %s\n", target.Identifier, s.Identifier)
 	return false
 }
 

evaluation/segment_test.go

@@ -1,27 +1,17 @@
 package evaluation
 
 import (
-	"github.com/google/uuid"
-
 	"testing"
 )
 
 func TestSegment_Evaluate(t *testing.T) {
 	type fields struct {
-		Identifier  string
-		Name        string
-		CreatedAt   *int64
-		ModifiedAt  *int64
-		Environment *string
-		Excluded    StrSlice
-		Included    StrSlice
-		Rules       Clauses
-		Tags        []Tag
-		Version     int64
-	}
-	type args struct {
-		target *Target
+		Identifier string
+		Excluded   StrSlice
+		Included   StrSlice
+		Rules      SegmentRules
 	}
+
 	f := false
 	m := make(map[string]interface{})
 	m["email"] = "john@doe.com"
@@ -35,54 +25,28 @@ func TestSegment_Evaluate(t *testing.T) {
 	tests := []struct {
 		name   string
 		fields fields
-		args   args
+		args   Target
 		want   bool
 	}{
-		{name: "test included", fields: struct {
-			Identifier  string
-			Name        string
-			CreatedAt   *int64
-			ModifiedAt  *int64
-			Environment *string
-			Excluded    StrSlice
-			Included    StrSlice
-			Rules       Clauses
-			Tags        []Tag
-			Version     int64
-		}{Identifier: "beta", Name: "Beta users", CreatedAt: nil, ModifiedAt: nil, Environment: nil, Excluded: nil,
-			Included: []string{"john"}, Rules: nil, Tags: nil, Version: 1}, args: struct{ target *Target }{target: &target}, want: true},
-		{name: "test rules", fields: struct {
-			Identifier  string
-			Name        string
-			CreatedAt   *int64
-			ModifiedAt  *int64
-			Environment *string
-			Excluded    StrSlice
-			Included    StrSlice
-			Rules       Clauses
-			Tags        []Tag
-			Version     int64
-		}{Identifier: "beta", Name: "Beta users", CreatedAt: nil, ModifiedAt: nil, Environment: nil, Excluded: nil,
-			Included: nil, Rules: []Clause{
-				{Attribute: "email", ID: uuid.New().String(), Negate: false, Op: equalOperator, Value: []string{"john@doe.com"}},
-			}, Tags: nil, Version: 1}, args: struct{ target *Target }{target: &target}, want: true},
+		{name: "test target included by list", fields: fields{Identifier: "beta", Included: []string{"john"}}, args: target, want: true},
+		{name: "test target excluded by list", fields: fields{Identifier: "beta", Included: []string{"john"}, Excluded: []string{"john"}}, args: target, want: false},
+		{name: "test target included by rules", fields: fields{Identifier: "beta", Rules: []Clause{{Attribute: "email", ID: "1", Op: equalOperator, Value: []string{"john@doe.com"}}}}, args: target, want: true},
+		{name: "test target not included by rules", fields: fields{Identifier: "beta", Rules: []Clause{{Attribute: "email", ID: "2", Op: equalOperator, Value: []string{"foo@doe.com"}}}}, args: target, want: false},
+		{name: "test target rules evaluating with OR", fields: fields{Identifier: "beta", Rules: []Clause{
+			{Attribute: "email", ID: "1", Op: equalOperator, Value: []string{"john@doe.com"}},
+			{Attribute: "email", ID: "2", Op: equalOperator, Value: []string{"foo@doe.com"}},
+		}}, args: target, want: true},
 	}
 	for _, tt := range tests {
 		val := tt
 		t.Run(val.name, func(t *testing.T) {
 			s := Segment{
-				Identifier:  val.fields.Identifier,
-				Name:        val.fields.Name,
-				CreatedAt:   val.fields.CreatedAt,
-				ModifiedAt:  val.fields.ModifiedAt,
-				Environment: val.fields.Environment,
-				Excluded:    val.fields.Excluded,
-				Included:    val.fields.Included,
-				Rules:       val.fields.Rules,
-				Tags:        val.fields.Tags,
-				Version:     val.fields.Version,
+				Identifier: val.fields.Identifier,
+				Excluded:   val.fields.Excluded,
+				Included:   val.fields.Included,
+				Rules:      val.fields.Rules,
 			}
-			if got := s.Evaluate(val.args.target); got != val.want {
+			if got := s.Evaluate(&val.args); got != val.want {
 				t.Errorf("Evaluate() = %v, want %v", got, val.want)
 			}
 		})

log/log.go

@@ -0,0 +1,66 @@
+package log
+
+import "github.com/drone/ff-golang-server-sdk/logger"
+
+// And just go global.
+var defaultLogger logger.Logger
+
+// init creates the default logger.  This can be changed
+func init() {
+	defaultLogger, _ = logger.NewZapLogger(false)
+}
+
+// SetLogger sets the default logger to be used by this package
+func SetLogger(logger logger.Logger) {
+	defaultLogger = logger
+}
+
+// Error logs an error message with the parameters
+func Error(args ...interface{}) {
+	defaultLogger.Error(args...)
+}
+
+// Errorf logs a formatted error message
+func Errorf(format string, args ...interface{}) {
+	defaultLogger.Errorf(format, args...)
+}
+
+// Fatalf logs a formatted fatal message.  This will terminate the application
+func Fatalf(format string, args ...interface{}) {
+	defaultLogger.Fatalf(format, args...)
+}
+
+// Fatal logs an fatal message with the parameters.  This will terminate the application
+func Fatal(args ...interface{}) {
+	defaultLogger.Fatal(args...)
+}
+
+// Infof logs a formatted info message
+func Infof(format string, args ...interface{}) {
+	defaultLogger.Infof(format, args...)
+}
+
+// Info logs an info message with the parameters
+func Info(args ...interface{}) {
+	defaultLogger.Info(args...)
+}
+
+// Warn logs an warn message with the parameters
+func Warn(args ...interface{}) {
+	defaultLogger.Warn(args...)
+}
+
+// Warnf logs a formatted warn message
+func Warnf(format string, args ...interface{}) {
+	defaultLogger.Warnf(format, args...)
+}
+
+// Debugf logs a formatted debug message
+func Debugf(format string, args ...interface{}) {
+	defaultLogger.Debugf(format, args...)
+}
+
+// Debug logs an debug message with the parameters
+func Debug(args ...interface{}) {
+	defaultLogger.Debug(args...)
+}

rest/adapter.go

@@ -1,6 +1,8 @@
 package rest
 
-import "github.com/drone/ff-golang-server-sdk/evaluation"
+import (
+	"github.com/drone/ff-golang-server-sdk/evaluation"
+)
 
 func (wv WeightedVariation) convert() *evaluation.WeightedVariation {
 	return &evaluation.WeightedVariation{
@@ -144,21 +146,21 @@ func (s Segment) Convert() evaluation.Segment {
 	if s.Excluded != nil {
 		excluded = make(evaluation.StrSlice, len(*s.Excluded))
 		for i, excl := range *s.Excluded {
-			excluded[i] = excl.Name
+			excluded[i] = excl.Identifier
 		}
 	}
 
 	included := make(evaluation.StrSlice, 0)
 	if s.Included != nil {
 		included = make(evaluation.StrSlice, len(*s.Included))
 		for i, incl := range *s.Included {
-			included[i] = incl.Name
+			included[i] = incl.Identifier
 		}
 	}
 
-	rules := make(evaluation.Clauses, 0)
+	rules := make(evaluation.SegmentRules, 0)
 	if s.Rules != nil {
-		rules = make(evaluation.Clauses, len(*s.Rules))
+		rules = make(evaluation.SegmentRules, len(*s.Rules))
 		for i, rule := range *s.Rules {
 			rules[i] = evaluation.Clause{
 				Attribute: rule.Attribute,