Address PR comments

* Use GlideString for MessageHandler responses * Have GlideString overloads for PubSub comamnds return String instead of GlideString OKs.
valkey-io · Jul 4, 2024 · bb32f85 · bb32f85
1 parent f0fd6fe
commit bb32f85
Show file tree

Hide file tree

Showing 8 changed files with 21 additions and 23 deletions.
diff --git a/java/client/src/main/java/glide/api/BaseClient.java b/java/client/src/main/java/glide/api/BaseClient.java
@@ -298,8 +298,6 @@ public abstract class BaseClient
     /** Redis simple string response with "OK" */
     public static final String OK = ConstantResponse.OK.toString();
 
-    public static final GlideString TOK = GlideString.of(OK);
-
     protected final CommandManager commandManager;
     protected final ConnectionManager connectionManager;
     protected final ConcurrentLinkedDeque<PubSubMessage> messageQueue;
@@ -432,12 +430,12 @@ public void close() throws ExecutionException {
 
     protected static MessageHandler buildMessageHandler(BaseClientConfiguration config) {
         if (config.getSubscriptionConfiguration() == null) {
-            return new MessageHandler(Optional.empty(), Optional.empty(), responseResolver);
+            return new MessageHandler(Optional.empty(), Optional.empty(), binaryResponseResolver);
         }
         return new MessageHandler(
                 config.getSubscriptionConfiguration().getCallback(),
                 config.getSubscriptionConfiguration().getContext(),
-                responseResolver);
+                binaryResponseResolver);
     }
 
     protected static ChannelHandler buildChannelHandler(
@@ -3772,15 +3770,15 @@ public CompletableFuture<String> publish(@NonNull String message, @NonNull Strin
     }
 
     @Override
-    public CompletableFuture<GlideString> publish(
+    public CompletableFuture<String> publish(
             @NonNull GlideString message, @NonNull GlideString channel) {
         return commandManager.submitNewCommand(
                 Publish,
                 new GlideString[] {channel, message},
                 response -> {
                     // Check, but ignore the number - it is never valid. A GLIDE bug/limitation TODO
                     handleLongResponse(response);
-                    return TOK;
+                    return OK;
                 });
     }
 

diff --git a/java/client/src/main/java/glide/api/RedisClusterClient.java b/java/client/src/main/java/glide/api/RedisClusterClient.java
@@ -1005,7 +1005,7 @@ public CompletableFuture<String> publish(
     }
 
     @Override
-    public CompletableFuture<GlideString> publish(
+    public CompletableFuture<String> publish(
             @NonNull GlideString message, @NonNull GlideString channel, boolean sharded) {
         if (!sharded) {
             return publish(message, channel);
@@ -1017,7 +1017,7 @@ public CompletableFuture<GlideString> publish(
                 response -> {
                     // Check, but ignore the number - it is never valid. A GLIDE bug/limitation TODO
                     handleLongResponse(response);
-                    return TOK;
+                    return OK;
                 });
     }
 

diff --git a/java/client/src/main/java/glide/api/commands/PubSubBaseCommands.java b/java/client/src/main/java/glide/api/commands/PubSubBaseCommands.java
@@ -35,9 +35,9 @@ public interface PubSubBaseCommands {
      * @return <code>OK</code>.
      * @example
      *     <pre>{@code
-     * String response = client.publish("The cat said 'meow'!", "announcements").get();
+     * String response = client.publish(gs("The cat said 'meow'!"), gs("announcements")).get();
      * assert response.equals("OK");
      * }</pre>
      */
-    CompletableFuture<GlideString> publish(GlideString message, GlideString channel);
+    CompletableFuture<String> publish(GlideString message, GlideString channel);
 }
diff --git a/java/client/src/main/java/glide/api/commands/PubSubClusterCommands.java b/java/client/src/main/java/glide/api/commands/PubSubClusterCommands.java
@@ -41,9 +41,9 @@ public interface PubSubClusterCommands {
      * @return <code>OK</code>.
      * @example
      *     <pre>{@code
-     * String response = client.publish("The cat said 'meow'!", "announcements", true).get();
+     * String response = client.publish(gs("The cat said 'meow'!"), gs("announcements"), true).get();
      * assert response.equals("OK");
      * }</pre>
      */
-    CompletableFuture<GlideString> publish(GlideString message, GlideString channel, boolean sharded);
+    CompletableFuture<String> publish(GlideString message, GlideString channel, boolean sharded);
 }
diff --git a/java/client/src/main/java/glide/api/models/configuration/BaseSubscriptionConfiguration.java b/java/client/src/main/java/glide/api/models/configuration/BaseSubscriptionConfiguration.java
@@ -86,7 +86,7 @@ protected <M extends ChannelMode> void addSubscription(
         /**
          * Set a callback and a context.
          *
-         * @param callback The {@link #callback}.
+         * @param callback The {@link #callback}. This can be null to unset the callback.
          * @param context The {@link #context}.
          */
         public B callback(MessageCallback callback, Object context) {
@@ -103,7 +103,7 @@ public B callback(MessageCallback callback, Object context) {
          * Set a callback without context. <code>null</code> will be supplied to all callback calls as a
          * context.
          *
-         * @param callback The {@link #callback}.
+         * @param callback The {@link #callback}. This can be null to unset the callback.
          */
         public B callback(MessageCallback callback) {
             if (callback == null && this.context.isPresent()) {

diff --git a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java
@@ -1,9 +1,8 @@
 /** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */
 package glide.connectors.handlers;
 
-import static glide.api.models.GlideString.gs;
-
 import glide.api.logging.Logger;
+import glide.api.models.GlideString;
 import glide.api.models.PubSubMessage;
 import glide.api.models.configuration.BaseSubscriptionConfiguration.MessageCallback;
 import glide.api.models.exceptions.RedisException;
@@ -49,8 +48,8 @@ public void handle(Response response) {
             throw new RedisException("Received invalid push: empty or in incorrect format.");
         }
         @SuppressWarnings("unchecked")
-        Map<String, Object> push = (Map<String, Object>) data;
-        PushKind pushType = Enum.valueOf(PushKind.class, (String) push.get("kind"));
+        Map<GlideString, Object> push = (Map<GlideString, Object>) data;
+        PushKind pushType = Enum.valueOf(PushKind.class, ((GlideString) push.get("kind")).getString());
         Object[] values = (Object[]) push.get("values");
 
         switch (pushType) {
@@ -63,11 +62,11 @@ public void handle(Response response) {
             case PMessage:
                 handle(
                         new PubSubMessage(
-                                gs((String) values[2]), gs((String) values[1]), gs((String) values[0])));
+                                (GlideString) values[2], (GlideString) values[1], (GlideString) values[0]));
                 return;
             case Message:
             case SMessage:
-                handle(new PubSubMessage(gs((String) values[1]), gs((String) values[0])));
+                handle(new PubSubMessage((GlideString) values[1], (GlideString) values[0]));
                 return;
             case Subscribe:
             case PSubscribe:

diff --git a/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java b/java/client/src/main/java/glide/ffi/resolvers/RedisValueResolver.java
@@ -1,6 +1,7 @@
 /** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */
 package glide.ffi.resolvers;
 
+import glide.api.models.GlideString;
 import response.ResponseOuterClass.Response;
 
 public class RedisValueResolver {
@@ -16,7 +17,8 @@ public class RedisValueResolver {
     }
 
     /**
-     * Resolve a value received from Redis using given C-style pointer.
+     * Resolve a value received from Redis using given C-style pointer. String data is assumed to be
+     * UTF-8 and exposed as <code>String</code> objects.
      *
      * @param pointer A memory pointer from {@link Response}
      * @return A RESP3 value
@@ -25,7 +27,7 @@ public class RedisValueResolver {
 
     /**
      * Resolve a value received from Redis using given C-style pointer. This method does not assume
-     * that strings are valid UTF-8 encoded strings
+     * that strings are valid UTF-8 encoded strings and will expose this data as {@link GlideString}.
      *
      * @param pointer A memory pointer from {@link Response}
      * @return A RESP3 value

diff --git a/java/client/src/main/java/glide/managers/ConnectionManager.java b/java/client/src/main/java/glide/managers/ConnectionManager.java
@@ -149,7 +149,6 @@ private ConnectionRequest.Builder setupConnectionRequestBuilderRedisClient(
             var subscriptionsBuilder = PubSubSubscriptions.newBuilder();
             for (var entry : configuration.getSubscriptionConfiguration().getSubscriptions().entrySet()) {
                 var channelsBuilder = PubSubChannelsOrPatterns.newBuilder();
-                System.out.println(entry.getValue());
                 for (var channel : entry.getValue()) {
                     channelsBuilder.addChannelsOrPatterns(ByteString.copyFrom(channel.getBytes()));
                 }