Merge 'bindings/java: Enhance exception handling logic' from Kim Seon…

… Woo ### Purpose of this PR - Enhance exception handling logic - When exceptions has to be thrown from Rust to Java, let's just return the error message directly. - Removes JNI call to get error message using `Java_org_github_tursodatabase_core_LimboDB_getErrorMessageUtf8` - Add `throwJavaException` to assure that the exception throwing logic works corretly Closes #642
tursodatabase · Jan 10, 2025 · 93e4b8d · 93e4b8d
2 parents 5c38cc8 + 90258a4
commit 93e4b8d
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 56 deletions.
diff --git a/bindings/java/rs_src/limbo_db.rs b/bindings/java/rs_src/limbo_db.rs
@@ -50,18 +50,34 @@ pub extern "system" fn Java_org_github_tursodatabase_core_LimboDB__1open_1utf8<'
     Box::into_raw(Box::new(db)) as jlong
 }
 
+#[no_mangle]
+pub extern "system" fn Java_org_github_tursodatabase_core_LimboDB_throwJavaException<'local>(
+    mut env: JNIEnv<'local>,
+    obj: JObject<'local>,
+    error_code: jint,
+) {
+    set_err_msg_and_throw_exception(
+        &mut env,
+        obj,
+        error_code,
+        "throw java exception".to_string(),
+    );
+}
+
 fn set_err_msg_and_throw_exception<'local>(
     env: &mut JNIEnv<'local>,
     obj: JObject<'local>,
     err_code: i32,
     err_msg: String,
 ) {
-    let error_message_pointer = Box::into_raw(Box::new(err_msg)) as i64;
+    let error_message_bytes = env
+        .byte_array_from_slice(err_msg.as_bytes())
+        .expect("Failed to convert to byte array");
     match env.call_method(
         obj,
-        "newSQLException",
-        "(IJ)Lorg/github/tursodatabase/exceptions/LimboException;",
-        &[err_code.into(), error_message_pointer.into()],
+        "throwLimboException",
+        "(I[B)V",
+        &[err_code.into(), (&error_message_bytes).into()],
     ) {
         Ok(_) => {
             // do nothing because above method will always return Err
@@ -71,16 +87,3 @@ fn set_err_msg_and_throw_exception<'local>(
         }
     }
 }
-
-#[no_mangle]
-pub unsafe extern "system" fn Java_org_github_tursodatabase_core_LimboDB_getErrorMessageUtf8<
-    'local,
->(
-    env: JNIEnv<'local>,
-    _obj: JObject<'local>,
-    error_message_ptr: jlong,
-) -> JByteArray<'local> {
-    let error_message = Box::from_raw(error_message_ptr as *mut String);
-    let error_message_bytes = error_message.as_bytes();
-    env.byte_array_from_slice(error_message_bytes).unwrap()
-}
diff --git a/bindings/java/src/main/java/org/github/tursodatabase/NativeInvocation.java b/bindings/java/src/main/java/org/github/tursodatabase/NativeInvocation.java
@@ -9,7 +9,7 @@
 /**
  * Annotation to mark methods that are called by native functions.
  */
-@Retention(RetentionPolicy.RUNTIME)
+@Retention(RetentionPolicy.SOURCE)
 @Target(ElementType.METHOD)
 public @interface NativeInvocation {
 }
diff --git a/bindings/java/src/main/java/org/github/tursodatabase/VisibleForTesting.java b/bindings/java/src/main/java/org/github/tursodatabase/VisibleForTesting.java
@@ -0,0 +1,14 @@
+package org.github.tursodatabase;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Annotation to mark methods that use larger visibility for testing purposes.
+ */
+@Retention(RetentionPolicy.SOURCE)
+@Target(ElementType.METHOD)
+public @interface VisibleForTesting {
+}
diff --git a/bindings/java/src/main/java/org/github/tursodatabase/core/AbstractDB.java b/bindings/java/src/main/java/org/github/tursodatabase/core/AbstractDB.java
@@ -172,35 +172,4 @@ public final synchronized long executeUpdate(CoreStatement stmt, Object[] vals)
         // TODO: add implementation
         throw new SQLFeatureNotSupportedException();
     }
-
-    /**
-     * Throws SQL Exception with error code.
-     *
-     * @param errorCode Error code to be passed.
-     * @throws SQLException Formatted SQLException with error code
-     */
-    @NativeInvocation
-    private LimboException newSQLException(int errorCode, long errorMessagePointer) throws SQLException {
-        throw newSQLException(errorCode, getErrorMessage(errorMessagePointer));
-    }
-
-    /**
-     * Throws formatted SQLException with error code and message.
-     *
-     * @param errorCode    Error code to be passed.
-     * @param errorMessage throw newSQLException(errorCode);Error message to be passed.
-     * @return Formatted SQLException with error code and message.
-     */
-    public static LimboException newSQLException(int errorCode, String errorMessage) {
-        LimboErrorCode code = LimboErrorCode.getErrorCode(errorCode);
-        String msg;
-        if (code == LimboErrorCode.UNKNOWN_ERROR) {
-            msg = String.format("%s:%s (%s)", code, errorCode, errorMessage);
-        } else {
-            msg = String.format("%s (%s)", code, errorMessage);
-        }
-        return new LimboException(msg, code);
-    }
-
-    protected abstract String getErrorMessage(long errorMessagePointer);
 }
diff --git a/bindings/java/src/main/java/org/github/tursodatabase/core/LimboDB.java b/bindings/java/src/main/java/org/github/tursodatabase/core/LimboDB.java
@@ -2,6 +2,9 @@
 
 
 import org.github.tursodatabase.LimboErrorCode;
+import org.github.tursodatabase.NativeInvocation;
+import org.github.tursodatabase.VisibleForTesting;
+import org.github.tursodatabase.exceptions.LimboException;
 
 import java.nio.charset.StandardCharsets;
 import java.sql.SQLException;
@@ -30,8 +33,7 @@ public final class LimboDB extends AbstractDB {
     // url example: "jdbc:sqlite:{fileName}
 
     /**
-     *
-     * @param url e.g. "jdbc:sqlite:fileName
+     * @param url      e.g. "jdbc:sqlite:fileName
      * @param fileName e.g. path to file
      */
     public static LimboDB create(String url, String fileName) throws SQLException {
@@ -83,7 +85,7 @@ public synchronized int _exec(String sql) throws SQLException {
     @Override
     protected void _open(String fileName, int openFlags) throws SQLException {
         if (isOpen) {
-            throw newSQLException(LimboErrorCode.UNKNOWN_ERROR.code, "Already opened");
+            throwLimboException(LimboErrorCode.UNKNOWN_ERROR.code, "Already opened");
         }
         dbPtr = _open_utf8(stringToUtf8ByteArray(fileName), openFlags);
         isOpen = true;
@@ -103,12 +105,38 @@ protected synchronized SafeStmtPtr prepare(String sql) throws SQLException {
     @Override
     public synchronized native int step(long stmt);
 
-    @Override
-    protected String getErrorMessage(long errorMessagePointer) {
-        return utf8ByteBufferToString(getErrorMessageUtf8(errorMessagePointer));
+    @VisibleForTesting
+    native void throwJavaException(int errorCode) throws SQLException;
+
+    /**
+     * Throws formatted SQLException with error code and message.
+     *
+     * @param errorCode         Error code.
+     * @param errorMessageBytes Error message.
+     */
+    @NativeInvocation
+    private void throwLimboException(int errorCode, byte[] errorMessageBytes) throws SQLException {
+        String errorMessage = utf8ByteBufferToString(errorMessageBytes);
+        throwLimboException(errorCode, errorMessage);
     }
 
-    private native byte[] getErrorMessageUtf8(long errorMessagePointer);
+    /**
+     * Throws formatted SQLException with error code and message.
+     *
+     * @param errorCode    Error code.
+     * @param errorMessage Error message.
+     */
+    public void throwLimboException(int errorCode, String errorMessage) throws SQLException {
+        LimboErrorCode code = LimboErrorCode.getErrorCode(errorCode);
+        String msg;
+        if (code == LimboErrorCode.UNKNOWN_ERROR) {
+            msg = String.format("%s:%s (%s)", code, errorCode, errorMessage);
+        } else {
+            msg = String.format("%s (%s)", code, errorMessage);
+        }
+
+        throw new LimboException(msg, code);
+    }
 
     private static String utf8ByteBufferToString(byte[] buffer) {
         if (buffer == null) {

diff --git a/bindings/java/src/test/java/org/github/tursodatabase/core/LimboDBTest.java b/bindings/java/src/test/java/org/github/tursodatabase/core/LimboDBTest.java
@@ -1,10 +1,13 @@
 package org.github.tursodatabase.core;
 
+import org.github.tursodatabase.LimboErrorCode;
 import org.github.tursodatabase.TestUtils;
+import org.github.tursodatabase.exceptions.LimboException;
 import org.junit.jupiter.api.Test;
 
 import java.sql.SQLException;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class LimboDBTest {
@@ -26,4 +29,20 @@ void should_throw_exception_when_opened_twice() throws Exception {
 
         assertThatThrownBy(() -> db.open(0)).isInstanceOf(SQLException.class);
     }
+
+    @Test
+    void throwJavaException_should_throw_appropriate_java_exception() throws Exception {
+        String dbPath = TestUtils.createTempFile();
+        LimboDB db = LimboDB.create("jdbc:sqlite:" + dbPath, dbPath);
+        db.load();
+
+        final int limboExceptionCode = LimboErrorCode.ETC.code;
+        try {
+            db.throwJavaException(limboExceptionCode);
+        } catch (Exception e)  {
+            assertThat(e).isInstanceOf(LimboException.class);
+            LimboException limboException = (LimboException) e;
+            assertThat(limboException.getResultCode().code).isEqualTo(limboExceptionCode);
+        }
+    }
 }