From 6278ad923ee031fae736c8429a138f837f5193be Mon Sep 17 00:00:00 2001 From: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com> Date: Mon, 22 Jan 2024 10:46:12 -0800 Subject: [PATCH] Java: Support most RESP3 types (#804) * Implement support for receiving Redis arrays * Add support for simpler RESP3 types * Add some unit tests for FFI * Add more tests and support for VerbatimStrings * Fix and test Map and Set types * Add TODO about using conditional compilation flag in a separate test Gradle task * Remove dependency on ordered_float * Fix spotless errors * Attempt to fix CI test failures by updating Gradle task to build Rust * Update build.gradle to try to fix CI tests again * Change assertTrue nilValue == null to assertNull Co-authored-by: Yury-Fridlyand * Remove init and teardown * Parameterize tests * Update client/build.gradle * Remove TODO about using primitive integers * Add tests for casting Redis Ints to Java Integers * Move FFI testing code to a separate module * Update build.gradle to use a new task to run FfiTests * Apply spotless changes * Address PR comments on FFI tests * Run spotless with new whitespace changes --------- Co-authored-by: Yury-Fridlyand --- java/client/build.gradle | 38 ++++- .../src/test/java/glide/ffi/FfiTest.java | 143 ++++++++++++++++++ java/src/ffi_test.rs | 143 ++++++++++++++++++ java/src/lib.rs | 77 ++++++++-- 4 files changed, 380 insertions(+), 21 deletions(-) create mode 100644 java/client/src/test/java/glide/ffi/FfiTest.java create mode 100644 java/src/ffi_test.rs diff --git a/java/client/build.gradle b/java/client/build.gradle index 0fb95b43ec..6118a27984 100644 --- a/java/client/build.gradle +++ b/java/client/build.gradle @@ -50,22 +50,34 @@ tasks.register('cleanProtobuf') { } } +tasks.register('cleanRust') { + doFirst { + project.delete(Paths.get(project.projectDir.path, '../target').toString()) + } +} + tasks.register('buildRustRelease', Exec) { commandLine 'cargo', 'build', '--release' workingDir project.rootDir - environment 'CARGO_TERM_COLOR', 'always' + environment CARGO_TERM_COLOR: 'always' } tasks.register('buildRustReleaseStrip', Exec) { commandLine 'cargo', 'build', '--release', '--strip' workingDir project.rootDir - environment 'CARGO_TERM_COLOR', 'always' + environment CARGO_TERM_COLOR: 'always' } tasks.register('buildRust', Exec) { commandLine 'cargo', 'build' workingDir project.rootDir - environment 'CARGO_TERM_COLOR', 'always' + environment CARGO_TERM_COLOR: 'always' +} + +tasks.register('buildRustFfi', Exec) { + commandLine 'cargo', 'build' + workingDir project.rootDir + environment CARGO_TERM_COLOR: 'always', CARGO_BUILD_RUSTFLAGS: '--cfg ffi_test' } tasks.register('buildWithRust') { @@ -88,13 +100,24 @@ tasks.register('buildWithProto') { finalizedBy 'build' } +tasks.register('testFfi', Test) { + dependsOn 'buildRustFfi' + include "glide/ffi/FfiTest.class" +} + tasks.register('buildAll') { - dependsOn 'protobuf', 'buildRust' + dependsOn 'protobuf', 'buildRust', 'testFfi' finalizedBy 'build' } -compileJava.dependsOn('protobuf') -clean.dependsOn('cleanProtobuf') +compileJava.dependsOn('protobuf', 'buildRustRelease') +clean.dependsOn('cleanProtobuf', 'cleanRust') +test.dependsOn('buildRust') +testFfi.dependsOn('buildRust') + +test { + exclude "glide/ffi/FfiTest.class" +} tasks.withType(Test) { testLogging { @@ -102,5 +125,6 @@ tasks.withType(Test) { events "started", "skipped", "passed", "failed" showStandardStreams true } - jvmArgs "-Djava.library.path=${projectDir}/../target/release:${projectDir}/../target/debug" + jvmArgs "-Djava.library.path=${projectDir}/../target/debug" } + diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java new file mode 100644 index 0000000000..5ee353f38d --- /dev/null +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -0,0 +1,143 @@ +package glide.ffi; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import glide.ffi.resolvers.RedisValueResolver; +import java.util.HashMap; +import java.util.HashSet; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +public class FfiTest { + + static { + System.loadLibrary("glide_rs"); + } + + public static native long createLeakedNil(); + + public static native long createLeakedSimpleString(String value); + + public static native long createLeakedOkay(); + + public static native long createLeakedInt(long value); + + public static native long createLeakedBulkString(byte[] value); + + public static native long createLeakedLongArray(long[] value); + + public static native long createLeakedMap(long[] keys, long[] values); + + public static native long createLeakedDouble(double value); + + public static native long createLeakedBoolean(boolean value); + + public static native long createLeakedVerbatimString(String value); + + public static native long createLeakedLongSet(long[] value); + + @Test + public void redisValueToJavaValue_Nil() { + long ptr = FfiTest.createLeakedNil(); + Object nilValue = RedisValueResolver.valueFromPointer(ptr); + assertNull(nilValue); + } + + @ParameterizedTest + @ValueSource(strings = {"hello", "cat", "dog"}) + public void redisValueToJavaValue_SimpleString(String input) { + long ptr = FfiTest.createLeakedSimpleString(input); + Object simpleStringValue = RedisValueResolver.valueFromPointer(ptr); + assertEquals(input, simpleStringValue); + } + + @Test + public void redisValueToJavaValue_Okay() { + long ptr = FfiTest.createLeakedOkay(); + Object okayValue = RedisValueResolver.valueFromPointer(ptr); + assertEquals("OK", okayValue); + } + + @ParameterizedTest + @ValueSource(longs = {0L, 100L, 774L, Integer.MAX_VALUE + 1, Integer.MIN_VALUE - 1}) + public void redisValueToJavaValue_Int(Long input) { + long ptr = FfiTest.createLeakedInt(input); + Object longValue = RedisValueResolver.valueFromPointer(ptr); + assertTrue(longValue instanceof Long); + assertEquals(input, longValue); + } + + @Test + public void redisValueToJavaValue_BulkString() { + String input = "šŸ˜€\nšŸ’Ž\nšŸ—æ"; + byte[] bulkString = input.getBytes(); + long ptr = FfiTest.createLeakedBulkString(bulkString); + Object bulkStringValue = RedisValueResolver.valueFromPointer(ptr); + assertEquals(input, bulkStringValue); + } + + @Test + public void redisValueToJavaValue_Array() { + long[] array = {1L, 2L, 3L}; + long ptr = FfiTest.createLeakedLongArray(array); + Object longArrayValue = RedisValueResolver.valueFromPointer(ptr); + assertTrue(longArrayValue instanceof Object[]); + Object[] result = (Object[]) longArrayValue; + assertArrayEquals(new Object[] {1L, 2L, 3L}, result); + } + + @Test + public void redisValueToJavaValue_Map() { + long[] keys = {12L, 14L, 23L}; + long[] values = {1L, 2L, 3L}; + long ptr = FfiTest.createLeakedMap(keys, values); + Object mapValue = RedisValueResolver.valueFromPointer(ptr); + assertTrue(mapValue instanceof HashMap); + HashMap result = (HashMap) mapValue; + assertAll( + () -> assertEquals(1L, result.get(12L)), + () -> assertEquals(2L, result.get(14L)), + () -> assertEquals(3L, result.get(23L))); + } + + @ParameterizedTest + @ValueSource(doubles = {1.0d, 25.2d, 103.5d}) + public void redisValueToJavaValue_Double(Double input) { + long ptr = FfiTest.createLeakedDouble(input); + Object doubleValue = RedisValueResolver.valueFromPointer(ptr); + assertEquals(input, doubleValue); + } + + @Test + public void redisValueToJavaValue_Boolean() { + long ptr = FfiTest.createLeakedBoolean(true); + Object booleanValue = RedisValueResolver.valueFromPointer(ptr); + assertTrue((Boolean) booleanValue); + } + + @ParameterizedTest + @ValueSource(strings = {"hello", "cat", "dog"}) + public void redisValueToJavaValue_VerbatimString(String input) { + long ptr = FfiTest.createLeakedVerbatimString(input); + Object verbatimStringValue = RedisValueResolver.valueFromPointer(ptr); + assertEquals(input, verbatimStringValue); + } + + @Test + public void redisValueToJavaValue_Set() { + long[] array = {1L, 2L, 2L}; + long ptr = FfiTest.createLeakedLongSet(array); + Object longSetValue = RedisValueResolver.valueFromPointer(ptr); + assertTrue(longSetValue instanceof HashSet); + HashSet result = (HashSet) longSetValue; + assertAll( + () -> assertTrue(result.contains(1L)), + () -> assertTrue(result.contains(2L)), + () -> assertEquals(result.size(), 2)); + } +} diff --git a/java/src/ffi_test.rs b/java/src/ffi_test.rs new file mode 100644 index 0000000000..fea4fdd2ef --- /dev/null +++ b/java/src/ffi_test.rs @@ -0,0 +1,143 @@ +use jni::{ + objects::{JClass, JLongArray}, + sys::jlong, + JNIEnv, +}; +use redis::Value; + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedNil<'local>( + _env: JNIEnv<'local>, + _class: JClass<'local>, +) -> jlong { + let redis_value = Value::Nil; + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedSimpleString<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + value: jni::objects::JString<'local>, +) -> jlong { + let value: String = env.get_string(&value).unwrap().into(); + let redis_value = Value::SimpleString(value); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedOkay<'local>( + _env: JNIEnv<'local>, + _class: JClass<'local>, +) -> jlong { + let redis_value = Value::Okay; + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedInt<'local>( + _env: JNIEnv<'local>, + _class: JClass<'local>, + value: jlong, +) -> jlong { + let redis_value = Value::Int(value); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBulkString<'local>( + env: JNIEnv<'local>, + _class: JClass<'local>, + value: jni::objects::JByteArray<'local>, +) -> jlong { + let value = env.convert_byte_array(&value).unwrap(); + let value = value.into_iter().collect::>(); + let redis_value = Value::BulkString(value); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedLongArray<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + value: JLongArray<'local>, +) -> jlong { + let array = java_long_array_to_value(&mut env, &value); + let redis_value = Value::Array(array); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedMap<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + keys: JLongArray<'local>, + values: JLongArray<'local>, +) -> jlong { + let keys_vec = java_long_array_to_value(&mut env, &keys); + let values_vec = java_long_array_to_value(&mut env, &values); + let map: Vec<(Value, Value)> = keys_vec.into_iter().zip(values_vec).collect(); + let redis_value = Value::Map(map); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedDouble<'local>( + _env: JNIEnv<'local>, + _class: JClass<'local>, + value: jni::sys::jdouble, +) -> jlong { + let redis_value = Value::Double(value.into()); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBoolean<'local>( + _env: JNIEnv<'local>, + _class: JClass<'local>, + value: jni::sys::jboolean, +) -> jlong { + let redis_value = Value::Boolean(value != 0); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedVerbatimString<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + value: jni::objects::JString<'local>, +) -> jlong { + use redis::VerbatimFormat; + let value: String = env.get_string(&value).unwrap().into(); + let redis_value = Value::VerbatimString { + format: VerbatimFormat::Text, + text: value, + }; + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedLongSet<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + value: JLongArray<'local>, +) -> jlong { + let set = java_long_array_to_value(&mut env, &value); + let redis_value = Value::Set(set); + Box::leak(Box::new(redis_value)) as *mut Value as jlong +} + +fn java_long_array_to_value<'local>( + env: &mut JNIEnv<'local>, + array: &JLongArray<'local>, +) -> Vec { + use jni::objects::ReleaseMode; + let elements = unsafe { + env.get_array_elements(array, ReleaseMode::NoCopyBack) + .unwrap() + }; + elements + .iter() + .map(|value| Value::Int(*value)) + .collect::>() +} diff --git a/java/src/lib.rs b/java/src/lib.rs index 31372c46b5..be17cc521d 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -1,20 +1,25 @@ use glide_core::start_socket_listener; -use jni::objects::{JClass, JObject, JThrowable}; +use jni::objects::{JClass, JObject, JObjectArray, JThrowable}; use jni::sys::jlong; use jni::JNIEnv; use log::error; use redis::Value; use std::sync::mpsc; -fn redis_value_to_java(mut env: JNIEnv, val: Value) -> JObject { +#[cfg(ffi_test)] +mod ffi_test; +#[cfg(ffi_test)] +pub use ffi_test::*; + +// TODO: Consider caching method IDs here in a static variable (might need RwLock to mutate) +fn redis_value_to_java<'local>(env: &mut JNIEnv<'local>, val: Value) -> JObject<'local> { match val { Value::Nil => JObject::null(), Value::SimpleString(str) => JObject::from(env.new_string(str).unwrap()), Value::Okay => JObject::from(env.new_string("OK").unwrap()), - // TODO use primitive integer Value::Int(num) => env - .new_object("java/lang/Integer", "(I)V", &[num.into()]) + .new_object("java/lang/Long", "(J)V", &[num.into()]) .unwrap(), Value::BulkString(data) => match std::str::from_utf8(data.as_ref()) { Ok(val) => JObject::from(env.new_string(val).unwrap()), @@ -23,16 +28,60 @@ fn redis_value_to_java(mut env: JNIEnv, val: Value) -> JObject { JObject::null() } }, - Value::Array(_array) => { - let _ = env.throw("Not implemented"); - JObject::null() + Value::Array(array) => { + let items: JObjectArray = env + .new_object_array(array.len() as i32, "java/lang/Object", JObject::null()) + .unwrap(); + + for (i, item) in array.into_iter().enumerate() { + let java_value = redis_value_to_java(env, item); + env.set_object_array_element(&items, i as i32, java_value) + .unwrap(); + } + + items.into() + } + Value::Map(map) => { + let hashmap = env.new_object("java/util/HashMap", "()V", &[]).unwrap(); + + for (key, value) in map { + let java_key = redis_value_to_java(env, key); + let java_value = redis_value_to_java(env, value); + env.call_method( + &hashmap, + "put", + "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;", + &[(&java_key).into(), (&java_value).into()], + ) + .unwrap(); + } + + hashmap } - Value::Map(_map) => todo!(), - Value::Double(_float) => todo!(), - Value::Boolean(_bool) => todo!(), - Value::VerbatimString { format: _, text: _ } => todo!(), + Value::Double(float) => env + .new_object("java/lang/Double", "(D)V", &[float.into_inner().into()]) + .unwrap(), + Value::Boolean(bool) => env + .new_object("java/lang/Boolean", "(Z)V", &[bool.into()]) + .unwrap(), + Value::VerbatimString { format: _, text } => JObject::from(env.new_string(text).unwrap()), Value::BigNumber(_num) => todo!(), - Value::Set(_array) => todo!(), + Value::Set(array) => { + let set = env.new_object("java/util/HashSet", "()V", &[]).unwrap(); + + for elem in array { + let java_value = redis_value_to_java(env, elem); + env.call_method( + &set, + "add", + "(Ljava/lang/Object;)Z", + &[(&java_value).into()], + ) + .unwrap(); + } + + set + } Value::Attribute { data: _, attributes: _, @@ -43,12 +92,12 @@ fn redis_value_to_java(mut env: JNIEnv, val: Value) -> JObject { #[no_mangle] pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPointer<'local>( - env: JNIEnv<'local>, + mut env: JNIEnv<'local>, _class: JClass<'local>, pointer: jlong, ) -> JObject<'local> { let value = unsafe { Box::from_raw(pointer as *mut Value) }; - redis_value_to_java(env, *value) + redis_value_to_java(&mut env, *value) } #[no_mangle]