From efd61ff88bc939eb2b3629b36f3836ce601a0d8d Mon Sep 17 00:00:00 2001
From: Bar Shaul <88437685+barshaul@users.noreply.github.com>
Date: Wed, 14 Aug 2024 19:07:03 +0300
Subject: [PATCH] Handle SPUBLISH as a non key-based command (#182)

---
 redis/src/cluster_async/mod.rs |  2 +-
 redis/src/cluster_routing.rs   | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/redis/src/cluster_async/mod.rs b/redis/src/cluster_async/mod.rs
index 55e1aeff8..53f09f08f 100644
--- a/redis/src/cluster_async/mod.rs
+++ b/redis/src/cluster_async/mod.rs
@@ -1868,7 +1868,7 @@ where
                         //   (e.g., sending management command to a different node than the user asked for); instead, raise the error.
                         let routable_cmd = cmd.and_then(|cmd| Routable::command(&*cmd));
                         if routable_cmd.is_some()
-                            && !RoutingInfo::is_key_based_cmd(&routable_cmd.unwrap())
+                            && !RoutingInfo::is_key_routing_command(&routable_cmd.unwrap())
                         {
                             return Err((
                                 ErrorKind::ClusterConnectionNotFound,
diff --git a/redis/src/cluster_routing.rs b/redis/src/cluster_routing.rs
index 5fe471682..27abd54fe 100644
--- a/redis/src/cluster_routing.rs
+++ b/redis/src/cluster_routing.rs
@@ -550,8 +550,10 @@ impl RoutingInfo {
         matches!(base_routing(cmd), RouteBy::AllNodes)
     }
 
-    /// Returns true if the `cmd` is a key-based command.
-    pub fn is_key_based_cmd(cmd: &[u8]) -> bool {
+    /// Returns true if the `cmd` is a key-based command that triggers MOVED errors.
+    /// A key-based command is one that will be accepted only by the slot owner,
+    /// while other nodes will respond with a MOVED error redirecting to the relevant primary owner.
+    pub fn is_key_routing_command(cmd: &[u8]) -> bool {
         match base_routing(cmd) {
             RouteBy::FirstKey
             | RouteBy::SecondArg
@@ -560,7 +562,18 @@ impl RoutingInfo {
             | RouteBy::SecondArgSlot
             | RouteBy::StreamsIndex
             | RouteBy::MultiShardNoValues
-            | RouteBy::MultiShardWithValues => true,
+            | RouteBy::MultiShardWithValues => {
+                if matches!(cmd, b"SPUBLISH") {
+                    // SPUBLISH does not return MOVED errors within the slot's shard. This means that even if READONLY wasn't sent to a replica,
+                    // executing SPUBLISH FOO BAR on that replica will succeed. This behavior differs from true key-based commands,
+                    // such as SET FOO BAR, where a non-readonly replica would return a MOVED error if READONLY is off.
+                    // Consequently, SPUBLISH does not meet the requirement of being a command that triggers MOVED errors.
+                    // TODO: remove this when PRIMARY_PREFERRED route for SPUBLISH is added
+                    false
+                } else {
+                    true
+                }
+            }
             RouteBy::AllNodes | RouteBy::AllPrimaries | RouteBy::Random | RouteBy::Undefined => {
                 false
             }