fix(geo): validate positive COUNT arguments#3459
fix(geo): validate positive COUNT arguments#3459jihuayu wants to merge 5 commits intoapache:unstablefrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens Geo command argument parsing by rejecting non-positive COUNT values and refactors the internal “count specified” logic to no longer rely on count == 0.
Changes:
- Added
ParseCountandhas_countplumbing forGEORADIUS*andGEOSEARCH*command parsing. - Updated
redis::GeoAPIs (Radius*,Search*) to accepthas_countseparately fromcount, and adjusted store logic accordingly. - Added/updated unit tests (Go and C++) to cover
COUNT <= 0parse errors and updated method signatures.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gocase/unit/geo/geo_test.go | Adds Go-level coverage asserting COUNT of 0 / -1 returns a parse error across geo commands. |
| tests/cppunit/types/geo_test.cc | Updates C++ unit tests to match the new Geo API signatures. |
| src/types/redis_geo.h | Updates Geo public method signatures to include has_count and use size_t for count. |
| src/types/redis_geo.cc | Threads has_count through to store logic; refactors stored-result limiting loop. |
| src/commands/cmd_geo.cc | Implements ParseCount, switches geo commands to explicit has_count tracking, and enforces COUNT > 0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Status::OK(); | ||
| } | ||
|
|
||
| auto unsigned_count = ParseInt<size_t>(raw_count, 10); |
There was a problem hiding this comment.
didn't get why you parse it twice.
There was a problem hiding this comment.
Ohhh. I got what your means. I will change it latter.
Since we need to check if it's a positive number, size_t is always non-negative.
Perhaps we could use a helper function to determine if it's a positive number? It might make the logic clearer.
PragmaTwice
left a comment
PragmaTwice
left a comment
There was a problem hiding this comment.
could you put why you make this change instead of what you change in PR description?
Done. Thanks! |
|
The original code overlooked the requirement for COUNT to be a positive integer, resulting in an inconsistency with the Redis protocol.
I have modified the implementation to refactor the special case where count == 0 is treated as if no count were provided, ensuring code cleanliness and logical transparency.
Assisted-by: Codex