Not sure that I agree with this not being a functional change: The null check used to be on cause.getCauseProxy(), now it is on cause.
About unit tests: I believe the problem scenario is when the Throwable class does not exist on the process that deserialized the ThrowableProxy. It is difficult to create a test for this. If I'm not mistaken though this is one of the main use cases for throwable proxy so we cannot ignore it. On Wednesday, December 3, 2014, Gary Gregory <[email protected]> wrote: > Yes: This is a change that refactors guard clauses, no behavioral change. > Tests pass. > No: I do not know if the specific case you mention is covered by the unit > tests or the code for that matter. The tests do pass though. > Thoughts? > > Gary > > On Tue, Dec 2, 2014 at 5:01 PM, Remko Popma <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> Are you sure this is correct? >> What about deserialized ThrowableProxies where the cause is null but the >> causeProxy is non-null? >> >> Sent from my iPhone >> >> > On 2014/12/03, at 3:10, [email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');> wrote: >> > >> > Repository: logging-log4j2 >> > Updated Branches: >> > refs/heads/master ab58a6d69 -> df71c934d >> > >> > >> > Let the formatCause method have a null guard clause instead of each call >> > site guarding the call. >> > >> > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo >> > Commit: >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/df71c934 >> > Tree: >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/df71c934 >> > Diff: >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/df71c934 >> > >> > Branch: refs/heads/master >> > Commit: df71c934d59570bbee10d8eb413d92eaa443bec4 >> > Parents: ab58a6d >> > Author: Gary Gregory <[email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');>> >> > Authored: Tue Dec 2 13:10:10 2014 -0500 >> > Committer: Gary Gregory <[email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');>> >> > Committed: Tue Dec 2 13:10:10 2014 -0500 >> > >> > ---------------------------------------------------------------------- >> > .../apache/logging/log4j/core/impl/ThrowableProxy.java | 11 >> +++++------ >> > 1 file changed, 5 insertions(+), 6 deletions(-) >> > ---------------------------------------------------------------------- >> > >> > >> > >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/df71c934/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > ---------------------------------------------------------------------- >> > diff --git >> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > index 3aadf6c..23065ab 100644 >> > --- >> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > +++ >> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > @@ -183,12 +183,13 @@ public class ThrowableProxy implements >> Serializable { >> > >> > @SuppressWarnings("ThrowableResultOfMethodCallIgnored") >> > private void formatCause(final StringBuilder sb, final >> ThrowableProxy cause, final List<String> ignorePackages) { >> > + if (cause == null) { >> > + return; >> > + } >> > sb.append("Caused by: ").append(cause).append(EOL); >> > this.formatElements(sb, cause.commonElementCount, >> cause.getThrowable().getStackTrace(), >> > cause.extendedStackTrace, ignorePackages); >> > - if (cause.getCauseProxy() != null) { >> > - this.formatCause(sb, cause.causeProxy, ignorePackages); >> > - } >> > + this.formatCause(sb, cause.causeProxy, ignorePackages); >> > } >> > >> > private void formatElements(final StringBuilder sb, final int >> commonCount, final StackTraceElement[] causedTrace, >> > @@ -342,9 +343,7 @@ public class ThrowableProxy implements Serializable >> { >> > } >> > sb.append('\n'); >> > this.formatElements(sb, 0, this.throwable.getStackTrace(), >> this.extendedStackTrace, ignorePackages); >> > - if (this.causeProxy != null) { >> > - this.formatCause(sb, this.causeProxy, ignorePackages); >> > - } >> > + this.formatCause(sb, this.causeProxy, ignorePackages); >> > return sb.toString(); >> > } >> > >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');> >> For additional commands, e-mail: [email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');> >> >> > > > -- > E-Mail: [email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | [email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');> > Java Persistence with Hibernate, Second Edition > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory >
