[ECO-5517] Implement getRoot implicit attach #1146
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReworks LiveObjects adapter to surface ChannelBase via getChannel, removes several per-message/channel APIs, adds coroutine-based attach helpers and implicit attach before getRoot, adds ChannelMode null-safety, and updates tests to the new flows (no public API signature changes besides getChannel). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultLiveObjects
participant Helpers
participant Adapter
participant Channel
Client->>DefaultLiveObjects: getRootAsync(channelName)
DefaultLiveObjects->>Adapter: getChannel(channelName)
Adapter-->>DefaultLiveObjects: ChannelBase
DefaultLiveObjects->>Helpers: ensureAttached(channelName)
Helpers->>Adapter: getChannel(channelName)
Adapter-->>Helpers: ChannelBase
alt channel.state == initialized
Helpers->>Channel: attach(CompletionListener)
Channel-->>Helpers: attached / error
else channel.state == attaching
Helpers-->>Helpers: await attach completion
else other state
Helpers-->>DefaultLiveObjects: throw error
end
Helpers-->>DefaultLiveObjects: attached
DefaultLiveObjects-->>Client: return Root object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-12T11:48:24.317ZApplied to files:
📚 Learning: 2025-05-27T12:11:25.084ZApplied to files:
🧬 Code Graph Analysis (1)live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
⏰ 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)
🔇 Additional comments (9)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ceb246c to
5ad4aec
Compare
5ad4aec to
047b7a3
Compare
- Adjusted channel state checks prior to channel modes for read and write objects operations - Added getChannel method to adapter interface, added relevant ensureAttached method with relevant implementation
047b7a3 to
a02b31a
Compare
…lpers - Removed unnecessary implementation from Adapter, made it clean - Updated unit tests/mocks accordingly
4b32a52 to
61cb6b2
Compare
|
@coderabbitai review |
There was a problem hiding this comment.
Pull Request Overview
This PR implements implicit channel attachment functionality for the getRoot method as part of ECO-5517. The main purpose is to ensure that channels are automatically attached when accessing live objects, eliminating the need for explicit attachment calls.
Key changes include:
- Implementation of implicit attach in
getRootmethod viaensureAttachedhelper - Refactoring of the LiveObjectsAdapter interface to use direct channel access patterns
- Addition of comprehensive test coverage for channel attachment and validation logic
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| DefaultLiveObjects.kt | Added ensureAttached call to getRoot method for implicit attachment |
| Helpers.kt | Implemented ensureAttached, attachAsync, and refactored channel validation logic |
| LiveObjectsAdapter.java | Simplified interface to focus on essential operations, removed redundant methods |
| Adapter.java | Updated implementation to align with simplified interface |
| ChannelBase.java | Added null safety check for channel modes |
| HelpersTest.kt | Added comprehensive unit tests for new helper functions |
| TestHelpers.kt | Enhanced mock setup to return proper channel instances |
| ObjectMessageSizeTest.kt | Updated to use direct property access instead of removed adapter methods |
| UtilsTest.kt | Increased delay timeout for test stability |
| Sandbox.kt | Removed redundant ensureAttached extension function |
| IntegrationTest.kt | Removed auto-attach parameter as attachment is now implicit |
| DefaultLiveObjectsTest.kt | Updated test call to reflect removed auto-attach parameter |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (3)
111-113: Race condition issue has been addressed.The previous review comment about the race condition between the
whenexpression check and the additional check at Line 111-113 appears to have been addressed. The current implementation correctly handles the case where the channel might already be attached before the listener is registered.
142-144: Control flow comment is outdated.The previous comment about confusing logic flow with early returns inside let blocks has been addressed. The current implementation uses clearer control flow with explicit null checks.
149-155: Logic inversion issue has been resolved.The previous comment about inverted logic has been addressed. The method now correctly checks if the channel state is in one of the invalid states and throws an exception when found.
🧹 Nitpick comments (4)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt (1)
36-36: LGTM; consider asserting implicit attach triggered by getRootThis migration to the new channel helper looks good. To harden coverage for ECO-5517, add an assertion in testObjectsSync verifying that calling
channel.objects.rootfrom an INITIALIZED channel results in the channel becoming ATTACHED, ensuring the implicit attach behavior is exercised explicitly.Example insertion after Line 71:
import io.ably.lib.realtime.ChannelState // Ensure implicit attach occurred when fetching root assertWaiter { channel.state == ChannelState.attached }Would you like me to add negative-path tests (e.g., suspended/failed channel) to assert that getRoot returns an appropriate error?
live-objects/src/test/kotlin/io/ably/lib/objects/unit/UtilsTest.kt (1)
256-256: Use awaitCancellation() instead of an arbitrary long delayFor deterministic cancellation tests, prefer suspending until cancellation over a large delay. This avoids any risk of real-time waiting if the cancellation timing changes.
Apply this change:
- delay(10000) // Long delay + kotlinx.coroutines.awaitCancellation() // Suspend until cancelledAnd add this import outside the selected range if not present:
import kotlinx.coroutines.awaitCancellationlive-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (1)
248-266: Fix the inconsistent spacing in the test method.There's an extra space at the beginning of lines 248-266 that should be removed for consistency with the rest of the file.
- @Test - fun testEnsureAttachedAttachingButReceivesNonAttachedEmitsError() = runTest { - val adapter = mockk<LiveObjectsAdapter>(relaxed = true) - val channel = mockk<Channel>(relaxed = true) - every { adapter.getChannel("ch") } returns channel - channel.state = ChannelState.attaching - every { channel.once(any()) } answers { - val listener = firstArg<ChannelStateListener>() - val stateChange = mockk<ChannelStateListener.ChannelStateChange>(relaxed = true) { - setPrivateField("current", ChannelState.suspended) - setPrivateField("reason", clientError("Not attached").errorInfo) - } - listener.onChannelStateChanged(stateChange) - } - val ex = assertFailsWith<AblyException> { adapter.ensureAttached("ch") } - assertEquals(ErrorCode.ChannelStateError.code, ex.errorInfo.code) - assertTrue(ex.errorInfo.message.contains("Not attached")) - verify(exactly = 1) { channel.once(any()) } - } + @Test + fun testEnsureAttachedAttachingButReceivesNonAttachedEmitsError() = runTest { + val adapter = mockk<LiveObjectsAdapter>(relaxed = true) + val channel = mockk<Channel>(relaxed = true) + every { adapter.getChannel("ch") } returns channel + channel.state = ChannelState.attaching + every { channel.once(any()) } answers { + val listener = firstArg<ChannelStateListener>() + val stateChange = mockk<ChannelStateListener.ChannelStateChange>(relaxed = true) { + setPrivateField("current", ChannelState.suspended) + setPrivateField("reason", clientError("Not attached").errorInfo) + } + listener.onChannelStateChanged(stateChange) + } + val ex = assertFailsWith<AblyException> { adapter.ensureAttached("ch") } + assertEquals(ErrorCode.ChannelStateError.code, ex.errorInfo.code) + assertTrue(ex.errorInfo.message.contains("Not attached")) + verify(exactly = 1) { channel.once(any()) } + }lib/src/main/java/io/ably/lib/objects/Adapter.java (1)
36-44: Consider using a more specific error code for channel not found.The method correctly implements the new
getChannelAPI, but using a generic 404 status code might not align with Ably's error code conventions. Consider using a more specific error code from the Ably error code range.@Override public @NotNull ChannelBase getChannel(@NotNull String channelName) throws AblyException { if (ably.channels.containsKey(channelName)) { return ably.channels.get(channelName); } else { Log.e(TAG, "attachChannel(): channel not found: " + channelName); - ErrorInfo errorInfo = new ErrorInfo("Channel not found: " + channelName, 404); + ErrorInfo errorInfo = new ErrorInfo("Channel not found: " + channelName, 40400, 404); throw AblyException.fromErrorInfo(errorInfo); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/src/main/java/io/ably/lib/objects/Adapter.java(2 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java(2 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(5 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt(2 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt(0 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(2 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/UtilsTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-07T07:17:33.340Z
Learnt from: sacOO7
PR: ably/ably-java#1137
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:6-6
Timestamp: 2025-08-07T07:17:33.340Z
Learning: In the ably-java LiveObjects test code, there are extension properties defined in TestHelpers.kt that provide access to private fields of classes for testing purposes. For example, `internal var DefaultLiveMap.LiveMapManager: LiveMapManager` allows tests to access the private `liveMapManager` field. These extension imports (like `import io.ably.lib.objects.unit.LiveMapManager`) should not be removed as they are necessary for test functionality and are not conflicting imports.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.ktlive-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-01T09:53:16.730Z
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.730Z
Learning: In the ably-java LiveObjects test code, extension properties with capital letter names (like `State`, `ObjectId`) are defined in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to internal fields of concrete implementations through their public interfaces. For example, `LiveObjects.State` casts to `DefaultLiveObjects` to access the internal `state` field for testing purposes.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.ktlive-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.ktlive-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-06T09:22:40.964Z
Learnt from: sacOO7
PR: ably/ably-java#1135
File: live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt:100-134
Timestamp: 2025-08-06T09:22:40.964Z
Learning: In DefaultLiveObjects.kt, the object creation pattern using objectsPool.get() followed by withContext(sequentialScope.coroutineContext) { objectsPool.createZeroValueObjectIfNotExists() } is thread-safe because sequentialScope uses limitedParallelism(1) ensuring sequential execution, and createZeroValueObjectIfNotExists() performs an internal get() check before creating, preventing duplicate object creation even when multiple coroutines initially see null from the first get() call.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
📚 Learning: 2025-08-01T10:30:27.049Z
Learnt from: sacOO7
PR: ably/ably-java#1130
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt:241-241
Timestamp: 2025-08-01T10:30:27.049Z
Learning: In DefaultLiveMapTest.kt integration tests, operations are performed sequentially one after another, and subscription callback list updates happen one at a time, so thread-safe collections are not needed for collecting updates in test scenarios.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt
📚 Learning: 2025-06-23T14:18:25.315Z
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:25-29
Timestamp: 2025-06-23T14:18:25.315Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class methods like writeMsgpackArray will have their type signatures updated to be non-nullable since the calling sites ensure objects are never null when passed to these methods.
Applied to files:
lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java
📚 Learning: 2025-05-27T12:11:25.084Z
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.
Applied to files:
lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.javalive-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
🔇 Additional comments (5)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
94-99: LGTM! The implicit attach implementation correctly implements the PR objectives.The addition of
adapter.ensureAttached(channelName)before syncing state correctly implements the implicit attach behavior forLiveObject.objects.getRootas specified in issue #1145. The implementation follows the expected pattern where channels in theinitializedstate are implicitly attached, and appropriate errors are thrown for invalid states.live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
47-51: LGTM! The mock adapter setup correctly supports the new channel-based API.The updated mock configuration properly stubs the new
getChannelmethod to return a consistent mock channel, aligning with the refactored adapter interface that now provides channel access throughgetChannel(String)rather than per-message APIs.live-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (1)
1-18: LGTM! Comprehensive test coverage for the new helper functions.The test file provides thorough coverage of the new helper functions introduced in
Helpers.kt, including edge cases and error scenarios for channel attachment, mode validation, and state management.live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
16-30: LGTM! The sendAsync implementation correctly respects queueMessages configuration.The updated implementation properly uses
clientOptions.queueMessagesinstead of hardcodingtrue, which aligns with the broader refactoring while maintaining the necessary queuing behavior for LiveObjects.lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java (1)
3-3: Import of ChannelBase aligns with the new channel-centric adapter APIUsing ChannelBase in the adapter interface is consistent with the PR’s direction to centralize channel operations. No issues here.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt
Show resolved
Hide resolved
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
Show resolved
Hide resolved
live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
Show resolved
Hide resolved
61cb6b2 to
2394558
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
149-154: Simplify channel state validation logicThe logic checks if the current state is null OR if it's in the invalid states array, which is confusing. A null state should probably be handled separately from checking invalid states.
Clarify the logic by separating null check from state validation:
private fun LiveObjectsAdapter.throwIfInChannelState(channelName: String, channelStates: Array<ChannelState>) { val currentState = getChannel(channelName).state - if (currentState == null || channelStates.contains(currentState)) { - throw ablyException("Channel is in invalid state: $currentState", ErrorCode.ChannelStateError) + if (currentState == null) { + throw ablyException("Channel state is null", ErrorCode.ChannelStateError) + } + if (channelStates.contains(currentState)) { + throw ablyException("Channel is in invalid state: $currentState", ErrorCode.ChannelStateError) } }
111-113: Handle race condition more robustlyThe current implementation has a potential race condition where the channel state could change between registering the listener and this check. While the check on line 111-113 attempts to handle this, there's still a window where the state could become attached after the listener is registered but before this check, potentially causing the deferred to complete twice.
Consider using
completeExceptionallywith a more defensive approach:} } - if (channel.state == ChannelState.attached) { - attachDeferred.complete(Unit) - } + // Use tryComplete to avoid completing twice + channel.state?.let { state -> + if (state == ChannelState.attached && !attachDeferred.isCompleted) { + attachDeferred.complete(Unit) + } + } attachDeferred.await()
🧹 Nitpick comments (2)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
56-73: Potential NPE when accessingchannel.optionsThe code accesses
channel.options?.letbut doesn't handle the case wherechannel.optionscould be null. Additionally, the nestedoptions.hasModes()check might not be necessary if you're already checking for null.Consider simplifying the null checks:
internal fun LiveObjectsAdapter.getChannelModes(channelName: String): Array<ChannelMode>? { val channel = getChannel(channelName) // RTO2a - channel.modes is only populated on channel attachment, so use it only if it is set channel.modes?.let { modes -> if (modes.isNotEmpty()) { return modes } } // RTO2b - otherwise as a best effort use user provided channel options - channel.options?.let { options -> - if (options.hasModes()) { - return options.modes - } - } - return null + return channel.options?.takeIf { it.hasModes() }?.modes }
94-119: Consider extracting the attaching state wait logic for better testabilityThe
ensureAttachedfunction handles multiple states with complex async logic. The attaching state handling is particularly complex with theCompletableDeferredpattern.Consider extracting the attaching state wait logic into a separate helper function for better testability and readability:
private suspend fun LiveObjectsAdapter.waitForChannelAttach(channelName: String): Unit { val attachDeferred = CompletableDeferred<Unit>() val channel = getChannel(channelName) channel.once { when(it.current) { ChannelState.attached -> attachDeferred.complete(Unit) else -> { val exception = ablyException("Channel $channelName is in invalid state: ${it.current}, " + "error: ${it.reason}", ErrorCode.ChannelStateError) attachDeferred.completeExceptionally(exception) } } } // Handle race condition where channel becomes attached before listener is set if (channel.state == ChannelState.attached) { attachDeferred.complete(Unit) } attachDeferred.await() }This would simplify the main function and make the attaching state logic independently testable.
📜 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/Helpers.kt(5 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-27T12:11:25.084Z
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.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🧬 Code Graph Analysis (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (2)
ablyException(10-18)ablyException(20-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest
- GitHub Check: check-rest-okhttp
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: build
- GitHub Check: check (24)
- GitHub Check: check (19)
🔇 Additional comments (2)
live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
122-125: Verify state checks are performed before mode checksThe implementation correctly checks channel state before mode validation, which aligns with the error handling priority mentioned in the summary.
18-18: Ensure reliable queuing for LiveObjects sendAsync
The Kotlin adapter is now forwardingclientOptions.queueMessagestoconnectionManager.send, whereas the Java implementation hard-coded queuing totrueto guarantee reliable state synchronization. If a user’squeueMessagesis everfalse, LiveObjects operations could fail rather than queue on disconnect.• File: live-objects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
Line 18:connectionManager.send(message, clientOptions.queueMessages, …)Please verify that allowing
queueMessages = falsewon’t compromise LiveObjects reliability. If not, consider hard-coding this argument totrueas before.
2394558 to
a035ee7
Compare
Fixed #1145
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests