[AIT-836][LiveObjects] Implement path-based LiveObjects public API#1213
Merged
sacOO7 merged 5 commits intoJun 11, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a comprehensive LiveObjects API: core subscription/value types, instance- and path-based view models, message/operation contracts, and write-side value-holder factories for map/counter mutations. ChangesLiveObjects Type System and API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
sacOO7
changed the base branch from
main
to
feature/path-based-liveobjects-implementation
sacOO7
force-pushed
the
feature/path-based-liveobjects-final-public-api
branch
from
a2531ed to
0410432
Compare
sacOO7
force-pushed
the
feature/path-based-liveobjects-final-public-api
branch
from
8264313 to
99d9dd9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/object/instance/types/BinaryInstance.java (1)
24-24: ⚡ Quick winInconsistent annotation style with other primitive instances.
All other primitive instance types (
BooleanInstance,StringInstance,NumberInstance,JsonArrayInstance,JsonObjectInstance) place@NotNullon a separate line before the return type, butBinaryInstanceuses inline type-use syntaxbyte@NotNull[]. While both are valid, consistency improves maintainability.♻️ Align with the annotation style used by other primitive instances
/** * Returns the wrapped binary value. * * <p>Spec: RTINS4 / RTTS10c * * `@return` the wrapped bytes */ - byte `@NotNull` [] value(); + `@NotNull` + byte[] value();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/object/instance/types/BinaryInstance.java` at line 24, The annotation placement in BinaryInstance's value() method is inconsistent—change the type-use form `byte `@NotNull` [] value()` to the same style used by other primitives by placing `@NotNull` on its own line above the return type so the signature reads with `@NotNull` on a separate line prior to `byte[]` in the BinaryInstance interface's value() method to match BooleanInstance, StringInstance, NumberInstance, JsonArrayInstance, and JsonObjectInstance.lib/src/main/java/io/ably/lib/object/message/ObjectData.java (1)
53-60: ⚡ Quick winClarify defensive-copy semantics for binary payloads.
Line 60 returns a mutable
byte[]; without a copy/immutability contract, callers can mutate shared state if implementations return backing storage.♻️ Suggested contract hardening
/** * Returns the binary value. * * <p>Spec: OD2c * - * `@return` the binary value, or {`@code` null} if not applicable + * <p>Implementations should return a defensive copy (or otherwise guarantee + * immutability) so callers cannot mutate internal state. + * + * `@return` the binary value, or {`@code` null} if not applicable */ byte `@Nullable` [] getBytes();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/object/message/ObjectData.java` around lines 53 - 60, The Javadoc for ObjectData.getBytes() is ambiguous about whether the returned byte[] is a live backing array or a defensive copy; update the contract in the getBytes() Javadoc of class ObjectData to state explicitly whether callers may mutate the returned array or not and what implementations must do (preferably: implementations must return a defensive copy of internal storage and callers may freely modify the returned array), and mention performance implications so implementers can optimize (e.g., offer an alternative method like getBytesView() returning an unmodifiable/readonly view if needed); reference the getBytes() method and ensure the Javadoc mentions nullability, copy semantics, and responsibilities for callers and implementers.lib/src/main/java/io/ably/lib/object/value/LiveMapValue.java (1)
289-306: ⚡ Quick winBinary array stored and returned without defensive copies.
BinaryValuestores thebyte[]reference directly (line 293) and returns it directly (line 305), allowing external code to mutate the array after construction or after callinggetAsBinary(). This can lead to unexpected state changes in theLiveMapValueafter it has been created or retrieved.🛡️ Recommended fix: add defensive copies
BinaryValue(byte `@NotNull` [] value) { - this.value = value; + this.value = value.clone(); } `@Override` public byte `@NotNull` [] getAsBinary() { - return value; + return value.clone(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/object/value/LiveMapValue.java` around lines 289 - 306, BinaryValue stores and exposes the mutable byte[] directly; to prevent external mutation, make defensive copies: in the BinaryValue(byte[] value) constructor copy the incoming array (e.g., Arrays.copyOf) into the private field, and in both getValue() and getAsBinary() return a copy of the internal array rather than the raw reference; update imports if needed and keep the field final and type byte[].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lib/src/main/java/io/ably/lib/object/path/PathObjectSubscriptionOptions.java`:
- Around line 21-23: The constructor in PathObjectSubscriptionOptions currently
accepts zero and negative depths; update the
PathObjectSubscriptionOptions(Integer depth) constructor to enforce the
documented invariant by validating that if depth is non-null it must be > 0, and
throw an IllegalArgumentException (or similar) when depth <= 0; leave null
accepted as before and assign this.depth only after validation.
In `@lib/src/main/java/io/ably/lib/object/path/types/JsonObjectPathObject.java`:
- Around line 10-13: Update the Javadoc on JsonObjectPathObject to remove the
incorrect reference to “primitive resolution” (since this type represents a
JsonObject, not a primitive): reword the sentence that explains why
PathObject#instance() returns null to say that this is a terminal/object
resolution for JsonObject rather than a primitive resolution, and clarify that
navigation via at(...) is not available because it’s only defined on
LiveMapPathObject while value() and inherited read APIs remain available.
In `@lib/src/main/java/io/ably/lib/object/path/types/LiveMapPathObject.java`:
- Around line 48-53: The Javadoc example incorrectly chains
liveMapPath.get("a").get("b").get("c") even though get(...) returns PathObject,
so update the example to be consistent by either using at(...) for each step
(e.g., liveMapPath.at("a").at("b").at("c")) or show the explicit cast using
PathObject#asLiveMap between get calls (e.g.,
liveMapPath.get("a").asLiveMap().get("b")...), and adjust the surrounding text
to reference LiveMapPathObject, get, at, and PathObject#asLiveMap accordingly.
---
Nitpick comments:
In `@lib/src/main/java/io/ably/lib/object/instance/types/BinaryInstance.java`:
- Line 24: The annotation placement in BinaryInstance's value() method is
inconsistent—change the type-use form `byte `@NotNull` [] value()` to the same
style used by other primitives by placing `@NotNull` on its own line above the
return type so the signature reads with `@NotNull` on a separate line prior to
`byte[]` in the BinaryInstance interface's value() method to match
BooleanInstance, StringInstance, NumberInstance, JsonArrayInstance, and
JsonObjectInstance.
In `@lib/src/main/java/io/ably/lib/object/message/ObjectData.java`:
- Around line 53-60: The Javadoc for ObjectData.getBytes() is ambiguous about
whether the returned byte[] is a live backing array or a defensive copy; update
the contract in the getBytes() Javadoc of class ObjectData to state explicitly
whether callers may mutate the returned array or not and what implementations
must do (preferably: implementations must return a defensive copy of internal
storage and callers may freely modify the returned array), and mention
performance implications so implementers can optimize (e.g., offer an
alternative method like getBytesView() returning an unmodifiable/readonly view
if needed); reference the getBytes() method and ensure the Javadoc mentions
nullability, copy semantics, and responsibilities for callers and implementers.
In `@lib/src/main/java/io/ably/lib/object/value/LiveMapValue.java`:
- Around line 289-306: BinaryValue stores and exposes the mutable byte[]
directly; to prevent external mutation, make defensive copies: in the
BinaryValue(byte[] value) constructor copy the incoming array (e.g.,
Arrays.copyOf) into the private field, and in both getValue() and getAsBinary()
return a copy of the internal array rather than the raw reference; update
imports if needed and keep the field final and type byte[].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b9e4b17-fc76-48eb-92d3-5620a4a45e55
📒 Files selected for processing (48)
lib/src/main/java/io/ably/lib/object/Subscription.javalib/src/main/java/io/ably/lib/object/ValueType.javalib/src/main/java/io/ably/lib/object/instance/Instance.javalib/src/main/java/io/ably/lib/object/instance/InstanceListener.javalib/src/main/java/io/ably/lib/object/instance/InstanceSubscriptionEvent.javalib/src/main/java/io/ably/lib/object/instance/package-info.javalib/src/main/java/io/ably/lib/object/instance/types/BinaryInstance.javalib/src/main/java/io/ably/lib/object/instance/types/BooleanInstance.javalib/src/main/java/io/ably/lib/object/instance/types/JsonArrayInstance.javalib/src/main/java/io/ably/lib/object/instance/types/JsonObjectInstance.javalib/src/main/java/io/ably/lib/object/instance/types/LiveCounterInstance.javalib/src/main/java/io/ably/lib/object/instance/types/LiveMapInstance.javalib/src/main/java/io/ably/lib/object/instance/types/NumberInstance.javalib/src/main/java/io/ably/lib/object/instance/types/StringInstance.javalib/src/main/java/io/ably/lib/object/instance/types/package-info.javalib/src/main/java/io/ably/lib/object/message/CounterCreate.javalib/src/main/java/io/ably/lib/object/message/CounterInc.javalib/src/main/java/io/ably/lib/object/message/MapClear.javalib/src/main/java/io/ably/lib/object/message/MapCreate.javalib/src/main/java/io/ably/lib/object/message/MapRemove.javalib/src/main/java/io/ably/lib/object/message/MapSet.javalib/src/main/java/io/ably/lib/object/message/ObjectData.javalib/src/main/java/io/ably/lib/object/message/ObjectDelete.javalib/src/main/java/io/ably/lib/object/message/ObjectMessage.javalib/src/main/java/io/ably/lib/object/message/ObjectOperation.javalib/src/main/java/io/ably/lib/object/message/ObjectOperationAction.javalib/src/main/java/io/ably/lib/object/message/ObjectsMapEntry.javalib/src/main/java/io/ably/lib/object/message/ObjectsMapSemantics.javalib/src/main/java/io/ably/lib/object/message/package-info.javalib/src/main/java/io/ably/lib/object/package-info.javalib/src/main/java/io/ably/lib/object/path/PathObject.javalib/src/main/java/io/ably/lib/object/path/PathObjectListener.javalib/src/main/java/io/ably/lib/object/path/PathObjectSubscriptionEvent.javalib/src/main/java/io/ably/lib/object/path/PathObjectSubscriptionOptions.javalib/src/main/java/io/ably/lib/object/path/package-info.javalib/src/main/java/io/ably/lib/object/path/types/BinaryPathObject.javalib/src/main/java/io/ably/lib/object/path/types/BooleanPathObject.javalib/src/main/java/io/ably/lib/object/path/types/JsonArrayPathObject.javalib/src/main/java/io/ably/lib/object/path/types/JsonObjectPathObject.javalib/src/main/java/io/ably/lib/object/path/types/LiveCounterPathObject.javalib/src/main/java/io/ably/lib/object/path/types/LiveMapPathObject.javalib/src/main/java/io/ably/lib/object/path/types/NumberPathObject.javalib/src/main/java/io/ably/lib/object/path/types/StringPathObject.javalib/src/main/java/io/ably/lib/object/path/types/package-info.javalib/src/main/java/io/ably/lib/object/value/LiveCounter.javalib/src/main/java/io/ably/lib/object/value/LiveMap.javalib/src/main/java/io/ably/lib/object/value/LiveMapValue.javalib/src/main/java/io/ably/lib/object/value/package-info.java
- PathObjectSubscriptionOptions: validate depth fail-fast per RTPO19c1a,
throwing AblyException with ErrorInfo(400, 40003) when depth <= 0.
Depth is now a primitive int; the "no depth / infinite depth" state is
expressed via a new no-arg constructor (mirrors ably-js `{}` options),
so no null handling is needed
- LiveMapValue: defensively copy binary payloads on creation and access,
making the RTLMV3d immutability guarantee real for byte[] values
- ObjectData#getBytes: document that the returned array is the underlying
message payload and must be treated as read-only
- JsonObjectPathObject/JsonArrayPathObject: reword "primitive resolution"
javadoc for clarity
- LiveMapPathObject#at: fix javadoc equivalence example to compile
(get() returns base PathObject, so chain via asLiveMap())
sacOO7
merged commit 4f35df8
into
feature/path-based-liveobjects-implementation
14 of 15 checks passed
sacOO7
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Out of scope
Default implementation for interface methods ( This will be handled by downstream kotlin classes )
Integration with
channel.objectReason: Current public API only focuses on new path-based liveobjects interface. It's intentionally kept clean and currently doesn't interfare with existing
objectsinterfacesSummary by CodeRabbit