Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
Daniel — the direction is right: invalid maxCount values like "abc" should return 400 instead of silently falling back to the default. Two things need to be fixed before this can merge.
Critical Issues (1 found)
-
Hard-capping
maxCountat 50 is not in the OpenAPI spec and breaks valid requests. The spec (testdata/openapi.yml:1029) definesmaxCountas a plain integer with no maximum — "The max number of results to return. Defaults to 20." A client requestingmaxCount=100should get 100 results, not a 400 error. The validation should reject non-positive values and non-integers, but should not impose an upper bound that the spec doesn't define. [search_stops_handler.go:55]Change:
// Before (too restrictive) if err != nil || parsed <= 0 || parsed > 50 { // After (matches spec) if err != nil || parsed <= 0 {
Important Issues (1 found)
- Default value should be 20, not 50. The OpenAPI spec says "Defaults to 20" but the code defaults to 50 (
limit := 50on line 52). This is a pre-existing issue, but since you're already touching this code, please fix it to match the spec. [search_stops_handler.go:52]
Suggestions (2 found)
-
The error message
"need to be a positive integer"is grammatically incorrect — consider"must be a positive integer". [search_stops_handler.go:56] -
The
"tooLarge"test case (maxCount=101) should be updated to expecthttp.StatusOKonce the upper bound cap is removed, since 101 would become a valid value.
Strengths
- Correctly identifies the silent fallback behavior as a problem
- Test structure is good — table-driven with expected status codes
- Adding the
"abc"invalid string test case is a nice touch
Recommended Action
- Remove the
parsed > 50upper bound check - Change default from 50 to 20
- Fix the error message grammar
- Update the
"tooLarge"test case accordingly
|
@aaronbrethorst Thank you for the feedback! I really appreciate it, I have committed the changes. |
Fixes #771
Problem
The maxCount query parameter is parsed, but errors are ignored.
Fix
Added validation to the parameter checking for a positive integer sending a error for negative and values greater then the limit. Also fixed test file to now accept the new requirements.