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

Get rid of eval in rule's "then" clause #19

Open
antisvin opened this issue Apr 23, 2015 · 5 comments
Open

Get rid of eval in rule's "then" clause #19

antisvin opened this issue Apr 23, 2015 · 5 comments
Assignees

Comments

@antisvin
Copy link
Collaborator

With #15 almost ready, it makes sense to limit all rule's clauses to safe operations. If it's sufficient to leave only math operations and reading from variables, this can be done with a simple parser (should take no more than a day to implement and test).

Or we can keep supporting the old approach with eval and add the new safe mode - allowing rule engine to run unsafe mode only in certain cases (i.e. if all rules would be coming from trusted source).

@miraculixx
Copy link
Owner

Interesting proposal indeed! I presume you suggest this for the TableRule's, right?

Let's see. then/target action needs to support

  • direct assignment
  • calculations based on context variables
  • function calls (at some point in the future)

What's your specific proposal how to make each of these safe? Are you thinking like a A(ction) object, e.g. A(var__mult=value|var) would express lambda context : context.var * value or lambda context : context.var * contest.var ?

Or we can keep supporting the old approach with eval and add the new safe mode

I like that. Having options is good.

@miraculixx
Copy link
Owner

I guess we can leave function calls for later, but really it may be quite simple, e.g.

A(funcname__fn=args) => func_name(context, *args)

where funcname is some known function, e.g. imported from a #13 -exported ruleset's .egg file. And args is a tuple passed into the function call in the then clause. E.g. - funcname(var, 5, 3) would create A(funcname__fn=(var, 5, 3)) and eventually call funcname(context, *args). If that makes sense we can do the same for Q objects, too.

What do you think?

@miraculixx
Copy link
Owner

I'm having second thoughts on this. Let say we want to allow function calls like this in then clauses:

then: math.sqrt(context.variable)

how can we do this?

@antisvin
Copy link
Collaborator Author

How about adding a directive that imports modules into context, i.e.

import: math, foobar
if: ...
then: math.sqrt(variable)
...

Btw, I've had an idea that we could make rule syntax a bit simpler if we just use a dict like:

then:
   math.sqrt(foo * 2): bar
   something + 1: another_thing

This way we exclude the situation where number of "then" and "target" clauses is mismatched.

As for implementation, this would be compiled parse trees, something like:

{['call', 'math.sqrt', [['var', 'foo',] '*', 2]]: 'bar',
 [['var', 'something'] + 1]: 'another_thing'}

Evaluating this is straightforward.

Note that even if we don't use the dictionary and keep a separate "target" clause, we'll probably end up with the same approach to parsing. At least I can't understang how to write a solution based on the stuff you've written regarding A-expressions - seems to be to compicated to me.

Using dictionaries like that has one important side effect - we don't know the order in wich then-clauses are executed. We should consider if that is a good thing or not. It may be easier to reason about rule's results if all statements in "then" clause would read from original context and write to a new one, rather than original - then the order of execution shouldn't matter. So "then" clauses would work like a transaction.

Also we could consider changing keys and values in this dict, to force targets to be unique.

then:
   bar: math.sqrt(foo * 2)
   another_thing: something + 1

I'm not advocating a specific approach here, just sharing some ideas :-)

@miraculixx
Copy link
Owner

I'm not advocating a specific approach here, just sharing some ideas :-)

that's a great idea. Never mind my Action object, that was before I knew you were going to use pyparsing...

the order of execution shouldn't matter. So "then" clauses would work like a transaction.

agree.

we could consider changing keys

In terms of syntax, I like the second approach, it gets rid of the need for the target clause and is easy to understand:

then:
   bar: math.sqrt(foo * 2)
   another_thing: something + 1

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

2 participants