remove non-spec outOfRange field from trips-for-route

[3rabiii]

Description

This PR fixes a compliance issue where the trips-for-route endpoint was emitting an undocumented outOfRange field in its response data envelope.

The API specification explicitly requires the data object to only contain limitExceeded, list, and references. The discrepancy was caused by using models.NewListResponseWithRange instead of the standard models.NewListResponse.

Changes Made

• Handler Update: Swapped models.NewListResponseWithRange to models.NewListResponse in internal/restapi/trips_for_route_handler.go (for both the early return and main response paths).
• Test Coverage:
  • Added TestTripsForRouteHandler_OutOfRangeNotEmitted to fetch the raw JSON response as a map[string]any.
  • Added strict programmatic assertions to guarantee the "outOfRange" key is completely absent from the JSON payload.
  • Verified that "limitExceeded", "list", and "references" remain present.
• Structural Integrity: Purposely retained the OutOfRange boolean in internal/restapi/response_types.go (ListData struct) to avoid breaking deserialization for other location-based endpoints (like stops-for-location) that legally share this struct.

Closes: #1234

Summary by CodeRabbit

• Bug Fixes
  • Updated trips-for-route responses to omit the outOfRange field when it is not applicable.
  • Preserved other response details, including trip lists, references, and limit information.

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The trips-for-route handler now constructs list responses without range metadata. Tests verify that outOfRange is omitted while limitExceeded, list, and references remain present.

Changes

Trips-for-route response shape

Layer / File(s)	Summary
Standardize response construction and validate response fields internal/restapi/trips_for_route_handler.go, internal/restapi/trips_for_route_handler_test.go	Missing-agency, empty, and successful responses now use models.NewListResponse(...). Tests verify that outOfRange is absent and required list response fields remain present.

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

Possibly related PRs

• OneBusAway/maglev#903: Updates REST list-response handling and tests for the newer response model.
• OneBusAway/maglev#1112: Replaces range-based list responses with models.NewListResponse.
• OneBusAway/maglev#1216: Modifies the same trips-for-route response flow and tests.

Suggested reviewers: burma-shave, ahmedhossamdev, arcoder181105

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes removal of the non-spec outOfRange field from the trips-for-route response.
Linked Issues check	✅ Passed	The changes remove outOfRange from all trips-for-route response paths and add coverage for populated, empty-list, and unknown-agency responses [#1234].
Out of Scope Changes check	✅ Passed	The code and test changes directly support the trips-for-route response requirement and do not introduce unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.}

[aaronbrethorst]

Code review

Found 1 issue:

1. Incomplete after conflict resolution: main gained a third response path in this handler that still emits outOfRange. This branch is based on e2a1eeda, where the unknown-agency case still called api.sendNotFound. On current main that branch was replaced (commit a678600, "Return empty list for unknown route IDs") with models.NewListResponseWithRange([]models.TripsForRouteListEntry{}, *references, false, api.Clock, false), so main now has three NewListResponseWithRange call sites in trips_for_route_handler.go (lines 38, 198, 468). This PR converts only two of them. Since git won't flag line 38 as a conflict, resolving the merge will silently leave the unknown-agency path emitting outOfRange, and the endpoint will still be off-spec for that case — the openapi spec's trips-for-route data schema has only limitExceeded, list, and references, and CLAUDE.md says "If the codebase diverges from the spec, the spec wins." Please convert that third call site too, and consider extending TestTripsForRouteHandler_OutOfRangeNotEmitted to cover the unknown-agency and empty-list paths (currently only the populated success path is asserted).

maglev/internal/restapi/trips_for_route_handler.go

Lines 33 to 37 in 71b8760

	currentAgency, err := api.GtfsManager.GtfsDB.Queries.GetAgency(ctx, agencyID)
	if err != nil {
	api.sendNotFound(w, r)
	return
	}

Otherwise this looks right: the wiki spec for trips-for-route never mentions outOfRange (only the trips‐for‐route.md file with the U+2010 hyphen does, and that one is marked DRAFT - DO NOT IMPLEMENT), testdata/openapi.yml omits it from this endpoint's data schema while keeping it for stops-for-location, routes-for-location, trips-for-location, search-stop, and search-route, and the removal is correctly scoped to this handler only — no other handler's outOfRange is touched, and keeping OutOfRange on the shared ListData test struct is the right call.

🤖 Generated with Claude Code

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

[aaronbrethorst]

The premise is right and I checked it independently. outOfRange appears nowhere in the authoritative trips-for-route spec — the response schema lists limitExceeded, list, and references and nothing else — and testdata/openapi.yml agrees: the 200 schema for /api/where/trips-for-route/{routeID}.json defines exactly those three keys. Meanwhile trips-for-location, stops-for-location, routes-for-location, search/stop, and search/route all legitimately declare outOfRange, several as required. So removing it from this endpoint only is correct, and leaving OutOfRange on the shared ListData struct is also correct since those other endpoints decode through it.

I also like that you asserted key absence on the raw JSON via fetchRawData rather than through a typed struct — a typed decode can't tell "absent" from "false", so this is the only way the test actually proves anything.

One problem, and it's specifically a hazard of resolving this PR's merge conflict.

There's a third call site on main that this PR doesn't convert.

Your branch's merge base predates commit a678600 ("Return empty list for unknown route IDs"), which added another models.NewListResponseWithRange(...) to this handler for the unknown-agency path. Current main has three:

 38:  response := models.NewListResponseWithRange([]models.TripsForRouteListEntry{}, *references, false, api.Clock, false)
198:  response := models.NewListResponseWithRange([]models.TripsForRouteListEntry{}, references, false, api.Clock, false)
468:  response := models.NewListResponseWithRange(result, references, false, api.Clock, false)

The PR converts the two that existed on its base (the empty-list and success paths). Line 38 is new, and git won't flag it as a conflict — it's in a region your diff doesn't touch. So a straightforward conflict resolution leaves the unknown-route-ID path still emitting outOfRange, and the endpoint stays off-spec for exactly the case where a client is most likely to be poking at it.

Please rebase onto main and convert that one too. While you're there, it'd be worth extending TestTripsForRouteHandler_OutOfRangeNotEmitted to cover the empty-list and unknown-agency responses as well — right now only the populated success path is checked for key absence, which is how the third site slipped through in the first place. A small table over the three response paths would make this robust against the next handler that gets added.

For the record on provenance: I traced the field's origin and there's no spec-driven rationale to preserve. It arrived with the handler's initial implementation in 8ebd4f73 and got copy-pasted into new branches from there. No client requirement, no earlier spec reading. So this is a clean removal, not a behavior we're knowingly dropping.

Rebase, convert the third call site, broaden the test, and I'll merge it.

[burma-shave]

@CodeRabbit pause

[coderabbitai]

✅ Action performed

Reviews paused.

[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]

OBA API Review: working tree (branch fix-trips-for-route-gap6) — trips-for-route

Input: Working tree / branch fix-trips-for-route-gap6 (2 files changed vs. up-to-date main; also linked to open PR #1236)
Stated goal: Close issue #1234 — "Non-spec outOfRange field emitted in response envelope for trips-for-route"
Changes: Both (production + test)

Overview

What this change does: The trips-for-route handler (internal/restapi/trips_for_route_handler.go) built its response using models.NewListResponseWithRange, which unconditionally adds an outOfRange: false key to the data JSON object. The OBA API spec for this endpoint defines data as containing only limitExceeded, list, and references — outOfRange isn't part of the contract here. The fix swaps all three response-construction call sites (the "unknown agency" early return, the "no trips active in this window" empty-list path, and the main populated-results path) to models.NewListResponse, which builds the same map without the extra key. Before: every trips-for-route response carried a spurious outOfRange: false. After: that key is simply absent; limitExceeded, list, and references are unaffected.

Domain background: OneBusAway's data envelope shape varies slightly by endpoint family. Location-radius-search endpoints (stops-for-location, routes-for-location, trips-for-location, etc.) legitimately report outOfRange — it's meaningful there because a search can be centered outside the agency's coverage area. trips-for-route is not a location search; it resolves an explicit route ID and time window, so "out of range" isn't a concept that applies. The two constructor functions in internal/models/response.go (NewListResponse vs. NewListResponseWithRange) exist precisely to serve these two families — this PR simply corrects which one trips-for-route was using, without touching the function itself or the legitimately-outOfRange endpoints.

Goal check

Goal check: trips-for-route

Stated goal: data should contain only limitExceeded, list, references — no outOfRange.

• Envelope matches spec in all three response paths (ErrNoRows/unknown-agency, empty-list, populated): ✓ addressed
• No regression to typed decode path (ListData.OutOfRange in response_types.go still deserializes fine via zero-value default): ✓
• NewListResponseWithRange still correctly used elsewhere (trips-for-location, routes-for-location, stops-for-location, vehicles-for-agency, search-stops, route-search): ✓ unaffected

Test coverage: adequate. New TestTripsForRouteHandler_OutOfRangeNotEmitted checks raw JSON (via the existing fetchRawData helper, not hand-rolled) across three scenarios that map 1:1 onto the three modified code paths — populated, empty-list-at-shifted-time, and unknown-agency (confirmed this specifically triggers the sql.ErrNoRows branch). The one weakened assertion (assert.False(t, model.Data.OutOfRange) removed from an existing test) is a correct removal, not a coverage loss — it only checked the field defaulted to false; its replacement (checking key absence) is strictly stronger.

Overall: fully closed

Client impact

Behaviour	Wayfinder/SDK	iOS	Android
data.outOfRange no longer emitted on trips-for-route responses	none	none	none

• JS SDK/Wayfinder: TripsForRouteListResponse.Data never declared outOfRange; no component reads it off this endpoint's response.
• iOS: doesn't call trips-for-route at all. Separately, outOfRange is decoded generically (optional, decodeIfPresent) on the shared RESTAPIResponse<T> and has no downstream reader anywhere in the codebase — dead property regardless.
• Android: ListWithReferences.outOfRange defaults to false via kotlinx.serialization when the key is missing — identical to what it always deserialized to before (spec confirms this field was always false for this endpoint, a documented Java-parity artifact). The only real consumer of .outOfRange (MapDataSource.kt) reads it from a different call (stopsForLocation); the actual trips-for-route adapter (asRouteTrips()) never touches it.

Spec check

Overall: spec-consistent

• outOfRange removal matches the spec's data schema exactly.
• Extended the check to the rest of the handler for drift beyond this PR's scope: maxCount (correctly unimplemented, matching the spec's Implementation Decisions entry), includeTrip/includeStatus/includeSchedule/includeReferences defaults (all correctly default true), data.limitExceeded (correctly hardcoded false, matching the documented Suspected Defect this PR doesn't touch), and data.list[] field shape (matches spec, including omitempty schedule/status). No new deviations found; no Implementation Decisions entry needed.

Summary

This is a clean, minimal, single-purpose fix: it closes issue #1234 exactly as scoped, with no client-visible regressions across Wayfinder/JS-SDK, iOS, or Android, and no spec drift elsewhere in the handler. Test coverage is precise and reuses existing helpers rather than hand-rolling new plumbing.

Comments have been addressed