Add opt-in fast event parsing (message.new)#6473
Open
VelikovPetar wants to merge 1 commit into
Open
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
WalkthroughAdds an opt-in fast event parsing path: a builder flag, config, DI wiring, and DirectEventParser with direct Moshi adapters (initially for message.new). MoshiChatParser attempts direct parsing then falls back to DTO mapping. Extensive tests and fixtures ensure parity and transformer application. ChangesFast Event Parsing Pipeline
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser/EventArguments.kt (1)
1096-1096: ⚡ Quick winDocument the new suppression rationale.
Line 1096 adds
@Suppress("LongMethod")without context. Please add a short rationale comment (or remove the suppression if no longer needed).♻️ Suggested tweak
- `@Suppress`("LongMethod") + // Kept as a single source-of-truth table for parser/event parity arguments. + `@Suppress`("LongMethod")As per coding guidelines:
**/*.kt: Use@OptInannotations explicitly in Kotlin code; avoid suppressions unless documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser/EventArguments.kt` at line 1096, The new `@Suppress`("LongMethod") in EventArguments.kt lacks a rationale; either remove it if the method no longer exceeds length limits or add a short comment explaining why the suppression is required (e.g., "Contains many test cases for event parsing; splitting would reduce readability") directly above the `@Suppress`("LongMethod") annotation so reviewers know why the rule is disabled; ensure the comment references the specific method or test block name in EventArguments.kt to make the intent clear.stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageTestData.kt (1)
26-27: ⚡ Quick winDocument or remove
@Suppress("LargeClass").This suppression is currently undocumented. Please add a short reason above it (or remove it if not needed) to match repo rules.
Suggested update
-@Suppress("LargeClass") +// Intentionally large: central fixture holder for parser parity scenarios. +@Suppress("LargeClass") internal object MessageTestData {As per coding guidelines "Use
@OptInannotations explicitly in Kotlin code; avoid suppressions unless documented".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageTestData.kt` around lines 26 - 27, The `@Suppress`("LargeClass") on the MessageTestData object is undocumented; either remove it if the class size is acceptable or add a one-line KDoc explaining why the suppression is necessary (e.g., test fixture grouping) directly above the annotation. Update the MessageTestData object declaration to include that brief rationale or delete the `@Suppress`("LargeClass") so the class follows the repository rule of avoiding undocumented suppressions.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt (1)
4878-4890: ⚡ Quick winAdd explicit thread/state notes to the new Builder API KDoc.
The new public method documents behavior, but it should also state invocation expectations (builder-time usage) and when the flag is applied (captured at
build()time).✍️ Suggested KDoc update
/** * Enables the fast event-parsing path for incoming WebSocket events. * + * Thread/state notes: + * - Configure this on [Builder] before calling [build]. + * - The value is captured into [ChatApiConfig] during [build] and cannot be changed afterward. + * * When enabled, supported event types are parsed directly into domain models, bypassing * the DTO intermediate layer. Unsupported event types fall back to the default DTO-based * parser, so behavior is preserved for events the fast path does not yet handle.As per coding guidelines "Document public APIs with KDoc, including thread expectations and state notes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt` around lines 4878 - 4890, Update the KDoc for the public Builder method fastEventParsing(enabled: Boolean) to include explicit thread/state expectations and when the flag takes effect: note that this is a builder-time API intended to be called from the constructing thread (not required to be thread-safe), that the provided value is captured at build() time, and that changes after build() have no effect on the ChatClient instance; reference the Builder class and build() method in the doc so callers know when the flag is applied and any concurrency assumptions.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ReactionAdapter.kt (1)
31-32: ⚡ Quick winAdd rationale for
LongMethodsuppression (or refactor to remove it).This suppression needs a short justification comment to satisfy the Kotlin suppression guideline.
Suggested minimal fix
- `@Suppress`("LongMethod") + `@Suppress`("LongMethod") // Intentional: single-pass direct parser for hot-path reaction payloads.As per coding guidelines:
**/*.kt:Use@OptInannotations explicitly in Kotlin code; avoid suppressions unless documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ReactionAdapter.kt` around lines 31 - 32, The `@Suppress`("LongMethod") on ReactionAdapter.fromJson lacks a justification; either add a one-line rationale comment above the annotation explaining why the method must remain long (e.g., complex JSON parsing for backward compatibility and multiple legacy formats) or refactor by extracting logical blocks (parsing of reaction fields, user parsing, and legacy compatibility handling) into private helper methods (e.g., parseReactionFields(), parseUser(), handleLegacyFormats()) and remove the suppression; update the annotation location on the fromJson function accordingly.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/UserAdapter.kt (1)
35-36: ⚡ Quick winDocument the
LongMethodsuppression infromJson.Please add a concise reason inline (or split method and drop suppression) to align with repo policy.
Suggested minimal fix
- `@Suppress`("LongMethod") + `@Suppress`("LongMethod") // Intentional: flat streaming parse to preserve DTO-parity and hot-path performance.As per coding guidelines:
**/*.kt:Use@OptInannotations explicitly in Kotlin code; avoid suppressions unless documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/UserAdapter.kt` around lines 35 - 36, The LongMethod suppression on the fromJson(JsonReader) method lacks justification; either remove the suppression by refactoring and splitting fromJson into smaller helper functions (e.g., parseUserBasicFields, parseUserNestedObjects) or keep the suppression but add a one-line reason comment above it explaining why the method must remain long (e.g., "Complex JSON parsing with many fields and performance-sensitive single-pass reader; split would add overhead"). Update the `@Suppress`("LongMethod") usage in UserAdapter.kt to include that concise rationale or refactor the logic into named private helpers and drop the suppression, referencing the fromJson method and any new helper names in your change.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/PollAdapter.kt (1)
48-49: ⚡ Quick winDocument or remove the
LongMethodsuppression.Please add a brief rationale for this suppression (or refactor to remove it) so it complies with repo rules.
Suggested minimal fix
- `@Suppress`("LongMethod") + `@Suppress`("LongMethod") // Intentional: single-pass hot-path parser; split would add overhead.As per coding guidelines:
**/*.kt:Use@OptInannotations explicitly in Kotlin code; avoid suppressions unless documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/PollAdapter.kt` around lines 48 - 49, The `@Suppress`("LongMethod") on PollAdapter.fromJson should be documented or removed: either add a short comment above the suppression explaining why the method is long and why splitting it is impractical (e.g., tightly coupled parsing logic for Poll and legacy JSON variants), or refactor fromJson into smaller private helpers (e.g., parsePollFields, parseOptions, parseVotes) and remove the suppression; update PollAdapter.fromJson to delegate to those helpers so the method length no longer needs suppressing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/AttachmentAdapter.kt`:
- Around line 26-27: The `@Suppress`("LongMethod") on AttachmentAdapter.fromJson
is undocumented; either remove it and refactor the long method into smaller
helpers (e.g., extract parsing blocks from fromJson into private functions) or
keep a suppression but add a brief inline rationale comment above it explaining
why the method must remain long; if the suppression was used to avoid an API
opt-in, prefer using the appropriate `@OptIn` annotation instead. Ensure changes
target AttachmentAdapter and the fromJson implementation so the file no longer
contains an undocumented `@Suppress`.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageAdapter.kt`:
- Around line 36-37: The class-level `@Suppress`("LongParameterList") on
MessageAdapter (and the other `@Suppress` usages around the same file) lacks a
rationale; either remove/refactor to avoid the suppression or keep it but add a
brief inline comment explaining why the suppression is necessary (e.g.,
unavoidable generated/adapter constructor shape or third‑party interface
requirement). Locate the suppressions on the MessageAdapter declaration and the
subsequent suppressions near lines 56–57 and either refactor to reduce parameter
count/complexity or retain the annotation with a one-line justification comment
immediately above it describing the reason and expected scope.
- Around line 189-201: The enrichment only fixes `channelInfo` one level deep
for quoted messages (in MessageAdapter where `enrichedQuotedMessage` is
computed), causing DTO parity to break for chains like message -> quoted_message
-> quoted_message; replace the one-off conditional with a recursive propagation
that walks the quoted-message chain and fills null `channelInfo` fields with
`resolvedChannelInfo` (e.g., implement a helper that given a `quotedMessage`
returns a copy with `channelInfo` set if missing and its inner `quotedMessage`
recursively normalized), and use that helper when computing
`enrichedQuotedMessage`.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/NewMessageEventAdapter.kt`:
- Around line 38-153: The fromJson method in NewMessageEventAdapter is too
complex; refactor it by extracting four focused helpers: 1) parseFields(reader):
move the reader.beginObject()/while loop/reader.endObject() and all field
assignments (including calls to userAdapter.fromJson, messageAdapter.fromJson
and parseChannelCustom) into a function that returns a data holder (e.g., a
private data class ParsedNewMessageFields). 2) validateRequiredFields(parsed):
move all JsonParsingUtils.requireField checks and the created_at parse/throw
logic into a validator that accepts the parsed fields. 3) enrichMessage(parsed):
move the channelInfo/cid/replyTo enrichment logic into a function that returns
the enriched Message (use parsed.message, parsed.cid, parsed.channelType,
parsed.channelId, parsed.channelMemberCount, parsed.channelCustomName,
parsed.channelCustomImage). 4) buildEvent(parsed, enrichedMessage): construct
and return the NewMessageEvent from the parsed values and enrichedMessage.
Replace the original monolithic code in fromJson to call these helpers in
sequence; keep existing helper names (parseChannelCustom, JsonParsingUtils.*,
messageAdapter, userAdapter) and behavior unchanged.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/DirectEventParser.kt`:
- Around line 111-115: In parse(raw: String) guard against adapter
deserialization throwing: after resolving type via extractType and fetching
adapter from adapterMap, call adapter.fromJson(raw) inside a try/catch; if it
throws, log/handle the error and fall back to the DTO parsing route (the
existing DTO deserializer/path used elsewhere in this class) and return its
result instead of propagating the exception; ensure this fallback only runs for
supported direct adapters and preserves returning null when both adapter and DTO
parsing fail.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`:
- Around line 4878-4890: Update the KDoc for the public Builder method
fastEventParsing(enabled: Boolean) to include explicit thread/state expectations
and when the flag takes effect: note that this is a builder-time API intended to
be called from the constructing thread (not required to be thread-safe), that
the provided value is captured at build() time, and that changes after build()
have no effect on the ChatClient instance; reference the Builder class and
build() method in the doc so callers know when the flag is applied and any
concurrency assumptions.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/PollAdapter.kt`:
- Around line 48-49: The `@Suppress`("LongMethod") on PollAdapter.fromJson should
be documented or removed: either add a short comment above the suppression
explaining why the method is long and why splitting it is impractical (e.g.,
tightly coupled parsing logic for Poll and legacy JSON variants), or refactor
fromJson into smaller private helpers (e.g., parsePollFields, parseOptions,
parseVotes) and remove the suppression; update PollAdapter.fromJson to delegate
to those helpers so the method length no longer needs suppressing.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ReactionAdapter.kt`:
- Around line 31-32: The `@Suppress`("LongMethod") on ReactionAdapter.fromJson
lacks a justification; either add a one-line rationale comment above the
annotation explaining why the method must remain long (e.g., complex JSON
parsing for backward compatibility and multiple legacy formats) or refactor by
extracting logical blocks (parsing of reaction fields, user parsing, and legacy
compatibility handling) into private helper methods (e.g.,
parseReactionFields(), parseUser(), handleLegacyFormats()) and remove the
suppression; update the annotation location on the fromJson function
accordingly.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/UserAdapter.kt`:
- Around line 35-36: The LongMethod suppression on the fromJson(JsonReader)
method lacks justification; either remove the suppression by refactoring and
splitting fromJson into smaller helper functions (e.g., parseUserBasicFields,
parseUserNestedObjects) or keep the suppression but add a one-line reason
comment above it explaining why the method must remain long (e.g., "Complex JSON
parsing with many fields and performance-sensitive single-pass reader; split
would add overhead"). Update the `@Suppress`("LongMethod") usage in UserAdapter.kt
to include that concise rationale or refactor the logic into named private
helpers and drop the suppression, referencing the fromJson method and any new
helper names in your change.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser/EventArguments.kt`:
- Line 1096: The new `@Suppress`("LongMethod") in EventArguments.kt lacks a
rationale; either remove it if the method no longer exceeds length limits or add
a short comment explaining why the suppression is required (e.g., "Contains many
test cases for event parsing; splitting would reduce readability") directly
above the `@Suppress`("LongMethod") annotation so reviewers know why the rule is
disabled; ensure the comment references the specific method or test block name
in EventArguments.kt to make the intent clear.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageTestData.kt`:
- Around line 26-27: The `@Suppress`("LargeClass") on the MessageTestData object
is undocumented; either remove it if the class size is acceptable or add a
one-line KDoc explaining why the suppression is necessary (e.g., test fixture
grouping) directly above the annotation. Update the MessageTestData object
declaration to include that brief rationale or delete the
`@Suppress`("LargeClass") so the class follows the repository rule of avoiding
undocumented suppressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9652c9fc-39ec-4026-b93a-3c8f8b57667c
📒 Files selected for processing (68)
stream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/ChatApiConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/di/ChatModule.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/extensions/ChatEvent.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/DirectEventParser.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/MoshiChatParser.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/AttachmentAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ChannelInfoAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/DeviceAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/JsonParsingUtils.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/LocationAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageModerationDetailsAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageReminderInfoAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ModerationAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/NewMessageEventAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/OptionAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/PollAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/PrivacySettingsAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ReactionAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/ReactionGroupAdapter.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/UserAdapter.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/EventChatJsonProvider.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/api/RetrofitCallAdapterFactoryTests.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser/EventArguments.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/AttachmentParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/ChannelInfoParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/DeviceParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/DirectEventParserTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/JsonParsingUtilsTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/LocationParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/MessageModerationDetailsParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/MessageParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/MessageReminderInfoParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/ModerationParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/MoshiChatParserTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/NewMessageEventParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/OptionParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/ParserFactory.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/PollParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/PrivacySettingsParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/ReactionGroupParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/ReactionParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/UserParsingTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/AnswerTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/AttachmentTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ChannelInfoTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ChannelTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ChannelUserReadTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/CommandTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ConfigTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/DeviceTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/LocationTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MemberTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageModerationDetailsTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageReminderInfoTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/MessageTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ModerationTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/NewMessageEventTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/OptionTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/PollTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/PrivacySettingsTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/PushPreferenceTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ReactionGroupTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/ReactionTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/UserTestData.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/testdata/VoteTestData.kt
Comment on lines
+26
to
+27
| @Suppress("LongMethod") | ||
| override fun fromJson(reader: JsonReader): Attachment? { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document or remove the suppression annotation.
@Suppress("LongMethod") is currently undocumented. Please add a brief rationale next to it or refactor to avoid suppression.
As per coding guidelines "**/*.kt: Use @OptIn annotations explicitly in Kotlin code; avoid suppressions unless documented`."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/AttachmentAdapter.kt`
around lines 26 - 27, The `@Suppress`("LongMethod") on AttachmentAdapter.fromJson
is undocumented; either remove it and refactor the long method into smaller
helpers (e.g., extract parsing blocks from fromJson into private functions) or
keep a suppression but add a brief inline rationale comment above it explaining
why the method must remain long; if the suppression was used to avoid an API
opt-in, prefer using the appropriate `@OptIn` annotation instead. Ensure changes
target AttachmentAdapter and the fromJson implementation so the file no longer
contains an undocumented `@Suppress`.
Comment on lines
+36
to
+37
| @Suppress("LongParameterList") | ||
| internal class MessageAdapter( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid undocumented suppressions in this adapter.
These @Suppress usages should include a short rationale or be removed via refactor.
As per coding guidelines "**/*.kt: Use @OptIn annotations explicitly in Kotlin code; avoid suppressions unless documented`."
Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageAdapter.kt`
around lines 36 - 37, The class-level `@Suppress`("LongParameterList") on
MessageAdapter (and the other `@Suppress` usages around the same file) lacks a
rationale; either remove/refactor to avoid the suppression or keep it but add a
brief inline comment explaining why the suppression is necessary (e.g.,
unavoidable generated/adapter constructor shape or third‑party interface
requirement). Locate the suppressions on the MessageAdapter declaration and the
subsequent suppressions near lines 56–57 and either refactor to reduce parameter
count/complexity or retain the annotation with a one-line justification comment
immediately above it describing the reason and expected scope.
Comment on lines
+189
to
+201
| // Known limit: channelInfo propagation is only one level deep. A two-deep chain | ||
| // (message -> quoted_message -> quoted_message) where the inner two messages have no | ||
| // `channel` field will leave the innermost message's channelInfo null, while the DTO | ||
| // path would propagate the outer message's channelInfo down two levels. Two-deep | ||
| // quoted_message chains are rare in practice; if support is needed, this fallback | ||
| // needs to be threaded recursively (or replaced with a post-hoc traversal). | ||
| val enrichedQuotedMessage = quotedMessage?.let { qm -> | ||
| if (resolvedChannelInfo != null && qm.channelInfo == null) { | ||
| qm.copy(channelInfo = resolvedChannelInfo) | ||
| } else { | ||
| qm | ||
| } | ||
| } |
There was a problem hiding this comment.
Deep quoted_message chains still diverge from DTO parity.
The current enrichment only fixes one level, so two-deep quoted chains can produce different channelInfo than the DTO path. That breaks the stated direct-vs-DTO parity contract for valid payloads.
Proposed fix sketch (recursive channelInfo propagation)
- val enrichedQuotedMessage = quotedMessage?.let { qm ->
- if (resolvedChannelInfo != null && qm.channelInfo == null) {
- qm.copy(channelInfo = resolvedChannelInfo)
- } else {
- qm
- }
- }
+ val enrichedQuotedMessage = propagateChannelInfoRecursively(
+ message = quotedMessage,
+ fallbackChannelInfo = resolvedChannelInfo,
+ )
@@
private fun parseMemberChannelRole(reader: JsonReader): String? {
@@
}
+
+ private fun propagateChannelInfoRecursively(
+ message: Message?,
+ fallbackChannelInfo: ChannelInfo?,
+ ): Message? {
+ if (message == null) return null
+
+ val resolvedForCurrent = message.channelInfo ?: fallbackChannelInfo
+ val current = if (message.channelInfo == null && resolvedForCurrent != null) {
+ message.copy(channelInfo = resolvedForCurrent)
+ } else {
+ message
+ }
+
+ val propagatedReplyTo = propagateChannelInfoRecursively(
+ message = current.replyTo,
+ fallbackChannelInfo = resolvedForCurrent,
+ )
+ return if (propagatedReplyTo !== current.replyTo) {
+ current.copy(replyTo = propagatedReplyTo)
+ } else {
+ current
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Known limit: channelInfo propagation is only one level deep. A two-deep chain | |
| // (message -> quoted_message -> quoted_message) where the inner two messages have no | |
| // `channel` field will leave the innermost message's channelInfo null, while the DTO | |
| // path would propagate the outer message's channelInfo down two levels. Two-deep | |
| // quoted_message chains are rare in practice; if support is needed, this fallback | |
| // needs to be threaded recursively (or replaced with a post-hoc traversal). | |
| val enrichedQuotedMessage = quotedMessage?.let { qm -> | |
| if (resolvedChannelInfo != null && qm.channelInfo == null) { | |
| qm.copy(channelInfo = resolvedChannelInfo) | |
| } else { | |
| qm | |
| } | |
| } | |
| // Known limit: channelInfo propagation is only one level deep. A two-deep chain | |
| // (message -> quoted_message -> quoted_message) where the inner two messages have no | |
| // `channel` field will leave the innermost message's channelInfo null, while the DTO | |
| // path would propagate the outer message's channelInfo down two levels. Two-deep | |
| // quoted_message chains are rare in practice; if support is needed, this fallback | |
| // needs to be threaded recursively (or replaced with a post-hoc traversal). | |
| val enrichedQuotedMessage = propagateChannelInfoRecursively( | |
| message = quotedMessage, | |
| fallbackChannelInfo = resolvedChannelInfo, | |
| ) | |
| private fun propagateChannelInfoRecursively( | |
| message: Message?, | |
| fallbackChannelInfo: ChannelInfo?, | |
| ): Message? { | |
| if (message == null) return null | |
| val resolvedForCurrent = message.channelInfo ?: fallbackChannelInfo | |
| val current = if (message.channelInfo == null && resolvedForCurrent != null) { | |
| message.copy(channelInfo = resolvedForCurrent) | |
| } else { | |
| message | |
| } | |
| val propagatedReplyTo = propagateChannelInfoRecursively( | |
| message = current.replyTo, | |
| fallbackChannelInfo = resolvedForCurrent, | |
| ) | |
| return if (propagatedReplyTo !== current.replyTo) { | |
| current.copy(replyTo = propagatedReplyTo) | |
| } else { | |
| current | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/MessageAdapter.kt`
around lines 189 - 201, The enrichment only fixes `channelInfo` one level deep
for quoted messages (in MessageAdapter where `enrichedQuotedMessage` is
computed), causing DTO parity to break for chains like message -> quoted_message
-> quoted_message; replace the one-off conditional with a recursive propagation
that walks the quoted-message chain and fills null `channelInfo` fields with
`resolvedChannelInfo` (e.g., implement a helper that given a `quotedMessage`
returns a copy with `channelInfo` set if missing and its inner `quotedMessage`
recursively normalized), and use that helper when computing
`enrichedQuotedMessage`.
Comment on lines
+38
to
+153
| override fun fromJson(reader: JsonReader): NewMessageEvent? { | ||
| if (reader.peek() == JsonReader.Token.NULL) return reader.nextNull() | ||
|
|
||
| reader.beginObject() | ||
|
|
||
| var type: String? = null | ||
| var createdAt: Date? = null | ||
| var rawCreatedAt: String? = null | ||
| var user: User? = null | ||
| var cid: String? = null | ||
| var channelMemberCount: Int? = null | ||
| var channelCustomName: String? = null | ||
| var channelCustomImage: String? = null | ||
| var channelType: String? = null | ||
| var channelId: String? = null | ||
| var message: Message? = null | ||
| var watcherCount: Int = 0 | ||
| var totalUnreadCount: Int = 0 | ||
| var unreadChannels: Int = 0 | ||
| var channelMessageCount: Int? = null | ||
|
|
||
| while (reader.hasNext()) { | ||
| when (reader.nextName()) { | ||
| "type" -> type = reader.nextString() | ||
| "created_at" -> { | ||
| if (reader.peek() != JsonReader.Token.NULL) { | ||
| val rawValue = reader.nextString() | ||
| rawCreatedAt = rawValue | ||
| createdAt = streamDateFormatter.parse(rawValue) | ||
| } else { | ||
| reader.skipValue() | ||
| } | ||
| } | ||
| "user" -> user = userAdapter.fromJson(reader) | ||
| "cid" -> cid = reader.nextString() | ||
| "channel_member_count" -> channelMemberCount = JsonParsingUtils.readNullableInt(reader) | ||
| "channel_custom" -> { | ||
| val (name, image) = parseChannelCustom(reader) | ||
| channelCustomName = name | ||
| channelCustomImage = image | ||
| } | ||
| "channel_type" -> channelType = reader.nextString() | ||
| "channel_id" -> channelId = reader.nextString() | ||
| "message" -> message = messageAdapter.fromJson(reader) | ||
| "watcher_count" -> watcherCount = reader.nextInt() | ||
| "total_unread_count" -> totalUnreadCount = reader.nextInt() | ||
| "unread_channels" -> unreadChannels = reader.nextInt() | ||
| "channel_message_count" -> channelMessageCount = JsonParsingUtils.readNullableInt(reader) | ||
| else -> reader.skipValue() | ||
| } | ||
| } | ||
| reader.endObject() | ||
|
|
||
| JsonParsingUtils.requireField(type, "type", reader) | ||
| JsonParsingUtils.requireField(rawCreatedAt, "created_at", reader) | ||
| if (createdAt == null) { | ||
| // rawCreatedAt was present but unparseable; mirror DTO path which throws via ExactDateAdapter. | ||
| throw JsonDataException("Unparseable 'created_at' value '$rawCreatedAt' at ${reader.path}") | ||
| } | ||
| JsonParsingUtils.requireField(user, "user", reader) | ||
| JsonParsingUtils.requireField(cid, "cid", reader) | ||
| JsonParsingUtils.requireField(channelType, "channel_type", reader) | ||
| JsonParsingUtils.requireField(channelId, "channel_id", reader) | ||
| JsonParsingUtils.requireField(message, "message", reader) | ||
|
|
||
| // Enrich inline: set channelInfo + cid so parseAndProcessEvent can skip enrichIfNeeded(). | ||
| // Only copy if something actually needs to change. | ||
| val needsChannelInfo = message.channelInfo == null | ||
| val needsCid = message.cid != cid | ||
| val replyTo = message.replyTo | ||
| val needsReplyToCid = replyTo != null && replyTo.cid != cid | ||
| val needsReplyToChannelInfo = replyTo != null && replyTo.channelInfo == null | ||
| val needsReplyToEnrichment = needsReplyToCid || needsReplyToChannelInfo | ||
|
|
||
| val enrichedMessage = if (needsChannelInfo || needsCid || needsReplyToEnrichment) { | ||
| val fallbackChannelInfo = ChannelInfo( | ||
| cid = cid, | ||
| id = channelId, | ||
| type = channelType, | ||
| memberCount = channelMemberCount ?: 0, | ||
| name = channelCustomName, | ||
| image = channelCustomImage, | ||
| ) | ||
| message.copy( | ||
| channelInfo = message.channelInfo ?: fallbackChannelInfo, | ||
| cid = if (needsCid) cid else message.cid, | ||
| replyTo = replyTo?.let { rt -> | ||
| if (needsReplyToEnrichment) { | ||
| rt.copy( | ||
| cid = if (needsReplyToCid) cid else rt.cid, | ||
| channelInfo = rt.channelInfo ?: fallbackChannelInfo, | ||
| ) | ||
| } else { | ||
| rt | ||
| } | ||
| }, | ||
| ) | ||
| } else { | ||
| message | ||
| } | ||
|
|
||
| return NewMessageEvent( | ||
| type = type, | ||
| createdAt = createdAt ?: Date(0), | ||
| rawCreatedAt = rawCreatedAt, | ||
| user = user, | ||
| cid = cid, | ||
| channelType = channelType, | ||
| channelId = channelId, | ||
| message = enrichedMessage, | ||
| watcherCount = watcherCount, | ||
| totalUnreadCount = totalUnreadCount, | ||
| unreadChannels = unreadChannels, | ||
| channelMessageCount = channelMessageCount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Refactor fromJson to pass the complexity quality gate.
This method is currently over the configured cognitive complexity limit and is reported as a failing static-analysis check. Please split it into focused helpers (field scan, required-field validation, enrichment, event construction).
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 38-38: Refactor this method to reduce its Cognitive Complexity from 25 to the 15 allowed.
[warning] 141-141: Remove this useless elvis operation ?:, it always succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/direct/NewMessageEventAdapter.kt`
around lines 38 - 153, The fromJson method in NewMessageEventAdapter is too
complex; refactor it by extracting four focused helpers: 1) parseFields(reader):
move the reader.beginObject()/while loop/reader.endObject() and all field
assignments (including calls to userAdapter.fromJson, messageAdapter.fromJson
and parseChannelCustom) into a function that returns a data holder (e.g., a
private data class ParsedNewMessageFields). 2) validateRequiredFields(parsed):
move all JsonParsingUtils.requireField checks and the created_at parse/throw
logic into a validator that accepts the parsed fields. 3) enrichMessage(parsed):
move the channelInfo/cid/replyTo enrichment logic into a function that returns
the enriched Message (use parsed.message, parsed.cid, parsed.channelType,
parsed.channelId, parsed.channelMemberCount, parsed.channelCustomName,
parsed.channelCustomImage). 4) buildEvent(parsed, enrichedMessage): construct
and return the NewMessageEvent from the parsed values and enrichedMessage.
Replace the original monolithic code in fromJson to call these helpers in
sequence; keep existing helper names (parseChannelCustom, JsonParsingUtils.*,
messageAdapter, userAdapter) and behavior unchanged.
Comment on lines
+111
to
+115
| fun parse(raw: String): ChatEvent? { | ||
| val type = extractType(raw) ?: return null | ||
| val adapter = adapterMap[type] ?: return null | ||
| return adapter.fromJson(raw) | ||
| } |
There was a problem hiding this comment.
Guard direct adapter failures and fall back to DTO path.
A thrown exception from a supported direct adapter currently bypasses fallback and fails the whole event parse. For this opt-in path, parser failures should degrade to DTO parsing.
🛡️ Suggested fallback-safe implementation
fun parse(raw: String): ChatEvent? {
val type = extractType(raw) ?: return null
val adapter = adapterMap[type] ?: return null
- return adapter.fromJson(raw)
+ return runCatching { adapter.fromJson(raw) }
+ .onFailure { e ->
+ logger.v { "Direct parse failed for '$type'; falling back to DTO path: ${e.message}" }
+ }
+ .getOrNull()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/parser2/DirectEventParser.kt`
around lines 111 - 115, In parse(raw: String) guard against adapter
deserialization throwing: after resolving type via extractType and fetching
adapter from adapterMap, call adapter.fromJson(raw) inside a try/catch; if it
throws, log/handle the error and fall back to the DTO parsing route (the
existing DTO deserializer/path used elsewhere in this class) and return its
result instead of propagating the exception; ensure this fallback only runs for
supported direct adapters and preserves returning null when both adapter and DTO
parsing fail.
Co-Authored-By: Claude <noreply@anthropic.com>
VelikovPetar
force-pushed
the
port/v6-to-develop/fast-event-parsing
branch
from
eb5e473 to
3a2bf83
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Port of v6 PR #6344 (squashed merge commit
993370813c— "Add opt-in fast event parsing (message.new)") todevelop.Introduces an opt-in direct JSON-to-Domain parsing path for WebSocket events that bypasses the intermediate DTO layer. Reduces object allocations and speeds up event deserialization on the hot path (currently
message.new). Disabled by default; enable viaChatClientConfig(fastEventParsing = true).Resolves: https://linear.app/stream/issue/AND-1143/improve-messagenew-event-parsing-memory-footprint
Implementation
This is a manual port, not a clean cherry-pick. Three v6↔develop drifts required adjustments:
fastEventParsingconfig placement. The flag's public source of truth isChatClientConfig.fastEventParsing(mirrored to the internalChatApiConfig.fastEventParsingat build time, following the precedent ofdebugRequests/notificationConfig). No dedicated Builder method — users set it viaChatClient.Builder.config(ChatClientConfig(fastEventParsing = true)). This aligns with the project's migration to config-based configuration and matches the priorMessageBufferConfigport pattern.Poll.maxVotesAllowed: Int → Int?on develop. The DirectPollAdapterno longer defaults missing/nullmax_votes_allowedto1— it preservesnullto match the develop DTO path. Four expected fixtures inPollTestData.ktupdated accordingly (the one fixture whose JSON explicitly has"max_votes_allowed":1still expects1).AttachmentDto.file_sizeis nullable on develop (since Accept null file_size in AttachmentDto #6462). The DirectAttachmentAdapterreadsfile_sizeas nullable and defaults to0(DTO-path parity). The twofile_size: nulltests inAttachmentParsingTest.ktare updated from "must throw" to "defaults to 0", matching the DTO path's new behavior.All other modified files (
ChatClient.kt,ChatModule.kt,MoshiChatParser.kt,extensions/ChatEvent.kt,EventChatJsonProvider.kt,EventArguments.kt,MoshiChatParserTest.kt,ParserFactory.kt,RetrofitCallAdapterFactoryTests.kt) were ported with the v6 changes applied as-is. All 57 net-new files (DirectEventParser+ 13 direct adapters + 16 parsing tests + 22 testdata + 5 supporting files) were copied verbatim from v6 (except thePollAdapterfix above).UI Changes
No UI changes.
Testing
./gradlew :stream-chat-android-client:detekt— passed./gradlew :stream-chat-android-client:spotlessApply— no reformats./gradlew :stream-chat-android-client:apiDump— addsChatClientConfig.getFastEventParsing(), new ctor overload, and updatedcopy()/copy$default()/component9()(noBuilder.fastEventParsingline)./gradlew :stream-chat-android-client:testDebugUnitTest— full client module suite passes (parser2 tests: 359 passed)Dual-path parity tests (DTO vs Direct) cover every adapter end-to-end, ensuring the new fast path produces identical domain objects to the existing DTO path.