Add maxCount validation to trips-for-route

[3rabiii]

Summary

Adds maxCount query parameter validation to the trips-for-route endpoint. The parameter is validated to strictly match the legacy Java OBA behavior: it silently accepts any integer (including 0, negative values, and large numbers) and only rejects non-numeric inputs. The parsed value is intentionally not applied to the result list, preserving the documented upstream defect.

Changes

• internal/restapi/trips_for_route_handler.go: Added a simple strconv.Atoi check to reject non-numeric inputs (which cause a 400 Bad Request upstream via Struts2 type conversion). Any valid integer is parsed but intentionally discarded via the blank identifier _ so it does not truncate the result set.
• internal/restapi/trips_for_route_handler_test.go:
  • Updated TestTripsForRouteHandler_MaxCountValidation to expect 200 OK for default/omitted, valid value, zero, negative, and out-of-bounds inputs. Only non-numeric inputs (abc) expect a 400 Bad Request.
  • Added a new test TestTripsForRouteHandler_MaxCountDoesNotTruncate using a fixture with multiple concurrently active trips to explicitly assert that passing maxCount=1 returns the exact same baseline count as a request without maxCount, proving the parameter is correctly ignored.

Closes: #1210

Summary by CodeRabbit

• Bug Fixes
  • Added validation for the maxCount query parameter when retrieving trips for a route.
  • Invalid values now return a clear validation error identifying the affected field.
  • Valid and omitted values continue to be accepted.

[coderabbitai]

Warning

Review limit reached

@burma-shave, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd724ab8-3b8f-41d6-accd-0d3db97c734b

📥 Commits

Reviewing files that changed from the base of the PR and between 8bed443 and 8a26bcc.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

📝 Walkthrough

Walkthrough

The trips-for-route handler now validates non-empty maxCount values as integers. Non-numeric values return validation errors. Valid values do not truncate results.

Changes

Trips-for-route maxCount validation

Layer / File(s)	Summary
Validate and test maxCount internal/restapi/trips_for_route_handler.go, internal/restapi/trips_for_route_handler_test.go	The handler rejects non-numeric maxCount values with a field error. Tests cover omitted, numeric, zero, negative, oversized, and boundary values. A regression test confirms that maxCount=1 does not truncate results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• OneBusAway/maglev#1183: Updates another REST API handler and tests for related maxCount pagination behavior.

Suggested reviewers: burma-shave

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation incorrectly rejects integer values 0, negative values, and values above 250, which issue #1210 requires accepting.	Reject only non-numeric maxCount values; accept all integer values without applying result limits.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The code and tests address maxCount handling for the trips-for-route endpoint and contain no unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding maxCount validation to the trips-for-route endpoint.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/trips_for_route_handler_test.go`:
- Around line 268-292: Add a table-driven test case alongside the existing
maxCount cases for the documented upper boundary, using maxCount "250" and
expecting http.StatusOK. Keep the existing "300" rejection case to verify values
above the limit remain invalid.

🪄 Autofix (Beta)

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: c3bb0135-e8ee-47c2-9491-ab54ab88b379

📥 Commits

Reviewing files that changed from the base of the PR and between 5d59661 and 7dce818.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

[aaronbrethorst]

Code review

Found 1 issue:

1. maxCount validation is stricter than the reference implementation and turns requests that upstream answers with 200 OK into 400 Bad Request. TripsForRouteAction builds its MaxCountSupport with the no-arg constructor (_maxCount = _absoluteMaxCount = Integer.MAX_VALUE), setMaxCount carries no @IntRangeFieldValidator, and there is no TripsForRouteAction-validation.xml — so maxCount=0, maxCount=-1 and maxCount=300 are all accepted and simply never applied. The only value that produces a 400 upstream is a non-integer, via Struts2 type conversion. The wiki spec agrees: trips-for-route.md lists no maxCount error path under Extensions, the request schema says only "Accepted but ignored — has no effect on result count.", and Minimal Guarantees says the response is always 200 OK. Where a ceiling does exist upstream (e.g. routes-for-location), the documented behaviour is "The server silently clamps maxCount ... No error is returned" — never a 400. Using utils.ParseMaxCount (reject-above-max) here therefore imports both a <= 0 rejection this action does not have and a 250 ceiling this action does not have. The corresponding assertions in TestTripsForRouteHandler_MaxCountValidation for 0, -1 and 300 encode the wrong expectation. (Note: the premise in issue maxCount parameter is not accepted or validated #1210 that "Java returns 400 for maxCount <= 0 or maxCount > 250" does not hold for this action.)

maglev/internal/restapi/trips_for_route_handler.go

Lines 32 to 39 in 48b14ef

	// NOTE: maxCount is parsed for validation but intentionally unimplemented
	// to match the upstream Java defect documented in the spec.
	if _, errs := utils.ParseMaxCount(r.URL.Query(), 0, nil); len(errs) > 0 {
	api.validationErrorResponse(w, r, errs)
	return
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[aaronbrethorst]

You got the hard part exactly right, and I want to call it out before the criticism: you discarded the parsed value with _ so maxCount never truncates the result list, and left limitExceeded hard-coded false. That's the central spec point for this endpoint and it's counterintuitive — the obvious "fix" would have been to apply the limit, which would have been wrong. The spec is explicit that TripStatusBeanServiceImpl.getTripsForRoute never reads query.getMaxCount(), so all matching trips are always returned. You matched that.

The problem is the validation half.

Returning 400 for maxCount <= 0 or maxCount > 250 isn't upstream behavior for this endpoint.

utils.ParseMaxCount produces a maxCount field error for values <= 0 and rejects values above the cap. But TripsForRouteAction has no range validation at all: _maxCount is a bare new MaxCountSupport(), whose no-arg constructor sets both the default and the absolute max to Integer.MAX_VALUE, and setMaxCount(int) carries no validator annotation. So upstream returns 200 OK for maxCount=0, -1, and 300 alike.

The wiki backs this up from the other direction. The Minimal Guarantees say the response is always 200 OK, and the Extensions section enumerates exactly one 400 case for this endpoint — a missing id path parameter. There is no maxCount error listed. The parameter's own schema entry reads simply "Accepted but ignored — has no effect on result count."

It's worth contrasting routes-for-location, where a ceiling does exist: even there the spec notes the server "silently clamps maxCount … No error is returned", and the <= 0 → 400 behavior comes from explicit code in RoutesForLocationAction.index that TripsForRouteAction simply doesn't have. Neither ParseMaxCount nor ParseMaxCountClamped matches an action with no ceiling whatsoever.

So as written, this converts requests that currently succeed into 400s. What I'd like instead: keep rejecting non-numeric input (maxCount=abc genuinely is a 400 upstream, via Struts2 type conversion) and accept any integer value silently.

I should flag the root cause so it doesn't bite again: linked issue #1210 states "Java returns 400 for maxCount <= 0 or maxCount > 250". That's accurate for some endpoints but not for trips-for-route, and it's what sent this PR in the wrong direction. Worth correcting the issue too — that's not on you to have caught.

One more thing while you're in here: since the whole point is that maxCount must not truncate, the test should assert that. Right now the 200 cases only check the status code, so if someone later wires the limit in, these tests still pass. Asserting that the result count is identical with maxCount absent and with maxCount=1 would lock in the behavior you're deliberately preserving.

This also has merge conflicts — its base predates the includeReferences := ShouldIncludeReferences(r) addition on main. The inserted block itself stays correct after a rebase since it sits before the GetAgency call, so that part is mechanical.

Drop the range validation, keep the non-numeric check, add the count-unchanged assertion, and rebase — then I'll merge it.

[coderabbitai]

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/trips_for_route_handler_test.go`:
- Around line 534-553: Add a second concurrently active trip in a different
block to the fixture setup used by
TestTripsForRouteHandler_MaxCountDoesNotTruncate, ensuring the baseline response
contains more than one trip. Keep the existing requests and assert that the
maxCount=1 response count equals the full baseline count, making parameter
truncation observable.

🪄 Autofix (Beta)

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: 15fcd50f-6174-40ce-a056-f9a225cb21af

📥 Commits

Reviewing files that changed from the base of the PR and between 7dce818 and c18027b.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[burma-shave]

Closing this one out without merging — the validation logic and tests are good, this is about whether maxCount should apply to this endpoint at all. I don't think it should.

• The result set isn't open-ended. maxCount matters on endpoints like stops-for-location or route-search, where the result set can genuinely be large. trips-for-route's list is one entry per currently-active vehicle on the route in a ±30/+10 min window — bounded by how many vehicles a route can physically have in service, not something that needs capping.
• There's nothing meaningful to keep when truncating. Per the wiki's "Ordering note," results come out of a hash-set collection in non-deterministic order. There's no "closest" or "soonest" to sort by, so honoring maxCount just returns an arbitrary N of whatever's active — not a usable "top N."
• Truncating actively misleads the clients that consume this. Checked how Wayfinder and Android use this response: Wayfinder's marker sweep (vehicleUtils.js) treats "not in this response" as "vehicle no longer active" and deletes its map marker; Android's extrapolation layer silently drops observations for anything missing. This isn't pagination where "there's more, fetch page 2" is fine — it reads as vehicles going offline that are still running.

So the "Suspected Defect" framing in the spec is probably wrong for this endpoint — Java isn't failing to apply maxCount, the parameter likely doesn't belong here at all, inherited from a shared request path rather than deliberately supported. Accept-and-validate-but-never-apply (your original approach, before I pushed toward enforcing it) may be the correct behavior, not a bug. Let me know if you see it differently.

I'll update spec accordingly.