Refactor boolean parsing in trips-for-route - #1233
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 trips-for-route handler now parses ChangesTrips-for-route boolean parsing
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 259-270: Extend the table-driven cases in the trips-for-route
handler tests to cover empty includeSchedule and includeStatus query values,
expecting both flags to be false, and add an independent includeStatus=true case
expecting status true. Keep the existing defaults and invalid-value cases
unchanged.
In `@internal/restapi/trips_for_route_handler.go`:
- Around line 30-37: Update the query-parameter parsing in the trip route
handler to independently read includeTrip and includeReferences, alongside
includeSchedule and includeStatus, with each defaulting to true and parsing
provided boolean values. Propagate the corresponding flags through
buildTripReferences so each parameter controls its own output behavior; do not
reuse includeSchedule for includeTrip.
- Around line 30-37: Update the includeSchedule and includeStatus
query-parameter handling in the trips-for-route handler to distinguish omitted
parameters from present-but-empty values. Apply the same
presence/check-before-parse behavior used by the trip-details implementation,
rejecting empty supplied values as invalid while preserving the default true
values when parameters are omitted.
🪄 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: 08fe1063-8ed6-493e-a2d9-9ebaf5c0985f
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Update includeSchedule and includeStatus to use strconv.ParseBool instead of basic string comparison. This ensures invalid values default to false while omitted values default to true, aligning with the API spec and the behavior of peer handlers like trips-for-location. Comprehensive test cases are added to strictly verify the parsing of omitted, valid, and invalid boolean query parameters.
Update boolean parsing in trips-for-route to use r.URL.Query().Has() instead of checking for empty strings via Get(). This ensures that explicitly provided but empty parameters (e.g., ?includeSchedule=) are correctly parsed as invalid and evaluate to false, rather than incorrectly falling back to the true default. Additionally, add missing test cases highlighted in code review: - Present-but-empty parameters expecting false - Explicit success case for includeStatus=true
Extract boolean parsing logic into a shared parseBoolQueryParam helper and apply it to includeSchedule, includeStatus, includeTrip, and includeReferences. This ensures omitted parameters correctly default to true, while present but empty or invalid values safely evaluate to false, resolving inconsistencies flagged in code review. Adds comprehensive matrix tests for all scenarios.
76d6e83 to
9fdf56b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/trips_for_route_handler_test.go (1)
422-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
wantRefsPopulatedfield.
wantRefsPopulatedis declared per test case but never read in the test body (Lines 448-461). Both subtests run identical assertions that expect empty references, regardless of the field's value. Since the handler builds references from an empty trip/stop set on this code path, references stay empty even whenincludeReferences=true, so the field cannot drive different expectations here. Remove the field, or add a one-line comment stating why it's always empty, so a future reader does not assume the test differentiates the two cases.♻️ Proposed cleanup
tests := []struct { name string includeReferences string - wantRefsPopulated bool }{ { - name: "Empty List - Include References Explicit", - includeReferences: "true", - wantRefsPopulated: true, + name: "Empty List - Include References Explicit", + includeReferences: "true", }, { - name: "Empty List - Exclude References", - includeReferences: "false", - wantRefsPopulated: false, + name: "Empty List - Exclude References", + includeReferences: "false", }, }🤖 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/trips_for_route_handler_test.go` around lines 422 - 437, Remove the unused wantRefsPopulated field from the test case struct and both test entries in the table-driven test. Keep the existing subtest inputs and identical empty-reference assertions unchanged.
🤖 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.
Outside diff comments:
In `@internal/restapi/trips_for_route_handler_test.go`:
- Around line 422-437: Remove the unused wantRefsPopulated field from the test
case struct and both test entries in the table-driven test. Keep the existing
subtest inputs and identical empty-reference assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73c0b700-9b66-43f9-92b1-32554883c1bc
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Clean up TestTripsForRouteHandler_ReferencesInclusion_EmptyList by removing the wantRefsPopulated field. It was declared but never evaluated since references are always empty on this path.
|
1 participant
Summary
Fixes inconsistent boolean parameter parsing in the
trips-for-routeendpoint. Updates the handler to usestrconv.ParseBoolfor all boolean query parameters (includeSchedule,includeStatus,includeTrip, andincludeReferences). This ensures that invalid values correctly evaluate tofalse, while preserving the spec-required default oftruewhen the parameters are completely omitted. This aligns the endpoint with the established pattern used in peer handlers (liketrips-for-location).Changes
internal/restapi/trips_for_route_handler.go: Replaced the legacy!= "false"string comparison checks with a standardizedstrconv.ParseBoolapproach for all boolean flags.internal/restapi/trips_for_route_handler_test.go: Added comprehensive table-driven tests to rigorously verify the parsing behavior across all scenarios (omitted/default, explicittrue, explicitfalse, and invalid/junk values).Closes: #1232
Summary by CodeRabbit
Bug Fixes
Tests