Skip to content

Commit

Permalink
Parse comparison expressions when loading policies (#300)
Browse files Browse the repository at this point in the history 
Expressions are functionally equivalent to before, but are now validated
as part of the validation endpoint. One behavior change is that we now
allow any number of spaces between the operator and the number and
before or after the expression as a whole.
  • Loading branch information
@bluekeyes
bluekeyes authored May 18, 2021
1 parent b80414e commit ccf65b1
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 51 deletions.
93 changes: 66 additions & 27 deletions policy/predicate/file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
package predicate

import (
"bytes"
"context"
"fmt"
"regexp"
"strconv"

"github.com/palantir/policy-bot/policy/common"
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down
82 changes: 58 additions & 24 deletions policy/predicate/file_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -208,7 +208,7 @@ func TestModifiedLines(t *testing.T) {
})

p = &ModifiedLines{
Total: ">100",
Total: ComparisonExpr{Op: OpGreaterThan, Value: 100},
}

runFileTests(t, p, []FileTestCase{
Expand All @@ -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 {
Expand Down

0 comments on commit ccf65b1

Please sign in to comment.