[ECO-5332] Implement OBJECT_SYNC
#21
Conversation
The one about array literals is because on multiple occasions it emitted code that upset our linters' trailing_comma rules. Would be worth investigating in the future how to get it to run linting automatically.
These will be useful in some upcoming tests, and also I think generally useful. (Cursor generated this code at my instruction.)
I didn't do this in ce8c022 because the JS documentation doesn't mention that they can throw. But per specification points RTLC5b and RTLM5c in [1] at dd25dca they can. [1] ably/specification#333
lawrence-forooghian
force-pushed
the
ECO-5332-object-sync
branch
2 times, most recently
from
d9cbf76 to
3c3ff03
Compare
github-actions
bot
temporarily deployed
to
staging/pull/21/AblyLiveObjects
Based on [1] at dd25dca. The development approach here was roughly the following: I decided the internal types and interfaces that I wanted to exist, and some of the implementation details (e.g. the mutations that DefaultRealtimeObjects performs during a sync sequence). I then asked Cursor to fill in the most of the implementation and tests (providing it with the relevant spec text). I have done some tweaking of the code that Cursor generated, but that doesn't mean that all of the code that's here is exactly the same as code that I would write, and I'm going to leave it that way. I have looked through all (and changed some) of the tests that Cursor generated, and in doing so also fixed the spec conformance tags (because it got those very wrong), so I am _fairly_ confident that the tests are testing what they claim to test. But I have not looked at them in minute detail (in particular, the exact data created by the canned data factories that it created). I also think that tests are one of those things where there's not always one right way to write them, so I'm not going to get hung up on "could it have done this test differently?" or "are the tests consistent with each other?" at the moment. There are some outstanding questions on the spec PR at the moment; have implemented to best of my understanding, and will update later once those are answered. Not yet implemented: - Checking of channel modes before performing operations (RTO2 and the points that refer to it); there's an outstanding question about this at [2] A note re the MutableState pattern that I've used here — done this so that the class can essentially have mutating instance methods which can call each other without having to worry about whose responsibility it is to acquire the mutex. [1] ably/specification#333 [2] https://github.com/ably/specification/pull/333/files#r2152297442
I'm going to port the JS integration tests to Swift, and need a way to intercept and inject ProtocolMessages. I didn't want to come up with something new, so have just copied this from ably-cocoa at 5fc038c and made the minimum amount of edits needed to get it to compile (mainly suppressing various concurrency warnings with @preconcurrency and @unchecked Sendable and nonisolated(unsafe)).
This is based on ably-js at 39e0677. I think that these tests will prove to be very useful and should help reduce the testing burden when implementing this plugin. I wrote an example of how to map the parameterised tests into Swift Testing, and wrote some of the Realtime-related helpers. Most of the laborious conversion of the tests and of ObjectsHelper was then done by Cursor, with some manual corrections. I have not looked in detail at its output and am taking an attitude of "something is better than nothing" towards this particular piece of work. I will implement the remaining test cases as we implement the relevant features.
lawrence-forooghian
force-pushed
the
ECO-5332-object-sync
branch
from
3c3ff03 to
fa255c1
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis update introduces a comprehensive implementation and test suite for Ably Live Objects' core synchronization, state management, and protocol compliance. It adds new internal classes for live maps, counters, object pools, and sync cursors, implements detailed protocol handling in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultRealtimeObjects
participant Channel
participant ObjectsPool
participant CoreSDK
Client->>DefaultRealtimeObjects: getRoot()
DefaultRealtimeObjects->>CoreSDK: check channelState
alt Channel attached and sync complete
DefaultRealtimeObjects->>ObjectsPool: get root object
DefaultRealtimeObjects-->>Client: return root LiveMap
else Channel not attached or sync incomplete
DefaultRealtimeObjects->>DefaultRealtimeObjects: await sync completion
DefaultRealtimeObjects->>ObjectsPool: get root object
DefaultRealtimeObjects-->>Client: return root LiveMap
end
sequenceDiagram
participant Channel
participant DefaultRealtimeObjects
participant ObjectsPool
Channel-->>DefaultRealtimeObjects: OBJECT_SYNC(protocolMessage)
DefaultRealtimeObjects->>DefaultRealtimeObjects: parse SyncCursor
DefaultRealtimeObjects->>DefaultRealtimeObjects: accumulate sync objects (by sequenceID)
alt End of sync sequence
DefaultRealtimeObjects->>ObjectsPool: applySyncObjectsPool(syncObjects)
DefaultRealtimeObjects->>DefaultRealtimeObjects: signal sync complete
end
Estimated code review effort5 (~2+ hours) Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
62-62: Track the open specification questionThe TODO references an unresolved specification detail. Consider creating a tracking issue to ensure this is addressed once the specification is finalized.
Would you like me to open an issue to track this specification question?
Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (1)
379-379: Another specification clarification neededSimilar to the previous TODO, this indicates uncertainty about whether to replace the root object or just clear its data.
Would you like me to open an issue to track this specification clarification as well?
Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (2)
6-8: Consider documenting the thread-safety strategy and migration plan.The comment mentions this is a temporary solution with a reference to issue #3. It would be helpful to briefly document what the long-term solution might be (e.g., actor-based concurrency) to guide future development.
82-104: Track unimplemented LiveCounter methods.Several core LiveCounter methods are marked as
notYetImplemented(). Consider adding TODO comments with issue references to track the implementation status of these methods.Would you like me to create GitHub issues to track the implementation of these methods:
increment,decrement,subscribe,unsubscribeAll,on, andoffAll?Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (2)
8-9: Re-enable SwiftLint rule after addressing the issue.The
trailing_closurerule is disabled for consistency with JS code. Consider creating a follow-up task to refactor the code to comply with Swift conventions while maintaining readability.
332-333: Track disabled integration tests.Several tests are disabled due to unimplemented features. Each has a clear reason documented. Consider creating a tracking issue to ensure these tests are enabled as features are implemented.
Would you like me to create a tracking issue listing all disabled tests and their dependencies?
Also applies to: 405-406, 523-524, 554-555, 610-611, 664-665
Tests/AblyLiveObjectsTests/JS Integration Tests/TestProxyTransport.swift (1)
1-2: Document the usage of @preconcurrency import.The
@preconcurrencyimport suggests potential concurrency issues with Ably.Private. Consider adding a comment explaining why this is needed and any plans to address it.Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (2)
6-7: Enhance the source reference comment with more details.Consider adding more specific information about the source file location for better traceability and future maintenance.
-// This file is copied from the file objects.test.js in ably-js. +// This file is copied from the file objects.test.js in ably-js. +// Source: https://github.com/ably/ably-js/blob/main/test/realtime/objects.test.js
536-539: Consider using UUID for random string generation.While the current implementation works for tests, using UUID would provide better uniqueness guarantees and cryptographic randomness.
private func randomString() -> String { - let letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - return String((0 ..< 16).map { _ in letters.randomElement()! }) + return UUID().uuidString.replacingOccurrences(of: "-", with: "").prefix(16).lowercased() }Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (2)
152-155: Track the unimplemented JSON value handling.The TODO comment indicates JSON value handling is not yet implemented. This should be tracked and implemented according to the specification.
Would you like me to create an issue to track the JSON value handling implementation?
185-219: Track unimplemented LiveMap protocol methods.Multiple required protocol methods are not yet implemented. Ensure these are properly tracked for future implementation.
Would you like me to create issues for tracking the implementation of these methods:
entries,keys,valuespropertiesset(key:value:)andremove(key:)methods- Subscription management methods
- Lifecycle event handling methods
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.cursor/rules/swift.mdc(1 hunks).cursor/rules/testing.mdc(1 hunks)Sources/AblyLiveObjects/DefaultRealtimeObjects.swift(5 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(2 hunks)Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift(1 hunks)Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(1 hunks)Sources/AblyLiveObjects/Protocol/SyncCursor.swift(1 hunks)Sources/AblyLiveObjects/Public/PublicTypes.swift(3 hunks)Sources/AblyLiveObjects/Utility/WeakRef.swift(1 hunks)Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift(1 hunks)Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift(1 hunks)Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/Ably+Concurrency.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/Assertions.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(1 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift(1 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(1 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/TestProxyTransport.swift(1 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(1 hunks)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(1 hunks)Tests/AblyLiveObjectsTests/SyncCursorTests.swift(1 hunks)
🧬 Code Graph Analysis (4)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1)
getObjectFromPool(81-85)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (2)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
sendObject(40-46)Tests/AblyLiveObjectsTests/Helpers/Assertions.swift (1)
protocolRequirementNotImplemented(2-7)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (3)
createZeroValued(89-102)replaceData(227-236)replaceData(293-337)Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (3)
createZeroValued(54-63)replaceData(109-113)replaceData(131-150)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
objectState(191-207)objectsMap(492-500)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (3)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
getObjectFromPool(22-24)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
onChannelAttached(97-99)handleObjectSyncProtocolMessage(113-124)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
createZeroValueObject(91-119)applySyncObjectsPool(126-196)
🧰 Additional context used
🧬 Code Graph Analysis (4)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1)
getObjectFromPool(81-85)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (2)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
sendObject(40-46)Tests/AblyLiveObjectsTests/Helpers/Assertions.swift (1)
protocolRequirementNotImplemented(2-7)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (3)
createZeroValued(89-102)replaceData(227-236)replaceData(293-337)Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (3)
createZeroValued(54-63)replaceData(109-113)replaceData(131-150)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
objectState(191-207)objectsMap(492-500)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (3)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
getObjectFromPool(22-24)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
onChannelAttached(97-99)handleObjectSyncProtocolMessage(113-124)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
createZeroValueObject(91-119)applySyncObjectsPool(126-196)
🔇 Additional comments (50)
.cursor/rules/swift.mdc (2)
14-16: LGTM! Clear import guidelines for consistency.The import syntax guidelines are well-defined and will help maintain consistency across the codebase. The distinction between regular and internal imports for AblyPlugin is particularly valuable.
17-27: Good formatting guideline with clear examples.The array literal formatting rule addresses a specific readability concern with proper before/after examples. This will help maintain consistent code style across the project.
Tests/AblyLiveObjectsTests/Helpers/Ably+Concurrency.swift (1)
18-28: LGTM! Consistent async/await wrapper implementation.The
detachAsync()method correctly mirrors the existingattachAsync()pattern, usingwithCheckedContinuationappropriately and maintaining consistent error handling. This provides a clean async interface for channel detachment operations.Tests/AblyLiveObjectsTests/Helpers/Assertions.swift (1)
1-7: LGTM! Well-designed test helper for protocol requirements.The
protocolRequirementNotImplementedfunction is well-implemented with:
- Appropriate use of
fatalErrorfor test scenarios@autoclosureparameter for lazy message evaluation- Clear error message formatting
- Correct
Neverreturn type annotationThis will be useful for marking unimplemented protocol methods in test mocks.
.cursor/rules/testing.mdc (4)
11-11: Excellent clarification on spec attribution.The guideline emphasizes following the exact format from CONTRIBUTING.md and understanding the difference between
@specand@specPartial. This will ensure consistent test documentation.
12-13: Good practices for test quality assurance.Adding comments for irrelevant test data and verifying tests actually pass are fundamental best practices that will improve test maintainability and reliability.
14-17: Clear import guidelines for test consistency.The import specifications are well-defined. The explicit prohibition of
internal importfor AblyPlugin in tests is important since tests should use the public API.
18-20: Good testing framework usage guidelines.Recommending
TestLogger()and preferring#requireoverguard letaligns with Swift Testing best practices and will make tests more robust.Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
10-11: LGTM! Good addition for channel state access.The
channelStateproperty provides a clean interface for components to check the realtime channel state, which is essential for proper error handling in live object operations.
48-50: Correct implementation of channelState property.The implementation correctly exposes the underlying channel's state through the protocol interface, maintaining encapsulation while providing necessary access.
Sources/AblyLiveObjects/Utility/WeakRef.swift (1)
10-16: Well-implemented workaround for Swift's type system limitation.The specialized weak reference struct is correctly implemented and properly documented. The comment clearly explains why this specialization is necessary due to the compiler's restriction on protocol existentials conforming to
AnyObject.Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
5-25: Thread-safe mock implementation follows best practices.The mock delegate correctly implements thread-safe access to the objects dictionary using
NSLockwithwithLock. The use ofnonisolated(unsafe)for the backing storage is appropriate since all access is properly synchronized. The implementation pattern aligns well with the real implementation shown in the codebase.Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
4-30: Well-implemented mock class with proper thread safety.The mock correctly implements thread-safe access to
channelStateusing the same pattern as other mocks in the codebase. The use ofprotocolRequirementNotImplemented()for the stubbedsendObjectmethod is appropriate and follows the established testing patterns.Sources/AblyLiveObjects/Protocol/SyncCursor.swift (2)
13-32: Robust parsing implementation following protocol specifications.The parsing logic correctly implements the RTO5a1 and RTO5a4 protocol rules using
Scanner. The error handling for missing colons is appropriate, and the logic properly handles edge cases like empty sequence IDs and cursor values. The use ofcurrentIndexto extract the remainder after the colon is the correct approach.
34-37: Correct implementation of sequence completion detection.The computed property correctly implements the RTO5a4 protocol rule by checking if
cursorValueis nil to determine sequence completion.Tests/AblyLiveObjectsTests/SyncCursorTests.swift (3)
6-18: Comprehensive test coverage for valid parsing scenarios.The test correctly verifies all properties of the parsed
SyncCursorincludingsequenceID,cursorValue, andisEndOfSequence. The Given-When-Then structure makes the test clear and readable.
36-53: Proper error handling test implementation.The test correctly verifies that the specific
channelSerialDoesNotMatchExpectedFormaterror is thrown when the colon is missing. The error unwrapping pattern properly handles theInternalErrorwrapper structure.
77-89: Good edge case coverage for empty sequence IDs.The test covers an important edge case where the sequence ID is empty but the cursor has a value. This scenario isn't explicitly mentioned in the spec but is properly handled by the implementation.
Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift (2)
7-35: Well-structured test coverage forvalueproperty!The tests comprehensively cover both error conditions (detached/failed channel states) and success cases, with proper spec annotations.
37-130: Comprehensive test coverage forreplaceDatamethod!The test organization into
WithoutCreateOpTestsandWithCreateOpTestsprovides excellent clarity. All specification points for RTLC6 are thoroughly covered.Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (2)
7-79: Excellent test coverage forcreateZeroValueObject!The tests properly verify object creation, reuse of existing objects, and edge cases for invalid object IDs. Good use of identity checks and property verification.
81-346: Thorough test coverage forapplySyncObjectsPool!The tests comprehensively cover all RTO5c specification points including:
- Updating existing objects (maps and counters)
- Creating new objects with proper initialization
- Handling unsupported object types
- Removing unsynced objects while preserving root
- Complex integration scenarios
The verification of side effects through data checks is particularly well done.
Sources/AblyLiveObjects/Public/PublicTypes.swift (2)
86-131: Excellent API enhancement with convenience getters!The convenience getters for
LiveMapValueare well-implemented using idiomatic Swift patterns. The direct access properties (e.g.,stringValue) that chain throughprimitiveValueprovide a clean API for users.
211-211: Well-designed API changes for error handling!The throwing signatures for
get(key:)andvalueappropriately handle channel state validation errors. The convenience getters forPrimitiveObjectValuemaintain consistency with theLiveMapValuepattern.Also applies to: 269-302, 307-307
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
6-40: Well-designed value type with proper encapsulation!The
ObjectsPoolstruct andEntryenum are well-structured with appropriate access control and convenience methods. Good use of value semantics for the pool.
84-119: Correct implementation of zero-value object creation!The method properly implements RTO6 specification requirements with appropriate object ID parsing and type creation. Good defensive comment about maintaining the root invariant.
121-196: Comprehensive implementation of sync pool application!The method correctly implements all RTO5c specification points:
- Updates existing objects
- Creates new objects with proper type detection
- Removes unsynced objects while preserving root
- Includes appropriate logging at debug and warning levels
The use of
Setfor tracking received object IDs is efficient, and the root preservation logic correctly implements RTO5c2a.Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (3)
17-287: Excellent test coverage for OBJECT_SYNC protocol handling!The tests comprehensively cover:
- Single and multi-message sync sequences (RTO5a)
- Sequence interruption and replacement (RTO5a2)
- Object removal with root preservation (RTO5c2)
- Error handling and edge cases
The test organization by specification points aids readability and traceability.
289-491: Thorough testing of channel attachment behavior!The tests effectively cover RTO4 specification points with realistic setup using OBJECT_SYNC messages. Good coverage of edge cases including multiple calls and complex sync states.
492-667: Comprehensive async testing of getRoot behavior!The tests effectively cover all RTO1 specification points:
- Async waiting for sync completion via different paths
- Immediate return when sync is complete
- Proper error throwing for detached/failed channels
Excellent use of
async letand proper task handling.Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (2)
67-80: LGTM! Correct implementation of RTLC5b and RTLC5c.The value property correctly validates channel state and throws the appropriate error before returning the counter value in a thread-safe manner.
109-151: Excellent implementation of RTLC6 specification.The
replaceDatamethod correctly implements all RTLC6 requirements with proper thread safety. The code is well-structured and the comments clearly map to specification points.Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift (1)
1-639: Excellent test coverage and organization!The test suite is well-structured with:
- Clear organization using nested structs for different functionality areas
- Comprehensive coverage of RTLM specification points
- Good use of parameterized tests for testing multiple scenarios
- Proper testing of edge cases and error conditions
The tests thoroughly validate the DefaultLiveMap implementation against the specification.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (1)
59-101: Well-implemented connection monitoring with proper cleanup.The helper correctly monitors connection state changes and ensures proper cleanup with the defer block and subscription termination handling.
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (4)
5-11: Good addition of LiveMapObjectPoolDelegate conformance.The class now properly implements the delegate protocol to serve pooled objects. The mutable state encapsulation with thread safety is well-designed.
89-112: Excellent implementation of RTO1 specification points.The
getRoot()method correctly implements:
- RTO1b: Channel state validation with appropriate error
- RTO1c: Waiting for sync sequence completion
- RTO1d: Returning the root object from the pool
The async handling and thread safety are properly implemented.
250-318: Comprehensive implementation of RTO5 OBJECT_SYNC handling.The method correctly implements all RTO5 specification points:
- RTO5a: Sync cursor parsing and sequence management
- RTO5b: Accumulating object messages
- RTO5c: Applying completed sync to the object pool
The handling of both multi-message sequences and single-message syncs is well-designed.
239-241: Clarify RTO4b specification requirement.The TODO comment indicates uncertainty about whether to replace the root object or just clear its data. This should be clarified with the specification authors to ensure correct implementation.
Please verify with the specification authors whether RTO4b requires replacing the root object entirely or just clearing its data. The current implementation replaces the entire object pool, which may be more than required.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (6)
13-37: LGTM!The Actions enum is well-structured with clear mappings between numeric and string representations. The exhaustive switch statement ensures all cases are handled.
45-50: LGTM!The initialization properly sets up the REST client with appropriate options for sandbox testing.
64-128: LGTM!The channel initialization method creates a comprehensive test fixture with proper documentation of the object tree structure. The async/await pattern is used correctly for sequential REST API operations.
133-219: LGTM!The wire object message creation methods are well-structured and consistent. They properly handle optional parameters and create protocol-compliant message structures.
318-353: LGTM!Good use of
withExtendedLifetimeto ensure the encoder remains alive during protocol message creation. The message processing flow is correct.
468-514: LGTM!The REST API operation methods have comprehensive error handling and properly extract objectId from different response formats. The error messages include useful debugging information.
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
187-554: Factory methods are well-structured but need verification.The factory methods follow good patterns with sensible defaults and comprehensive coverage. However, given the AI-generation disclaimer, please verify:
- Default values match actual test requirements
- Object construction aligns with domain model constraints
- All required fields are properly initialized
Consider adding unit tests for these factory methods to ensure they produce valid objects.
5-5: Verify AI-generated code in TestFactories.swiftPlease thoroughly review all factory method implementations in Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift to ensure:
- Default values align with your codebase conventions
- Parameter types and optionality are correct
- Objects are constructed properly according to the domain model
Once you’ve confirmed everything is correct, update or remove the AI-generated disclaimer at the top of the file.
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (4)
107-176: LGTM! The get() implementation follows the specification correctly.The method properly implements RTLM5 with correct channel state validation, tombstone handling, and data type processing in the specified order.
223-269: LGTM! Data manipulation methods are well-implemented.The methods correctly implement RTLM6, RTLM7, and RTLM8 specifications with proper mutex synchronization. The
testsOnly_prefix appropriately indicates future broader usage.
405-442: Excellent implementation of operation ordering logic!The
canApplyMapOperationmethod correctly implements RTLM9 with proper timeserial normalization and lexicographic comparison. The switch pattern matching makes the logic clear and maintainable.
370-374: Duplicate zero-value object creation is safely handled
TheObjectsPool.createZeroValueObjectimplementation checksentries[objectID]and returns the existing entry if present, preventing any duplicate instantiation. No further changes are required.
Tests/AblyLiveObjectsTests/JS Integration Tests/TestProxyTransport.swift
Show resolved
Hide resolved
|
i didn't look into the js integration tests but everything else looks good to me |
Note: This PR is based on top of #20; please review that one first.
This implements the
OBJECT_SYNCspecification andLiveMapandLiveCounteraccess methods; that is, the contents of ably/specification#333. It also copies across some of the integration tests from ably-js. See commit messages for more details.Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Tests
Chores