Update GoldenRule exception handling to be more transparent

[Clayton7510]

GoldenRule currently swallows all exceptions. While this may be helpful in some cases (e.g. a rule attempts to use a fact that does not yet exist), it can hide other exceptions that should be thrown. GoldenRule should therefore be updated to throw non-whitelisted exceptions.

[Clayton7510]

Ha, I completely forgot that this is already accounted for when executing actions for a rule since a rule can have multiple actions. The catch block that was hiding an error is really intended to catch when errors - the error that was being hidden was a bug with filtering facts in a specific scenario. Given that, I think it's appropriate that the catch stands as is.

[rizjoj]

I would have really liked my exceptions in my pojo rules to bubble up (and halt the rule chain) so that I can report them back to the user (this is because my rules calls other code which could throw custom Exceptions in certain situations and would like to report to the user why the process failed)

As a workaround, I can catch my exception within the rule's Then/When clause, store the message in a NameValueReferable "error" variable, break the rule chain, inspect the "error" variable in Results and re-throw my custom exception with the same error message. It does feel like I'm artificially recreating the exception throw due to its being swallowed - also bloats the otherwise simple and easy-to-read POJO rules.

I don't have a concrete opinion on this yet (I see merits to the current implementation). Just thought I'd mention my use case.

[Clayton7510]

I've kind of been going back and forth with this. On one hand, I think conditions should be pretty specific and that it's the responsibility of the implementer to ensure that their conditions are error free (or that they are appropriately trapped and handled in the condition) - rules are executed in a chain, but they should be relatively autonomous. On the other hand, I see benefit in knowing if a rule failed or if it wasn't executed - that's kind of where rule auditing went.

Now, I'm kind of thinking... rules are not aware of auditing. An AuditableRule is really just a decorator on top of a Rule. But what if there was an option for rules to become audit aware to the extent that a Rule could supply the audit with additional information, like exception info?

Additionally, there is already functionality to stop the rule chain on failure. That was kind of an afterthought and possibly not as elegantly implemented (in terms of the interface) as it could be. What if we took that idea and and extended it to meet your use case? So, perhaps in the case when stop on failure was set then the exception would be thrown and the rule chain would stop execution.

[rizjoj]

I actually like the thought in your last paragraph - stop on failure and bubble the exception up top. If so implemented, would this be automatic? or would I have to set a rule state or set a flag? Sounds interesting.

If I could make a case for it being automatic (I'm totally glossing over the practical limitations here!) ... my reasoning goes back to why I'm a huge fan of Rulebook to begin with (and I've used drools and others years ago and abandoned them): The rules are in native java (yay!). So because of that I expected exceptions to behave as they do natively in java. In that, they would bubble up to where the rulebook is run and I get obviously get that the rule chain is broken (just like the stack is stopped on any exception that isn't caught) and I have to decide whether to swallow my exception or let it continue up the call stack.

Purely my opinion but, whenever I've dealt with rules I've always felt a rule had 3 states: rule succeeds, rule fails, and something extraordinary happened from preventing the rule from running: rule exception.

[Clayton7510]

I think in the case of stop on failure, the exception should automatically be thrown up the call stack. I think that makes sense.

[Clayton7510]

I need to amend my last statement a little bit. If I changed to it automatic then that would break compatibility with previous versions and I don't think a major version update is needed for this. So, instead, I am adding a new RuleChainActionType of ERROR_ON_FAILURE, which would throw back up any exception encountered while running a rule.

https://github.com/rulebook-rules/rulebook/tree/feature/Issue127_GoldenRuleExceptionHandling

[rizjoj]

Does it work if the exception is also thrown in the @When method? If so, then that works

[Clayton7510]

Yes, it would throw an exception if it occurred in the "when" method or in any "then" method. I'll put in some tests to prove that out. But that's the direction I was going with it.

[Clayton7510]

I have to amend my previous statement. The default behavior is still CONTINUE_ON_FAILURE. So, basically exceptions in rules just trigger the next rule in the chain (they are logged, however). But, if you specify ERROR_ON_FAILURE when creating a rule, either in the constructor, in the create() method of the rule builder or as the value of the ruleChainAction in the @Rule annotation, then any errors in a rule will be bubbled up, wrapped in a RuleException RuntimeException.

There were a couple of reasons behind this approach. The biggest reason was maintaining compatibility. The other reason was that I didn't want to impose a throws declaration in the Rule interface/contract. Anyway, I think this allows for the desired behavior with relatively little impact to existing code and it keeps things pretty simple.

It's currently in the 0.11-SNAPSHOT release.

[rizjoj]

It hasn't worked for me yet when I tried. I added the ruleChainAction property to my POJO rule like so: @Rule(ruleChainAction = ERROR_ON_FAILURE)

and I got a log of the stacktrace like so:

java.lang.reflect.InvocationTargetException: null
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.deliveredtechnologies.rulebook.model.runner.RuleAdapter.lambda$null$12(RuleAdapter.java:284)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.deliveredtechnologies.rulebook.model.GoldenRule.lambda$invoke$8(GoldenRule.java:176)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.GoldenRule.invoke(GoldenRule.java:173)
    at com.deliveredtechnologies.rulebook.model.runner.RuleAdapter.invoke(RuleAdapter.java:191)
    at com.deliveredtechnologies.rulebook.model.AuditableRule.invoke(AuditableRule.java:94)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:30)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.lambda$handleRequest$1(RuleHandler.java:34)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.RuleHandler.handleRequest(RuleHandler.java:32)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.CoRRuleBook.lambda$run$2(CoRRuleBook.java:46)
    at java.util.Optional.ifPresent(Optional.java:159)
    at com.deliveredtechnologies.rulebook.model.rulechain.cor.CoRRuleBook.run(CoRRuleBook.java:44)
    at com.deliveredtechnologies.rulebook.model.runner.AbstractRuleBookRunner.run(AbstractRuleBookRunner.java:69)
    at ly.urgent.microservice.pricing.v1.calculators.VolvoPricingCalculator.runRules(VolvoPricingCalculator.java:49)
    at ly.urgent.microservice.pricing.v1.calculators.VolvoPricingCalculator.getPartnerPrice(VolvoPricingCalculator.java:38)
    at ly.urgent.microservice.pricing.v1.VolvoPricingCalculatorTests.test_customer_price_when_on_demand_for_100_plus_miles_tow_before_provider_assign(VolvoPricingCalculatorTests.java:409)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:19)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:239)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Caused by: ly.urgent.microservice.pricing.v1.PricingException: Cannot determine provider payments without jobId
    at ly.urgent.microservice.pricing.v1.calculators.volvo.rules.partner.TowExcessDistanceChargeRule.then(TowExcessDistanceChargeRule.java:43)
    ... 72 common frames omitted

where PricingException is my exception thrown in my pojo rule's then method.

So, it looks like I got the old log of the InvocationTargetException instead of the RuleException itself bubbled up. Could the null in RuleAdapter.java:284 have anything to do with it? Maybe that's how it's supposed to behave, not sure.

[rizjoj]

@Clayton7510 I think I've narrowed it down for POJO rules via the debugger. Since AbstractRuleBookRunner wraps the GoldenRule in a RuleAdapter, when the POJO rule's (in my case) @Then method throws a custom exception, it gets caught by RuleAdapter's catch clause (which logs the error) and does not propagate it to GoldenRule's catch clause. I'm currently stuck as getting RuleAdapter's getThenMethodAsBiConsumer method to propagate the exception is proving tricky (RuleAdapter.java:283) due to the Optionals and lambdas. Any feedback would be appreciated.

[Clayton7510]

I thought that was sorted in the latest SNAPSHOT release. But if you are seeing something different then I’ll revisit the tests.

Thanks,

Clayton Long

[xPlutox]

I would also love to see this feature in Rulebook, the "continue on error" behaviour wil now swallow exceptions and hide possible development errors.

These development errors are in some cases even tricky to catch with unit testing.

[Clayton7510]

We do what we can ;) Picking it back up and sorting how this issue cloud have popped up now.

[Clayton7510]

@xPlutox and @rizjoj I think I sorted the issue. The tests I had for errors bubbling up when ruleChainAction = ERROR_ON_FAILURE were only testing when an error occurred in the "when" and not the "then." In addition, the errors were buried under a couple of rethrows due to RuleAdapter, which made it difficult to determine the actual error thrown. So, I just submitted an update, which is now in rulebook-core v0.11-SNAPSHOT. You should now see errors thrown in the "when" and/or "then" sections as the immediate cause of the RuleException thrown. And neither "when" or "then" errors are squashed.

[xPlutox]

Ran my tests, works like a charm.

[Clayton7510]

Glad to hear it.

[rizjoj]

Hi @Clayton7510, sorry to burst the bubble but I had some time to test this and I'm still not getting the ERROR_ON_FAILURE working. As before I'm getting my exception from within my rule wrapped inside InvocationTargetException.

One thing I noticed is that while the develop branch had updates related to this issue in the RuleAdapter class: specifically lines 292-294 and 312-314 (the if ERROR_ON_FAILURE condition in the catch clause; commit f2e4f815975b063c75fcb6c5952fd2779da62f04), my maven downloaded 0.11-SNAPSHOT jar is missing this update in this class: rulebook-core:0.11-SNAPSHOT.

Do you see this in the maven jar for rulebook-core:0.11-SNAPSHOT as well? I checked and double checked to make sure I'm not reporting a red herring.

[Clayton7510]

@rizjoj I wasn't able to duplicate that. Here is a project (under the examples in the release/v0.11 branch) that I created to test it out: https://github.com/rulebook-rules/rulebook/tree/release/v0.11/rulebook-examples/megabank-exception-breaks-rulechain

I am using the 0.11-SNAPSHOT maven dependency from SonaType's SNAPSHOT repo.

Of course, I could be missing something.

If you are still seeing this issue, would you mind attaching the code that is not working in a gzip to this issue so I can take a look?

[rizjoj]

@Clayton7510 thanks. I had forgotten that the snapshot respository is on sonatype-snapshot (not maven central) and had never imported it before. Once I imported the snapshot repo, it works! (tested SpringAwareRuleBookRunner and ERROR_ON_FAILURE). Eagerly awaiting 0.11 release on maven central! :)