chore: Bump ktor to 3.2.3 (WPB-8645) #3590
Conversation
|
Bencher Report| Branch | bump-ktor |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | microseconds (µs) |
|---|---|---|
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles | 📈 view plot | 704.07 µs |
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory | 📈 view plot | 465,426.31 µs |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark | 📈 view plot | 1,482,051.96 µs |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark | 📈 view plot | 28,462.86 µs |
| val observer: ResponseHandler = observer@{ | ||
| if (level == LogLevel.NONE || it.call.attributes.contains(DisableLogging)) { | ||
| return@observer | ||
| client.receivePipeline.intercept(HttpReceivePipeline.After) { response -> |
There was a problem hiding this comment.
did you test with this logger enabled, i remember using intercept will consume the response stream and later stages will not have anything to read
There was a problem hiding this comment.
yes I can see the response in logs
network/src/commonMain/kotlin/com/wire/kalium/network/api/v9/authenticated/NotificationApiV9.kt
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates Ktor from version 2.3.10 to 3.2.3 and resolves related API compatibility issues that prevented building release apps. The update includes necessary proguard rules and code changes to adapt to breaking changes in the Ktor API.
- Updates Ktor version from 2.3.10 to 3.2.3 with accompanying dependency updates
- Adapts code to handle Ktor 3.x API changes including deprecated methods and new property names
- Adds comprehensive proguard rules to support the new Ktor version in release builds
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Updates Ktor version and related dependencies (kermit, cells-sdk) |
| network/consumer-proguard-rules.pro | Adds Ktor-specific proguard rules to prevent obfuscation issues in release builds |
| network/src/commonMain/kotlin/com/wire/kalium/network/session/SessionManager.kt | Updates API usage from deprecated content to rawContent property |
| network/src/commonMain/kotlin/com/wire/kalium/network/serialization/ByteArrayConverter.kt | Replaces deprecated serializeNullable with serialize method |
| network/src/commonMain/kotlin/com/wire/kalium/network/utils/NetworkUtils.kt | Changes from deprecated pathSegments to rawSegments property |
| network/src/commonMain/kotlin/com/wire/kalium/network/KaliumKtorCustomLogging.kt | Refactors response logging to use new API patterns |
| network/src/commonMain/kotlin/com/wire/kalium/network/KaliumObservingUtils.kt | Updates channel closing method from close() to flushAndClose() |
| Multiple test files | Adds null safety checks for refresh tokens |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import io.ktor.utils.io.ByteReadChannel | ||
| import io.mockative.Mockable | ||
| import kotlin.coroutines.CoroutineContext | ||
| import io.ktor.utils.io.InternalAPI |
There was a problem hiding this comment.
Using @InternalAPI annotation suggests this code relies on internal Ktor APIs that may change in future versions. Consider finding a stable public API alternative or document this dependency risk.
| import io.ktor.utils.io.InternalAPI |
| xerialDriver = "3.48.0.0" | ||
| kotlinx-io = "0.5.3" | ||
| cells-sdk = "0.1.1-alpha10" | ||
| cells-sdk = "0.1.1-alpha15" |
There was a problem hiding this comment.
| cells-sdk = "0.1.1-alpha15" | |
| cells-sdk = "0.1.1-alpha15.02" |
This newer version has artifacts for all apple targets we support. So... no more warnings and we can ditch x64 and use simulatorArm64 instead.
| override val responseTime: GMTDate = response.responseTime | ||
| override val content: ByteReadChannel = response.content | ||
|
|
||
| @InternalAPI |
There was a problem hiding this comment.
What is the goal of adding this @InternalAPI annotation?
If it's delicate, we can/should create an annotation of our own.
@RequiresOptIn(
level = RequiresOptIn.Level.ERROR,
message = "Some explanation here."
)
@Target(
AnnotationTarget.CLASS,
AnnotationTarget.TYPEALIAS,
AnnotationTarget.FUNCTION,
AnnotationTarget.PROPERTY,
AnnotationTarget.FIELD,
AnnotationTarget.CONSTRUCTOR,
AnnotationTarget.PROPERTY_SETTER,
AnnotationTarget.PROPERTY_SETTER
)
public annotation class InternalNetworkAPI
|
@claude review |
Pull Request Review - Ktor 3.2.3 UpgradeOverviewThis PR upgrades Ktor from version 2.3.10 to 3.2.3, addressing issues with ProGuard rules that prevented building release apps. The changes also include updates to Kermit (2.0.3 → 2.0.6) and Cells SDK (0.1.1-alpha10 → 0.1.1-alpha15). Summary✅ Recommendation: Approve with minor observations The PR properly addresses the ProGuard configuration issues and adapts the codebase to Ktor 3.x API changes. The implementation is sound with good attention to detail in API migration. Detailed Review1. Dependencies Update ✅Files:
Assessment: Major version upgrades require careful testing, especially for networking libraries. The Ktor 3.x upgrade includes breaking API changes that have been properly addressed. 2. ProGuard Rules ✅File: Added 22 new rules to prevent obfuscation of Ktor 3.x classes:
Assessment: The ProGuard rules are comprehensive and follow best practices. These rules are essential for preventing runtime crashes in release builds. 3. API Migration Changes ✅A. WebSocket ConfigurationFile: // Old (Ktor 2.x)
pingInterval = WEBSOCKET_PING_INTERVAL_MILLIS // Long value
// New (Ktor 3.x)
pingInterval = WEBSOCKET_PING_INTERVAL_MILLIS.milliseconds // Duration typeAssessment: ✅ Correct migration to Kotlin Duration API. This is a breaking change in Ktor 3.x that has been properly handled. B. WebSocket Close HandlingFiles: // Old
.onCompletion {
defaultClientWebSocketSession.close()
emit(WebSocketEvent.Close(it))
}
// New
.onCompletion { cause ->
val closeReason = if (cause == null || cause is CancellationException) {
CloseReason(CloseReason.Codes.NORMAL, "Normal closure")
} else {
CloseReason(CloseReason.Codes.INTERNAL_ERROR, "Error: ${cause.message}")
}
defaultClientWebSocketSession.close(closeReason)
emit(WebSocketEvent.Close(cause))
}Assessment: ✅ Excellent improvement! Ktor 3.x requires explicit close reasons. This implementation:
C. Content Converter APIFile: // Old
override suspend fun serializeNullable(...)
// New
override suspend fun serialize(...)Assessment: ✅ Correct API migration. Ktor 3.x renamed this method, properly updated. D. HTTP Response Content AccessFile: // Old
override val content: ByteReadChannel = response.content
// New
@InternalAPI
override val rawContent: ByteReadChannel get() = response.rawContentAssessment: ✅ Correct migration to the new property name. The E. URL Path SegmentsFiles: // Old
pathSegments = baseUrl.pathSegments
// New
pathSegments = baseUrl.rawSegmentsAssessment: ✅ Proper migration. Ktor 3.x changed the property name for raw path segments. F. Logging PluginFile: Replaced // Old - Used ResponseObserver.install()
// New - Direct pipeline interception
client.receivePipeline.intercept(HttpReceivePipeline.After) { response ->
// Log response body
logger.logResponseBody(response.contentType(), ByteReadChannel(response.readRawBytes()))
}Assessment: ✅ Correct approach for Ktor 3.x. The ResponseObserver API changed, and this is the recommended pattern. G. I/O UtilitiesFile: // Old
log.close()
// New
log.flushAndClose()Assessment: ✅ Better practice - ensures data is flushed before closing. 4. Null Safety Improvements ✅Files: Test files and // Old
oldRefreshToken = oldTokens\!\!.refreshToken
// New
oldRefreshToken = oldTokens\!\!.refreshToken ?: error("Old refresh token is null\!")Assessment: ✅ Improved error handling with explicit error messages, making debugging easier. 5. Test Adjustments ✅A. Build ConfigurationFile: Improved test configuration structure with better nesting and organization. Assessment: ✅ Good refactoring for clarity. B. Test DependenciesFiles: Added Assessment: ✅ Necessary for tests that verify content negotiation behavior. C. API Call ChangesFile: // Old
nodeServiceApi.listNamespaceValues(namespace = TAGS_METADATA, operationValues = listOf())
// New
nodeServiceApi.listNamespaceValues(namespace = TAGS_METADATA)Assessment: ✅ Simplification - removed unnecessary empty list parameter. Potential Concerns & Recommendations1. Testing Coverage
|
|
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
Ktor update was reverted in this commit d30f3df because it's not possible to build a release app
Causes (Optional)
Some proguard rules are missing
Solutions
Added them
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.