Conversation
Remove the legacy fallback in zrange(desc=True) that delegated to zrevrange() which sends the deprecated ZREVRANGE command. Since redis-py >= 6.0.0 only supports Redis 7.2+, and Redis 7.2+ supports ZRANGE ... REV, the _zrange helper already handles desc correctly by appending REV to the ZRANGE command. This means zrange(desc=True) now sends ZRANGE ... REV instead of ZREVRANGE, aligning with the Redis deprecation roadmap. Fixes redis#4268 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Hey @aryansk, thank you for your contribution! I will have a look at it shortly. |
Mukller
left a comment
There was a problem hiding this comment.
Verified locally by diffing the command-construction behavior of zrange(desc=True, ...) between installed redis-py 8.1.0 (has the fallback) and this branch, using a captured execute_command (no server needed):
| call | 8.1.0 (before) | this branch (after) |
|---|---|---|
zrange('z', 0, -1, desc=True) |
ZREVRANGE |
ZRANGE ... REV |
zrange('z', 0, -1, desc=True, withscores=True) |
ZREVRANGE |
ZRANGE ... REV WITHSCORES |
zrange('z', 0, 5, desc=True, offset=1, num=2) |
already ZRANGE |
ZRANGE ... REV LIMIT |
So before this PR the same logical operation dispatched to two different commands depending on whether offset/num were passed; after, it's consistent. Checked the compatibility angles that make the removal safe:
- Server version floor: README states support starts at Redis 7.2 (7.2/7.4/8.x). The
REVargument shipped in Redis 6.2, well below the floor — the old fallback only served servers the library no longer targets. - Escape hatch intact: the public
zrevrange()method still exists for anyone on older servers who wants the deprecated command explicitly. - Reply format:
ZREVRANGE ... WITHSCORESandZRANGE ... REV WITHSCORESshare the same wire format and response callback, so parsed results (includingscore_cast_func, which I confirmed flows through_zrange) are unchanged.
Also grepped the tree: remaining ZREVRANGE references are command registries (cache allowlist, cluster commands) plus the kept zrevrange* methods themselves — nothing else silently depends on the removed branch.
Looks correct. One CI note: the single failing job is "Redis 7.2.14; Python 3.10; default-legacy_responses" — 7.2 fully supports ZRANGE REV, so this looks unrelated/flaky rather than caused by the change; worth a re-run.
|
Thank you @petyaslavova — happy to adjust anything that comes up in review. |
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @aryansk!
The direction is right: the branch you removed only exists for servers below Redis 6.2, the byscore and bylex reverse paths already emit ZRANGE ... REV, and _zrange builds the same options and shares identical response callbacks, so returned values are unchanged. No async mirror is needed, since AsyncSortedSetCommands is the same mixin object. Also, please ignore the one failing CI job: it is test_context_manager_not_raise_on_release_lock_error, a 100 ms lock timing flake unrelated to this change, and we will re-run it.
We do not want to break users who are still on Redis 6.0 or 6.1, so we cannot flip the default outside a major release. Could you rework this as an opt-in? Keep zrange(desc=True) sending ZREVRANGE by default, and add a keyword that selects ZRANGE ... REV for callers who want the non-deprecated form. It needs to go on the two @overload signatures as well as the implementation, with a docstring note that it only affects the rank-based reverse path.
Please also add regression tests asserting the command actually sent on both branches, including withscores=True with a custom score_cast_func, since test_zrange only asserts returned values today and passes either way. See test_client_kill_filter_accepts_replica_type for the mock.patch.object(r, "execute_command") pattern. Once that is in and the PR is marked ready for review, it should be good for another look. We will flip the default in the next major release and document zrevrange() as the path for servers older than 6.2.
Summary
Fixes #4268
Remove the legacy fallback in
zrange(desc=True)that delegated tozrevrange()which sends the deprecatedZREVRANGEcommand.Problem
Since redis-py >= 6.0.0 only supports Redis 7.2+, and Redis 7.2+ supports
ZRANGE ... REV, the_zrangehelper already handlesdesccorrectly by appendingREVto theZRANGEcommand. But thezrangemethod had a shortcut that bypassed_zrangeand calledzrevrange()directly, sending the deprecatedZREVRANGEcommand.Fix
Removed the 5-line legacy fallback.
zrange(desc=True)now sendsZRANGE ... REVinstead ofZREVRANGE, aligning with the Redis deprecation roadmap.The existing
zrevrange()method is preserved for direct callers who want the explicitZREVRANGEcommand.🤖 Generated with Codebuff
Co-Authored-By: Codebuff noreply@codebuff.com