[ECO-5386] Liveobjects serialization using msgpack-jackson #1101
Conversation
WalkthroughThis change introduces comprehensive serialization and deserialization support for LiveObjects within the Ably Java library. It adds new serializers for both JSON and MessagePack formats, integrates them into protocol and object message classes, updates build dependencies, and provides extensive unit tests and fixtures to ensure correct round-trip serialization of LiveObjects data structures. Changes
Sequence Diagram(s)sequenceDiagram
participant AblyRealtime
participant LiveObjectsHelper
participant LiveObjectSerializer
participant ProtocolMessage
participant MsgPack/JSON
AblyRealtime->>LiveObjectsHelper: tryInitializeLiveObjectsPlugin(this)
LiveObjectsHelper->>LiveObjectSerializer: getLiveObjectSerializer()
ProtocolMessage->>LiveObjectSerializer: serialize/deserialize state (array)
LiveObjectSerializer->>MsgPack/JSON: Perform (de)serialization
MsgPack/JSON-->>LiveObjectSerializer: Serialized/Deserialized data
LiveObjectSerializer-->>ProtocolMessage: Object[] state
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
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
force-pushed
the
chore/liveobjects-serialization
branch
from
0592f72 to
ec2c2d3
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
ec2c2d3 to
6a4e367
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
6a4e367 to
9c79bbd
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
9c79bbd to
d227718
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
42ca7a6 to
564a31b
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
564a31b to
83ff3c0
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
83ff3c0 to
9d19c2b
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
10a09e3 to
e3d62d9
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
c4f91d5 to
b32e76f
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
b32e76f to
86c746e
Compare
…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
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
86c746e to
7e4f5d6
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
ef65124 to
ee6b90f
Compare
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
ee6b90f to
b68d962
Compare
1. Updated test for testOmitNullsInObjectMessageSerialization 2. Added test HandleNullsInObjectMessageDeserialization
sacOO7
force-pushed
the
chore/liveobjects-serialization
branch
from
b68d962 to
792ffc9
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: 9
🧹 Nitpick comments (5)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
203-211: Robust MessagePack serialization with proper error handling.The MessagePack serialization and deserialization logic for the
statefield includes appropriate null checks and error handling when the LiveObjectSerializer is unavailable. The warning logs provide helpful debugging information.Consider caching the
LiveObjectSerializerinstance to avoid repeated lookups:+ private static LiveObjectSerializer cachedSerializer; + + private static LiveObjectSerializer getOrCreateSerializer() { + if (cachedSerializer == null) { + cachedSerializer = LiveObjectsHelper.getLiveObjectSerializer(); + } + return cachedSerializer; + }Then use
getOrCreateSerializer()instead of repeatedLiveObjectsHelper.getLiveObjectSerializer()calls.Also applies to: 274-282
lib/src/main/java/io/ably/lib/objects/LiveObjectsJsonSerializer.java (1)
14-17: Consider lazy initialization of the serializer field.Storing the
LiveObjectSerializerin an instance field during construction could lead to issues if the serializer becomes unavailable later or if this class is instantiated before the LiveObjects plugin is properly initialized.Consider lazy initialization:
- private final LiveObjectSerializer serializer = LiveObjectsHelper.getLiveObjectSerializer(); + private LiveObjectSerializer getSerializer() { + return LiveObjectsHelper.getLiveObjectSerializer(); + }Then replace
serializerreferences withgetSerializer()calls.lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java (1)
13-25: Consider caching the plugin instance or making the method more robust.The
tryInitializeLiveObjectsPluginmethod creates a new plugin instance on every call, which could be inefficient. Also, consider whether the method should handle the case whereablyRealtimeis null.Consider adding a null check for the
ablyRealtimeparameter and potentially caching the plugin instance if it's meant to be reused.live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt (1)
55-56: Fix incorrect comment.The comment says "MsgPack format" but the code is actually serializing to JSON format.
- // Serialize the ProtocolMessage containing ObjectMessages to MsgPack format + // Serialize the ProtocolMessage containing ObjectMessages to JSON formatlive-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (1)
33-36: Remove redundant inline comment.The implementation is correct, but the inline comment is unnecessary as it merely restates what the code does.
internal fun ObjectMessage.writeTo(packer: MessagePacker) { - val msgpackBytes = msgpackMapper.writeValueAsBytes(this) // returns correct msgpack map structure + val msgpackBytes = msgpackMapper.writeValueAsBytes(this) packer.writePayload(msgpackBytes) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
gradle/libs.versions.toml(2 hunks)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(2 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 (30)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (3)
3-3: LGTM - Import addition supports type migration.The JsonObject import is necessary for the extras field type change from Map to JsonObject.
84-84: Verify the test data change aligns with the new Binary constructor.The initialValue has been changed from a binary representation of "initial" to "some-value" as a string converted to ByteArray. This change should align with the Binary class constructor changes that now require non-nullable ByteArray.
Ensure this change doesn't affect the test's intent or size calculations.
37-40: Verify the JSON serialization size calculation.The extras field has been changed from Map to JsonObject with explicit property additions. The comment indicates the total size should be 26 bytes for the JSON
{"meta":"data","count":42}, which seems reasonable.However, ensure that the Gson serialization of this JsonObject produces the expected byte length when converted to UTF-8.
#!/bin/bash # Verify JsonObject serialization behavior in tests ast-grep --pattern $'JsonObject().apply { $$$ }'lib/src/main/java/io/ably/lib/types/ProtocolSerializer.java (2)
31-31: Improved error handling with proper exception declaration.The method signature change to declare
AblyExceptionimproves the API contract by making potential exceptions explicit.
38-40: Enhanced error propagation replaces null return.The change from returning
nullonIOExceptionto throwingAblyExceptionprovides better error handling and consistency with thereadMsgpackmethod. This ensures callers can properly handle serialization failures instead of dealing with null return values.live-objects/build.gradle.kts (3)
14-14: LGTM - Common bundle dependency addition.The common bundle dependency supports shared functionality across the live-objects module.
16-17: Essential dependencies for Jackson serialization support.The Jackson MessagePack and parameter names modules are necessary for the new serialization framework. These dependencies enable:
- MessagePack format serialization/deserialization
- Automatic JSON property to constructor parameter mapping without explicit annotations
51-57: Well-documented compiler option for Jackson integration.The
javaParameters = trueoption enables Jackson to map JSON properties to constructor parameters without@JsonPropertyannotations. The comment correctly explains that this adds parameter metadata to bytecode while maintaining binary compatibility.live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (4)
42-42: Improved API design with non-nullable ByteArray.Changing the constructor to require a non-nullable
ByteArraysimplifies theBinaryclass design and eliminates potential null pointer issues. This is a good API improvement that makes the data contract clearer.
46-46: Simplified equals method after nullable removal.The removal of null checks in the
equalsmethod is consistent with the non-nullableByteArrayconstructor change. The direct use ofcontentEqualsis now safe and more concise.
50-50: Consistent hashCode implementation.The
hashCodemethod correctly usescontentHashCode()directly without null checks, consistent with the non-nullable data field.
55-55: Simplified size calculation.The
size()extension function now directly returnsdata.sizewithout null checks, which is consistent with the non-nullableByteArraydesign.lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (2)
8-8: Good refactoring to centralize LiveObjects initialization.The import change to
LiveObjectsHelpersupports the architectural improvement of centralizing live objects plugin management in a dedicated helper class.
75-75: Improved separation of concerns with helper delegation.Delegating the live objects plugin initialization to
LiveObjectsHelper.tryInitializeLiveObjectsPlugin(this)is a good architectural decision. This removes reflection logic from the mainAblyRealtimeclass and centralizes it in a specialized helper, improving maintainability and code organization.lib/src/main/java/io/ably/lib/objects/LiveObjectSerializer.java (1)
1-51: Well-designed interface with clear separation of concerns.The
LiveObjectSerializerinterface provides a clean abstraction for live object serialization with proper method signatures, comprehensive documentation, and appropriate nullability annotations. The separation between MessagePack and JSON serialization methods is logical and maintainable.gradle/libs.versions.toml (1)
28-29: ```shell
#!/bin/bashVerify latest versions of the two Jackson libraries
echo "Latest jackson-dataformat-msgpack version:"
curl -s "https://repo1.maven.org/maven2/org/msgpack/jackson-dataformat-msgpack/maven-metadata.xml"
| grep -oP "(?<=)[^<]+" | sort -V | tail -n1echo "Latest jackson-module-parameter-names version:"
curl -s "https://repo1.maven.org/maven2/com/fasterxml/jackson/module/jackson-module-parameter-names/maven-metadata.xml"
| grep -oP "(?<=)[^<]+" | sort -V | tail -n1</details> <details> <summary>lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)</summary> `7-11`: **Well-integrated state field with proper annotations.** The addition of the `state` field with custom JSON adapter is well-implemented. The nullable annotation and comprehensive documentation provide clear expectations for usage. Also applies to: 131-138 </details> <details> <summary>lib/src/main/java/io/ably/lib/objects/LiveObjectsJsonSerializer.java (1)</summary> `18-28`: **Excellent error handling and validation logic.** The implementation provides robust error handling for both serialization and deserialization cases, with appropriate validation of JSON input and proper fallback behavior when the serializer is unavailable. Also applies to: 30-37 </details> <details> <summary>live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt (1)</summary> `19-22`: **Well-implemented deserialization methods with proper error handling.** The deserialization methods provide good error handling and use appropriate Kotlin idioms for array processing. The JSON deserialization includes proper type validation for individual elements. Also applies to: 30-35 </details> <details> <summary>live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt (2)</summary> `30-47`: **Well-structured test with good round-trip validation.** The test properly validates round-trip serialization fidelity using MsgPack format and checks each ObjectMessage instance against its expected value. --- `68-101`: **Excellent test coverage for null field omission.** This test comprehensively validates that null fields are properly omitted during serialization, which is important for maintaining clean serialized output and reducing payload size. </details> <details> <summary>live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixture.kt (1)</summary> `1-174`: **Well-designed test fixtures with comprehensive data type coverage.** The fixture file provides excellent coverage of different data types and scenarios for testing ObjectMessage serialization. The use of immutable data structures and consistent naming conventions makes it easy to understand and maintain. </details> <details> <summary>live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt (1)</summary> `71-81`: **Robust JSON parsing with proper error handling.** The JSON string parsing logic correctly handles both JsonObject and JsonArray cases with appropriate error handling for invalid JSON strings. </details> <details> <summary>live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (4)</summary> `330-332`: **Good type safety improvement for extras field.** Changing the `extras` field type from `Any?` to `JsonObject?` provides better type safety and aligns with the JSON-encodable object specification mentioned in the documentation. --- `45-47`: **Appropriate use of multiple serialization frameworks.** The use of both Gson and Jackson annotations allows the same data classes to work with both serialization frameworks, which is a good design for flexibility. --- `239-240`: **Well-documented deprecation.** The deprecation of `initialValueEncoding` is properly documented with a clear explanation of the future direction. --- `348-350`: **Correct property mapping for objectState field.** The use of both `@SerializedName("object")` and `@JsonProperty("object")` properly maps the `objectState` field to the JSON property name "object" for both Gson and Jackson serialization frameworks. </details> <details> <summary>live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (3)</summary> `24-31`: **Well-configured MessagePack mapper with appropriate settings.** The mapper configuration is well-thought-out with NON_NULL inclusion, enum ordinal serialization, and parameter names module for proper constructor-based deserialization. The singleton pattern ensures efficient reuse. --- `38-44`: **Excellent resource management and conversion logic.** The use of Kotlin's `use` function ensures proper resource cleanup of the buffer packer, and the two-step conversion process correctly handles MessagePack value to object mapping. --- `93-103`: **Clean binary serialization implementation.** The Binary serializer and deserializer correctly handle raw binary data with minimal overhead. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| override fun writeMsgpackArray(objects: Array<out Any>?, packer: MessagePacker) { | ||
| val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage> | ||
| packer.packArrayHeader(objectMessages.size) | ||
| objectMessages.forEach { it.writeTo(packer) } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for unsafe casting operations.
The unsafe casting from Array<out Any>? to Array<ObjectMessage> could cause ClassCastException at runtime if incorrect types are passed.
Add proper type validation:
override fun writeMsgpackArray(objects: Array<out Any>?, packer: MessagePacker) {
- val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage>
+ val objectMessages: Array<ObjectMessage> = objects?.let { array ->
+ if (array.all { it is ObjectMessage }) {
+ @Suppress("UNCHECKED_CAST")
+ array as Array<ObjectMessage>
+ } else {
+ throw IllegalArgumentException("All elements must be ObjectMessage instances")
+ }
+ } ?: emptyArray()
packer.packArrayHeader(objectMessages.size)
objectMessages.forEach { it.writeTo(packer) }
}📝 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.
| override fun writeMsgpackArray(objects: Array<out Any>?, packer: MessagePacker) { | |
| val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage> | |
| packer.packArrayHeader(objectMessages.size) | |
| objectMessages.forEach { it.writeTo(packer) } | |
| } | |
| override fun writeMsgpackArray(objects: Array<out Any>?, packer: MessagePacker) { | |
| - val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage> | |
| + val objectMessages: Array<ObjectMessage> = objects?.let { array -> | |
| + if (array.all { it is ObjectMessage }) { | |
| + @Suppress("UNCHECKED_CAST") | |
| + array as Array<ObjectMessage> | |
| + } else { | |
| + throw IllegalArgumentException("All elements must be ObjectMessage instances") | |
| + } | |
| + } ?: emptyArray() | |
| packer.packArrayHeader(objectMessages.size) | |
| objectMessages.forEach { it.writeTo(packer) } | |
| } |
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt
around lines 24 to 28, the code unsafely casts an Array of Any to Array of
ObjectMessage, risking a ClassCastException. Add a type check to verify each
element is an ObjectMessage before casting or processing. If any element is not
an ObjectMessage, handle the error appropriately, such as throwing an
informative exception or skipping invalid elements, to ensure safe operation.
| override fun asJsonArray(objects: Array<out Any>?): JsonArray { | ||
| val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage> | ||
| val jsonArray = JsonArray() | ||
| for (objectMessage in objectMessages) { | ||
| jsonArray.add(objectMessage.toJsonObject()) | ||
| } | ||
| return jsonArray | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply similar type safety improvements to JSON serialization.
The same type safety concern applies to the asJsonArray method.
override fun asJsonArray(objects: Array<out Any>?): JsonArray {
- val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage>
+ val objectMessages: Array<ObjectMessage> = objects?.let { array ->
+ if (array.all { it is ObjectMessage }) {
+ @Suppress("UNCHECKED_CAST")
+ array as Array<ObjectMessage>
+ } else {
+ throw IllegalArgumentException("All elements must be ObjectMessage instances")
+ }
+ } ?: emptyArray()
val jsonArray = JsonArray()
for (objectMessage in objectMessages) {
jsonArray.add(objectMessage.toJsonObject())
}
return jsonArray
}📝 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.
| override fun asJsonArray(objects: Array<out Any>?): JsonArray { | |
| val objectMessages: Array<ObjectMessage> = objects as Array<ObjectMessage> | |
| val jsonArray = JsonArray() | |
| for (objectMessage in objectMessages) { | |
| jsonArray.add(objectMessage.toJsonObject()) | |
| } | |
| return jsonArray | |
| } | |
| override fun asJsonArray(objects: Array<out Any>?): JsonArray { | |
| val objectMessages: Array<ObjectMessage> = objects?.let { array -> | |
| if (array.all { it is ObjectMessage }) { | |
| @Suppress("UNCHECKED_CAST") | |
| array as Array<ObjectMessage> | |
| } else { | |
| throw IllegalArgumentException("All elements must be ObjectMessage instances") | |
| } | |
| } ?: emptyArray() | |
| val jsonArray = JsonArray() | |
| for (objectMessage in objectMessages) { | |
| jsonArray.add(objectMessage.toJsonObject()) | |
| } | |
| return jsonArray | |
| } |
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt
around lines 37 to 44, the asJsonArray method unsafely casts the input array to
Array<ObjectMessage> without checking its type, risking a ClassCastException.
Modify the method to safely handle the input by verifying the type of each
element before casting or by using a safer approach such as filtering or mapping
only valid ObjectMessage instances to JSON, ensuring type safety during JSON
serialization.
| public static LiveObjectSerializer getLiveObjectSerializer() { | ||
| if (liveObjectSerializer == null) { | ||
| try { | ||
| Class<?> serializerClass = Class.forName("io.ably.lib.objects.serialization.DefaultLiveObjectSerializer"); | ||
| liveObjectSerializer = (LiveObjectSerializer) serializerClass.getDeclaredConstructor().newInstance(); | ||
| } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException | | ||
| InvocationTargetException e) { | ||
| Log.e(TAG, "Failed to init LiveObjectSerializer, LiveObjects plugin not included in the classpath", e); | ||
| return null; | ||
| } | ||
| } | ||
| return liveObjectSerializer; | ||
| } |
There was a problem hiding this comment.
Fix thread safety issue in singleton pattern.
The singleton implementation for liveObjectSerializer is not thread-safe. Multiple threads could simultaneously check the null condition and create multiple instances, violating the singleton pattern.
public static LiveObjectSerializer getLiveObjectSerializer() {
- if (liveObjectSerializer == null) {
+ if (liveObjectSerializer == null) {
+ synchronized (LiveObjectsHelper.class) {
+ if (liveObjectSerializer == null) {
try {
Class<?> serializerClass = Class.forName("io.ably.lib.objects.serialization.DefaultLiveObjectSerializer");
liveObjectSerializer = (LiveObjectSerializer) serializerClass.getDeclaredConstructor().newInstance();
} catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException |
InvocationTargetException e) {
Log.e(TAG, "Failed to init LiveObjectSerializer, LiveObjects plugin not included in the classpath", e);
return null;
}
+ }
+ }
}
return liveObjectSerializer;
}📝 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.
| public static LiveObjectSerializer getLiveObjectSerializer() { | |
| if (liveObjectSerializer == null) { | |
| try { | |
| Class<?> serializerClass = Class.forName("io.ably.lib.objects.serialization.DefaultLiveObjectSerializer"); | |
| liveObjectSerializer = (LiveObjectSerializer) serializerClass.getDeclaredConstructor().newInstance(); | |
| } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException | | |
| InvocationTargetException e) { | |
| Log.e(TAG, "Failed to init LiveObjectSerializer, LiveObjects plugin not included in the classpath", e); | |
| return null; | |
| } | |
| } | |
| return liveObjectSerializer; | |
| } | |
| public static LiveObjectSerializer getLiveObjectSerializer() { | |
| if (liveObjectSerializer == null) { | |
| synchronized (LiveObjectsHelper.class) { | |
| if (liveObjectSerializer == null) { | |
| try { | |
| Class<?> serializerClass = Class.forName("io.ably.lib.objects.serialization.DefaultLiveObjectSerializer"); | |
| liveObjectSerializer = (LiveObjectSerializer) serializerClass.getDeclaredConstructor().newInstance(); | |
| } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException | | |
| InvocationTargetException e) { | |
| Log.e(TAG, "Failed to init LiveObjectSerializer, LiveObjects plugin not included in the classpath", e); | |
| return null; | |
| } | |
| } | |
| } | |
| } | |
| return liveObjectSerializer; | |
| } |
🤖 Prompt for AI Agents
In lib/src/main/java/io/ably/lib/objects/LiveObjectsHelper.java around lines 27
to 39, the singleton pattern for liveObjectSerializer is not thread-safe,
allowing multiple threads to create separate instances. To fix this, synchronize
the block that checks and initializes liveObjectSerializer or use a thread-safe
lazy initialization approach such as double-checked locking to ensure only one
instance is created even under concurrent access.
| } | ||
| obj.has("string") -> ObjectValue(obj.get("string").asString) | ||
| obj.has("number") -> ObjectValue(obj.get("number").asDouble) | ||
| obj.has("bytes") -> ObjectValue(Binary(Base64.getDecoder().decode(obj.get("bytes").asString))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Base64 decoding.
Base64 decoding operations should handle potential IllegalArgumentException that can be thrown for invalid Base64 input.
obj.has("bytes") -> ObjectValue(Binary(Base64.getDecoder().decode(obj.get("bytes").asString)))Consider wrapping in try-catch:
obj.has("bytes") -> {
+ try {
ObjectValue(Binary(Base64.getDecoder().decode(obj.get("bytes").asString)))
+ } catch (e: IllegalArgumentException) {
+ throw JsonParseException("Invalid base64 encoding in bytes field", e)
+ }
}Apply similar handling to the InitialValueJsonSerializer.deserialize method at line 97.
Also applies to: 97-97
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt
at lines 84 and 97, the Base64 decoding calls do not handle potential
IllegalArgumentException from invalid Base64 input. Wrap the
Base64.getDecoder().decode calls in try-catch blocks to catch
IllegalArgumentException and handle it appropriately, such as logging the error
or throwing a custom exception, to prevent crashes and improve robustness.
| override fun deserialize(json: JsonElement, typeOfT: Type, context: JsonDeserializationContext): T { | ||
| val code = json.asInt | ||
| return enumValues.first { getCode(it) == code } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for enum deserialization.
The enum deserialization could throw NoSuchElementException if no matching enum value is found for the given code, which would result in an unhelpful error message.
override fun deserialize(json: JsonElement, typeOfT: Type, context: JsonDeserializationContext): T {
val code = json.asInt
- return enumValues.first { getCode(it) == code }
+ return enumValues.firstOrNull { getCode(it) == code }
+ ?: throw JsonParseException("Unknown enum code: $code for type $typeOfT")
}📝 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.
| override fun deserialize(json: JsonElement, typeOfT: Type, context: JsonDeserializationContext): T { | |
| val code = json.asInt | |
| return enumValues.first { getCode(it) == code } | |
| } | |
| override fun deserialize(json: JsonElement, typeOfT: Type, context: JsonDeserializationContext): T { | |
| val code = json.asInt | |
| return enumValues.firstOrNull { getCode(it) == code } | |
| ?: throw JsonParseException("Unknown enum code: $code for type $typeOfT") | |
| } |
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt
around lines 37 to 40, the deserialize method can throw a NoSuchElementException
if no enum matches the given code. To fix this, add error handling by catching
this exception and throwing a more descriptive exception or returning a default
value. This will provide clearer error messages when deserialization fails due
to an unknown code.
| node.has("string") -> ObjectValue(node.get("string").asText()) | ||
| node.has("number") -> ObjectValue(node.get("number").doubleValue()) | ||
| node.has("bytes") -> ObjectValue(Binary(node.get("bytes").binaryValue())) | ||
| else -> throw IllegalArgumentException("ObjectData must have one of the fields: boolean, string, number, or bytes") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete error message for deserialization failure.
The error message doesn't mention the JSON encoding scenario, which could confuse users when debugging.
- else -> throw IllegalArgumentException("ObjectData must have one of the fields: boolean, string, number, or bytes")
+ else -> throw IllegalArgumentException("ObjectData must have one of the fields: boolean, string (with optional encoding), number, or bytes")📝 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.
| else -> throw IllegalArgumentException("ObjectData must have one of the fields: boolean, string, number, or bytes") | |
| else -> throw IllegalArgumentException("ObjectData must have one of the fields: boolean, string (with optional encoding), number, or bytes") |
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
at line 87, the IllegalArgumentException message for deserialization failure is
incomplete and does not mention the JSON encoding scenario. Update the error
message to explicitly include that the ObjectData must have one of the fields:
boolean, string, number, bytes, or JSON encoding, so users can better understand
the cause during debugging.
| val parsed = JsonParser.parseString(jsonStr) | ||
| ObjectValue( | ||
| when { | ||
| parsed.isJsonObject -> parsed.asJsonObject | ||
| parsed.isJsonArray -> parsed.asJsonArray | ||
| else -> throw IllegalArgumentException("Invalid JSON string for encoding=json") | ||
| } | ||
| ) |
There was a problem hiding this comment.
Handle all valid JSON types when encoding="json".
The current implementation only accepts JSON objects and arrays, but valid JSON can also be primitives (string, number, boolean, null). Consider supporting all JSON value types or document why only objects/arrays are supported.
If only objects/arrays are intended:
- else -> throw IllegalArgumentException("Invalid JSON string for encoding=json")
+ else -> throw IllegalArgumentException("JSON encoding only supports objects and arrays, not primitive values")If all JSON types should be supported, additional handling would be needed for primitive JSON values.
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
around lines 75 to 82, the code currently only handles JSON objects and arrays
but ignores valid JSON primitives like strings, numbers, booleans, and null. To
fix this, extend the when clause to detect and properly handle these primitive
JSON types by converting them into appropriate ObjectValue representations or
document clearly why only objects and arrays are supported if that is the
intended behavior.
| internal class JsonObjectMsgpackDeserializer : com.fasterxml.jackson.databind.JsonDeserializer<JsonObject>() { | ||
| override fun deserialize(p: com.fasterxml.jackson.core.JsonParser, ctxt: DeserializationContext): JsonObject { | ||
| return Serialisation.msgpackToGson(p.binaryValue) as JsonObject | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe cast to JsonObject without type validation.
The deserializer assumes msgpackToGson returns a JsonObject, but it could return other JSON types (JsonArray, JsonPrimitive, etc.), leading to a ClassCastException.
Add type validation before casting:
internal class JsonObjectMsgpackDeserializer : com.fasterxml.jackson.databind.JsonDeserializer<JsonObject>() {
override fun deserialize(p: com.fasterxml.jackson.core.JsonParser, ctxt: DeserializationContext): JsonObject {
- return Serialisation.msgpackToGson(p.binaryValue) as JsonObject
+ val element = Serialisation.msgpackToGson(p.binaryValue)
+ if (element is JsonObject) {
+ return element
+ } else {
+ throw IllegalArgumentException("Expected JsonObject but got ${element?.javaClass?.simpleName}")
+ }
}
}📝 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.
| internal class JsonObjectMsgpackDeserializer : com.fasterxml.jackson.databind.JsonDeserializer<JsonObject>() { | |
| override fun deserialize(p: com.fasterxml.jackson.core.JsonParser, ctxt: DeserializationContext): JsonObject { | |
| return Serialisation.msgpackToGson(p.binaryValue) as JsonObject | |
| } | |
| } | |
| internal class JsonObjectMsgpackDeserializer : com.fasterxml.jackson.databind.JsonDeserializer<JsonObject>() { | |
| override fun deserialize(p: com.fasterxml.jackson.core.JsonParser, ctxt: DeserializationContext): JsonObject { | |
| val element = Serialisation.msgpackToGson(p.binaryValue) | |
| if (element is JsonObject) { | |
| return element | |
| } else { | |
| throw IllegalArgumentException("Expected JsonObject but got ${element.javaClass.simpleName}") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
around lines 111 to 115, the code unsafely casts the result of msgpackToGson to
JsonObject without verifying its actual type, risking a ClassCastException.
Modify the deserialize method to first check if the returned value is an
instance of JsonObject before casting. If it is not, handle the case
appropriately, such as throwing a descriptive exception or returning a default
value, to ensure type safety.
| internal class ObjectDataMsgpackSerializer : com.fasterxml.jackson.databind.JsonSerializer<ObjectData>() { | ||
| override fun serialize(value: ObjectData, gen: JsonGenerator, serializers: SerializerProvider) { | ||
| gen.writeStartObject() | ||
| value.objectId?.let { gen.writeStringField("objectId", it) } | ||
| value.value?.let { v -> | ||
| when (val data = v.value) { | ||
| is Boolean -> gen.writeBooleanField("boolean", data) | ||
| is String -> gen.writeStringField("string", data) | ||
| is Number -> gen.writeNumberField("number", data.toDouble()) | ||
| is Binary -> gen.writeBinaryField("bytes", data.data) | ||
| is JsonObject, is JsonArray -> { | ||
| gen.writeStringField("string", data.toString()) | ||
| gen.writeStringField("encoding", "json") | ||
| } | ||
| } | ||
| } | ||
| gen.writeEndObject() | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential precision loss when converting Number to Double.
The serializer converts all Number types to Double, which could lose precision for Long values that exceed Double's precision range (values beyond ±2^53).
Consider preserving the numeric type or using a more appropriate representation:
- is Number -> gen.writeNumberField("number", data.toDouble())
+ is Number -> gen.writeNumberField("number", data)Note: If the protocol specifically requires doubles, document this limitation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
between lines 46 and 64, the serialization converts all Number types to Double,
risking precision loss for Long values exceeding Double's precision range. To
fix this, modify the serialization logic to detect the specific Number subtype
and serialize it accordingly, preserving the original numeric type (e.g.,
writeLongField for Long, writeIntField for Int) instead of always converting to
Double. If the protocol mandates Double, add a comment documenting this
limitation clearly.
|
Closing in favor of #1106 and https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1750676723112209 |
sacOO7
changed the title
|
Related issue was #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
Bug Fixes
Tests
Chores