Hi, Sorry for the late reply!
I've changed according to your suggestion, the test fails with a NullPointerException without the fix so at least it will test the previous error. [[[ Index: subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java =================================================================== --- subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java (revision 1902721) +++ subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java (working copy) @@ -86,7 +86,11 @@ */ public String getMessage() { - StringBuffer msg = new StringBuffer(super.getMessage()); + StringBuilder msg = new StringBuilder(); + String message = super.getMessage(); + if (message != null) { + msg.append(message); + } // ### This might be better off in JNIUtil::handleSVNError(). String src = getSource(); if (src != null) Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java =================================================================== --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java (revision 1902721) +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java (working copy) @@ -85,7 +85,11 @@ */ public String getMessage() { - StringBuffer msg = new StringBuffer(super.getMessage()); + StringBuilder msg = new StringBuilder(); + String message = super.getMessage(); + if (message != null) { + msg.append(message); + } // ### This might be better off in JNIUtil::handleSVNError(). String src = getSource(); if (src != null) Index: subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java =================================================================== --- subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java (revision 1902721) +++ subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java (working copy) @@ -27,6 +27,7 @@ import org.apache.subversion.javahl.callback.*; import org.apache.subversion.javahl.remote.*; import org.apache.subversion.javahl.types.*; +import org.apache.subversion.javahl.NativeException; import java.io.File; import java.io.FileOutputStream; @@ -4747,6 +4748,17 @@ } /** + * Test getMessage in NativeException. + * @throws Throwable + */ + public void testGetMessage() throws Throwable + { + /* NativeException with a null message previously threw a NullPointerException */ + assertEquals("", new NativeException(null, null, null, 0).getMessage()); + assertEquals("messagesvn: source: (apr_err=0)", new NativeException("message", "source", null, 0).getMessage());+ } + + /** * @return <code>file</code> converted into a -- possibly * <code>canonical</code>-ized -- Subversion-internal path * representation. Index: subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java =================================================================== --- subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java (revision 1902721) +++ subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java (working copy) @@ -22,6 +22,8 @@ */ package org.tigris.subversion.javahl; +import org.tigris.subversion.javahl.NativeException; + import java.io.File; import java.io.FileOutputStream; import java.io.FileNotFoundException; @@ -3321,6 +3323,17 @@ } /** + * Test getMessage in NativeException. + * @throws Throwable + */ + public void testGetMessage() throws Throwable + { + /* NativeException with a null message previously threw a NullPointerException */ + assertEquals("", new NativeException(null, null, 0).getMessage()); + assertEquals("messagesvn: source: (apr_err=0)", new NativeException("message", "source", 0).getMessage()); + } + + /** * @return <code>file</code> converted into a -- possibly * <code>canonical</code>-ized -- Subversion-internal path * representation. ]]] One last thing: When committing this, I would like to give you due credit [1] as Patch by. Is it ok that I use your name/e-mail this way or do you want it some other way? Kind regards, Daniel Sahlberg [1] https://subversion.apache.org/docs/community-guide/conventions.html#crediting Den mån 18 juli 2022 kl 09:36 skrev Thomas Singer <thomas.sin...@syntevo.com >: > Hi Daniel, > > Thanks for the quick investigation. I would simplify the test: > > > @Test > > public void testGetMessage() { > > assertEquals("", new NativeException(null, null, null, > 0).getMessage()); > > assertEquals("messagesvn: source: )apr_err=0)", new > NativeException("message", "source", null, 0).getMessage()); > > } > > -- > Best regards, > Thomas Singer > > > > On 2022-07-16 22:49, Daniel Sahlberg wrote: > > Den fre 15 juli 2022 kl 23:19 skrev Daniel Sahlberg < > > daniel.l.sahlb...@gmail.com>: > > > >> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer < > >> thomas.sin...@syntevo.com>: > >> > >>> Hi SVN developers, > >>> > >> > >> Hi Thomas, > >> > >> Thanks for the detailed report! > >> > >> > >>> First, there are 2 classes of NativeException, one in > >>> org.tigris.subversion.javahl package, the other in > >>> org.apache.subversion.javahl package. They only slightly differ in a > >>> constructor parameter. I recommend to use one class with 2 constructors > >>> instead. > >>> > >> > >> I'm not experienced enough in the Java world to fully grasp this but I > >> assume the org.tigtis package is older and that it has been kept for > >> historical reasons (not to break an API used by older applications). > >> > >> If you have a suggestion on how to consolidate this without breaking the > >> old API, feel free to send to the list! > >> > >> > >>> > >>> Second, the getMessage() method might throw a NullPointerException in > >>> the StringBuffer constructor if the NativeException was created with a > >>> null message (happened for a user's bug report). > >>> > >> > >> Alright. I've been working on making a test case for this and I believe > >> I've managed to reproduce your issue. I would like to sleep on it and > >> verify once more tomorrow. > >> > >> > >>> Third, the usage of StringBuffer is discouraged in favor of > StringBuilder. > >>> > >> > >> Sounds good. > >> > >> > >>> I recommend to change the code for NativeException from > >>> > >>>> public String getMessage() > >>>> { > >>>> StringBuffer msg = new StringBuffer(super.getMessage()); > >>> > >>> to > >>> > >>>> public String getMessage() > >>>> { > >>>> StringBuilder msg = new StringBuilder(); > >>>> String message = super.getMessage(); > >>>> if (message != null) { > >>>> msg.append(message); > >>>> } > >>> > >> > >> I've implemented the above and I believe this also resolves the failure. > >> > > > > I believe I have everything covered with the patch below. @Thomas Singer > > <thomas.sin...@syntevo.com> Can you check if this looks good and in > > particular if it works for you? If you have the possibility, please also > > run the javahl test suite in your environment to make sure I didn't screw > > up anything else. > > > > dev@: I would prefer to commit this in two separate commits, first the > test > > cases and second the fix. But I could not find any way to "XFail" the > test > > case. If I commit the tests as-is, they will fail. Obviously this will be > > resolved in a second commit a few moments later but I'm reluctant to > > intentionally screwing up the test suite. Is there something I'm missing > or > > should I commit as below? > > > > [[[ > > Index: > > > subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java > > =================================================================== > > --- > > > subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java > > (revision > > 1902721) > > +++ > > > subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java > > (working > > copy) > > @@ -86,7 +86,11 @@ > > */ > > public String getMessage() > > { > > - StringBuffer msg = new StringBuffer(super.getMessage()); > > + StringBuilder msg = new StringBuilder(); > > + String message = super.getMessage(); > > + if (message != null) { > > + msg.append(message); > > + } > > // ### This might be better off in JNIUtil::handleSVNError(). > > String src = getSource(); > > if (src != null) > > Index: > > > subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java > > =================================================================== > > --- > > > subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java > > (revision > > 1902721) > > +++ > > > subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java > > (working > > copy) > > @@ -85,7 +85,11 @@ > > */ > > public String getMessage() > > { > > - StringBuffer msg = new StringBuffer(super.getMessage()); > > + StringBuilder msg = new StringBuilder(); > > + String message = super.getMessage(); > > + if (message != null) { > > + msg.append(message); > > + } > > // ### This might be better off in JNIUtil::handleSVNError(). > > String src = getSource(); > > if (src != null) > > Index: > > > subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java > > =================================================================== > > --- > > > subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java > > (revision > > 1902721) > > +++ > > > subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java > > (working > > copy) > > @@ -27,6 +27,7 @@ > > import org.apache.subversion.javahl.callback.*; > > import org.apache.subversion.javahl.remote.*; > > import org.apache.subversion.javahl.types.*; > > +import org.apache.subversion.javahl.NativeException; > > > > import java.io.File; > > import java.io.FileOutputStream; > > @@ -4747,6 +4748,28 @@ > > } > > > > /** > > + * Test null message in NativeException. > > + * @throws Throwable > > + */ > > + public void testNativeException() throws Throwable > > + { > > + try { > > + /* Throw a NativeException with a null message */ > > + throw new NativeException(null, null, new Throwable(""), > -1); > > + } catch (NativeException ex) { > > + /* Catch the NativeException */ > > + try { > > + /* Try to get the message, which shouldn't throw an > > exception */ > > + ex.getMessage(); > > + } catch (Exception ex2) { > > + assertEquals("ex.getMessage() throw exception", "", > ex2); > > + } > > + } catch (Exception ex) { > > + assertEquals("Unexpected exception", "", ex); > > + } > > + } > > + > > + /** > > * @return <code>file</code> converted into a -- possibly > > * <code>canonical</code>-ized -- Subversion-internal path > > * representation. > > Index: > > > subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java > > =================================================================== > > --- > > > subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java > > (revision > > 1902721) > > +++ > > > subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java > > (working > > copy) > > @@ -22,6 +22,8 @@ > > */ > > package org.tigris.subversion.javahl; > > > > +import org.tigris.subversion.javahl.NativeException; > > + > > import java.io.File; > > import java.io.FileOutputStream; > > import java.io.FileNotFoundException; > > @@ -3321,6 +3323,29 @@ > > } > > > > /** > > + * Test null message in NativeException. > > + * @throws Throwable > > + * @since 1.15 > > + */ > > + public void testNativeException() throws Throwable > > + { > > + try { > > + /* Throw a NativeException with a null message */ > > + throw new NativeException(null, null, -1); > > + } catch (NativeException ex) { > > + /* Catch the NativeException */ > > + try { > > + /* Try to get the message, which shouldn't throw an > > exception */ > > + ex.getMessage(); > > + } catch (Exception ex2) { > > + assertEquals("ex.getMessage() throw exception", "", > ex2); > > + } > > + } catch (Exception ex) { > > + assertEquals("Unexpected exception", "", ex); > > + } > > + } > > + > > + /** > > * @return <code>file</code> converted into a -- possibly > > * <code>canonical</code>-ized -- Subversion-internal path > > * representation. > > ]]] > > > > Kind regards, > > Daniel Sahlberg > > >