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

Wrong evaluation result #19

Closed
inikolaev opened this issue Sep 17, 2023 · 1 comment · Fixed by #20
Closed

Wrong evaluation result #19

inikolaev opened this issue Sep 17, 2023 · 1 comment · Fixed by #20

Comments

@inikolaev
Copy link
Contributor

I've been playing around with this CEL implementation and I noticed one odd thing with the following expressions:

b && (c == "string")

b && c == "string"

c == "string" && b

Given this context

{"b": True, "c": "string"}

they should all evaluate to true, but this is not what's happening:

True <= b && (c == "string")
False <= b && c == "string"
True <= c == "string" && b

Here's a simple reproducer:

use cel_interpreter::{Context,Program, Value};

fn main() {
    let expressions = [
        "b && (c == \"string\")",
        "b && c == \"string\"",
        "c == \"string\" && b",
    ];

    for expression in expressions {
        let program = Program::compile(expression).unwrap();
        let mut context = Context::default();
        context.add_variable("b", Value::Bool(true));
        context.add_variable("c", Value::String(String::from("string").into()));

        let result = program.execute(&context);

        println!("{:?} <= {}", result, expression)
    }
}

It produces the following output:

Ok(Bool(true)) <= b && (c == "string")
Ok(Bool(false)) <= b && c == "string"
Ok(Bool(true)) <= c == "string" && b

It seems like in the case of b && c == "string" the interpreter effectively evaluates this expression

(b && c) == "string"

I'm also using a Python version of CEL interpreter and it evaluates it properly:

import celpy

expressions = [
    'b && (c == "string")',
    'b && c == "string"',
    'c == "string" && b',
]

for expression in expressions:
    env = celpy.Environment()
    ast = env.compile(expression)
    prgm = env.program(ast)

    activation = celpy.json_to_cel({"a": 1, "b": True, "c": "string"})
    result = prgm.evaluate(activation)
    print(f"{result} <= {expression}")

Produces

True <= b && (c == "string")
True <= b && c == "string"
True <= c == "string" && b
@clarkmcc
Copy link
Owner

clarkmcc commented Sep 18, 2023

Thank you for uncovering this issue! It looks like operator precedence is incorrect, where precedence should be given to == when it is instead given to the &&. I'll need to review our parser and see what I can find. As a side note, I checked the spec and Go implementation and they behave in the same way as your Python implementation confirming the bug in this Rust implementation.

Edit: Okay, I think I've got a fix that resolves the issue and passes all the existing tests. I'll implement additional tests and final cleanup tomorrow.

clarkmcc added a commit that referenced this issue Sep 18, 2023
@clarkmcc clarkmcc linked a pull request Sep 18, 2023 that will close this issue
clarkmcc added a commit that referenced this issue Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants