Re: svn commit: r19803 - in trunk/subversion/bindings/java/javahl: native src/org/tigris/subversion/javahl

On Wed, 24 May 2006, Mark Phippard wrote:

> Daniel Rall <dlr@collab.net> wrote on 05/24/2006 04:19:25 PM:
>
> > On a related note, here at CollabNet we were wondering if the
> > SVNClientInterface.username() and password() Java methods should even
> > accept null input, or should instead throw IllegalArgumentException
> > [1]. Setting a null username seems like a pretty bogus operation to
> > me, setting a null password perhaps less so. Thoughts?
>
> In Subclipse we use the callback interface, PromptUserPassword, and I
> remember wondering if I should set username/password to null to be clear I
> wanted the callback. I guess I do not do that since the JVM does not
> crash. That being said, receiving an exception would have likely answered
> my question the first time I ran it.

Mark, David, thanks for your feedback. How about this patch?

[[[
Disallow use of null arguments to the client credential portion of the
JavaHL API (which previous to r19803 resulted in a segfault and thus a
hard JVM crash), instead throwing an IllegalArgumentException.

[ in subversion/bindings/java/javahl/ ]

* native/JNIUtil.h
  (isExceptionThrown): Update doc string for slight change in
   behavior, where any Throwable set this flag (rather than solely
   JNIError).

  (raiseThrowable): New function to create and throw an arbitrary
   java.lang.Throwable instance.

  (throwError): Inlined implementation, which now delegates to
   raiseThrowable().

* native/JNIUtil.cpp
  (raiseThrowable): Refactored old throwError() function to be more
   generic.

* native/org_tigris_subversion_javahl_SVNClient.cpp
  (Java_org_tigris_subversion_javahl_SVNClient_username,
   Java_org_tigris_subversion_javahl_SVNClient_password): Throw an
   IllegalArgumentException when a null argument is passed in.

* src/org/tigris/subversion/javahl/SVNClientInterface.java
  (username, password): Update JavaDoc.

Review by: djames
           markphip
]]]

Index: /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.h
===================================================================
--- /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.h (revision 19805)
+++ /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.h (working copy)
@@ -59,7 +59,12 @@
     static bool isJavaExceptionThrown();
     static JNIEnv * getEnv();
     static void setEnv(JNIEnv *);
+
+ /**
+ * @return Whether any Throwable has been raised.
+ */
     static bool isExceptionThrown();
+
     static void handleAPRError(int error, const char *op);
 
     /**
@@ -79,7 +84,24 @@
 
     static void handleSVNError(svn_error_t *err);
     static jstring makeSVNErrorMessage(svn_error_t *err);
- static void throwError(const char *message);
+
+ /**
+ * Create and throw a java.lang.Throwable instance.
+ *
+ * @param name The class name (in path form, with slashes in lieu
+ * of dots) of the Throwable to create and raise.
+ * @param message The message text of the Throwable.
+ */
+ static void raiseThrowable(const char *name, const char *message);
+
+ /**
+ * Creates and throws a JNIError.
+ *
+ * @param message The message text of the JNIError.
+ */
+ static void throwError(const char *message)
+ { raiseThrowable(JAVA_PACKAGE"/JNIError", message); }
+
     static apr_pool_t * getPool();
         static bool JNIGlobalInit(JNIEnv *env);
     static bool JNIInit(JNIEnv *env);
Index: /home/dlr/src/subversion/subversion/bindings/java/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp
===================================================================
--- /home/dlr/src/subversion/subversion/bindings/java/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (revision 19805)
+++ /home/dlr/src/subversion/subversion/bindings/java/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (working copy)
@@ -241,6 +241,12 @@
         JNIUtil::throwError(_("bad c++ this"));
         return;
     }
+ if (jusername == NULL)
+ {
+ JNIUtil::raiseThrowable("java/lang/IllegalArgumentException",
+ _("Provide a username (null is not supported)"));
+ return;
+ }
     JNIStringHolder username(jusername);
     if(JNIUtil::isExceptionThrown())
     {
@@ -264,6 +270,12 @@
         JNIUtil::throwError(_("bad c++ this"));
         return;
     }
+ if (jpassword == NULL)
+ {
+ JNIUtil::raiseThrowable("java/lang/IllegalArgumentException",
+ _("Provide a password (null is not supported)"));
+ return;
+ }
     JNIStringHolder password(jpassword);
     if(JNIUtil::isExceptionThrown())
     {
Index: /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.cpp
===================================================================
--- /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.cpp (revision 19805)
+++ /home/dlr/src/subversion/subversion/bindings/java/javahl/native/JNIUtil.cpp (working copy)
@@ -303,20 +303,16 @@
 {
     return g_globalPoolMutext;
 }
-/**
- * throw an error
- * @param message the message text of the error
- */
-void JNIUtil::throwError(const char *message)
+void JNIUtil::raiseThrowable(const char *name, const char *message)
 {
     if (getLogLevel() >= errorLog)
     {
         JNICriticalSection cs(*g_logMutex);
- g_logStream << "Error thrown <" << message << ">" << std::endl;
+ g_logStream << "Throwable raised <" << message << ">" << std::endl;
     }
     JNIEnv *env = getEnv();
- jclass clazz = env->FindClass(JAVA_PACKAGE"/JNIError");
- if(isJavaExceptionThrown())
+ jclass clazz = env->FindClass(name);
+ if (isJavaExceptionThrown())
     {
         return;
     }
Index: /home/dlr/src/subversion/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java
===================================================================
--- /home/dlr/src/subversion/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java (revision 19805)
+++ /home/dlr/src/subversion/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java (working copy)
@@ -125,13 +125,19 @@
     Status singleStatus(String path, boolean onServer) throws ClientException;
     /**
      * Sets the username used for authentication.
- * @param username The username (<code>null</code> means none).
+ * @param username The username, ignored if the empty string. Set
+ * to the empty string to clear it.
+ * @throws IllegalArgumentException If <code>username</code> is
+ * <code>null</code>.
      * @see #password(String)
      */
     void username(String username);
     /**
      * Sets the password used for authentication.
- * @param password The password (<code>null</code> means none).
+ * @param password The password, ignored if the empty string. Set
+ * to the empty string to clear it.
+ * @throws IllegalArgumentException If <code>password</code> is
+ * <code>null</code>.
      * @see #username(String)
      */
     void password(String password);