Extract error sorting code to module. (#1077)

* Extract error sorting code to module. This will allow FFI clients to use the same logic. * ++redis-rs * fix comments.
valkey-io · Mar 6, 2024 · ffb34fc · ffb34fc
1 parent b6892bc
commit ffb34fc
Show file tree

Hide file tree

Showing 9 changed files with 64 additions and 43 deletions.
diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs
@@ -51,7 +51,7 @@ pub(crate) fn convert_to_expected_type(
                         match inner_value {
                             Value::BulkString(_) => Ok((
                                 key_str,
-                                Value::Double(from_owned_redis_value::<f64>(inner_value)?.into()),
+                                Value::Double(from_owned_redis_value::<f64>(inner_value)?),
                             )),
                             Value::Double(_) => Ok((key_str, inner_value)),
                             _ => Err((
@@ -89,13 +89,11 @@ pub(crate) fn convert_to_expected_type(
             )
                 .into()),
         },
-        ExpectedReturnType::Double => {
-            Ok(Value::Double(from_owned_redis_value::<f64>(value)?.into()))
-        }
+        ExpectedReturnType::Double => Ok(Value::Double(from_owned_redis_value::<f64>(value)?)),
         ExpectedReturnType::Boolean => Ok(Value::Boolean(from_owned_redis_value::<bool>(value)?)),
         ExpectedReturnType::DoubleOrNull => match value {
             Value::Nil => Ok(value),
-            _ => Ok(Value::Double(from_owned_redis_value::<f64>(value)?.into())),
+            _ => Ok(Value::Double(from_owned_redis_value::<f64>(value)?)),
         },
         ExpectedReturnType::ZrankReturnType => match value {
             Value::Nil => Ok(value),
@@ -306,10 +304,7 @@ mod tests {
                 Value::BulkString(b"key2".to_vec()),
                 Value::BulkString(b"20.8".to_vec()),
             ),
-            (
-                Value::Double(20.5.into()),
-                Value::BulkString(b"30.2".to_vec()),
-            ),
+            (Value::Double(20.5), Value::BulkString(b"30.2".to_vec())),
         ];
 
         let converted_map = convert_to_expected_type(
@@ -328,15 +323,15 @@ mod tests {
 
         let (key, value) = &converted_map[0];
         assert_eq!(*key, Value::BulkString(b"key1".to_vec()));
-        assert_eq!(*value, Value::Double(10.5.into()));
+        assert_eq!(*value, Value::Double(10.5));
 
         let (key, value) = &converted_map[1];
         assert_eq!(*key, Value::BulkString(b"key2".to_vec()));
-        assert_eq!(*value, Value::Double(20.8.into()));
+        assert_eq!(*value, Value::Double(20.8));
 
         let (key, value) = &converted_map[2];
         assert_eq!(*key, Value::BulkString(b"20.5".to_vec()));
-        assert_eq!(*value, Value::Double(30.2.into()));
+        assert_eq!(*value, Value::Double(30.2));
 
         let array_of_arrays = vec![
             Value::Array(vec![
@@ -345,7 +340,7 @@ mod tests {
             ]),
             Value::Array(vec![
                 Value::BulkString(b"key2".to_vec()),
-                Value::Double(20.5.into()),
+                Value::Double(20.5),
             ]),
         ];
 
@@ -365,11 +360,11 @@ mod tests {
 
         let (key, value) = &converted_map[0];
         assert_eq!(*key, Value::BulkString(b"key1".to_vec()));
-        assert_eq!(*value, Value::Double(10.5.into()));
+        assert_eq!(*value, Value::Double(10.5));
 
         let (key, value) = &converted_map[1];
         assert_eq!(*key, Value::BulkString(b"key2".to_vec()));
-        assert_eq!(*value, Value::Double(20.5.into()));
+        assert_eq!(*value, Value::Double(20.5));
 
         let array_of_arrays_err: Vec<Value> = vec![Value::Array(vec![
             Value::BulkString(b"key".to_vec()),
@@ -410,7 +405,7 @@ mod tests {
         assert_eq!(array_result.len(), 2);
 
         assert_eq!(array_result[0], Value::BulkString(b"key".to_vec()));
-        assert_eq!(array_result[1], Value::Double(20.5.into()));
+        assert_eq!(array_result[1], Value::Double(20.5));
 
         let array_err = vec![Value::BulkString(b"key".to_vec())];
         assert!(convert_to_expected_type(

diff --git a/glide-core/src/errors.rs b/glide-core/src/errors.rs
@@ -0,0 +1,34 @@
+/*
+ * Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0
+ */
+
+use redis::RedisError;
+
+#[repr(C)]
+pub enum RequestErrorType {
+    Unspecified = 0,
+    ExecAbort = 1,
+    Timeout = 2,
+    Disconnect = 3,
+}
+
+pub fn error_type(error: &RedisError) -> RequestErrorType {
+    if error.is_timeout() {
+        RequestErrorType::Timeout
+    } else if error.is_unrecoverable_error() {
+        RequestErrorType::Disconnect
+    } else if matches!(error.kind(), redis::ErrorKind::ExecAbortError) {
+        RequestErrorType::ExecAbort
+    } else {
+        RequestErrorType::Unspecified
+    }
+}
+
+pub fn error_message(error: &RedisError) -> String {
+    let error_message = error.to_string();
+    if matches!(error_type(error), RequestErrorType::Disconnect) {
+        format!("Received connection error `{error_message}`. Will attempt to reconnect")
+    } else {
+        error_message
+    }
+}
diff --git a/glide-core/src/lib.rs b/glide-core/src/lib.rs
@@ -8,4 +8,5 @@ mod retry_strategies;
 pub mod rotating_buffer;
 mod socket_listener;
 pub use socket_listener::*;
+pub mod errors;
 pub mod scripts_container;
diff --git a/glide-core/src/socket_listener.rs b/glide-core/src/socket_listener.rs
@@ -4,6 +4,7 @@
 use super::rotating_buffer::RotatingBuffer;
 use crate::client::Client;
 use crate::connection_request::ConnectionRequest;
+use crate::errors::{error_message, error_type, RequestErrorType};
 use crate::redis_request::{
     command, redis_request, Command, RedisRequest, RequestType, Routes, ScriptInvocation,
     SlotTypes, Transaction,
@@ -217,28 +218,20 @@ async fn write_result(
             Some(response::response::Value::RequestError(request_error))
         }
         Err(ClienUsageError::Redis(err)) => {
-            let error_message = err.to_string();
+            let error_message = error_message(&err);
             log_warn("received error", error_message.as_str());
             log_debug("received error", format!("for callback {}", callback_index));
-            let mut request_error = response::RequestError::default();
-            if err.is_connection_dropped() {
-                request_error.type_ = response::RequestErrorType::Disconnect.into();
-                request_error.message = format!(
-                    "Received connection error `{error_message}`. Will attempt to reconnect"
-                )
-                .into();
-            } else if err.is_timeout() {
-                request_error.type_ = response::RequestErrorType::Timeout.into();
-                request_error.message = error_message.into();
-            } else {
-                request_error.type_ = match err.kind() {
-                    redis::ErrorKind::ExecAbortError => {
-                        response::RequestErrorType::ExecAbort.into()
-                    }
-                    _ => response::RequestErrorType::Unspecified.into(),
-                };
-                request_error.message = error_message.into();
-            }
+            let request_error = response::RequestError {
+                type_: match error_type(&err) {
+                    RequestErrorType::Unspecified => response::RequestErrorType::Unspecified,
+                    RequestErrorType::ExecAbort => response::RequestErrorType::ExecAbort,
+                    RequestErrorType::Timeout => response::RequestErrorType::Timeout,
+                    RequestErrorType::Disconnect => response::RequestErrorType::Disconnect,
+                }
+                .into(),
+                message: error_message.into(),
+                ..Default::default()
+            };
             Some(response::response::Value::RequestError(request_error))
         }
     };

diff --git a/glide-core/tests/test_client.rs b/glide-core/tests/test_client.rs
@@ -458,7 +458,7 @@ pub(crate) mod shared_client_tests {
                     Value::Boolean(true),
                     Value::Int(1),
                     Value::Okay,
-                    Value::Double(0.5.into()),
+                    Value::Double(0.5),
                     Value::Int(1),
                 ]),)
             );

diff --git a/java/src/lib.rs b/java/src/lib.rs
@@ -62,7 +62,7 @@ fn redis_value_to_java<'local>(env: &mut JNIEnv<'local>, val: Value) -> JObject<
             hashmap
         }
         Value::Double(float) => env
-            .new_object("java/lang/Double", "(D)V", &[float.into_inner().into()])
+            .new_object("java/lang/Double", "(D)V", &[float.into()])
             .unwrap(),
         Value::Boolean(bool) => env
             .new_object("java/lang/Boolean", "(Z)V", &[bool.into()])

diff --git a/node/rust-client/src/lib.rs b/node/rust-client/src/lib.rs
@@ -184,9 +184,7 @@ fn redis_value_to_js(val: Value, js_env: Env) -> Result<JsUnknown> {
             }
             Ok(obj.into_unknown())
         }
-        Value::Double(float) => js_env
-            .create_double(float.into())
-            .map(|val| val.into_unknown()),
+        Value::Double(float) => js_env.create_double(float).map(|val| val.into_unknown()),
         Value::Boolean(bool) => js_env.get_boolean(bool).map(|val| val.into_unknown()),
         // format is ignored, as per the RESP3 recommendations -
         // "Normal client libraries may ignore completely the difference between this"

diff --git a/python/src/lib.rs b/python/src/lib.rs
@@ -139,7 +139,7 @@ fn glide(_py: Python, m: &PyModule) -> PyResult<()> {
                 let set = PySet::new(py, set.iter())?;
                 Ok(set.into_py(py))
             }
-            Value::Double(double) => Ok(PyFloat::new(py, double.into()).into_py(py)),
+            Value::Double(double) => Ok(PyFloat::new(py, double).into_py(py)),
             Value::Boolean(boolean) => Ok(PyBool::new(py, boolean).into_py(py)),
             Value::VerbatimString { format: _, text } => Ok(text.into_py(py)),
             Value::BigNumber(bigint) => Ok(bigint.into_py(py)),

diff --git a/submodules/redis-rs b/submodules/redis-rs
+9 −7		.github/workflows/rust.yml
+4 −14		Cargo.lock
+7 −2		Makefile
+1 −6		redis-test/src/lib.rs
+4 −5		redis/Cargo.toml
+1 −1		redis/benches/bench_basic.rs
+1 −1		redis/examples/async-multiplexed.rs
+1 −1		redis/examples/basic.rs
+6 −9		redis/src/aio/connection.rs
+17 −5		redis/src/aio/connection_manager.rs
+2 −1		redis/src/aio/mod.rs
+173 −38		redis/src/aio/multiplexed_connection.rs
+1 −6		redis/src/client.rs
+1 −6		redis/src/cluster.rs
+1 −1		redis/src/cluster_async/connections_container.rs
+1 −4		redis/src/cluster_async/connections_logic.rs
+55 −44		redis/src/cluster_async/mod.rs
+2 −1		redis/src/cluster_client.rs
+162 −140		redis/src/cluster_routing.rs
+0 −3		redis/src/cluster_slotmap.rs
+2 −2		redis/src/cluster_topology.rs
+7 −0		redis/src/cmd.rs
+1 −1		redis/src/commands/mod.rs
+95 −36		redis/src/connection.rs
+2 −0		redis/src/lib.rs
+15 −15		redis/src/parser.rs
+234 −0		redis/src/push_manager.rs
+34 −2		redis/src/script.rs
+37 −14		redis/src/types.rs
+1 −6		redis/tests/parser.rs
+5 −2		redis/tests/support/cluster.rs
+0 −5		redis/tests/support/mock_cluster.rs
+13 −2		redis/tests/support/mod.rs
+145 −4		redis/tests/test_async.rs
+1 −1		redis/tests/test_async_async_std.rs
+76 −2		redis/tests/test_basic.rs
+35 −0		redis/tests/test_cluster.rs
+154 −75		redis/tests/test_cluster_async.rs
+52 −24		redis/tests/test_module_json.rs
+27 −1		redis/tests/test_types.rs