You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Thomas Singer <th...@syntevo.com> on 2022/07/15 08:46:12 UTC
JavaHL problem with NativeException
Hi SVN developers,
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.
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).
Third, the usage of StringBuffer is discouraged in favor of StringBuilder.
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);
> }
--
Best regards,
Thomas Singer
=============
syntevo GmbH
www.syntevo.com
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
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.singer@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.sahlberg@gmail.com>:
> >
> >> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
> >> thomas.singer@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
> > <th...@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
> >
>
Re: JavaHL problem with NativeException
Posted by Thomas Singer <th...@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.sahlberg@gmail.com>:
>
>> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
>> thomas.singer@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
> <th...@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
>
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
Den lör 16 juli 2022 kl 23:11 skrev James McCoy <ja...@jamessan.com>:
> On Sat, Jul 16, 2022 at 10:49:50PM +0200, Daniel Sahlberg wrote:
> > 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.
>
> You can use junit's Ignore annotation.
>
> [[[
> import org.junit.Ignore;
> ...
> @Ignore
> public void testNativeException() throws Throwable
> ]]]
>
> You can include a reason, too, if needed -- @Ignore("..."). There's no
> XFail, that I know of, but this will at least prevent it from running.
>
Thanks! However @Ignore (as I understand it) would sort of defeat the
reason for committing the test separately - my idea being that it should be
possible to checkout revision X and see that a testcase fails as expected
and then update to revision Y (with the fix) and see that the testcase now
succeeds.
I believe I will commit it all in one single commit to avoid leaving /trunk
in a broken state (even for just a few moments).
Kind regards,
Daniel
Re: JavaHL problem with NativeException
Posted by James McCoy <ja...@jamessan.com>.
On Sat, Jul 16, 2022 at 10:49:50PM +0200, Daniel Sahlberg wrote:
> 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.
You can use junit's Ignore annotation.
[[[
import org.junit.Ignore;
...
@Ignore
public void testNativeException() throws Throwable
]]]
You can include a reason, too, if needed -- @Ignore("..."). There's no
XFail, that I know of, but this will at least prevent it from running.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 15 juli 2022 kl 23:19 skrev Daniel Sahlberg <
daniel.l.sahlberg@gmail.com>:
> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
> thomas.singer@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
<th...@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
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 29 juli 2022 kl 16:29 skrev Branko Čibej <br...@apache.org>:
> On 27.07.2022 22:18, Daniel Sahlberg wrote:
>
> Den tis 26 juli 2022 kl 10:55 skrev Branko Čibej <br...@apache.org>:
>
>> On 15.07.2022 23:19, Daniel Sahlberg wrote:
>> > Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer
>> > <th...@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).
>>
>> The org.tigris package is obsolete but indeed kept for compatibility. We
>> shouldn't even try to "improve" it by using some common implementation.
>>
>
> When compiling, there are a bunch of warnings:
>
> [[[
> ...
> /home/dsg/svn_trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java:1605:
> warning: [dep-ann] deprecated item is not annotated with @Deprecated
> public void diff(String target1, Revision revision1, String target2,
> ...
> ]]]
>
> I assume this is because the function is @deprecated in the docstring but
> not formally annotated as @Deprecated.
>
> Would this be a reasonable improvement or should the code be considered
> frozen?
>
>
> It's frozen, don't bother with it. Nobody should be using it any longer.
>
Still it causes compile time warnings that could mask other warnings.
/Daniel
Re: JavaHL problem with NativeException
Posted by Branko Čibej <br...@apache.org>.
On 27.07.2022 22:18, Daniel Sahlberg wrote:
> Den tis 26 juli 2022 kl 10:55 skrev Branko Čibej <br...@apache.org>:
>
> On 15.07.2022 23:19, Daniel Sahlberg wrote:
> > Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer
> > <th...@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).
>
> The org.tigris package is obsolete but indeed kept for
> compatibility. We
> shouldn't even try to "improve" it by using some common
> implementation.
>
>
> When compiling, there are a bunch of warnings:
>
> [[[
> ...
> /home/dsg/svn_trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java:1605:
> warning: [dep-ann] deprecated item is not annotated with @Deprecated
> public void diff(String target1, Revision revision1, String target2,
> ...
> ]]]
>
> I assume this is because the function is @deprecated in the docstring
> but not formally annotated as @Deprecated.
>
> Would this be a reasonable improvement or should the code be
> considered frozen?
It's frozen, don't bother with it. Nobody should be using it any longer.
-- Brane
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 26 juli 2022 kl 10:55 skrev Branko Čibej <br...@apache.org>:
> On 15.07.2022 23:19, Daniel Sahlberg wrote:
> > Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer
> > <th...@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).
>
> The org.tigris package is obsolete but indeed kept for compatibility. We
> shouldn't even try to "improve" it by using some common implementation.
>
When compiling, there are a bunch of warnings:
[[[
...
/home/dsg/svn_trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java:1605:
warning: [dep-ann] deprecated item is not annotated with @Deprecated
public void diff(String target1, Revision revision1, String target2,
...
]]]
I assume this is because the function is @deprecated in the docstring but
not formally annotated as @Deprecated.
Would this be a reasonable improvement or should the code be considered
frozen?
Kind regards,
Daniel Sahlberg
Re: JavaHL problem with NativeException
Posted by Branko Čibej <br...@apache.org>.
On 15.07.2022 23:19, Daniel Sahlberg wrote:
> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer
> <th...@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).
The org.tigris package is obsolete but indeed kept for compatibility. We
shouldn't even try to "improve" it by using some common implementation.
>
>
> Third, the usage of StringBuffer is discouraged in favor of
> StringBuilder.
>
>
> Sounds good.
StringBuilder didn't exist when the code was written. :)
-- Brane
Re: JavaHL problem with NativeException
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <thomas.singer@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.
Kind regards,
Daniel Sahlberg