SLING-11299 : JSP source/linenumber is not always shown when an exception occurs
diff --git a/src/main/java/org/apache/sling/scripting/jsp/JspScriptEngineFactory.java b/src/main/java/org/apache/sling/scripting/jsp/JspScriptEngineFactory.java
index 34c49f1..d9b5342 100644
--- a/src/main/java/org/apache/sling/scripting/jsp/JspScriptEngineFactory.java
+++ b/src/main/java/org/apache/sling/scripting/jsp/JspScriptEngineFactory.java
@@ -24,17 +24,14 @@
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.script.Bindings;
import javax.script.ScriptContext;
import javax.script.ScriptEngine;
import javax.script.ScriptException;
import javax.servlet.ServletContext;
-import javax.servlet.ServletException;
import org.apache.sling.api.SlingHttpServletRequest;
-import org.apache.sling.api.SlingServletException;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.resource.observation.ExternalResourceChangeListener;
import org.apache.sling.api.resource.observation.ResourceChange;
@@ -54,7 +51,6 @@
import org.apache.sling.scripting.jsp.jasper.runtime.AnnotationProcessor;
import org.apache.sling.scripting.jsp.jasper.runtime.JspApplicationContextImpl;
import org.apache.sling.scripting.jsp.jasper.servlet.JspServletWrapper;
-import org.apache.sling.scripting.jsp.util.TagUtil;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
import org.osgi.service.component.annotations.Activate;
@@ -63,7 +59,6 @@
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.component.annotations.ReferenceCardinality;
import org.osgi.service.component.annotations.ReferencePolicy;
-import org.osgi.service.component.annotations.ReferencePolicyOption;
import org.osgi.service.metatype.annotations.AttributeDefinition;
import org.osgi.service.metatype.annotations.Designate;
import org.osgi.service.metatype.annotations.ObjectClassDefinition;
@@ -585,36 +580,13 @@
if (!contextHasPrecompiledJsp) {
callJsp(slingBindings);
}
- } catch (final SlingServletException e) {
- // ServletExceptions use getRootCause() instead of getCause(),
- // so we have to extract the actual root cause and pass it as
- // cause in our new ScriptException
- if (e.getCause() != null) {
- // SlingServletException always wraps ServletExceptions
- Throwable rootCause = TagUtil.getRootCause((ServletException) e.getCause());
- // the ScriptException unfortunately does not accept a Throwable as cause,
- // but only a Exception, so we have to wrap it with a dummy Exception in Throwable cases
- if (rootCause instanceof Exception) {
- throw new BetterScriptException(rootCause.toString(), (Exception) rootCause);
- }
- throw new BetterScriptException(rootCause.toString(),
- new Exception("Wrapping Throwable: " + rootCause.toString(), rootCause));
- }
-
- // fallback to standard behaviour
- throw new BetterScriptException(e.getMessage(), e);
} catch (final SlingPageException sje) {
try {
callErrorPageJsp(slingBindings, sje.getErrorPage());
} catch (final Exception e) {
-
- throw new BetterScriptException(e.getMessage(), e);
+ throw e;
}
- } catch (final Exception e) {
-
- throw new BetterScriptException(e.getMessage(), e);
-
} finally {
// make sure the context loader is reset after setting up the
@@ -654,29 +626,6 @@
return this.jspRuntimeContext;
}
- /**
- * Fixes {@link ScriptException} that overwrites the
- * {@link ScriptException#getMessage()} method to display its own
- * <code>message</code> instead of the <code>detailMessage</code>
- * defined in {@link Throwable}. Unfortunately using the constructor
- * {@link ScriptException#ScriptException(Exception)} does not set the
- * <code>message</code> member of {@link ScriptException}, which leads to
- * a message of <code>"null"</code>, effectively supressing the detailed
- * information of the cause. This class provides a way to do that explicitly
- * with a new constructor accepting both a message and a causing exception.
- *
- */
- private static class BetterScriptException extends ScriptException {
-
- private static final long serialVersionUID = -6490165487977283019L;
-
- public BetterScriptException(final String message, final Exception cause) {
- super(message);
- this.initCause(cause);
- }
-
- }
-
@Override
public void onChange(final List<ResourceChange> changes) {
for(final ResourceChange change : changes){
diff --git a/src/main/java/org/apache/sling/scripting/jsp/jasper/JspCompilationContext.java b/src/main/java/org/apache/sling/scripting/jsp/jasper/JspCompilationContext.java
index 616c545..437ecbf 100644
--- a/src/main/java/org/apache/sling/scripting/jsp/jasper/JspCompilationContext.java
+++ b/src/main/java/org/apache/sling/scripting/jsp/jasper/JspCompilationContext.java
@@ -204,7 +204,15 @@
}
public Compiler getCompiler() {
- return createCompiler();
+ return this.jspCompiler;
+ }
+
+ public Compiler activateCompiler() {
+ if ( jspCompiler == null ) {
+ // create compile and compile
+ this.compile(); // we ignore the exception
+ }
+ return jspCompiler;
}
/** ---------- Access resources in the webapp ---------- */
@@ -495,14 +503,10 @@
// ==================== Compile and reload ====================
public JasperException compile() {
- return compile(false);
- }
-
- public JasperException compile(boolean fromJSPC) {
final Compiler c = createCompiler();
try {
c.removeGeneratedFiles();
- c.compile(true, fromJSPC);
+ c.compile(true, false);
} catch (final JasperException ex) {
return ex;
} catch (final IOException ioe) {
diff --git a/src/main/java/org/apache/sling/scripting/jsp/jasper/compiler/TagLibraryInfoImpl.java b/src/main/java/org/apache/sling/scripting/jsp/jasper/compiler/TagLibraryInfoImpl.java
index d831ca0..0b9a9af 100644
--- a/src/main/java/org/apache/sling/scripting/jsp/jasper/compiler/TagLibraryInfoImpl.java
+++ b/src/main/java/org/apache/sling/scripting/jsp/jasper/compiler/TagLibraryInfoImpl.java
@@ -164,7 +164,7 @@
parseTLD(ctxt, location[0], in, null);
// Add TLD to dependency list
- PageInfo pageInfo = ctxt.getCompiler().getPageInfo();
+ PageInfo pageInfo = ctxt.activateCompiler().getPageInfo();
if (pageInfo != null) {
pageInfo.addDependant(location[0]);
}
diff --git a/src/main/java/org/apache/sling/scripting/jsp/jasper/servlet/JspServletWrapper.java b/src/main/java/org/apache/sling/scripting/jsp/jasper/servlet/JspServletWrapper.java
index 2dc7480..ca3f8a7 100644
--- a/src/main/java/org/apache/sling/scripting/jsp/jasper/servlet/JspServletWrapper.java
+++ b/src/main/java/org/apache/sling/scripting/jsp/jasper/servlet/JspServletWrapper.java
@@ -41,8 +41,6 @@
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.sling.api.SlingException;
-import org.apache.sling.api.SlingIOException;
-import org.apache.sling.api.SlingServletException;
import org.apache.sling.api.scripting.ScriptEvaluationException;
import org.apache.sling.api.scripting.SlingBindings;
import org.apache.sling.commons.classloader.DynamicClassLoader;
@@ -434,29 +432,36 @@
}
/**
- * @param bindings
- * @throws SlingIOException
- * @throws SlingServletException
- * @throws IllegalArgumentException if the Jasper Precompile controller
- * request parameter has an illegal value.
+ * Call the jsp
+ * @param bindings The bindings for the jsp
+ * @throws SlingPageException JSP page exception handler exceptions
+ * @throws SlingException for any non runtime exception
+ * @throws RuntimeException for runtime exceptions
+ *
*/
public void service(final SlingBindings bindings) {
try {
service(bindings.getRequest(), bindings.getResponse());
- } catch (SlingException se) {
- // rethrow as is
+ } catch ( final ServletException se ) {
+ if ( se.getRootCause() != null ) {
+ final Throwable t = se.getRootCause();
+ if( t instanceof Exception ) {
+ handleJspException((Exception)t);
+ }
+ }
+ handleJspException(se);
+ } catch ( final SlingPageException se) {
+ // don't handle SlingPageExceptions, just rethrow
throw se;
- } catch (IOException ioe) {
- throw new SlingIOException(ioe);
- } catch (ServletException se) {
- throw new SlingServletException(se);
+ } catch (final Exception ex) {
+ handleJspException(ex);
}
}
/**
* Process the request.
*/
- public void service(final HttpServletRequest request,
+ private void service(final HttpServletRequest request,
final HttpServletResponse response)
throws ServletException, IOException {
try {
@@ -515,16 +520,6 @@
(HttpServletResponse.SC_SERVICE_UNAVAILABLE,
ex.getMessage());
return;
- } catch (final SlingException ex) {
- throw ex;
- } catch (final ServletException ex) {
- handleJspException(ex);
- } catch (final IOException ex) {
- handleJspException(ex);
- } catch (final IllegalStateException ex) {
- handleJspException(ex);
- }catch (final Exception ex) {
- handleJspException(ex);
}
}
@@ -584,6 +579,15 @@
}
}
+ private RuntimeException wrapException(final Exception e) {
+ if ( e instanceof RuntimeException ) {
+ return (RuntimeException) e;
+ }
+ // wrap in ScriptEvaluationException
+ return new ScriptEvaluationException(this.ctxt.getJspFile(),
+ e.getMessage() == null ? e.toString() : e.getMessage(), e);
+ }
+
/**
* <p>Attempts to construct a JasperException that contains helpful information
* about what went wrong. Uses the JSP compiler system to translate the line
@@ -595,43 +599,13 @@
*</p>
*
* @param ex the exception that was the cause of the problem.
- * @throws a ServletException with more detailed information
+ * @throws a SlingException Wrapping the original exception
*/
- protected void handleJspException(final Exception ex)
- throws ServletException {
- final Exception jspEx = handleJspExceptionInternal(ex);
- if ( jspEx instanceof ServletException ) {
- throw (ServletException)jspEx;
- }
- throw (SlingException)jspEx;
- }
-
- /**
- * Returns only a ServletException or a SlingException
- */
- private Exception handleJspExceptionInternal(final Exception ex)
- throws ServletException {
- Throwable realException = ex;
- String exMessage = realException.getMessage();
- if (ex instanceof ServletException) {
- realException = ((ServletException) ex).getRootCause();
- // root cause might be null (eg. for a JasperException ex)
- if (realException == null) {
- realException = ex;
- } else {
- exMessage = ex.toString();
- }
- }
-
- // avoid nested ScriptEvaluationExceptions (eg. in nested jsp includes)
- while (realException instanceof ScriptEvaluationException) {
- realException = realException.getCause();
- exMessage = realException.getMessage();
- }
-
+ protected void handleJspException(final Exception ex) {
+ RuntimeException result = null;
try {
// First identify the stack frame in the trace that represents the JSP
- StackTraceElement[] frames = realException.getStackTrace();
+ final StackTraceElement[] frames = ex.getStackTrace();
StackTraceElement jspFrame = null;
for (int i=0; i<frames.length; ++i) {
@@ -641,50 +615,43 @@
}
}
- if (jspFrame == null) {
- // If we couldn't find a frame in the stack trace corresponding
- // to the generated servlet class, we can't really add anything
- if ( ex instanceof ServletException ) {
- return ex;
+
+ // If we couldn't find a frame in the stack trace corresponding
+ // to the generated servlet class, we can't really add anything
+ if (jspFrame != null) {
+ final int javaLineNumber = jspFrame.getLineNumber();
+ final JavacErrorDetail detail = ErrorDispatcher.createJavacError(
+ jspFrame.getMethodName(),
+ this.ctxt.activateCompiler().getPageNodes(),
+ null,
+ javaLineNumber,
+ ctxt);
+
+ // If the line number is less than one we couldn't find out
+ // where in the JSP things went wrong
+ final int jspLineNumber = detail.getJspBeginLineNumber();
+ if (jspLineNumber > 0) {
+ final String origMsg = ex.getMessage() == null ? ex.toString() : ex.getMessage();
+ final String message;
+ if (options.getDisplaySourceFragment() && detail.getJspExtract() != null ) {
+ message = Localizer.getMessage("jsp.exception", detail.getJspFileName(), String.valueOf(jspLineNumber))
+ .concat(" : ").concat(origMsg).concat("\n\n").concat(detail.getJspExtract()).concat("\n");
+
+ } else {
+ message = Localizer.getMessage("jsp.exception", detail.getJspFileName(), String.valueOf(jspLineNumber))
+ .concat(" : ").concat(origMsg);
+ }
+ result = new SlingException(message, ex);
}
- return new SlingException(ex.getMessage(), ex);
+
}
- int javaLineNumber = jspFrame.getLineNumber();
- JavacErrorDetail detail = ErrorDispatcher.createJavacError(
- jspFrame.getMethodName(),
- this.ctxt.getCompiler().getPageNodes(),
- null,
- javaLineNumber,
- ctxt);
-
- // If the line number is less than one we couldn't find out
- // where in the JSP things went wrong
- int jspLineNumber = detail.getJspBeginLineNumber();
- if (jspLineNumber < 1) {
- if ( realException instanceof ServletException ) {
- return (ServletException)realException;
- }
- return new SlingException(exMessage, realException);
- }
-
- if (options.getDisplaySourceFragment() && detail.getJspExtract() != null ) {
- return new SlingException(Localizer.getMessage
- ("jsp.exception", detail.getJspFileName(),
- "" + jspLineNumber) +
- "\n\n" + detail.getJspExtract() +
- "\n", realException);
-
- }
- return new SlingException(Localizer.getMessage
- ("jsp.exception", detail.getJspFileName(),
- "" + jspLineNumber), realException);
} catch (final Exception je) {
// If anything goes wrong, just revert to the original behaviour
- if (realException instanceof ServletException) {
- return (ServletException)realException;
- }
- return new SlingException(exMessage, realException);
}
+ if ( result == null ) {
+ result = wrapException(ex);
+ }
+ throw result;
}
/**
