ExpressionFactory.createNewExpression should throw an Exception in case of a parsing error, not Error

• Key: JEXL-17
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Kohsuke Kawaguchi
• Affects Version/s: 1.0
• Fix Version/s: 1.1

Description

When ExpressionFactory.createNewExpression(...) takes an syntactically incorrect expression, JEXL throws TokenMgrError. However, typical callers (such as Jelly) don't expect such syntax error to be a java.lang.Error, so it fails to catch it.

A typical outcome is therefore the thread to die completely, and even worse the stack trace doesn't report neither what was the expression that was being parsed, nor does it report any contextual information (again in case of Jelly, that would be where in the jelly script this error happened.)

This makes the error diagnosis unnecessarily hard. I believe the proper thing to do is to wrap such an exception into java.lang.Exception or its sub-class, allowing the caller to catch it.

Comments

Patch for this change.

[Rahul Akolkar]

Can you supply a test case (just the expression String should do) where this happens? Usually, I see oacj.parser.ParseException if the expression is malformed. It would seem more appropriate to (re)use that here, if needed, rather than defining a new exception.

[Kohsuke Kawaguchi]

For example, "a?b:c" should do, since the lexer wouldn't understand '?'.
I overlooked that existing exception, and I agree that it's better to reuse it.

[Rahul Akolkar]

Thanks, fixed in r429169:

http://svn.apache.org/viewvc?view=rev&revision=429169

Reused the existing ParseException and added a couple of test cases to ensure the Error is rethrown as this Exception.