Chore/wpb 18240 fix typo and refactor #146
Conversation
allows to easily extend type of network exceptions SDK handles
| val errorResponse = try { | ||
| this.response.body<ApiError>() | ||
| } catch (e: JsonConvertException) { | ||
| logger.error( | ||
| "HttpResponseValidator exception could not be mapped to any ApiError type; " + | ||
| "mapped as WireException.UnknownError. To handle this exception, " + | ||
| "extend the ApiError interface and update the mapper." | ||
| ) | ||
| throw WireException.UnknownError(e.cause?.message, e) | ||
| } | ||
|
|
||
| throw when (this) { | ||
| is ClientRequestException -> { | ||
| WireException.ClientError(errorResponse, this) |
There was a problem hiding this comment.
As I remember we said that mapToWireException was only used when dealing with BackendClient and therefore only dealt with Ktor based errors. Then we added the errorHandler (HttpResponseValidator) in Ktor config (which is nice).
If we map ktor exceptions, I don't think it makes sense to have here a generic mapper and try to deserialize or handle only Ktor errors.
I think we can do the simplest thing, remove this mapper completely and also ApiError and put all the ktor error handling inside HttpResponseValidator in the Modules.kt. In that file we map ktor exceptions to WireException, and if we know that Wire backend errors have a specific format (label and message), then we use that.
Instead of introducing ApiError, we can just subclass WireException.ClientError and create subclasses for specific issues.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors exception handling by making the exception mapping more extensible and fixes a typo. The changes improve the interface design to handle different types of backend responses beyond the single standard format.
- Renamed
ErrorResponsetoStandardErrorand moved MLS stale message logic to the response object - Refactored exception mapping to be more type-safe and extensible for different payload types
- Fixed a spelling error in a test method name from "Unknow" to "Unknown"
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| WireExceptionMapper.kt | Refactored mapping logic to be more type-safe and handle StandardError directly |
| WireException.kt | Updated to use StandardError and removed duplicate MLS logic |
| StandardError.kt | Renamed from ErrorResponse and added MLS stale message check method |
| Modules.kt | Added proper type checking for ResponseException in validator |
| WireApplicationManager.kt | Updated to use response.isMlsStaleMessage() method |
| HttpClientExceptionTest.kt | Fixed typo and updated test to use response property |
|
|
||
| @Test | ||
| fun whenErrorResponseDoesNotContainLabelThenThrowUnknowException() { | ||
| fun whenErrorResponseDoesNotContainLabelThenThrowUnknownException() { |
There was a problem hiding this comment.
Fixed spelling error: 'Unknow' corrected to 'Unknown' in the test method name.
Copilot uses AI. Check for mistakes.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Exception mapping is more suited for handling new types of exceptions.
Issues
Previous implementation was too tied with single type of backend response consisting of 'code', 'message' and 'label' fields.
Solutions
Add extendable interface in case we need to handle different payloads.
PR Post Submission Checklist for internal contributors (Optional)
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.