JSON conditions for conditional policy #814
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
|
||
| function _M.evaluate(expression) | ||
| local match_attr = ngx.re.match(expression, [[^([\w]+)$]], 'oj') | ||
| function _M.evaluate(condition, context) |
There was a problem hiding this comment.
cyclomatic complexity of function '_M.evaluate' is too high (8 > 7)
| local res = Operation.new('1', 'plain', '!=', '2', 'plain'):evaluate({}) | ||
| assert.is_true(res) | ||
|
|
||
| local res = Operation.new('1', 'plain', '!=', '1', 'plain'):evaluate({}) |
There was a problem hiding this comment.
variable 'res' was previously defined on line 23
| local res = Operation.new('1', 'plain', '==', '1', 'plain'):evaluate({}) | ||
| assert.is_true(res) | ||
|
|
||
| local res = Operation.new('1', 'plain', '==', '2', 'plain'):evaluate({}) |
There was a problem hiding this comment.
variable 'res' was previously defined on line 15
| local res = Operation.new('1', nil, '==', '1', nil):evaluate({}) | ||
| assert.is_true(res) | ||
|
|
||
| local res = Operation.new('1', nil, '==', '2', nil):evaluate({}) |
There was a problem hiding this comment.
variable 'res' was previously defined on line 31
|
|
||
| assert.is_true(res) | ||
|
|
||
| local res = Operation.new( |
There was a problem hiding this comment.
variable 'res' was previously defined on line 41
davidor
force-pushed
the
json-conditions
branch
3 times, most recently
from
d445a71
to
6242062
Compare
Start with something simple. We will change if use cases that require more complex operations appear.
davidor
changed the title
davidor
force-pushed
the
json-conditions
branch
2 times, most recently
from
27b0d1e
to
8ec6e53
Compare
| function _M:evaluate(context) | ||
| if #self.operations == 0 then return true end | ||
|
|
||
| if self.combine_op == 'and' then |
There was a problem hiding this comment.
This can be done at initialization time, right?
It would allow for better JIT if there would be no branching in this method.
| local left_value = self.templated_left:render(context) | ||
| local right_value = self.templated_right:render(context) | ||
|
|
||
| if self.op == '==' then |
There was a problem hiding this comment.
I think we can have a dispatch table for operations like:
local operations = {
['=='] = function(left, right) return left == right end
}And assign the comparator function during initialization for better JIT.
edit: that would allow us to get rid of the supported_ops variable
There was a problem hiding this comment.
Fixed the same way as the one above 👍
| local context = { var_1 = '1', var_2 = '2' } | ||
|
|
||
| local res_true = Operation.new( | ||
| '{{ var_1 }}', 'liquid', '==', '1', 'plain' |
There was a problem hiding this comment.
Are we going to handle types somehow? What if the variable comes from nginx and is actually a number ?
There was a problem hiding this comment.
In that example, even if var_1 was a number, the comparison would end up being between 2 strings, because LiquidTemplateString:render() returns a string.
I included a test that checks this. Also, I added tostring() in the == and != operations in Operation to make this explicit.
There was a problem hiding this comment.
I think this is great for now 👍 🥇
There might be some details to clarify and improve, but that can be done later during some performance optimization phase.
The only real concern I have is regarding types. If everything is string based we might need to call tostring on the values coming from nginx like ports.
This new module the Operation module introduced in the previous commits.
Closes #744
This PR defines the conditions supported in the conditional policy. It also includes an engine that evaluates them.
The kinds of conditions supported are very basic (
==,!=,and,or) and they can be expressed in JSON.The alternative was to define a custom DSL. I implemented a prototype in this branch: https://github.com/3scale/apicast/tree/lpeg-grammar-condition-engine . This approach would allow us to express more complex conditions. However, the implementation is more complicated, and it would tie us to lpeg. The solution implemented in this PR should be enough for our current needs so it looks like a better option.
For now this only support
==and!=. We can add more operations likestarts_withormatch_exactly, later. I'd rather agree on the approach and the basics first.