Resolve crash that occurs when a connection to Keycloak fails#3822
Resolve crash that occurs when a connection to Keycloak fails#3822
Conversation
Refactored fetchAccessToken to simplify and catch all exceptions generically
allan-on
added
Error-Handling
Bug-fix
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (24.6%) is below the target coverage (25.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3822 +/- ##
=========================================
- Coverage 25.0% 24.6% -0.4%
- Complexity 844 849 +5
=========================================
Files 297 305 +8
Lines 16102 16516 +414
Branches 2689 2747 +58
=========================================
+ Hits 4038 4076 +38
- Misses 11580 11956 +376
Partials 484 484
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| Result.failure(sslHandShakeException) | ||
| } catch (e: Exception) { | ||
| Result.failure(e) | ||
| } |
There was a problem hiding this comment.
Instead of omitting all these, you can retain them and add.
catch (e: Exception) {
Result.failure(e)
}At the end, to catch any missed exceptions.
…ix-auth-server-connection-crash
….com:opensrp/fhircore into bugfix/3821-fix-auth-server-connection-crash
There was a problem hiding this comment.
Pull request overview
This PR addresses app crashes when Keycloak is unreachable by ensuring fetchAccessToken() returns a failed Result instead of letting uncaught connection-related exceptions propagate.
Changes:
- Add a generic exception catch in
TokenAuthenticator.fetchAccessToken()to prevent crashes on previously-unhandled failures (e.g., connection errors). - Update the corresponding unit test to validate failure handling across multiple exception types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| android/engine/src/main/java/org/smartregister/fhircore/engine/data/remote/shared/TokenAuthenticator.kt | Adds a fallback catch (e: Exception) when fetching tokens to avoid crashes on unhandled exceptions. |
| android/engine/src/test/java/org/smartregister/fhircore/engine/auth/TokenAuthenticatorTest.kt | Refactors exception-handling test to iterate over multiple exception types and assert failures consistently. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Test with various exception types to verify generic exception handling | ||
| val exceptions = | ||
| listOf( | ||
| HttpException(Response.error<OAuthResponse>(401, mockk(relaxed = true))), | ||
| UnknownHostException(), | ||
| SSLHandshakeException("SSL error"), | ||
| IOException("IO error"), | ||
| ) |
There was a problem hiding this comment.
The new test aims to cover “all network exceptions”, but it doesn’t include the ConnectException that triggered the reported crash (issue #3821). Add ConnectException (and ideally SocketTimeoutException) to this list so the regression is directly covered.
| } catch (httpException: HttpException) { | ||
| Result.failure(httpException) | ||
| } catch (unknownHostException: UnknownHostException) { | ||
| Result.failure(unknownHostException) | ||
| } catch (sslHandShakeException: SSLHandshakeException) { | ||
| Result.failure(sslHandShakeException) | ||
| } catch (e: Exception) { | ||
| Result.failure(e) |
There was a problem hiding this comment.
fetchAccessToken now catches Exception, which will also catch kotlinx.coroutines.CancellationException and prevent cooperative cancellation (and can cause structured-concurrency bugs). Consider adding a catch (e: CancellationException) { throw e } before this, and narrowing the generic catch to IOException (plus HttpException) to avoid masking programmer errors while still covering ConnectException, SocketTimeoutException, UnknownHostException, SSLHandshakeException, etc.
There was a problem hiding this comment.
@allan-on Can you apply this change it is okay.
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes #3821
Engineer Checklist
strings.xmlfile./gradlew spotlessApplyand./gradlew spotlessCheckto check my code follows the project's style guideCode Reviewer Checklist
strings.xmlfile