Pass zrank/zrevrank keys so they are cacheable (fixes ValueError under CSC) - #4249
uttam12331 wants to merge 2 commits into
Conversation
eeshsaxena
left a comment
eeshsaxena
left a comment
There was a problem hiding this comment.
The fix is right. Every other command in DefaultCache.DEFAULT_ALLOW_LIST reaches execute_command with keys=[...], either directly or through a helper (_zrange, _zaggregate, _geosearchgeneric, the JSON and TS command builders), and Connection.send_command raises ValueError("Cannot create cache key.") for any allow-listed command that arrives without keys. ZRANK and ZREVRANK were the two that slipped through, so this closes the gap.
I went through the whole allow-list looking for siblings with the same problem. Everything else is covered, so the crash side of this is complete.
One related thing turned up that is pre-existing and out of scope here, but may be worth its own issue: SORT_RO, GEORADIUS_RO, and GEORADIUSBYMEMBER_RO are in the allow-list, but nothing in the client ever sends those command names. sort_ro() delegates to sort() which sends SORT, and georadius() / georadiusbymember() send GEORADIUS / GEORADIUSBYMEMBER. Those base names are correctly kept out of the allow-list because they can write with STORE. The net effect is that read-only geo and sort queries are never actually cached, even though the _RO entries look like they were added to cache them. That is a silent functional gap rather than a crash, so it does not block this change.
The added test covering both commands under single-connection and pooled clients looks good.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @uttam12331, thank you for your contribution! Overall, it looks good!
Reviewing your change, I noticed that there are a few other gaps like these ones. Would you please add similar handling for the following:
xpending_range()sendsXPENDINGwithparse_detail=Trueand nokeys, so it still raisesValueError("Cannot create cache key.")under CSC. It needskeys=[name].SORT_ROis on the allow list, butsort_ro()delegates tosort(), which sendsSORT. Please make it sendSORT_ROwithkeys=[key].GEORADIUS_ROandGEORADIUSBYMEMBER_ROare on the allow list, but_georadiusgeneric()always sends the baseGEORADIUS/GEORADIUSBYMEMBERnames. Please send the_ROvariants withkeys=[name]when neitherstorenorstore_distis given.
The last two are silent gaps rather than crashes — read-only sort and geo queries are simply never cached today, even though the allow-list entries suggest they are.
Could you also rebase onto master? #4244 landed a sibling test in tests/test_cache.py at the same insertion point, so the branch conflicts there. A regression test per item, in the style of the one you already added, would round this out.
ZRANK and ZREVRANK are in the client-side cache allow list, but neither set
options["keys"], so building the cache key raised
ValueError("Cannot create cache key.") whenever caching was enabled.
Set options["keys"] = [name] for both, matching the range commands, and add
a regression test that caches zrank/zrevrank and checks invalidation.
4d16982 to
8551ac9
Compare
|
Thanks @petyaslavova! Addressed all of it:
I verified the command construction directly (each now reaches |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # because they can write). | ||
| if not kwargs["store"] and not kwargs["store_dist"]: | ||
| command += "_RO" | ||
| kwargs["keys"] = [args[0]] |
There was a problem hiding this comment.
Geo _RO responses left unparsed
High Severity
Read-only georadius / georadiusbymember now send GEORADIUS_RO / GEORADIUSBYMEMBER_RO, but response_callbacks still only maps the non-_RO names to parse_geosearch_generic. With withdist, withcoord, or withhash, distances, hashes, and coordinates stay as raw server values instead of the typed structures callers expect. This affects all clients, including those not using caching.
Reviewed by Cursor Bugbot for commit 8551ac9. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8551ac9026
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # works (the base GEORADIUS names are kept off the cache allow list | ||
| # because they can write). | ||
| if not kwargs["store"] and not kwargs["store_dist"]: | ||
| command += "_RO" |
There was a problem hiding this comment.
When a read-only georadius() or georadiusbymember() call includes withdist, withcoord, or withhash, this rewrite changes the parser lookup key to GEORADIUS_RO/GEORADIUSBYMEMBER_RO, but the GEO response callbacks are registered only for the base command names. Those option-bearing calls now return the raw nested Redis reply instead of the established floats, ints, and coordinate tuples, so existing non-STORE callers see different response shapes even without client-side caching. Register the _RO aliases with the same parser or parse under the base command name.
AGENTS.md reference: AGENTS.md:L158-L162
Useful? React with 👍 / 👎.
| get=get, | ||
| desc=desc, | ||
| alpha=alpha, | ||
| _command="SORT_RO", |
There was a problem hiding this comment.
When client-side caching is enabled and sort_ro() is called with by or get patterns, this now makes the command cacheable while sort() still records only keys=[name]. The sorted reply can depend on external keys such as score:* or object keys; invalidations for those keys will not match this cache entry, so repeated sort_ro() calls can return stale order or values until the source collection itself changes. Either include all dependencies or bypass caching for BY/GET variants.
AGENTS.md reference: AGENTS.md:L158-L162
Useful? React with 👍 / 👎.
| alpha: bool = False, | ||
| store: str | None = None, | ||
| groups: bool | None = False, | ||
| _command: str = "SORT", |
There was a problem hiding this comment.
sort() is a public command method, so adding the _command keyword changes its runtime signature and sort_ro() now depends on that hidden keyword when it calls self.sort(...). Any subclass or wrapper that overrides sort() with the documented signature will now raise TypeError: ... unexpected keyword argument '_command' when sort_ro() is used; route both commands through a private helper instead of extending the public method signature.
AGENTS.md reference: AGENTS.md:L111-L113
Useful? React with 👍 / 👎.
| # works (the base GEORADIUS names are kept off the cache allow list | ||
| # because they can write). | ||
| if not kwargs["store"] and not kwargs["store_dist"]: | ||
| command += "_RO" |
There was a problem hiding this comment.
When a read-only georadius() or georadiusbymember() call includes withdist, withcoord, or withhash, this rewrite changes the parser lookup key to GEORADIUS_RO/GEORADIUSBYMEMBER_RO, but the GEO response callbacks are registered only for the base command names. Those option-bearing calls now return the raw nested Redis reply instead of the established floats, ints, and coordinate tuples, so existing non-STORE callers see different response shapes even without client-side caching. Register the _RO aliases with the same parser or parse under the base command name.
Useful? React with 👍 / 👎.
| # works (the base GEORADIUS names are kept off the cache allow list | ||
| # because they can write). | ||
| if not kwargs["store"] and not kwargs["store_dist"]: | ||
| command += "_RO" |
There was a problem hiding this comment.
On Redis 3.2.0 through 3.2.9, GEORADIUS and GEORADIUSBYMEMBER exist but the _RO variants do not; Redis documents GEORADIUS_RO and GEORADIUSBYMEMBER_RO as available since 3.2.10. Because this branch rewrites every non-STORE GEO radius call, those supported servers now get unknown command for the common read path; only switch to _RO when the server supports it or limit this to CSC-capable connections.
Useful? React with 👍 / 👎.
Following review, cover the other allow-listed commands that reached
execute_command without `keys`:
- xpending_range() sent XPENDING with no keys, so it raised
ValueError("Cannot create cache key.") under CSC. Pass keys=[name].
- sort_ro() delegated to sort(), which sends SORT, so SORT_RO (on the allow
list) was never cached. Send SORT_RO with keys=[key] via a private
_command argument to sort(); the write path (sort(store=...)) still sends
SORT. The SORT response callback is a no-op without `groups` (which sort_ro
never uses), so response parsing is unaffected.
Add a regression test per command in tests/test_cache.py.
(Deferred: georadius/georadiusbymember _RO caching. The GEO response
callbacks are keyed on the base command names, so sending the _RO variants
would change response parsing for withdist/withcoord/withhash calls, and the
_RO commands only exist since Redis 3.2.10. That needs _RO parser aliases and
version gating and is better handled separately.)
8551ac9 to
f84b16f
Compare
|
Good catch from Bugbot/Codex on the GEO change — I've addressed it. The GEO response callbacks are registered on the base command names ( Kept the safe fixes:
Rebased onto |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit f84b16f. Configure here.
| options = {"groups": len(get) if groups else None} | ||
| options["keys"] = [name] | ||
| return self.execute_command("SORT", *pieces, **options) | ||
| return self.execute_command(_command, *pieces, **options) |
There was a problem hiding this comment.
Stale sort_ro cache with BY/GET
Medium Severity
sort_ro now issues allow-listed SORT_RO and caches under keys=[name] only. With by or get, Redis tracking invalidates the external keys those options touch, but those keys are not in redis_keys, so delete_by_redis_keys leaves the entry in place and later hits can return stale results. Previously sort_ro sent non-allow-listed SORT, so this path was not cached.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f84b16f. Configure here.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @uttam12331, thanks for the updates you have added!
The zrank / zrevrank fix is good, and the sort_ro direction is right — sending SORT_RO is what the method documents. Two more changes before we can merge:
-
Keep
_commandoff the publicsort()signature (Codex flagged this too) and route both commands through a private helper:_sort(self, command, name, ...)holding the current body, withsort()callingself._sort("SORT", ...)andsort_ro()callingself._sort("SORT_RO", ...)._zrangeis the existing precedent for that shape, and the@overloaddeclarations forsort()should stay as they are. -
Set
options["keys"] = [name]only when bothbyandgetareNone. WithBY score:*orGET pattern:*the reply also depends on keys the command never declares, and Redis tracking remembers only declared keys — a change toscore:1invalidatesscore:1, never the sorted key, so an entry stored underkeys=[name]can never be evicted and later calls would return a stale ordering.
| alpha: bool = False, | ||
| store: str | None = None, | ||
| groups: bool | None = False, | ||
| _command: str = "SORT", |
Mukller
left a comment
Mukller
left a comment
There was a problem hiding this comment.
Verified locally by capturing execute_command payloads on this branch vs installed 8.1.0 (no server needed):
| command | 8.1.0 (before) | this branch |
|---|---|---|
zrank(k, m) |
('ZRANK', k, m) keys=None |
keys=[k] ✓ |
zrank(k, m, withscore=True) |
keys=None | keys=[k] ✓ |
zrevrank(k, m) |
keys=None | keys=[k] ✓ |
sort_ro("mylist") |
sends SORT (!), keys set |
sends SORT_RO, keys=[mylist] ✓ |
xpending_range(s, g, ...) |
keys=None | keys=[s] ✓ |
Two things worth calling out:
- The
SORT_ROfix is a bigger deal than the title suggests: before this PRsort_ro()literally dispatchedSORT, i.e. the read-only variant was never used — a user with read-only ACL (-SORT +SORT_RO) would get a permission error from a method documented as SORT_RO. Now correct. - Declaring
keys=on ZRANK/ZREVRANK/XPENDING is what makes these commands usable under client-side caching: without key declaration the invalidation tracking can't bind to the key (and in CSC setups this surfaces as the ValueError mentioned in the PR description).
Minor nit, non-blocking: _command is a private-ish kwarg on public sort(). It's prefixed correctly and pragmatic (avoids duplicating ~40 lines of option building); just noting it in case maintainers prefer an underscore-excluded docstring line so IDEs don't suggest it.
CI: 363 green / 3 failures scattered across unrelated matrix cells (8.0.6+py3.14 unified, 8.8 cluster, PyPy standalone) — that scatter pattern reads as infra flakiness rather than something caused by adding keys= options; worth a re-run if maintainers agree.
4 participants
Summary
ZRANKandZREVRANKare in the client-side cache allow list (redis/cache.py), but neither setsoptions["keys"]. When client-side caching is enabled, building the cache key raises:(from
redis/connection.py,if kwargs.get("keys") is None: raise ValueError("Cannot create cache key.")), so these two commands can't be used with caching at all.This was pointed out by @petyaslavova in #4244:
Fix
Set
options["keys"] = [name]in bothzrankandzrevrank, matching the range commands:options = {"withscore": withscore, "score_cast_func": score_cast_func} + options["keys"] = [name] return self.execute_command(*pieces, **options)Tests
Added
test_zrank_zrevrank_are_cacheable(intests/test_cache.py), which cacheszrank/zrevrank(noValueError), asserts the entries are stored underredis_keys=("myzset",), and that mutating the sorted set from a second client invalidates them.Note
Low Risk
Read-path caching fixes with targeted key metadata; behavior unchanged when CSC is off, covered by new integration tests.
Overview
Fixes client-side caching for several allow-listed read commands that previously omitted
options["keys"], which triggeredValueError: Cannot create cache key.when CSC was enabled.zrank/zrevrankandxpending_rangenow setoptions["keys"] = [name](stream/sorted-set key) so cache keys can be built and invalidations track the right Redis key.sort_ronow forwards_command="SORT_RO"intosort()so the wire command matches the CSC allow list instead of reusingSORT.Adds cache tests for all three areas: rank commands with invalidation on
zadd,xpending_rangepopulation, andsort_rowith invalidation on list mutation.Reviewed by Cursor Bugbot for commit f84b16f. Bugbot is set up for automated code reviews on this repo. Configure here.