fix: update the SSE message parsing to not cause number exception#289
fix: update the SSE message parsing to not cause number exception#289
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Android SDK’s Server-Sent Events (SSE) message parsing to prevent runtime number/parsing exceptions when handling realtime update messages, improving robustness of the realtime-config refresh path.
Changes:
- Extracted SSE message parsing into a dedicated
handleSSEMessagefunction and addedJSONExceptionhandling. - Made parsing tolerant to different SSE payload shapes (outer envelope with
"data"vs direct inner payload) and to mixed field types. - Updated
initEventSourceto use the new handler reference.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (e: JSONException) { | ||
| DevCycleLogger.w(e, "SSE Message: Error parsing SSE message data: ${messageEvent.data}") | ||
| } |
There was a problem hiding this comment.
Avoid logging the full raw SSE payload on JSONException. messageEvent.data may be large and can include config values; logging it can leak sensitive data and create noisy logs. Consider logging only the exception plus a truncated payload, payload length, or an event id/type extracted safely via opt* calls.
|
that is correct @bencehornak its to fix #288, will add that as part of the PR description |
suthar26
force-pushed
the
ICP-3942-update-sse-message-parsing
branch
from
265afdf to
fc3a216
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * 2. **Direct format** (legacy): The `data` field directly contains the message | ||
| * payload as a JSON object (with `type`, `lastModified`, `etag`). |
There was a problem hiding this comment.
SSEMessage KDoc says the "Direct format" is where the data field directly contains the payload as a JSON object, but the implementation (and tests) also support a direct/top-level payload with no data field. Update the KDoc to describe both supported direct forms to avoid confusing consumers.
| * 2. **Direct format** (legacy): The `data` field directly contains the message | |
| * payload as a JSON object (with `type`, `lastModified`, `etag`). | |
| * 2. **Direct format** (legacy): The message payload (with `type`, `lastModified`, | |
| * `etag`) is provided either as the top-level JSON object itself, or directly | |
| * in the `data` field as a JSON object. |
| } else "" | ||
|
|
||
| val etag = if (innerData.has("etag") && !innerData.isNull("etag")) { | ||
| innerData.getString("etag") |
There was a problem hiding this comment.
innerData.getString("etag") will throw JSONException if etag is present but not a JSON string (e.g., number/boolean/object). Since parse() is documented to return null when data cannot be parsed, consider using optString/opt + toString() (with NULL handling) or wrapping the field extraction in a try/catch and returning null to keep parsing exception-safe.
| innerData.getString("etag") | |
| innerData.opt("etag").toString() |
fix to combat the SSE read error when parsing the message
closes #288