Skip to content

Prefix location results with each stop's agency - #1796

Closed
aaronbrethorst wants to merge 1 commit into
ab/stop-id-routesfrom
ab/stop-id-single
Closed

aaronbrethorst wants to merge 1 commit into
ab/stop-id-routesfrom
ab/stop-id-single

Conversation

@aaronbrethorst

Copy link
Copy Markdown
Member

Stacked on #1795. Part of the stop consolidation work (#1771–#1779). No issue filed yet.

What it does

stops-for-location, arrivals-and-departures-for-location and the nearbyStopIds of arrivals-and-departures-for-stop now prefix each stop with its own agency.

Root cause

All three chose a stop's prefix from GetAgenciesForStops, the agencies whose routes serve it, taking the first by agency ID. Java OneBusAway uses the stop's own agency, its feed's first. So a stop shared between agencies could come out under the wrong one.

All three now read loadStopAgencyIDs:

  • The location arrivals index fills from it through addStopAgencies, which also records each agency's timezone for the service-day math.
  • GetAgenciesForStops stays only for search's references.agencies, and its query comment now says so.

Testing

  • Test failing on main before this change: TestLocationStopIDs_UseTheStopsOwnAgency, on the two-agency fixture, where other sorts before own.
    • It checks stops-for-location and arrivals-and-departures-for-location around stop-1.
    • Both returned other_stop-1 and other_stop-2 before this change, and return own_stop-1 and own_stop-2 now.
  • Changed fixtures: three stops-for-location tests build stops by hand. They now give those stops their agency with assignStopAgencies.

Response comparison

  • Java: a stop's ID is set once, from its feed (StopEntriesFactory). Production stop/29_16140 returns null even though Community Transit route 29_130 serves 1_16140.
  • Maglev before: stops-for-location near 1_16140 would pick between 1_ and 29_ by agency ID order.
  • Maglev after: always 1_16140.

Checklist

  • This PR addresses one issue.
  • It is under ~200 changed lines, or the description says why it can't be split.
  • This PR is not a bug fix, or the new or changed test fails on main and passes on this branch.
  • Each commit is one logical change with a clear message.
  • I did not hand-edit sqlc-generated files under gtfsdb/ (db.go, models.go, query.sql.go) or testdata/openapi.yml (synced from upstream).
  • This PR changes no handler, or handlers pass r.Context() to DB/service calls, not context.Background().
  • The response comparison above is filled in, or this PR doesn't change a Java-served response.
  • This PR adds no endpoint, or each new endpoint matches testdata/openapi.yml, has an issue open on OneBusAway/sdk-config, or is outside the SDK and the description says why.
  • I read every line of this diff and can explain it if asked.

Notes for reviewers

  • Stops-for-location with a query. It used to drop a stop with no routes at all even when a query (stop code) was given, because no agency resolved for it. Every stop has an agency now, so a stop-code query finds such a stop, as Java's stop index does. Searches without a query still drop stops with no routes that day.
  • Next in the stack. stopReferences still accepts several IDs per stop (idsByBareID), for trips-for-route, trips-for-location and stops-for-location parent stations. That collapses next.

stops-for-location, arrivals-and-departures-for-location and the
nearbyStopIds of arrivals-and-departures-for-stop chose a stop's prefix
from the agencies whose routes serve it, taking the first by ID. Java
OneBusAway uses the stop's own agency, so a shared stop could come out
under the wrong one.

All three now read loadStopAgencyIDs. The location arrivals index fills
from it through addStopAgencies, which also records each agency's
timezone. GetAgenciesForStops remains for references.agencies only, and
its comment says so.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 8d9ea38e-5f87-41c1-bdd7-fd472e6d922d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

@github-actions

Copy link
Copy Markdown

@aaronbrethorst, the PR description does not follow the PR template:

  • No linked issue. Add Fixes #N (or Part of #N) for a triaged issue.
  • Testing: "Manual check:" is empty.
  • Response comparison: "Request URL(s):" is empty.
  • Response comparison: "Java server, feeds and date checked:" is empty.
  • Checklist item is not checked: "This PR addresses one issue." If this item doesn't apply to your PR, ask a maintainer.
  • Checklist item is not checked: "I read every line of this diff and can explain it if asked." If this item doesn't apply to your PR, ask a maintainer.

Keep this PR as Draft until these are fixed. This comment updates when the description changes.

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

Performance Smoke Test Results

Status: PASSED

Metric Value
p(95) latency 1.6 ms
Error rate 0.00%
Total requests 341
Req/sec 11.2

Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%.

Full results uploaded as workflow artifact: k6-smoke-summary.

@aaronbrethorst

Copy link
Copy Markdown
Member Author

Closing as superseded. The multi-feed static GTFS spec (#1811, docs/multi-agency-static-feeds-spec.md §10) folds this stack's stop-ID rules into the per-feed default agency and the stored-ID identity model; they return as phases B and C of that plan. The Java-recorded data in testdata/java_parity and the tests on this branch will be reused there. #1806, #1807 and #1810 are not identity work and stay open to be re-cut against main.

@aaronbrethorst
aaronbrethorst deleted the ab/stop-id-single branch November 8, 2026 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant