Skip to content

Add validation for maxCount query parameter (return 400 on invalid input)#772

Open
jeyamoorthi wants to merge 3 commits intoOneBusAway:mainfrom
jeyamoorthi:fix/maxcount-validation
Open

Add validation for maxCount query parameter (return 400 on invalid input)#772
jeyamoorthi wants to merge 3 commits intoOneBusAway:mainfrom
jeyamoorthi:fix/maxcount-validation

Conversation

@jeyamoorthi
Copy link
Copy Markdown

Problem

The maxCount query 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.

@Adityatorgal17
Copy link
Copy Markdown
Contributor

Hey! @jeyamoorthi
Just a small suggestion — it would be great if we include issue references in the PR description using keywords like:

  • Fixes #<issue_number>
  • Closes #<issue_number>

This way, the issue gets automatically closed when the PR is merged & keeps issue tracking simpler.

@jeyamoorthi
Copy link
Copy Markdown
Author

Got it, thanks for pointing that out! I’ll update the PR description with the issue reference.

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go entirely 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/healthz

This 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 maxCount validation in search_stops_handler.go is clean and correct
  • Good test coverage: invalid strings, floats, zero, negative values, and large valid values
  • The TestArrivalAndDepartureForStopHandler_AgencyNotFound test is well-written (just needs to be in its own PR)

Recommended Action

  1. Remove validation_middleware.go and validation_middleware_test.go (dead code)
  2. Remove the README changes (broken markdown, unrelated)
  3. Move the arrival handler fix to a separate PR
  4. Keep only the search_stops_handler.go maxCount validation and its tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants