feat(api): added full trigger validation in api #892
Conversation
- included tests and benchmarks
- optimizing get calls can make vm.Program return wrong values because variables have been replaced before caching, and new set `WARN_VALUE`, `t1, t2, ...` are not evaluated properly. - If validation of variables is not necessary, this commit can be reverted for speed purpose
lordvidex
changed the title
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #892 +/- ##
==========================================
+ Coverage 68.28% 69.76% +1.48%
==========================================
Files 187 187
Lines 10453 10439 -14
==========================================
+ Hits 7138 7283 +145
+ Misses 2906 2736 -170
- Partials 409 420 +11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -26,19 +31,52 @@ func BenchmarkDefault2Expr(b *testing.B) { | |||
| MainTargetValue: 10.0, | |||
| WarnValue: &warnValue, | |||
| ErrorValue: &errorValue, | |||
| TriggerType: moira.RisingTrigger, | |||
There was a problem hiding this comment.
hmmm, why it becomes necessary?
There was a problem hiding this comment.
I debugged the benchmark and it was actually returning error "trigger_type is not set", therefore the tests were not really testing the speed of expression parsing.
There was a problem hiding this comment.
I think this issue can be resolved already. @kissken ?
This is necessary because govalute is used to verify expression evaluation and `expr.Compile` is enough to validate expression.
includes benchmarks
|
/build |
|
Build and push Docker images with tag: feature-expression-validation.2023-08-14.c302f68 |
1 similar comment
|
Build and push Docker images with tag: feature-expression-validation.2023-08-14.c302f68 |
Tetrergeru
changed the base branch from
master
to
feature/expression-validation
PR Summary
Trigger is fully validated using
exprwhen trigger is created or updated.Problem solved
It was possible for
govaluteto skip validations for expressions that do not fall into the current conditional branch based on the target values.For example,
t1 > 10 ? ok : (t2 * 10 : error ? warn)would return result without error wheret1 > 10without considering the syntax error (misplacement of ternary operators) in the expression.Notes
Commit 7bb6f5a removes expression optimization because the
vm.Programin expr caches the result of Get() function which may produce wrong result inexpr.Runfunction. Check benchmark and commit description for more.Possible problems
expris not fully compatible withgovalute[a, b, c,...]while govalute uses(a,b,c,...)for array literalsBecause of the following errors,
exprmight mark an expression as valid wheregovalutewill return errors and vice versa.Relations
Augments #881