[AIT-1142] fix(liveobjects): spec-compliance fixes from the cross-SDK objects audit (inbound op handling, counter noop guards)#1228
Open
sacOO7 wants to merge 1 commit into
Conversation
…dit (inbound op handling, counter noop guards) Port of the ably-js objects deviations audit (ably/ably-js#2263) to ably-java. - canApplyOperation: log a warning and refuse to apply on empty serial/siteCode (RTLO4a3) instead of throwing; a throw aborted every sibling operation in the same ProtocolMessage batch. The caller's RTLC7b/RTLM15b skip log now only phrases the serial comparison when both values exist. - nil operation payloads (counterInc / mapSet / mapRemove absent): log a warning and skip only the offending operation instead of throwing (same batch-abort class), matching the existing unsupported-action gates (RTLC7d3 / RTLM15d4). - WireCounterInc.number / WireCounterCreate.count are now nullable: the spec defines the absent case (RTLC9h / RTLC16d), but msgpack decoding threw on a missing field (aborting the whole ProtocolMessage decode) while JSON silently defaulted to 0.0 and emitted a spurious event - transport-inconsistent, both non-compliant. Msgpack now round-trips absence; the public CounterInc/ CounterCreate accessors are @nullable with documented semantics. - COUNTER_INC without number is a noop (RTLC9h); COUNTER_CREATE without count is a noop that still sets createOperationIsMerged first (RTLC16b, so RTLC8b duplicate-create dedup engages). - tests: RTLO4b4c1 noop-no-trigger un-gated (was RUN_DEVIATIONS-gated with the deviation documented as "to be fixed in both SDKs together" - both halves now fixed); the corresponding deviations.md section removed.
sacOO7
changed the title
WalkthroughLiveObjects counter payloads now accept missing numeric fields, serialization preserves nulls, and managers skip malformed operations or apply no-op updates. Related subscription coverage runs unconditionally. UTS documentation paths and command examples were also updated. ChangesLiveObjects payload handling
Documentation path updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@uts/README.md`:
- Around line 166-167: Update the parenthetical location text accompanying the
proxy.md link in the README so it matches the canonical uts/docs/proxy.md path,
or remove the outdated parenthetical entirely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af7c0c4d-abbc-4f51-a70b-cf74185cad86
📒 Files selected for processing (12)
.claude/skills/uts-to-kotlin/SKILL.mdlib/src/main/java/io/ably/lib/liveobjects/message/CounterCreate.javalib/src/main/java/io/ably/lib/liveobjects/message/CounterInc.javaliveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/DefaultObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/WireObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/serialization/MsgpackSerialization.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.ktuts/README.mduts/src/test/kotlin/io/ably/lib/uts/deviations.mduts/src/test/kotlin/io/ably/lib/uts/unit/liveobjects/LiveObjectSubscribeTest.kt
💤 Files with no reviewable changes (2)
- uts/src/test/kotlin/io/ably/lib/uts/deviations.md
- uts/src/test/kotlin/io/ably/lib/uts/unit/liveobjects/LiveObjectSubscribeTest.kt
Comment on lines
+166
to
167
| > [`docs/proxy.md`](https://github.com/ably/specification/blob/main/uts/docs/proxy.md) | ||
| > (in the spec repo under `uts/realtime/integration/helpers/`). It defines the proxy's control API, rule format, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale proxy-document location.
The link now points to uts/docs/proxy.md, but the following text still says the file is under uts/realtime/integration/helpers/. Update or remove that parenthetical so the documented location matches the canonical path.
Proposed fix
> [`docs/proxy.md`](https://github.com/ably/specification/blob/main/uts/docs/proxy.md)
-> (in the spec repo under `uts/realtime/integration/helpers/`). It defines the proxy's control API, rule format,
+> (in the spec repo under `uts/docs/`). It defines the proxy's control API, rule format,📝 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
| > [`docs/proxy.md`](https://github.com/ably/specification/blob/main/uts/docs/proxy.md) | |
| > (in the spec repo under `uts/realtime/integration/helpers/`). It defines the proxy's control API, rule format, | |
| > [`docs/proxy.md`](https://github.com/ably/specification/blob/main/uts/docs/proxy.md) | |
| > (in the spec repo under `uts/docs/`). It defines the proxy's control API, rule format, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@uts/README.md` around lines 166 - 167, Update the parenthetical location text
accompanying the proxy.md link in the README so it matches the canonical
uts/docs/proxy.md path, or remove the outdated parenthetical entirely.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR brings liveobjects in ably-java into closer alignment with the reconciled LiveObjects spec by making inbound operation handling more resilient (skip malformed operations without aborting a whole batch) and by correctly treating missing counterInc.number / counterCreate.count as spec-defined no-ops. It also ungates the previously-deviating UTS test and updates related documentation links.
Changes:
- Make counter wire fields nullable and ensure both MsgPack and JSON transports preserve “field absent” vs “0” semantics; apply-path treats absence as a no-op (no listener event).
- Avoid throwing on malformed/partial inbound operations (invalid serial/siteCode, missing op payloads) to prevent sibling-operation loss within a batch.
- Update/ungate UTS tests and clean up related deviation/docs references.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uts/src/test/kotlin/io/ably/lib/uts/unit/liveobjects/LiveObjectSubscribeTest.kt | Removes deviation gate so the noop-listener behavior is asserted by default. |
| uts/src/test/kotlin/io/ably/lib/uts/deviations.md | Removes the shared-gap deviation entry tied to missing counter fields. |
| uts/README.md | Updates spec doc links to the relocated proxy document. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.kt | Warn-and-skip for missing MapSet/MapRemove payloads instead of throwing. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.kt | No-op handling for missing counter inc/create numeric fields; warn-and-skip for missing inc payload. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt | canApplyOperation now warns and returns false for invalid serial/siteCode rather than throwing. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/serialization/MsgpackSerialization.kt | MsgPack codec round-trips absent count/number without decode-time failure. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/WireObjectMessage.kt | Makes WireCounterInc.number and WireCounterCreate.count nullable. |
| liveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/DefaultObjectMessage.kt | Propagates nullable counter fields to the public message wrappers. |
| lib/src/main/java/io/ably/lib/liveobjects/message/CounterInc.java | Public API now returns nullable number with documented noop semantics. |
| lib/src/main/java/io/ably/lib/liveobjects/message/CounterCreate.java | Public API now returns nullable count with documented noop semantics. |
| .claude/skills/uts-to-kotlin/SKILL.md | Updates example paths to use a portable placeholder for spec repo clones. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > There is also a fifth, *referenced* spec: | ||
| > [`realtime/integration/helpers/proxy.md`](https://github.com/ably/specification/blob/main/uts/realtime/integration/helpers/proxy.md) | ||
| > [`docs/proxy.md`](https://github.com/ably/specification/blob/main/uts/docs/proxy.md) | ||
| > (in the spec repo under `uts/realtime/integration/helpers/`). It defines the proxy's control API, rule format, |
Comment on lines
81
to
83
| --- | ||
|
|
||
| # 2) Shared gap — open in BOTH SDKs (objects) | ||
|
|
||
| *ably-js has the same documented deviation — the spec is ahead of both implementations. Optional joint fix.* | ||
|
|
||
| ## RTLC9h / RTLC16 / RTLO4b4c1 — missing-field counter ops are not treated as no-ops | ||
|
|
||
| **Spec points:** RTLC9h, RTLC16d, RTLO4b4c1 | ||
| **What the spec requires:** A `COUNTER_INC` whose `counterInc.number` is **absent** produces a no-op | ||
| `LiveObjectUpdate` (`update.noop == true`, RTLC9h), so a subscribed listener must NOT be invoked | ||
| (RTLO4b4c1). For the RTLO4b4c1 stimulus (`01` real inc → `02` missing-number noop → `03` real inc), the | ||
| listener fires exactly twice (`updates == 2`). | ||
| **What the SDK does:** `WireCounterInc.number` is a **non-nullable `Double`** | ||
| (`liveobjects/.../message/WireObjectMessage.kt`), so an absent `number` deserialises to `0.0` — | ||
| indistinguishable from `number: 0`. `LiveCounterManager.applyCounterInc` unconditionally returns | ||
| `ObjectUpdate.CounterUpdate(amount, message)` (RTLC9g) and calls `notifyUpdated`; there is **no** | ||
| missing-number/RTLC9h noop branch on the operation path (the only counter noop, RTLC14b via | ||
| `calculateUpdateFromDataDiff`, exists on the sync/`replaceData` path, not the op path). So the | ||
| missing-number `02` op fires an amount-0 event and the listener is invoked a 3rd time (`updates == 3`). | ||
| **Same family — RTLC16 (COUNTER_CREATE with absent `count`):** RTLC16d requires the create-op merge to | ||
| return a noop when `counterCreate.count` does not exist; `LiveCounterManager.mergeInitialDataFromCreateOperation` | ||
| returns a normal amount-0 `CounterUpdate` instead (`?: 0.0`). Same root cause: the wire types don't track | ||
| field presence, so an absent count is indistinguishable from an explicit `0` (which per RTLC16c correctly | ||
| yields an amount-0 update, NOT a noop). | ||
| **ably-js status:** same deviations (documented in its `deviations.md`) — `_applyCounterInc` applies | ||
| `op.number` unconditionally, so a missing number becomes `NaN` and an event fires; and its create-op merge | ||
| does the identical `counterCreate?.count ?? 0`, applying an amount-0 update where RTLC16d wants a noop. | ||
| **Workaround in tests:** `RTLO4b4c1 - noop update does not trigger listener` is gated behind | ||
| `RUN_DEVIATIONS` (early `return@runTest`), keeping the spec-correct `assertEquals(2, updates.size)`. | ||
| Repro: `RUN_DEVIATIONS=1 ./gradlew :uts:runUtsUnitTests --tests "*LiveObjectSubscribeTest"`. | ||
| **Root cause / fix (SDK):** make `WireCounterInc.number` (and `WireCounterCreate.count`) nullable (or | ||
| track field presence) and add the missing-field → noop branches (RTLC9h in `applyCounterInc`, RTLC16d in | ||
| `mergeInitialDataFromCreateOperation`) so no event is emitted. To be fixed in both SDKs together. | ||
| **Tests affected (LiveObjectSubscribeTest.kt):** | ||
| - `RTLO4b4c1 - noop update does not trigger listener` (RTLO4b4c1/noop-no-trigger-0) — env-gated. | ||
|
|
||
| > Note: the internal `LiveObjectUpdate.noop` diff flag is also not exposed on the public | ||
| > `InstanceSubscriptionEvent` (only `getObject()`/`getMessage()`, mapping §8); the spec frames noop | ||
| > suppression as *the listener not firing*, which is the observable the env-gated test asserts. | ||
|
|
||
| --- | ||
|
|
||
| # 3) Expected — typed-SDK / language adaptations (objects) — NOT bugs, no action |
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.
Summary
Ports the outcome of the ably-js LiveObjects spec-compliance audit (ably/ably-js#2263) to ably-java. The same bug classes were checked against
liveobjects/src/main/kotlinwith the reconciled objects spec (objects-features.md+uts/objects) as the reference. Two of the four ably-js bug classes were present here — one of them in a worse, transport-dependent form — and are fixed in this PR.Bug 1: one malformed inbound operation discarded its siblings
The batch loop in
ObjectsManager.applyObjectMessageshas no per-operation catch, so any throw from the apply path aborts every remaining operation in the sameProtocolMessage(silent data loss within the batch). Two wire-triggerable throws existed:canApplyOperationthrew for an emptyserial/siteCode. The spec (RTLO4a3) mandates log a warning and do not apply. It now warns and returnsfalse; the caller's RTLC7b/RTLM15b "op serial ≤ site serial" skip log is guarded so it doesn't fire with null values for this case.counterInc/mapSet/mapRemoveabsent) threwobjectError. They now log a warning and skip only the offending operation — matching the existing unsupported-action gates (RTLC7d3/RTLM15d4), which were already correct.Bug 2: a missing
number/countwas unrepresentable — and each transport failed differentlyWireCounterInc.numberandWireCounterCreate.countwere non-nullable, but the spec explicitly defines the absent case (RTLC9h/RTLC16d). The result was transport-inconsistent:numberreadCounterIncthrew at decode time, aborting the decode of the wholeProtocolMessage0.0→ applied a zero increment and emitted a spurious update eventFixes:
applyCounterInc: missingnumberreturns the noop update — no event (RTLC9h).mergeInitialDataFromCreateOperation: missingcountreturns the noop update, butcreateOperationIsMergedis set before the noop return (RTLC16b is unconditional, so RTLC8b's duplicate-create dedup still engages).CounterInc.getNumber()/CounterCreate.getCount()are now@Nullable, with the noop semantics documented in the Javadoc.Verified as NOT present in ably-java
generateNonce()already produces exactly 16 chars from a 62-symbol alphanumeric set (~95 bits) — RTLCV4d compliant.validateObjectIdmismatch, MAP_SET invalid-value (92000), MAP_CREATE semantics mismatch, and create-op validation during sync.Test changes
LiveObjectSubscribeTest.RTLO4b4c1 - noop update does not trigger listenerwas gated behindRUN_DEVIATIONS, with the deviation recorded indeviations.mdas "to be fixed in both SDKs together". The ably-js half landed in [AIT-1141] fix(liveobjects): spec-compliance fixes from the objects deviations audit (nonce length, inbound op handling, counter noop guards) ably-js#2263; this PR is the other half — the gate is removed and the test runs the spec-verbatimcounterInc: {}noop stimulus.deviations.mdsection ("Shared gap — open in BOTH SDKs") is removed; it was its only entry.Testing
:liveobjects:test+:uts:runUtsUnitTests: 189 tests, 0 failures (including the un-gated RTLO4b4c1).:liveobjects:compileKotlinand:java:compileJavaclean (public interface nullability change verified against all consumers).Follow-up
uts/objects/unit/internal_live_counter.md/internal_live_map.mdare not yet derived in ably-java; the spec tests that pin these behaviors directly (RTLO4a/warn-invalid-serial-0,RTLC9/counter-inc-missing-number-0, the RTLC16d noop) will gain first-class regression coverage when those files are derived. The fixes were validated line-by-line against those spec test bodies in the meantime.Related
Also included: two pre-existing one-line doc-link corrections (
uts/README.md,.claude/skills/uts-to-kotlin/SKILL.md) pointing at the relocateduts/docs/proxy.md.Summary by CodeRabbit
Bug Fixes
Documentation