Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid collecting stacktrace when building an OgnlException [Fixes OGNL-261] #138

Merged
merged 3 commits into from
Dec 30, 2021
Merged

Avoid collecting stacktrace when building an OgnlException [Fixes OGNL-261] #138

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e46b294
OGNL-261 Avoid collecting stacktrace when building an OgnlException
Nov 18, 2021
6ddf10a
OGNL-261 Avoid collecting stacktrace when building an OgnlException
Nov 19, 2021
65776a0
OGNL-261 Avoid collecting stacktrace when building an OgnlException
Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 13 additions & 75 deletions src/main/java/ognl/OgnlException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@
//--------------------------------------------------------------------------
package ognl;

import java.lang.reflect.Method;


/**
* Superclass for OGNL exceptions, incorporating an optional encapsulated exception.
*
Expand All @@ -41,27 +38,11 @@
*/
public class OgnlException extends Exception
{
// cache initCause method - if available..to be used during throwable constructor
// to properly setup superclass.

static Method _initCause;
static {
try {
_initCause = OgnlException.class.getMethod("initCause", new Class[] { Throwable.class});
} catch (NoSuchMethodException e) { /** ignore */ }
}

/**
/**
* The root evaluation of the expression when the exception was thrown
*/
private Evaluation _evaluation;

/**
* Why this exception was thrown.
* @serial
*/
private Throwable _reason;

/** Constructs an OgnlException with no message or encapsulated exception. */
public OgnlException()
{
Expand All @@ -84,15 +65,16 @@ public OgnlException( String msg )
*/
public OgnlException( String msg, Throwable reason )
{
super( msg );
this._reason = reason;
super( msg , reason, true, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be controllable from outside?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's was the point of my question about the way to control this behavior change using whatever configuration mechanism is available.
Sorry that I was not clear enough. :-/

Personally, I would add a java prop and initialize a static boolean in OgnlContext (mimicking what's already done) to allow the user to control whether the stacktraces are collected when throwing OgnlExceptions or not.
This would allow to cope for unexpected situations when the previous behavior is actually required.

However, since OgnlContext is not already provided in any of the OgnlExceptionconstructor methods, this would stay a JVM-wide setting (meaning you would not be able to control this per OGNL evaluation).
Adding the OgnlContext and changing all code paths throwing this exception looks overkill to me.

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that you can decide when creating the exception to collect or not the stack trace. I would just add another constructor and expose these true, falses and that's all :)

Then we can extend your idea and use OgnlContext to control if the exception should be created with stack trace or note (delegate the control out of exception itself).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Indeed, we can add the signature from java.lang.Exception.Exception(String, Throwable, boolean, boolean)
No big deal, I'll do it right away.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I'll leave this new constructor protected as this is intended to be used by sub-classes and not by code path throwing the exception - see java.lang.Throwable.Throwable(String, Throwable, boolean, boolean).
Makes sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and pushed.

}

if (_initCause != null)
{
try {
_initCause.invoke(this, new Object[] { reason });
} catch (Exception t) { /** ignore */ }
}
/**
* Constructs an OgnlException with the given message and encapsulated exception,
* with control on exception suppression and stack trace collection.
* See {@code java.lang.Throwable.Throwable(String, Throwable, boolean, boolean)} for more info.
*/
protected OgnlException(String message, Throwable reason, boolean enableSuppression, boolean writableStackTrace) {
super(message, reason, enableSuppression, writableStackTrace);
}

/**
Expand All @@ -101,7 +83,7 @@ public OgnlException( String msg, Throwable reason )
*/
public Throwable getReason()
{
return _reason;
return getCause();
}

/**
Expand Down Expand Up @@ -130,53 +112,9 @@ public void setEvaluation(Evaluation value)
*/
public String toString()
{
if ( _reason == null )
if ( getCause() == null )
return super.toString();

return super.toString() + " [" + _reason + "]";
}


/**
* Prints the stack trace for this (and possibly the encapsulated) exception on
* System.err.
*/
public void printStackTrace()
{
printStackTrace( System.err );
}

/**
* Prints the stack trace for this (and possibly the encapsulated) exception on the
* given print stream.
*/
public void printStackTrace(java.io.PrintStream s)
{
synchronized (s)
{
super.printStackTrace(s);
if ( _reason != null ) {
s.println( "/-- Encapsulated exception ------------\\" );
_reason.printStackTrace(s);
s.println( "\\--------------------------------------/" );
}
}
}

/**
* Prints the stack trace for this (and possibly the encapsulated) exception on the
* given print writer.
*/
public void printStackTrace(java.io.PrintWriter s)
{
synchronized (s)
{
super.printStackTrace(s);
if ( _reason != null ) {
s.println( "/-- Encapsulated exception ------------\\" );
_reason.printStackTrace(s);
s.println( "\\--------------------------------------/" );
}
}
return super.toString() + " [" + getCause() + "]";
}
}