Use endpoint as default connection option (ADR-119) #590
Conversation
WalkthroughThe pull request modifies the host configuration logic in the Ably SDK. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Options
participant Defaults
Client->>Options: Instantiate with endpoint (or environment)
Options->>Options: Check mutual exclusivity (raise error if both provided)
Options->>Defaults: Call get_hostname(endpoint)
Defaults-->>Options: Return primary hostname
Options->>Defaults: Call get_fallback_hosts(endpoint)
Defaults-->>Options: Return fallback hosts
Options-->>Client: Return configured Options instance
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/590/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
ably/transport/defaults.py (2)
47-57: Refine hostname derivation logic
These conditionals cover typical cases (FQDN, localhost, nonprod, main), but consider additional validations for custom endpoints or IPv6 addresses (beyond a simple::check).
60-69: Evaluate fallback logic for different endpoint patterns
The fallback domain logic is straightforward fornonprod:andmain, but consider whether the same approach is needed for other future or custom endpoint patterns.Also applies to: 72-76
test/ably/rest/restinit_test.py (2)
79-79: Fix Yoda conditions in assertions.The assertions use Yoda conditions which are not recommended in Python. Let's make them more readable.
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host - assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % hostAlso applies to: 84-84
🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
185-185: Fix line length and Yoda conditions in assertions.The lines exceed the maximum line length of 115 characters and use Yoda conditions.
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host) + expected_url = 'https://main.realtime.ably.net' + actual_url = '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host) + assert actual_url == expected_url - assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host) + expected_url = 'http://main.realtime.ably.net' + actual_url = '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host) + assert actual_url == expected_urlAlso applies to: 191-191
🧰 Tools
🪛 Ruff (0.8.2)
185-185: Yoda condition detected
(SIM300)
🪛 GitHub Actions: Linting check
[error] 185-185: E501 line too long (123 > 115 characters)
ably/types/options.py (2)
12-13: Fix line length in constructor parameters.The line exceeds the maximum line length of 115 characters.
- tls_port=0, use_binary_protocol=True, queue_messages=False, recover=False, endpoint=None, - environment=None, http_open_timeout=None, http_request_timeout=None, realtime_request_timeout=None, + tls_port=0, use_binary_protocol=True, queue_messages=False, + recover=False, endpoint=None, environment=None, + http_open_timeout=None, http_request_timeout=None, + realtime_request_timeout=None,🧰 Tools
🪛 GitHub Actions: Linting check
[error] 13-13: E501 line too long (116 > 115 characters)
286-290: Consider adding a docstring for the fallback hosts logic.The fallback hosts logic has been updated to use endpoint-based configuration, but the logic could benefit from documentation explaining the behavior.
fallback_hosts = self.fallback_hosts if fallback_hosts is None: + # If rest_host is explicitly set, don't use fallback hosts + # Otherwise, get fallback hosts based on the endpoint if self.rest_host: fallback_hosts = [] else: fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restrequest_test.py(1 hunks)test/ably/testapp.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/ably/rest/restrequest_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-185: Yoda condition detected
(SIM300)
191-191: Yoda condition detected
(SIM300)
🪛 GitHub Actions: Linting check
test/ably/rest/restinit_test.py
[error] 185-185: E501 line too long (123 > 115 characters)
[error] 191-191: E501 line too long (122 > 115 characters)
ably/types/options.py
[error] 13-13: E501 line too long (116 > 115 characters)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (9)
ably/transport/defaults.py (1)
5-5: Set default endpoint to 'main'
This aligns with ADR-119.test/ably/rest/restpaginatedresult_test.py (4)
34-34: Update base_url to new default domain
Replacingrest.ably.iowithmain.realtime.ably.netis consistent with the new endpoint approach.
47-48: Update Link header references
Ensures the HTTP link headers match the new domain.
58-58: Use new domain in PaginatedResult query
Matches the endpoint logic in defaults and test configurations.
62-62: Adopt the updated domain for channel queries
Keeps consistency with the rest of the codebase changes.test/ably/testapp.py (3)
17-18: Adopt nonprod host defaults
Replaces sandbox host references to point tosandbox.realtime.ably-nonprod.net, aligning test configuration with ADR-119 changes.
20-20: Switch environment default to 'nonprod:sandbox'
Reflects the new host naming convention and test alignment.
25-25: Verify conditional logic for custom rest_host values
Whenrest_hostis not withinrealtime.ably-nonprod.net, the test harness forces ports to 8080/8081. Confirm this remains valid for future endpoint expansions.ably/types/options.py (1)
45-57: LGTM! Good implementation of endpoint configuration.The implementation correctly:
- Enforces mutual exclusivity between endpoint and environment
- Migrates environment value to endpoint if environment is provided
- Sets default endpoint from Defaults class
surminus
force-pushed
the
laura/endpoint
branch
from
3146e69 to
3c1a33d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/590/features
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/ably/rest/restinit_test.py (1)
184-221: 🛠️ Refactor suggestionFix Yoda conditions and update environment to endpoint.
Update the test to use endpoint instead of environment and fix Yoda conditions.
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'https://main.realtime.ably.net' - assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'http://main.realtime.ably.net' - ably = AblyRest(token='token', environment='custom') + ably = AblyRest(token='token', endpoint='custom')🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
207-207: Yoda condition detected
Rewrite as
excinfo.value.status_code == 401(SIM300)
208-208: Yoda condition detected
Rewrite as
excinfo.value.code == 40103(SIM300)
209-209: Yoda condition detected
(SIM300)
215-218: Use
contextlib.suppress(AblyException)instead oftry-except-passReplace with
contextlib.suppress(AblyException)(SIM105)
🧹 Nitpick comments (3)
ably/transport/defaults.py (1)
60-77: Consider adding docstring to document the fallback host pattern.The method correctly implements the fallback host generation logic but would benefit from documentation explaining the pattern used for both production and non-production environments.
@staticmethod def get_fallback_hosts(endpoint="main"): + """Generate a list of fallback hosts based on the endpoint. + + Args: + endpoint (str, optional): The endpoint to generate fallback hosts for. Defaults to "main". + Can be a routing policy name (e.g., "main"), a non-production routing policy name + (e.g., "nonprod:sandbox"), or a custom endpoint. + + Returns: + list[str]: A list of fallback hostnames following the pattern: + - For nonprod: <root>.{a-e}.fallback.ably-realtime-nonprod.com + - For others: <endpoint>.{a-e}.fallback.ably-realtime.com + """ably/types/options.py (1)
173-174: Consider adding setter for endpoint property.While the current implementation treats endpoint as read-only, consider adding a setter to allow runtime modifications, similar to other properties in the class.
@property def endpoint(self): return self.__endpoint + +@endpoint.setter +def endpoint(self, value): + self.__endpoint = value + self.__rest_hosts = self.__get_rest_hosts() + self.__realtime_hosts = self.__get_realtime_hosts()test/ably/rest/restinit_test.py (1)
79-80: Fix Yoda condition.Rewrite the condition to improve readability.
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restrequest_test.py(1 hunks)test/ably/testapp.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/ably/rest/restpaginatedresult_test.py
- test/ably/rest/restrequest_test.py
- test/ably/testapp.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (6)
ably/transport/defaults.py (2)
5-5: LGTM!The default endpoint value 'main' aligns with ADR-119 specifications.
47-58: LGTM! Well-structured hostname resolution logic.The
get_hostnamemethod correctly handles all endpoint formats specified in ADR-119:
- Fully qualified domain names (contains ".")
- IPv6 addresses (contains "::")
- Localhost
- Non-production routing policy (starts with "nonprod:")
- Production routing policy ("main")
- Custom routing policy (fallback case)
ably/types/options.py (4)
12-18: LGTM! Constructor signature updated correctly.The
endpointparameter has been added while maintaining backward compatibility withenvironment.
46-58: LGTM! Robust validation and fallback logic.The code correctly:
- Prevents simultaneous use of endpoint and environment
- Falls back to environment value if endpoint is not provided
- Uses default endpoint from Defaults class as final fallback
277-291: LGTM! Host resolution logic updated correctly.The code properly handles:
- Custom rest_host override
- Endpoint-based hostname resolution
- Fallback host generation based on endpoint
307-308: LGTM! Realtime host resolution updated.The code correctly uses the new endpoint-based hostname resolution for realtime hosts.
surminus
force-pushed
the
laura/endpoint
branch
from
3c1a33d to
d9706dd
Compare
github-actions
bot
temporarily deployed
to
staging/pull/590/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ably/types/options.py (1)
176-177: Add endpoint property documentation.Consider adding docstring to the endpoint property to describe its purpose and valid values.
@property def endpoint(self): + """ + The endpoint used for host resolution. Can be: + - A routing policy name (e.g., "main") + - A non-production routing policy (e.g., "nonprod:sandbox") + - A fully qualified domain name (e.g., "foo.example.com") + """ return self.__endpointtest/ably/rest/restinit_test.py (3)
79-79: Fix Yoda conditions in assertions.Update the assertions to follow Python's conventional comparison order.
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host - assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % hostAlso applies to: 84-84
🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
106-108: Update environment to endpoint in test.For consistency with ADR-119, update the test to use endpoint instead of environment.
- ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10) + ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10) assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted( ably.options.get_fallback_rest_hosts())
185-187: Fix Yoda conditions in URL assertions.Update the URL assertions to follow Python's conventional comparison order.
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'https://main.realtime.ably.net' - assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'http://main.realtime.ably.net'Also applies to: 194-196
🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restrequest_test.py(1 hunks)test/ably/testapp.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/ably/rest/restrequest_test.py
- test/ably/rest/restpaginatedresult_test.py
- test/ably/testapp.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (7)
ably/transport/defaults.py (3)
5-5: LGTM! Endpoint configuration aligns with ADR-119.The default endpoint is set to 'main', which is the correct default for production environments.
47-58: LGTM! Hostname resolution logic is well-structured.The
get_hostnamemethod correctly handles all endpoint formats specified in ADR-119:
- Fully qualified domain names (contains ".")
- IPv6 addresses (contains "::")
- Localhost
- Non-production routing policy (starts with "nonprod:")
- Production routing policy ("main")
- Custom endpoints
60-77: LGTM! Fallback host generation is comprehensive.The
get_fallback_hostsmethod correctly generates fallback hosts based on the endpoint type:
- Non-production endpoints use the ably-realtime-nonprod.com domain
- Production endpoints use the ably-realtime.com domain
ably/types/options.py (4)
12-18: LGTM! Constructor parameters updated correctly.The endpoint parameter has been added while maintaining backward compatibility with the environment parameter.
46-48: LGTM! Mutual exclusivity check added.Correctly enforces mutual exclusivity between endpoint and environment parameters as specified in ADR-119.
53-61: LGTM! Environment to endpoint transition logic is correct.The code properly handles the transition from environment to endpoint:
- Maps "production" environment to default endpoint
- Uses environment value as endpoint if endpoint is not specified
- Falls back to default endpoint if neither is specified
280-281: LGTM! Host resolution logic updated correctly.The code properly uses the new endpoint-based host resolution while maintaining backward compatibility with rest_host.
Also applies to: 289-294
There was a problem hiding this comment.
Added some comments about implementation details.
Some other things:
Given the impact of this changes, I would also like to see some endpoint tests (could be simple unit tests) to check that providing a new endpoint option results in the expected rest host, realtime host and fallback hosts used.
- can check that providing incomatible options (e.g.
endpointwithenvironment) results in the error.
I can also see that the ably-go PR deprecated the restHost, realtimeHost and environment options: https://github.com/ably/ably-go/pull/679/files#diff-721ab670921034c0c26f7999ad630004f73001cb85be84c768e7bf93c6abf14fR256-R280.
Should we do the same here, or can create a separate ticket for it? Don't know if there was an explicit decision to deprecate those.
test/ably/rest/resthttp_test.py::TestRestHttp::test_request_headers - test consistently fails after changes, should investigate
|
Great, thank you for the review.
I will add these tests.
I can do that as part of this PR, that's fine. I'll also need to update my ably-js and ably-ruby PRs. |
|
Just a heads up:
This is not related to this PR, fixed in #593. You can ignore |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/ably/rest/restinit_test.py (2)
106-112: 🛠️ Refactor suggestionUpdate environment to endpoint in fallback hosts test.
For consistency with ADR-119, update the test to use endpoint instead of environment.
- ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10) + ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10) assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted( ably.options.get_fallback_rest_hosts())
76-84: 🛠️ Refactor suggestionFix Yoda conditions and update environment to endpoint.
The tests use environment parameters but should be updated to use endpoint parameters for consistency with ADR-119. Also, fix the Yoda conditions flagged by the static analyzer.
# environment: main - ably = AblyRest(token='foo', environment="main") + ably = AblyRest(token='foo', endpoint="main") host = ably.options.get_rest_host() - assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host # environment: other - ably = AblyRest(token='foo', environment="nonprod:sandbox") + ably = AblyRest(token='foo', endpoint="nonprod:sandbox") host = ably.options.get_rest_host() - assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
84-84: Yoda condition detected
Rewrite as
host == "sandbox.realtime.ably-nonprod.net"(SIM300)
🧹 Nitpick comments (5)
ably/realtime/realtime.py (1)
51-59: Documentation improvements needed.The endpoint parameter is structured correctly, but the deprecated parameters documentation could be improved for better readability and consistency with the rest.py file.
endpoint: str Endpoint specifies either a routing policy name or fully qualified domain name to connect to Ably. realtime_host: str - Deprecated: this property is deprecated and will be removed in a future version. - Enables a non-default Ably host to be specified for realtime connections. - For development environments only. The default value is realtime.ably.io. + Deprecated: this property is deprecated and will be removed in a future version. Enables a non-default + Ably host to be specified for realtime connections. For development environments only. + The default value is realtime.ably.io. environment: str - Deprecated: this property is deprecated and will be removed in a future version. - Enables a custom environment to be used with the Ably service. Defaults to `production` + Deprecated: this property is deprecated and will be removed in a future version. Enables a custom + environment to be used with the Ably service. Defaults to `production`test/ably/rest/restinit_test.py (2)
185-187: Fix Yoda condition in HTTPS assertion.- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( - ably.http.preferred_scheme, ably.http.preferred_host - ) + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'https://main.realtime.ably.net'🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
194-196: Fix Yoda condition in HTTP assertion.- assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( - ably.http.preferred_scheme, ably.http.preferred_host - ) + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'http://main.realtime.ably.net'🧰 Tools
🪛 Ruff (0.8.2)
194-196: Yoda condition detected
(SIM300)
ably/types/options.py (2)
287-290: Condition should check explicitly if rest_host is None.Using
if self.rest_host is not None:is not as clear as an explicit comparison to None. This pattern is also inconsistent with other checks in the codebase.- if self.rest_host is not None: + if self.rest_host is not None: fallback_hosts = [] else: fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)
173-174: Consider adding an endpoint.setter method.All other properties in this class have both getter and setter methods. For consistency, it would be good to add a setter for the endpoint property as well, even if it's not currently needed.
@property def endpoint(self): return self.__endpoint +@endpoint.setter +def endpoint(self, value): + self.__endpoint = value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
ably/realtime/realtime.py(1 hunks)ably/rest/rest.py(1 hunks)ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linting check
ably/rest/rest.py
[error] 36-38: E501 line too long (122 > 115 characters)
[error] 37-37: E501 line too long (154 > 115 characters)
[error] 38-38: E501 line too long (156 > 115 characters)
ably/types/options.py
[error] 46-46: E501 line too long (116 > 115 characters)
[error] 47-47: E117 over-indented
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (5)
ably/transport/defaults.py (1)
3-5: Implementation looks good.The changes to replace environment with endpoint and set the default to 'main' align with ADR-119 requirements.
ably/types/options.py (4)
12-14: The new endpoint parameter implementation looks good.Adding the
endpointparameter and maintaining backward compatibility with theenvironmentparameter aligns with the objectives of ADR-119.
278-278:✅ Verification successful
Verify that the Defaults.get_hostname and Defaults.get_fallback_hosts methods exist.
The code now calls two new methods from the Defaults class:
get_hostnameandget_fallback_hosts. Let's verify that these methods exist and work as expected.Also applies to: 290-290, 307-307
🏁 Script executed:
#!/bin/bash # Check for the implementation of get_hostname and get_fallback_hosts in Defaults rg -A 10 "def get_(hostname|fallback_hosts)" --type py ably/transport/defaults.pyLength of output: 923
Verification Complete: Defaults Methods Implementation Verified
Both
Defaults.get_hostnameandDefaults.get_fallback_hostshave been confirmed to exist and are correctly implemented inably/transport/defaults.py. The usage inably/types/options.py(lines 278, 290, and 307) is valid with no issues detected.
46-47:❓ Verification inconclusive
Verify that all legacy options are properly validated against endpoint.
The current validation checks for incompatibility with
environment,rest_host, andrealtime_host, but based on previous comments, we should also check for compatibility withfallback_hosts_use_defaultas implemented in ably-go.
🏁 Script executed:
#!/bin/bash # Check if fallback_hosts_use_default is mentioned in the PR or the defaults.py file rg -i "fallback_hosts_use_default" --type pyLength of output: 44
Action Required: Validate legacy option "fallback_hosts_use_default" against endpoint
It appears that while the validation in
ably/types/options.pycurrently checks for incompatibility betweenendpointand the optionsenvironment,rest_host, andrealtime_host, there is no check forfallback_hosts_use_default. Our previous shell search did not reveal any references tofallback_hosts_use_defaultin the repository. Please verify manually whether this legacy option should also be validated (as in ably-go) and, if so, update the logic accordingly. For example, you might consider modifying the conditional to:if endpoint is not None and (environment is not None or rest_host is not None or realtime_host is not None or fallback_hosts_use_default is not None): raise ValueError('endpoint is incompatible with any of environment, rest_host, realtime_host, or fallback_hosts_use_default')Please double-check the correct parameter name and type before making changes.
🧰 Tools
🪛 GitHub Actions: Linting check
[error] 46-46: E501 line too long (116 > 115 characters)
[error] 47-47: E117 over-indented
53-57:❓ Verification inconclusive
Consider adding special handling for 'production' environment.
There appears to be a discrepancy between the ably-go implementation and this one regarding how the 'production' environment is handled. In a previous review comment, it was noted that ably-go has special handling for the 'production' environment, but this implementation doesn't seem to have it.
🏁 Script executed:
#!/bin/bash # Check how 'production' is handled in the Defaults class and any other related files rg -A 5 -B 5 "production" --type py ably/transport/defaults.pyLength of output: 62
Action: Verify and Align 'production' Environment Handling
In the current implementation in
ably/types/options.py(lines 53–57), if anenvironmentis provided without an explicitendpoint, the code directly assignsendpoint = environment—even when the value is"production". This contrasts with the ably-go implementation, which applies special handling for the production environment.
- A search in
ably/transport/defaults.pyfor the term"production"did not reveal any special-case logic.- Please verify whether the Python client should also implement explicit handling for a
"production"environment (for instance, mapping it to a specific default production URL) to mirror the ably-go behavior, or if the current behavior is intentional and the documentation should be updated accordingly.
This implements ADR-119[1], which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042[2]. The endpoint may be one of the following: * a routing policy name (such as main) * a nonprod routing policy name (such as nonprod:sandbox) * a FQDN such as foo.example.com The endpoint option is not valid with any of environment, restHost or realtimeHost, but we still intend to support the legacy options. If the client has been configured to use any of these legacy options, then they should continue to work in the same way, using the same primary and fallback hostnames. If the client has not been explicitly configured, then the hostnames will change to the new ably.net domain when the package is upgraded. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3428810778/ADR-119+ClientOptions+for+new+DNS+structure [2] https://ably.atlassian.net/wiki/spaces/ENG/pages/1791754276/ADR-042+DNS+Restructure
surminus
force-pushed
the
laura/endpoint
branch
from
b04697e to
485ce5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/ably/rest/restinit_test.py (3)
79-79: Fix Yoda conditionThe assertion is using a Yoda condition (placing the literal first in a comparison).
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
82-84: Properly updated environment to nonprod:sandbox with Yoda conditionThe test has been correctly updated to use the new "nonprod:sandbox" environment, which aligns with the hostname changes mentioned in the PR objectives.
However, there's another Yoda condition that should be fixed:
- assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
84-84: Yoda condition detected
Rewrite as
host == "sandbox.realtime.ably-nonprod.net"(SIM300)
185-196: Fix Yoda conditions in HTTPS/HTTP testsBoth the HTTPS and HTTP tests use Yoda conditions.
For HTTPS test:
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( - ably.http.preferred_scheme, ably.http.preferred_host - ) + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'https://main.realtime.ably.net'For HTTP test:
- assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( - ably.http.preferred_scheme, ably.http.preferred_host - ) + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'http://main.realtime.ably.net'🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
ably/types/options.py (1)
46-48: Validation for endpoint vs. environment is correct but could be simplifiedThe validation is correct but could be simplified by using a single if statement when checking for incompatible options.
- if environment is not None and endpoint is not None: - raise ValueError('specify endpoint or environment, not both') + if endpoint is not None and (environment is not None or rest_host is not None or realtime_host is not None): + raise ValueError('endpoint is incompatible with any of environment, rest_host or realtime_host')This approach would also align with the ADR-119 requirements by checking that endpoint cannot be used with any of the legacy options (environment, restHost, or realtimeHost).
🧰 Tools
🪛 Ruff (0.8.2)
46-47: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (11)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restrequest_test.py(1 hunks)test/ably/testapp.py(1 hunks)ably/realtime/realtime.py(1 hunks)ably/rest/rest.py(1 hunks)ably/transport/defaults.py(1 hunks)ably/types/options.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- test/ably/rest/restrequest_test.py
- test/ably/rest/restpaginatedresult_test.py
- test/ably/testapp.py
- ably/rest/rest.py
- ably/types/options.py
- test/ably/rest/restinit_test.py
- ably/transport/defaults.py
🧰 Additional context used
🪛 Ruff (0.8.2)
ably/types/options.py
46-47: Use a single if statement instead of nested if statements
(SIM102)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (6)
ably/realtime/realtime.py (1)
51-59: New endpoint parameter correctly implemented with deprecation noticesThe addition of the
endpointparameter is well-implemented, with clear documentation about its purpose. Good job marking therealtime_hostandenvironmentparameters as deprecated with clear notifications that they will be removed in a future version. This aligns perfectly with the ADR-119 requirements mentioned in the PR objectives.ably/transport/defaults.py (2)
47-57: Well-structured hostname resolution logicThe
get_hostnamemethod properly handles various endpoint formats as specified in ADR-119:
- FQDNs (containing "." or "::" or "localhost")
- Non-production routing policies (starting with "nonprod:")
- Default "main" endpoint
- Other endpoints
The implementation is clean and follows the requirements correctly.
60-77: Verify REC2c2 fallback behavior implementationWhile the fallback host generation is implemented correctly, the code appears to be missing the full REC2c2-compliant fallback behavior. According to the specification, besides generating fallback host addresses, the SDK should also implement a robust retry mechanism.
Please verify that the REC2c2 implementation includes:
- Logic to iterate through fallback hosts when the primary host is unreachable
- A timeout mechanism to restrict fallback retries
- Correct toggling between production and non-production endpoint formats
Reviewing the ably-go implementation referenced in previous comments would help ensure full compliance with REC2c2.
test/ably/rest/restinit_test.py (1)
106-112: Consider updating environment to endpointThe test is still using
environmentparameter instead of the newendpointparameter. For consistency with ADR-119, it would be better to update the test to use the endpoint option.- ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10) + ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10) assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted( ably.options.get_fallback_rest_hosts())ably/types/options.py (2)
53-60: Good logic for handling endpoint defaultsThe code correctly:
- Maps "production" environment to the default endpoint
- Falls back to using environment as endpoint if no endpoint is provided
- Uses the default endpoint when neither is provided
This implementation handles backward compatibility while encouraging the use of the new endpoint parameter.
290-294: Proper fallback hosts selection based on rest_host and endpointThe logic correctly determines whether to use custom fallback hosts based on whether a custom rest_host is provided. If not, it uses the fallback hosts based on the endpoint.
surminus
force-pushed
the
laura/endpoint
branch
from
485ce5b to
7e99ed3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ably/transport/defaults.py (1)
47-58:⚠️ Potential issueFQDN handling in get_hostname needs REC2c2 implementation.
The hostname resolution logic is well-structured for different endpoint formats, but appears to be missing implementation of REC2c2 requirements for fallback behavior. This was also highlighted in previous reviews.
#!/bin/bash # Description: Check the Ably specification for REC2c2 requirements # Lookup ably-go implementation for comparison rg -A 10 "REC2c2" --glob "*.py" # Check if there's any retry/fallback logic implemented rg -A 5 "fallback_retry_timeout" --glob "*.py"
🧹 Nitpick comments (3)
test/ably/rest/restinit_test.py (2)
79-79: Fix Yoda condition and update hostname assertion.The hostname assertion has been correctly updated to reflect the new main.realtime.ably.net domain, but uses a non-preferred Yoda condition format.
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
185-187: Fix Yoda conditions in URL assertions.The URL assertions have been correctly updated to reflect the new main.realtime.ably.net domain, but use non-preferred Yoda condition format.
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'https://main.realtime.ably.net' - assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( + ably.http.preferred_scheme, ably.http.preferred_host + ) == 'http://main.realtime.ably.net'Also applies to: 194-196
🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
ably/types/options.py (1)
46-48: Redundant validation check.This validation check is redundant as it's already covered by the broader check on lines 22-24.
- if environment is not None and endpoint is not None: - raise ValueError('specify endpoint or environment, not both') -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)ably/realtime/realtime.py(1 hunks)ably/rest/rest.py(1 hunks)ably/transport/defaults.py(1 hunks)ably/types/options.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ably/rest/rest.py
- test/ably/rest/restinit_test.py
- ably/types/options.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (12)
ably/realtime/realtime.py (2)
51-53: Good documentation for the new endpoint parameter.The documentation clearly explains the purpose of the endpoint parameter and its valid formats (routing policy name or FQDN) as specified in ADR-119.
54-59: Effective deprecation notices for legacy parameters.Properly marking realtime_host and environment as deprecated with clear messaging that they'll be removed in future versions aligns with the PR objectives of transitioning to endpoint-based configuration.
ably/transport/defaults.py (2)
5-5: Appropriate default endpoint value.Setting the default endpoint to 'main' aligns with ADR-119 and ensures backward compatibility when upgrading the package.
60-77: Well-structured fallback hosts generation.The implementation correctly generates appropriate fallback hosts based on the endpoint type (production vs. non-production) as required in ADR-119.
test/ably/rest/restinit_test.py (3)
82-84: Update environment to endpoint and fix Yoda condition.The test correctly uses the new nonprod:sandbox format, but should be updated to use the endpoint parameter instead of environment for consistency with ADR-119. Also fix the Yoda condition.
- ably = AblyRest(token='foo', environment="nonprod:sandbox") + ably = AblyRest(token='foo', endpoint="nonprod:sandbox") host = ably.options.get_rest_host() - assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
84-84: Yoda condition detected
Rewrite as
host == "sandbox.realtime.ably-nonprod.net"(SIM300)
106-112: Update environment to endpoint in fallback hosts test.For consistency with ADR-119, update the test to use endpoint instead of environment.
- ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10) + ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10) assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted( ably.options.get_fallback_rest_hosts())
220-220: Updated endpoint URL for environment test.The test correctly uses the new domain format for custom environments.
ably/types/options.py (5)
12-13: Added endpoint parameter to constructor.The endpoint parameter has been properly added to the Options constructor, implementing the changes required by ADR-119.
22-24: Good validation for mutually exclusive parameters.The validation ensures endpoint cannot be used with legacy options as required by ADR-119.
🧰 Tools
🪛 Ruff (0.8.2)
22-23: Use a single
ifstatement instead of nestedifstatements(SIM102)
53-61: Logical endpoint assignment flow.The endpoint assignment logic correctly handles all specified cases and defaults, ensuring backward compatibility while implementing the new endpoint-based approach.
176-177: Transition from environment to endpoint property.Good replacement of the environment property with endpoint to reflect the architectural change.
289-294: Improved fallback hosts logic.The code correctly determines whether to use custom fallback hosts or generate them based on the endpoint.
surminus
force-pushed
the
laura/endpoint
branch
from
7e99ed3 to
e4b87b4
Compare
surminus
force-pushed
the
laura/endpoint
branch
from
e4b87b4 to
9bd9857
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/ably/rest/restinit_test.py (3)
79-79: Fix Yoda condition.Rewrite the condition to follow standard Python style.
- assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host + assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
79-79: Yoda condition detected
Rewrite as
host == "main.realtime.ably.net"(SIM300)
185-187: Fix Yoda condition in HTTPS URL assertion.Rewrite condition to follow standard Python style.
- assert 'https://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'https://main.realtime.ably.net'🧰 Tools
🪛 Ruff (0.8.2)
185-187: Yoda condition detected
(SIM300)
194-196: Fix Yoda condition in HTTP URL assertion.Rewrite condition to follow standard Python style.
- assert 'http://main.realtime.ably.net' == '{0}://{1}'.format( + assert '{0}://{1}'.format( ably.http.preferred_scheme, ably.http.preferred_host - ) + ) == 'http://main.realtime.ably.net'🧰 Tools
🪛 Ruff (0.8.2)
194-196: Yoda condition detected
(SIM300)
ably/types/options.py (1)
289-311: Ensure consistent fallback host handling in realtime hosts.The code checks for
self.rest_hostwhen determining fallback hosts for REST, but doesn't do an equivalent check for realtime hosts. Consider making this behavior consistent.def __get_realtime_hosts(self): if self.realtime_host is not None: host = self.realtime_host return [host] host = Defaults.get_hostname(self.endpoint) - return [host] + self.__fallback_hosts + # Fallback hosts + fallback_hosts = self.fallback_hosts + if fallback_hosts is None: + if self.realtime_host: + fallback_hosts = [] + else: + fallback_hosts = Defaults.get_fallback_hosts(self.endpoint) + + return [host] + fallback_hosts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
ably/transport/defaults.py(2 hunks)ably/types/options.py(6 hunks)test/ably/rest/restinit_test.py(4 hunks)ably/realtime/realtime.py(1 hunks)ably/rest/rest.py(1 hunks)ably/transport/defaults.py(1 hunks)ably/types/options.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)test/unit/options_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ably/transport/defaults.py
- ably/realtime/realtime.py
- ably/types/options.py
- ably/rest/rest.py
- test/ably/rest/restinit_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/ably/rest/restinit_test.py
79-79: Yoda condition detected
Rewrite as host == "main.realtime.ably.net"
(SIM300)
84-84: Yoda condition detected
Rewrite as host == "sandbox.realtime.ably-nonprod.net"
(SIM300)
185-187: Yoda condition detected
(SIM300)
194-196: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (12)
test/unit/options_test.py (5)
6-14: LGTM! Thorough validation tests for incompatible client options.The tests correctly verify that a ValueError is raised when endpoint is used together with any of the legacy options (environment, rest_host, or realtime_host). This aligns with the PR objective of making the endpoint option mutually exclusive with legacy options.
17-20: LGTM! Good verification of default hostname behavior.Tests confirm that when no endpoint is specified, the default hosts are correctly set to "main.realtime.ably.net" with appropriate fallback hosts.
23-27: LGTM! Comprehensive test for routing policy hostnames.Verifies that when a routing policy name (e.g., "foo") is provided as endpoint, the hostnames are correctly generated with the appropriate domain structure.
29-32: LGTM! Proper validation for nonprod routing policy hostnames.Test correctly verifies that when a non-production routing policy (e.g., "nonprod:foo") is specified, the realtime host and fallback hosts use the nonprod domain structure.
35-38: LGTM! Good verification of FQDN endpoint handling.Test confirms that when a fully qualified domain name is provided as endpoint, it's used directly as the realtime host and no fallback hosts are provided, which aligns with the ADR-119 requirements.
test/ably/rest/restinit_test.py (2)
82-84: Fix Yoda condition and update environment to endpoint.Update the test to use endpoint instead of environment for consistency with ADR-119.
- ably = AblyRest(token='foo', environment="nonprod:sandbox") + ably = AblyRest(token='foo', endpoint="nonprod:sandbox") host = ably.options.get_rest_host() - assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host + assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host🧰 Tools
🪛 Ruff (0.8.2)
84-84: Yoda condition detected
Rewrite as
host == "sandbox.realtime.ably-nonprod.net"(SIM300)
106-107: Update environment to endpoint in fallback hosts test.For consistency with ADR-119, update the test to use endpoint instead of environment.
- ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10) + ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10) assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted( ably.options.get_fallback_rest_hosts())ably/transport/defaults.py (2)
47-57: LGTM! Well-implemented hostname resolution logic.The hostname resolution logic correctly handles the different endpoint formats:
- FQDN when the endpoint contains a dot, double colon, or "localhost"
- Non-production routing policy names that start with "nonprod:"
- Default routing policy "main"
- Other routing policy names
This implementation aligns well with the ADR-119 requirements.
60-77:❓ Verification inconclusive
Implement REC2c2 fallback behavior.
While the fallback host generation is correctly implemented, the code doesn't fully implement the REC2c2 requirement for fallback behavior. According to REC2c2, when the primary host is unreachable, the SDK should:
- Iterate through fallback hosts in a shuffled order
- Implement a fallback retry timeout (default 10 minutes)
- Differentiate between production and non-production environments
From the Ably SDK specification, REC2c2 requires implementing a robust fallback mechanism that iterates through fallback hosts when the primary host is unreachable and implements a retry timeout before reverting to the primary host.
Please ensure this fallback behavior is fully implemented or confirm if it's handled elsewhere in the codebase.
🌐 Web query:
What is REC2c2 in Ably SDK specification?💡 Result:
Based on the provided search results, there is no explicit mention or definition of "REC2c2" in the Ably SDK specification documents. The specification repository ([1]) details versioning protocols like:
- Protocol Version (integer tracking wire protocol changes, specified under
RSC7afor REST andRTN2ffor Realtime connections)- Build Version (SemVer for SDK package versions)
- Specification Version (SemVer for backward-compatibility tracking in SDK APIs)
The "REC2c2" identifier doesn’t appear in:
- Protocol/versioning tables
- Feature spec point tracking systems
- Connection state documentation ([5])
- Client library examples ([2][6][7])
This suggests it might either be:
- A deprecated/legacy spec point identifier
- A typographical error in the query
- Part of internal documentation not included in public repositories
For authoritative clarification, consult the latest Ably SDK specification directly or contact Ably support with the exact context where this identifier was referenced.
Citations:
- 1: https://github.com/ably/specification
- 2: https://github.com/ably/ably-go
- 3: https://ably.com/docs/sdks
- 4: https://auth0.com/blog/ably-fully-featured-scalable-chat-app/
- 5: https://ably.com/docs/connect/states
- 6: https://devcenter.heroku.com/articles/ably
- 7: https://github.com/ably/ably-cocoa
- 8: https://www.youtube.com/watch?v=eSxda_gQRaI
- 9: https://www.youtube.com/watch?v=fYD0-wFlrXU
- 10: https://www.youtube.com/watch?v=ZB-mrfmSX48
- 11: https://www.youtube.com/watch?v=_70uOFiBeo8
Clarify Fallback Behavior Implementation per REC2c2
- The fallback URL generation implemented in this file appears to correctly construct the expected endpoints for both production and non-production environments.
- However, the additional fallback behavior—as implied by REC2c2 (namely, iterating through fallback hosts in a shuffled order and enforcing a fallback retry timeout of 10 minutes)—is not apparent in this snippet.
- Given that public Ably SDK documentation does not clearly define a "REC2c2" requirement, please confirm if:
- The shuffled iteration and retry timeout logic are handled elsewhere in the codebase, or
- They need to be implemented here to meet internal specifications.
ably/types/options.py (3)
22-24: LGTM! Good validation for incompatible options.This validation correctly enforces the mutual exclusivity between the new endpoint option and legacy options (environment, rest_host, realtime_host) as required by ADR-119.
🧰 Tools
🪛 Ruff (0.8.2)
22-23: Use a single
ifstatement instead of nestedifstatements(SIM102)
53-60: LGTM! Good handling of environment and endpoint mapping.The code correctly maps "production" environment to the default endpoint, preserves backward compatibility by mapping environment to endpoint when needed, and sets a default endpoint when none is provided.
176-177: LGTM! Property updated from environment to endpoint.This change properly updates the property from environment to endpoint to align with the new connection options model.
|
@surminus as discussed on Slack, please could you make sure that this PR implements / tests the updates to REC1b2 from ably/specification#302? Thanks! |
This implements ADR-119[1], which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042[2].
The endpoint may be one of the following:
a routing policy name (such as
main)a nonprod routing policy name (such as
nonprod:sandbox)a FQDN such as
foo.example.comThe endpoint option is not valid with any of
environment,restHostorrealtimeHost, but we still intend to support the legacy options.If the client has been configured to use any of these legacy options, then they should continue to work in the same way, using the same primary and fallback hostnames.
If the client has not been explicitly configured, then the hostnames will change to the new ably.net domain when the package is upgraded.
Notes on implementation
As per previous guidance, I have not updated tests to use the
endpointclient option, but since the hostnames are changing, then the assertions and default environment must be updated (fromsandboxtononprod:sandbox).I have not added any explicit tests for the endpoint client option, but I can do this if we think it's necessary, although using
environmentis essentially the same as usingendpoint.See related Go and Ruby PRs:
[1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3428810778/ADR-119+ClientOptions+for+new+DNS+structure
[2] https://ably.atlassian.net/wiki/spaces/ENG/pages/1791754276/ADR-042+DNS+Restructure
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor