Repository navigation
Return raw bytes from CF.SCANDUMP when decode_responses=True - #4574
Conversation
CF.SCANDUMP returns binary filter data, but cf().scandump() did not pass NEVER_DECODE the way bf().scandump() does. On a client with decode_responses=True the reply was decoded as UTF-8, which raises UnicodeDecodeError for most filters. Signed-off-by: abhishekacharya200 <abhishekacharya200@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f828559d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| @pytest.mark.redismod | ||
| async def test_cf_scandump_and_loadchunk(decoded_r: redis.Redis): |
There was a problem hiding this comment.
Skip the async CF dump test on Redis Enterprise
When RE_CLUSTER=true routes the module URL to Redis Enterprise, this async test runs without the Enterprise guard on the synchronous test_cf_scandump_and_loadchunk twin. That test is explicitly excluded there, while this test uses a cursor loop with no timeout, so the same server behavior can fail or hang the async suite instead of being skipped; apply the same guard. The repository requires sync and async paths to remain aligned.
AGENTS.md reference: AGENTS.md:L252-L254
Useful? React with 👍 / 👎.
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @abhishekacharya200, thank you for your contribution!
The fix is correct. CF.SCANDUMP returns binary chunks, the return type already declares bytes, and this now matches how bf().scandump() handles it. The round trip tests cover both the sync and async clients.
About the Codex note on the async test: the existing async test_bf_scandump_and_loadchunk has no Redis Enterprise guard either, so the new test is consistent with it. No change is needed there.
Description of change
cf().scandump()fails on a client created withdecode_responses=True:CF.SCANDUMP returns binary filter data.
bf().scandump()already passesNEVER_DECODEfor this reason, but the Cuckoo filter version never did, so the chunk is decoded as UTF-8. This makesCFCommands.scandumppassNEVER_DECODEthe same way, so the chunks come back asbytesand can be handed toloadchunk()as is. Sync and asyncio share the mixin, so both are fixed.Tests: a scandump/loadchunk round trip for CF in
tests/test_bloom.pyandtests/test_asyncio/test_bloom.py, plus a small unit test that checks the command is sent withNEVER_DECODEand doesn't need a server. The new tests fail on master and pass with the change. I ran both bloom test files against redis-stack-server with the default protocol,--protocol=2,--protocol=3and--legacy-responses=false.Pull Request check-list
Note
Low Risk
Narrow bugfix in RedisBloom command wiring with tests; behavior change only affects CF.SCANDUMP response decoding when decode_responses is enabled.
Overview
Fixes
cf().scandump()raisingUnicodeDecodeErroron clients created withdecode_responses=True, because Cuckoo dump chunks are binary while the command was still letting the client decode them as UTF-8.CFCommands.scandumpnow passesNEVER_DECODEthroughexecute_command, matchingbf().scandump()so chunk payloads staybytesand work withloadchunk()unchanged (sync and asyncio share the same mixin).Adds integration tests for a full scandump → delete → loadchunk round trip (sync and asyncio, with
decode_responses=True) and a small mock test thatCF.SCANDUMPis issued withNEVER_DECODE.Reviewed by Cursor Bugbot for commit d1deaf6. Bugbot is set up for automated code reviews on this repo. Configure here.