Use Null ValueNode instead of nil on allowUndefinedFacts

[juannorris]

Description of the issue

When allowUndefinedFacts is true, left-hand side values are treated as nil instead of ValueNode of Type: Null, which causes the evaluation of a condition to be skipped.

Perhaps this is the desired behavior, but I would expect the following code snippet to work and it currently does not:

func Test() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()
    facts := `{
        "user": {
            "firstName": "John",
            "lastName": "Doe",
            "department": "sales"
        }
    }`

    rules := `[
            {
                "event": {"type": "testEvent", "params": {"result": "doNotSendEmail"}},
                "conditions": {
                    "all": [
                        {"fact": "user.department", "operator": "equal", "value": "sales"},
                        {"fact": "user.email", "operator": "missing", "value": ""}
                    ]
                }
            },
            {
                "event": {"type": "testEvent", "params": {"result": "doNotSendEmail"}},
                "conditions": {
                    "all": [
                        {"fact": "user.department", "operator": "equal", "value": "sales"},
                        {"fact": "user.email", "operator": "missing", "value": ""}
                    ]
                }
            }
        ]`

    ep := &rulesEngine.RuleEngineOptions{
        AllowUndefinedFacts: true,
    }
    engine := rulesEngine.NewEngine(nil, ep)

    if missingOperator, err := rulesEngine.NewOperator("missing", func(a, b *rulesEngine.ValueNode) bool {
        return a.Type == rulesEngine.Null
    }, nil); err == nil {
        engine.AddOperator(*missingOperator, nil)
    }

    // PARSE RULE
    var ruleConfigList []rulesEngine.RuleConfig
    if err := json.Unmarshal([]byte(rules), &ruleConfigList); err != nil {
        panic(err)
    }

    onSuccess := func(result *rulesEngine.RuleResult) interface{} {
        fmt.Println("[client_log] On Success")
        engine.Stop()
        return nil
    }

    onFailure := func(result *rulesEngine.RuleResult) interface{} {
        fmt.Println("[client_log] On Failure")
        return nil
    }

    totalRules := len(ruleConfigList)

    for ruleIndex, ruleConfig := range ruleConfigList {
        priority := totalRules + 1 - ruleIndex
        ruleConfig.Priority = &priority
        ruleConfig.OnSuccess = onSuccess
        ruleConfig.OnFailure = onFailure

        rule, err := rulesEngine.NewRule(&ruleConfig)
        if err == nil {
            engine.AddRule(rule)
        }
    }

    res, err := engine.Run(ctx, []byte(facts))
    if err != nil {
        panic(err)
    }

    var events = res["events"].(*[]rulesEngine.Event)

    if events != nil && len(*events) > 0 {
        firstEvent := (*events)[0]
        fmt.Printf("[client_log] Result: %s %s\n", firstEvent.Type, firstEvent.Params["result"])
    } else {
        fmt.Println("[client_log] No events")
    }
}

• Expected result: output is [client_log] Result: testEvent doNotSendEmail
• Actual result: output is [client_log] No events

[nimbit-software]

@juannorris as far as i can tell this makes sense. im just wondering of there is also a way to elimitate rules from the processing queue based on missing facts. Sort of like a rete engine in the matching phase. But i think its fine like this for now and we can revisit it.

allowUndefinedFacts - By default, when a running engine encounters an undefined fact, an exception is thrown. Turning this option on will cause the engine to treat undefined facts as undefined. (default: false)

[juannorris]

@juannorris as far as i can tell this makes sense. im just wondering of there is also a way to elimitate rules from the processing queue based on missing facts. Sort of like a rete engine in the matching phase. But i think its fine like this for now and we can revisit it.

allowUndefinedFacts - By default, when a running engine encounters an undefined fact, an exception is thrown. Turning this option on will cause the engine to treat undefined facts as undefined. (default: false)

Thanks @nimbit-software!

I might be misinterpreting this, but with allowUndefinedFacts set, I don't think we should eliminate rules based on missing facts, because this is the way consumers can effectively check against missing or null values inside more complex rules.

Before this change, the test named should succeed when undefined facts are allowed would fail for all these cases:

{ some: { } }
{ some: { undefinedFact: undefined } }
{ some: { undefinedFact: null } }

[d-sooter]

Hi @juannorris good point. Maybe an additional prop for turning that on. We want to use it for an iot system. So a lot of the time data will be missing for the rules which means the rule should not be considered.

Maybe it makes sense to add some options for rules. Then we can skip rules where all facts are needed (which i assume will be most of them). That way its a lot more flexible