-
-
Notifications
You must be signed in to change notification settings - Fork 14
Replace okhttp with ktor (okhttp engine) #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
# Conflicts: # app/src/androidTest/java/at/bitfire/icsdroid/model/ValidationUseCaseTest.kt # app/src/main/java/at/bitfire/icsdroid/model/ValidationUseCase.kt
Signed-off-by: Arnau Mora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the HTTP networking implementation from OkHttp to Ktor with OkHttp as the underlying engine. This migration maintains the same underlying networking behavior while adopting Ktor's more modern Kotlin-first API.
- Replaces direct OkHttp client usage with Ktor HttpClient while keeping OkHttp as the engine
- Migrates test infrastructure from MockWebServer to Ktor's MockEngine with a custom wrapper
- Updates dependency injection to provide HttpClient through Hilt
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
gradle/libs.versions.toml | Adds Ktor dependencies for core, mock, and OkHttp engine |
app/build.gradle.kts | Replaces OkHttp dependencies with Ktor equivalents |
app/src/main/java/at/bitfire/icsdroid/HttpClient.kt | Refactors singleton HttpClient to use Ktor with OkHttp engine and adds Hilt module |
app/src/main/java/at/bitfire/icsdroid/CalendarFetcher.kt | Updates HTTP calls to use Ktor API instead of OkHttp |
app/src/main/java/at/bitfire/icsdroid/model/ValidationUseCase.kt | Adds HttpClient dependency injection |
app/src/main/java/at/bitfire/icsdroid/ProcessEventsTask.kt | Updates to use injected HttpClient |
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt | New wrapper for Ktor MockEngine to simplify test setup |
app/src/androidTest/java/at/bitfire/icsdroid/model/ValidationUseCaseTest.kt | Migrates from MockWebServer to MockEngineWrapper |
app/src/androidTest/java/at/bitfire/icsdroid/CalendarFetcherTest.kt | Updates test infrastructure to use Ktor MockEngine |
Comments suppressed due to low confidence (3)
gradle/libs.versions.toml:29
- Ktor version 3.2.1 does not exist. The latest stable version of Ktor is 2.3.x series. Please use a valid Ktor version like "2.3.12" or check the official Ktor releases.
ktor = "3.2.1"
app/src/main/java/at/bitfire/icsdroid/HttpClient.kt:28
- [nitpick] The alias 'AppHttpClient' is confusing since it's aliasing the same class being defined. Consider using a more descriptive name or removing the alias if not needed.
import at.bitfire.icsdroid.HttpClient as AppHttpClient
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt:28
- [nitpick] The fallback response uses HttpStatusCode.NotImplemented which may not clearly indicate the actual issue (empty queue). Consider using a more descriptive status code or error message for debugging test failures.
respond("No more responses", HttpStatusCode.NotImplemented)
Signed-off-by: Arnau Mora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! A few minor style comments. Feel free to ask or discard if you don't agree.
app/src/androidTest/java/at/bitfire/icsdroid/CalendarFetcherTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/at/bitfire/icsdroid/MockEngineWrapper.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK 👍
Purpose
Migrate the current okhttp implementation to ktor.
Short description
MockWebServer
usages to Ktor's mock server, using a custom wrapper to simplify the structure.Checklist