zeroSteiner / rule-engine

A lightweight, optionally typed expression language with a custom grammar for matching arbitrary Python objects.
https://zerosteiner.github.io/rule-engine/
BSD 3-Clause "New" or "Revised" License
445 stars 54 forks source link

No match when using default_value for nested objects #10

Closed ishfx closed 3 years ago

ishfx commented 3 years ago

Hello,

I was playing with this cool lib when i stumbled upon the following issue.

When i set default_value to None (or any other value) in a rule's context, i don't get any matches for nested objects. Here is a code example :

#! /usr/bin/env python3 

import rule_engine

dic = {
    "a": 1,
    "b": "test1",
    "c": {
        "c1": 2,
        "c2": "test2"
    }
}

rule_text = 'a == 1'

rule1 = rule_engine.Rule(rule_text, context=rule_engine.Context())
rule2 = rule_engine.Rule(rule_text, context=rule_engine.Context(default_value=None))

a_match1 = rule1.matches(dic)
a_match2 = rule2.matches(dic)

assert a_match1 == a_match2

rule_text = 'c.c1 == 2'

rule1 = rule_engine.Rule(rule_text, context=rule_engine.Context())
rule2 = rule_engine.Rule(rule_text, context=rule_engine.Context(default_value=None))

c_match1 = rule1.matches(dic)
c_match2 = rule2.matches(dic)

assert c_match1 == c_match2

Only c_match assertion fails.

I think what happens here is c.c1 is replaced with None. The docs says that default_value is used for missing attributes only. c.c1 is not missing therefore it shouldn't be replaced with None (or any set default_value).

Great lib by the way.

Thanks in advance!

ishfx commented 3 years ago

I looked into the code and found that the error comes from here : https://github.com/zeroSteiner/rule-engine/blob/master/lib/rule_engine/ast.py#L933-L944

When default_value is not set :

When default_value is set :

zeroSteiner commented 3 years ago

Thanks for all the details, I'll take a look.

ishfx commented 3 years ago

fyi, i went with a quick one line "dirty" fix.

try:
    value = self.context.resolve_attribute(thing, resolved_obj, self.name)
except errors.AttributeResolutionError:
    pass
else:
    if (value is not None): # quick fix
        return coerce_value(value, verify_type=False)

try:
    value = self.context.resolve(resolved_obj, self.name)
except errors.SymbolResolutionError:
    raise errors.AttributeResolutionError(self.name, resolved_obj, thing=thing) from None
return coerce_value(value, verify_type=False)
zeroSteiner commented 3 years ago

I really appreciate all the detail you put into this report. I was able to reproduce the error and confirm it's due to a change between v2.1 and now. I changed the precedence order for attributes and symbols when I added the mapping data type. I suspect this issue is related to that.

zeroSteiner commented 3 years ago

Should be fixed in commit 2754422dcbffa2c83f7fa67ac60cd052519a64f2. I made it the caller's responsibility to determine whether or not the context's default value should be used. This seemed like the easiest and most accurate way to avoid a possible scenario where the legitimate value is the same as the default value and the resolution process carried on because it couldn't tell the difference.

I'm going to close this out since I'm confident I've addressed the issue. If you want to give it a test, that'd be great. I'll plan on publishing the new version to include the patch in a few days.

ishfx commented 3 years ago

Looks good to me, thank you for the quick fix!

zeroSteiner commented 3 years ago

This bug fix has just been published in version v2.4.1.