Parse comparison expressions when loading policies

policy/predicate/file.go

@@ -15,9 +15,9 @@
 package predicate
 
 import (
+	"bytes"
 	"context"
 	"fmt"
-	"regexp"
 	"strconv"
 
 	"github.com/palantir/policy-bot/policy/common"
@@ -96,35 +96,80 @@ type ModifiedLines struct {
 	Total     ComparisonExpr `yaml:"total"`
 }
 
-type ComparisonExpr string
+type CompareOp uint8
 
-var (
-	numCompRegexp = regexp.MustCompile(`^(<|>) ?(\d+)$`)
+const (
+	OpNone CompareOp = iota
+	OpLessThan
+	OpGreaterThan
 )
 
+type ComparisonExpr struct {
+	Op    CompareOp
+	Value int64
+}
+
 func (exp ComparisonExpr) IsEmpty() bool {
-	return exp == ""
+	return exp.Op == OpNone && exp.Value == 0
 }
 
-func (exp ComparisonExpr) Evaluate(n int64) (bool, error) {
-	match := numCompRegexp.FindStringSubmatch(string(exp))
-	if match == nil {
-		return false, errors.Errorf("invalid comparison expression: %q", exp)
+func (exp ComparisonExpr) Evaluate(n int64) bool {
+	switch exp.Op {
+	case OpLessThan:
+		return n < exp.Value
+	case OpGreaterThan:
+		return n > exp.Value
 	}
+	return false
+}
 
-	op := match[1]
-	v, err := strconv.ParseInt(match[2], 10, 64)
-	if err != nil {
-		return false, errors.Wrapf(err, "invalid commparison expression: %q", exp)
+func (exp ComparisonExpr) MarshalText() ([]byte, error) {
+	if exp.Op == OpNone {
+		return nil, nil
 	}
 
-	switch op {
-	case "<":
-		return n < v, nil
-	case ">":
-		return n > v, nil
+	var op string
+	switch exp.Op {
+	case OpLessThan:
+		op = "<"
+	case OpGreaterThan:
+		op = ">"
+	default:
+		return nil, errors.Errorf("unknown operation: %d", exp.Op)
 	}
-	return false, errors.Errorf("invalid comparison expression: %q", exp)
+	return []byte(fmt.Sprintf("%s %d", op, exp.Value)), nil
+}
+
+func (exp *ComparisonExpr) UnmarshalText(text []byte) error {
+	text = bytes.TrimSpace(text)
+	if len(text) == 0 {
+		*exp = ComparisonExpr{}
+		return nil
+	}
+
+	i := 0
+	var op CompareOp
+	switch text[i] {
+	case '<':
+		op = OpLessThan
+	case '>':
+		op = OpGreaterThan
+	default:
+		return errors.Errorf("invalid comparison operator: %c", text[i])
+	}
+
+	i++
+	for i < len(text) && (text[i] == ' ' || text[i] == '\t') {
+		i++
+	}
+
+	v, err := strconv.ParseInt(string(text[i:]), 10, 64)
+	if err != nil {
+		return errors.Wrapf(err, "invalid comparison value")
+	}
+
+	*exp = ComparisonExpr{Op: op, Value: v}
+	return nil
 }
 
 func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) {
@@ -144,14 +189,8 @@ func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bo
 		pred.Deletions: deletions,
 		pred.Total:     additions + deletions,
 	} {
-		if !expr.IsEmpty() {
-			ok, err := expr.Evaluate(v)
-			if err != nil {
-				return false, "", err
-			}
-			if ok {
-				return true, "", nil
-			}
+		if !expr.IsEmpty() && expr.Evaluate(v) {
+			return true, "", nil
 		}
 	}
 	return false, fmt.Sprintf("modification of (+%d, -%d) does not match any conditions", additions, deletions), nil

policy/predicate/file_test.go

@@ -176,8 +176,8 @@ func TestOnlyChangedFiles(t *testing.T) {
 
 func TestModifiedLines(t *testing.T) {
 	p := &ModifiedLines{
-		Additions: ">100",
-		Deletions: ">10",
+		Additions: ComparisonExpr{Op: OpGreaterThan, Value: 100},
+		Deletions: ComparisonExpr{Op: OpGreaterThan, Value: 10},
 	}
 
 	runFileTests(t, p, []FileTestCase{
@@ -208,7 +208,7 @@ func TestModifiedLines(t *testing.T) {
 	})
 
 	p = &ModifiedLines{
-		Total: ">100",
+		Total: ComparisonExpr{Op: OpGreaterThan, Value: 100},
 	}
 
 	runFileTests(t, p, []FileTestCase{
@@ -230,55 +230,89 @@ func TestComparisonExpr(t *testing.T) {
 		Expr   ComparisonExpr
 		Value  int64
 		Output bool
-		Err    bool
 	}{
 		"greaterThanTrue": {
-			Expr:   ">100",
+			Expr:   ComparisonExpr{Op: OpGreaterThan, Value: 100},
 			Value:  200,
 			Output: true,
 		},
 		"greaterThanFalse": {
-			Expr:   "> 100",
+			Expr:   ComparisonExpr{Op: OpGreaterThan, Value: 100},
 			Value:  50,
 			Output: false,
 		},
 		"lessThanTrue": {
-			Expr:   "<100",
+			Expr:   ComparisonExpr{Op: OpLessThan, Value: 100},
 			Value:  50,
 			Output: true,
 		},
 		"lessThanFalse": {
-			Expr:   "< 100",
+			Expr:   ComparisonExpr{Op: OpLessThan, Value: 100},
 			Value:  200,
 			Output: false,
 		},
-		"invalidOperator": {
-			Expr: "=200",
-			Err:  true,
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			ok := test.Expr.Evaluate(test.Value)
+			assert.Equal(t, test.Output, ok, "evaluation was not correct")
+		})
+	}
+
+	t.Run("isEmpty", func(t *testing.T) {
+		assert.True(t, ComparisonExpr{}.IsEmpty(), "expression was not empty")
+		assert.False(t, ComparisonExpr{Op: OpGreaterThan, Value: 100}.IsEmpty(), "expression was empty")
+	})
+
+	parseTests := map[string]struct {
+		Input  string
+		Output ComparisonExpr
+		Err    bool
+	}{
+		"lessThan": {
+			Input:  "<100",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 100},
+		},
+		"greaterThan": {
+			Input:  ">100",
+			Output: ComparisonExpr{Op: OpGreaterThan, Value: 100},
+		},
+		"innerSpaces": {
+			Input:  "<   35",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
+		},
+		"leadingSpaces": {
+			Input:  "   < 35",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
+		},
+		"trailngSpaces": {
+			Input:  "< 35   ",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
 		},
-		"invalidNumber": {
-			Expr: ">12ab",
-			Err:  true,
+		"invalidOp": {
+			Input: "=10",
+			Err:   true,
+		},
+		"invalidValue": {
+			Input: "< 10ab",
+			Err:   true,
 		},
 	}
 
-	for name, test := range tests {
+	for name, test := range parseTests {
 		t.Run(name, func(t *testing.T) {
-			ok, err := test.Expr.Evaluate(test.Value)
+			var expr ComparisonExpr
+			err := expr.UnmarshalText([]byte(test.Input))
 			if test.Err {
-				assert.Error(t, err, "expected error evaluating expression, but got nil")
+				assert.Error(t, err, "expected error parsing expression, but got nil")
 				return
 			}
-			if assert.NoError(t, err, "unexpected error evaluating expression") {
-				assert.Equal(t, test.Output, ok, "evaluation was not correct")
+			if assert.NoError(t, err, "unexpected error parsing expression") {
+				assert.Equal(t, test.Output, expr, "parsed expression was not correct")
 			}
 		})
 	}
-
-	t.Run("isEmpty", func(t *testing.T) {
-		assert.True(t, ComparisonExpr("").IsEmpty(), "expression was not empty")
-		assert.False(t, ComparisonExpr(">100").IsEmpty(), "expression was empty")
-	})
 }
 
 type FileTestCase struct {