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

[davoustp]

No description provided.

[lukaszlenart]

@harawata @quaff @JCgH4164838Gh792C124B5 could you take a look on this PR?
I opt to merge this, here you can find more details.

[harawata]

Hello @davoustp @lukaszlenart !

Does this mean we lose some information that can be useful when investigating an issue?
Could you post a test code so that I can verify the difference before and after the change?

[lukaszlenart]

Shouldn't this be controllable from outside?

[davoustp]

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?

[lukaszlenart]

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).

[davoustp]

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.

[davoustp]

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?

[lukaszlenart]

Sure, makes sense 👍

[davoustp]

Done and pushed.

[davoustp]

Hi @harawata

Does this mean we lose some information that can be useful when investigating an issue?

It depends.

If a cause exception is provided when building the OgnlException, then no, you won't loose any info since this cause exception is very likely to have a stacktrace collected (as this is the default for any Throwable), so you'll have the full code path that leads to the cause exception.

When raised by OnglRuntime in the case described by OGNL-261 , then yes, you don't have the stack trace anymore.
That's a consequence of using exceptions in nominal code path to signal a non-error condition (in this case, property could not be found while evaluating a compound statement).

If this is a problem, then that's why I asked if a configuration switch would be required (on or off by default, whatever you think is best - either maintain the previous behavior or go for the new behavior by default).

Could you post a test code so that I can verify the difference before and after the change?

You can use ognl.TestOgnlException.test_Throwable_Reason() and add a System.out.println(e.toString()); statement to see how that goes when a cause exception is provided.

There is currently no test around OgnlExceptionwhen no cause is provided.

[davoustp]

BTW, I'm wondering about the two ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods:
the super.printStackTrace(s); invocation already prints the stack trace with the cause and suppressed exception stacktraces if any already.
Any reason to keep these methods (this currently actually prints the cause exception twice) ?

[davoustp]

Any reason to keep these methods (this currently actually prints the cause exception twice) ?

Any objection against removal of these ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods?

[lukaszlenart]

Any objection against removal of these ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods?

I think it's ok to remove them

[davoustp]

I think it's ok to remove them

Done.

[lukaszlenart]

I will release this changes as 3.4.x and also officially announce switching to Java 8 (I thought I already did that)

[harawata]

Thank you for the explanation @davoustp ,
I may not fully understand the impact of this change, but it seems to be the case that the current implementation has a problem, so I have no objection.

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart , @davoustp , and @harawata .

Thanks for including me in the discussion. :)

Also, thanks for the detailed explanation behind the changes, @davoustp .

After reviewing the changes for the PR, it looks like maybe some of the old code logic might have been intended to handle pre-JDK 1.4 conditions, but I could be mistaken ?

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace. If someone is relying on that extra information for diagnostics, then I think that they would have to modify their code to check if there is a non-null cause/reason, and print that stacktrace themselves when the OgnlException is caught. Probably an edge case, but you never know ...

It looks like the changes preserve the exception reason/cause via the "normal" Exception constructor call sequence, so that information appears to be preserved.

Other than that, there are two additional comments related to the discussion above:

1. Should an issue be opened against this GitHub repository to "match" OGNL-261, since this repository is somewhat distinct from commons-ognl ?

2. Did @lukaszlenart mean to mention 3.3.x (instead of 3.4.x) for a new OGNL release to include this change ? If so, does that mean he might create a new ognl-3-2-x branch as well (to separate out future main branch changes) ?

[davoustp]

Also, thanks for the detailed explanation behind the changes, @davoustp .

You're welcome. The better a change is understood, the more accepted it gets, in my opinion. ;-)

After reviewing the changes for the PR, it looks like maybe some of the old code logic might have been intended to handle pre-JDK 1.4 conditions, but I could be mistaken ?

I suppose you refer to the initCause() invocation - if yes, then I'd agree with you, this is my take on this as well: this looks like pre-JDK 1.4 exception chaining support.
As mentioned in the Throwable javadoc :

An example of using this method on a legacy throwable type without other support for setting the cause...

About:

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace.

I'm not sure what you mean here.
From the JDK 11 source code for java.lang.Throwable.printStackTrace(PrintStreamOrWriter), it first prints the exception (basically invoking toString()), then prints the stacktrace of this exception if any, moves on to print stacktraces of suppressed exceptions if any, and finally prints the stacktrace of the cause Throwable if any.

I don't see any way to have this method to "skip" printing the cause Throwable's stacktrace (if it has one), even if the OgnlException has no stack trace...
Did I overlook something?

[lukaszlenart]

1. Should an issue be opened against this GitHub repository to "match" OGNL-261, since this repository is somewhat distinct from commons-ognl ?

Yeah, it would be good to address this split personality and just keep one OGNL repository - I was planning move some changes from this repo into commons-ognl but this looks like overwhelming task, also preparing a release under ASF Commons is more complicated than here. So I was thinking about closing commons-ognl and keep supporting future of OGNL only here.

2. Did @lukaszlenart mean to mention 3.3.x (instead of 3.4.x) for a new OGNL release to include this change ? If so, does that mean he might create a new ognl-3-2-x branch as well (to separate out future main branch changes) ?

Yes & no - I missed that I had already switched to Java 8 but didn't push a new release, a few days ago I have released a new 3.3.0 version with Java 8 requirement and some fix. This change can be included in 3.3.1 or as a breaking change it can be included in 3.4.x. And I don't want to create dedicated branches if not needed, it's very hard to keep them in sync.

[JCgH4164838Gh792C124B5]

Hi @davoustp .

About:

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace.

I'm not sure what you mean here. From the JDK 11 source code for java.lang.Throwable.printStackTrace(PrintStreamOrWriter), it first prints the exception (basically invoking toString()), then prints the stacktrace of this exception if any, moves on to print stacktraces of suppressed exceptions if any, and finally prints the stacktrace of the cause Throwable if any.

I don't see any way to have this method to "skip" printing the cause Throwable's stacktrace (if it has one), even if the OgnlException has no stack trace... Did I overlook something?

Ooops ... I missed the fact that the standard Throwable.printStackTrace() should print out the stacktrace of a non-null cause as well. That same behaviour shows in the JDK 8 source code as well. Thanks for pointing that out. :)

Since your changes set the cause (reason) in the constructor, the same information should be available, I think. With that in mind, only if someone is parsing logs for the old delimiters (e.g. "/-- Encapsulated exception ------------\\"), does it seem there might be a (probably very) small chance for an impact.

Thanks for your work on this PR, @davoustp. After the discussion, it looks good to me. 👍

[davoustp]

@JCgH4164838Gh792C124B5 you're welcome!
Glad I could help. :-)

[lukaszlenart]

LGTM and released, thanks @davoustp @harawata and @JCgH4164838Gh792C124B5 for the review and discussion 💪