[AIT-322] Support the protocol v6 ObjectMessage structure#114
[AIT-322] Support the protocol v6 ObjectMessage structure#114lawrence-forooghian wants to merge 1 commit intoAIT-324-apply-on-ACKfrom
ObjectMessage structure#114Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request restructures the object operation protocol and wire message format by replacing granular operation fields (mapOp, counterOp, map, counter, nonce, initialValue) with semantic operation types (MapSet, MapCreate, MapCreateWithObjectId, CounterInc, CounterCreate, etc.), updates internal implementations to use the new types, and refreshes test fixtures accordingly. Swift package dependencies are also updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
99ec084 to
274e9a3
Compare
274e9a3 to
4ebc381
Compare
ObjectMessage structure
ff2ec7f to
54b2d88
Compare
54b2d88 to
5ce9d74
Compare
5ce9d74 to
8225d23
Compare
8225d23 to
11afc2e
Compare
11afc2e to
99de9b3
Compare
99de9b3 to
64a017a
Compare
64a017a to
9d11807
Compare
|
There seem to be some intermittent failures in the smoke test; need to investigate |
a77505b to
cbb49fd
Compare
7689879 to
dadaade
Compare
dadaade to
ee5b2f9
Compare
ee5b2f9 to
dff72b2
Compare
dff72b2 to
ffc512f
Compare
ffc512f to
02bfedb
Compare
02bfedb to
3a18180
Compare
3a18180 to
7ec81af
Compare
ObjectMessage structureObjectMessage structure
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
Package.swift (1)
24-24: Consider using full commit hash for consistency and stability.The
ably-cocoadependency uses a short 7-character commit hash whileably-cocoa-plugin-support(line 29) uses a full 40-character hash. Short hashes can become ambiguous as repositories grow. Additionally, the Package.resolved shows this was resolved as a branch named "154b6cd" rather than a revision—verify this is the intended behavior.Suggested fix for consistency
.package( url: "https://github.com/ably/ably-cocoa.git", - revision: "154b6cd", + revision: "154b6cd796185a04985c06ed85aa12c7b0462755", ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Package.swift` at line 24, The ably-cocoa dependency in Package.swift currently pins revision: "154b6cd" (a short 7-char hash); replace it with the full 40-character commit SHA for consistency with ably-cocoa-plugin-support and to avoid ambiguity, and ensure the package dependency is specified as a revision (not a branch name) so Package.resolved records the exact commit; update the revision value for the ably-cocoa entry to the repository's full commit hash and re-resolve packages to verify it records a revision.Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
390-392: UseDoublefor counter-inc test factories to match runtime semantics.These helpers are currently Int-only, which reduces test coverage for fractional increments.
Suggested tweak
- static func counterInc(number: Int = 10) -> WireCounterInc { + static func counterInc(number: Double = 10) -> WireCounterInc { WireCounterInc(number: NSNumber(value: number)) } static func counterIncOperationMessage( objectId: String = "counter:test@123", - number: Int = 10, + number: Double = 10, serial: String = "ts1", siteCode: String = "site1", ) -> InboundObjectMessage {Also applies to: 635-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift` around lines 390 - 392, Update the counter-inc test factory(s) to use Double to match runtime semantics: change the signature of functions like counterInc to accept a Double (e.g., number: Double = 10.0) and construct the WireCounterInc using NSNumber(value: number) with a Double value; apply the same change to any other counter-inc helpers in this file (the other factory functions that create WireCounterInc instances) so tests cover fractional increments while still defaulting to a sensible double value.Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (1)
353-533: Place access control on extension declarations for new wire/debug extensions.These new extensions should carry the ACL on the extension (
internal extension ...) and omit per-memberinternalmodifiers.As per coding guidelines, `When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)`.Example pattern to apply
-extension WireMapSet: WireObjectCodable { - internal enum WireKey: String { +internal extension WireMapSet: WireObjectCodable { + enum WireKey: String { case key case value } - internal init(wireObject: [String: WireValue]) throws(ARTErrorInfo) { + init(wireObject: [String: WireValue]) throws(ARTErrorInfo) { ... } - internal var toWireObject: [String: WireValue] { + var toWireObject: [String: WireValue] { ... } }Also applies to: 704-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift` around lines 353 - 533, The extensions for the new wire/debug types should have the access control on the extension declaration instead of on each member: change extension WireMapSet, WireMapRemove, WireMapCreate, WireCounterCreate, WireCounterInc, WireObjectDelete, WireMapCreateWithObjectId, and WireCounterCreateWithObjectId to be declared as internal extension ... and remove the per-member 'internal' modifiers from their nested enum WireKey, init(wireObject:), and toWireObject properties so members use the extension's access level.Sources/AblyLiveObjects/Protocol/ObjectMessage.swift (1)
589-640: Apply ACL on extension declarations in new debug extensions.Please move
internalto the extension declaration and drop member-levelinternalinside these newly added extensions.As per coding guidelines,
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AblyLiveObjects/Protocol/ObjectMessage.swift` around lines 589 - 640, The debug extensions declare members as "internal" instead of applying the access control on the extension; update each extension declaration for MapSet, MapCreate, MapCreateWithObjectId, and CounterCreateWithObjectId to include "internal" (e.g., "internal extension MapSet") and remove the "internal" modifier from the debugDescription property inside each extension so the access level is applied at the extension level rather than per-member.Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
663-666: Consider extracting the repeated delegate/pool sync snippet into a small helper.The same setup appears in both MAP_SET test groups; a helper would reduce drift and keep intent-focused test bodies.
Also applies to: 731-734
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift` around lines 663 - 666, Extract the repeated sync logic that updates the delegate's objects from the pool into a small helper function (e.g., syncDelegateWithPool(delegate:pool:expectedCreatedObjectID:)) and replace the duplicated blocks inside the MAP_SET test groups with calls to that helper; the helper should check for a non-nil expectedCreatedObjectID and, if present, perform delegate.objects[expectedCreatedObjectID] = pool.entries[expectedCreatedObjectID] so both occurrences (current use of delegate, pool, expectedCreatedObjectID) are centralized and the tests remain intent-focused.Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift (1)
215-236: Consider extendingWireObjectOperation.encodesAllFieldsto include all newly supported optional fields.You already cover sub-struct encoding separately, but adding
mapRemove,objectDelete, andmapCreateWithObjectId/counterCreateWithObjectIdat operation-level encoding would catch wiring regressions earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift` around lines 215 - 236, The test encodesAllFields in WireObjectMessageTests should be extended to include the newly supported optional operation-level fields so wiring regressions are caught: update the WireObjectOperation instance in encodesAllFields to set mapRemove, objectDelete, mapCreateWithObjectId and counterCreateWithObjectId (in addition to the existing mapCreate/mapSet/counterCreate/counterInc), then expand the expected wire dictionary used with `#expect`(wire == [...]) to include the corresponding keys ("mapRemove", "objectDelete", "mapCreateWithObjectId", "counterCreateWithObjectId") and their encoded shapes; keep using op.toWireObject to produce wire and mirror the exact nested structures used by the existing entries to validate encoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift`:
- Around line 12-18: The initializer DefaultInternalPlugin.init currently uses a
process-terminating precondition to check pluginAPI.usesLiveObjectsProtocolV6;
change this to a non-crashing failure path by making the initializer throw (or
failable) and validating pluginAPI.usesLiveObjectsProtocolV6 with a guard that
returns a descriptive error (or nil) instead of calling precondition, so the
host can reject the plugin cleanly; update callers to handle the thrown error
(or nil) and include a clear error message mentioning LiveObjects protocol v6
compatibility.
In `@Tests/AblyLiveObjectsTests/JS` Integration Tests/ObjectsHelper.swift:
- Around line 180-193: counterCreateOp currently omits the "counterCreate" field
when count is nil, causing wire fixtures to differ from REST which sends
counterCreate: {}; update the counterCreateOp function to always set
operation["counterCreate"] to an object (either ["count": .number(...)] when
count is provided or an empty object when count is nil) so the "operation"
payload shape matches the REST helper; locate the counterCreateOp function and
the construction of the operation dictionary (types WireValue /
Actions.counterCreate) and replace conditional omission with always-emitted
counterCreate object.
---
Nitpick comments:
In `@Package.swift`:
- Line 24: The ably-cocoa dependency in Package.swift currently pins revision:
"154b6cd" (a short 7-char hash); replace it with the full 40-character commit
SHA for consistency with ably-cocoa-plugin-support and to avoid ambiguity, and
ensure the package dependency is specified as a revision (not a branch name) so
Package.resolved records the exact commit; update the revision value for the
ably-cocoa entry to the repository's full commit hash and re-resolve packages to
verify it records a revision.
In `@Sources/AblyLiveObjects/Protocol/ObjectMessage.swift`:
- Around line 589-640: The debug extensions declare members as "internal"
instead of applying the access control on the extension; update each extension
declaration for MapSet, MapCreate, MapCreateWithObjectId, and
CounterCreateWithObjectId to include "internal" (e.g., "internal extension
MapSet") and remove the "internal" modifier from the debugDescription property
inside each extension so the access level is applied at the extension level
rather than per-member.
In `@Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift`:
- Around line 353-533: The extensions for the new wire/debug types should have
the access control on the extension declaration instead of on each member:
change extension WireMapSet, WireMapRemove, WireMapCreate, WireCounterCreate,
WireCounterInc, WireObjectDelete, WireMapCreateWithObjectId, and
WireCounterCreateWithObjectId to be declared as internal extension ... and
remove the per-member 'internal' modifiers from their nested enum WireKey,
init(wireObject:), and toWireObject properties so members use the extension's
access level.
In `@Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift`:
- Around line 390-392: Update the counter-inc test factory(s) to use Double to
match runtime semantics: change the signature of functions like counterInc to
accept a Double (e.g., number: Double = 10.0) and construct the WireCounterInc
using NSNumber(value: number) with a Double value; apply the same change to any
other counter-inc helpers in this file (the other factory functions that create
WireCounterInc instances) so tests cover fractional increments while still
defaulting to a sensible double value.
In `@Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift`:
- Around line 663-666: Extract the repeated sync logic that updates the
delegate's objects from the pool into a small helper function (e.g.,
syncDelegateWithPool(delegate:pool:expectedCreatedObjectID:)) and replace the
duplicated blocks inside the MAP_SET test groups with calls to that helper; the
helper should check for a non-nil expectedCreatedObjectID and, if present,
perform delegate.objects[expectedCreatedObjectID] =
pool.entries[expectedCreatedObjectID] so both occurrences (current use of
delegate, pool, expectedCreatedObjectID) are centralized and the tests remain
intent-focused.
In `@Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift`:
- Around line 215-236: The test encodesAllFields in WireObjectMessageTests
should be extended to include the newly supported optional operation-level
fields so wiring regressions are caught: update the WireObjectOperation instance
in encodesAllFields to set mapRemove, objectDelete, mapCreateWithObjectId and
counterCreateWithObjectId (in addition to the existing
mapCreate/mapSet/counterCreate/counterInc), then expand the expected wire
dictionary used with `#expect`(wire == [...]) to include the corresponding keys
("mapRemove", "objectDelete", "mapCreateWithObjectId",
"counterCreateWithObjectId") and their encoded shapes; keep using
op.toWireObject to produce wire and mirror the exact nested structures used by
the existing entries to validate encoding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2e08c09-b3dd-4959-90db-131a2fbca941
📒 Files selected for processing (20)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolvedPackage.resolvedPackage.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/InternalLiveMapValue.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftSources/AblyLiveObjects/Protocol/ObjectMessage.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swiftTests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swiftTests/AblyLiveObjectsTests/WireObjectMessageTests.swift
Implements the ObjectOperation field changes specified in spec commit 47a9d51. Integration test updates ported from JS commit fdc33cb (excluding the public API changes, since we don't yet expose any sort of ObjectOperation, nor does the spec). Note that this is the first time that we've had to bump the protocol version in a way that affects LiveObjects, and here we're following the process documented in the pinned plugin-support commit. Also note that this spec change formalises the then-unspecified change that we made in ec37cca. The downside of getting Claude to do this sort of enormous mechanical change is that for a human to review its changes with 100% confidence — in particular, all the spec point reference updates — would require almost as much work as just doing it by hand. So I haven't reviewed it with 100% confidence. I've: - glanced over the whole diff a few times - spot-checked that the most interesting behaviours are implemented and tested (specifically, the creation of the initial value JSON, the retention of the derivedFrom op and its usage when applying the create op) - prompted Claude to review its own work from scratch a few times Beyond that, I'm relying on the fact that the barely-changed integration tests continue to pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7ec81af to
65b36dc
Compare
Note: This is currently based on top of #118, but once that's merged, I'll create an
integration/protocol-v6branch frommainand change this PR to target the integration branch.Summary
Implements the
ObjectOperationfield changes specified in ably/specification#426. See commit message for more details.Related PRs
Summary by CodeRabbit
Chores
Bug Fixes
Refactor