Add validation for maxCount query parameter (return 400 on invalid input)#772
Add validation for maxCount query parameter (return 400 on invalid input)#772jeyamoorthi wants to merge 3 commits intoOneBusAway:mainfrom
Conversation
…ncorrect 500 response for missing agency
|
Hey! @jeyamoorthi
This way, the issue gets automatically closed when the PR is merged & keeps issue tracking simpler. |
|
Got it, thanks for pointing that out! I’ll update the PR description with the issue reference. |
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Jeyamoorthi, the core idea is right — returning 400 for invalid maxCount values instead of silently falling back is better API behavior. However, this PR bundles several unrelated changes and includes dead code that needs to be addressed.
Critical Issues (2 found)
1. validation_middleware.go is dead code — never wired into any route
internal/restapi/validation_middleware.go (94 lines)
The ValidateQueryParams middleware, IntRangeRule, and NonNegativeIntRule are defined but never used anywhere in the codebase. They're not referenced in routes.go or any handler. The actual maxCount validation in search_stops_handler.go is done inline — it doesn't use this middleware at all.
Either:
- Wire the middleware into the appropriate routes and remove the inline validation, or
- Remove
validation_middleware.goentirely and keep the inline approach (which is what's actually working)
Don't ship 94 lines of unused framework code plus 220 lines of tests for code that nothing calls.
2. README change breaks markdown formatting
README.markdown:40-50
The PowerShell section is inserted inside an existing bash code block, creating broken markdown:
```bash
curl http://localhost:4000/healthz
### Windows (PowerShell) Note ← this is inside the code block
When using PowerShell, you may see...
```powershell ← nested code block
curl http://localhost:4000/healthzThis corrupts the README rendering. Please remove this change — it's unrelated to maxCount validation and should be in its own PR if needed (with correct markdown).
Important Issues (1 found)
1. Unrelated sql.ErrNoRows fix in arrival handler should be a separate PR
internal/restapi/arrival_and_departure_for_stop_handler.go:175-180
The GetAgency error handling change (returning 404 instead of 500 for missing agencies) is a valid fix, but it has nothing to do with maxCount validation. Please split this into its own PR so it can be reviewed and tracked independently.
Strengths
- The inline
maxCountvalidation insearch_stops_handler.gois clean and correct - Good test coverage: invalid strings, floats, zero, negative values, and large valid values
- The
TestArrivalAndDepartureForStopHandler_AgencyNotFoundtest is well-written (just needs to be in its own PR)
Recommended Action
- Remove
validation_middleware.goandvalidation_middleware_test.go(dead code) - Remove the README changes (broken markdown, unrelated)
- Move the arrival handler fix to a separate PR
- Keep only the
search_stops_handler.gomaxCount validation and its tests
Problem
The
maxCountquery parameter is parsed using strconv.Atoi, but errors are ignored.Current Behavior
Invalid values (e.g., maxCount=abc) silently fall back to default (50).
Expected Behavior
The API should return a 400 Bad Request when invalid input is provided.
Proposed Fix
Add validation to ensure maxCount is a positive integer and return an error response otherwise.
Impact
Improves API correctness and prevents silent failures.