[ECO-4968] Added NS_SWIFT_SENDABLE to all query types
#1973
Conversation
WalkthroughThe pull request introduces the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1973/features
github-actions
bot
temporarily deployed
to
staging/pull/1973/jazzydoc
![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
Outside diff range and nitpick comments (1)
Source/ARTDataQuery.m (1)
Line range hint
100-131: LGTM, but consider adding documentation comments.The new instance variable
_untilAttachand its corresponding getter and setter methods are implemented correctly in theARTRealtimeHistoryQueryclass. The setter method includes the necessary safeguard to prevent changes when the object is frozen, and the exception provides a clear reason for the failure.However, the purpose of the
untilAttachproperty is not immediately clear from its name. Consider adding documentation comments to clarify its behavior and usage.+ /** + * Whether the query should continue until an attachment occurs. + */ - (BOOL)untilAttach { return _untilAttach; } + /** + * Sets whether the query should continue until an attachment occurs. + * + * @param value Whether the query should continue until an attachment occurs. + * @throws NSObjectInaccessibleException If the query is frozen. + */ - (void)setUntilAttach:(BOOL)value { ... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- Ably.xcodeproj/project.pbxproj (9 hunks)
- Source/ARTBaseQuery.m (1 hunks)
- Source/ARTDataQuery.m (3 hunks)
- Source/ARTRealtimePresence.m (3 hunks)
- Source/ARTRest.m (1 hunks)
- Source/ARTRestChannel.m (1 hunks)
- Source/ARTRestPresence.m (4 hunks)
- Source/ARTStats.m (2 hunks)
- Source/Ably.modulemap (1 hunks)
- Source/include/Ably/ARTBaseQuery.h (1 hunks)
- Source/include/Ably/ARTDataQuery.h (3 hunks)
- Source/include/Ably/ARTRealtimePresence.h (1 hunks)
- Source/include/Ably/ARTRestPresence.h (2 hunks)
- Source/include/Ably/ARTStats.h (1 hunks)
- Test/Tests/RealtimeClientChannelTests.swift (1 hunks)
- Test/Tests/RealtimeClientPresenceTests.swift (2 hunks)
- Test/Tests/RestClientChannelTests.swift (1 hunks)
- Test/Tests/RestClientPresenceTests.swift (1 hunks)
- Test/Tests/RestClientStatsTests.swift (1 hunks)
Files skipped from review due to trivial changes (1)
- Source/ARTBaseQuery.m
Additional comments not posted (43)
Source/include/Ably/ARTDataQuery.h (4)
2-2: LGTM!The import statement is correct and consistent with the change in the base class of
ARTDataQuery.
17-17: LGTM!The addition of the
NS_SWIFT_SENDABLEattribute toARTDataQueryis consistent with the PR objective to enhance compatibility with Swift's concurrency model. This change allows instances ofARTDataQueryto be safely used across Swift concurrency domains.
45-45: LGTM!The addition of the
NS_SWIFT_SENDABLEattribute toARTRealtimeHistoryQueryis consistent with the PR objective to enhance compatibility with Swift's concurrency model. This change allows instances ofARTRealtimeHistoryQueryto be safely used across Swift concurrency domains.
18-18: Verify the impact of inheriting fromARTBaseQuery.The change in the base class of
ARTDataQueryfromNSObjecttoARTBaseQuerysuggests a refactoring effort to leverage common functionality from a base query class. While this is a good approach to promote code reuse and maintainability, it's crucial to ensure that this change doesn't break existing functionality or introduce unexpected behavior.Please run the following script to verify the impact of this change:
Source/include/Ably/ARTRestPresence.h (2)
5-5: LGTM!The import statement is necessary and correctly placed to support the change in the inheritance structure of
ARTPresenceQueryclass.
14-15: Verify the impact of the changes.The modifications to the
ARTPresenceQueryclass declaration look good! The changes align with the PR objectives and enhance the functionality of the class.However, please ensure that:
- The change in the inheritance structure does not introduce any unintended side effects or break existing functionality.
- All the necessary adjustments are made in the implementation file (
ARTRestPresence.m) to accommodate the new inheritance structure.- The addition of the
NS_SWIFT_SENDABLEattribute does not lead to any unexpected behavior or performance issues when used in Swift code.Run the following script to verify the impact of the changes:
Please review the results of the above tests and confirm if any additional changes are required to ensure the stability and correctness of the codebase.
Verification successful
Changes verified successfully.
The modifications to the
ARTPresenceQueryclass, including the change in inheritance and the addition of theNS_SWIFT_SENDABLEattribute, have been verified. The usage and initialization patterns are consistent with the rest of the codebase, and no issues or unintended side effects were found. The changes align well with the PR objectives and enhance the functionality of the class.
- The
ARTPresenceQueryclass is used appropriately in both Objective-C and Swift code.- The inheritance from
ARTBaseQueryis consistent with other query classes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the changes to `ARTPresenceQuery` class. echo "Checking the usage of ARTPresenceQuery..." # Test 1: Search for the usage of `ARTPresenceQuery`. # Expect: Occurrences in the implementation file and Swift code (if any). rg --type objc --type swift -w $'ARTPresenceQuery' echo "Checking the initialization of ARTPresenceQuery..." # Test 2: Search for the initialization of `ARTPresenceQuery`. # Expect: Initialization using the designated initializers declared in the header file. rg --type objc --type swift -w $'ARTPresenceQuery alloc] init' echo "Checking the usage of ARTBaseQuery..." # Test 3: Search for the usage of `ARTBaseQuery`. # Expect: Occurrences in other query classes (if any). rg --type objc --type swift -w $'ARTBaseQuery'Length of output: 2445
Source/Ably.modulemap (1)
17-17: LGTM!The addition of
ARTBaseQuery.hto thePrivatemodule of theAblyframework appears to be a valid change that aligns with the PR objective of adding Swift concurrency annotations to query types. This change expands the module's internal capabilities related to query handling, as indicated by the AI-generated summary.Source/ARTDataQuery.m (4)
4-9: LGTM!The new instance variables are declared correctly and follow good encapsulation practices by keeping them private to the class.
46-57: LGTM!The getter and setter methods for the
startproperty are implemented correctly. The setter method includes the necessary safeguard to prevent changes when the object is frozen, and the exception provides a clear reason for the failure.
72-83: LGTM!The getter and setter methods for the
limitproperty are implemented correctly. The setter method includes the necessary safeguard to prevent changes when the object is frozen, and the exception provides a clear reason for the failure.
85-96: LGTM!The getter and setter methods for the
directionproperty are implemented correctly. The setter method includes the necessary safeguard to prevent changes when the object is frozen, and the exception provides a clear reason for the failure.Source/ARTStats.m (3)
4-6: LGTM!The addition of the
_unitinstance variable to theARTStatsQueryclass enhances its functionality by allowing the specification of a granularity unit for statistics queries. The default initialization toARTStatsGranularityMinutein theinitmethod provides a sensible default value. The code changes are consistent with the existing codebase and follow the Objective-C naming conventions.
39-41: LGTM!The
unitmethod provides a getter for the_unitinstance variable, allowing external code to retrieve the current granularity unit of the query. The method follows the Objective-C naming convention for getter methods and is simple and straightforward.
43-50: LGTM!The
setUnit:method provides a setter for the_unitinstance variable, allowing external code to modify the granularity unit of the query. The method follows the Objective-C naming convention for setter methods.The check to prevent modification of a frozen query object is important to maintain the integrity of the query once it has been passed to a receiver. The exception thrown in case of an attempt to modify a frozen query provides a clear and informative error message.
Source/ARTRestPresence.m (6)
17-21: LGTM!The addition of the new instance variables
_limit,_clientId, and_connectionIdto theARTPresenceQueryclass aligns with the PR objectives. The variables are declared as private, which is a good practice for encapsulation, and the naming convention is consistent with the existing codebase.
56-67: LGTM!The getter and setter methods for the
limitproperty provide a clean interface for accessing and modifying the property. The check in the setter method to prevent modifications after the query is frozen is a good practice to maintain the integrity of the query, and the exception thrown provides a clear explanation of the error.
69-80: LGTM!The getter and setter methods for the
clientIdproperty provide a clean interface for accessing and modifying the property. The check in the setter method to prevent modifications after the query is frozen is a good practice to maintain the integrity of the query, and the exception thrown provides a clear explanation of the error.
82-93: LGTM!The getter and setter methods for the
connectionIdproperty provide a clean interface for accessing and modifying the property. The check in the setter method to prevent modifications after the query is frozen is a good practice to maintain the integrity of the query, and the exception thrown provides a clear explanation of the error.
176-177: LGTM!Setting the
frozenstate of thequerytotrueafter it is used in thegetmethod is a good practice to ensure that the query parameters cannot be altered post-processing. This change aligns with the PR objectives of enhancing the robustness of theARTPresenceQueryclass by enforcing stricter control over its state and parameters.
223-224: LGTM!Setting the
frozenstate of thequerytotrueafter it is used in thehistorymethod is a good practice to ensure that the query parameters cannot be altered post-processing. This change aligns with the PR objectives of enhancing the robustness of theARTPresenceQueryclass by enforcing stricter control over its state and parameters.Source/include/Ably/ARTRealtimePresence.h (1)
13-13: LGTM!The addition of the
NS_SWIFT_SENDABLEannotation to theARTRealtimePresenceQueryinterface is a valid change that enhances compatibility with Swift's concurrency model. It aligns with the PR objectives and does not alter the existing functionality or logic of the interface.Source/include/Ably/ARTStats.h (1)
33-33: LGTM!The addition of the
NS_SWIFT_SENDABLEattribute to theARTStatsQueryinterface is a valid change that aligns with the PR objectives. It ensures that instances of this type can be safely used across concurrency domains in Swift, enhancing compatibility with Swift's concurrency model.Test/Tests/RestClientStatsTests.swift (1)
114-118: LGTM!The test case is well-structured and follows the given-when-then pattern. It is focused on a specific behavior and has clear assertions. The use of
tryInObjCto capture the Objective-C exception and theXCTAssertNotNilandXCTAssertEqualassertions to verify the expected behavior are good practices.Source/ARTRestChannel.m (1)
164-165: LGTM!Freezing the
queryobject after this point is a good practice to ensure immutability and thread-safety.Verify if the
frozenproperty is part of the public API ofARTDataQuery. If so, consider adding documentation to explain its purpose and behavior.Verification successful
Confirm
frozenProperty as Public APIThe
frozenproperty is part of the public API ofARTDataQuerythrough its inheritance fromARTBaseQuery. Consider adding documentation to explain its purpose and behavior, as it ensures the immutability of the query object.
Source/include/Ably/ARTBaseQuery.h:@property (nonatomic, getter=isFrozen) BOOL frozen;Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `frozen` is a public property of `ARTDataQuery` and suggest adding documentation if necessary. # Test: Search for the declaration of the `frozen` property in `ARTDataQuery`. # Expect: The property to be declared in the header file, indicating it's part of the public API. rg --type objc $'@property.*frozen' -- '*.h'Length of output: 89
Script:
#!/bin/bash # Locate the definition of the `ARTDataQuery` class to find the correct file. ast-grep --lang objc --pattern 'interface ARTDataQuery { $$$ }' # Once the file is located, search for the `frozen` property declaration within it. # This will help determine if `frozen` is part of the public API. fd -e h | xargs rg --type objc $'@property.*frozen'Length of output: 542
Script:
#!/bin/bash # Search for the definition of `ARTDataQuery` to check its inheritance hierarchy. rg --type objc 'interface ARTDataQuery' -A 5Length of output: 845
Test/Tests/RestClientPresenceTests.swift (1)
78-99: LGTM!The test case is well-structured and comprehensively covers the expected behavior of the
limitproperty on theARTPresenceQueryclass. The addition of the exception handling tests for the frozenqueryobject aligns with the PR objective of adding theNS_SWIFT_SENDABLEannotation to query types.I particularly like the improvement in the test case where a new instance of
ARTPresenceQuery(query2) is used for the successful call tochannel.presence.get(query)with a limit of 1000, instead of reusing the frozenqueryobject. This ensures that the test accurately reflects the expected behavior when the query is not frozen.Source/ARTRest.m (1)
670-671: LGTM!Freezing the
queryobject before validating the limit and executing the request is a good practice to ensure the query remains unmodified throughout thestatsmethod. This change enhances the robustness and predictability of the method.Source/ARTRealtimePresence.m (3)
34-36: LGTM!The
waitForSyncmethod provides public read access to the private_waitForSyncinstance variable. This flag seems to be used to control whether the query should wait for synchronization before returning results.
38-45: LGTM!The
setWaitForSync:method allows setting the value of the private_waitForSyncflag. The check to prevent modification of a frozen query is a good practice to ensure immutability and avoid unexpected behavior. Throwing an exception with a clear message is an appropriate way to handle the case when attempting to modify a frozen query.
239-239: LGTM!Setting the
frozenproperty totrueensures that the query becomes immutable after being passed to thegetmethod. This aligns with the immutability check introduced in thesetWaitForSync:method and helps maintain a consistent state throughout the processing of thegetrequest.Test/Tests/RestClientChannelTests.swift (6)
595-618: LGTM!The test covers the scenario described in the linked issue #1074 and verifies that publishing messages with no clientId in the client options and credentials that can assume any clientId does not fail.
Line range hint
619-630: LGTM!The test verifies that publishing with a mismatched clientId fails with an expected error message.
Line range hint
631-694: LGTM!The test verifies that the extras are correctly included when publishing a message and can be retrieved from history.
Line range hint
695-715: LGTM!The test verifies that the attributes supplied by the caller, such as id, name, and data, are correctly included in the encoded message when publishing.
Line range hint
716-751: LGTM!The test verifies that the channel status API returns a channel details object populated with the expected channel metrics when there are subscribers. It checks metrics such as connections, publishers, subscribers, presenceMembers, presenceConnections, and presenceSubscribers.
Line range hint
752-771: LGTM!The test verifies that when a channel name contains a slash character, it is correctly URL-encoded in the REST request path. This ensures that the slash is properly handled and does not cause issues with the request URL.
Test/Tests/RealtimeClientPresenceTests.swift (2)
3248-3252: LGTM!Good test case to verify that the
waitForSyncproperty ofARTRealtimePresenceQuerycannot be modified after initialization, as expected. The object should be frozen.
3555-3561: Looks good!Another good test to ensure the
untilAttachproperty ofARTRealtimeHistoryQueryis also frozen after initialization, matching the immutability semantics tested forwaitForSyncabove.Test/Tests/RealtimeClientChannelTests.swift (1)
3664-3669: Test looks good!The test correctly verifies that when publishing a message with both name and data set:
- The client connects and reaches the connected state
- The published message's name and data are sent in the ProtocolMessage
- The ProtocolMessage also contains the correct connectionId
Ably.xcodeproj/project.pbxproj (5)
302-304: LGTM!Adding
ARTBaseQuery.hto the "Headers" build phase with the private attribute is the correct way to include it in the project.
305-307: LGTM!Adding
ARTBaseQuery.mto the "Sources" build phase is the correct way to include it in the project.
1254-1255: LGTM!The file references for
ARTBaseQuery.handARTBaseQuery.mare correctly added. The header file is placed in theinclude/Ablydirectory, and the implementation file is at the root level, which follows the project's file structure conventions.
2136-2137: LGTM!Adding
ARTBaseQuery.handARTBaseQuery.mto this group of files is appropriate, as it keeps related files together in the project structure.
2245-2245: LGTM!Adding
ARTBaseQuery.hto the "Headers" build phase is consistent with the previous changes and ensures that the header file is included in the appropriate build phase.
…code 16) I've added `NS_SWIFT_SENDABLE` to other query types as well. Also added NS_SWIFT_SENDABLE to `ARTChannelProperties`.
maratal
force-pushed
the
fix/1971-swift-concurrency-annotations
branch
from
179db8e
to
bfaaba1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1973/features
github-actions
bot
temporarily deployed
to
staging/pull/1973/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/1973/features
github-actions
bot
temporarily deployed
to
staging/pull/1973/jazzydoc
|
|
github-actions
bot
temporarily deployed
to
staging/pull/1973/features
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
Test/Tests/RestClientChannelsTests.swift (1)
6-6: Improve the code comment for clarityConsider rephrasing the comment to be more informative and professional. Instead of stating that "Swift isn't yet smart enough," you might specify the limitation or refer to the issue with bridging Objective-C APIs in Swift versions prior to Swift 6.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Source/ARTDataEncoder.m (2 hunks)
- Test/Test Utilities/TestUtilities.swift (2 hunks)
- Test/Tests/RealtimeClientChannelsTests.swift (1 hunks)
- Test/Tests/RestClientChannelsTests.swift (1 hunks)
Additional comments not posted (2)
Test/Tests/RestClientChannelsTests.swift (1)
5-11: Conditional conformance implementation is appropriateThe use of conditional compilation to provide retroactive conformance to
Sequencefor Swift versions 6 and above is a good approach to maintain compatibility across different Swift versions.Test/Tests/RealtimeClientChannelsTests.swift (1)
5-12: Appropriate use of@retroactiveattribute for Swift 6 and aboveThe conditional compilation correctly applies the
@retroactiveattribute to theSequenceconformance for Swift versions 6 and above. This ensures compatibility with the latest Swift features while maintaining functionality for earlier versions of Swift.Also applies to: 19-19
github-actions
bot
temporarily deployed
to
staging/pull/1973/jazzydoc
maratal
force-pushed
the
fix/1971-swift-concurrency-annotations
branch
from
42e5ba9
to
75e9554
Compare
|
LGTM - I checked this against the sending and receiving messages work and I have no warnings 🎉 worth checking if @lawrence-forooghian wants to give it another once over |
By analogy with
ARTRealtimePresenceQuery(which causes warning in Xcode 16) I've addedNS_SWIFT_SENDABLEto other query types as well.Closes #1971
Summary by CodeRabbit
Release Notes
New Features
ARTBaseQuerycomponent, enhancing query capabilities within the Ably framework.Bug Fixes
nilvalues during encoding and decoding processes to prevent unexpected behavior.Tests