[AIT-454] Implement MAP_CLEAR operation#122
Draft
lawrence-forooghian wants to merge 6 commits intoAIT-207-partial-object-syncfrom
Draft
[AIT-454] Implement MAP_CLEAR operation#122lawrence-forooghian wants to merge 6 commits intoAIT-207-partial-object-syncfrom
MAP_CLEAR operation#122lawrence-forooghian wants to merge 6 commits intoAIT-207-partial-object-syncfrom
Conversation
Add the MAP_CLEAR enum case (OOP2), WireMapClear empty struct (MCL1, MCL2), mapClear field on ObjectOperation (OOP3r), and clearTimeserial field on ObjectsMap (OMP3c). Plumb these through wire encoding, decoding, and domain type conversions. Spec: 5852722 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add clearTimeserial to LiveMap's mutable state (RTLM25) and set it from ObjectState during replaceData (RTLM6i). Add .mapClear to the RTO9a2a operation routing switch (needed for compilation). Spec: 5852722 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add applyMapClearOperation stub (RTLM24) and route to it from the RTLM15 switch (RTLM15d8). Add testsOnly_applyMapClearOperation accessor. Add placeholder failing tests for RTLM24, RTLM15d8, and RTO9a2a3 MAP_CLEAR routing. Spec: 5852722 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Discard a MAP_SET operation if the map's clearTimeserial is non-null and the operation's serial is null or <= clearTimeserial (RTLM7h). Spec: 5852722 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Discard a MAP_REMOVE operation if the map's clearTimeserial is non-null and the operation's serial is null or <= clearTimeserial (RTLM8g). WIP spec: c35735c (TODO: update reference once spec is finalised) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
changed the base branch from
main
to
AIT-207-partial-object-sync
|
Important Review skippedDraft detected. 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 adds support for a MAP_CLEAR operation to the Live Objects protocol, introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
github-actions
bot
temporarily deployed
to
staging/pull/122/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/122/AblyLiveObjects
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Implement applyMapClearOperation which checks clearTimeserial (RTLM24c), updates it (RTLM24d), then removes entries from the internal data whose timeserial is nil or <= the clear serial (RTLM24e). Returns a LiveMapUpdate with the removed keys (RTLM24f). Note: this follows the WIP spec change in 91dd01c where MAP_CLEAR removes entries from data rather than tombstoning them, since the clearTimeserial guards on MAP_SET and MAP_REMOVE make tombstoning redundant. TODO: bring in line with the final spec once Andrii has finalised his PR. Tests: - RTLM24c: parameterised clearTimeserial check (4 cases) - RTLM24: operation application with older, equal, newer, and nil timeserial entries - RTLM15d8: routing through nosync_apply with subscriber emission - RTO9a2a: routing through handleObjectProtocolMessage - mapClearOperationMessage test factory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-454-map-clear
branch
from
15f8798 to
826195e
Compare
![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
🧹 Nitpick comments (3)
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (1)
504-512: Move the access modifier to the extension declaration.Line 504 adds a new non-test Swift extension without an explicit ACL on the extension itself, and the members then repeat
internal.♻️ Suggested cleanup
-extension WireMapClear: WireObjectCodable { - internal init(wireObject _: [String: WireValue]) throws(ARTErrorInfo) { +internal extension WireMapClear: WireObjectCodable { + init(wireObject _: [String: WireValue]) throws(ARTErrorInfo) { // No fields to decode } - internal var toWireObject: [String: WireValue] { + var toWireObject: [String: WireValue] { [:] } }As per coding guidelines,
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)andWhen 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/WireObjectMessage.swift` around lines 504 - 512, The extension for WireMapClear declares members with the internal ACL; move the access control to the extension itself by declaring the extension as internal (e.g. internal extension WireMapClear: WireObjectCodable) and remove the redundant internal modifiers from the initializer init(wireObject _: [String: WireValue]) throws(ARTErrorInfo) and the computed property toWireObject so the extension-level ACL applies to both members.Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
535-544: Consider surfacingclearTimeserialin the higher-level map factories too.
objectsMap(...)now understands the new field, butmapObjectState(...)/mapObjectMessage(...)still can't build a cleared map directly, so tests already have to drop down toobjectState(map:).🤖 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 535 - 544, Add a clearTimeserial parameter to the higher-level map factories so tests can build cleared maps without calling objectState(map:); update mapObjectState(...) and mapObjectMessage(...) signatures to accept clearTimeserial: String? (default nil) and pass it through when creating the underlying ObjectsMap via objectsMap(semantics:entries:clearTimeserial:) or when calling objectState(map:), and update any internal calls to preserve the new argument so existing callers remain backwards-compatible.Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift (1)
167-202: Cover the new wire opcode with a round-trip test.These assertions only prove that
mapClearcan be decoded when the field is present. Because the fixture still usesaction: 0, they won't catch a broken MAP_CLEAR raw value or an omitted"mapClear": [:]on encode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift` around lines 167 - 202, Add a round-trip test that constructs a WireObjectOperation with the MAP_CLEAR opcode (use the enum case .mapClear or its raw value instead of action: 0) and a "mapClear": [:] field, then encode it to the wire dictionary and decode it back asserting action == .known(.mapClear), objectId matches, and mapClear is present; place this alongside or as a new test (e.g., in WireObjectMessageTests/WireObjectOperation tests) so encode+decode will catch any broken MAP_CLEAR raw value or omitted mapClear on encode.
🤖 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/InternalDefaultLiveMap.swift`:
- Around line 470-471: When zeroing the map, also clear the persisted RTLM25
serial: set clearTimeserial = nil inside the map-reset paths so stale serials
aren’t retained. Update the resetData(userCallbackQueue:) implementation and
resetDataToZeroValued() (and any other zero-root code path that currently resets
map contents) to explicitly assign clearTimeserial = nil immediately after the
map/state is cleared so subsequent MAP_SET/MAP_REMOVE/MAP_CLEAR operations use a
fresh serial.
---
Nitpick comments:
In `@Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift`:
- Around line 504-512: The extension for WireMapClear declares members with the
internal ACL; move the access control to the extension itself by declaring the
extension as internal (e.g. internal extension WireMapClear: WireObjectCodable)
and remove the redundant internal modifiers from the initializer init(wireObject
_: [String: WireValue]) throws(ARTErrorInfo) and the computed property
toWireObject so the extension-level ACL applies to both members.
In `@Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift`:
- Around line 535-544: Add a clearTimeserial parameter to the higher-level map
factories so tests can build cleared maps without calling objectState(map:);
update mapObjectState(...) and mapObjectMessage(...) signatures to accept
clearTimeserial: String? (default nil) and pass it through when creating the
underlying ObjectsMap via objectsMap(semantics:entries:clearTimeserial:) or when
calling objectState(map:), and update any internal calls to preserve the new
argument so existing callers remain backwards-compatible.
In `@Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift`:
- Around line 167-202: Add a round-trip test that constructs a
WireObjectOperation with the MAP_CLEAR opcode (use the enum case .mapClear or
its raw value instead of action: 0) and a "mapClear": [:] field, then encode it
to the wire dictionary and decode it back asserting action == .known(.mapClear),
objectId matches, and mapClear is present; place this alongside or as a new test
(e.g., in WireObjectMessageTests/WireObjectOperation tests) so encode+decode
will catch any broken MAP_CLEAR raw value or omitted mapClear on encode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e50b0da5-1be4-4c6f-954e-15a37b1eda23
📒 Files selected for processing (8)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Protocol/ObjectMessage.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/WireObjectMessageTests.swift
Comment on lines
+470
to
+471
| /// RTLM25 | ||
| internal var clearTimeserial: String? |
There was a problem hiding this comment.
Reset clearTimeserial when the map is zeroed.
clearTimeserial is now persisted alongside the map data, but resetData(userCallbackQueue:) and resetDataToZeroValued() still leave it behind. After an empty-root reset, later MAP_SET/MAP_REMOVE/MAP_CLEAR operations can be rejected against a stale serial from the previous snapshot.
Possible fix
internal mutating func resetData(userCallbackQueue: DispatchQueue) {
// RTO4b2
let previousData = data
data = [:]
+ clearTimeserial = nil
// RTO4b2a
let mapUpdate = DefaultLiveMapUpdate(update: previousData.mapValues { _ in .removed })
liveObjectMutableState.emit(.update(mapUpdate), on: userCallbackQueue)
}
mutating func resetDataToZeroValued() {
// RTLM4
data = [:]
+ clearTimeserial = nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift` around lines
470 - 471, When zeroing the map, also clear the persisted RTLM25 serial: set
clearTimeserial = nil inside the map-reset paths so stale serials aren’t
retained. Update the resetData(userCallbackQueue:) implementation and
resetDataToZeroValued() (and any other zero-root code path that currently resets
map contents) to explicitly assign clearTimeserial = nil immediately after the
map/state is cleared so subsequent MAP_SET/MAP_REMOVE/MAP_CLEAR operations use a
fresh serial.
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.
Note: This PR is based on top of #117; please review that one first.
Implements the
MAP_CLEARoperation as specified in ably/specification#432, plus some changes that I've made on https://github.com/ably/specification/tree/AIT-466/map-clear-Lawrence-modfications-for-swift which are waiting to be incorporated into the spec PR.TODO:
Summary by CodeRabbit
New Features
Tests