[ECO-5426][LiveObjects] Handle incoming object messages#1113
[ECO-5426][LiveObjects] Handle incoming object messages#1113
Conversation
## Walkthrough
This change implements the core synchronization and operation handling logic for Ably LiveObjects, including object sync, state management, and operation application for LiveMap and LiveCounter types. It introduces multiple new internal classes, updates interfaces, adds error handling, and provides comprehensive unit tests for all major components and flows.
## Changes
| Files / Groups | Change Summary |
|-------------------------------------------------------------------------------|---------------|
| `lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java`, `ChannelBase.java`, `LiveCounter.java` | Added `handleStateChange` to `LiveObjectsPlugin` and integrated calls in channel state changes; changed `LiveCounter.value()` return type to `Number`. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt`, `DefaultLiveObjectsPlugin.kt` | Implemented async, sequential message processing, state management, and channel state change handling in `DefaultLiveObjects` and plugin. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ErrorCodes.kt`, `Utils.kt` | Added new error codes and helper for object-related errors. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt` | Added extension to set channel serial from protocol messages. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ObjectId.kt` | Introduced `ObjectId` class with parsing and stringification. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt` | Added `Unknown` enum values, new extension for object data validation, and doc clarifications. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt` | Added `ObjectsManager` for sync, buffering, and applying object messages. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt` | Added `ObjectsPool` for managing live objects and GC logic. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsSyncTracker.kt` | Added `ObjectsSyncTracker` for sync sequence tracking. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt`, `MsgpackSerialization.kt` | Improved error handling and fallback for unknown enum codes in deserialization; simplified JSON encoding/decoding. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt` | Introduced `BaseLiveObject` abstract class and `ObjectType` enum. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt`, `LiveCounterManager.kt` | Added internal `DefaultLiveCounter` class and manager for state/operation logic. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt`, `LiveMapManager.kt` | Added internal `DefaultLiveMap` class and manager for map state/operation logic. |
| `live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt`| Added `LiveMapEntry` data class and extension functions for tombstone and GC logic. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt`, `TestHelpers.kt` | Added test helpers for reflection, mocking, and dependency injection. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectIdTest.kt`, `ObjectsSyncTrackerTest.kt` | Added unit tests for `ObjectId` and `ObjectsSyncTracker`. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt`, `ObjectMessageSizeTest.kt` | Adjusted imports and literals in serialization tests. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/unit/RealtimeObjectsTest.kt` | Renamed test class for clarity. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt`, `ObjectsManagerTest.kt`, `ObjectsPoolTest.kt` | Added comprehensive tests for live objects orchestration, sync, and pool logic. |
| `live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt`, `livecounter/DefaultLiveCounterTest.kt`, `livecounter/LiveCounterManagerTest.kt`, `livemap/DefaultLiveMapTest.kt`, `livemap/LiveMapManagerTest.kt` | Added detailed unit tests for all live object types and their managers. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant ChannelBase
participant LiveObjectsPlugin
participant DefaultLiveObjects
participant ObjectsManager
participant ObjectsPool
ChannelBase->>LiveObjectsPlugin: handleStateChange(channelName, state, hasObjects)
LiveObjectsPlugin->>DefaultLiveObjects: handleStateChange(state, hasObjects)
DefaultLiveObjects->>ObjectsManager: startNewSync() / endSync()
DefaultLiveObjects->>ObjectsPool: resetToInitialPool() / clearObjectsData()
DefaultLiveObjects->>ObjectsManager: handleObjectMessages() / handleObjectSyncMessages()sequenceDiagram
participant DefaultLiveObjects
participant ObjectsManager
participant ObjectsPool
participant BaseLiveObject
DefaultLiveObjects->>ObjectsManager: handleObjectMessages(list)
ObjectsManager->>ObjectsPool: get(objectId) or createZeroValueObjectIfNotExists()
ObjectsPool->>BaseLiveObject: applyObjectSync() / applyObject()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
|
sacOO7
changed the title
sacOO7
force-pushed
the
feature/object-sync
branch
from
d57e57b to
a67ea0c
Compare
sacOO7
force-pushed
the
feature/object-sync
branch
from
1449ae8 to
38a7c34
Compare
context engineered prompts 1. Fixed build issues for some of the code. 2. Added spec annoations for code / code blocks
sacOO7
force-pushed
the
feature/object-sync
branch
from
38a7c34 to
632a518
Compare
1. Added missing error codes and updated error handling for the same 2. Created separate class for ObjectId with static constructor
sacOO7
changed the title
… management - Added impl. for handling channel attached state - Updated ObjectSyncTracker along with related unit tests
sacOO7
force-pushed
the
feature/object-sync
branch
from
aa0fd50 to
557dac6
Compare
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Looks very good for me, added couple of minor suggestions
| throw objectError("Invalid object id: $objectId") | ||
| } | ||
|
|
||
| val typeStr = parts[0] |
There was a problem hiding this comment.
small thing but you can do like this:
val (typeStr, rest) = parts| /** | ||
| * Creates ObjectId instance from hashed object id string. | ||
| */ | ||
| fun fromString(objectId: String?): ObjectId { |
There was a problem hiding this comment.
I would make objectId non-nullable here, if we send null, this means something went wrong somewhere before and it always better to throw earlier
| } | ||
|
|
||
| internal fun LiveObjectsAdapter.setChannelSerial(channelName: String, protocolMessage: ProtocolMessage) { | ||
| if (protocolMessage.action == ProtocolMessage.Action.`object`) { |
There was a problem hiding this comment.
I would suggest early return to reduce nesting level
… Long - Fixed serialization for those types, from double to long - Added mocks for LiveMap and LiveCounter to TestHelpers - Added tests for LiveMap and LiveCounter
- Added LiveMap/LiveCounter level objectstate validation - Updated enums to include unknown type to skip processing of unknown actions - Added more unit tests for LiveMap and LiveCounter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectId.kt (1)
21-24: Consider making objectId parameter non-nullable.Based on the past review feedback, consider making the
objectIdparameter non-nullable to fail fast if null is passed. This would help catch issues earlier in the call chain.-fun fromString(objectId: String): ObjectId { - if (objectId.isEmpty()) { - throw objectError("Invalid object id: $objectId") - } +fun fromString(objectId: String): ObjectId { + require(objectId.isNotEmpty()) { "Object ID cannot be empty" }
🧹 Nitpick comments (9)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/RealtimeObjectsTest.kt (2)
7-7: Redundant “Test” suffix in class nameThe class is already inside a test source-set/package; appending
Testtwice (RealtimeObjectsTest) is verbose and inconsistent with neighbouring test classes (e.g.DefaultLiveObjectsTest). Consider renaming toRealtimeObjectsorRealtimeObjectsSpecfor brevity.
9-13: Method name repetition
testChannelObjectGetterTest()contains “test” twice. A single descriptive verb phrase (e.g.channelObjectsGetter) is clearer and aligns with JUnit-5/Kotlin-test naming conventions.live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt (1)
45-47: Optional: favourassertIs<ObjectMessage>over manual castInstead of
(actual as? ObjectMessage)followed byassertEquals, you can express intent more clearly and avoid the nullable path:assertIs<ObjectMessage>(actual) assertEquals(expected, actual)This removes the silent
nullfallback and yields more readable failure messages.Also applies to: 64-66, 172-180
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (1)
67-90: Consider using more specific exception type.The tests use generic
Exceptiontype, but based on the implementation,canApplyOperationthrowsAblyExceptionviaobjectError. Consider being more specific for better test clarity.- assertFailsWith<Exception>("Should throw error for null serial") { + assertFailsWith<AblyException>("Should throw error for null serial") { liveMap.canApplyOperation("site1", null) }Apply similar changes to other assertions in this test.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (2)
48-55: Consider lazy initialization of GC jobThe GC job is started immediately in the init block. Consider whether GC is needed from the start or if it could be lazily initialized when the first non-root object is added to save resources.
132-156: Consider adding GC performance monitoringThe GC interval runs every 5 minutes but has no timeout or performance monitoring. For large object pools, the GC operation could take significant time.
Consider adding:
- Metrics for GC duration
- A timeout for individual object GC operations
- Batching for very large pools
private fun onGCInterval() { + val startTime = System.currentTimeMillis() + var removedCount = 0 pool.entries.removeIf { (_, obj) -> if (obj.isEligibleForGc()) { + removedCount++ true } else { obj.onGCInterval() false } } + val duration = System.currentTimeMillis() - startTime + Log.v(tag, "GC completed: removed=$removedCount, duration=${duration}ms, remaining=${pool.size}") }live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
57-91: Multiple unimplemented public interface methodsSeveral public LiveMap interface methods are not yet implemented. This could cause runtime failures if these methods are called.
Would you like me to help implement these LiveMap interface methods or create an issue to track their implementation?
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (2)
41-47: Well-designed sequential processing architecture.The single-threaded coroutine scope ensures thread safety for state management. The unlimited buffer is acceptable for channel messages but consider monitoring for potential memory issues in high-traffic scenarios.
192-192: Track the pending state change event implementation.The TODO comment indicates that state change events are not yet implemented, which may be required for complete functionality.
Would you like me to create an issue to track the implementation of state change event emission?
| count = 123 | ||
| ) |
There was a problem hiding this comment.
Compile-time type mismatch: Int supplied where Long? expected
ObjectCounter.count and ObjectCounterOp.amount were migrated to Long?. Passing bare integer literals (123, 10) will not compile in Kotlin.
- count = 123
+ count = 123L
...
- amount = 10
+ amount = 10LAlso applies to: 47-48
🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt
at lines 38-39 and 47-48, integer literals are assigned to variables expecting
nullable Long types, causing a compile-time type mismatch. Fix this by
explicitly specifying the literals as Longs using the `L` suffix (e.g., `123L`,
`10L`) to match the expected `Long?` type.
| amount = 10 // Size: 8 bytes (number is always 8 bytes) | ||
| ), // Total ObjectCounterOp size: 8 bytes |
There was a problem hiding this comment.
Long literal required
amount is now Long?; use 10L to avoid a type mismatch.
- amount = 10 // Size: 8 bytes (number is always 8 bytes)
+ amount = 10L // Size: 8 bytes (number is always 8 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.
| amount = 10 // Size: 8 bytes (number is always 8 bytes) | |
| ), // Total ObjectCounterOp size: 8 bytes | |
| amount = 10L // Size: 8 bytes (number is always 8 bytes) | |
| ), // Total ObjectCounterOp size: 8 bytes |
🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
at lines 56 to 57, the value assigned to the nullable Long variable 'amount' is
an integer literal 10, causing a type mismatch. Change the literal from 10 to
10L to explicitly specify it as a Long type and match the expected variable
type.
| count = 50 // Size: 8 bytes | ||
| ) // Total ObjectCounter size: 8 bytes |
There was a problem hiding this comment.
Long literal required
- count = 50 // Size: 8 bytes
+ count = 50L // Size: 8 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.
| count = 50 // Size: 8 bytes | |
| ) // Total ObjectCounter size: 8 bytes | |
| count = 50L // Size: 8 bytes | |
| ) // Total ObjectCounter size: 8 bytes |
🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
at lines 118 to 119, the numeric literal assigned to count should be explicitly
marked as a Long type. Fix this by appending an 'L' to the number 50 to indicate
it is a Long literal, ensuring type correctness.
| count = 100 // Size: 8 bytes (number is always 8 bytes) | ||
| ), // Total ObjectCounter size: 8 bytes |
There was a problem hiding this comment.
Long literal required
- count = 100 // Size: 8 bytes (number is always 8 bytes)
+ count = 100L // Size: 8 bytes (number is always 8 bytes)🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
at lines 80 to 81, the numeric literal assigned to count should be explicitly
marked as a Long type. Fix this by appending an 'L' to the number 100 to make it
a Long literal, ensuring the type matches the expected Long type in the code.
| override fun increment() { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun incrementAsync(callback: Callback<Void>) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun decrement() { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun decrementAsync(callback: Callback<Void>) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun value(): Long { | ||
| TODO("Not yet implemented") | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Track completion timeline for core counter operations.
The core counter operations (increment, decrement, value) are currently TODO stubs. While the architectural foundation with delegation to LiveCounterManager is sound, the incomplete implementation limits functionality.
Could you provide the timeline for implementing these core operations? This would help assess the readiness of the counter functionality.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that might be expecting these methods to work
rg -A 5 -B 5 "increment\(\)|decrement\(\)|value\(\)" --type kotlin | grep -E "(test|Test)" | head -20Length of output: 103
Implement core LiveCounter operations – timeline needed
The methods in DefaultLiveCounter (increment, incrementAsync, decrement, decrementAsync, value) are all still TODO stubs (lines 32–50). While the delegation to LiveCounterManager and lifecycle setup look solid, none of the core counter operations are functional, and currently no tests exercise these methods.
• File: live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt (lines 32–50)
• Missing implementations:
– increment() / incrementAsync(callback: Callback<Void>)
– decrement() / decrementAsync(callback: Callback<Void>)
– value(): Long
Please provide an estimated timeline for completing these implementations so we can verify readiness and update tests accordingly.
🤖 Prompt for AI Agents
In
live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
between lines 32 and 50, the core LiveCounter methods increment, incrementAsync,
decrement, decrementAsync, and value are currently unimplemented stubs. To fix
this, implement these methods by delegating the increment and decrement
operations to the LiveCounterManager instance managing the counter state,
ensuring both synchronous and asynchronous variants properly update the counter
and invoke callbacks as needed. For the value() method, return the current
counter value from the LiveCounterManager. Provide an estimated timeline for
completing these implementations to coordinate testing and verification.
| @Test | ||
| fun `(RTLO4a4, RTLO4a5) canApplyOperation should return true when existing siteSerial is null or empty`() { | ||
| val liveMap: BaseLiveObject = DefaultLiveMap.zeroValue("map:testObject@1", mockk(), mockk()) | ||
| assertTrue(liveMap.siteTimeserials.isEmpty(), "Initial siteTimeserials should be empty") | ||
|
|
||
| // RTLO4a4 - Get siteSerial from siteTimeserials map | ||
| // RTLO4a5 - Return true when siteSerial is null (no entry in map) | ||
| assertTrue(liveMap.canApplyOperation("site1", "serial1"), | ||
| "Should return true when no siteSerial exists for the site") | ||
|
|
||
| // RTLO4a5 - Return true when siteSerial is empty string | ||
| liveMap.siteTimeserials["site1"] = "" | ||
| assertTrue(liveMap.canApplyOperation("site1", "serial1"), | ||
| "Should return true when siteSerial is empty string") | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify internal state access pattern.
The test directly modifies siteTimeserials on line 103, which appears to be an internal mutable map. This might indicate that the map is not properly encapsulated.
🏁 Script executed:
#!/bin/bash
# Check if siteTimeserials is properly encapsulated in BaseLiveObject
ast-grep --pattern 'internal val siteTimeserials = $_'Length of output: 205
Encapsulate Mutable siteTimeserials
The test at BaseLiveObjectTest.kt:103 directly mutates siteTimeserials, which is currently declared as an internal val in BaseLiveObject.kt:32. Exposing the mutable map this way leaks implementation details and ties tests to internal state.
• In live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt (line 32), change
internal val siteTimeserials = mutableMapOf<String, String>()
to a private property.
• Provide an internal (or protected) API—for example, a clearSiteSerial(siteId: String) or setSiteSerial(siteId: String, serial: String) method—that tests can call.
• Update the test in BaseLiveObjectTest.kt to use the new API rather than writing directly into the map.
This ensures proper encapsulation of siteTimeserials and keeps tests focused on the public behavior.
🤖 Prompt for AI Agents
In live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt at
line 32, change the declaration of siteTimeserials from an internal mutable map
to a private mutable map to encapsulate it. Add an internal or protected method
such as setSiteSerial(siteId: String, serial: String) or clearSiteSerial(siteId:
String) to allow controlled modification of siteTimeserials. Then, in
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt
between lines 92 and 106, update the test to use this new method instead of
directly mutating the siteTimeserials map. This will maintain encapsulation and
keep the test focused on the public API.
| // RTLO4a6 - Return false when message serial is less (lexicographically) | ||
| assertTrue(liveMap.canApplyOperation("site1", "serialA"), | ||
| "Should return false when message serial 'serialA' < siteSerial 'serial2'") |
There was a problem hiding this comment.
Fix incorrect comment.
The comment on line 142 says "Should return false" but the test correctly uses assertTrue. The comment should be updated to match the test expectation.
- assertTrue(liveMap.canApplyOperation("site1", "serialA"),
- "Should return false when message serial 'serialA' < siteSerial 'serial2'")
+ assertTrue(liveMap.canApplyOperation("site1", "serialA"),
+ "Should return true when message serial 'serialA' > siteSerial 'serial2'")📝 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.
| // RTLO4a6 - Return false when message serial is less (lexicographically) | |
| assertTrue(liveMap.canApplyOperation("site1", "serialA"), | |
| "Should return false when message serial 'serialA' < siteSerial 'serial2'") | |
| // RTLO4a6 - Return false when message serial is less (lexicographically) | |
| assertTrue(liveMap.canApplyOperation("site1", "serialA"), | |
| "Should return true when message serial 'serialA' > siteSerial 'serial2'") |
🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt
around lines 141 to 143, the comment incorrectly states "Should return false"
while the test asserts true. Update the comment to correctly reflect that the
test expects the method to return true when the message serial is less
lexicographically.
| @Test | ||
| fun `(RTLO4a1, RTLO4a2) canApplyOperation should accept ObjectMessage params and return boolean`() { | ||
| // RTLO4a1a - Assert parameter types and return type based on method signature using reflection | ||
| val method = BaseLiveObject::class.java.findMethod("canApplyOperation") | ||
|
|
||
| // RTLO4a1a - Verify parameter types | ||
| val parameters = method.parameters | ||
| assertEquals(2, parameters.size, "canApplyOperation should have exactly 2 parameters") | ||
|
|
||
| // First parameter should be String? (siteCode) | ||
| assertEquals(String::class.java, parameters[0].type, "First parameter should be of type String?") | ||
| assertTrue(parameters[0].isVarArgs.not(), "First parameter should not be varargs") | ||
|
|
||
| // Second parameter should be String? (timeSerial) | ||
| assertEquals(String::class.java, parameters[1].type, "Second parameter should be of type String?") | ||
| assertTrue(parameters[1].isVarArgs.not(), "Second parameter should not be varargs") | ||
|
|
||
| // RTLO4a2 - Verify return type | ||
| assertEquals(Boolean::class.java, method.returnType, "canApplyOperation should return Boolean") | ||
| } |
There was a problem hiding this comment.
Fix the reflection method lookup.
The findMethod call on line 49 is missing parameter types, which will cause a NoSuchMethodException at runtime. The method canApplyOperation takes two String parameters.
Apply this fix:
- val method = BaseLiveObject::class.java.findMethod("canApplyOperation")
+ val method = BaseLiveObject::class.java.getDeclaredMethod("canApplyOperation", String::class.java, String::class.java)🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt
around lines 46 to 65, the reflection call to find the method
"canApplyOperation" is missing the parameter types, causing a
NoSuchMethodException. Fix this by specifying the parameter types (two String
classes) in the findMethod call to correctly locate the method signature.
| internal fun applyObject(objectMessage: ObjectMessage) { | ||
| validateObjectId(objectMessage.operation?.objectId) | ||
|
|
||
| val msgTimeSerial = objectMessage.serial | ||
| val msgSiteCode = objectMessage.siteCode | ||
| val objectOperation = objectMessage.operation as ObjectOperation | ||
|
|
||
| if (!canApplyOperation(msgSiteCode, msgTimeSerial)) { | ||
| // RTLC7b, RTLM15b | ||
| Log.v( | ||
| tag, | ||
| "Skipping ${objectOperation.action} op: op serial $msgTimeSerial <= site serial ${siteTimeserials[msgSiteCode]}; " + | ||
| "objectId=$objectId" | ||
| ) | ||
| return | ||
| } | ||
| // should update stored site serial immediately. doesn't matter if we successfully apply the op, | ||
| // as it's important to mark that the op was processed by the object | ||
| siteTimeserials[msgSiteCode!!] = msgTimeSerial!! // RTLC7c, RTLM15c | ||
|
|
||
| if (isTombstoned) { | ||
| // this object is tombstoned so the operation cannot be applied | ||
| return; | ||
| } | ||
| applyObjectOperation(objectOperation, objectMessage) // RTLC7d | ||
| } |
There was a problem hiding this comment.
Add null safety check for operation cast.
Line 70 performs an unsafe cast of objectMessage.operation without checking if it's null first. This could cause a NullPointerException.
- val objectOperation = objectMessage.operation as ObjectOperation
+ val objectOperation = objectMessage.operation as? ObjectOperation
+ ?: throw objectError("Missing operation in object message")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt
around lines 65 to 90, the code performs an unsafe cast of
objectMessage.operation to ObjectOperation without checking for null, which can
cause a NullPointerException. To fix this, add a null check for
objectMessage.operation before casting it. If it is null, handle the case
appropriately, such as returning early or throwing a controlled exception, to
avoid unsafe casting and potential crashes.
| // RTLC8b - Should skip if already merged | ||
| liveCounterManager.applyOperation(operation) | ||
|
|
||
| assertEquals(4L, liveCounter.data) // Should not change (still 0) |
There was a problem hiding this comment.
Fix incorrect comment.
The comment states "Should not change (still 0)" but the test expects the value to remain 4L, not 0.
- assertEquals(4L, liveCounter.data) // Should not change (still 0)
+ assertEquals(4L, liveCounter.data) // Should not change (still 4)📝 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.
| assertEquals(4L, liveCounter.data) // Should not change (still 0) | |
| assertEquals(4L, liveCounter.data) // Should not change (still 4) |
🤖 Prompt for AI Agents
In
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt
at line 124, the comment next to the assertion incorrectly states "Should not
change (still 0)" while the expected value is 4L. Update the comment to
correctly reflect that the value should remain 4L, not 0, to match the assertion
and avoid confusion.
…andle exceptions 1. Added try catch to avoid crashing collector 2. Fixed defaultLiveObjects flaky tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (2)
13-22: Consider the necessity of 24-hour grace period.The 24-hour grace period seems quite conservative compared to the minimum requirement of > 2 minutes mentioned in the comment. This could potentially lead to memory bloat if many objects become eligible for GC.
Consider if a shorter period (2-4 hours) would provide sufficient safety while reducing memory overhead. The current value may be intentionally conservative for safety, but it's worth evaluating based on expected usage patterns.
48-55: Consider making gcJob immutable.Since
gcJobis only assigned once in the init block and never reassigned, it can be declared asvalfor better immutability.- private var gcJob: Job // Job for the garbage collection coroutine + private val gcJob: Job // Job for the garbage collection coroutinelive-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
194-203: State management method with pending event emission.The state change logic is correct with appropriate guards against redundant changes. The TODO indicates incomplete event emission functionality.
Would you like me to help implement the state change event emission or open an issue to track this pending work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt(1 hunks)
🧠 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (6)
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the action field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
🧬 Code Graph Analysis (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (3)
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
onGCInterval(108-110)live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt (1)
onGCInterval(191-191)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt (1)
onGCInterval(66-69)
🧰 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (6)
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the action field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
🧬 Code Graph Analysis (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (3)
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
onGCInterval(108-110)live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseLiveObject.kt (1)
onGCInterval(191-191)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt (1)
onGCInterval(66-69)
🔇 Additional comments (10)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (5)
1-9: Clean import structure and package organization.The imports are well-organized and include all necessary dependencies for coroutines-based garbage collection and live object management.
62-107: Well-implemented pool operations with proper root object handling.The pool operations correctly implement the required functionality:
- Root object is properly preserved during reset and delete operations
- Data clearing supports configurable event emission
- Thread-safe operations for the sequential execution context
114-127: Excellent zero-value object creation logic.The method correctly implements the spec requirements:
- Checks for existing objects first (RTO6a)
- Parses object ID to determine type (RTO6b)
- Creates appropriate zero-value objects for Map/Counter types
- Uses apply block to ensure objects are added to the pool (RTO6b4)
132-156: Robust garbage collection implementation with proper error handling.The GC implementation is well-designed:
- Two-phase GC: removes eligible objects then calls onGCInterval on remaining ones
- Proper error handling that doesn't stop the GC loop
- Cooperative cancellation with isActive check
- Appropriate logging for debugging
162-167: Proper resource cleanup in dispose method.The dispose method correctly handles cleanup in the right order:
- Cancel the GC job
- Cancel the coroutine scope
- Clear the pool
This ensures no memory leaks and proper resource management.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (5)
3-18: Well-defined state management and appropriate imports.The ObjectsState enum provides clear lifecycle states for sync management, and the imports include all necessary coroutines components for the asynchronous architecture.
24-52: Excellent asynchronous architecture for thread-safe sequential processing.The design effectively ensures thread safety:
- Single-threaded coroutine scope with
limitedParallelism(1)prevents concurrency issuesMutableSharedFlowwith unlimited buffer prevents message lossSupervisorJobprovides proper fault isolation- Clear separation of concerns with ObjectsPool and ObjectsManager
101-147: Robust message handling with proper field enrichment and error handling.The implementation correctly follows the specifications:
- Sets channel serial per RTL15b
- Validates message state before processing
- Enriches ObjectMessage fields per OM2 spec
- Sequential processing ensures correct operation order
- Error handling prevents single message failures from stopping the flow
149-187: Comprehensive channel state handling following specifications.The method correctly implements the required state transitions:
- RTO4a: Proper sync initiation logic based on hasObjects and current state
- RTO4b: Correct handling when no objects are present
- Appropriate cleanup for detached/failed states without event emission
- Thread-safe execution using sequentialScope
206-211: Proper resource cleanup with clear documentation.The dispose method correctly handles cleanup:
- Cancels the coroutine handler first
- Disposes managed components (pool and manager)
- Helpful comment about automatic garbage collection of event bus
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
106-126: Fix incorrect comment.The comment states "Should not change (still 0)" but the test expects the value to remain 4.0, not 0.
- assertEquals(4.0, liveCounter.data) // Should not change (still 0) + assertEquals(4.0, liveCounter.data) // Should not change (still 4)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (1)
6-6: Remove the incorrect LiveMapManager import.This import conflicts with the production import on line 5 and will cause compilation errors.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
57-57: Update the documentation to reflect the new return type.The Javadoc comment still refers to "Long" but the method now returns
Double.- * @return the current value of the counter as a Long. + * @return the current value of the counter as a Double.live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (1)
635-818: Consider splitting this test into multiple focused test methods.This test method contains 11 different test cases. For better maintainability and clarity, consider splitting each test case into its own
@Testmethod with descriptive names. This would make it easier to identify which specific scenario fails and improve test readability.Example refactoring for the first few cases:
@Test fun `calculateUpdateFromDataDiff should return empty map when no changes`() { val prevData = mapOf<String, LiveMapEntry>() val newData = mapOf<String, LiveMapEntry>() val result = livemapManager.calculateUpdateFromDataDiff(prevData, newData) assertEquals(emptyMap<String, String>(), result) } @Test fun `calculateUpdateFromDataDiff should detect added entries`() { val prevData = mapOf<String, LiveMapEntry>() val newData = mapOf( "key1" to LiveMapEntry( isTombstoned = false, timeserial = "1", data = ObjectData(value = ObjectValue("value1")) ) ) val result = livemapManager.calculateUpdateFromDataDiff(prevData, newData) assertEquals(mapOf("key1" to "updated"), result) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt(4 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt(5 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt(1 hunks)
🧠 Learnings (5)
📓 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
Learnt from: sacOO7
PR: #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/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt (6)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the action field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Learnt from: sacOO7
PR: #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.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (2)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
🚧 Files skipped from review as they are similar to previous changes (8)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt
🧰 Additional context used
🧠 Learnings (5)
📓 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
Learnt from: sacOO7
PR: #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/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt (6)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the action field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Learnt from: sacOO7
PR: #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.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (2)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
🔇 Additional comments (19)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt (7)
1-28: Well-structured imports and package declaration.The package structure and imports are comprehensive and appropriate for the test functionality. All imported classes are utilized in the test methods.
29-29: Standard test class declaration.The class follows standard Kotlin test naming conventions.
31-47: Excellent test implementation for RTO4a protocol specification.The test correctly verifies that when the channel becomes ATTACHED with HAS_OBJECTS flag set to true, the sync sequence starts properly. The assertions appropriately check state transitions and method invocations.
49-77: Comprehensive test implementation for RTO4b protocol specification.The test thoroughly covers the immediate sync completion scenario when HAS_OBJECTS flag is false. The setup, execution, and verification are comprehensive, covering all specification requirements (RTO4b1-RTO4b5).
79-95: Important edge case test for INITIALIZED state transitions.The test correctly verifies that sync is always started when transitioning from INITIALIZED state, even with HAS_OBJECTS flag false. The verification of both startNewSync and endSync calls with appropriate parameters is accurate.
97-157: Thorough test of protocol message delegation to ObjectsManager.The test comprehensively verifies RTO5 and RTO7 specifications by testing delegation of both OBJECT and OBJECT_SYNC protocol messages with realistic test data. The verification of method calls with correct parameters is accurate.
159-236: Excellent test for OM2 protocol specification compliance.The test thoroughly verifies field population logic for ObjectMessage instances with missing fields. The sophisticated mocking approach to capture passed messages and comprehensive verification of field population rules (OM2a, OM2c, OM2e) demonstrates good test design for this critical protocol requirement.
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
61-61: Breaking API Change: Confirm All Consumers HandleDoubleReturnThe return type of
LiveCounter.value()has changed fromLongtoDouble, which is a breaking change for any code—internal or external—that invokes this method. While no internal calls tovalue()were found in the current codebase, you must ensure:
- Any downstream modules or external clients depending on
LiveCounter.value()are updated to handle aDoublereturn (e.g., adjusting variable types, rounding logic, or precision handling).- Documentation and release notes reflect this change and recommend a major-version bump.
- Automated tests (if present) for any consumers are updated to expect
Doubleinstead ofLong.live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (11)
1-9: LGTM - Well-structured test imports and package declaration.The imports are appropriate and the package structure follows the expected conventions.
13-32: LGTM - Comprehensive state synchronization test.The test correctly validates RTLC6, RTLC6b, and RTLC6c specifications, properly checking state override behavior and the difference calculation.
36-62: LGTM - Proper create operation merge test.The test correctly validates RTLC6 and RTLC6d specifications, ensuring create operations are merged correctly during state synchronization.
66-85: LGTM - Appropriate error handling test.The test correctly validates RTLC7 and RTLC7d3 specifications, ensuring unsupported actions throw appropriate exceptions with correct error codes.
88-103: LGTM - Valid create operation application test.The test properly validates RTLC7, RTLC7d1, and RTLC8 specifications for applying counter create operations.
129-150: LGTM - Proper create operation merge logic test.The test correctly validates RTLC8 and RTLC8c specifications for applying create operations when not already merged.
153-172: LGTM - Appropriate null handling test.The test properly validates RTLC8, RTLC10, and RTLC10a specifications for handling null count values in create operations.
175-192: LGTM - Valid increment operation test.The test correctly validates RTLC7, RTLC7d2, and RTLC9 specifications for applying counter increment operations.
195-214: LGTM - Proper error handling for missing payload.The test appropriately validates RTLC7 and RTLC7d2 specifications, ensuring missing payload errors are handled correctly.
218-235: LGTM - Correct increment operation logic test.The test properly validates RTLC9 and RTLC9b specifications for counter increment operations.
238-255: LGTM - Appropriate null amount handling test.The test correctly validates RTLC9 and RTLC9b specifications for handling null amounts in increment operations.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt
Show resolved
Hide resolved
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
29-42: Logic is correct with minor optimization opportunity.The method properly implements the buffering vs immediate application logic based on sync state.
Consider caching the state to avoid accessing it twice:
internal fun handleObjectMessages(objectMessages: List<ObjectMessage>) { + val currentState = liveObjects.state - if (liveObjects.state != ObjectsState.SYNCED) { + if (currentState != ObjectsState.SYNCED) { // RTO7 - The client receives object messages in realtime over the channel concurrently with the sync sequence. // Some of the incoming object messages may have already been applied to the objects described in // the sync sequence, but others may not; therefore we must buffer these messages so that we can apply // them to the objects once the sync is complete. - Log.v(tag, "Buffering ${objectMessages.size} object messages, state: $liveObjects.state") + Log.v(tag, "Buffering ${objectMessages.size} object messages, state: $currentState") bufferedObjectOperations.addAll(objectMessages) // RTO8a return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt(4 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt(1 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(5 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(1 hunks)
🧠 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (4)
Learnt from: sacOO7
PR: #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: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
🧬 Code Graph Analysis (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(33-33)
🚧 Files skipped from review as they are similar to previous changes (5)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (4)
Learnt from: sacOO7
PR: #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: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
🧬 Code Graph Analysis (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(33-33)
⏰ 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-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check
- GitHub Check: check (19)
- GitHub Check: build
- GitHub Check: check (21)
🔇 Additional comments (10)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (10)
12-23: Well-structured class with clear separation of concerns.The class design follows good practices with proper encapsulation, clear field purposes, and comprehensive spec documentation. The dependency injection pattern with
DefaultLiveObjectsprovides good testability.
49-66: Correctly implements sync sequence protocol.The method properly handles the sync sequence lifecycle with appropriate checks for new sync sequences and completion. The deferred state event mechanism ensures proper event ordering for listeners.
73-81: Properly implements sync sequence initialization.The method correctly clears stale data and sets up the new sync sequence according to the specification. The order of operations ensures clean state before starting the new sync.
88-99: Correctly implements sync sequence completion.The method follows the proper order of operations: apply sync data first, then buffered operations, followed by cleanup. This ensures consistent object state and proper resource management.
105-115: Simple and correct utility methods.These methods provide controlled access for clearing internal state, which is appropriate for the integration with
DefaultLiveObjects.handleStateChange.
122-155: Correctly implements sync data application with proper object lifecycle management.The method handles both existing and new objects appropriately, maintains object pool consistency by removing stale objects, and efficiently batches notifications after all updates are complete.
162-186: Well-designed object message application with robust error handling.The zero-value object creation strategy elegantly handles operations for unknown object IDs, while the error handling appropriately skips invalid messages with proper logging without stopping the entire batch.
193-208: Proper sync message validation and application.The method correctly validates that object states contain the required data (counter or map) and handles invalid messages gracefully with appropriate logging.
215-221: Clean object creation with proper type dispatch and validation.The method uses appropriate pattern matching to create the correct object type and properly validates that the object state contains required data, throwing a meaningful error for invalid states.
223-227: Appropriate cleanup implementation.The dispose method properly clears all internal state, providing clean resource management for the ObjectsManager lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (3)
51-51: Fix the reflection method lookup missing parameter types.The
findMethodcall is missing parameter types, which will causeNoSuchMethodExceptionat runtime sincecanApplyOperationrequires two String parameters.Apply this fix:
- val method = BaseLiveObject::class.java.findMethod("canApplyOperation") + val method = BaseLiveObject::class.java.getDeclaredMethod("canApplyOperation", String::class.java, String::class.java)
143-145: Fix incorrect test comment.The comment states "Should return false" but the test correctly uses
assertTrue. The comment should match the test expectation.- assertTrue(liveMap.canApplyOperation("site1", "serialA"), - "Should return false when message serial 'serialA' < siteSerial 'serial2'") + assertTrue(liveMap.canApplyOperation("site1", "serialA"), + "Should return true when message serial 'serialA' > siteSerial 'serial2'")
105-105: Avoid direct mutation of internal state in tests.The test directly mutates
siteTimeserials, which is an internal mutable map. This breaks encapsulation and ties tests to implementation details.Consider providing an internal test API in
BaseLiveObjectfor controlled state manipulation, such assetSiteSerial(siteId: String, serial: String), and update the test to use this API instead of direct map access.live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
124-124: Fix incorrect comment.The comment states "Should not change (still 0)" but the test expects the value to remain 4.0, not 0.
- assertEquals(4.0, liveCounter.data.get()) // Should not change (still 0) + assertEquals(4.0, liveCounter.data.get()) // Should not change (still 4)
🧹 Nitpick comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
40-74: Track TODO implementations for future completion.All public API methods are currently stubbed with TODO. This is expected for the current PR scope but should be tracked for future implementation.
Would you like me to create an issue to track the implementation of these LiveMap public API methods (
get,set,remove,entries,keys,values,size, and async variants)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(2 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt(1 hunks)
🧠 Learnings (8)
📓 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (3)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (4)
Learnt from: sacOO7
PR: #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: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (3)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (8)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/LiveObjectTest.kt:1-6
Timestamp: 2025-06-05T10:24:28.789Z
Learning: In Kotlin, functions, classes, and other declarations within the same package are automatically accessible without explicit import statements. Do not suggest adding imports for functions/classes that are already in the same package.
Learnt from: sacOO7
PR: #1059
File: lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java:124-143
Timestamp: 2025-01-22T13:13:02.809Z
Learning: In test classes, prefer keeping setup code within individual test methods rather than extracting to shared helpers, as it makes tests more independent and readable by keeping the context visible within each test.
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (2)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
🧬 Code Graph Analysis (3)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getDefaultLiveObjectsWithMockedDeps(79-97)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(33-33)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getDefaultLiveCounterWithMockedDeps(113-124)
🚧 Files skipped from review as they are similar to previous changes (8)
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
🧰 Additional context used
🧠 Learnings (8)
📓 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.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
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#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (3)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (4)
Learnt from: sacOO7
PR: #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: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (3)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (8)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: #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: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/LiveObjectTest.kt:1-6
Timestamp: 2025-06-05T10:24:28.789Z
Learning: In Kotlin, functions, classes, and other declarations within the same package are automatically accessible without explicit import statements. Do not suggest adding imports for functions/classes that are already in the same package.
Learnt from: sacOO7
PR: #1059
File: lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java:124-143
Timestamp: 2025-01-22T13:13:02.809Z
Learning: In test classes, prefer keeping setup code within individual test methods rather than extracting to shared helpers, as it makes tests more independent and readable by keeping the context visible within each test.
Learnt from: sacOO7
PR: #1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Learnt from: sacOO7
PR: #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: #1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
Learnt from: sacOO7
PR: #1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (2)
Learnt from: sacOO7
PR: #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: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
Learnt from: sacOO7
PR: #1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
🧬 Code Graph Analysis (3)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/BaseLiveObjectTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getDefaultLiveObjectsWithMockedDeps(79-97)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(33-33)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getDefaultLiveCounterWithMockedDeps(113-124)
⏰ 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 (21)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check-liveobjects
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (39)
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (4)
10-15: LGTM: Well-structured data class for live map entries.The data class appropriately models live map entry state with optional tombstone metadata and data payload. Default values and nullable types are correctly applied.
21-33: LGTM: Robust tombstone checking logic.The extension function correctly implements the tombstone checking logic per RTLM14 specification, handling both direct tombstoning and reference tombstoning with proper null safety.
39-53: LGTM: Comprehensive value resolution with tombstone awareness.The value resolution logic correctly handles all scenarios including tombstones, primitive values, and object references with proper null safety and tombstone filtering.
58-61: LGTM: Correct GC eligibility implementation.The GC eligibility check correctly validates tombstone status and grace period using current system time. The logic properly handles null tombstone timestamps.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (6)
12-23: LGTM: Well-structured manager with clear responsibilities.The class properly encapsulates sync data pooling and message buffering with clear documentation referencing specifications. The mutable collections are appropriately scoped as private.
29-42: LGTM: Correct buffering logic for non-synced states.The message handling properly buffers operations during non-synced states per RTO7/RTO8 specifications, with appropriate logging and immediate application when synced.
164-186: LGTM: Robust operation application with comprehensive validation.The method correctly handles missing operations, unknown actions, and creates zero-value objects for unknown IDs per RTO9 specification. The logging for skipped messages provides good debugging information.
193-208: LGTM: Proper sync message validation and collection.The sync message handling correctly validates object state presence and content, with appropriate logging for invalid messages. The validation ensures either counter or map data is present.
215-221: LGTM: Type-safe object creation from state.The factory method correctly creates objects based on state type with appropriate error handling using the established
clientErrorutility for protocol violations.
223-227: LGTM: Proper resource cleanup in dispose method.The dispose method correctly clears both internal collections, ensuring no memory leaks.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (5)
18-22: LGTM: Well-structured class with proper inheritance.The class correctly implements
LiveMapand extendsBaseLiveObjectwith appropriate constructor parameters and default semantics. The private constructor with companion factory method is a good pattern.
29-29: LGTM: Thread-safe data storage choice.Using
ConcurrentHashMapis appropriate for thread-safe access from both public APIs and internal manager operations.
76-84: LGTM: Proper delegation to manager for core operations.The delegation pattern to
LiveMapManagerfor validation, state application, and operation handling is well-implemented and maintains separation of concerns.
86-89: LGTM: Efficient data clearing with diff calculation.The
clearDatamethod efficiently calculates updates from current state to empty state before clearing, enabling proper change notifications.
100-103: LGTM: Standard zero-value factory implementation.The factory method follows the established pattern for creating zero-value live objects as seen in the codebase.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (7)
14-23: LGTM: Well-defined configuration constants.The GC configuration constants are appropriately defined with clear documentation explaining the 24-hour grace period rationale for avoiding serial ordering issues.
35-57: LGTM: Proper initialization with root object and GC setup.The pool correctly initializes with a root object and starts the GC coroutine. The use of
SupervisorJobensures GC failures don't affect the parent scope.
77-80: LGTM: Safe pool reset preserving root object.The reset logic correctly preserves the root object while clearing others, maintaining object reference stability as documented.
106-119: LGTM: Robust zero-value object creation.The method correctly handles existing objects, parses object IDs for type determination, and creates appropriate zero-value objects with proper pool registration.
124-132: LGTM: Safe garbage collection with eligibility checks.The GC interval handler correctly removes eligible objects and calls per-object GC methods on retained objects.
137-148: LGTM: Robust GC coroutine with error handling.The GC coroutine properly handles exceptions without terminating the loop and uses
isActivefor clean cancellation. The exception logging provides good debugging information.
154-159: LGTM: Complete resource cleanup in dispose method.The disposal method correctly cancels the GC job, cancels the coroutine scope, and clears the pool, ensuring no resource leaks.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (6)
3-10: LGTM! Import structure is clear and appropriate.The combination of wildcard and specific imports provides good clarity for the test helper dependencies.
21-45: LGTM! Well-structured channel mock setup.The function creates a realistic channel mock with proper state management and flexible configuration options.
47-58: LGTM! Simple and effective mock factory functions.The functions provide clean abstractions for creating test dependencies, and the size extension follows the established pattern for reflection-based field access.
65-97: LGTM! Comprehensive DefaultLiveObjects mocking setup.The helper provides flexible mocking options with clear access to internal dependencies, enabling thorough testing of DefaultLiveObjects behavior.
109-124: LGTM! Consistent LiveCounter mocking pattern.The helper maintains the same flexible mocking approach as other components, enabling controlled testing of DefaultLiveCounter behavior.
136-151: LGTM! Consistent LiveMap mocking pattern.The helper follows the established pattern, providing uniform mocking capabilities across all live object types.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (11)
1-9: LGTM! Appropriate imports for LiveCounterManager testing.All necessary dependencies are imported for comprehensive testing of the LiveCounterManager component.
12-32: LGTM! Comprehensive test for state override functionality.The test correctly validates RTLC6 specification requirements, including proper data override and update calculation.
35-62: LGTM! Correct test for create operation merging.The test properly validates the merging of create operations during state synchronization according to RTLC6d requirements.
65-85: LGTM! Proper error handling test for unsupported actions.The test correctly validates that unsupported operations throw appropriate exceptions with the right error codes.
87-103: LGTM! Correct test for counter create operation.The test properly validates the application of counter create operations and the setting of the merged flag.
128-150: LGTM! Correct test for applying unmerged create operations.The test properly validates the application of create operations when the merged flag is false, including proper data accumulation.
152-172: LGTM! Proper test for null count handling in create operations.The test correctly validates that null counts default to 0 and the merged flag is properly set.
174-192: LGTM! Correct test for counter increment operations.The test properly validates the application of counter increment operations according to RTLC specification requirements.
194-214: LGTM! Proper error handling test for missing payloads.The test correctly validates that increment operations with missing payloads throw appropriate exceptions.
217-235: LGTM! Focused test for RTLC9b increment behavior.The test properly validates the specific requirements of RTLC9b for counter increment operations.
237-255: LGTM! Proper test for null amount handling in increments.The test correctly validates that null amounts in increment operations are treated as 0, resulting in no change to the counter value.
- Removed unnecessary `encoding` field used to detect json values
…ad of checking type at runtime
|
closing in favor of #1137 |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor