[ECO-5076][LiveObjects] Implement getters for objects, map and counter #1138
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added ObjectsStateEvent enum for SYNCING and SYNCED states - Created ObjectsStateChange interface with listener pattern for state changes - Implemented ObjectsSubscription interface for subscription management - Added ObjectsState enum and coordination system with internal/external event emitters - Created HandlesObjectsStateChange interface for state management coordination
- Added getChannelModes and getChannelState methods to LiveObjectsAdapter interface - Implemented channel mode retrieval with fallback from channel options (RTO2a, RTO2b) - Enhanced LiveObjectsPlugin interface with disposal lifecycle documentation - Fixed DefaultLiveObjectsPlugin to use clientError for proper exception handling - Updated Adapter class with channel state and mode management functionality
…h lifecycle tied to given objects instance - Added channel mode and state validation error codes (ChannelModeRequired, ChannelStateError) - Implemented throwIfInvalidAccessApiConfiguration with channel mode validation (RTO2) - Created ObjectsAsyncScope for channel-specific async operations with proper error handling - Added launchWithCallback and launchWithVoidCallback methods for safe async execution - Enhanced validation helpers for channel state and mode requirements
…andle exceptions - Updated ObjectsManager to extend ObjectsStateCoordinator for state management - Fixed state enum references from SYNCED/SYNCING to Synced/Syncing - Added stateChange method with proper state transition logging (RTO2) - Integrated state change event emission through ObjectsStateCoordinator - Enhanced disposal process to include state listener cleanup - Removed unused import from BaseLiveObject
- Updated LiveCounter#data type to AtomicLong to make it thread safe - Replaced generic Callback with ObjectsCallback in async methods - Implemented value() method with channel configuration validation - Added proper adapter validation using throwIfInvalidAccessApiConfiguration - Maintained thread-safe access to counter data through AtomicReference
…ctor param - Added channel state checks for detached and failed - Implemented LiveMap accessor methods: get, entries, keys, values and size - Updated LiveMap data field to be ConcurrentMap for thread safety - Added proper channel configuration validation for all accessor methods - Implemented sequence-based iteration for entries, keys, and values with tombstone filtering - Replaced generic Callback with ObjectsCallback in async methods
- Added getChannelModes and getChannelState methods to adapter - Implemented getRoot method with blocking and async variants using ObjectsAsyncScope - Moved ObjectsState enum to separate file and updated state references - Added proper disposal handling with AblyException instead of string reason - Enhanced async scope management for callback operations - Updated all async method signatures to use ObjectsCallback instead of Callback
- Added getOptions() method to ChannelBase for accessing channel options - Initialize LiveObjects instance on channel creation to process sync messages - Enhanced channel integration with LiveObjects plugin lifecycle management - Fixed channel mode fallback logic by exposing channel options
…ionality - Added UtilsTest with 283 lines of comprehensive utility function tests - Enhanced DefaultLiveObjectsTest with updated state management tests - Updated ObjectsManagerTest with improved sync sequence and state handling tests - Created test coverage for ObjectId validation, state transitions, and error handling - Added tests for ObjectsAsyncScope callback execution and error scenarios
…und async ops - Enhanced IntegrationTest setup with improved test configuration - Updated Sandbox configuration for integration test environment - Improved test infrastructure for Live Objects integration testing - Added better error handling and setup validation for integration tests
- Created PayloadBuilder for constructing test payloads with 135 lines of functionality - Added RestObjects helper with 119 lines for REST API integration testing - Implemented CounterFixtures with test data for LiveCounter integration tests - Created DataFixtures with 84 lines of common test data structures - Added MapFixtures with 157 lines for LiveMap integration test scenarios - Enhanced integration test utilities with comprehensive helper functions
…ad of checking type at runtime - Added DefaultLiveCounterTest with 205 lines of comprehensive integration tests - Created DefaultLiveMapTest with 213 lines covering LiveMap integration scenarios - Implemented test coverage for LiveCounter and LiveMap accessor methods - Added integration tests for object state validation and error handling - Enhanced test suite with real-time synchronization and state change validation
- Added ObjectsCallback interface replacing generic Callback for Live Objects operations - Updated LiveObjects interface to extend ObjectsStateChange and use ObjectsCallback - Refactored LiveCounter and LiveMap async methods to use ObjectsCallback instead of Callback - Added comprehensive Javadoc for ObjectsCallback with operation-specific guidance
WalkthroughThis update introduces and implements core LiveObjects API features, including the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LiveObjects
participant ObjectsManager
participant ObjectsAsyncScope
participant Callback
User->>LiveObjects: getRootAsync(callback)
LiveObjects->>ObjectsAsyncScope: launchWithCallback(callback, suspend getRootAsync)
ObjectsAsyncScope->>LiveObjects: suspend getRootAsync()
LiveObjects->>ObjectsManager: ensureSynced()
ObjectsManager-->>LiveObjects: (returns when synced)
LiveObjects-->>ObjectsAsyncScope: return root LiveMap
ObjectsAsyncScope->>Callback: onSuccess(root LiveMap)
sequenceDiagram
participant User
participant LiveMap
participant LiveObjectsAdapter
User->>LiveMap: get(key)
LiveMap->>LiveObjectsAdapter: throwIfInvalidAccessApiConfiguration(channelName)
LiveObjectsAdapter-->>LiveMap: (returns if valid)
LiveMap-->>User: value or null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
72a8ebb to
b5d0b45
Compare
…alue union type - Added DefaultLiveObjectsTest with 187 lines of comprehensive Live Objects integration tests - Implemented complete integration test suite covering getRoot functionality - Enhanced test coverage for Objects state management and synchronization - Added integration tests for ObjectsCallback interface and async operations - Completed comprehensive test coverage for all Live Objects core functionality
b5d0b45 to
b1c9e47
Compare
0476f23 to
34449c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
60-60: Fix inconsistent return typeThe
value()method returnsDoublebut the JavaDoc on line 56 states it returns "the current value of the counter as a Long". Counter values are typically integers, so this should beLongfor consistency.- Double value(); + Long value();
♻️ Duplicate comments (5)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt (1)
62-63: Same observation as above on pollingPolling for
ObjectsState.Syncedrepeats the potential flakiness; the suggestion above applies here as well.live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt (4)
75-77: Keep assertion messages in sync with enum renamesSame wording mismatch (
SYNCEDvsSynced) – adjust to reflect the new enum naming for consistency.
100-103: Direct state mutations—same concern as in DefaultLiveObjectsTestAssigning
defaultLiveObjects.state = ObjectsState.Syncedsidesteps production pathways. Provide a helper that triggers the real transition to guard against future logic changes.
169-170: Assertion message mismatch for enum renameUpdate “INITIALIZED” → “Initialized” here as well.
180-183: State mutation concern repeatedThe same recommendation about avoiding direct state assignment applies.
🧹 Nitpick comments (13)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/RealtimeObjectsTest.kt (1)
7-14: Rename the test for brevity and intent clarity
testChannelObjectGetterTestrepeats the word Test and does not convey the behaviour being asserted. A concise name such aschannelObjectsGetter_returnsInstance()improves readability and keeps CI reports succinct.- fun testChannelObjectGetterTest() = runTest { + fun channelObjectsGetter_returnsInstance() = runTest {lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java (2)
49-53: Align Javadoc with public API namesThe note references
ablyRealtimeClient.channels.release(channelName)but the public class isAblyRealtime. Consider using the exact type name (AblyRealtime) to avoid confusion for API consumers and future searches.
57-59: Clarify disposal semanticsThe comment says “AblyClient has been closed using client.close()”. The public class is
AblyRealtime, not AblyClient. Updating this wording keeps the documentation consistent and discoverable.live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultLiveObjectsTest.kt (1)
37-38: Potential flakiness due to busy-wait polling
assertWaiter { defaultLiveObjects.state == ObjectsState.Syncing }relies on polling every 100 ms for up to 10 s.
If the coroutine advancing the state is slow under heavy CI load the test may still pass but with a long delay, increasing build time.
Consider emitting aMutableStateFlowor using aCountDownLatchso the test can suspend until the exact signal is emitted instead of polling.live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt (1)
20-22: Message text still refers to legacy constant namesThe assertion message says “state should be INITIALIZED” but the enum name is now
Initialized. Updating messages avoids confusion when reading test failures.- "Initial state should be INITIALIZED" + "Initial state should be Initialized"live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (2)
65-72: Consider direct implementation for keys() method.The current implementation delegates to
entries()which may be less efficient since it resolves all values just to extract keys.Consider a more direct implementation:
override fun keys(): Iterable<String> { - val iterableEntries = entries() - return sequence { - for (entry in iterableEntries) { - yield(entry.key) // RTLM12b - } - }.asIterable() + adapter.throwIfInvalidAccessApiConfiguration(channelName) + return sequence { + for ((key, entry) in data.entries) { + if (!entry.isEntryOrRefTombstoned(objectsPool)) { + yield(key) + } + } + }.asIterable() }
74-81: Consider direct implementation for values() method.Similar to
keys(), this could be more efficient with direct implementation rather than delegating toentries().Consider a more direct implementation:
override fun values(): Iterable<Any> { - val iterableEntries = entries() - return sequence { - for (entry in iterableEntries) { - yield(entry.value) // RTLM13b - } - }.asIterable() + adapter.throwIfInvalidAccessApiConfiguration(channelName) + return sequence { + for ((_, entry) in data.entries) { + val value = entry.getResolvedValue(objectsPool) + value?.let { yield(it) } + } + }.asIterable() }live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt (2)
3-5: Remove duplicate importsLines 4-5 contain redundant imports that are already covered by the wildcard import on line 3.
import io.ably.lib.objects.* -import io.ably.lib.objects.ObjectData -import io.ably.lib.objects.ObjectValue import io.ably.lib.objects.integration.helpers.fixtures.createUserMapObject
205-205: Use consistent Long literal for size comparisonFor consistency with other size assertions in the test, use
3Linstead of3.- assertEquals(3, testMap.size(), "Final map should have exactly 3 entries") + assertEquals(3L, testMap.size(), "Final map should have exactly 3 entries")live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt (1)
10-10: Remove unused importThe
sizeimport is not used in this file. Thesize()calls in the tests are methods on LiveMap instances, not this imported function.-import io.ably.lib.objects.size import io.ably.lib.objects.state.ObjectsStateEventlive-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
62-98: Consider extracting duplicated error handling logicThe error handling logic is duplicated between
launchWithCallbackandlaunchWithVoidCallback. Consider extracting it to reduce duplication and improve maintainability.+ private fun handleError(tag: String, callback: ObjectsCallback<*>, throwable: Throwable) { + when (throwable) { + is AblyException -> callback.onError(throwable) + else -> { + val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) + callback.onError(ex) + } + } + } + internal fun <T> launchWithCallback(callback: ObjectsCallback<T>, block: suspend () -> T) { scope.launch { try { val result = block() try { callback.onSuccess(result) } catch (t: Throwable) { Log.e(tag, "Error occurred while executing callback's onSuccess handler", t) } // catch and don't rethrow error from callback } catch (throwable: Throwable) { - when (throwable) { - is AblyException -> { callback.onError(throwable) } - else -> { - val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) - callback.onError(ex) - } - } + handleError(tag, callback, throwable) } } } internal fun launchWithVoidCallback(callback: ObjectsCallback<Void>, block: suspend () -> Unit) { scope.launch { try { block() try { callback.onSuccess(null) } catch (t: Throwable) { Log.e(tag, "Error occurred while executing callback's onSuccess handler", t) } // catch and don't rethrow error from callback } catch (throwable: Throwable) { - when (throwable) { - is AblyException -> { callback.onError(throwable) } - else -> { - val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) - callback.onError(ex) - } - } + handleError(tag, callback, throwable) } } }live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.kt (2)
22-24: Improve readability of null-check with exception.The line break between the Elvis operator and exception makes the code less readable.
- return operationRequest(channelName, mapCreateOp).objectId ?: - throw Exception("Failed to create map: no objectId returned") + return operationRequest(channelName, mapCreateOp).objectId + ?: throw Exception("Failed to create map: no objectId returned")
58-60: Improve readability of null-check with exception.Same formatting issue as above for consistency.
- return operationRequest(channelName, counterCreateOp).objectId - ?: throw Exception("Failed to create counter: no objectId returned") + return operationRequest(channelName, counterCreateOp).objectId + ?: throw Exception("Failed to create counter: no objectId returned")
live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/PayloadBuilder.kt
Show resolved
Hide resolved
ttypic
approved these changes
Aug 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation