From a36e1ee2e482df7a8dde3bbb078910f051454495 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Fri, 10 May 2024 09:46:36 -0700 Subject: [PATCH 01/12] Python: add ZRANDMEMBER command --- CHANGELOG.md | 1 + python/python/glide/async_commands/core.py | 83 +++++++++++++++++++ .../glide/async_commands/transaction.py | 56 +++++++++++++ python/python/tests/test_async_client.py | 78 +++++++++++++++++ python/python/tests/test_transaction.py | 10 +++ 5 files changed, 228 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b38788bf15..52b9c11982 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ * Python: Added BLPOP and BRPOP commands ([#1369](https://github.com/aws/glide-for-redis/pull/1369)) * Python: Added ZRANGESTORE command ([#1377](https://github.com/aws/glide-for-redis/pull/1377)) * Python: Added ZDIFFSTORE command ([#1378](https://github.com/aws/glide-for-redis/pull/1378)) +* Python: Added ZRANDMEMBER command (TODO: add PR link) #### Fixes diff --git a/python/python/glide/async_commands/core.py b/python/python/glide/async_commands/core.py index 921535ab84..efa19fb27a 100644 --- a/python/python/glide/async_commands/core.py +++ b/python/python/glide/async_commands/core.py @@ -2746,6 +2746,89 @@ async def zdiffstore(self, destination: str, keys: List[str]) -> int: ), ) + async def zrandmember(self, key: str) -> Optional[str]: + """ + Returns a random element from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + + Returns: + Optional[str]: A string representing a random element from the sorted set. + If the sorted set does not exist or is empty, the response will be None. + + Examples: + >>> await client.zrandmember("my_sorted_set") + "GLIDE" # "GLIDE" is a random member of "my_sorted_set". + >>> await client.zrandmember("non_existing_sorted_set") + None # "non_existing_sorted_set" is not an existing key, so None was returned. + """ + return cast( + Optional[str], + await self._execute_command(RequestType.ZRandMember, [key]), + ) + + async def zrandmember_count(self, key: str, count: int) -> List[str]: + """ + Retrieves random elements from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + count (int): The number of elements to return. + If `count` is positive, returns unique elements. + If negative, allows for duplicates. + + Returns: + List[str]: A list of elements from the sorted set. + If the sorted set does not exist or is empty, the response will be an empty list. + + Examples: + >>> await client.zrandmember("my_sorted_set", -3) + ["GLIDE", "GLIDE", "PYTHON"] # "GLIDE" and "PYTHON" are random members of "my_sorted_set". + >>> await client.zrandmember("non_existing_sorted_set", 3) + [] # "non_existing_sorted_set" is not an existing key, so an empty list was returned. + """ + return cast( + List[str], + await self._execute_command(RequestType.ZRandMember, [key, str(count)]), + ) + + async def zrandmember_withscores( + self, key: str, count: int + ) -> List[List[Union[str, float]]]: + """ + Retrieves random elements along with their scores from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + count (int): The number of elements to return. + If `count` is positive, returns unique elements. + If negative, allows for duplicates. + + Returns: + List[List[Union[str, float]]]: A list of `[member, score]` lists, where `member` is a random member from + the sorted set and `score` is the associated score. + If the sorted set does not exist or is empty, the response will be an empty list. + + Examples: + >>> await client.zrandmember_withscores("my_sorted_set", -3) + [{"GLIDE": 1.0, "GLIDE": 1.0, "PYTHON": 2.0}] # "GLIDE" and "PYTHON" are random members of "my_sorted_set", and have scores of 1.0 and 2.0, respectively. + >>> await client.zrandmember_withscores("non_existing_sorted_set", 3) + [] # "non_existing_sorted_set" is not an existing key, so an empty list was returned. + """ + return cast( + List[List[Union[str, float]]], + await self._execute_command( + RequestType.ZRandMember, [key, str(count), "WITHSCORES"] + ), + ) + async def invoke_script( self, script: Script, diff --git a/python/python/glide/async_commands/transaction.py b/python/python/glide/async_commands/transaction.py index dbf3651d8f..883380fd10 100644 --- a/python/python/glide/async_commands/transaction.py +++ b/python/python/glide/async_commands/transaction.py @@ -1966,6 +1966,62 @@ def zdiffstore( RequestType.ZDiffStore, [destination, str(len(keys))] + keys ) + def zrandmember(self: TTransaction, key: str) -> TTransaction: + """ + Returns a random element from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + + Command response: + Optional[str]: A string representing a random element from the sorted set. + If the sorted set does not exist or is empty, the response will be None. + """ + return self.append_command(RequestType.ZRandMember, [key]) + + def zrandmember_count(self: TTransaction, key: str, count: int) -> TTransaction: + """ + Retrieves random elements from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + count (int): The number of elements to return. + If `count` is positive, returns unique elements. + If negative, allows for duplicates. + + Command response: + List[str]: A list of elements from the sorted set. + If the sorted set does not exist or is empty, the response will be an empty list. + """ + return self.append_command(RequestType.ZRandMember, [key, str(count)]) + + def zrandmember_withscores( + self: TTransaction, key: str, count: int + ) -> TTransaction: + """ + Retrieves random elements along with their scores from the sorted set stored at 'key'. + + See https://valkey.io/commands/zrandmember for more details. + + Args: + key (str): The key of the sorted set. + count (int): The number of elements to return. + If `count` is positive, returns unique elements. + If negative, allows for duplicates. + + Command response: + List[List[Union[str, float]]]: A list of `[member, score]` lists, where `member` is a random member from + the sorted set and `score` is the associated score. + If the sorted set does not exist or is empty, the response will be an empty list. + """ + return self.append_command( + RequestType.ZRandMember, [key, str(count), "WITHSCORES"] + ) + def dbsize(self: TTransaction) -> TTransaction: """ Returns the number of keys in the currently selected database. diff --git a/python/python/tests/test_async_client.py b/python/python/tests/test_async_client.py index 0461e8132a..aca0d35dd0 100644 --- a/python/python/tests/test_async_client.py +++ b/python/python/tests/test_async_client.py @@ -2378,6 +2378,84 @@ async def test_zdiffstore(self, redis_client: TRedisClient): await redis_client.zdiffstore("abc", ["zxy", "lkn"]) assert "CrossSlot" in str(e) + @pytest.mark.parametrize("cluster_mode", [True, False]) + @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3]) + async def test_zrandmember(self, redis_client: TRedisClient): + key = get_random_string(10) + string_key = get_random_string(10) + scores = {"one": 1, "two": 2} + assert await redis_client.zadd(key, scores) == 2 + + member = await redis_client.zrandmember(key) + assert member in scores + assert await redis_client.zrandmember("non_existing_key") is None + + # key exists, but it is not a set + assert await redis_client.set(string_key, "value") == OK + with pytest.raises(RequestError): + await redis_client.zrandmember(string_key) + + @pytest.mark.parametrize("cluster_mode", [True, False]) + @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3]) + async def test_zrandmember_count(self, redis_client: TRedisClient): + key = get_random_string(10) + string_key = get_random_string(10) + scores = {"one": 1, "two": 2} + assert await redis_client.zadd(key, scores) == 2 + + # unique values are expected as count is positive + members = await redis_client.zrandmember_count(key, 4) + assert len(members) == 2 + assert set(members) == {"one", "two"} + + # duplicate values are expected as count is negative + members = await redis_client.zrandmember_count(key, -4) + assert len(members) == 4 + for member in members: + assert member in scores + + assert await redis_client.zrandmember_count(key, 0) == [] + assert await redis_client.zrandmember_count("non_existing_key", 0) == [] + + # key exists, but it is not a set + assert await redis_client.set(string_key, "value") == OK + with pytest.raises(RequestError): + await redis_client.zrandmember_count(string_key, 5) + + @pytest.mark.parametrize("cluster_mode", [True, False]) + @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3]) + async def test_zrandmember_withscores(self, redis_client: TRedisClient): + key = get_random_string(10) + string_key = get_random_string(10) + scores = {"one": 1, "two": 2} + assert await redis_client.zadd(key, scores) == 2 + + # unique values are expected as count is positive + elements = await redis_client.zrandmember_withscores(key, 4) + assert len(elements) == 2 + print("The score type: " + type(elements[0][1]).__name__) + + for element in elements: + member = element[0] + score = element[1] + assert scores.get(str(member)) == score + + # duplicate values are expected as count is negative + elements = await redis_client.zrandmember_withscores(key, -4) + assert len(elements) == 4 + for element in elements: + member = element[0] + score = element[1] + assert scores.get(str(member)) == score + + assert await redis_client.zrandmember_withscores(key, 0) == [] + assert await redis_client.zrandmember_withscores("non_existing_key", 0) == [] + + # key exists, but it is not a set + assert await redis_client.set(string_key, "value") == OK + with pytest.raises(RequestError): + await redis_client.zrandmember_withscores(string_key, 5) + @pytest.mark.parametrize("cluster_mode", [True, False]) @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3]) async def test_type(self, redis_client: TRedisClient): diff --git a/python/python/tests/test_transaction.py b/python/python/tests/test_transaction.py index df45b3b8e4..0fd0651f22 100644 --- a/python/python/tests/test_transaction.py +++ b/python/python/tests/test_transaction.py @@ -46,6 +46,7 @@ async def transaction_test( key10 = "{{{}}}:{}".format(keyslot, get_random_string(3)) # hyper log log key11 = "{{{}}}:{}".format(keyslot, get_random_string(3)) # streams key12 = "{{{}}}:{}".format(keyslot, get_random_string(3)) # geo + key13 = "{{{}}}:{}".format(keyslot, get_random_string(3)) # sorted set value = datetime.now(timezone.utc).strftime("%m/%d/%Y, %H:%M:%S") value2 = get_random_string(5) @@ -238,6 +239,15 @@ async def transaction_test( transaction.zdiffstore(key8, [key8, key8]) args.append(0) + transaction.zadd(key13, {"one": 1.0}) + args.append(1) + transaction.zrandmember(key13) + args.append("one") + transaction.zrandmember_count(key13, 1) + args.append(["one"]) + transaction.zrandmember_withscores(key13, 1) + args.append([["one", 1.0]]) + transaction.pfadd(key10, ["a", "b", "c"]) args.append(1) From 1db5c635ab365a298bbeb83dd059784c4517bc09 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Fri, 10 May 2024 11:30:06 -0700 Subject: [PATCH 02/12] wip --- glide-core/src/client/value_conversion.rs | 188 ++++++++++++++++++---- 1 file changed, 161 insertions(+), 27 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 6afaf56e17..47d9281f78 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -20,7 +20,8 @@ pub(crate) enum ExpectedReturnType { ArrayOfDoubleOrNull, Lolwut, ArrayOfArraysOfDoubleOrNull, - ArrayOfKeyValuePairs, + ArrayOfPairs, + ArrayOfMemberScorePairs, } pub(crate) fn convert_to_expected_type( @@ -270,7 +271,7 @@ pub(crate) fn convert_to_expected_type( .into()), } } - ExpectedReturnType::ArrayOfKeyValuePairs => match value { + ExpectedReturnType::ArrayOfPairs => match value { Value::Nil => Ok(value), Value::Array(ref array) if array.is_empty() || matches!(array[0], Value::Array(_)) => { Ok(value) @@ -278,7 +279,7 @@ pub(crate) fn convert_to_expected_type( Value::Array(array) if matches!(array[0], Value::BulkString(_) | Value::SimpleString(_)) => { - convert_flat_array_to_key_value_pairs(array) + convert_flat_array_to_array_of_pairs(array) } _ => Err(( ErrorKind::TypeError, @@ -287,6 +288,57 @@ pub(crate) fn convert_to_expected_type( ) .into()), }, + // Used by ZRANDMEMBER when the WITHSCORES arg is passed. + // The server response can be a nil value, an empty array, a flat array of member-score pairs, or a two-dimensional array of member-score pairs. + // The server response scores can be strings or doubles. The conversions we do here are as follows: + // + // - if the server returned nil, return nil + // - if the server returned an empty array, return an empty array + // - otherwise, return a two-dimensional array of member-score pairs, where scores are of type double + ExpectedReturnType::ArrayOfMemberScorePairs => match value { + Value::Nil => Ok(value), + Value::Array(ref array) if array.is_empty() => { + Ok(value) + } + Value::Array(array) if matches!(array[0], Value::Array(_)) => { + first_pair = array[0]; + if first_pair.len() != 2 { + Err(( + ErrorKind::TypeError, + "Expected an array of member-score pairs but encountered an inner array with unexpected length", + format!("(length was {:?}, expected length 2)", pair.len()), + ) + .into()); + } + + match first_pair[1] => { + Value::Double(_) { + Ok(value) + }, + Value::BulkString(_) | Value::SimpleString(_) => { + convert_pair_scores_to_double(array) + }, + Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ) + .into()); + } + } + Value::Array(array) + if matches!(array[0], Value::BulkString(_) | Value::SimpleString(_)) => + { + array_of_pairs = convert_flat_array_to_array_of_pairs(array); + convert_pair_scores_to_double(array_of_pairs) + } + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ) + .into()), + }, } } @@ -367,10 +419,10 @@ fn convert_array_to_map( Ok(Value::Map(map)) } -/// Converts a flat array of values to a two-dimensional array, where the inner arrays are two-length arrays representing key-value pairs. Normally a map would be more suitable for these responses, but some commands (eg HRANDFIELD) may return duplicate key-value pairs depending on the command arguments. These duplicated pairs cannot be represented by a map. +/// Converts a flat array of values to a two-dimensional array, where the inner arrays are length=2 arrays representing key-value pairs. Normally a map would be more suitable for these responses, but some commands (eg HRANDFIELD) may return duplicate key-value pairs depending on the command arguments. These duplicated pairs cannot be represented by a map. /// /// `array` is a flat array containing keys at even-positioned elements and their associated values at odd-positioned elements. -fn convert_flat_array_to_key_value_pairs(array: Vec) -> RedisResult { +fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult { if array.len() % 2 != 0 { return Err(( ErrorKind::TypeError, @@ -387,6 +439,27 @@ fn convert_flat_array_to_key_value_pairs(array: Vec) -> RedisResult) -> RedisResul { + for i in (0..member_score_pairs.len()) { + pair = member_score_pairs[i] + if pair.len() != 2 { + return Err(( + ErrorKind::TypeError, + "Expected an array of member-score pairs but encountered an inner array with unexpected length", + format!("(inner array length was {:?}, expected length 2)", pair.len()), + ) + .into()); + } + + pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; + } + + Ok(Value::Array(member_score_pairs)) +} + pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option { let command = cmd.command()?; @@ -407,10 +480,10 @@ pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option { b"GEOPOS" => Some(ExpectedReturnType::ArrayOfArraysOfDoubleOrNull), b"HRANDFIELD" => cmd .position(b"WITHVALUES") - .map(|_| ExpectedReturnType::ArrayOfKeyValuePairs), + .map(|_| ExpectedReturnType::ArrayOfPairs), b"ZRANDMEMBER" => cmd .position(b"WITHSCORES") - .map(|_| ExpectedReturnType::ArrayOfKeyValuePairs), + .map(|_| ExpectedReturnType::ArrayOfMemberScorePairs), b"ZADD" => cmd .position(b"INCR") .map(|_| ExpectedReturnType::DoubleOrNull), @@ -517,7 +590,7 @@ mod tests { } #[test] - fn convert_array_of_kv_pairs() { + fn convert_to_array_of_pairs() { assert!(matches!( expected_type_for_cmd( redis::cmd("HRANDFIELD") @@ -525,25 +598,12 @@ mod tests { .arg("1") .arg("withvalues") ), - Some(ExpectedReturnType::ArrayOfKeyValuePairs) + Some(ExpectedReturnType::ArrayOfPairs) )); assert!(expected_type_for_cmd(redis::cmd("HRANDFIELD").arg("key").arg("1")).is_none()); assert!(expected_type_for_cmd(redis::cmd("HRANDFIELD").arg("key")).is_none()); - assert!(matches!( - expected_type_for_cmd( - redis::cmd("ZRANDMEMBER") - .arg("key") - .arg("1") - .arg("withscores") - ), - Some(ExpectedReturnType::ArrayOfKeyValuePairs) - )); - - assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key").arg("1")).is_none()); - assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key")).is_none()); - let flat_array = Value::Array(vec![ Value::BulkString(b"key1".to_vec()), Value::BulkString(b"value1".to_vec()), @@ -561,13 +621,13 @@ mod tests { ]), ]); let converted_flat_array = - convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfKeyValuePairs)) + convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfPairs)) .unwrap(); assert_eq!(two_dimensional_array, converted_flat_array); let converted_two_dimensional_array = convert_to_expected_type( two_dimensional_array.clone(), - Some(ExpectedReturnType::ArrayOfKeyValuePairs), + Some(ExpectedReturnType::ArrayOfPairs), ) .unwrap(); assert_eq!(two_dimensional_array, converted_two_dimensional_array); @@ -575,20 +635,94 @@ mod tests { let empty_array = Value::Array(vec![]); let converted_empty_array = convert_to_expected_type( empty_array.clone(), - Some(ExpectedReturnType::ArrayOfKeyValuePairs), + Some(ExpectedReturnType::ArrayOfPairs), + ) + .unwrap(); + assert_eq!(empty_array, converted_empty_array); + + let converted_nil_value = + convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)) + .unwrap(); + assert_eq!(Value::Nil, converted_nil_value); + + let array_of_doubles = Value::Array(vec![Value::Double(5.5)]); + assert!(convert_to_expected_type( + array_of_doubles, + Some(ExpectedReturnType::ArrayOfPairs) + ) + .is_err()); + } + + #[test] + fn convert_to_member_score_pairs() { + assert!(matches!( + expected_type_for_cmd( + redis::cmd("ZRANDMEMBER") + .arg("key") + .arg("1") + .arg("withscores") + ), + Some(ExpectedReturnType::ArrayOfPairs) + )); + + assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key").arg("1")).is_none()); + assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key")).is_none()); + + let flat_array = Value::Array(vec![ + Value::BulkString(b"one".to_vec()), + Value::BulkString(b"1.0".to_vec()), + Value::BulkString(b"two".to_vec()), + Value::BulkString(b"2.0".to_vec()), + ]); + let expected_response = Value::Array(vec![ + Value::Array(vec![ + Value::BulkString(b"one".to_vec()), + Value::Double(1.0), + ]), + Value::Array(vec![ + Value::BulkString(b"two".to_vec()), + Value::Double(2.0), + ]), + ]); + let converted_flat_array = + convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfMemberScorePairs)) + .unwrap(); + assert_eq!(expected_response, converted_flat_array); + + let array_of_pairs_string_scores = Value::Array(vec![ + Value::Array(vec![ + Value::BulkString(b"one".to_vec()), + Value::BulkString(b"1.0".to_vec()), + ]), + Value::Array(vec![ + Value::BulkString(b"two".to_vec()), + Value::BulkString(b"2.0".to_vec()), + ]), + ]); + let converted_array_of_pairs_string_scores = convert_to_expected_type( + array_of_pairs_string_scores.clone(), + Some(ExpectedReturnType::ArrayOfMemberScorePairs), + ) + .unwrap(); + assert_eq!(expected_response, converted_two_dimensional_array); + + let empty_array = Value::Array(vec![]); + let converted_empty_array = convert_to_expected_type( + empty_array.clone(), + Some(ExpectedReturnType::ArrayOfPairs), ) .unwrap(); assert_eq!(empty_array, converted_empty_array); let converted_nil_value = - convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfKeyValuePairs)) + convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)) .unwrap(); assert_eq!(Value::Nil, converted_nil_value); let array_of_doubles = Value::Array(vec![Value::Double(5.5)]); assert!(convert_to_expected_type( array_of_doubles, - Some(ExpectedReturnType::ArrayOfKeyValuePairs) + Some(ExpectedReturnType::ArrayOfPairs) ) .is_err()); } From 2d598d0ab4df4158a6b41efe0f5d16436a0fc0b5 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Fri, 10 May 2024 16:05:16 -0700 Subject: [PATCH 03/12] wip2 --- glide-core/src/client/value_conversion.rs | 122 +++++++++++----------- 1 file changed, 63 insertions(+), 59 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 47d9281f78..922465cd61 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -295,49 +295,47 @@ pub(crate) fn convert_to_expected_type( // - if the server returned nil, return nil // - if the server returned an empty array, return an empty array // - otherwise, return a two-dimensional array of member-score pairs, where scores are of type double - ExpectedReturnType::ArrayOfMemberScorePairs => match value { - Value::Nil => Ok(value), - Value::Array(ref array) if array.is_empty() => { - Ok(value) - } - Value::Array(array) if matches!(array[0], Value::Array(_)) => { - first_pair = array[0]; - if first_pair.len() != 2 { - Err(( - ErrorKind::TypeError, - "Expected an array of member-score pairs but encountered an inner array with unexpected length", - format!("(length was {:?}, expected length 2)", pair.len()), - ) - .into()); - } + ExpectedReturnType::ArrayOfMemberScorePairs => { + let array_of_pairs = convert_to_expected_type(value, Some(ExpectedReturnType::ArrayOfPairs))?; + match array_of_pairs { + Value::Nil => Ok(value), + Value::Array(ref array) => { + if array.is_empty() { + return Ok(array_of_pairs); + } + + let first_pair = array[0]; + if first_pair.len() != 2 { + Err(( + ErrorKind::TypeError, + "Expected an array of member-score pairs but encountered an inner array with unexpected length", + format!("(length was {:?}, expected length 2)", first_pair.len()), + ) + .into()); + } - match first_pair[1] => { - Value::Double(_) { - Ok(value) - }, - Value::BulkString(_) | Value::SimpleString(_) => { - convert_pair_scores_to_double(array) - }, - Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), - ) - .into()); - } - } - Value::Array(array) - if matches!(array[0], Value::BulkString(_) | Value::SimpleString(_)) => - { - array_of_pairs = convert_flat_array_to_array_of_pairs(array); - convert_pair_scores_to_double(array_of_pairs) + match first_pair[1] { + Value::Double(_) => { + Ok(value) + }, + Value::BulkString(_) | Value::SimpleString(_) => { + convert_pair_scores_to_double(array) + }, + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ) + .into()), + } + }, + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ) + .into()), } - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), - ) - .into()), }, } } @@ -442,9 +440,9 @@ fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult /// Given an array of member-score pairs, converts the scores in the pairs to type double. /// /// `member_score_pairs` is a two-dimensional array, where the inner arrays are length=2 arrays representing member-score pairs. -fn convert_pair_scores_to_double(member_score_pairs: Vec) -> RedisResul { - for i in (0..member_score_pairs.len()) { - pair = member_score_pairs[i] +fn convert_pair_scores_to_double(member_score_pairs: Vec) -> RedisResult { + for i in 0..member_score_pairs.len() { + let pair = member_score_pairs[i]; if pair.len() != 2 { return Err(( ErrorKind::TypeError, @@ -662,7 +660,7 @@ mod tests { .arg("1") .arg("withscores") ), - Some(ExpectedReturnType::ArrayOfPairs) + Some(ExpectedReturnType::ArrayOfMemberScorePairs) )); assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key").arg("1")).is_none()); @@ -689,18 +687,8 @@ mod tests { .unwrap(); assert_eq!(expected_response, converted_flat_array); - let array_of_pairs_string_scores = Value::Array(vec![ - Value::Array(vec![ - Value::BulkString(b"one".to_vec()), - Value::BulkString(b"1.0".to_vec()), - ]), - Value::Array(vec![ - Value::BulkString(b"two".to_vec()), - Value::BulkString(b"2.0".to_vec()), - ]), - ]); - let converted_array_of_pairs_string_scores = convert_to_expected_type( - array_of_pairs_string_scores.clone(), + let converted_two_dimensional_array = convert_to_expected_type( + expected_response.clone(), Some(ExpectedReturnType::ArrayOfMemberScorePairs), ) .unwrap(); @@ -719,9 +707,25 @@ mod tests { .unwrap(); assert_eq!(Value::Nil, converted_nil_value); - let array_of_doubles = Value::Array(vec![Value::Double(5.5)]); + let unexpected_pair_length = Value::Array(vec![ + Value::Array(vec![ + Value::BulkString(b"one".to_vec()), + ]), + ]); assert!(convert_to_expected_type( - array_of_doubles, + unexpected_pair_length, + Some(ExpectedReturnType::ArrayOfPairs) + ) + .is_err()); + + let unexpected_score_type = Value::Array(vec![ + Value::Array(vec![ + Value::BulkString(b"one".to_vec()), + Value::Int(1), + ]), + ]); + assert!(convert_to_expected_type( + unexpected_score_type, Some(ExpectedReturnType::ArrayOfPairs) ) .is_err()); From 8366d80801365e12ccbc17643342ee638236cb19 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Fri, 10 May 2024 16:30:17 -0700 Subject: [PATCH 04/12] wip3 --- glide-core/src/client/value_conversion.rs | 73 +++++++++++++---------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 922465cd61..622f6466b0 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -299,34 +299,36 @@ pub(crate) fn convert_to_expected_type( let array_of_pairs = convert_to_expected_type(value, Some(ExpectedReturnType::ArrayOfPairs))?; match array_of_pairs { Value::Nil => Ok(value), - Value::Array(ref array) => { + Value::Array(array) => { if array.is_empty() { return Ok(array_of_pairs); } - let first_pair = array[0]; - if first_pair.len() != 2 { - Err(( - ErrorKind::TypeError, - "Expected an array of member-score pairs but encountered an inner array with unexpected length", - format!("(length was {:?}, expected length 2)", first_pair.len()), - ) - .into()); - } + match array[0] { + Value::Array(first_pair) => { + if first_pair.len() != 2 { + return Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ).into()); + } - match first_pair[1] { - Value::Double(_) => { - Ok(value) - }, - Value::BulkString(_) | Value::SimpleString(_) => { - convert_pair_scores_to_double(array) - }, - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), - ) - .into()), + match first_pair[1] { + Value::Double(_) => { + Ok(value) + }, + Value::BulkString(_) | Value::SimpleString(_) => { + convert_pair_scores_to_double(array) + }, + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", value), + ) + .into()), + } + } } }, _ => Err(( @@ -442,17 +444,24 @@ fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult /// `member_score_pairs` is a two-dimensional array, where the inner arrays are length=2 arrays representing member-score pairs. fn convert_pair_scores_to_double(member_score_pairs: Vec) -> RedisResult { for i in 0..member_score_pairs.len() { - let pair = member_score_pairs[i]; - if pair.len() != 2 { - return Err(( + match member_score_pairs[0] { + Value::Array(pair) => { + if pair.len() != 2 { + return Err(( + ErrorKind::TypeError, + "Response value couldn't be converted to an array of member-score pairs", + format!("(response value was {:?})", member_score_pairs), + ).into()); + } + + pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; + } + _ => Err(( ErrorKind::TypeError, - "Expected an array of member-score pairs but encountered an inner array with unexpected length", - format!("(inner array length was {:?}, expected length 2)", pair.len()), - ) - .into()); + "Response value couldn't be converted to an array of member-score pairs", + format!("(response value was {:?})", member_score_pairs), + ).into()), } - - pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; } Ok(Value::Array(member_score_pairs)) From fc24ec72cd0781b2f0d937a8bb1d57b047139e46 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Fri, 10 May 2024 17:35:09 -0700 Subject: [PATCH 05/12] Value conversion compiling --- glide-core/src/client/value_conversion.rs | 59 ++++++++++++----------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 622f6466b0..ff952c6b29 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -296,45 +296,51 @@ pub(crate) fn convert_to_expected_type( // - if the server returned an empty array, return an empty array // - otherwise, return a two-dimensional array of member-score pairs, where scores are of type double ExpectedReturnType::ArrayOfMemberScorePairs => { - let array_of_pairs = convert_to_expected_type(value, Some(ExpectedReturnType::ArrayOfPairs))?; + let mut array_of_pairs = convert_to_expected_type(value, Some(ExpectedReturnType::ArrayOfPairs))?; match array_of_pairs { - Value::Nil => Ok(value), - Value::Array(array) => { + Value::Nil => Ok(array_of_pairs), + Value::Array(ref mut array) => { if array.is_empty() { return Ok(array_of_pairs); } - match array[0] { + match &array[0] { Value::Array(first_pair) => { if first_pair.len() != 2 { return Err(( ErrorKind::TypeError, "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), + format!("(response was {:?})", array_of_pairs), ).into()); } match first_pair[1] { Value::Double(_) => { - Ok(value) + Ok(array_of_pairs) }, Value::BulkString(_) | Value::SimpleString(_) => { - convert_pair_scores_to_double(array) + convert_pair_scores_to_double(array.to_vec()) }, _ => Err(( ErrorKind::TypeError, "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), + format!("(response was {:?})", array_of_pairs), ) .into()), } } + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", array_of_pairs), + ) + .into()), } }, _ => Err(( ErrorKind::TypeError, "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), + format!("(response was {:?})", array_of_pairs), ) .into()), } @@ -442,25 +448,24 @@ fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult /// Given an array of member-score pairs, converts the scores in the pairs to type double. /// /// `member_score_pairs` is a two-dimensional array, where the inner arrays are length=2 arrays representing member-score pairs. -fn convert_pair_scores_to_double(member_score_pairs: Vec) -> RedisResult { +fn convert_pair_scores_to_double(mut member_score_pairs: Vec) -> RedisResult { for i in 0..member_score_pairs.len() { - match member_score_pairs[0] { - Value::Array(pair) => { - if pair.len() != 2 { - return Err(( - ErrorKind::TypeError, - "Response value couldn't be converted to an array of member-score pairs", - format!("(response value was {:?})", member_score_pairs), - ).into()); - } - - pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; + if let Value::Array(ref mut pair) = &mut member_score_pairs[i] { + if pair.len() != 2 { + return Err(( + ErrorKind::TypeError, + "Response value couldn't be converted to an array of member-score pairs", + format!("(response value was {:?})", member_score_pairs), + ).into()); } - _ => Err(( + + pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; + } else { + return Err(( ErrorKind::TypeError, "Response value couldn't be converted to an array of member-score pairs", format!("(response value was {:?})", member_score_pairs), - ).into()), + ).into()); } } @@ -706,13 +711,13 @@ mod tests { let empty_array = Value::Array(vec![]); let converted_empty_array = convert_to_expected_type( empty_array.clone(), - Some(ExpectedReturnType::ArrayOfPairs), + Some(ExpectedReturnType::ArrayOfMemberScorePairs), ) .unwrap(); assert_eq!(empty_array, converted_empty_array); let converted_nil_value = - convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)) + convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfMemberScorePairs)) .unwrap(); assert_eq!(Value::Nil, converted_nil_value); @@ -723,7 +728,7 @@ mod tests { ]); assert!(convert_to_expected_type( unexpected_pair_length, - Some(ExpectedReturnType::ArrayOfPairs) + Some(ExpectedReturnType::ArrayOfMemberScorePairs) ) .is_err()); @@ -735,7 +740,7 @@ mod tests { ]); assert!(convert_to_expected_type( unexpected_score_type, - Some(ExpectedReturnType::ArrayOfPairs) + Some(ExpectedReturnType::ArrayOfMemberScorePairs) ) .is_err()); } From 536f551b4a22b2741f31cf2d93ac04bb15436b49 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Mon, 13 May 2024 09:03:41 -0700 Subject: [PATCH 06/12] Simplify Rust code by converting scores before array --- glide-core/src/client/value_conversion.rs | 160 +++++++--------------- 1 file changed, 47 insertions(+), 113 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index ff952c6b29..7cd4b39f55 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -296,51 +296,34 @@ pub(crate) fn convert_to_expected_type( // - if the server returned an empty array, return an empty array // - otherwise, return a two-dimensional array of member-score pairs, where scores are of type double ExpectedReturnType::ArrayOfMemberScorePairs => { - let mut array_of_pairs = convert_to_expected_type(value, Some(ExpectedReturnType::ArrayOfPairs))?; - match array_of_pairs { - Value::Nil => Ok(array_of_pairs), - Value::Array(ref mut array) => { - if array.is_empty() { - return Ok(array_of_pairs); - } - - match &array[0] { - Value::Array(first_pair) => { - if first_pair.len() != 2 { - return Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", array_of_pairs), - ).into()); - } - - match first_pair[1] { - Value::Double(_) => { - Ok(array_of_pairs) - }, - Value::BulkString(_) | Value::SimpleString(_) => { - convert_pair_scores_to_double(array.to_vec()) - }, - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", array_of_pairs), - ) - .into()), - } - } - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", array_of_pairs), - ) - .into()), + match value { + Value::Nil => Ok(value), + Value::Array(ref array) + if array.is_empty() || matches!(array[0], Value::Array(_)) => + { + // The server response is an empty array or a RESP3 array of pairs, with scores of type double, + // so it is already in the correct format. + Ok(value) + } + Value::Array(mut array) + if array.len() > 1 + && matches!(array[1], Value::BulkString(_) | Value::SimpleString(_)) => + { + // The server response is a RESP2 flat array with members at even indices and their associated + // scores as strings at odd indices. Here we convert the scores to type double and then convert the + // flat array to an array of pairs. + for i in (1..array.len()).step_by(2) { + array[i] = convert_to_expected_type( + array[i].clone(), + Some(ExpectedReturnType::Double), + )? } - }, + convert_flat_array_to_array_of_pairs(array) + } _ => Err(( ErrorKind::TypeError, "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", array_of_pairs), + format!("(response was {:?})", value), ) .into()), } @@ -445,33 +428,6 @@ fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult Ok(Value::Array(result)) } -/// Given an array of member-score pairs, converts the scores in the pairs to type double. -/// -/// `member_score_pairs` is a two-dimensional array, where the inner arrays are length=2 arrays representing member-score pairs. -fn convert_pair_scores_to_double(mut member_score_pairs: Vec) -> RedisResult { - for i in 0..member_score_pairs.len() { - if let Value::Array(ref mut pair) = &mut member_score_pairs[i] { - if pair.len() != 2 { - return Err(( - ErrorKind::TypeError, - "Response value couldn't be converted to an array of member-score pairs", - format!("(response value was {:?})", member_score_pairs), - ).into()); - } - - pair[1] = convert_to_expected_type(pair[1].clone(), Some(ExpectedReturnType::Double))?; - } else { - return Err(( - ErrorKind::TypeError, - "Response value couldn't be converted to an array of member-score pairs", - format!("(response value was {:?})", member_score_pairs), - ).into()); - } - } - - Ok(Value::Array(member_score_pairs)) -} - pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option { let command = cmd.command()?; @@ -633,8 +589,7 @@ mod tests { ]), ]); let converted_flat_array = - convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfPairs)) - .unwrap(); + convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfPairs)).unwrap(); assert_eq!(two_dimensional_array, converted_flat_array); let converted_two_dimensional_array = convert_to_expected_type( @@ -645,24 +600,20 @@ mod tests { assert_eq!(two_dimensional_array, converted_two_dimensional_array); let empty_array = Value::Array(vec![]); - let converted_empty_array = convert_to_expected_type( - empty_array.clone(), - Some(ExpectedReturnType::ArrayOfPairs), - ) - .unwrap(); + let converted_empty_array = + convert_to_expected_type(empty_array.clone(), Some(ExpectedReturnType::ArrayOfPairs)) + .unwrap(); assert_eq!(empty_array, converted_empty_array); let converted_nil_value = - convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)) - .unwrap(); + convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)).unwrap(); assert_eq!(Value::Nil, converted_nil_value); let array_of_doubles = Value::Array(vec![Value::Double(5.5)]); - assert!(convert_to_expected_type( - array_of_doubles, - Some(ExpectedReturnType::ArrayOfPairs) - ) - .is_err()); + assert!( + convert_to_expected_type(array_of_doubles, Some(ExpectedReturnType::ArrayOfPairs)) + .is_err() + ); } #[test] @@ -687,18 +638,14 @@ mod tests { Value::BulkString(b"2.0".to_vec()), ]); let expected_response = Value::Array(vec![ - Value::Array(vec![ - Value::BulkString(b"one".to_vec()), - Value::Double(1.0), - ]), - Value::Array(vec![ - Value::BulkString(b"two".to_vec()), - Value::Double(2.0), - ]), + Value::Array(vec![Value::BulkString(b"one".to_vec()), Value::Double(1.0)]), + Value::Array(vec![Value::BulkString(b"two".to_vec()), Value::Double(2.0)]), ]); - let converted_flat_array = - convert_to_expected_type(flat_array, Some(ExpectedReturnType::ArrayOfMemberScorePairs)) - .unwrap(); + let converted_flat_array = convert_to_expected_type( + flat_array, + Some(ExpectedReturnType::ArrayOfMemberScorePairs), + ) + .unwrap(); assert_eq!(expected_response, converted_flat_array); let converted_two_dimensional_array = convert_to_expected_type( @@ -716,28 +663,15 @@ mod tests { .unwrap(); assert_eq!(empty_array, converted_empty_array); - let converted_nil_value = - convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfMemberScorePairs)) - .unwrap(); - assert_eq!(Value::Nil, converted_nil_value); - - let unexpected_pair_length = Value::Array(vec![ - Value::Array(vec![ - Value::BulkString(b"one".to_vec()), - ]), - ]); - assert!(convert_to_expected_type( - unexpected_pair_length, - Some(ExpectedReturnType::ArrayOfMemberScorePairs) + let converted_nil_value = convert_to_expected_type( + Value::Nil, + Some(ExpectedReturnType::ArrayOfMemberScorePairs), ) - .is_err()); + .unwrap(); + assert_eq!(Value::Nil, converted_nil_value); - let unexpected_score_type = Value::Array(vec![ - Value::Array(vec![ - Value::BulkString(b"one".to_vec()), - Value::Int(1), - ]), - ]); + let unexpected_score_type = + Value::Array(vec![Value::BulkString(b"one".to_vec()), Value::Int(1)]); assert!(convert_to_expected_type( unexpected_score_type, Some(ExpectedReturnType::ArrayOfMemberScorePairs) From b06ee893e9e787f4e0adb3645abe7a6e069f2d99 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Mon, 13 May 2024 09:12:05 -0700 Subject: [PATCH 07/12] Fix rust formatting --- glide-core/src/client/value_conversion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 7cd4b39f55..f4514f9379 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -327,7 +327,7 @@ pub(crate) fn convert_to_expected_type( ) .into()), } - }, + } } } From bd11dfcef9540915a6a37e2658963d2156580698 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Mon, 13 May 2024 16:29:17 -0700 Subject: [PATCH 08/12] wip --- glide-core/src/client/value_conversion.rs | 103 ++++++++++++---------- 1 file changed, 54 insertions(+), 49 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index f4514f9379..6a3b36c36a 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -271,22 +271,15 @@ pub(crate) fn convert_to_expected_type( .into()), } } - ExpectedReturnType::ArrayOfPairs => match value { - Value::Nil => Ok(value), - Value::Array(ref array) if array.is_empty() || matches!(array[0], Value::Array(_)) => { - Ok(value) - } - Value::Array(array) - if matches!(array[0], Value::BulkString(_) | Value::SimpleString(_)) => - { - convert_flat_array_to_array_of_pairs(array) - } - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of key-value pairs", - format!("(response was {:?})", value), - ) - .into()), + // Used by HRANDFIELD when the WITHVALUES arg is passed. + // The server response can be a nil value, an empty array, a flat array of key-value pairs, or a two-dimensional array of key-value pairs. + // The conversions we do here are as follows: + // + // - if the server returned nil, return nil + // - if the server returned an empty array, return an empty array + // - otherwise, return a two-dimensional array of key-value pairs + ExpectedReturnType::ArrayOfPairs => { + convert_to_array_of_pairs(value, None) }, // Used by ZRANDMEMBER when the WITHSCORES arg is passed. // The server response can be a nil value, an empty array, a flat array of member-score pairs, or a two-dimensional array of member-score pairs. @@ -296,37 +289,8 @@ pub(crate) fn convert_to_expected_type( // - if the server returned an empty array, return an empty array // - otherwise, return a two-dimensional array of member-score pairs, where scores are of type double ExpectedReturnType::ArrayOfMemberScorePairs => { - match value { - Value::Nil => Ok(value), - Value::Array(ref array) - if array.is_empty() || matches!(array[0], Value::Array(_)) => - { - // The server response is an empty array or a RESP3 array of pairs, with scores of type double, - // so it is already in the correct format. - Ok(value) - } - Value::Array(mut array) - if array.len() > 1 - && matches!(array[1], Value::BulkString(_) | Value::SimpleString(_)) => - { - // The server response is a RESP2 flat array with members at even indices and their associated - // scores as strings at odd indices. Here we convert the scores to type double and then convert the - // flat array to an array of pairs. - for i in (1..array.len()).step_by(2) { - array[i] = convert_to_expected_type( - array[i].clone(), - Some(ExpectedReturnType::Double), - )? - } - convert_flat_array_to_array_of_pairs(array) - } - _ => Err(( - ErrorKind::TypeError, - "Response couldn't be converted to an array of member-score pairs", - format!("(response was {:?})", value), - ) - .into()), - } + // RESP2 returns scores as strings, but we want scores as type double. + convert_to_array_of_pairs(value, Some(ExpectedReturnType::Double)) } } } @@ -408,10 +372,49 @@ fn convert_array_to_map( Ok(Value::Map(map)) } +/// Used by commands like ZRANDMEMBER and HRANDFIELD. Normally a map would be more suitable for these key-value responses, but these commands may return duplicate key-value pairs depending on the command arguments. These duplicated pairs cannot be represented by a map. +/// +/// Converts a server response as follows: +/// - if the server returned nil, return nil. +/// - if the server returned an empty array, return an empty array. +/// - if the server returned a flat array (RESP2), convert it to a two-dimensional array, where the inner arrays are length=2 arrays representing key-value pairs. +/// - if the server returned a two-dimensional array (RESP3), return the response as is, since it is already in the correct format. +/// - otherwise, return an error. +/// +/// `response` is a server response that we should attempt to convert as described above. +/// `value_expected_return_type` indicates the desired return type of the values in the key-value pairs. +fn convert_to_array_of_pairs(response: Value, value_expected_return_type: Option) -> RedisResult { + match response { + Value::Nil => Ok(response), + Value::Array(ref array) + if array.is_empty() || matches!(array[0], Value::Array(_)) => + { + // The server response is an empty array or a RESP3 array of pairs. In RESP3, if the response contains + // scores, they will already be of type double, so `response` is already in the correct format. + Ok(response) + } + Value::Array(array) + if array.len() > 1 + && matches!(array[1], Value::BulkString(_) | Value::SimpleString(_)) => + { + // The server response is a RESP2 flat array with keys at even indices and their associated values at + // odd indices. + convert_flat_array_to_array_of_pairs(array, value_expected_return_type) + } + _ => Err(( + ErrorKind::TypeError, + "Response couldn't be converted to an array of member-score pairs", + format!("(response was {:?})", response), + ) + .into()), + } +} + /// Converts a flat array of values to a two-dimensional array, where the inner arrays are length=2 arrays representing key-value pairs. Normally a map would be more suitable for these responses, but some commands (eg HRANDFIELD) may return duplicate key-value pairs depending on the command arguments. These duplicated pairs cannot be represented by a map. /// /// `array` is a flat array containing keys at even-positioned elements and their associated values at odd-positioned elements. -fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult { +/// `value_expected_return_type` indicates the desired return type of the values in the key-value pairs. +fn convert_flat_array_to_array_of_pairs(array: Vec, value_expected_return_type: Option) -> RedisResult { if array.len() % 2 != 0 { return Err(( ErrorKind::TypeError, @@ -422,7 +425,9 @@ fn convert_flat_array_to_array_of_pairs(array: Vec) -> RedisResult let mut result = Vec::with_capacity(array.len() / 2); for i in (0..array.len()).step_by(2) { - let pair = vec![array[i].clone(), array[i + 1].clone()]; + let key = array[i].clone(); + let value = convert_to_expected_type(array[i + 1].clone(), value_expected_return_type)?; + let pair = vec![key, value]; result.push(Value::Array(pair)); } Ok(Value::Array(result)) From f8a21bcfdffda47b58c8d22ef7624fab08aedb80 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Tue, 14 May 2024 10:47:48 -0700 Subject: [PATCH 09/12] Polish value_conversion.rs --- glide-core/src/client/value_conversion.rs | 25 +++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 6a3b36c36a..4aa4abdd66 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -278,9 +278,7 @@ pub(crate) fn convert_to_expected_type( // - if the server returned nil, return nil // - if the server returned an empty array, return an empty array // - otherwise, return a two-dimensional array of key-value pairs - ExpectedReturnType::ArrayOfPairs => { - convert_to_array_of_pairs(value, None) - }, + ExpectedReturnType::ArrayOfPairs => convert_to_array_of_pairs(value, None), // Used by ZRANDMEMBER when the WITHSCORES arg is passed. // The server response can be a nil value, an empty array, a flat array of member-score pairs, or a two-dimensional array of member-score pairs. // The server response scores can be strings or doubles. The conversions we do here are as follows: @@ -382,19 +380,21 @@ fn convert_array_to_map( /// - otherwise, return an error. /// /// `response` is a server response that we should attempt to convert as described above. -/// `value_expected_return_type` indicates the desired return type of the values in the key-value pairs. -fn convert_to_array_of_pairs(response: Value, value_expected_return_type: Option) -> RedisResult { +/// `value_expected_return_type` indicates the desired return type of the values in the key-value pairs. The values will only be converted if the response was a flat array, since RESP3 already returns an array of pairs with values already of the correct type. +fn convert_to_array_of_pairs( + response: Value, + value_expected_return_type: Option, +) -> RedisResult { match response { Value::Nil => Ok(response), - Value::Array(ref array) - if array.is_empty() || matches!(array[0], Value::Array(_)) => - { - // The server response is an empty array or a RESP3 array of pairs. In RESP3, if the response contains - // scores, they will already be of type double, so `response` is already in the correct format. + Value::Array(ref array) if array.is_empty() || matches!(array[0], Value::Array(_)) => { + // The server response is an empty array or a RESP3 array of pairs. In RESP3, the values in the pairs are + // already of the correct type, so we do not need to convert them and `response` is in the correct format. Ok(response) } Value::Array(array) if array.len() > 1 + && array.len() % 2 == 0 && matches!(array[1], Value::BulkString(_) | Value::SimpleString(_)) => { // The server response is a RESP2 flat array with keys at even indices and their associated values at @@ -414,7 +414,10 @@ fn convert_to_array_of_pairs(response: Value, value_expected_return_type: Option /// /// `array` is a flat array containing keys at even-positioned elements and their associated values at odd-positioned elements. /// `value_expected_return_type` indicates the desired return type of the values in the key-value pairs. -fn convert_flat_array_to_array_of_pairs(array: Vec, value_expected_return_type: Option) -> RedisResult { +fn convert_flat_array_to_array_of_pairs( + array: Vec, + value_expected_return_type: Option, +) -> RedisResult { if array.len() % 2 != 0 { return Err(( ErrorKind::TypeError, From 07351eed54161b4455f696fd2d0485b19a3f887a Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Tue, 14 May 2024 10:47:59 -0700 Subject: [PATCH 10/12] PR suggestions --- python/python/glide/async_commands/core.py | 10 +++++----- python/python/glide/async_commands/transaction.py | 8 ++++---- python/python/tests/test_async_client.py | 1 - 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/python/python/glide/async_commands/core.py b/python/python/glide/async_commands/core.py index efa19fb27a..6af9295a30 100644 --- a/python/python/glide/async_commands/core.py +++ b/python/python/glide/async_commands/core.py @@ -983,7 +983,7 @@ async def hrandfield_count(self, key: str, count: int) -> List[str]: key (str): The key of the hash. count (int): The number of field names to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Returns: List[str]: A list of random field names from the hash. @@ -1010,7 +1010,7 @@ async def hrandfield_withvalues(self, key: str, count: int) -> List[List[str]]: key (str): The key of the hash. count (int): The number of field names to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Returns: List[List[str]]: A list of `[field_name, value]` lists, where `field_name` is a random field name from the @@ -2780,7 +2780,7 @@ async def zrandmember_count(self, key: str, count: int) -> List[str]: key (str): The key of the sorted set. count (int): The number of elements to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Returns: List[str]: A list of elements from the sorted set. @@ -2809,7 +2809,7 @@ async def zrandmember_withscores( key (str): The key of the sorted set. count (int): The number of elements to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Returns: List[List[Union[str, float]]]: A list of `[member, score]` lists, where `member` is a random member from @@ -2818,7 +2818,7 @@ async def zrandmember_withscores( Examples: >>> await client.zrandmember_withscores("my_sorted_set", -3) - [{"GLIDE": 1.0, "GLIDE": 1.0, "PYTHON": 2.0}] # "GLIDE" and "PYTHON" are random members of "my_sorted_set", and have scores of 1.0 and 2.0, respectively. + [["GLIDE", 1.0], ["GLIDE", 1.0], ["PYTHON", 2.0]] # "GLIDE" and "PYTHON" are random members of "my_sorted_set", and have scores of 1.0 and 2.0, respectively. >>> await client.zrandmember_withscores("non_existing_sorted_set", 3) [] # "non_existing_sorted_set" is not an existing key, so an empty list was returned. """ diff --git a/python/python/glide/async_commands/transaction.py b/python/python/glide/async_commands/transaction.py index 883380fd10..a49c04b934 100644 --- a/python/python/glide/async_commands/transaction.py +++ b/python/python/glide/async_commands/transaction.py @@ -636,7 +636,7 @@ def hrandfield_count(self: TTransaction, key: str, count: int) -> TTransaction: key (str): The key of the hash. count (int): The number of field names to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Command response: List[str]: A list of random field names from the hash. @@ -654,7 +654,7 @@ def hrandfield_withvalues(self: TTransaction, key: str, count: int) -> TTransact key (str): The key of the hash. count (int): The number of field names to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Command response: List[List[str]]: A list of `[field_name, value]` lists, where `field_name` is a random field name from the @@ -1991,7 +1991,7 @@ def zrandmember_count(self: TTransaction, key: str, count: int) -> TTransaction: key (str): The key of the sorted set. count (int): The number of elements to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Command response: List[str]: A list of elements from the sorted set. @@ -2011,7 +2011,7 @@ def zrandmember_withscores( key (str): The key of the sorted set. count (int): The number of elements to return. If `count` is positive, returns unique elements. - If negative, allows for duplicates. + If `count` is negative, allows for duplicates elements. Command response: List[List[Union[str, float]]]: A list of `[member, score]` lists, where `member` is a random member from diff --git a/python/python/tests/test_async_client.py b/python/python/tests/test_async_client.py index aca0d35dd0..d3c6c9ea88 100644 --- a/python/python/tests/test_async_client.py +++ b/python/python/tests/test_async_client.py @@ -2433,7 +2433,6 @@ async def test_zrandmember_withscores(self, redis_client: TRedisClient): # unique values are expected as count is positive elements = await redis_client.zrandmember_withscores(key, 4) assert len(elements) == 2 - print("The score type: " + type(elements[0][1]).__name__) for element in elements: member = element[0] From b7b6b8b5a4c8deef12c81c1e33bb1b5867976ac4 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Tue, 14 May 2024 11:16:47 -0700 Subject: [PATCH 11/12] More value conversion cleanup --- glide-core/src/client/value_conversion.rs | 46 +++++------------------ 1 file changed, 9 insertions(+), 37 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 4aa4abdd66..7d3e9820a6 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -393,9 +393,8 @@ fn convert_to_array_of_pairs( Ok(response) } Value::Array(array) - if array.len() > 1 - && array.len() % 2 == 0 - && matches!(array[1], Value::BulkString(_) | Value::SimpleString(_)) => + if array.len() % 2 == 0 + && matches!(array[0], Value::BulkString(_) | Value::SimpleString(_)) => { // The server response is a RESP2 flat array with keys at even indices and their associated values at // odd indices. @@ -566,7 +565,7 @@ mod tests { } #[test] - fn convert_to_array_of_pairs() { + fn convert_to_array_of_pairs_return_type() { assert!(matches!( expected_type_for_cmd( redis::cmd("HRANDFIELD") @@ -617,15 +616,15 @@ mod tests { convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)).unwrap(); assert_eq!(Value::Nil, converted_nil_value); - let array_of_doubles = Value::Array(vec![Value::Double(5.5)]); + let flat_array_unexpected_length = Value::Array(vec![Value::BulkString(b"somekey".to_vec())]); assert!( - convert_to_expected_type(array_of_doubles, Some(ExpectedReturnType::ArrayOfPairs)) + convert_to_expected_type(flat_array_unexpected_length, Some(ExpectedReturnType::ArrayOfPairs)) .is_err() ); } #[test] - fn convert_to_member_score_pairs() { + fn convert_to_member_score_pairs_return_type() { assert!(matches!( expected_type_for_cmd( redis::cmd("ZRANDMEMBER") @@ -639,6 +638,9 @@ mod tests { assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key").arg("1")).is_none()); assert!(expected_type_for_cmd(redis::cmd("ZRANDMEMBER").arg("key")).is_none()); + // convert_to_array_of_pairs_return_type already tests most functionality since the conversion for ArrayOfPairs + // and ArrayOfMemberScorePairs is mostly the same. Here we also test that the scores are converted to double + // when the server response was a RESP2 flat array. let flat_array = Value::Array(vec![ Value::BulkString(b"one".to_vec()), Value::BulkString(b"1.0".to_vec()), @@ -655,36 +657,6 @@ mod tests { ) .unwrap(); assert_eq!(expected_response, converted_flat_array); - - let converted_two_dimensional_array = convert_to_expected_type( - expected_response.clone(), - Some(ExpectedReturnType::ArrayOfMemberScorePairs), - ) - .unwrap(); - assert_eq!(expected_response, converted_two_dimensional_array); - - let empty_array = Value::Array(vec![]); - let converted_empty_array = convert_to_expected_type( - empty_array.clone(), - Some(ExpectedReturnType::ArrayOfMemberScorePairs), - ) - .unwrap(); - assert_eq!(empty_array, converted_empty_array); - - let converted_nil_value = convert_to_expected_type( - Value::Nil, - Some(ExpectedReturnType::ArrayOfMemberScorePairs), - ) - .unwrap(); - assert_eq!(Value::Nil, converted_nil_value); - - let unexpected_score_type = - Value::Array(vec![Value::BulkString(b"one".to_vec()), Value::Int(1)]); - assert!(convert_to_expected_type( - unexpected_score_type, - Some(ExpectedReturnType::ArrayOfMemberScorePairs) - ) - .is_err()); } #[test] From 8f37a9a2c419e8a02c45e35c41fdfb0248ebdb83 Mon Sep 17 00:00:00 2001 From: aaron-congo Date: Tue, 14 May 2024 11:37:04 -0700 Subject: [PATCH 12/12] Fix rust formatting --- glide-core/src/client/value_conversion.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index 7d3e9820a6..ca4c217cc3 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -616,11 +616,13 @@ mod tests { convert_to_expected_type(Value::Nil, Some(ExpectedReturnType::ArrayOfPairs)).unwrap(); assert_eq!(Value::Nil, converted_nil_value); - let flat_array_unexpected_length = Value::Array(vec![Value::BulkString(b"somekey".to_vec())]); - assert!( - convert_to_expected_type(flat_array_unexpected_length, Some(ExpectedReturnType::ArrayOfPairs)) - .is_err() - ); + let flat_array_unexpected_length = + Value::Array(vec![Value::BulkString(b"somekey".to_vec())]); + assert!(convert_to_expected_type( + flat_array_unexpected_length, + Some(ExpectedReturnType::ArrayOfPairs) + ) + .is_err()); } #[test]