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
