[jboss-jira] [JBoss JIRA] (DROOLS-687) Helpful error message for MvelConstraint

[Geoffrey De Smet (JIRA)]

    [ https://issues.jboss.org/browse/DROOLS-687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13033054#comment-13033054 ] 

Geoffrey De Smet commented on DROOLS-687:
-----------------------------------------

After discussion with Marco we see several ways to implement this:

Proposal A) In mvel add a SourceContext object during bootstrap, so when evaluations turn bad, it can include the context in the error message.
Note: you might be able to reuse ParserContext directly instead of having to add a SourceContext interface and instance.

{code}
interface SourceContext {
    String buildErrorMessagePart();
}
class ReflectiveAccessorOptimizer {
   ...
   SourceContext sourceContext;

   public ReflectiveAccessorOptimizer(....,  SourceContext sourceContext) {
       this.sourceContext = sourceContext;
   }

   public Object evaluateExpression(...) {
        ...
        // Old code
        // if (curr == null) throw new NullPointerException();
        // New code
        if (curr == null) throw new NullPointerException("NPE happened at " + sourceContext.buildErrorMessagePart());
   }
{code}

Pro: no performance impact! (the if's, catches etc are already there. Only when it's broken it might take a few nanoseconds longer to build the exception, but nobody cares how slow a broken app is)
Con: A lot of work (need to be applied throughout mvel, including jitted constraints)


Proposal B) try-catch around every mvel calls in drools and somehow figure out the context from drools.
This means that the encapsulating drools code needs to know the exact line of every mvel object it uses...
Con: performance impact! Try-catches during runtime code, all the time.
Con: easy to forgot try-catch when writing new drools code that uses mvel (maintenance prob)

Proposal C) Introduce boolean assertMode that if true does B) (so a try catch around every mvel call) and otherwise not
Con: User has to be educated about this assertMode
Con: 2 paths of execution that need to be kept in sync.


Personally, I prefer A) - anything worth doing is worth doing well ;)

> Helpful error message for MvelConstraint
> ----------------------------------------
>
>                 Key: DROOLS-687
>                 URL: https://issues.jboss.org/browse/DROOLS-687
>             Project: Drools
>          Issue Type: Enhancement
>    Affects Versions: 6.2.0.CR4
>            Reporter: Toshiya Kobayashi
>            Assignee: Mario Fusco
>            Priority: Critical
>
> When you hit NullPointerException with LHS constraint during runtime, the error message is not very helpful for users to detect the root cause.
> For example,
> {noformat}
> rule 'hello person'
>    when
>        Person( address.street == 'abbey' )
>    then
> end
> {noformat}
> If Person.address is null, the error message is:
> {noformat}
> [Error: null pointer: address.street]
> [Near : {... address.street == "abbey" ....}]
>              ^
> [Line: 1, Column: 1]
> 	at org.mvel2.optimizers.impl.refl.ReflectiveAccessorOptimizer.compileGetChain(ReflectiveAccessorOptimizer.java:427)
> 	at org.mvel2.optimizers.impl.refl.ReflectiveAccessorOptimizer.optimizeAccessor(ReflectiveAccessorOptimizer.java:140)
> 	at org.mvel2.ast.ASTNode.optimize(ASTNode.java:159)
> 	at org.mvel2.ast.ASTNode.getReducedValueAccelerated(ASTNode.java:115)
> 	at org.mvel2.ast.BinaryOperation.getReducedValueAccelerated(BinaryOperation.java:117)
> 	at org.mvel2.MVELRuntime.execute(MVELRuntime.java:86)
> 	at org.mvel2.compiler.CompiledExpression.getDirectValue(CompiledExpression.java:123)
> 	at org.mvel2.compiler.CompiledExpression.getValue(CompiledExpression.java:119)
> 	at org.mvel2.compiler.CompiledExpression.getValue(CompiledExpression.java:113)
> 	at org.mvel2.MVEL.executeExpression(MVEL.java:930)
> 	at org.drools.core.util.MVELSafeHelper$RawMVELEvaluator.executeExpression(MVELSafeHelper.java:481)
> 	at org.drools.core.rule.constraint.MvelConditionEvaluator.evaluate(MvelConditionEvaluator.java:77)
> 	at org.drools.core.rule.constraint.MvelConditionEvaluator.evaluate(MvelConditionEvaluator.java:62)
> 	at org.drools.core.rule.constraint.MvelConstraint.evaluate(MvelConstraint.java:226)
> 	at org.drools.core.rule.constraint.MvelConstraint.isAllowed(MvelConstraint.java:183)
> 	at org.drools.core.reteoo.AlphaNode.assertObject(AlphaNode.java:138)
> 	at org.drools.core.reteoo.SingleObjectSinkAdapter.propagateAssertObject(SingleObjectSinkAdapter.java:60)
> 	at org.drools.core.reteoo.ObjectTypeNode.assertObject(ObjectTypeNode.java:290)
> 	at org.drools.core.reteoo.EntryPointNode.assertObject(EntryPointNode.java:253)
> 	at org.drools.core.common.NamedEntryPoint.insert(NamedEntryPoint.java:281)
> 	at org.drools.core.common.NamedEntryPoint.insert(NamedEntryPoint.java:241)
> 	at org.drools.core.impl.StatefulKnowledgeSessionImpl.insert(StatefulKnowledgeSessionImpl.java:1481)
> 	at org.drools.core.impl.StatefulKnowledgeSessionImpl.insert(StatefulKnowledgeSessionImpl.java:1436)
> 	at org.drools.compiler.integrationtests.Misc2Test.testMvelConstraintErrorMessage(Misc2Test.java:7086)
> ...
> Caused by: java.lang.NullPointerException
> 	at org.mvel2.optimizers.impl.refl.ReflectiveAccessorOptimizer.compileGetChain(ReflectiveAccessorOptimizer.java:393)
> 	... 46 more
> {noformat}
> Users cannot identify which rule is associated with the constraint when they have many similar rules. Also users cannot identify which fact (Person) caused the issue when they insert many facts.
> I'm not sure if it's really possible to provide a better message in this case. But I filed this as an enhancement request.



--
This message was sent by Atlassian JIRA
(v6.3.11#6341)