Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The method onSucces() of the RuleListener is not executed on Rule that are registered in a CompositeRule #273

Closed
tthebaud opened this issue Apr 24, 2020 · 3 comments

Comments

@tthebaud
Copy link

tthebaud commented Apr 24, 2020

I have noticed that the method onSuccess() in the RuleListener is only executed on the Rule that are registered directly in the Rules object but not when registered in a CompositeRule such as ConditionalRuleGroup.

If I have this method in my RuleListener :

@Override
public void onSuccess(final Rule rule, final Facts facts) {
    System.out.println(rule.getName());
}

I get the following results in the following examples :

Example : onSuccess(Rule rule, Facts facts) is executed

Rule rule1 = new RuleBuilder() 
    .name("rule1") 
    .when(Condition.TRUE)
    .then(action1)
    .build();

Rule rule2 = new RuleBuilder() 
    .name("rule2")
    .when(Condition.TRUE)
    .then(action2)
    .build();

Rules myRules = new Rules();
myRules.register(rule1);
myRules.register(rule2);

The results :

rule1
rule2

Example: onSuccess(Rule rule, Facts facts) is not executed

Rule rule1 = new RuleBuilder() 
    .name("rule1") 
    .when(Condition.TRUE)
    .then(action1)
    .build();

Rule rule2 = new RuleBuilder() 
    .name("rule2")
    .when(Condition.TRUE)
    .then(action2)
    .build();
        
final ConditionalRuleGroup conditionalRuleGroup1 = new ConditionalRuleGroup("conditionalRuleGroup1");
conditionalRuleGroup1.addRule(rule1);
conditionalRuleGroup1.addRule(rule2);

Rule rule3 = new RuleBuilder() 
    .name("rule3")
    .when(Condition.TRUE)
    .then(action3)
    .build();

final ConditionalRuleGroup conditionalRuleGroup2 = new ConditionalRuleGroup("conditionalRuleGroup2");
conditionalRuleGroup2.addRule(rule3);
conditionalRuleGroup2.addRule(conditionalRuleGroup1);

final Rules myRules = new Rules();
myRules.register(conditionalRuleGroup2);

The results :

conditionalRuleGroup2

I think it is because in the ConditionalRuleGroup, the method execute() does not call the RuleListener's method.

@Override
public void execute(Facts facts) throws Exception {
    conditionalRule.execute(facts);
    for (Rule rule : sort(successfulEvaluations)) {
        rule.execute(facts);
    }
}

Is there a reason as to not call it like you do with the DefaulRuleEngine in the execute() method ?

try {
    triggerListenersBeforeExecute(rule, facts);
    rule.execute(facts);
    LOGGER.debug("Rule '{}' performed successfully", name);
    triggerListenersOnSuccess(rule, facts);
    if (parameters.isSkipOnFirstAppliedRule()) {
        LOGGER.debug("Next rules will be skipped since parameter skipOnFirstAppliedRule is set");
        break;
}
@fmbenhassine
Copy link
Member

I think it is because in the ConditionalRuleGroup, the method execute() does not call the RuleListener's method.

Please note that a rule should not be aware that there is a listener around it. It should not call the listener itself, it is the engine driving the process that calls the listener before/after each rule.

However, when using a composite rule, the engine sees the composite rule as a regular rule (This is inherent to the composite design pattern according to which composite rules are implemented). The engine works against the Rule interface and should not be aware of the rule implementation (if it is a composite or not). That's why you see the listener invoked around the composite rule and not around each nested composing rule.

Hope this helps. I'm closing this for now, but if you need more support, please do not hesitate to add a comment.

@tthebaud
Copy link
Author

tthebaud commented May 4, 2020

Thank you for your response,

I understand that the engine is the one which drives the process and should be the one to call the listener and I understand that a composite rule is seen as a regular rule. But should not it at least be aware that a regular rule can be decomposed into several rules ?

The problem is that if I want to do some things before/after each rule, I will have to create other condition/action which will make the code less maintainable and readable. This why I wanted to use the listener.

@fmbenhassine
Copy link
Member

But should not it at least be aware that a regular rule can be decomposed into several rules ?

No, as far as the engine is concerned, that's an implementation detail. If the engine starts looking if a Rule is an instance of X or Y, that would be problematic and poorly designed.

The problem is that if I want to do some things before/after each rule, I will have to create other condition/action which will make the code less maintainable and readable. This why I wanted to use the listener.

RuleListener is not the right choice if you want to apply logic before/after the internal (composing) rules of a composite rule. You did not share what kind of logic you want to apply, but there should be a clean way of doing that without a rule listener. I'm thinking of extracting that code in a separate class (that you can unit test, etc) and (re)use it in a rule decorator. Using AOP is another option depending on your context (if you use Spring that should be fairly easy). Good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants