Add OTP 2.x GraphQL trip-planning support#1794
Merged
Merged
Conversation
Sound Transit's OTP server has dropped the OTP1 REST /plan endpoint in favor of GraphQL, so this adds an OTP 2.x planning path alongside the existing OTP1 REST stack, targeting the same app-owned TripItinerary domain model (#1778) so the UI/ViewModels/trip monitor need no changes. - Vendors OTP's GraphQL schema (pinned to opentripplanner/OpenTripPlanner v2.9.0) and wires up Apollo Kotlin codegen for the planConnection query. - Adds an explicit Region.usesOtp2GraphQl protocol flag (threaded through RegionDto/Room, migration 5->6) plus a manual "Use GraphQL (OTP 2.x)" toggle in Advanced Settings for the custom-OTP-URL case. Infra only: no bundled region is flipped on by this change. - Otp2PlanRequestBuilder / Otp2PlanAdapters translate to/from the GraphQL wire format (ISO-8601 times/durations, structural vertexType inference, extended TripMode enum) using the same "mint at the boundary" discipline as the OTP1 adapter. - Otp2Planner executes queries via a per-base-URL cached ApolloClient; DefaultTripPlanRepository dispatches OTP1 vs OTP2 on the resolved TripRequestBuilder.usesOtp2 flag. Verified against a real OTP2 GraphQL server (Sound Transit) end-to-end via an instrumented smoke test exercising the production repository directly; 823 unit tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 14 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds OTP2 GraphQL trip planning with Apollo code generation, explicit OTP1/OTP2 selection, Room persistence, custom URL settings, request and response adapters, planner dispatch, and tests covering migration, modes, decoding, and OTP1 defaults. ChangesOTP2 GraphQL planning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TripPlanRepository
participant TripRequestBuilder
participant Otp2Planner
participant ApolloClient
participant Otp2PlanAdapters
TripPlanRepository->>TripRequestBuilder: resolve protocol and base URL
TripPlanRepository->>Otp2Planner: plan OTP2 request
Otp2Planner->>ApolloClient: execute planConnection
ApolloClient-->>Otp2Planner: return GraphQL plan data
Otp2Planner->>Otp2PlanAdapters: map response
Otp2PlanAdapters-->>TripPlanRepository: return TripItinerary list
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt (1)
64-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOld
ApolloClientis not closed when the base URL changes.
ApolloClientimplementsCloseable(owns a coroutine scope and cached state). When a new URL replaces the cached entry, the previous client is orphaned withoutclose(). Low impact in practice (one URL per instance per the comment), but still a leak on region switches.♻️ Proposed fix: close the old client before replacing
private fun apolloClientFor(baseUrl: String): ApolloClient = synchronized(this) { cachedClient?.takeIf { it.first == baseUrl }?.second ?: ApolloClient.Builder().serverUrl(baseUrl).okHttpClient(okHttpClient).build() - .also { cachedClient = baseUrl to it } + .also { + cachedClient?.second?.close() + cachedClient = baseUrl to it + } }🤖 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 `@onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt` around lines 64 - 68, Update apolloClientFor so that when the requested baseUrl differs from cachedClient, the previously cached ApolloClient is closed before replacing cachedClient with the newly built client. Preserve reuse of the cached client when the URLs match and keep the existing synchronized construction flow.
🤖 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
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt`:
- Around line 214-219: Update the AdvancedSettingsScreen GraphQL
SwitchPreferenceItem visibility to render only when state.customOtpApiUrl is
set, while preserving its existing title, summary, checked state, and change
handler for custom URLs.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt`:
- Around line 80-82: Update the ApolloException handling in the trip-planning
request flow to distinguish actual timeout failures from other ApolloException
subtypes. Keep tripplanner_error_request_timeout only for timeout exceptions,
and map non-timeout failures to the existing tripplanner_error_not_defined
fallback when no dedicated network-error string is available; preserve the
original exception as the cause.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt`:
- Around line 64-68: Update apolloClientFor so that when the requested baseUrl
differs from cachedClient, the previously cached ApolloClient is closed before
replacing cachedClient with the newly built client. Preserve reuse of the cached
client when the URLs match and keep the existing synchronized construction flow.
🪄 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: 4cd9556a-5643-4c8f-beb9-72bdd6839cc5
📒 Files selected for processing (35)
build.gradleonebusaway-android/build.gradleonebusaway-android/schemas/org.onebusaway.android.database.AppDatabase/6.jsononebusaway-android/src/androidTest/java/org/onebusaway/android/database/AppDatabaseMigrationTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/mock/MockRegion.javaonebusaway-android/src/main/graphql/otp2/Plan.graphqlonebusaway-android/src/main/graphql/otp2/schema.graphqlsonebusaway-android/src/main/java/org/onebusaway/android/api/adapters/Otp2PlanAdapters.ktonebusaway-android/src/main/java/org/onebusaway/android/api/adapters/RegionAdapters.ktonebusaway-android/src/main/java/org/onebusaway/android/api/adapters/TripPlanAdapters.ktonebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/DatabaseModule.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/Otp2HttpClient.ktonebusaway-android/src/main/java/org/onebusaway/android/database/AppDatabase.ktonebusaway-android/src/main/java/org/onebusaway/android/database/Migrations.ktonebusaway-android/src/main/java/org/onebusaway/android/database/oba/ObaEntities.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/model/TripItinerary.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilder.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/util/TripRequestBuilder.ktonebusaway-android/src/main/java/org/onebusaway/android/region/Region.ktonebusaway-android/src/main/java/org/onebusaway/android/region/RegionMapper.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanRepository.ktonebusaway-android/src/main/java/org/onebusaway/android/util/RegionUtils.javaonebusaway-android/src/main/res/values/donottranslate.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/test/java/org/onebusaway/android/api/Otp2PlanDecodeTest.ktonebusaway-android/src/test/java/org/onebusaway/android/api/RegionsDecodeTest.ktonebusaway-android/src/test/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilderTest.ktonebusaway-android/src/test/java/org/onebusaway/android/region/RegionTestFixtures.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt
…ence
OTP2's GraphQL API replaced the old binary optimize=TRANSFERS/QUICK enum with
a continuous PlanPreferencesInput.transit.transfer.cost (seconds, added per
transfer). Converts Plan.graphql's preferences argument from a hardcoded
wheelchair-only literal to a full $preferences variable so both settings can
be built together in Otp2PlanRequestBuilder.buildPreferences().
The 1800-second value isn't a guess: cloned OpenTripPlanner at v1.5.0 and
found optimize=TRANSFERS's actual historical behavior in
RoutingResource.java (request.transferPenalty += 1800), confirmed the same
concept carries forward to OTP2's transfer.cost via
LegacyRouteRequestMapper's callWith.argument("transferPenalty", tx::withCost).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oggle - Otp2Planner.apolloClientFor: close the previously-cached ApolloClient before replacing it on a base-URL change, instead of leaking its coroutine scope. - Otp2Planner.plan: only map ApolloNetworkException (actual transport failures/timeouts) to the "request timeout" message; other ApolloException subtypes (HTTP status, parse failure, GraphQL-protocol error) now fall back to the generic error message instead of incorrectly claiming a timeout. - AdvancedSettingsScreen: hide the "Use GraphQL (OTP 2.x)" toggle unless a custom OTP URL is actually set, since TripRequestBuilder.usesOtp2 only reads it in that case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
bmander
added a commit
to bmander/onebusaway-android
that referenced
this pull request
Jul 12, 2026
…sAway#1780) OneBusAway#1794 selected OTP2 with a `usesOtp2GraphQl` boolean layered on the single `otpBaseUrl` — but the OBA regions directory doesn't publish that key, and a region's GraphQL endpoint is a different host than its OTP1 REST server (Puget Sound's REST server is otp.prod.sound.obaweb.org/otp/routers/default), so the two URLs can't share one field. The directory carries a dedicated `otpBaseGraphqlUrl` key instead — the mechanism for pointing a region at its OTP2 server on demand. Replace the boolean with an `otpBaseGraphqlUrl` field end to end (RegionDto → Room → Region): a non-blank value is both the explicit OTP2 signal and the OTP mount base. `TripRequestBuilder.usesOtp2`/`formattedOtpBaseUrl` derive from it, and `Otp2Planner` appends OTP2's fixed `/gtfs/v1` gtfs GraphQL mount to reach the endpoint — mirroring the OTP1 path appending `/plan`, and matching the custom-URL hint that already takes an `…/otp` base. Selection stays explicit, never sniffed. Point Puget Sound at its OTP2 GraphQL server in the bundled regions seed (otpBaseGraphqlUrl = …/prod/otp; the base serves the OTP root, base + /gtfs/v1 serves GraphQL — both verified live). Other bundled regions stay on OTP1. Fix the OTP2 bikeshare mode encoding: `Otp2PlanRequestBuilder` sent bare BICYCLE_RENTAL in the access/egress (Transit+Bike) and direct (Bikeshare) mode lists, which OTP2 rejects with a BadRequestError ("BIKE_RENTAL needs to be combined with WALK mode for the same leg"). Both lists now include WALK, verified against the live server. Device-verified end to end (Pixel 7 Pro, Puget Sound). Because OneBusAway#1794 is unreleased, this revises the v6 schema/migration in place (regions.uses_otp2_graphql → regions.otp_base_graphql_url) rather than shipping a dead column plus a churn migration; contributors on merged main clear app data. NOTE: the live regions directory must be repointed to match before this ships — it currently serves Puget Sound otpBaseGraphqlUrl = sound-transit-otp.ibi- transit.com/prod/, which 404s, and would break Puget Sound trip planning as soon as a device refreshes regions over the bundled seed.
bmander
added a commit
that referenced
this pull request
Jul 12, 2026
#1799) #1794 selected OTP2 with a `usesOtp2GraphQl` boolean layered on the single `otpBaseUrl` — but the OBA regions directory doesn't publish that key, and a region's GraphQL endpoint is a different host than its OTP1 REST server (Puget Sound's REST server is otp.prod.sound.obaweb.org/otp/routers/default), so the two URLs can't share one field. The directory carries a dedicated `otpBaseGraphqlUrl` key instead — the mechanism for pointing a region at its OTP2 server on demand. Replace the boolean with an `otpBaseGraphqlUrl` field end to end (RegionDto → Room → Region): a non-blank value is both the explicit OTP2 signal and the OTP mount base. `TripRequestBuilder.usesOtp2`/`formattedOtpBaseUrl` derive from it, and `Otp2Planner` appends OTP2's fixed `/gtfs/v1` gtfs GraphQL mount to reach the endpoint — mirroring the OTP1 path appending `/plan`, and matching the custom-URL hint that already takes an `…/otp` base. Selection stays explicit, never sniffed. Point Puget Sound at its OTP2 GraphQL server in the bundled regions seed (otpBaseGraphqlUrl = …/prod/otp; the base serves the OTP root, base + /gtfs/v1 serves GraphQL — both verified live). Other bundled regions stay on OTP1. Fix the OTP2 bikeshare mode encoding: `Otp2PlanRequestBuilder` sent bare BICYCLE_RENTAL in the access/egress (Transit+Bike) and direct (Bikeshare) mode lists, which OTP2 rejects with a BadRequestError ("BIKE_RENTAL needs to be combined with WALK mode for the same leg"). Both lists now include WALK, verified against the live server. Device-verified end to end (Pixel 7 Pro, Puget Sound). Because #1794 is unreleased, this revises the v6 schema/migration in place (regions.uses_otp2_graphql → regions.otp_base_graphql_url) rather than shipping a dead column plus a churn migration; contributors on merged main clear app data. NOTE: the live regions directory must be repointed to match before this ships — it currently serves Puget Sound otpBaseGraphqlUrl = sound-transit-otp.ibi- transit.com/prod/, which 404s, and would break Puget Sound trip planning as soon as a device refreshes regions over the bundled seed.
This was referenced Jul 18, 2026
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.
Summary
Sound Transit's OTP server has dropped the OTP1 REST
/planendpoint entirely in favor of GraphQL (planConnection). This adds an OTP 2.x GraphQL trip-planning path alongside the existing OTP1 REST stack, targeting the same app-ownedTripItinerarydomain model from #1782/#1787/#1790 (#1778) so the UI/ViewModels/trip monitor need zero changes.opentripplanner/OpenTripPlannertagv2.9.0— not live-introspected, per the issue's guidance) and adds Apollo Kotlin codegen for theplanConnectionquery, using aPlaceFieldsfragment soLeg.from/Leg.toshare one generated type.Region.usesOtp2GraphQlprotocol flag, threaded throughRegionDto→ Room (migration5→6, tested inAppDatabaseMigrationTest) →Region, plus a manual "Use GraphQL (OTP 2.x)" toggle in Advanced Settings for the custom-OTP-URL case (there's noRegionto carry the flag there). Infra only — no bundled region (including Puget Sound) is flipped to OTP2 by this PR; testing happens via the custom-URL override. Flipping a real region is a follow-up once Sound Transit's endpoint is stable.Otp2PlanRequestBuilder/Otp2PlanAdapterstranslate to/from the GraphQL wire format (ISO-8601 offset-datetimes and durations, seconds vs. OTP1's ms, structuralvertexTypeinference fromstop/rentalVehicle/vehicleParking/vehicleRentalStationrather than OTP2's own deprecated field, extendedTripModeenum for OTP2's mode superset) using the same "mint at the boundary" discipline as the OTP1 adapter —requireField/toEnum<E>()are shared between both adapters.optimize=TRANSFERSpreference through to OTP2'sPlanPreferencesInput.transit.transfer.cost. OTP2 replaced the old binary enum with a continuous per-transfer cost (seconds); the1800s value isn't a guess — clonedOpenTripPlanneratv1.5.0and confirmedoptimize=TRANSFERShistorically did exactlyrequest.transferPenalty += 1800(RoutingResource.java), and that the sametransferPenaltyconcept carries forward to OTP2'stransfer.costvia the current repo's ownLegacyRouteRequestMapper(callWith.argument("transferPenalty", tx::withCost)).Otp2Plannerexecutes queries via anApolloClientcached per base URL (avoids rebuilding it on the trip monitor's 60s repeating tick) and mapsroutingErrorsto user-facing messages, reusing OTP1's strings where the failure mode is equivalent and adding two new ones where it isn't.DefaultTripPlanRepositorydispatches OTP1 vs OTP2 onTripRequestBuilder.usesOtp2— explicit, never sniffed from the URL or a failed request, per the issue's acceptance criteria.Test plan
./gradlew :onebusaway-android:compileObaGoogleDebugKotlin -PwarningsAsErrors=true(and unit-test/androidTest source sets) — clean under the strict CI gate./gradlew :onebusaway-android:testObaGoogleDebugUnitTest— 826 tests, 0 failures, includingOtp2PlanDecodeTest,Otp2PlanRequestBuilderTest(incl. the transfer-cost/wheelchair preference wiring), and aRegionsDecodeTestguard that every bundled region still defaults to OTP1AppDatabaseMigrationTest.migrate5To6_addsUsesOtp2GraphQlColumn_defaultingExistingRowsToFalse— Room migration verified against the exported6.jsonschemaDefaultTripPlanRepositorydirectly with the custom-URL + GraphQL toggle set exactly as a user would configure them — returned a correct WALK → TRAM (2 Line) → WALK itinerary through both theplan()andplanBlocking()(trip-monitor) paths; also verified thetransit.transfer.costpreference is accepted by the live server via a direct curl request🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes