refactor: replace default network client with OkHttp #1180
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCI jobs were renamed to run the HttpURLConnection variant; Gradle property logic was inverted so OkHttp is the default unless Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client code
participant OkCall as OkHttpCall
participant OkHttp as OkHttpClient
participant Helper as buildHttpBody()
Client->>OkCall: execute(request)
OkCall->>OkHttp: newCall(request).execute()
OkHttp-->>OkCall: Response
OkCall->>Helper: buildHttpBody(Response)
alt content-type present
Helper-->>OkCall: HttpBody (constructed)
else no content-type
Helper-->>OkCall: null
end
OkCall-->>Client: Response + optional HttpBody
sequenceDiagram
autonumber
participant Push as CI push
participant Workflow as integration-test.yml
participant Gradle as Gradle
Push->>Workflow: trigger integration tests
Workflow->>Gradle: ./gradlew :java:testRestSuite -PhttpURLConnection
Workflow->>Gradle: ./gradlew :java:testRealtimeSuite -PhttpURLConnection
note right of Gradle: okhttp-specific runs no longer include -Pokhttp flag
Gradle-->>Workflow: test results & artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
github-actions
bot
temporarily deployed
to
staging/pull/1180/features
github-actions
bot
temporarily deployed
to
staging/pull/1180/javadoc
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/integration-test.yml(5 hunks)README.md(1 hunks)android/build.gradle.kts(1 hunks)java/build.gradle.kts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (19)
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (24)
🔇 Additional comments (3)
android/build.gradle.kts (1)
57-57: Make OkHttp explicit as Android's runtime network client.Switching to
runtimeOnlyscope for network-client-okhttp appropriately makes OkHttp the default HTTP client for Android users without exposing OkHttp internals at compile time. This aligns with the SDK's shift to OkHttp as the default across all platforms.README.md (1)
130-178: Clear and practical proxy setup documentation.The updated proxy guidance using
ProxyOptionsis cleaner and more user-friendly than requiring explicit OkHttp dependency management. The code example comprehensively covers both REST and Realtime clients with optional authentication, making it straightforward for users to implement proxy support.Verify that all classes and fields referenced in the example (
ProxyOptions,HttpAuth.Type.BASIC,HttpAuth.Type.DIGEST) exist and have the expected API..github/workflows/integration-test.yml (1)
11-92: Test matrix comprehensively covers both HTTP client variants; verify branch protection configuration.The updated workflow maintains test coverage for both OkHttp (default via okhttp jobs without the property) and HttpURLConnection (via httpurlconnection jobs with
-PhttpURLConnection). The renamed jobs clearly indicate what's being tested.Repository scan found no internal references to the old job names. However, verify that any branch protection rules on the main branch have been updated to reference the new job names (
check-rest-httpurlconnection,check-realtime-httpurlconnection,check-rest-okhttp,check-realtime-okhttp). Check this in GitHub repository settings under Branches → Branch protection rules and ensure required status checks reference the correct job names.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the network client configuration to use OkHttp as the default HTTP client instead of HttpURLConnection. The change simplifies proxy setup by making OkHttp (which has better proxy support) the default choice while still allowing users to opt-in to HttpURLConnection when needed.
- Inverted the build logic to make OkHttp the default network client, with HttpURLConnection as an opt-in via property flag
- Updated CI/CD workflows to test both HTTP client implementations separately
- Simplified README documentation by removing OkHttp-specific dependency instructions since it's now the default
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| java/build.gradle.kts | Inverted dependency logic to use network-client-okhttp by default, falling back to network-client-default (HttpURLConnection) when -PhttpURLConnection property is set |
| android/build.gradle.kts | Changed Android builds to unconditionally use network-client-okhttp as the runtime dependency |
| README.md | Removed OkHttp dependency installation instructions from proxy setup guide since OkHttp is now the default |
| .github/workflows/integration-test.yml | Renamed and restructured test jobs to explicitly test both HTTP client variants, with httpurlconnection tests using -PhttpURLConnection flag and okhttp tests running without flags |
Comments suppressed due to low confidence (1)
.github/workflows/integration-test.yml:57
- The artifact name for the Realtime test reports will conflict between the httpurlconnection and okhttp test jobs. When both
check-realtime-httpurlconnectionandcheck-realtime-okhttpjobs run, they will both try to upload artifacts with the namejava-build-reports-realtime, causing one to overwrite the other.
Consider making the artifact name unique by including the HTTP client variant, for example: java-build-reports-realtime-httpurlconnection.
- uses: actions/upload-artifact@v4
if: always()
with:
name: java-build-reports-realtime
path: java/build/reports/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ttypic
force-pushed
the
make-okhttp-default
branch
from
64f62a9 to
e26bc88
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1180/features
github-actions
bot
temporarily deployed
to
staging/pull/1180/javadoc
ttypic
force-pushed
the
make-okhttp-default
branch
from
e26bc88 to
cc1cd37
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1180/features
github-actions
bot
temporarily deployed
to
staging/pull/1180/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/integration-test.yml (1)
58-75: Consider uploading artifacts for OkHttp test jobs.The
check-rest-okhttpandcheck-realtime-okhttpjobs don't upload test reports, while the httpurlconnection variants do (lines 29-33, 53-57). This inconsistency may make debugging OkHttp test failures more difficult.Consider adding artifact uploads for consistency:
- uses: actions/upload-artifact@v4 if: always() with: name: java-build-reports-rest-okhttp path: java/build/reports/Also applies to: 76-93
♻️ Duplicate comments (1)
java/build.gradle.kts (1)
25-29: Breaking change still requires documentation.The inverted logic that makes OkHttp the default network client remains undocumented. The previous review comment correctly identified this as a breaking change requiring CHANGELOG entries and README updates.
As noted in the previous review: add entries to CHANGELOG.md documenting the default change and add a section to README.md explaining HTTP client selection and the
-PhttpURLConnectionflag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/integration-test.yml(5 hunks)README.md(1 hunks)android/build.gradle.kts(1 hunks)java/build.gradle.kts(1 hunks)network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java (1)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
Response(530-568)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: build
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (21)
🔇 Additional comments (5)
android/build.gradle.kts (1)
57-57: LGTM: Android now uses OkHttp by default.This change aligns with the PR's objective to make OkHttp the default network client for Android.
README.md (1)
137-137: LGTM: Simplified proxy setup documentation.The updated documentation correctly reflects the streamlined proxy configuration using
ProxyOptions, removing the previous OkHttp-specific guidance. The example code is clear and complete..github/workflows/integration-test.yml (2)
11-11: LGTM: Job names now clearly indicate HTTP client variant.The renamed jobs (
check-rest-httpurlconnectionandcheck-realtime-httpurlconnection) improve clarity by explicitly identifying which HTTP client implementation is being tested.Also applies to: 35-35
27-27: LGTM: Test flags correctly aligned with inverted default.The httpurlconnection jobs now use
-PhttpURLConnectionto test the non-default client, while okhttp jobs run without flags to test the new OkHttp default. This correctly validates both network client implementations.Also applies to: 51-51, 74-74, 92-92
network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java (1)
44-51: The review comment is based on an incorrect assumption about the code behavior.The
buildHttpBodymethod correctly returnsnullwhencontentTypeisnull. This is intentional design, not a bug:
Content-Type is required for deserialization: Without a
Content-Typeheader, the response body bytes cannot be properly interpreted or deserialized. Returningnullis the correct response.Framework explicitly expects null bodies: HttpCore.java (line 477) shows the pattern:
if (response.statusCode == HttpURLConnection.HTTP_NO_CONTENT || rawResponse.getBody() == null)— the framework treats null bodies as valid and expected for certain responses (e.g., 204 No Content).Callers properly handle null: The framework consistently checks for null before accessing body content (HttpCore.java line 481), confirming this pattern is intentional.
Lombok builder accepts null: HttpResponse uses
@Builderwith no@NonNullconstraint on the body field, sonullis a valid value.Likely an incorrect or invalid review comment.
… support setup - Updated build scripts to use `network-client-okhttp` by default where appropriate. - Revised README to simplify proxy setup instructions. - Adjusted integration workflows to correctly configure HTTP client preferences.
ttypic
force-pushed
the
make-okhttp-default
branch
from
cc1cd37 to
c3018d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/emulate.yml (1)
42-42: Conditional flag application aligns with build default inversion.Line 42 correctly applies
-PhttpURLConnectiononly for API level 19, allowing other API levels to use the new OkHttp default. Verify that API 19's specific requirement for HttpURLConnection is documented (e.g., via inline comment or linked issue/PR) to prevent future maintenance confusion about why this API level is special-cased.Consider adding a clarifying inline comment above the
scriptline:disable-animations: true # Print emulator logs if tests fail + # API 19 uses HttpURLConnection; other levels use OkHttp (the new default) script: ./gradlew :android:connectedAndroidTest ${{ matrix.android-api-level == 19 && '-PhttpURLConnection' || '' }} || (adb logcat -d System.out:I && exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/emulate.yml(1 hunks).github/workflows/integration-test.yml(5 hunks)README.md(1 hunks)android/build.gradle.kts(1 hunks)java/build.gradle.kts(1 hunks)network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java
- android/build.gradle.kts
- README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (21)
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check-liveobjects
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/integration-test.yml (1)
11-34: The artifact naming concern is unfounded—okhttp variant jobs do not upload artifacts.Verification of the full workflow file shows:
check-rest-httpurlconnectionuploads tojava-build-reports-restcheck-rest-okhttphas no artifact upload step (only runs gradle)check-realtime-httpurlconnectionuploads tojava-build-reports-realtimecheck-realtime-okhttphas no artifact upload step (only runs gradle)The review comment incorrectly assumed okhttp jobs would upload artifacts with identical names. They do not upload artifacts at all, so there is no collision. The suggested diff changes are unnecessary.
Likely an incorrect or invalid review comment.
network-client-okhttpby default where appropriate.Summary by CodeRabbit
Documentation
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.