Using `AND` (`&`) with a predicate that returns `None` incorrectly returns `True`

[sjdemartini]

When using & to combine predicates, if one of the predicates returns None (a falsey value), the overall predicate will mistakenly/confusingly return True. This is not consistent with running the predicate by itself or ORed with other predicates.

This behavior can be demonstrated with the following test added to test_predicates.py—the final assertion fails:

    def test_returns_none(self):
        @predicate
        def returns_none(arg1, arg2):
            return None

        # Just the predicate (works)
        assert not returns_none(2, 3)

        # OR the predicate with itself (works)
        p_OR = returns_none | returns_none
        assert not p_OR()

        # AND the predicate with a truthy-predicate (FAILS)
        p_AND = always_true & returns_none
        assert not p_AND()

I noticed this in production in my app, where my logic in one predicate was doing return obj and obj.boolean_field, expecting that if obj were None, it'd still be treated as False in all contexts. It took me a while to figure out what was going on. I didn't feel sure about where/how to fix this in django-rules, but hope the above example will help to resolve quickly if possible. Thanks in advance (and thanks for building this great library)!

[sjdemartini]

Ah, I see you have this note in the README, which I apparently missed:

You may skip evaluation by returning None from your predicate

So I guess this is by design! Conditionals like the one I described above return obj and obj.boolean_field are dangerous then (though this wasn't known/obvious to me), since they're falsey on their own but skipped in combined predicates. (And None is falsey in other Python contexts, of course, so I wouldn't have assumed this behavior.) For instance, setting the following will result in "read" returning "no permission" if the sometimes_returns_none predicate returns None, but "add" returning "has permission" if the user is staff (even if sometimes_returns_none has returned None):

        rules_permissions = {
            "read": sometimes_returns_none,
            "add": rules.is_staff & sometimes_returns_none,
        }

I'll leave this open for now for any discussion. Thanks again.

[sjdemartini]

It may be worth at least moving "Skipping predicates" up to the "Combining predicates" portion of the doc so it's more discoverable, since it's important in that context (rather than it being under "Advanced features").

[dfunckt]

I think a truth table for all possible combinations of operations and return values close to the "Combining predicates" section would be very useful.

[sjdemartini]

Yeah, that could be useful. I also noticed in the "Upgrading from 1.x" section of the docs, it mentions that skipping used to be done with raising a SkipPredicate exception. That more explicit approach seems significantly more preferable/safer to me, particularly for a security-oriented library, where a mistake in allowing access can be rather severe. (And then all truthy/falsey conditions can behave more in line with what's typical in python.)

[dfunckt]

I agree it's more "Pythonic" to raise an exception and I have unfortunately no recollection why I made the switch to None. I don't think it's sensible to revert back to the old behaviour as it is a breaking change however I'm willing to be convinced otherwise if others feel the change makes sense.