Refactor/jni (#116)

* refactor(jni): functions reordering

* refactor(jni): modif throw_exception macro

* refactor(jni): modifying visibility of key expr functions

* refactor(jni): logger.rs

* refactor(jni): queryable refactor

* refactor(jni): key expr declaration

* refactor(jni): moving reply logic below getViaJNI function

* refactor(jni): fix session ptr being liberated in case of publisher declaration error

* refactor(jni): fixes after cargo update + creating ZenohID class

* refactor(jni): using macros for errors

* refactor(jni): using AutoLocal instead of manually deleting local references

* refactor(jni): key expr inner code refactor

* refactor(jni): tidying up jni comments

* refactor(jni): cargo fmt + clippy

zenoh-jni/src/errors.rs

@@ -18,10 +18,41 @@ use jni::JNIEnv;
 
 #[macro_export]
 macro_rules! throw_exception {
-    ($env:expr, $err:expr) => {
-        $err.throw_on_jvm(&mut $env).map_err(|err| {
+    ($env:expr, $err:expr) => {{
+        let _ = $err.throw_on_jvm(&mut $env).map_err(|err| {
             tracing::error!("Unable to throw exception: {}", err);
-        })
+        });
+    }};
+}
+
+#[macro_export]
+macro_rules! jni_error {
+    ($arg:expr) => {
+        Error::Jni($arg.to_string())
+    };
+    ($fmt:expr, $($arg:tt)*) => {
+        Error::Jni(format!($fmt, $($arg)*))
+    };
+}
+
+#[macro_export]
+macro_rules! session_error {
+    ($arg:expr) => {
+        Error::Session($arg.to_string())
+    };
+    ($fmt:expr, $($arg:tt)*) => {
+        Error::Session(format!($fmt, $($arg)*))
+    };
+
+}
+
+#[macro_export]
+macro_rules! key_expr_error {
+    ($arg:expr) => {
+        Error::KeyExpr($arg.to_string())
+    };
+    ($fmt:expr, $($arg:tt)*) => {
+        Error::KeyExpr(format!($fmt, $($arg)*))
     };
 }
 
@@ -58,8 +89,8 @@ impl Error {
         let exception_name = self.get_associated_kotlin_exception();
         let exception_class = env
             .find_class(&exception_name)
-            .map_err(|err| Error::Jni(format!("Failed to retrieve exception class: {}", err)))?;
+            .map_err(|err| jni_error!("Failed to retrieve exception class: {}", err))?;
         env.throw_new(exception_class, self.to_string())
-            .map_err(|err| Error::Jni(format!("Failed to throw exception: {}", err)))
+            .map_err(|err| jni_error!("Failed to throw exception: {}", err))
     }
 }

zenoh-jni/src/key_expr.rs

@@ -23,29 +23,42 @@ use zenoh::key_expr::KeyExpr;
 use crate::errors::Error;
 use crate::errors::Result;
 use crate::utils::decode_string;
+use crate::{jni_error, key_expr_error, throw_exception};
 
+/// Validates the provided `key_expr` to be a valid key expression, returning it back
+/// in case of success or throwing an exception in case of failure.
+///
+/// # Parameters:
+/// `env`: The JNI environment.
+/// `_class`: the Java class (unused).
+/// `key_expr`: Java string representation of the intended key expression.
+///
 #[no_mangle]
 #[allow(non_snake_case)]
-pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_tryFromViaJNI(
+pub extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_tryFromViaJNI(
     mut env: JNIEnv,
     _class: JClass,
     key_expr: JString,
 ) -> jstring {
     decode_key_expr(&mut env, &key_expr)
-        .and_then(|key_expr| {
-            env.new_string(key_expr.to_string())
-                .map(|kexp| kexp.as_raw())
-                .map_err(|err| Error::KeyExpr(err.to_string()))
-        })
+        .map(|_| **key_expr)
         .unwrap_or_else(|err| {
-            let _ = err.throw_on_jvm(&mut env);
+            throw_exception!(env, err);
             JString::default().as_raw()
         })
 }
 
+/// Returns a java string representation of the autocanonized version of the provided `key_expr`.
+/// In case of failure and exception will be thrown.
+///
+/// # Parameters:
+/// `env`: The JNI environment.
+/// `_class`: the Java class (unused).
+/// `key_expr`: Java string representation of the intended key expression.
+///
 #[no_mangle]
 #[allow(non_snake_case)]
-pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_autocanonizeViaJNI(
+pub extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_autocanonizeViaJNI(
     mut env: JNIEnv,
     _class: JClass,
     key_expr: JString,
@@ -54,105 +67,120 @@ pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_auto
         .and_then(|key_expr| {
             env.new_string(key_expr.to_string())
                 .map(|kexp| kexp.as_raw())
-                .map_err(|err| Error::KeyExpr(err.to_string()))
+                .map_err(|err| jni_error!(err))
         })
         .unwrap_or_else(|err| {
-            let _ = err.throw_on_jvm(&mut env);
+            throw_exception!(env, err);
             JString::default().as_raw()
         })
 }
 
+/// Returns true in case key_expr_1 intersects key_expr_2.
+///
+/// # Params:
+/// - `key_expr_ptr_1`: Pointer to the key expression 1, differs from null only if it's a declared key expr.
+/// - `key_expr_ptr_1`: String representation of the key expression 1.
+/// - `key_expr_ptr_2`: Pointer to the key expression 2, differs from null only if it's a declared key expr.
+/// - `key_expr_ptr_2`: String representation of the key expression 2.
+///
+/// # Safety
+/// - This function is marked as unsafe due to raw pointer manipulation, which happens only when providing
+/// key expressions that were declared from a session (in that case the key expression has a pointer associated).
+/// In that case, this function assumes the pointers are valid pointers to key expressions and those pointers
+/// remain valid after the call to this function.
+///
 #[no_mangle]
 #[allow(non_snake_case)]
-pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_intersectsViaJNI(
+pub unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_intersectsViaJNI(
     mut env: JNIEnv,
     _: JClass,
-    key_expr_ptr_1: *const KeyExpr<'static>,
+    key_expr_ptr_1: /*nullable*/ *const KeyExpr<'static>,
     key_expr_str_1: JString,
-    key_expr_ptr_2: *const KeyExpr<'static>,
+    key_expr_ptr_2: /*nullable*/ *const KeyExpr<'static>,
     key_expr_str_2: JString,
 ) -> jboolean {
-    let key_expr_1 =
-        match process_kotlin_key_expr_or_throw(&mut env, &key_expr_str_1, key_expr_ptr_1) {
-            Ok(key_expr) => key_expr,
-            Err(_) => return false as jboolean,
-        };
-    let key_expr_2 =
-        match process_kotlin_key_expr_or_throw(&mut env, &key_expr_str_2, key_expr_ptr_2) {
-            Ok(key_expr) => key_expr,
-            Err(_) => return false as jboolean,
-        };
-    let intersects = key_expr_1.intersects(&key_expr_2);
-    intersects as jboolean
+    || -> Result<jboolean> {
+        let key_expr_1 = process_kotlin_key_expr(&mut env, &key_expr_str_1, key_expr_ptr_1)?;
+        let key_expr_2 = process_kotlin_key_expr(&mut env, &key_expr_str_2, key_expr_ptr_2)?;
+        Ok(key_expr_1.intersects(&key_expr_2) as jboolean)
+    }()
+    .unwrap_or_else(|err| {
+        throw_exception!(env, err);
+        false as jboolean
+    })
 }
 
+/// Returns true in case key_expr_1 includes key_expr_2.
+///
+/// # Params:
+/// - `key_expr_ptr_1`: Pointer to the key expression 1, differs from null only if it's a declared key expr.
+/// - `key_expr_ptr_1`: String representation of the key expression 1.
+/// - `key_expr_ptr_2`: Pointer to the key expression 2, differs from null only if it's a declared key expr.
+/// - `key_expr_ptr_2`: String representation of the key expression 2.
+///
+/// # Safety
+/// - This function is marked as unsafe due to raw pointer manipulation, which happens only when providing
+/// key expressions that were declared from a session (in that case the key expression has a pointer associated).
+/// In that case, this function assumes the pointers are valid pointers to key expressions and those pointers
+/// remain valid after the call to this function.
+///
 #[no_mangle]
 #[allow(non_snake_case)]
-pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_includesViaJNI(
+pub unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_00024Companion_includesViaJNI(
     mut env: JNIEnv,
     _: JClass,
-    key_expr_ptr_1: *const KeyExpr<'static>,
+    key_expr_ptr_1: /*nullable*/ *const KeyExpr<'static>,
     key_expr_str_1: JString,
-    key_expr_ptr_2: *const KeyExpr<'static>,
+    key_expr_ptr_2: /*nullable*/ *const KeyExpr<'static>,
     key_expr_str_2: JString,
 ) -> jboolean {
-    let key_expr_1 =
-        match process_kotlin_key_expr_or_throw(&mut env, &key_expr_str_1, key_expr_ptr_1) {
-            Ok(key_expr) => key_expr,
-            Err(_) => return false as jboolean,
-        };
-    let key_expr_2 =
-        match process_kotlin_key_expr_or_throw(&mut env, &key_expr_str_2, key_expr_ptr_2) {
-            Ok(key_expr) => key_expr,
-            Err(_) => return false as jboolean,
-        };
-    key_expr_1.includes(&key_expr_2) as jboolean
+    || -> Result<jboolean> {
+        let key_expr_1 = process_kotlin_key_expr(&mut env, &key_expr_str_1, key_expr_ptr_1)?;
+        let key_expr_2 = process_kotlin_key_expr(&mut env, &key_expr_str_2, key_expr_ptr_2)?;
+        Ok(key_expr_1.includes(&key_expr_2) as jboolean)
+    }()
+    .unwrap_or_else(|err| {
+        throw_exception!(env, err);
+        false as jboolean
+    })
 }
 
+/// Frees a declared key expression.
+///
+/// # Parameters
+/// - `_env`: Unused. The JNI environment.
+/// - `_class`: Unused. The java class from which the function was called.
+/// - `key_expr_ptr`: the pointer to the key expression.
+///
+/// # Safety
+/// - This function assumes the provided pointer is valid and points to a native key expression.
+/// - The memory associated to the pointer is freed after returning from this call, turning the
+///   pointer invalid after that.
+///
 #[no_mangle]
 #[allow(non_snake_case)]
-pub(crate) unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_freePtrViaJNI(
+pub unsafe extern "C" fn Java_io_zenoh_jni_JNIKeyExpr_freePtrViaJNI(
     _env: JNIEnv,
     _: JClass,
-    ptr: *const KeyExpr<'static>,
+    key_expr_ptr: *const KeyExpr<'static>,
 ) {
-    Arc::from_raw(ptr);
+    Arc::from_raw(key_expr_ptr);
 }
 
-pub(crate) fn decode_key_expr(env: &mut JNIEnv, key_expr: &JString) -> Result<KeyExpr<'static>> {
-    let key_expr_str = decode_string(env, key_expr).map_err(|err| {
-        Error::Jni(format!(
-            "Unable to get key expression string value: {}",
-            err
-        ))
-    })?;
+fn decode_key_expr(env: &mut JNIEnv, key_expr: &JString) -> Result<KeyExpr<'static>> {
+    let key_expr_str = decode_string(env, key_expr)
+        .map_err(|err| jni_error!("Unable to get key expression string value: '{}'.", err))?;
 
-    KeyExpr::try_from(key_expr_str).map_err(|err| {
-        Error::Jni(format!(
-            "Unable to create key expression from string: {}",
-            err
-        ))
-    })
+    KeyExpr::try_from(key_expr_str)
+        .map_err(|err| key_expr_error!("Unable to create key expression: '{}'.", err))
 }
 
-pub(crate) fn autocanonize_key_expr(
-    env: &mut JNIEnv,
-    key_expr: &JString,
-) -> Result<KeyExpr<'static>> {
+fn autocanonize_key_expr(env: &mut JNIEnv, key_expr: &JString) -> Result<KeyExpr<'static>> {
     decode_string(env, key_expr)
-        .map_err(|err| {
-            Error::Jni(format!(
-                "Unable to get key expression string value: {}",
-                err
-            ))
-        })
+        .map_err(|err| jni_error!("Unable to get key expression string value: '{}'.", err))
         .and_then(|key_expr_str| {
-            KeyExpr::autocanonize(key_expr_str).map_err(|err| {
-                Error::Jni(format!(
-                    "Unable to create key expression from string: {}",
-                    err
-                ))
-            })
+            KeyExpr::autocanonize(key_expr_str)
+                .map_err(|err| key_expr_error!("Unable to create key expression: '{}'", err))
         })
 }
 
@@ -171,28 +199,11 @@ pub(crate) unsafe fn process_kotlin_key_expr(
 ) -> Result<KeyExpr<'static>> {
     if key_expr_ptr.is_null() {
         decode_key_expr(env, key_expr_str)
-            .map_err(|err| Error::Jni(format!("Unable to process key expression: {}", err)))
+            .map_err(|err| jni_error!("Unable to process key expression: '{}'.", err))
     } else {
         let key_expr = Arc::from_raw(key_expr_ptr);
         let key_expr_clone = key_expr.deref().clone();
         std::mem::forget(key_expr);
         Ok(key_expr_clone)
     }
 }
-
-/// Internal helper function that wraps [process_kotlin_key_expr] and throws an Exception through the JVM in
-/// case of error.
-unsafe fn process_kotlin_key_expr_or_throw(
-    env: &mut JNIEnv,
-    key_expr_str: &JString,
-    key_expr_ptr: *const KeyExpr<'static>,
-) -> core::result::Result<KeyExpr<'static>, ()> {
-    process_kotlin_key_expr(env, key_expr_str, key_expr_ptr).map_err(|err| {
-        let _ = err.throw_on_jvm(env).map_err(|err| {
-            tracing::error!(
-                "Unable to throw exception while processing key expression: '{}'.",
-                err
-            )
-        });
-    })
-}

zenoh-jni/src/lib.rs

@@ -18,7 +18,6 @@ mod logger;
 mod publisher;
 mod query;
 mod queryable;
-mod reply;
 mod session;
 mod subscriber;
 mod utils;