[ECO-5551] Use endpoint as default connection option (ADR-119)#2109
[ECO-5551] Use endpoint as default connection option (ADR-119)#2109
Conversation
WalkthroughReplaces legacy environment/host routing with an endpoint-centric model: introduces ARTDomainSelector, adds ARTClientOptions.endpoint/connectivityCheckUrl, removes ARTFallbackHosts, and updates REST/Realtime/WebSocket/components and tests to resolve primaryDomain and fallbackDomains via the new selector. Changes
sequenceDiagram
participant Client as Client / Tests
participant Options as ARTClientOptions
participant Selector as ARTDomainSelector
participant Rest as ARTRest
participant Realtime as ARTRealtime
Note over Client,Options: Configure options (endpoint or legacy fields)
Client->>Options: set endpoint / restHost / realtimeHost / fallbackHosts
Options->>Selector: initWithEndpointClientOption(..., fallbackHostsUseDefault: ...)
Selector-->>Options: primaryDomain / fallbackDomains
Client->>Rest: REST request (host = Selector.primaryDomain)
Rest->>Selector: on failure -> get fallbackDomains
alt fallback available
Rest->>Rest: switch host -> next fallback and retry
Rest->>Client: return result
else no fallback
Rest->>Client: report no-fallback error
end
Client->>Realtime: open realtime (host = Selector.primaryDomain)
Realtime->>Selector: on transport failure -> get fallbackDomains
Realtime->>Realtime: reconnect using fallback hosts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Comment |
a9e9148 to
5512392
Compare
5512392 to
0cd88a0
Compare
3795b94 to
c0284d8
Compare
86766ef to
7051593
Compare
7051593 to
c016fd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Source/ARTClientInformation.m (1)
9-10: Avoid hard‑codingARTClientInformationLibraryVersion
ARTClientInformationLibraryVersionis still a hard‑coded string (@"1.2.53") and is now referenced both bylibraryAgentandARTDefault.libraryVersion. This risks the agent string drifting from the actual package/framework version over time.Consider deriving this constant from a single source of truth (e.g. build settings or generated header from Version.xcconfig) so you don’t have to touch code/tests every time the version changes.
Also applies to: 55-57, 21-23
🧹 Nitpick comments (3)
Source/include/Ably/ARTDefault.h (1)
13-17: Clarify publicrestHost/realtimeHostsemantics with brief docsThe public reintroduction/cleanup of
+restHostand+realtimeHostonARTDefaultlooks fine and matches expected Obj‑C API style. Given they now sit on top of the new endpoint/domain selection, consider adding short doc comments to clarify:
- Whether these represent the effective primary REST/Realtime hosts after applying endpoint/environment.
- That they are primarily for legacy/diagnostic use and may be superseded by endpoint‑based configuration.
This would help avoid confusion for integrators reading the headers, especially around how these interact with
endpointvs legacy options.Source/ARTDomainSelector.m (1)
16-34: Endpoint classification should handle all IPv6 literals, not just those containing::The overall REC1/REC2 mapping here (routing policies vs legacy environment/hosts and the generated fallback domains) looks consistent and well factored. One edge case is how
endpointClientOptionis classified:if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || [endpointClientOption isEqualToString:@"localhost"]) { // Hostname/IP }This treats IPv4, most FQDNs, and IPv6 addresses containing
::as “hostname” endpoints, but fully expanded IPv6 literals that don’t include::(e.g.2001:0db8:0000:0000:0000:0000:0000:0001) will fall through and be treated as routing policy IDs, yielding invalid primary/fallback domains.Consider centralizing the “is this an IP/FQDN vs routing policy ID?” check and making it robust for all IPv4/IPv6 forms (e.g. by normalizing host:port/brackets and validating with inet_pton, or by reusing/improving any existing helper in ARTClientOptions). That will keep
endpointsemantics predictable across all allowed endpoint types.Also applies to: 66-118, 120-214
Source/ARTClientOptions.m (1)
22-38: Endpoint vs legacy host/environment integration is sound, but copying via the setter is a bit fragileThe new endpoint plumbing looks good overall:
setEndpoint:forbids mixingendpointwithenvironmentorrestHost/realtimeHost(REC1b1), and the legacy setters similarly reject being used whenendpointis non‑empty or when environment/host combinations are invalid (REC1c1).domainSelectoris lazily constructed from the current_endpoint,_environment,_restHost,_realtimeHost,_fallbackHosts, and_fallbackHostsUseDefault, and is reset whenever any of those options change. UsingprimaryDomain/fallbackDomainsas the single source of truth simplifies host logic across REST and Realtime.isProductionEnvironment/hasEnvironmentare consistent with the newARTDefaultProductionEnvironment.One subtlety:
copyWithZone:later callsoptions.endpoint = self.endpoint;. That will still execute the “endpoint cannot be used with legacy options” guard even whenself.endpointisnil. Today this is safe because the copied instance starts with no environment/host configured, but it does couple correctness of copying to the implementation details ofsuper.copyWithZone:. To future‑proof this, you might consider:
- Skipping the setter when
self.endpointisnil/empty and assigning_endpointdirectly in the copy, or- Adjusting
setEndpoint:to treat anil/empty value as a no‑op that doesn’t run the legacy‑option guard.Also applies to: 48-82, 88-126, 130-191, 331-343
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientInformation.m(2 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(2 hunks)Source/ARTRest.m(6 hunks)Source/ARTWebSocketTransport.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientInformation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (6)
- Source/ARTWebSocketTransport.m
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/module.modulemap
- Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
- .github/workflows/integration-test.yaml
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
🧰 Additional context used
🧬 Code graph analysis (3)
Test/AblyTests/Tests/RestClientTests.swift (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (5)
match(1516-1524)getTestEndpoint(626-635)at(1431-1433)at(1440-1445)extract(1526-1537)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
Test/AblyTests/Tests/RestClientTests.swift (13)
deprecated(302-314)deprecated(317-330)deprecated(354-364)deprecated(367-374)deprecated(377-382)deprecated(385-390)deprecated(418-424)deprecated(865-894)deprecated(896-900)deprecated(903-930)deprecated(1014-1039)deprecated(1098-1122)deprecated(1226-1235)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (5)
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)Test/AblyTests/Tests/RestClientTests.swift (15)
deprecated(302-314)deprecated(317-330)deprecated(354-364)deprecated(367-374)deprecated(377-382)deprecated(385-390)deprecated(418-424)deprecated(865-894)deprecated(896-900)deprecated(903-930)deprecated(1014-1039)deprecated(1098-1122)deprecated(1226-1235)deprecated(1238-1244)deprecated(1247-1253)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)Test/AblyTests/Test Utilities/TestProxyTransportFactory.swift (1)
transport(12-24)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
🔇 Additional comments (34)
Source/Ably.modulemap (1)
140-143: ARTDomainSelector correctly exposed via Private moduleAdding
ARTDomainSelector.hto theAbly.Privatemodule keeps the new domain-selection logic internal while making it available to tests and internal callers. This looks consistent with the rest of the private header list and with the ARTFallbackHosts → ARTDomainSelector migration.Source/PrivateHeaders/Ably/ARTClientInformation+Private.h (1)
5-5: All references properly updated; export macro suggestion not applicable for private headersThe constant rename from
ARTClientInformation_libraryVersiontoARTClientInformationLibraryVersionis complete and correct:
- No stale references to the old name remain in the codebase
- The new constant is properly declared and used across 4 locations (header declaration, implementation definition, and 2 usages)
- The declaration style (
extern NSString *const) is consistent with the SDK's convention for private headers; public headers ininclude/Ably/useFOUNDATION_EXPORT, while private headers inPrivateHeaders/useexternwithout the macroThe suggestion about export macro alignment does not apply here, as this constant resides in a private header and already follows the established SDK pattern.
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is correctly wired across all targets
ARTDomainSelector.m/ARTDomainSelector.hhave matchingPBXFileReferenceandPBXBuildFileentries, are grouped underSource/Utilities, and are included in the iOS, macOS, and tvOSSourcesandHeadersphases with the header markedPrivate. This avoids the dangling‑reference problem seen with other headers and ensures consistent availability across all three frameworks. AssumingARTDomainSelectoris intended to be an internal helper rather than a public API surface, this setup looks correct.Also applies to: 1224-1225, 1993-1994, 2142-2142, 2318-2318, 2527-2527, 2966-2966, 3110-3110, 3254-3254
Scripts/log-environment-information.sh (1)
3-11: LGTM!The domain updates correctly reflect the new endpoint-based configuration. The sandbox environment now uses
sandbox.realtime.ably-nonprod.netwhich aligns with thenonprod:sandboxendpoint specified in the PR objectives.Examples/Tests/TestsTests/TestsTests.swift (3)
21-21: LGTM!The sandbox provisioning endpoint correctly updated to the new domain structure.
49-51: LGTM!The migration from
options.environment = "sandbox"tooptions.endpoint = "nonprod:sandbox"correctly implements the new endpoint-first model per ADR-119.
67-77: LGTM!The URL change from
ably.iotoably.comfor background task triggering is fine - these URLs are only used to initiate async work on a background queue and don't need to match Ably API endpoints.Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-157: LGTM!The deprecation attribute correctly suppresses compiler warnings when testing the deprecated
realtimeHostproperty. This pattern is consistent with similar tests inRestClientTests.swiftthat test deprecated options likeenvironment,restHost, andfallbackHostsUseDefault.Test/AblyTests/Test Utilities/TestUtilities.swift (4)
119-119: LGTM!Using
options.primaryDomain()for app creation correctly leverages the new domain selector to derive the appropriate host from endpoint configuration.
146-146: LGTM!The migration from
environmenttoendpointproperty correctly implements the new endpoint-first model for test client configuration.
626-635: LGTM!The
getTestEndpoint()function provides a sensible fallback chain:
ABLY_ENDPOINT- explicit endpoint configurationABLY_ENV- backward compatibility with existing CI/local setups"nonprod:sandbox"- default for new endpoint-based configurationThis enables gradual migration while maintaining compatibility.
606-606: Based on my investigation, I have gathered sufficient information to verify this review comment. Here are my findings:Evidence gathered:
Current code state: The
getJWTTokenfunction in TestUtilities.swift (line 606 area) constructs query parameters with:keyName,keySecret,expiresIn,clientId,capability,jwtType, andencrypted. Noenvironmentparameter is present.Web search results: The Ably echo server's createJWT endpoint documentation shows it accepts standard JWT parameters (ttl/expiresIn, capability, clientId), and all of these are included in the current code.
Codebase analysis:
- No references to
environmentas a query parameter in JWT/echo server context- Multiple tests in AuthTests.swift successfully use
getJWTTokenwithout an environment parameter- The latest commit (ADR-119) implements endpoint support as mentioned in the review
No failures or special handling: The tests using
getJWTTokenare defined to work with the current parameter set, with no evidence of bypass logic or known issues.Conclusion: The concern has either already been addressed by the ADR-119 endpoint-based routing implementation, or the environment parameter was never actually required by the echo server for the createJWT endpoint. The current implementation sends all the required parameters that the createJWT endpoint expects.
Verify the echo server's parameter handling.
The
environmentparameter is absent from the JWT token request in the new endpoint-based routing. Web search shows the createJWT endpoint expectskeyName,keySecret,expiresIn,clientId, andcapability—all of which are present in the query. Confirm that existing tests pass with this parameter set, or verify the echo server documentation ifenvironmentis actually needed for your use case.Source/ARTRealtime.m (2)
1520-1527: LGTM!The simplified condition
options.fallbackDomains.count > 0correctly implements the fallback retry logic per RTN17h and RSC15n. The domain-based approach replaces the legacyARTFallbackHostsimplementation while maintaining the same behavior - retry with fallback only when fallback domains are available.
1672-1686: LGTM!The fallback initialization correctly migrates from
ARTFallbackHoststoclientOptions.fallbackDomains:
- Fallbacks are constructed from the domain selector's fallback domains
- The
shuffleArrayoption is preserved for randomization- Empty fallback handling and logging are maintained
This aligns with the broader refactor introducing
ARTDomainSelectorfor endpoint-based domain resolution.Test/AblyTests/Tests/ARTDomainSelectorTests.swift (2)
1-440: Comprehensive test coverage for domain selection logic.The test suite thoroughly covers:
- Primary domain resolution (REC1) across multiple input scenarios
- Fallback domain resolution (REC2) including custom, default, and routing-policy-based fallbacks
- Legacy option handling with proper priority ordering
- Edge cases including empty strings, IP addresses, and localhost
The spec reference comments (REC1a, REC1b2, REC2c1, etc.) help trace tests back to ADR-119 requirements.
400-411: Based on my verification, I can now provide the rewritten review comment.This test correctly reflects the implementation's intentional handling of empty policy IDs.
The test name
test__028__ARTDomainSelector__should_handle_nonprod_with_empty_idexplicitly documents that this edge case is handled intentionally. The implementation inARTDomainSelector.m(line 132) uses a format string that naturally produces a leading dot when the policy ID is empty:return [NSString stringWithFormat:@"%@.%@.%@", self.policyId, ARTDefaultRoutingSubdomain, ARTDefaultNonprodPrimaryTLD];For
"nonprod:", the policy ID becomes an empty string, resulting in".realtime.ably-nonprod.net". This is the expected and tested behavior. If input validation is needed to reject"nonprod:"without a valid policy ID, that should be implemented inARTClientOptionsbefore the value reachesARTDomainSelector. The test is correct as written.Source/ARTRest.m (3)
677-677: Connectivity check URL fallback chain looks correct.The nil-coalescing (
?:) properly falls back fromself.options.connectivityCheckUrltoARTDefault.connectivityCheckUrl, aligning with REC3a/REC3b spec markers in the comment.
448-470: Fallback host logic correctly updated for endpoint-based domains.The changes properly:
- Obtain fallback domains from
options.fallbackDomainsinstead of the removedARTFallbackHostsclass- Set the
Hostheader on fallback requests per RSC15j specification- Maintain existing retry count and shuffling behavior
374-381: Fallback retry expiration correctly resets to primaryDomain.When
fallbackRetryExpirationexpires, the code properly resets bothcurrentFallbackHostandprioritizedHost, then switches the request host back to the primary domain viaprimaryDomain. This aligns with RSC15f behavior.Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
5-40: Well-designed API for domain selection policy.The interface cleanly encapsulates domain selection logic with:
- A designated initializer accepting all relevant client options
- Clear readonly properties for
primaryDomainandfallbackDomains- Proper nullability annotations
- Documentation referencing the REC1/REC2 spec requirements
The
NS_UNAVAILABLEon-initcorrectly enforces use of the designated initializer.Test/AblyTests/Tests/RestClientTests.swift (3)
316-330: Good coverage of endpoint/deprecated option mutual exclusion.This test verifies that setting
endpointmakes it invalid to also set deprecated options (environment,restHost,realtimeHost), which aligns with REC1b1 specification. ThetryInObjCblocks correctly expect exceptions.
1185-1253: Comprehensive fallback domain tests for REC2c1-REC2c6.The new test methods thoroughly verify fallback domain generation:
- REC2c1: Default fallbacks (
main.[a-e].fallback.ably-realtime.com)- REC2c2: Explicit domain has empty fallbacks
- REC2c3: Nonprod routing policy fallbacks (
sandbox.[a-e].fallback.ably-realtime-nonprod.com)- REC2c4: Production routing policy fallbacks
- REC2c5: Deprecated environment option fallbacks
- REC2c6: restHost/realtimeHost have empty fallbacks
Each test validates both the domain pattern and count.
865-893: Test correctly validates fallbackHostsUseDefault behavior with custom hosts.When
fallbackHostsUseDefaultistrue, the test now correctly expects the new default fallback domains (main.[a-e].fallback.ably-realtime.com) even when customrestHost/realtimeHostare set. The updated assertions align with REC2b specification.Source/include/Ably/ARTClientOptions.h (3)
22-26: Clear endpoint property documentation.The documentation properly explains that
endpointcan accept:
- A routing policy name (e.g.,
main)- A nonprod routing policy name (e.g.,
nonprod:sandbox)- A fully qualified domain name (e.g.,
foo.example.com)This aligns with ADR-119 specification.
28-54: Consistent deprecation messaging for legacy host options.All three deprecated properties (
restHost,realtimeHost,environment) have consistent deprecation messages directing users to the newendpointoption. TheDEPRECATED_MSG_ATTRIBUTEcompiler annotations ensure users get warnings during compilation.
61-64:connectivityCheckUrlis correctly copied in-copyWithZone:— no action needed.Verification confirms that
connectivityCheckUrlis properly propagated when copyingARTClientOptions. At line 235 ofSource/ARTClientOptions.m, the property is explicitly copied:options.connectivityCheckUrl = self.connectivityCheckUrl;Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4)
46-55: Default primary and fallback host expectations line up with new domain selectionThe updated expectations for the initial and fallback WebSocket URLs (main.realtime.ably.net and main.[a‑e].fallback.ably‑realtime.com) and the timeout test using
options.endpoint = "10.255.255.1"are consistent with the new ARTDomainSelector behavior (default routing policy + no fallbacks for a literal IP endpoint). The assertions and regexes are aligned and look correct.Also applies to: 181-195, 2180-2203
3843-4030: Deprecation annotations aroundrealtimeHosttests are appropriateMarking the
test__086a__andtest__087__Host Fallback tests as@available(*, deprecated, ...)keeps coverage for the deprecatedrealtimeHostoption without introducing compiler warnings. The behavior under test remains unchanged; this is a pure annotation/wording improvement.
4031-4560: REC2/REC3 host‑fallback tests comprehensively cover new endpoint and fallback semanticsThe new/updated tests around host fallback (
test__089__,test__089b__,_test__091__*,_test__092__*,test__093__,test__094__,test__095__,test__096__) correctly:
- Assert that the first attempt is always to the primary domain (derived from
endpointor defaultmain.realtime.ably.net).- Validate fallback domains for:
- Default routing policy (
main.[a‑e].fallback.ably‑realtime.com),- Nonprod routing policies (
sandbox.realtime.ably‑nonprod.net+sandbox.[a‑e].fallback.ably‑realtime‑nonprod.com),- Custom fallbackHosts arrays and empty arrays (disabling fallback).
- Verify the “internet‑up” connectivity check host and the ordering of shuffled fallback hosts.
This matches ARTDomainSelector’s REC1/REC2 logic and provides solid regression coverage.
4835-5013: Reachability/OS‑connectivity tests correctly targetprimaryDomainand endpoint reconnect behaviorThe updated tests for OS network events:
- Compare
TestReachability.hostagainstclient.internal.options.primaryDomain(), which is the right abstraction now that domains are derived via ARTDomainSelector.- Use a non‑routable
endpoint(10.255.255.1) plustestOptions.reconnectionRealtimeHost = primaryDomainto exercise restart of a pending connection attempt when reachability changes.The state assertions and injected hooks (reconnect call counting) are consistent with the intended behavior and look sound.
Source/ARTClientInformation.m (1)
19-53: Agent aggregation and identifier formatting look correctThe new
+agents,+agentIdentifierWithAdditionalAgents:,+libraryAgent,+libraryAgentIdentifier, and+platformAgentIdentifiermethods:
- Build the composite agent map from platform and library agents.
- Merge additional agents by overriding existing entries when keys collide (which is desirable).
- Produce deterministic identifiers by sorting agent names before formatting.
The sentinel
ARTClientInformationAgentNotVersionedhandling is straightforward, and iterating over anullableadditionalAgentsdictionary is safe in Objective‑C. No issues here.Also applies to: 59-75
Source/ARTDefault.m (1)
4-14: Centralizing defaults throughARTDomainSelectorand new constants looks correct
+apiVersionand+libraryVersionnow delegate toARTDefaultAPIVersionandARTClientInformationLibraryVersion, keeping those values in one place.+fallbackHostsWithEnvironment:/+fallbackHosts,+restHost, and+realtimeHostare all derived viaARTDomainSelector, so the default primary domain (main.realtime.ably.net) and fallback hosts (e.g.main.[a‑e].fallback.ably-realtime.com) are consistent with the new endpoint model.- The max message size and connectionStateTtl globals, plus their setters, are straightforward and thread‑safe enough for scalar values.
+connectivityCheckUrlreturninghttps://internet-up.ably-realtime.com/is-the-internet-up.txtmatches the test expectations that look for that host/path.No issues spotted here.
Also applies to: 25-55, 68-90, 92-119, 128-130
Source/ARTClientOptions.m (2)
296-312: Fallback host setters and copying behavior are consistent with new domain‑selector model
setFallbackHosts:andsetFallbackHostsUseDefault:correctly enforce mutual exclusivity (throwingARTFallbackIncompatibleOptionsExceptionon invalid combinations) and reset_domainSelectorso subsequentfallbackDomainscalls see the new configuration.copyWithZone:now also copiesendpoint,connectivityCheckUrl, and legacyrestHost/realtimeHost/environmentvia their setters, which ensures the derivedprimaryDomainandfallbackDomainsin the copy match the original configuration.Given the validation baked into these setters (especially around mixing endpoint with legacy options), it would be worth running/keeping tests that exercise copying for:
- An endpoint‑only configuration,
- An environment‑only configuration,
- A legacy host‑only configuration,
to ensure no unexpected exceptions are thrown when copying those options.
Also applies to: 213-261
193-211: I'll help you verify this review comment. Let me start by examining the relevant code in the file to understand the implementation and the concerns raised.
<function_calls>
#!/bin/bashFirst, let's find the file and check its size
find . -name "ARTClientOptions.m" -type f
</function_calls>
<function_calls>
#!/bin/bashGet the file size to know how much to read
wc -l Source/ARTClientOptions.m 2>/dev/null || echo "File not found at that path"
</function_calls>
<function_calls>
#!/bin/bashRead the relevant sections: lines around 168-211 to see legacy properties and new implementations
sed -n '150,220p' Source/ARTClientOptions.m
</function_calls>Let me search for this file across the repository:
<function_calls>
#!/bin/bashSearch for ARTClientOptions.m in the repo
find . -name "ARTClientOptions" -type f 2>/dev/null | head -20
</function_calls>End of response. Let me wait for the script results.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Source/ARTClientInformation.m (1)
8-11: Avoid hard‑codingARTClientInformationLibraryVersion; derive from build metadata instead.The new
ARTClientInformationLibraryVersion = @"1.2.53"andARTLibraryNameare used correctly bylibraryAgent/agents, but the version string remains a hard-coded literal. This risks divergence from the actual framework version and duplicates source-of-truth already present in build settings (e.g.,CURRENT_PROJECT_VERSION/Info.plist).Consider generating this constant at build time from the existing version metadata (e.g., via
Scripts/version-constants.shor a generated header), so the library agent always matches the package version. The surrounding agent-composition logic looks good and needn’t change.Also applies to: 19-61
🧹 Nitpick comments (6)
Source/ARTDefault.m (2)
25-31: Consider cachingARTDomainSelectorinstances for performance.This method creates a new
ARTDomainSelectorinstance on every call. SincefallbackHostsWithEnvironment:is a public API method, frequent calls could impact performance. Consider caching instances based on the environment parameter, or document if this overhead is acceptable given expected usage patterns.
38-54: Based on my verification, I can now provide the rewritten review comment:Optimize
ARTDomainSelectorinstantiation in default host methods by adding caching.Both
restHostandrealtimeHostcreate newARTDomainSelectorinstances on every call with identical parameters, returning the sameprimaryDomain. While this semantic equivalence is intentional (both provide the default domain when no client options override them), the repeated instantiation is inefficient.ARTClientOptions already implements lazy initialization with caching for its
domainSelectorproperty. Apply the same pattern to these default methods to avoid unnecessary object allocation:+ (NSString *)restHost { static ARTDomainSelector *_restHostSelector = nil; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ _restHostSelector = [[ARTDomainSelector alloc] initWithEndpointClientOption:nil fallbackHostsClientOption:nil environmentClientOption:nil restHostClientOption:nil realtimeHostClientOption:nil fallbackHostsUseDefault:false]; }); return _restHostSelector.primaryDomain; }Apply the same caching pattern to
realtimeHost.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
46-56: Host regex patterns are slightly too permissive (unescaped dots).All the
NSRegularExpression.matchpatterns for hosts (e.g."//main.realtime.ably.net","//main.[a-e].fallback.ably-realtime.com") leave.unescaped, so each dot matches any character. Tests could still pass if host segments change in unexpected ways, as long as the general shape is similar.Consider tightening these to avoid false positives, e.g.:
XCTAssertTrue(NSRegularExpression.match(url.absoluteString, pattern: #"//main\.realtime\.ably\.net"#))or by using
NSRegularExpression.escapedPattern(for:)when building patterns.Also applies to: 4060-4070, 4110-4111, 4184-4185, 4510-4512, 4546-4547
4264-4353: _test__092 helper thoroughly validates fallback ordering and internet-up checks.This helper does a solid job of:
- Forcing host-unreachable on the WS transport.
- Using
options.fallbackDomains()and a deterministicshuffleArrayInExpectedHostOrderto define expected fallback order.- Verifying that each host change is preceded by a request to
internet-up.ably-realtime.comand that the observed fallback host sequence matchesexpectedFallbackHosts, for prod,nonprod:sandbox, andtestendpoints.The approach is deterministic yet close to real behavior; no issues from a correctness standpoint.
Source/ARTDomainSelector.m (2)
66-118: Endpoint classification policy is clear, but consider centralizing the “is hostname vs routing policy” check.The
parsePrimaryDomainSelectionPolicyFromOptionsmethod cleanly implements REC1: hostnames (FQDN/localhost) →Hostname,nonprod:prefix →NonProductionRoutingPolicy, non-empty other strings →ProductionRoutingPolicy, then legacyenvironment/restHost/realtimeHost, finally default.This same classification logic (checking
".","::","localhost", andnonprod:prefix) now exists here as well as inARTClientOptions’ endpoint handling. To avoid future drift, consider factoring this into a shared helper (e.g., anisEndpointHostname/parseEndpointPolicyutility) that bothARTClientOptionsandARTDomainSelectorcall, so any future tweaks to endpoint semantics only need to be made once.
148-214: Fallback-domain generation matches REC2, with a minor immutability nit.
fallbackDomainscorrectly implements REC2:
- Explicit
fallbackHostsClientOptiontakes precedence.fallbackHostsUseDefaultand the Default policy usedefaultFallbackDomains(main.[a–e].fallback.ably-realtime.com).- Routing-policy/nonprod/legacy-environment policies build
<policyId>.[a–e].fallback.ably-realtime(.nonprod).com.- Hostname/LegacyHost policies yield no automatic fallbacks.
One small improvement:
fallbackHostsClientOptionis stored directly. If callers ever mutate the array after passing it in, that would affect the selector’s behavior. Making the propertycopy(and copying in the initializer) would protectARTDomainSelectorfrom external mutations:@property (nonatomic, nullable, copy) NSArray<NSString *> *fallbackHostsClientOption;and copy in
initWithEndpointClientOption:....
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientInformation.m(2 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(2 hunks)Source/ARTRest.m(6 hunks)Source/ARTWebSocketTransport.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientInformation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/integration-test.yaml
- Source/Ably.modulemap
- Source/include/module.modulemap
- Scripts/log-environment-information.sh
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Test/AblyTests/Tests/PushAdminTests.swift
- Ably.xcodeproj/project.pbxproj
🧰 Additional context used
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (34)
Examples/Tests/TestsTests/TestsTests.swift (3)
50-50: LGTM! Correctly implements ADR-119.The change from
environment = "sandbox"toendpoint = "nonprod:sandbox"properly implements the new endpoint option as specified in ADR-119, using the correct nonprod routing policy name format.
67-67: Background network call domain change is intentional and verified as functional.Both
https://ably.ioandhttps://ably.comare accessible. The domainably.ioredirects (HTTP 301) toably.com, which returns HTTP 200. The change from.ioto.comreflects the official Ably domain migration and will not break test execution. The arbitrary network calls at lines 67 and 77 function correctly with the new domain, as they are only used to trigger background queue execution rather than serving as critical SDK endpoints.
21-21: The sandbox provisioning endpoint URL change is correct and part of the coordinated infrastructure migration.The change from
sandbox-rest.ably.iotosandbox.realtime.ably-nonprod.netaligns with the SDK's domain configuration system. Multiple test files confirmsandbox.realtime.ably-nonprod.netas the primary domain for non-production environments:
RestClientTests.swift:343—primaryDomain()returns"sandbox.realtime.ably-nonprod.net"ARTDomainSelectorTests.swift:87— primaryDomain assertion:"sandbox.realtime.ably-nonprod.net"ARTDomainSelector.m:7— definesARTDefaultNonprodPrimaryTLD = "ably-nonprod.net"Line 50's
endpoint = "nonprod:sandbox"correctly maps to this domain through the domain selector logic. This is a systematic infrastructure update, not an isolated endpoint error.Source/include/Ably/ARTDefault.h (1)
15-17: LGTM! Public API methods preserved.The
restHostandrealtimeHostclass methods are correctly declared as public API, maintaining backward compatibility while delegating internally to the newARTDomainSelector-based implementation.Source/ARTDefault.m (3)
6-7: LGTM! Constants properly defined.The new constants
ARTDefaultAPIVersionandARTDefaultProductionEnvironmentare well-defined and replace previous implementations.
76-81: LGTM! Conditional compilation for max message size is appropriate.The conditional compilation correctly returns different message size limits for DEBUG (sandbox) vs production builds.
128-130: LGTM! Connectivity check URL properly formatted.The new
connectivityCheckUrlmethod correctly formats the connectivity check URL with the HTTPS scheme.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
25-31: LGTM! Private convenience methods well-documented.The new convenience methods
primaryDomainandfallbackDomainsare appropriately documented and provide a clean interface to the internalARTDomainSelector. ThesetDefaultEndpoint:class method enables test configuration as noted in the past review.Source/PrivateHeaders/Ably/ARTClientInformation+Private.h (1)
5-5: LGTM! Constant renamed for consistency.The constant rename from
ARTClientInformation_libraryVersiontoARTClientInformationLibraryVersionimproves naming consistency by following standard conventions.Source/PrivateHeaders/Ably/ARTDefault+Private.h (1)
3-3: LGTM! Private header updates are consistent.The constant rename to
ARTDefaultProductionEnvironmentimproves clarity, and the newconnectivityCheckUrlmethod declaration is consistent with its implementation.Also applies to: 13-13
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
119-119: LGTM! Test setup URL construction updated for endpoint model.The URL construction now correctly uses
options.primaryDomain()instead of the deprecatedrestHost, aligning with the endpoint-centric approach.
146-146: LGTM! Test client options migrated to endpoint configuration.The test utilities correctly configure
options.endpointinstead of the deprecatedoptions.environment, using the newgetTestEndpoint()function.
626-635: LGTM! Test endpoint resolution with proper fallback chain.The new
getTestEndpoint()function correctly prioritizesABLY_ENDPOINToverABLY_ENVand defaults to"nonprod:sandbox", maintaining backward compatibility while supporting the new endpoint-centric model.Source/ARTRest.m (5)
375-381: LGTM! Fallback retry timeout logic correctly implemented.The fallback retry expiration logic correctly resets the prioritized and fallback hosts and switches back to
primaryDomain, implementing the RSC15f specification requirement.
440-445: LGTM! Sticky fallback host behavior correctly implemented.The code correctly promotes a successful fallback host to
prioritizedHost, implementing the RSC15f specification for sticky fallback behavior.
514-520: LGTM! Current host resolution migrated to primaryDomain.The
currentHostmethod correctly returnsprimaryDomainwhen no prioritized host is set, completing the migration from the deprecatedrestHostproperty.
676-678: LGTM! Connectivity check URL now configurable.The connectivity check correctly uses the configurable
options.connectivityCheckUrlwith a fallback toARTDefault.connectivityCheckUrl, implementing the REC3a and REC3b specification requirements.
448-461: Excellent. Let me get the full context of the code snippet to ensure there are no other issues:
<function_calls>
# Get complete context of the fallback host logic sed -n '440,475p' Source/ARTRest.mSource/ARTWebSocketTransport.m (1)
247-253: ThesetHost:andhostmethod changes are semantically compatible with existing usage.The change from
options.realtimeHosttooptions.endpointworks correctly because:
- Fallback hosts are valid FQDNs: Both callers pass FQDNs (e.g., "fallback-host1.ably.io", "fallback-host2.ably.io") which contain dots.
- ARTDomainSelector correctly classifies FQDNs as hostnames: When an endpoint contains a dot, is "localhost", or contains "::", it's classified as a hostname (REC1b2).
- primaryDomain returns the hostname directly: For hostname endpoints,
primaryDomainreturns the hostname value unchanged, preserving the original behavior.Callers verified:
transportReconnectWithExistingParameters(line 1102): usestestOptions.reconnectionRealtimeHost(test-only, nil in production)transportReconnectWithHost:(line 1109): receives FQDNs from the fallback host mechanismSource/ARTRealtime.m (2)
1520-1527: LGTM! Fallback retry logic correctly simplified.The refactored
shouldRetryWithFallbackForErrornow delegates domain availability toARTDomainSelectorviaoptions.fallbackDomains, correctly implementing RSC15m. The single condition check is cleaner and aligns with the endpoint-first domain resolution design.
1659-1690: LGTM! Fallback construction updated correctly.The
realtimeTransportFailedmethod now sources fallback domains fromclientOptions.fallbackDomainsinstead ofARTFallbackHosts, correctly implementing the endpoint-first domain resolution pattern. The fallback behavior and error handling remain intact.Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-42: LGTM! Clean interface design for domain selection.The
ARTDomainSelectorinterface is well-designed with:
- Clear documentation referencing REC1 (primary domain) and REC2 (fallback domains)
- Comprehensive initializer accepting all relevant client options
- Proper encapsulation with readonly properties
- Forced use of designated initializer via
NS_UNAVAILABLENote: The static analysis error about
Foundation/Foundation.hnot found is a false positive and can be safely ignored.Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
1-441: LGTM! Comprehensive test coverage for ARTDomainSelector.The test suite provides excellent coverage of the domain selection logic:
- ✅ REC1 tests validate primary domain resolution for all cases (default, hostname, nonprod/prod routing)
- ✅ REC2 tests validate fallback domain generation for all scenarios
- ✅ Legacy option tests ensure backward compatibility with deprecated options
- ✅ Edge case tests cover empty strings, complex routing IDs, and option priority
- ✅ Clear test naming follows the pattern:
test__###__ARTDomainSelector__feature__scenarioThe test organization by REC specification sections makes it easy to verify compliance with ADR-119.
Test/AblyTests/Tests/RestClientTests.swift (3)
45-46: LGTM! Host patterns correctly updated to new domain structure.The test expectations have been correctly updated from the old patterns:
rest.ably.io→main.realtime.ably.net[a-e].ably-realtime.com→main.[a-e].fallback.ably-realtime.comThis aligns with the endpoint-first domain resolution and the default
mainrouting policy.Also applies to: 66-67, 112-112, 134-135, 402-402
316-403: LGTM! Comprehensive REC specification test coverage.The new tests thoroughly validate the endpoint-first domain resolution per ADR-119:
- ✅ REC1b1-REC1b4: Endpoint option validation and domain resolution
- ✅ REC1c1-REC1c2: Deprecated environment option handling
- ✅ REC1d1-REC1d2: Deprecated restHost/realtimeHost option handling
- ✅ REC1a: Default domain validation
- ✅ REC2a-REC2c: Fallback domain generation for all scenarios
The tests correctly validate both the happy paths and error cases (invalid option combinations).
Also applies to: 902-1095, 1185-1253
302-303: LGTM! Proper handling of deprecated option tests.The deprecated option tests are correctly annotated with
@available(*, deprecated, ...)to:
- Suppress compiler warnings for intentional use of deprecated APIs
- Document that these tests validate legacy behavior
- Include clear messages referencing the replacement (
endpointoption)This is the correct approach for maintaining backward compatibility tests during a deprecation period.
Also applies to: 317-318, 354-355, 367-368, 377-378, 385-386, 418-419
Source/include/Ably/ARTClientOptions.h (3)
22-26: LGTM! Endpoint property well-documented.The new
endpointproperty is clearly documented with:
- Explanation of valid values (routing policy name, nonprod routing policy, FQDN)
- Example values (
main,nonprod:sandbox,foo.example.com)- Reference to Platform Customization documentation
This provides users with clear guidance on how to use the new option.
28-38: LGTM! Clear deprecation path for legacy options.The deprecated properties (
restHost,realtimeHost,environment) have consistent deprecation messages directing users to theendpointoption. TheDEPRECATED_MSG_ATTRIBUTEapproach:
- Generates compiler warnings for new code using these properties
- Maintains backward compatibility for existing code
- Provides clear migration guidance
Also applies to: 50-54
61-64: No action required —connectivityCheckUrlis already properly copied in-copyWithZone:.The property is correctly propagated at line 235 of
ARTClientOptions.m:options.connectivityCheckUrl = self.connectivityCheckUrl;The new property has been integrated into the copy semantics as expected.
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4)
2181-2203: Endpoint-based timeout test looks correct.Using
options.endpoint = "10.255.255.1"to force a connection timeout and asserting a DISCONNECTED transition with a “timed out” reason and duration close torealtimeRequestTimeoutmatches the new endpoint semantics and REC/RTN14 expectations.
4189-4241: _test__091 helper nicely exercises primary-domain selection for default and routed endpoints.The shared helper correctly covers:
- Default (no endpoint) →
main.realtime.ably.net+ fallback.- Nonprod routing policy (
nonprod:sandbox) →sandbox.realtime.ably-nonprod.net+sandbox.[a–e].fallback.ably-realtime-nonprod.com.- Prod routing policy (
"test") →test.realtime.ably.net+test.[a–e].fallback.ably-realtime.com.Lazy
fakeNetworkResponseclearing on first connect and the assertions onurlConnections[0]/[1]give good confidence the selector is wired correctly for these cases.
4471-4512: EmptyfallbackHostsbehavior test is clear and correct.
test__095__...won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedcorrectly asserts that:
- Connection transitions are CONNECTING → DISCONNECTED → CONNECTING.
- Only the primary host (
main.realtime.ably.net) is ever used, even though the error would normally qualify for fallback.The DataGatherer pattern keeps this precise without being brittle.
4835-4943: RTN20 connectivity tests now correctly depend onprimaryDomain.The updated tests for OS reachability (RTN20a/b/c) verify that:
- Reachability’s
hostmatchesclient.internal.options.primaryDomain().- Simulated reachability changes drive CONNECTING/CONNECTED and DISCONNECTED/SUSPENDED transitions as expected, including the reconnect-restart case in
test__106_b.This keeps the tests aligned with the new endpoint/domain-selector source of truth and avoids hard-coding hostnames in reachability assertions.
Source/ARTClientOptions.m (1)
22-38: Endpoint/domain-selector integration looks consistent and preserves legacy invariants.The new wiring in
ARTClientOptionsis coherent:
_endpointis initialized fromARTDefaultEndpoint, withprimaryDomain/fallbackDomainsnow delegated toARTDomainSelectorand reused for both REST and Realtime URLs.setEndpoint:,setEnvironment:,setRestHost:, andsetRealtimeHost:enforce the intended mutual exclusivity (endpoint vs environment/hosts, environment vs hosts) and invalidate the cacheddomainSelectoron change.restHost/realtimeHostgetters fall back toprimaryDomain, keeping legacy callers working while routing all host derivation through the selector.setFallbackHosts:andsetFallbackHostsUseDefault:also reset the selector, ensuring fallback changes are picked up.copyWithZone:propagatesendpoint, connectivity URL, fallback config, and any explicitly set legacy host/environment options, relying on the same invariants as the setters.Overall this is a clean encapsulation of the new endpoint-first behavior with good backward-compatibility hooks.
Also applies to: 48-82, 88-191, 193-211, 213-261, 296-312, 314-316, 331-343
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
4031-4071: Minor: comment in custom fallback-host test is now misleadingIn
test__089__Connection__Host_Fallback__applies_when_an_array_of_ClientOptions_fallbackHosts_is_provided, the comment on Line 4060 still refers towss://main.[a-e].fallback.ably-realtime.comeven though the test is exercising custom fallback hosts ([f-j].ably-realtime.com), and the assertions correctly match only those custom hosts.Consider updating the comment to clarify that this test is about explicitly supplied fallback hosts, not the default
main.[a-e].fallback...set.
4374-4399: Fix stale fallback-host regex so the test still guards against using fallbacks
test__093__Connection__Host_Fallback__doesn_t_try_fallback_host_if_Internet_connection_check_failsstill matches fallback hosts with the old pattern"[a-e].ably-realtime.com", but default fallbacks are now of the formmain.[a-e].fallback.ably-realtime.com. As a result, this test will no longer detect unintended fallback-host usage.Update the regex to match the new default fallback host shape so the assertion remains meaningful.
- let extractHostname = { (url: URL) in - NSRegularExpression.extract(url.absoluteString, pattern: "[a-e].ably-realtime.com") - } + let extractHostname = { (url: URL) in + // Match default fallback hosts, e.g. "main.a.fallback.ably-realtime.com" + NSRegularExpression.extract( + url.absoluteString, + pattern: "main.[a-e].fallback.ably-realtime.com" + ) + }
♻️ Duplicate comments (2)
Source/ARTClientInformation.m (1)
9-9: Hard-coded version string still present.The library version remains hard-coded despite previous feedback to derive it from build metadata (CURRENT_PROJECT_VERSION). This creates maintenance burden and risks version drift between the library and package metadata.
Test/AblyTests/Tests/RestClientTests.swift (1)
181-181: TODO comments are already tracked in issue #2145.These TODO comments about ineffective test assertions (checking
publishTask.errorimmediately when it's nil) were previously discussed and tracked in issue #2145. The fix is appropriately deferred to a separate PR focused on test improvements.Also applies to: 206-206
🧹 Nitpick comments (7)
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-40: Minor API polish forARTDomainSelectorpropertiesThe interface is clear and matches the ADR intent. Two small nits you may want to consider:
- Use
copyfor the exposed collection/string properties to follow common Cocoa patterns and guard against accidental external mutation:-@property (nonatomic, readonly) NSString *primaryDomain; +@property (nonatomic, copy, readonly) NSString *primaryDomain; -@property (nonatomic, readonly) NSArray<NSString *> *fallbackDomains; +@property (nonatomic, copy, readonly) NSArray<NSString *> *fallbackDomains;
- (Optional) Mark the initializer as a designated initializer for clarity if this type is ever subclassed:
-- (instancetype)initWithEndpointClientOption:(nullable NSString *)endpointClientOption +- (instancetype)initWithEndpointClientOption:(nullable NSString *)endpointClientOption NS_DESIGNATED_INITIALIZERTest/AblyTests/Test Utilities/TestUtilities.swift (1)
626-635: HardengetTestEndpoint()against missing Info.plist entries (optional)The lookup logic (ABLY_ENDPOINT → ABLY_ENV →
"nonprod:sandbox") is sound. To avoid crashing ifinfoDictionaryis unexpectedlynil, you could defensively unwrap once and reuse it:-public func getTestEndpoint() -> String { - let bundle = Bundle(for: AblyTests.self) - if let endpoint = bundle.infoDictionary!["ABLY_ENDPOINT"] as? String, endpoint.count > 0 { - return endpoint - } - if let env = bundle.infoDictionary!["ABLY_ENV"] as? String, env.count > 0 { - return env - } - return "nonprod:sandbox" -} +public func getTestEndpoint() -> String { + let bundle = Bundle(for: AblyTests.self) + let info = bundle.infoDictionary ?? [:] + if let endpoint = info["ABLY_ENDPOINT"] as? String, !endpoint.isEmpty { + return endpoint + } + if let env = info["ABLY_ENV"] as? String, !env.isEmpty { + return env + } + return "nonprod:sandbox" +}Not required for tests, but makes this helper more robust.
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
25-31: New domain helper APIs are useful; minor nitsExposing
primaryDomain/fallbackDomainsas convenience accessors and asetDefaultEndpoint:hook makes sense for tests and internal wiring. A couple of small improvements you might consider:
- Fix the typo in the comments (
Convinience→Convenience).- Optionally, mirror the
copysemantics suggestion fromARTDomainSelectorfor any public/bridged NSString/NSArray properties that wrap these values, to keep everything consistent.Functionally this header looks fine.
Source/ARTRest.m (1)
676-688: Configurable connectivity check URL is a good extensionUsing
self.options.connectivityCheckUrlwith a fallback toARTDefault.connectivityCheckUrlmakesinternetIsUpconfigurable while retaining a sensible default. The method still requires a 200 response body of"yes\n", so documenting that contract alongside theconnectivityCheckUrloption will help users who override it.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
4264-4353: Randomized fallback-order tests are well-factored and exercise endpoint variants
_test__092__...and its three variants validate that fallback hosts (for prod, nonprod sandbox, and"test") are tried in the expected shuffled order after connectivity check, usingoptions.fallbackDomains()as the source of truth. The use ofshuffleArrayInExpectedHostOrderand explicit collection of both WebSocket and HTTP URLs gives good coverage of the REC2 randomized behavior.You might consider hoisting the
hostPrefixes/ regex logic for prod vs nonprod into a small helper to reduce duplication across the three variants, but it's not essential.Source/ARTDomainSelector.m (1)
5-12: Domain selection and fallback generation match REC1/REC2 and testsThe
ARTDomainSelectorimplementation cleanly encodes the REC1 primary-domain policies and REC2 fallback rules:
- Priority order in
parsePrimaryDomainSelectionPolicyFromOptions(endpoint → environment → restHost/realtimeHost → default) is clear and matches the new endpoint‑first model.- Primary domains are formatted as:
- Default:
main.realtime.ably.net- Nonprod routing:
<policyId>.realtime.ably-nonprod.net- Prod/legacy env:
<policyId>.realtime.ably.net- Hostname/legacy host: raw hostname.
- Fallbacks:
- Honor explicit
fallbackHostsClientOption(including the empty-array “disable fallbacks” case).- Use
defaultFallbackDomains(main.[a-e].fallback.ably-realtime.com) for default andfallbackHostsUseDefault.- Generate policy-based fallbacks for routing/environment policies with the correct TLDs, and none for hostname/legacy-host.
The code is straightforward and lines up with the updated Swift tests for prod/nonprod/test and legacy environment behavior.
You could factor the
aToEarray into a shared static constant to avoid reallocation, but it’s not performance‑critical here.Also applies to: 66-118, 120-214
Source/ARTClientOptions.m (1)
88-119: Mutual-exclusion rules and domainSelector invalidation are correctly enforced
setEndpoint:forbids combiningendpointwithenvironment,restHost, orrealtimeHost, per REC1b1, and resetsdomainSelector.setEnvironment:,setRestHost:, andsetRealtimeHost:all guard against an already-set non‑emptyendpointand against environment+host mixes, then resetdomainSelectoron change.setFallbackHosts:andsetFallbackHostsUseDefault:still enforce their mutual exclusivity and now also invalidatedomainSelector.This keeps domain-selection state consistent regardless of which legacy or new options are mutated.
It might be worth adding a brief comment near
domainSelector’s initializer noting that it relies on these setters to uphold the invariants (so future callers don’t bypass them).Also applies to: 296-313, 314-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientInformation.m(2 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(2 hunks)Source/ARTRest.m(6 hunks)Source/ARTWebSocketTransport.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientInformation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (9)
- Scripts/log-environment-information.sh
- Source/include/module.modulemap
- Test/AblyTests/Tests/ARTDomainSelectorTests.swift
- Test/AblyTests/Tests/RealtimeClientTests.swift
- .github/workflows/integration-test.yaml
- Source/PrivateHeaders/Ably/ARTClientInformation+Private.h
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/Ably/ARTClientOptions.h
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (5)
match(1516-1524)at(1431-1433)at(1440-1445)data(934-936)newRealtime(184-198)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
match(1516-1524)getTestEndpoint(626-635)getTestToken(505-507)getTestToken(509-513)at(1431-1433)at(1440-1445)clientOptions(144-159)extract(1526-1537)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (41)
Source/ARTClientInformation.m (1)
56-56: LGTM!The use of renamed constants is consistent with the refactoring in this file.
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is correctly wired into all targets (file refs, groups, headers, and sources).
ARTDomainSelector.mhas a singlePBXFileReferenceand threePBXBuildFileentries, each used once in the iOS, macOS, and tvOSSourcesphases.ARTDomainSelector.hhas a singlePBXFileReferenceunderPrivateHeaders/Ablyand threePBXBuildFileentries, each used once in the iOS, macOS, and tvOSHeadersphases withATTRIBUTES = (Private,).- Group placement under
Source→Utilitiesmatches existing private utility types and keeps the project graph coherent.I don’t see any dangling IDs or missing references here; the new files should build correctly for all three frameworks.
Also applies to: 1224-1225, 1993-1994, 2142-2142, 2318-2318, 2527-2527, 2966-2966, 3110-3110, 3254-3254
Source/Ably.modulemap (1)
142-142: ExposeARTDomainSelectorin private modulemapAdding
ARTDomainSelector.hto theAbly.Privatemodule is consistent with the new domain-selection internals and keeps it available to tests without changing the public API.Source/ARTWebSocketTransport.m (1)
247-253: Aligninghostwith endpoint/primaryDomain is reasonableWiring the setter to
options.endpointand the getter tooptions.primaryDomainkeeps this property aligned with the new endpoint-based domain selection while preserving the existing externalhostAPI surface. Behavior here looks consistent with the rest of the refactor.Examples/Tests/TestsTests/TestsTests.swift (2)
20-52: Update to sandbox apps URL and endpoint usage looks coherentUsing
https://sandbox.realtime.ably-nonprod.net:443/appsfor app provisioning and configuringoptions.endpoint = "nonprod:sandbox"aligns this example test with the new routing policy–based endpoint model; no issues from a correctness standpoint.
65-78: Background URL host changes toably.comare safeThese background
URLSessioncalls are just used to hop onto a background queue before creating Realtime/REST clients. Switching the URL host fromably.iotohttps://ably.comdoesn’t affect the test’s purpose and should be fine.Source/include/Ably/ARTDefault.h (1)
15-16: PublicrestHost/realtimeHostremain exposedOnly formatting changed here; keeping these accessors public and delegating to the new domain-selection logic (as per the PR description) is appropriate.
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
119-141: App provisioning URL now respects selected primary domainBuilding the apps URL with
https://\(options.primaryDomain()):\(options.tlsPort)/appsremoves the hard-coded sandbox host and reuses the same primary domain and TLS port that the client under test will use. This keeps test provisioning aligned with the configured endpoint.
144-159: Usingendpointin test client options is correctSetting
options.endpoint = getTestEndpoint()here is the right way to route tests through the new endpoint-based configuration while preserving all existing behavior controlled via Info.plist.
607-607: No functional impact in JWT helper changeThe small tweak around the
"encrypted"query item does not affect behavior; the helper still sends the correct parameter.Source/ARTRest.m (4)
19-19: ImportingARTDefault+Private.his appropriateThis import is needed now that
ARTRestInternalusesARTDefault.connectivityCheckUrl; no issues here.
374-381: Fallback host behavior correctly wired toprimaryDomain/fallbackDomainsThe updated fallback logic appears consistent with the new domain-selection model:
- On
fallbackRetryExpiration, you clearcurrentFallbackHostandprioritizedHostand rewrite the request host to_options.primaryDomain, so fallbacks are not sticky beyond their TTL.- On a successful response from a fallback host, you promote it to
prioritizedHost, as per RSC15f.- When constructing fallback hosts, you now pull from
_options.fallbackDomains, which delegates host generation to the domain selector.currentHostnow returnsoptions.primaryDomainwhen no prioritized host is set, aligning all host reporting with the same underlying source.Assuming
fallbackDomainsis guaranteed not to includeprimaryDomain, this preserves the expected retry/fallback behavior.Also applies to: 441-452, 514-520
448-452: Explicit Host header on fallback requests matches RSC15jWhen retrying against a fallback domain, setting the
Hostheader to that fallback and updating the URL’s host ensures the request is correctly targeted and matches the spec’s RSC15j requirement. The use ofARTFallbackwithoptions.fallbackDomainshere is consistent.Also applies to: 460-461
751-758: Base URL construction continues to respect prioritized host
getBaseUrlstill derives components from_options restUrlComponents]and then overridescomponents.hostwithprioritizedHostwhen present. Combined with the newprimaryDomain-based host selection, this keeps REST traffic correctly routed through the currently selected primary or fallback host.Source/ARTRealtime.m (2)
1520-1527: LGTM! Fallback retry logic correctly checks for available domains.The simplified check
options.fallbackDomains.count > 0is cleaner than the previous implementation and correctly determines whether fallback hosts are available for retry. This aligns with the endpoint-based domain selection approach.
1659-1690: LGTM! Transport failure handling correctly uses endpoint-based fallback domains.The migration from
ARTFallbackHoststoclientOptions.fallbackDomainsis properly implemented. The method correctly:
- Sources fallback hosts from the options (line 1675)
- Handles empty fallback lists by resetting and logging appropriately (lines 1679-1682)
- Maintains the fallback flow with proper state transitions
Test/AblyTests/Tests/RestClientTests.swift (14)
45-46: LGTM! Domain patterns correctly updated for new default endpoint.The test now validates requests against the new default domain (
main.realtime.ably.net) and fallback pattern (main.[a-e].fallback.ably-realtime.com), aligning with the endpoint-based domain selection implementation.
177-177: LGTM! Proper use of new endpoint-based APIs.Line 177 correctly uses
options.primaryDomain()to set the prioritized host, and lines 201-202 properly set up and clean up the default endpoint for the test context.Also applies to: 201-202
318-330: LGTM! Properly validates endpoint incompatibility with legacy options.This test correctly verifies the REC1b1 requirement that
endpointcannot be used together with deprecated options (environment,restHost,realtimeHost). The use oftryInObjCto catch exceptions is appropriate.
333-351: LGTM! Comprehensive coverage of endpoint value types.These three tests (REC1b2, REC1b3, REC1b4) properly validate all three types of endpoint values:
- Fully qualified domain names
- Non-production routing policies
- Production routing policies
Each test correctly verifies the resulting
primaryDomain()value.
355-390: LGTM! Proper testing of deprecated options behavior.These tests (REC1c1, REC1c2, REC1d1, REC1d2) correctly validate:
- Mutual exclusivity of deprecated options
- Correct
primaryDomain()values when legacy options are used- All properly annotated as deprecated to avoid compiler warnings
393-403: LGTM! Default endpoint correctly updated.Test now validates that the default endpoint is
https://main.realtime.ably.netinstead of the legacyrest.ably.io, aligning with the REC1a requirement.
419-424: LGTM! Deprecated properties maintain default values.The test correctly verifies that deprecated
restHostandrealtimeHostproperties still have their default values fromARTDefault. Properly marked as deprecated to avoid compiler warnings.
512-531: LGTM! Endpoint configuration test properly validates domain selection.Test correctly verifies (REC1a overridden) that:
- Setting
endpointchanges theprimaryDomain()- Requests are sent to the configured endpoint domain
685-688: LGTM! Test setup and fallback expectations correctly updated.Lines 685-688: Proper test setup with default endpoint and cleanup.
Lines 875-894: Test expectations updated to match new fallback domain pattern (
main.[a-e].fallback.ably-realtime.com), aligning with the endpoint-based domain selection.Also applies to: 875-894
933-984: LGTM! Fallback domain tests cover key scenarios.Test__055 (RSC15m, REC2c1): Correctly validates default fallback domains when using default endpoint.
Test__056 (RSC15m, REC2c2): Correctly validates that explicit domain endpoints have no fallback hosts, demonstrating that only the specified domain is used.
1070-1095: LGTM! Routing policy fallback test correctly validates derived domains.Test (REC2c4) properly verifies that when
endpointis set to a routing policy name (test), both primary and fallback domains are correctly derived with the policy prefix (test.realtime.ably.netandtest.[a-e].fallback.ably-realtime.com).
1125-1146: LGTM! Fallback usage tests validate correct retry behavior.Test__047 (RSC15m, TO3k6): Correctly validates that setting
fallbackHosts = []prevents fallback usage.Test__049 (RSC25): Correctly validates that each new HTTP request first attempts the primary domain, even after fallback usage, respecting the
fallbackRetryTimeout.Also applies to: 1149-1183
1186-1254: LGTM! Comprehensive validation of implicit fallback domain derivation.Tests 065a-065g provide excellent coverage of how fallback domains are implicitly determined based on configuration:
- Default endpoint → default fallbacks
- Explicit domain → no fallbacks
- Routing policies → policy-specific fallbacks
- Deprecated options → appropriate fallbacks or none
All expected patterns and counts are correct.
1258-1294: LGTM! Fallback host ordering tests validate retry behavior.Test__065 (RSC15h): Correctly validates default fallback host pattern and count.
Test__067: Properly validates that:
- Fallback hosts are retried in the expected (shuffled) order
- Retry stops after
httpMaxRetryCountis reached- The extracted fallback hosts match the expected order from
fallbackDomains()Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (5)
44-56: Default host and fallback assertions align with new ably.net domainsThe updated expectations for the default host (
main.realtime.ably.net) and fallback hosts (main.[a-e].fallback.ably-realtime.com) match the new domain-selection model andARTDomainSelectorbehavior; the helper and RTN2 default URL test look consistent and accurate.Also applies to: 181-195
2178-2203: Timeout test correctly exercises endpoint-as-host behaviorUsing
options.endpoint = "10.255.255.1"as a non‑routable address is a good way to drive the RTN14c timeout path under the new endpoint semantics, and the timing expectations (disconnected withinrealtimeRequestTimeout) look sound.
4187-4260: Endpoint-based primary host tests for prod/nonprod look correctThe shared helper
_test__091__...and its callers for default,nonprod:sandbox, and"test"endpoints assert primary and first-fallback hosts that line up withARTDomainSelector’s REC1/REC2 behavior (prod:*.realtime.ably.net/*.fallback.ably-realtime.com, nonprod:*.realtime.ably-nonprod.net/*.fallback.ably-realtime-nonprod.com). The sequencing and error expectations also look coherent.
4471-4512: EmptyfallbackHostssemantics are correctly asserted
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedclearly demonstrates that providing an emptyfallbackHostslist disables fallback usage: the state-change sequence and URL assertions ensure only the primary host (main.realtime.ably.net) is used. This matches theARTDomainSelector.fallbackDomainsbehavior where an empty-but-non-nil list is honored as-is.
4835-4903: Reachability tests correctly switched toprimaryDomain()The RTN20 tests now assert that
TestReachability.hostequalsclient.internal.options.primaryDomain()in CONNECTING, CONNECTED, DISCONNECTED and SUSPENDED flows. That ties OS reachability checks to the same REC1 primary domain logic used for connections and aligns with the new endpoint/routing-policy abstraction.Also applies to: 4908-4943, 4946-5013
Source/ARTDefault.m (1)
4-14: Default host, fallback, and connectivity constants are wired correctly
fallbackHostsWithEnvironment:andfallbackHostsnow delegate toARTDomainSelector, so default and environment-specific fallback hosts come from a single REC1/REC2 implementation.restHost/realtimeHostresolve toprimaryDomain(main.realtime.ably.netby default), matching the updated tests.- Connection TTL and max message sizes use shared globals with synchronized setters, preserving previous mutability semantics.
connectivityCheckUrl’shttps://internet-up.ably-realtime.com/is-the-internet-up.txtvalue aligns with the connectivity-check tests.Overall this centralizes domain and connectivity defaults cleanly.
Also applies to: 25-55, 68-91, 92-119, 128-130
Source/ARTClientOptions.m (5)
5-14: Endpoint defaulting and initialization are coherentThe introduction of
ARTDefaultEndpointplus the new ivars (_endpoint,_restHost,_realtimeHost,_environment) and default initialization (endpoint from global, legacy hosts/env cleared, connectivity URL fromARTDefault) gives a clean baseline for endpoint‑first configuration while still allowing tests or embedding apps to override the default endpoint via+setDefaultEndpoint:.No issues with the initialization order or defaults.
Also applies to: 22-38, 40-82
105-118: Primary/fallback domain exposure and URL construction are consistent
primaryDomainandfallbackDomainssimply delegate todomainSelector, ensuring all host/endpoint/env combinations go through the same REC1/REC2 logic.restUrlComponentsandrealtimeUrlnow useself.primaryDomainfor the host, so REST and Realtime share the same routing decision by default.- Legacy host overrides still work because
ARTDomainSelector’sLegacyHostpolicy uses the overridden host as the primary domain.The resulting URLs line up with the updated tests (e.g. default
main.realtime.ably.net).Also applies to: 120-127, 193-211
130-191: Legacy environment/host accessors integrate cleanly with endpoint modelThe reworked
environment,restHost,realtimeHost,hasCustom*, andhasEnvironmentaccessors:
- Keep deprecated options usable but isolated from
endpoint.- Default
restHost/realtimeHosttoprimaryDomainwhen unset, ensuring callers don’t accidentally seenil.- Use
environmentonly forisProductionEnvironmentandhasEnvironment, which is appropriate now that environment is legacy-only.This maintains backward compatibility while nudging new users toward the endpoint‑centric API.
Also applies to: 272-286, 338-343
213-261: Copy semantics respect invariants and propagate new endpoint/connectivity fields
copyWithZone:now:
- Copies
endpointandconnectivityCheckUrlto the new instance.- Preserves legacy
fallbackHosts/fallbackHostsUseDefaultvia ivars to avoid triggering mutual-exclusion exceptions.- Re-applies
restHost,realtimeHost, andenvironmentonly if they were explicitly set on the source, using the standard setters (so illegal combinations remain impossible).The order of operations ensures no invalid endpoint+env/host mix is introduced during copy.
331-343: Environment-based production detection is legacy-only but coherent
isProductionEnvironmentandhasEnvironmentnow rely solely on the (deprecated)environmentproperty and the newARTDefaultProductionEnvironmentconstant. This keeps behavior intact for legacy callers that still configureenvironmentwhile leaving the newendpoint‑based flows unaffected.If
isProductionEnvironmentis used for any behavior that should also treat the default endpoint (nil/"main") as “production”, it may be worth double‑checking those call sites to ensure they don’t rely solely onenvironment.
c016fd8 to
1049e40
Compare
Source/ARTWebSocketTransport.m
Outdated
|
|
||
| - (void)setHost:(NSString *)host { | ||
| self.options.realtimeHost = host; | ||
| self.options.endpoint = host; |
There was a problem hiding this comment.
This seems a bit hard to understand — it's weird to use the indirection of the endpoint client option when all that you want to do is set a hostname on the transport. The fundamental problem here is that the transport is misusing the client options as its way of getting the hostname to use; this should be passed in as an argument. I'd suggest adding a preceding refactor commit to address this.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Source/ARTDefault.m (1)
6-14: Host/fallback resolution and connectivity defaults are cleanly centralised
- Using
ARTDomainSelectorinfallbackHostsWithEnvironment:,restHost, andrealtimeHostmakes both primary and fallback domains follow the same REC1/REC2 policy asARTClientOptions, instead of the older ad‑hoc logic.- Exposing
ARTConnectionStateTtland the max message size globals via simple accessors/setters keeps existing configuration points intact while making the defaults explicit for tests.+maxMessageSizeswitching between sandbox/production globals and the dedicated+maxSandboxMessageSize/+maxProductionMessageSizeaccessors keeps behaviour clear and testable.+connectivityCheckUrlgives ARTRest a single default URL to hit, and is trivially overridable viaoptions.connectivityCheckUrl.If you find yourself touching the default domain logic again, consider a tiny helper to construct a “default”
ARTDomainSelectorinstance to avoid repeating the same nil arguments in three methods, but that’s purely cosmetic.Also applies to: 25-55, 68-90, 92-118, 128-130
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-40:ARTDomainSelectorinterface matches REC1/REC2 needs and prevents misuseThe header cleanly gathers all domain‑related client options into one initializer and exposes just
primaryDomainandfallbackDomains, withinitmarked unavailable to avoid partially‑initialised instances. You might optionally mark the custom initializer asNS_DESIGNATED_INITIALIZERin the implementation to help static analysis, but the current shape is already solid.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
4031-4071: Custom fallbackHosts test matches new primary + fallback domain scheme, but comment is staleThe logic in
test__089__...applies_when_an_array_of_ClientOptions_fallbackHosts_is_providedcorrectly:
- Uses
main.realtime.ably.netas the primary host.- Verifies that only the user‑supplied
[f-j].ably-realtime.comhosts are used as fallbacks.The inline comment
// wss://main.[a-e].fallback.ably-realtime.com: when a timeout occursno longer reflects this behavior (custom fallbacks are used instead of the defaultmain.[a-e].fallback.ably-realtime.comset), so it may be worth updating or removing it to avoid confusion.Source/ARTClientOptions.m (1)
314-316: Consider allowingsetDefaultEndpoint:to be used more explicitly in testsThe static
+setDefaultEndpoint:setter aroundARTDefaultEndpointis straightforward and gives a hook for global default changes. If you expect tests or internal callers to toggle this during a process, it may be worth documenting its intended usage (e.g., “test-only” vs. production) near the declaration to avoid accidental runtime changes in app code, but the implementation itself is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(2 hunks)Source/ARTRest.m(6 hunks)Source/ARTWebSocketTransport.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (9)
- Source/include/module.modulemap
- .github/workflows/integration-test.yaml
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/Ably/ARTDefault.h
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Source/ARTDomainSelector.m
- Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
- Source/Ably.modulemap
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (5)
match(1516-1524)getTestEndpoint(626-635)at(1431-1433)at(1440-1445)extract(1526-1537)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (31)
Scripts/log-environment-information.sh (1)
3-3: Domain updates correctly reflect ADR-119 sandbox configuration.The changes consistently update domain references from
sandbox-realtime.ably.iotosandbox.realtime.ably-nonprod.netacross the comment, echo label, and dig command. This aligns with ADR-119's nonprod sandbox endpoint naming and directly addresses the reviewer feedback about updating the script to reflect the new domain.Also applies to: 10-10, 11-11
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
115-141: UsingprimaryDomain()for app provisioning is consistent with endpoint-based defaultsBuilding the test app setup URL from
options.primaryDomain()andoptions.tlsPortaligns the provisioning call with the same domain-selection policy the client itself uses, rather than legacy environment hosts. This should keep tests representative of real client behavior under ADR‑119.
144-159: Centralising test endpoint selection viagetTestEndpoint()looks good; please confirm ABLY_ENV expectationsRouting all test
ARTClientOptionsthroughoptions.endpoint = getTestEndpoint()and resolving that fromABLY_ENDPOINT/ABLY_ENV(with default"nonprod:sandbox") cleanly unifies how tests select domains under the new endpoint model. The only behavior change to watch is thatABLY_ENVis now interpreted as anendpointvalue (e.g."sandbox"), not as the deprecatedenvironmentoption; if any CI or local setups relied on non‑sandboxABLY_ENVvalues, it’s worth double‑checking that the new mapping still does what you expect.Also applies to: 626-635
598-607: JWT echo request no longer sendsenvironment; ensure the backend no longer depends on itThe JWT token request now omits any
environmentquery item and only includesencryptedin addition to the existing fields. That matches the move away from environment‑based routing, but it does rely on the echo endpoint atechoServerAddressnot requiring the old parameter; your JWT tests will catch any mismatch, but it’s worth confirming against whatever backshttps://echo.ably.io/createJWT.Source/ARTWebSocketTransport.m (1)
247-253: Routing WebSocket host viaendpoint/primaryDomainmatches the new domain modelUpdating
setHostto write tooptions.endpointand the getter to returnoptions.primaryDomainkeeps reachability and reconnect logic aligned withARTDomainSelector. Forced host overrides (e.g. fallback reconnects) still work as before because a concrete hostname passed as the endpoint will be echoed back as the primary domain with no further fallbacks.Examples/Tests/TestsTests/TestsTests.swift (1)
20-83: Example test updated consistently to the new non‑prod endpoint and ably.comProvisioning against
sandbox.realtime.ably-nonprod.net, configuringARTClientOptions.endpoint = "nonprod:sandbox", and switching the background connectivity pings tohttps://ably.comkeeps this example in line with the new routing/domain scheme and the rest of the test harness. Nothing stands out as problematic here.Source/ARTRest.m (3)
375-381: HTTP fallback behaviour now correctly tracks timeout, prioritised host, and endpoint-derived domains
- On
fallbackRetryExpirationelapsing, you now clearcurrentFallbackHostandprioritizedHostand rewrite the request host back tooptions.primaryDomain, which matches RSC15f’s requirement to return to the primary domain after the retry window.- When a request succeeds while using a fallback (
currentFallbackHostnon‑nil), settingprioritizedHost = currentFallbackHostensures subsequent requests continue to hit the known‑good fallback until the retry window expires.- Building
ARTFallbackfromoptions.fallbackDomainsties REST’s fallback hosts directly to the DomainSelector logic and naturally disables fallback for endpoints (e.g. custom hostnames, localhost, IPs) where no REC2 fallbacks are defined.currentHost’s default path now also usesoptions.primaryDomain, so anything introspecting the “current host” sees the same endpoint‑driven value ARTRest actually uses.This all looks coherent with the new domain‑selection model.
Also applies to: 440-452, 460-462, 514-520
676-689:internetIsUphonoursconnectivityCheckUrlwhile defaulting toARTDefaultSwitching the connectivity check to
self.options.connectivityCheckUrl ?: ARTDefault.connectivityCheckUrllets callers override the endpoint when needed while keeping a single canonical default. The rest of the logic (expecting200and bodyyes\n) is unchanged, so behaviour should remain compatible apart from the hostname.
785-841: Shared device access is correctly serialised throughdeviceAccessQueueIntroducing
+deviceAccessQueueand funnelling all access to the staticdeviceinstance andsharedDeviceNeedsLoading_onlyAccessOnDeviceAccessQueueflag through that dedicated serial queue makes the APNS device state consistent across multipleARTRestInternalinstances. As long as callers avoid invokingdevice_nosyncfrom code already running ondeviceAccessQueue, this should prevent subtle races around device loading/reset.Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
1-440:ARTDomainSelectorTestsgive comprehensive coverage of domain and fallback selectionThese tests exercise the main REC1/REC2 paths (default endpoint, hostnames, IPs, localhost, nonprod/prod routing IDs), fallback generation (custom fallbacks, default fallbacks, “no fallback” cases, nonprod fallbacks), and legacy/deprecated options (environment, restHost, realtimeHost, fallbackHostsUseDefault) including precedence rules (endpoint > environment > restHost). That’s a solid net around the new selector; any mistakes in
ARTDomainSelector’s mapping logic should show up here quickly.Source/ARTRealtime.m (1)
1521-1525: Realtime fallback retries now correctly hinge onfallbackDomainsavailability
shouldRetryWithFallbackForErrorcontinues to gate fallback on 5xx/host‑unreachable/timeout transport errors, but now also checksoptions.fallbackDomains.count > 0(RSC15m). That prevents pointless fallback attempts for endpoints that, by design, have no REC2 fallbacks (e.g. custom hostnames, localhost, IPs).- In
realtimeTransportFailed:withError:, initialising_fallbacksfromclientOptions.fallbackDomainsbrings realtime into line with the new DomainSelector‑ and endpoint‑driven fallback lists used by REST.Together with the ARTRest changes, this keeps fallback behaviour consistent across transports and tied to the configured endpoint semantics.
Also applies to: 1670-1689
Test/AblyTests/Tests/RestClientTests.swift (8)
45-46: LGTM! Host patterns correctly updated to new endpoint-based domains.The test assertions now correctly expect the new domain patterns (
main.realtime.ably.netfor primary andmain.[a-e].fallback.ably-realtime.comfor fallbacks) as per ADR-119.Also applies to: 66-67, 112-112, 134-136
177-177: LGTM! Correct use of endpoint-based configuration APIs.The tests properly use
primaryDomain()accessor andsetDefaultEndpoint()class method with appropriate cleanup viadefer.Also applies to: 201-202, 685-687
318-330: LGTM! Correctly validates endpoint option incompatibility with legacy options.Test
test__026a__RestClient__if_endpoint_option_is_specified_and_any_of_deprecated_options_are_also_specified_then_options_are_invalidproperly enforces REC1b1 by verifying that settingendpointthrows exceptions when combined withenvironment,restHost, orrealtimeHost.
333-351: LGTM! Comprehensive coverage of endpoint value types.The three tests (
test__026b__,test__026c__,test__026d__) correctly validate all endpoint value types as specified in ADR-119:
- Domain names (REC1b2)
- Nonprod routing policies (REC1b3)
- Production routing policies (REC1b4)
355-390: LGTM! Proper backward compatibility testing for deprecated options.Tests correctly verify:
- REC1c1: Mutual exclusivity of deprecated
environmentwithrestHost/realtimeHost- REC1c2: Legacy
environmentoption behavior and default primary domain migration tomain.realtime.ably.net- REC1d1, REC1d2: Legacy
restHost/realtimeHostoptions serve as primary domainAppropriate use of
@available(*, deprecated)to suppress compiler warnings.
393-403: LGTM! Default endpoint and configuration tests updated correctly.Tests now verify:
- REC1a (default): Default primary domain is
main.realtime.ably.net(line 402)- REC1a (overridden): Custom endpoint configuration properly overrides the default (lines 515-517, 530)
- Proper use of
endpointoption for test scenarios (line 431)Also applies to: 431-431, 512-531
875-893: LGTM! Comprehensive fallback domain testing with new endpoint semantics.All fallback behavior tests correctly updated to:
- Use new domain patterns (
main.realtime.ably.net,main.[a-e].fallback.ably-realtime.com)- Cover REC2 spec requirements (REC2a1-REC2c6)
- Test routing policy-specific fallback patterns
- Validate backward compatibility with deprecated fallback options
Also applies to: 933-1146, 1149-1253
464-464: LGTM! All host pattern references consistently updated.Remaining test assertions and helper usages correctly reflect the new endpoint-based domain structure:
- Primary domain:
main.realtime.ably.net- Fallback domains:
main.[a-e].fallback.ably-realtime.com- Proper use of
fallbackDomains()accessor for dynamic fallback retrievalAlso applies to: 954-956, 1008-1010, 1036-1038, 1065-1066, 1092-1094, 1121-1121, 1180-1182, 1258-1294, 1333-1369, 1406-1406, 1659-1659, 1948-1948
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is correctly wired into the Xcode project across all targets
ARTDomainSelector.m/.hhave properPBXFileReferenceentries and are grouped underUtilitiesconsistently with similar helper types.- Each platform (iOS, macOS, tvOS) has its own
PBXBuildFileforARTDomainSelector.min the correspondingSourcesphase andARTDomainSelector.hin the correspondingHeadersphase, withATTRIBUTES = (Private, )for the headers, which matches the intended internal API surface.- There are no dangling references: every new build-file ID for
ARTDomainSelectoris used exactly once in the appropriate build phase, and all references point to the correct file-ref IDs.From the project-graph perspective, this looks complete and coherent; no changes requested here.
Also applies to: 1224-1225, 1993-1994, 2142-2142, 2318-2318, 2527-2527, 2966-2966, 3110-3110, 3254-3254
Source/include/Ably/ARTClientOptions.h (1)
22-26: Endpoint and connectivity-check options look consistent with ADR-119The new
endpointandconnectivityCheckUrlproperties, along with the deprecation messaging forrestHost,realtimeHost, andenvironment, form a coherent public surface and clearly steer users toward the endpoint-first model. No functional or API-shape issues from this header change.Also applies to: 28-33, 35-38, 50-55, 61-64
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (6)
181-195: Updated default-host expectation matches new endpoint defaultAsserting
url.host == "main.realtime.ably.net"intest__017__Connection__url__should_connect_to_the_default_hostlines up with the new endpoint-based default primary domain and gives a clear regression guard for future domain changes.
2178-2203: Usingendpointwith a non‑routable IP in RTN14c timeout test is appropriateConfiguring
options.endpoint = "10.255.255.1"to simulate an unreachable host keeps the test aligned with the endpoint‑first model while still exercising the RTN14c timeout path; the timing assertions remain valid.
4187-4260: Endpoint-specific primary/fallback host tests give strong coverage of REC2c behavior*The
_test__091__...every_connection_is_first_attempted_to_the_primary_host...helper plus its three wrappers for:
- default (nil) endpoint,
nonprod:sandbox,test,nicely exercise that:
- The first connection always targets the endpoint-derived primary domain (prod vs nonprod routing),
- The second connection moves to the corresponding fallback domain (
[a-e].fallback.ably-realtime(.nonprod).com),- And failures due to invalid credentials are surfaced as
.failedwith the expected error codes.The structure is clear and will make future domain changes easier to validate.
4262-4353: Randomized fallback sequence tests look sound and deterministic via injected shuffle
_test__092__...should_retry_hosts_in_random_order_after_checkin_if_an_internet_connection_is_availableuses:
options.testOptions.shuffleArray = shuffleArrayInExpectedHostOrder,options.fallbackDomains()to deriveexpectedFallbackHosts,- Combined WebSocket + HTTP URL capture and the
internet-up.ably-realtime.comcheck,to assert that fallback hosts are retried in the expected order for:
- default endpoint,
nonprod:sandbox,test.Given the injected shuffle and explicit parsing of hostnames, these tests should be deterministic while still approximating “random” behavior.
4471-4512: Empty fallbackHosts disabling fallback behavior is well specified
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedclearly defines the semantics thatfallbackHosts = []:
- Still allows retries, but only to the primary host.
- Produces the
.connecting → .disconnected → .connectingsequence with two connection attempts, both tomain.realtime.ably.net.Good regression protection for the empty‑list edge case in the domain selector.
4837-4867: Reachability tests now correctly bind toprimaryDomain()The three tests
test__109__,test__110__, andtest__106_b__now assert that:
TestReachability.hostequalsclient.internal.options.primaryDomain(), and- Reachability transitions drive the expected
.disconnected/ reconnection behavior.This keeps the reachability wiring aligned with the endpoint/domain-selection logic instead of legacy host/environment options, and is a good long‑term coupling point.
Also applies to: 4872-4903, 4907-5012
Source/ARTClientOptions.m (4)
33-38: Endpoint/domainSelector integration and legacy-option guards look correctThe new ivars (
_endpoint,_restHost,_realtimeHost,_environment), lazydomainSelector, and:
setEndpoint:rejecting any existingenvironment/restHost/realtimeHost,setEnvironment:rejecting existingendpointor host overrides,setRestHost:/setRealtimeHost:rejecting an existingendpointorenvironment,- Resetting
_domainSelectorin all of these setters and ininitDefaults,establish a clean invariant: only one of the legacy environment/host options or the new
endpointcan be active, and the domain selector always recomputes from the latest configuration. UsingprimaryDomainfor bothrestUrlComponents.hostandrealtimeUrl.hostalso matches the updated tests and consolidates domain resolution.Also applies to: 40-54, 80-82, 90-99, 105-126, 130-191
193-211: Copy semantics for endpoint, connectivityCheckUrl, and legacy hosts are now completeIn
copyWithZone::
options.endpoint = self.endpoint;ensures endpoint-based configuration is preserved for test helpers likeAblyTests.newRealtime(_:).options.connectivityCheckUrl = self.connectivityCheckUrl;fixes the previously noted omission so connectivity checks survive a copy.- The conditional copies of
_restHost,_realtimeHost, and_environmentvia their setters maintain legacy behaviors while still going through the new validation/reset logic.This brings
ARTClientOptionscopies in line with the new endpoint/domain-selection design.Also applies to: 213-261
296-305: Fallback-host mutators correctly invalidate domainSelectorBoth
setFallbackHosts:andsetFallbackHostsUseDefault:now reset_domainSelectorafter updating internal state. Together with the new tests around custom fallback hosts and emptyfallbackHosts, this ensures that any change in fallback configuration will be reflected in subsequentprimaryDomain/fallbackDomainsusage without stale state.
331-343: Environment helpers remain legacy-focused but are safely isolated
isProductionEnvironmentandhasEnvironmentstill consult the deprecatedenvironmentoption only, with deprecation warnings suppressed. That’s appropriate for legacy behavior and avoids conflating endpoint-based configuration with the old environment model.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Test/AblyTests/Tests/RestClientTests.swift (1)
1069-1095: Bug: third fallback assertion uses index 1 instead of 2In
test__061__...endpoint_fallback_hosts_when_ClientOptions_endpoint_is_set_to_a_routing_policy, the third assertion repeatscapturedURLs.at(1)instead of checking the third request:XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(0), pattern: "//test.realtime.ably.net")) XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) // <- should check index 2This leaves the third URL unvalidated and redundantly re‑checks the second.
Apply this minimal fix:
- XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) + XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(2), pattern: "//test.[a-e].fallback.ably-realtime.com"))
♻️ Duplicate comments (3)
Source/ARTWebSocketTransport.m (1)
248-254: Verify setter/getter asymmetry in host property.The
setHost:method assigns tooptions.endpoint(line 249), but thehostgetter returnsoptions.domainSelector.primaryDomain(line 253). This creates an asymmetric property where setting and getting may not round-trip correctly.If
domainSelectortransforms theendpointvalue, code liketransport.host = "foo.com"; assert(transport.host == "foo.com")might fail unexpectedly.Additionally, a previous review comment noted that the transport is misusing client options for hostname configuration and suggested passing the hostname as an argument instead.
Verify that this asymmetry is intentional and document the transformation behavior, or consider addressing the architectural concern raised in the previous review by refactoring hostname configuration.
Test/AblyTests/Tests/RestClientTests.swift (2)
181-181: TODO onpublishTask.erroris already trackedThe TODO about
publishTask.errorstarting asnilis covered by existing GitHub issue #2145; leaving it as‑is in this PR is reasonable.
201-202: Default endpoint override around token initializer looks correctTemporarily overriding
ARTClientOptions.setDefaultEndpoint(getTestEndpoint())for the token initializer test ensures theARTRest(token:)path uses the same endpoint logic as the rest of the suite; restoring it indeferavoids cross‑test contamination. The TODO onpublishTask.erroris already tracked by issue #2145.Also applies to: 206-206
🧹 Nitpick comments (7)
Source/include/Ably/ARTDefault.h (1)
10-16: Clarify how default hosts/fallbacks relate to the new endpoint/domainSelector modelThe new
fallbackHosts/fallbackHostsWithEnvironment:and therestHost/realtimeHostaccessors expose host information that is now driven byARTDomainSelector. It would help future maintainers and SDK users if the header briefly documented:
- That these values are derived from the same routing/domain-selection logic as
endpoint(viaARTDomainSelector), and- For the legacy host methods, whether their “default” values refer to the legacy
.ably.iodefaults or the new.ably.net/ably‑realtime.comdefaults when no legacy options are explicitly configured.This is purely a documentation clarity point; the API surface itself looks fine.
Source/include/Ably/ARTClientOptions.h (1)
22-27: Document thatendpointmust not be combined withenvironment/restHost/realtimeHostThe
endpointoption’s doc correctly explains routing-policy and FQDN forms, but ADR‑119 also requires thatendpointbe invalid when used together with the legacyenvironment,restHost, orrealtimeHostoptions.Consider adding a brief note here (and/or on the deprecated properties) stating that these options are mutually exclusive and that combining them will result in an error. That keeps the public header self‑contained and aligned with the ADR, independent of the implementation and tests.
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
23-56: Fallback-host expectations now correctly track default primary and REC2 fallbacksThe updated assertions in
testUsesAlternativeHostOnResponse:
- Expect the first connection to hit
main.realtime.ably.net, and- The second to hit a
main.[a-e].fallback.ably-realtime.comhost,which matches the new default
ARTDomainSelectorprimary and fallback domains.One tiny nit: the inline comment still refers generically to “default host” / “when a timeout occurs” without mentioning the specific
main.realtime.ably.net→main.[a-e].fallback.ably-realtime.compattern. Consider tightening the wording to match the new behavior for future readers, but the test logic itself looks solid.
4187-4260: Endpoint‑aware primary/fallback host coverage looks good; minor test-name nitThe helper
_test__091__…primary_host_main_realtime_ably_net(endpoint:test:)plus its three wrappers nicely exercise:
- Default endpoint (
nil) →main.realtime.ably.netandmain.[a-e].fallback.ably-realtime.com.- Nonprod routing policy (
"nonprod:sandbox") →sandbox.realtime.ably-nonprod.netandsandbox.[a-e].fallback.ably-realtime-nonprod.com.- Production routing policy (
"test") →test.realtime.ably.netandtest.[a-e].fallback.ably-realtime.com.One minor polish point: the third test function name ends with
_test_realtime_ably_nonprod_neteven though it’s assertingtest.realtime.ably.net. Renaming it to end in_test_realtime_ably_netwould avoid confusion; the behavior is already correct.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
18-23: Consider documentingdomainSelectorandsetDefaultEndpoint:as internal/test-facing hooksThe new
domainSelectorproperty and+setDefaultEndpoint:class method form the internal bridge betweenARTClientOptionsand the REC1/REC2 domain logic.Given they’re exposed in a private header:
- It may be worth adding a brief comment indicating that
setDefaultEndpoint:exists primarily for tests (to change the SDK‑wide default endpoint) and should not be used in production code paths.- Likewise, a short doc on
domainSelector(e.g. “Provides the resolved primary domain and fallbacks for these options”) would make its intent clearer wherever it’s consumed.No functional issues here; this is just for maintainability.
Source/ARTDomainSelector.m (1)
66-118: Primary-domain policy selection is correct; consider broadening hostname detection for IPv6 literalsThe policy selection logic cleanly implements the intended precedence:
endpoint(hostname vs nonprod vs production routing policy),- legacy
environment,- legacy
restHost/realtimeHost,- default.
For
endpoint, hostnames are detected via".","::", or"localhost". That works for:
- FQDNs and IPv4 literals (contain
"."),- IPv6 literals that use the
::shorthand,- The special
localhostcase.If you ever want to support full IPv6 literals that don’t contain
::(only:), they’ll currently be misclassified as production routing policy IDs. A small tweak such as also checking for":"(or a more explicit IP/hostname detection helper) would make this more robust, while keeping the existing behavior unchanged for routing-policy strings.Not urgent if the
endpointcontract is “routing policy or FQDN only”, but worth considering.Source/ARTDefault.m (1)
25-54: Consider caching ARTDomainSelector instances for efficiency.Each call to
fallbackHostsWithEnvironment,restHost, andrealtimeHostcreates a new ARTDomainSelector instance. While this is functionally correct and likely acceptable for infrequent calls during client initialization, repeated calls could be inefficient.If profiling shows these methods are called frequently, consider caching default ARTDomainSelector instances (e.g., using static variables or a singleton pattern) to avoid repeated allocations.
Example approach:
+ (NSString *)restHost { static ARTDomainSelector *defaultSelector = nil; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ defaultSelector = [[ARTDomainSelector alloc] initWithEndpointClientOption:nil fallbackHostsClientOption:nil environmentClientOption:nil restHostClientOption:nil realtimeHostClientOption:nil fallbackHostsUseDefault:false]; }); return defaultSelector.primaryDomain; }However, only implement this optimization if measurements indicate it's beneficial, as the current implementation is simpler and clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(3 hunks)Source/ARTRest.m(7 hunks)Source/ARTWebSocketTransport.m(2 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/integration-test.yaml
- Test/AblyTests/Tests/ARTDomainSelectorTests.swift
- Test/AblyTests/Test Utilities/TestUtilities.swift
- Examples/Tests/TestsTests/TestsTests.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)Test/AblyTests/Test Utilities/TestProxyTransportFactory.swift (1)
transport(12-24)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
match(1516-1524)getTestEndpoint(626-635)getTestToken(505-507)getTestToken(509-513)at(1431-1433)at(1440-1445)clientOptions(144-159)extract(1526-1537)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (54)
Source/include/module.modulemap (1)
142-142: LGTM: Module map updated for domain selector.The replacement of ARTFallbackHosts.h with ARTDomainSelector.h correctly reflects the architectural migration to endpoint-based domain selection.
Source/Ably.modulemap (1)
142-142: LGTM: Xcode module map updated consistently.The module header replacement is consistent with the SPM module map changes.
Source/ARTWebSocketTransport.m (1)
23-23: LGTM: Import added for domain selector.The import of ARTDomainSelector.h supports the new domain resolution mechanism.
Test/AblyTests/Tests/PushAdminTests.swift (1)
236-236: LGTM: Test updated for domain-based API.The change from
options.restHosttooptions.primaryDomain()correctly reflects the migration to endpoint-based domain selection.Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-42: LGTM: Well-designed domain selector interface.The ARTDomainSelector header provides a clean abstraction for domain selection logic:
- Clear initialization contract accepting all domain-affecting options
- Readonly properties expose computed primary and fallback domains
- Proper nullability annotations and documentation
Note: The static analysis error about Foundation/Foundation.h is a false positive in the review context and can be safely ignored.
Scripts/log-environment-information.sh (1)
3-11: LGTM: CI script updated for new nonprod domain.The script correctly updates all references from
sandbox-realtime.ably.iotosandbox.realtime.ably-nonprod.net, aligning with the ADR-119 domain migration.Source/PrivateHeaders/Ably/ARTDefault+Private.h (2)
3-3: LGTM: Constant renamed for clarity.Renaming
ARTDefaultProductiontoARTDefaultProductionEnvironmentimproves naming consistency with the environment-based domain selection logic.
13-14: LGTM: Connectivity check URL accessor added.The new
connectivityCheckUrlmethod provides a centralized way to obtain the connectivity check URL, supporting the domain-based configuration approach.Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-180: LGTM: Test properly annotated for deprecated API.The deprecation annotation appropriately suppresses compiler warnings while preserving test coverage for the legacy
realtimeHostoption. The explanatory message clearly documents the rationale.Source/ARTRealtime.m (3)
57-57: Domain selector import is appropriateARTDomainSelector is now the source for fallback domains via ARTClientOptions, so importing it here is appropriate and keeps the dependency explicit.
1521-1526: Fallback retry guard now correctly depends on domainSelector fallback domainsThe updated
shouldRetryWithFallbackForErrorgate that checksoptions.domainSelector.fallbackDomains.count > 0aligns fallback behavior with the configured domain set (RSC15m) and avoids creating ARTFallback when there are no fallbacks. This is consistent with the new REC2c* tests.
1671-1688: Realtime fallback host list now sourced from domainSelectorUsing
clientOptions.domainSelector.fallbackDomainsto initialize_fallbackscentralizes all runtime fallback host behavior on ARTDomainSelector. Combined with the RSC15m guard and the existing suspend‑mode check, this keeps fallback behavior consistent with the new endpoint model.Test/AblyTests/Tests/RestClientTests.swift (22)
45-47: Host and fallback URL expectations updated correctly to new domainsThe updated regex expectations and host checks for primary (
main.realtime.ably.net,test.realtime.ably.net, etc.) and fallback domains (main.[a-e].fallback.ably-realtime.com,test.[a-e].fallback.ably-realtime.com, custom fallback hosts) are consistent with the new domain model and ARTDomainSelector behavior, while preserving the existing test semantics for retry paths and fallback usage.Also applies to: 65-67, 111-113, 134-135, 462-465, 955-957, 1008-1009, 1036-1038, 1065-1066, 1121-1121, 1145-1146, 1180-1182
177-177: Prioritized host now aligned with domainSelector primaryDomain in key‑initializer testSetting
client.internal.prioritizedHost = options.domainSelector.primaryDomainensures this initializer test exercises the same primary domain resolution as the main options/domainSelector pipeline, avoiding any hidden dependence on legacy default host values.
301-303: REC1 tests correctly encode endpoint vs deprecated options and primaryDomain derivationThe new REC1 tests:
- Enforce that
endpointcannot be combined with deprecatedenvironment,restHost, orrealtimeHost(throwing viatryInObjC),- Verify that domain‑style endpoints map directly to
primaryDomain,- Verify nonprod (
nonprod:sandbox) and production routing policy names (test) map to the expected primary domains.These are accurate and give solid coverage for the new configuration rules.
Also applies to: 316-337, 340-351
353-374: REC1c/d tests validate deprecated options’ impact on primaryDomainThe added tests around
environment,restHost, andrealtimeHostconfirm:
- Deprecated options remain mutually incompatible where specified,
primaryDomainchanges as expected when each is set,- The previous default primary domain is
main.realtime.ably.net.This matches the intended backward‑compatibility and migration behavior.
Also applies to: 376-390
392-403: Default endpoint test correctly asserts https://main.realtime.ably.net
test__028__now explicitly checks that the default REST endpoint begins withhttps://main.realtime.ably.net, which aligns with ADR‑119 and the new default primary domain.
418-424: Legacy restHost/realtimeHost default test properly marked deprecatedMarking
test__030__...options_restHost_realtimeHost_are_default_values_if_not_setas deprecated to avoid warnings while still validating legacy defaults is a pragmatic way to keep this coverage until the properties are removed.
431-432: Using endpoint to force a non‑routable IP in timeout test is appropriateSetting
options.endpoint = "10.255.255.1"is a clear way to exercise HTTP timeout behavior via the new endpoint mechanism instead of legacy host options, while keeping the rest of the test logic unchanged.
511-518: Endpoint override test robustly validates primaryDomain and request URL
test__003__...endpoint_should_be_configurable...correctly:
- Asserts the default primary domain is
main.realtime.ably.net,- Overrides
endpointand re‑assertsprimaryDomain,- Verifies the outbound URL includes the overridden host.
Good direct coverage of endpoint→primaryDomain wiring.
Also applies to: 528-531
685-688: Default endpoint override for basic auth test keeps behavior predictableTemporarily calling
ARTClientOptions.setDefaultEndpoint(getTestEndpoint())before constructingARTRest(key:), then restoring it indefer, ensures this basic‑auth test runs against the same configurable endpoint pipeline as other tests while remaining isolated.
875-894: fallbackHostsUseDefault test now asserts domainSelector.fallbackDomains explicitlyUsing
client.internal.options.domainSelector.fallbackDomainsand checking for the fivemain.[a-e].fallback.ably-realtime.comdomains accurately verifies thatfallbackHostsUseDefaultroutes through DomainSelector and still produces the expected default fallback set.
932-957: Default primary domain fallback behavior test matches RSC15m/REC2c1
test__055__verifies that with the default primary domain:
- One primary request is followed by two fallback attempts on
main.[a-e].fallback.ably-realtime.comwhen the host is unreachable,- The total request count is 3.
This is consistent with the new fallback domain model.
959-984: Explicit endpoint with no fallbacks correctly yields a single failed attempt
test__056__confirms that whenoptions.endpointis set to an explicit domain (fake.ably.io),domainSelector.fallbackDomainsis empty and no fallback attempts occur (single request, hostUnreachable). This matches REC2c2.
986-1011: Fallback behavior with explicit ClientOptions.fallbackHosts is preserved
test__059__checks that:
- The first attempt goes to the default primary (
main.realtime.ably.net),- Subsequent retries target the custom
a.cocoa.ably/b.cocoa.ablyfallback hosts.This validates that DomainSelector respects explicitly provided
fallbackHosts.
1013-1039: fallbackHostsUseDefault + no explicit fallbackHosts still uses default fallback list
test__060__ensures that whenfallbackHostsUseDefaultis true andfallbackHostsis nil:
- Requests cycle through the primary + default fallback hosts,
- The URL patterns match
main.realtime.ably.netandmain.[a-e].fallback.ably-realtime.com.This is the expected REC2b/TO3k7 behavior.
1121-1121: Updated default fallback host pattern in legacy environment test is consistentThe environment‑based fallback test now checks for
main.[a-e].fallback.ably-realtime.com, which is consistent with the new default fallback domain scheme while still validating behavior in the deprecatedenvironment + fallbackHostsUseDefaultpath.
1124-1146: Empty fallbackHosts array correctly disables fallback behavior
test__047__setsoptions.fallbackHosts = []and asserts only one request to the primary domain. This confirms that an explicitly empty list prevents any fallback attempts, matching RSC15m/TO3k6 expectations.
1148-1183: RSC25 test correctly validates primary‑then‑fallback‑then‑primary sequence
test__049__demonstrates:
- First attempt to
main.realtime.ably.net,- Fallback to
main.[a-e].fallback.ably-realtime.com,- After timeout, a fresh request again to the primary domain.
The URL pattern checks now reflect the new domains and still match the intended RSC25 behavior.
1185-1253: REC2c tests give comprehensive coverage of implicit fallbackDomains*The new
test__065a–test__065gsuite validatesoptions.domainSelector.fallbackDomainsfor:
- Default configuration (five
main.[a-e].fallback.ably-realtime.comdomains),- Explicit endpoint hostnames (empty list),
- Nonprod routing policy names (
sandbox.[a-e].fallback.ably-realtime-nonprod.com),- Plain routing policy names (
sandbox.[a-e].fallback.ably-realtime.com),- Deprecated
environment,restHost, andrealtimeHost.These tests collectively ensure DomainSelector’s fallbackDomains contract is well‑specified across all configuration permutations.
1259-1263: ARTDefault.fallbackHosts() assertion matches new default fallback domains
test__065__asserting that allARTDefault.fallbackHosts()entries matchmain.[a-e].fallback.ably-realtime.com(and count == 5) is consistent with the new default fallback set and ties defaults directly to the spec.
1288-1292: Ordering test now uses domainSelector.fallbackDomains as expected host listIn the “retry hosts in random order” test, computing
expectedFallbackHostsfromoptions.domainSelector.fallbackDomainsinstead of a separate constant keeps the test aligned with the actual runtime list while still validating the shuffle order againstexpectedHostOrder.
1332-1342: Header test for fallback requests now tied to domainSelector.fallbackDomains
test__072__uses a custom fallback host list and asserts:
- The number of requests is
options.domainSelector.fallbackDomains.count + 1,- All fallback requests to
[f-j].ably-realtime.comhave aHostheader equal to the request host.This is a good end‑to‑end check that Host headers are set correctly for all fallback domains produced by DomainSelector.
Also applies to: 1358-1358
1405-1407: Bad‑request test correctly updated to assert new primary domainThe “no alternative host on 400” test now checks that the single request goes to
//main.realtime.ably.net, consistent with the new primary domain, while still validating that no fallback is attempted for 4xx responses.Source/ARTRest.m (5)
19-20: Imports for ARTDefault+Private and ARTDomainSelector are justifiedThese imports are needed for
ARTDefault.connectivityCheckUrlandoptions.domainSelectorusage in this file; keeping them explicit is correct.Also applies to: 50-50
375-382: Fallback retry expiration now cleanly resets to primaryDomainWhen
fallbackRetryExpirationhas passed, clearingcurrentFallbackHostandprioritizedHostand explicitly callingreplaceHostWith:_options.domainSelector.primaryDomainensures:
- Subsequent requests target the correct primary domain from DomainSelector,
- Any lingering fallback host in the request URL is removed.
This matches the RSC15f intent and keeps behavior coherent with
currentHost.
449-453: REST fallback host list and Host header now driven by domainSelectorUsing
self->_options.domainSelector.fallbackDomainsto constructARTFallback, and then setting theHostheader to the chosen fallback host when retrying, cleanly re‑routes REST fallback behavior through DomainSelector while preserving the RSC15j requirement that the Host header matches the fallback host.Also applies to: 461-462
515-521: currentHost correctly defers to domainSelector.primaryDomainWith
_prioritizedHostcleared, returningself.options.domainSelector.primaryDomainmakes ARTRest’s notion of “current primary host” consistent with all of the endpoint/environment/restHost/realtimeHost rules encoded in DomainSelector.
677-679: internetIsUp now honors configurable connectivityCheckUrl with a sane defaultConstructing the connectivity‑check URL from
self.options.connectivityCheckUrlwhen provided, or falling back toARTDefault.connectivityCheckUrl, lets callers override the probe target (e.g. for nonprod) without changing the rest of the fallback logic.Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is cleanly wired across file refs, groups, and all three framework targets; just confirm intended header visibility.
ARTDomainSelector.mhas a properPBXFileReferenceand threePBXBuildFileentries, each used exactly once in the iOS, macOS, and tvOSSourcesphases.ARTDomainSelector.hlikewise has aPBXFileReferenceand threePBXBuildFileentries, correctly added to theHeadersphases for all three framework targets.- The files sit under
Source/Utilities, matching how other cross-cutting helpers are grouped, and there are no dangling or unused IDs.One thing to double‑check: the header lives under
PrivateHeaders/Ably/ARTDomainSelector.hand is markedATTRIBUTES = (Private)everywhere. If ADR‑119 or downstream SDKs/wrappers expectARTDomainSelectoritself to be part of the public surface (vs. only theendpoint/domainSelectoroptions on other types), you may need to:
- move the header to
include/Ably/ARTDomainSelector.h, and- flip the build‑file attributes to
Public.If it’s meant to be purely internal behind
ARTClientOptionsetc., the current Private setup is consistent and looks good.Also applies to: 1224-1225, 1993-1994, 2142-2142, 2318-2318, 2527-2527, 2966-2966, 3110-3110, 3254-3254
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
4264-4353: Randomized fallback ordering tests correctly couple todomainSelector.fallbackDomainsThe
_test__092__…retry_hosts_in_random_order…(endpoint:test:)helper:
- Derives
expectedFallbackHostsfromoptions.domainSelector.fallbackDomainsand shuffles usingexpectedHostOrder.- Observes both WebSocket connect URLs and HTTP connectivity-check URLs, ensuring:
- An
internet-up.ably-realtime.comcheck occurs between fallback host changes, and- The sequence of fallback hosts matches the shuffled
expectedFallbackHosts.This gives solid coverage for REC2/REC3 behavior across default, nonprod (
"nonprod:sandbox"), and production ("test") endpoints, without hard‑coding the fallback host list in the test itself. Nicely done; no changes needed.
4471-4512: EmptyfallbackHostscorrectly disables automatic fallbacks
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedasserts that:
- With
fallbackHosts = []and the error qualifying for fallback, the client:
- Transitions
.connecting → .disconnected → .connecting, and- Makes two connection attempts, both to
main.realtime.ably.net.This matches the intended “empty list means no fallback” semantics for
fallbackHosts, and aligns withARTDomainSelector.fallbackDomainsreturning an empty array when an explicit empty list is supplied. Looks good.
4837-5013: Reachability tests correctly validatedomainSelector.primaryDomainusageThe updated OS reachability tests:
- Use
AblyTests.newRealtime(options)so thatTestReachabilityis installed viasetReachabilityClass.- Assert that
reachability.hostequalsclient.internal.options.domainSelector.primaryDomainin:
- CONNECTING and CONNECTED states (tests
__109and__110), and- DISCONNECTED / SUSPENDED states before simulating the OS bringing the network back (tests
__106and__106_b).These changes nicely couple the reachability host to the new domain-selection mechanism across default and endpoint‑override scenarios. The logic and assertions look consistent with ARTDomainSelector; no changes needed.
Source/ARTDomainSelector.m (1)
148-214: Fallback-domain computation cleanly matches REC2 and explicit overrides
fallbackDomainsanddefaultFallbackDomainsbehave as expected:
- Explicit
fallbackHostsClientOptionalways wins.- Deprecated
fallbackHostsUseDefaultcorrectly forces the fixed default set, regardless of policy.- Default/no-override policy (
ARTPrimaryDomainSelectionPolicyDefault) usesdefaultFallbackDomains, yielding["main.a.fallback.ably-realtime.com", …].- Nonprod and production routing policies, and legacy environments, generate
<policy>.[a-e].fallback.ably-realtime(-nonprod).comhostnames.- Legacy host overrides return an empty list, so custom hosts don’t implicitly pick up Ably-managed fallbacks.
- An explicit empty
fallbackHostsClientOption([]) results in no fallbacks, which matches the tests.This neatly encodes the REC2 behavior and aligns with the updated tests; I don’t see any functional issues here.
Source/ARTClientOptions.m (8)
1-38: LGTM! Clean structural changes for endpoint-centric design.The additions of ARTDomainSelector and ARTNSArray+ARTFunctional imports, the renaming of ARTDefaultEnvironment to ARTDefaultEndpoint, and the new ivars properly support the endpoint-based domain resolution architecture.
40-82: LGTM! Proper initialization of endpoint and legacy options.The initialization correctly sets endpoint to the default while leaving legacy host options nil, ensuring clean separation between the new endpoint-based and deprecated host-based configurations.
88-127: LGTM! Robust endpoint validation and domain selector initialization.The mutual exclusion validation correctly implements ADR-119 requirements (REC1b1), ensuring endpoint cannot be used with deprecated environment, restHost, or realtimeHost options. The lazy initialization of domainSelector and proper ivar usage avoid circular dependencies, and resetting _domainSelector on endpoint changes maintains consistency.
128-191: LGTM! Comprehensive bidirectional validation for legacy options.The setters correctly enforce both REC1b1 (endpoint incompatible with deprecated options) and REC1c1 (environment incompatible with host options). The restHost and realtimeHost getters intelligently fall back to domainSelector.primaryDomain when not explicitly set, preserving backward compatibility while supporting the new endpoint-based flow.
193-211: LGTM! URL generation correctly uses primaryDomain.The migration to use
self.primaryDomainfor both REST and Realtime URLs properly centralizes domain resolution through the new domainSelector architecture.
213-261: LGTM! Copy semantics correctly preserve explicit settings.The conditional copying of restHost, realtimeHost, and environment (lines 241-243) by checking the ivars ensures only explicitly-set legacy options are propagated, while endpoint and connectivityCheckUrl are always copied. This maintains proper distinction between user-provided and default values.
263-327: LGTM! Utility methods properly support the new architecture.The hasCustomRestHost/hasCustomRealtimeHost methods correctly check ivars to distinguish explicit settings from defaults. The domainSelector resets in setFallbackHosts and setFallbackHostsUseDefault maintain consistency, and the deprecation pragmas around isProductionEnvironment and hasEnvironment appropriately handle legacy API usage.
105-118: Based on my verification of the ARTDomainSelector implementation, I can now provide the rewritten review comment:
The referenced
isEndpointFQDNmethod does not exist; however, the IPv6 detection concern remains valid in the actual implementation.The past review comment references an
isEndpointFQDNmethod that doesn't exist in the codebase. The actual logic resides inparsePrimaryDomainSelectionPolicyFromOptions(lines 72–75), where hostname detection uses a naive substring check:if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || [endpointClientOption isEqualToString:@"localhost"]) {The IPv6 detection concern is valid:
containsString:@"::"will match any string containing::, including malformed or incomplete IPv6 addresses. For robust IPv6 validation, use standard socket APIs likeinet_pton()rather than substring matching.The default FQDN edge case (where
endpointequals"main.realtime.ably.net") is handled correctly by design: the presence of a dot causes it to be treated as a hostname (REC1b2), which is the intended behavior.Source/ARTDefault.m (2)
1-13: LGTM! Constants and globals properly defined.The new constants (ARTDefaultAPIVersion, ARTDefaultProductionEnvironment, ARTDefaultConnectivityCheckUrl) and mutable globals for message size limits are well-structured and support the endpoint-based architecture.
56-131: LGTM! Default value methods and connectivity URL properly implemented.The methods correctly reference the new mutable globals for message size limits (allowing runtime configuration for testing), use appropriate DEBUG conditionals to distinguish sandbox and production defaults, and properly format the connectivity check URL with the https scheme.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Test/AblyTests/Tests/RestClientTests.swift (1)
1069-1095: Bug intest__062: second fallback assertion checks the wrong request indexIn
test__062__RestClient__Host_Fallback__fallback_hosts_list_and_priorities__should_use_endpoint_fallback_hosts_when_ClientOptions_endpoint_is_set_to_a_routing_policy__and_ClientOptions_fallbackHosts_is_not_set(), the third assertion accidentally inspectscapturedURLs.at(1)twice. It should verify the third request (at(2)), otherwise the second fallback attempt is never checked.- XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(0), pattern: "//test.realtime.ably.net")) - XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) - XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) + XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(0), pattern: "//test.realtime.ably.net")) + XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//test.[a-e].fallback.ably-realtime.com")) + XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(2), pattern: "//test.[a-e].fallback.ably-realtime.com"))
♻️ Duplicate comments (2)
Source/ARTWebSocketTransport.m (1)
23-23: Transport still mutatesARTClientOptionsto switch hostsUsing
setHost:to assignself.options.endpointandhostto readself.options.domainSelector.primaryDomainworks with the new domain-selector model, but it continues to overload client options as the transport’s mutable host source instead of passing the host in explicitly. This keeps the earlier indirection concern in place and makes the transport dependent on option-mutation side effects rather than a clear host parameter.Also applies to: 248-254
Test/AblyTests/Tests/RestClientTests.swift (1)
172-181: TODOs aroundpublishTask.errorremain tracked; OK to leave for nowThe
expect(publishTask.error).toEventually(...)checks are still ineffective (the error starts asnil), but this is already captured in issue #2145 and prior review. Given this PR’s focus on endpoint/domain behavior, deferring structural test fixes to that issue while only touching endpoint-related setup (setDefaultEndpoint/prioritizedHost) is reasonable.Also applies to: 199-207, 685-688
🧹 Nitpick comments (5)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
144-159:clientOptions(for:)now always setsendpoint; be mindful when testing legacy host/env optionsDefaulting
options.endpoint = getTestEndpoint()is a good way to push tests onto the new endpoint-based path. However, since ADR‑119 disallows mixingendpointwith legacyenvironment/restHost/realtimeHost, any tests that want to exercise legacy behavior will now need to explicitly clearendpoint(e.g.options.endpoint = nil) before setting those legacy fields.I’d recommend:
- Auditing tests that mutate
environment/restHost/realtimeHostafter callingclientOptions(for:)to ensure they clearendpointfirst; and- Optionally documenting this in a comment or via a separate helper that builds “legacy-style” options without setting
endpoint.
626-635:getTestEndpointfallback order matches ADR‑119 intent; minor robustness thoughtsThis helper cleanly prefers:
ABLY_ENDPOINT(explicit endpoint),- then
ABLY_ENV(legacy env value reused as an endpoint string),- then a default routing policy of
"nonprod:sandbox".That aligns tests with endpoint‑first configuration while preserving existing ABLY_ENV overrides.
Two minor considerations (optional):
- Avoid the forced
infoDictionary!unwrap by binding it once, to make this more robust to misconfigured test bundles.- Confirm that all existing ABLY_ENV values are valid as endpoint strings under the new resolver (or document expected forms), to avoid surprises if someone sets an older env-style value that the endpoint parser no longer accepts identically.
Otherwise this function looks appropriate for the new model.
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
4189-4260: REC2c1/3/4: helper_test__091...and wrapper tests robustly cover primary-domain selection for various endpoints
- Using
.endpoint = nil,.endpoint = "nonprod:sandbox", and.endpoint = "test"and then validating the first and (fallback) second URLs against:
//main.realtime.ably.net///main.[a-e].fallback.ably-realtime.com//sandbox.realtime.ably-nonprod.net///sandbox.[a-e].fallback.ably-realtime-nonprod.com//test.realtime.ably.net///test.[a-e].fallback.ably-realtime.com
gives good coverage of routing policy names and nonprod prefixes.Only minor nit: the function name for the
"test"case mentionsably_nonprod_netwhile the assertion checks.ably.net; consider renaming for clarity with the actual host being tested.Source/ARTDomainSelector.m (1)
148-214: Fallback logic aligns with REC2; consider minor cleanupsThe fallback resolution order and per‑policy behavior look correct and consistent with REC2 (explicit list →
fallbackHostsUseDefault→ per‑policy computed fallbacks, with no fallbacks for hostname/legacyHost cases).defaultFallbackDomainsalso matches its comment.As optional cleanups you could:
- Share the
aToElist betweenfallbackDomainsanddefaultFallbackDomainsvia a small helper or static constant.- Mark
fallbackHostsClientOptionascopyto avoid external mutation.Both are minor and not blocking.
Source/ARTClientOptions.m (1)
105-126: DomainSelector integration looks good; just keep thread-safety and docs in mindThe lazy
domainSelectorconstruction and its invalidation insetEndpoint:,setEnvironment:,setRestHost:,setRealtimeHost:,setFallbackHosts:, andsetFallbackHostsUseDefault:are all consistent and ensure domain selection reflects current options. The newprimaryDomain/fallbackDomainsaccessors, URL construction usingprimaryDomain, and defaulting ofconnectivityCheckUrlfromARTDefaultalso look coherent.Two optional suggestions:
- If ARTClientOptions is ever accessed from multiple threads concurrently, consider making
domainSelectorinitialization thread-safe (e.g.dispatch_onceor a small lock around the lazy init).- Given
+setDefaultEndpoint:is primarily a test helper, a short comment in the header could prevent accidental production use.These are non-blocking.
Also applies to: 168-191, 280-296
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientOptions.m(9 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(3 hunks)Source/ARTRest.m(7 hunks)Source/ARTWebSocketTransport.m(2 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/ARTFallbackHosts.m
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
🚧 Files skipped from review as they are similar to previous changes (4)
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/module.modulemap
- .github/workflows/integration-test.yaml
- Scripts/log-environment-information.sh
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
match(1516-1524)at(1431-1433)at(1440-1445)data(934-936)newRealtime(184-198)clientOptions(144-159)extract(1526-1537)commonAppSetup(107-142)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
Test/AblyTests/Tests/RealtimeClientTests.swift (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
deprecated(3843-3890)deprecated(3991-4029)deprecated(4074-4111)Test/AblyTests/Tests/RestClientTests.swift (13)
deprecated(302-314)deprecated(317-330)deprecated(354-364)deprecated(367-374)deprecated(377-382)deprecated(385-390)deprecated(418-424)deprecated(865-894)deprecated(896-900)deprecated(903-930)deprecated(1014-1039)deprecated(1098-1122)deprecated(1226-1235)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (37)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
115-136: Endpoint-based app bootstrap URL relies onprimaryDomainreturning a resolved hostUsing
options.domainSelector.primaryDomainandoptions.tlsPortto build the/appsURL keeps test app creation aligned with the same domain selection logic as the client. The only assumption is thatprimaryDomainalways yields a concrete hostname (not a routing policy token likenonprod:sandbox), otherwiseURL(string:)could fail or produce an unexpected host. Likewise, this hardcodes HTTPS and the TLS port regardless of any lateroptions.tlsoverrides.Consider double-checking that:
primaryDomainis always a valid host string for all supported endpoint forms; and- no tests rely on toggling
tlsbefore this call.If both hold, this change looks safe and correctly aligned with ADR‑119.
598-607: JWTencryptedquery item change is behavior‑neutralThe addition/adjustment of the
"encrypted"query item usesString(encrypted)as before; there’s no functional change here, and it remains consistent with the other query parameters.Source/PrivateHeaders/Ably/ARTDefault+Private.h (2)
3-3:ARTDefaultProductionEnvironmentconstant fits endpoint/domain terminologyIntroducing
ARTDefaultProductionEnvironmentas a named constant clarifies that this string represents the production environment identifier, which is helpful in the new endpoint/domain-selection world.Just ensure:
- Its definition in the implementation file matches the existing production endpoint/environment semantics; and
- Any uses of any older
ARTDefaultProduction-style constants have been updated accordingly.
7-14:+connectivityCheckUrlcentralizes connectivity URL derivationAdding
+ (NSString *)connectivityCheckUrltoARTDefault (Private)is a sensible way to keep connectivity-check URL formatting consistent with the new domain/endpoint logic and default environments.When wiring up the implementation, it’s worth confirming that:
- It always returns a full URL (scheme + host + path) compatible with the platform networking stack; and
- It delegates to or stays in sync with whatever logic
ARTDomainSelectoruses for connectivity checks, to avoid having two divergent sources of truth.Ably.xcodeproj/project.pbxproj (3)
360-365: ARTDomainSelector PBXBuildFile + PBXFileReference wiring looks correct
ARTDomainSelector.m/.hhave matchingPBXFileReferenceentries, threePBXBuildFileentries (one per target), and are grouped underUtilities, following the existing project conventions with no dangling IDs.Also applies to: 1224-1225, 1993-1994
2142-2142: ARTDomainSelector.h correctly added as a Private header for all frameworksThe header build-file IDs for iOS, macOS, and tvOS all point to
ARTDomainSelector.hwithATTRIBUTES = (Private,), so the type is available internally across platforms without leaking into the public API.Also applies to: 2318-2318, 2527-2527
2966-2966: ARTDomainSelector.m included in all framework Sources phasesEach framework target’s
Sourcesphase references a distinctARTDomainSelector.m in Sourcesbuild file, ensuring the implementation is compiled for iOS, macOS, and tvOS consistently.Also applies to: 3110-3110, 3254-3254
Source/Ably.modulemap (1)
141-143: ExposeARTDomainSelectorvia Private module looks correctAdding
ARTDomainSelector.hto theAbly.Privatemodule aligns with the new domain-selection internals and keeps it available to tests/internal users without changing the public surface.Examples/Tests/TestsTests/TestsTests.swift (1)
20-21: Domain updates in example test align with new endpoint modelSwitching the sandbox provisioning URL to
sandbox.realtime.ably-nonprod.net, configuringoptions.endpoint = "nonprod:sandbox", and usinghttps://ably.comfor background reachability look consistent with the new routing/endpoint scheme and don’t introduce test fragility beyond the external service dependency already present.Also applies to: 49-51, 65-78
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
155-180: Deprecation annotation onrealtimeHosttest is appropriateMarking the
realtimeHost-based test as deprecated to avoid compiler warnings while keeping coverage until the property is removed is a reasonable transitional approach.Source/ARTRealtime.m (1)
1521-1527: Realtime fallback now correctly delegates todomainSelector
shouldRetryWithFallbackForError(_:options:)gating onoptions.domainSelector.fallbackDomains.count > 0and seeding_fallbacksfromclientOptions.domainSelector.fallbackDomainsmakes the realtime fallback behavior consistent with the new endpoint/domain model and ensures no fallback is attempted when the selector exposes an empty list (e.g. explicit host/IP).Also applies to: 1671-1688
Test/AblyTests/Tests/RestClientTests.swift (3)
41-47: Host/fallback expectations updated coherently to new domainsThe assertions in the shared helper tests now consistently expect
main.realtime.ably.netas the primary REST host andmain.[a-e].fallback.ably-realtime.comas fallback hosts, which matches the new default/fallback domain scheme and keeps the host-fallback behavior well covered.Also applies to: 65-67, 111-113, 133-137
878-895: REST fallback tests now exercisedomainSelector-driven behaviorThe updated Host Fallback tests (e.g.
test__052,test__055,test__056,test__059,test__060,test__061,test__065__, etc.) now assert against domains derived fromoptions.domainSelectorandARTDefault.fallbackHosts(), covering:
- default primary/fallback domains for
mainand routing-policy endpoints,- behavior when
fallbackHosts,fallbackHostsUseDefault, or explicit endpoint/host overrides are set,- implicit fallback-domain generation for default, environment, nonprod and sandbox endpoints.
These look internally consistent and give good coverage for the new endpoint/fallback rules.
Also applies to: 932-957, 980-1011, 1034-1039, 1063-1067, 1087-1095, 1180-1183, 1259-1264
1789-1790: Request URL expectations updated to new default REST hostThe hard-coded URL and header expectations in the custom
requesttests now targethttps://main.realtime.ably.netand still validate headers likeX-Ably-Errorcode, keeping the low-level REST client path aligned with the new default host without weakening the assertions.Also applies to: 1858-1862
Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
9-357: Comprehensive coverage ofARTDomainSelectorbehaviorThis test suite does a good job of pinning down
ARTDomainSelectorsemantics: default vs endpoint-driven primary domains, custom vs implicit fallbacks, and precedence between endpoint, environment, restHost, realtimeHost, andfallbackHostsUseDefault. It should make future changes to domain logic much safer.Source/ARTRest.m (3)
19-20: Imports for private defaults andARTDomainSelectorare appropriateIncluding
ARTDefault+Private.handARTDomainSelector.hhere matches the new usage forconnectivityCheckUrland domain selection; no issues with coupling given ARTRest is already part of the core internal surface.Also applies to: 50-50
375-382: Fallback host reset and Host header behavior look consistent with REC/RSC requirements
- Resetting
currentFallbackHostandprioritizedHostwhenfallbackRetryExpirationhas passed and switching the request host back tooptions.domainSelector.primaryDomainaligns with RSC15f.- Using
options.domainSelector.fallbackDomainsas the source of fallback hosts, and setting theHostheader to the chosen fallback host (RSC15j), is correct and ensures the server can see the intended host even when fronted.- Given the timeout check runs when building each request (before any fallback attempt) and the
Hostheader is only set for explicit fallback retries, this flow avoids inconsistent URL/Host combinations in normal operation.Also applies to: 441-447, 449-463
515-521: Primary host and connectivity check URL now correctly tied to domain selector and options
- Updating
currentHostto returnoptions.domainSelector.primaryDomainwhen there is noprioritizedHostensures all REST host introspection reflects endpoint-based resolution.internetIsUp:usingoptions.connectivityCheckUrlwith a fallback toARTDefault.connectivityCheckUrlcleanly exposes the new configurability without changing existing behavior by default.Also applies to: 677-689
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
5-40:ARTDomainSelectorinterface cleanly centralizes domain selection policyThe initializer surface and readonly
primaryDomain/fallbackDomainsproperties give a clear, minimal contract for endpoint/environment/host resolution; the header is well-scoped for internal use.Source/include/Ably/ARTDefault.h (1)
13-19: PublicrestHost/realtimeHostaccessors remain appropriateExposing these as public methods while driving their values from the new domain-selection logic preserves the existing API surface and keeps callers insulated from the endpoint/DomainSelector refactor.
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (12)
46-56: Default host and fallback URL expectations updated correctly to new domainsThe assertions now validating a first connection to
//main.realtime.ably.netfollowed by a fallback to//main.[a-e].fallback.ably-realtime.commatch the new default primary and fallback domain scheme and keep RTN17 behavior covered.
181-192: RTN2 default-host test now aligned with endpoint-based primary domainExpecting
url.host == "main.realtime.ably.net"for a basicARTClientOptions(key:)configuration correctly reflects the new primary domain when no legacy options are set.
2178-2203: RTN14c timeout test correctly exercises an endpoint pointing at a non-routable IPSwitching this test to set
options.endpoint = "10.255.255.1"(instead of a legacy host override) keeps the behavior the same while validating timeout semantics through the new endpoint-based configuration path.
3843-3890: Deprecation annotation around realtimeHost-based fallback test is a sensible compromiseMarking
test__086a__...custom_endpointas deprecated to avoid compiler warnings while it continues to exercise legacyrealtimeHostbehavior is a reasonable way to keep coverage until that option is removed.
3991-4029: Second realtimeHost-based fallback test deprecation handled consistentlyThe deprecation attribute on
test__087__...custom_endpoint_should_result_in_time_outsmirrors the approach above and keeps legacy behavior tested without surfacing warnings to SDK consumers.
4031-4071: REC2a2: customfallbackHoststest correctly updated for new default primary domain
- Default primary host expectation moved to
//main.realtime.ably.net.- Custom fallback host expectations now match
//[f-j].ably-realtime.com, which is appropriate as user-specified fallback hosts are not tied to the new.netproduction domain.
The host-count logic (fallbackHosts.count + 1) and event wiring remain intact.
4073-4111: REC2c1:fallbackHostsUseDefaultlegacy behavior still validated against new domainsUsing
port = 123to force connection attempts through the fallback path and asserting first and second URLs against//main.realtime.ably.netand//main.[a-e].fallback.ably-realtime.comkeeps the deprecatedfallbackHostsUseDefaultsemantics covered under the new routing scheme.
4145-4185: RTN17 “no fallback on 400” test now asserts the correct new primary domainThe final assertion that all connection attempts target
//main.realtime.ably.net(with no fallback usage) is consistent with the updated default domain and the existing behavior for non-retriable 400 errors.
4264-4353: RTN17i/REC3:_test__092...and its prod/sandbox/test variants thoroughly exercise fallback ordering and connectivity checks
- Wiring
options.endpointandoptions.testOptions.shuffleArraytogether withexpectedHostOrderandoptions.domainSelector.fallbackDomainsvalidates that fallback hosts are tried in the shuffled order implied by the jitter coefficients.- Capturing both WebSocket and HTTP URLs and asserting the presence of connectivity checks to
//internet-up.ably-realtime.com/is-the-internet-up.txtbetween changes of fallback host ensures REC3a/REC3b are honored.- The endpoint-specific extraction patterns for prod,
nonprod:sandbox, and"test"correctly mirror the expectations exercised in_test__091....
4471-4512: RTN17g: emptyfallbackHostsarray correctly disables fallback behaviorThe new test
test__095__...won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedconfirms that:
- Even though the error qualifies for fallback, an empty explicit
fallbackHostslist results in two attempts, both to//main.realtime.ably.net, with no fallback-host connections attempted.
This is an important regression check for the new domainSelector-based fallback implementation.
4837-4867: RTN20a/b: reachability tests now correctly usedomainSelector.primaryDomainand common app setup
- Switching to
AblyTests.commonAppSetup(for:)+AblyTests.newRealtime(options)simplifies setup and ensuresTestReachabilityis correctly injected.- Asserting
reachability.host == client.internal.options.domainSelector.primaryDomainin the CONNECTING and CONNECTED cases ties OS reachability checks to the effective primary domain driven byendpoint/legacy options, which is the right abstraction.Also applies to: 4871-4903, 4935-4936
4947-5013: RTN20c: CONNECTING reachability restart test correctly parameterized against the primary domainCapturing
let primaryDomain = options.domainSelector.primaryDomain, settingoptions.endpointto a non-routable IP, and then settingoptions.testOptions.reconnectionRealtimeHost = primaryDomainensures that:
- The initial failure path is driven by an unreachable endpoint.
- Subsequent reachability-driven reconnects are targeted at the correct primary domain, and the injected hook on
transportReconnectWithExistingParametersverifies that we only restart the pending attempt when the OS reports a previous unreachable state.Source/include/Ably/ARTClientOptions.h (1)
22-27: No action required —connectivityCheckUrlis correctly copied incopyWithZone:Verification confirms the property is properly propagated. Line 235 of
Source/ARTClientOptions.massignsoptions.connectivityCheckUrl = self.connectivityCheckUrl;within thecopyWithZone:method, ensuring that cloned options preserve non-default values as expected.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-23: Domain selector exposure and default endpoint API look consistentForward-declaring
ARTDomainSelectorand exposing it via a readonly property, plus replacingsetDefaultEnvironment:withsetDefaultEndpoint:, matches the new endpoint-centric design and the ARTClientOptions.m implementation. No issues from my side here.Source/ARTDefault.m (1)
25-54: DomainSelector-backed defaults and connectivity URL look consistentUsing
ARTDomainSelectorforfallbackHostsWithEnvironment:,restHost, andrealtimeHostcleanly centralizes primary/fallback domain logic and yields the newmain.realtime.ably.net/main.[a‑e].fallback.ably-realtime.comdefaults. The new TTL/message-size globals andconnectivityCheckUrlconstant also look coherent with this model.Also applies to: 76-90, 128-130
Source/ARTDomainSelector.m (1)
103-144: Based on my comprehensive verification of the codebase, I can now provide the rewritten review comment:
Deprecated restHost/realtimeHost now share a single primary domain, causing coupling that affects both REST and Realtime clients
The REC1d handling collapses both
restHostClientOptionandrealtimeHostClientOptioninto a singleARTPrimaryDomainSelectionPolicyLegacyHostwith onehostnamevalue (ARTDomainSelector.m:104-114). While the code prioritizesrestHostClientOptionoverrealtimeHostClientOptionat the ARTDomainSelector level, both REST and Realtime clients retrieve their hosts from the sameoptions.domainSelector.primaryDomainproperty (ARTRest.m and ARTRealtime.m).This means:
- Setting only
realtimeHostwill now affect both REST and Realtime connections- Setting only
restHostwill now affect both REST and Realtime connectionsThis breaks the historical separation where
restHostaffected only REST andrealtimeHostaffected only Realtime. The unit tests (ARTDomainSelectorTests.swift:020-023) verify that each option individually returns the correct value toprimaryDomain, but do not test or document that both clients share this single domain source.If ADR-119 requires maintaining independent rest/realtime overrides as "unchanged" legacy behavior, this coupling violates that requirement. Consider preserving separate domain sources for REST and Realtime clients, or explicitly document this as an intended (and breaking) behavior change.
Source/ARTClientOptions.m (1)
168-191: # Verify that REST and Realtime host unification is intentional and compatible with legacy deprecationBoth
restHostandrealtimeHostare deprecated properties that now share a singleprimaryDomainthrough ARTDomainSelector's REC1d logic:- (NSString *)restHost { return _restHost ?: self.domainSelector.primaryDomain; } - (NSString *)realtimeHost { return _realtimeHost ?: self.domainSelector.primaryDomain; }Because both
restUrlComponents(line 196) andrealtimeUrl(line 208) useself.primaryDomain, they are now bound to the same host. In ARTDomainSelector's initialization logic, if bothrestHostandrealtimeHostare provided,restHosttakes precedence (lines 104–114); otherwise the first set value becomes the primary domain.Unable to locate ADR-119 in the codebase to verify the specific backward-compatibility requirement. However, since
restHostandrealtimeHostare marked as deprecated (with message "Use the endpoint client option instead"), confirm that this unification is intentional and that no code path depends on independent REST and Realtime host overrides for these legacy options.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
Test/AblyTests/Tests/RestClientTests.swift (1)
177-181: Existing TODO’d expectations remain ineffective but are already trackedThe
expect(publishTask.error).toEventually(beNil(), …)checks intest__015__…API_keyandtest__018__…tokenare still ineffective becausepublishTask.errorstarts asnil. This was already raised and is tracked in GitHub issue #2145, so leaving the TODOs in place in this PR is reasonable.Also applies to: 201-207
Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
400-411: Endpoint"nonprod:"producing.realtime.ably-nonprod.netstill looks like an undesirable edge caseThis test codifies that a
nonprod:endpoint with no routing ID yields a primary domain of.realtime.ably-nonprod.net(leading dot, empty policy ID). That hostname is highly likely to be invalid or at least unusable, and it makes a simple typo ("nonprod:"vs"nonprod:sandbox") fail at runtime in a non-obvious way.Consider tightening the parsing in
ARTDomainSelectorso that a nonprod-prefixed endpoint with an empty/whitespace-only suffix is treated as invalid or as “no nonprod routing ID” (e.g. falling back to the default primary domain or surfacing configuration as invalid). If you make that change, this test should be updated to assert the new, safer behavior instead of the leading-dot hostname.Source/ARTDomainSelector.m (2)
66-88: Endpoint classification is too naive for IP/IPv6 literalsThe hostname-vs-routing-policy decision only checks:
containsString:@"."containsString:@"::"- equality with
"localhost"Anything else non-empty is treated as a routing policy ID. This will misclassify several valid endpoint forms, e.g.:
- Fully expanded IPv6 literals without
::(2001:0db8:1:2:3:4:5:6).- Bracketed IPv6 with optional port (
[::1]:8080).- Potential other IP literal syntaxes.
Those values will be turned into routing policies and combined with
.realtime.ably.net, which is almost certainly not what the user intended.Consider normalizing and validating IP literals explicitly (e.g. strip brackets/ports/zone IDs, then detect IPv4/IPv6 using a system API like
inet_ptonor a focused regex) before falling back to the routing-policy heuristics. That way:
- All valid IPv4/IPv6 endpoints are consistently treated as hostnames.
- Only “clean” nonprod/prod routing IDs go down the policyId path.
81-88:endpoint = "nonprod:"yields an emptypolicyIdand leading-dot hostWhen
endpointClientOptionhas the"nonprod:"prefix but nothing afterwards,policyIdbecomes the empty string. For the NonProductionRoutingPolicy branch this produces:
- Primary domain:
.realtime.ably-nonprod.net- Fallbacks:
.a.fallback.ably-realtime-nonprod.com, etc.Those hostnames are almost certainly invalid and arise from a plausible misconfiguration or typo.
It would be safer to treat an empty/whitespace-only suffix after
"nonprod:"as “no routing ID” and either:
- fall back to the default primary/fallback domains, or
- surface an explicit configuration error.
If you change the behavior, remember to update the corresponding Swift test (
test__028__ARTDomainSelector__should_handle_nonprod_with_empty_id) to assert the new semantics.Source/ARTClientOptions.m (2)
90-99: setEndpoint still enforces REC1b1 even when clearing/propagating a nil endpoint (can break copy and legacy-only configs)
-setEndpoint:unconditionally raises if any legacy options are set, regardless of the value passed:- (void)setEndpoint:(NSString *)endpoint { // REC1b1: endpoint cannot be used with deprecated options if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { [NSException raise:...]; } _endpoint = endpoint; _domainSelector = nil; }This means:
- Copying an options instance that only uses
environment/restHost/realtimeHost(withendpoint == nil) will still invokesetEndpoint:nilincopyWithZone:, andhasEnvironment/hasCustom*Hostmay already be true, causingcopyto raise even though no endpoint is actually being configured.- Clearing an endpoint (setting it back to
nil/empty) after legacy options have been set will also throw, even though the resulting configuration is valid (endpoint no longer in use).REC1b1 only needs to be enforced when a non-empty endpoint is set. Suggest gating the check on
endpointitself:- (void)setEndpoint:(NSString *)endpoint { - // REC1b1: endpoint cannot be used with deprecated options - if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { - [NSException raise:NSInvalidArgumentException - format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; - } - _endpoint = endpoint; - // Reset domain selector when endpoint changes - _domainSelector = nil; -} + (void)setEndpoint:(NSString *)endpoint { + // Only enforce REC1b1 when actually setting a non-empty endpoint + if (endpoint && [endpoint isNotEmptyString]) { + if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { + [NSException raise:NSInvalidArgumentException + format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; + } + } + _endpoint = endpoint; + // Reset domain selector when endpoint changes (including clearing it) + _domainSelector = nil; +}This preserves the REC1b1 constraint for real endpoint usage while allowing
copyWithZone:and “clear endpoint” flows to work for legacy-only configurations.
319-324: isProductionEnvironment now returns NO when environment is nil; likely diverges from historical “nil == production” semanticsCurrent implementation:
- (BOOL)isProductionEnvironment { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" return [[self.environment lowercaseString] isEqualToString:[ARTDefaultProductionEnvironment lowercaseString]]; #pragma clang diagnostic pop }When
environment == nil(the default case for clients not explicitly configuring a non‑production environment),-[NSString lowercaseString]is sent tonil, and the expression evaluates toNO. Historically, “no environment configured” has been treated as production, so this inverts that meaning.If call sites still rely on the traditional behavior (which the method name suggests), this should instead treat
nilor"production"(case‑insensitive) as production, e.g.:- (BOOL)isProductionEnvironment { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" NSString *env = [[self environment] lowercaseString]; NSString *prod = [ARTDefaultProductionEnvironment lowercaseString]; return (env == nil || [env isEqualToString:prod]); #pragma clang diagnostic pop }This keeps legacy semantics while still allowing explicit non‑production environments to be detected.
🧹 Nitpick comments (2)
Source/ARTClientOptions.m (2)
193-215: Confirm DomainSelector’s host outputs are bare hostnames suitable for NSURLComponents.host (esp. for IPv6/FQDN/IP cases)
restUrlComponentsandrealtimeUrlForHost:now do:components.host = self.primaryDomain; // rest ... components.host = host; // realtimewith
primaryDomainand thehostargument ultimately coming fromARTDomainSelector.For
NSURLComponents.hostto remain valid across all supported endpoint forms (routing policy, FQDN, IPv4, IPv6, localhost),ARTDomainSelectorneeds to guarantee that:
- Hosts are bare hostnames or IP literals, without embedded ports.
- IPv6 literals, if supported, are in the form expected by
NSURLComponents(no[]wrapping in thehostproperty; ports handled viacomponents.port).Given ADR‑119 explicitly allows FQDN/IP/localhost endpoints, it would be good to double‑check DomainSelector’s outputs (and add tests) to ensure they are always safe to assign to
NSURLComponents.hostwithout additional normalization.
22-22: Global default endpoint is simple but relies on call order; consider whether it ever needs to change after first options instantiationYou now have:
NSString *ARTDefaultEndpoint = nil; @implementation ARTClientOptions { NSString *_endpoint; ... } - (instancetype)initDefaults { ... _endpoint = ARTDefaultEndpoint; ... _connectivityCheckUrl = [ARTDefault connectivityCheckUrl]; } + (void)setDefaultEndpoint:(NSString *)endpoint { ARTDefaultEndpoint = endpoint; }This works fine if
+setDefaultEndpoint:is only ever called once, before anyARTClientOptionsare created (e.g., from a library initialization path or tests). If there’s any scenario where the default endpoint might be updated at runtime, already‑constructedARTClientOptionsinstances will keep the old value because they snapshotARTDefaultEndpointininitDefaults.If runtime changes are not required, this is acceptable as‑is; otherwise, consider either:
- Having
initDefaultsread a source of truth that can change (e.g.ARTDefault), or- Documenting that
setDefaultEndpoint:is a one‑time, pre‑instantiation configuration hook.Also applies to: 32-38, 48-48, 80-80, 302-304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientOptions.m(8 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(3 hunks)Source/ARTRest.m(7 hunks)Source/ARTWebSocketTransport.m(5 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/ARTFallbackHosts.m
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
🚧 Files skipped from review as they are similar to previous changes (8)
- Scripts/log-environment-information.sh
- Source/Ably.modulemap
- Source/ARTWebSocketTransport.m
- Source/include/Ably/ARTClientOptions.h
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/module.modulemap
- Source/ARTRealtime.m
🧰 Additional context used
🧬 Code graph analysis (3)
Test/AblyTests/Tests/RealtimeClientTests.swift (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
deprecated(3843-3890)deprecated(3991-4029)deprecated(4074-4111)Test/AblyTests/Tests/RestClientTests.swift (13)
deprecated(302-314)deprecated(317-330)deprecated(354-364)deprecated(367-374)deprecated(377-382)deprecated(385-390)deprecated(418-424)deprecated(865-894)deprecated(896-900)deprecated(903-930)deprecated(1014-1039)deprecated(1098-1122)deprecated(1226-1235)
Test/AblyTests/Tests/RestClientTests.swift (3)
Test/AblyTests/Test Utilities/TestUtilities.swift (7)
match(1516-1524)publishTestMessage(484-486)publishTestMessage(488-490)getTestEndpoint(626-635)at(1431-1433)at(1440-1445)extract(1526-1537)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
match(1516-1524)at(1431-1433)at(1440-1445)data(934-936)dispose(1639-1645)splitDone(248-258)clientOptions(144-159)commonAppSetup(107-142)Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (30)
.github/workflows/integration-test.yaml (1)
35-35: ABLY_ENV change is correctly applied; no other integration-test workflow files exist to replicate.Verification confirms only one
integration-test.yamlfile exists in.github/workflows/, with the correctABLY_ENV: nonprod:sandboxvalue at line 35. The environment variable naming aligns with ADR-119's convention for non-production sandbox routing policies. The comment on line 11 serves as a template reminder for future variants, but currently there are no other integration-test-*.yaml files requiring updates.Ably.xcodeproj/project.pbxproj (2)
360-365: ARTDomainSelector file wiring looks correct and consistent.
PBXBuildFileentries forARTDomainSelector.m/.hcorrectly point at the newPBXFileReferenceIDs.- File references place
ARTDomainSelector.munderSource/UtilitiesandARTDomainSelector.hunderPrivateHeaders/Ably, matching existing private utilities.- The Utilities group now includes both
.hand.m, so Xcode navigation and grouping remain coherent.No issues here; this wiring is good to go.
Also applies to: 1224-1225, 1993-1994
2142-2142: DomainSelector is correctly hooked into all framework targets as a private utility.
ARTDomainSelector.his added once to each Headers phase for iOS, macOS, and tvOS withATTRIBUTES = (Private,), aligning with its intended internal role.ARTDomainSelector.mis added once to each Sources phase for the same three targets, reusing the correspondingPBXBuildFileentries.- No duplicate or dangling references for these new build-file IDs.
Assuming DomainSelector is intentionally non‑public API, this target integration looks correct.
Also applies to: 2318-2318, 2527-2527, 2966-2966, 3110-3110, 3254-3254
Examples/Tests/TestsTests/TestsTests.swift (1)
21-21: Endpoint/nonprod and ably.com background URLs look consistent with ADR-119Using
https://sandbox.realtime.ably-nonprod.net:443/appsplusoptions.endpoint = "nonprod:sandbox"keeps sandbox provisioning and client connections aligned on the same nonprod routing policy, and switching the backgroundURLSessionpings tohttps://ably.comis purely cosmetic and safe here.Also applies to: 50-50, 67-78
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
119-119: Test utilities correctly pivoted to endpoint/domainSelector
- Using
https://\(options.domainSelector.primaryDomain):\(options.tlsPort)/appskeeps sandbox app provisioning in lockstep with whatever endpoint/domainSelector the tests are exercising.- Wiring
clientOptionsviaoptions.endpoint = getTestEndpoint()and deriving that fromABLY_ENDPOINT→ABLY_ENV→"nonprod:sandbox"cleanly centralizes test configuration.- Dropping the environment query item from the JWT endpoint and only sending
encryptedmatches the new endpoint-centric model.Given this is test-only code, the existing
infoDictionary!force unwrap is acceptable.Also applies to: 146-158, 607-607, 626-635
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-180: Deprecation wrapper around realtimeHost test is appropriateMarking the
realtimeHost-based test as deprecated (with a clear message) is a good way to preserve coverage for legacy behavior without surfacing compiler warnings while endpoint/domainSelector become primary.Source/ARTRest.m (1)
19-20: DomainSelector-driven REST host/fallback behavior is wired correctly
- Resetting
currentFallbackHost/prioritizedHostonfallbackRetryExpirationand restoring the host viaoptions.domainSelector.primaryDomainkeeps RSC15f semantics while honoring endpoint/environment-derived defaults.- Using
options.domainSelector.fallbackDomainsas the source forARTFallbackand promoting a successful fallback toprioritizedHostmaintains the expected REC2 fallback behavior.- Setting the
Hostheader explicitly to the fallback host for retried requests (// RSC15j) is important to keep SNI/Host consistent with the URL when going through intermediaries.currentHostnow defers todomainSelector.primaryDomain, which matches the new endpoint-centric configuration.internetIsUp:now respects a configurableconnectivityCheckUrlwith a sane default fromARTDefault, which is a straightforward, backward-compatible enhancement.- Exposing
+deviceAccessQueueand reusing it in the device accessors centralizes synchronization around the sharedARTLocalDevicewithout changing behavior.No issues spotted with these changes.
Also applies to: 375-383, 441-452, 460-463, 515-521, 677-689, 793-802
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-40: ARTDomainSelector interface matches the endpoint/REC designExplicitly requiring all domain-affecting client options in the initializer, disabling
init, and exposingprimaryDomain/fallbackDomainsas read‑only properties gives a clear, constrained surface for domain resolution. This is a solid foundation for the endpoint‑centric behavior used elsewhere in the PR.Test/AblyTests/Tests/RestClientTests.swift (1)
45-47: Endpoint/default-domain and fallback host tests are consistent with DomainSelector semantics
- All expectations that were previously tied to
ably.iohave been updated to assert againstmain.realtime.ably.netandmain.[a-e].fallback.ably-realtime.com, matching the new default primary domain and default fallback domain family.- The new REC1 tests (
test__026a…027d) correctly capture:
- endpoint + any deprecated option (environment/restHost/realtimeHost) being invalid,
- endpoint as FQDN →
primaryDomain == endpoint,- endpoint as nonprod routing policy (
"nonprod:sandbox") →sandbox.realtime.ably-nonprod.net,- endpoint as production routing policy (
"test") →test.realtime.ably.net,- environment/restHost/realtimeHost overriding the default primary domain as specified.
- The new REC2 tests around
options.domainSelector.fallbackDomainscover:
- implicit default fallback list for the default primary domain,
- empty fallback list when the primary domain is explicitly configured as a domain name,
- nonprod routing policy endpoints (
nonprod:sandbox) mapping tosandbox.[a-e].fallback.ably-realtime-nonprod.com,- environment and deprecated host options yielding the expected implicit fallback lists or empty lists.
- Tests using
ARTClientOptions.setDefaultEndpoint(getTestEndpoint())andoptions.endpoint = …ensure both static and per-instance endpoint configuration surfaces are exercised.- RSC15h/RSC25/RSC15m/RSC15n host-fallback tests now drive against
ARTDefault.fallbackHosts()andoptions.domainSelector.fallbackDomains, so they meaningfully validate the new selector‑based fallback behavior.These changes give good coverage of the new endpoint/domainSelector model across default, custom, routing-policy, and deprecated-option scenarios.
Also applies to: 66-67, 112-112, 134-135, 392-403, 428-448, 450-476, 515-531, 552-575, 585-607, 685-694, 903-930, 932-957, 959-985, 987-1011, 1041-1067, 1070-1095, 1180-1183
Source/include/Ably/ARTDefault.h (1)
13-19: ARTDefault host/fallback accessors match new usageKeeping
+restHost,+realtimeHost, and thefallbackHostsAPIs public is consistent with how tests and DomainSelector now consume defaults, and the minor signature tidy-up doesn’t change behavior.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (13)
44-56: Fallback host URL expectations now correctly align withARTDomainSelectordefaultsThe updated comment and regex expectations for
testUsesAlternativeHostOnResponse(defaultmain.realtime.ably.netandmain.[a-e].fallback.ably-realtime.com) match the new default primary domain and fallback domains exposed viaARTDomainSelector. The sequencing (primary then fallback) is still validated as before and looks consistent with REC2.
181-195: Default realtime connection host assertion matches new primary domain
test__017__Connection__url__should_connect_to_the_default_hostnow assertsurl.host == "main.realtime.ably.net". This is consistent withARTDomainSelector’s default policy (main.realtime.ably.net), so the change keeps the test aligned with the new endpoint-first behavior.
2178-2203: Using a non-routable IP viaendpointis a good way to exercise RTN14c timeout behaviorSetting
options.endpoint = "10.255.255.1"(non-routable) for RTN14c ensures the connection attempt goes to a single, unreachable host classified as a hostname/IP, so no automatic fallbacks are used. The subsequent timing assertions againstrealtimeRequestTimeoutremain valid and cleanly exercise the timeout path under the new endpoint semantics.
3843-3890: Deprecation annotations aroundrealtimeHosttests are appropriateMarking the
realtimeHost-based host-fallback tests as@available(*, deprecated, ...)avoids compiler warnings while keeping coverage for legacy options. This is a sensible transition pattern while endpoint becomes the preferred configuration surface.
4031-4071: REC2a2 customfallbackHoststest remains correct after domain changes
test__089__Connection__Host_Fallback__applies_when_an_array_of_ClientOptions_fallbackHosts_is_providednow expects the first connection tomain.realtime.ably.netand subsequent attempts to the explicit custom fallback hosts[f-j].ably-realtime.com. This matches the new default primary domain while still verifying that customfallbackHostsfully override generated fallbacks.
4073-4111: DeprecatedfallbackHostsUseDefaultpath still correctly uses generated default fallbacks
test__089b__...fallbackHostsUseDefault...now asserts that withfallbackHostsUseDefault = trueand a customport, the client first connects tomain.realtime.ably.netand then amain.[a-e].fallback.ably-realtime.comhost. That matchesARTDomainSelector.defaultFallbackDomainsand confirms the deprecated flag still routes through the REC2c1-style defaults.
4189-4260: Helper_test__091and wrappers correctly validate primary vs fallback hosts for different endpointsThe new
_test__091__Connection__Host_Fallback__every_connection_is_first_attempted_to_the_primary_host_main_realtime_ably_net(endpoint:test:)plus its three wrappers verify:
endpoint == nil→ first URL usesmain.realtime.ably.net, then amain.[a-e].fallback.ably-realtime.comhost.endpoint == "nonprod:sandbox"→ first URL usessandbox.realtime.ably-nonprod.net, thensandbox.[a-e].fallback.ably-realtime-nonprod.com.endpoint == "test"→ first URL usestest.realtime.ably.net, thentest.[a-e].fallback.ably-realtime.com.This matches the primary/fallback logic in
ARTDomainSelectorfor default, nonprod, and production routing-policy endpoints and provides good regression coverage for REC2c1/3/4 under the new endpoint API.
4264-4353: Host retry ordering tests correctly reusedomainSelector.fallbackDomainsfor prod/nonprod endpoints
_test__092__Connection__Host_Fallback__should_retry_hosts_in_random_order_after_checkin_if_an_internet_connection_is_availablenow:
- Drives fallback generation via
options.domainSelector.fallbackDomainsfor the current endpoint (nil,"nonprod:sandbox", or"test").- Uses the injected
shuffleArrayInExpectedHostOrderto produce a deterministic “random” sequence.- Extracts hostnames from both websocket URLs and the connectivity-check HTTP requests, verifying that the actual retry sequence exactly matches the expected fallback host list for each endpoint variant.
The three wrapper tests for prod/sandbox/test ensure this behavior is exercised for all relevant domain families. The logic looks consistent with the fallback-domain construction in
ARTDomainSelector.
4471-4512: EmptyfallbackHostsarray correctly disables fallback behavior
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedconfirms:
- With
fallbackHosts = [], a hostUnreachable error still causes CONNECTING → DISCONNECTED → CONNECTING transitions.- Only two connection attempts occur, both to
main.realtime.ably.net, and no generated fallback hosts are used.This matches the intended semantics that an explicit empty array means “no fallbacks”, and it aligns with
ARTDomainSelector.fallbackDomainsreturning the provided empty list verbatim.
4835-4903: Reachability now correctly targets the primary domain fromdomainSelectorIn tests 109 and 110, the assertions that
reachability.host == client.internal.options.domainSelector.primaryDomainensure that:
- OS reachability checks are aligned with whatever REC1 primary domain is selected (default or endpoint-based).
- Simulated reachability changes drive the expected DISCONNECTED transitions in both CONNECTING and CONNECTED states.
This is a good update from older environment/host assumptions and keeps network-change behavior tied to the same domain-selection source used elsewhere.
4948-4953: RTN20c test correctly pins reconnection host against the original primary domainIn
test__106_b__...restart_the_pending_connection_attempt..., capturinglet primaryDomain = options.domainSelector.primaryDomainbefore overridingoptions.endpointwith a non-routable IP and then settingoptions.testOptions.reconnectionRealtimeHost = primaryDomainensures:
- The initial failed connection uses the “bad” endpoint, exercising the reachability path.
- The subsequent reconnection attempt uses the original, valid primary domain.
This keeps reconnection semantics coherent with domain selection while still allowing the test to force a CONNECTING → DISCONNECTED → CONNECTING cycle via an unreachable endpoint.
4546-4556: Fallback HTTP requests remain tied to the selected fallback hostIn
test__096__Connection__Host_Fallback__client_is_connected_to_a_fallback_host_endpoint_should_do_HTTP_requests_to_the_same_data_centre, the updated regex checking//main.[a-e].fallback.ably-realtime.comfor the websocket URL, and then asserting that subsequent RESTtimerequests go to the same host, matches the new default fallback-domain schema and keeps the “same data centre” invariant intact.
4180-4185: Bad-request (400) path correctly kept on primary host after domain changes
test__090__Connection__Host_Fallback__should_not_use_an_alternative_host_when_the_client_receives_a_bad_requestnow asserts that both connection attempts targetmain.realtime.ably.net. This continues to verify that 400-class fake responses do not trigger fallback-host usage under the new domain scheme.Source/ARTDomainSelector.m (1)
148-214: Fallback-domain generation logic matches REC2 and is internally consistentThe
fallbackDomainsimplementation:
- Honors explicit
fallbackHostsClientOption(including an explicit empty array) before any generated defaults.- Respects
fallbackHostsUseDefaultby routing throughdefaultFallbackDomains.- Generates:
- Default:
main.[a–e].fallback.ably-realtime.com.- Nonprod routing policy:
<policyId>.[a–e].fallback.ably-realtime-nonprod.com.- Prod routing policy and legacy env:
<policyId>.[a–e].fallback.ably-realtime.com.- Hostname/legacy host: no automatic fallbacks.
defaultFallbackDomainsalso correctly returns themain.[a–e].fallback.ably-realtime.comset. This aligns with the new tests and REC2 requirements.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
18-26: ExposingdomainSelectoronARTClientOptionsis a clean internal API choiceThe new readonly
domainSelectorproperty and+setDefaultEndpoint:class method give internal code and tests a single, well-defined source of truth for REC1/REC2 domain behavior. This avoids duplicating domain/fallback state onARTClientOptionsitself and matches the direction suggested in earlier reviews. TherealtimeUrlForHost:helper is also a sensible internal utility for constructing alternate-host URLs without mutating options.Source/ARTClientOptions.m (2)
105-126: DomainSelector invalidation on all relevant options looks correct and keeps host resolution coherentResetting
_domainSelectorinsetEndpoint:,setEnvironment:,setRestHost:,setRealtimeHost:,setFallbackHosts:, andsetFallbackHostsUseDefault:ensures that any change to the inputs that drive domain selection will force a freshARTDomainSelectorinstance next timedomainSelector,primaryDomain, orfallbackDomainsare accessed.Given how much logic now sits inside
ARTDomainSelector, this pattern is important to avoid stale host/fallback state leaking between configurations. The current implementation (lazy creation + explicit resets on all the right setters) looks consistent and appropriate.Also applies to: 130-187, 284-300
217-265: copyWithZone correctly propagates new endpoint/domain-related state, assuming setEndpoint is gated as aboveThe updated
copyWithZone::
- Copies
endpointandconnectivityCheckUrl, so the clone preserves the new configuration surface.- Copies
_fallbackHostsand_fallbackHostsUseDefaultvia ivars, intentionally bypassing the setters (which enforce mutual-exclusion rules) — this is appropriate for a faithful copy.- Reapplies legacy overrides (
restHost,realtimeHost,environment) only when the source ivars are non‑nil, so defaults are not “invented” on the clone.Once
setEndpoint:is updated to only enforce REC1b1 for non‑empty endpoints (see earlier comment), this method should no longer raise when copying legacy‑only options and will produce an options object semantically equivalent to the original.Source/ARTDefault.m (3)
4-7: DomainSelector integration for default hosts/fallbacks looks consistent with the new endpoint-centric model
ARTDefaultnow usesARTDomainSelectorin:
+fallbackHostsWithEnvironment:to derive fallback domains for a given environment.+fallbackHoststo get the default fallback set.+restHost/+realtimeHostto resolve the primary default domain.This mirrors how
ARTClientOptionsdelegates domain selection, which should keep host and fallback behavior consistent across “static default” and “per‑client options” code paths. The constructor arguments (nil endpoint, nil custom hosts,fallbackHostsUseDefault:false) look coherent for “library defaults” that are independent of any particular client options instance.Also applies to: 25-55
9-12: Mutable global defaults for TTL and message sizes are wired up correctlyThe introduction of:
ARTConnectionStateTtlARTMaxProductionMessageSizeARTMaxSandboxMessageSizecombined with:
+connectionStateTtland+setConnectionStateTtl:+maxMessageSizewith DEBUG/!DEBUG switching+setMaxMessageSize:,+setMaxProductionMessageSize:,+setMaxSandboxMessageSize:preserves the prior behavior while allowing tests (and consumers, if needed) to adjust these values at runtime. The use of
@synchronized(self)in the setters is sufficient to avoid racy updates on these simple globals in typical SDK usage.Also applies to: 68-90, 92-118
13-13: connectivityCheckUrl constant and helper are straightforward and align with the new ClientOptions defaultDefining:
NSString *const ARTDefaultConnectivityCheckUrl = @"internet-up.ably-realtime.com/is-the-internet-up.txt"; ... + (NSString *)connectivityCheckUrl { return [NSString stringWithFormat:@"https://%@", ARTDefaultConnectivityCheckUrl]; }and then wiring this into
ARTClientOptionsvia_connectivityCheckUrl = [ARTDefault connectivityCheckUrl];gives a single, centralized source of truth for the connectivity check endpoint. Thehttps://prefix is handled here, and the host/path remain unchanged from the existing “is-the-internet-up” check, so this looks good.Also applies to: 128-130
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Source/ARTDomainSelector.m (1)
66-118: Endpoint policy parsing is clear, but IPv6/IP literal detection remains too naiveThe REC1 policy parsing (endpoint → env → restHost → realtimeHost → default) is straightforward and matches the documented comments. However, the “hostname vs routing policy” heuristic:
if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || [endpointClientOption isEqualToString:@"localhost"])will still misclassify many valid IP literals as routing-policy IDs (e.g. full IPv6 addresses without
"::", bracketed IPv6 with ports, IPv4 addresses, or IPv6 with zone IDs). That means a user passing an IP literal asendpointmay silently get routed via REC1b3/b4 rather than REC1b2.Consider normalizing and validating the endpoint as an IP literal before deciding it’s a routing policy, for example:
- Strip brackets (
[::1]), ports (:8080), and any zone suffix (%eth0).- Use
inet_ptonfor IPv4/IPv6 detection.- Treat any successfully parsed IP literal as a hostname (REC1b2 path), not as a routing policy.
Also consider guarding against an empty policy ID after
nonprod:(e.g.endpoint = "nonprod:") to avoid generating".realtime.ably-nonprod.net".
🧹 Nitpick comments (5)
Source/ARTRealtime.m (1)
217-237: ARTDomainSelector import here is benign but currently unused
ARTRealtimeInternalrelies ongetClientOptions].domainSelectorrather than directly on ARTDomainSelector symbols, so this import isn’t strictly necessary. It’s harmless, but could be removed later if you’re cleaning up includes.Source/ARTClientOptions.m (4)
90-99: Be careful thatsetEndpoint:+copyWithZone:don’t throw when simply copying or clearing options
setEndpoint:,setEnvironment:,setRestHost:, andsetRealtimeHost:correctly enforce the REC1b1 / REC1c1 mutual‑exclusion rules for endpoint vs legacy options, and they reset_domainSelectorwhen inputs change, which is good.However, because:
setEndpoint:unconditionally checksself.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost, andcopyWithZone:always doesoptions.endpoint = self.endpointbefore copying legacy fields,there are edge cases where you may raise
NSInvalidArgumentExceptioneven though the effective configuration doesn’t introduce a new endpoint. Examples to consider:
- Copying an options instance configured only via
environment/ custom hosts but with a nil or default endpoint.- Clearing an endpoint (setting it to
nilor an empty string) on an instance that already has legacy host/environment configured.To make these flows safer while still enforcing REC1b1, consider:
- Gating the conflict check in
setEndpoint:on the incoming argument being non‑nil/non‑empty, e.g.:- (void)setEndpoint:(NSString *)endpoint { - // REC1b1: endpoint cannot be used with deprecated options - if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { + (void)setEndpoint:(NSString *)endpoint { + // REC1b1: endpoint cannot be used with deprecated options + if (endpoint && [endpoint isNotEmptyString] && + (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost)) { [NSException raise:NSInvalidArgumentException format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; } _endpoint = endpoint; _domainSelector = nil; }
- Alternatively, bypassing the setter when copying (
options->_endpoint = self->_endpoint;) if you’re confident the source object already satisfied the invariants.This would still reject genuinely mixed configurations, but avoid surprising exceptions when copying or clearing options.
Also applies to: 130-187, 217-248
193-215: rest/realtime URL helpers now shareprimaryDomain– confirm this matches desired host split
restUrlComponents,restUrl, andrealtimeUrlnow all derive their host fromprimaryDomain(either directly or viarealtimeUrlForHost:), andrestHost/realtimeHostfall back todomainSelector.primaryDomainwhen no custom host is set. This is internally consistent; just double‑check thatARTDomainSelector.primaryDomainis indeed the correct canonical host for both REST and Realtime in the ADR‑119 world (i.e. you don’t still expect a distinct “rest” vs “realtime” hostname pair by default). If separate defaults are still intended, you may want distinct helpers or a small abstraction around “rest primary domain” vs “realtime primary domain”.
302-305:setDefaultEndpoint:as a process‑wide default is fine but worth documenting as global stateHaving a global
ARTDefaultEndpointthat influences subsequentinitDefaultscalls is a reasonable way to support test setups and app‑wide defaults. It might be worth documenting (in comments or higher‑level docs) that this is global mutable state affecting all futureARTClientOptionsinstances, so it’s not something applications should typically change at runtime outside of controlled test harnesses.
319-324:isProductionEnvironmentsemantics no longer treat “no environment” as production
isProductionEnvironmentcurrently returns:return [[self.environment lowercaseString] isEqualToString:[ARTDefaultProductionEnvironment lowercaseString]];Given
environmentisnilby default, this now reportsNOwhen no environment is configured, whereas historically “no environment set” has been equivalent to production. If any callers use!isProductionEnvironmentto branch non‑production behavior, this change could misclassify the default configuration.To preserve the usual meaning while still handling explicit environment overrides, consider:
- (BOOL)isProductionEnvironment { - return [[self.environment lowercaseString] isEqualToString:[ARTDefaultProductionEnvironment lowercaseString]]; -} + (BOOL)isProductionEnvironment { + NSString *env = [self.environment lowercaseString]; + NSString *prod = [ARTDefaultProductionEnvironment lowercaseString]; + // Treat a nil/unspecified environment as production. + return (env == nil || [env isEqualToString:prod]); +}(or an equivalent implementation using
hasEnvironment).That keeps behavior intuitive for both unset and explicitly “production” environments.
Also applies to: 326-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/integration-test.yaml(1 hunks)Ably.xcodeproj/project.pbxproj(9 hunks)Examples/Tests/TestsTests/TestsTests.swift(3 hunks)Scripts/log-environment-information.sh(1 hunks)Source/ARTClientOptions.m(8 hunks)Source/ARTDefault.m(4 hunks)Source/ARTDomainSelector.m(1 hunks)Source/ARTFallbackHosts.m(0 hunks)Source/ARTRealtime.m(3 hunks)Source/ARTRest.m(7 hunks)Source/ARTWebSocketTransport.m(5 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDomainSelector.h(1 hunks)Source/PrivateHeaders/Ably/ARTFallbackHosts.h(0 hunks)Source/include/Ably/ARTClientOptions.h(2 hunks)Source/include/Ably/ARTDefault.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/ARTDomainSelectorTests.swift(1 hunks)Test/AblyTests/Tests/PushAdminTests.swift(1 hunks)Test/AblyTests/Tests/RealtimeClientConnectionTests.swift(22 hunks)Test/AblyTests/Tests/RealtimeClientTests.swift(1 hunks)Test/AblyTests/Tests/RestClientTests.swift(35 hunks)
💤 Files with no reviewable changes (2)
- Source/ARTFallbackHosts.m
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
🚧 Files skipped from review as they are similar to previous changes (6)
- Scripts/log-environment-information.sh
- Source/Ably.modulemap
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/module.modulemap
- Test/AblyTests/Tests/ARTDomainSelectorTests.swift
- Examples/Tests/TestsTests/TestsTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (5)
match(1516-1524)getTestEndpoint(626-635)at(1431-1433)at(1440-1445)extract(1526-1537)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTDomainSelector.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (44)
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector added cleanly across file references, groups, and all targets
ARTDomainSelector.m/.hhave matchingPBXFileReferenceandPBXBuildFileentries, are placed under theUtilitiesgroup, and are correctly wired into the iOS/macOS/tvOS Sources and Headers phases as Private headers with no dangling IDs. This wiring looks consistent and should compile fine.Also applies to: 1224-1225, 1993-1994, 2142-2142, 2318-2318, 2527-2527, 2965-2967, 3110-3111, 3254-3255
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
115-141: Usingoptions.domainSelector.primaryDomainfor app creation looks correctSwitching the test app creation URL to
options.domainSelector.primaryDomainkeeps tests aligned with the REC1/endpoint resolution path, and avoids coupling tests to legacy rest/realtime host behavior. No issues spotted here.
145-159: Endpoint-based test client options wiring is consistentSetting
options.endpoint = getTestEndpoint()is the right way to feed the new domain selection path in tests. The rest of the options setup (debug, key, token, dispatch queues) remains unchanged and compatible.
598-607: JWT helper no longer passes environment – confirm this is intentionalDropping any environment-related query param and only sending
encrypted(plus keyName/keySecret/etc.) changes how the echo server might infer routing. If the echo endpoint now relies purely on the API key and global routing config, this is fine; otherwise it might still expect an environment-like hint.Can you confirm the echo JWT service used here is independent of
environment/endpointinputs and that no server-side logic relied on the old env param?Source/ARTDomainSelector.m (3)
120-146: Primary-domain construction matches REC1 semanticsThe switch in
primaryDomaincorrectly yields:
- Default:
main.realtime.ably.net- Hostname: the raw hostname
- Non-production routing policy:
<policyId>.realtime.ably-nonprod.net- Production routing / legacy environment:
<policyId>.realtime.ably.net- Legacy host: the raw hostname
This aligns with the policy comments and REC1* references; no issues found.
148-204: Fallback-domain generation is consistent with REC2 and legacy behaviorsThe fallback selection order—explicit
fallbackHostsClientOption→fallbackHostsUseDefault→ policy-derived fallbacks—is sensible and matches the REC2a/b/c comments. For routing-policy and legacy-environment cases, generating<policyId>.[a–e].fallback.<tld>looks correct, and returning@[]for hostname/legacy-host is appropriate (no implicit fallbacks on arbitrary hostnames).
206-214:defaultFallbackDomainsimplementation & doc are now alignedThe helper now clearly documents and returns
["main.a.fallback.ably-realtime.com", ..., "main.e.fallback.ably-realtime.com"], matching the internal constants and REC2 default behavior. All good here.Source/ARTDefault.m (4)
4-14: New default constants and connectivity URL are coherentThe introduction of
ARTDefaultAPIVersion,ARTDefaultProductionEnvironment, connection-state TTL, production/sandbox message sizes, andARTDefaultConnectivityCheckUrlconsolidates mutable defaults into explicit globals. The connectivity check URLhttps://internet-up.ably-realtime.com/is-the-internet-up.txtmatches the new non-sandbox infrastructure and is suitable as a single canonical check endpoint.
25-36: UsingARTDomainSelectorfromARTDefaultmatches endpoint/env semantics
fallbackHostsWithEnvironment:,fallbackHosts,restHost, andrealtimeHostnow delegate toARTDomainSelectorwithendpoint=niland (optionally)environment, which:
- Preserves legacy
environmentbehavior via the LegacyEnvironment policy.- Makes the no-env case return the REC1 default primary domain and REC2 default fallbacks.
This keeps
ARTDefault’s public API consistent with the new endpoint-first domain resolution.
68-118: Max message size & TTL setters map cleanly onto new globals
connectionStateTtl,maxMessageSize,maxSandboxMessageSize,maxProductionMessageSizeand the corresponding setters correctly manipulate the new global variables, with DEBUG builds using sandbox limits and release builds using production limits. Synchronization via@synchronized(self)is sufficient for these rarely-mutated shared values.
128-130:connectivityCheckUrlhelper is simple and sufficientReturning a formatted
https://URL fromARTDefaultConnectivityCheckUrlis straightforward and matches howARTRest.internetIsUpis expected to work. No issues here..github/workflows/integration-test.yaml (1)
32-36: CIABLY_ENVvalue now matches endpoint-style nonprod routing policyUpdating
ABLY_ENVtononprod:sandboxis consistent withgetTestEndpoint()(which usesABLY_ENVwhenABLY_ENDPOINTis unset) and with the new non-production routing-policy semantics from ADR-119. This should exercise the DomainSelector nonprod path in integration tests as intended.Source/ARTWebSocketTransport.m (4)
69-71: Introducing_hostivar is a good decoupling stepStoring the host separately from
optionsavoids the previous pattern where the transport mutated client options just to change its hostname. This makes later host overrides (e.g. fallbacks) more straightforward.
76-90: Initializing_hostfromoptions.domainSelector.primaryDomainis correctSeeding
_hostwithoptions.domainSelector.primaryDomainat construction ensures WebSocket transports always use the REC1-resolved primary domain by default, independent of legacy rest/realtimeHost options. Copyingoptionsafterward keeps other option values stable for the lifetime of the transport.
195-208: WebSocket URL now correctly respects the transport’s current hostSwitching to:
NSURL *url = [urlComponents URLRelativeToURL:[options realtimeUrlForHost:self.host]];means reconnects and fallbacks that call
setHost:will correctly affect subsequent WebSocket URLs without mutating options. This aligns the transport’s behavior with the new DomainSelector-driven host model.
251-257: Host getter/setter now operate purely on transport-local state
setHost:andhostnow wrap the_hostivar instead of reading/writing client options. That keeps responsibility for the active host within the transport and avoids side effects on shared configuration.Source/include/Ably/ARTDefault.h (1)
13-17: Public API additions for fallback hosts align with DomainSelector usageExposing
+fallbackHostsand+fallbackHostsWithEnvironment:here matches the implementation inARTDefault.mand gives SDK consumers a REC2-compliant way to inspect fallback behavior. Using the samerestHost/realtimeHostsignatures keeps the public surface stable.Source/PrivateHeaders/Ably/ARTDefault+Private.h (1)
3-14: Private default and connectivity accessors are consistent with implementationDeclaring
ARTDefaultProductionEnvironment, the max message size accessors,setConnectionStateTtl:,setMaxMessageSize:, andconnectivityCheckUrlhere matches the backing symbols and methods inARTDefault.m. This provides the expected private hooks for tests and internal helpers without altering the public surface.Source/ARTRealtime.m (3)
1497-1519: Fallback reconnection behavior remains consistent with new host source
reconnectWithFallbackstill operates on_fallbacksas before, and only the population of_fallbackshas changed elsewhere to usedomainSelector.fallbackDomains. The reconnect sequence itself (internet check, prioritizedHost override, reconnect) is unchanged and remains compliant with RTN17’s requirements.
1521-1528: RSC15m check now correctly depends on the presence of DomainSelector fallbacksUpdating
shouldRetryWithFallbackForError:options:to:// RSC15m return options.domainSelector.fallbackDomains.count > 0;ensures fallback retries are only considered when the configured endpoint/env/hosts actually yield a non-empty fallback set. This nicely decouples RSC15m from legacy ARTFallbackHosts logic and aligns it with ADR-119’s endpoint semantics.
1671-1688: Realtime fallback initialization now usesdomainSelector.fallbackDomainsWhen the transport fails with a retryable error and no
_fallbacksexist, initializing:NSArray *hosts = clientOptions.domainSelector.fallbackDomains; _fallbacks = [[ARTFallback alloc] initWithFallbackHosts:hosts shuffleArray:clientOptions.testOptions.shuffleArray];replaces ARTFallbackHosts-based lookup with DomainSelector-derived fallbacks. This maintains shuffle behavior and cleanly supports:
- Default/routing-policy/env endpoints → generated fallback list.
- Hostname/legacy host → empty fallback list, skipping fallback attempts.
Behavior for empty
_fallbacksis already handled below, so this change looks correct.Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-157: Appropriate deprecation annotation for testing deprecated API.The
@available(*, deprecated, ...)annotation correctly suppresses compiler warnings while testing the deprecatedrealtimeHostproperty. The message clearly documents why this test is marked deprecated and when the annotation should be removed.Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-42: Well-designed domain selector interface.The
ARTDomainSelectorinterface is clean and well-documented:
- Clear parameter documentation for the initializer
- Appropriate use of nullable annotations
- Correctly disables default
initwithNS_UNAVAILABLE- Read-only properties for
primaryDomainandfallbackDomainsalign with the REC1/REC2 specificationsThe static analysis error about
Foundation/Foundation.hnot being found is a false positive from running clang in isolation without the proper SDK paths.Source/ARTRest.m (3)
376-382: Correct fallback retry expiration handling with domain selector.The reset logic correctly clears
currentFallbackHostandprioritizedHost, then switches back todomainSelector.primaryDomainwhen the fallback retry timeout expires. This aligns with RSC15f.
449-462: Fallback host resolution correctly migrated to ARTDomainSelector.The fallback mechanism now sources hosts from
domainSelector.fallbackDomainsinstead of the removedARTFallbackHosts. The RSC15j spec reference for setting theHostheader is correctly placed.
677-690: Connectivity check URL is now configurable per REC3a/REC3b.The implementation correctly uses
options.connectivityCheckUrlwith a fallback toARTDefault.connectivityCheckUrl. The inline spec references (REC3a, REC3b) help document the behavior.Test/AblyTests/Tests/RestClientTests.swift (6)
45-46: Test URL patterns correctly updated for new domain scheme.The fallback host tests now correctly expect
main.realtime.ably.netfor the primary domain andmain.[a-e].fallback.ably-realtime.comfor fallback domains, aligning with the ADR-119 implementation.
316-351: Comprehensive REC1b tests for endpoint option behavior.These tests properly validate the endpoint option behavior:
- REC1b1: Invalid to combine endpoint with deprecated options
- REC1b2: FQDN endpoint becomes primary domain directly
- REC1b3: Nonprod routing policy resolves correctly
- REC1b4: Production routing policy resolves correctly
The deprecation annotations are appropriately applied where deprecated APIs are used.
1069-1095: REC2c4 test now correctly validates all three request URLs.The assertion indices are now correct:
at(0)for the primary domain,at(1)andat(2)for the two fallback requests. This addresses the issue from the previous review where both fallback assertions incorrectly used index 1.
1185-1253: Comprehensive REC2c tests for implicit fallback domain resolution.These tests thoroughly validate the fallback domain resolution rules:
- REC2c1: Default fallback domains (main.[a-e].fallback.ably-realtime.com)
- REC2c2: Explicit domain → empty fallback set
- REC2c3: Nonprod routing policy fallback domains
- REC2c4: Production routing policy fallback domains
- REC2c5: Deprecated environment option fallback domains
- REC2c6: Deprecated restHost/realtimeHost → empty fallback set
181-181: TODO tracked in issue #2145.The TODO comment about the ineffective assertion is already tracked in GitHub issue #2145, as noted in the past review comments.
206-206: TODO tracked in issue #2145.Same as above - this TODO is covered by the existing GitHub issue.
Source/include/Ably/ARTClientOptions.h (3)
22-26: Well-documented endpoint property addition.The
endpointproperty documentation clearly explains the three accepted value types (routing policy name, nonprod routing policy name, or FQDN) and links to the Platform Customization documentation.
30-38: Consistent deprecation messaging for legacy host properties.Both
restHostandrealtimeHostare properly deprecated with clear messages directing users to theendpointoption. The@deprecatedDoxygen tag in the doc comment and theDEPRECATED_MSG_ATTRIBUTEmacro provide both documentation and compiler warnings.
61-64:connectivityCheckUrlis properly copied in-copyWithZone:.Verification confirms that the
connectivityCheckUrlproperty is correctly assigned at line 239 ofSource/ARTClientOptions.mwithin the-copyWithZone:method implementation (options.connectivityCheckUrl = self.connectivityCheckUrl;). No action needed.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (6)
24-56: Host and fallback expectations align with new endpoint semanticsThe updated regex expectations for the default primary host (
main.realtime.ably.net) and fallback pattern (main.[a-e].fallback.ably-realtime.com), along with the analogous checks in the RTN17h/REC2* tests, correctly reflect the ADR‑119 routing model and keep the assertions implementation‑agnostic via regex rather than hardcoding specific letters. This looks solid and gives good coverage of the new default/fallback behavior.Also applies to: 4055-4071, 4101-4111, 4180-4185
2180-2203: Usingendpointwith a non‑routable IP cleanly exercises timeout behaviorSwitching this test to drive failure via
options.endpoint = "10.255.255.1"rather than older host/environment knobs is a good fit with ADR‑119. It keeps the test focused on realtime request timeout behavior while validating the endpoint surface for IPs. No issues spotted here.
4189-4242: Coverage for primary host selection across prod/nonprod/test endpoints looks goodThe shared helper
_test__091__...every_connection_is_first_attempted_to_the_primary_host_main_realtime_ably_netplus the three concrete tests (default,nonprod:sandbox,test) nicely validate:
- Primary host is always hit first.
- Nonprod routing policy names map to the expected
*.realtime.ably-nonprod.net/*.fallback.ably-realtime-nonprod.comdomains.- Plain policy names (e.g.
"test") map to the*.realtime.ably.net/*.fallback.ably-realtime.comdomains.The use of regex and
dropFirst(8)fornonprod:is a reasonable way to keep tests robust against minor formatting changes.Also applies to: 4244-4260
4264-4353: Fallback retry ordering tests strongly exercise domain selector + REC3 behavior
_test__092__...retry_hosts_in_random_order_after_checkin(and the prod/sandbox/test variants) give thorough coverage of:
- Using
domainSelector.fallbackDomainsas the source of truth for retry ordering.- The interaction between the deterministic
shuffleArrayInExpectedHostOrdertest hook and the observed retry sequence.- The “internet up” check-in semantics between fallback host changes.
These tests appear well‑constructed and should quickly catch regressions in fallback ordering or REC3 connectivity‑check wiring.
4471-4512: EmptyfallbackHostscorrectly disables fallback behavior
test__095__...won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedclearly documents and verifies that an explicitly emptyfallbackHostsarray disables fallback usage, even when the error type would otherwise qualify. The DataGatherer‑based assertion on connection attempts and states is precise and matches the intent described in the comments.
4837-4867: Reachability tests now correctly couple todomainSelector.primaryDomainUpdating the RTN20* tests to assert that
TestReachability.hostequalsclient.internal.options.domainSelector.primaryDomain(rather than legacy realtimeHost/environment fields) gives good end‑to‑end validation that:
- Reachability is monitoring the same primary domain that connections target under the endpoint/domain selector model.
- OS‑driven connectivity changes transition the connection state machine as expected in both CONNECTING and CONNECTED scenarios, and in DISCONNECTED/SUSPENDED with reconnection.
This is a clean way to exercise the new domain selection pipeline via existing reachability hooks.
Also applies to: 4871-4903, 4919-4937, 4947-5013
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-8: ExposingdomainSelectorvia the private header simplifies callersForward‑declaring
ARTDomainSelectorand exposing a readonlydomainSelectorplusrealtimeUrlForHost:and+setDefaultEndpoint:onARTClientOptionsis a good cleanup compared to duplicating domain/fallback properties. It makes the intent behind domain selection clearer and keeps legacy host/environment handling encapsulated inARTClientOptions.m. No issues from the interface perspective.Also applies to: 18-26
Source/ARTClientOptions.m (2)
32-38: Endpoint and domainSelector initialization matches ADR‑119 defaultsThe new ivars for
_endpoint,_restHost,_realtimeHost,_environment, and_domainSelector, combined with initializing_endpointfrom the process‑wideARTDefaultEndpointand_connectivityCheckUrlfromARTDefault, look consistent with the ADR‑119 goal of centralizing domain selection. CachingARTDomainSelectorand resetting it whenever inputs change is a sensible approach.Also applies to: 48-54, 80-82
276-283: Domain selector invalidation on fallback changes is correctThe modifications to
setFallbackHosts:andsetFallbackHostsUseDefault:to reset_domainSelectorensure that any cached primary/fallback domains are recomputed when fallback configuration changes. This matches how you handle endpoint/environment/host changes and should keep the selector state consistent.Also applies to: 284-301
| @interface ARTDomainSelector () | ||
|
|
||
| @property (nonatomic) ARTPrimaryDomainSelectionPolicy primaryDomainSelectionPolicy; | ||
| @property (nonatomic, nullable) NSString *policyId; |
There was a problem hiding this comment.
Please can you document these properties and explain for which primaryDomainSelectionPolicy values they are valid?
Source/ARTDomainSelector.m
Outdated
| The policy that the library will use to determine its REC1 primary domain. | ||
| */ | ||
| typedef NS_ENUM(NSInteger, ARTPrimaryDomainSelectionPolicy) { | ||
| /// REC1a: The `endpoint` client option has not been specified. |
There was a problem hiding this comment.
For each of these please can you document which of the policyId and hostname properties are relevant?
There was a problem hiding this comment.
What's going on in this commit "Fix detection of production environment"? Is it fixing something that was already broken or just a mistake in the implementation of the endpoint client option?
Also:
- why are we re-parsing the
endpointclient option? - when would the "environment" client option start with
nonprod? - what's even the purpose of this check? it seems like we are trying to impose a different outbound message size check for non-production environments (introduced in 9f4bb56?) but why? Realtime already has the ability to communicate a different maxMessageSize, why do we need to hardcode anything?
There was a problem hiding this comment.
It was broken and you are right, neither this check not the property itself is not needed (and was not needed). I've removed both, but let me know if you would like to see it in a separated PR (now I think the maxMessageSize test value is not needed in the SDK code itself either and should be set before each test that relies on it).
There was a problem hiding this comment.
I don't mind the change being in this PR given that it's in a separate commit, but doesn't it need to go before the endpoint commit? otherwise (and I haven't checked which) presumably one of the following is the case:
- you've "fixed" the non-prod check in the
endpointcommit and then immediately removed it (seems unnecessary and as mentioned the previous "fix" for it didn't seem right), or - you've not fixed the non-prod check in the
endpointcommit in which case theendpointcommit is broken
There was a problem hiding this comment.
Also, we still seem to have ARTMaxProductionMessageSize and ARTMaxSandboxMessageSize if I'm reading correctly?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Source/ARTDefault.m (1)
24-53: Confirm DomainSelector configuration forfallbackHostsWithEnvironment:/restHost/realtimeHostAll three helpers construct a fresh
ARTDomainSelectorwithendpointClientOption:nil,fallbackHostsClientOption:nil, the supplied (or nil)environmentClientOption, andfallbackHostsUseDefault:false, then useprimaryDomain/fallbackDomains. That’s conceptually neat, but it subtly changes responsibility fromARTDefault/ARTFallbackHoststoARTDomainSelector. Please confirm that:
fallbackDomainsreturns the expected legacy fallbacks for production and non‑prod environments with this argument set (especially withfallbackHostsUseDefault:false), andrestHost/realtimeHostreturning the sameprimaryDomainmatches your compatibility expectations for any caller that previously relied on distinct defaults.If
ARTDomainSelectorTestsalready cover these static helpers, that should be sufficient.Source/ARTClientOptions.m (3)
22-38: Default endpoint & connectivity URL wiring are sensible; be aware of global stateInitialising
_endpointfrom the process‑wideARTDefaultEndpointand_connectivityCheckUrlfrom[ARTDefault connectivityCheckUrl]gives you a single place to adjust defaults (especially useful in tests). Just keep in mind that+setDefaultEndpoint:only affects instances created after the call; existingARTClientOptionswith their ownendpointordomainSelectorwon’t see that change. If this is intended purely for test setup, that behavior is fine, but it might be worth documenting that implicit lifecycle assumption where this helper is used.Also applies to: 48-54, 80-81
130-191: Legacyenvironment/restHost/realtimeHostconflict checks are strict; consider allowing clearsThe mutual‑exclusion rules between
endpoint,environment, and the legacy host options insetEnvironment:,setRestHost:, andsetRealtimeHost:are consistent with ADR‑119 and nicely symmetric. One side effect, though, is that these checks also run when the new value isnil(i.e. when a caller wants to clear an option), so you can’t currently “unset”environmentwhile anendpointis configured, or vice versa. That’s probably fine for production (options are usually set once), but it may be slightly awkward in tests or reconfiguration code. If you want to make clears cheaper, you could gate the conflict checks on non‑empty values, e.g. only run them whenenvironment/hostorendpointare non‑empty strings.
284-305: Fallback hosts invalidation and default endpoint API are coherent; watch for test cross‑talkResetting
_domainSelectorinsetFallbackHosts:andsetFallbackHostsUseDefault:ensuresprimaryDomain/fallbackDomainsalways reflect the latest legacy fallback settings, andhasEnvironmentcorrectly encapsulates the deprecatedenvironmentproperty. The simple+setDefaultEndpoint:setter forARTDefaultEndpointis fine for a process‑wide default, but because it’s a global mutable string, it’s easy for tests or embedding apps to accidentally leak configuration across call sites. If that becomes a problem, a follow‑up could introduce a small helper to save/restore the default endpoint around tests, or document a convention for doing so.Also applies to: 319-324
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Source/ARTClientOptions.m(8 hunks)Source/ARTConnection.m(1 hunks)Source/ARTDefault.m(4 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTDefault+Private.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (5)
Source/ARTConnection.m (1)
199-203: Confirm environment‑agnosticmaxMessageSizedefaultWhen
_maxMessageSizeis unset, this now always returns[ARTDefault maxMessageSize], removing any prior prod vs sandbox branching. That looks consistent with the new endpoint/domain‑selector model, but it is a behavior change for clients relying on legacy sandbox defaults.Please double‑check that
ARTDefault.maxMessageSizematches the effective legacy defaults across all environments (prod, sandbox, nonprod), and consider adding a focused test to lock this behavior in.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-24: Private exposure ofdomainSelector/URL helpers is consistent and minimalThe added
domainSelectorproperty,setDefaultEndpoint:class method, andrealtimeUrlForHost:helper line up cleanly with the implementation inARTClientOptions.mand keep the internal surface focused while supporting the new endpoint/domain‑selection flow. No changes requested.Source/ARTDefault.m (1)
6-13: Centralised defaults for API version, TTL, max message size, and connectivity URL look goodExtracting these values into module‑level constants and wiring
connectionStateTtl,maxMessageSize, andconnectivityCheckUrlthrough them makes the defaults clear and keeps the setters simple. The@synchronizedwrites on the scalar globals are adequate for the usual “tests tweak then restore” pattern. Please just double‑check that the concrete numbers forARTConnectionStateTtl,ARTMaxProductionMessageSize/ARTMaxSandboxMessageSize, and theARTDefaultConnectivityCheckUrlhost/path are what you intend per ADR/spec/backend limits.Also applies to: 67-81, 83-97, 107-109
Source/ARTClientOptions.m (2)
88-126: Endpoint / DomainSelector lazy initialisation and invalidation look correctThe
setEndpoint:implementation enforcing REC1b1 againsthasEnvironment/hasCustomRestHost/hasCustomRealtimeHost, coupled with resetting_domainSelectoron change, and the lazydomainSelectorthat’s constructed from_endpoint,_fallbackHosts,_environment,_restHost,_realtimeHost, and_fallbackHostsUseDefault, all hang together well.primaryDomain/fallbackDomainssimply forwarding intodomainSelectormakes the rest of the class read clearly. No changes requested here.
193-215: Host/URL builders now consistently useprimaryDomain; confirm it is always a bare host
restUrlComponentsusingself.primaryDomain(rather than a legacyrestHostresolver) andrealtimeUrldelegating torealtimeUrlForHost:self.primaryDomaincleanly route both REST and Realtime throughARTDomainSelector. This looks correct as long asprimaryDomainis always a hostname without scheme or port; ifendpointcan ever be configured as a full URL or include a port,NSURLComponents.hostwould misinterpret it. From the surrounding code it appearsendpointis intended to be a routing policy/FQDN/IP, not a URL, but it would be good to ensure tests cover odd cases like IPv6 literals and non‑standard ports.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Source/ARTDomainSelector.m (1)
70-98: Endpoint “hostname vs routing policy” detection is still too naïve (IPv6 and IP literals)The
endpointClientOptionclassification still only looks for".","::"or"localhost"to decide it’s a hostname:if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || [endpointClientOption isEqualToString:@"localhost"]) { self.primaryDomainSelectionPolicy = ARTPrimaryDomainSelectionPolicyHostname; self.hostname = endpointClientOption; return; }This will misclassify several legitimate endpoint forms:
- IPv6 literals without
::, e.g.@"2001:db8:1:2:3:4:5:6"→ treated as a routing policy ID and turned into2001:db8:1:2:3:4:5:6.realtime.ably.net.- Bracketed IPv6 with ports, or IPv6 with zone IDs (e.g.
[::1]:8080,fe80::1%en0), which don’t necessarily contain"::"in the raw string you want to interpret.- Any future IP/host formats that don’t contain
"."but are still valid host tokens.Given
endpointis intended to accept fully qualified hostnames and IP literals as well as routing policy IDs, this heuristic can lead to incorrect primary domains and bypass the “no automatic fallback for explicit host/IP” rule.Consider:
- Normalizing potential IP literals (strip brackets, ports, zone IDs), then using
inet_ptonfor IPv4/IPv6 to decide “IP literal vs not”.- Treating “localhost” explicitly as you do now.
- Falling back to a “contains '.' → hostname” rule when it’s not an IP literal.
- Only then treating the residual cases as routing policy IDs (after the
nonprod:prefix check).This will avoid misclassifying valid IP endpoints as routing policies while keeping the current routing‑policy behavior intact.
🧹 Nitpick comments (1)
Source/ARTDomainSelector.m (1)
40-45: Minor docstring typos/wording forpolicyIdandhostnameTwo small nits:
- Line 40: “primary and fallback domains names” → “primary and fallback domain names”.
- Line 43: “Sores the value of the primary domain…” → “Stores the value of the primary domain…”.
Tidying these up will make the internal docs easier to read, especially since they explain which policies use
policyIdvshostname.-/// Prefix used for construction of the primary and fallback domains names (for `ARTPrimaryDomainSelectionPolicyNonProductionRoutingPolicy`, `ARTPrimaryDomainSelectionPolicyProductionRoutingPolicy` and `ARTPrimaryDomainSelectionPolicyLegacyEnvironment` values of ``primaryDomainSelectionPolicy``). +/// Prefix used for construction of the primary and fallback domain names (for `ARTPrimaryDomainSelectionPolicyNonProductionRoutingPolicy`, `ARTPrimaryDomainSelectionPolicyProductionRoutingPolicy` and `ARTPrimaryDomainSelectionPolicyLegacyEnvironment` values of ``primaryDomainSelectionPolicy``). @@ -/// Sores the value of the primary domain if assigned directly (for `ARTPrimaryDomainSelectionPolicyHostname` and `ARTPrimaryDomainSelectionPolicyLegacyHost` values of ``primaryDomainSelectionPolicy``). +/// Stores the value of the primary domain if assigned directly (for `ARTPrimaryDomainSelectionPolicyHostname` and `ARTPrimaryDomainSelectionPolicyLegacyHost` values of ``primaryDomainSelectionPolicy``).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Source/ARTDomainSelector.m(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (2)
Source/ARTDomainSelector.m (2)
124-145: Confirm LegacyEnvironment primary domain behavior vs ECO‑5551 requirementFor
ARTPrimaryDomainSelectionPolicyLegacyEnvironmentthe primary domain is:return [NSString stringWithFormat:@"%@.%@.%@", self.policyId, ARTDefaultRoutingSubdomain, ARTDefaultPrimaryTLD];i.e.
<environment>.realtime.ably.net.ECO‑5551 / ADR‑119 state that when legacy options are explicitly configured, existing primary/fallback hostname behavior should be preserved. Switching primary for
environmentfrom the legacy domains toably.netmay be intentional, but if “preserve” is meant literally here, this path might need to stay on the legacy primary TLD instead.Please double‑check this against ADR‑119 and the other language implementations (e.g. ably‑js / ably‑go) to ensure the LegacyEnvironment case matches the intended cross‑SDK behavior.
152-218: Fallback domain behavior and defaults look consistent with REC2The fallback logic appears coherent and matches the described REC2 behavior:
- Explicit
fallbackHostsClientOptionshort‑circuits everything else.- Deprecated
fallbackHostsUseDefaultreusesdefaultFallbackDomains.- Default policy and
LegacyEnvironment/routing‑policy cases generate fivea–efallbacks with the appropriate TLD:
- Default:
main.[a-e].fallback.ably-realtime.com.- Nonprod routing policy:
<policy>.[a-e].fallback.ably-realtime-nonprod.com.- Production + legacy environment:
<policy>.[a-e].fallback.ably-realtime.com.- Hostname/LegacyHost correctly return no automatic fallbacks.
defaultFallbackDomains’s comment now matches the actual return values.This aligns with the earlier REC2 description and the “no fallbacks for explicit hosts” requirement.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
626-635: Address the force-unwrapping safety concern.The
bundle.infoDictionaryis force-unwrapped twice on lines 628 and 631. As noted in the previous review, this will crash if the Info.plist is missing or malformed in the test environment.🔎 Proposed fix: safely unwrap infoDictionary
public func getTestEndpoint() -> String { let bundle = Bundle(for: AblyTests.self) - if let endpoint = bundle.infoDictionary!["ABLY_ENDPOINT"] as? String, endpoint.count > 0 { - return endpoint + guard let info = bundle.infoDictionary else { + return "nonprod:sandbox" } - if let env = bundle.infoDictionary!["ABLY_ENV"] as? String, env.count > 0 { + if let endpoint = info["ABLY_ENDPOINT"] as? String, !endpoint.isEmpty { + return endpoint + } + if let env = info["ABLY_ENV"] as? String, !env.isEmpty { return env } return "nonprod:sandbox" }Source/ARTDomainSelector.m (1)
70-99: IPv6/IP endpoint detection is still heuristic and will misclassify valid IPv6 literalsThe hostname vs routing‑policy decision still relies on
containsString:@"."/@"::"/"localhost". That means many valid IPv6 literals without"::"(e.g.2001:db8:1:2:3:4:5:6) will be treated as routing policy IDs and end up aspolicyId.realtime.ably.net, which is incorrect for an IP endpoint. Same for bracketed/zone‑ID forms.Consider normalizing and validating IP literals explicitly (e.g. strip brackets/port/zone ID, then use
inet_ptonfor IPv4/IPv6) and base the policy choice on that, so any IP literal is always treated as a hostname and never as a routing policy.
🧹 Nitpick comments (1)
Source/ARTDomainSelector.m (1)
40-45: Fix typo in hostname property documentationThe comment says “Sores the value of the primary domain…”. It should be “Stores…”.
Proposed doc fix
-/// Sores the value of the primary domain if assigned directly (for `ARTPrimaryDomainSelectionPolicyHostname` and `ARTPrimaryDomainSelectionPolicyLegacyHost` values of ``primaryDomainSelectionPolicy``). +/// Stores the value of the primary domain if assigned directly (for `ARTPrimaryDomainSelectionPolicyHostname` and `ARTPrimaryDomainSelectionPolicyLegacyHost` values of ``primaryDomainSelectionPolicy``).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/integration-test.yamlAbly.xcodeproj/project.pbxprojExamples/Tests/TestsTests/TestsTests.swiftScripts/log-environment-information.shSource/ARTClientOptions.mSource/ARTConnection.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTFallbackHosts.mSource/ARTRealtime.mSource/ARTRest.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/PrivateHeaders/Ably/ARTDefault+Private.hSource/PrivateHeaders/Ably/ARTDomainSelector.hSource/PrivateHeaders/Ably/ARTFallbackHosts.hSource/include/Ably/ARTClientOptions.hSource/include/Ably/ARTDefault.hSource/include/module.modulemapTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/ARTDomainSelectorTests.swiftTest/AblyTests/Tests/PushAdminTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (8)
- Examples/Tests/TestsTests/TestsTests.swift
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Source/include/Ably/ARTClientOptions.h
- Source/PrivateHeaders/Ably/ARTDomainSelector.h
- Source/Ably.modulemap
- Scripts/log-environment-information.sh
- Source/ARTConnection.m
- Test/AblyTests/Tests/ARTDomainSelectorTests.swift
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTRealtime.mSource/ARTRest.mSource/ARTClientOptions.mSource/ARTDomainSelector.mSource/ARTDefault.mSource/ARTWebSocketTransport.m
🧬 Code graph analysis (1)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (7)
at(1431-1433)at(1440-1445)data(934-936)newRealtime(184-198)clientOptions(144-159)extract(1526-1537)commonAppSetup(107-142)
🪛 GitHub Actions: Check SPM build
Source/include/module.modulemap
[error] 144-144: header '../PrivateHeaders/Ably/ARTSummaryTypes+Private.h' not found. emit-module command failed with exit code 1 during swift build.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
🔇 Additional comments (36)
.github/workflows/integration-test.yaml (1)
35-35: LGTM! Correctly adopts the ADR-119 nonprod routing policy format.The update to
nonprod:sandboxproperly implements the new endpoint-based configuration specified in ADR-119, ensuring integration tests use the correct nonprod environment.Source/include/module.modulemap (1)
145-145: ARTDomainSelector.h is correctly added and exists at the expected path resolved by the relative reference in module.modulemap. No action needed.Test/AblyTests/Test Utilities/TestUtilities.swift (2)
119-119: LGTM! Domain selector integration in test app setup.The POST URL correctly uses
domainSelector.primaryDomainto construct the app creation endpoint, aligning with the new endpoint-driven architecture.
146-146: LGTM! Endpoint configuration in test utilities.The test helper now properly configures
options.endpointusinggetTestEndpoint(), which reads from environment variables with appropriate fallback behavior.Source/PrivateHeaders/Ably/ARTDefault+Private.h (1)
8-8: LGTM! Connectivity check URL accessor added.The new
connectivityCheckUrlmethod provides access to the default connectivity check URL, supporting the configurable connectivity check feature introduced in this PR.Test/AblyTests/Tests/PushAdminTests.swift (1)
236-236: LGTM! Migration to domain selector API.The test correctly uses
options.domainSelector.primaryDomaininstead of the deprecatedrestHost, aligning with the new endpoint-driven domain resolution.Source/ARTWebSocketTransport.m (1)
70-70: LGTM! Host management improved with backing ivar.The WebSocket transport now properly manages its host using a dedicated
_hostivar initialized fromoptions.domainSelector.primaryDomain, rather than misusing client options. The URL construction at line 197 correctly uses[options realtimeUrlForHost:self.host]to build the WebSocket URL.This addresses the previous review concern about indirection and makes the code more maintainable.
Also applies to: 79-79, 197-197, 252-256
Source/ARTRest.m (5)
19-19: LGTM! Necessary imports added.The imports for
ARTDefault+Private.handARTDomainSelector.hare required for the new connectivity check URL and domain selector functionality.Also applies to: 50-50
381-381: LGTM! Fallback expiration reset uses primary domain.When the fallback retry timeout expires (RSC15f), the request host is correctly reset to
domainSelector.primaryDomain, ensuring the client returns to the primary host after successful fallback recovery.
451-452: LGTM! Fallback domains from domain selector.Fallback hosts are now correctly sourced from
domainSelector.fallbackDomainsinstead of the removedARTFallbackHostsclass, aligning with the new endpoint-driven architecture.
520-520: LGTM! Current host returns primary domain.The
currentHostmethod correctly returnsdomainSelector.primaryDomainwhen no prioritized host is set, providing consistent domain resolution across the codebase.
678-678: LGTM! Configurable connectivity check URL.The connectivity check now supports a configurable URL via
options.connectivityCheckUrl, falling back toARTDefault.connectivityCheckUrl. This aligns with REC3a and REC3b spec requirements mentioned in the comment.Source/ARTRealtime.m (1)
1687-1694: LGTM! Fallback host resolution migrated to domainSelector.The fallback host construction correctly migrates from
ARTFallbackHoststoclientOptions.domainSelector.fallbackDomains, preserving theshuffleArraybehavior. The changes align with the endpoint-driven architecture introduced in this PR.Source/include/Ably/ARTDefault.h (1)
13-19: LGTM! Public API updated for endpoint-driven domain resolution.The conversion of
restHostandrealtimeHostto class methods, along with the addition offallbackHosts,fallbackHostsWithEnvironment,port, andtlsPortclass methods, correctly exposes the new domain-selector-driven defaults. These changes align with ADR-119's endpoint-centric architecture.Test/AblyTests/Tests/RestClientTests.swift (4)
176-177: Clarify the need for manually setting prioritizedHost from domainSelector.The test manually sets
client.internal.prioritizedHostfromoptions.domainSelector.primaryDomain. This suggests the client doesn't automatically use the domain selector's primary domain.Is this workaround necessary for the test, or should the client automatically resolve the host from
domainSelector.primaryDomainwhen using an API key? If this is a test-specific workaround, consider adding a comment explaining why it's needed.
199-207: LGTM! Test setup correctly uses setDefaultEndpoint with cleanup.The test properly uses
ARTClientOptions.setDefaultEndpoint()to configure the test endpoint and ensures cleanup withdefer. This pattern is appropriate for test setup.
318-330: LGTM! Endpoint incompatibility validation is thorough.The REC1b1 test correctly validates that the
endpointoption is incompatible with deprecated options (environment,restHost,realtimeHost), usingtryInObjCto verify exceptions are thrown. This aligns with the ADR-119 requirement for mutual exclusivity.
44-46: LGTM! Default domain migration to main.realtime.ably.net is comprehensive.The test assertions have been updated throughout to reflect the new default domain (
main.realtime.ably.net) and fallback pattern (main.[a-e].fallback.ably-realtime.com). This represents the intentional migration from legacy domains (rest.ably.io,realtime.ably.io) to the unifiedably.netdomain structure per ADR-119.This is a significant behavioral change: users who don't explicitly configure endpoint/host options will see different default hostnames after upgrading. The comprehensive test coverage validates this migration is working as designed.
Also applies to: 66-67, 112-112, 134-135, 402-402, 954-956, 1008-1008, 1036-1038, 1065-1065, 1092-1094, 1145-1145, 1180-1182
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector wiring across all targets looks consistent
ARTDomainSelector.m/.hhave properPBXFileReferenceentries and are added under theUtilitiesgroup.- Each framework target (iOS, macOS, tvOS) includes:
- a
PBXBuildFileforARTDomainSelector.min itsSourcesphase, and- a
PBXBuildFileforARTDomainSelector.hin itsHeadersphase, markedPrivateand pointing toPrivateHeaders/Ably/ARTDomainSelector.h.- No dangling IDs or missing phase entries detected for these files.
No changes needed here.
Also applies to: 1236-1237, 2013-2014, 2163-2165, 2342-2344, 2554-2556, 2995-2997, 3140-3142, 3285-3287
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-24: Domain selector/private API exposure looks consistentThe added
ARTDomainSelectorforward declaration,domainSelectorproperty,setDefaultEndpoint:, andrealtimeUrlForHost:all line up with the implementation and give tests/internal code a clear, consistent surface over the new domain‑selection logic.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (9)
44-56: Updated default + fallback host assertions match new domain modelThe expectations:
- First URL host matches
//main.realtime.ably.net- Second URL matches
//main.[a-e].fallback.ably-realtime.comline up with
ARTDomainSelector’s defaultprimaryDomainanddefaultFallbackDomains. This keeps RTN17 host‑fallback tests aligned with ADR‑119.
181-195: Default realtime URL host assertion correctly reflects new primaryDomainChecking
url.host == "main.realtime.ably.net"for the default connection target is consistent withARTDomainSelector’s default primary domain and the newARTDefault.realtimeHostimplementation.
2178-2203: Timeout test now correctly exercises endpoint-based non-routable IPUsing
options.endpoint = "10.255.255.1"to trigger a connection timeout is appropriate with the new endpoint semantics: it drives the domain selector into hostname mode (no fallbacks) and validates RTN14c against an explicit endpoint rather than implicit legacy hosts.
4031-4071: REC2a2 / REC2c1: fallbackHosts & fallbackHostsUseDefault coverage looks good
test__089__...verifies that whenfallbackHostsis a non-empty custom array, connections first hitmain.realtime.ably.netthen each custom host, with no default[a-e]fallbacks.test__089b__...(deprecatedfallbackHostsUseDefault) asserts a first attempt tomain.realtime.ably.netthen amain.[a-e].fallback.ably-realtime.comhost.Both tests correctly reflect the new default primary domain and fallback pattern under the REC2 rules.
4180-4185: Bad-request test correctly asserts no fallback and only primary host retriesThe expectation that both connection attempts target
//main.realtime.ably.netmatches the behavior that 400 errors do not trigger fallback hosts, and the transition sequence[.connecting, .disconnected, .connecting]is consistent with RTN14d/RTB1 semantics.
4189-4241: _test__091 helper nicely covers endpoint routing-policy vs default behaviorsThe helper:
- Uses
options.endpointparameter (nil,nonprod:sandbox,test).- Confirms first attempt is to the appropriate primary host:
- Default:
//main.realtime.ably.net- Nonprod:
//sandbox.realtime.ably-nonprod.net- Prod routing policy:
//test.realtime.ably.net- Then verifies a corresponding
[a-e].fallbackhost on the correct TLD.This is a solid, centralized assertion that
ARTDomainSelector.primaryDomainandfallbackDomainsmatch REC1/REC2 for default, nonprod, and prod routing policies.
4264-4353: _test__092 helper correctly ties randomized fallback order to DomainSelectorUsing:
options.endpoint(nil /nonprod:sandbox/test) andexpectedFallbackHosts = expectedHostOrder.map { options.domainSelector.fallbackDomains[$0] }to define the expected randomized fallback sequence, then reconstructing hosts from actual URLs (including
internet-up.ably-realtime.comchecks) provides good end‑to‑end coverage of:
- REC2c3/REC2c4/REC2c5 fallback host shapes, and
- REC3a connectivity‑check behavior.
The three public tests (prod, sandbox, test) give strong confidence that domain selector + shuffle behavior is exercised correctly.
4835-4890: Reachability tests correctly updated to use domainSelector.primaryDomainBoth connectivity tests assert that
TestReachability.hostequalsclient.internal.options.domainSelector.primaryDomain. This aligns the reachability target with the new endpoint/domain logic, avoiding stale references to legacy host/defaults.
4947-5013: Reachability reconnection test correctly preserves original primary domainCapturing
let primaryDomain = options.domainSelector.primaryDomainbefore settingoptions.endpoint = "10.255.255.1"and then forcingoptions.testOptions.reconnectionRealtimeHost = primaryDomainensures:
- The initial non-routable endpoint exercises the “disconnect while CONNECTING” path.
- Subsequent OS-reachability-triggered reconnects use the canonical primary domain, not the bad IP.
This is a good, targeted regression test for the new endpoint + reachability interaction.
Source/ARTDefault.m (1)
4-13: Default host/fallback/connectivity constants align with DomainSelector + tests
restHost/realtimeHostnow derive from a defaultARTDomainSelector(primary domainmain.realtime.ably.net).fallbackHostsWithEnvironment:andfallbackHostsdelegate tofallbackDomains, so default fallbacks aremain.[a-e].fallback.ably-realtime.com.connectivityCheckUrlwrapsinternet-up.ably-realtime.com/is-the-internet-up.txt, matching the new REC3 connectivity checks used in tests.This keeps global defaults consistent with the endpoint/domain-selection model exercised throughout the suite.
Source/ARTClientOptions.m (6)
105-126: Domain selector integration for primaryDomain/fallbackDomains looks correctThe lazy
domainSelectorconstruction and theprimaryDomain/fallbackDomainsaccessors correctly:
- Feed endpoint, fallbackHosts, environment, restHost, realtimeHost, and fallbackHostsUseDefault into
ARTDomainSelector.- Reset
_domainSelectorwhen any of those inputs change.This centralizes REC1/REC2 logic in one place and keeps
ARTClientOptionsa thin wrapper.
130-191: Legacy environment/restHost/realtimeHost mutual-exclusion rules are consistentThe setters:
- Forbid mixing
endpointwithenvironment/restHost/realtimeHost.- Forbid mixing
environmentwithrestHost/realtimeHost.- Reset
_domainSelectorwhenever any legacy input changes.- Cause
restHost/realtimeHostgetters to default back todomainSelector.primaryDomainwhen no explicit override is set.This matches ADR‑119’s “endpoint vs legacy options” constraints while preserving explicit legacy overrides.
193-215: URL helpers correctly delegate to primaryDomain and allow test overrides
restUrlComponentsnow usesself.primaryDomainfor the host, aligning REST with the REC1 primary domain.realtimeUrlForHost:lets tests build a websocket URL for arbitrary hosts without disturbing other parameters.realtimeUrlsimply wrapsrealtimeUrlForHost:self.primaryDomain.This matches how tests and internals expect to construct URLs in the new endpoint-centric model.
284-300: Fallback host setters correctly invalidate domainSelector cacheBoth
setFallbackHosts:andsetFallbackHostsUseDefault::
- Enforce mutual exclusivity via existing exceptions, and
- Reset
_domainSelectorsofallbackDomainswill be recomputed on next access.That keeps
ARTDomainSelector’s view of the fallback configuration in sync with the options.
302-304: setDefaultEndpoint: wiring looks fine for test/global defaultsStoring the default endpoint in
ARTDefaultEndpointand seeding_endpointfrom it ininitDefaultsis a simple, predictable mechanism for tests to override the default endpoint before creating options. Behavior is local to new instances, which is appropriate.
319-324: hasEnvironment still relies on deprecated property but is safely constrainedUsing
self.environment != nil && [self.environment isNotEmptyString](with deprecation suppressed) is a reasonable way to keep internal checks (setEndpoint:,setRestHost:,setRealtimeHost:) working while the publicenvironmentoption is being phased out.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Source/ARTRealtime.m (1)
1528-1529: Spec reference still incorrect: RSC15m in Realtime code.The comment references
RSC15m(REST Client specification), but this is Realtime client code. This issue was previously flagged but hasn't been addressed yet. The spec reference should be updated to the appropriate RTN (Realtime) specification point.Test/AblyTests/Test Utilities/TestUtilities.swift (1)
626-635: Unsafe force-unwrapping of bundle.infoDictionary.The function force-unwraps
bundle.infoDictionary!on lines 628 and 631, which could crash if the Info.plist is missing or malformed. This issue was previously flagged but hasn't been addressed.🔎 Safer approach using guard
public func getTestEndpoint() -> String { let bundle = Bundle(for: AblyTests.self) - if let endpoint = bundle.infoDictionary!["ABLY_ENDPOINT"] as? String, endpoint.count > 0 { + guard let info = bundle.infoDictionary else { + return "nonprod:sandbox" + } + if let endpoint = info["ABLY_ENDPOINT"] as? String, !endpoint.isEmpty { return endpoint } - if let env = bundle.infoDictionary!["ABLY_ENV"] as? String, env.count > 0 { + if let env = info["ABLY_ENV"] as? String, !env.isEmpty { return env } return "nonprod:sandbox" }Test/AblyTests/Tests/RestClientTests.swift (1)
199-207: Test setup usessetDefaultEndpointfor token-based initialization.The test correctly sets the default endpoint for the test environment and cleans up with
defer. The TODO comment at line 206 is tracked in issue #2145.Source/ARTClientOptions.m (1)
90-99: setEndpoint: still raises for legacy-only configs when copying (needs nil/empty guard)
setEndpoint:enforces REC1b1 unconditionally based on existing legacy options:- (void)setEndpoint:(NSString *)endpoint { // REC1b1: endpoint cannot be used with deprecated options if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { [NSException raise:...]; } _endpoint = endpoint; _domainSelector = nil; }
copyWithZone:always doesoptions.endpoint = self.endpoint;. For an options instance that uses onlyenvironment(or rest/realtime host) and no endpoint, the copied instance already has those legacy fields populated via[super copyWithZone:], sosetEndpoint:nilon the copy will now hithasEnvironment/hasCustom*Hostand raise, even though no endpoint value is being set. This breaks copying valid legacy configurations.The REC1b1 conflict check should only run when a non-empty endpoint is being set; clearing or copying a nil/empty endpoint must be allowed.
Suggested fix: gate REC1b1 on the incoming endpoint value
- (void)setEndpoint:(NSString *)endpoint { - // REC1b1: endpoint cannot be used with deprecated options - if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { - [NSException raise:NSInvalidArgumentException - format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; - } - _endpoint = endpoint; - // Reset domain selector when endpoint changes - _domainSelector = nil; -} + (void)setEndpoint:(NSString *)endpoint { + // REC1b1: endpoint cannot be used with deprecated options when setting a non-empty endpoint + if (endpoint && [endpoint isNotEmptyString] && + (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost)) { + [NSException raise:NSInvalidArgumentException + format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; + } + _endpoint = endpoint; + // Reset domain selector when endpoint changes (including when cleared) + _domainSelector = nil; +}(Use
selfvs class context to match your existing instance method; the key change is conditioning the conflict check onendpointbeing non-nil and non-empty.)This preserves REC1b1 for real endpoint configuration while allowing
copyWithZone:and other code paths that setendpoint = nilon legacy-only options to work correctly.Also applies to: 217-248
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/integration-test.yamlAbly.xcodeproj/project.pbxprojExamples/Tests/TestsTests/TestsTests.swiftScripts/log-environment-information.shSource/ARTClientOptions.mSource/ARTConnection.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTFallbackHosts.mSource/ARTRealtime.mSource/ARTRest.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/PrivateHeaders/Ably/ARTDefault+Private.hSource/PrivateHeaders/Ably/ARTDomainSelector.hSource/PrivateHeaders/Ably/ARTFallbackHosts.hSource/include/Ably/ARTClientOptions.hSource/include/Ably/ARTDefault.hSource/include/module.modulemapTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/ARTDomainSelectorTests.swiftTest/AblyTests/Tests/PushAdminTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (9)
- Test/AblyTests/Tests/ARTDomainSelectorTests.swift
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Ably.xcodeproj/project.pbxproj
- Source/include/module.modulemap
- .github/workflows/integration-test.yaml
- Source/PrivateHeaders/Ably/ARTDomainSelector.h
- Source/Ably.modulemap
- Test/AblyTests/Tests/PushAdminTests.swift
- Scripts/log-environment-information.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTConnection.mSource/ARTRest.mSource/ARTRealtime.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTClientOptions.mSource/ARTWebSocketTransport.m
🧬 Code graph analysis (1)
Test/AblyTests/Tests/RestClientTests.swift (3)
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
getTestEndpoint(626-635)at(1431-1433)at(1440-1445)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (45)
Source/include/Ably/ARTClientOptions.h (3)
22-26: LGTM! Clear documentation for the new endpoint property.The documentation effectively explains the three types of endpoint values (routing policy, nonprod routing policy, FQDN) and references the Platform Customization documentation for more details.
30-38: LGTM! Consistent deprecation of legacy options.The deprecation messages clearly guide users to the new
endpointoption, providing a smooth migration path from the legacyrestHost,realtimeHost, andenvironmentproperties.Also applies to: 52-54
61-64: No action required. TheconnectivityCheckUrlproperty is correctly propagated in the-copyWithZone:implementation (line 21 of the method). The property declaration at lines 61-64 in the header file is consistent with the implementation.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (3)
7-7: LGTM! Domain selector properly exposed for internal use.The forward declaration and readonly property appropriately expose the domain resolution logic to internal consumers while maintaining encapsulation.
Also applies to: 18-18
20-20: LGTM! Endpoint-based default setter replaces environment-based approach.The
setDefaultEndpoint:class method appropriately replaces the previous environment-based default configuration, aligning with the new endpoint-driven architecture.
23-23: LGTM! Helper method improves host override flexibility.The
realtimeUrlForHost:method provides a clean way to construct a realtime URL with an alternative host while preserving other URL parameters, improving upon the previous approach of directly accessing options properties.Source/ARTWebSocketTransport.m (3)
23-23: LGTM! Host is now explicit and properly initialized from domain selector.The addition of the
_hostivar and its initialization fromoptions.domainSelector.primaryDomainaddresses previous feedback about misusing client options. This makes the transport's host explicit and decouples it from repeated options access.Also applies to: 70-70, 79-79
197-197: LGTM! URL construction uses explicit host parameter.The change from
[options realtimeUrl]to[options realtimeUrlForHost:self.host]properly utilizes the new helper method with the explicit host, making the URL construction clearer and more maintainable.
251-257: LGTM! Host accessors properly encapsulate the private ivar.The setter and getter correctly use the
_hostivar, completing the encapsulation of host state within the transport without leaking implementation details through options access.Source/ARTConnection.m (1)
199-203: No action required—the simplification is correct.
ARTDefault.maxMessageSizecorrectly handles sandbox vs production environments using compile-time DEBUG directives (#if DEBUG), not domain/endpoint context at runtime. The simplified implementation inARTConnection.maxMessageSizeappropriately delegates toARTDefault, which returns the correct constant for the build configuration. No verification issues found.Examples/Tests/TestsTests/TestsTests.swift (2)
21-21: LGTM! Sandbox endpoint migration looks correct.The updates align with ADR-119:
- Line 21: Sandbox provisioning URL correctly updated to the new
sandbox.realtime.ably-nonprod.netdomain.- Line 50: Using
endpoint = "nonprod:sandbox"instead of the deprecatedenvironmentoption.Also applies to: 50-50
67-67: Verify background task domain change is intentional.The background URLSession tasks now use
ably.cominstead of the previous domain. This aligns with the new endpoint-based domain resolution whereably.comserves as the default domain.Also applies to: 77-77
Source/ARTRealtime.m (2)
58-58: LGTM! Required import for domain selector integration.
1692-1692: LGTM! Fallback domains now sourced from domain selector.The migration from
ARTFallbackHoststodomainSelector.fallbackDomainsis correct and aligns with ADR-119.Test/AblyTests/Test Utilities/TestUtilities.swift (2)
119-119: LGTM! Using domain selector for app setup.The migration from constructing URLs with
restHostto usingdomainSelector.primaryDomainis correct and aligns with the endpoint-based architecture.
146-146: LGTM! Endpoint replaces environment option.Correctly using
options.endpointinstead of the deprecatedenvironmentfield, consistent with ADR-119.Source/ARTRest.m (6)
19-19: LGTM! Required imports for endpoint migration.The additional imports for
ARTDefault+Private.handARTDomainSelector.hare necessary for the endpoint-based domain resolution.Also applies to: 50-50
381-381: LGTM! Fallback expiration resets to primary domain.When
fallbackRetryExpirationexpires, the code correctly resets todomainSelector.primaryDomain, implementing RSC15f.
445-445: LGTM! Prioritized host updated on successful fallback.When a fallback host succeeds, it becomes the prioritized host per RSC15f.
451-452: LGTM! Fallback host resolution with proper Host header.The fallback logic correctly:
- Sources fallback domains from
domainSelector.fallbackDomainsinstead of the removedARTFallbackHosts- Sets the Host header explicitly per RSC15j
Also applies to: 461-461
520-520: LGTM! currentHost() returns primary domain by default.The method correctly returns
domainSelector.primaryDomainwhen no prioritized host is set, completing the migration away from legacy host options.
678-678: LGTM! Configurable connectivity check URL.The connectivity check now uses
options.connectivityCheckUrlwith a proper fallback toARTDefault.connectivityCheckUrl, providing flexibility while maintaining backward compatibility.Source/include/Ably/ARTDefault.h (1)
15-16: Correct factual error: These are newly introduced class methods, not a change from instance to class methods.ARTDefault.h is a new file created as part of ADR-119 (endpoint support migration). The
restHostandrealtimeHostclass methods are new additions, not a conversion from instance methods. The prior instance-like properties onARTClientOptionsare deprecated (marked with@available(*, deprecated)warnings) in favor of the newendpointproperty, but remain present for backwards compatibility.All usages in the codebase correctly call the new class methods via
ARTDefault.restHost()andARTDefault.realtimeHost()syntax (RestClientTests.swift, lines 422-423). The migration is documented as part of ADR-119 and properly deprecates the old properties rather than removing them.Likely an incorrect or invalid review comment.
Test/AblyTests/Tests/RestClientTests.swift (10)
45-46: URL patterns correctly updated for endpoint-based domain resolution.The fallback host patterns are updated from the legacy
[a-e].ably-realtime.comto the newmain.[a-e].fallback.ably-realtime.comformat, which aligns with the REC2c1 specification for default fallback domains.
177-177: Correctly usesdomainSelector.primaryDomainfor test setup.Using
options.domainSelector.primaryDomainto configure the prioritized host ensures the test uses the same domain resolution logic as production code.
316-330: REC1b1 test validates mutual exclusivity of endpoint and deprecated options.The test correctly verifies that setting
endpointmakesenvironment,restHost, andrealtimeHostoptions invalid, as required by REC1b1.
332-351: REC1b2-REC1b4 tests validate endpoint parsing logic.These tests correctly verify:
- REC1b2: Domain name endpoint → primaryDomain equals the endpoint value
- REC1b3:
nonprod:sandbox→sandbox.realtime.ably-nonprod.net- REC1b4: Production routing policy →
test.realtime.ably.net
392-403: REC1a default endpoint test correctly validatesmain.realtime.ably.net.The test verifies the default primary domain is
https://main.realtime.ably.net, which aligns with ADR-119's default behavior.
510-531: REC1a override test validates custom endpoint configuration.The test correctly verifies that setting
options.endpointto a custom domain updatesdomainSelector.primaryDomainand routes requests accordingly.
875-893: Fallback domains assertion updated correctly for new format.The expected fallback domains are correctly updated to the new
main.[a-e].fallback.ably-realtime.comformat. The test validates thatfallbackHostsUseDefault = trueuses these default fallback domains.
1185-1253: REC2c1-REC2c6 fallback domain tests provide comprehensive coverage.These tests verify:
- REC2c1: Default →
main.[a-e].fallback.ably-realtime.com- REC2c2: Explicit hostname → empty fallback domains
- REC2c3:
nonprod:sandbox→sandbox.[a-e].fallback.ably-realtime-nonprod.com- REC2c4: Production policy →
sandbox.[a-e].fallback.ably-realtime.com- REC2c5: Legacy environment → same as production policy
- REC2c6: Legacy hosts → empty fallback domains
1288-1291: Fallback host extraction uses updateddomainSelector.fallbackDomains.Line 1291 correctly accesses fallback domains via
options.domainSelector.fallbackDomainsinstead of the legacy approach, ensuring consistency with the new domain selection architecture.
1358-1358: Fallback request count assertion usesdomainSelector.fallbackDomains.count.This ensures the test dynamically adapts to the configured fallback domains rather than hardcoding the count.
Source/ARTDomainSelector.m (5)
5-12: Constants are well-organized and clearly named.The TLD constants (
ARTDefaultPrimaryTLD,ARTDefaultNonprodPrimaryTLD, etc.) and routing constants provide good maintainability and make the domain construction logic readable.
16-34: Enum documentation clearly maps policies to REC1 spec sections.Each enum case is well-documented with the corresponding REC1 section and clarifies which properties (
policyIdvshostname) are relevant for each policy.
124-150:primaryDomainswitch correctly implements REC1 domain construction.Each case correctly constructs the domain:
- Default:
main.realtime.ably.net- Hostname: returns the hostname directly
- NonProduction:
{policyId}.realtime.ably-nonprod.net- Production/LegacyEnv:
{policyId}.realtime.ably.net- LegacyHost: returns the hostname directly
The implementation aligns with the REC1 specification.
152-208:fallbackDomainscorrectly implements REC2 fallback logic.The implementation correctly handles:
- REC2a2: Explicit fallback hosts take precedence
- REC2b: Deprecated
fallbackHostsUseDefaultuses default domains- REC2c1-REC2c6: Policy-specific fallback domain generation
The fallback domain format
{policyId}.{letter}.fallback.{TLD}is correct.
210-218:defaultFallbackDomainscomment now matches implementation.The comment correctly describes the return format as
["main.a.fallback.ably-realtime.com", ...], which matches the implementation.Source/ARTDefault.m (5)
6-12: Global constants and mutable state correctly declared.The constants are appropriately typed and the mutable state variables (
ARTConnectionStateTtl,ARTMaxProductionMessageSize,ARTMaxSandboxMessageSize) support runtime configuration via setters.
24-31:fallbackHostsWithEnvironment:correctly delegates to ARTDomainSelector.The method creates an ARTDomainSelector with only the environment option set, allowing legacy environment-based fallback domain resolution to work correctly.
37-53:restHostandrealtimeHostreturn the same default primary domain.Both methods delegate to
ARTDomainSelectorwith all options nil, which returns the default primary domain (main.realtime.ably.net). This is correct per REC1a where both REST and Realtime use the same primary domain.
83-97: Setters use@synchronized(self)for thread safety.The synchronization on the class object (
selfin a class method context) provides thread-safe updates to the global state variables.
107-109:connectivityCheckUrlcorrectly formats the URL.The method returns
https://internet-up.ably-realtime.com/is-the-internet-up.txt, which is the standard Ably connectivity check endpoint.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
44-56: Endpoint / host-fallback test updates align with new domain-selection semanticsThe changes to host expectations and new helpers (
_test__091*,_test__092*) correctly exercise:
- Default primary domain
main.realtime.ably.netand fallback patternmain.[a-e].fallback.ably-realtime.com.- Endpoint-based routing for nil,
"nonprod:sandbox", and"test"endpoints (including nonprod fallback domains).- Custom
fallbackHosts,fallbackHostsUseDefault, and the “empty list disables fallback” case.- Connectivity-check behaviour via
internet-up.ably-realtime.comand validation that reachability usesoptions.domainSelector.primaryDomain.I don’t see issues in these updated tests; they provide good coverage of the new endpoint/primaryDomain flow.
Also applies to: 181-195, 2178-2203, 4031-4512, 4516-4556, 4836-5013
Source/ARTClientOptions.m (1)
32-38: Domain selector / URL plumbing and fallback invalidation look sound
- Initialisation wires
_endpoint, legacy hosts,_environment, and_connectivityCheckUrlcleanly, leaving host/env nil by default.domainSelector,primaryDomain, andfallbackDomainscentralise domain resolution, and all mutating setters (setEndpoint:,setEnvironment:,setRestHost:,setRealtimeHost:,setFallbackHosts:,setFallbackHostsUseDefault:) correctly reset_domainSelectorso changes are picked up.restUrlComponents/restUrlandrealtimeUrlForHost:/realtimeUrlnow rely onprimaryDomainor an explicit host, which matches the new “primary + fallback domain” model.hasCustomRestHost/hasCustomRealtimeHostnow reflect explicit overrides via the backing ivars, which is what the REC1* compatibility checks use.Aside from the
setEndpoint:nil-guard issue called out separately, the rest of this integration looks coherent and consistent with the endpoint/DomainSelector design.Also applies to: 48-82, 105-126, 151-191, 193-215, 276-283, 284-300, 302-304, 319-324
Source/ARTDomainSelector.m
Outdated
| - (void)parsePrimaryDomainSelectionPolicyFromOptions:(nullable NSString *)endpointClientOption | ||
| environment:(nullable NSString *)environmentClientOption | ||
| restHost:(nullable NSString *)restHostClientOption | ||
| realtimeHost:(nullable NSString *)realtimeHostClientOption { | ||
| // Check for endpoint first (REC1b) | ||
| if (endpointClientOption != nil && endpointClientOption.length > 0) { | ||
| // REC1b2 - Check if it's a hostname (contains "." or "::" or is "localhost") | ||
| if ([endpointClientOption containsString:@"."] || | ||
| [endpointClientOption containsString:@"::"] || | ||
| [endpointClientOption isEqualToString:@"localhost"]) { | ||
| self.primaryDomainSelectionPolicy = ARTPrimaryDomainSelectionPolicyHostname; | ||
| self.hostname = endpointClientOption; | ||
| return; | ||
| } | ||
|
|
||
| // REC1b3 - Check if it has "nonprod:" prefix | ||
| NSString *nonprodPrefix = @"nonprod:"; | ||
| if ([endpointClientOption hasPrefix:nonprodPrefix]) { | ||
| NSString *policyId = [endpointClientOption substringFromIndex:nonprodPrefix.length]; | ||
| self.primaryDomainSelectionPolicy = ARTPrimaryDomainSelectionPolicyNonProductionRoutingPolicy; | ||
| self.policyId = policyId; | ||
| return; | ||
| } | ||
|
|
||
| // REC1b4 - Production routing policy | ||
| self.primaryDomainSelectionPolicy = ARTPrimaryDomainSelectionPolicyProductionRoutingPolicy; | ||
| self.policyId = endpointClientOption; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ADR-119 Ably endpoint option IPv6 handling
💡 Result:
Short answer: Ably's endpoint option is a plain string you can set to a custom URL/host (used for enterprise/custom endpoints). When that value is an IPv6 literal you must follow URI rules: enclose the IPv6 address in square brackets and include the port after the bracket if required (e.g. https://[2001:db8::1]:443). See Ably docs for the endpoint option and RFC for IPv6-in-URI syntax. [1][2]
If you want, tell me the exact endpoint string you plan to use and I’ll validate the correct formatting.
Sources:
- Ably realtime constructor / endpoint option. [1]
- RFC 2732 / RFC 3986 — IPv6 literal addresses in URLs (use [addr] form). [2]
🏁 Script executed:
# Search for IPv6-related references in the codebase
rg -i "ipv6|ipv4|inet|hostname.*ipv" --type objc --type markdownRepository: ably/ably-cocoa
Length of output: 157
🏁 Script executed:
# Search for hostname validation logic and tests
rg -i "hostname|endpoint.*ipv|ipv.*endpoint" --type objc --type swift -A 3Repository: ably/ably-cocoa
Length of output: 14138
🏁 Script executed:
# Look for ADR or architecture decision documentation in the repo
fd -i "adr.*119|design.*decision|architecture.*record" -e md -e txtRepository: ably/ably-cocoa
Length of output: 41
🏁 Script executed:
# Check for any documentation about domain selection policies
fd -i "domain.*selection|endpoint.*option|routing.*policy" -e md -e txtRepository: ably/ably-cocoa
Length of output: 41
🏁 Script executed:
# Search for all domain selector tests to see if there are other IPv6 tests
cat Test/AblyTests/Tests/ARTDomainSelectorTests.swift | grep -A 10 "test__.*endpoint.*contains\|test__.*ipv6\|::1\|\[.*:.*:\|2001:"Repository: ably/ably-cocoa
Length of output: 1460
🏁 Script executed:
# Check the full IPv6 test to understand what it expects
rg -A 15 "test__003__ARTDomainSelector__primaryDomain__should_return_hostname_when_endpoint_contains_double_colon" Test/AblyTests/Tests/ARTDomainSelectorTests.swiftRepository: ably/ably-cocoa
Length of output: 751
🏁 Script executed:
# Look for documentation or comments about endpoint option format
rg -B 5 -A 5 "endpoint.*option|endpoint.*format" Source/ARTDomainSelector.m | head -50Repository: ably/ably-cocoa
Length of output: 1117
🏁 Script executed:
# Search for bracket notation handling anywhere in the codebase
rg "\\\[.*:.*:\\\]|\\\[.*ipv" --type objc --type swiftRepository: ably/ably-cocoa
Length of output: 156
🏁 Script executed:
# Search for ARTClientOptions to see how endpoint option is documented
rg -B 5 -A 10 "class.*ARTClientOptions|@interface.*ARTClientOptions|endpoint.*description|endpoint.*documentation" Source/ --type objc --type swift | head -80Repository: ably/ably-cocoa
Length of output: 6129
🏁 Script executed:
# Look for any comments about URI format or RFC in the codebase
rg -i "rfc.*2732|rfc.*3986|uri.*format|bracket.*ipv|ipv.*bracket" Source/ --type objc --type swiftRepository: ably/ably-cocoa
Length of output: 511
🏁 Script executed:
# Check if there are any other hostname validation methods
rg -B 3 -A 8 "hostname.*validation|valid.*hostname|isValidHostname|hostname.*check" Source/ --type objc --type swiftRepository: ably/ably-cocoa
Length of output: 41
🏁 Script executed:
# Look at the complete ARTDomainSelector.m file for context on REC1b2
rg -B 10 -A 20 "REC1b2.*Check if" Source/ARTDomainSelector.mRepository: ably/ably-cocoa
Length of output: 1671
Add bracket notation support for RFC 2732-compliant IPv6 literals.
The hostname detection at lines 77-79 checks for ".", "::", or "localhost", but does not check for bracket notation. Per RFC 2732/RFC 3986, IPv6 addresses in URIs must be enclosed in brackets (e.g., [2001:db8::1]). This means bracket-enclosed uncompressed IPv6 addresses like [2001:db8:1:2:3:4:5:6] will be misclassified as routing policy IDs instead of hostnames. Consider extending the check to also detect "[" to properly handle RFC 2732-compliant endpoints.
🤖 Prompt for AI Agents
Source/ARTDomainSelector.m around lines 70 to 98: the hostname detection treats
IPv6 bracketed literals (RFC 2732) as routing policy IDs because it only checks
for "." or "::" or "localhost"; update the check to also recognize bracket
notation by testing for "[" (preferably verifying a leading "[" and trailing "]"
to match bracketed IPv6 literals) and treat such inputs as hostnames, setting
primaryDomainSelectionPolicy to ARTPrimaryDomainSelectionPolicyHostname and
hostname to the endpoint string.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
626-635: HardengetTestEndpointagainst missing/malformed Info.plistThis still force-unwraps
bundle.infoDictionary, so a missing or malformed Info.plist in the test runner will crash the entire suite. You can safely unwrap once and then read keys from that dictionary:Suggested safer implementation
-public func getTestEndpoint() -> String { - let bundle = Bundle(for: AblyTests.self) - if let endpoint = bundle.infoDictionary!["ABLY_ENDPOINT"] as? String, endpoint.count > 0 { - return endpoint - } - if let env = bundle.infoDictionary!["ABLY_ENV"] as? String, env.count > 0 { - return env - } - return "nonprod:sandbox" -} +public func getTestEndpoint() -> String { + let bundle = Bundle(for: AblyTests.self) + guard let info = bundle.infoDictionary else { + return "nonprod:sandbox" + } + if let endpoint = info["ABLY_ENDPOINT"] as? String, !endpoint.isEmpty { + return endpoint + } + if let env = info["ABLY_ENV"] as? String, !env.isEmpty { + return env + } + return "nonprod:sandbox" +}Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
400-411: Treatendpoint = "nonprod:"as invalid/empty rather than yielding.realtime.ably-nonprod.netThis test currently asserts that
"nonprod:"produces a primary domain of.realtime.ably-nonprod.net, which is a syntactically odd hostname (leading dot) and almost certainly not a useful routing target. It would be safer forARTDomainSelectorto treat an empty nonprod routing ID ("nonprod:"or"nonprod: ") as invalid/absent and either:
- fall back to the default primary domain (e.g.
main.realtime.ably.net), or- surface an invalid configuration rather than emitting a malformed host.
Whichever behavior you choose, this test should be updated to assert that, rather than the leading-dot host.
Source/ARTDomainSelector.m (1)
79-107: Hostname detection still misses RFC‑2732 bracketed IPv6 literals
parsePrimaryDomainSelectionPolicyFromEndpointClientOption:treats an endpoint as a hostname only if it contains"."or"::"or equals"localhost". That still classifies bracketed IPv6 literals (e.g."[2001:db8::1]") as routing policy IDs instead of hostnames.Consider extending the hostname detection to also recognize bracket notation (ideally: leading
"["and trailing"]"), and treat such endpoints asARTPrimaryDomainSelectionPolicyHostnamewithhostname = endpointClientOption.
🧹 Nitpick comments (6)
Source/ARTClientOptions.m (1)
31-37: Endpoint/domainSelector refactor looks solid; consider clear-endpoint edge caseThe new design cleanly centralizes host resolution:
endpoint, deprecatedenvironment/restHost/realtimeHost, and fallback settings all feedARTDomainSelector, andprimaryDomain/fallbackDomainsare consistently used by REST and Realtime.- Incompatibility checks in
setEnvironment:,setRestHost:, andsetRealtimeHost:against a non-emptyendpointcorrectly enforce REC1b/REC1c.- Lazily constructing
_domainSelectorand resetting it in all relevant setters (endpoint,environment,restHost,realtimeHost,fallbackHosts,fallbackHostsUseDefault) avoids stale domains.copyWithZone:copying the_endpointivar (and legacy host/env ivars) while leaving_domainSelectornil avoids the earliersetEndpoint:/copy recursion issue and ensures the selector recomputes for the copy.One minor ergonomics point:
setEndpoint:currently raises if any deprecated options are set, even when the newendpointvalue isnilor empty. If you ever want to support “clearing”endpointon an options instance that already hasenvironment/hosts configured (instead of forcing callers to construct a new options object), you could gate the REC1b1 check onendpointbeing non-empty.Also applies to: 47-81, 89-190, 216-264, 283-303, 318-323
Source/include/Ably/ARTDefault.h (1)
13-19: Check public API compatibility for rest/realtime host accessorsExposing
+restHost,+realtimeHost, and the new fallback host methods as class-level APIs matches the “global defaults” intent, but if previous releases exposed-restHost/-realtimeHostas instance methods, this is a source/binary-breaking change for any external code using[[ARTDefault alloc] init].restHoststyle calls.If those instance methods existed in prior public headers, consider:
- reintroducing them as deprecated instance methods that forward to the new class methods, or
- confirming (via a quick search of your public examples and known integrators) that there are no external callers relying on the old instance form.
This keeps the endpoint/domain selector refactor from inadvertently breaking consumers.
Source/ARTDomainSelector.m (1)
94-101: Guard againstnonprod:without a policy id (optional)If a caller passes
endpoint = "nonprod:",policyIdbecomes an empty string andprimaryDomain/fallbackDomainswill generate malformed hosts like.realtime.ably-nonprod.net. It’s an extreme edge case, but you may want to either:
- Treat an empty suffix as invalid and fall back to
ARTPrimaryDomainSelectionPolicyDefault, or- Assert/log and avoid constructing an empty
policyId.Source/ARTDefault.m (1)
56-77: Global connectionStateTtl/maxMessageSize behavior unchanged in spiritUsing
ARTConnectionStateTtlandARTMaxMessageSizeas module‑level globals with synchronized setters and trivial getters preserves the previous behavior while making the defaults explicit. Just ensure that any sandbox-specific override still runs early enough thatARTDefault.maxMessageSize()returns16384where tests expect that.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-24: ExposingdomainSelectorvia the private header is a good simplificationMaking
ARTClientOptions.domainSelectoravailable (privately) and addingsetDefaultEndpoint:/realtimeUrlForHost:gives tests and internals a clear way to interact with the new domain-selection logic without duplicating state. This also addresses the earlier concern about distinguishingfallbackHostsfrom computedfallbackDomains.A short comment on
setDefaultEndpoint:’s intended usage (primarily for tests that bypasscommonAppSetup) would be helpful but is not critical.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
4264-4338: REC2/RTN17i tests correctly tie fallback retry order todomainSelector.fallbackDomainsand connectivity checks
_test__092__Connection__Host_Fallback__should_retry_hosts_in_random_order_after_checkin_if_an_internet_connection_is_available(endpoint:test:)(and its prod/sandbox/test wrappers):
- build
expectedFallbackHostsfromoptions.domainSelector.fallbackDomainsand a deterministic shuffle,- assert that each change of fallback host is preceded by a connectivity check to
https://internet-up.ably-realtime.com/is-the-internet-up.txt,- handle both prod (
ably-realtime.com) and nonprod (ably-realtime-nonprod.com) fallback TLDs based on the endpoint.This gives excellent coverage of REC2 and REC3 interactions. One minor suggestion: both the transport
networkConnectEventand the HTTP executor listener append to the sharedurlsarray from different callbacks; if you ever hit flakiness, consider funneling both appends throughDispatchQueue.main.asyncfor consistency.Also applies to: 4340-4353
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/integration-test.yamlAbly.xcodeproj/project.pbxprojExamples/Tests/TestsTests/TestsTests.swiftScripts/log-environment-information.shSource/ARTClientOptions.mSource/ARTConnection.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTFallbackHosts.mSource/ARTRealtime.mSource/ARTRest.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/PrivateHeaders/Ably/ARTDefault+Private.hSource/PrivateHeaders/Ably/ARTDomainSelector.hSource/PrivateHeaders/Ably/ARTFallbackHosts.hSource/include/Ably/ARTClientOptions.hSource/include/Ably/ARTDefault.hSource/include/module.modulemapTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/ARTDomainSelectorTests.swiftTest/AblyTests/Tests/PushAdminTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Source/ARTFallbackHosts.m
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
🚧 Files skipped from review as they are similar to previous changes (10)
- Scripts/log-environment-information.sh
- Examples/Tests/TestsTests/TestsTests.swift
- Source/ARTWebSocketTransport.m
- Source/ARTRealtime.m
- Source/Ably.modulemap
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
- Source/include/module.modulemap
- Test/AblyTests/Tests/PushAdminTests.swift
- Source/include/Ably/ARTClientOptions.h
- Source/ARTConnection.m
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTDomainSelector.mSource/ARTRest.mSource/ARTClientOptions.mSource/ARTDefault.m
🧬 Code graph analysis (3)
Test/AblyTests/Tests/RealtimeClientTests.swift (2)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
deprecated(3843-3890)deprecated(3991-4029)deprecated(4074-4111)Test/AblyTests/Tests/RestClientTests.swift (13)
deprecated(302-314)deprecated(317-330)deprecated(354-364)deprecated(367-374)deprecated(377-382)deprecated(385-390)deprecated(418-424)deprecated(865-894)deprecated(896-900)deprecated(903-930)deprecated(1014-1039)deprecated(1098-1122)deprecated(1226-1235)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (7)
match(1516-1524)getTestEndpoint(626-635)getTestToken(505-507)getTestToken(509-513)at(1431-1433)at(1440-1445)clientOptions(144-159)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
Test/AblyTests/Tests/RealtimeClientTests.swift (1)
deprecated(156-180)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (17)
.github/workflows/integration-test.yaml (1)
35-35: ABLY_ENV default aligns with new nonprod routing policySetting
ABLY_ENVtononprod:sandboxmatches the new endpoint-based test defaults (e.g.getTestEndpoint()), and looks correct for integration tests. Just ensure the same value is applied in the otherintegration-test-*.yamlworkflows per the header comment.Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-180: Deprecation wrapper around realtimeHost test is appropriateMarking this test as deprecated to contain the deprecation warning from
ARTClientOptions.realtimeHostis consistent with the pattern used elsewhere and keeps the test while signalling future removal.Test/AblyTests/Test Utilities/TestUtilities.swift (1)
115-159: Test helpers now correctly route via endpoint/domainSelectorSwitching the
/appsPOST tooptions.domainSelector.primaryDomainand defaultingoptions.endpointfromgetTestEndpoint()keeps the test app lifecycle and client options consistent with the new endpoint/domain-selection model. The JWT helper’s simplified query (dropping environment and keepingencrypted) is also in line with using the key material rather than an environment hint. No issues spotted here.Also applies to: 588-612
Source/ARTRest.m (1)
19-20: REST now correctly uses ARTDomainSelector for primary/fallback domains and connectivity checksRouting REST traffic via
options.domainSelector.primaryDomain(including resetting hosts afterfallbackRetryExpiration) and seedingARTFallbackfromdomainSelector.fallbackDomainsaligns fallback behavior with the new endpoint-based domain model. Explicitly setting theHostheader when using a fallback (RSC15j) and usingoptions.connectivityCheckUrlwith anARTDefaultfallback forinternetIsUpare also consistent and leave room for configuration. No issues found.Also applies to: 50-51, 375-383, 441-462, 449-453, 515-521, 677-689
Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is correctly wired into all targets
ARTDomainSelector.m/.hhave properPBXFileReferenceentries, appear in theUtilitiesgroup, and are referenced by matchingPBXBuildFileentries in the iOS, macOS, and tvOS Sources and Headers phases, withPrivateheader visibility consistent with other internal utilities andARTFallback. I don’t see any dangling or missing references like the olderARTMessageOperation+Private.hissue.Also applies to: 1236-1237, 2013-2014, 2163-2163, 2342-2342, 2554-2554, 2995-2995, 3140-3140, 3285-3285
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
5-41: ARTDomainSelector public API is clear and matches REC1/REC2 intentThe initializer and the
primaryDomain/fallbackDomainsproperties cleanly expose the domain-selection policy in a way that matches the implementation inARTDomainSelector.mand the ADR/REC terminology. No issues from the header side.Source/ARTDomainSelector.m (1)
155-210: Fallback-domain behavior matches REC2 and testsThe precedence (
fallbackHostsClientOption→fallbackHostsUseDefault→ REC2c per policy) and the shapes of the generated domains (e.g.main.[a–e].fallback.ably-realtime.com,sandbox.[a–e].fallback.ably-realtime-nonprod.com) line up with the REC2 comments and the updated tests. Using an explicit emptyfallbackHostsClientOptionto disable fallbacks is also correctly supported.Source/ARTDefault.m (3)
6-12: Defaults for API version and connectivity check URL align with testsSetting
ARTDefaultAPIVersionto@"4"and introducingARTDefaultConnectivityCheckUrlwithinternet-up.ably-realtime.com/is-the-internet-up.txtmatch the expectations in the connection tests that assertv=4and the connectivity-check host. No issues here.
23-42: DomainSelector-based defaults for REST/Realtime hosts look correct
+fallbackHostsWithEnvironment:,+fallbackHosts,+restHost, and+realtimeHostnow delegate toARTDomainSelector, so the default and legacy-environment hosts follow the same REC1/REC2 domain logic used elsewhere. This keeps the default paths consistent with the new endpoint semantics.
88-90: connectivityCheckUrl helper is simple and sufficient
+connectivityCheckUrlcentralizes construction of the HTTPS URL for connectivity checks and matches the tests’ regex against//internet-up.ably-realtime.com/is-the-internet-up.txt. Looks good.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (7)
44-56: Default primary and fallback host expectations now correctly trackARTDomainSelectorIn
testUsesAlternativeHostOnResponseandtest__017__Connection__url__should_connect_to_the_default_host, the expectations:
- primary host:
main.realtime.ably.net- fallback host pattern:
main.[a-e].fallback.ably-realtime.commatch the
ARTDomainSelectordefault primary domain anddefaultFallbackDomains()implementation. These tests give good coverage of the happy path for REC1a/REC2c1 in the realtime stack.Also applies to: 181-195
2178-2203: Usingendpointfor the non‑routable host case is consistent with new config modelSwitching
test__058__...connection_attempt_should_fail_if_not_connected_within_the_default_realtime_request_timeoutto use:options.endpoint = "10.255.255.1" // non-routable IP addressaligns with ADR‑119, exercises the Hostname primary-domain path in
ARTDomainSelector, and keeps the rest of the timeout behavior unchanged. Looks correct.
3991-4030: REC2/RTN17 tests cover explicit, default, and empty fallback host configurations wellThe updated/added tests:
test__087__...custom_endpoint_should_result_in_time_outs(deprecated, usingrealtimeHost)test__089__...applies_when_an_array_of_ClientOptions_fallbackHosts_is_providedtest__089b__...applies_when_deprecated_fallbackHostsUseDefault_option_is_set_to_truetest__095__...won_t_use_fallback_hosts_feature_if_an_empty_array_is_providednow assert:
- primary connections go to
main.realtime.ably.netby default,- custom
fallbackHostsarrays are used exactly (no extra implicit fallbacks),fallbackHostsUseDefault = trueresults inmain.[a-e].fallback.ably-realtime.com,fallbackHosts = []disables fallback usage entirely (two attempts, both to the primary host).This matches the
ARTDomainSelector.fallbackDomainsprecedence rules and nicely exercises the edge case of an explicit empty fallback list.Also applies to: 4031-4071, 4471-4512
4189-4242: New_test__091__...helper thoroughly validates primary host mapping for different endpoints
_test__091__Connection__Host_Fallback__every_connection_is_first_attempted_to_the_primary_host_main_realtime_ably_net(endpoint:test:)and its three wrappers:
- check default (
endpoint = nil):main.realtime.ably.netthenmain.[a-e].fallback.ably-realtime.com,- check nonprod routing policy (
endpoint = "nonprod:sandbox"):sandbox.realtime.ably-nonprod.netthensandbox.[a-e].fallback.ably-realtime-nonprod.com,- check production routing policy (
endpoint = "test"):test.realtime.ably.netthentest.[a-e].fallback.ably-realtime.com.That matches
ARTDomainSelector’sDefault,NonProductionRoutingPolicy, andProductionRoutingPolicycases and provides strong integration coverage of REC1/REC2 behavior end‑to‑end.Also applies to: 4244-4260
4337-4337: Connectivity-check host expectations matchARTDefault.connectivityCheckUrlThe tests that look for URLs matching:
"//internet-up.ably-realtime.com/is-the-internet-up.txt"align with
ARTDefaultConnectivityCheckUrland+connectivityCheckUrl. This ensures the fallback logic and HTTP executor use the intended REC3 connectivity endpoint.Also applies to: 4455-4468
4836-4867: Reachability integration now explicitly tracksoptions.domainSelector.primaryDomainIn
test__109,test__110, andtest__106__.../test__106_b__...the assertions that:XCTAssertEqual(reachability.host, client.internal.options.domainSelector.primaryDomain)ensure that OS/network reachability is always monitoring the same primary host that
ARTDomainSelectorselected (including whenendpointis overridden). That’s a solid regression guard around the new endpoint/domain logic.Also applies to: 4870-4903, 4908-4939, 4947-5013
4947-4953: RTN20c test correctly pins reconnection to the original primary domainIn
test__106_b__...should_restart_the_pending_connection_attempt..., capturing:let primaryDomain = options.domainSelector.primaryDomain options.endpoint = "10.255.255.1" options.testOptions.reconnectionRealtimeHost = primaryDomainensures that when reachability flips back to “reachable”, reconnection is attempted against the original primary domain, not the non‑routable endpoint. That’s a good regression test tying together endpoint overrides and
reconnectionRealtimeHost.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
628-637: Defensive unwrap recommended forbundle.infoDictionaryThe past review comment on these lines correctly identified that force-unwrapping
bundle.infoDictionary!twice (lines 630, 633) can crash if the Info.plist is missing or malformed in the test runner. Consider adopting the defensive unwrap pattern suggested in the previous review.Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
400-411: Test validates questionable behavior for empty routing IDThe past review comment correctly identified that
endpoint = "nonprod:"producing.realtime.ably-nonprod.netcreates a syntactically invalid hostname with a leading dot. A simple misconfiguration would result in an unusable host at runtime. Consider implementing the validation and fallback logic suggested in the previous review.Source/ARTDomainSelector.m (1)
85-88: Add bracket notation detection for RFC 2732-compliant IPv6 literals.The hostname detection checks for
".","::", or"localhost"but does not check for bracket notation"[". Per RFC 2732/3986, IPv6 addresses in URIs must be enclosed in brackets (e.g.,[2001:db8::1]). This means uncompressed bracketed IPv6 addresses like[2001:db8:1:2:3:4:5:6]will be misclassified as routing policy IDs instead of hostnames.🔎 Suggested fix
// REC1b2 - Check if it's a hostname (contains "." or "::" or is "localhost") if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || + [endpointClientOption hasPrefix:@"["] || [endpointClientOption isEqualToString:@"localhost"]) { self.primaryDomainSelectionPolicy = ARTPrimaryDomainSelectionPolicyHostname; self.hostname = endpointClientOption; return; }
🧹 Nitpick comments (3)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (3)
46-55: Default and fallback host expectations updated correctlyThe updated expectations for the primary host (
main.realtime.ably.net) and fallback hosts (main.[a-e].fallback.ably-realtime.com) across these tests look consistent with the new endpoint/domain-selection semantics and with the rest of the file. The use of regexes is fine; for fixed-host checks you could simplify to directurl.host == "main.realtime.ably.net"comparisons if you ever revisit this, but that’s purely cosmetic.Also applies to: 181-195, 4059-4070, 4101-4111, 4182-4185, 4546-4556
2180-2203: Endpoint +commonAppSetupcombination may violate ADR‑119 invariantsUsing
options.endpoint = "10.255.255.1"intest__058__..._timeoutandtest__106_b__..._restart_the_pending_connection_attempt...is a good way to exercise non‑routable endpoint behavior and OS reachability integration. One thing to double‑check is whetherAblyTests.commonAppSetup(for:)sets any legacy host options (environment,restHost, orrealtimeHost): ADR‑119 treats those as incompatible withendpoint.If it does, consider for these two tests either:
- Constructing a fresh
ARTClientOptionsinstead of callingcommonAppSetup, or- Explicitly clearing any legacy host/environment fields before assigning
endpoint,so the tests exercise a well‑defined “endpoint‑only” configuration rather than an undefined mix.
Also applies to: 4948-4953
4264-4353: REC2 “random order after check-in” helper looks correct; transportFactory re-assignment is probably redundantParameterising the REC2/RTN17i test via
_test__092__...should_retry_hosts_in_random_order_after_checkin...and reusing it for prod,nonprod:sandbox, andtestendpoints is a good refactor:
expectedFallbackHostsis derived fromoptions.domainSelector.fallbackDomainsplus the deterministicexpectedHostOrder, so expectations track the domain selector.extractHostnameand theinternet-up.ably-realtime.comcheck make sure each host change is preceded by a connectivity check, as required by REC3a.One small nit: after
let testEnv = AblyTests.newRealtime(options), you dooptions.testOptions.transportFactory = testEnv.transportFactory. That only mutates the localoptionsinstance; theclientalready has its own copied options, so this assignment doesn’t affect the running client. UnlessTestProxyHTTPExecutor’s logger depends on that specific field inoptions, you can likely drop this line to avoid confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/integration-test.yamlAbly.xcodeproj/project.pbxprojExamples/Tests/TestsTests/TestsTests.swiftScripts/log-environment-information.shSource/ARTClientOptions.mSource/ARTConnection.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTFallbackHosts.mSource/ARTRealtime.mSource/ARTRest.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/PrivateHeaders/Ably/ARTDefault+Private.hSource/PrivateHeaders/Ably/ARTDomainSelector.hSource/PrivateHeaders/Ably/ARTFallbackHosts.hSource/include/Ably/ARTClientOptions.hSource/include/Ably/ARTDefault.hSource/include/module.modulemapTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/ARTDomainSelectorTests.swiftTest/AblyTests/Tests/PushAdminTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Source/ARTFallbackHosts.m
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
🚧 Files skipped from review as they are similar to previous changes (6)
- Source/ARTRealtime.m
- Source/ARTRest.m
- Source/PrivateHeaders/Ably/ARTDefault+Private.h
- Examples/Tests/TestsTests/TestsTests.swift
- .github/workflows/integration-test.yaml
- Source/ARTWebSocketTransport.m
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTConnection.mSource/ARTDomainSelector.mSource/ARTClientOptions.mSource/ARTDefault.m
🧬 Code graph analysis (1)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
match(1518-1526)at(1433-1435)at(1442-1447)data(936-938)newRealtime(186-200)clientOptions(146-161)extract(1528-1539)commonAppSetup(107-144)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)Test/AblyTests/Test Utilities/TestProxyTransportFactory.swift (1)
transport(12-24)Test/AblyTests/Test Utilities/DataGatherer.swift (2)
waitForData(36-57)waitForData(62-64)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (35)
Scripts/log-environment-information.sh (1)
3-3: LGTM! Domain references correctly updated.All references to the old sandbox domain (
sandbox-realtime.ably.io) have been consistently updated to the new nonprod domain (sandbox.realtime.ably-nonprod.net), aligning with ADR-119. The changes cover the comment, echo output, and dig command execution.Also applies to: 10-11
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-45: Well-structured header for domain selection policy.The
ARTDomainSelectorinterface is cleanly designed with appropriate nullability annotations and documentation. The separation of the designated initializer with all client options from a defaultinitfollows good Objective-C patterns.Minor observation: The documentation references "REC1" and "REC2" (lines 6, 34, 39) which appear to be spec references. Consider adding a brief comment or link to the ADR-119 specification for maintainability.
Test/AblyTests/Tests/PushAdminTests.swift (1)
236-236: Correct migration todomainSelector.primaryDomain.The URL construction now uses the new
ARTDomainSelectorAPI, which aligns with the endpoint-based domain resolution introduced in this PR.Source/Ably.modulemap (1)
144-144: Module map correctly updated forARTDomainSelector.The private header
ARTDomainSelector.his properly added to thePrivatemodule, enabling test access while keeping it out of the public API surface.Source/ARTConnection.m (1)
199-203: SimplifiedmaxMessageSizeimplementation.The removal of production vs. sandbox branching aligns with the endpoint-based configuration model. The server-provided
maxMessageSizefromConnectionDetailsstill takes precedence (viasetMaxMessageSize:), so this default only applies before connection or when the server doesn't specify a value.Source/include/module.modulemap (1)
144-144: SPM module map correctly updated.The
ARTDomainSelector.hheader is properly added with the correct relative path for SPM builds, consistent with the Xcode module map update.Test/AblyTests/Tests/RealtimeClientTests.swift (1)
156-157: Appropriate use of deprecation annotation for testing deprecated API.The
@available(*, deprecated, ...)annotation is a clean way to test the deprecatedrealtimeHostproperty without triggering compiler warnings. The message clearly explains the intent and when to remove the annotation.Source/include/Ably/ARTDefault.h (1)
15-17: No backward compatibility issue found.These are class methods on
ARTDefault(appropriate for returning default values), not instance methods. A search of the codebase reveals no calls to these methods as instance methods—no code exists like[[ARTDefault alloc] init].restHost. TherestHostandrealtimeHostproperties inARTClientOptionsare separate and marked as deprecated. No breaking changes are present.Likely an incorrect or invalid review comment.
Test/AblyTests/Tests/RestClientTests.swift (3)
302-390: LGTM: Comprehensive endpoint validation testsThe new endpoint validation tests (REC1b1-REC1d2) provide excellent coverage of the domain selector behavior per ADR-119. The tests correctly:
- Validate incompatibility rules between endpoint and legacy options
- Check primary domain derivation from various option combinations
- Use appropriate deprecation markers to suppress compiler warnings
1185-1253: LGTM: Comprehensive fallback domain testsThe new fallback domain validation tests (REC2c1-REC2c6) thoroughly cover the various scenarios for fallback host generation. The tests correctly validate:
- Default fallback patterns for production and nonprod environments
- Empty fallback arrays for explicit domain names
- Routing policy-specific fallback host patterns
- Legacy option handling with appropriate deprecation markers
45-46: LGTM: Domain pattern updates align with ADR-119All domain assertion updates throughout the file correctly reflect the migration to the new endpoint-based domain resolution:
- Primary domain:
main.realtime.ably.net(replacingrest.ably.io)- Fallback pattern:
main.[a-e].fallback.ably-realtime.com(replacing old environment-based patterns)The changes are consistent and align with the ADR-119 implementation.
Also applies to: 66-67, 112-112, 134-135, 402-402, 464-464, 955-956, 1008-1008, 1037-1038, 1065-1065, 1092-1094, 1146-1146, 1180-1182, 1261-1261, 1406-1406, 1659-1659
Ably.xcodeproj/project.pbxproj (5)
360-365: ARTDomainSelector build files are correctly defined for all frameworks.The three
ARTDomainSelector.mand threeARTDomainSelector.hPBXBuildFile entries consistently reference the shared file references and follow the same pattern as other cross-target sources/headers. No dangling IDs or attribute issues here.
1236-1237: File references for ARTDomainSelector are consistent with existing private utilities.
ARTDomainSelector.mandARTDomainSelector.hare declared once, with the header placed underPrivateHeaders/Ablyand the implementation in the Source root, matching the layout of other private utility types.
2013-2014: Placement of ARTDomainSelector in the Utilities group looks appropriate.Adding both the header and implementation into the
Utilitiesgroup keeps the structure coherent and mirrors how similar cross-cutting helpers are organized.
2163-2163: ARTDomainSelector.h is wired into all Headers phases as a private header.Each framework target (iOS, macOS, tvOS) now exposes
ARTDomainSelector.hvia its Headers build phase withATTRIBUTES = (Private), giving internal visibility while keeping it out of the public API surface. The wiring is symmetric and matches other private headers.Also applies to: 2342-2342, 2554-2554
2995-2995: ARTDomainSelector.m is compiled into all three framework targets.The new
ARTDomainSelector.m in Sourcesentries are present for iOS, macOS, and tvOSSourcesphases, all pointing to the same file reference. This ensures the new domain selection logic is available across all supported platforms without build gaps.Also applies to: 3140-3140, 3285-3285
Source/include/Ably/ARTClientOptions.h (3)
22-26: LGTM! Clear documentation for the new endpoint option.The property declaration and documentation align well with ADR-119 requirements, clearly describing the three supported formats (routing policy, nonprod routing policy, and FQDN).
30-38: LGTM! Proper deprecation of legacy options.The deprecation attributes provide clear migration guidance, directing developers to the new
endpointoption. This maintains backward compatibility while encouraging adoption of the new API.Also applies to: 52-54
61-65: LGTM! connectivityCheckUrl property properly implemented.The property is well-documented and correctly propagated in
copyWithZone:(line 238 in ARTClientOptions.m), addressing the previously flagged issue.Source/ARTDomainSelector.m (2)
133-153: LGTM! Clean primary domain resolution logic.The switch statement correctly handles all REC1 cases with proper domain construction for each policy type. The implementation aligns with the ADR-119 specification.
155-210: LGTM! Fallback domain logic correctly implements REC2 specification.The implementation properly prioritizes explicit fallback hosts, then
fallbackHostsUseDefault, then policy-based fallbacks. The domain construction for each policy type (default, nonprod, production, hostname) aligns with the specification.Source/ARTDefault.m (3)
6-11: LGTM! Constants properly externalized.The constants are well-defined and the comment on line 9 provides helpful context about the sandbox value difference.
23-42: LGTM! Clean integration with ARTDomainSelector.The domain resolution correctly delegates to
ARTDomainSelectorfor both primary and fallback domain determination. Creating new instances for default value access is acceptable.
88-90: LGTM! Connectivity check URL helper correctly implemented.The method properly constructs the HTTPS URL from the constant, providing a clean public API for connectivity checking.
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
7-23: LGTM! Clean private API surface.The private header properly exposes
domainSelectorfor internal use and replacessetDefaultEnvironmentwithsetDefaultEndpoint, aligning with the endpoint-centric design. The removal of unused properties (per past comments) improves maintainability.Source/ARTClientOptions.m (6)
31-37: LGTM! Instance variables properly declared.The ivars provide clean backing storage for the endpoint and legacy options, allowing controlled access via properties while supporting efficient copying.
89-102: LGTM! Endpoint setter correctly enforces mutual exclusivity.The setter properly implements REC1b1 by preventing
endpointfrom being combined with deprecated options. The domain selector reset ensures fresh computation. The direct ivar access incopyWithZone:(line 220) correctly bypasses the setter to avoid exceptions when copying legacy-only configurations.
104-125: LGTM! Clean lazy initialization and delegation.The
domainSelectorproperty uses proper lazy initialization, correctly suppresses deprecation warnings for internal usage, and cleanly delegatesprimaryDomainandfallbackDomainsto the selector.
129-190: LGTM! Legacy host setters properly enforce constraints.The setters correctly implement mutual exclusivity per REC1b1 and REC1c1, checking both
endpointpresence and non-empty values before raising exceptions. Domain selector reset logic ensures consistency. Getters provide appropriate fallback toprimaryDomain.
216-264: LGTM! Copy semantics correctly implemented.The
copyWithZone:method properly:
- Uses direct ivar access for
endpointto bypass setter validation (line 220)- Copies
connectivityCheckUrl(line 238), addressing the previously flagged issue- Conditionally copies legacy options with direct ivar access (lines 244-246)
- Suppresses deprecation warnings appropriately
283-303: LGTM! Fallback setters and default endpoint method properly implemented.The fallback host setters correctly reset the domain selector to invalidate cached values. The
setDefaultEndpoint:class method appropriately replaces the legacysetDefaultEnvironment:approach.Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4)
4190-4242: Endpoint-based primary/fallback host helper covers prod & nonprod nicelyThe new
_test__091__...every_connection_is_first_attempted_to_the_primary_host...helper plus its three wrappers (prod default,nonprod:sandbox, andtest) give clear coverage of endpoint semantics:
- For nil endpoint:
main.realtime.ably.net+main.[a-e].fallback.ably-realtime.com.- For
nonprod:<name>:<name>.realtime.ably-nonprod.net+<name>.[a-e].fallback.ably-realtime-nonprod.com.- For named endpoint:
<endpoint>.realtime.ably.net+<endpoint>.[a-e].fallback.ably-realtime.com.The way you gate on
.disconnectedand.failedand then assert the first two connection URLs matches this matrix cleanly. No issues here.Also applies to: 4245-4260
4471-4512: RTN17g behavior forfallbackHosts = []is well specified
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providednicely captures the intended semantics for an explicit emptyfallbackHosts:
- With
.hostUnreachableforced, the connection goes.connecting → .disconnected → .connecting.- Exactly two connection attempts are observed, both to the primary host (
main.realtime.ably.net), and no fallback hosts are used despite the error qualifying for fallback.The DataGatherer usage and expectations look solid; this should guard against regressions in the “empty list disables fallback” behavior.
3843-3889: Deprecation annotations around legacyrealtimeHost/fallbackHostsUseDefaulttests make senseMarking the tests that exercise
ARTClientOptions.realtimeHostandfallbackHostsUseDefaultas@available(*, deprecated, ...)is a pragmatic way to keep coverage for those legacy options without polluting the build with deprecation warnings. The deprecation messages clearly indicate these tests should be removed when the underlying options are dropped. No changes needed.Also applies to: 3991-4030, 4073-4111
4837-4854: UsingdomainSelector.primaryDomainin reachability tests is the right abstractionIn the RTN20* tests, asserting that the
TestReachabilityinstance is constructed withclient.internal.options.domainSelector.primaryDomainties OS reachability behavior directly to the domain selection logic, rather than a hard‑coded host. This keeps the tests aligned with whatever primary host DomainSelector chooses for prod vs nonprod, and matches how other utilities (e.g.RealtimeClientTests.commonAppSetup) usedomainSelector. Looks good.Also applies to: 4871-4890, 4909-4936
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
628-637: Force-unwrappingbundle.infoDictionarycan crash if Info.plist is missing or malformed.The previous review flagged this concern, but it hasn't been addressed. While unlikely in practice, defensive unwrapping improves robustness.
🔎 Proposed defensive unwrapping
public func getTestEndpoint() -> String { let bundle = Bundle(for: AblyTests.self) - if let endpoint = bundle.infoDictionary!["ABLY_ENDPOINT"] as? String, endpoint.count > 0 { + guard let info = bundle.infoDictionary else { + return "nonprod:sandbox" + } + if let endpoint = info["ABLY_ENDPOINT"] as? String, !endpoint.isEmpty { return endpoint } - if let env = bundle.infoDictionary!["ABLY_ENV"] as? String, env.count > 0 { + if let env = info["ABLY_ENV"] as? String, !env.isEmpty { return env } return "nonprod:sandbox" }Test/AblyTests/Tests/ARTDomainSelectorTests.swift (1)
400-411: The endpoint"nonprod:"produces an invalid hostname with a leading dot.This test asserts that
"nonprod:"yields".realtime.ably-nonprod.net", which is syntactically invalid and would fail DNS resolution. Consider whetherARTDomainSelectorshould either:
- Treat empty routing ID after
nonprod:as invalid and fall back to default, or- Surface a configuration error
This prevents misconfiguration (e.g., typo
"nonprod:"instead of"nonprod:sandbox") from silently producing unusable hostnames.Source/ARTDomainSelector.m (1)
84-92: Add bracket notation support for RFC 2732-compliant IPv6 literals.The hostname detection checks for
".","::", or"localhost", but does not check for bracket notation ([). Per RFC 2732/RFC 3986, IPv6 addresses in URIs must be enclosed in brackets (e.g.,[2001:db8::1]). Uncompressed IPv6 addresses like[2001:db8:1:2:3:4:5:6]will be misclassified as routing policy IDs.🔎 Proposed fix to handle bracketed IPv6
// REC1b2 - Check if it's a hostname (contains "." or "::" or is "localhost" or starts with "[") if ([endpointClientOption containsString:@"."] || [endpointClientOption containsString:@"::"] || + [endpointClientOption hasPrefix:@"["] || [endpointClientOption isEqualToString:@"localhost"]) {Source/ARTClientOptions.m (1)
89-98: Asymmetric nil handling compared to legacy setters.The conflict check runs unconditionally, whereas
setEnvironment:,setRestHost:, andsetRealtimeHost:(lines 131, 152, 173) only throw when setting a non-empty value (self.endpoint && [self.endpoint isNotEmptyString]).This asymmetry means:
options.environment = nilsucceeds even when endpoint is set (allows clearing)options.endpoint = nilthrows if environment is set (cannot clear)The critical
copyWithZone:path is already handled (line 220 bypasses the setter), so this is a minor consistency concern.Suggested fix for consistency
- (void)setEndpoint:(NSString *)endpoint { // REC1b1: endpoint cannot be used with deprecated options - if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { - [NSException raise:NSInvalidArgumentException - format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; + if (endpoint && [endpoint isNotEmptyString]) { + if (self.hasEnvironment || self.hasCustomRestHost || self.hasCustomRealtimeHost) { + [NSException raise:NSInvalidArgumentException + format:@"The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options."]; + } } _endpoint = endpoint; // Reset domain selector when endpoint changes _domainSelector = nil; }
🧹 Nitpick comments (4)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
4187-4254: Endpoint-based primary host tests (test__091_...) look correct; consider minor naming clarityThe helper
_test__091__...every_connection_is_first_attempted_to_the_primary_host_main_realtime_ably_net(endpoint:test:)plus the three public tests:
- Correctly cover:
- Default endpoint (
nil) →main.realtime.ably.net+main.[a-e].fallback.ably-realtime.com.- Non‑prod sandbox (
"nonprod:sandbox") →sandbox.realtime.ably-nonprod.net+sandbox.[a-e].fallback.ably-realtime-nonprod.com.- Custom routing policy (
"test") →test.realtime.ably.net+test.[a-e].fallback.ably-realtime.com.- Use one forced
.hostUnreachablefollowed by a real connection failure (invalid key) to guarantee at least one primary and one fallback connection are observed.The only nit is that the helper’s name still hard‑codes “main_realtime_ably_net” even when testing other endpoints; if you touch this again, consider renaming it to something endpoint‑agnostic. Behaviour‑wise, this is solid.
4264-4353: test__092_... randomised fallback ordering correctly accounts for endpoint variants (small dead-code nit)The
_test__092__Connection__Host_Fallback__should_retry_hosts_in_random_order_after_checkin_if_an_internet_connection_is_available(endpoint:test:)helper:
- Uses
options.endpointandoptions.domainSelector.fallbackDomainsto computeexpectedFallbackHostsacross:
- Default (
nil) primarymain.realtime.ably.netfallback set."nonprod:sandbox"withsandbox.[a-e].fallback.ably-realtime-nonprod.com."test"withtest.[a-e].fallback.ably-realtime.com.- Forces
.hostUnreachable, injects a deterministic shuffle viashuffleArrayInExpectedHostOrder, and verifies that:
- Each fallback host is tried in the expected order.
- There is an
internet-up.ably-realtime.comconnectivity check between host changes.Two small points:
- The line
options.testOptions.transportFactory = testEnv.transportFactorymutates the originaloptionsafterAblyTests.newRealtime(options)has already copied it, so it’s effectively a no‑op and can be removed.- If you ever split connectivity checks per environment, the hard‑coded
"internet-up.ably-realtime.com"pattern will need to be revisited; for now it matches the current behaviour.Test/AblyTests/Tests/RealtimeClientChannelTests.swift (1)
2796-2799: GlobalARTDefault.maxMessageSizeoverride is well-scoped but could be fragile under parallel testsThe save/set/
deferpattern is correct and keeps this test deterministic even if the library default changes. However, becauseARTDefault.maxMessageSizeis process‑global, this override will affect any other tests running concurrently. If you ever enable parallel test execution, consider injectingmaxMessageSizevia configuration onclient(or a test-only hook) instead of mutating the global default.Source/ARTWebSocketTransport.m (1)
23-23: Host handling viaARTDomainSelectorlooks correct; consider copying in the setter for safetyRouting the WebSocket URL through
options.domainSelector.primaryDomainand[options realtimeUrlForHost:self.host]is a clean way to centralize endpoint/fallback selection in the transport.One small improvement: the custom
setHost:currently just assigns the incomingNSStringto_host. If the property inARTWebSocketTransport+Private.his declared ascopy(which is common for string properties), this bypasses the intended semantics. Even if it’sstrong, copying is a low‑cost, defensive choice that avoids surprises if a mutable string were ever passed in.You can make the setter match typical string property behavior like this:
Suggested setter tweak
- (void)setHost:(NSString *)host { - _host = host; -} + (void)setHost:(NSString *)host { + _host = [host copy]; +}Everything else in this change (init defaulting
_hostfromdomainSelector.primaryDomainand usingrealtimeUrlForHost:self.host) looks consistent with the new endpoint/domain model.Also applies to: 69-71, 79-79, 197-197, 252-257
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/integration-test.yamlAbly.xcodeproj/project.pbxprojExamples/Tests/TestsTests/TestsTests.swiftScripts/log-environment-information.shSource/ARTClientOptions.mSource/ARTConnection.mSource/ARTDefault.mSource/ARTDomainSelector.mSource/ARTFallbackHosts.mSource/ARTRealtime.mSource/ARTRest.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/PrivateHeaders/Ably/ARTDefault+Private.hSource/PrivateHeaders/Ably/ARTDomainSelector.hSource/PrivateHeaders/Ably/ARTFallbackHosts.hSource/include/Ably/ARTClientOptions.hSource/include/Ably/ARTDefault.hSource/include/module.modulemapTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/ARTDomainSelectorTests.swiftTest/AblyTests/Tests/PushAdminTests.swiftTest/AblyTests/Tests/RealtimeClientChannelTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Source/PrivateHeaders/Ably/ARTFallbackHosts.h
- Source/ARTFallbackHosts.m
🚧 Files skipped from review as they are similar to previous changes (6)
- Scripts/log-environment-information.sh
- Source/Ably.modulemap
- Source/include/Ably/ARTClientOptions.h
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Source/ARTRealtime.m
- Source/ARTConnection.m
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTDomainSelector.mSource/ARTDefault.mSource/ARTRest.mSource/ARTClientOptions.mSource/ARTWebSocketTransport.m
🧬 Code graph analysis (2)
Test/AblyTests/Tests/RestClientTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (5)
match(1518-1526)getTestEndpoint(628-637)at(1433-1435)at(1442-1447)extract(1528-1539)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (7)
match(1518-1526)at(1433-1435)at(1442-1447)data(936-938)newRealtime(186-200)clientOptions(146-161)extract(1528-1539)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/RestClientTests.swift
[Warning] 181-181: TODOs should be resolved (this check is useless bc publi...)
(todo)
[Warning] 206-206: TODOs should be resolved (this check is useless bc publi...)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
🔇 Additional comments (47)
.github/workflows/integration-test.yaml (1)
32-36: ABLY_ENV value now encodes routing policy; ensure consistency across workflows and scripts
ABLY_ENVis nownonprod:sandbox, which matches the new endpoint/routing-policy naming. Please double‑check:
- Other
integration-test-*.yamlworkflows use the same value where appropriate.Scripts/log-environment-information.shand any other consumers correctly interpretABLY_ENVas an endpoint/routing policy rather than the oldsandboxenvironment string.Examples/Tests/TestsTests/TestsTests.swift (1)
20-22: Endpoint/domain updates in example test look coherent; just confirm default client endpointThe changes here line up with the new non‑prod sandbox routing:
- Provisioning now targets
sandbox.realtime.ably-nonprod.net:443/apps, matching the new non‑prod sandbox domain.ARTClientOptionsusesendpoint = "nonprod:sandbox"instead ofenvironment = "sandbox", which aligns with ADR‑119.- Background reachability pings use
https://ably.com, which is fine for a generic external call.One thing to verify: the background
ARTRealtime(key: key as String)andARTRest(key: key as String)instances (which don’t receive explicit options) should pick up the same non‑prod sandbox endpoint via whatever global/default configuration you’ve introduced. If they still default to production, these example assertions may become flaky or fail.Also applies to: 49-52, 65-78
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (6)
44-56: Domain and fallback host expectations now match the new default routingThe updated expectations around hostnames look consistent with the new domain model:
- Default realtime host assertions now target
main.realtime.ably.net.- Fallback host patterns use
main.[a-e].fallback.ably-realtime.com, which matches the new default fallback set.- Tests that ensure we stay on the primary host in non‑fallback scenarios (e.g. bad request in
test__090__...) correctly assert onlymain.realtime.ably.netis used.These changes appear internally consistent with the new
ARTDomainSelectorbehaviour.Also applies to: 181-195, 4180-4185
2178-2203: Use ofendpointfor connection timeout and reconnect tests looks reasonable
- In
test__058__...connection_attempt_should_fail_if_not_connected_within_the_default_realtime_request_timeout, settingoptions.endpoint = "10.255.255.1"is a good way to force a connection timeout via a non‑routable IP.- In
test__106_b__...restart_the_pending_connection_attempt..., capturinglet primaryDomain = options.domainSelector.primaryDomainand then:
- Setting
options.endpoint = "10.255.255.1"to simulate an unreachable endpoint, and- Using
options.testOptions.reconnectionRealtimeHost = primaryDomain
ensures that the “restart” path goes back to the real primary domain while the initial attempt still fails as intended.Assuming
ARTDomainSelectortreats a bare IPv4 string inendpointas a direct host (not as a routing policy), this setup should exercise the intended code paths without unintended side‑effects.Also applies to: 4947-4953
3843-3890: Deprecation annotations aroundrealtimeHostusage are appropriateThe tests that still depend on
ARTClientOptions.realtimeHostare now explicitly marked@available(*, deprecated, ...), which:
- Prevents compiler warnings without hiding the legacy behaviour you still need to validate.
- Keeps the tests aligned with the intended plan to remove
realtimeHostin a future breaking change.The host/fallback behaviour asserted in these tests is still compatible with the new domain scheme, so this transition strategy looks good.
Also applies to: 3991-4029
4031-4111: Fallback-host tests correctly cover custom arrays and default fallback behaviourThe updated host fallback tests behave as expected under the new domain model:
test__089__...applies_when_an_array_of_ClientOptions_fallbackHosts_is_provided:
- Ensures the sequence hits the primary
main.realtime.ably.netplus each customf–jhost exactly once.test__089b__...applies_when_deprecated_fallbackHostsUseDefault_option_is_set_to_true:
- Uses a custom port to avoid ambiguity with other tests.
- Asserts exactly one primary connection and one fallback connection to
main.[a-e].fallback.ably-realtime.com.Together they give decent coverage of both custom and default fallback modes under the new hostname scheme.
4471-4512: EmptyfallbackHostsbehaviour test matches expected semantics
test__095__Connection__Host_Fallback__won_t_use_fallback_hosts_feature_if_an_empty_array_is_providedasserts that:
- With
fallbackHosts = [], even though the error would normally qualify for fallback use, the client:
- Connects only to the primary host (
main.realtime.ably.net),- Transitions through
.connecting → .disconnected → .connecting, and- Performs exactly two connection attempts, both to the primary host.
This is the right contract for “empty array disables fallback”. The use of
DataGathererandTestProxyTransportFactoryto assert both state transitions and URL targets is well put together.
4835-4867: Reachability tests now correctly bind todomainSelector.primaryDomainAcross the RTN20 tests, the new assertions that:
TestReachability.host == client.internal.options.domainSelector.primaryDomainensure that:
- For both CONNECTING and CONNECTED states, OS‑level reachability monitoring is attached to the same primary domain that the connection logic uses.
- When the client is DISCONNECTED or SUSPENDED because of network loss, the subsequent
reachability.simulate(true)drives reconnection attempts consistently against that primary domain.This neatly ties the reachability behaviour into the new domain selection policy.
Also applies to: 4871-4903, 4931-4937
Test/AblyTests/Tests/PushAdminTests.swift (1)
227-237: Push publish test now targetsdomainSelector.primaryDomain; confirm that’s the intended hostUsing:
"ablyUrl": "https://\(options.domainSelector.primaryDomain)"to build the push
publishObjectmakes this test respect the new endpoint/domainSelector routing rather than hard‑coding a REST/Realtime host, which is good.Please just confirm that, under ADR‑119, the
ablyUrlexpected by the push service is indeed the same host asdomainSelector.primaryDomain(i.e. there isn’t a separate REST‑specific primary domain that should be used here instead).Source/include/module.modulemap (1)
144-144: ExposeARTDomainSelectorvia the Private module (SPM)The
ARTDomainSelector.hheader exists at the expected path and is properly tracked in the project. This addition to the module map correctly exposes the new domain-selection API via SPM.Test/AblyTests/Test Utilities/TestUtilities.swift (4)
111-112: LGTM!Setting
ARTDefault.setMaxMessageSize(16384)in the common test setup ensures consistent message size limits across tests.
121-121: LGTM!The URL construction correctly uses
options.domainSelector.primaryDomaininstead of the legacyrestHost, aligning with the new endpoint-based domain selection architecture.
148-148: LGTM!Correctly updated to use the new
endpointproperty withgetTestEndpoint(), replacing the legacy environment-based configuration.
600-609: This review comment is based on incorrect assumptions.The
environmentparameter was never a query item for JWT token requests to the echo server. Theenvironmentproperty inARTClientOptionsis a deprecated client-level configuration option used to specify which Ably environment to connect to (e.g., "sandbox", "production"), not a parameter for JWT token generation. The/createJWTendpoint athttps://echo.ably.ioaccepts the parameters shown (keyName, keySecret, expiresIn, clientId, capability, jwtType, encrypted) and does not require an environment parameter.Likely an incorrect or invalid review comment.
Test/AblyTests/Tests/ARTDomainSelectorTests.swift (6)
1-21: LGTM!Good test structure with clear organization. The default domain test correctly validates that
nilendpoint producesmain.realtime.ably.net.
23-102: LGTM!Comprehensive coverage of primary domain scenarios including hostnames with dots, IPv6 addresses, localhost, IP addresses, and routing policies. The test cases align well with the REC1 specification requirements.
104-240: LGTM!Thorough fallback domain testing covering custom hosts, default fallbacks, hostname/localhost/IP edge cases (no fallbacks), and routing policy fallback generation for both nonprod and production environments.
242-334: LGTM!Good coverage of legacy options (environment, restHost, realtimeHost) and their interaction with the new domain selector. Tests correctly verify the deprecation path behavior.
360-385: LGTM!Edge case tests for empty string endpoint (falls back to default) and empty fallback array (respects explicit empty configuration) are valuable.
413-439: LGTM!Priority tests correctly verify that
endpointtakes precedence overenvironment, andenvironmenttakes precedence overrestHost. This aligns with the ADR-119 specification for option precedence.Source/ARTRest.m (6)
19-19: LGTM!Import changes correctly add
ARTDefault+Private.handARTDomainSelector.hto support the new domain selection architecture.Also applies to: 50-50
376-382: LGTM!The fallback retry expiration logic correctly resets to
domainSelector.primaryDomaininstead of the legacyrestHost. The RSC15f spec reference is helpful for traceability.
443-446: LGTM!Correctly switches
prioritizedHostto the successful fallback host per RSC15f specification.
449-471: LGTM!Fallback host handling correctly uses
domainSelector.fallbackDomainsinstead of the removedARTFallbackHosts. The RSC15j spec reference for the Host header override is a good documentation practice.
515-521: LGTM!The
currentHostmethod correctly returnsdomainSelector.primaryDomainwhen no prioritized host is set, maintaining backward compatibility while using the new domain selection mechanism.
677-690: LGTM!The connectivity check URL is now configurable via
options.connectivityCheckUrlwith a fallback toARTDefault.connectivityCheckUrl. The REC3a/REC3b spec references document the behavior well.Ably.xcodeproj/project.pbxproj (1)
360-365: ARTDomainSelector is correctly wired into the project for all platforms
ARTDomainSelector.m/ARTDomainSelector.hhave consistentPBXFileReferenceentries (Objective‑C source + Private header underPrivateHeaders/Ably), and are placed in theUtilitiesgroup alongside similar internal helpers.- Three
PBXBuildFileentries forARTDomainSelector.mare correctly hooked into the iOS, macOS, and tvOSSourcesbuild phases; likewise, threePBXBuildFileentries forARTDomainSelector.hare present in the correspondingHeadersphases, all markedATTRIBUTES = (Private, );.- This matches the existing pattern for internal/private utility types and ensures the new selector is compiled and available across all Ably framework targets without leaking into the public header surface.
No issues spotted in this wiring; it looks ready to ship.
Also applies to: 1236-1237, 2013-2014, 2163-2163, 2342-2342, 2554-2554, 2995-2995, 3140-3140, 3285-3285
Source/include/Ably/ARTDefault.h (1)
10-19: LGTM! Public API surface is clean and consistent.The class methods for
restHost,realtimeHost, andfallbackHostscorrectly expose the new domain-selector-based defaults. The signature changes from instance to class methods align with the static nature of these defaults.Source/PrivateHeaders/Ably/ARTDefault+Private.h (1)
3-9: LGTM! Private interface is appropriately scoped.The private methods for setting connection state TTL, max message size, and accessing the connectivity check URL are correctly exposed for internal SDK use and testing.
Source/ARTDefault.m (2)
23-30: Verify: new ARTDomainSelector allocation on each call.Each call to
fallbackHostsWithEnvironment:allocates a newARTDomainSelectorinstance. While correct, this could be a minor performance consideration if called frequently. The same pattern applies torestHostandrealtimeHost. This is likely acceptable given typical usage patterns, but worth noting.
36-42: Confirm: restHost and realtimeHost returning identical values is intentional.Both methods return
primaryDomainfrom a default-initializedARTDomainSelector, yielding the same value (main.realtime.ably.net). Per ADR-119, this aligns with the new unified endpoint model. Please confirm this is the intended behavior.Test/AblyTests/Tests/RestClientTests.swift (5)
45-47: LGTM! Fallback host patterns correctly updated.The test assertions now verify the new domain format
main.[a-e].fallback.ably-realtime.comwhich aligns with ADR-119 specifications.
316-330: Good coverage of REC1b1: endpoint + deprecated options conflict.This test correctly verifies that setting
endpointtogether with deprecated options (environment,restHost,realtimeHost) throws an exception, enforcing the mutual exclusivity requirement from ADR-119.
333-351: Comprehensive REC1b2-REC1b4 endpoint parsing tests.The tests correctly verify:
- REC1b2: FQDN endpoint → primaryDomain equals the endpoint value
- REC1b3:
nonprod:sandbox→sandbox.realtime.ably-nonprod.net- REC1b4: Production routing policy →
test.realtime.ably.net
1186-1223: Excellent REC2c fallback domain tests.These tests thoroughly verify fallback domain generation for:
- REC2c1: Default →
main.[a-e].fallback.ably-realtime.com- REC2c2: Explicit hostname → empty fallbacks
- REC2c3: Nonprod policy →
sandbox.[a-e].fallback.ably-realtime-nonprod.com- REC2c4: Production policy →
sandbox.[a-e].fallback.ably-realtime.com
181-181: Tracked: TODO comments for ineffective test assertions.These TODO comments flag assertions that check
publishTask.errorimmediately when it's nil. This is tracked in issue #2145 per prior review discussion.Also applies to: 206-206
Source/PrivateHeaders/Ably/ARTDomainSelector.h (1)
1-45: LGTM! Well-designed domain selector interface.The
ARTDomainSelectorclass provides a clean abstraction for REC1/REC2 domain selection logic. The designated initializer accepts all relevant client options, and the documentation clearly explains each parameter's purpose, including noting which options are deprecated.Source/ARTDomainSelector.m (2)
133-153: LGTM! Primary domain construction is correct.The switch statement correctly maps each policy to its domain format:
- Default:
main.realtime.ably.net- Hostname/LegacyHost: uses provided hostname directly
- NonProductionRoutingPolicy:
{policyId}.realtime.ably-nonprod.net- ProductionRoutingPolicy/LegacyEnvironment:
{policyId}.realtime.ably.net
155-199: LGTM! Fallback domain logic correctly implements REC2.The implementation properly handles:
- REC2a2: Explicit
fallbackHostsClientOptiontakes precedence- REC2b: Deprecated
fallbackHostsUseDefaulttriggers default fallbacks- REC2c1: Default policy uses default fallback domains
- REC2c2/c6: Hostname/LegacyHost returns empty array
- REC2c3: NonProd uses
ably-realtime-nonprod.comTLD- REC2c4/c5: Production/LegacyEnvironment uses
ably-realtime.comTLDSource/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
16-24: LGTM! Clean private interface for domain selection.The
domainSelectorproperty correctly exposes the domain selector for internal use, andsetDefaultEndpoint:provides test infrastructure to override the default endpoint. TherealtimeUrlForHost:method supports fallback host URL construction.Source/ARTClientOptions.m (7)
31-37: Good encapsulation with private ivars for domain-driven configuration.Using private ivars for
_endpoint,_restHost,_realtimeHost,_environment, and_domainSelectorallows the computed getters (restHost,realtimeHost) to fall back todomainSelector.primaryDomainwhile still distinguishing between "not explicitly set" (nil) and "explicitly set". This supports the ADR-119 requirement to preserve legacy behavior when legacy options are explicitly configured.
104-117: LGTM - Clean lazy initialization with proper deprecation handling.The lazy initialization of
ARTDomainSelectorcorrectly passes all relevant configuration options and uses pragma directives to suppress warnings for deprecated option access. The selector is appropriately invalidated (set tonil) in each setter that affects domain resolution.
129-190: Well-structured conflict validation and computed getters.The setters correctly implement ADR-119 requirements:
- REC1b1: Endpoint cannot combine with environment/restHost/realtimeHost
- REC1c1: Environment cannot combine with restHost/realtimeHost
The getters (
restHost,realtimeHost) providing fallback todomainSelector.primaryDomainensure backward compatibility—callers get a valid host whether using the new endpoint model or legacy options.
192-214: LGTM - Consistent URL construction using primaryDomain.The URL methods correctly delegate to
self.primaryDomainfor host resolution, which flows through the new domain selector. TherealtimeUrlForHost:method appropriately accepts an explicit host parameter to support fallback scenarios.
216-264: Correct handling of endpoint and conditional legacy option copying.The implementation correctly:
- Bypasses
setEndpoint:via direct ivar access (line 220) to avoid conflict checks during copy- Conditionally copies legacy options only when explicitly set (lines 244-246), preserving the distinction between "not configured" (
nil) and "explicitly configured"This addresses the past review concern about
copyWithZone:throwing for legacy-only configurations.
275-299: LGTM - Correct ivar checks and domain selector invalidation.The
hasCustomRestHostandhasCustomRealtimeHostmethods correctly check the backing ivars to determine explicit configuration, distinct from the computed getters that fall back todomainSelector. The fallback setters appropriately invalidate_domainSelectorwhen configuration changes.
301-303: LGTM - Class-level default endpoint configuration.This follows the SDK's existing pattern for configurable defaults (like
+setDefaultEnvironment:which this replaces per the AI summary). The globalARTDefaultEndpointis read duringinitDefaults(line 47).
| NSTimeInterval ARTConnectionStateTtl = 60.0; | ||
| NSInteger ARTMaxMessageSize = 65536; // 16384 in sandbox |
There was a problem hiding this comment.
Thread-safety: reads of mutable globals lack synchronization.
ARTConnectionStateTtl and ARTMaxMessageSize are written under @synchronized(self) in the setters (lines 68-77), but the corresponding getters (connectionStateTtl at line 57 and maxMessageSize at line 65) read these values without synchronization. This creates a data race if one thread reads while another writes.
🔎 Proposed fix to add synchronization to getters
+ (NSTimeInterval)connectionStateTtl {
- return ARTConnectionStateTtl;
+ @synchronized (self) {
+ return ARTConnectionStateTtl;
+ }
} + (NSInteger)maxMessageSize {
- return ARTMaxMessageSize;
+ @synchronized (self) {
+ return ARTMaxMessageSize;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Source/ARTDefault.m near lines 8-9 (and the getters at lines ~57 and ~65),
reads of ARTConnectionStateTtl and ARTMaxMessageSize occur without
synchronization while their setters use @synchronized(self), causing a potential
data race; fix by wrapping the getter bodies in @synchronized(self) (use the
same lock as the setters) so getters return the values under the same mutex,
preserving thread-safety and consistency.
Closes #2098
Summary by CodeRabbit
New Features
Deprecations
Removals
Bug Fixes / Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.