[LiveObject] Refactor/rename liveobjects classes #1148
Conversation
WalkthroughRenames and migrates the LiveObjects subsystem to Objects/RealtimeObjects across Java and Kotlin modules: plugins, adapters, serializers, protocol handling, internal models, pools/managers, and tests. Constructor signatures and type names updated to Objects*/Realtime* variants; optional plugin/serializer loading via reflection introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant AblyRealtime
participant ConnectionManager
participant ObjectsPlugin
participant RealtimeObjects
App->>AblyRealtime: createChannel(name)
AblyRealtime->>ObjectsPlugin: getInstance(name)
ObjectsPlugin-->>AblyRealtime: RealtimeObjects
AblyRealtime-->>App: Channel (bound to RealtimeObjects)
ConnectionManager->>ObjectsPlugin: handle(msg: ProtocolMessage)
ObjectsPlugin->>RealtimeObjects: route msg/state
RealtimeObjects-->>ObjectsPlugin: applied updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1148/features
sacOO7
force-pushed
the
rename/liveobject-classes
branch
from
7f06e1f to
09ba0c0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1148/features
…s prefix - Renamed LiveObjects interface to RealtimeObjects - Removed Live prefix from other interfaces, kept them with Objects* - Removed use of liveobject keyword from the library since its reserved for business use case
sacOO7
force-pushed
the
rename/liveobject-classes
branch
from
09ba0c0 to
a49e370
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1148/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (13)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt (2)
95-96: Internal listeners are not disposed; potential memory leak
disposeObjectsStateListeners()currently callsoffAll()which only clears the external emitter. Listeners registered viainternalObjectStateEmitter.once(...)(used in ensureSynced earlier) remain and can leak.Apply this diff to dispose both internal and external listeners and cancel pending waits:
- override fun disposeObjectsStateListeners() = offAll() + override fun disposeObjectsStateListeners() { + // Clear both internal and external listeners + internalObjectStateEmitter.off() + externalObjectStateEmitter.off() + // Cancel any pending ensureSynced waits to avoid hanging coroutines on disposal + if (!syncedSignal.isCompleted) { + syncedSignal.cancel(CancellationException("Disposed ObjectsStateCoordinator")) + } + }
84-93: ensureSynced can miss SYNCED — race fixes requiredShort: ensureSynced registers a once() listener and can miss a SYNCED that fires before registration, causing an indefinite suspend. I verified ensureSynced is only invoked from DefaultRealtimeObjects.getRootAsync (rg output).
Files to change:
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt — ObjectsStateCoordinator: track latest state and re-check after once() registration to avoid the race.
Suggested minimal patch:
internal abstract class ObjectsStateCoordinator : ObjectsStateChange, HandlesObjectsStateChange { private val tag = "ObjectsStateCoordinator" + // Keep the most recent state so callers can re-check after registering a once() listener, + // avoiding the race where SYNCED was emitted just before registration. + @Volatile + private var latestState: ObjectsState = ObjectsState.Initialized private val internalObjectStateEmitter = ObjectsStateEmitter() // related to RTC10, should have a separate EventEmitter for users of the library private val externalObjectStateEmitter = ObjectsStateEmitter() @@ override fun objectsStateChanged(newState: ObjectsState) { + // Update latestState first so listeners that register after the event can observe the current state. + latestState = newState objectsStateToEventMap[newState]?.let { objectsStateEvent -> internalObjectStateEmitter.emit(objectsStateEvent) externalObjectStateEmitter.emit(objectsStateEvent) } } override suspend fun ensureSynced(currentState: ObjectsState) { if (currentState != ObjectsState.Synced) { - val deferred = CompletableDeferred<Unit>() - internalObjectStateEmitter.once(ObjectsStateEvent.SYNCED) { - Log.v(tag, "Objects state changed to SYNCED, resuming ensureSynced") - deferred.complete(Unit) - } - deferred.await() + val deferred = CompletableDeferred<Unit>() + internalObjectStateEmitter.once(ObjectsStateEvent.SYNCED) { + Log.v(tag, "Objects state changed to SYNCED, resuming ensureSynced") + deferred.complete(Unit) + } + // Re-check latestState after registering the once() listener to avoid missing a SYNCED + // that happened just before registration. + if (latestState == ObjectsState.Synced && !deferred.isCompleted) { + deferred.complete(Unit) + } + deferred.await() } }Reasoning: registering once() then immediately re-checking the latest known state prevents the window where SYNCED fires before registration. This is minimal, thread-visible (@volatile) and fits existing event model.
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (2)
68-85: Treat coroutine cancellation distinctly and harden error-callback invocationCurrently,
CancellationExceptionis funneled into a generic AblyException, andcallback.onError(...)can throw and crash the coroutine. Handle cancellation explicitly and wraponErrorsimilar toonSuccess.Apply this diff:
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) { + } catch (ce: CancellationException) { + // Swallow cancellation to respect disposal semantics; do not report as an error + Log.v(tag, "Operation cancelled", ce) + return@launch + } catch (throwable: Throwable) { when (throwable) { - is AblyException -> { callback.onError(throwable) } + is AblyException -> { + try { callback.onError(throwable) } catch (t: Throwable) { + Log.e(tag, "Error occurred while executing callback's onError handler", t) + } + } else -> { val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) - callback.onError(ex) + try { callback.onError(ex) } catch (t: Throwable) { + Log.e(tag, "Error occurred while executing callback's onError handler", t) + } } } } } }
87-104: Mirror cancellation and error handling for void-callback variantSame issues apply here. Handle
CancellationExceptionseparately and guardonError.Apply this diff:
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) { + } catch (ce: CancellationException) { + Log.v(tag, "Operation cancelled", ce) + return@launch + } catch (throwable: Throwable) { when (throwable) { - is AblyException -> { callback.onError(throwable) } + is AblyException -> { + try { callback.onError(throwable) } catch (t: Throwable) { + Log.e(tag, "Error occurred while executing callback's onError handler", t) + } + } else -> { val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) - callback.onError(ex) + try { callback.onError(ex) } catch (t: Throwable) { + Log.e(tag, "Error occurred while executing callback's onError handler", t) + } } } } } }lib/src/main/java/io/ably/lib/objects/type/map/LiveMapUpdate.java (1)
37-41: getUpdate() violates @NotNull contract when no-op update; also includes unchecked castThe no-op constructor sets update = null, but getUpdate() returns (Map<String, Change>) update annotated as @NotNull. This can return null and violate the contract, surprising downstream callers. It also performs an unchecked cast.
Recommend safely handling null and preserving the @NotNull contract by returning an empty map, and guarding the cast.
Apply this diff:
- @NotNull - public Map<String, Change> getUpdate() { - return (Map<String, Change>) update; - } + @NotNull + @SuppressWarnings("unchecked") + public Map<String, Change> getUpdate() { + return (update instanceof Map) + ? (Map<String, Change>) update + : java.util.Collections.emptyMap(); + }Optionally, add a runtime type check (as shown) to fail fast if update ever holds a non-Map instance due to misuse.
lib/src/main/java/io/ably/lib/objects/RealtimeObjects.java (1)
56-69: Confirm examples and references compile after rename — update remaining "LiveObjects" mentionsSearch output shows leftover identifiers that need updating (strings, javadocs, and docs). Please replace/update these to match the new API/name (e.g. RealtimeObjects) and update README/snippets/generated docs:
- lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java — lines 22, 37 (log messages)
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java — lines 46, 48 (javadoc)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java — line 102 (ErrorInfo message)
- lib/src/main/java/io/ably/lib/transport/ConnectionManager.java — lines 98, 100 (javadoc)
- CONTRIBUTING.md — line 56 (module mention)
After applying fixes, re-run the same search to confirm no remaining matches.
lib/src/main/java/io/ably/lib/objects/type/counter/LiveCounterUpdate.java (1)
36-39: getUpdate() nullability contract is unsafe; can NPE for no-op updatesObjectUpdate.update is annotated @nullable, but getUpdate() is annotated @NotNull and unconditionally casts the possibly-null field. This can trigger a NullPointerException if callers access getUpdate() on a no-op instance (constructed via the default constructor).
Recommend updating getUpdate() to return @nullable and keep behavior consistent with ObjectUpdate.
Apply this diff to the method:
- @NotNull - public LiveCounterUpdate.Update getUpdate() { - return (Update) update; - } + @org.jetbrains.annotations.Nullable + public LiveCounterUpdate.Update getUpdate() { + return (Update) update; + }Additionally, ensure the Nullable import is available (optional if you use the FQCN as above). If you prefer the import, add:
import io.ably.lib.objects.type.ObjectUpdate; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable;Alternative (if you want to keep a @NotNull contract): change the no-op constructor to supply a sentinel Update(0.0) instead of null, and document that 0.0 denotes no-op. This alters semantics, so only apply if that’s acceptable.
lib/src/main/java/io/ably/lib/objects/ObjectsJsonSerializer.java (2)
17-28: Handle JsonNull gracefully in deserialize to avoid false parse errorsGson can pass JsonNull when the field is present but null. Treat that as null instead of throwing.
Apply this diff:
@Override public Object[] deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { + if (json == null || json.isJsonNull()) { + return null; + } ObjectsSerializer serializer = ObjectsHelper.getSerializer(); if (serializer == null) { Log.w(TAG, "Skipping 'state' field json deserialization because ObjectsSerializer not found."); return null; } if (!json.isJsonArray()) { throw new JsonParseException("Expected a JSON array for 'state' field, but got: " + json); } return serializer.readFromJsonArray(json.getAsJsonArray()); }
30-38: Null-check src in serialize to prevent NPE with non-nullable serializer APIGson can pass null src for absent fields; ObjectsSerializer.asJsonArray likely expects non-null. Return JsonNull for null src to avoid NPEs.
Apply this diff:
@Override public JsonElement serialize(Object[] src, Type typeOfSrc, JsonSerializationContext context) { + if (src == null) { + return JsonNull.INSTANCE; + } ObjectsSerializer serializer = ObjectsHelper.getSerializer(); if (serializer == null) { Log.w(TAG, "Skipping 'state' field json serialization because ObjectsSerializer not found."); return JsonNull.INSTANCE; } return serializer.asJsonArray(src); }live-objects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt (1)
12-24: Kotlin named + positional arguments mixed — this won’t compileAfter using a named argument, all following arguments must be named in Kotlin. Calls like ObjectData(objectId = "object-id", ObjectValue.String(...)) are invalid and will fail compilation.
Apply this diff to fix all occurrences by naming the value parameter explicitly:
- internal val dummyObjectDataStringValue = ObjectData(objectId = "object-id", ObjectValue.String("dummy string")) + internal val dummyObjectDataStringValue = ObjectData(objectId = "object-id", value = ObjectValue.String("dummy string")) - internal val dummyBinaryObjectValue = ObjectData(objectId = "object-id", ObjectValue.Binary(Binary(byteArrayOf(1, 2, 3)))) + internal val dummyBinaryObjectValue = ObjectData(objectId = "object-id", value = ObjectValue.Binary(Binary(byteArrayOf(1, 2, 3)))) - internal val dummyNumberObjectValue = ObjectData(objectId = "object-id", ObjectValue.Number(42.0)) + internal val dummyNumberObjectValue = ObjectData(objectId = "object-id", value = ObjectValue.Number(42.0)) - internal val dummyBooleanObjectValue = ObjectData(objectId = "object-id", ObjectValue.Boolean(true)) + internal val dummyBooleanObjectValue = ObjectData(objectId = "object-id", value = ObjectValue.Boolean(true)) - internal val dummyJsonObjectValue = ObjectData(objectId = "object-id", ObjectValue.JsonObject(dummyJsonObject)) + internal val dummyJsonObjectValue = ObjectData(objectId = "object-id", value = ObjectValue.JsonObject(dummyJsonObject)) - internal val dummyJsonArrayValue = ObjectData(objectId = "object-id", ObjectValue.JsonArray(dummyJsonArray)) + internal val dummyJsonArrayValue = ObjectData(objectId = "object-id", value = ObjectValue.JsonArray(dummyJsonArray))live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (1)
116-120: Error message prints the same objectId twiceThe second interpolation should reference the instance’s objectId, not the incoming one.
Apply this diff:
- throw objectError("Invalid object: incoming objectId=${objectId}; $objectType objectId=$objectId") + throw objectError("Invalid object: incoming objectId=$objectId; $objectType objectId=${this.objectId}")live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (2)
704-712: Replace direct ablyException() usage with objectError(); fix missing importsThis branch throws via
ablyException(..., ErrorCode..., HttpStatusCode...)but this file does not importHttpStatusCodeorablyException. Elsewhere in this file, you consistently useobjectError(...). Keep it consistent and avoid missing imports.Apply:
- else -> - throw ablyException("Invalid JSON string for json field", ErrorCode.MapValueDataTypeUnsupported, HttpStatusCode.InternalServerError) + else -> + throw objectError("Invalid JSON string for json field")
160-178: Enforce objectId presence on read side (Msgpack/JSON) — write already requires itShort summary: writeMsgpack already calls require(objectId.isNotEmpty()). The Msgpack read path (readObjectOperation) and the JSON path (gson.fromJson in JsonSerialization.toObjectMessage) do not validate objectId and can return an empty string. Add validation on read to fail fast (or gate by action only if the protocol explicitly allows missing objectId).
Files to update
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
- private fun readObjectOperation(...) — currently sets var objectId = "" and returns it without checking.
- private fun readObjectState(...) — consider same validation for state.objectId.
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt
- internal fun JsonObject.toObjectMessage() — uses gson.fromJson(...) with no post-validation.
- Note: tests create an ObjectOperation with objectId = "" in live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt but that only exercises size(), not serialization.
Suggested minimal changes (apply to Msgpack read + JSON deserialiser)
After the existing action null check in readObjectOperation, add:
if (objectId.isEmpty()) { throw objectError("Missing required 'objectId' field in ObjectOperation") }In JsonSerialization.kt, post-parse validate and throw (JsonParseException) for missing objectId, e.g.:
internal fun JsonObject.toObjectMessage(): ObjectMessage { val msg = gson.fromJson(this, ObjectMessage::class.java) if (msg.operation?.objectId?.isEmpty() == true) throw JsonParseException("Missing required 'objectId' in operation") if (msg.objectState?.objectId?.isEmpty() == true) throw JsonParseException("Missing required 'objectId' in object state") return msg }If the protocol actually allows missing objectId for some actions (confirm with spec), instead gate these checks by action (e.g. only require objectId for all actions except a documented exception). Otherwise mirror the write-side invariant on the read side as above.
♻️ Duplicate comments (3)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt (3)
47-49: Repeat: dispose RealtimeObjects in testsSame resource lifecycle concern as above; consider disposing at the end of this test.
81-83: Repeat: dispose RealtimeObjects in testsSame resource lifecycle concern as above; consider disposing at the end of this test.
110-112: Repeat: dispose RealtimeObjects in testsSame resource lifecycle concern as above; consider disposing at the end of this test.
🧹 Nitpick comments (33)
lib/src/main/java/io/ably/lib/objects/type/map/LiveMap.java (1)
24-26: Clarify Javadoc phrasing around objectId reference to RealtimeObjectWording can be tightened to avoid ambiguity. Suggest changing “objectId string of another RealtimeObject” to “objectId referencing another RealtimeObject”.
Apply this diff to the Javadoc:
- * If the value associated with the provided key is an objectId string of another RealtimeObject, a reference to - * that RealtimeObject is returned, provided it exists in the local pool and is not tombstoned. Otherwise, null is returned. + * If the value associated with the provided key is an objectId referencing another RealtimeObject, a reference to + * that RealtimeObject is returned, provided it exists in the local pool and is not tombstoned. Otherwise, null is returned.live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
114-117: Nonce generation uses non-cryptographic randomnessIf the nonce is used for identifiers only, this is fine. If it carries any security or collision-resistance requirement, prefer a cryptographically secure generator.
Proposed implementation using SecureRandom (if stronger guarantees are required):
// New utility (Kotlin) private val secureRandom by lazy { java.security.SecureRandom() } internal fun generateNonce(): String { val chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789" val sb = StringBuilder(16) repeat(16) { sb.append(chars[secureRandom.nextInt(chars.length)]) } return sb.toString() }Please confirm intended use of nonce so we can select the appropriate implementation.
lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.java (2)
6-11: Docs: eliminate “live object” phrasing and fix minor grammarTo align with the rename and improve clarity, prefer “realtime object” (or simply “object”), and fix the article before ObjectUpdate.
Apply this diff:
- * Abstract base class for all LiveMap/LiveCounter update notifications. - * Provides common structure for updates that occur on LiveMap and LiveCounter objects. - * Contains the update data that describes what changed in the live object. + * Abstract base class for all LiveMap/LiveCounter update notifications. + * Provides common structure for updates that occur on LiveMap and LiveCounter objects. + * Contains the update data that describes what changed in the object. @@ -public abstract class ObjectUpdate { +public abstract class ObjectUpdate {And for the constructor docs:
- * Creates a ObjectUpdate with the specified update data. + * Creates an ObjectUpdate with the specified update data.
11-27: Public rename is a breaking change — add a deprecated compatibility shim for LiveObjectUpdateRenaming LiveObjectUpdate → ObjectUpdate is a source/binary-breaking change for consumers. I attempted to search for remaining usages but the provided rg run produced no output, so please confirm there are no remaining references or add a deprecated alias to preserve compatibility.
- Changed file to review: lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.java
- Suggested compatibility shim (new file: lib/src/main/java/io/ably/lib/objects/type/LiveObjectUpdate.java):
package io.ably.lib.objects.type; import org.jetbrains.annotations.Nullable; /** * @deprecated Use {@link ObjectUpdate} instead. */ @Deprecated public abstract class LiveObjectUpdate extends ObjectUpdate { protected LiveObjectUpdate(@Nullable Object update) { super(update); } }
- Quick local verification command you can run: rg -n --hidden --word-regexp LiveObjectUpdate -S
Would you like me to prepare a follow-up PR/commit adding this shim and deprecation annotations across affected types?
live-objects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (1)
248-266: Optional: prefer stubbing accessors over private-field mutation in mockUsing setPrivateField on ChannelStateChange is brittle. Mockk can stub accessors instead:
- every { stateChange.current } returns ChannelState.suspended
- every { stateChange.reason } returns clientError("Not attached").errorInfo
This keeps the test resilient to internal refactors.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt (1)
117-118: Nit: explicit import for kotlin.text.toByteArray is unnecessaryThe Kotlin stdlib extension is available without explicit import. You can drop the import for brevity.
lib/src/main/java/io/ably/lib/objects/Adapter.java (2)
14-14: Use the concrete class name for the logging tagLogging under the interface name obscures the emitting class. Prefer Adapter.class.getName().
- private static final String TAG = ObjectsAdapter.class.getName(); + private static final String TAG = Adapter.class.getName();
36-44: Fix log message to match method nameThe error log references attachChannel() while the method is getChannel(). Adjust for clarity.
- Log.e(TAG, "attachChannel(): channel not found: " + channelName); + Log.e(TAG, "getChannel(): channel not found: " + channelName);live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt (1)
81-86: Consider suppressing updates for zero-amount increments (verify spec intent)Currently, a 0.0 increment updates the data store (no-op) and emits a LiveCounterUpdate(0.0). If the spec treats zero increments as no-op, we can skip the write and return a no-op update to reduce noise.
private fun applyCounterInc(counterOp: ObjectsCounterOp): LiveCounterUpdate { - val amount = counterOp.amount ?: 0.0 - val previousValue = liveCounter.data.get() - liveCounter.data.set(previousValue + amount) // RTLC9b - return LiveCounterUpdate(amount) + val amount = counterOp.amount ?: 0.0 + if (amount == 0.0) return noOpCounterUpdate + val previousValue = liveCounter.data.get() + liveCounter.data.set(previousValue + amount) // RTLC9b + return LiveCounterUpdate(amount) }If zero-increment updates are required for observability, please ignore this suggestion.
live-objects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt (1)
27-33: Unify return semantics on first fetch vs cached path (optional)First computation returns serverTime directly, while subsequent calls return System.currentTimeMillis() + offset. For consistency and to avoid small jumps in reported time, consider returning now + offset in both paths.
- val serverTime: Long = withContext(Dispatchers.IO) { adapter.time } - serverTimeOffset = serverTime - System.currentTimeMillis() - return serverTime + val serverTime: Long = withContext(Dispatchers.IO) { adapter.time } + serverTimeOffset = serverTime - System.currentTimeMillis() + return System.currentTimeMillis() + serverTimeOffset!!lib/src/main/java/io/ably/lib/objects/ObjectsSerializer.java (1)
10-12: Clarify expected element type and failure mode in JavadocMost implementations (e.g., DefaultObjectsSerializer) expect arrays of ObjectMessage and intentionally throw ClassCastException on mismatches. Consider documenting this to set expectations for implementors and callers.
Proposed doc tweak:
/** - * Serializer interface for converting between objects and their MessagePack or JSON representations. + * Serializer interface for converting between objects and their MessagePack or JSON representations. + * <p> + * Note: Implementations commonly expect arrays whose elements are a specific type (e.g., ObjectMessage). + * Passing arrays containing incompatible element types may result in a ClassCastException by design.live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (2)
25-26: Doc nit: avoid referencing concrete implementation (“DefaultRealtimeObjects”)The parameter describes an ObjectsPool and shouldn’t tie to a concrete pool implementation.
- * @param objectsPool The object pool containing referenced DefaultRealtimeObjects + * @param objectsPool The object pool containing referenced RealtimeObjects
80-84: Harden casts to catch mismatches with clear errorsCurrent unchecked casts may throw ClassCastException with less helpful messages if objectType is inconsistent with runtime type. Prefer safe casts with explicit errors.
-private fun fromRealtimeObject(realtimeObject: BaseRealtimeObject): LiveMapValue { - return when (realtimeObject.objectType) { - ObjectType.Map -> LiveMapValue.of(realtimeObject as LiveMap) - ObjectType.Counter -> LiveMapValue.of(realtimeObject as LiveCounter) - } -} +private fun fromRealtimeObject(realtimeObject: BaseRealtimeObject): LiveMapValue { + return when (realtimeObject.objectType) { + ObjectType.Map -> (realtimeObject as? LiveMap) + ?.let { LiveMapValue.of(it) } + ?: error("ObjectType.Map but got ${realtimeObject::class.simpleName}") + ObjectType.Counter -> (realtimeObject as? LiveCounter) + ?.let { LiveMapValue.of(it) } + ?: error("ObjectType.Counter but got ${realtimeObject::class.simpleName}") + } +}live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (1)
13-16: KDoc nit: remove “Jackson” referenceThis implementation uses org.msgpack for MessagePack and Gson for JSON; mentioning Jackson is misleading.
- * of ObjectMessage arrays for both JSON and MessagePack formats using Jackson and Gson. + * of ObjectMessage arrays for both MessagePack and JSON formats using org.msgpack and Gson.lib/src/main/java/io/ably/lib/realtime/Connection.java (1)
126-130: Constructor now requires ObjectsPlugin; add a fast-fail null checkTo avoid subtle NPEs later in
ConnectionManager, defensively validate theobjectsPluginargument at construction time.Apply this diff:
Connection(AblyRealtime ably, ConnectionManager.Channels channels, PlatformAgentProvider platformAgentProvider, ObjectsPlugin objectsPlugin) throws AblyException { this.ably = ably; this.state = ConnectionState.initialized; + if (objectsPlugin == null) { + throw new NullPointerException("objectsPlugin must not be null"); + } this.connectionManager = new ConnectionManager(ably, this, channels, platformAgentProvider, objectsPlugin); }live-objects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt (1)
22-24: Dispose DefaultRealtimeObjects in tests to avoid coroutine leaks
DefaultRealtimeObjectsspins up asequentialScopeand a collector; not disposing can leak coroutines between tests. Wrap usage in try/finally or add a JUnit teardown to calldispose(...).Example (within the test body):
val defaultRealtimeObjects = DefaultRealtimeObjects("dummyChannel", mockk(relaxed = true)) try { val objectsPool = defaultRealtimeObjects.objectsPool // assertions... } finally { defaultRealtimeObjects.dispose(mockk(relaxed = true)) }lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
46-50: Update Javadoc to remove “LiveObjects” wording per PR objectiveThe field docs still mention “LiveObjects”; update to “Objects” to align with the rename and avoid reserved identifier usage.
Apply this diff:
- /** - * A nullable reference to the LiveObjects plugin. - * <p> - * This field is initialized only if the LiveObjects plugin is present in the classpath. - */ + /** + * A nullable reference to the Objects plugin. + * <p> + * This field is initialized only if the Objects plugin is present in the classpath. + */lib/src/main/java/io/ably/lib/objects/ObjectsPlugin.java (1)
8-13: Doc wording nit: prefer “RealtimeObjects” for instance nomenclatureTo keep the terminology consistent with the type name, use “RealtimeObjects instances” instead of “Objects instances”.
Apply this diff:
- * live data objects in a real-time environment. It allows for the retrieval, disposal, and - * management of Objects instances associated with specific channel names. + * live data objects in a real-time environment. It allows for the retrieval, disposal, and + * management of RealtimeObjects instances associated with specific channel names.Also applies to: 15-21
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
98-103: Update field comment to remove “LiveObjects” wordingComment still references “LiveObjects plugin”. Replace with “Objects plugin” to match the refactor.
Apply this diff:
- /** - * A nullable reference to the LiveObjects plugin. - * <p> - * This field is initialized only if the LiveObjects plugin is present in the classpath. - */ + /** + * A nullable reference to the Objects plugin. + * <p> + * This field is initialized only if the Objects plugin is present in the classpath. + */
14-39: Annotate objectsPlugin as nullable (field and ctor), and import @nullableThis improves clarity about optional runtime presence and helps nullability tooling.
Apply this diff:
@@ import io.ably.lib.util.PlatformAgentProvider; import io.ably.lib.util.ReconnectionStrategy; +import org.jetbrains.annotations.Nullable; @@ - private final ObjectsPlugin objectsPlugin; + @Nullable + private final ObjectsPlugin objectsPlugin; @@ - public ConnectionManager(final AblyRealtime ably, final Connection connection, final Channels channels, final PlatformAgentProvider platformAgentProvider, ObjectsPlugin objectsPlugin) throws AblyException { + public ConnectionManager(final AblyRealtime ably, final Connection connection, final Channels channels, final PlatformAgentProvider platformAgentProvider, @Nullable ObjectsPlugin objectsPlugin) throws AblyException {Also applies to: 102-103, 776-783
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java (1)
13-25: Optional: avoid logging stack traces for expected absenceAbsence of the plugin on the classpath is expected in deployments that don’t use Objects. Consider logging at DEBUG without the exception for ClassNotFoundException to reduce noise.
Example:
- } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException | - InvocationTargetException e) { - Log.i(TAG, "Objects plugin not found in classpath. Objects functionality will not be available.", e); + } catch (ClassNotFoundException e) { + Log.d(TAG, "Objects plugin not found in classpath. Objects functionality will not be available."); + return null; + } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) { + Log.w(TAG, "Failed to initialize Objects plugin via reflection", e); return null; }live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt (2)
252-254: Avoid repeated map allocations when merging updatesfold with map concatenation creates intermediate maps per iteration. Prefer a single mutable map accumulation.
Apply this diff:
- return LiveMapUpdate( - aggregatedUpdate.map { it.update }.fold(emptyMap()) { acc, map -> acc + map } - ) + val merged = mutableMapOf<String, LiveMapUpdate.Change>() + aggregatedUpdate.forEach { merged.putAll(it.update) } + return LiveMapUpdate(merged)
327-333: Use actual current semantics in error message instead of constantImproves diagnostics if/when more semantics are introduced.
Apply this diff:
private fun validateMapSemantics(semantics: ObjectsMapSemantics?) { if (semantics != liveMap.semantics) { throw objectError( - "Invalid object: incoming object map semantics=$semantics; current map semantics=${ObjectsMapSemantics.LWW}" + "Invalid object: incoming object map semantics=$semantics; current map semantics=${liveMap.semantics}" ) } }live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
102-116: Public cleanup helpers: document call sites and lifecycleclearSyncObjectsDataPool and clearBufferedObjectOperations are meant to be used by DefaultRealtimeObjects.handleStateChange; consider adding KDoc @see links or unit tests to prevent regression of their use during state transitions.
live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (1)
181-186: Minor KDoc grammar“The returned map is used to notifying...” -> “used to notify...”.
Apply this doc fix:
- * The returned map is used to notifying other components about what entries were removed. + * The returned map is used to notify other components about what entries were removed.live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (1)
804-806: Clarify test comment about semantics mismatchThe comment says “This should match, but we’ll test error case” while the code intentionally uses Unknown to cause a mismatch. Clarify intent.
Apply this change:
- semantics = ObjectsMapSemantics.Unknown, // This should match, but we'll test error case + semantics = ObjectsMapSemantics.Unknown, // Intentionally mismatched to trigger errorlive-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
87-89: Narrow mutability of parameter typedeleteExtraObjectIds doesn’t mutate the provided set; prefer Set to better express intent and reduce API surface mutability.
Apply this diff:
- internal fun deleteExtraObjectIds(objectIds: MutableSet<String>) { + internal fun deleteExtraObjectIds(objectIds: Set<String>) { pool.entries.removeIf { (key, _) -> key !in objectIds && key != ROOT_OBJECT_ID } // RTO5c2a - Keep root object }Call sites passing a MutableSet (e.g., from mutableSetOf(...)) will continue to work.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
99-107: Consider updating the error message to match the new naming convention.The error message still references "LiveObjects plugin" but should be updated to "Objects plugin" for consistency with the renaming objectives of this PR.
throw AblyException.fromErrorInfo( - new ErrorInfo("LiveObjects plugin hasn't been installed, " + - "add runtimeOnly('io.ably:live-objects:<ably-version>') to your dependency tree", 400, 40019) + new ErrorInfo("Objects plugin hasn't been installed, " + + "add runtimeOnly('io.ably:live-objects:<ably-version>') to your dependency tree", 400, 40019) );live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (5)
8-14: Imports align with the Objects rename; minor duplication/style nitpick*You have both a wildcard import
io.ably.lib.objects.*and explicit imports for some symbols from the same package. Consider choosing one style consistently to avoid redundancy and potential warnings from static analysis.
235-241: Consistency nit: standardize “UNKNOWN” fallback casing in error messageThe message says “no Unknown fallback found” while elsewhere you use “UNKNOWN”. Standardizing improves diagnostics.
Apply:
- ?: throw objectError("Unknown ObjectOperationAction code: $actionCode and no Unknown fallback found") + ?: throw objectError("Unknown ObjectOperationAction code: $actionCode and no UNKNOWN fallback found")
364-378: Enforce non-empty map key during serializationThe comment states “key is required”; add a guard so invalid operations are caught early.
Apply:
private fun ObjectsMapOp.writeMsgpack(packer: MessagePacker) { + require(key.isNotEmpty()) { "key must be non-empty per Objects protocol" } var fieldCount = 1 // key is required
479-516: Error string still mentions old type name; align with new naming and casingMessage mentions “MapSemantics”; rename to “ObjectsMapSemantics” for clarity and consistency with the new types. Also keep “UNKNOWN” casing consistent.
Apply:
- ?: throw objectError("Unknown MapSemantics code: $semanticsCode and no UNKNOWN fallback found") + ?: throw objectError("Unknown ObjectsMapSemantics code: $semanticsCode and no UNKNOWN fallback found")
362-366: KDoc/comment naming driftSeveral doc comments still refer to the old type names (e.g., “ObjectMapOp”, “ObjectMap”, “ObjectCounter”). Consider updating KDoc to “ObjectsMapOp”, “ObjectsMap”, “ObjectsCounter” for consistency with the rename.
Also applies to: 381-383, 451-454, 477-480, 519-523, 561-565
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt
Show resolved
Hide resolved
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt
Show resolved
Hide resolved
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.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
and RealtimeChannel classes
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
89-100: Remove unused deferStateEvent parameter — stateChange ignores itendSync currently forwards deferStateEvent to stateChange, but stateChange ignores the flag and already documents that deferral isn't needed. This contradicts the "defer until next tick" comment in handleObjectSyncMessages. Either implement true deferral or remove the unused parameter to avoid confusion. I recommend the minimal cleanup: remove the parameter and update call sites in this file.
Files/locations to change:
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
- handleObjectSyncMessages: remove the "defer ..." comment and call endSync() without args
- endSync: remove deferStateEvent parameter
- startNewSync: call stateChange(ObjectsState.Syncing) (remove boolean arg)
- stateChange: remove deferEvent parameter and the unused deferEvent comment
Proposed minimal diff:
@@ - if (syncTracker.hasSyncEnded()) { - // defer the state change event until the next tick if this was a new sync sequence - // to allow any event listeners to process the start of the new sequence event that was emitted earlier during this event loop. - endSync(isNewSync) - } + if (syncTracker.hasSyncEnded()) { + endSync() + } @@ - internal fun endSync(deferStateEvent: Boolean) { + internal fun endSync() { @@ - currentSyncId = null // RTO5c3 - stateChange(ObjectsState.Synced, deferStateEvent) + currentSyncId = null // RTO5c3 + stateChange(ObjectsState.Synced) } @@ - internal fun startNewSync(syncId: String?) { + internal fun startNewSync(syncId: String?) { @@ - currentSyncId = syncId - stateChange(ObjectsState.Syncing, false) + currentSyncId = syncId + stateChange(ObjectsState.Syncing) } @@ - private fun stateChange(newState: ObjectsState, deferEvent: Boolean) { + private fun stateChange(newState: ObjectsState) { if (realtimeObjects.state == newState) { return } Log.v(tag, "Objects state changed to: $newState from ${realtimeObjects.state}") realtimeObjects.state = newState - - // deferEvent not needed since objectsStateChanged processes events in a sequential coroutine scope - objectsStateChanged(newState) + // objectsStateChanged processes events in a sequential coroutine scope + objectsStateChanged(newState) }If you do need to preserve "defer to next tick" semantics, confirm the preferred scheduling mechanism (sequential coroutine scope vs. Handler/Looper) and I can propose an implementation consistent with DefaultRealtimeObjects' scheduling.
♻️ Duplicate comments (2)
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java (2)
40-41: Replace “LiveObjects” in serializer log message; improve clarityUpdate to “Objects” and slightly clarify wording.
Apply this diff:
- Log.w(TAG, "Failed to init ObjectsSerializer, LiveObjects plugin not included in the classpath", e); + Log.w(TAG, "Failed to initialize ObjectsSerializer; Objects plugin not included in the classpath.", e);
24-26: Replace “LiveObjects” in log message to align with the PR objectiveThe message still mentions “LiveObjects” and should be updated to “Objects”.
Apply this diff:
- Log.i(TAG, "LiveObjects plugin not found in classpath. LiveObjects functionality will not be available.", e); + Log.i(TAG, "Objects plugin not found in classpath. Objects functionality will not be available.", e);
🧹 Nitpick comments (4)
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java (1)
22-26: Differentiate between ‘plugin not present’ vs ‘plugin present but failed to initialize’Catching all reflection exceptions together as “not found” hides real initialization failures. Treat ClassNotFoundException as “optional plugin missing” (info), and warn for other reflective failures (or ClassCastException) to aid debugging.
Apply this diff:
- } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | NoSuchMethodException | - InvocationTargetException e) { - Log.i(TAG, "LiveObjects plugin not found in classpath. LiveObjects functionality will not be available.", e); - return null; - } + } catch (ClassNotFoundException e) { + Log.i(TAG, "Objects plugin not found in classpath. Objects functionality will not be available.", e); + return null; + } catch (InstantiationException | IllegalAccessException | NoSuchMethodException + | InvocationTargetException | ClassCastException e) { + Log.w(TAG, "Failed to initialize ObjectsPlugin; Objects plugin is present but failed to initialize.", e); + return null; + }lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
150-156: Fix log message to reference the correct method nameThe log says objectsPlugin.handle but the call is handleStateChange; align for accurate diagnostics.
Apply this diff:
- Log.e(TAG, "Unexpected exception in objectsPlugin.handle", t); + Log.e(TAG, "Unexpected exception in objectsPlugin.handleStateChange", t);
452-458: Fix log message to reference the correct method nameSame issue here when handling attached state.
Apply this diff:
- Log.e(TAG, "Unexpected exception in objectsPlugin.handle", t); + Log.e(TAG, "Unexpected exception in objectsPlugin.handleStateChange", t);live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (1)
23-24: Potential races on shared buffers/maps unless call sites are single-thread/serialbufferedObjectOperations and syncObjectsDataPool are mutated from both handleObjectMessages and handleObjectSyncMessages (and cleared in startNewSync/endSync). Without explicit synchronization, this relies on a strong guarantee that both paths run on the same serial dispatcher/event loop.
- If that guarantee exists, add a short comment noting thread-confinement to the objects event loop.
- If not, consider protecting these structures with a Mutex or similar.
I can draft a minimal Mutex-based guard tailored to your coroutine context if needed.
Also applies to: 30-43, 50-67, 89-100, 165-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
java/src/main/java/io/ably/lib/realtime/Channel.java(1 hunks)lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(7 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(7 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- java/src/main/java/io/ably/lib/realtime/Channel.java
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:25-29
Timestamp: 2025-06-23T14:18:25.315Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class methods like writeMsgpackArray will have their type signatures updated to be non-nullable since the calling sites ensure objects are never null when passed to these methods.
Learnt from: sacOO7
PR: ably/ably-java#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.
📚 Learning: 2025-06-03T09:15:15.338Z
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Applied to files:
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.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/realtime/ChannelBase.java
📚 Learning: 2025-06-06T09:28:12.298Z
Learnt from: sacOO7
PR: ably/ably-java#1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Applied to files:
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
📚 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/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
📚 Learning: 2025-06-23T14:28:23.301Z
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt:207-211
Timestamp: 2025-06-23T14:28:23.301Z
Learning: In the Ably Java LiveObjects MessagePack deserialization code, the `action` field in ObjectOperation is guaranteed to always be present in the protocol, so using a default value during deserialization is acceptable and won't mask real protocol errors.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
🧬 Code Graph Analysis (2)
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java (3)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
AblyRealtime(31-317)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt (1)
adapter(7-35)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultObjectsPlugin.kt (2)
getInstance(11-13)handleStateChange(20-22)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt (1)
handleStateChange(248-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check-liveobjects
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: build
🔇 Additional comments (10)
lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java (2)
12-13: Thread-safe lazy init looks correct (volatile + double-checked locking)The volatile field + DCL pattern is properly implemented for the serializer singleton.
Also applies to: 31-47
35-36: Serializer FQCN verified — no change requiredFound the reflected class in the repo; the FQCN matches:
- lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:35 — Class.forName("io.ably.lib.objects.serialization.DefaultObjectsSerializer")
- live-objects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt:18 — internal class DefaultObjectsSerializer : ObjectsSerializer
No code changes needed.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
98-108: Good migration to ObjectsPlugin/RealtimeObjects; null-safe optional plugin accessField and accessor changes correctly reflect the new plugin and instance pattern with appropriate nullability.
1330-1343: Proactive initialization of per-channel Objects instance looks goodEagerly creating the Objects instance ensures it can process sync right away on first messages.
103-105: Avoid “LiveObjects” in user-facing error; use “Objects”This message should avoid the reserved identifier “LiveObjects” per the PR goal.
Apply this diff:
- new ErrorInfo("LiveObjects plugin hasn't been installed, " + + new ErrorInfo("Objects plugin hasn't been installed, " + "add runtimeOnly('io.ably:live-objects:<ably-version>') to your dependency tree", 400, 40019)Note: Keeping the artifact coordinate as io.ably:live-objects if that module name is intentionally retained.
⛔ Skipped due to learnings
Learnt from: sacOO7 PR: ably/ably-java#1148 File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:0-0 Timestamp: 2025-08-14T10:43:48.143Z Learning: In the ably-java codebase, some "LiveObjectsPlugin" references in log messages, Javadoc, build files, and workflows are intentionally retained due to ongoing internal technical debate about isolating API naming from product naming, as discussed in LODR-033. These are not considered incomplete refactoring work but deliberate exceptions to the broader effort to eliminate "liveobject" keyword usage.Learnt from: sacOO7 PR: ably/ably-java#1148 File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:24-25 Timestamp: 2025-08-14T10:43:57.955Z Learning: In the ably-java LiveObjects renaming effort, log messages may intentionally retain "LiveObjects" terminology even when other parts of the codebase are being renamed to "Objects", due to internal technical debates about where the renaming should be applied. The team makes deliberate decisions about which parts of the codebase to rename versus which to keep as-is.live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt (5)
31-38: Buffering while not Synced + fixed log interpolation look correctThe buffering condition against ObjectsState.Synced and the corrected Kotlin interpolation for realtimeObjects.state are both correct and align with RTO7/RTO8.
151-153: Pool pruning is correct; ensure side-effects for removal are handleddeleteExtraObjectIds(receivedObjectIds) is the right place to prune missing objects after sync. Double-check that this also handles any required teardown (subscriptions/listeners) for removed objects inside the pool to avoid leaks.
36-36: Follow-up: log interpolation fix applied (acknowledging prior comment)Log now uses ${realtimeObjects.state} and will print the actual state value rather than the instance toString.
218-224: Incorrect —clientErroris defined in the same package (no import needed)I found a top-level definition:
- live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt:37
- internal fun clientError(errorMessage: String) = ablyException(errorMessage, ErrorCode.BadRequest, HttpStatusCode.BadRequest)
ObjectsManager.kt is in the same package (io.ably.lib.objects), so the call in createObjectFromState(...) is valid. Ignore the original unresolved-reference concern — no import or change required.
Likely an incorrect or invalid review comment.
220-221: Confirm whether Live type names should be renamed (DefaultLiveCounter / DefaultLiveMap)*I scanned the repo — DefaultLiveCounter/DefaultLiveMap and many Live* identifiers remain across production code, public Java API, and tests. Please confirm whether these type names are intentionally retained or should be renamed as part of the PR.
Files/locations found (non-exhaustive):
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt — lines 220–221 (uses DefaultLiveCounter.zeroValue / DefaultLiveMap.zeroValue)
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt — class + companion (zeroValue, tag = "LiveCounter")
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt — class + companion (zeroValue, tag = "LiveMap")
- live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt — creates DefaultLiveMap/DefaultLiveCounter in pool
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt — calls DefaultLiveMap.initialValue / DefaultLiveCounter.initialValue
- lib/src/main/java/io/ably/lib/objects/RealtimeObjects.java — public API still uses LiveMap / LiveCounter types
- lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java and other Java files — log/Javadoc mention LiveObjects plugin
- many tests under live-objects/src/test/kotlin reference DefaultLiveMap/DefaultLiveCounter and LiveMap/LiveCounter
Note: there’s an existing internal decision (LODR-033) to deliberately retain some "LiveObjects" plugin references in logs/Javadoc/builds — please confirm whether type/class names are subject to the same exception or must be renamed.
LiveObjectin the context of code is not recommended as per -> Add spec points to describe theObjectsplugin specification#332 (comment)Summary by CodeRabbit