[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195
Open
ttypic wants to merge 3 commits intoAIT-276/apply-on-ackfrom
Open
[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195ttypic wants to merge 3 commits intoAIT-276/apply-on-ackfrom
ttypic wants to merge 3 commits intoAIT-276/apply-on-ackfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughProtocol version constant bumped from "5" to "6". LiveObjects refactor: ObjectData replaced ObjectValue with explicit primitive fields; new typed payloads (MapCreate/MapSet/MapRemove, CounterCreate/CounterInc, MapCreateWithObjectId/CounterCreateWithObjectId, ObjectDelete) added; serialization, managers, factories, fixtures, and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (creator)
participant Serializer as Serializer (Msgpack/JSON)
participant Transport as Transport/Network
participant Server as Receiver/Deserializer
participant Manager as LiveMap/LiveCounter Manager
Client->>Serializer: build ObjectOperation (MapCreate/MapSet/CounterInc/...)
Serializer->>Transport: encode (msgpack/json, Base64 bytes)
Transport->>Server: deliver message
Server->>Serializer: decode -> ObjectOperation
Server->>Manager: apply operation (mapSet/mapRemove/counterInc/...)
Manager->>Manager: update state, resolve object references
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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)
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
liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
Show resolved
Hide resolved
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-208/protocol-v6
branch
from
031a4db to
8b2616f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (1)
445-447:⚠️ Potential issue | 🔴 CriticalUse UTF-8 byte lengths in payload size methods (still undercounting multi-byte chars).
These size helpers use
String.length, which can undercount UTF-8 bytes and let oversized payloads pass local checks.💡 Proposed fix
private fun MapCreate.size(): Int { - return entries.entries.sumOf { it.key.length + it.value.size() } + return entries.entries.sumOf { it.key.byteSize + it.value.size() } } private fun MapSet.size(): Int { - return key.length + value.size() + return key.byteSize + value.size() } private fun MapRemove.size(): Int { - return key.length + return key.byteSize } private fun MapCreateWithObjectId.size(): Int { - return initialValue.length + nonce.length + return initialValue.byteSize + nonce.byteSize } private fun CounterCreateWithObjectId.size(): Int { - return initialValue.length + nonce.length + return initialValue.byteSize + nonce.byteSize }#!/bin/bash set -euo pipefail # Verify size helpers still using String.length in ObjectMessage payload size methods. rg -nP 'private fun (MapCreate|MapSet|MapRemove|MapCreateWithObjectId|CounterCreateWithObjectId)\.size\(\)|return .*length' \ liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -C2Also applies to: 452-454, 459-461, 480-482, 487-489
🤖 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 445 - 447, The size helpers use String.length which undercounts multi-byte UTF-8 bytes; update all occurrences in MapCreate.size(), MapSet.size(), MapRemove.size(), MapCreateWithObjectId.size(), CounterCreateWithObjectId.size() (and any similar helpers) to compute byte lengths using UTF-8 (e.g. replace uses of key.length and any String.value.length with key.toByteArray(Charsets.UTF_8).size and stringValue.toByteArray(Charsets.UTF_8).size) and keep using value.size() for nested/structured values that already return byte counts.liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (1)
169-176:⚠️ Potential issue | 🟠 MajorEnforce
action↔ payload shape invariants forObjectOperation.Line 169–176 and Line 189–227 allow multiple payload blocks to coexist, and Line 243–310 deserializes without validating that payload matches
action. This can generate/accept invalid wire operations.🔒 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 -> + require(objectDelete != null) { "OBJECT_DELETE payload missing" } + else -> Unit + } +} + private fun ObjectOperation.writeMsgpack(packer: MessagePacker) { + validatePayloadShape() var fieldCount = 1 // action is always required require(objectId.isNotEmpty()) { "objectId must be non-empty per Objects protocol" } ... }private fun readObjectOperation(unpacker: MessageUnpacker): ObjectOperation { ... - return ObjectOperation( + val operation = ObjectOperation( action = action, objectId = objectId, mapCreate = mapCreate, mapSet = mapSet, mapRemove = mapRemove, counterCreate = counterCreate, counterInc = counterInc, objectDelete = objectDelete, mapCreateWithObjectId = mapCreateWithObjectId, counterCreateWithObjectId = counterCreateWithObjectId, nonce = nonce, initialValue = initialValue, ) + operation.validatePayloadShape() + return operation }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 payload logic allows multiple payload fields to be set and skips validating that the payload corresponds to the declared action; update the serializer and deserializer in MsgpackSerialization (the code touching mapCreate, mapSet, mapRemove, counterCreate, counterInc, objectDelete, mapCreateWithObjectId, counterCreateWithObjectId and the ObjectOperation read/write paths) to (1) enforce at write time that exactly one payload field is present and that it matches the ObjectOperation.action value, and (2) when reading, validate that only the payload field appropriate for the decoded action is present (reject or error on multiple or mismatched payload fields). Locate the write logic incrementing fieldCount and the read logic that populates those same payload variables and add a single consistent guard that maps each Action enum value to the single allowed payload symbol and throws/returns an error on violation.
🧹 Nitpick comments (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
31-85: Consider adding Unicode coverage for key/nonce/initialValue sizing paths.Current Unicode coverage is strong for
ObjectData.string, but not forMapSet.keyand*CreateWithObjectIdstring fields. Adding a focused test there would better lock size-accounting regressions.Also applies to: 134-147
🤖 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 31 - 85, The test lacks Unicode/multibyte character coverage for size calculations on MapSet.key and various create-with-object-id string fields (nonce, initialValue, and any "*CreateWithObjectId" fields); update ObjectMessageSizeTest by adding at least one map entry and a MapSet.key containing multibyte characters (e.g., "κey𝄞") and set nonce/initialValue (and any CreateWithObjectId string fields used in ObjectOperation/MapCreate/CounterCreate) to Unicode strings, then adjust the expected byte-size calculations (using UTF-8 byte lengths via the same gson/toJson or .toByteArray(Charsets.UTF_8) method used elsewhere) so assertions for ObjectMessage, ObjectOperation, MapSet, MapCreate, and CounterCreate total sizes account for the multibyte byte counts.
🤖 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/serialization/JsonSerialization.kt`:
- Around line 62-64: JSON deserializer (JsonSerialization.readObjectData)
rejects an ObjectData with no objectId and no value fields, but
MsgpackSerialization.readObjectData does not, causing inconsistent behavior; add
the same validation to MsgpackSerialization.readObjectData by checking that if
objectId == null then at least one of string, number, boolean, bytes, or json is
non-null and throw the same parsing exception (mirror the JsonParseException
behavior used in JsonSerialization.readObjectData) so both transports reject the
invalid all-null ObjectData.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt`:
- Line 53: The Base64 decoding call using Base64.getDecoder().decode(d.bytes) in
LiveMapEntry (where LiveMapValue.of is returned) can throw
IllegalArgumentException on malformed input; wrap the decode in a try/catch that
catches IllegalArgumentException (and optionally NullPointerException) and
return null when decoding fails instead of letting the exception propagate,
i.e., replace the direct decode call with a guarded decode inside a try block
and on exception return null so that the function continues to honor its
nullable LiveMapValue? contract.
---
Duplicate comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt`:
- Around line 445-447: The size helpers use String.length which undercounts
multi-byte UTF-8 bytes; update all occurrences in MapCreate.size(),
MapSet.size(), MapRemove.size(), MapCreateWithObjectId.size(),
CounterCreateWithObjectId.size() (and any similar helpers) to compute byte
lengths using UTF-8 (e.g. replace uses of key.length and any String.value.length
with key.toByteArray(Charsets.UTF_8).size and
stringValue.toByteArray(Charsets.UTF_8).size) and keep using value.size() for
nested/structured values that already return byte counts.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt`:
- Around line 169-176: The ObjectOperation payload logic allows multiple payload
fields to be set and skips validating that the payload corresponds to the
declared action; update the serializer and deserializer in MsgpackSerialization
(the code touching mapCreate, mapSet, mapRemove, counterCreate, counterInc,
objectDelete, mapCreateWithObjectId, counterCreateWithObjectId and the
ObjectOperation read/write paths) to (1) enforce at write time that exactly one
payload field is present and that it matches the ObjectOperation.action value,
and (2) when reading, validate that only the payload field appropriate for the
decoded action is present (reject or error on multiple or mismatched payload
fields). Locate the write logic incrementing fieldCount and the read logic that
populates those same payload variables and add a single consistent guard that
maps each Action enum value to the single allowed payload symbol and
throws/returns an error on violation.
---
Nitpick comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt`:
- Around line 31-85: The test lacks Unicode/multibyte character coverage for
size calculations on MapSet.key and various create-with-object-id string fields
(nonce, initialValue, and any "*CreateWithObjectId" fields); update
ObjectMessageSizeTest by adding at least one map entry and a MapSet.key
containing multibyte characters (e.g., "κey𝄞") and set nonce/initialValue (and
any CreateWithObjectId string fields used in
ObjectOperation/MapCreate/CounterCreate) to Unicode strings, then adjust the
expected byte-size calculations (using UTF-8 byte lengths via the same
gson/toJson or .toByteArray(Charsets.UTF_8) method used elsewhere) so assertions
for ObjectMessage, ObjectOperation, MapSet, MapCreate, and CounterCreate total
sizes account for the multibyte byte counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aad2b48b-d936-48d0-b72c-abd125adc0f3
📒 Files selected for processing (28)
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/JsonSerialization.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/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.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 (2)
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
✅ Files skipped from review due to trivial changes (1)
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/src/main/java/io/ably/lib/transport/Defaults.java
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt
- lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
Comment on lines
+62
to
64
| if (objectId == null && string == null && number == null && boolean == null && bytes == null && json == null) { | ||
| throw JsonParseException("Since objectId is not present, at least one of the value fields must be present") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the MsgpackSerialization readObjectData implementation
rg -n -A40 'private fun readObjectData' liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.ktRepository: ably/ably-java
Length of output: 1409
Validation inconsistency between JSON and Msgpack deserializers.
JsonSerialization.readObjectData throws JsonParseException when objectId and all value fields are null (lines 62-64), but MsgpackSerialization.readObjectData (lines 870-905) lacks this validation and silently accepts the all-null state. This creates divergent behavior across transports: JSON rejects invalid payloads while Msgpack accepts them.
Consider either:
- Adding the same validation to
MsgpackSerialization.readObjectData, or - Removing this validation from
JsonSerializationand handling invalidObjectDatadownstream
🤖 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/JsonSerialization.kt`
around lines 62 - 64, JSON deserializer (JsonSerialization.readObjectData)
rejects an ObjectData with no objectId and no value fields, but
MsgpackSerialization.readObjectData does not, causing inconsistent behavior; add
the same validation to MsgpackSerialization.readObjectData by checking that if
objectId == null then at least one of string, number, boolean, bytes, or json is
non-null and throw the same parsing exception (mirror the JsonParseException
behavior used in JsonSerialization.readObjectData) so both transports reject the
invalid all-null ObjectData.
liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
Show resolved
Hide resolved
| d.string?.let { return LiveMapValue.of(it) } | ||
| d.number?.let { return LiveMapValue.of(it) } | ||
| d.boolean?.let { return LiveMapValue.of(it) } | ||
| d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's see all Base64 decode calls in the codebase
echo "=== All Base64.decode calls ==="
rg -nP 'Base64\.getDecoder\(\)\.decode\(' liveobjects/src/main/kotlin/io/ably/lib/objects -C2
echo ""
echo "=== File structure around target file ==="
fd -t f "LiveMapEntry" liveobjects/src/main/kotlin/io/ably/lib/objects/Repository: ably/ably-java
Length of output: 2012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the LiveMapEntry.kt file to understand context
echo "=== LiveMapEntry.kt content ==="
cat -n liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt | head -100Repository: ably/ably-java
Length of output: 3673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what function contains line 53
echo "=== Function context around line 53 ==="
cat -n liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt | sed -n '40,70p'Repository: ably/ably-java
Length of output: 1471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check MsgpackSerialization.kt around line 855
echo "=== MsgpackSerialization.kt around line 855 ==="
cat -n liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt | sed -n '845,865p'
echo ""
echo "=== ObjectMessage.kt around line 530 ==="
cat -n liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt | sed -n '520,540p'Repository: ably/ably-java
Length of output: 1485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any try-catch blocks around Base64 decode in the entire codebase
echo "=== Search for try-catch patterns with Base64 ==="
rg -B5 -A5 'Base64.*decode' liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt | head -40
echo ""
echo "=== Check ObjectMessage context ==="
rg -B10 -A5 'Base64.*decode' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.ktRepository: ably/ably-java
Length of output: 844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what callers of getResolvedValue() do with exceptions
echo "=== Find all callers of getResolvedValue ==="
rg 'getResolvedValue' liveobjects/src/main/kotlin/io/ably/lib/objects -A2 -B2
echo ""
echo "=== Check if there's any exception handling at the callsite ==="
rg 'getResolvedValue' liveobjects/src/main/kotlin -A5 -B5 | head -100Repository: ably/ably-java
Length of output: 5625
🌐 Web query:
Java Base64.getDecoder().decode() exception behavior malformed input
💡 Result:
Base64.getDecoder() returns the “Basic” Base64 decoder (RFC 4648). Its decode(...) methods are strict and will fail fast on malformed input:
- Throws
IllegalArgumentExceptionif the input is not valid Base64 (e.g., contains characters outside the Base64 alphabet, bad length/quantum structure, etc.). The Basic decoder specifically rejects data containing characters outside the Base64 alphabet. [1][2] - Padding rules: missing
=padding at the end is allowed (2 or 3 chars in the final quartet are treated as if padded), but if=is present, the number of=must be correct or decoding throwsIllegalArgumentException. [2] nullinput →NullPointerException. [2]- If you decode from a stream via
decoder.wrap(InputStream), malformed data is reported as anIOExceptionduring reads (rather thanIllegalArgumentException). [2]
If your “malformed input” is actually due to whitespace/line breaks or other separators (common in PEM/MIME), use Base64.getMimeDecoder(), whose decoding ignores line separators and other non-alphabet characters. [1]
Sources:
[1] Java Base64 (Basic/URL/MIME behavior) Javadoc (Java SE 24) (docs.oracle.com)
[2] Java Base64.Decoder (exceptions, padding, wrap-stream behavior) Javadoc (Java SE 24) (docs.oracle.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/util/Base64.html
- 2: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/util/Base64.Decoder.html
Guard Base64 decoding to prevent crashes on malformed payloads.
The Base64.getDecoder().decode() call at line 53 lacks error handling and will throw IllegalArgumentException if the bytes contain invalid Base64 characters. Since the function returns LiveMapValue? (nullable) and safely handles other missing/invalid conditions, decode failures should return null instead of propagating exceptions up to user-facing map read operations.
Proposed fix
- d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) }
+ d.bytes?.let { encoded ->
+ val decoded = runCatching { Base64.getDecoder().decode(encoded) }.getOrNull()
+ ?: return null
+ return LiveMapValue.of(decoded)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } | |
| d.bytes?.let { encoded -> | |
| val decoded = runCatching { Base64.getDecoder().decode(encoded) }.getOrNull() | |
| ?: return null | |
| return LiveMapValue.of(decoded) | |
| } |
🤖 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/LiveMapEntry.kt`
at line 53, The Base64 decoding call using Base64.getDecoder().decode(d.bytes)
in LiveMapEntry (where LiveMapValue.of is returned) can throw
IllegalArgumentException on malformed input; wrap the decode in a try/catch that
catches IllegalArgumentException (and optionally NullPointerException) and
return null when decoding fails instead of letting the exception propagate,
i.e., replace the direct decode call with a guarded decode inside a try block
and on exception return null so that the function continues to honor its
nullable LiveMapValue? contract.
8b2616f to
104dbbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (1)
53-53:⚠️ Potential issue | 🟠 MajorGuard Base64 decode failures in map reads.
A malformed
bytespayload will throw out ofgetResolvedValue()here and breakLiveMap.get()/entries()instead of behaving like an unreadable entry. Returningnullon decode failure keeps this resolver aligned with its nullable contract.Proposed fix
- d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } + d.bytes?.let { encoded -> + val decoded = runCatching { Base64.getDecoder().decode(encoded) }.getOrNull() + ?: return null + return LiveMapValue.of(decoded) + }🤖 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/LiveMapEntry.kt` at line 53, Guard the Base64 decoding in LiveMapEntry.getResolvedValue(): avoid letting Base64.getDecoder().decode(...) throw out of getResolvedValue() by wrapping the decode call (the d.bytes?.let { ... } branch that constructs LiveMapValue.of(...)) in a try/catch for decoding failures (e.g., IllegalArgumentException) and return null when decoding fails so the resolver honors its nullable contract instead of propagating an exception.liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (1)
403-409:⚠️ Potential issue | 🟠 MajorMeasure
extrasin UTF-8 bytes too.
gson.toJson(it).lengthstill counts UTF-16 code units, so multibyte content inextrasis undercounted and can slip past max-size checks. UsebyteSizehere like the other fixed string paths.Proposed fix
- val extrasSize = extras?.let { gson.toJson(it).length } ?: 0 // Spec: OM3d + val extrasSize = extras?.let { gson.toJson(it).byteSize } ?: 0 // Spec: OM3d🤖 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 403 - 409, ObjectMessage.size() is undercounting extras because gson.toJson(it).length measures UTF-16 code units; change the extras size calculation to measure UTF-8 byte length (use the same byteSize helper used for clientId, e.g. replace gson.toJson(it).length with gson.toJson(it).byteSize or call the existing byteSize extension/function) so extras is counted in actual UTF-8 bytes for correct max-size checks; update the expression referenced as extrasSize and run related size tests.liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt (1)
62-64:⚠️ Potential issue | 🟠 MajorMirror this
ObjectDatavalidation in the Msgpack path.This all-null guard only exists on JSON here, so the same invalid payload is still transport-dependent unless
MsgpackSerialization.readObjectData()performs the same check.🤖 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/JsonSerialization.kt` around lines 62 - 64, The Msgpack path is missing the same all-null guard as in JsonSerialization, so add the identical validation inside MsgpackSerialization.readObjectData(): after parsing objectId, string, number, boolean, bytes and json values, check if objectId == null && string == null && number == null && boolean == null && bytes == null && json == null and if so throw JsonParseException("Since objectId is not present, at least one of the value fields must be present") to ensure transport-independent validation of ObjectData.liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt (2)
822-865:⚠️ Potential issue | 🟠 MajorAlign
ObjectDataMsgPack validation with the JSON path.This path still permits both an empty
{}payload and mixed typed values likestring+number.JsonSerialization.deserializealready rejects the empty case, so the same logical payload can be accepted over MsgPack and rejected over JSON.Suggested fix
+private fun ObjectData.validateValueShape() { + val valueCount = listOf(string, number, boolean, bytes, json).count { it != null } + require(!(objectId == null && valueCount == 0)) { + "Since objectId is not present, at least one of the value fields must be present" + } + require(valueCount <= 1) { "ObjectData must contain at most one typed value" } +} + private fun ObjectData.writeMsgpack(packer: MessagePacker) { + validateValueShape() var fieldCount = 0 ... } private fun readObjectData(unpacker: MessageUnpacker): ObjectData { ... - return ObjectData(objectId = objectId, string = string, number = number, boolean = boolean, bytes = bytes, json = json) + return ObjectData( + objectId = objectId, + string = string, + number = number, + boolean = boolean, + bytes = bytes, + json = json, + ).also { it.validateValueShape() } }Also applies to: 870-905
🤖 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 822 - 865, ObjectData.writeMsgpack currently allows empty objects and multiple typed value fields; update it to mirror JsonSerialization.deserialize by validating the payload before packing: compute the count of value fields (string, number, boolean, bytes, json) and throw an exception if that count is 0 (empty payload) or >1 (mixed types), then proceed to pack; apply the same validation logic to the corresponding MsgPack reader/writer pair referenced around lines 870-905 (the related ObjectData msgpack method) so both serialization and deserialization enforce the same single-value constraint.
164-238:⚠️ Potential issue | 🟠 MajorReject invalid
ObjectOperationshapes before they hit the wire.This still accepts impossible combinations like
action = MapSetwithcounterInc != null, or multiple payload blocks at once. That makes the wire format non-canonical and pushes undefined behavior downstream into apply logic.Suggested fix
+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 -> + require(objectDelete != null) { "OBJECT_DELETE payload missing" } + else -> Unit + } +} + private fun ObjectOperation.writeMsgpack(packer: MessagePacker) { + validatePayloadShape() var fieldCount = 1 ... }Apply the same validation after
readObjectOperation(...)builds theObjectOperation.Also applies to: 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 164 - 238, The write path currently serializes invalid ObjectOperation shapes (e.g., action = MapSet while counterInc != null or multiple payloads set); add validation in ObjectOperation.writeMsgpack to reject impossible combinations by checking that exactly one payload block (mapCreate, mapSet, mapRemove, counterCreate, counterInc, objectDelete, mapCreateWithObjectId, counterCreateWithObjectId, nonce, initialValue) matches the declared action and that no other payload fields are non-null, and throw/require with a clear message if invalid; mirror the same validation in the read side immediately after readObjectOperation constructs the ObjectOperation so deserialized objects are also canonical and rejected early.
🤖 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/serialization/JsonSerialization.kt`:
- Around line 60-65: The current JsonSerialization.kt allows "json" primitives
or nulls to parse and later disappear; update the parsing of the json field
(where json is set from JsonParser.parseString(obj.get("json").asString)) to
validate the parsed JsonElement: if obj.has("json") then parse it and throw
JsonParseException from JsonSerialization (or the existing JsonParseException)
unless the parsed element is either a JsonObject or JsonArray; also treat
JsonNull as an explicit error (do not accept or silently ignore it). Ensure
ObjectData(json = ...) only receives a JsonObject/JsonArray and raise a clear
JsonParseException if the "json" field is present but not an object/array.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt`:
- Around line 197-200: The test builds an invalid ObjectOperation by using
action = ObjectOperationAction.CounterCreate while populating counterInc; change
the payload to match the action (use the counterCreate field) so the operation
shape is valid. Locate the ObjectOperation construction in ObjectsManagerTest
(the instance with action = ObjectOperationAction.CounterCreate) and replace the
counterInc usage with the correct counterCreate payload (or populate both
appropriately if the constructor expects a specific wrapper), ensuring the field
names align with the CounterCreate action.
---
Duplicate comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt`:
- Around line 403-409: ObjectMessage.size() is undercounting extras because
gson.toJson(it).length measures UTF-16 code units; change the extras size
calculation to measure UTF-8 byte length (use the same byteSize helper used for
clientId, e.g. replace gson.toJson(it).length with gson.toJson(it).byteSize or
call the existing byteSize extension/function) so extras is counted in actual
UTF-8 bytes for correct max-size checks; update the expression referenced as
extrasSize and run related size tests.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt`:
- Around line 62-64: The Msgpack path is missing the same all-null guard as in
JsonSerialization, so add the identical validation inside
MsgpackSerialization.readObjectData(): after parsing objectId, string, number,
boolean, bytes and json values, check if objectId == null && string == null &&
number == null && boolean == null && bytes == null && json == null and if so
throw JsonParseException("Since objectId is not present, at least one of the
value fields must be present") to ensure transport-independent validation of
ObjectData.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt`:
- Around line 822-865: ObjectData.writeMsgpack currently allows empty objects
and multiple typed value fields; update it to mirror
JsonSerialization.deserialize by validating the payload before packing: compute
the count of value fields (string, number, boolean, bytes, json) and throw an
exception if that count is 0 (empty payload) or >1 (mixed types), then proceed
to pack; apply the same validation logic to the corresponding MsgPack
reader/writer pair referenced around lines 870-905 (the related ObjectData
msgpack method) so both serialization and deserialization enforce the same
single-value constraint.
- Around line 164-238: The write path currently serializes invalid
ObjectOperation shapes (e.g., action = MapSet while counterInc != null or
multiple payloads set); add validation in ObjectOperation.writeMsgpack to reject
impossible combinations by checking that exactly one payload block (mapCreate,
mapSet, mapRemove, counterCreate, counterInc, objectDelete,
mapCreateWithObjectId, counterCreateWithObjectId, nonce, initialValue) matches
the declared action and that no other payload fields are non-null, and
throw/require with a clear message if invalid; mirror the same validation in the
read side immediately after readObjectOperation constructs the ObjectOperation
so deserialized objects are also canonical and rejected early.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt`:
- Line 53: Guard the Base64 decoding in LiveMapEntry.getResolvedValue(): avoid
letting Base64.getDecoder().decode(...) throw out of getResolvedValue() by
wrapping the decode call (the d.bytes?.let { ... } branch that constructs
LiveMapValue.of(...)) in a try/catch for decoding failures (e.g.,
IllegalArgumentException) and return null when decoding fails so the resolver
honors its nullable contract instead of propagating an exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad64cbcd-5a84-4b25-be9b-c3295c1ef235
📒 Files selected for processing (16)
liveobjects/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/JsonSerialization.ktliveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.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/ObjectsManagerTest.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 (2)
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.kt
Comment on lines
+60
to
+65
| val json = if (obj.has("json")) JsonParser.parseString(obj.get("json").asString) else null | ||
|
|
||
| if (objectId == null && string == null && number == null && boolean == null && bytes == null && json == null) { | ||
| throw JsonParseException("Since objectId is not present, at least one of the value fields must be present") | ||
| } | ||
| return ObjectData(objectId, value) | ||
| return ObjectData(objectId = objectId, string = string, number = number, boolean = boolean, bytes = bytes, json = json) |
There was a problem hiding this comment.
Reject json primitives and null at parse time.
ObjectData.json is documented as object/array only, and liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt only surfaces JsonObject / JsonArray. Right now payloads like "json":"1" deserialize successfully and then disappear as null later.
Proposed fix
- val json = if (obj.has("json")) JsonParser.parseString(obj.get("json").asString) else null
+ val json = if (obj.has("json")) {
+ JsonParser.parseString(obj.get("json").asString).also {
+ if (!it.isJsonObject && !it.isJsonArray) {
+ throw JsonParseException("json must contain a JSON object or array")
+ }
+ }
+ } else null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val json = if (obj.has("json")) JsonParser.parseString(obj.get("json").asString) else null | |
| if (objectId == null && string == null && number == null && boolean == null && bytes == null && json == null) { | |
| throw JsonParseException("Since objectId is not present, at least one of the value fields must be present") | |
| } | |
| return ObjectData(objectId, value) | |
| return ObjectData(objectId = objectId, string = string, number = number, boolean = boolean, bytes = bytes, json = json) | |
| val json = if (obj.has("json")) { | |
| JsonParser.parseString(obj.get("json").asString).also { | |
| if (!it.isJsonObject && !it.isJsonArray) { | |
| throw JsonParseException("json must contain a JSON object or array") | |
| } | |
| } | |
| } else null | |
| if (objectId == null && string == null && number == null && boolean == null && bytes == null && json == null) { | |
| throw JsonParseException("Since objectId is not present, at least one of the value fields must be present") | |
| } | |
| return ObjectData(objectId = objectId, string = string, number = number, boolean = boolean, bytes = bytes, json = json) |
🤖 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/JsonSerialization.kt`
around lines 60 - 65, The current JsonSerialization.kt allows "json" primitives
or nulls to parse and later disappear; update the parsing of the json field
(where json is set from JsonParser.parseString(obj.get("json").asString)) to
validate the parsed JsonElement: if obj.has("json") then parse it and throw
JsonParseException from JsonSerialization (or the existing JsonParseException)
unless the parsed element is either a JsonObject or JsonArray; also treat
JsonNull as an explicit error (do not accept or silently ignore it). Ensure
ObjectData(json = ...) only receives a JsonObject/JsonArray and raise a clear
JsonParseException if the "json" field is present but not an object/array.
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the LiveObjects Kotlin implementation to protocol v6 by renaming operation payload fields, introducing typed ObjectData primitives, and extending serialization/size-calculation logic to cover the new operation shapes (including *CreateWithObjectId).
Changes:
- Renames
ObjectOperationpayload fields to protocol v6 names (e.g.,mapCreate,mapSet,mapRemove,counterCreate,counterInc) and removes prior field usage across code/tests. - Replaces
ObjectData(value = ObjectValue.*)with typedObjectDatafields (string,number,boolean,bytes,json) throughout unit/integration fixtures and logic. - Updates MsgPack/JSON serialization and message sizing to support the new payloads and bumps Ably protocol version to 6.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt | Updates unit tests to use mapCreate/mapSet/mapRemove and typed ObjectData fields. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt | Updates map object tests to use new operation payload types and typed ObjectData. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt | Updates counter manager tests for counterCreate/counterInc and adjusts missing-payload behavior expectations. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt | Updates counter tests to new payload fields and typed ObjectData. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt | Updates ACK/apply-on-ACK tests to use new operation payload field names. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt | Updates realtime objects tests to use counterInc payload. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt | Refactors fixtures to typed ObjectData, adds base64 encoding for bytes, and switches to new op payloads. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt | Updates size tests to use new payload names and typed ObjectData fields. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt | Updates serialization null-handling test to match new operation fields. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt | Removes tests related to the removed Binary helper. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.kt | Updates integration fixtures to typed ObjectData primitives. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.kt | Updates integration fixtures to typed ObjectData and base64 bytes representation. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.kt | Changes setMapValue helper API to accept ObjectData instead of ObjectValue. |
| liveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt | Updates integration tests to pass typed ObjectData to REST helper methods. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt | Adapts map operation application to MapCreate/MapSet/MapRemove payloads. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt | Resolves typed ObjectData primitives (including base64 bytes decode) into LiveMapValue. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt | Emits mapSet/mapRemove operations and builds initial MapCreate payload from entries with typed ObjectData. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt | Switches increment handling to CounterInc(number) and create merge to counterCreate. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt | Emits counterInc operations and builds initial CounterCreate payload. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt | Implements MsgPack read/write for new operation payloads and typed ObjectData fields. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/JsonSerialization.kt | Updates ObjectData JSON adapter to typed fields and new json/bytes representations. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt | Introduces typed ObjectData, new payload classes, updates operation fields, and revises size calculation logic. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt | Removes now-unused Binary helper and legacy create-payload wrappers. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt | Updates object creation operations to use mapCreate/counterCreate directly. |
| lib/src/test/java/io/ably/lib/transport/DefaultsTest.java | Updates protocol version expectation to 6. |
| lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java | Updates REST header version expectation to 6. |
| lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java | Updates realtime version param expectation to 6. |
| lib/src/main/java/io/ably/lib/transport/Defaults.java | Bumps Defaults.ABLY_PROTOCOL_VERSION from 5 to 6. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
Show resolved
Hide resolved
Comment on lines
+56
to
+60
| val string = if (obj.has("string")) obj.get("string").asString else null | ||
| val number = if (obj.has("number")) obj.get("number").asDouble else null | ||
| val boolean = if (obj.has("boolean")) obj.get("boolean").asBoolean else null | ||
| val bytes = if (obj.has("bytes")) obj.get("bytes").asString else null | ||
| val json = if (obj.has("json")) JsonParser.parseString(obj.get("json").asString) else null |
There was a problem hiding this comment.
ObjectDataJsonSerializer.deserialize() now parses json into a JsonElement without validating it is an object/array, and it will happily populate multiple typed fields if present. This can lead to ambiguous values (different code paths pick different precedence) and incorrect size calculations. Consider enforcing that at most one of {string, number, boolean, bytes, json} is present (and that json is object/array, bytes is valid base64), otherwise throw JsonParseException.
| } else { | ||
| // RTLM17a1 - entry in MAP_CREATE op is not removed, try to set it via MAP_SET op | ||
| applyMapSet(ObjectsMapOp(key, entry.data), opTimeserial) | ||
| applyMapSet(MapSet(key, entry.data ?: return@forEach), opTimeserial) |
There was a problem hiding this comment.
In mergeInitialDataFromCreateOperation, non-tombstoned entries with data == null are silently skipped via return@forEach. Previously this path would flow through applyMapSet and raise an invalid-data error. Silently dropping malformed entries can mask protocol issues and leave the map in a partially-applied state; consider throwing an objectError (or otherwise failing the operation) when a non-tombstone entry is missing data.
Suggested change
| applyMapSet(MapSet(key, entry.data ?: return@forEach), opTimeserial) | |
| val data = entry.data ?: run { | |
| objectError("Invalid MAP_CREATE entry for key '$key': non-tombstone entry is missing data") | |
| } | |
| applyMapSet(MapSet(key, data), opTimeserial) |
Collaborator
There was a problem hiding this comment.
@coderabbitai can you check above suggestion keenly and provide more details around the same
There was a problem hiding this comment.
@sacOO7, here's a detailed analysis of the Copilot suggestion:
1. The suggestion has a bug
The most important issue: the suggested code is syntactically valid but semantically wrong. Throughout this file, objectError(...) is a function that returns an AblyException — it does not throw by itself (every usage is throw objectError(...)). So this block:
val data = entry.data ?: run {
objectError("Invalid MAP_CREATE entry for key '$key': non-tombstone entry is missing data")
}would compile, but it would assign the exception object to data instead of throwing it — silently continuing with an AblyException as the ObjectData value. The correct Kotlin idiom is:
val data = entry.data ?: throw objectError("Invalid MAP_CREATE entry for key '$key': non-tombstone entry is missing data")
applyMapSet(MapSet(key, data), opTimeserial)2. The behavioral concern is valid
The concern about silent skipping is legitimate. Here's the comparison across all three behaviors:
| Code version | entry.data == null on non-tombstone entry |
|---|---|
Old code (ObjectsMapOp) |
applyMapSet was called with mapOp.data = null; the ?.isInvalid() null-safety returned null (falsy), so no error was thrown and the entry was stored with null data — arguably worse |
| New code (current PR) | return@forEach — entry silently skipped, map partially applied |
| Copilot's intent | Throw an error — defensive, surfaces protocol violations |
The current PR's return@forEach is actually an improvement over the old behavior (which stored null data), but throwing would be the most defensive option and consistent with the error-throwing pattern used elsewhere in applyMapSet:
if (mapSet.value.isInvalid()) {
throw objectError("Invalid object data for MAP_SET op on objectId=${objectId} on key=${mapSet.key}")
}Recommendation
If you want to adopt the spirit of the suggestion correctly, the fix would be:
} else {
// RTLM17a1 - entry in MAP_CREATE op is not removed, try to set it via MAP_SET op
val data = entry.data ?: throw objectError("Invalid MAP_CREATE entry: non-tombstone entry missing data for key='$key'; objectId=${objectId}")
applyMapSet(MapSet(key, data), opTimeserial)
}However, if the intent is to be resilient to malformed protocol messages (e.g., partial sync payloads from server), keeping return@forEach with an added warning log would also be reasonable:
val data = entry.data ?: run {
Log.w(tag, "Skipping MAP_CREATE entry: non-tombstone entry missing data for key='$key'; objectId=${objectId}")
return@forEach
}
applyMapSet(MapSet(key, data), opTimeserial)The key takeaway: the Copilot suggestion as written should not be applied as-is due to the missing throw.
Comment on lines
+527
to
+532
| string?.let { return it.byteSize } // Spec: OD3e | ||
| number?.let { return 8 } // Spec: OD3d | ||
| boolean?.let { return 1 } // Spec: OD3b | ||
| bytes?.let { return Base64.getDecoder().decode(it).size } // Spec: OD3c | ||
| json?.let { return it.toString().byteSize } // Spec: OD3e | ||
| return 0 |
There was a problem hiding this comment.
ObjectData.size() returns on the first non-null typed field and Base64-decodes bytes without guarding. If multiple fields are present (possible via JSON/MsgPack input), the size is undercounted; and if bytes is not valid base64 this will throw IllegalArgumentException during size calculation. Consider enforcing “exactly one of {string,number,boolean,bytes,json,objectId}” during deserialization/construction, or summing sizes for all populated fields and wrapping invalid base64 in a protocol/AblyException.
Suggested change
| string?.let { return it.byteSize } // Spec: OD3e | |
| number?.let { return 8 } // Spec: OD3d | |
| boolean?.let { return 1 } // Spec: OD3b | |
| bytes?.let { return Base64.getDecoder().decode(it).size } // Spec: OD3c | |
| json?.let { return it.toString().byteSize } // Spec: OD3e | |
| return 0 | |
| var totalSize = 0 | |
| string?.let { | |
| totalSize += it.byteSize // Spec: OD3e | |
| } | |
| number?.let { | |
| totalSize += 8 // Spec: OD3d | |
| } | |
| boolean?.let { | |
| totalSize += 1 // Spec: OD3b | |
| } | |
| bytes?.let { | |
| totalSize += try { | |
| Base64.getDecoder().decode(it).size // Spec: OD3c | |
| } catch (e: IllegalArgumentException) { | |
| 0 | |
| } | |
| } | |
| json?.let { | |
| totalSize += it.toString().byteSize // Spec: OD3e | |
| } | |
| objectId?.let { | |
| totalSize += it.byteSize // Spec: OD3e (objectId treated as string-like) | |
| } | |
| return totalSize |
Collaborator
There was a problem hiding this comment.
As per spec, only one field will be populated, so not sure if this is really needed
| d.string?.let { return LiveMapValue.of(it) } | ||
| d.number?.let { return LiveMapValue.of(it) } | ||
| d.boolean?.let { return LiveMapValue.of(it) } | ||
| d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } |
There was a problem hiding this comment.
LiveMapEntry.getResolvedValue() Base64-decodes ObjectData.bytes directly. If the server (or malformed local input) provides an invalid base64 string, this will throw and break LiveMap.get() for that entry. Consider validating/normalizing bytes on decode (or catching IllegalArgumentException here and treating the entry as invalid/null, ideally with a logged warning or protocol error).
Suggested change
| d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } | |
| d.bytes?.let { | |
| return try { | |
| LiveMapValue.of(Base64.getDecoder().decode(it)) | |
| } catch (e: IllegalArgumentException) { | |
| // Invalid Base64 input; treat as no value for this entry | |
| null | |
| } | |
| } |
Collaborator
There was a problem hiding this comment.
Is this scenario possible? If yes, then spec and relevant code can be updated
liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.kt
Show resolved
Hide resolved
Comment on lines
+855
to
+858
| val rawBytes = Base64.getDecoder().decode(bytes) | ||
| packer.packString("bytes") | ||
| packer.packBinaryHeader(rawBytes.size) | ||
| packer.writePayload(rawBytes) |
There was a problem hiding this comment.
ObjectData.writeMsgpack() Base64-decodes bytes without error handling. An invalid base64 string will throw IllegalArgumentException during serialization and can crash publishing. Catch/validate base64 and convert failures into a controlled AblyException (or mark the data invalid) so callers get a meaningful error.
Suggested change
| val rawBytes = Base64.getDecoder().decode(bytes) | |
| packer.packString("bytes") | |
| packer.packBinaryHeader(rawBytes.size) | |
| packer.writePayload(rawBytes) | |
| packer.packString("bytes") | |
| try { | |
| val rawBytes = Base64.getDecoder().decode(bytes) | |
| packer.packBinaryHeader(rawBytes.size) | |
| packer.writePayload(rawBytes) | |
| } catch (e: IllegalArgumentException) { | |
| // Invalid base64 in bytes field; mark as invalid by encoding a nil value | |
| packer.packNil() | |
| } |
Collaborator
There was a problem hiding this comment.
Is this scenario possible? If yes, then relevant code can be updated
Contributor
Author
There was a problem hiding this comment.
Not sure it's possible, but we definitely shouldn't swallow error here
ttypic
force-pushed
the
AIT-208/protocol-v6
branch
from
104dbbf to
0a9ee7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (2)
40-61:⚠️ Potential issue | 🔴 CriticalThis
ObjectDataAPI change still leaves compile-breaking call sites behind.
ObjectData("testValue1")is still used inliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt:84-89andliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt:51-56. With this new primary constructor shape, those sites no longer compile. Please migrate them in this PR or add a compatibility constructor/factory.🤖 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 40 - 61, The new primary constructor for ObjectData changed to named fields and breaks call sites using positional construction like ObjectData("testValue1"); either restore a backwards-compatible constructor or migrate callers. Fix by adding a secondary constructor (e.g., constructor(value: String) : this(string = value)) or a companion factory method (e.g., fun fromString(value: String): ObjectData = ObjectData(string = value)), placed in the ObjectData declaration, or update the tests to call ObjectData(string = "testValue1") (references: ObjectData constructor, tests calling ObjectData("...")).
389-395:⚠️ Potential issue | 🟠 MajorCount
extrasas UTF-8 bytes too.
gson.toJson(it).lengthstill counts UTF-16 code units. Multibyte characters insideextraswill be undercounted, so client-side size checks can still accept payloads that the server rejects.Suggested fix
internal fun ObjectMessage.size(): Int { val clientIdSize = clientId?.byteSize ?: 0 // Spec: OM3f val operationSize = operation?.size() ?: 0 // Spec: OM3b, OOP4 val objectStateSize = objectState?.size() ?: 0 // Spec: OM3c, OST3 - val extrasSize = extras?.let { gson.toJson(it).length } ?: 0 // Spec: OM3d + val extrasSize = extras?.let { gson.toJson(it).byteSize } ?: 0 // Spec: OM3d return clientIdSize + operationSize + objectStateSize + extrasSize }🤖 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 389 - 395, ObjectMessage.size() currently measures extras using gson.toJson(it).length which counts UTF-16 code units and underestimates multibyte UTF‑8 byte length; change the extras calculation to count UTF‑8 bytes instead (e.g. replace gson.toJson(it).length with the UTF‑8 byte length of gson.toJson(it) via toByteArray(Charsets.UTF_8).size) so the extras sizing (Spec: OM3d) correctly reflects wire size; update the extrasSize expression and ensure the return still sums clientIdSize, operationSize, objectStateSize and the new extras byte size.
♻️ Duplicate comments (2)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt (1)
197-200:⚠️ Potential issue | 🟡 MinorUse a
counterCreatepayload withCounterCreateactions.This fixture builds
action = CounterCreatewithcounterInc. If create-payload validation is tightened, the test will fail before it exercises the buffering path.Suggested fix
operation = ObjectOperation( action = ObjectOperationAction.CounterCreate, objectId = "counter:testObject@1", - counterInc = CounterInc(number = 5.0) + counterCreate = CounterCreate(count = 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/objects/ObjectsManagerTest.kt` around lines 197 - 200, The test builds an ObjectOperation using ObjectOperationAction.CounterCreate but supplies a CounterInc payload (counterInc); change the fixture to use the correct create payload field (counterCreate with a CounterCreate instance) so the operation for ObjectOperation (where action == ObjectOperationAction.CounterCreate) contains counterCreate instead of counterInc, ensuring validation passes and the buffering path is exercised.liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (1)
53-53:⚠️ Potential issue | 🟠 MajorGuard Base64 decoding before surfacing map values.
Line 53 can throw on malformed payloads, which turns
get()/entries()into user-visible failures instead of returningnullfor an unreadable value.Suggested fix
- d.bytes?.let { return LiveMapValue.of(Base64.getDecoder().decode(it)) } + d.bytes?.let { encoded -> + val decoded = runCatching { Base64.getDecoder().decode(encoded) }.getOrNull() + ?: return null + return LiveMapValue.of(decoded) + }What exception does java.util.Base64.Decoder.decode(String) throw for malformed Base64 input?🤖 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/LiveMapEntry.kt` at line 53, The Base64 decode call in LiveMapEntry (the d.bytes -> LiveMapValue.of(Base64.getDecoder().decode(it)) path) can throw on malformed input, so wrap the decode in a guard that catches the decoding exception and returns null (or a null-equivalent) instead of letting the exception propagate; specifically, catch the decoder's runtime exception when calling Base64.getDecoder().decode(it) inside the LiveMapEntry get/entries flow and return null for that entry (or skip it) so callers of LiveMapEntry.get()/entries() see null/unreadable value rather than a thrown error.
🧹 Nitpick comments (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt (1)
284-301: Duplicate test case.This test (
LiveCounterManager should throw error when counterInc payload missing) duplicates the test at lines 241-260 (LiveCounterManager should throw error for missing payload for counter increment operation). Both test the same behavior: thatapplyOperationthrows whencounterInc = nullfor aCounterIncaction.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 284 - 301, The test "LiveCounterManager should throw error when counterInc payload missing" in LiveCounterManagerTest duplicates an earlier test ("LiveCounterManager should throw error for missing payload for counter increment operation"); remove one of the two duplicate test methods to avoid redundancy. Locate the duplicate by searching for the test method names in LiveCounterManagerTest (references to getDefaultLiveCounterWithMockedDeps, liveCounter.LiveCounterManager, ObjectOperation with action = ObjectOperationAction.CounterInc and counterInc = null) and delete the redundant test block so only a single assertion of applyOperation throwing an AblyException with errorInfo.code == 92000 remains.
🤖 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 509-515: ObjectData.size() currently ignores the objectId
reference so nested-object map entries are undercounted; update
ObjectData.size() to account for objectId by adding a branch that returns the
byte size of objectId (similar to the string/json branches) when objectId is
present (use the same byteSize helper used for string/json). Ensure you
reference the objectId field in the ObjectData.size() function (and place the
check alongside the other primitive checks) so object references contribute to
operation and state sizing.
- Around line 402-410: ObjectOperation.size() currently inspects
mapCreateWithObjectId?.derivedFrom and counterCreateWithObjectId?.derivedFrom
(or falls back to 0), which measures a transient local derived object instead of
the actual on-wire payload; replace those branches to use the serialized
*CreateWithObjectId payload sizes (e.g., call mapCreateWithObjectId?.size() ?: 0
and counterCreateWithObjectId?.size() ?: 0) so the size accounts for the wire
fields (initialValue + nonce) rather than derivedFrom.
- Around line 518-525: The isInvalid() implementation no longer enforces the
one-of invariant; update ObjectData?.isInvalid() to treat the instance as
invalid when zero OR more than one of the mutually-exclusive value fields are
populated. Specifically, count how many of these value-like fields are present:
objectId (non-empty), string (non-null), number (non-null), boolean (non-null),
bytes (non-null), json (non-null) and return true if that count != 1 (or if the
receiver is null). Modify the ObjectData?.isInvalid function to perform this
count and use that result to determine invalidity so cases like
ObjectData(string="x", number=1.0) become invalid.
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt`:
- Around line 112-117: The current merge logic in LiveCounterManager (using
operation.counterCreateWithObjectId?.derivedFrom?.count ?:
operation.counterCreate?.count ?: 0.0) silently treats a missing
CounterCreate.count as 0.0 and still marks liveCounter.createOperationIsMerged;
change this to fail-fast: check for a non-null count from either
operation.counterCreateWithObjectId.derivedFrom.count or
operation.counterCreate.count, and if absent throw or return an error (e.g.,
IllegalStateException) rather than defaulting to 0.0, and only set
liveCounter.data and liveCounter.createOperationIsMerged when a valid count is
present. Ensure the null-check references the same symbols
(operation.counterCreateWithObjectId, derivedFrom, operation.counterCreate,
liveCounter.createOperationIsMerged) so malformed/partial create-with-objectId
ops do not lose their initial value.
---
Outside diff comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt`:
- Around line 40-61: The new primary constructor for ObjectData changed to named
fields and breaks call sites using positional construction like
ObjectData("testValue1"); either restore a backwards-compatible constructor or
migrate callers. Fix by adding a secondary constructor (e.g., constructor(value:
String) : this(string = value)) or a companion factory method (e.g., fun
fromString(value: String): ObjectData = ObjectData(string = value)), placed in
the ObjectData declaration, or update the tests to call ObjectData(string =
"testValue1") (references: ObjectData constructor, tests calling
ObjectData("...")).
- Around line 389-395: ObjectMessage.size() currently measures extras using
gson.toJson(it).length which counts UTF-16 code units and underestimates
multibyte UTF‑8 byte length; change the extras calculation to count UTF‑8 bytes
instead (e.g. replace gson.toJson(it).length with the UTF‑8 byte length of
gson.toJson(it) via toByteArray(Charsets.UTF_8).size) so the extras sizing
(Spec: OM3d) correctly reflects wire size; update the extrasSize expression and
ensure the return still sums clientIdSize, operationSize, objectStateSize and
the new extras byte size.
---
Duplicate comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt`:
- Line 53: The Base64 decode call in LiveMapEntry (the d.bytes ->
LiveMapValue.of(Base64.getDecoder().decode(it)) path) can throw on malformed
input, so wrap the decode in a guard that catches the decoding exception and
returns null (or a null-equivalent) instead of letting the exception propagate;
specifically, catch the decoder's runtime exception when calling
Base64.getDecoder().decode(it) inside the LiveMapEntry get/entries flow and
return null for that entry (or skip it) so callers of
LiveMapEntry.get()/entries() see null/unreadable value rather than a thrown
error.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt`:
- Around line 197-200: The test builds an ObjectOperation using
ObjectOperationAction.CounterCreate but supplies a CounterInc payload
(counterInc); change the fixture to use the correct create payload field
(counterCreate with a CounterCreate instance) so the operation for
ObjectOperation (where action == ObjectOperationAction.CounterCreate) contains
counterCreate instead of counterInc, ensuring validation passes and the
buffering path is exercised.
---
Nitpick comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/LiveCounterManagerTest.kt`:
- Around line 284-301: The test "LiveCounterManager should throw error when
counterInc payload missing" in LiveCounterManagerTest duplicates an earlier test
("LiveCounterManager should throw error for missing payload for counter
increment operation"); remove one of the two duplicate test methods to avoid
redundancy. Locate the duplicate by searching for the test method names in
LiveCounterManagerTest (references to getDefaultLiveCounterWithMockedDeps,
liveCounter.LiveCounterManager, ObjectOperation with action =
ObjectOperationAction.CounterInc and counterInc = null) and delete the redundant
test block so only a single assertion of applyOperation throwing an
AblyException with errorInfo.code == 92000 remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82ddb513-f3b3-4d57-8010-56b9b7297923
📒 Files selected for processing (28)
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/JsonSerialization.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/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.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 (2)
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.kt
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSerializationTest.kt
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt
Show resolved
Hide resolved
…l v6 - Update `ObjectOperation` fields to protocol v6 names (`mapCreate`, `mapSet`, `mapRemove`, `counterCreate`, `counterInc`). Internally and externally - Remove previous field names on `ObjectOperation`
…l v6 - Expose typed fields on public `ObjectData` (`boolean`, `bytes`, `number`, `string`, `json`) - Remove previous field names on `ObjectData`
- Added to `derivedFrom` field `MapCreateWithObjectId` and `CounterCreateWithObjectId` for client-initiated operations. - Updated `LiveMapManager` and `LiveCounterManager` to handle derived operations seamlessly. - Adjusted size calculation logic and validation functions to account for effective operations. - Enhanced test cases to cover derived operations.
ttypic
force-pushed
the
AIT-208/protocol-v6
branch
from
0a9ee7a to
195ebff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt (3)
605-610:⚠️ Potential issue | 🔴 CriticalMigrate legacy
counterOptocounterIncfor consistency.Same issue as above — this test uses the removed
counterOpfield instead ofcounterInc.Suggested fix
val msg = ObjectMessage( operation = ObjectOperation( action = ObjectOperationAction.CounterInc, objectId = "counter:test@1", - counterOp = ObjectsCounterOp(amount = 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/objects/ObjectsManagerTest.kt` around lines 605 - 610, The test constructs an ObjectMessage using ObjectOperation with the removed field counterOp; update the construction to use the new counterInc field instead (i.e., replace counterOp = ObjectsCounterOp(...) with counterInc = ObjectsCounterOp(...)) so ObjectOperation with action ObjectOperationAction.CounterInc and objectId "counter:test@1" uses the current schema; ensure any other tests in ObjectsManagerTest referencing counterOp are similarly migrated to counterInc.
642-648:⚠️ Potential issue | 🔴 CriticalMigrate legacy
counterOptocounterIncfor consistency.Same issue — this test uses the removed
counterOpfield.Suggested fix
val msg = ObjectMessage( operation = ObjectOperation( action = ObjectOperationAction.CounterInc, objectId = "counter:test@1", - counterOp = ObjectsCounterOp(amount = 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/objects/ObjectsManagerTest.kt` around lines 642 - 648, The test constructs an ObjectMessage using the removed legacy field counterOp; update the ObjectOperation initialization to use counterInc instead (e.g., replace counterOp = ObjectsCounterOp(amount = 5.0) with counterInc = ObjectsCounterOp(amount = 5.0)) so the ObjectMessage/ObjectOperation creation uses the current API (referencing ObjectMessage, ObjectOperation, ObjectOperationAction.CounterInc, and ObjectsCounterOp).
556-564:⚠️ Potential issue | 🔴 CriticalMigrate legacy
counterOptocounterIncfor consistency.This test still uses the legacy
counterOp = ObjectsCounterOp(amount = 5.0)field, which contradicts the PR objective to remove previous field names. TheObjectOperationdata class no longer definescounterOp, so this code will fail to compile.Suggested fix
val msg = ObjectMessage( operation = ObjectOperation( action = ObjectOperationAction.CounterInc, objectId = "counter:test@1", - counterOp = ObjectsCounterOp(amount = 5.0) + counterInc = CounterInc(number = 5.0) ), serial = "ser-01", siteCode = "site1" )🤖 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/objects/ObjectsManagerTest.kt` around lines 556 - 564, The test constructs ObjectMessage/ObjectOperation using the now-removed legacy field; update the operation to use the new counterInc field instead of counterOp: in the ObjectOperation instance (used when action = ObjectOperationAction.CounterInc) replace counterOp = ObjectsCounterOp(amount = 5.0) with counterInc = ObjectsCounterOp(amount = 5.0) so the ObjectMessage/ObjectOperation creation compiles against the updated data class.
♻️ Duplicate comments (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt (1)
197-201:⚠️ Potential issue | 🟡 MinorMatch the
CounterCreateaction with acounterCreatepayload.This fixture uses
action = ObjectOperationAction.CounterCreatebut populatescounterIncinstead ofcounterCreate. This creates an invalid operation shape that may cause test failures if action/payload validation is enforced.Suggested fix
operation = ObjectOperation( action = ObjectOperationAction.CounterCreate, objectId = "counter:testObject@1", - counterInc = CounterInc(number = 5.0) + counterCreate = CounterCreate(count = 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/objects/ObjectsManagerTest.kt` around lines 197 - 201, The ObjectOperation fixture uses action = ObjectOperationAction.CounterCreate but supplies counterInc instead of the matching counterCreate payload; update the ObjectOperation instance (in ObjectsManagerTest) to replace the CounterInc(...) field with a CounterCreate(...) payload (or add a counterCreate property) so the action and payload shape match (reference symbols: ObjectOperation, ObjectOperationAction.CounterCreate, CounterInc, CounterCreate).
🧹 Nitpick comments (4)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt (2)
244-244: Same import inconsistency as noted above.
MapRemoveis imported at line 7, so the fully qualified name is redundant.♻️ Suggested fix
- mapRemove = io.ably.lib.objects.MapRemove(key = "key1") + mapRemove = MapRemove(key = "key1")🤖 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` at line 244, The code uses a fully-qualified type name when assigning mapRemove (io.ably.lib.objects.MapRemove) despite MapRemove already being imported; replace the fully-qualified reference with the simple imported type name (MapRemove) in the assignment to mapRemove so the import is used consistently and the redundant qualifier is removed.
153-153: Consider using imported names consistently.
MapSetis already imported at line 6, so the fully qualified name is redundant. Similarly,ObjectDatacould be added to the imports for consistency with howMapCreateis used elsewhere in this file.♻️ Suggested refactor for consistent import usage
Add
ObjectDatato imports:import io.ably.lib.objects.MapCreate import io.ably.lib.objects.MapSet import io.ably.lib.objects.MapRemove +import io.ably.lib.objects.ObjectDataThen simplify the constructions (example for line 153):
- mapSet = io.ably.lib.objects.MapSet(key = "key1", value = io.ably.lib.objects.ObjectData(string = "value1")) + mapSet = MapSet(key = "key1", value = ObjectData(string = "value1"))Apply similar changes to lines 178, 201, and 222.
🤖 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` at line 153, The test uses fully-qualified types instead of the imported ones; add ObjectData to the file imports (alongside the already-imported MapSet) and replace fully-qualified usages like io.ably.lib.objects.MapSet(...) and io.ably.lib.objects.ObjectData(...) with the simple MapSet(...) and ObjectData(...) constructors (apply the same change for the other occurrences referenced: the constructions currently at lines corresponding to mapSet and the instances at the later usages shown in the diff, e.g., the spots analogous to lines 178, 201, and 222).liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
4-10: Redundant explicit imports alongside wildcard import.Lines 5-10 explicitly import types that are already covered by the wildcard import on line 4 (
io.ably.lib.objects.*). Consider removing either the wildcard or the explicit imports for consistency.🤖 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 4 - 10, The file currently has a wildcard import io.ably.lib.objects.* plus explicit imports for types like CounterCreate, CounterCreateWithObjectId, CounterInc, MapCreate, MapCreateWithObjectId, and MapSet; remove the redundancy by deleting either the wildcard import or the explicit type imports so each type is imported only once (e.g., keep the wildcard and remove the explicit imports for CounterCreate, CounterCreateWithObjectId, CounterInc, MapCreate, MapCreateWithObjectId, MapSet, or conversely remove the wildcard and retain the explicit imports).liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt (1)
125-142: Unused variablejsonMapCreate.
jsonMapCreateis created at line 128 but never used—jsonObjectOperationusesMapSetinstead. Consider removing the dead variable for clarity.🧹 Suggested cleanup
internal fun dummyObjectMessageWithJsonObjectData(): ObjectMessage { val jsonObjectMapEntry = dummyObjectsMapEntry.copy(data = dummyJsonObjectValue) val jsonObjectMap = dummyObjectsMap.copy(entries = mapOf("dummy-key" to jsonObjectMapEntry)) - val jsonMapCreate = dummyMapCreate.copy(entries = mapOf("dummy-key" to jsonObjectMapEntry)) val jsonObjectOperation = dummyObjectOperation.copy( action = ObjectOperationAction.MapSet, mapCreate = null, mapSet = MapSet(key = "dummy-key", value = dummyJsonObjectValue) )🤖 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 125 - 142, The function dummyObjectMessageWithJsonObjectData() declares an unused local jsonMapCreate (assigned from dummyMapCreate.copy...) which is dead code; remove the jsonMapCreate declaration to clean up the function, or if intended to be used, replace the MapSet construction in jsonObjectOperation with the correct reference to jsonMapCreate. Ensure changes only affect dummyObjectMessageWithJsonObjectData and that jsonObjectOperation, jsonObjectMapEntry, jsonObjectMap, jsonObjectState and the returned dummyObjectMessage remain consistent.
🤖 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/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.kt`:
- Line 21: The emptyBytesData fixture is double-encoding empty bytes by calling
Base64.getEncoder().encodeToString(ByteArray(0)); replace that expression so the
ObjectData.bytes field contains the correct base64 representation for empty
content (an empty string) rather than encoding twice—i.e. set emptyBytesData =
ObjectData(bytes = "") (referencing the emptyBytesData variable and ObjectData
constructor).
- Around line 17-18: bytesData and emptyBytesData are double Base64-encoding the
payload by calling Base64.getEncoder().encodeToString(...toByteArray()) on a
string that's already Base64; ObjectData.bytes expects a Base64-encoded string,
so remove the extra encoding and assign the original Base64 literal
("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9") directly to bytes
in the ObjectData constructors for bytesData and emptyBytesData.
---
Outside diff comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt`:
- Around line 605-610: The test constructs an ObjectMessage using
ObjectOperation with the removed field counterOp; update the construction to use
the new counterInc field instead (i.e., replace counterOp =
ObjectsCounterOp(...) with counterInc = ObjectsCounterOp(...)) so
ObjectOperation with action ObjectOperationAction.CounterInc and objectId
"counter:test@1" uses the current schema; ensure any other tests in
ObjectsManagerTest referencing counterOp are similarly migrated to counterInc.
- Around line 642-648: The test constructs an ObjectMessage using the removed
legacy field counterOp; update the ObjectOperation initialization to use
counterInc instead (e.g., replace counterOp = ObjectsCounterOp(amount = 5.0)
with counterInc = ObjectsCounterOp(amount = 5.0)) so the
ObjectMessage/ObjectOperation creation uses the current API (referencing
ObjectMessage, ObjectOperation, ObjectOperationAction.CounterInc, and
ObjectsCounterOp).
- Around line 556-564: The test constructs ObjectMessage/ObjectOperation using
the now-removed legacy field; update the operation to use the new counterInc
field instead of counterOp: in the ObjectOperation instance (used when action =
ObjectOperationAction.CounterInc) replace counterOp = ObjectsCounterOp(amount =
5.0) with counterInc = ObjectsCounterOp(amount = 5.0) so the
ObjectMessage/ObjectOperation creation compiles against the updated data class.
---
Duplicate comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt`:
- Around line 197-201: The ObjectOperation fixture uses action =
ObjectOperationAction.CounterCreate but supplies counterInc instead of the
matching counterCreate payload; update the ObjectOperation instance (in
ObjectsManagerTest) to replace the CounterInc(...) field with a
CounterCreate(...) payload (or add a counterCreate property) so the action and
payload shape match (reference symbols: ObjectOperation,
ObjectOperationAction.CounterCreate, CounterInc, CounterCreate).
---
Nitpick comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/fixtures/ObjectMessageFixtures.kt`:
- Around line 125-142: The function dummyObjectMessageWithJsonObjectData()
declares an unused local jsonMapCreate (assigned from dummyMapCreate.copy...)
which is dead code; remove the jsonMapCreate declaration to clean up the
function, or if intended to be used, replace the MapSet construction in
jsonObjectOperation with the correct reference to jsonMapCreate. Ensure changes
only affect dummyObjectMessageWithJsonObjectData and that jsonObjectOperation,
jsonObjectMapEntry, jsonObjectMap, jsonObjectState and the returned
dummyObjectMessage remain consistent.
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt`:
- Around line 4-10: The file currently has a wildcard import
io.ably.lib.objects.* plus explicit imports for types like CounterCreate,
CounterCreateWithObjectId, CounterInc, MapCreate, MapCreateWithObjectId, and
MapSet; remove the redundancy by deleting either the wildcard import or the
explicit type imports so each type is imported only once (e.g., keep the
wildcard and remove the explicit imports for CounterCreate,
CounterCreateWithObjectId, CounterInc, MapCreate, MapCreateWithObjectId, MapSet,
or conversely remove the wildcard and retain the explicit imports).
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/DefaultLiveMapTest.kt`:
- Line 244: The code uses a fully-qualified type name when assigning mapRemove
(io.ably.lib.objects.MapRemove) despite MapRemove already being imported;
replace the fully-qualified reference with the simple imported type name
(MapRemove) in the assignment to mapRemove so the import is used consistently
and the redundant qualifier is removed.
- Line 153: The test uses fully-qualified types instead of the imported ones;
add ObjectData to the file imports (alongside the already-imported MapSet) and
replace fully-qualified usages like io.ably.lib.objects.MapSet(...) and
io.ably.lib.objects.ObjectData(...) with the simple MapSet(...) and
ObjectData(...) constructors (apply the same change for the other occurrences
referenced: the constructions currently at lines corresponding to mapSet and the
instances at the later usages shown in the diff, e.g., the spots analogous to
lines 178, 201, and 222).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7e782c6-4ead-4834-8e8f-3a8cb67a727e
📒 Files selected for processing (28)
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/JsonSerialization.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/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/RestObjects.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.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 (2)
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/src/main/java/io/ably/lib/transport/Defaults.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livecounter/DefaultLiveCounterTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterManager.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt
Comment on lines
17
to
+18
| internal val bytesData = ObjectData( | ||
| value = ObjectValue.Binary(Binary("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9".toByteArray()))) | ||
| bytes = Base64.getEncoder().encodeToString("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9".toByteArray())) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how bytes field is serialized in MsgpackSerialization
rg -n -A5 -B2 '"bytes"' --type=kt liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/Repository: ably/ably-java
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Search for ObjectData class definition
find . -name "*.kt" -type f | xargs grep -l "class ObjectData" | head -5Repository: ably/ably-java
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Search for bytes field usage in ObjectData context
rg -n "bytes\s*=" -A2 -B2 liveobjects/src/main/kotlin/io/ably/lib/objects/ | head -100Repository: ably/ably-java
Length of output: 3973
🏁 Script executed:
#!/bin/bash
# Check the DataFixtures file to understand the full context
cat -n liveobjects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/DataFixtures.ktRepository: ably/ably-java
Length of output: 3587
🏁 Script executed:
#!/bin/bash
# Find and display ObjectData class definition
cat liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt | grep -A 20 "data class ObjectData"Repository: ably/ably-java
Length of output: 603
🏁 Script executed:
#!/bin/bash
# Search for tests that use bytesData fixture to understand expected behavior
rg -n "bytesData" liveobjects/src/test/kotlin/ -A 3 -B 1 | head -50Repository: ably/ably-java
Length of output: 1523
🏁 Script executed:
#!/bin/bash
# Search for tests that use bytesData and check what they expect
rg -n "bytesData" liveobjects/src/test/kotlin/ -B 5 -A 10 | grep -E "(test|assert|expect|decode|Base64)" | head -40Repository: ably/ably-java
Length of output: 4422
🏁 Script executed:
#!/bin/bash
# Decode the Base64 string to verify what it represents
python3 << 'EOF'
import base64
encoded = "eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9"
decoded = base64.b64decode(encoded).decode('utf-8')
print(f"Decoded JSON: {decoded}")
# Show what happens with double encoding
double_encoded = base64.b64encode(encoded.encode('utf-8')).decode('utf-8')
print(f"Double-encoded (current code): {double_encoded}")
# Show what correct single encoding should be
single_encoded = base64.b64encode(decoded.encode('utf-8')).decode('utf-8')
print(f"Correct single-encoded: {single_encoded}")
EOFRepository: ably/ably-java
Length of output: 303
Double Base64 encoding creates incorrect test data on lines 17-18 and 21.
The string "eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9" is already the Base64 encoding of the JSON {"productId": "001", "productName": "car"}. Converting it to bytes and Base64-encoding again produces double-encoding: ZXlKd2NtOWtkV04wU1dRaU9pQWlNREF4SWl3Z0luQnliMlIxWTNST1lXMWxJam9nSW1OaGNpSjk=. Since ObjectData.bytes expects a Base64-encoded string, pass the original Base64 string directly. The same issue affects emptyBytesData on line 21.
Suggested fix
/** Test fixture for binary data containing encoded JSON */
- internal val bytesData = ObjectData(
- bytes = Base64.getEncoder().encodeToString("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9".toByteArray()))
+ internal val bytesData = ObjectData(
+ bytes = "eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9")
/** Test fixture for empty binary data (zero-length byte array) */
- internal val emptyBytesData = ObjectData(bytes = Base64.getEncoder().encodeToString(ByteArray(0)))
+ internal val emptyBytesData = ObjectData(bytes = "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| internal val bytesData = ObjectData( | |
| value = ObjectValue.Binary(Binary("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9".toByteArray()))) | |
| bytes = Base64.getEncoder().encodeToString("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9".toByteArray())) | |
| internal val bytesData = ObjectData( | |
| bytes = "eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9") |
🤖 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/integration/helpers/fixtures/DataFixtures.kt`
around lines 17 - 18, bytesData and emptyBytesData are double Base64-encoding
the payload by calling Base64.getEncoder().encodeToString(...toByteArray()) on a
string that's already Base64; ObjectData.bytes expects a Base64-encoded string,
so remove the extra encoding and assign the original Base64 literal
("eyJwcm9kdWN0SWQiOiAiMDAxIiwgInByb2R1Y3ROYW1lIjogImNhciJ9") directly to bytes
in the ObjectData constructors for bytesData and emptyBytesData.
|
|
||
| /** Test fixture for empty binary data (zero-length byte array) */ | ||
| internal val emptyBytesData = ObjectData(value = ObjectValue.Binary(Binary(ByteArray(0)))) | ||
| internal val emptyBytesData = ObjectData(bytes = Base64.getEncoder().encodeToString(ByteArray(0))) |
There was a problem hiding this comment.
Same double-encoding issue for empty bytes fixture.
If the above concern is valid, this line has the same issue.
Suggested fix
/** Test fixture for empty binary data (zero-length byte array) */
- internal val emptyBytesData = ObjectData(bytes = Base64.getEncoder().encodeToString(ByteArray(0)))
+ internal val emptyBytesData = ObjectData(bytes = "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| internal val emptyBytesData = ObjectData(bytes = Base64.getEncoder().encodeToString(ByteArray(0))) | |
| internal val emptyBytesData = ObjectData(bytes = "") |
🤖 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/integration/helpers/fixtures/DataFixtures.kt`
at line 21, The emptyBytesData fixture is double-encoding empty bytes by calling
Base64.getEncoder().encodeToString(ByteArray(0)); replace that expression so the
ObjectData.bytes field contains the correct base64 representation for empty
content (an empty string) rather than encoding twice—i.e. set emptyBytesData =
ObjectData(bytes = "") (referencing the emptyBytesData variable and ObjectData
constructor).
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
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 calculationSpec: ably/specification#426
Js: ably/ably-js#2159
Summary by CodeRabbit
New Features
Bug Fixes
Chores