deliveredtechnologies / rulebook

100% Java, Lambda Enabled, Lightweight Rules Engine with a Simple and Intuitive DSL
http://www.deliveredtechnologies.com
Apache License 2.0
721 stars 124 forks source link

RuleBookRunner.getResult() is declared as Optional<Result> but always returns a value #130

Closed mill5james closed 6 years ago

mill5james commented 6 years ago

I would expect that public Optional<Result> getResult() on the RuleBookRunner class would not return a value if any result was not set during the run of the rules.

Given a very simple rule

@Rule()
public class IsEven {
  @Given("key") Integer value;
  @Result Boolean result;
  @When
  public boolean when() { return value % 2 == 0; }
  @Then
  public RuleState then() {
    result = true;
    return RuleState.NEXT;
  }
}

But if I run the rules expecting no result if my rule does not match

public class App {
  public static void main(String[] args) {
    RuleBookRunner rules = new RuleBookRunner("rules");

    NameValueReferableMap<Integer> facts = new FactMap<>();
    facts.put("key", new Fact<>(1));

    rules.run(facts);

    Optional<Result> optionalResult = rules.getResult();
    if (optionalResult.isPresent()) {
      System.out.println("There is a result");
      Boolean result = (Boolean) optionalResult.get().getValue();
      System.out.println("The value is even!");
    }
  }
}

I get an exception:

13:05:17.411 [main] DEBUG com.deliveredtechnologies.rulebook.model.runner.RuleBookRunner - rules URI -> file:/D:/temp/simple-result-pojo/out/production/classes/rules
Disconnected from the target VM, address: '127.0.0.1:50601', transport: 'socket'
There is a result
Exception in thread "main" java.lang.ClassCastException: java.lang.Object cannot be cast to java.lang.Boolean
    at App.main(App.java:21)

Process finished with exit code 1

I believe that this is a bug in the AbstractRuleBookRunner class

https://github.com/rulebook-rules/rulebook/blob/400495a53a6129c15481871400a1ea0e6c49395c/rulebook-core/src/main/java/com/deliveredtechnologies/rulebook/model/runner/AbstractRuleBookRunner.java#L56

I don't think you should create a new Object() if the _result.getValue() == null. This has the effect of setting the default result value to return true for a call to Optional<T>.isPresent().

Attached is a sample project simple-result-pojo.zip

Clayton7510 commented 6 years ago

For RuleBookRunner, you are correct. There is always some result. This allows the result to be injected and chained in POJO rules. However, if you were to create a RuleBook using the Java DSL or, say, CoRRuleBook then it would not always have a result. The implementation of a RuleBook and the RuleBook interface are separate, so getResult returns an Optional as part of the interface. It's up to the implementation of each RuleBook as to how it maintains a Result.

mill5james commented 6 years ago

@Clayton7510 I think I understand what you are saying. It just seems that if you have introduced an Optional<Result> then it should be optional. Why does the RuleBookRunner require a result while other methods allow the optional behavior?

I have submitted a Pull Request that implements the behavior that if the result is not specified, the Optional<Result>.isPresent() returns false when no rule as set a result, and returns true when a result is returned.

mill5james commented 6 years ago

Also, there is a setDefaultResult(…) method on RuleBookRunner so that if you wanted to always return well-know value and avoid the Optional<Result>.isPresent() test, that is still possible.

Clayton7510 commented 6 years ago

I don't remember exactly what the reason was for RuleBookRunner doing that. I think it may have had something to do with some rules having a default result and others not having a default result and it not maintaining the reference from one to the next... honestly, I don't recall.

But I agree that it would be nice to not enforce a default result if none is specified. Thanks for the PR. I'll check it out shortly.

Clayton7510 commented 6 years ago

Merged.