Pass zrevrange key as a list in options["keys"] to match sibling range commands - #4244
Conversation
ZREVRANGE stored its key in options["keys"] as a bare string, while every
sibling range command (zrange, zrangebyscore, zrevrangebyscore) passes it as
a list: options["keys"] = [name].
Consumers of options["keys"] iterate it as a sequence of keys. For a bare
string that walks the key character by character -- for example client-side
cache key construction builds CacheKey(redis_keys=tuple(keys)), turning key
"myzset" into ('m', 'y', 'z', 's', 'e', 't') instead of ('myzset',).
Wrap the key in a list to match the other range commands, and add a
regression test asserting zrevrange passes keys=[name].
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @uttam12331, thank you for your contribution!
That was a nice catch - the issue is a real defect, and it's really nice that you have found and fixed it!
Two things before we can merge:
- Please check linters errors.
- Please add the regression test at the level where the defect is observable, rather than asserting the internal
keyskwarg.tests/test_cache.py::test_get_from_given_cacheis a good model: cache azrevrangeresult, mutate the sorted set from a second client, and assert both the CacheKey shape (redis_keys=("myzset",)) and that the entry is invalidated. That test fails on master and passes with your fix, and it protects the behavior rather than the plumbing.
Separately, and please do not expand this PR for it, in case you are interested in another contribution :) -->ZRANK and ZREVRANK are in the cache allow list but pass no keys at all, so they currently will raise ValueError("Cannot create cache key.") when client-side caching is enabled.
| # character instead of treating it as a single key. | ||
| with mock.patch.object(r, "execute_command", return_value=[]) as execute_command: | ||
| r.zrevrange("myzset", 0, -1) | ||
| assert execute_command.call_args.kwargs["keys"] == ["myzset"] |
There was a problem hiding this comment.
It will be great if you also add a test that goes through the client-side caching case.. - this one can go into tests/test_cache.py
Address review: replace the test that asserted the internal `keys` kwarg with
a behavior-level regression test in tests/test_cache.py. It caches a
`zrevrange` result, asserts the entry is stored under the whole-key cache key
(`redis_keys=("myzset",)`) rather than per-character, and that mutating the
sorted set from a second client invalidates it. This fails on master and
passes with the fix, and protects the behavior rather than the plumbing.
|
Thanks for the review @petyaslavova, and glad it was a useful catch! Addressed both points:
And thanks for the ZRANK/ZREVRANK pointer — I'd be happy to take that on in a separate PR so this one stays focused. |
Summary
SortedSetCommands.zrevrangestores its key inoptions["keys"]as a bare string, while every sibling range command passes it as a list:Impact
options["keys"]is consumed as a sequence of keys. When it is a bare string, consumers iterate it character by character. The clearest case is client-side caching inredis/connection.py:For
zrevrange("myzset", 0, -1)this buildsredis_keys=('m', 'y', 'z', 's', 'e', 't')instead of('myzset',), so the cache key / invalidation mapping forzrevrangeis wrong.Fix
Wrap the key in a list, matching the other range commands:
Tests
Added
test_zrevrange_passes_key_as_list, which mocksexecute_commandand assertszrevrangepasseskeys=["myzset"]. It fails on the previous behavior ("myzset" != ["myzset"]) and passes with the fix. Verified locally without a server.Note
Low Risk
One-line behavioral fix in command options plus cache regression tests; no auth, security, or broad API changes.
Overview
zrevrangenow setsoptions["keys"]to[name]instead of a bare string, aligning withzrange,zrangebyscore, and other range commands.That fixes client-side caching:
redis_keysis built fromtuple(kwargs["keys"]), so a string key was iterated per character (('m','y','z',...)) rather than as a single Redis key (('myzset',)), breaking cache lookup and invalidation forZREVRANGE.A regression test exercises cached
zrevrangeresults and invalidation when the sorted set is updated from another client (single-connection and pool modes).Reviewed by Cursor Bugbot for commit 9f0d891. Bugbot is set up for automated code reviews on this repo. Configure here.