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

Invalid variables in left hand of JEXL or expression are not evaluated #7449

Closed
jaredlockhart opened this issue Jun 23, 2022 · 0 comments
Closed
Assignees

Comments

@jaredlockhart
Copy link
Collaborator

jaredlockhart commented Jun 23, 2022

While working on #7440 I noticed that the left hand operands of an || expression in JEXL are not evaluated in either the Rust or JS implementations, see below:

In Rust:

(Pdb) targetting_helper.eval_jexl('asdf || true')
True
(Pdb) targetting_helper.eval_jexl('asdf || false')
False
(Pdb) targetting_helper.eval_jexl('true || asdf')
True
(Pdb) targetting_helper.eval_jexl('false || asdf')
*** nimbus_rust.NimbusError.EvaluationError: EvaluationError: Identifier 'asdf' is undefined

In JS:

await targetingContext.evalWithDefault('asdf || true');
true
await targetingContext.evalWithDefault('asdf || false');
false
await targetingContext.evalWithDefault('true || asdf');
true
await targetingContext.evalWithDefault('false || asdf');
null

This means that if someone includes an invalid variable in their JEXL expression it will not be caught by the integration tests. In theory we could make changes to both the JS and Rust implementations of the JEXL evaluator to change this behaviour, but in the mean time we can account for this by tokenizing the expression and evaluating them each individually in addition to the entire expression.

Let's update both the desktop and sdk integration tests to account for this.

┆Issue is synchronized with this Jira Task

jaredlockhart added a commit that referenced this issue Jun 27, 2022
Because

* We recently learned that JEXL will not evaluate the left hand side of an || expression
* When we validate JEXL expressions in our integration tests we want to know if any part of the expression is invalid

This commit

* Parses all JEXL expressions in the integration tests and collects all their subexpressions
* Evaluates every subexpression in the integration tests in desktop Firefox and the mobile SDK
jaredlockhart added a commit that referenced this issue Jun 27, 2022
Because

* We recently learned that JEXL will not evaluate the left hand side of an || expression
* When we validate JEXL expressions in our integration tests we want to know if any part of the expression is invalid

This commit

* Parses all JEXL expressions in the integration tests and collects all their subexpressions
* Evaluates every subexpression in the integration tests in desktop Firefox and the mobile SDK
jaredlockhart added a commit that referenced this issue Jun 27, 2022
Because

* We recently learned that JEXL will not evaluate the left hand side of an || expression
* When we validate JEXL expressions in our integration tests we want to know if any part of the expression is invalid

This commit

* Parses all JEXL expressions in the integration tests and collects all their subexpressions
* Evaluates every subexpression in the integration tests in desktop Firefox and the mobile SDK
jaredlockhart added a commit that referenced this issue Jun 28, 2022
Because

* We recently learned that JEXL will not evaluate the left hand side of an || expression
* When we validate JEXL expressions in our integration tests we want to know if any part of the expression is invalid

This commit

* Parses all JEXL expressions in the integration tests and collects all their subexpressions
* Evaluates every subexpression in the integration tests in the mobile SDK
* Desktop support requires changes to the JEXL runtime and will have to happen in a followup
jaredlockhart added a commit that referenced this issue Jun 29, 2022
Because

* We recently learned that JEXL will not evaluate the left hand side of an || expression
* When we validate JEXL expressions in our integration tests we want to know if any part of the expression is invalid

This commit

* Parses all JEXL expressions in the integration tests and collects all their subexpressions
* Evaluates every subexpression in the integration tests in the mobile SDK
* Desktop support requires changes to the JEXL runtime and will have to happen in a followup
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

No branches or pull requests

1 participant