From 99e7256ddb895e0f2d56cb2dfb2501bfa3170a07 Mon Sep 17 00:00:00 2001 From: eifrah-aws Date: Tue, 18 Jun 2024 13:13:58 +0300 Subject: [PATCH] Java: reverted byte[] overloading (#1598) As discussed, the first step before introducing GlideString is to revert commit https://github.com/aws/glide-for-redis/pull/1526 This commit does that --- .../src/main/java/glide/api/BaseClient.java | 45 +++----------- .../main/java/glide/api/ResponseFlags.java | 2 - .../api/commands/StringBaseCommands.java | 34 ----------- .../ffi/resolvers/RedisValueResolver.java | 9 --- .../test/java/glide/SharedCommandTests.java | 19 ++---- java/src/lib.rs | 58 +++++-------------- 6 files changed, 27 insertions(+), 140 deletions(-) diff --git a/java/client/src/main/java/glide/api/BaseClient.java b/java/client/src/main/java/glide/api/BaseClient.java index b0f2b84d2b..671e9b4771 100644 --- a/java/client/src/main/java/glide/api/BaseClient.java +++ b/java/client/src/main/java/glide/api/BaseClient.java @@ -206,7 +206,6 @@ import glide.managers.BaseCommandResponseResolver; import glide.managers.CommandManager; import glide.managers.ConnectionManager; -import java.util.Arrays; import java.util.EnumSet; import java.util.List; import java.util.Map; @@ -324,13 +323,9 @@ protected static CommandManager buildCommandManager(ChannelHandler channelHandle @SuppressWarnings("unchecked") protected T handleRedisResponse( Class classType, EnumSet flags, Response response) throws RedisException { - boolean encodingUtf8 = flags.contains(ResponseFlags.ENCODING_UTF8); boolean isNullable = flags.contains(ResponseFlags.IS_NULLABLE); Object value = - encodingUtf8 - ? new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response) - : new BaseCommandResponseResolver(RedisValueResolver::valueFromPointerBinary) - .apply(response); + new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response); if (isNullable && (value == null)) { return null; } @@ -346,21 +341,15 @@ protected T handleRedisResponse( } protected Object handleObjectOrNullResponse(Response response) throws RedisException { - return handleRedisResponse( - Object.class, EnumSet.of(ResponseFlags.IS_NULLABLE, ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(Object.class, EnumSet.of(ResponseFlags.IS_NULLABLE), response); } protected String handleStringResponse(Response response) throws RedisException { - return handleRedisResponse(String.class, EnumSet.of(ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(String.class, EnumSet.noneOf(ResponseFlags.class), response); } protected String handleStringOrNullResponse(Response response) throws RedisException { - return handleRedisResponse( - String.class, EnumSet.of(ResponseFlags.IS_NULLABLE, ResponseFlags.ENCODING_UTF8), response); - } - - protected byte[] handleBytesOrNullResponse(Response response) throws RedisException { - return handleRedisResponse(byte[].class, EnumSet.of(ResponseFlags.IS_NULLABLE), response); + return handleRedisResponse(String.class, EnumSet.of(ResponseFlags.IS_NULLABLE), response); } protected Boolean handleBooleanResponse(Response response) throws RedisException { @@ -384,14 +373,11 @@ protected Double handleDoubleOrNullResponse(Response response) throws RedisExcep } protected Object[] handleArrayResponse(Response response) throws RedisException { - return handleRedisResponse(Object[].class, EnumSet.of(ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(Object[].class, EnumSet.noneOf(ResponseFlags.class), response); } protected Object[] handleArrayOrNullResponse(Response response) throws RedisException { - return handleRedisResponse( - Object[].class, - EnumSet.of(ResponseFlags.IS_NULLABLE, ResponseFlags.ENCODING_UTF8), - response); + return handleRedisResponse(Object[].class, EnumSet.of(ResponseFlags.IS_NULLABLE), response); } /** @@ -401,7 +387,7 @@ protected Object[] handleArrayOrNullResponse(Response response) throws RedisExce */ @SuppressWarnings("unchecked") // raw Map cast to Map protected Map handleMapResponse(Response response) throws RedisException { - return handleRedisResponse(Map.class, EnumSet.of(ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(Map.class, EnumSet.noneOf(ResponseFlags.class), response); } /** @@ -411,8 +397,7 @@ protected Map handleMapResponse(Response response) throws RedisEx */ @SuppressWarnings("unchecked") // raw Map cast to Map protected Map handleMapOrNullResponse(Response response) throws RedisException { - return handleRedisResponse( - Map.class, EnumSet.of(ResponseFlags.IS_NULLABLE, ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(Map.class, EnumSet.of(ResponseFlags.IS_NULLABLE), response); } /** @@ -434,7 +419,7 @@ protected Map> handleXReadResponse(Response resp @SuppressWarnings("unchecked") // raw Set cast to Set protected Set handleSetResponse(Response response) throws RedisException { - return handleRedisResponse(Set.class, EnumSet.of(ResponseFlags.ENCODING_UTF8), response); + return handleRedisResponse(Set.class, EnumSet.noneOf(ResponseFlags.class), response); } /** Process a FUNCTION LIST standalone response. */ @@ -471,24 +456,12 @@ public CompletableFuture get(@NonNull String key) { Get, new String[] {key}, this::handleStringOrNullResponse); } - @Override - public CompletableFuture get(@NonNull byte[] key) { - return commandManager.submitNewCommand( - Get, Arrays.asList(key), this::handleBytesOrNullResponse); - } - @Override public CompletableFuture getdel(@NonNull String key) { return commandManager.submitNewCommand( GetDel, new String[] {key}, this::handleStringOrNullResponse); } - @Override - public CompletableFuture set(@NonNull byte[] key, @NonNull byte[] value) { - return commandManager.submitNewCommand( - Set, Arrays.asList(key, value), this::handleStringResponse); - } - @Override public CompletableFuture set(@NonNull String key, @NonNull String value) { return commandManager.submitNewCommand( diff --git a/java/client/src/main/java/glide/api/ResponseFlags.java b/java/client/src/main/java/glide/api/ResponseFlags.java index 690a9ca00a..722607314f 100644 --- a/java/client/src/main/java/glide/api/ResponseFlags.java +++ b/java/client/src/main/java/glide/api/ResponseFlags.java @@ -2,8 +2,6 @@ package glide.api; public enum ResponseFlags { - /** Strings in the response are UTF-8 encoded */ - ENCODING_UTF8, /** Null is a valid response */ IS_NULLABLE, } diff --git a/java/client/src/main/java/glide/api/commands/StringBaseCommands.java b/java/client/src/main/java/glide/api/commands/StringBaseCommands.java index b38e21c573..2eb3cdb0fd 100644 --- a/java/client/src/main/java/glide/api/commands/StringBaseCommands.java +++ b/java/client/src/main/java/glide/api/commands/StringBaseCommands.java @@ -37,25 +37,6 @@ public interface StringBaseCommands { */ CompletableFuture get(String key); - /** - * Gets the value associated with the given key, or null if no such - * value exists. - * - * @see redis.io for details. - * @param key The key to retrieve from the database. - * @return Response from Redis. If key exists, returns the value of - * key as a String. Otherwise, return null. - * @example - * 
{@code
-     * byte[] value = client.get("key").get();
-     * assert Arrays.equals(value, "value".getBytes());
-     *
-     * String value = client.get("non_existing_key").get();
-     * assert value.equals(null);
-     * }
- */ - CompletableFuture get(byte[] key); - /** * Gets a string value associated with the given key and deletes the key. * @@ -89,21 +70,6 @@ public interface StringBaseCommands { */ CompletableFuture set(String key, String value); - /** - * Sets the given key with the given value. - * - * @see redis.io for details. - * @param key The key to store. - * @param value The value to store with the given key. - * @return Response from Redis containing "OK". - * @example - * 
{@code
-     * String value = client.set("key".getBytes(), "value".getBytes()).get();
-     * assert value.equals("OK");
-     * }
- */ - CompletableFuture set(byte[] key, byte[] value); - /** * Sets the given key with the given value. Return value is dependent on the passed options. * diff --git a/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java b/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java index 4aaa4a3123..e1693078c8 100644 --- a/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java +++ b/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java @@ -17,13 +17,4 @@ public class RedisValueResolver { * @return A RESP3 value */ public static native Object valueFromPointer(long pointer); - - /** - * Resolve a value received from Redis using given C-style pointer. This method does not assume - * that strings are valid UTF-8 encoded strings - * - * @param pointer A memory pointer from {@link Response} - * @return A RESP3 value - */ - public static native Object valueFromPointerBinary(long pointer); } diff --git a/java/integTest/src/test/java/glide/SharedCommandTests.java b/java/integTest/src/test/java/glide/SharedCommandTests.java index d26e656a72..1406c4593d 100644 --- a/java/integTest/src/test/java/glide/SharedCommandTests.java +++ b/java/integTest/src/test/java/glide/SharedCommandTests.java @@ -322,17 +322,6 @@ public void set_only_if_does_not_exists_missing_key(BaseClient client) { assertEquals(ANOTHER_VALUE, data); } - @SneakyThrows - @ParameterizedTest(autoCloseArguments = false) - @MethodSource("getClients") - public void set_get_binary_data(BaseClient client) { - byte[] key = "set_get_binary_data_key".getBytes(); - byte[] value = {(byte) 0x01, (byte) 0x00, (byte) 0x01, (byte) 0x00, (byte) 0x02}; - assert client.set(key, value).get().equals("OK"); - byte[] data = client.get(key).get(); - assert Arrays.equals(data, value); - } - @SneakyThrows @ParameterizedTest(autoCloseArguments = false) @MethodSource("getClients") @@ -4537,9 +4526,9 @@ public void bitop(BaseClient client) { // First bit is flipped to 1 and throws 'utf-8' codec can't decode byte 0x9e in position 0: // invalid start byte // TODO: update once fix is implemented for https://github.com/aws/glide-for-redis/issues/1447 - ExecutionException executionException = - assertThrows(ExecutionException.class, () -> client.get(destination).get()); - assertTrue(executionException.getCause() instanceof RuntimeException); + // ExecutionException executionException = + // assertThrows(ExecutionException.class, () -> client.get(destination).get()); + // assertTrue(executionException.getCause() instanceof RuntimeException); assertEquals(0, client.setbit(key1, 0, 1).get()); assertEquals(1L, client.bitop(BitwiseOperation.NOT, destination, new String[] {key1}).get()); assertEquals("\u001e", client.get(destination).get()); @@ -4557,7 +4546,7 @@ public void bitop(BaseClient client) { // Exception thrown due to the key holding a value with the wrong type assertEquals(1, client.sadd(emptyKey1, new String[] {value1}).get()); - executionException = + ExecutionException executionException = assertThrows( ExecutionException.class, () -> client.bitop(BitwiseOperation.AND, destination, new String[] {emptyKey1}).get()); diff --git a/java/src/lib.rs b/java/src/lib.rs index 9d42b8e298..4da2a07981 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -16,11 +16,7 @@ mod 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, - encoding_utf8: bool, -) -> JObject<'local> { +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()), @@ -29,28 +25,14 @@ fn redis_value_to_java<'local>( .new_object("java/lang/Long", "(J)V", &[num.into()]) .unwrap(), Value::BulkString(data) => { - if encoding_utf8 { - let Ok(utf8_str) = String::from_utf8(data) else { - let _ = env.throw("Failed to construct UTF-8 string"); - return JObject::null(); - }; - match env.new_string(utf8_str) { - Ok(string) => JObject::from(string), - Err(e) => { - let _ = env.throw(format!( - "Failed to construct Java UTF-8 string from Rust UTF-8 string. {:?}", - e - )); - JObject::null() - } - } - } else { - let Ok(bytearr) = env.byte_array_from_slice(data.as_ref()) else { - let _ = env.throw("Failed to allocate byte array"); - return JObject::null(); - }; - bytearr.into() - } + // TODO: Uncomment the below code to return binary string (next PR) + // let Ok(bytearr) = env.byte_array_from_slice(data.as_ref()) else { + // let _ = env.throw("Failed to allocate byte array"); + // return JObject::null(); + // }; + // bytearr.into() + let s = String::from_utf8_lossy(&data); + JObject::from(env.new_string(s).unwrap()) } Value::Array(array) => { let items: JObjectArray = env @@ -58,7 +40,7 @@ fn redis_value_to_java<'local>( .unwrap(); for (i, item) in array.into_iter().enumerate() { - let java_value = redis_value_to_java(env, item, encoding_utf8); + let java_value = redis_value_to_java(env, item); env.set_object_array_element(&items, i as i32, java_value) .unwrap(); } @@ -71,8 +53,8 @@ fn redis_value_to_java<'local>( .unwrap(); for (key, value) in map { - let java_key = redis_value_to_java(env, key, encoding_utf8); - let java_value = redis_value_to_java(env, value, encoding_utf8); + let java_key = redis_value_to_java(env, key); + let java_value = redis_value_to_java(env, value); env.call_method( &linked_hash_map, "put", @@ -96,7 +78,7 @@ fn redis_value_to_java<'local>( let set = env.new_object("java/util/HashSet", "()V", &[]).unwrap(); for elem in array { - let java_value = redis_value_to_java(env, elem, encoding_utf8); + let java_value = redis_value_to_java(env, elem); env.call_method( &set, "add", @@ -123,19 +105,7 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin pointer: jlong, ) -> JObject<'local> { let value = unsafe { Box::from_raw(pointer as *mut Value) }; - redis_value_to_java(&mut env, *value, true) -} - -#[no_mangle] -pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPointerBinary< - '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(&mut env, *value, false) + redis_value_to_java(&mut env, *value) } #[no_mangle]