Is predicates invocation too magic ? about django-rules HOT 10 CLOSED

If you anticipate your predicate to be used with less arguments than required (and you should), then you should handle the case accordingly inside the predicate. What could the value be for the second argument of your predicate if the rule is tested with one argument, if not None? If the condition your predicate tests cannot be tested with less arguments, then check for that and return True or False as appropriate. It all depends on what conditions predicates test, on how you break down the problem you're trying to solve.

Predicates should generally be simple one-liners that are then ANDed, ORed, etc, with other predicates to check more complex conditions. You can mix predicates with different number of arguments each, it's ok, each one will receive the appropriate arguments, depending both on how many arguments it accepts and how many arguments were given in the test_rule call -- whatever is less.

A simple trick I'm often using is to AND my predicate with another that simply checks for a condition that my predicate likes to take for granted. This allows you to keep predicates simple and reusable. For example, assume that you need to check whether a user can send an invite. Assume your predicate expects the first argument to be a proper User instance, and that this instance is properly associated with a Profile instance. Here's how I'd write my rule:

# helper predicate
@predicate
def has_profile(obj):
  return obj and hasattr(obj, 'get_profile') and obj.get_profile() is not None

# condition predicate
@predicate
def has_invites_left(obj):
  return obj.get_profile().number_of_invites_left > 0

add_rule('can_invite', has_profile & has_invites_left)

In this case, if has_profile returns False, has_invites_left will not be called -- i.e. normal boolean short-circuiting behaviour. You can similarly apply this to check for None arguments etc.

If I'm not helping you, write up!

from django-rules.

I think that the issue is more about that e.g. an exception should be raised, if a call to a predicate does not provide all expected (keyword) arguments.

if I defined my predicate to accept only two positional arguments and the caller gives only one argument, my predicate will be called with None for the second argument

from django-rules.

Limiting the way test_rule is called would limit the potential to combine predicates, since you'd only be able to combine predicates that accept the same number of arguments.

The thing I tried to allude to with my previous comment was that testing a rule with fewer arguments (or simply passing None for any of them) is fine and should be anticipated. The predicate (or a combination of predicates) is the best place to handle this case.

When testing a rule, you are essentially offloading work. Why check for "obj is not None" before calling test_rule? What if you test the same rule in 13 different places in your code? Handling the case centrally with a predicate is the best option in my opinion. You may even choose to add a predicate that raises an exception for being given less arguments if you like ;)

from django-rules.

Hi, @dfunckt thank you for your answer.
I find your approach very interesting and I will definitly use it.
@blueyed understood correctly what I implied with my question. I think raising an exception would be more expected. At least this is what I felt when I started to play with django-rules.

Let me introduce you how I see the predicates handling:
First we should honour predicate signature, if there is 2 positional argument expected then we can not call the predicate if we have only one argument under the hood. This is provided out of the box by python
We should not try to be smarter than this (*args, **kwargs).

In order to achieve this, we could register as many as predicates we need under the same name.
As an eaxmple speaks better than words, here you go.

@rules.predicate
def are_equal(a, b):
  return a == b

@are_equal.derivate  # the predicate decorate another predicate. I know the name `derivate` sucks :)
def is_equal(a):
  return a == 'a'

add_rule('can_proceed', are_equal)

assert test_rule('can_proceed', 'a', 'a')  # are_equal predicate is called
assert test_rule('can_proceed', 'a') #  is_equal predicate is called
test_rule('can_proceed')  # KeyError, TypeError, PredicateNotFoundError, ... we can decide later which one

In this example I do not extend the complexity of adding rule for every combination of arguments (there is only one call of add_rule()).
And still we have a fair guarantee that the predicates will behave as expected. As if there was no 'rulesets' in between.

That said, I didn't started to develop a POC, thus I do not know yet if it is really feasible. But at least it shows how it will be handled by the user.

How does it sounds ?

from django-rules.

Hi Nicolas, here's how you can do this now:

@predicate
def are_equal(a, b):
  return a == b

@predicate
def is_equal(a):
  return a == 'a'

@predicate
def equal(*args):
  if len(args) == 1:
    return is_equal(*args)
  elif len(args) == 2:
    return are_equal(*args)
  else:
    return False  # or raise TypeError()

add_rule('can_proceed', equal)

I agree that it can be useful to factor out equal or similar functions as helpers that come with rules to do these kind of things. I can't think of other use cases though, nor what the API should be.

I'm sure I don't quite like predicates decorating each other though (i.e. @are_equal.derive). I'd prefer a separate function that takes predicates as its arguments, does its thing and returns a new predicate, similar to what is_group_member does -- it's more explicit and allows the predicates to be used in other rules too.

I'm also not convinced that changing test_rule to enforce the correct number of arguments is a good choice -- certainly it shouldn't be the default, but I wouldn't mind a helper that does this should you like to do this.

test_rule should be, in my opinion, totally ignorant of what the underlying predicates need to do their work, so that it can be used from anywhere with ease (imagine testing a rule in a Django template). This kind of logic should be placed inside predicates, or handled by composing a bunch of them.

But, like I said, we could improve rules as you suggest to allow more complex compositions of predicates. I'd also love to see any other use cases. Any ideas? Anyone?

Thanks!

from django-rules.

@dfunckt If I register a predicate with (*args) as a signature, then Predicate.num_args == 0.
So your example is not yet supported.

You are right for predicate that decorate other predicates, it is not the most elegant solution. What you suggest is better.

It test_rule should be ignorant of what underlying predicates need, to do their work, the best is to give the arguments as they come. You capture them with (*args, **kw) and you pass them as they are.
We can reduce them safely if the predicate accept 0 or 1 argument, but extending them is more complex.
What we could do, instead of defaulting to None, is to use a marker object. A singleton instance that tells the predicate he can ignore this argument.

from rules import NOT_SET

@rules.predicate
def are_equal(a, b):
  if b is NOT_SET:  # Here I'm sure that the caller didn't pass the b argument
    return a == 'a'
  else:
    return a == b

from django-rules.

The reason Predicate.test is defined as test(obj=None, target=None) is to limit the scope of rules and enforce a consistent API between predicates and callers.

Your idea about the marker is however interesting, and I can see it can be useful to be able to distinguish between None and "Not Given". I guess it's our final option if we rule out raising exceptions for testing with less arguments. Also, this change is backwards incompatible, but I guess we can allow ourselves this flexibility since the project is still so new.

I don't really mind changing Predicate.test and test_rule to allow passing arbitrary arguments. The only problem I see is that it would limit our ability to extend the API (for example, I think it would be useful to start passing predicates a context dict that can be used to keep state between predicates during a single test_rule invocation). A solution would be to extend test_rule to accept _args, but not *_kwargs.

Regarding the example, you're right, the example should be like this:

@predicate
def equal(a=None, b=None):
  if target is None:
    return is_equal(a)
  else:
    return are_equal(a, b)

from django-rules.

You are right, exclusive usage of (*args, **kwargs) will prevent us to extend predicate API because there might be conflicts. so let's forget about it for now.

Regarding NOT_GIVEN marker topic.
We might break current implementations if people starts to wrote in their predicate if target is not None:.
But like you said:

the project is still so new

so let's do it ! 😈
I can submit a PR if there is concensus from the involved paticipants.

from django-rules.

Sure, please go ahead. Let's have a PR and we'll take it from there. Thanks!

from django-rules.

This issue is now resolved by merging #15

from django-rules.