diff --git a/lib/src/test/java/org/automerge/TestDocument.java b/lib/src/test/java/org/automerge/TestDocument.java index 2a2d2bc..d680640 100644 --- a/lib/src/test/java/org/automerge/TestDocument.java +++ b/lib/src/test/java/org/automerge/TestDocument.java @@ -63,4 +63,15 @@ public void testFree() { Document doc = new Document(); doc.free(); } + + @Test + public void testNullObjectIdThrows() { + // this test is actually a test for any path which uses an `ObjectId` + // as the code which throws the exception is in the rust implementation + // which converts the `ObjectId` into an internal rust type + Document doc = new Document(); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + doc.get(null, "key"); + }); + } } diff --git a/lib/src/test/java/org/automerge/TestSplice.java b/lib/src/test/java/org/automerge/TestSplice.java index 767feb2..80876f9 100644 --- a/lib/src/test/java/org/automerge/TestSplice.java +++ b/lib/src/test/java/org/automerge/TestSplice.java @@ -15,7 +15,6 @@ public final class TestSplice { interface InsertedAssertions { void assertInserted(Object elem1, Object elem2); } - public TestSplice() { super(); } diff --git a/rust/src/cursor.rs b/rust/src/cursor.rs index 460014d..536a12e 100644 --- a/rust/src/cursor.rs +++ b/rust/src/cursor.rs @@ -58,8 +58,7 @@ impl Cursor { .map_err(errors::FromRaw::GetByteArray)?; let bytes = std::slice::from_raw_parts(arr.as_ptr() as *const u8, arr.size().unwrap() as usize); - let cursor: automerge::Cursor = - bytes.try_into().map_err(errors::FromRaw::Invalid)?; + let cursor: automerge::Cursor = bytes.try_into().map_err(errors::FromRaw::Invalid)?; Ok(Self(cursor)) } } diff --git a/rust/src/obj_id.rs b/rust/src/obj_id.rs index 5059e03..abd8636 100644 --- a/rust/src/obj_id.rs +++ b/rust/src/obj_id.rs @@ -48,8 +48,17 @@ impl JavaObjId { Ok(raw_obj) } - pub(crate) unsafe fn from_raw(env: &JNIEnv<'_>, raw: jobject) -> Result { + pub(crate) unsafe fn from_raw( + env: &JNIEnv<'_>, + raw: jobject, + ) -> Result, errors::FromRaw> { let obj = JObject::from_raw(raw); + let id_is_null = env + .is_same_object(obj, JObject::null()) + .map_err(errors::FromRaw::GetRaw)?; + if id_is_null { + return Ok(None); + } let bytes_jobject = env .get_field(obj, "raw", "[B") .map_err(errors::FromRaw::GetRaw)? @@ -62,10 +71,57 @@ impl JavaObjId { let bytes = std::slice::from_raw_parts(arr.as_ptr() as *const u8, arr.size().unwrap() as usize); let obj: automerge::ObjId = bytes.try_into()?; - Ok(Self(obj)) + //Ok(Some(Self(obj))) + Ok(Some(Self(obj))) } } +/// Get the `ObjId` from a `jobject` or throw an exception and return the given value. +/// +/// This macro performs an early return if the `jobject` is null, which means the macro has two +/// forms. The first form, which looks like this: +/// +/// ```rust,ignore +/// let obj = obj_id_or_throw!(env, some_obj_id); +/// ``` +/// +/// Takes a [`jni::JNIEnv`] and a `jobject` and returns a [`JavaObjId`] or throws an exception and +/// early returns a `jobject` from the surrounding function. +/// +/// The second form, which looks like this: +/// +/// ```rust,ignore +/// let obj = obj_id_or_throw!(env, some_obj_id, false); // the `false` here can be anything +/// ``` +/// +/// Takes a [`jni::JNIEnv`], a `jobject`, and a value to return from the surrounding function if +/// the `jobject` is null. +macro_rules! obj_id_or_throw { + ($env:expr, $obj_id:expr) => { + obj_id_or_throw!($env, $obj_id, JObject::null().into_raw()) + }; + ($env:expr, $obj_id:expr,$returning:expr) => { + match JavaObjId::from_raw($env, $obj_id) { + Ok(Some(id)) => id, + Ok(None) => { + $env.throw_new( + "java/lang/IllegalArgumentException", + "Object ID cannot be null", + ) + .unwrap(); + return $returning; + } + Err(e) => { + use crate::AUTOMERGE_EXCEPTION; + $env.throw_new(AUTOMERGE_EXCEPTION, format!("{}", e)) + .unwrap(); + return $returning; + } + } + }; +} +pub(crate) use obj_id_or_throw; + #[no_mangle] #[jni_fn] pub unsafe extern "C" fn rootObjectId( @@ -82,7 +138,7 @@ pub unsafe extern "C" fn isRootObjectId( _class: jni::objects::JClass, obj: jni::sys::jobject, ) -> bool { - let obj = JavaObjId::from_raw(&env, obj).unwrap(); + let obj = obj_id_or_throw!(&env, obj, false); obj.as_ref() == &automerge::ROOT } @@ -93,7 +149,7 @@ pub unsafe extern "C" fn objectIdToString( _class: jni::objects::JClass, obj: jni::sys::jobject, ) -> jobject { - let obj = JavaObjId::from_raw(&env, obj).unwrap(); + let obj = obj_id_or_throw!(&env, obj); let s = obj.as_ref().to_string(); let jstr = env.new_string(s).unwrap(); jstr.into_raw() @@ -104,9 +160,9 @@ pub unsafe extern "C" fn objectIdToString( pub unsafe extern "C" fn objectIdHash( env: jni::JNIEnv, _class: jni::objects::JClass, - left: jni::sys::jobject, + obj: jni::sys::jobject, ) -> jint { - let obj = JavaObjId::from_raw(&env, left).unwrap(); + let obj = obj_id_or_throw!(&env, obj, 0); let mut hasher = DefaultHasher::new(); obj.as_ref().hash(&mut hasher); hasher.finish() as i32 @@ -122,7 +178,17 @@ pub unsafe extern "C" fn objectIdsEqual( ) -> jboolean { let left = JavaObjId::from_raw(&env, left).unwrap(); let right = JavaObjId::from_raw(&env, right).unwrap(); - (left.as_ref() == right.as_ref()).into() + match (left, right) { + (None, _) | (_, None) => { + env.throw_new( + "java/lang/IllegalArgumentException", + "Object ID cannot be null", + ) + .unwrap(); + return false.into(); + } + (Some(left), Some(right)) => (left.as_ref() == right.as_ref()).into(), + } } pub mod errors { diff --git a/rust/src/read_methods.rs b/rust/src/read_methods.rs index b67f96d..93bd8f5 100644 --- a/rust/src/read_methods.rs +++ b/rust/src/read_methods.rs @@ -11,7 +11,7 @@ use crate::cursor::Cursor; use crate::interop::{changehash_to_jobject, heads_from_jobject, CHANGEHASH_CLASS}; use crate::java_option::{make_empty_option, make_optional}; use crate::mark::mark_to_java; -use crate::obj_id::JavaObjId; +use crate::obj_id::{obj_id_or_throw, JavaObjId}; use crate::prop::JProp; use crate::AUTOMERGE_EXCEPTION; use crate::{interop::AsPointerObj, read_ops::ReadOps}; @@ -63,7 +63,7 @@ impl SomeReadPointer { key: P, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let key = catch!(env, key.into().try_into_prop(env)); let result = catch!(env, read.get(obj, key)); @@ -79,7 +79,7 @@ impl SomeReadPointer { heads_pointer: jobject, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = heads_from_jobject(&env, heads_pointer).unwrap(); let key = catch!(env, key.into().try_into_prop(env)); @@ -96,7 +96,7 @@ impl SomeReadPointer { heads: Option, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let key = catch!(env, key.into().try_into_prop(env)); let heads = heads.map(|h| heads_from_jobject(&env, h).unwrap()); @@ -155,7 +155,7 @@ impl SomeReadPointer { heads: Option, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = heads.map(|h| heads_from_jobject(&env, h).unwrap()); let keys = match read.object_type(&obj) { Ok(automerge::ObjType::Map) => match heads { @@ -187,7 +187,7 @@ impl SomeReadPointer { heads: Option, ) -> jlong { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer, 0); match heads { Some(h) => { let heads = heads_from_jobject(&env, h).unwrap(); @@ -204,7 +204,7 @@ impl SomeReadPointer { heads: Option, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = heads.map(|h| heads_from_jobject(&env, h).unwrap()); let items = match read.object_type(&obj) { Ok(am::ObjType::List) => match heads { @@ -244,7 +244,7 @@ impl SomeReadPointer { heads: Option, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = heads.map(|h| heads_from_jobject(&env, h).unwrap()); let entries = match read.object_type(&obj) { @@ -300,7 +300,7 @@ impl SomeReadPointer { heads: Option, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = heads.map(|h| heads_from_jobject(&env, h).unwrap()); let text = match read.object_type(&obj) { Ok(am::ObjType::Text) => match heads { @@ -328,7 +328,7 @@ impl SomeReadPointer { heads_option: jobject, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = maybe_heads(env, heads_option).unwrap(); let marks = if let Some(h) = heads { read.marks_at(obj, &h) @@ -359,7 +359,7 @@ impl SomeReadPointer { heads_option: jobject, ) -> jobject { let read = SomeRead::from_pointer(env, self); - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = maybe_heads(env, heads_option).unwrap(); let marks = if let Some(h) = heads { read.get_marks(obj, index as usize, Some(&h)) @@ -395,7 +395,7 @@ impl SomeReadPointer { index: jlong, maybe_heads_pointer: jobject, ) -> jobject { - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer); let heads = maybe_heads(env, maybe_heads_pointer).unwrap(); let read = SomeRead::from_pointer(env, self); if index < 0 { @@ -421,7 +421,7 @@ impl SomeReadPointer { cursor_pointer: jobject, maybe_heads_pointer: jobject, ) -> jlong { - let obj = JavaObjId::from_raw(&env, obj_pointer).unwrap(); + let obj = obj_id_or_throw!(&env, obj_pointer, 0); let heads = maybe_heads(env, maybe_heads_pointer).unwrap(); let read = SomeRead::from_pointer(env, self); diff --git a/rust/src/transaction/delete.rs b/rust/src/transaction/delete.rs index c3f9583..18a313f 100644 --- a/rust/src/transaction/delete.rs +++ b/rust/src/transaction/delete.rs @@ -5,7 +5,10 @@ use jni::{ sys::{jlong, jobject}, }; -use crate::{obj_id::JavaObjId, AUTOMERGE_EXCEPTION}; +use crate::{ + obj_id::{obj_id_or_throw, JavaObjId}, + AUTOMERGE_EXCEPTION, +}; use super::{do_tx_op, TransactionOp}; @@ -18,7 +21,7 @@ impl TransactionOp for DeleteOp { type Output = (); unsafe fn execute(self, env: jni::JNIEnv, tx: &mut T) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); match tx.delete(obj, self.key) { Ok(_) => {} Err(e) => { diff --git a/rust/src/transaction/increment.rs b/rust/src/transaction/increment.rs index 3b30d90..58eca89 100644 --- a/rust/src/transaction/increment.rs +++ b/rust/src/transaction/increment.rs @@ -4,7 +4,10 @@ use jni::{ sys::{jlong, jobject}, }; -use crate::{obj_id::JavaObjId, AUTOMERGE_EXCEPTION}; +use crate::{ + obj_id::{obj_id_or_throw, JavaObjId}, + AUTOMERGE_EXCEPTION, +}; use super::{do_tx_op, TransactionOp}; @@ -22,7 +25,7 @@ impl TransactionOp for IncrementOp { env: jni::JNIEnv, tx: &mut T, ) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); match tx.increment(obj, self.key, self.value) { Ok(_) => {} Err(e) => { diff --git a/rust/src/transaction/insert.rs b/rust/src/transaction/insert.rs index 90cb74b..17e856f 100644 --- a/rust/src/transaction/insert.rs +++ b/rust/src/transaction/insert.rs @@ -6,7 +6,11 @@ use jni::{ sys::{jboolean, jbyteArray, jdouble, jlong, jobject}, }; -use crate::{obj_id::JavaObjId, obj_type::JavaObjType, AUTOMERGE_EXCEPTION}; +use crate::{ + obj_id::{obj_id_or_throw, JavaObjId}, + obj_type::JavaObjType, + AUTOMERGE_EXCEPTION, +}; use super::{do_tx_op, TransactionOp}; @@ -23,7 +27,7 @@ impl TransactionOp for InsertOp { env: jni::JNIEnv, tx: &mut T, ) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); let idx = match usize::try_from(self.index) { Ok(i) => i, Err(_) => { @@ -49,7 +53,7 @@ impl TransactionOp for InsertOp { env: jni::JNIEnv, tx: &mut T, ) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj); let idx = match usize::try_from(self.index) { Ok(i) => i, Err(_) => { diff --git a/rust/src/transaction/mark.rs b/rust/src/transaction/mark.rs index 4d743a4..5d3d0aa 100644 --- a/rust/src/transaction/mark.rs +++ b/rust/src/transaction/mark.rs @@ -6,7 +6,11 @@ use jni::{ sys::{jboolean, jdouble, jlong, jobject, jstring}, }; -use crate::{expand_mark, obj_id::JavaObjId, AUTOMERGE_EXCEPTION}; +use crate::{ + expand_mark, + obj_id::{obj_id_or_throw, JavaObjId}, + AUTOMERGE_EXCEPTION, +}; use super::{do_tx_op, TransactionOp}; @@ -28,7 +32,7 @@ impl TransactionOp for MarkOp { let name_str = JString::from_raw(self.name); let name: String = env.get_string(name_str).unwrap().into(); let mark = am::marks::Mark::new(name, self.value, self.start, self.end); - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); match tx.mark(obj, mark, expand) { Ok(_) => {} Err(e) => { @@ -315,7 +319,7 @@ impl TransactionOp for Unmark { let expand = expand_mark::from_java(&env, expand_obj).unwrap(); let name_str = JString::from_raw(self.name); let name: String = env.get_string(name_str).unwrap().into(); - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); match tx.unmark(obj, &name, self.start, self.end, expand) { Ok(_) => {} Err(e) => { diff --git a/rust/src/transaction/set.rs b/rust/src/transaction/set.rs index a6e37b2..b92964b 100644 --- a/rust/src/transaction/set.rs +++ b/rust/src/transaction/set.rs @@ -7,6 +7,7 @@ use jni::{ sys::{jbyteArray, jint, jlong, jobject}, }; +use crate::obj_id::obj_id_or_throw; use crate::obj_type::JavaObjType; use crate::prop::JProp; use crate::{obj_id::JavaObjId, AUTOMERGE_EXCEPTION}; @@ -30,7 +31,7 @@ impl<'a, V: Into> TransactionOp for SetOp<'a, V> { return; } }; - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); match tx.put(obj, key, self.value) { Ok(_) => {} @@ -443,7 +444,7 @@ impl TransactionOp for SetObjOp { type Output = jobject; unsafe fn execute(self, env: jni::JNIEnv, tx: &mut T) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj); let oid = match tx.put_object(obj, self.key, self.value) { Ok(oid) => oid, Err(e) => { diff --git a/rust/src/transaction/splice.rs b/rust/src/transaction/splice.rs index 9c6296a..bff6514 100644 --- a/rust/src/transaction/splice.rs +++ b/rust/src/transaction/splice.rs @@ -5,7 +5,7 @@ use jni::{ sys::{jlong, jobject}, }; -use crate::{JavaObjId, AUTOMERGE_EXCEPTION}; +use crate::{obj_id::obj_id_or_throw, JavaObjId, AUTOMERGE_EXCEPTION}; use super::{do_tx_op, TransactionOp}; @@ -30,7 +30,7 @@ impl TransactionOp for SpliceOp { type Output = (); unsafe fn execute(self, env: jni::JNIEnv, tx: &mut T) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); let iter = JObjToValIter { jiter: JObject::from_raw(self.values), env: &env, diff --git a/rust/src/transaction/splice_text.rs b/rust/src/transaction/splice_text.rs index 6608a34..a34bf53 100644 --- a/rust/src/transaction/splice_text.rs +++ b/rust/src/transaction/splice_text.rs @@ -4,7 +4,10 @@ use jni::{ sys::{jlong, jobject}, }; -use crate::{obj_id::JavaObjId, AUTOMERGE_EXCEPTION}; +use crate::{ + obj_id::{obj_id_or_throw, JavaObjId}, + AUTOMERGE_EXCEPTION, +}; use super::{do_tx_op, TransactionOp}; @@ -23,7 +26,7 @@ impl<'a> TransactionOp for SpliceTextOp<'a> { env: jni::JNIEnv, tx: &mut T, ) -> Self::Output { - let obj = JavaObjId::from_raw(&env, self.obj).unwrap(); + let obj = obj_id_or_throw!(&env, self.obj, ()); let value: String = env.get_string(self.value).unwrap().into(); match tx.splice_text(obj, self.idx as usize, self.delete as isize, &value) { Ok(_) => {}