diff --git a/common/operators/operators.go b/common/operators/operators.go index 8a2e9094..fa25dfb7 100644 --- a/common/operators/operators.go +++ b/common/operators/operators.go @@ -141,3 +141,13 @@ func Precedence(symbol string) int { } return op.precedence } + +// Arity returns the number of argument the operator takes +// -1 is returned if an undefined symbol is provided +func Arity(symbol string) int { + op, found := operatorMap[symbol] + if !found { + return -1 + } + return op.arity +} diff --git a/parser/unparser.go b/parser/unparser.go index 6a610ff7..64440a94 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -36,9 +36,29 @@ import ( // - Floating point values are converted to the small number of digits needed to represent the value. // - Spacing around punctuation marks may be lost. // - Parentheses will only be applied when they affect operator precedence. -func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo) (string, error) { - un := &unparser{info: info} - err := un.visit(expr) +// +// This function optionally takes in one or more UnparserOption to alter the unparsing behavior, such as +// performing word wrapping on expressions. +func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { + unparserOpts := &unparserOption{ + wrapOnColumn: defaultWrapOnColumn, + wrapAfterColumnLimit: defaultWrapAfterColumnLimit, + operatorsToWrapOn: defaultOperatorsToWrapOn, + } + + var err error + for _, opt := range opts { + unparserOpts, err = opt(unparserOpts) + if err != nil { + return "", err + } + } + + un := &unparser{ + info: info, + options: unparserOpts, + } + err = un.visit(expr) if err != nil { return "", err } @@ -47,8 +67,10 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo) (string, error) { // unparser visits an expression to reconstruct a human-readable string from an AST. type unparser struct { - str strings.Builder - info *exprpb.SourceInfo + str strings.Builder + info *exprpb.SourceInfo + options *unparserOption + lastWrappedIndex int } func (un *unparser) visit(expr *exprpb.Expr) error { @@ -135,9 +157,8 @@ func (un *unparser) visitCallBinary(expr *exprpb.Expr) error { if !found { return fmt.Errorf("cannot unmangle operator: %s", fun) } - un.str.WriteString(" ") - un.str.WriteString(unmangled) - un.str.WriteString(" ") + + un.writeOperatorWithWrapping(fun, unmangled) return un.visitMaybeNested(rhs, rhsParen) } @@ -151,7 +172,8 @@ func (un *unparser) visitCallConditional(expr *exprpb.Expr) error { if err != nil { return err } - un.str.WriteString(" ? ") + un.writeOperatorWithWrapping(operators.Conditional, "?") + // add parens if operand is a conditional itself. nested = isSamePrecedence(operators.Conditional, args[1]) || isComplexOperator(args[1]) @@ -159,6 +181,7 @@ func (un *unparser) visitCallConditional(expr *exprpb.Expr) error { if err != nil { return err } + un.str.WriteString(" : ") // add parens if operand is a conditional itself. nested = isSamePrecedence(operators.Conditional, args[2]) || @@ -444,3 +467,130 @@ func bytesToOctets(byteVal []byte) string { } return b.String() } + +// writeOperatorWithWrapping outputs the operator and inserts a newline for operators configured +// in the unparser options. +func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool { + _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] + lineLength := un.str.Len() - un.lastWrappedIndex + len(fun) + + if wrapOperatorExists && lineLength >= un.options.wrapOnColumn { + un.lastWrappedIndex = un.str.Len() + // wrapAfterColumnLimit flag dictates whether the newline is placed + // before or after the operator + if un.options.wrapAfterColumnLimit { + // Input: a && b + // Output: a &&\nb + un.str.WriteString(" ") + un.str.WriteString(unmangled) + un.str.WriteString("\n") + } else { + // Input: a && b + // Output: a\n&& b + un.str.WriteString("\n") + un.str.WriteString(unmangled) + un.str.WriteString(" ") + } + return true + } else { + un.str.WriteString(" ") + un.str.WriteString(unmangled) + un.str.WriteString(" ") + } + return false +} + +// Defined defaults for the unparser options +var ( + defaultWrapOnColumn = 80 + defaultWrapAfterColumnLimit = true + defaultOperatorsToWrapOn = map[string]bool{ + operators.LogicalAnd: true, + operators.LogicalOr: true, + } +) + +// UnparserOption is a functional option for configuring the output formatting +// of the Unparse function. +type UnparserOption func(*unparserOption) (*unparserOption, error) + +// Internal representation of the UnparserOption type +type unparserOption struct { + wrapOnColumn int + operatorsToWrapOn map[string]bool + wrapAfterColumnLimit bool +} + +// WrapOnColumn wraps the output expression when its string length exceeds a specified limit +// for operators set by WrapOnOperators function or by default, "&&" and "||" will be wrapped. +// +// Example usage: +// +// Unparse(expr, sourceInfo, WrapOnColumn(40), WrapOnOperators(Operators.LogicalAnd)) +// +// This will insert a newline immediately after the logical AND operator for the below example input: +// +// Input: +// 'my-principal-group' in request.auth.claims && request.auth.claims.iat > now - duration('5m') +// +// Output: +// 'my-principal-group' in request.auth.claims && +// request.auth.claims.iat > now - duration('5m') +func WrapOnColumn(col int) UnparserOption { + return func(opt *unparserOption) (*unparserOption, error) { + if col < 1 { + return nil, fmt.Errorf("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got %v instead", col) + } + opt.wrapOnColumn = col + return opt, nil + } +} + +// WrapOnOperators specifies which operators to perform word wrapping on an output expression when its string length +// exceeds the column limit set by WrapOnColumn function. +// +// Word wrapping is supported on non-unary symbolic operators. Refer to operators.go for the full list +// +// This will replace any previously supplied operators instead of merging them. +func WrapOnOperators(symbols ...string) UnparserOption { + return func(opt *unparserOption) (*unparserOption, error) { + opt.operatorsToWrapOn = make(map[string]bool) + for _, symbol := range symbols { + _, found := operators.FindReverse(symbol) + if !found { + return nil, fmt.Errorf("Invalid unparser option. Unsupported operator: %s", symbol) + } + arity := operators.Arity(symbol) + if arity < 2 { + return nil, fmt.Errorf("Invalid unparser option. Unary operators are unsupported: %s", symbol) + } + + opt.operatorsToWrapOn[symbol] = true + } + + return opt, nil + } +} + +// WrapAfterColumnLimit dictates whether to insert a newline before or after the specified operator +// when word wrapping is performed. +// +// Example usage: +// +// Unparse(expr, sourceInfo, WrapOnColumn(40), WrapOnOperators(Operators.LogicalAnd), WrapAfterColumnLimit(false)) +// +// This will insert a newline immediately before the logical AND operator for the below example input, ensuring +// that the length of a line never exceeds the specified column limit: +// +// Input: +// 'my-principal-group' in request.auth.claims && request.auth.claims.iat > now - duration('5m') +// +// Output: +// 'my-principal-group' in request.auth.claims +// && request.auth.claims.iat > now - duration('5m') +func WrapAfterColumnLimit(wrapAfter bool) UnparserOption { + return func(opt *unparserOption) (*unparserOption, error) { + opt.wrapAfterColumnLimit = wrapAfter + return opt, nil + } +} diff --git a/parser/unparser_test.go b/parser/unparser_test.go index 7bb41d39..fad688e9 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -20,6 +20,8 @@ import ( "testing" "github.com/google/cel-go/common" + "github.com/google/cel-go/common/operators" + "google.golang.org/protobuf/proto" exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" @@ -31,6 +33,7 @@ func TestUnparse(t *testing.T) { in string out interface{} requiresMacroCalls bool + unparserOptions []UnparserOption }{ {name: "call_add", in: `a + b - c`}, {name: "call_and", in: `a && b && c && d && e`}, @@ -138,6 +141,281 @@ func TestUnparse(t *testing.T) { in: `[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)`, requiresMacroCalls: true, }, + + // These expressions will not be wrapped because they haven't met the + // conditions required by the provided unparser options + { + name: "call_no_wrap_no_operators", + in: "a + b + c + d", + out: "a + b + c + d", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + }, + }, + { + name: "call_no_wrap_column_limit_large_val", + in: "a + b + c + d", + out: "a + b + c + d", + unparserOptions: []UnparserOption{ + WrapOnColumn(1000), + WrapOnOperators(operators.Add), + }, + }, + { + name: "call_no_wrap_column_limit_equal_length_to_input", + in: "a + b + c + d", + out: "a + b + c + d", + unparserOptions: []UnparserOption{ + WrapOnColumn(13), + WrapOnOperators(operators.Add), + }, + }, + + // These expressions will be formatted based on the unparser options provided + { + name: "call_wrap_add", + in: "a + b - d * e", + out: "a +\nb - d * e", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add), + }, + }, + { + name: "call_wrap_add_subtract", + in: "a * b + c - d * e", + out: "a * b +\nc -\nd * e", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + }, + }, + { + name: "call_wrap_add_subtract", + in: "a * b + c - d * e", + out: "a * b +\nc -\nd * e", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + }, + }, + { + name: "call_wrap_logical_and", + in: "a && b && c && d && e", + out: "a &&\nb &&\nc &&\nd &&\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_logical_and_2", + in: "a && b", + out: "a &&\nb", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_conditional", + in: "a ? b : c ? d : e", + out: "a ?\nb : (c ?\nd : e)", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Conditional), + }, + }, + { + name: "call_wrap_or", + in: "a || b || c || d || e", + out: "a ||\nb ||\nc ||\nd ||\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalOr), + }, + }, + { + name: "call_wrap_equals", + in: "a == b == c == d == e", + out: "a ==\nb ==\nc ==\nd ==\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Equals), + }, + }, + { + name: "call_wrap_greater", + in: "a > b > c > d > e", + out: "a >\nb >\nc >\nd >\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Greater), + }, + }, + { + name: "call_wrap_greater_equals", + in: "a >= b >= c >= d >= e", + out: "a >=\nb >=\nc >=\nd >=\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.GreaterEquals), + }, + }, + { + name: "call_wrap_in", + in: "a in b in c in d in e", + out: "a in\nb in\nc in\nd in\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.In), + }, + }, + { + name: "call_wrap_less", + in: "a < b < c < d < e", + out: "a <\nb <\nc <\nd <\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Less), + }, + }, + { + name: "call_wrap_less_equals", + in: "a <= b <= c <= d <= e", + out: "a <=\nb <=\nc <=\nd <=\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LessEquals), + }, + }, + { + name: "call_wrap_not_equals", + in: "a != b != c != d != e", + out: "a !=\nb !=\nc !=\nd !=\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.NotEquals), + }, + }, + { + name: "call_wrap_divide", + in: "a / b / c / d / e", + out: "a /\nb /\nc /\nd /\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Divide), + }, + }, + { + name: "call_wrap_modulo", + in: "a % b % c % d % e", + out: "a %\nb %\nc %\nd %\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Modulo), + }, + }, + { + name: "call_wrap_multiply", + in: "a * b * c * d * e", + out: "a *\nb *\nc *\nd *\ne", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Multiply), + }, + }, + { + name: "call_wrap_logical_and_long_variables", + in: "longVariableA && longVariableB && longVariableC", + out: "longVariableA &&\nlongVariableB &&\nlongVariableC", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "comp_chained_wrap_comparisons", + in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", + out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", + requiresMacroCalls: true, + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.GreaterEquals, operators.LessEquals), + }, + }, + { + name: "call_wrap_before_add", + in: "a + b - d * e", + out: "a\n+ b - d * e", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_before_add_subtract", + in: "a * b + c - d * e", + out: "a * b\n+ c\n- d * e", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_logical_and_long_variables", + in: "longVariableA && longVariableB && longVariableC", + out: "longVariableA\n&& longVariableB\n&& longVariableC", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_logical_and_long_input", + in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, + out: `"my-principal-group" in request.auth.claims &&` + "\n" + `request.auth.claims.iat > now - duration("5m")`, + unparserOptions: []UnparserOption{ + WrapOnColumn(40), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_before_logical_and_long_input", + in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, + out: `"my-principal-group" in request.auth.claims` + "\n" + `&& request.auth.claims.iat > now - duration("5m")`, + unparserOptions: []UnparserOption{ + WrapOnColumn(40), + WrapOnOperators(operators.LogicalAnd), + WrapAfterColumnLimit(false), + }, + }, + { + // By default: + // - Column limit is at 80 + // - && and || are wrapped + // - Wrapping occurs after the symbol + name: "call_wrap_default", + in: `jwt.extra_claims.filter(c, c.startsWith("group")).all(c, jwt.extra_claims[c].all(g, g.endsWith("@acme.co"))) && jwt.extra_claims.exists(c, c.startsWith("group")) || request.auth.claims.group == "admin" || request.auth.principal == "user:me@acme.co"`, + out: `jwt.extra_claims.filter(c, c.startsWith("group")).all(c, jwt.extra_claims[c].all(g, g.endsWith("@acme.co"))) &&` + + "\n" + + `jwt.extra_claims.exists(c, c.startsWith("group")) || request.auth.claims.group == "admin" ||` + + "\n" + + `request.auth.principal == "user:me@acme.co"`, + requiresMacroCalls: true, + }, + { + // && and || are wrapped by default if only the column limit is specified + name: "call_wrap_default_operators", + in: "longVariableA && longVariableB || longVariableC + longVariableD - longVariableE", + out: "longVariableA &&\nlongVariableB ||\nlongVariableC + longVariableD - longVariableE", + unparserOptions: []UnparserOption{ + WrapOnColumn(3), + }, + }, } for _, tst := range tests { @@ -154,7 +432,8 @@ func TestUnparse(t *testing.T) { if len(iss.GetErrors()) > 0 { t.Fatalf("parser.Parse(%s) failed: %v", tc.in, iss.ToDisplayString()) } - out, err := Unparse(p.GetExpr(), p.GetSourceInfo()) + out, err := Unparse(p.GetExpr(), p.GetSourceInfo(), tc.unparserOptions...) + if err != nil { t.Fatalf("Unparse(%s) failed: %v", tc.in, err) } @@ -179,10 +458,18 @@ func TestUnparse(t *testing.T) { } func TestUnparseErrors(t *testing.T) { + validConstantExpression := &exprpb.Expr{ + ExprKind: &exprpb.Expr_ConstExpr{ + ConstExpr: &exprpb.Constant{ + ConstantKind: &exprpb.Constant_NullValue{}, + }, + }, + } tests := []struct { - name string - in *exprpb.Expr - err error + name string + in *exprpb.Expr + err error + unparserOptions []UnparserOption }{ {name: "empty_expr", in: &exprpb.Expr{}, err: errors.New("unsupported expression")}, { @@ -245,12 +532,44 @@ func TestUnparseErrors(t *testing.T) { }, err: errors.New("unsupported expression"), }, + { + name: "bad_unparser_option_wrap_column_zero", + in: validConstantExpression, + err: errors.New("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got 0 instead"), + unparserOptions: []UnparserOption{ + WrapOnColumn(0), + }, + }, + { + name: "bad_unparser_option_wrap_column_negative", + in: validConstantExpression, + err: errors.New("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got -1 instead"), + unparserOptions: []UnparserOption{ + WrapOnColumn(-1), + }, + }, + { + name: "bad_unparser_option_unsupported_operator", + in: validConstantExpression, + err: errors.New("Invalid unparser option. Unsupported operator: bogus"), + unparserOptions: []UnparserOption{ + WrapOnOperators("bogus"), + }, + }, + { + name: "bad_unparser_option_unary_operator", + in: validConstantExpression, + err: errors.New("Invalid unparser option. Unary operators are unsupported: " + operators.Negate), + unparserOptions: []UnparserOption{ + WrapOnOperators(operators.Negate), + }, + }, } for _, tst := range tests { tc := tst t.Run(tc.name, func(t *testing.T) { - out, err := Unparse(tc.in, &exprpb.SourceInfo{}) + out, err := Unparse(tc.in, &exprpb.SourceInfo{}, tc.unparserOptions...) if err == nil { t.Fatalf("Unparse(%v) got %v, wanted error %v", tc.in, out, tc.err) }