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 <[email protected]
>:
> 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 <
> > [email protected]>:
> >
> >> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
> >> [email protected]>:
> >>
> >>> 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
> > <[email protected]> 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
> >
>
