Java: reverted byte[] overloading (valkey-io#1598)

As discussed, the first step before introducing GlideString is to
revert commit valkey-io#1526
This commit does that

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> T handleRedisResponse(
             Class<T> classType, EnumSet<ResponseFlags> 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> 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<String, V>
     protected <V> Map<String, V> 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 <V> Map<String, V> handleMapResponse(Response response) throws RedisEx
      */
     @SuppressWarnings("unchecked") // raw Map cast to Map<String, V>
     protected <V> Map<String, V> 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<String, Map<String, String[][]>> handleXReadResponse(Response resp
 
     @SuppressWarnings("unchecked") // raw Set cast to Set<String>
     protected Set<String> 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 <code>FUNCTION LIST</code> standalone response. */
@@ -471,24 +456,12 @@ public CompletableFuture<String> get(@NonNull String key) {
                 Get, new String[] {key}, this::handleStringOrNullResponse);
     }
 
-    @Override
-    public CompletableFuture<byte[]> get(@NonNull byte[] key) {
-        return commandManager.submitNewCommand(
-                Get, Arrays.asList(key), this::handleBytesOrNullResponse);
-    }
-
     @Override
     public CompletableFuture<String> getdel(@NonNull String key) {
         return commandManager.submitNewCommand(
                 GetDel, new String[] {key}, this::handleStringOrNullResponse);
     }
 
-    @Override
-    public CompletableFuture<String> set(@NonNull byte[] key, @NonNull byte[] value) {
-        return commandManager.submitNewCommand(
-                Set, Arrays.asList(key, value), this::handleStringResponse);
-    }
-
     @Override
     public CompletableFuture<String> set(@NonNull String key, @NonNull String value) {
         return commandManager.submitNewCommand(

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,
 }

java/client/src/main/java/glide/api/commands/StringBaseCommands.java

@@ -37,25 +37,6 @@ public interface StringBaseCommands {
      */
     CompletableFuture<String> get(String key);
 
-    /**
-     * Gets the value associated with the given <code>key</code>, or <code>null</code> if no such
-     * value exists.
-     *
-     * @see <a href="https://redis.io/commands/get/">redis.io</a> for details.
-     * @param key The <code>key</code> to retrieve from the database.
-     * @return Response from Redis. If <code>key</code> exists, returns the <code>value</code> of
-     *     <code>key</code> as a <code>String</code>. Otherwise, return <code>null</code>.
-     * @example
-     *     <pre>{@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);
-     * }</pre>
-     */
-    CompletableFuture<byte[]> get(byte[] key);
-
     /**
      * Gets a string value associated with the given <code>key</code> and deletes the key.
      *
@@ -89,21 +70,6 @@ public interface StringBaseCommands {
      */
     CompletableFuture<String> set(String key, String value);
 
-    /**
-     * Sets the given <code>key</code> with the given value.
-     *
-     * @see <a href="https://redis.io/commands/set/">redis.io</a> for details.
-     * @param key The <code>key</code> to store.
-     * @param value The value to store with the given <code>key</code>.
-     * @return Response from Redis containing <code>"OK"</code>.
-     * @example
-     *     <pre>{@code
-     * String value = client.set("key".getBytes(), "value".getBytes()).get();
-     * assert value.equals("OK");
-     * }</pre>
-     */
-    CompletableFuture<String> set(byte[] key, byte[] value);
-
     /**
      * Sets the given key with the given value. Return value is dependent on the passed options.
      *

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);
 }

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());

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,36 +25,22 @@ 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
                 .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, 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]