[ECO-5386] Liveobjects serialization without jackson deps #1106
Conversation
1. Added LiveObjectsHelper for initializing LiveObjects plugin and LiveObjectsSerializer impl. 2. Updated ProtocolMessage to handle msgpack serialization/deserialization for state field 3. Implemented LiveObjectsJsonSerializer for json serialization/deserialization of state field
…sage
1. Added jackson-dataformat-msgpack 0.8.11 dependency for automatic msgpack handling
2. Implemented JSON serialization via ObjectMessage.toJsonObject() and JsonObject.toObjectMessage() extensions
3. Implemented MessagePack serialization via ObjectMessage.writeTo() and MessageUnpacker.readObjectMessage() extensions
4. Added @SerializedName("object") annotation for proper field naming consistency
1. Added JSON serializer/deserializer using ObjectDataJsonSerializer 2, Added msgpack serializer/deserializer using ObjectDataMsgpackSerializer and ObjectDataMsgpackDeserializer
…ialization 1. Created ObjectMessageFixtures to represent dummy data in various formats 2. Added jackson-param dependency to fix jackson deserialization issue 3. Marked javaParameters as true on compilerOptions
1. Updated test for testOmitNullsInObjectMessageSerialization 2. Added test HandleNullsInObjectMessageDeserialization
WalkthroughThis change introduces comprehensive serialization and deserialization support for LiveObjects in both JSON and MessagePack formats. It adds interfaces, default implementations, helper classes, and custom serializers for handling arrays of live objects, and integrates these into protocol messages. Extensive unit tests and fixtures are included to verify correct serialization behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AblyRealtime
participant LiveObjectsHelper
participant LiveObjectSerializer
participant ProtocolMessage
Client->>AblyRealtime: Send/receive ProtocolMessage with state
AblyRealtime->>LiveObjectsHelper: getLiveObjectSerializer()
LiveObjectsHelper->>LiveObjectSerializer: (via reflection) instantiate if needed
AblyRealtime->>ProtocolMessage: Serialize/deserialize state field
ProtocolMessage->>LiveObjectSerializer: Serialize/deserialize ObjectMessage[] (JSON/MsgPack)
LiveObjectSerializer-->>ProtocolMessage: Return serialized/deserialized data
ProtocolMessage-->>AblyRealtime: Complete (de)serialization
AblyRealtime-->>Client: Return processed ProtocolMessage
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sacOO7
changed the title
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
f97b3e7 to
4c07a19
Compare
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
4c07a19 to
1d85b5c
Compare
1. Implemented manual msgpack serialization 2. Updated tests
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
1d85b5c to
8d23a1f
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: 5
🧹 Nitpick comments (4)
live-objects/build.gradle.kts (1)
14-14: Verify that the common bundle doesn’t reintroduce MessagePack dependencies
This PR’s goal is to remove MsgPack from the module; please double-check thatlibs.bundles.commonin your version catalog only contains the intended helper and JSON serialization libraries and does not transitively pull in anyorg.msgpackartifacts. If it does, explicitly exclude them:implementation(libs.bundles.common) { exclude(group = "org.msgpack", module = "msgpack-core") }live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt (1)
1-18: Consider narrowing the suppression scope.Suppressing
UNCHECKED_CASTat the file level is too broad. Consider suppressing only at specific unsafe cast locations after adding appropriate type checks.Apply this pattern to add type safety:
-@file:Suppress("UNCHECKED_CAST") - package io.ably.lib.objects.serialization // ... imports ... @Suppress("unused") // Used via reflection in LiveObjectsHelper internal class DefaultLiveObjectSerializer : LiveObjectSerializer {live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt (1)
49-66: Fix incorrect comment.The comment mentions MsgPack but the code performs JSON serialization.
- // Serialize the ProtocolMessage containing ObjectMessages to MsgPack format + // Serialize the ProtocolMessage containing ObjectMessages to JSON format val serializedProtoMsg = ProtocolSerializer.writeJSON(protocolMessage).toString(Charsets.UTF_8)live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixture.kt (1)
5-10: Remove redundant importsThe individual class imports on lines 6-10 are redundant since line 5 already imports all classes from
io.ably.lib.objects.*.import io.ably.lib.objects.* -import io.ably.lib.objects.Binary -import io.ably.lib.objects.ObjectData -import io.ably.lib.objects.ObjectMessage -import io.ably.lib.objects.ObjectState -import io.ably.lib.objects.ObjectValue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/src/main/java/io/ably/lib/objects/LiveObjectSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsJsonSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(2 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(5 hunks)lib/src/main/java/io/ably/lib/types/ProtocolSerializer.java(2 hunks)live-objects/build.gradle.kts(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt(5 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Serialization.kt(0 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(4 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixture.kt(1 hunks)
💤 Files with no reviewable changes (1)
- live-objects/src/main/kotlin/io/ably/lib/objects/Serialization.kt
🔇 Additional comments (18)
lib/src/main/java/io/ably/lib/objects/LiveObjectSerializer.java (1)
10-51: Excellent interface design for LiveObjects serialization.The interface is well-structured with:
- Clear separation between MessagePack and JSON serialization methods
- Proper nullability annotations ensuring type safety
- Appropriate exception declarations (IOException for MessagePack operations)
- Comprehensive Javadoc documentation
- Consistent method naming conventions
The design provides a clean abstraction for converting LiveObject arrays between different serialization formats.
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
42-56: Verify the impact of making Binary.data non-nullable.The change from
ByteArray?toByteArrayremoves null safety, which could be a breaking change. Ensure that:
- All existing code that creates Binary instances provides non-null data
- No existing code relies on the nullable behavior
- The serialization/deserialization logic handles this correctly
Run the following script to verify Binary usage across the codebase:
#!/bin/bash # Description: Check for Binary class usage and potential null handling # Search for Binary class instantiation echo "=== Binary class instantiation ===" rg -A 3 "Binary\(" --type kotlin --type java # Search for null checks on Binary.data echo "=== Null checks on Binary.data ===" rg "\.data\s*[!=]=\s*null" --type kotlin --type java # Search for Binary.data usage echo "=== Binary.data property usage ===" rg "\.data\b" --type kotlin --type java -A 2 -B 2lib/src/main/java/io/ably/lib/types/ProtocolSerializer.java (1)
31-41: I need to see how exceptions are propagated in the rest ofsend(...), so I’ll pull more lines around the method:#!/bin/bash # Show send() including its catch blocks rg -n "public void send" -A 60 lib/src/main/java/io/ably/lib/transport/WebSocketTransport.javalive-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
3-3: Test updates align well with the new serialization framework.The changes correctly update test data to match the new serialization approach:
- Using
JsonObjectinstead ofMapfor extras field aligns with the new JSON serialization- Simplified
Binarydata creation using string conversion- Import additions for required types
The test logic and size calculations remain consistent, ensuring backward compatibility verification.
Also applies to: 21-21, 37-40, 84-84
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
8-8: Excellent refactoring to centralize LiveObjects plugin initialization.This change improves code organization by:
- Moving reflection-based plugin loading to a dedicated
LiveObjectsHelperclass- Reducing complexity in the
AblyRealtimeconstructor- Centralizing LiveObjects-related initialization logic
- Maintaining the same functionality while improving separation of concerns
The refactoring enhances maintainability and makes the code more modular.
Also applies to: 75-75
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (3)
7-11: Imports look good.The new imports are appropriate for the LiveObjects serialization functionality.
131-138: State field declaration is well-designed.The nullable
statefield with custom JSON serialization adapter is properly documented and implemented.
274-283: MsgPack deserialization logic is correct.The implementation properly handles the case where
LiveObjectSerializeris unavailable by logging and skipping the value.live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt (1)
31-36: JSON array deserialization is well-implemented.Proper validation of JSON elements with appropriate error handling.
lib/src/main/java/io/ably/lib/objects/LiveObjectsJsonSerializer.java (1)
14-38: JSON serializer implementation is well-designed.The implementation properly handles cases where
LiveObjectSerializeris unavailable, with appropriate logging and fallback behavior.lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java (1)
13-25: Plugin initialization via reflection is well-implemented.Good error handling with appropriate logging for when the plugin is not available.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt (2)
19-47: MsgPack serialization test is well-implemented.Good test coverage with proper assertions for round-trip serialization.
68-180: Excellent test coverage for edge cases.The tests for null handling and enum serialization are comprehensive and well-structured.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixture.kt (1)
12-174: Well-structured test fixturesThe test fixture data is comprehensive and well-organized, providing good coverage for different data types (string, binary, number, boolean, JSON object, and JSON array). The factory functions follow a consistent pattern that makes the test data easy to understand and maintain.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (2)
6-11: Excellent serialization enhancementsThe addition of Gson annotations and custom serializers improves the robustness of the serialization framework:
- Custom serializers for
ObjectDataandBinaryprovide proper type-specific handling- Restricting
extrastoJsonObject?improves type safety- The
@SerializedName("object")annotation ensures correct JSON field mappingAlso applies to: 37-37, 221-222, 318-318, 334-334
227-228: Proper deprecation handlingGood practice marking
initialValueEncodingas deprecated with a clear migration message. This helps maintain backward compatibility while signaling future removal.live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt (1)
1-100: Well-implemented JSON serialization frameworkThe JSON serialization implementation is comprehensive and follows best practices:
- Custom enum serialization by code values ensures consistent behavior
- Polymorphic
ObjectDataserialization correctly handles all value types- Special JSON encoding handling (lines 54-58, 71-81) properly implements the specifications
- Base64 encoding for binary data is implemented correctly
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (1)
1-206: High-quality MessagePack serialization implementationThe MessagePack serialization is well-implemented with several notable strengths:
- Consistent null-safe field counting approach
- Memory-efficient string interning (e.g., line 107)
- Proper enum validation with descriptive error messages (lines 230-231, 489-490)
- Correct handling of JSON-encoded strings (lines 694-705)
- Comprehensive coverage of all object types
Also applies to: 232-709
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt
Outdated
Show resolved
Hide resolved
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt
Outdated
Show resolved
Hide resolved
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
Show resolved
Hide resolved
sacOO7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (1)
112-112: Consider removing unnecessary string interning.The
intern()calls on field names may add unnecessary overhead. Since these are typically short strings used for field matching in when statements, the benefits of interning are minimal.- val fieldName = unpacker.unpackString().intern() + val fieldName = unpacker.unpackString()Also applies to: 226-226, 327-327, 393-393, 437-437, 489-489, 542-542, 598-598, 678-678
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java(1 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(5 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.298Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
🔇 Additional comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (3)
26-88: Clean implementation of ObjectMessage serialization.The write/read functions properly handle optional fields and maintain symmetry between serialization and deserialization.
Also applies to: 90-145
209-269: Excellent handling of required fields.The implementation correctly validates that the required
actionfield is present, throwing a clear exception if it's missing. This ensures data integrity without masking potential protocol errors.
657-662: Well-designed JSON value handling.The approach of serializing JSON objects/arrays as strings with an encoding field is elegant and avoids nested complexity while maintaining type safety during deserialization.
Also applies to: 703-714
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
7755418 to
6f55903
Compare
1. Made LiveObjectSerializer variable thread safe inside LiveObjectsHelper 2. Added null check for MessageUnpacker inside readObjectMessage
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
6f55903 to
36f9844
Compare
sacOO7
changed the title
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
de8a957 to
284837a
Compare
sacOO7
force-pushed
the
chore/liveojects-serialization-without-msgpack-deps
branch
from
284837a to
6fa439d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (1)
12-15: Fix inconsistent documentation reference.The documentation mentions "Jackson and Gson" but the implementation only uses Gson for JSON operations. MessagePack operations use the msgpack-java library directly.
- * of ObjectMessage arrays for both JSON and MessagePack formats using Jackson and Gson. + * of ObjectMessage arrays for both JSON and MessagePack formats using Gson and msgpack-java.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(4 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (2)
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: build
🔇 Additional comments (5)
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (5)
1-1: LGTM: Appropriate suppression for intentional unsafe casting.The file-level suppression is correctly applied given the intentional unsafe casting used in the implementation methods, as confirmed by the retrieved learnings.
20-23: LGTM: Clean MessagePack array deserialization.The method correctly unpacks the array header and creates an array of ObjectMessage instances using the appropriate helper method.
25-29: LGTM: Intentional unsafe casting as per design decision.The unsafe casting on line 26 is intentional according to the retrieved learnings, allowing ClassCastException to occur naturally for type mismatches while keeping the dynamically-loaded class simple.
31-36: LGTM: Robust JSON array deserialization with proper error handling.The method correctly handles JSON array parsing with appropriate error checking for non-JsonObject elements, providing clear error messages for debugging.
38-46: LGTM: Consistent JSON array serialization approach.The method follows the same intentional unsafe casting pattern as
writeMsgpackArrayand correctly converts ObjectMessage instances to JsonObjects.
| return out.toByteArray(); | ||
| } catch(IOException e) { return null; } | ||
| } catch (IOException ioe) { | ||
| throw AblyException.fromThrowable(ioe); |
There was a problem hiding this comment.
Could you double-check that we won't introduce regression here
There was a problem hiding this comment.
Actually, there's no point in returning null when writeMsgpack fails.
Catching error silently means although our send/publish operation failed, we are not returning any error for the operation. This is not expected ( even from customer use-case point of view ) and relevant error should be thrown to understand and fix the failed operation.
PS. I faced similar issue when writeMsgpack failed for publish op. and was not able to trace the error thrown for the same.
There was a problem hiding this comment.
Also, we already throw error for similar readMsgpack method =>
ably-java/lib/src/main/java/io/ably/lib/types/ProtocolSerializer.java
Lines 18 to 25 in 80a655e
Seems, the relevant code for the
writeMsgpack method wasn't updated.
|
Closing in favor of #1109 |
Fixed #1098
Decided to have separate serialization over just single gson serialization because =>
msgpack, needs two layers of serialization, first is always json and second is tomsgpack.binaryfield )ObjectMessageencoding and decoding specification#335 (comment)Summary by CodeRabbit
New Features
Improvements
Tests
Chores