[ECO-5333] Apply OBJECT ProtocolMessage operations
#23
Conversation
This is based on the values stated in RTOL3 of [1] at 29276a5. (We'll formalise the fact that these are common LiveObject properties in an upcoming commit.) [1] ably/specification#343
Based on [1] at 29276a5. [1] ably/specification#343
All done by Cursor at my instruction.
Based on [1] at 29276a5. Development approach as described in cb427d8. [1] ably/specification#343
This applies the changes from [1] at 29276a5. Development approach as described in cb427d8. [1] ably/specification#343
github-actions
bot
temporarily deployed
to
staging/pull/23/AblyLiveObjects
Based on [1] at 29276a5. I wrote the implementation, and for the tests followed the development approach described in cb427d8. We have a separate issue for applying RTO8a's buffering during a sync, so I haven't done that here. [1] ably/specification#343
lawrence-forooghian
force-pushed
the
ECO-5333-apply-OBJECT
branch
from
70eb995 to
6430358
Compare
github-actions
bot
temporarily deployed
to
staging/pull/23/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/23/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5333-apply-OBJECT
branch
from
22c71cf to
6430358
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change set introduces robust, thread-safe, and logged handling of OBJECT protocol messages and object operations in AblyLiveObjects. It centralizes mutable state management for live objects, refactors creation and application logic for maps and counters, and propagates logging throughout. Extensive new and updated tests provide comprehensive coverage of operation application, state transitions, and protocol compliance. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultRealtimeObjects
participant Mutex
participant MutableState
participant ObjectsPool
participant Logger
Client->>DefaultRealtimeObjects: Receive OBJECT protocol message
DefaultRealtimeObjects->>Mutex: Lock
Mutex-->>DefaultRealtimeObjects: Locked
DefaultRealtimeObjects->>MutableState: handleObjectProtocolMessage(messages)
MutableState->>Logger: Log receipt
MutableState->>ObjectsPool: For each message, apply operation
ObjectsPool->>Logger: Log object creation/warnings
ObjectsPool->>ObjectsPool: Create or fetch object as needed
ObjectsPool->>ObjectsPool: Apply operation to object (map/counter)
MutableState->>Logger: Log operation results
DefaultRealtimeObjects->>Mutex: Unlock
Mutex-->>DefaultRealtimeObjects: Unlocked
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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: 1
🧹 Nitpick comments (2)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (2)
247-271: Test methods properly implemented with thread safety.The test-only methods correctly delegate to mutableState while maintaining thread safety. Consider whether these could be tested through public APIs instead of exposing internal methods.
579-603: MAP_CREATE operation handler correctly prevents duplicates.The implementation properly checks for already-merged CREATE operations and logs appropriately. Note the TODO for semantics comparison (RTLM16c) that needs to be addressed.
Would you like me to open an issue to track the TODO for implementing the semantics comparison?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift(5 hunks)Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift(3 hunks)Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift(11 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(5 hunks)Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift(5 hunks)Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift(23 hunks)Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(2 hunks)Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(11 hunks)
🔇 Additional comments (41)
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (3)
11-11: LGTM: Consistent logger integration in testsThe addition of
TestLogger()instances throughout the test methods properly supports the enhanced logging capabilities introduced in the production code. The changes are mechanical and maintain test integrity.Also applies to: 25-25, 45-45, 63-63, 75-75
14-15: LGTM: Proper parameter passing for updated constructorsThe tests correctly pass the required
loggerparameter to the updated constructors and factory methods. The changes maintain compatibility with the enhanced production code signatures.Also applies to: 28-28, 48-48, 66-66, 78-78
17-17: LGTM: Consistent method signature updatesAll method calls are updated to include the new
loggerparameter, ensuring comprehensive test coverage of the logging functionality introduced across the codebase.Also applies to: 30-30, 50-50, 68-68, 80-80
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (1)
1-117: Excellent comprehensive test coverage for LiveObjectMutableStateThis test suite provides thorough coverage of the
canApplyOperationmethod with well-structured parameterized tests. The test cases cover all the RTLO4 specification points including:
- Invalid serial/siteCode validation (RTLO4a3-4)
- Missing/empty siteSerial handling (RTLO4a5)
- Lexicographical comparison logic (RTLO4a6)
The use of a
TestCasestruct with descriptive names makes the tests highly maintainable and clear about what each scenario validates.Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (3)
6-13: Well-structured mutable state containerThe
LiveObjectMutableStatestruct effectively centralizes the common mutable state properties (objectID, siteTimeserials, createOperationIsMerged) that are shared across live object implementations. The properties correctly map to the RTLO3 specification points.
17-20: Good design choice for ApplicableOperationThe
ApplicableOperationstruct is a clean way to return both the decision (can apply) and the validated parameters (non-nil serial and siteCode) needed for subsequent operation processing. This avoids duplicate validation logic downstream.
25-49: Solid implementation of operation applicability logicThe
canApplyOperationmethod correctly implements the RTLO4a specification with:
- Proper validation of serial and siteCode (RTLO4a3)
- Appropriate warning logging for invalid inputs
- Correct handling of missing/empty siteSerial (RTLO4a5)
- Accurate lexicographical comparison (RTLO4a6)
The guard clauses make the logic clear and prevent invalid operations from proceeding.
Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (1)
668-926: Comprehensive test coverage for OBJECT protocol message handlingThe new
HandleObjectProtocolMessageTestssuite provides excellent coverage of the RTO8 and RTO9 specification points:Strengths:
- Well-organized nested structure separating different test concerns
- Realistic test setup using sync messages to establish initial state
- Proper verification of side effects (updated values, site timeserials, merge flags)
- Coverage of all major operation types (MAP_CREATE, MAP_SET, MAP_REMOVE, COUNTER_CREATE, COUNTER_INC)
- Clear test naming and documentation with spec references
Good testing practices:
- Tests verify both object creation (RTO9a2a1) and operation application (RTO9a2a3)
- Each test establishes known initial state before applying operations
- Side effects are checked to ensure operations are properly applied
- Proper use of test factories for creating consistent test data
The test structure effectively validates the new operation application logic introduced in the pull request.
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (4)
32-55: Well-designed operation delegation methodThe new
applymethod in theEntryenum provides clean delegation to the underlying live object types. The method signature properly passes through all necessary parameters including the mutableObjectsPoolreference, enabling objects to modify the pool state during operation application.
72-73: Consistent logger parameter integrationThe logger parameter is properly threaded through all constructors and factory methods, enabling comprehensive logging throughout the object pool operations. The consistent parameter passing maintains API coherence.
Also applies to: 78-79, 86-87, 91-91
118-120: Proper logger propagation in object creationThe
createZeroValueObjectmethod correctly passes the logger parameter to the zero-value constructors of bothDefaultLiveMapandDefaultLiveCounter, ensuring logging capabilities are available during object creation.Also applies to: 138-141
160-162: Effective logging integration for debuggingThe logging statements provide valuable debugging information at appropriate levels:
- Debug level for normal operations (object counts, creation, updates)
- Warning level for unsupported operations
- Clear, informative messages that aid in troubleshooting
This will be helpful for debugging synchronization and operation application issues.
Also applies to: 172-173, 183-184, 203-204, 218-219, 224-224
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
389-392: LGTM! Well-structured test utilityThe
counterOphelper method follows the existing factory pattern and provides a clean way to create counter operations for testing.
524-617: Well-implemented operation message factoriesThe new factory methods for creating operation messages (MAP_SET, MAP_REMOVE, MAP_CREATE, COUNTER_CREATE, COUNTER_INC) are well-structured and follow the existing patterns. They provide comprehensive test data creation utilities that align with the broader operation handling enhancements.
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (4)
76-76: Logger parameter properly integratedGood addition of the logger parameter to the ObjectsPool initialization, ensuring consistent logging throughout the object lifecycle.
169-180: Well-structured OBJECT protocol message handlingThe implementation properly delegates to the mutable state with mutex protection and passes all necessary parameters including the logger. This follows the RTO8 specification requirements.
333-406: Comprehensive implementation of OBJECT handling (RTO8/RTO9)The implementation correctly:
- Yields messages to the continuation stream
- Applies each object message individually
- Creates zero-value objects when needed (RTO9a2a2)
- Handles all specified operation types
- Logs warnings for unsupported operations (RTO9a2b)
The thread-safe design and detailed logging make this a robust implementation.
283-287: Review Sync Cursor Parsing Error HandlingThe code in Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (lines 283–287) currently catches and logs any
SyncCursorparsing error, then returns early—silently dropping the message. Please confirm whether, per the RTO5a specification, a parsing failure should instead
• be propagated (e.g., throw up to the caller or surface via a client callback)
• or if aborting without propagation is the intended behavior.Points to review:
- File: Sources/AblyLiveObjects/DefaultRealtimeObjects.swift
- Lines: 283–287
Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift (3)
12-13: Consistent logger usage across testsGood practice to consistently use TestLogger throughout all tests, enabling better debugging capabilities during test execution.
816-890: Comprehensive coverage of merge initial value functionality (RTLM17)Excellent test coverage for the merge initial value functionality, including:
- MAP_SET operations from create operations
- MAP_REMOVE operations with proper timeserial ordering
- Verification of the createOperationIsMerged flag
The tests effectively verify the specification requirements.
942-1084: Thorough testing of apply operation method (RTLM15)The ApplyOperationTests struct provides excellent coverage of the apply method, including:
- Operations that cannot be applied due to timeserial ordering
- All operation types (MAP_CREATE, MAP_SET, MAP_REMOVE)
- Site timeserial updates
- Proper mutex protection verification
The tests are well-structured and cover both positive and negative cases.
Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift (3)
12-13: Consistent test setup with logger and objectIDGood consistency in using TestLogger and explicit objectID throughout all tests, aligning with the refactored DefaultLiveCounter implementation.
126-177: Well-structured merge initial value tests (RTLC10)Excellent coverage of the merge initial value functionality with clear test cases for:
- Adding counter count to existing data
- Handling missing count values
- Setting the createOperationIsMerged flag
The tests effectively verify all aspects of RTLC10.
244-335: Comprehensive apply operation tests (RTLC7)The ApplyOperationTests provide thorough coverage including:
- Operations that cannot be applied due to timeserial constraints
- Both COUNTER_CREATE and COUNTER_INC operations
- Site timeserial updates
- Proper verification of side effects
The parameterized approach for testing different scenarios is well-implemented.
Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (5)
2-2: Proper integration of logger dependencyGood addition of the AblyPlugin import and logger property, enabling comprehensive logging throughout the counter implementation.
Also applies to: 31-31
12-28: Clean abstraction of mutable state propertiesThe test-only properties now properly delegate to the centralized LiveObjectMutableState, providing a clean interface while maintaining encapsulation.
185-189: Consider extracting common merge logicThe merge initial value logic is duplicated between
replaceDataandmergeInitialValue. Consider callingmergeInitialValue(from: createOp)directly here to reduce duplication.- // RTLC6d: If ObjectState.createOp is present, merge the initial value into the LiveCounter as described in RTLC10 - if let createOp = state.createOp { - mergeInitialValue(from: createOp) - } + // RTLC6d: If ObjectState.createOp is present, merge the initial value into the LiveCounter as described in RTLC10 + if let createOp = state.createOp { + mergeInitialValue(from: createOp) + }Actually, the code is already doing this correctly. My apologies for the confusion.
147-232: Well-structured operation application with proper validationThe apply method implementation is excellent:
- Proper validation using canApplyOperation (RTLC7b)
- Site timeserial updates (RTLC7c)
- Correct dispatching to operation-specific handlers
- Comprehensive logging for debugging
- Thread-safe implementation
234-257: Robust implementation of counter operationsBoth COUNTER_CREATE and COUNTER_INC operations are implemented correctly:
- Proper check for duplicate create operations (RTLC8b)
- Clean increment logic with nil handling
- Appropriate warning logs for error conditions
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (12)
2-2: LGTM! Appropriate import for logging support.The
internalvisibility modifier correctly restricts the import scope.
23-45: Good refactoring to centralize mutable state management.The migration of properties to
LiveObjectMutableStateimproves code organization and maintains thread safety through consistent mutex usage.
54-55: LGTM! Logger property follows dependency injection pattern.The logger is correctly declared as an immutable private property.
58-88: Initialization properly updated for new state structure and logging.The logger parameter is correctly threaded through both initializers, and the new
LiveObjectMutableStateis properly initialized.
93-110: Factory method correctly updated with required parameters.The
createZeroValuedfactory method properly accepts and passes through the newobjectIDandloggerparameters.
235-246: LGTM! Logging support added to replaceData.The logger is correctly passed through while maintaining thread safety.
273-291: Well-structured method for applying operations.The
applymethod properly handles thread safety and follows the established delegation pattern.
310-310: LGTM! Logger parameter added consistently.
330-339: Excellent refactoring to consolidate common state.Moving shared mutable state into
LiveObjectMutableStateimproves code organization and promotes reusability across LiveObject implementations.
343-369: Proper implementation of create operation handling.The method now correctly handles
createOpby delegating tomergeInitialValue, following the RTLM6d specification.
371-406: Well-implemented merge logic following RTLM17 specification.The method correctly handles both tombstoned and non-tombstoned entries, with clear spec references and proper delegation to operation handlers.
474-510: Logger parameter properly integrated into MAP_SET operation.The logger is correctly passed through to enable logging during zero-value object creation.
umair-ably
left a comment
umair-ably
left a comment
There was a problem hiding this comment.
didn't look at the tests but impl looks good
Note: This PR is based on top of #22; please review that one first.
Implements the behaviours specified in ably/specification#343, except for buffering during a sync, which we have a separate task for. See commit messages for more details.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores