[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195
Open
ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
Open
[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
Conversation
WalkthroughThis pull request updates the protocol version from "5" to "6" and refactors the LiveObjects operation payload system, replacing generic operation wrappers with explicit typed payloads for map and counter operations across creation, modification, and removal scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This change involves substantial refactoring across multiple interconnected components: new public data types, revised ObjectOperation field signatures, updated serialization/deserialization logic, and coordinated changes across creator, manager, and test layers. While individual changes follow a consistent pattern, the breadth of affected files and the density of interdependent logic modifications require careful cross-file verification. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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 (11)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt (1)
131-139: KeepObjectState.createOpas a create operation in fixtures.In these fixtures,
createOpis switched toMapSet, which conflicts with create-op semantics and LiveMap validation expectations. Consider keepingcreateOpasMapCreateand usingoperationfor update scenarios.Also applies to: 149-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt` around lines 131 - 139, The fixture wrongly assigns a MapSet to ObjectState.createOp (see jsonObjectOperation and jsonObjectState based on dummyObjectOperation), breaking create-op semantics and LiveMap validation; revert createOp to a MapCreate instance (or keep the original dummyObjectOperation with action = ObjectOperationAction.MapCreate) and move the MapSet usage into the update/operation field (e.g., operation or a separate update-op fixture) so createOp remains a proper create operation; apply the same change to the similar fixtures around the other block (lines ~149-157).liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
42-87: Prefer protocol-valid operation shapes in size tests.This test currently combines multiple mutually exclusive payloads into one
ObjectOperation. Splitting this into per-action cases would better protect against invalid payload-shape regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt` around lines 42 - 87, The test builds an invalid ObjectOperation containing multiple mutually exclusive payloads; instead create separate, protocol-valid test cases each constructing a single ObjectOperation per action (e.g., one ObjectOperation with only mapSet set to a MapSet instance to test MAP_SET size, another with only counterInc set to a CounterInc for COUNTER_INC, one with only mapCreate set to a MapCreate with its entries for MAP_CREATE, and one with only counterCreate set to a CounterCreate for COUNTER_CREATE), and update the size assertions to target each operation’s expected size; locate and modify the ObjectOperation construction in ObjectMessageSizeTest (and the related assertions) to split into these per-action instances using the same MapSet, CounterInc, MapCreate, CounterCreate, and ObjectsMapEntry/ObjectData/ObjectValue symbols.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt (4)
208-208: Same redundant FQN as above.Can be simplified to
CounterInc(number = 5.0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt` at line 208, The test uses a redundant fully-qualified name when constructing the CounterInc instance; replace the FQN io.ably.lib.objects.CounterInc(number = 5.0) with a simple CounterInc(number = 5.0) (update the variable counterInc assignment in DefaultLiveCounterTest.kt to use the unqualified constructor).
163-163: Same redundant FQN as above.Can be simplified to
CounterInc(number = 5.0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt` at line 163, The test uses a redundant fully-qualified name when creating counterInc; replace the FQN io.ably.lib.objects.CounterInc with the simple class name CounterInc in DefaultLiveCounterTest (the assignment to variable counterInc) so it reads CounterInc(number = 5.0), ensuring imports remain present or add an import for CounterInc if needed.
138-138: Consider removing the fully-qualified name.The
io.ably.lib.objects.CounterIncis already imported at line 4, so you can use justCounterInc(number = 5.0)for consistency with other usages.♻️ Suggested simplification
- counterInc = io.ably.lib.objects.CounterInc(number = 5.0) + counterInc = CounterInc(number = 5.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt` at line 138, Replace the fully-qualified constructor call io.ably.lib.objects.CounterInc(number = 5.0) with the imported simple name CounterInc(number = 5.0) in DefaultLiveCounterTest.kt; update the assignment to counterInc to use CounterInc(...) (the import for CounterInc at the top already covers this).
187-187: Same redundant FQN as above.Can be simplified to
CounterInc(number = 5.0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt` at line 187, Replace the redundant fully qualified name in the test assignment by using the simple class name: change the initialization currently using io.ably.lib.objects.CounterInc(number = 5.0) to just CounterInc(number = 5.0) in DefaultLiveCounterTest (the counterInc variable assignment) so it matches the other usages and improves readability.liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt (1)
4-5: Redundant imports with wildcard.Lines 4-5 explicitly import
CounterCreateandCounterInc, but these are already included via the wildcard importio.ably.lib.objects.*at line 3.♻️ Remove redundant imports
import io.ably.lib.objects.* -import io.ably.lib.objects.CounterCreate -import io.ably.lib.objects.CounterInc import io.ably.lib.objects.ObjectOperation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt` around lines 4 - 5, The file DefaultLiveCounter.kt has redundant explicit imports for CounterCreate and CounterInc that are already covered by the existing wildcard import io.ably.lib.objects.*; remove the specific import lines for CounterCreate and CounterInc (the import statements at the top) so only the wildcard import remains, leaving class DefaultLiveCounter and any references to CounterCreate/CounterInc unchanged.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (2)
4-6: Redundant imports with wildcard.Lines 4-6 explicitly import
CounterCreate,CounterInc, andMapCreate, but these are already included via the wildcard importio.ably.lib.objects.*at line 3.♻️ Remove redundant imports
import io.ably.lib.objects.* -import io.ably.lib.objects.CounterCreate -import io.ably.lib.objects.CounterInc -import io.ably.lib.objects.MapCreate import io.ably.lib.objects.unit.LiveCounterManager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt` around lines 4 - 6, Remove the redundant explicit imports CounterCreate, CounterInc, and MapCreate because they are already brought in by the existing wildcard import io.ably.lib.objects.*; update the imports block by deleting the specific lines importing CounterCreate, CounterInc, and MapCreate so only the wildcard (or the explicit needed imports) remains to avoid duplication.
283-301: Duplicate test case.This test (
LiveCounterManager should throw error when counterInc payload missing) tests the same scenario as the test at lines 241-260 (LiveCounterManager should throw error for missing payload for counter increment operation). Both verify that anAblyExceptionwith error code 92000 is thrown whencounterInc = null.Consider removing one of these duplicate tests to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt` around lines 283 - 301, Remove the duplicate test in LiveCounterManagerTest: either delete the test named "(RTLC7, RTLC7d2) LiveCounterManager should throw error when counterInc payload missing" or consolidate it with the existing test "LiveCounterManager should throw error for missing payload for counter increment operation" so only one assertion verifies that liveCounterManager.applyOperation(ObjectOperation(action = ObjectOperationAction.CounterInc, objectId = "testCounterId", counterInc = null), null) throws an io.ably.lib.types.AblyException with errorInfo.code == 92000; update/remove the duplicate test method accordingly to avoid redundant coverage.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt (1)
153-154: Consider using imported types instead of fully qualified names.
MapSetis already imported at line 6, but here and in several other test methods (lines 178, 201, 222, 244), fully qualified names likeio.ably.lib.objects.MapSetare used. This inconsistency reduces readability.♻️ Example fix for this line
- mapSet = io.ably.lib.objects.MapSet(key = "key1", value = io.ably.lib.objects.ObjectData(value = io.ably.lib.objects.ObjectValue.String("value1"))) + mapSet = MapSet(key = "key1", value = ObjectData(value = ObjectValue.String("value1")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt` around lines 153 - 154, The tests use fully qualified names like io.ably.lib.objects.MapSet, io.ably.lib.objects.ObjectData and io.ably.lib.objects.ObjectValue.String even though MapSet (and likely ObjectData/ObjectValue) are already imported; replace those fully qualified usages with the imported type names (MapSet, ObjectData, ObjectValue.String) in DefaultLiveMapTest (and the other occurrences around the file at the referenced test methods) to make the code consistent and more readable.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt (1)
4-6: Redundant imports with wildcard.Lines 4-6 explicitly import
MapCreate,MapRemove, andMapSet, but these are already included via the wildcard importio.ably.lib.objects.*at line 3.♻️ Remove redundant imports
import io.ably.lib.objects.* -import io.ably.lib.objects.MapCreate -import io.ably.lib.objects.MapRemove -import io.ably.lib.objects.MapSet import io.ably.lib.objects.type.livemap.LiveMapEntry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt` around lines 4 - 6, The imports MapCreate, MapRemove, and MapSet are redundant because io.ably.lib.objects.* already brings them in; remove the explicit import lines for MapCreate, MapRemove, and MapSet from LiveMapManagerTest.kt so only the wildcard import io.ably.lib.objects.* remains, keeping the imports clean and avoiding duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt`:
- Around line 450-452: The size calculations currently use UTF-16 code unit
counts (String.length) causing undercount for multi-byte UTF-8 chars; update the
listed size methods to use the existing byteSize extension instead: in
MapCreate.size() replace it.key.length with it.key.byteSize; in MapSet.size()
replace key.length with key.byteSize; in MapRemove.size() replace key.length
with key.byteSize; in MapCreateWithObjectId.size() and
CounterCreateWithObjectId.size() replace initialValue.length and nonce.length
with initialValue.byteSize and nonce.byteSize; and in ObjectMessage.size()
replace clientId?.length with clientId?.byteSize so all payload string sizes
reflect UTF-8 byte length.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt`:
- Around line 169-176: The ObjectOperation writer currently counts and emits
multiple optional payload fields (mapCreate, mapSet, mapRemove, counterCreate,
counterInc, objectDelete, mapCreateWithObjectId, counterCreateWithObjectId)
without enforcing that exactly one payload is present and that the payload
matches the declared action; update the writer (where those fields are checked)
to validate there is exactly one non-null payload and that it is allowed for the
chosen action enum, throwing/raising an error if not, and update the reader
(after readObjectOperation(...)) to perform the same validation on the parsed
fields so mismatched or multiple payloads are rejected; add explicit checks
referencing the fields above and the function readObjectOperation to locate the
reader validation point.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt`:
- Around line 109-112: The apply paths currently only read
mapCreate/counterCreate and ignore
mapCreateWithObjectId/counterCreateWithObjectId; update
LiveMapManager.applyOperation and LiveCounterManager.applyOperation to detect
and handle the "*WithObjectId" variants (or canonicalize them to the base
structures) before calling applyMapCreate/applyCounterCreate and
mergeInitialDataFromCreateOperation, and update any helpers that read
operation.mapCreate?.entries or operation.counterCreate?.value to fallback to
operation.mapCreateWithObjectId?.entries and
operation.counterCreateWithObjectId?.value respectively (or normalize the
operation object once at the start of applyOperation so downstream methods like
mergeInitialDataFromCreateOperation always see a populated
mapCreate/counterCreate).
---
Nitpick comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt`:
- Around line 4-5: The file DefaultLiveCounter.kt has redundant explicit imports
for CounterCreate and CounterInc that are already covered by the existing
wildcard import io.ably.lib.objects.*; remove the specific import lines for
CounterCreate and CounterInc (the import statements at the top) so only the
wildcard import remains, leaving class DefaultLiveCounter and any references to
CounterCreate/CounterInc unchanged.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt`:
- Around line 131-139: The fixture wrongly assigns a MapSet to
ObjectState.createOp (see jsonObjectOperation and jsonObjectState based on
dummyObjectOperation), breaking create-op semantics and LiveMap validation;
revert createOp to a MapCreate instance (or keep the original
dummyObjectOperation with action = ObjectOperationAction.MapCreate) and move the
MapSet usage into the update/operation field (e.g., operation or a separate
update-op fixture) so createOp remains a proper create operation; apply the same
change to the similar fixtures around the other block (lines ~149-157).
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt`:
- Around line 42-87: The test builds an invalid ObjectOperation containing
multiple mutually exclusive payloads; instead create separate, protocol-valid
test cases each constructing a single ObjectOperation per action (e.g., one
ObjectOperation with only mapSet set to a MapSet instance to test MAP_SET size,
another with only counterInc set to a CounterInc for COUNTER_INC, one with only
mapCreate set to a MapCreate with its entries for MAP_CREATE, and one with only
counterCreate set to a CounterCreate for COUNTER_CREATE), and update the size
assertions to target each operation’s expected size; locate and modify the
ObjectOperation construction in ObjectMessageSizeTest (and the related
assertions) to split into these per-action instances using the same MapSet,
CounterInc, MapCreate, CounterCreate, and ObjectsMapEntry/ObjectData/ObjectValue
symbols.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt`:
- Line 208: The test uses a redundant fully-qualified name when constructing the
CounterInc instance; replace the FQN io.ably.lib.objects.CounterInc(number =
5.0) with a simple CounterInc(number = 5.0) (update the variable counterInc
assignment in DefaultLiveCounterTest.kt to use the unqualified constructor).
- Line 163: The test uses a redundant fully-qualified name when creating
counterInc; replace the FQN io.ably.lib.objects.CounterInc with the simple class
name CounterInc in DefaultLiveCounterTest (the assignment to variable
counterInc) so it reads CounterInc(number = 5.0), ensuring imports remain
present or add an import for CounterInc if needed.
- Line 138: Replace the fully-qualified constructor call
io.ably.lib.objects.CounterInc(number = 5.0) with the imported simple name
CounterInc(number = 5.0) in DefaultLiveCounterTest.kt; update the assignment to
counterInc to use CounterInc(...) (the import for CounterInc at the top already
covers this).
- Line 187: Replace the redundant fully qualified name in the test assignment by
using the simple class name: change the initialization currently using
io.ably.lib.objects.CounterInc(number = 5.0) to just CounterInc(number = 5.0) in
DefaultLiveCounterTest (the counterInc variable assignment) so it matches the
other usages and improves readability.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt`:
- Around line 4-6: Remove the redundant explicit imports CounterCreate,
CounterInc, and MapCreate because they are already brought in by the existing
wildcard import io.ably.lib.objects.*; update the imports block by deleting the
specific lines importing CounterCreate, CounterInc, and MapCreate so only the
wildcard (or the explicit needed imports) remains to avoid duplication.
- Around line 283-301: Remove the duplicate test in LiveCounterManagerTest:
either delete the test named "(RTLC7, RTLC7d2) LiveCounterManager should throw
error when counterInc payload missing" or consolidate it with the existing test
"LiveCounterManager should throw error for missing payload for counter increment
operation" so only one assertion verifies that
liveCounterManager.applyOperation(ObjectOperation(action =
ObjectOperationAction.CounterInc, objectId = "testCounterId", counterInc =
null), null) throws an io.ably.lib.types.AblyException with errorInfo.code ==
92000; update/remove the duplicate test method accordingly to avoid redundant
coverage.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt`:
- Around line 153-154: The tests use fully qualified names like
io.ably.lib.objects.MapSet, io.ably.lib.objects.ObjectData and
io.ably.lib.objects.ObjectValue.String even though MapSet (and likely
ObjectData/ObjectValue) are already imported; replace those fully qualified
usages with the imported type names (MapSet, ObjectData, ObjectValue.String) in
DefaultLiveMapTest (and the other occurrences around the file at the referenced
test methods) to make the code consistent and more readable.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt`:
- Around line 4-6: The imports MapCreate, MapRemove, and MapSet are redundant
because io.ably.lib.objects.* already brings them in; remove the explicit import
lines for MapCreate, MapRemove, and MapSet from LiveMapManagerTest.kt so only
the wildcard import io.ably.lib.objects.* remains, keeping the imports clean and
avoiding duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17065938-46ca-4898-add7-69593a37aa89
📒 Files selected for processing (21)
lib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.javaliveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt
💤 Files with no reviewable changes (1)
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
Comment on lines
+450
to
+452
| private fun MapCreate.size(): Int { | ||
| return entries.entries.sumOf { it.key.length + it.value.size() } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check the file structure and line counts
echo "=== File info ==="
wc -l liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Examine the specific lines mentioned
echo "=== Lines 450-452 (MapCreate.size) ==="
sed -n '450,452p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 457-459 (MapSet.size) ==="
sed -n '457,459p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 464-466 (MapRemove.size) ==="
sed -n '464,466p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 485-487 (MapCreateWithObjectId.size) ==="
sed -n '485,487p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 492-494 (CounterCreateWithObjectId.size) ==="
sed -n '492,494p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Check if byteSize is defined in the file
echo "=== Searching for byteSize definition ==="
rg -n 'fun.*byteSize|extension.*byteSize' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Check context around ObjectValue.String to see how byteSize is used
echo "=== ObjectValue.String references ==="
rg -n 'ObjectValue\.String' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -B2 -A2Repository: ably/ably-java
Length of output: 831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search entire codebase for byteSize
echo "=== Searching for byteSize definition in codebase ==="
rg -n 'byteSize|fun.*byteSize|extension.*byteSize' --max-count=20
# Check imports in the ObjectMessage.kt file
echo "=== Imports in ObjectMessage.kt ==="
head -30 liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Search for how size() is calculated for ObjectValue
echo "=== ObjectValue.size() implementation ==="
rg -n 'sealed class ObjectValue|is ObjectValue\.String' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -B2 -A5 | head -40
# Look for all .size() method implementations in the file
echo "=== All size() methods in ObjectMessage.kt ==="
rg -n 'fun.*size\(\)' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -A2Repository: ably/ably-java
Length of output: 4683
Use UTF-8 byte length in payload size calculations.
String.length counts UTF-16 code units, not UTF-8 bytes. The byteSize extension already exists and is correctly used for ObjectValue.String. The following methods must be updated:
MapCreate.size()(line 450):it.key.length→it.key.byteSizeMapSet.size()(line 457):key.length→key.byteSizeMapRemove.size()(line 464):key.length→key.byteSizeMapCreateWithObjectId.size()(line 485):initialValue.length,nonce.length→byteSizeCounterCreateWithObjectId.size()(line 492):initialValue.length,nonce.length→byteSizeObjectMessage.size()(line 409):clientId?.length→clientId?.byteSize(also uses char count instead of bytes)
Multi-byte UTF-8 characters (e.g., "你" = 3 bytes, "😊" = 4 bytes) will be undercounted, risking client-side size checks passing while the server rejects the message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt` around
lines 450 - 452, The size calculations currently use UTF-16 code unit counts
(String.length) causing undercount for multi-byte UTF-8 chars; update the listed
size methods to use the existing byteSize extension instead: in MapCreate.size()
replace it.key.length with it.key.byteSize; in MapSet.size() replace key.length
with key.byteSize; in MapRemove.size() replace key.length with key.byteSize; in
MapCreateWithObjectId.size() and CounterCreateWithObjectId.size() replace
initialValue.length and nonce.length with initialValue.byteSize and
nonce.byteSize; and in ObjectMessage.size() replace clientId?.length with
clientId?.byteSize so all payload string sizes reflect UTF-8 byte length.
Comment on lines
+169
to
+176
| if (mapCreate != null) fieldCount++ | ||
| if (mapSet != null) fieldCount++ | ||
| if (mapRemove != null) fieldCount++ | ||
| if (counterCreate != null) fieldCount++ | ||
| if (counterInc != null) fieldCount++ | ||
| if (objectDelete != null) fieldCount++ | ||
| if (mapCreateWithObjectId != null) fieldCount++ | ||
| if (counterCreateWithObjectId != null) fieldCount++ |
There was a problem hiding this comment.
Enforce action ↔ payload consistency for ObjectOperation.
Current read/write paths allow multiple payloads and mismatched payload/action combinations. This can produce invalid wire objects and non-deterministic application behavior.
🔒 Suggested guard (writer + reader)
+private fun ObjectOperation.validatePayloadShape() {
+ val payloadCount = listOfNotNull(
+ mapCreate, mapSet, mapRemove, counterCreate, counterInc,
+ objectDelete, mapCreateWithObjectId, counterCreateWithObjectId
+ ).size
+ require(payloadCount <= 1) { "ObjectOperation must contain at most one payload" }
+
+ when (action) {
+ ObjectOperationAction.MapCreate ->
+ require(mapCreate != null || mapCreateWithObjectId != null) { "MAP_CREATE payload missing" }
+ ObjectOperationAction.MapSet ->
+ require(mapSet != null) { "MAP_SET payload missing" }
+ ObjectOperationAction.MapRemove ->
+ require(mapRemove != null) { "MAP_REMOVE payload missing" }
+ ObjectOperationAction.CounterCreate ->
+ require(counterCreate != null || counterCreateWithObjectId != null) { "COUNTER_CREATE payload missing" }
+ ObjectOperationAction.CounterInc ->
+ require(counterInc != null) { "COUNTER_INC payload missing" }
+ ObjectOperationAction.ObjectDelete -> Unit
+ else -> Unit
+ }
+}
+
private fun ObjectOperation.writeMsgpack(packer: MessagePacker) {
+ validatePayloadShape()
var fieldCount = 1
...
}And apply equivalent validation after readObjectOperation(...) parses fields.
Also applies to: 189-227, 243-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt`
around lines 169 - 176, The ObjectOperation writer currently counts and emits
multiple optional payload fields (mapCreate, mapSet, mapRemove, counterCreate,
counterInc, objectDelete, mapCreateWithObjectId, counterCreateWithObjectId)
without enforcing that exactly one payload is present and that the payload
matches the declared action; update the writer (where those fields are checked)
to validate there is exactly one non-null payload and that it is allowed for the
chosen action enum, throwing/raising an error if not, and update the reader
(after readObjectOperation(...)) to perform the same validation on the parsed
fields so mismatched or multiple payloads are rejected; add explicit checks
referencing the fields above and the function readObjectOperation to locate the
reader validation point.
Comment on lines
+109
to
112
| validateMapSemantics(operation.mapCreate?.semantics) // RTLM16c | ||
|
|
||
| return mergeInitialDataFromCreateOperation(operation) // RTLM16d | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== mapCreateWithObjectId producers/consumers ==="
rg -n --type=kt '\bmapCreateWithObjectId\b|\bMapCreateWithObjectId\b' liveobjects/src/main/kotlin
echo
echo "=== call paths to LiveMapManager.applyOperation ==="
rg -n --type=kt '\bapplyOperation\s*\(' liveobjects/src/main/kotlin/io/ably/lib/objects -C3
echo
echo "=== any explicit conversion from mapCreateWithObjectId to mapCreate ==="
rg -n --type=kt 'mapCreateWithObjectId.*mapCreate|initialValue.*MapCreate|readMapCreateWithObjectId' \
liveobjects/src/main/kotlin -C3Repository: ably/ably-java
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== mapCreateWithObjectId producers/consumers ==="
rg -n 'mapCreateWithObjectId|MapCreateWithObjectId' liveobjects/src/main/kotlin --glob='*.kt'
echo
echo "=== call paths to LiveMapManager.applyOperation ==="
rg -n 'applyOperation\s*\(' liveobjects/src/main/kotlin/io/ably/lib/objects --glob='*.kt' -C3
echo
echo "=== any explicit conversion from mapCreateWithObjectId to mapCreate ==="
rg -n 'mapCreateWithObjectId.*mapCreate|initialValue.*MapCreate|readMapCreateWithObjectId' \
liveobjects/src/main/kotlin --glob='*.kt' -C3Repository: ably/ably-java
Length of output: 12220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== LiveMapManager.applyMapCreate and surrounding context (lines 100-130) ==="
sed -n '100,130p' liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
echo
echo "=== LiveMapManager operation dispatch and action handling (lines 56-80) ==="
sed -n '56,80p' liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
echo
echo "=== Lines 236-260 for similar patterns ==="
sed -n '236,260p' liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
echo
echo "=== ObjectOperationAction enum to understand action types ==="
rg -n 'enum class ObjectOperationAction|^\s*(MapCreate|MapCreateWithObjectId)' liveobjects/src/main/kotlin --glob='*.kt' -A5Repository: ably/ably-java
Length of output: 4439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if mapCreateWithObjectId is ever converted to mapCreate ==="
rg -n 'mapCreateWithObjectId.*mapCreate|operation\.mapCreateWithObjectId' liveobjects/src/main/kotlin --glob='*.kt' -B2 -A2
echo
echo "=== Check action assignment for operations ==="
rg -n 'action\s*=' liveobjects/src/main/kotlin --glob='*.kt' | head -20
echo
echo "=== ObjectOperation definition to see how action is determined ==="
rg -n 'data class ObjectOperation|val action' liveobjects/src/main/kotlin --glob='*.kt' -B2 -A5Repository: ably/ably-java
Length of output: 5763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check ObjectsManager.kt for operation processing ==="
sed -n '210,240p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
echo
echo "=== Search for any initialization or conversion of mapCreate from mapCreateWithObjectId ==="
rg -n 'mapCreate\s*=' liveobjects/src/main/kotlin --glob='*.kt' -B3 -A3
echo
echo "=== Check if applyObjectOperation gets modified before call ==="
rg -n 'applyObjectOperation|applyOperation' liveobjects/src/main/kotlin/io/ably/lib/objects -B5 -A2 --glob='*.kt' | head -60Repository: ably/ably-java
Length of output: 10758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Complete ObjectOperation definition ==="
sed -n '199,270p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo
echo "=== Check if there's any conditional logic for mapCreateWithObjectId ==="
rg -n 'mapCreateWithObjectId' liveobjects/src/main/kotlin/io/ably/lib/objects/type --glob='*.kt'Repository: ably/ably-java
Length of output: 1764
Handle MAP_CREATE_WITH_OBJECT_ID and COUNTER_CREATE_WITH_OBJECT_ID operations in the apply paths.
When these payloads arrive with action = MapCreate or action = CounterCreate, the corresponding mapCreateWithObjectId and counterCreateWithObjectId fields are populated while the base mapCreate and counterCreate fields are null. The applyMapCreate and applyCounterCreate methods (and their downstream helpers like mergeInitialDataFromCreateOperation) read only from the base payload fields, silently skipping the create-with-objectId variants.
Add handlers in LiveMapManager.applyOperation and LiveCounterManager.applyOperation to check for and apply the *WithObjectId payloads, or convert them to their base counterparts before the apply methods.
This also applies to lines 236–247 and 251–255 where operation.mapCreate?.entries and operation.counterCreate?.value are read without fallback to the *WithObjectId variants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt`
around lines 109 - 112, The apply paths currently only read
mapCreate/counterCreate and ignore
mapCreateWithObjectId/counterCreateWithObjectId; update
LiveMapManager.applyOperation and LiveCounterManager.applyOperation to detect
and handle the "*WithObjectId" variants (or canonicalize them to the base
structures) before calling applyMapCreate/applyCounterCreate and
mergeInitialDataFromCreateOperation, and update any helpers that read
operation.mapCreate?.entries or operation.counterCreate?.value to fallback to
operation.mapCreateWithObjectId?.entries and
operation.counterCreateWithObjectId?.value respectively (or normalize the
operation object once at the start of applyOperation so downstream methods like
mergeInitialDataFromCreateOperation always see a populated
mapCreate/counterCreate).
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
1 participant
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.
ObjectOperationfields to protocol v6 names (mapCreate,mapSet,mapRemove,counterCreate,counterInc). Internally and externallyObjectData(boolean,bytes,number,string,json)ObjectOperationandObjectData*CreateWithObjectIdoperations for apply-on-ACK and message size calculationSummary by CodeRabbit
Release Notes
New Features
Chores