Repository navigation
fix(geo): encode GEOSEARCH zeros instead of dropping them - #4356
Conversation
Truthiness checks in _geosearchgeneric omitted FROMMEMBER/BYRADIUS/BYBOX/COUNT when the value was 0, and allowed FROMMEMBER plus FROMLONLAT at the origin.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @00200200, thank you for your contribution!
The change looks good - the truthiness check really drops zeroes and need to be changed.
One thing to fix before merge: assert "COUNT" not in wire_args(...) in both new tests can never fail, because the command pieces contain b"COUNT" (bytes). Please compare against b"COUNT" so the count=None case is actually guarded.
Also, please mention in the PR description that count=0 is now forwarded and the server rejects it with COUNT must be > 0, where before it was silently ignored. That is the correct behavior, but users will notice it.
petyaslavova
added
maintenance
Guard count=None in both sync and async tests, and clarify that zero arguments are forwarded for server validation.
|
Addressed both requested changes in 7badeda: the sync and async omission assertions now compare against b"COUNT", and the PR description explicitly explains that count=0 is forwarded for Redis to reject with COUNT must be > 0. Both targeted tests pass; Ruff lint and formatting checks pass. |
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the quick update - both points are addressed. The b"COUNT" assertions now actually guard the count=None case, and the description makes the count=0 behavior change clear.
LGTM, I will approve once CI passes.
Description of change
_geosearchgenericguardedmember,radius,width/height, andcountwith truthiness tests. Zero arguments must be forwarded rather than silently treated as "argument omitted", so Redis can apply its own validation:geosearch(..., longitude=0, latitude=0, radius=0)sentFROMLONLAT 0 0with noBYRADIUSgeosearch(..., member=0, radius=10)omittedFROMMEMBERentirelygeosearch(..., member="x", longitude=0, latitude=0, radius=10)sent bothFROMMEMBERandFROMLONLATinstead of raisingDataErrorwidth=0droppedBYBOX;count=0droppedCOUNTGEORADIUSalready usescount is not None. This uses the sameis not Nonechecks for GEOSEARCH/GEOSEARCHSTORE.count=0is now forwarded to Redis, which rejects it withCOUNT must be > 0; previously it was silently ignored.Pull Request check-list
Tests:
tests/test_commands.py::TestRedisCommands::test_geosearch_forwards_zero_numeric_arguments(mock, no server)tests/test_asyncio/test_commands.py::TestRedisCommands::test_geosearch_forwards_zero_numeric_arguments(async mirror)test_geosearch_negativeGeoCommandsis shared withAsyncGeoCommands, so the encoding change covers both stacks.Note
Medium Risk
Changes command-building for GEOSEARCH/GEOSEARCHSTORE (including
count=0now reaching the server); behavior shifts for callers that relied on zeros being silently dropped.Overview
Fixes GEOSEARCH/GEOSEARCHSTORE argument encoding so numeric
0values are sent to Redis instead of being treated as omitted._geosearchgenericnow uses explicitis not Nonechecks (aligned with GEORADIUS-style handling) formember,longitude/latitude,radius,width/height, andcount. That restores correct wire commands for cases likeradius=0,member=0,width=0, andcount=0, and fixes mutual-exclusion validation somemberpluslongitude=0/latitude=0raisesDataErrorinstead of emitting bothFROMMEMBERandFROMLONLAT.Tests add mock-based coverage that asserted command args on the wire (sync, async, and
geosearchstore), plus a negative case for the origin conflict at(0, 0).Reviewed by Cursor Bugbot for commit 8be4856. Bugbot is set up for automated code reviews on this repo. Configure here.