NPE reporting fix#580
Conversation
Codecov Report
@@ Coverage Diff @@
## master #580 +/- ##
============================================
+ Coverage 70.35% 70.43% +0.07%
- Complexity 2692 2695 +3
============================================
Files 106 106
Lines 8945 8945
Branches 1765 1765
============================================
+ Hits 6293 6300 +7
+ Misses 2219 2215 -4
+ Partials 433 430 -3
Continue to review full report at Codecov.
|
nickl-
left a comment
There was a problem hiding this comment.
I agree with the changes made to errorMessage but please remove the iniCause redundancies as the Throwable is already passed in the constructor.
| } catch ( EvalError e ) { | ||
| throw new BSFException(BSFException.REASON_OTHER_ERROR, "bsh internal error: "+e, e); | ||
| BSFException bsf = new BSFException(BSFException.REASON_OTHER_ERROR, "bsh internal error: "+e, e); | ||
| bsf.initCause(e); |
There was a problem hiding this comment.
This is redundant as we are already passing e to the constructor.
There was a problem hiding this comment.
Happy to make this change, although the reason why this was done is to ensure the BSFException supported Exception chaining.
AFAIK The BSFException class does not populate the cause of the exception use the constructor (Most up to date one I found was at: https://github.com/apache/commons-bsf/blob/master/src/main/java/org/apache/bsf/BSFException.java) :
public BSFException (int reason, String msg, Throwable t) {
this (reason, msg);
targetThrowable = t;
}
It tracks the throwable via a class variable "targetThrowable" which is exposed using the getTargetException() method. The getTargetException() method was superseded by getCause() for general purpose exception chaining.
Initialising the cause ensures they getCause() returns the expected, removing the initCause() stopped exception chaining working for me.
There was a problem hiding this comment.
@nickl- I'll hold off making the change until you've commented on the exception chaining issue and if the desire is to keep exception chaining disabled. BR.
There was a problem hiding this comment.
I see well spotted!! initCause will be the desired route then.
…orting NPE reporting fix on BSFEngine
Beanshell emitting NPE's via the BSF integeration just throws an error reporting a "NullPointerException" in line 0.
This fix adds meta data to the NPE giving the line number and the offending line.
Fixes #572