Repository: redis/redis-py
Pull request #3456 (2024-12-12 12:31:07)

Fix incorrect attribute reuse in redis.connection.CacheProxyConnection
### Pull Request check-list

_Please make sure to review and check all of these items:_

- [ ] Do tests and lints pass with this change?
- [ ] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [x] Is the new or changed code fully tested?
- [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [ ] Is there an example added to the examples folder (if applicable)?
- [ ] Was the change added to CHANGES file?

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._

### Description of change

Attemp to adress https://github.com/redis/redis-py/issues/3453

`CacheProxyConnection` adds caching capabilities to the `send_command` and `read_response` methods, but does not clear the `_current_command_cache_key` attribute. `Pipeline` and `ClusterPipeline` use `send_packed_command` to send requests, which may cause the wrong `_current_command_cache_key` to be reused during read_response.

vladvildanov: @zs-neo Thanks for your contribution! Will mark this as a bug and will be included into next patch release
vladvildanov: @zs-neo Few unit tests are still failing
zs-neo: @vladvildanov Thanks to the useful comments in CacheProxyConnection, which helped me understand the relevant logic. I tried to fix the remaining problems, please take a look at this PR again, any suggestions are welcome.
vladvildanov: @zs-neo Thank you for your input! Let's finish it up and I will include it into next 5.3 GA
zs-neo: `tests.test_auth.test_token_manager.TestTokenManager.test_async_success_token_renewal` failed, I'll check again.
```
2024-12-23T12:43:09.9515473Z >       assert len(tokens) == tokens_refreshed
2024-12-23T12:43:09.9516084Z E       assert 3 == 2
2024-12-23T12:43:09.9517386Z E        +  where 3 = len([<redis.auth.token.SimpleToken object at 0x0000000009d708e0>, <redis.auth.token.SimpleToken object at 0x0000000009d70918>, <redis.auth.token.SimpleToken object at 0x0000000009d70950>])
```
vladvildanov: @zs-neo It's a flacky test, I'll fix and you could rebase a branch
zs-neo: > @zs-neo It's a flacky test, I'll fix and you could rebase a branch

Thanks a lot, I tried to fix it but I'm not too familiar with TokenManager, it would be great if you could fix it.

Patch:
diff --git a/redis/connection.py b/redis/connection.py
index 9d29b4ab..ace1ed87 100644
--- a/redis/connection.py
+++ b/redis/connection.py
@@ -904,9 +904,11 @@ class CacheProxyConnection(ConnectionInterface):
                 and self._cache.get(self._current_command_cache_key).status
                 != CacheEntryStatus.IN_PROGRESS
             ):
-                return copy.deepcopy(
+                res = copy.deepcopy(
                     self._cache.get(self._current_command_cache_key).cache_value
                 )
+                self._current_command_cache_key = None
+                return res
 
         response = self._conn.read_response(
             disable_decoding=disable_decoding,
@@ -932,6 +934,8 @@ class CacheProxyConnection(ConnectionInterface):
                 cache_entry.cache_value = response
                 self._cache.set(cache_entry)
 
+            self._current_command_cache_key = None
+
         return response
 
     def pack_command(self, *args):


Test patch:
diff --git a/tests/test_connection.py b/tests/test_connection.py
index 65d80e25..fbc23ae8 100644
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -501,9 +501,9 @@ class TestUnitCacheProxyConnection:
         )
         proxy_connection.send_command(*["GET", "foo"], **{"keys": ["foo"]})
         assert proxy_connection.read_response() == b"bar"
+        assert proxy_connection._current_command_cache_key is None
         assert proxy_connection.read_response() == b"bar"
 
-        mock_connection.read_response.assert_called_once()
         mock_cache.set.assert_has_calls(
             [
                 call(
@@ -530,9 +530,6 @@ class TestUnitCacheProxyConnection:
                 call(CacheKey(command="GET", redis_keys=("foo",))),
                 call(CacheKey(command="GET", redis_keys=("foo",))),
                 call(CacheKey(command="GET", redis_keys=("foo",))),
-                call(CacheKey(command="GET", redis_keys=("foo",))),
-                call(CacheKey(command="GET", redis_keys=("foo",))),
-                call(CacheKey(command="GET", redis_keys=("foo",))),
             ]
         )