[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195
Open
ttypic wants to merge 2 commits intoAIT-276/apply-on-ackfrom
Open
[AIT-208] feat: update LiveObjects messages fields to support protocol v6 #1195ttypic wants to merge 2 commits intoAIT-276/apply-on-ackfrom
ttypic wants to merge 2 commits intoAIT-276/apply-on-ackfrom
Conversation
WalkthroughThis PR updates the protocol version from "5" to "6" and refactors LiveObjects to replace generic map/counter wrappers with explicit, typed payloads (MapCreate/MapSet/MapRemove/CounterCreate/CounterInc and related variants), updating serialization, managers, creators, and tests accordingly. Changes
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
Comment on lines
+450
to
+452
| private fun MapCreate.size(): Int { | ||
| return entries.entries.sumOf { it.key.length + it.value.size() } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check the file structure and line counts
echo "=== File info ==="
wc -l liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Examine the specific lines mentioned
echo "=== Lines 450-452 (MapCreate.size) ==="
sed -n '450,452p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 457-459 (MapSet.size) ==="
sed -n '457,459p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 464-466 (MapRemove.size) ==="
sed -n '464,466p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 485-487 (MapCreateWithObjectId.size) ==="
sed -n '485,487p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
echo "=== Lines 492-494 (CounterCreateWithObjectId.size) ==="
sed -n '492,494p' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Check if byteSize is defined in the file
echo "=== Searching for byteSize definition ==="
rg -n 'fun.*byteSize|extension.*byteSize' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Check context around ObjectValue.String to see how byteSize is used
echo "=== ObjectValue.String references ==="
rg -n 'ObjectValue\.String' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -B2 -A2Repository: ably/ably-java
Length of output: 831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search entire codebase for byteSize
echo "=== Searching for byteSize definition in codebase ==="
rg -n 'byteSize|fun.*byteSize|extension.*byteSize' --max-count=20
# Check imports in the ObjectMessage.kt file
echo "=== Imports in ObjectMessage.kt ==="
head -30 liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt
# Search for how size() is calculated for ObjectValue
echo "=== ObjectValue.size() implementation ==="
rg -n 'sealed class ObjectValue|is ObjectValue\.String' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -B2 -A5 | head -40
# Look for all .size() method implementations in the file
echo "=== All size() methods in ObjectMessage.kt ==="
rg -n 'fun.*size\(\)' liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt -A2Repository: ably/ably-java
Length of output: 4683
Use UTF-8 byte length in payload size calculations.
String.length counts UTF-16 code units, not UTF-8 bytes. The byteSize extension already exists and is correctly used for ObjectValue.String. The following methods must be updated:
MapCreate.size()(line 450):it.key.length→it.key.byteSizeMapSet.size()(line 457):key.length→key.byteSizeMapRemove.size()(line 464):key.length→key.byteSizeMapCreateWithObjectId.size()(line 485):initialValue.length,nonce.length→byteSizeCounterCreateWithObjectId.size()(line 492):initialValue.length,nonce.length→byteSizeObjectMessage.size()(line 409):clientId?.length→clientId?.byteSize(also uses char count instead of bytes)
Multi-byte UTF-8 characters (e.g., "你" = 3 bytes, "😊" = 4 bytes) will be undercounted, risking client-side size checks passing while the server rejects the message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt` around
lines 450 - 452, The size calculations currently use UTF-16 code unit counts
(String.length) causing undercount for multi-byte UTF-8 chars; update the listed
size methods to use the existing byteSize extension instead: in MapCreate.size()
replace it.key.length with it.key.byteSize; in MapSet.size() replace key.length
with key.byteSize; in MapRemove.size() replace key.length with key.byteSize; in
MapCreateWithObjectId.size() and CounterCreateWithObjectId.size() replace
initialValue.length and nonce.length with initialValue.byteSize and
nonce.byteSize; and in ObjectMessage.size() replace clientId?.length with
clientId?.byteSize so all payload string sizes reflect UTF-8 byte length.
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
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`
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.
Comment on lines
825
to
+830
| if (objectId != null) fieldCount++ | ||
| value?.let { | ||
| fieldCount++ | ||
| } | ||
| if (string != null) fieldCount++ | ||
| if (number != null) fieldCount++ | ||
| if (boolean != null) fieldCount++ | ||
| if (bytes != null) fieldCount++ | ||
| if (json != null) fieldCount++ |
There was a problem hiding this comment.
Guard ObjectData against multi-typed payloads.
Line 825–830 and Line 870–905 currently allow multiple value variants in one ObjectData (string + number, etc.). That creates ambiguous payloads and non-canonical encoding.
🧩 Suggested one-of validation
+private fun ObjectData.validateSingleVariant() {
+ val variants = listOfNotNull(objectId, string, number, boolean, bytes, json)
+ require(variants.size <= 1) { "ObjectData must contain at most one value variant" }
+}
+
private fun ObjectData.writeMsgpack(packer: MessagePacker) {
+ validateSingleVariant()
var fieldCount = 0
...
} private fun readObjectData(unpacker: MessageUnpacker): ObjectData {
...
- return ObjectData(objectId = objectId, string = string, number = number, boolean = boolean, bytes = bytes, json = json)
+ val data = ObjectData(
+ objectId = objectId,
+ string = string,
+ number = number,
+ boolean = boolean,
+ bytes = bytes,
+ json = json
+ )
+ data.validateSingleVariant()
+ return data
}Also applies to: 839-864, 870-905
| 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ObjectOperationfields to protocol v6 names (mapCreate,mapSet,mapRemove,counterCreate,counterInc). Internally and externallyObjectData(boolean,bytes,number,string,json)ObjectOperationandObjectData*CreateWithObjectIdoperations for apply-on-ACK and message size calculationSpec: ably/specification#426
Js: ably/ably-js#2159
Summary by CodeRabbit
New Features
Chores