routes-for-location: silently clamp maxCount to 50 - #1291
Conversation
The wiki spec caps maxCount at 50 for this endpoint and requires silent clamping rather than a validation error. The handler used the rejecting ParseMaxCount variant, so requests above the global 250 ceiling returned HTTP 400, and values between 51-250 were passed through uncapped. Switch to ParseMaxCountClamped and apply an endpoint-specific 50 cap on top of the global ceiling.
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe routes-for-location endpoint now silently clamps ChangesRoutes-for-location maxCount clamping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/routes_for_location_handler_test.go`:
- Around line 166-177: The test
TestRoutesForLocationHandlerClampsMaxCountAboveCap only covers the global clamp
at maxCount=300 and misses endpoint-specific clamping for values from 51 through
250. Extend it with table-driven cases such as maxCount=51 and maxCount=250,
comparing each response against maxCount=50 or using data with over 50 matching
routes, and assert the returned list length demonstrates the endpoint cap while
preserving successful response checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f7ef526-1ef8-43e3-b409-81e739077424
📒 Files selected for processing (3)
internal/models/constants.gointernal/restapi/routes_for_location_handler.gointernal/restapi/routes_for_location_handler_test.go
CodeRabbit review flagged that the clamp test only checked maxCount=300, missing the 51-250 range between the endpoint's 50 cap and the global 250 ceiling. Made the test table-driven over 51, 250, and 300, all asserting HTTP 200 with no fieldErrors. The RABA fixture has only 13 routes total, so no maxCount value can push the response list length past 50 - list length alone can't distinguish a 50 cap from a 250 cap here. The added cases verify the narrower but still real regression: values above 50 must not be rejected the way the pre-fix rejecting parser rejected values above 250.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/restapi/routes_for_location_handler_test.go (1)
191-192: 🎯 Functional Correctness | 🟡 MinorMake the test prove the endpoint-specific clamp.
The fixture has only 13 matching routes. Therefore, the length assertion passes even if
51or250is sent toGetRoutesForLocation. The test verifies acceptance of oversized values, but not the 50-route clamp.Use a test double that records the manager argument, or a fixture with more than 50 matching routes and compare
maxCount=51andmaxCount=250withmaxCount=50.As per coding guidelines,
**/*_test.gomust cover every new branch or condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/routes_for_location_handler_test.go` around lines 191 - 192, Strengthen the oversized max-count test around GetRoutesForLocation so it proves the endpoint-specific clamp to 50 rather than relying on the 13-route fixture. Use a recording manager argument or a fixture with more than 50 matching routes, and verify that requests with maxCount=51 and maxCount=250 produce the same result or manager value as maxCount=50 while retaining the existing non-empty assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/restapi/routes_for_location_handler_test.go`:
- Around line 191-192: Strengthen the oversized max-count test around
GetRoutesForLocation so it proves the endpoint-specific clamp to 50 rather than
relying on the 13-route fixture. Use a recording manager argument or a fixture
with more than 50 matching routes, and verify that requests with maxCount=51 and
maxCount=250 produce the same result or manager value as maxCount=50 while
retaining the existing non-empty assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0143f6c3-56c8-41d6-816d-541bae79818c
📒 Files selected for processing (1)
internal/restapi/routes_for_location_handler_test.go
The RABA fixture has only 13 routes total, so the previous test's "len(list) <= 50" assertion held regardless of whether the handler clamped maxCount to 50, to the global 250 ceiling, or not at all - it proved requests were no longer rejected but not that the cap is actually 50. Seed synthetic routes on the nearest fixture stop so the response list can exceed 50, then assert exact lengths across the 10/50/51/ 250/300 boundary. This fails without the endpoint's min(maxCount, 50) clamp, confirming the test exercises the fix.
|
2 participants
Summary
maxCountvalues above the endpoint's cap must be silently clamped to 50, not rejected.utils.ParseMaxCount(the rejecting variant), so requests withmaxCountbetween 51-250 were passed through uncapped, and requests above 250 returned HTTP 400 instead of being clamped.utils.ParseMaxCountClampedand applied a newmodels.MaxCountForRoutesForLocation = 50cap on top of the global 250 ceiling.Closes #1231
Test plan
TestRoutesForLocationHandlerClampsMaxCountAboveCapgo test ./internal/restapi -run TestRoutesForLocation -tags "sqlite_fts5 sqlite_math_functions" -vgo vet -tags "sqlite_fts5 sqlite_math_functions" ./...andgo vet -tags "purego" ./...make testNote for reviewer
Spec says
maxCountdefault is 10; code defaults tomodels.DefaultMaxCountForRoutes = 50. Separate, unfiled gap under #1225 — left out of scope here.Summary by CodeRabbit