Fix/arrival-and-departure-for-stop endpoint#962
Draft
3rabiii wants to merge 15 commits into
Draft
Conversation
📝 WalkthroughWalkthroughThis pull request standardizes database resource cleanup patterns across gtfsdb, fixes a model serialization issue in the ChangesArrival and Departure Response Flow Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/arrival_and_departure_for_stop_handler.go`:
- Around line 373-375: The code is deriving a zero-based stop ordinal from raw
GTFS stop_times.stop_sequence (stopSequenceZeroBased) which can be sparse and
causes mismatches with the lookup that uses params.StopSequence; instead compute
a trip-relative ordinal in the DB/query layer and return that same ordinal with
targetStopTime and totalStopsInTrip so request matching and response use the
same value. Update the call site that uses
GetTargetStopTimeWithTotalStopsBySequence(...) to request and receive a
trip-relative ordinal (not raw stop_sequence), stop using params.StopSequence
directly for lookup, and replace stopSequenceZeroBased, arrivalEnabled and
departureEnabled calculations to use the returned trip-relative ordinal and
totalStopsInTrip (reference symbols: GetTargetStopTimeWithTotalStopsBySequence,
params.StopSequence, targetStopTime, totalStopsInTrip, stopSequenceZeroBased,
arrivalEnabled, departureEnabled).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e6ad429-2af9-4ee3-badc-d1d38685fff4
📒 Files selected for processing (6)
gtfsdb/block_layover_test.gogtfsdb/stops_rtree.gointernal/models/arrival_and_departure.gointernal/models/current_time_test.gointernal/models/trip_details_test.gointernal/restapi/arrival_and_departure_for_stop_handler.go
Comment on lines
+373
to
+375
| stopSequenceZeroBased := int(targetStopTime.StopSequence) - 1 | ||
| arrivalEnabled := stopSequenceZeroBased > 0 | ||
| departureEnabled := stopSequenceZeroBased < totalStopsInTrip-1 |
There was a problem hiding this comment.
Don’t derive the public stop sequence from raw stop_times.stop_sequence.
This now emits a zero-based stopSequence, but the lookup path above still passes params.StopSequence straight into GetTargetStopTimeWithTotalStopsBySequence(...). That means a client echoing back the value returned here can miss the row entirely, and arrivalEnabled / departureEnabled also become wrong on trips whose GTFS stop sequences are sparse or don’t start at 1. Please compute a trip-relative ordinal in the query layer and use that same ordinal consistently for both request matching and response fields.
Also applies to: 417-417
🤖 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/arrival_and_departure_for_stop_handler.go` around lines 373
- 375, The code is deriving a zero-based stop ordinal from raw GTFS
stop_times.stop_sequence (stopSequenceZeroBased) which can be sparse and causes
mismatches with the lookup that uses params.StopSequence; instead compute a
trip-relative ordinal in the DB/query layer and return that same ordinal with
targetStopTime and totalStopsInTrip so request matching and response use the
same value. Update the call site that uses
GetTargetStopTimeWithTotalStopsBySequence(...) to request and receive a
trip-relative ordinal (not raw stop_sequence), stop using params.StopSequence
directly for lookup, and replace stopSequenceZeroBased, arrivalEnabled and
departureEnabled calculations to use the returned trip-relative ordinal and
totalStopsInTrip (reference symbols: GetTargetStopTimeWithTotalStopsBySequence,
params.StopSequence, targetStopTime, totalStopsInTrip, stopSequenceZeroBased,
arrivalEnabled, departureEnabled).
3rabiii
force-pushed
the
fix/arrival-departure-endpoint
branch
from
64f5c18 to
b9228ce
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses several critical functional parity gaps in the
arrival-and-departure-for-stopAPI endpoint to ensure it matches the legacy OneBusAway implementation and strictly follows the API specification.Key Changes
Gap 1
vehicleId: Formats the vehicle ID into the combined{agencyId}_{vehicleId}structure instead of returning the raw GTFS ID.Gap 2: Sparse Sequence & Arrival/Departure Fix`: Computed a 0-based stop_ordinal in SQL to handle sparse GTFS sequences. This stable ordinal dynamically controls the arrival/departureEnabled flags and prevents 404 errors by safely matching client echo-requests.
Gap 3
routeShortName: Implements the 3-step narrative fallback for route short names.Gap 4
tripHeadsign: Implements the stop-level narrative override for trip headsigns.Gap 5
lastUpdateTime: Fixes the JSON serialization tag so it correctly emits0when there is no real-time vehicle data, instead of omitting the field entirely.Gap 6
status& Cancelled Trips: Properly handlesCANCELEDschedule relationships by setting the entrystatusto"CANCELED", zeroing out predictions, stripping the vehicle ID, and omitting thetripStatusobject.Gap 7 Dynamic
status: Maps non-scheduled GTFS-RT statuses (likeADDEDorDUPLICATED) directly to the top-level entrystatusinstead of unconditionally returning"default".Gap 8 Prediction Zeroing: Ensures
predictedArrivalTimeandpredictedDepartureTimeare reset to0whenpredictedisfalse, rather than erroneously falling back to the scheduled times.Gap 9 serviceDate Parsing: Supports YYYY-MM-DD date strings in the serviceDate parameter and correctly localizes dates to the agency's timezone to prevent calendar-day shifts.
Gap 10 time Parsing: Supports yyyy-MM-dd_HH-mm-ss datetime strings in the time parameter and localizes it to the agency's timezone.
Gap 11 includeReferences: Supports includeReferences=false for the singular endpoint to omit reference details (Agencies, Routes, Stops, Trips) from the response when requested.
Gap 12 stopSequence Search: Implements the expand-outward search algorithm for stopSequence matching, permitting tolerance to shifted indices from schedule modifications.
Gap 13 Closest Stop Visit: Resolves loop route ambiguity by selecting the closest scheduled stop visit using SQLite-driven absolute time difference calculations when stopSequence is absent.
Gap 14 blockTripSequence Fix: Accurately computes the 0-based trip sequence within a block by filtering active services. Updates the sentinel value to -1 for blockless trips and ensures the first trip (index 0) is correctly sent to the client.
Summary by CodeRabbit
Bug Fixes
Chores