Add arrivals-and-departures-for-location endpoint - #1408
Open
ARCoder181105 wants to merge 11 commits into
Open
Conversation
The arrivals-and-departures-for-stop handler carried its entire pipeline inline: the +/-1 day service window scan, the batch route and trip resolution, the per-row arrival construction, and the reference assembly. Nothing was callable from anywhere else, so a second endpoint needing arrivals for many stops would have to duplicate all of it. Move that pipeline into arrivals_core.go behind arrivalsForStop and buildArrivalsReferences, with an arrivalsAccumulator gathering the routes, trips, stops and situations that the arrivals reference. The accumulator is what makes the code reusable across several stops: a caller loops over stops sharing one accumulator and gets a single deduplicated references block. Behavior is unchanged; the test suite passes without modification. Two details worth noting for review: BuildTripStatus now receives the vehicle the caller already looked up rather than nil. It is not a fix -- BuildTripStatus falls back to the same GetVehicleForTrip call when handed nil -- so the result is identical, but it drops one redundant lookup per arrival row. The extracted stop reference deliberately keeps its inline literal instead of calling buildStopModel. That helper defaults Code to the stop ID when stops.code is NULL, where this endpoint emits an empty string, so adopting it would be a response change rather than a refactor.
Contributor
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
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 |
Four functions in the extracted core sat above the quality gate's
cognitive complexity threshold of 15: activeStopTimesForWindow at 21,
appendStopReferences at 18, buildArrival at 17 and batchArrivalEntities
at 16.
Pull the distinct sub-tasks out of each:
- stopTimesForServiceDay, the per-service-day scan
- uniqueRouteAndTripIDs and tripStopCounts, the batch input and
stop-count preparation
- combinedVehicleID and tripStatusForArrival, the vehicle and trip
status resolution
- loadStopReferenceData and collectStopRoutes, the reference batch
load and per-stop route rendering
One behaviour does change. A failed GetStopsByIDs in the stop reference
load is now returned rather than logged and skipped past. Swallowing it
dropped every stop reference and still answered 200, so the entry named
stops the client had no way to resolve; the handler now surfaces it as
a 500. The routes lookup keeps its warn-and-continue, since losing it
only costs each stop its routeIds.
The query returned rows in whatever order SQLite produced them. Callers resolving a stop served by several agencies take the first row they see, so the agency a stop was namespaced under could change between runs on identical data. Order by stop then agency so that choice is stable.
parseMaxCount hardcoded models.MaxAllowedCount as the ceiling for every caller. arrivals-and-departures-for-location documents a maximum of 1000, which that shared value of 250 cannot express. Take the ceiling as a parameter and add ParseMaxCountClampedTo for callers that need their own. The existing ParseMaxCount and ParseMaxCountClamped keep passing MaxAllowedCount, so nothing changes for current callers.
Serves GET /api/where/arrivals-and-departures-for-location.json, which
returns arrivals for every stop in a bounding box or radius. It reuses
the per-stop arrivals core, so the handler is a loop over the stops the
spatial query returns plus the response assembly around it.
Parameters follow the OpenAPI spec: lat and lon required, radius taking
precedence over latSpan/lonSpan, minutesBefore 5, minutesAfter 35,
maxCount 250 clamped to 1000, plus routeType and emptyReturnsNotFound.
routeType filters the arrivals and prunes nearby stops that serve no
route of a requested type, but deliberately does not filter the stop
search itself -- stopIds is unaffected by it, matching both the Java
implementation and the deployed server.
nearbyStopIds is the union of the stops within 100m of each matched
stop, each excluding itself, measured from the centre of the search
area and ordered nearest first. It is deliberately not "every stop in
the box": a matched stop with no neighbour within 100m does not appear,
while a stop just outside the box does if it neighbours one inside.
Verified against the deployed Puget Sound server over the same query.
Restricted to the one agency a single static feed can load, the stop
and nearby-stop sets match exactly, with distances agreeing to within
0.05m. Five behaviours diverge, each because the deployed server
contradicts the OpenAPI spec:
- stopIds is deduplicated. The Java service adds every matched stop
to the list twice.
- An empty result keeps the entry/references envelope. The Java empty
path skips the bean factory and emits a different shape.
- emptyReturnsNotFound returns 404. The Java code sets the 404 and
then overwrites it with a 200.
- lat and lon alone search the default 600m radius. The Java action
never configures a default and builds a zero-area box.
- Arrivals are sorted by arrival time. The Java sort compares a
distance that is never populated, so it does nothing.
ARCoder181105
force-pushed
the
feat/arrivals-and-departures-for-location
branch
from
August 28, 2026 13:50
6186d40 to
d954239
Compare
Three functions sat above the quality gate's threshold of 15: the
handler itself at 19, agenciesForStops at 17 and
parseArrivalsForLocationParams at 16.
Pull out the sub-tasks so each reads as a sequence of named steps:
- sendArrivalsForLocationError, replacing two copies of the
cancelled-versus-failed branch
- locationLists with truncateLocationLists, combinedStopIDs,
registerReferencedStops and locationReferences
- agencyLocationOrUTC and mostCommonAgency
- parseRequiredLocation, parseEpochMillisParam,
parseOptionalBoolParam and forwardFieldErrors, which replaces two
copies of the field-error funnelling loop
No behaviour change.
Collaborator
Author
|
Cleared the SonarCloud findings in c5489a0. Quality gate is green (0 issues). Three functions were over the cognitive complexity threshold of 15. Split each into named steps:
Two of these removed real duplication rather than just moving lines: the cancelled-versus-failed error branch was written out twice, and the field-error funnelling loop was written out twice. No behaviour change — the handler tests and the full suite pass unchanged. Also rebased onto the latest #1407 after its review fixes. Once that merges, the diff here drops to the endpoint alone. |
Collaborator
|
I'll generate a spec for this, hopefully next week. We'll hold off on merging until this can be reviewed against a spec like the other endpoints. |
# Conflicts: # internal/restapi/arrivals_and_departures_for_stop_handler.go
The extraction in refactor/extract-arrivals-core predates main's GTFS frequency work, so merging main in silently dropped frequency data from arrivals-and-departures-for-stop: BuildTripStatus's freqMap parameter had nothing feeding it. Batch-fetch frequencies alongside routes and trips in batchArrivalEntities, thread the map through arrivalInput, and apply it in a small applyFrequency helper kept separate from buildArrival to avoid pushing its cognitive complexity back over the SonarCloud limit. This also gives the location endpoint frequency support for free, since it shares the same core.
A hard 500 here means a single failed batch stop lookup takes down an otherwise complete arrivals response. Match the treatment already given to the sibling route lookup: log and continue with an incomplete references.stops rather than failing the whole request.
The prior wording read as if any caller could rely on this field to namespace alerts. It is only ever read by the single-stop handler; a multi-stop caller must pass a per-stop agency ID directly to situations.add instead. Reword to state that plainly rather than scoping the field, since it is genuinely single-caller today.
…departures-for-location
|
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
2 participants
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.
Closes #799. Replaces #787.
Summary
Implements
GET /api/where/arrivals-and-departures-for-location.json— arrivals for every stop in a bounding box or radius. Built on the arrivals core extracted in #1407, so the handler is a loop over the spatial query's stops plus response assembly.What changed
arrivals_and_departures_for_location_handler.gomodels/stops.go,models/response.go,models/arrival_and_departure.goStopWithDistance, entry type, response constructorutils/api.go,models/constants.gomaxCountceiling is now per-endpoint; this one needs 1000, the shared default is 250gtfsdb/query.sqlORDER BYonGetAgenciesForStops— multi-stop agency resolution was otherwise nondeterministicroutes.goParameters
Per the OpenAPI spec:
lat/lonrequired,radiustakes precedence overlatSpan/lonSpan,minutesBefore5,minutesAfter35,maxCount250 clamped to 1000, plusrouteTypeandemptyReturnsNotFound.nearbyStopIdsThe union of the stops within 100 m of each matched stop, each excluding itself, measured from the centre of the search area and ordered nearest first.
Deliberately not "every stop in the box" — a matched stop with no neighbour within 100 m does not appear, while a stop just outside the box does if it neighbours one inside.
Verified against the live deployed server
Ran locally against the King County Metro feed and compared to
api.pugetsound.onebusaway.orgon the same query. Puget Sound aggregates several agencies where a maglev instance loads one static feed, so the comparison is restricted to agency1:stopIdsnearbyStopIdsParameter behaviour matches on
maxCount(truncates all three lists, setslimitExceeded),maxCountclamping,routeType,radiusvs span precedence, and the empty-area case. The two response-key differences are both explained:tripStatus.scheduledis a pre-existing maglev field on the existing for-stop endpoint too, and the situation keys are absent only because the sole alert in that area belongs to agency40, which this config does not load — alerts resolve correctly elsewhere in the feed.Divergences from the deployed server — please review these
Five behaviors differ, each because the deployed server contradicts the OpenAPI spec:
stopIdsis deduplicated. The Java service adds every matched stop to the list twice (StopWithArrivalsAndDeparturesBeanServiceImpllines 115 and 122; its own comment says 115 shouldn't be there). Live response: 44 entries, 22 unique.entry/referencesenvelope. The Java empty path skipsBeanFactoryV2and emits a different shape ({stops, nearbyStops, situations, timeZone}, noentry, noreferences). The spec marks both required.emptyReturnsNotFound=truereturns 404. Java sets the 404 and then overwrites it with a 200, so it never ships.lat/lonalone search the default 600 m radius. The Java action never configures a default radius, so it builds a zero-area box and always returns empty.routeTypefiltering the arrivals and pruning nearby stops but not the stop search is preserved as-is — spec and deployed server agree, and changing it would be the bigger break. Confirmed live:routeType=99returns the fullstopIdslist with zero arrivals and zero nearby stops.Testing
13 tests covering parameter validation,
radiusvs span precedence, the default-radius fallback,maxCounttruncating all three lists,routeTypefiltering arrivals but not stops,emptyReturnsNotFound,stopIdsdedup,nearbyStopIdsordering and reference resolution, andincludeReferences=false. Added toTestOpenAPIConformance_LocationEndpoints, which validates the response against the spec schema.