Surface coordinator error body from apiCall - #1798
Conversation
parseError consumes the HTTP body. Checking auth and then rethrowing the raw HttpException left callers with only "HTTP 400". Co-authored-by: Cursor <cursoragent@cursor.com>
|
@CodeRabbit review |
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
✅ Action performedReview finished.
|
PratimMallick
added
pr:improvement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe client now parses HTTP error bodies once, retries authentication failures with a refreshed token, and exposes coordinator messages for other failures. Tests cover configurable token providers, error responses, message preservation, and successful authentication retry. ChangesCoordinator error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Coordinator failures now expose the server message while authentication retries remain unchanged. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant apiCall
participant ErrorParser
participant TokenProvider
apiCall->>ErrorParser: Parse HttpException once
ErrorParser-->>apiCall: Return Error.NetworkError
apiCall->>TokenProvider: Load fresh token for auth error
TokenProvider-->>apiCall: Return refreshed token
apiCall->>apiCall: Retry request
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes the required Goal, Implementation, and Testing sections and accurately explains the error-body handling, authentication retry, unit tests, and verification steps. UI, checklist, reviewer, and GIF sections are not completed, but they are not critical for this non-UI change.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SDK Size Comparison 📏
|
Avoid wrapping coordinator HTTP errors in a generic Exception so callers can still classify retries, and parse the post-refresh attempt the same way. Co-authored-by: Cursor <cursoragent@cursor.com>
|
| ): Failure { | ||
| val networkError = parsed ?: parseError(exception).value as Error.NetworkError | ||
| logger.e { "[apiCall] HTTP ${exception.code()}: ${networkError.message}" } | ||
| return Failure(Error.ThrowableError(networkError.message, exception)) |
There was a problem hiding this comment.
Non-JSON error bodies now produce a worse message than before this PR.
coordinatorHttpFailure uses networkError.message unconditionally, but when the body isn't Stream JSON parseError never reaches the branch that reads error.message — it returns one of two fallbacks, both of which say "failed to parse error response from server". (Empty bodies land there too: errorBody()?.bytes() returns a zero-length array, not null, so decodeFromString("") throws into the parse-failure branch.)
So for a 502/504/captive-portal response the caller used to get "HTTP 502 Bad Gateway" and now gets "failed to parse error response from server: Expected start of the object '{' ...". The status code is gone from the message entirely — strictly less useful than what was there before, on the failure mode users hit most.
Suggest falling back to "HTTP ${exception.code()} ${exception.message()}" when the parse didn't produce a real coordinator error. Watch out that the no-body branch sets serverErrorCode = e.code() rather than PARSER_ERROR, so both fallbacks need covering, not just the PARSER_ERROR one.
Worth a test that throws an HttpException with a 502 and an HTML body, and asserts the status still appears in the resulting ThrowableError.message. That case is also most of the gap to the 80% Sonar gate this PR is currently under.
| ): Result<T> { | ||
| val firstError = parseError(first).value as Error.NetworkError | ||
| if (firstError.isAuthError()) { | ||
| val newToken = tokenProvider.loadToken() |
There was a problem hiding this comment.
A failing loadToken() swallows the coordinator message.
If the customer's TokenProvider throws here, the exception escapes to the outer catch (e: Exception) in apiCall and the caller ends up with "Safe call failed with <token provider message>". The original 401 reason — the thing this PR exists to surface — is discarded, on the one path where the SDK definitely has it.
Wrapping the refresh and falling back to coordinatorHttpFailure(first, firstError) keeps the coordinator message and still reports the refresh failure via the log.
| serverErrorCode = error.code, | ||
| statusCode = error.statusCode, | ||
| cause = Throwable(error.moreInfo), | ||
| cause = e, |
There was a problem hiding this comment.
Two pieces of the parsed error end up unreachable by callers.
Switching cause to e is right — that's what fixes @rahul-lohra's classification point — but moreInfo isn't kept anywhere else, and the conversion to ThrowableError keeps only the message, so serverErrorCode goes too. That half of rahul's comment is still open, and parseError consumes the body so callers can't re-derive it.
If keeping ThrowableError is the decision — fair, CallCrudTest depends on it — folding both into the message ("[<code>] <message> (<moreInfo>)") at least keeps them recoverable to a human reading logs.
3 participants
Goal
Fixes AND-1478 — Coordinator rejections from
StreamVideoClient.apiCallshould expose the server message, not a bare HTTP status.parseErrorconsumes the response body. The old path used it only for the auth-retry check, then rethrew the rawHttpException, so callers only sawHTTP 400.Implementation
apiCall.NetworkErrorfor the auth-retry check.Testing
./gradlew :stream-video-android-core:testDebugUnitTest --tests "io.getstream.video.android.core.StreamVideoClientTest"./gradlew spotlessApplyusercallingUpdateCall) and confirm the log/result includes the server reason.Made with Cursor
Summary by CodeRabbit