-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/WPB-18240 handle MLS HTTP exceptions #145
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
Feat/WPB-18240 handle MLS HTTP exceptions #145
Conversation
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.
Great work! I just added some nitpick comments but overall super great! now we have better handling of exceptions 🙌🏻
lib/src/main/kotlin/com/wire/integrations/jvm/client/BackendClientDemo.kt
Show resolved
Hide resolved
lib/src/main/kotlin/com/wire/integrations/jvm/config/Modules.kt
Outdated
Show resolved
Hide resolved
lib/src/main/kotlin/com/wire/integrations/jvm/model/ErrorResponse.kt
Outdated
Show resolved
Hide resolved
lib/src/main/kotlin/com/wire/integrations/jvm/service/WireApplicationManager.kt
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.
Pull Request Overview
This PR refactors HTTP exception handling to provide more specific error information, particularly for MLS (Message Layer Security) HTTP exceptions. The changes replace a generic exception mapping approach with a structured response validator that can identify specific error types through response labels.
Key changes:
- Implemented native Ktor HTTP response validation for better error handling granularity
- Added structured error response mapping with label-based exception identification
- Replaced generic HTTP status code checking with specific label-based exception detection for MLS stale message errors
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
WireExceptionMapperTest.kt |
Removed old generic exception mapping tests |
HttpClientExceptionTest.kt |
Added comprehensive tests for new HTTP response validation with MLS-specific error scenarios |
WireApplicationManager.kt |
Updated to use label-based MLS stale message detection instead of HTTP status code |
ErrorResponse.kt |
New data class for structured error response parsing |
WireExceptionMapper.kt |
Refactored to handle Ktor ResponseException with structured error response parsing |
WireException.kt |
Updated ClientError and InternalSystemError to use ErrorResponse with MLS-specific helper methods |
NetworkErrorLabel.kt |
Added constants for network error labels |
Modules.kt |
Integrated new HTTP response validator into HttpClient configuration |
BackendClientImpl.kt |
Removed generic exception wrapper calls |
BackendClientDemo.kt |
Removed generic exception wrapper calls and updated exception handling |
} | ||
|
||
@Test | ||
fun whenErrorResponseDoesNotContainLabelThenThrowUnknowException() { |
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.
Function name contains a spelling error: 'Unknow' should be 'Unknown'
fun whenErrorResponseDoesNotContainLabelThenThrowUnknowException() { | |
fun whenErrorResponseDoesNotContainLabelThenThrowUnknownException() { |
Copilot uses AI. Check for mistakes.
) | ||
else -> WireException.UnknownError(throwable = this) | ||
} | ||
suspend fun Throwable.mapToWireException() { |
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.
The function returns Unit but always throws an exception. Consider changing the return type to Nothing to make the non-returning behavior explicit
suspend fun Throwable.mapToWireException() { | |
suspend fun Throwable.mapToWireException(): Nothing { |
Copilot uses AI. Check for mistakes.
throwable = this | ||
) | ||
} | ||
} catch (_: JsonConvertException) { |
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.
Consider logging the JsonConvertException before falling back to UnknownError to aid in debugging serialization issues
} catch (_: JsonConvertException) { | |
} catch (e: JsonConvertException) { | |
logger.error("JsonConvertException occurred while processing the response: ${e.message}", e) |
Copilot uses AI. Check for mistakes.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
Not possible to specify exactly what HTTP exception indicates
Causes
Standard exceptions were mapped into custom Wire exception without exposing additional information
Solutions
Testing
Test Coverage
PR Post Submission Checklist for internal contributors (Optional)
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.