Python: fixing a bug with transaction exec (valkey-io#1796)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

CHANGELOG.md

@@ -85,12 +85,15 @@
 * Python: Added XINFO STREAM command ([#1816](https://github.com/aws/glide-for-redis/pull/1816))
 * Python: Added transaction supports for DUMP, RESTORE, FUNCTION DUMP and FUNCTION RESTORE ([#1814](https://github.com/aws/glide-for-redis/pull/1814))
 
-### Breaking Changes
+#### Breaking Changes
 * Node: Update XREAD to return a Map of Map ([#1494](https://github.com/aws/glide-for-redis/pull/1494))
 * Node: Rename RedisClient to GlideClient and RedisClusterClient to GlideClusterClient ([#1670](https://github.com/aws/glide-for-redis/pull/1670))
 * Python: Rename RedisClient to GlideClient, RedisClusterClient to GlideClusterClient and BaseRedisClient to BaseClient([#1669](https://github.com/aws/glide-for-redis/pull/1669))
 * Python: Rename ClusterClientConfiguration to GlideClusterClientConfiguration ([#1806](https://github.com/aws/glide-for-redis/pull/1806))
 
+#### Fixes
+* Python: fixing a bug with transaction exec ([#1796](https://github.com/aws/glide-for-redis/pull/1796))
+
 ## 0.4.1 (2024-02-06)
 
 #### Fixes

python/python/glide/async_commands/cluster_commands.py

@@ -12,7 +12,7 @@
     InfoSection,
     _build_sort_args,
 )
-from glide.async_commands.transaction import BaseTransaction, ClusterTransaction
+from glide.async_commands.transaction import ClusterTransaction
 from glide.constants import (
     TOK,
     TClusterResponse,
@@ -83,24 +83,24 @@ async def info(
 
     async def exec(
         self,
-        transaction: BaseTransaction | ClusterTransaction,
+        transaction: ClusterTransaction,
         route: Optional[TSingleNodeRoute] = None,
     ) -> Optional[List[TResult]]:
         """
         Execute a transaction by processing the queued commands.
         See https://redis.io/topics/Transactions/ for details on Transactions.
 
         Args:
-            transaction (ClusterTransaction): A ClusterTransaction object containing a list of commands to be executed.
+            transaction (ClusterTransaction): A `ClusterTransaction` object containing a list of commands to be executed.
             route (Optional[TSingleNodeRoute]): If `route` is not provided, the transaction will be routed to the slot owner of the
-            first key found in the transaction. If no key is found, the command will be sent to a random node.
-            If `route` is provided, the client will route the command to the nodes defined by `route`.
+                first key found in the transaction. If no key is found, the command will be sent to a random node.
+                If `route` is provided, the client will route the command to the nodes defined by `route`.
 
         Returns:
             Optional[List[TResult]]: A list of results corresponding to the execution of each command
-            in the transaction. If a command returns a value, it will be included in the list. If a command
-            doesn't return a value, the list entry will be None.
-            If the transaction failed due to a WATCH command, `exec` will return `None`.
+                in the transaction. If a command returns a value, it will be included in the list. If a command
+                doesn't return a value, the list entry will be `None`.
+                If the transaction failed due to a WATCH command, `exec` will return `None`.
         """
         commands = transaction.commands[:]
         return await self._execute_transaction(commands, route)

python/python/glide/async_commands/standalone_commands.py

@@ -12,7 +12,7 @@
     InfoSection,
     _build_sort_args,
 )
-from glide.async_commands.transaction import BaseTransaction, Transaction
+from glide.async_commands.transaction import Transaction
 from glide.constants import (
     OK,
     TOK,
@@ -66,20 +66,20 @@ async def info(
 
     async def exec(
         self,
-        transaction: BaseTransaction | Transaction,
+        transaction: Transaction,
     ) -> Optional[List[TResult]]:
         """
         Execute a transaction by processing the queued commands.
         See https://redis.io/topics/Transactions/ for details on Transactions.
 
         Args:
-            transaction (Transaction): A Transaction object containing a list of commands to be executed.
+            transaction (Transaction): A `Transaction` object containing a list of commands to be executed.
 
         Returns:
             Optional[List[TResult]]: A list of results corresponding to the execution of each command
-            in the transaction. If a command returns a value, it will be included in the list. If a command
-            doesn't return a value, the list entry will be None.
-            If the transaction failed due to a WATCH command, `exec` will return `None`.
+                in the transaction. If a command returns a value, it will be included in the list. If a command
+                doesn't return a value, the list entry will be `None`.
+                If the transaction failed due to a WATCH command, `exec` will return `None`.
         """
         commands = transaction.commands[:]
         return await self._execute_transaction(commands)

python/python/tests/test_transaction.py

@@ -2,7 +2,7 @@
 
 import time
 from datetime import date, datetime, timedelta, timezone
-from typing import List, Union, cast
+from typing import List, Optional, Union, cast
 
 import pytest
 from glide import RequestError
@@ -777,14 +777,28 @@ async def transaction_test(
 
 @pytest.mark.asyncio
 class TestTransaction:
+
+    async def exec_transaction(
+        self, glide_client: TGlideClient, transaction: BaseTransaction
+    ) -> Optional[List[TResult]]:
+        """
+        Exec a transaction on a client with proper typing. Casts are required to satisfy `mypy`.
+        """
+        if isinstance(glide_client, GlideClient):
+            return await cast(GlideClient, glide_client).exec(
+                cast(Transaction, transaction)
+            )
+        else:
+            return await cast(GlideClusterClient, glide_client).exec(
+                cast(ClusterTransaction, transaction)
+            )
+
     @pytest.mark.parametrize("cluster_mode", [True])
     @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
-    async def test_transaction_with_different_slots(self, glide_client: TGlideClient):
-        transaction = (
-            Transaction()
-            if isinstance(glide_client, GlideClient)
-            else ClusterTransaction()
-        )
+    async def test_transaction_with_different_slots(
+        self, glide_client: GlideClusterClient
+    ):
+        transaction = ClusterTransaction()
         transaction.set("key1", "value1")
         transaction.set("key2", "value2")
         with pytest.raises(RequestError, match="CrossSlot"):
@@ -801,7 +815,7 @@ async def test_transaction_custom_command(self, glide_client: TGlideClient):
         )
         transaction.custom_command(["HSET", key, "foo", "bar"])
         transaction.custom_command(["HGET", key, "foo"])
-        result = await glide_client.exec(transaction)
+        result = await self.exec_transaction(glide_client, transaction)
         assert result == [1, b"bar"]
 
     @pytest.mark.parametrize("cluster_mode", [True, False])
@@ -817,7 +831,7 @@ async def test_transaction_custom_unsupported_command(
         )
         transaction.custom_command(["WATCH", key])
         with pytest.raises(RequestError) as e:
-            await glide_client.exec(transaction)
+            await self.exec_transaction(glide_client, transaction)
         assert "WATCH inside MULTI is not allowed" in str(
             e
         )  # TODO : add an assert on EXEC ABORT
@@ -836,7 +850,7 @@ async def test_transaction_discard_command(self, glide_client: TGlideClient):
         transaction.custom_command(["INCR", key])
         transaction.custom_command(["DISCARD"])
         with pytest.raises(RequestError) as e:
-            await glide_client.exec(transaction)
+            await self.exec_transaction(glide_client, transaction)
         assert "EXEC without MULTI" in str(e)  # TODO : add an assert on EXEC ABORT
         value = await glide_client.get(key)
         assert value == b"1"
@@ -848,7 +862,7 @@ async def test_transaction_exec_abort(self, glide_client: TGlideClient):
         transaction = BaseTransaction()
         transaction.custom_command(["INCR", key, key, key])
         with pytest.raises(RequestError) as e:
-            await glide_client.exec(transaction)
+            await self.exec_transaction(glide_client, transaction)
         assert "wrong number of arguments" in str(
             e
         )  # TODO : add an assert on EXEC ABORT
@@ -894,7 +908,7 @@ async def test_can_return_null_on_watch_transaction_failures(
         result2 = await client2.set(keyslot, "foo")
         assert result2 == OK
 
-        result3 = await glide_client.exec(transaction)
+        result3 = await self.exec_transaction(glide_client, transaction)
         assert result3 is None
 
         await client2.close()
@@ -999,7 +1013,8 @@ async def test_transaction_chaining_calls(self, glide_client: TGlideClient):
         transaction = ClusterTransaction() if cluster_mode else Transaction()
         transaction.set(key, "value").get(key).delete([key])
 
-        assert await glide_client.exec(transaction) == [OK, b"value", 1]
+        result = await self.exec_transaction(glide_client, transaction)
+        assert result == [OK, b"value", 1]
 
     # The object commands are tested here instead of transaction_test because they have special requirements:
     # - OBJECT FREQ and OBJECT IDLETIME require specific maxmemory policies to be set on the config
@@ -1029,7 +1044,7 @@ async def test_transaction_object_commands(
             transaction.config_set({maxmemory_policy_key: "allkeys-random"})
             transaction.object_idletime(string_key)
 
-            response = await glide_client.exec(transaction)
+            response = await self.exec_transaction(glide_client, transaction)
             assert response is not None
             assert response[0] == OK  # transaction.set(string_key, "foo")
             assert response[1] == b"embstr"  # transaction.object_encoding(string_key)
@@ -1083,7 +1098,7 @@ async def test_transaction_lastsave(
         yesterday_unix_time = time.mktime(yesterday.timetuple())
         transaction = ClusterTransaction() if cluster_mode else Transaction()
         transaction.lastsave()
-        response = await glide_client.exec(transaction)
+        response = await self.exec_transaction(glide_client, transaction)
         assert isinstance(response, list)
         lastsave_time = response[0]
         assert isinstance(lastsave_time, int)
@@ -1092,7 +1107,7 @@ async def test_transaction_lastsave(
     @pytest.mark.parametrize("cluster_mode", [True])
     @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
     async def test_lolwut_transaction(self, glide_client: GlideClusterClient):
-        transaction = Transaction()
+        transaction = ClusterTransaction()
         transaction.lolwut().lolwut(5).lolwut(parameters=[1, 2]).lolwut(6, [42])
         results = await glide_client.exec(transaction)
         assert results is not None