Predicate call context (WIP)

[dfunckt]

Related to #6, #7, #9.

Adds the ability to specify that a predicate wants a dict containing information about the current test_rule() invocation.

>>> @rules.predicate(takes_context=True)
... def mypred(a, b, **kwargs):
...    print(kwargs['context'])
...    return True
...
>>> rules.add_rule('myrule', mypred)
>>> rules.test_rule('myrule', 1, 2)
{'args': (1, 2), 'kwargs': {}}
True

Predicates that want context must accept **kwargs.

A nice advantage of this PR is that it allows predicates to store arbitrary data that other predicates may later use. It is possible for example, to store an expensive computation done in one predicate (maybe fetch a bunch of objects from db, related to the given user/object) and another predicate access it later on.

This is backwards-compatible -- at least all tests pass.

Here are some things that need to be done in order for this PR to be merged:

• Decide on the API -- pretty close already, I guess.
• Write docs -- add a new section in README for now.
• Find any glaring bugs.
• Some more tests maybe.

All feedback welcome.

[dfunckt]

@ticosax, want to have a look?

[ticosax]

if context is already in kwargs ?
I think we should raise something.
Just an idea: Maybe we can accept an argument that will define de name for context. then the predicate will find it without being forced to use context if already taken.

[dfunckt]

I could make takes_context to accept either a boolean or a string for the key under which it wants it to be available.

There is also the option of using mypred.context, the property defined just above (line 67) -- this way there would be no need for **kwargs and the 'context' key.

[ticosax]

mypred.context provide a better separation of concerns. I like it better.

[dfunckt]

I like it better too. It's clearer too -- no need to change the predicate's signature to accept new arguments. Need to add tests to make sure it actually works this way. Will do later today.

[dfunckt]

apply() turned out to be really ugly. I think I'm going to ditch opening up predicates to accept arbitrary args.

[ticosax]

That's a trade-off which make sense in the context of a django application.
If you consider using django-rules outside django, then it might become too restricted.
As I'm writing, I do not have such use-case. So it worth the try and reach simplicity.
Otherwise, good work !

[dfunckt]

I have rewritten the PR. permissions.has_perm() and rulesets.test_rule() now merely forward args and kwargs to Predicate.test(). test is still accepting only obj and target to somehow have named arguments so that rules.test_rule(obj=user) continues to work. Their default values are a marker instance though instead of None, similar to #7, so that we can deduce how many arguments test was called with.

Context is a proper dict subclass providing a single instance attribute: args which is a tuple of the arguments initially provided to Predicate.test(). Predicates can access this context via mypred.context and decide what to do based on what arguments are given (fixes #6). Predicates can also kind-of communicate by writing to and reading from the context dict. Contexts are pushed/popped in a thread-safe manner -- presumably, need to add tests.

Predicate.test() is the proper way to invoke a predicate -- it takes care of creating a new invocation context with any given arguments and then call the underlying callable. Direct invocation (i.e. __call__()) can be used however to bypass the creation of context. If the predicate uses the context though, it must be prepared to handle the case that it is None or totally unrelated to its own invocation (that is because a new context is not pushed onto the stack via __call__). To avoid this, the best advice is not to create predicates that are used both as predicates and normal functions.

I'll leave the PR open for a few days, so that it can be reviewed by anyone interested and then perhaps add a couple more tests and merge it into master. I won't write any documentation for it just now, mainly because it's hard for me, but I'd also like the new API to stay "semi-private" for the time being in case I need to change it further.

Edit: Forgot to say that a predicate accepting *args will now get passed all arguments, not zero as was the case until now. This is backwards incompatible, but I doubt this mis-feature is used by anyone.

[ticosax]

You can not use an immutable here. Because a legit passed argument might being interpreted as not given.

>>> frozenset() is frozenset()
True
>>> set() is set()
False

[luzfcb]

this is totally out of context, but reminded me of this article:

http://radar.oreilly.com/2014/10/python-tuples-immutable-but-potentially-changing.html

[dfunckt]

Nice catch! Not testing this, did me no good :)

[dfunckt]

Great article @luzfcb.

I tried to play smart and got bitten. I'll create a simple object subclass with a single nonzero method returning False so that we always get both immutability and identity.

[ticosax]

I will test your branch on my project.
But it looks pretty neat.

[dfunckt]

Just pushed a commit that fixes the NO_VALUE issue.

There is one issue currently: defining a predicate with arguments that have default values will result in them becoming None due to line 152. It's too late here to fix this just now, so I'll have a look at it tomorrow.

[dfunckt]

@ticosax did you get to use this patch? Any feedback?

[ticosax]

@dfunckt sorry, I'm always postponing. But I'll force myself to give a feedback this week.
In the mean time, please can you rebase your branch 😄 ?

[dfunckt]

No worries. I’ll rebase the branch later today.

[ticosax]

https://github.com/ticosax/django-rules/compare/unstable/call-context

I tried it, and I feel the need to improve it a bit to serve for my use-case by adding 2 features:

• Binding:
  Excactly like celery is handling it, we can access the predicate instance through self argument by giving bind=True to the decorator
  http://celery.readthedocs.org/en/latest/userguide/tasks.html#context
  The advantage, is that if my predicate has a long name, I do not need to pollute my code with this long names again, just self is enough.
• DiscardPredicate: Do you remember, the problem I wanted to solve initially ? When my Predicate expect two arguments and only one is given, my predicate is irrelevant. So to circumvent this, the predicate can decide to discard itself, then the test() method will return the result like the discarded predicate never existed. A predicate can discard itself by raising DiscardPredicate exception.

Note that the implementation is a bit tricky because of of the INVERT operator.
I can open a new PR to focus the discussion on those two new features if you like.

[dfunckt]

Hi Nicolas, this looks really good to me. I really like both your suggestions re: binding and skipping a predicate. Combined with the ability to store things in context, I think this is a big improvement to rules. I'll merge your changes to this PR and finally merge onto master the soonest possible.

[ticosax]

That's awesome news !
this library is a 🚀 for us.

[dfunckt]

Just merged this PR. Thanks @ticosax -- awesome work!

[ticosax]

Thank you @dfunckt !
Nice trick the self.skip() !