[ECO-5148] Implement “async room get” spec points #171
Conversation
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce significant enhancements to error handling and room state management within the Ably Chat SDK. A new error case, Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
Sources/AblyChat/Errors.swift (1)
Line range hint
305-315: Address the TODO comments inARTErrorInfoextensionThe TODO comments indicate temporary workarounds related to error domain handling and the use of
NSUnderlyingErrorKey. Consider refactoring this code to eliminate the need for copied implementations and to properly handle error domains and documentation.Would you like assistance in proposing a refactored solution or opening a GitHub issue to track this task?
Sources/AblyChat/DefaultMessages.swift (1)
237-244: Enhance error message with more detailsWhile the error handling structure is good, the error message could be more informative by including the current channel state and any relevant error details from the state change.
Consider updating the error message like this:
- message: "Channel failed to attach" + message: "Channel failed to attach. Current state: \(stateChange.current). Reason: \(stateChange.reason?.message ?? "unknown")"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/Errors.swift(6 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Sources/AblyChat/Rooms.swift(2 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(13 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomFactory.swift(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- Sources/AblyChat/RoomLifecycleManager.swift
- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
🔇 Additional comments (22)
Sources/AblyChat/Errors.swift (5)
35-35: Addition of roomReleasedBeforeOperationCompleted error code
The new error code roomReleasedBeforeOperationCompleted (value 102_106) is appropriately added and follows the existing error code pattern.
39-129: Improved error code and status code mapping
The introduction of CaseThatImpliesFixedStatusCode, CaseThatImpliesVariableStatusCode, and ErrorCodeAndStatusCode enums enhances the structure and clarity of error handling by systematically associating error codes with their corresponding HTTP status codes.
171-176: Update of ChatError.codeAndStatusCode property
Replacing the code property with codeAndStatusCode provides a more structured representation of errors, encapsulating both error codes and status codes.
272-273: Clear localized description for roomReleasedBeforeOperationCompleted
The localized description "Room was released before the operation could complete." is clear and conveys the error effectively.
288-288: Proper handling of cause in roomTransitionedToInvalidStateForPresenceOperation
Including the cause in the roomTransitionedToInvalidStateForPresenceOperation case allows for better error tracing and debugging.
Sources/AblyChat/Rooms.swift (5)
24-32: Well-structured addition of RoomState enum
The RoomState enum effectively manages the different states of a room, improving the clarity of room state transitions.
33-70: Introduction of RoomMapEntry enhances room mapping
The RoomMapEntry enum encapsulates room creation and request states, providing a clear distinction between pending and created rooms.
82-115: Conditional compilation for testing utilities
The testing utilities wrapped in #if DEBUG are appropriately conditionally compiled, ensuring they are included only in debug builds.
208-228: Efficient creation failure signaling with makeCreationFailureFunctions()
The approach for signaling room creation failures using AsyncThrowingStream is effective and well-structured.
283-297: Proper cleanup in release(roomID:) method
The use of a defer block to remove the room state after release ensures that the room state is cleaned up appropriately, even if future modifications make room.release() throwable.
Tests/AblyChatTests/DefaultRoomsTests.swift (6)
8-30: Useful SignallableReleaseOperation helper for controlled testing
The SignallableReleaseOperation class provides a valuable way to control and test asynchronous release operations in a predictable manner.
63-82: Test get_whenRoomExistsInRoomMap_returnsExistingRoomWithGivenID verifies room reuse
The test correctly verifies that an existing room is returned when get is called with the same room ID and options.
86-127: Test get_whenFutureExistsInRoomMap_returnsExistingRoomWithGivenID ensures proper synchronization
The test effectively checks that concurrent calls to get wait appropriately when a release is in progress, and the same room instance is returned.
129-153: Test get_whenRoomExistsInRoomMap_throwsErrorWhenOptionsDoNotMatch correctly handles option mismatch
The test accurately asserts that an error is thrown when get is called with different options for an existing room.
156-196: Test get_whenFutureExistsInRoomMap_throwsErrorWhenOptionsDoNotMatch handles pending creations
The test properly verifies that an error is thrown when a get call with different options is made while a previous get is pending.
296-348: Test release_withReleaseInProgress_failsPendingGetOperations validates error propagation
The test correctly ensures that pending get operations fail with the appropriate error when a second release is called during an ongoing release operation.
Tests/AblyChatTests/Helpers/Helpers.swift (1)
7-14: Update isChatError function to accommodate new error structure
The function signature and internal logic are appropriately updated to handle ErrorCodeAndStatusCode, ensuring accurate error validation in tests.
Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1)
Line range hint 5-17: Tracking of createRoom calls enhances test reliability
Introducing createRoomCallCount and storing createRoomArguments allows for more precise assertions in tests, improving test accuracy.
Tests/AblyChatTests/Mocks/MockRoom.swift (3)
11-11: LGTM: Proper initialization of async stream
The initialization using makeStream() is the correct approach for creating an async stream with continuation.
56-56: LGTM: Proper async stream usage
The yield operation is correctly placed in the release() method to track calls.
63-68: LGTM: Well-documented public API
Good documentation and proper encapsulation of the async stream implementation.
Sources/AblyChat/RoomFeature.swift (1)
44-48: LGTM: Comprehensive documentation update
The documentation clearly outlines the error handling behavior for different room states and references the relevant specification points (CHA-PR3d, CHA-PR3e, etc.).
lawrence-forooghian
force-pushed
the
152-implement-async-room-get-spec
branch
from
23062c8 to
90f9c55
Compare
lawrence-forooghian
changed the title
|
Tests are failing due to #169. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Tests/AblyChatTests/DefaultRoomsTests.swift (4)
6-7: Addition of Test Helpers SectionThe
// MARK: - Test helperssection is added for better organization of test helper classes and functions.Consider adding a brief comment explaining the purpose of the test helpers for clarity.
8-30: Implementation ofSignallableReleaseOperationThe
SignallableReleaseOperationclass is a well-designed mock to simulate thereleaseoperation with controllable completion for testing purposes.However, there is a minor simplification that can be made.
Suggestion to Simplify
releaseImplementationFunctionYou can simplify the use of
AsyncStreamin thereleaseImplementationby removing the unnecessary closure in thefirstmethod.Apply this diff to simplify the code:
-releaseImplementation = { @Sendable () async in - await (stream.first { _ in true }) // this will return if we yield to the continuation or if the Task is cancelled +releaseImplementation = { @Sendable () async in + await stream.firstThis change makes the code cleaner and more straightforward.
34-35: Spec Annotations for Test MethodThe addition of spec annotations
// @spec CHA-RC1fand// @spec CHA-RC1f3provides clarity on which specification points the test covers.Ensure that all test methods have corresponding spec annotations for consistency.
86-128: Addition of Test for Pending Release OperationThe new test
get_whenFutureExistsInRoomMap_returnsExistingRoomWithGivenID()effectively checks the behavior when a previousgetcall is awaiting a release operation.Consider adding comments to explain the use of
operationWaitSubscriptionfor improved readability.Sources/AblyChat/Rooms.swift (1)
116-207: Enhanced Logic inget(roomID:options:)MethodThe updated
getmethod accurately handles various room states, including waiting for release operations and managing room creation tasks.Consider adding documentation comments to explain complex logic sections for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Sources/AblyChat/Errors.swift(8 hunks)Sources/AblyChat/Rooms.swift(2 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/AblyChatTests/Mocks/MockRoom.swift
🔇 Additional comments (27)
Sources/AblyChat/Errors.swift (8)
35-36: Addition of new error case in ErrorCode enum
The new error case roomReleasedBeforeOperationCompleted with code 102_106 is appropriately added to the ErrorCode enum.
55-55: Inclusion of new case in CaseThatImpliesFixedStatusCode
The roomReleasedBeforeOperationCompleted case is correctly added to the CaseThatImpliesFixedStatusCode internal enum.
87-88: Correct mapping to numeric error code
The mapping of .roomReleasedBeforeOperationCompleted to its numeric error code in toNumericErrorCode is accurately implemented.
99-100: Appropriate HTTP status code assignment
Assigning the HTTP status code 400 to .roomReleasedBeforeOperationCompleted in the statusCode property aligns with other client error cases and follows the specification.
171-171: New error case added to ChatError enum
The roomReleasedBeforeOperationCompleted case is properly included in the ChatError internal enum.
211-212: Accurate error code and status code mapping
The codeAndStatusCode property correctly maps .roomReleasedBeforeOperationCompleted to its corresponding fixed status code.
272-273: Clear and specific localized description
The localized description for roomReleasedBeforeOperationCompleted—"Room was released before the operation could complete."—is clear and helpful for users.
294-294: Consistent cause property handling
The cause property correctly returns nil for .roomReleasedBeforeOperationCompleted, consistent with similar cases where there is no underlying error.
Tests/AblyChatTests/DefaultRoomsTests.swift (11)
37-37: Renaming Test Method for Clarity
The test method get_returnsRoomWithGivenIDAndOptions() has been appropriately renamed to reflect that it now verifies both the room ID and options.
51-56: Verification of Room Map Entry Creation
The added assertions to verify the creation of a room map entry are valuable for ensuring that the room state is correctly managed.
63-65: Enhanced Test Coverage for Existing Room Retrieval
The method get_whenRoomExistsInRoomMap_returnsExistingRoomWithGivenID() improves test clarity by specifying the condition under which the room is retrieved.
66-82: Correct Testing of Room Retrieval Logic
The test accurately verifies that when a room already exists in the room map, a new room is not created, and the existing room is returned.
132-154: Test for Inconsistent Room Options Error
The test get_whenRoomExistsInRoomMap_throwsErrorWhenOptionsDoNotMatch() correctly verifies that an error is thrown when attempting to get a room with different options.
156-197: Test for Error Handling During Pending Release
The test get_whenFutureExistsInRoomMap_throwsErrorWhenOptionsDoNotMatch() appropriately ensures that an inconsistentRoomOptions error is thrown in scenarios with pending release operations.
199-236: Test for Get Operation During Release
The test get_whenReleaseInProgress validates that the get operation waits for the release to complete and handles the room map entry correctly.
243-255: Test for No-Op Release
The test release_withNoRoomMapEntry_andNoReleaseInProgress() ensures that releasing a room with no room map entry and no release in progress completes without issues.
259-295: Test for Waiting on Release in Progress
The test release_withNoRoomMapEntry_andReleaseInProgress() confirms that a subsequent release call waits for the in-progress release operation to complete.
299-349: Test for Failing Pending Get Operations
The test release_withReleaseInProgress_failsPendingGetOperations() effectively checks that pending get operations fail with roomReleasedBeforeOperationCompleted when a release is initiated.
Line range hint 354-371: Ensuring Room Map Entry Removal Before Release
In the release test, verifying that the room map entry is removed before calling release on the room aligns with specification CHA-RC1g5.
Sources/AblyChat/Rooms.swift (8)
24-32: Introduction of RoomState Enum
The new RoomState enum enhances the management of room states by distinguishing between release operations and room map entries.
33-70: Definition of RoomMapEntry Enum
The RoomMapEntry enum provides a clear structure for representing the state of room creation and pending requests.
82-114: Addition of Operation Tracking for Testing
The inclusion of OperationType and OperationWaitEvent, along with related methods, is beneficial for testing and debugging operation flows.
209-229: Utility Functions for Handling Creation Failures
The makeCreationFailureFunctions() method is a useful addition for managing room creation errors in asynchronous operations.
231-239: Implementation of waitForOperation Helper Method
The waitForOperation method centralizes the logic for waiting on other operations, improving code readability and reuse.
240-244: Refactored createRoom Method
The createRoom method is appropriately refactored to use the new state management structures and logs the creation process.
248-257: Testing Helpers for Room Map Entry Checks
The testsOnly_hasRoomMapEntryWithID method aids in testing by allowing checks on the internal state of room entries.
262-298: Updated Logic in release(roomID:) Method
The release method now correctly handles different room states, waits for ongoing operations, and ensures the room state is updated appropriately.
Ensure that exception handling is in place if future changes allow room.release() to throw errors.
Consider adding unit tests to cover scenarios where room.release() might throw an error.
lawrence-forooghian
force-pushed
the
CHA-PR3h-etc
branch
from
cf769a1 to
ba4658d
Compare
Missed this in 7d6acde.
The existing check was not great, since the factory only ever returns a single instance.
That is, CHA-RC1f and CHA-RC1g. Note that I’ve taken a slightly different approach to "one operation waits for another" here than that which I took in the room lifecyle manager. I’m using Task instances to represent the operation’s work instead of using subscriptions; this new way feels easier to work with and seems more intuitive, but I guess it means that I maybe create Tasks where I wouldn’t need to otherwise; let’s see which we prefer working with over time. Resolves #152.
lawrence-forooghian
force-pushed
the
152-implement-async-room-get-spec
branch
from
90f9c55 to
e7612c9
Compare
Note: This PR is based on top of #163; please review that one first.
Implements spec points CHA-RC1f and CHA-RC1g.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores