(MAINT) If an operator is nil, it can cause a panic

What: Adding check to ensure we only use operator if it is non-nul.

Why: Its possible for a clause to have a nil operator, but the only valid
scenario for this, is if it is a segmentMatch operation.  We need protective code
to ensure we don't accidently call a nil operator.

Author: Dave Johnston

evaluation/feature.go

@@ -4,6 +4,8 @@ import (
 	"encoding/json"
 	"fmt"
 
+	"github.com/labstack/gommon/log"
+
 	"github.com/drone/ff-golang-server-sdk/types"
 
 	"reflect"
@@ -43,26 +45,34 @@ type Clause struct {
 
 // Evaluate clause using target but it can be used also with segments if Op field is segmentMach
 func (c *Clause) Evaluate(target *Target, segments Segments, operator types.ValueType) bool {
-	switch c.Op {
-	case segmentMatchOperator:
+
+	// Special case - segment matcher doesn't require a
+	// valid operator.
+	if c.Op == segmentMatchOperator {
 		if segments == nil {
 			return false
 		}
 		return segments.Evaluate(target)
-	case inOperator:
-		return operator.In(c.Value)
-	case equalOperator:
-		return operator.Equal(c.Value)
-	case gtOperator:
-		return operator.GreaterThan(c.Value)
-	case startsWithOperator:
-		return operator.StartsWith(c.Value)
-	case endsWithOperator:
-		return operator.EndsWith(c.Value)
-	case containsOperator:
-		return operator.Contains(c.Value)
-	case equalSensitiveOperator:
-		return operator.EqualSensitive(c.Value)
+	}
+
+	// Ensure operator is valid and not nil
+	if operator != nil {
+		switch c.Op {
+		case inOperator:
+			return operator.In(c.Value)
+		case equalOperator:
+			return operator.Equal(c.Value)
+		case gtOperator:
+			return operator.GreaterThan(c.Value)
+		case startsWithOperator:
+			return operator.StartsWith(c.Value)
+		case endsWithOperator:
+			return operator.EndsWith(c.Value)
+		case containsOperator:
+			return operator.Contains(c.Value)
+		case equalSensitiveOperator:
+			return operator.EqualSensitive(c.Value)
+		}
 	}
 	return false
 }
@@ -71,11 +81,17 @@ func (c *Clause) Evaluate(target *Target, segments Segments, operator types.Valu
 type Clauses []Clause
 
 // Evaluate clauses using target but it can be used also with segments if Op field is segmentMach
+// TODO this func can return false because of an error.  We need a way to indicate to the caller if this is false
+// because it evaluated false, or because it actually failed to work.
 func (c Clauses) Evaluate(target *Target, segments Segments) bool {
 	// AND operation
 	for _, clause := range c {
 		// operator should be evaluated based on type of attribute
-		op := target.GetOperator(clause.Attribute)
+		op, err := target.GetOperator(clause.Attribute)
+		if err != nil {
+			log.Warn(err)
+			//return false
+		}
 		if !clause.Evaluate(target, segments, op) {
 			return false
 		}

evaluation/target.go

@@ -1,6 +1,8 @@
 package evaluation
 
 import (
+	"fmt"
+
 	"github.com/drone/ff-golang-server-sdk/types"
 
 	"reflect"
@@ -28,22 +30,22 @@ func (t Target) GetAttrValue(attr string) reflect.Value {
 }
 
 // GetOperator returns interface based on attribute value
-func (t Target) GetOperator(attr string) types.ValueType {
+func (t Target) GetOperator(attr string) (types.ValueType, error) {
 	value := t.GetAttrValue(attr)
 	switch value.Kind() {
 	case reflect.Bool:
-		return types.Boolean(value.Bool())
+		return types.Boolean(value.Bool()), nil
 	case reflect.String:
-		return types.String(value.String())
+		return types.String(value.String()), nil
 	case reflect.Float64, reflect.Float32:
-		return types.Number(value.Float())
+		return types.Number(value.Float()), nil
 	case reflect.Int, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Int8, reflect.Uint, reflect.Uint16,
 		reflect.Uint32, reflect.Uint64, reflect.Uint8:
-		return types.Integer(value.Int())
+		return types.Integer(value.Int()), nil
 	case reflect.Array, reflect.Chan, reflect.Complex128, reflect.Complex64, reflect.Func, reflect.Interface,
 		reflect.Invalid, reflect.Map, reflect.Ptr, reflect.Slice, reflect.Struct, reflect.Uintptr, reflect.UnsafePointer:
-		return nil
+		fallthrough
 	default:
-		return nil
+		return nil, fmt.Errorf("unexpected type: %s", value.Kind().String())
 	}
 }

evaluation/target_test.go

@@ -21,10 +21,11 @@ func TestTarget_GetOperator(t1 *testing.T) {
 		attr string
 	}
 	tests := []struct {
-		name   string
-		fields fields
-		args   args
-		want   types.ValueType
+		name    string
+		fields  fields
+		args    args
+		want    types.ValueType
+		wantErr bool
 	}{
 		{name: "boolean operator", fields: struct {
 			Identifier string
@@ -68,8 +69,13 @@ func TestTarget_GetOperator(t1 *testing.T) {
 				Anonymous:  &val.fields.Anonymous,
 				Attributes: &val.fields.Attributes,
 			}
-			if got := t.GetOperator(val.args.attr); !reflect.DeepEqual(got, val.want) {
-				t1.Errorf("GetOperator() = %v, want %v", got, val.want)
+			got, err := t.GetOperator(val.args.attr)
+			if (err != nil) != val.wantErr {
+				t1.Errorf("GetOperator() error = %v, wantErr %v", err, val.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, val.want) {
+				t1.Errorf("GetOperator() error = %v, want %v", err, val.want)
 			}
 		})
 	}
@@ -142,10 +148,11 @@ func TestTarget_GetOperator1(t1 *testing.T) {
 
 	name := "John"
 	tests := []struct {
-		name   string
-		fields fields
-		args   args
-		want   types.ValueType
+		name    string
+		fields  fields
+		args    args
+		want    types.ValueType
+		wantErr bool
 	}{
 		{name: "bool operator", fields: struct {
 			Identifier string
@@ -189,8 +196,13 @@ func TestTarget_GetOperator1(t1 *testing.T) {
 				Anonymous:  &val.fields.Anonymous,
 				Attributes: &val.fields.Attributes,
 			}
-			if got := t.GetOperator(val.args.attr); !reflect.DeepEqual(got, val.want) {
-				t1.Errorf("GetOperator() = %v, want %v", got, val.want)
+			got, err := t.GetOperator(val.args.attr)
+			if (err != nil) != val.wantErr {
+				t1.Errorf("GetOperator() error = %v, wantErr %v", err, val.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, val.want) {
+				t1.Errorf("GetOperator() error = %v, want %v", err, val.want)
 			}
 		})
 	}