Improve rulesIf evaluation#1876
Open
ticapix wants to merge 5 commits into
Open
Conversation
…ect \${VAR}
- Replace regex-based string manipulation in evaluateRuleIf with jsep AST
parsing for correct operator precedence and cleaner expression handling
- Fix: null !~ /pattern/ now returns true (undefined variable does not match,
so negated check passes) instead of always returning false
- Fix: reject \${VAR} curly-bracket syntax in the new evaluator, matching
the guard already present in the legacy _evaluateRuleIf path
- Assert that the RHS of =~/!~ is a regex pattern (not a plain quoted string), matching the guard already present in _evaluateRuleIf - Wrap jsep(evalStr) in try-catch so parse failures (e.g. invalid regex flags like /pattern/ur/) surface as "Error attempting to evaluate the following rules:" instead of a raw jsep exception
- Remove evalSpy and jsExpression assertions — these were white-box checks of an internal implementation detail, not behaviour - Compact test data accordingly - Fix /Hello (?i)world/ expectation: (?i) is not valid JS regex syntax so the new jsep path correctly rejects it; update to expectedErrSubStr
- Delete _evaluateRuleIf: its regex-substitution logic (pattern1/pattern2, null.matchRE2JS replacements, re-expansion) was entirely unreachable when called from the jsep walk, which handles =~/!~/&&/|| before falling through - Inline a direct eval in the walk fallback (for ==, !=, comparisons, bare literals), replacing the _evaluateRuleIf call - Drop unused envs parameter from _nodeToAtom and remove commented-out block
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Replaces the silent `return ""` default with an explicit error, making unexpected AST node types fail loudly instead of producing a wrong result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello,
This is a proposal to improve the robustness of the rules evaluations.
This address #1859
The main changes are:
&&,||,(and)operators./Hello (?i)world/as invalid regex, even if RE2JS supports it. (regex101.com, regexr.com are considering it as invalid)eval()only to evaluate the full rule, we can't spy on it. Removing the expected evaluation string. Keep the final expected result verification.Summary by cubic
Make rules:if evaluation AST-based with
jsep+@jsep-plugin/regexfor correct precedence, safer regex handling, clearer errors, and fast failures on unsupported AST nodes. Keeps RE2JS matching and$VARexpansion, and fixes edge cases in complex rules.Bug Fixes
${VAR}syntax./!to be a regex; reject quoted strings and invalid flags with helpful errors._nodeToAtomto avoid silent mis-evaluation.$VARexpansion behavior./Hello (?i)world/as invalid.Dependencies
jsepand@jsep-plugin/regex.Written for commit 919bb83. Summary will update on new commits.