[ECO-5204] Wrapper SDK proxy #2014
Conversation
WalkthroughThe changes implement a Wrapper SDK Proxy feature. New public and private headers and implementation files have been added to define proxy classes (e.g., ARTWrapperSDKProxyRealtime, ARTWrapperSDKProxyRealtimeChannel, ARTWrapperSDKProxyRealtimeChannels, ARTWrapperSDKProxyOptions) and to extend ARTRealtime with proxy creation methods. Multiple existing methods across the SDK have been updated to accept an extra Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Wrapper SDK Client
participant RT as ARTRealtime
participant Proxy as ARTWrapperSDKProxyRealtime
participant Rest as ARTRest
Client->>RT: createWrapperSDKProxyWithOptions(options)
RT-->>Client: returns Proxy instance
Client->>Proxy: request(method, path, params, body,\nheaders, wrapperSDKAgents)
Proxy->>Rest: executeRequest(request, wrapperSDKAgents)
Rest-->>Proxy: response
Proxy-->>Client: returns response
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (35)
🚧 Files skipped from review as they are similar to previous changes (29)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (45)
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/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
5287a37 to
b60479b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
b60479b to
03c6218
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
03c6218 to
93a2144
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
927ac69 to
7158c90
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
7158c90 to
209035f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
209035f to
7410363
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
1591189 to
80868e5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
cac11ff to
25fdac0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
changed the title
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
25fdac0 to
0cdf8fd
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
0cdf8fd to
aeb0c3e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
aeb0c3e to
d0ee8a5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2014/features
github-actions
bot
temporarily deployed
to
staging/pull/2014/jazzydoc
lawrence-forooghian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
Test/Tests/RestClientTests.swift (1)
785-785: Test coverage for non-nil wrapperSDKAgents is missing.The output confirms that all tests using the
wrapperSDKAgentsparameter (in bothRestClientTests.swiftandRealtimeClientConnectionTests.swift) passnil. There are no test cases that provide non-nil values to exercise the wrapper SDK agents behavior. Consider adding additional tests to confirm that providing a non-nil value triggers the correct functionality.🔗 Analysis chain
Verify test coverage for the new parameter.
The test case
test__014__RestClient__client_should_handle_error_messages_in_plaintext_and_HTML_formathas been updated to include the newwrapperSDKAgentsparameter, but it's passingnil. Consider adding additional test cases to verify the behavior when non-nil wrapper SDK agents are provided.Run this script to check for existing test coverage of the
wrapperSDKAgentsparameter:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases that verify wrapperSDKAgents parameter behavior # Test: Look for test cases that pass non-nil wrapperSDKAgents. Expect: Test cases that verify the parameter's functionality. echo "Searching for test cases using wrapperSDKAgents..." rg -A 5 "wrapperSDKAgents" Test/Tests/Length of output: 3068
🧹 Nitpick comments (22)
Source/ARTRest.m (3)
115-115: Consider allowing user-specified agents here.
Currently,wrapperSDKAgentsis hard-coded asnilwhen invoking_internal request:. If there's a use case for passing custom agents from public ARTRest calls, consider adding a parameter here rather than defaulting tonil.
322-333: Be aware of agent name collisions.
When merging_options.agentswithwrapperSDKAgents, the wrapper agents overwrite any duplicates in_options.agents. Confirm that overwriting is intended or consider a different merge strategy.
549-549: Time request always usesARTAuthenticationOff.
Currently, this omitswrapperSDKAgents. If you intend to collect usage data or track agent info for time requests, consider making the parameter configurable here. Otherwise, this is fine.Ably.xcodeproj/project.pbxproj (11)
1900-1901: Repeated references forAblyPublic.handAblyInternal.h.
Double-check whether these are required for separate build targets or are unintentional duplicates.
2466-2466:ARTWrapperSDKProxyRealtime+Private.hreferences.
Ensure Private headers are not accidentally exposed through a bridging header or combined frameworks.
2578-2579: CheckARTWrapperSDKProxyRealtime.hreferences for multiple build phases.
Duplication might be intentional for multiple platform targets, but remove if unneeded.Also applies to: 2587-2587
2725-2727: Check references forARTWrapperSDKProxyOptions.h,ARTWrapperSDKProxyRealtimeChannel/Channels.h.
Ensure you’re not duplicating references across different PBXGroups.
2776-2776:ARTWrapperSDKProxyRealtime+Private.hreference again.
Ensure that test and internal builds rely on this private header if needed, but it isn’t exposed in distribution.
2839-2839: Another reference forARTRealtime+WrapperSDKProxy.h.
Confirm that you need to declare it again in this section.
3267-3268: Multiple references forARTWrapperSDKProxyRealtimeChannel.mandARTWrapperSDKProxyRealtimeChannels.m.
Potential duplication for multiple build phases. Confirm or remove.
3393-3393: Additional reference forWrapperSDKProxyTests.swift.
Ensure your test plan or schemes pick up these tests.
3528-3528: One more insertion forARTWrapperSDKProxyOptions.m.
Ensure you’re not overshadowing or conflicting with prior references.
3643-3644: Repeated references forARTWrapperSDKProxyRealtimeChannel.mand.mfor Channels.
Verify you only need them once per platform or build configuration.
3670-3670: Extra reference toARTWrapperSDKProxyRealtime.m.
Looks consistent with the rest, but double-check for unneeded duplicates.Source/include/Ably/AblyInternal.h (1)
1-2: Consider enhancing the documentation comment.While the comment clearly states these APIs are for Ably-authored SDKs, it could be more specific about which SDKs (e.g., Chat SDK, other wrapper SDKs) are intended to use these APIs and provide examples.
Source/include/Ably/ARTRealtime+WrapperSDKProxy.h (1)
10-17: Enhance method documentation.While the current documentation is good, consider adding:
- Example usage code
- Description of what analytics information is collected
- Link to ADR-117 for reference
Here's a suggested documentation enhancement:
/** Creates a proxy client to be used to supply analytics information for Ably-authored SDKs. The proxy client shares the state of the `ARTRealtime` instance on which this method is called. - Important: This method should only be called by Ably-authored SDKs. + + This proxy client enables tracking of wrapper SDK usage by appending the wrapper SDK's agents + to the `Ably-Agent` header in REST requests and including them as channel parameters. + + For more information, see ADR-117. + + @param options The options containing the wrapper SDK's agents. + @return A proxy client that shares state with this instance. + + @code + ARTWrapperSDKProxyOptions *options = [[ARTWrapperSDKProxyOptions alloc] + initWithAgents:@{@"chat": @"1.0.0"}]; + ARTWrapperSDKProxyRealtime *proxy = [realtime createWrapperSDKProxyWithOptions:options]; + @endcode */Source/include/Ably/ARTWrapperSDKProxyOptions.h (1)
12-17: Enhance documentation for agents property.The documentation should:
- Include example usage showing how to specify agents with and without versions
- Document the format and structure of the agents dictionary
- Reference where ARTClientInformationAgentNotVersioned is defined
/** * A set of additional entries for the Ably agent header and the `agent` realtime channel param. * * If an agent does not have a version, represent this by using the `ARTClientInformationAgentNotVersioned` pointer as the version. + * + * Example usage: + * ```objc + * NSDictionary *agents = @{ + * @"chat-sdk": @"1.0.0", // Agent with version + * @"wrapper-sdk": ARTClientInformationAgentNotVersioned // Agent without version + * }; + * ARTWrapperSDKProxyOptions *options = [[ARTWrapperSDKProxyOptions alloc] initWithAgents:agents]; + * ``` */Source/ARTWrapperSDKProxyRealtimeChannels.m (1)
41-54: Consider adding nil check for required parameters.The method should validate that both
nameandchannelare non-nil before proceeding.- (ARTWrapperSDKProxyRealtimeChannel *)get:(NSString *)name maybeOptions:(nullable ARTRealtimeChannelOptions *)options { + if (!name) { + return nil; + } ARTRealtimeChannelOptions *resolvedOptions = [self addingAgentToOptions:options]; ARTRealtimeChannel *channel; if (resolvedOptions) { channel = [self.underlyingChannels get:name options:resolvedOptions]; } else { channel = [self.underlyingChannels get:name]; } + if (!channel) { + return nil; + }Source/ARTWrapperSDKProxyRealtime.m (1)
19-29: Consider adding nil checks in initializer.The initializer should validate that neither
realtimenorproxyOptionsare nil.- (instancetype)initWithRealtime:(ARTRealtime *)realtime proxyOptions:(ARTWrapperSDKProxyOptions *)proxyOptions { + if (!realtime || !proxyOptions) { + return nil; + } if (self = [super init]) { _underlyingRealtime = realtime; _proxyOptions = proxyOptions;Source/PrivateHeaders/Ably/ARTRest+Private.h (1)
73-75: Consider adding documentation for the wrapperSDKAgents parameter.Adding documentation would help developers understand the purpose and expected format of the wrapperSDKAgents dictionary.
- (nullable NSObject<ARTCancellable> *)executeRequest:(NSURLRequest *)request + /** Dictionary of wrapper SDK agent information to be included in requests. + Keys and values should be strings identifying the wrapper SDK details. */ wrapperSDKAgents:(nullable NSDictionary<NSString *, NSString *> *)wrapperSDKAgents completion:(nullable ARTURLRequestCallback)callback;Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
55-55: Consider adding documentation for the wrapperSDKAgents parameter.Adding documentation would help developers understand the purpose and expected format of the wrapperSDKAgents dictionary.
+ /** Dictionary of wrapper SDK agent information to be included in requests. + Keys and values should be strings identifying the wrapper SDK details. */ wrapperSDKAgents:(nullable NSStringDictionary *)wrapperSDKAgentsTest/Tests/WrapperSDKProxyTests.swift (1)
208-208: Replace unused parameter with underscore.The
headersparameter in the closure is unused. Replace it with_to address the SwiftLint warning.- ) { response, error in + ) { response, _ in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 208-208: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
Ably.xcodeproj/project.pbxproj(37 hunks)CONTRIBUTING.md(1 hunks)Scripts/jazzy.sh(1 hunks)Source/ARTAuth.m(2 hunks)Source/ARTHTTPPaginatedResponse.m(4 hunks)Source/ARTPaginatedResult.m(1 hunks)Source/ARTPushActivationStateMachine.m(4 hunks)Source/ARTPushAdmin.m(1 hunks)Source/ARTPushChannel.m(4 hunks)Source/ARTPushChannelSubscriptions.m(2 hunks)Source/ARTPushDeviceRegistrations.m(4 hunks)Source/ARTRealtime.m(5 hunks)Source/ARTRest.m(11 hunks)Source/ARTRestChannel.m(2 hunks)Source/ARTWrapperSDKProxyOptions.m(1 hunks)Source/ARTWrapperSDKProxyRealtime.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannels.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientInformation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTHTTPPaginatedResponse+Private.h(3 hunks)Source/PrivateHeaders/Ably/ARTHttp.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRest+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtime+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeChannels+Private.h(1 hunks)Source/include/Ably.modulemap(1 hunks)Source/include/Ably/ARTRealtime+WrapperSDKProxy.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyOptions.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtime.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannels.h(1 hunks)Source/include/Ably/Ably.h(1 hunks)Source/include/Ably/AblyInternal.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Test/Tests/RealtimeClientConnectionTests.swift(4 hunks)Test/Tests/RestClientTests.swift(1 hunks)Test/Tests/WrapperSDKProxyTests.swift(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- Scripts/jazzy.sh
- Source/PrivateHeaders/Ably/ARTHttp.h
- Test/Tests/RealtimeClientConnectionTests.swift
🧰 Additional context used
📓 Learnings (1)
Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeChannel+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~40-~40: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...this header is intended for use only by Ably-authored SDKs and should not be included in the ...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 SwiftLint (0.57.0)
Test/Tests/WrapperSDKProxyTests.swift
[Warning] 208-208: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS17_2)
- GitHub Check: check (iOS, test_iOS17_2)
🔇 Additional comments (80)
Source/ARTRest.m (7)
245-265: Consistent propagation of wrapperSDKAgents.
All branches ofexecuteRequest:withAuthOption:properly forwardwrapperSDKAgentsto the underlying request methods, ensuring consistent agent handling.
268-273: Straightforward pass-through logic.
This overload simply calls the more complex version of the same method, neatly passing alongwrapperSDKAgents.
278-314: Verify concurrency on updating request headers.
Although everything appears correct, these methods dispatch different requests based on token renewal paths. Verify no race conditions occur when multiple threads invoke these methods concurrently.
316-321: Consistent delegation to fallback-based method.
UsingexecuteRequestwith a fresh fallback context ensures robust error handling while supporting the newwrapperSDKAgentsparameter.
343-463: Good handling of fallback logic plus wrapper agents.
PassingwrapperSDKAgentsthrough fallback retry logic is consistent, and preceding code sets the “Ably-Agent” header viaagentIdentifierWithWrapperSDKAgents:. Implementation looks solid.
570-570: New parameter aligns method signature with internal usage.
This addition to the publicrequest:method mirrors the internal structure for wrapperSDKAgents. No issues found.
659-660: Agent handling in paginated responses looks correct.
PropagatingwrapperSDKAgentstoARTHTTPPaginatedResponseensures the same agent context is preserved.Ably.xcodeproj/project.pbxproj (26)
79-87: Check for duplicate build file entries for AblyInternal/AblyPublic headers.
These repeated references (all marked as Public) may be intentional for multi-target builds, but if not, consider consolidating them to avoid confusion.
181-195: Validate new entries forARTWrapperSDKProxyOptionsand related files.
Ensure that making them all Public is intentional, since these options might be for internal use only.
299-316: ConfirmARTWrapperSDKProxyRealtimeChannelandARTWrapperSDKProxyRealtimeChannelsreferences.
They appear multiple times as Public. If you only need them in certain targets, you might scope them accordingly.
1198-1200: Check the file references for AblyInternal.h, AblyPublic.h, and ARTRealtime+WrapperSDKProxy.h.
Redundant entries can cause project file bloat and potential build inconsistencies.
1232-1237: Review new file references forARTWrapperSDKProxyOptionsandARTWrapperSDKProxyRealtime.
All references are Public; confirm whether they need to be exposed externally.
1290-1295: Confirm the new references forARTWrapperSDKProxyRealtimeChannel[+Private].hand.m.
If these headers are meant to be private, switching their attribute to Private can reduce external exposure.
1653-1669: New PBXGroup “Wrapper SDK Proxy” creation.
This grouping is a good organizational step. Just ensure that all relevant header and source references are correctly placed within it.
1816-1816: AddedWrapperSDKProxyTests.swiftto the test group.
No obvious issues, but confirm that your test target references this file.
1914-1914: Added "Wrapper SDK Proxy" group reference.
Seems fine. Ensure the group’s path is correct for all supported platforms (iOS, macOS, tvOS).
2127-2128: File references forARTWrapperSDKProxyOptions.h/.m.
Both are marked as Public in the “Types” group. Verify that public exposure is needed or switch to Private if only used internally.
2338-2338: Added new public headers forARTWrapperSDKProxyOptionsandARTWrapperSDKProxyRealtime.
Check that the public scope matches your distribution strategy and that these are included in the final build artifact properly.Also applies to: 2343-2343, 2347-2347
2385-2386: Private headers forARTWrapperSDKProxyRealtimeChannel/Channels.
Marking them Private is consistent with an internal usage pattern; just confirm all bridging to the public API is covered.
2441-2442: Multiple references forARTWrapperSDKProxyRealtimeChannel.h/.h.
These files are listed as Public multiple times. If you only intend one reference per target, remove duplicates.Also applies to: 2444-2445
2545-2547: Confirm the addition ofARTWrapperSDKProxyOptions.has Public.
If this is purely for advanced usage, consider making it Private or documenting it thoroughly.
2596-2596: Reference to private header in line 2596.
No immediate concerns, just confirm that your test or internal builds include this correctly.
2618-2619: Repeated references toAblyInternal.handAblyPublic.h.
As before, verify that each is necessary.
2659-2659: Added new reference forARTRealtime+WrapperSDKProxy.h.
If this extension should be widely available, Public is correct. Otherwise, consider scoping it to internal usage only.
2758-2759: Check multiple references forARTWrapperSDKProxyRealtime.h.
Consolidate if possible, or confirm multiple target requirements.Also applies to: 2767-2767
2798-2799: Repeated references forAblyInternal.handAblyPublic.h.
Check if your specific build phases or target conditions genuinely require duplication.
3177-3177:WrapperSDKProxyTests.swiftin Sources.
Looks fine; confirm that the test target includes this file or if it belongs in a dedicated test group.
3280-3280: Check reference forARTWrapperSDKProxyOptions.m.
No immediate issues; confirm that build phases have this properly assigned.
3294-3294: One more reference forARTWrapperSDKProxyRealtime.m.
Stay consistent across all references. Consider combining them if they belong to the same build configuration.
3453-3453:WrapperSDKProxyTests.swiftin another PBXGroup.
Double-check that you don’t have conflicting references for the same file.
3515-3516: Check references forARTWrapperSDKProxyRealtimeChannel.mandARTWrapperSDKProxyRealtimeChannels.m.
Likely repeated for a different build configuration. If so, keep them; otherwise, remove duplicates.
3542-3542: Reference forARTWrapperSDKProxyRealtime.m.
Confirm you are listing it only under the relevant build phases (e.g., iOS/macOS).
3656-3656:ARTWrapperSDKProxyOptions.mreference again.
If you intend each build variant to include it, keep it. Otherwise, consolidate.Source/ARTWrapperSDKProxyOptions.m (1)
5-11: LGTM! Well-structured initialization with proper copy semantics.The implementation correctly:
- Follows the two-stage initialization pattern
- Uses copy semantics for the agents dictionary to prevent external mutations
- Handles nullable input appropriately
Source/include/Ably/Ably.h (1)
15-16: LGTM! Clean separation of public and internal APIs.The consolidation of imports into
AblyPublic.handAblyInternal.himproves the organization and maintainability of the SDK's API surface.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtime+Private.h (1)
7-13: LGTM! Well-designed private interface with proper initialization constraints.The interface correctly:
- Declares a designated initializer with required dependencies
- Prevents incorrect initialization by marking default init as unavailable
- Uses proper nullability annotations
Source/include/Ably/AblyInternal.h (1)
3-7: LGTM! Comprehensive inclusion of wrapper SDK proxy components.The imports include all necessary components for implementing wrapper SDK tracking as outlined in ADR-117.
Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeChannel+Private.h (1)
1-16: LGTM! Well-structured initialization pattern.The implementation follows best practices:
- Proper use of forward declarations
- Correct nullability annotations
- Well-defined initialization pattern with unavailable default initializer and designated initializer
Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeChannels+Private.h (1)
1-16: LGTM! Consistent implementation pattern.The implementation maintains consistency with ARTWrapperSDKProxyRealtimeChannel+Private.h and follows the same best practices.
Source/PrivateHeaders/Ably/ARTClientInformation+Private.h (1)
12-13: LGTM! Clear and well-documented API addition.The new method is well-documented and follows consistent naming patterns with existing methods in the class.
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h (1)
8-12: LGTM! Clear documentation of purpose and usage restrictions.The documentation clearly states the purpose of the class and correctly emphasizes that it's for Ably-authored SDKs only.
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannels.h (1)
8-12: LGTM! Clear documentation of purpose and usage restrictions.The documentation clearly states the purpose of the class and correctly emphasizes that it's for Ably-authored SDKs only.
Source/include/Ably/ARTWrapperSDKProxyRealtime.h (1)
11-15: LGTM! Clear documentation of purpose and usage restrictions.The documentation clearly states the purpose of the class and correctly emphasizes that it's for Ably-authored SDKs only.
Source/PrivateHeaders/Ably/ARTHTTPPaginatedResponse+Private.h (3)
12-12: LGTM! Property declaration looks good.The property is correctly declared as nullable and readonly, with appropriate type specification.
14-22: LGTM! Method signature update is well-structured.The initializer method signature is updated correctly to include the wrapperSDKAgents parameter while maintaining proper nullability annotations.
29-33: LGTM! Method signature replacement is appropriate.The new method signature correctly replaces the deprecated one and includes proper parameter annotations.
Source/include/Ably/AblyPublic.h (1)
1-49: LGTM! Comprehensive and well-organized public header imports.The imports are logically grouped and include all necessary components for the SDK's public interface.
Source/ARTWrapperSDKProxyRealtimeChannels.m (2)
11-12: LGTM! Properties are properly declared.Properties are correctly declared as readonly with appropriate memory management semantics.
56-68: LGTM! Robust options handling.The method properly handles both nil and non-nil cases for options and agents, with clean parameter resolution.
Source/ARTWrapperSDKProxyRealtime.m (1)
65-80: LGTM! Request method properly handles wrapper SDK agents.The implementation correctly forwards the agents from proxy options to the underlying request.
Source/ARTPushAdmin.m (1)
85-86: LGTM!The addition of
wrapperSDKAgents:nilparameter aligns with the broader SDK changes for wrapper SDK tracking.Source/Ably.modulemap (1)
115-117: LGTM!The new private headers for wrapper SDK proxy functionality are correctly placed and follow the existing naming conventions.
Source/PrivateHeaders/Ably/ARTRest+Private.h (1)
73-75: LGTM!The addition of
wrapperSDKAgentsparameter is consistent across methods and uses appropriate types.Also applies to: 79-82
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
50-57: LGTM!The addition of
wrapperSDKAgentsparameter is consistent with other files and uses appropriate types.Source/include/Ably.modulemap (1)
113-115: LGTM!The new private headers for the wrapper SDK proxy feature are correctly placed in the
Privatemodule and follow the existing naming convention.Source/ARTHTTPPaginatedResponse.m (2)
23-28: LGTM!The wrapper SDK agents parameter is correctly added to the initializer and stored as a copy, maintaining immutability.
65-65: LGTM!The wrapper SDK agents are correctly propagated through the pagination chain, ensuring consistent behavior across paginated requests.
Also applies to: 86-86, 96-96
Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
16-23: LGTM!The initialization is well-implemented with proper property initialization and immutability.
Source/ARTPushChannelSubscriptions.m (2)
93-93: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
210-210: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.Source/ARTPushChannel.m (4)
132-132: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
166-166: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
203-203: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
238-238: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.Source/ARTPushDeviceRegistrations.m (4)
94-94: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
142-142: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
215-215: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.
260-260: LGTM!The addition of
wrapperSDKAgents:nilparameter is consistent with the API changes for wrapper SDK support.Test/Tests/WrapperSDKProxyTests.swift (4)
8-73: Well-structured connection state tests!The test cases effectively validate the connection state sharing and subscription management between the proxy and underlying client. Good use of cleanup and clear documentation of test flow.
77-181: Comprehensive channel state and subscription tests!The test cases thoroughly validate the channel state sharing and subscription management between the proxy and underlying channel. Good coverage of both state changes and message handling.
185-320: Well-implemented request handling tests!The test cases effectively validate the addition of wrapper SDK agent information to request headers, including proper handling in paginated requests.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 208-208: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
324-404: Well-designed agent channel parameter tests!The test cases effectively validate the agent parameter handling in various scenarios using a clean parameterized test approach. Good coverage of different channel option combinations.
Source/ARTRestChannel.m (1)
234-234: LGTM!The addition of
wrapperSDKAgents:nilparameter to theexecuteRequestcalls is consistent with the PR objectives and maintains proper request handling.Also applies to: 364-364
Source/ARTPushActivationStateMachine.m (1)
199-199: LGTM!The addition of
wrapperSDKAgents:nilparameter to theexecuteRequestcalls in push-related methods is consistent with the PR objectives and maintains proper request handling.Also applies to: 265-265, 313-313, 368-368
Source/ARTAuth.m (1)
373-373: LGTM!The addition of
wrapperSDKAgents:nilparameter to theexecuteRequestcalls in authentication methods is consistent with the PR objectives and maintains proper request handling.Also applies to: 511-511
Source/ARTRealtime.m (3)
7-7: LGTM!The addition of imports for wrapper SDK proxy functionality is appropriate and necessary for the new feature.
Also applies to: 56-56
187-195: LGTM!The new category implementation cleanly extends
ARTRealtimewith wrapper SDK proxy functionality, following good Objective-C practices.
507-516: LGTM!The addition of
wrapperSDKAgentsparameter to therequestmethod is consistent with the PR objectives and maintains proper request handling.Test/Tests/RestClientTests.swift (1)
785-785: LGTM! Test updated for new method signature.The test has been correctly updated to include the new
wrapperSDKAgentsparameter, appropriately set tonilas this test case doesn't involve wrapper SDK functionality.CONTRIBUTING.md (1)
38-40: LGTM! Clear guidance for header organization.The updated instructions provide clear guidance on organizing public headers based on their intended usage:
AblyPublic.hfor general-use APIsAblyInternal.hfor Ably-authored SDK-only APIsThis separation helps maintain clear boundaries between public and internal interfaces.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~40-~40: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...this header is intended for use only by Ably-authored SDKs and should not be included in the ...(HYPHENATED_LY_ADVERB_ADJECTIVE)
Part of #2020; that is, implementing ADR-117 [1]. This adds a -createWrapperSDKProxyWithOptions: method to ARTRealtime. This method is only to be called by Ably-authored wrapper SDKs and accepts a set of agents describing the wrapper SDK. It returns a proxy object that wraps the underlying ARTRealtime instance. The intention is that when certain methods are called on this proxy, the proxy will insert information that allows us to track the usage of the wrapper SDK. I'll implement this behaviour in subsequent commits. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued
Part of #2020; that is, implementing ADR-117 [1]. When -request is called on an ARTWrapperSDKProxyRealtime, add the wrapper SDK's agents to the Ably-Agent header. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued
This extends the change in b006650 to also apply to loading subsequent pages.
Part of #2020; that is, implementing ADR-117 [1]. There are no functional changes here; it's preparation for adding the `agent` channel param when a channel is fetched via an ARTWrapperSDKProxyRealtime. I initially implemented this with internal storage of proxy instances to ensure that there be only one proxy instance per underlying channel. However, it wasn't really clear to me _why_ I did this, other than for some sort of symmetry with the underlying ARTRealtimeChannels behaviour. So it didn't really seem worth the time of trying to figure out the right behaviour. So I decided to not implement this behaviour in the end, instead creating a new proxy instance each time -get:… is called. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued
Part of #2020; that is, implementing ADR-117 [1]. When a realtime channel is fetched via an ARTWrapperSDKProxyRealtime, add the `agent` channel param so that the channel attach analytics event gets attributed to the wrapper SDK. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
d0ee8a5 to
58bbd14
Compare
This implements as much of ADR-117: Tracking wrapper SDK usage, continued as is needed for the Chat SDK. That is:
-createWrapperSDKProxyWithOptions:method toARTRealtimeARTRealtimeinstanceARTRealtimeinstance-request:…on the proxy instance, it adds the wrapper SDK's agents to theAbly-Agentheader-get:…on the proxy instance, it sets anagentchannel param containing the wrapper SDK's agentsably/ably-chat-swift#211 demonstrates this new API being used in the Chat SDK.
I'm putting this up for review to get opinions on it; remaining work includes:
ARTRest, adding to theAbly-Agentheader for all of the other REST methods that ADR-117 says we should cover)Summary by CodeRabbit
New Features
Documentation
Tests
WrapperSDKProxyTeststo ensure correct behavior of the wrapper SDK proxy client.