Setup : LiveObject plugin #1085
Conversation
|
""" WalkthroughThis change introduces the LiveObjects plugin architecture into the codebase. It adds new interfaces and implementations for live data objects, updates core SDK classes to integrate with the plugin, and sets up a dedicated Kotlin module for LiveObjects. Build scripts, enums, and protocol message types are extended to support new object-related operations and capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AblyRealtime
participant Plugin as LiveObjectsPlugin
participant Channel as Channel
participant ConnMgr as ConnectionManager
Client->>Plugin: tryInitializeLiveObjectsPlugin()
Plugin-->>Client: LiveObjectsPlugin instance or null
Client->>ConnMgr: new ConnectionManager(..., liveObjectsPlugin)
Client->>Channel: new Channel(..., liveObjectsPlugin)
Channel->>Plugin: getObjects()
Note over ConnMgr,Plugin: On protocol message (object/object_sync)
ConnMgr->>Plugin: handle(message)
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java (1)
19-24: Consider adding lifecycle information to dispose method documentationWhile the documentation clearly explains what the method does, consider adding information about when this method should be called and what happens to any resources held by the LiveObjects instance after disposal.
/** * Disposes of the LiveObjects instance associated with the specified channel name. * + * This method should be called when the channel is no longer needed to free up resources. + * After disposal, any references to the LiveObjects instance should be considered invalid. * * @param channelName the name of the channel whose LiveObjects instance is to be removed. */lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java (1)
49-56: Consider clarifying the purpose of the value parameter in remove methodThe
removemethod requires both a key and a value, which is unusual for map operations that typically only require the key. If this is for optimistic concurrency control (i.e., the value must match what's currently stored), it should be documented explicitly./** * Removes the specified key-value pair from the live map. * * @param keyName the name of the key to remove. - * @param value the value associated with the key to remove. + * @param value the expected value associated with the key; removal occurs only if the current value matches. */ void remove(String keyName, Object value);lib/src/main/java/io/ably/lib/objects/LiveMap.java (1)
49-56: Consider clarifying the value parameter's role in the remove methodSimilar to BatchContextLiveMap, the remove method takes both key and value parameters. If this is for optimistic concurrency control, this should be explicitly documented.
/** * Removes the specified key and its associated value from the map. * * @param keyName the key to be removed. - * @param value the value associated with the key to be removed. + * @param value the expected value associated with the key; removal occurs only if the current value matches. */ void remove(String keyName, Object value);live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
7-49: Add implementation timeline or specs for TODOsAll methods currently use
TODO("Not yet implemented"). Consider adding more specific information in these TODO messages, such as implementation plans, requirements, or JIRA ticket references to make it clear what the expected behavior should be and to guide future implementation work.-override fun getRoot(): LiveMap { - TODO("Not yet implemented") +override fun getRoot(): LiveMap { + TODO("Implement according to live objects spec - LO-123")lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
45-50: Consider adding a public method to check if LiveObjects plugin is availableThe LiveObjectsPlugin is initialized via reflection, making it optional, but there's no public way to check if it's available. Consider adding a
hasLiveObjectsPlugin()method to allow clients to determine if they can use LiveObjects functionality.+/** + * Checks if the LiveObjects plugin is available. + * + * @return true if the LiveObjects plugin is available, false otherwise. + */ +public boolean hasLiveObjectsPlugin() { + return liveObjectsPlugin != null; +}lib/src/main/java/io/ably/lib/objects/LiveObjects.java (2)
22-28: Consider return type for batch operationThe
batchmethod doesn't return anything, which means callers have no way to know when the batch operation completes except through the callback. Consider returning aFuture,CompletableFuture, or similar to allow more flexible handling of the asynchronous operation.-void batch(Callback<BatchContext> batchContextCallback); +CompletableFuture<BatchContext> batch(Callback<BatchContext> batchContextCallback);
53-60: Clarify default behavior for null initialValueThe
createCountermethod accepts aLong initialValueparameter, but it's not clear what happens if null is passed. Consider documenting the default behavior./** * Creates a new LiveCounter with an initial value. * - * @param initialValue the initial value of the LiveCounter. + * @param initialValue the initial value of the LiveCounter. If null, the counter will start at 0. * @return the newly created LiveCounter instance. */ LiveCounter createCounter(Long initialValue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
android/src/main/java/io/ably/lib/realtime/Channel.java(2 hunks)java/src/main/java/io/ably/lib/realtime/Channel.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveCounter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjects.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(4 hunks)live-objects/build.gradle.kts(1 hunks)live-objects/gradle.properties(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)settings.gradle.kts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java (2)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (2)
getInstance(7-9)dispose(11-13)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
channelName(6-50)
lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (2)
batch(11-13)getRoot(7-9)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (2)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
channelName(6-50)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (1)
dispose(11-13)
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (11)
batch(11-13)getRoot(7-9)createMap(15-17)createMap(19-21)createMap(23-25)createCounter(27-29)getRootAsync(31-33)createMapAsync(35-37)createMapAsync(39-41)createMapAsync(43-45)createCounterAsync(47-49)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check-realtime
- GitHub Check: check
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest
- GitHub Check: build
🔇 Additional comments (21)
settings.gradle.kts (1)
18-18: LGTM: New module integrationThe addition of the "live-objects" module to the Gradle settings is correct and follows the established naming convention in the project.
lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java (1)
1-17:❌ Incorrect review comment
Well-structured interface definition with good documentation
The BatchContext interface is clearly documented and defines a focused API for batch operations on live data objects. The interface follows good Java practices with comprehensive Javadoc.
However, I notice that while this interface's
getRoot()returns aBatchContextLiveMap, the corresponding method inDefaultLiveObjects.ktreturns aLiveMap. Please ensure these types are compatible in the implementation.
🏁 Script executed:
#!/bin/bash # Verify the relationship between BatchContextLiveMap and LiveMap echo "Checking for BatchContextLiveMap definition..." fd --type file --extension java --exec grep -l "BatchContextLiveMap" {} \; echo "Searching for relationship between BatchContextLiveMap and LiveMap..." fd --type file --extension java --exec grep -l "BatchContextLiveMap.*extends.*LiveMap\|interface.*BatchContextLiveMap.*LiveMap" {} \;Length of output: 603
I’ll pull in the definitions of BatchContextLiveMap and inspect how getRoot() is implemented in DefaultLiveObjects.kt to confirm their types align.
🏁 Script executed:
#!/bin/bash set -e # Show BatchContextLiveMap definition echo "===== lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java =====" sed -n '1,200p' lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java # Locate DefaultLiveObjects.kt echo echo "Searching for DefaultLiveObjects.kt..." KT_FILE=$(fd --type f --extension kt -x grep -l "class DefaultLiveObjects" {} | head -n1) if [ -z "$KT_FILE" ]; then echo "ERROR: DefaultLiveObjects.kt not found" exit 1 fi echo "Found: $KT_FILE" # Find and show getRoot() implementation echo echo "Looking for getRoot() in DefaultLiveObjects.kt..." grep -R -n "getRoot" "$KT_FILE" || true LINE=$(grep -R -n "getRoot" "$KT_FILE" | head -n1 | cut -d: -f1) if [ -n "$LINE" ]; then START=$((LINE-5)) [ "$START" -lt 1 ] && START=1 END=$((LINE+15)) echo echo "===== Context around getRoot() (lines $START–$END) =====" sed -n "${START},${END}p" "$KT_FILE" else echo "No getRoot() method found in $KT_FILE" fiLength of output: 3136
I’ll search the Kotlin sources for where
BatchContextis implemented and howgetRoot()is defined to verify the return types align.
🏁 Script executed:
#!/bin/bash set -e echo "=== Looking for override fun getRoot in Kotlin sources ===" grep -R -n "override fun getRoot" live-objects/src/main/kotlin || true echo echo "=== Finding classes that implement BatchContext ===" grep -R -n ":.*BatchContext" live-objects/src/main/kotlin || true echo echo "=== All references to BatchContext in Kotlin ===" grep -R -n "BatchContext" live-objects/src/main/kotlinLength of output: 1200
Gathering the
LiveMapdefinition to confirm it implementsBatchContextLiveMap.
🏁 Script executed:
#!/bin/bash set -e echo "=== Locating LiveMap interface in Kotlin sources ===" fd --type f --extension kt -x grep -l "interface LiveMap" {} || true echo echo "=== Locating LiveMap class in Kotlin sources ===" fd --type f --extension kt -x grep -l "class LiveMap" {} || true echo for file in $(fd --type f --extension kt -x grep -l "interface LiveMap" {}); do echo "===== $file =====" sed -n '1,200p' "$file" doneLength of output: 452
I’ll locate all
LiveMapdefinitions across the repo (both Java and Kotlin) to confirm whether it implements or aliasesBatchContextLiveMap:
🏁 Script executed:
#!/bin/bash set -e echo "=== Files named *LiveMap*.java or *.kt ===" fd --type f -e java -e kt "LiveMap" . echo echo "=== Java: looking for LiveMap in lib/src/main/java ===" grep -R -n "interface LiveMap" lib/src/main/java || true grep -R -n "class LiveMap" lib/src/main/java || true echo echo "=== Kotlin: looking for LiveMap/interface/typealias definitions ===" grep -R -n "interface LiveMap" . || true grep -R -n "class LiveMap" . || true grep -R -n "typealias LiveMap" . || trueLength of output: 929
Inspecting the actual LiveMap definition and the Kotlin implementation:
🏁 Script executed:
#!/bin/bash set -e echo "===== lib/src/main/java/io/ably/lib/objects/LiveMap.java =====" sed -n '1,200p' lib/src/main/java/io/ably/lib/objects/LiveMap.java echo echo "===== live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt =====" sed -n '1,200p' live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.ktLength of output: 4291
Ignore incorrect type-compatibility concern
The
getRoot()inBatchContext(returningBatchContextLiveMap) is part of the batch-operation API, whereasgetRoot()inDefaultLiveObjectsimplements theLiveObjectsinterface and correctly returns aLiveMap. These are two separate methods in different interfaces—there’s no need forBatchContextLiveMapandLiveMapto be related.No changes are required here.
Likely an incorrect or invalid review comment.
android/src/main/java/io/ably/lib/realtime/Channel.java (2)
6-6: LGTM: Import for LiveObjectsPluginThe import is correctly added to support the new parameter in the constructor.
17-18: LGTM: Constructor updated for LiveObjects supportThe constructor has been properly expanded to include the LiveObjectsPlugin parameter and correctly passes it to the superclass constructor.
java/src/main/java/io/ably/lib/realtime/Channel.java (2)
3-3: LGTM: Import for LiveObjectsPluginThe import is correctly added to support the new parameter in the constructor.
8-9: LGTM: Constructor updated for LiveObjects supportThe constructor has been properly expanded to include the LiveObjectsPlugin parameter and correctly passes it to the superclass constructor. This mirrors the same change made in the Android version, ensuring consistent implementation across platforms.
live-objects/gradle.properties (1)
1-4: Maven POM properties are well-defined and appropriateThe Maven POM properties for the new 'live-objects' module are clearly defined with appropriate values. The artifact ID, name, description, and packaging type follow standard Maven/Gradle conventions and accurately describe the purpose of this module.
live-objects/build.gradle.kts (3)
1-4: Good use of Gradle pluginsThe build script correctly applies the
java-libraryplugin and uses the plugin alias for Kotlin JVM, which is a good practice for centralizing version management.
10-13: Dependencies look appropriate for the moduleThe dependencies section correctly sets up the implementation dependency on the
:javaproject module and test dependencies on Kotlin test libraries.
19-21: Excellent use of explicitApi for library developmentUsing
explicitApi()is a great practice for library modules as it enforces explicit visibility modifiers and return types, making the API surface clearer for consumers.lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java (2)
3-7: Clear and concise documentationThe JavaDoc provides a good high-level description of the plugin's purpose. This helps users understand the intent of the interface.
10-16: Well-documented API methodThe
getInstancemethod is well-documented with clear parameter and return descriptions.live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (2)
7-9: Good use of Kotlin's getOrPut functionThe implementation efficiently uses Kotlin's
getOrPutfunction, which handles the cache lookup and creation pattern in a concise way.
11-13:❌ Incorrect review comment
Consider adding resource cleanup in dispose
The
disposemethod only removes the entry from the cache. If theLiveObjectsinstance holds any resources that need explicit cleanup (such as event listeners, timers, or network connections), consider adding cleanup logic before removing it from the cache.
🏁 Script executed:
#!/bin/bash # Check if LiveObjects implementation has any cleanup methods or resources that need disposal echo "Checking LiveObjects interface for cleanup methods..." grep -r "close\|dispose\|cleanup" --include="*.java" --include="*.kt" lib/src/main/java/io/ably/lib/objects/ live-objects/src/main/kotlin/io/ably/lib/objects/ echo "Checking DefaultLiveObjects implementation for resources that might need cleanup..." grep -r "private val\|private var" --include="DefaultLiveObjects.kt" live-objects/src/main/kotlin/io/ably/lib/objects/Length of output: 948
I’d like to inspect the plugin itself to see what resources it holds. Let’s dump the contents of DefaultLiveObjectsPlugin.kt and look for any private fields:
🏁 Script executed:
#!/bin/bash echo "===== Contents of DefaultLiveObjectsPlugin.kt =====" sed -n '1,200p' live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt echo echo "===== Any private fields in DefaultLiveObjectsPlugin.kt =====" grep -E "private (val|var)" -n live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.ktLength of output: 890
Let’s inspect the
DefaultLiveObjectsimplementation to see if it holds any subscriptions, listeners, or other resources needing explicit cleanup:
🏁 Script executed:
#!/bin/bash echo "===== DefaultLiveObjects.kt =====" sed -n '1,200p' live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt echo echo "===== Resource-like members in DefaultLiveObjects =====" grep -E "private (val|var)" -n live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.ktLength of output: 1849
No additional cleanup needed in
disposeThe
DefaultLiveObjectsPlugin.dispose(channelName)method only drops the cachedDefaultLiveObjectsinstance, andDefaultLiveObjectsitself currently holds no subscriptions, listeners, timers, or other disposable resources—just a channel name string. There’s nothing to clean up today. If you introduce resource-holding fields inDefaultLiveObjectsin the future, you can extenddisposeto call their cleanup methods at that time.Likely an incorrect or invalid review comment.
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
1-42: Good implementation of the LiveCounter interfaceThe LiveCounter interface is well-designed with a clear separation between synchronous and asynchronous operations. The documentation is thorough and properly explains the purpose of each method.
lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java (1)
1-63: Well-structured batch context interfaceThe BatchContextLiveMap interface provides a clear and consistent API for batch operations on live maps. The methods align well with standard map operations while preserving the stateful nature of batch processing.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
96-106: Good integration of LiveObjects plugin with clear error messagingThe LiveObjects plugin integration is well-implemented with appropriate null checking and a helpful error message that guides users on how to add the missing dependency.
1301-1312: Appropriate constructor update for plugin integrationThe constructor change properly integrates the LiveObjectsPlugin while maintaining backward compatibility with the existing functionality.
lib/src/main/java/io/ably/lib/objects/LiveMap.java (1)
1-82: Well-designed LiveMap interface with consistent sync/async patternsThe LiveMap interface provides a comprehensive API for real-time map operations with both synchronous and asynchronous variants. The method signatures and documentation are clear and consistent with other interfaces in the codebase.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (1)
6-6: Class visibility is appropriately restrictedThe
internalmodifier correctly limits the accessibility of this implementation to within the module, enforcing that consumers should work with the interface rather than the implementation.lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
238-240:❌ Incorrect review comment
Ensure thread safety when accessing liveObjectsPlugin
While the
liveObjectsPluginfield is final, it's good practice to synchronize or use atomic operations when accessing it in thereleasemethod to ensure thread safety, especially since it might be called from different threads.
🏁 Script executed:
#!/bin/bash # Search for synchronization patterns in Channel-related operations rg -A 2 -B 2 "synchronized.*Channel" lib/src/main/java/io/ably/lib/realtime/Length of output: 1642
I’d like to confirm how
liveObjectsPluginis used and whether its access is already protected. Let’s pull in the surrounding context and look for any existing synchronization inAblyRealtime.java:
🏁 Script executed:
#!/bin/bash # Show references to liveObjectsPlugin with context rg -n "liveObjectsPlugin" -A3 -B3 lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java # Find where dispose is called in this class rg -n "dispose" -n lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java # Check for any synchronized blocks in AblyRealtime rg -n "synchronized" -n lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java # Locate the release method to see its signature and annotations rg -n "release" -A5 -B5 lib/src/main/java/io/ably/lib/realtime/AblyRealtime.javaLength of output: 3552
No additional synchronization required for liveObjectsPlugin
TheliveObjectsPluginfield isfinaland safely published once during construction. Reading a final field is thread-safe, and because you remove the channel from the concurrentmapbefore callingdispose(), you’re guaranteed it only runs once per channel. If the plugin itself needs to guard its internal state against concurrent disposal, that logic belongs inside its own implementation, not here.Likely an incorrect or invalid review comment.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
Outdated
Show resolved
Hide resolved
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
Outdated
Show resolved
Hide resolved
1. Declared required liveobject java interfaces 2. Implemented liveobject interface for LiveObjectsPlugin 3. Declared batch specific public interface methods
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
abe200c to
c435f5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (3)
8-13: Add thread safety documentation to interfaceThe interface lacks information about thread safety guarantees. Consider adding explicit documentation about whether implementations should be thread-safe and any synchronization requirements for callers.
/** * The LiveObjects interface provides methods to interact with live data objects, * such as maps and counters, in a real-time environment. It supports both synchronous * and asynchronous operations for retrieving and creating live objects. + * + * <p>Implementations of this interface must be thread-safe as they may be accessed + * from multiple threads concurrently. */
15-20: Specify error handling behavior for synchronous methodsThe synchronous methods like
getRoot()don't specify how errors are communicated to callers. Consider documenting whether these methods throw exceptions and which types of exceptions callers should handle./** * Retrieves the root LiveMap object. * * @return the root LiveMap instance. + * @throws LiveObjectException if the root map cannot be retrieved or accessed. */ LiveMap getRoot();
45-52: Document nullability expectations for parametersThe interface doesn't specify whether
nullvalues are acceptable for parameters. Consider adding@NonNullannotations or explicit documentation about null handling for each parameter./** * Creates a new LiveMap based on a standard Java Map. * - * @param map the Java Map to base the new LiveMap on. + * @param map the Java Map to base the new LiveMap on. Must not be null. * @return the newly created LiveMap instance. + * @throws IllegalArgumentException if map is null. */ LiveMap createMap(Map<String, Object> map);
🧹 Nitpick comments (1)
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (1)
53-60: Consider using primitive type for initialValueThe
initialValueparameter is defined asLong(object wrapper) rather thanlong(primitive). Unless you need to represent a null initial value, consider using the primitive type for better performance./** * Creates a new LiveCounter with an initial value. * - * @param initialValue the initial value of the LiveCounter. + * @param initialValue the initial value of the LiveCounter. If null, a default value of 0 will be used. * @return the newly created LiveCounter instance. */ -LiveCounter createCounter(Long initialValue); +LiveCounter createCounter(Long initialValue); // Keep as Long if null values are intended to be supported
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (16)
android/src/main/java/io/ably/lib/realtime/Channel.java(2 hunks)java/src/main/java/io/ably/lib/realtime/Channel.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveCounter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjects.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContextBuilder.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(4 hunks)live-objects/build.gradle.kts(1 hunks)live-objects/gradle.properties(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)settings.gradle.kts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/main/java/io/ably/lib/objects/batch/BatchContextBuilder.java
🚧 Files skipped from review as they are similar to previous changes (14)
- settings.gradle.kts
- android/src/main/java/io/ably/lib/realtime/Channel.java
- java/src/main/java/io/ably/lib/realtime/Channel.java
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
- lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java
- lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java
- lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java
- live-objects/build.gradle.kts
- lib/src/main/java/io/ably/lib/objects/LiveCounter.java
- lib/src/main/java/io/ably/lib/objects/LiveMap.java
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- live-objects/gradle.properties
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (11)
batch(11-13)getRoot(7-9)createMap(15-17)createMap(19-21)createMap(23-25)createCounter(27-29)getRootAsync(31-33)createMapAsync(35-37)createMapAsync(39-41)createMapAsync(43-45)createCounterAsync(47-49)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/src/test/java/io/ably/lib/test/common/Setup.java (1)
172-187: Consider improvements to the internet availability check implementation.The
checkInternetAvailability()method has a few areas that could be improved:
- The URL and timeout values are hard-coded. Consider making these configurable, perhaps as constants or properties.
- The method wraps all exceptions into a generic Exception, which loses specific error details. Consider preserving the original exception type or providing more detailed error messages.
- The connection is not explicitly closed after use. Although Java will eventually clean it up, it's best practice to close resources explicitly.
public static void checkInternetAvailability() throws Exception { HttpURLConnection connection = null; try { String testUrl = "https://internet-up.ably-realtime.com/is-the-internet-up.txt"; connection = (HttpURLConnection) new URL(testUrl).openConnection(); connection.setRequestMethod("GET"); connection.setConnectTimeout(5000); // Set timeout to 5 seconds connection.setReadTimeout(5000); int responseCode = connection.getResponseCode(); if (responseCode != 200) { throw new IOException("Host is not available. Response code: " + responseCode); } } catch (Exception e) { throw new Exception("Internet is not available", e); + } finally { + if (connection != null) { + connection.disconnect(); + } } }lib/src/main/java/io/ably/lib/plugins/objects/LiveMap.java (1)
67-83: Consider consistent return types between sync and async methodsThe asynchronous methods are correctly defined with callbacks, but there's an inconsistency in the return types compared to their synchronous counterparts:
- Synchronous methods
setandremovereturnvoid- Asynchronous methods
setAsyncandremoveAsyncuseCallback<Void>Consider whether it would be more consistent to have:
- Both return
void(current implementation)- Synchronous methods return some result type that's passed to the async callback
- Or add a
booleanreturn value for synchronous methods to indicate success/failureAlso, consider adding asynchronous equivalents for other synchronous methods like
get,entries,keys,values, andsizefor consistency and to provide non-blocking alternatives for all operations.lib/src/main/java/io/ably/lib/plugins/objects/LiveObjects.java (2)
63-109: Consistent async API designThe asynchronous methods are correctly defined with callbacks that match their synchronous counterparts. The pattern of providing both synchronous and asynchronous versions of each method is a good practice for a Java library, allowing flexibility for different threading models.
One minor improvement would be to consider adding an
@NotNullannotation to theinitialValueparameter ofcreateCounterandcreateCounterAsyncmethods to be consistent with other parameters, although the current implementation correctly uses@NotNullin the JavaDoc for these parameters.
37-53: Consider validation for map transformationsThe methods
createMap(LiveMap),createMap(LiveCounter), andcreateMap(Map<String, Object>)transform different objects into a LiveMap.Since these are interface methods, they don't contain implementation details, but it would be helpful to document:
- What happens with null values in the source map/counter?
- Are there any restrictions on the types of objects that can be stored as values?
- Is this a deep or shallow copy of the source data?
This additional documentation would help implementers understand the expected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (15)
gradle/libs.versions.toml(2 hunks)java/build.gradle.kts(1 hunks)java/src/main/java/io/ably/lib/realtime/Channel.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/LiveCounter.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/LiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/LiveObjects.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/LiveObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContext.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContextBuilder.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContextLiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(4 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContextBuilder.java
- gradle/libs.versions.toml
- lib/src/main/java/io/ably/lib/plugins/objects/LiveCounter.java
- lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContextLiveMap.java
🚧 Files skipped from review as they are similar to previous changes (5)
- java/src/main/java/io/ably/lib/realtime/Channel.java
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContext.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (2)
batch(14-16)getRoot(10-12)
lib/src/main/java/io/ably/lib/plugins/objects/LiveObjects.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (12)
batch(14-16)getRoot(10-12)createMap(18-20)createMap(22-24)createMap(26-28)createCounter(54-56)getRootAsync(30-32)batchAsync(34-36)createMapAsync(38-40)createMapAsync(42-44)createMapAsync(46-48)createCounterAsync(50-52)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check-realtime
- GitHub Check: build
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: check
🔇 Additional comments (6)
java/build.gradle.kts (1)
23-23: Added JetBrains annotations as a compile-only dependency
UsingcompileOnly(libs.jetbrains)is appropriate for null-safety and purity annotations without impacting the runtime classpath. Please verify that thelibs.jetbrainsalias is correctly defined in the version catalog and resolves to the intendedorg.jetbrains:annotationsartifact.lib/src/test/java/io/ably/lib/test/common/Setup.java (1)
4-5: LGTM - appropriate imports for HttpURLConnection functionality.The added imports support the new network connectivity check feature.
lib/src/main/java/io/ably/lib/plugins/objects/batch/BatchContext.java (1)
1-19: Well-designed interface with clear documentationThe
BatchContextinterface is well-structured with appropriate JavaDoc comments explaining its purpose and contract. The@NotNullannotation on the return type ofgetRoot()ensures API consumers are aware the method won't return null.Consider whether the interface should include methods for committing or rolling back a batch operation if these aren't handled elsewhere in the API.
lib/src/main/java/io/ably/lib/plugins/objects/LiveObjectsPlugin.java (1)
1-26: Clean interface design following best practicesThe
LiveObjectsPlugininterface provides a clear contract for managing LiveObjects instances associated with channel names. The method signatures are well-defined, and the JavaDoc comments provide good documentation on the purpose of each method.The use of
@NotNullannotations correctly enforces that channel names must not be null, which helps prevent null pointer exceptions at runtime.The interface is appropriately focused on a single responsibility - managing the lifecycle of LiveObjects instances.
lib/src/main/java/io/ably/lib/plugins/objects/LiveMap.java (1)
1-66: Well-structured interface with good documentationThe
LiveMapinterface provides a comprehensive API for interacting with a live, real-time map structure. The synchronous methods are well-defined with appropriate return types and parameter annotations.The
@Contract(pure = true)annotation on thesize()method is a good practice, indicating that the method doesn't modify the state of the object.All methods have clear JavaDoc comments explaining their purpose and parameters.
lib/src/main/java/io/ably/lib/plugins/objects/LiveObjects.java (1)
1-62: Well-structured interface with comprehensive operationsThe
LiveObjectsinterface provides a comprehensive set of methods for interacting with live data objects. The synchronous methods are well-defined with appropriate return types and parameter annotations.All methods have clear JavaDoc comments explaining their purpose and parameters.
The interface provides good abstraction for creating different types of live objects (maps and counters) and supports batch operations.
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
86a035e to
6b291d0
Compare
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
6b291d0 to
d2181b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java (1)
1-46: Consider adding support for custom increment/decrement valuesThe interface is well-defined with both synchronous and asynchronous methods, but it only supports incrementing and decrementing by 1. Consider extending the interface to support incrementing/decrementing by arbitrary values, which could provide more flexibility.
public interface LiveCounter { void increment(); void incrementAsync(@NotNull Callback<Void> callback); void decrement(); void decrementAsync(@NotNull Callback<Void> callback); + void incrementBy(long delta); + void incrementByAsync(long delta, @NotNull Callback<Void> callback); + void decrementBy(long delta); + void decrementByAsync(long delta, @NotNull Callback<Void> callback); @NotNull @Contract(pure = true) Long value(); }lib/src/main/java/io/ably/lib/objects/LiveObjects.java (5)
20-25: Consider adding error handling documentation.The
getRoot()method doesn't specify how errors are communicated to callers. While I understand from previous comments that specific exception types aren't known yet, it would be helpful to at least indicate that exceptions might be thrown in case of errors./** * Retrieves the root LiveMap object. * * @return the root LiveMap instance. + * @throws Exception if an error occurs while retrieving the root map. */ LiveMap getRoot();
58-64: Consider using primitive long instead of Long object.The
initialValueparameter is defined asLong(object) rather thanlong(primitive). This requires boxing/unboxing and could have performance implications for a potentially frequently used counter. Unless you specifically need to support null values (which contradicts the@NotNullannotation), consider using the primitive type./** * Creates a new LiveCounter with an initial value. * - * @param initialValue the initial value of the LiveCounter. + * @param initialValue the initial value of the LiveCounter as a primitive long. * @return the newly created LiveCounter instance. */ -LiveCounter createCounter(@NotNull Long initialValue); +LiveCounter createCounter(long initialValue);
105-111: Maintain consistency with primitive types if changing the synchronous method.If you update the synchronous
createCountermethod to use the primitivelongas suggested, make sure to update this asynchronous version as well for consistency./** * Asynchronously creates a new LiveCounter with an initial value. * - * @param initialValue the initial value of the LiveCounter. + * @param initialValue the initial value of the LiveCounter as a primitive long. * @param callback the callback to handle the result or error. */ -void createCounterAsync(@NotNull Long initialValue, @NotNull Callback<LiveCounter> callback); +void createCounterAsync(long initialValue, @NotNull Callback<LiveCounter> callback);
50-56: Document acceptable value types for the map.The
createMapmethod accepts aMap<String, Object>but doesn't specify what types of objects are allowed as values. Consider documenting the acceptable value types to help API users understand limitations./** * Creates a new LiveMap based on a standard Java Map. * - * @param map the Java Map to base the new LiveMap on. + * @param map the Java Map to base the new LiveMap on. Values must be of supported types + * (e.g., String, Number, Boolean, Map, List, or other supported Ably types). * @return the newly created LiveMap instance. */ LiveMap createMap(@NotNull Map<String, Object> map);
97-103: Maintain value type documentation consistency.If you add documentation about acceptable value types to the synchronous
createMapmethod, make sure to add similar documentation to this asynchronous version for consistency./** * Asynchronously creates a new LiveMap based on a standard Java Map. * - * @param map the Java Map to base the new LiveMap on. + * @param map the Java Map to base the new LiveMap on. Values must be of supported types + * (e.g., String, Number, Boolean, Map, List, or other supported Ably types). * @param callback the callback to handle the result or error. */ void createMapAsync(@NotNull Map<String, Object> map, @NotNull Callback<LiveMap> callback);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
android/build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)java/build.gradle.kts(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveCounter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjects.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContextBuilder.java(1 hunks)lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(6 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/main/java/io/ably/lib/objects/batch/BatchContext.java
- lib/src/main/java/io/ably/lib/objects/batch/BatchContextBuilder.java
🚧 Files skipped from review as they are similar to previous changes (7)
- java/build.gradle.kts
- android/build.gradle.kts
- gradle/libs.versions.toml
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
- lib/src/test/java/io/ably/lib/test/common/Setup.java
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt (12)
batch(11-13)getRoot(7-9)createMap(15-17)createMap(19-21)createMap(23-25)createCounter(51-53)getRootAsync(27-29)batchAsync(31-33)createMapAsync(35-37)createMapAsync(39-41)createMapAsync(43-45)createCounterAsync(47-49)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
🔇 Additional comments (3)
lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java (1)
1-26: Well-structured interface with clear documentationThe LiveObjectsPlugin interface is well-designed with thorough JavaDoc comments and proper null safety annotations. It provides a clean contract for managing LiveObjects instances associated with channels.
lib/src/main/java/io/ably/lib/objects/batch/BatchContextLiveMap.java (1)
1-76: Interface properly defined with appropriate annotationsThe BatchContextLiveMap interface is well-structured with appropriate null safety, unmodifiability, and pure contract annotations. The documentation is thorough and clear.
lib/src/main/java/io/ably/lib/objects/LiveObjects.java (1)
1-18: Good interface definition with clear documentation.The LiveObjects interface is well-documented with clear descriptions of its purpose and thread safety requirements. The imports are appropriate for the functionality being defined.
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
d2181b1 to
0a108a1
Compare
…methods 2. Added internet connectivity check to test setup
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
0a108a1 to
bce4ba9
Compare
2. Added live objects specific channel modes and flags to protocol message
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
21-28: Consider updating class documentation.The class documentation should be updated to mention that
ProtocolMessagenow also supports Live Objects functionality through the new action types and flags./** * A message sent and received over the Realtime protocol. * A ProtocolMessage always relates to a single channel only, but * can contain multiple individual Messages or PresenceMessages. * ProtocolMessages are serially numbered on a connection. + * ProtocolMessage also supports Live Objects functionality through dedicated + * action types and flags. * See the Ably client library developer documentation for further * details on the members of a ProtocolMessage. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveMap.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjects.java(1 hunks)lib/src/main/java/io/ably/lib/types/ChannelMode.java(1 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/main/java/io/ably/lib/types/ChannelMode.java
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/main/java/io/ably/lib/objects/LiveCounter.java
- lib/src/main/java/io/ably/lib/objects/LiveObjects.java
- lib/src/main/java/io/ably/lib/objects/LiveMap.java
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check
🔇 Additional comments (4)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (4)
48-52: Appropriate extension of Action enum for Live Objects support.The new action types
auth,activate,object,object_sync, andannotationare added to support the Live Objects plugin functionality. These follow the existing naming convention and extend the protocol message capabilities appropriately.
64-65: Clear documentation and appropriate flag addition.The
has_objectsflag is properly documented with a clear comment and placed at bit position 7, maintaining the existing pattern for flag declarations.
70-76: Well-organized flags with appropriate categorization.The new flags are logically grouped with clear comments separating different categories:
- Channel mode flags (presence_subscribe)
- Annotation flags (annotation_publish, annotation_subscribe)
- Object flags (object_subscribe, object_publish)
The bit positions are appropriately spaced and follow the existing pattern.
89-91:❓ Verification inconclusive
Consider whether ackRequired should be updated.
The
ackRequiredmethod currently only returns true formessageandpresenceactions. Consider whether any of the new actions (object,annotation, etc.) require acknowledgments and should be included in this method.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase that might indicate whether # the new message types require acknowledgment # Look for pattern matching in the codebase related to ack requirements rg -i "ack.*required|require.*ack" --type javaLength of output: 4241
Verify ackRequired covers new Actions in ProtocolMessage
A search for “ackRequired” and related patterns shows it’s only ever used for
Action.messageandAction.presence. TheActionenum also includesobjectandannotation(and potentially other new actions), so please confirm whether these should trigger acknowledgments. If they do, update the method accordingly:public static boolean ackRequired(ProtocolMessage msg) { - return msg.action == Action.message || msg.action == Action.presence; + return msg.action == Action.message + || msg.action == Action.presence + || msg.action == Action.object + || msg.action == Action.annotation; }Location:
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java:89-91
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (1)
10-12: Appropriate use of ConcurrentHashMap for thread-safety.The implementation correctly uses ConcurrentHashMap for thread-safe caching of LiveObjects instances, addressing the previous review comment about potential concurrency issues.
🧹 Nitpick comments (4)
lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java (1)
138-142: Consider supplying a stub instead ofnullfor the new plugin parameterA hard-coded
nullis perfectly legal, but if at any point the test (or the code under test) starts to callliveObjectsPlugin.handle(..)you will get an NPE instead of a clean Mockito verification failure.
Providing a mocked plugin keeps the test future-proof and self-documenting:LiveObjectsPlugin plugin = Mockito.mock(LiveObjectsPlugin.class); ConnectionManager connectionManager = new ConnectionManager(ably, connection, channels, new EmptyPlatformAgentProvider(), plugin) { @Override protected boolean checkConnectivity() { return false; } };lib/src/main/java/io/ably/lib/realtime/Connection.java (1)
123-130: Constructor signature changed – update javadoc & usage comments
Connection(..)now expects aLiveObjectsPlugin, but the class-level and constructor comments still describe only the previous four parameters.
Consider updating the Javadoc to mention the plugin and to clarify thatnullis acceptable when the feature is disabled.
This prevents confusion for maintainers and for downstream code that may reflectively inspect the constructor.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
84-91: Immutable reference to plugin is good – but mention nullabilityNice addition; holding a final reference ensures thread-safety.
A brief comment (or @nullable annotation) would make it explicit that the field may be null when the plugin is absent, helping static-analysis tools.live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (1)
22-34: Consider preserving exception details when transforming errors.When transforming the ErrorInfo to an Exception, you're currently only keeping the message, which loses other potentially useful error details (code, statusCode, etc.).
- deferred.completeExceptionally(Exception(reason.message)) + deferred.completeExceptionally(Exception(reason.toString()).apply { + initCause(reason.cause) + })This preserves more error context for debugging and error handling upstream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/PluginConnectionAdapter.java(1 hunks)lib/src/main/java/io/ably/lib/plugins/PluginInstance.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/Connection.java(2 hunks)lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(6 hunks)lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java(1 hunks)live-objects/build.gradle.kts(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/main/java/io/ably/lib/plugins/PluginConnectionAdapter.java
- lib/src/main/java/io/ably/lib/plugins/PluginInstance.java
🚧 Files skipped from review as they are similar to previous changes (3)
- live-objects/build.gradle.kts
- lib/src/main/java/io/ably/lib/objects/LiveObjectsPlugin.java
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (2)
handle(36-38)send(22-34)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (19)
🔇 Additional comments (4)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
1686-1689: Overloadedsenddelegates correctly but always queues – validate semantics
send(msg, listener)forwards tosend(msg, /*queueEvents=*/true, listener).
Is queuing always desirable for plugin-originated messages? If the plugin expects an immediate NACK when the connection is not writable, passingqueueEvents=truemay hide problems.Please double-check desired behaviour; if immediate failure is preferable, delegate with
queueEvents=falsewhenlistener != null, or expose both options.No code change required if current behaviour is intentional.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt (3)
3-8: Good use of imports and dependencies.The imports are appropriate for the functionality implemented in this class, including thread-safety (ConcurrentHashMap) and coroutine support (CompletableDeferred).
14-16: Efficient implementation of getInstance.The
getOrPutmethod provides a clean, thread-safe way to retrieve cached instances or create new ones when needed. This is an idiomatic Kotlin approach.
36-38: Implement the handle method as a priority.The
handlemethod seems critical for processing incoming protocol messages but is currently unimplemented. Consider implementing this method before moving forward, as it appears to be a core functionality of the plugin.Without implementation, how will the plugin respond to incoming "object" and "object_sync" protocol messages mentioned in the summary? This could block the functionality of the entire live objects feature.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
Outdated
Show resolved
Hide resolved
2. Added bridging interfaces to send and receive protocol messages 3. Added impl. for dispose method
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
6446864 to
d87b365
Compare
lib/src/main/java/io/ably/lib/objects/batch/BatchContextBuilder.java
Outdated
Show resolved
Hide resolved
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
5ae9249 to
7fab87f
Compare
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
7fab87f to
15cd909
Compare
2. Removed liveobject batching operation specific interfaces
sacOO7
force-pushed
the
feature/liveobject-plugin-setup
branch
from
15cd909 to
cd7d075
Compare
Fixed #1084
Summary by CodeRabbit
New Features
Bug Fixes
Chores