-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[#3618] Fix client-side cache invalidation for mixed str
and bytes
Redis keys
#3766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[#3618] Fix client-side cache invalidation for mixed str
and bytes
Redis keys
#3766
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a client-side cache invalidation bug in redis-py where cache entries were not being properly deleted when Redis keys were provided in different formats (str vs bytes) than how they were originally stored.
- Modified
delete_by_redis_keys
method to handle bothstr
andbytes
key formats by checking for both versions during cache lookup - Updated the method signature to accept
Union[List[bytes], List[str]]
instead of justList[bytes]
- Added comprehensive test coverage for mixed key type scenarios including non-UTF-8 bytes handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
redis/cache.py | Enhanced delete_by_redis_keys to support mixed str/bytes key deletion with UTF-8 encoding/decoding |
tests/test_cache.py | Added test case for non-UTF-8 bytes key handling in cache invalidation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
for cache_key in self._cache: | ||
if redis_key in cache_key.redis_keys: | ||
if any(candidate in cache_key.redis_keys for candidate in candidates): | ||
keys_to_delete.append(cache_key) | ||
response.append(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic appends True
to response
for each cache key that matches, but it should append one boolean per input redis_key
. If multiple cache keys match the same redis_key
, this will append multiple True
values, making the response list longer than the input list.
Copilot uses AI. Check for mistakes.
Hi @ShubhamKaudewar, thank you for your contribution! I'll review your change soon. Meanwhile, can you please check the Copilot's comments and linters failures? |
…s_key' into fix_client_side_cache_using_bytes_key
I have checked the copilot's reply, it did not provide correct suggestion and fixed linter failures |
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Problem:
Client-side caching in
redis-py
was not consistently invalidating cache entries when Redis keys were passed asbytes
while stored asstr
(or vice versa) as mentioned issue #3618. This caused stale cache values to persist in cases where keys were mixed betweenstr
andbytes
.Cause:
The
delete_by_redis_keys
method previously assumed all Redis keys werestr
and attempted deletion by encoding bytes to strings. After recent changes that normalized input to bytes, cache entries stored asstr
were no longer being matched, leading to failed deletions.Change / Fix:
delete_by_redis_keys
to temporarily support bothstr
andbytes
keys:str
, it checks the cache for both thestr
and UTF-8 encodedbytes
version.bytes
, it checks both thebytes
and UTF-8 decodedstr
version (if possible).str
/bytes
scenarios, including overlapping keys and MGET commands.Benefit:
str
keys.Testing:
bytes
forstr
keys.str
forbytes
keys.