Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResidualAst mutates input *Ast #384

Closed
1 of 3 tasks
tmthrgd opened this issue Aug 17, 2020 · 1 comment · Fixed by #387
Closed
1 of 3 tasks

ResidualAst mutates input *Ast #384

tmthrgd opened this issue Aug 17, 2020 · 1 comment · Fixed by #387
Assignees
Labels
bug Something isn't working P1

Comments

@tmthrgd
Copy link

tmthrgd commented Aug 17, 2020

Describe the bug
ResidualAst mutates the input *Ast which causes subsequent evaluations to fail or return incorrect results.

To Reproduce
Check which components this affects:

  • parser
  • checker
  • interpreter

Sample expression and input that reproduces the issue:

Test setup:

func Test_ResidualAst_Modified(t *testing.T) {
	e, _ := cel.NewEnv(
		cel.Declarations(
			decls.NewVar("x", decls.NewMapType(decls.String, decls.Int)),
			decls.NewVar("y", decls.Int),
		),
	)
	ast, _ := e.Parse("x == y")
	prg, _ := e.Program(ast,
		cel.EvalOptions(cel.OptTrackState, cel.OptPartialEval),
	)
	for _, x := range []int{123, 456} {
		vars, _ := cel.PartialVars(map[string]interface{}{
			"x": x,
		}, cel.AttributePattern("y"))
		out, det, err := prg.Eval(vars)
		if !types.IsUnknown(out) {
			t.Fatalf("got %v, expected unknown", out)
		}
		if err != nil {
			t.Fatal(err)
		}
		residual, err := e.ResidualAst(ast, det)
		if err != nil {
			t.Fatal(err)
		}
		orig, err := cel.AstToString(ast)
		if err != nil {
			t.Fatal(err)
		}
		if orig != "x == y" {
			t.Errorf("parsed ast: got expr: %s, wanted x == y", orig)
		}
		expr, err := cel.AstToString(residual)
		if err != nil {
			t.Fatal(err)
		}
		want := fmt.Sprintf("%d == y", x)
		if expr != want {
			t.Errorf("residual ast: got expr: %s, wanted %s", expr, want)
		}
	}
}
=== RUN   Test_ResidualAst_Modified
    filter_test.go:83: parsed ast: got expr: 123 == y, wanted x == y
    filter_test.go:83: parsed ast: got expr: 123 == y, wanted x == y
    filter_test.go:91: residual ast: got expr: 123 == y, wanted 456 == y
--- FAIL: Test_ResidualAst_Modified (0.00s)

Expected behavior
ResidualAst should not be mutating it's input *Ast and should instead return a copy.

Additional context
Either ResidualAst or interpreter.PruneAst should be making a copy of the expr that can then be safely mutated.

@tmthrgd
Copy link
Author

tmthrgd commented Aug 17, 2020

Actually the problem is in prune. All the lines like:

newExpr.GetExprKind().(*exprpb.Expr_SelectExpr).SelectExpr = ...

are mutating a pointer rather than allocating a new struct. These need to be changed to something like:

newExpr.ExprKind = &exprpb.Expr_SelectExpr{...}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants