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

What:
  * change logic so the include/exclude rules use target identifier rather name to be consistent
  with how the rules are stored.
  * Make exclude list take precedence over include list and rules.
  * Process segment rules as OR rather than AND.

Why:
  When creating segment include/exclude lists the server only stores target identifiers not
  names, so this logic needed to be updated so the lists were correctly evaluated.

  The exclude list was being evaluated last, which meant we could not include all users via
  a rule, but exclude one via a list.  This is consistent with how the Java SDK behaves.

  Segment rules should be OR'd, but they were being treated like flag clauses and were being
  AND'd which meant conflicting rules would prevent targets from correctly being included.

Author: Dave Johnston

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"] = "[email protected]"
@@ -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{"[email protected]"}},
-			}, 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{"[email protected]"}}}}, 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{"[email protected]"}}}}, 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{"[email protected]"}},
+			{Attribute: "email", ID: "2", Op: equalOperator, Value: []string{"[email protected]"}},
+		}}, 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)
 			}
 		})

evaluation/target.go

@@ -46,6 +46,6 @@ func (t Target) GetOperator(attr string) (types.ValueType, error) {
 		reflect.Invalid, reflect.Map, reflect.Ptr, reflect.Slice, reflect.Struct, reflect.Uintptr, reflect.UnsafePointer:
 		fallthrough
 	default:
-		return nil, fmt.Errorf("unexpected type: %s", value.Kind().String())
+		return nil, fmt.Errorf("unexpected type: %s\n", value.Kind().String())
 	}
 }

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,