From ffba43441ecd7b27e1ffb50ef3029f0cd2b1e48b Mon Sep 17 00:00:00 2001 From: Brandon Martin-Anderson Date: Sat, 11 Jul 2026 17:39:35 -0700 Subject: [PATCH] Add an OTP2 GraphQL protocol field and point Puget Sound at it (#1780) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- .../6.json | 9 ++- .../database/AppDatabaseMigrationTest.kt | 9 +-- .../onebusaway/android/mock/MockRegion.java | 16 ++--- .../android/api/adapters/RegionAdapters.kt | 2 +- .../android/api/contract/ObaApiModels.kt | 9 ++- .../android/database/AppDatabase.kt | 5 +- .../onebusaway/android/database/Migrations.kt | 7 ++- .../android/database/oba/ObaEntities.kt | 2 +- .../directions/util/Otp2PlanRequestBuilder.kt | 12 +++- .../directions/util/TripRequestBuilder.kt | 63 +++++++++++-------- .../org/onebusaway/android/region/Region.kt | 10 +-- .../onebusaway/android/region/RegionMapper.kt | 4 +- .../ui/settings/AdvancedSettingsScreen.kt | 6 +- .../ui/settings/AdvancedSettingsViewModel.kt | 6 +- .../android/ui/settings/SettingsUiState.kt | 5 +- .../android/ui/tripplan/Otp2Planner.kt | 28 ++++++--- .../onebusaway/android/util/RegionUtils.java | 2 +- .../src/main/res/raw/regions_v3.json | 1 + .../android/api/RegionsDecodeTest.kt | 19 ++++-- .../util/Otp2PlanRequestBuilderTest.kt | 8 ++- .../android/region/RegionTestFixtures.kt | 2 +- .../ui/tripplan/Otp2GraphQlEndpointTest.kt | 45 +++++++++++++ 22 files changed, 181 insertions(+), 89 deletions(-) create mode 100644 onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2GraphQlEndpointTest.kt diff --git a/onebusaway-android/schemas/org.onebusaway.android.database.AppDatabase/6.json b/onebusaway-android/schemas/org.onebusaway.android.database.AppDatabase/6.json index 3ee72abba0..0647c803ef 100644 --- a/onebusaway-android/schemas/org.onebusaway.android.database.AppDatabase/6.json +++ b/onebusaway-android/schemas/org.onebusaway.android.database.AppDatabase/6.json @@ -437,7 +437,7 @@ }, { "tableName": "regions", - "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`_id` INTEGER NOT NULL, `name` TEXT NOT NULL, `oba_base_url` TEXT NOT NULL, `siri_base_url` TEXT NOT NULL, `lang` TEXT NOT NULL, `contact_email` TEXT NOT NULL, `supports_api_discovery` INTEGER NOT NULL, `supports_api_realtime` INTEGER NOT NULL, `supports_siri_realtime` INTEGER NOT NULL, `twitter_url` TEXT, `experimental` INTEGER, `stop_info_url` TEXT, `otp_base_url` TEXT, `otp_contact_email` TEXT, `supports_otp_bikeshare` INTEGER, `uses_otp2_graphql` INTEGER DEFAULT 0, `supports_embedded_social` INTEGER, `payment_android_app_id` TEXT, `payment_warning_title` TEXT, `payment_warning_body` TEXT, `sidecar_base_url` TEXT DEFAULT 'https://onebusaway.co', `plausible_analytics_server_url` TEXT, `umami_analytics_url` TEXT, `umami_analytics_id` TEXT, PRIMARY KEY(`_id`))", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`_id` INTEGER NOT NULL, `name` TEXT NOT NULL, `oba_base_url` TEXT NOT NULL, `siri_base_url` TEXT NOT NULL, `lang` TEXT NOT NULL, `contact_email` TEXT NOT NULL, `supports_api_discovery` INTEGER NOT NULL, `supports_api_realtime` INTEGER NOT NULL, `supports_siri_realtime` INTEGER NOT NULL, `twitter_url` TEXT, `experimental` INTEGER, `stop_info_url` TEXT, `otp_base_url` TEXT, `otp_contact_email` TEXT, `supports_otp_bikeshare` INTEGER, `otp_base_graphql_url` TEXT, `supports_embedded_social` INTEGER, `payment_android_app_id` TEXT, `payment_warning_title` TEXT, `payment_warning_body` TEXT, `sidecar_base_url` TEXT DEFAULT 'https://onebusaway.co', `plausible_analytics_server_url` TEXT, `umami_analytics_url` TEXT, `umami_analytics_id` TEXT, PRIMARY KEY(`_id`))", "fields": [ { "fieldPath": "id", @@ -524,10 +524,9 @@ "affinity": "INTEGER" }, { - "fieldPath": "usesOtp2GraphQl", - "columnName": "uses_otp2_graphql", - "affinity": "INTEGER", - "defaultValue": "0" + "fieldPath": "otpBaseGraphqlUrl", + "columnName": "otp_base_graphql_url", + "affinity": "TEXT" }, { "fieldPath": "supportsEmbeddedSocial", diff --git a/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/AppDatabaseMigrationTest.kt b/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/AppDatabaseMigrationTest.kt index 9a2d9a9f5f..debba9b6ae 100644 --- a/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/AppDatabaseMigrationTest.kt +++ b/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/AppDatabaseMigrationTest.kt @@ -19,6 +19,7 @@ import androidx.room.testing.MigrationTestHelper import androidx.test.platform.app.InstrumentationRegistry import androidx.test.ext.junit.runners.AndroidJUnit4 import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -29,7 +30,7 @@ import org.junit.runner.RunWith * empty — the data import from the legacy ContentProvider DB is a separate slice); and that * [MIGRATION_3_4] reconciles `routes.favorite` from the authoritative `route_headsign_favorites` table * before dropping it (#1751) and adds the `surveys.study_id` foreign-key child index (#1739); and that - * [MIGRATION_5_6] adds `regions.uses_otp2_graphql` defaulting existing rows to OTP1 (#1780). + * [MIGRATION_5_6] adds `regions.otp_base_graphql_url` defaulting existing rows to OTP1 (#1780). */ @RunWith(AndroidJUnit4::class) class AppDatabaseMigrationTest { @@ -143,7 +144,7 @@ class AppDatabaseMigrationTest { } @Test - fun migrate5To6_addsUsesOtp2GraphQlColumn_defaultingExistingRowsToFalse() { + fun migrate5To6_addsOtpBaseGraphqlUrlColumn_defaultingExistingRowsToNull() { helper.createDatabase(TEST_DB, 5).use { db -> db.execSQL( "INSERT INTO regions (_id, name, oba_base_url, siri_base_url, lang, contact_email, " + @@ -155,9 +156,9 @@ class AppDatabaseMigrationTest { // runMigrationsAndValidate asserts the resulting schema matches the exported 6.json. val db = helper.runMigrationsAndValidate(TEST_DB, 6, true, MIGRATION_5_6) - db.query("SELECT uses_otp2_graphql FROM regions WHERE _id = 1").use { c -> + db.query("SELECT otp_base_graphql_url FROM regions WHERE _id = 1").use { c -> c.moveToFirst() - assertEquals("pre-existing region should default to OTP1 (0)", 0, c.getInt(0)) + assertTrue("pre-existing region should default to OTP1 (NULL GraphQL URL)", c.isNull(0)) } db.close() } diff --git a/onebusaway-android/src/androidTest/java/org/onebusaway/android/mock/MockRegion.java b/onebusaway-android/src/androidTest/java/org/onebusaway/android/mock/MockRegion.java index 2ab968ee97..5fa614fb5b 100644 --- a/onebusaway-android/src/androidTest/java/org/onebusaway/android/mock/MockRegion.java +++ b/onebusaway-android/src/androidTest/java/org/onebusaway/android/mock/MockRegion.java @@ -89,7 +89,7 @@ public static Region getRegionWithPathNoSeparator(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -131,7 +131,7 @@ public static Region getRegionNoSeparator(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, null, null, @@ -173,7 +173,7 @@ public static Region getRegionWithPort(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -215,7 +215,7 @@ public static Region getRegionNoScheme(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -257,7 +257,7 @@ public static Region getRegionWithHttps() { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -299,7 +299,7 @@ public static Region getRegionWithHttpsAndPort() { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -339,7 +339,7 @@ public static Region getRegionWithoutObaApis(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, @@ -379,7 +379,7 @@ public static Region getInactiveRegion(Context context) { null, null, false, - false, + null, // otpBaseGraphqlUrl (#1780): OTP1 REST region false, "co.bytemark.hart", null, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/adapters/RegionAdapters.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/adapters/RegionAdapters.kt index 97d37757ff..e76467364d 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/api/adapters/RegionAdapters.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/api/adapters/RegionAdapters.kt @@ -46,7 +46,7 @@ fun RegionDto.toObaRegion(): Region = Region( otpBaseUrl, otpContactEmail, supportsOtpBikeshare, - usesOtp2GraphQl, + otpBaseGraphqlUrl, supportsEmbeddedSocial, paymentAndroidAppId, paymentWarningTitle, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.kt index 396f0f849f..5aefcfa8ed 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.kt @@ -458,9 +458,12 @@ data class RegionDto( val otpBaseUrl: String? = null, val otpContactEmail: String? = null, val supportsOtpBikeshare: Boolean = false, - // OTP protocol selection (#1780); absent in the regions directory today, so every region - // decodes to OTP1 REST (false) until explicitly set otherwise. - val usesOtp2GraphQl: Boolean = false, + // OTP 2.x GraphQL endpoint (#1780). Its presence is the explicit protocol signal: a non-blank + // value routes trip planning through the OTP2 `planConnection` path against this URL; a null/ + // absent value (every region but the OTP2-enabled ones) stays on the OTP1 REST `/plan` path. + // A distinct field because a region's GraphQL endpoint is a different host from its OTP1 + // `otpBaseUrl` (e.g. Puget Sound), so the two URLs can't share one field. + val otpBaseGraphqlUrl: String? = null, val supportsEmbeddedSocial: Boolean = false, val paymentAndroidAppId: String? = null, val paymentWarningTitle: String? = null, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/database/AppDatabase.kt b/onebusaway-android/src/main/java/org/onebusaway/android/database/AppDatabase.kt index b9ef0114e1..3af0113d80 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/database/AppDatabase.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/database/AppDatabase.kt @@ -46,8 +46,9 @@ import org.onebusaway.android.database.widealerts.entity.AlertEntity * cache of nearby-stops loads (separate from the user-state `stops` table) so the map renders stops * instantly on a slow/cold-start load. * - * v6 adds `regions.uses_otp2_graphql` (#1780): explicit per-region OTP protocol selection (OTP1 REST - * vs. OTP 2.x GraphQL). Defaults to 0 (OTP1) for every existing cached row. + * v6 adds `regions.otp_base_graphql_url` (#1780): the per-region OTP 2.x GraphQL endpoint. A non-null + * value routes that region through the OTP2 `planConnection` path; NULL (every existing cached row) + * stays on OTP1 REST. */ @Database( entities = [ diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/database/Migrations.kt b/onebusaway-android/src/main/java/org/onebusaway/android/database/Migrations.kt index d5149de9fd..af7d62bf88 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/database/Migrations.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/database/Migrations.kt @@ -158,13 +158,14 @@ val MIGRATION_4_5 = object : Migration(4, 5) { } /** - * Adds `regions.uses_otp2_graphql` (#1780): explicit per-region OTP protocol selection. Defaults - * every existing row to 0 (OTP1 REST) — purely additive, no other v5 table/column changes. + * Adds `regions.otp_base_graphql_url` (#1780): the per-region OTP 2.x GraphQL endpoint. A non-null + * value routes that region's trip planning through the OTP2 `planConnection` path; NULL (every + * existing row) stays on OTP1 REST. Purely additive, no other v5 table/column changes. */ val MIGRATION_5_6 = object : Migration(5, 6) { override fun migrate(db: SupportSQLiteDatabase) { db.execSQL( - "ALTER TABLE `regions` ADD COLUMN `uses_otp2_graphql` INTEGER DEFAULT 0" + "ALTER TABLE `regions` ADD COLUMN `otp_base_graphql_url` TEXT" ) } } diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/database/oba/ObaEntities.kt b/onebusaway-android/src/main/java/org/onebusaway/android/database/oba/ObaEntities.kt index 01ce672858..f303f0acc8 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/database/oba/ObaEntities.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/database/oba/ObaEntities.kt @@ -135,7 +135,7 @@ data class RegionRecord( @ColumnInfo(name = "otp_base_url") val otpBaseUrl: String? = null, @ColumnInfo(name = "otp_contact_email") val otpContactEmail: String? = null, @ColumnInfo(name = "supports_otp_bikeshare") val supportsOtpBikeshare: Int? = null, - @ColumnInfo(name = "uses_otp2_graphql", defaultValue = "0") val usesOtp2GraphQl: Int? = null, + @ColumnInfo(name = "otp_base_graphql_url") val otpBaseGraphqlUrl: String? = null, @ColumnInfo(name = "supports_embedded_social") val supportsEmbeddedSocial: Int? = null, @ColumnInfo(name = "payment_android_app_id") val paymentAndroidAppId: String? = null, @ColumnInfo(name = "payment_warning_title") val paymentWarningTitle: String? = null, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilder.kt b/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilder.kt index b4d4641f5b..9d1c9fdf88 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilder.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilder.kt @@ -157,8 +157,13 @@ object Otp2PlanRequestBuilder { PlanModesInput( transit = Optional.present( PlanTransitModesInput( - access = Optional.present(listOf(PlanAccessMode.BICYCLE_RENTAL)), - egress = Optional.present(listOf(PlanEgressMode.BICYCLE_RENTAL)), + // WALK must accompany BICYCLE_RENTAL in the same access/egress list — + // OTP2 rejects a bare BICYCLE_RENTAL leg ("BIKE_RENTAL needs to be + // combined with WALK mode for the same leg", BadRequestError), since a + // rental trip always walks to/from the vehicle. Verified against the + // live OTP 2.x server. #1780. + access = Optional.present(listOf(PlanAccessMode.WALK, PlanAccessMode.BICYCLE_RENTAL)), + egress = Optional.present(listOf(PlanEgressMode.WALK, PlanEgressMode.BICYCLE_RENTAL)), ) ) ) @@ -167,9 +172,10 @@ object Otp2PlanRequestBuilder { Optional.Absent } + // WALK must accompany BICYCLE_RENTAL here too (see the TRANSIT_AND_BIKE branch above). TripModes.BIKESHARE -> Optional.present( PlanModesInput( - direct = Optional.present(listOf(PlanDirectMode.BICYCLE_RENTAL)), + direct = Optional.present(listOf(PlanDirectMode.WALK, PlanDirectMode.BICYCLE_RENTAL)), directOnly = Optional.present(true), ) ) diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/TripRequestBuilder.kt b/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/TripRequestBuilder.kt index 06c0b0049a..b346375d56 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/TripRequestBuilder.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/directions/util/TripRequestBuilder.kt @@ -206,31 +206,51 @@ class TripRequestBuilder(context: Context, private val mBundle: Bundle) { } /** - * The user's custom OTP API URL preference, or null if unset/blank — the shared "is a custom - * server configured" signal both [formattedOtpBaseUrl] and [usesOtp2] branch on. + * The user's custom OTP API URL preference, or null if unset/blank — the "is a custom server + * configured" signal [otpTarget] branches on. */ private val customOtpApiUrl: String? get() = PreferencesEntryPoint.get(mContext) .getString(mContext.getString(R.string.preference_key_otp_api_url), null as String?) ?.takeUnless { TextUtils.isEmpty(it) } + /** The OTP server a request targets: its [baseUrl] and whether it speaks OTP 2.x GraphQL. */ + private data class OtpTarget(val baseUrl: String?, val usesOtp2: Boolean) + /** - * Resolves and formats the OTP base URL (the user's custom URL if set, otherwise the current - * region's), or null if neither is available. + * Resolves the custom-URL-or-region branch once so [formattedOtpBaseUrl] and [usesOtp2] can't + * disagree (#1780). Protocol selection is explicit — a custom server's manual `..._is_graphql` + * preference, or a region publishing an `otpBaseGraphqlUrl` — never sniffed from the URL shape + * or a failed request. [baseUrl] is null when neither a custom URL nor a region is available. */ - val formattedOtpBaseUrl: String? + private val otpTarget: OtpTarget get() { - var otpBaseUrl: String? val customUrl = customOtpApiUrl if (customUrl != null) { - otpBaseUrl = customUrl - Log.d(TAG, "Using custom OTP API URL set by user '$otpBaseUrl'.") - } else { - // No custom URL and no selected region: return null so the caller - // (TripPlanRepository) surfaces a "no server selected" error instead of crashing. - val region = RegionEntryPoint.get(mContext).currentRegion() ?: return null - otpBaseUrl = region.otpBaseUrl + Log.d(TAG, "Using custom OTP API URL set by user '$customUrl'.") + // No [Region] to carry the setting for a custom server, so the user sets it. + return OtpTarget( + baseUrl = customUrl, + usesOtp2 = PreferencesEntryPoint.get(mContext) + .getBoolean(R.string.preference_key_otp_api_url_is_graphql, false), + ) } + // No custom URL and no selected region: baseUrl stays null so the caller + // (TripPlanRepository) surfaces a "no server selected" error instead of crashing. + val region = RegionEntryPoint.get(mContext).currentRegion() ?: return OtpTarget(null, false) + // An OTP2 region publishes its GraphQL endpoint separately (a different host than the + // OTP1 REST server); route to it when present, else the OTP1 REST base URL. + val graphqlBase = region.otpBaseGraphqlUrl?.takeUnless { it.isBlank() } + return OtpTarget(baseUrl = graphqlBase ?: region.otpBaseUrl, usesOtp2 = graphqlBase != null) + } + + /** + * The [otpTarget] base URL with a scheme ensured and formatted, or null if no server is + * available. + */ + val formattedOtpBaseUrl: String? + get() { + var otpBaseUrl = otpTarget.baseUrl ?: return null try { // URI.parse() doesn't tell us if the scheme is missing, so use URL() instead (#126) URL(otpBaseUrl) @@ -238,23 +258,12 @@ class TripRequestBuilder(context: Context, private val mBundle: Bundle) { // Assume HTTPS scheme, since without a scheme the Uri won't parse the authority otpBaseUrl = mContext.getString(R.string.https_prefix) + otpBaseUrl } - return if (otpBaseUrl != null) RegionUtils.formatOtpBaseUrl(otpBaseUrl) else null + return RegionUtils.formatOtpBaseUrl(otpBaseUrl) } - /** - * Whether this request should go through the OTP 2.x GraphQL path rather than OTP1 REST - * (#1780). Resolved the same way as [formattedOtpBaseUrl] (custom-URL-or-region): with a - * custom OTP URL set, the manual override preference applies (there's no [Region] to carry - * the flag); otherwise the current region's `usesOtp2GraphQl` flag applies. Explicit, never - * sniffed from the URL shape or a failed request. - */ + /** Whether this request goes through the OTP 2.x GraphQL path rather than OTP1 REST (#1780). */ val usesOtp2: Boolean - get() = if (customOtpApiUrl != null) { - PreferencesEntryPoint.get(mContext) - .getBoolean(R.string.preference_key_otp_api_url_is_graphql, false) - } else { - RegionEntryPoint.get(mContext).currentRegion()?.usesOtp2GraphQl ?: false - } + get() = otpTarget.usesOtp2 private fun getAddressString(address: CustomAddress?): String? { if (address == null) { diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/region/Region.kt b/onebusaway-android/src/main/java/org/onebusaway/android/region/Region.kt index 2ef38369e2..d0b8926e62 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/region/Region.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/region/Region.kt @@ -44,10 +44,12 @@ data class Region( val otpBaseUrl: String? = "", val otpContactEmail: String? = "", val supportsOtpBikeshare: Boolean = false, - // Explicit protocol selection (#1780): false selects the OTP1 REST `/plan` path (the default, - // and the only path any bundled region uses today), true selects the OTP 2.x GraphQL - // `planConnection` path. Never sniffed/inferred from the URL or a failed request. - val usesOtp2GraphQl: Boolean = false, + // OTP 2.x GraphQL endpoint (#1780). A non-blank value is the explicit protocol signal — trip + // planning routes through the OTP2 `planConnection` path against this URL; null/blank (the + // default, and every region but the OTP2-enabled ones) stays on OTP1 REST `/plan`. Distinct + // from [otpBaseUrl] because the GraphQL endpoint is a different host than the OTP1 REST server. + // Never sniffed/inferred from the URL shape or a failed request. + val otpBaseGraphqlUrl: String? = null, val supportsEmbeddedSocial: Boolean = false, val paymentAndroidAppId: String? = null, val paymentWarningTitle: String? = null, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/region/RegionMapper.kt b/onebusaway-android/src/main/java/org/onebusaway/android/region/RegionMapper.kt index ffab4895a0..00e8098c1f 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/region/RegionMapper.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/region/RegionMapper.kt @@ -59,7 +59,7 @@ object RegionMapper { otpBaseUrl = r.otpBaseUrl, otpContactEmail = r.otpContactEmail, supportsOtpBikeshare = (r.supportsOtpBikeshare ?: 0) > 0, - usesOtp2GraphQl = (r.usesOtp2GraphQl ?: 0) > 0, + otpBaseGraphqlUrl = r.otpBaseGraphqlUrl, supportsEmbeddedSocial = (r.supportsEmbeddedSocial ?: 0) > 0, paymentAndroidAppId = r.paymentAndroidAppId, paymentWarningTitle = r.paymentWarningTitle, @@ -88,7 +88,7 @@ object RegionMapper { otpBaseUrl = region.otpBaseUrl, otpContactEmail = region.otpContactEmail, supportsOtpBikeshare = region.supportsOtpBikeshare.toInt(), - usesOtp2GraphQl = region.usesOtp2GraphQl.toInt(), + otpBaseGraphqlUrl = region.otpBaseGraphqlUrl, supportsEmbeddedSocial = region.supportsEmbeddedSocial.toInt(), paymentAndroidAppId = region.paymentAndroidAppId, paymentWarningTitle = region.paymentWarningTitle, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt index eeb6d04aaa..e134cd1730 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt @@ -212,9 +212,9 @@ fun AdvancedSettingsScreen( onValueChange = onOtpUrlChange, ) // Only meaningful once a custom OTP URL is set — TripRequestBuilder.usesOtp2 only - // reads this preference in that case, falling back to the region's own flag - // otherwise, so showing it unconditionally would suggest it does something it - // doesn't. + // reads this preference in that case, falling back to the region's own + // otpBaseGraphqlUrl otherwise, so showing it unconditionally would suggest it does + // something it doesn't. if (!state.customOtpApiUrl.isNullOrEmpty()) { SwitchPreferenceItem( title = stringResource(R.string.preferences_otp_api_url_is_graphql_title), diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt index 27a1f79adc..c243ead776 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt @@ -163,9 +163,9 @@ class AdvancedSettingsViewModel @Inject constructor( } /** - * Manual OTP protocol override for the custom URL (#1780) — there's no [Region] to carry - * [Region.usesOtp2GraphQl] when a custom OTP server is in use, so this is set explicitly by the - * user rather than inferred. + * Manual OTP protocol override for the custom URL (#1780) — there's no [Region] to carry an + * [Region.otpBaseGraphqlUrl] when a custom OTP server is in use, so this is set explicitly by + * the user rather than inferred. */ fun onCustomOtpApiUrlUsesGraphQlChanged(usesGraphQl: Boolean) = prefs.setBoolean(R.string.preference_key_otp_api_url_is_graphql, usesGraphQl) diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt index 6f2683de72..9263a9a9f9 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt @@ -137,8 +137,9 @@ data class AdvancedPrefSnapshot( val displayTestAlerts: Boolean, val customObaApiUrl: String?, val customOtpApiUrl: String?, - // Manual OTP protocol override for the custom URL above (#1780) — mirrors Region.usesOtp2GraphQl - // for the no-region / custom-server case, where there's no Region to carry the flag. + // Manual OTP protocol override for the custom URL above (#1780) — the custom-server counterpart + // to a region's Region.otpBaseGraphqlUrl, for the no-region case where there's no Region to + // carry the setting. val customOtpApiUrlUsesGraphQl: Boolean, val mapStopCacheSize: Int, ) diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt index 6c1e61cec7..e4e3ead4fc 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt @@ -53,8 +53,8 @@ class Otp2Planner @Inject constructor( ) { /** - * The last [ApolloClient] built, keyed by the base URL it targets. `TripPlanMonitorService` - * calls [plan] repeatedly (every 60s) against the *same* base URL for the life of one monitored + * The last [ApolloClient] built, keyed by the endpoint URL it targets. `TripPlanMonitorService` + * calls [plan] repeatedly (every 60s) against the *same* endpoint for the life of one monitored * trip, and only one URL is realistically in play per [Otp2Planner] instance — a single cached * slot avoids rebuilding the client (its own coroutine scope + interceptor chain) on every tick, * without needing a general-purpose cache. @@ -62,15 +62,15 @@ class Otp2Planner @Inject constructor( @Volatile private var cachedClient: Pair? = null - private fun apolloClientFor(baseUrl: String): ApolloClient = synchronized(this) { - cachedClient?.takeIf { it.first == baseUrl }?.second - ?: ApolloClient.Builder().serverUrl(baseUrl).okHttpClient(okHttpClient).build() + private fun apolloClientFor(endpointUrl: String): ApolloClient = synchronized(this) { + cachedClient?.takeIf { it.first == endpointUrl }?.second + ?: ApolloClient.Builder().serverUrl(endpointUrl).okHttpClient(okHttpClient).build() .also { // Close the client this one replaces (e.g. a region/custom-URL switch) — // ApolloClient owns a coroutine scope that otherwise leaks until this // Otp2Planner itself is garbage collected. cachedClient?.second?.close() - cachedClient = baseUrl to it + cachedClient = endpointUrl to it } } @@ -81,7 +81,7 @@ class Otp2Planner @Inject constructor( */ fun plan(builder: TripRequestBuilder, baseUrl: String): List { val query = Otp2PlanRequestBuilder.build(builder, context) - val apolloClient = apolloClientFor(baseUrl) + val apolloClient = apolloClientFor(otp2GraphQlEndpoint(baseUrl)) val data = try { runBlocking { apolloClient.query(query).execute() }.dataOrThrow() } catch (e: ApolloNetworkException) { @@ -137,3 +137,17 @@ class Otp2Planner @Inject constructor( RoutingErrorCode.UNKNOWN__ -> context.getString(R.string.tripplanner_error_not_defined) } } + +/** OTP2's standard gtfs GraphQL mount, relative to the OTP base (`…/otp`). */ +private const val OTP2_GTFS_GRAPHQL_PATH = "/gtfs/v1" + +/** + * Resolves the OTP2 gtfs GraphQL endpoint from a region/custom OTP base URL. `otpBaseGraphqlUrl` + * (and the custom-URL setting) is the OTP mount base — e.g. `https://…/prod/otp` — mirroring the + * OTP1 REST path where the base is `…/otp/routers/default` and the client appends `/plan`; here the + * fixed gtfs GraphQL mount `/gtfs/v1` is appended. Not a heuristic: `/gtfs/v1` is OTP2's standard + * gtfs API path, identical across OTP2 servers (verified against the live endpoint). Trailing slash + * on the base is tolerated so `…/otp` and `…/otp/` both resolve to `…/otp/gtfs/v1`. + */ +internal fun otp2GraphQlEndpoint(otpBaseUrl: String): String = + otpBaseUrl.trimEnd('/') + OTP2_GTFS_GRAPHQL_PATH diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/util/RegionUtils.java b/onebusaway-android/src/main/java/org/onebusaway/android/util/RegionUtils.java index 4fc0c918c2..1510c55c9c 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/util/RegionUtils.java +++ b/onebusaway-android/src/main/java/org/onebusaway/android/util/RegionUtils.java @@ -373,7 +373,7 @@ public static Region getRegionFromBuildFlavor() { BuildConfig.FIXED_REGION_OTP_BASE_URL, BuildConfig.FIXED_REGION_OTP_CONTACT_EMAIL, BuildConfig.FIXED_REGION_SUPPORTS_OTP_BIKESHARE, - false, // usesOtp2GraphQl: the fixed-region build flavor doesn't support OTP2 yet + null, // otpBaseGraphqlUrl: the fixed-region build flavor doesn't support OTP2 yet false, BuildConfig.FIXED_REGION_PAYMENT_ANDROID_APP_ID, BuildConfig.FIXED_REGION_PAYMENT_WARNING_TITLE, diff --git a/onebusaway-android/src/main/res/raw/regions_v3.json b/onebusaway-android/src/main/res/raw/regions_v3.json index 817940b42d..129902e565 100644 --- a/onebusaway-android/src/main/res/raw/regions_v3.json +++ b/onebusaway-android/src/main/res/raw/regions_v3.json @@ -129,6 +129,7 @@ "stopInfoUrl": null, "open311Servers": [], "otpBaseUrl": "https://otp.prod.sound.obaweb.org/otp/routers/default/", + "otpBaseGraphqlUrl": "https://peq6qe6fei.execute-api.us-west-2.amazonaws.com/prod/otp", "otpContactEmail": "otp-pugetsound@onebusaway.org", "supportsEmbeddedSocial": false, "supportsOtpBikeshare": false, diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/api/RegionsDecodeTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/api/RegionsDecodeTest.kt index 3a9d3b8063..144c3a11c0 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/api/RegionsDecodeTest.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/api/RegionsDecodeTest.kt @@ -56,14 +56,21 @@ class RegionsDecodeTest { } /** - * #1780 is infra-only: no bundled region is flipped to OTP2 GraphQL by this change, so every - * region in the shipped directory must still default to OTP1 (the field is absent from the - * JSON entirely today). Guards against a future edit accidentally flipping one. + * OTP2 protocol selection (#1780) is carried by a region's `otpBaseGraphqlUrl` (the OTP mount + * base; the client appends `/gtfs/v1`). Puget Sound is pointed at its OTP2 GraphQL server; + * every other bundled region stays on OTP1 REST (null). Guards both the intended flip and + * against an accidental one elsewhere. */ @Test - fun bundledRegionsDefaultToOtp1() { - val regions = decode("src/main/res/raw/regions_v3.json") - regions.forEach { assertFalse(it.toObaRegion().usesOtp2GraphQl) } + fun bundledRegionsSelectOtpProtocolByGraphqlUrl() { + val regions = decode("src/main/res/raw/regions_v3.json").map { it.toObaRegion() } + val pugetSound = regions.single { it.name == "Puget Sound" } + assertEquals( + "https://peq6qe6fei.execute-api.us-west-2.amazonaws.com/prod/otp", + pugetSound.otpBaseGraphqlUrl, + ) + regions.filter { it.name != "Puget Sound" } + .forEach { assertNull(it.otpBaseGraphqlUrl) } } /** Ports testUmamiAnalyticsParsing: the nested umamiAnalytics object maps onto the region. */ diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilderTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilderTest.kt index 2cfa7f9bdd..b614118245 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilderTest.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/directions/util/Otp2PlanRequestBuilderTest.kt @@ -62,8 +62,9 @@ class Otp2PlanRequestBuilderTest { Otp2PlanRequestBuilder.buildModes(TripModes.TRANSIT_AND_BIKE, bikeshareEnabled = true) ) val transit = requirePresent(modes.transit) - assertEquals(listOf(PlanAccessMode.BICYCLE_RENTAL), requirePresent(transit.access)) - assertEquals(listOf(PlanEgressMode.BICYCLE_RENTAL), requirePresent(transit.egress)) + // WALK must accompany BICYCLE_RENTAL — OTP2 rejects a bare BICYCLE_RENTAL leg (#1780). + assertEquals(listOf(PlanAccessMode.WALK, PlanAccessMode.BICYCLE_RENTAL), requirePresent(transit.access)) + assertEquals(listOf(PlanEgressMode.WALK, PlanEgressMode.BICYCLE_RENTAL), requirePresent(transit.egress)) } @Test @@ -78,7 +79,8 @@ class Otp2PlanRequestBuilderTest { @Test fun bikeshareRequestsDirectBicycleRentalOnly() { val modes = requirePresent(Otp2PlanRequestBuilder.buildModes(TripModes.BIKESHARE, bikeshareEnabled = true)) - assertEquals(listOf(PlanDirectMode.BICYCLE_RENTAL), requirePresent(modes.direct)) + // WALK must accompany BICYCLE_RENTAL — OTP2 rejects a bare BICYCLE_RENTAL leg (#1780). + assertEquals(listOf(PlanDirectMode.WALK, PlanDirectMode.BICYCLE_RENTAL), requirePresent(modes.direct)) assertEquals(true, requirePresent(modes.directOnly)) } diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/region/RegionTestFixtures.kt b/onebusaway-android/src/test/java/org/onebusaway/android/region/RegionTestFixtures.kt index ecc5262cc2..86f871ff1b 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/region/RegionTestFixtures.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/region/RegionTestFixtures.kt @@ -78,7 +78,7 @@ internal fun region( null, // otpBaseUrl otpContactEmail, // otpContactEmail supportsOtpBikeshare, // supportsOtpBikeshare - false, // usesOtp2GraphQl + null, // otpBaseGraphqlUrl false, // supportsEmbeddedSocial null, // paymentAndroidAppId null, // paymentWarningTitle diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2GraphQlEndpointTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2GraphQlEndpointTest.kt new file mode 100644 index 0000000000..31683d97f3 --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2GraphQlEndpointTest.kt @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.ui.tripplan + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The OTP2 base-URL → gtfs GraphQL endpoint resolution (#1780). A region's `otpBaseGraphqlUrl` + * (and the custom-URL setting) is the OTP mount base through `…/otp`; the client appends the fixed + * `/gtfs/v1` gtfs GraphQL mount, mirroring how the OTP1 path appends `/plan` to its own base. + */ +class Otp2GraphQlEndpointTest { + + @Test + fun appendsGtfsV1ToOtpBase() { + // The Puget Sound OTP2 server this wires up (#1780) — base verified to serve the OTP root, + // base + /gtfs/v1 verified to serve the GraphQL endpoint. + assertEquals( + "https://peq6qe6fei.execute-api.us-west-2.amazonaws.com/prod/otp/gtfs/v1", + otp2GraphQlEndpoint("https://peq6qe6fei.execute-api.us-west-2.amazonaws.com/prod/otp"), + ) + } + + @Test + fun toleratesTrailingSlashWithoutDoublingIt() { + assertEquals( + "https://example.opentripplanner.org/otp/gtfs/v1", + otp2GraphQlEndpoint("https://example.opentripplanner.org/otp/"), + ) + } +}