Added handling to "?" and NULL hostnames in CLUSTER SLOTS

Author: barshaul

redis/src/cluster_topology.rs

@@ -103,14 +103,20 @@ pub(crate) fn parse_and_count_slots(
                         if node.len() < 2 {
                             return None;
                         }
-
+                        // According to the CLUSTER SLOTS documentation:
+                        // If the received hostname is an empty string or NULL, clients should utilize the hostname of the responding node.
+                        // However, if the received hostname is "?", it should be regarded as an indication of an unknown node.
                         let hostname = if let Value::BulkString(ref ip) = node[0] {
                             let hostname = String::from_utf8_lossy(ip);
                             if hostname.is_empty() {
                                 addr_of_answering_node.into()
+                            } else if hostname == "?" {
+                                return None;
                             } else {
                                 hostname
                             }
+                        } else if let Value::Nil = node[0] {
+                            addr_of_answering_node.into()
                         } else {
                             return None;
                         };
@@ -141,6 +147,13 @@ pub(crate) fn parse_and_count_slots(
             slots.push(Slot::new(start, end, nodes.pop().unwrap(), replicas));
         }
     }
+    if slots.is_empty() {
+        return Err(RedisError::from((
+            ErrorKind::ResponseError,
+            "Error parsing slots: No healthy node found",
+            format!("Raw slot map response: {:?}", raw_slot_resp),
+        )));
+    }
     // we sort the slots, because different nodes in a cluster might return the same slot view
     // in different orders, which might cause the views to be considered evaluated as not equal.
     slots.sort_unstable_by(|first, second| match first.start().cmp(&second.start()) {

redis/tests/test_cluster.rs

@@ -294,6 +294,101 @@ fn test_cluster_can_connect_to_server_that_sends_cluster_slots_without_host_name
     assert_eq!(value, Ok(Value::Nil));
 }
 
+#[test]
+fn test_cluster_can_connect_to_server_that_sends_cluster_slots_with_null_host_name() {
+    let name = "test_cluster_can_connect_to_server_that_sends_cluster_slots_with_null_host_name";
+
+    let MockEnv { mut connection, .. } = MockEnv::new(name, move |cmd: &[u8], _| {
+        if contains_slice(cmd, b"PING") {
+            Err(Ok(Value::SimpleString("OK".into())))
+        } else if contains_slice(cmd, b"CLUSTER") && contains_slice(cmd, b"SLOTS") {
+            Err(Ok(Value::Array(vec![Value::Array(vec![
+                Value::Int(0),
+                Value::Int(16383),
+                Value::Array(vec![Value::Nil, Value::Int(6379)]),
+            ])])))
+        } else {
+            Err(Ok(Value::Nil))
+        }
+    });
+
+    let value = cmd("GET").arg("test").query::<Value>(&mut connection);
+
+    assert_eq!(value, Ok(Value::Nil));
+}
+
+#[test]
+fn test_cluster_cannot_connect_to_server_with_unknown_host_name() {
+    let name = "test_cluster_cannot_connect_to_server_with_unknown_host_name";
+    let handler = move |cmd: &[u8], _| {
+        if contains_slice(cmd, b"PING") {
+            Err(Ok(Value::SimpleString("OK".into())))
+        } else if contains_slice(cmd, b"CLUSTER") && contains_slice(cmd, b"SLOTS") {
+            Err(Ok(Value::Array(vec![Value::Array(vec![
+                Value::Int(0),
+                Value::Int(16383),
+                Value::Array(vec![
+                    Value::BulkString("?".as_bytes().to_vec()),
+                    Value::Int(6379),
+                ]),
+            ])])))
+        } else {
+            Err(Ok(Value::Nil))
+        }
+    };
+    let client_builder = ClusterClient::builder(vec![&*format!("redis://{name}")]);
+    let client = client_builder.build().unwrap();
+    MOCK_CONN_UTILS.write().unwrap().insert(
+        name.to_string(),
+        MockConnectionUtils {
+            handler: Some(Arc::new(move |cmd, port| handler(cmd, port))),
+            ..Default::default()
+        },
+    );
+    let connection = client.get_generic_connection::<MockConnection>();
+    assert!(connection.is_err());
+    let err = connection.err().unwrap();
+    assert!(err
+        .to_string()
+        .contains("Error parsing slots: No healthy node found"))
+}
+
+#[test]
+fn test_cluster_can_connect_to_server_that_sends_cluster_slots_with_partial_nodes_with_unknown_host_name(
+) {
+    let name = "test_cluster_can_connect_to_server_that_sends_cluster_slots_with_partial_nodes_with_unknown_host_name";
+
+    let MockEnv { mut connection, .. } = MockEnv::new(name, move |cmd: &[u8], port| {
+        if contains_slice(cmd, b"PING") {
+            Err(Ok(Value::SimpleString("OK".into())))
+        } else if contains_slice(cmd, b"CLUSTER") && contains_slice(cmd, b"SLOTS") {
+            Err(Ok(Value::Array(vec![
+                Value::Array(vec![
+                    Value::Int(0),
+                    Value::Int(7000),
+                    Value::Array(vec![
+                        Value::BulkString(name.as_bytes().to_vec()),
+                        Value::Int(6379),
+                    ]),
+                ]),
+                Value::Array(vec![
+                    Value::Int(7001),
+                    Value::Int(16383),
+                    Value::Array(vec![
+                        Value::BulkString("?".as_bytes().to_vec()),
+                        Value::Int(6380),
+                    ]),
+                ]),
+            ])))
+        } else {
+            Err(Ok(Value::Nil))
+        }
+    });
+
+    let value = cmd("GET").arg("test").query::<Value>(&mut connection);
+    assert_eq!(value, Ok(Value::Nil));
+}
+
 #[test]
 fn test_cluster_pipeline_command_ordering() {
     let cluster = TestClusterContext::new(3, 0);