Retire HttpURLConnection for OTP /plan: move to Retrofit OtpWebService (#1625)#1653
Conversation
OneBusAway#1625) The trip-planner's OTP /plan call was the last hand-rolled java.net.HttpURLConnection in the directions/trip-planner subsystem (the OneBusAway#1625 island). Move it onto the Retrofit/OkHttp stack every other endpoint uses: - Add OtpWebService (@get fun plan(@url url): Call<ResponseBody>) — a synchronous Call so the Java RealtimeService worker's blocking planBlocking() path needs no runBlocking, and the body still flows through the shared OtpPlanParser (keeping the OTP-library Response mapping + malformed-body handling in one place and letting the repo detect the old-server 404 to fall back to the legacy URL structure). - Provide it in NetworkModule like the other @url sidecar services (no ApiParamsInterceptor), but on a client with the legacy 15s connect/read timeouts OTP has always used. Extracted plainClient() so plainRetrofit() can take a configured client. - DefaultTripPlanRepository.requestPlan now calls otpWebService.plan(url).execute(): IOException/timeout -> the same request-timeout message, HTTP 404 -> the same old-URL-structure retry + preference flip, success -> OtpPlanParser as before. Behavior-preserving: URL construction, parsing, fallback, and error mapping are unchanged; only the transport moves off HttpURLConnection. Part of OneBusAway#1625 (AsyncTask + Jackson already done); the directions/ Kotlin port and java.time migration remain. Note: a separate hand-rolled HttpURLConnection remains in analytics/UmamiAnalytics.java, outside OneBusAway#1625's scope. Remaining HttpURLConnection.HTTP_* references elsewhere are status-code constants, not networking.
📝 WalkthroughWalkthroughThis PR migrates OTP trip planning networking from raw HttpURLConnection to Retrofit. It adds an OtpWebService interface, configures its OkHttp client/timeouts in NetworkModule with new helper functions, and updates DefaultTripPlanRepository to use the injected service for request/response/error handling and retries. ChangesOTP Retrofit Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Repository as DefaultTripPlanRepository
participant OtpService as OtpWebService
participant OTP as OTP Server
participant Parser as OtpPlanParser
Repository->>OtpService: plan(url)
OtpService->>OTP: GET url
OTP-->>OtpService: HTTP response
OtpService-->>Repository: Response<ResponseBody>
alt response.isSuccessful and body present
Repository->>Parser: parse(body.byteStream())
Parser-->>Repository: parsed trip plan
else 404 on new URL structure
Repository->>OtpService: plan(url) [old URL structure]
else IOException
Repository-->>Repository: map to request-timeout message
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/api/contract/OtpWebService.kt (1)
37-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
@Streamingto avoid double-buffering the trip-plan response.Without
@Streaming, Retrofit's built-in converter for a rawResponseBodyreturn type eagerly reads the entire response into memory (BufferingResponseBodyConverter) before returning it — the body is materialized once by Retrofit and then read again viabyteStream()inOtpPlanParser.parse. For potentially large multi-itinerary OTP responses over slow OTP servers, this doubles memory pressure and adds latency versus parsing directly off the wire. SinceplainClientonly logs atLevel.BASIC(notBODY), adding@Streaminghere would not be defeated by the debug logging interceptor.⚡ Proposed fix
+import retrofit2.http.Streaming + interface OtpWebService { + `@Streaming` `@GET` fun plan(`@Url` url: String): Call<ResponseBody> }🤖 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/api/contract/OtpWebService.kt` around lines 37 - 41, Add `@Streaming` to `OtpWebService.plan` so Retrofit does not eagerly buffer the `ResponseBody` before `OtpPlanParser.parse` reads it via `byteStream()`. Update the `plan(`@Url` url: String): Call<ResponseBody>` declaration in `OtpWebService` to stream the OTP response directly off the wire and keep `plainClient` behavior unchanged.
🤖 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/tripplan/TripPlanRepository.kt`:
- Around line 150-156: The TripPlanRepository plan call only maps IOException
failures to the REQUEST_TIMEOUT message, so invalid `@Url` cases from
otpWebService.plan(url).execute() can escape as IllegalArgumentException. Update
the try/catch in TripPlanRepository to also catch IllegalArgumentException
around the execute() call and wrap it in the same
IOException(errorMessage(Message.REQUEST_TIMEOUT.id), e) path used for transport
failures.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/api/contract/OtpWebService.kt`:
- Around line 37-41: Add `@Streaming` to `OtpWebService.plan` so Retrofit does
not eagerly buffer the `ResponseBody` before `OtpPlanParser.parse` reads it via
`byteStream()`. Update the `plan(`@Url` url: String): Call<ResponseBody>`
declaration in `OtpWebService` to stream the OTP response directly off the wire
and keep `plainClient` behavior unchanged.
🪄 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: fb9a8377-9135-4ba0-ae0f-60e95336a627
📒 Files selected for processing (3)
onebusaway-android/src/main/java/org/onebusaway/android/api/contract/OtpWebService.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanRepository.kt
| val response = try { | ||
| otpWebService.plan(url).execute() | ||
| } catch (e: IOException) { | ||
| // Transport failure / timeout (SocketTimeoutException is an IOException) — mirror the legacy | ||
| // catch-all mapping to the request-timeout message. | ||
| throw IOException(errorMessage(Message.REQUEST_TIMEOUT.id), e) | ||
| } catch (e: FileNotFoundException) { | ||
| if (!useOldUrlStructure) { | ||
| connection?.disconnect() | ||
| connection = null | ||
| return requestPlan(request, baseUrl, useOldUrlStructure = true) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines
FILE="onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanRepository.kt"
wc -l "$FILE"
sed -n '120,220p' "$FILE"
# Find the caller and surrounding error handling
rg -n "planBlocking|runCatching|REQUEST_TIMEOUT|IllegalArgumentException|IOException|otpWebService.plan" onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan -n -A 3 -B 3
# Locate the Retrofit service definition for plan(`@Url`)
rg -n "`@Url`|interface .*WebService|fun plan\(" onebusaway-android/src/main/java -n -A 3 -B 3Repository: OneBusAway/onebusaway-android
Length of output: 35111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanRepository.kt"
echo "=== file size ==="
wc -l "$FILE"
echo "=== relevant slice ==="
sed -n '1,260p' "$FILE"
echo "=== plan-related references ==="
rg -n "fun planBlocking|runCatching|REQUEST_TIMEOUT|otpWebService.plan|IllegalArgumentException|IOException" "$FILE" -A 4 -B 4
echo "=== Retrofit service defs ==="
rg -n "`@Url`|interface .*WebService|fun plan\(" onebusaway-android/src/main/java -A 4 -B 4Repository: OneBusAway/onebusaway-android
Length of output: 38461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Retrofit dependency/version
rg -n "com.squareup.retrofit2:retrofit|retrofitVersion|retrofit" -g '!*build' -g '!*.png' .Repository: OneBusAway/onebusaway-android
Length of output: 3867
🌐 Web query:
Retrofit @url malformed URL IllegalArgumentException execute requestFactory.create Call.execute
💡 Result:
The IllegalArgumentException: Malformed URL in Retrofit typically occurs when the library, while attempting to resolve a relative URL against a base URL, interprets parts of the path as a scheme (e.g., http:// or similar) [1][2][3]. This often happens because OkHttp, which powers Retrofit, follows RFC 3986, where a colon in the first segment of a relative path can be mistaken for a URI scheme [2]. Common Causes and Solutions: 1. Colons in Path Parameters: If a path parameter contains a colon (e.g., part of a timestamp or a specific identifier), Retrofit may misinterpret it as a scheme definition [2][3]. If you are using a recent version of Retrofit, ensure you are utilizing the latest features designed to handle this. If the issue persists, manually encode the parameter value before passing it to the service method [1]. 2. Using @Url with Invalid Inputs: When using the @Url annotation, the provided string must be a valid, absolute URL that OkHttp can parse [4]. If you provide a string that is misinterpreted or structurally invalid, it will trigger an IllegalArgumentException during the request factory creation or execution phase [5][4]. Validate the string using HttpUrl.parse(url) before passing it to the method if the input is dynamic [4]. 3. Incorrect Relative Path Formatting: If your relative path resembles a full URL (e.g., it starts with something that looks like a scheme), Retrofit may attempt to parse it as an absolute URL [1]. To fix this, you can force it to be treated as a relative path by prefixing it with./ (e.g., @POST("./{path}")) [1]. Technical Context: The exception is thrown by RequestBuilder when baseUrl.resolve(relativeUrl) returns null [5][4]. This is an intentional design choice in Retrofit; because malformed URLs are considered exceptional cases, the library throws an unchecked IllegalArgumentException rather than forcing developers to handle checked exceptions in most method calls [4]. To troubleshoot, inspect the exact string being generated after parameter substitution [2]. If it contains a colon in a position where OkHttp expects a path segment, that is likely the root cause [2].
Citations:
- 1: https://stackoverflow.com/questions/40877342/malformed-url-exception-on-retrofit2-when-path-has-another-protocol-name
- 2: Fix malformed URL error when relative path contains colon in first segment lysine-dev/retrofit#4623
- 3: https://stackoverflow.com/questions/79474880/retrofit-recognizes-google-cloud-rest-api-relative-path-as-malformed-url
- 4: Passing malformed url triggers illigalArgumentException url == null lysine-dev/retrofit#1575
- 5: https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit2/RequestBuilder.java
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate build files and Retrofit version declarations
git ls-files | rg '(^|/)(build\.gradle(\.kts)?|settings\.gradle(\.kts)?|gradle/libs\.versions\.toml|versions\.toml)$'
rg -n "retrofit2|okhttp|converter-moshi|converter-jackson" . -g '!*build' -g '!*.png'Repository: OneBusAway/onebusaway-android
Length of output: 5253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how formattedOtpBaseUrl is produced, to see whether malformed URLs are reachable.
rg -n "formattedOtpBaseUrl|otpBaseUrl|TripRequestBuilder" onebusaway-android/src/main/java -A 4 -B 4
# Read the relevant TripRequestBuilder implementation sections.
FILE=$(git ls-files | rg 'TripRequestBuilder\.kt$|TripRequestBuilder\.java$' | head -n 1)
echo "FILE=$FILE"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
sed -n '1,260p' "$FILE"
fiRepository: OneBusAway/onebusaway-android
Length of output: 50385
🌐 Web query:
site:github.com square retrofit RequestFactory.create IllegalArgumentException malformed URL execute
💡 Result:
An IllegalArgumentException with the message "Malformed URL" in Retrofit occurs when the library cannot successfully resolve a full URL from the provided base URL and the relative path (or @Url parameter) [1][2]. This error is typically triggered during the execution of a request when Retrofit's RequestBuilder attempts to combine these components [1]. Common causes include: 1. Colon in the first path segment: A colon in the first part of a relative URL can be misinterpreted by OkHttp as the start of a URL scheme (e.g., "user:email={value}/login"), leading to a resolution failure [3]. Recent updates have addressed this by encoding such colons as %3A [3]. 2. Invalid path or URL formatting: Providing a relative path that cannot be resolved against the base URL, or using a dynamic @Url that is syntactically invalid, will cause the URL builder to return null, triggering the exception [1][2]. 3. Parameter substitution issues: If path parameters, when substituted, produce characters that break URL structure (such as unencoded colons or invalid delimiters), the resulting URL becomes malformed [4][5][3]. To debug and fix this issue: - Verify that your relative paths and dynamic URLs do not contain characters that violate URL standards [2]. - If you are using dynamic values in your paths, ensure they are properly encoded [5]. - You can pre-validate your URLs before passing them to Retrofit by using HttpUrl.parse(url). If this returns null, the URL is invalid and will cause this exception at runtime [2]. - Ensure you are using the latest version of Retrofit, as recent releases include fixes for common edge cases involving URL resolution and colon encoding [3][6].
Citations:
- 1: https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit2/RequestBuilder.java
- 2: Passing malformed url triggers illigalArgumentException url == null lysine-dev/retrofit#1575
- 3: Fix malformed URL error when relative path contains colon in first segment lysine-dev/retrofit#4623
- 4: Invalid Malformed URL lysine-dev/retrofit#3080
- 5:
:not being Url Encoded lysine-dev/retrofit#3126 - 6: https://github.com/square/retrofit/blob/trunk/CHANGELOG.md
🌐 Web query:
Retrofit @url invalid url IllegalArgumentException documentation
💡 Result:
In Retrofit, a java.lang.IllegalArgumentException involving the @Url annotation typically arises from one of the following scenarios: 1. Conflicting URL Definitions You cannot use a path in the HTTP method annotation (e.g., @GET("path")) while also providing an @Url parameter [1]. If you use @Url, the path must be omitted from the HTTP method annotation [2][3][4]. Incorrect: @GET("users") Call getUser(@Url String url); Correct: @GET Call getUser(@Url String url); 2. Incorrect Parameter Positioning The @Url parameter must be the first parameter in the method signature [5]. If it is placed elsewhere, Retrofit will throw an exception [5]. 3. Malformed URL Strings If the string passed to the @Url parameter is malformed (e.g., invalid format, missing protocol), Retrofit will fail to resolve the URL, often resulting in an IllegalArgumentException [6]. To debug this, ensure the string is a valid absolute or relative URL [6][7]. If you need to validate the URL before passing it to Retrofit, you can use HttpUrl.parse(string) to check for null [6]. General Best Practices: - Ensure your baseUrl in the Retrofit builder includes a trailing slash (e.g., http://example.com/api/) to avoid path resolution errors [7]. - If you intend to use a dynamic full URL, the @Url parameter will override the base URL defined in the Retrofit instance [2][7].
Citations:
- 1: https://stackoverflow.com/questions/41168853/java-lang-illegalargumentexception-url-cannot-be-used-with-get-url-parameter
- 2: https://square.github.io/retrofit/2.x/retrofit/retrofit2/http/Url.html
- 3: https://square.github.io/retrofit/2.x/retrofit/retrofit2/http/GET.html
- 4: https://square.github.io/retrofit/2.x/retrofit/retrofit2/http/POST.html
- 5: Check for @Url as the first parameter doesn't work on Kotlin lysine-dev/retrofit#2845
- 6: Passing malformed url triggers illigalArgumentException url == null lysine-dev/retrofit#1575
- 7: https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit2/Retrofit.java
Catch malformed @Url failures too. otpWebService.plan(url).execute() can throw IllegalArgumentException for an invalid OTP base URL, which skips this IOException mapping and bypasses the REQUEST_TIMEOUT user message. Catch it here and wrap it the same way.
🤖 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/TripPlanRepository.kt`
around lines 150 - 156, The TripPlanRepository plan call only maps IOException
failures to the REQUEST_TIMEOUT message, so invalid `@Url` cases from
otpWebService.plan(url).execute() can escape as IllegalArgumentException. Update
the try/catch in TripPlanRepository to also catch IllegalArgumentException
around the execute() call and wrap it in the same
IOException(errorMessage(Message.REQUEST_TIMEOUT.id), e) path used for transport
failures.
1 participant
Part of #1625 (does not fully close it — see "Remaining" below).
Retires the trip-planner's hand-rolled
java.net.HttpURLConnection— the last one in thedirections//trip-planner subsystem — by moving the OpenTripPlanner/plancall onto the Retrofit/OkHttp stack every other endpoint already uses.What changed
OtpWebService(api/contract/OtpWebService.kt):@GET fun plan(@Url url: String): Call<ResponseBody>. Deliberately a synchronousCallreturning the raw body (not asuspendfun / decoded DTO like the siblingBikeWebService), for two reasons:OtpPlanParser, which maps onto the OTP library's ownorg.opentripplanner.api.ws.Responsemodel — so no re-modeling of the whole OTP response as a serialization DTO, and the malformed-body handling stays in one place.planBlockingpath (the JavaRealtimeServiceworker) that must call it without a coroutine; a synchronous.execute()avoids arunBlockingbridge. Both callers are already off-main (planviawithContext(Dispatchers.IO),planBlockingis@WorkerThread).NetworkModule.provideOtpWebService: provides it like the other@Urlsidecar services (noApiParamsInterceptor), but on a dedicated client with the legacy 15s connect/read timeouts OTP has always used (OTP servers can be slow). Extracted a smallplainClient { … }helper soplainRetrofit(json, client)can take a configured client rather than duplicating the debug-logging setup.DefaultTripPlanRepository.requestPlan: swapped theHttpURLConnectionblock forotpWebService.plan(url).execute(). Behavior preserved exactly:/routers/defaultstructure → the same retry against the old/planstructure +preference_key_otp_api_url_versionflip;OtpPlanParseras before.URL construction, parsing, the fallback, and error mapping are unchanged — only the transport moves.
Remaining for #1625
directions/package to Kotlin (still 9 Java files).Date/Calendarsites tojava.time.(The AsyncTask + Jackson goals of #1625 were already done in earlier work.)
Testing
compileObaGoogleDebugKotlin+ Hilt processing green; trip-planner / trip-results /OtpPlanDecodeTestunit tests pass. Rebased cleanly on currentmain(compiles on the rebased base).HttpUrlnow percent-encodes a literal space as%20; commas in coordinates are unaffected).Summary by CodeRabbit
New Features
Bug Fixes
Chores