fix[live-objects]: replace unsafe array cast with .map
#1152
Conversation
WalkthroughRemoved a file-level unchecked-cast suppression and replaced two array-wide casts with per-element casts via map in writeMsgpackArray and asJsonArray within DefaultSerialization.kt. Logic flow and public API remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1152/features
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Can you also remove
@file:Suppress("UNCHECKED_CAST")
from top of the file
…version in serialization methods
ttypic
force-pushed
the
fix/array-casting
branch
from
520f8cf to
e52e002
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (2)
24-27: Optional: avoid the intermediate list while preserving “fail-before-write” semantics.Current approach allocates a List. If you want to drop that allocation yet still ensure nothing is written to the packer unless all elements are valid, a two-pass validation works. Trade-off: slightly more code and two casts per element.
Apply this diff if you prefer the two-pass approach:
- val objectMessages = objects.map { it as ObjectMessage } - packer.packArrayHeader(objectMessages.size) - objectMessages.forEach { it.writeMsgpack(packer) } + // First pass: validate elements (throws before any write on invalid element) + for (o in objects) { + o as ObjectMessage + } + // Second pass: write + packer.packArrayHeader(objects.size) + for (o in objects) { + (o as ObjectMessage).writeMsgpack(packer) + }
37-43: Optional: remove the temporary list to reduce allocations.For the JSON path, you can avoid the intermediate List and directly build the JsonArray. Since JsonArray is only returned on success, partially filled arrays on exception aren’t observable to callers.
Apply this diff if you prefer the single-pass approach:
- val objectMessages = objects.map { it as ObjectMessage } - - val jsonArray = JsonArray() - for (objectMessage in objectMessages) { - jsonArray.add(objectMessage.toJsonObject()) - } - return jsonArray + val jsonArray = JsonArray() + for (o in objects) { + val objectMessage = o as ObjectMessage + jsonArray.add(objectMessage.toJsonObject()) + } + return jsonArray
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:38-46
Timestamp: 2025-06-23T14:14:17.847Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class uses intentional unsafe casting (`objects as Array<ObjectMessage>`) without type validation in both writeMsgpackArray and asJsonArray methods. This is a deliberate design decision to keep the dynamically-loaded class simple and let ClassCastException occur naturally for type mismatches.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:25-29
Timestamp: 2025-06-23T14:18:25.315Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class methods like writeMsgpackArray will have their type signatures updated to be non-nullable since the calling sites ensure objects are never null when passed to these methods.
📚 Learning: 2025-06-23T14:18:25.315Z
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:25-29
Timestamp: 2025-06-23T14:18:25.315Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class methods like writeMsgpackArray will have their type signatures updated to be non-nullable since the calling sites ensure objects are never null when passed to these methods.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt
📚 Learning: 2025-06-23T14:14:17.847Z
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:38-46
Timestamp: 2025-06-23T14:14:17.847Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class uses intentional unsafe casting (`objects as Array<ObjectMessage>`) without type validation in both writeMsgpackArray and asJsonArray methods. This is a deliberate design decision to keep the dynamically-loaded class simple and let ClassCastException occur naturally for type mismatches.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check
🔇 Additional comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (3)
24-27: Safer per-element cast avoids relying on array reified type — good change.Switching from an array-wide cast to per-element casts via map removes the brittle dependency on the runtime component type of the array. This aligns with the PR objective.
37-43: LGTM: consistent per-element cast for JSON path as well.Mirrors the MessagePack path, keeping semantics aligned.
24-27: Confirm intentional change to per-element type casting behavior
The new implementation replaces the previous array-wide cast (objects as Array<ObjectMessage>) with per-element casts (it as ObjectMessage), shifting failure from the initial cast to individual elements during packing. This alters when and how aClassCastExceptionis raised.Please verify that:
- This behavioral change is desired and documented.
- No existing tests or code rely on the old “fail-fast” array-cast semantics.
Recommended checks (run in the repo root):
# Find tests using @Test(expected=ClassCastException) rg -n -P --glob '*Test*' '@Test\s*\(\s*expected\s*=\s*ClassCastException' # Find assertThrows usage for ClassCastException rg -n -P --glob '*Test*' 'assertThrows\s*(ClassCastException|TypeCastException)' # Find any remaining array-wide casts to ObjectMessage rg -n -P 'as\s+Array<\s*ObjectMessage\s*>'If you do find tests expecting the old behavior, update or remove them to align with per-element casting.
— DefaultSerialization.kt (lines 24–27)
Array<Object>can't be cast toArray<ObjectMessages>even if every element is an instance ofObjectMessages, so it's safer to usemapSummary by CodeRabbit