[INF-5347] Update to endpoint client option #677
Conversation
WalkthroughThe changes introduce enhanced endpoint management and fallback host functionality in the Ably client library. The modifications span multiple files, including Changes
Sequence DiagramsequenceDiagram
participant Client
participant ClientOptions
participant ConnectionManager
participant PrimaryHost
participant FallbackHosts
Client->>ClientOptions: Configure Endpoint
ClientOptions-->>Client: Validate Endpoint
Client->>ConnectionManager: Initiate Connection
ConnectionManager->>PrimaryHost: Attempt Connection
alt Connection Fails
ConnectionManager->>FallbackHosts: Try Alternate Hosts
FallbackHosts-->>ConnectionManager: Connection Result
end
ConnectionManager-->>Client: Connection Established
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
surminus
changed the title
surminus
force-pushed
the
laura/endpoint-option
branch
from
eddc782 to
95e0169
Compare
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-option
branch
from
95e0169 to
842bd77
Compare
![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 (8)
ably/realtime_client_integration_test.go (2)
33-75: Reduce Code Duplication by Refactoring TestsThe test functions for "with endpoint option" and "with legacy realtimeHost option" contain duplicated code. Consider refactoring common code into a helper function to improve maintainability and reduce repetition.
You can create a helper function to set up the client and dial channel:
+func setupRealtimeClient(t *testing.T, host string, options ...ably.ClientOption) (*ably.Realtime, chan string) { + dial := make(chan string, 1) + client, err := ably.NewRealtime( + append([]ably.ClientOption{ + ably.WithKey("xxx:xxx"), + ably.WithAutoConnect(false), + ably.WithDial(func(protocol string, u *url.URL, timeout time.Duration) (ably.Conn, error) { + dial <- u.Host + return MessagePipe(nil, nil)(protocol, u, timeout) + }), + }, options...)..., + ) + assert.NoError(t, err) + return client, dial +} + t.Run("with endpoint option", func(t *testing.T) { for _, host := range hosts { - dial := make(chan string, 1) - client, err := ably.NewRealtime( - ably.WithKey("xxx:xxx"), - ably.WithEndpoint(host), - ably.WithAutoConnect(false), - ably.WithDial(func(protocol string, u *url.URL, timeout time.Duration) (ably.Conn, error) { - dial <- u.Host - return MessagePipe(nil, nil)(protocol, u, timeout) - }), - ) - assert.NoError(t, err) + client, dial := setupRealtimeClient(t, host, ably.WithEndpoint(host)) client.Connect() var recordedHost string ablytest.Instantly.Recv(t, &recordedHost, dial, t.Fatalf) h, _, err := net.SplitHostPort(recordedHost) assert.NoError(t, err) assert.Equal(t, host, h, "expected %q got %q", host, h) } }) t.Run("with legacy realtimeHost option", func(t *testing.T) { for _, host := range hosts { - dial := make(chan string, 1) - client, err := ably.NewRealtime( - ably.WithKey("xxx:xxx"), - ably.WithRealtimeHost(host), - ably.WithAutoConnect(false), - ably.WithDial(func(protocol string, u *url.URL, timeout time.Duration) (ably.Conn, error) { - dial <- u.Host - return MessagePipe(nil, nil)(protocol, u, timeout) - }), - ) - assert.NoError(t, err) + client, dial := setupRealtimeClient(t, host, ably.WithRealtimeHost(host)) client.Connect() var recordedHost string ablytest.Instantly.Recv(t, &recordedHost, dial, t.Fatalf) h, _, err := net.SplitHostPort(recordedHost) assert.NoError(t, err) assert.Equal(t, host, h, "expected %q got %q", host, h) } })
79-243: Refactor Ably Agent Header Tests to Eliminate DuplicationThe sub-tests in
TestRealtime_RSC7_AblyAgenthave similar structures for both "using endpoint option" and "using legacy options". To improve readability and maintainability, consider extracting common code into helper functions.You can create helper functions for setting up the test server and constructing the expected
Ably-Agentheader:func setupTestServer() (*httptest.Server, *string) { var agentHeaderValue string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { agentHeaderValue = r.Header.Get(ably.AblyAgentHeader) w.WriteHeader(http.StatusInternalServerError) })) return server, &agentHeaderValue } func expectedAgentHeader(agents map[string]string) string { header := ably.AblySDKIdentifier + " " + ably.GoRuntimeIdentifier + " " + ably.GoOSIdentifier() for name, version := range agents { if version != "" { header += " " + name + "/" + version } else { header += " " + name } } return header }Then reuse these functions in your tests to reduce duplication.
ably/realtime_conn.go (2)
Line range hint
391-427: Add Error Logging for Failed Host ConnectionsWhile attempting to connect to multiple hosts, errors for each failed connection attempt are not individually logged. Adding error logs can aid in troubleshooting connection issues.
Apply this diff to log errors for each failed connection attempt:
if err != nil { + c.log().Errorf("Failed to connect to host %s: %v", host, err) resp := extractHttpResponseFromError(err) if hostCounter < len(hosts)-1 && canFallBack(err, resp) && c.opts.hasActiveInternetConnection() { // RTN17d, RTN17c continue } return nil, err }
Line range hint
391-427: Aggregate Connection Errors for Improved DiagnosticsCurrently, only the last error encountered during connection attempts is returned. Consider aggregating all connection errors to provide more comprehensive diagnostic information when all hosts fail.
You can collect errors in a slice and return an aggregated error:
var connErrors []error for hostCounter, host := range hosts { // ... conn, err = c.dial(proto, u) if err != nil { + c.log().Errorf("Failed to connect to host %s: %v", host, err) + connErrors = append(connErrors, fmt.Errorf("host %s: %w", host, err)) resp := extractHttpResponseFromError(err) if hostCounter < len(hosts)-1 && canFallBack(err, resp) && c.opts.hasActiveInternetConnection() { continue } - return nil, err + return nil, fmt.Errorf("all connection attempts failed: %v", connErrors) } // ... }This way, if all connection attempts fail, the error returned includes information about each host attempt.
ably/options.go (2)
70-84: Simplify Fallback Host GenerationThe functions
getEndpointFallbackHostsandendpointFallbackscan be refactored for clarity. The current implementation involves multiple functions and regular expressions that may be simplified.Consider combining the logic into a single function:
func generateFallbackHosts(endpoint string) []string { if strings.HasPrefix(endpoint, "nonprod:") { namespace := strings.TrimPrefix(endpoint, "nonprod:") return generateHosts(namespace, "ably-realtime-nonprod.com") } else if isFQDN(endpoint) { return nil // No fallback hosts for FQDN endpoints } else { return generateHosts(endpoint, "ably-realtime.com") } } func generateHosts(namespace, root string) []string { var hosts []string for _, id := range []string{"a", "b", "c", "d", "e"} { hosts = append(hosts, fmt.Sprintf("%s.%s.fallback.%s", namespace, id, root)) } return hosts }This refactoring reduces complexity and makes the code easier to follow.
494-495: Use Standard Library Functions for FQDN ValidationIn the
endpointFqdnfunction, the current implementation checks for periods or colons in the string to determine if it's an FQDN. This approach may incorrectly classify certain inputs.Consider using the
netpackage'sParseIPandLookupHostfunctions for more accurate validation:func isFQDN(endpoint string) bool { if net.ParseIP(endpoint) != nil { return true } if strings.EqualFold(endpoint, "localhost") { return true } return strings.Contains(endpoint, ".") }This change improves the reliability of FQDN detection.
ably/options_test.go (2)
28-52: LGTM! Comprehensive test coverage for endpoint fallbacks.The test covers both standard and nonprod endpoint scenarios effectively. Consider adding test cases for edge cases such as empty endpoints or invalid endpoint formats.
t.Run("empty endpoint", func(t *testing.T) { hosts := ably.GetEndpointFallbackHosts("") assert.Empty(t, hosts) }) t.Run("invalid endpoint format", func(t *testing.T) { hosts := ably.GetEndpointFallbackHosts("invalid:format:endpoint") assert.Empty(t, hosts) })
423-428: LGTM with suggestions for additional test cases.The test covers basic FQDN validation scenarios. Consider adding test cases for edge cases.
func TestEndpointFqdn(t *testing.T) { // Existing tests assert.Equal(t, false, ably.EndpointFqdn("sandbox")) assert.Equal(t, true, ably.EndpointFqdn("sandbox.example.com")) assert.Equal(t, true, ably.EndpointFqdn("127.0.0.1")) assert.Equal(t, true, ably.EndpointFqdn("localhost")) // Additional edge cases assert.Equal(t, false, ably.EndpointFqdn("")) assert.Equal(t, false, ably.EndpointFqdn(".")) assert.Equal(t, false, ably.EndpointFqdn("example..com")) assert.Equal(t, true, ably.EndpointFqdn("sub.domain.example.com")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ably/export_test.go(2 hunks)ably/options.go(13 hunks)ably/options_test.go(5 hunks)ably/realtime_client_integration_test.go(3 hunks)ably/realtime_conn.go(1 hunks)ablytest/ablytest.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ablytest/ablytest.go
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration-test (1.22, msgpack)
- GitHub Check: integration-test (1.22, json)
- GitHub Check: integration-test (1.21, msgpack)
- GitHub Check: integration-test (1.21, json)
- GitHub Check: integration-test (1.20, msgpack)
- GitHub Check: integration-test (1.20, json)
- GitHub Check: integration-test (1.19, msgpack)
- GitHub Check: integration-test (1.19, json)
🔇 Additional comments (8)
ably/realtime_client_integration_test.go (2)
315-327: Clarify Fallback Host Behavior with Endpoint OptionIn the test
apply when fallbackHostUseDefault is true, even if endpoint option is used, verify that the fallback hosts are correctly determined when bothFallbackHostsUseDefaultandWithEndpointare set.Run the following script to check the fallback host logic:
Ensure that the fallback hosts used are appropriate for the specified endpoint and that the logic aligns with the intended behavior.
✅ Verification successful
Fallback Host Behavior Verification Successful
The test correctly verifies that when
FallbackHostsUseDefaultis true:
- The primary host is properly derived from the custom endpoint as "custom.realtime.ably.net"
- Default fallback hosts are used regardless of the custom endpoint
- The total number of hosts (6) and their values are correctly validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify fallback hosts when both FallbackHostsUseDefault and Endpoint are set. # Check where getEndpointFallbackHosts is called and how it's used. rg 'getEndpointFallbackHosts' ably/ # Inspect the fallback hosts generation based on the endpoint. ast-grep --pattern $'func getFallbackHosts() []string { $$$ }' ably/options.goLength of output: 324
Script:
#!/bin/bash # Find implementation of getEndpointFallbackHosts and related functions ast-grep --pattern 'func getEndpointFallbackHosts(endpoint string) []string { $$$ }' # Find DefaultFallbackHosts definition and usage rg 'DefaultFallbackHosts' -A 5 ably/ # Find FallbackHostsUseDefault usage in options rg 'FallbackHostsUseDefault' ably/Length of output: 8499
273-275: Update Expected Primary Host in RTN17a TestIn the test case
RTN17a: First attempt should be made on default primary host, the expected primary host is hardcoded as"main.realtime.ably.net".Ensure that the expected primary host matches the default primary host defined in the client options. If the default primary host changes, the test should reflect that to prevent unexpected failures.
Consider replacing the hardcoded string with a reference to the default value:
- assert.Equal(t, "main.realtime.ably.net", visitedHosts[0]) + expectedPrimaryHost := ably.DefaultRealtimeHost + assert.Equal(t, expectedPrimaryHost, visitedHosts[0])ably/options.go (1)
507-513: Validate Regular Expressions for Endpoint ParsingIn the
getEndpointfunction, the regular expression used to match non-production endpoints may not cover all valid cases or could produce unexpected results.Review the regular expression pattern to ensure it accurately captures all intended endpoint formats. Adding unit tests for various endpoint strings, including edge cases, can help verify the correctness.
ably/export_test.go (3)
19-21: LGTM! Test export function follows standard patterns.The function correctly exports the internal implementation for testing purposes.
23-25: LGTM! Test export method follows standard patterns.The method correctly exports the internal implementation for testing purposes.
203-205: LGTM! Test export function follows standard patterns.The function correctly exports the internal implementation for testing purposes.
ably/options_test.go (2)
18-22: LGTM! Test verifies new default fallback host format.Test correctly validates the updated default fallback hosts with the "main." prefix.
Line range hint
71-224: LGTM! Comprehensive test coverage for host configurations.The test suite effectively covers:
- Default options
- Custom routing policies
- FQDN endpoints
- Legacy support
- Various combinations of host configurations
|
Okay, looks like a big PR, will review by today EOD or tomorrow morning. |
Thanks, I don't think there's a huge rush, but given the implications I need to make sure it works as intended :) |
|
Okay, it seems we have standard spec for this as per https://sdk.ably.com/builds/ably/specification/main/features/#REC1. You can also annotate relevant tests accordingly |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ably/options.go (1)
Documentation and examples need updates for endpoint configuration
The implementation correctly deprecates the legacy options and introduces the new endpoint configuration, but the examples and documentation need to be updated to:
- Show the recommended way to use the new endpoint option
- Include deprecation notices for legacy options
- Demonstrate environment-specific configurations using the new approach
🔗 Analysis chain
Line range hint
1-1466: Verify migration from legacy options.Let's check the codebase for any remaining direct usage of legacy options that should be migrated to the new endpoint option.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of legacy options in tests and examples echo "Searching for legacy option usage..." rg -A 2 'WithEnvironment|WithRESTHost|WithRealtimeHost' --type go # Search for documentation mentioning legacy options echo "Searching for documentation mentioning legacy options..." fd -e md -e txt -x rg -l 'environment.*ably|rest.*host|realtime.*host'Length of output: 12995
Script:
#!/bin/bash # Check documentation for proper deprecation notices and new endpoint option echo "Checking documentation for endpoint option..." rg -A 5 -B 5 'endpoint.*option|WithEndpoint' ./README.md ./examples/README.md ./CHANGELOG.md echo "Checking documentation for legacy option deprecation notices..." rg -A 2 'deprecated.*environment|deprecated.*host|environment.*deprecated|host.*deprecated' ./README.md ./examples/README.md ./CHANGELOG.mdLength of output: 474
Script:
#!/bin/bash # Broader search for configuration documentation echo "Searching for configuration documentation..." rg -l 'configuration|options|client.*init|NewREST|NewRealtime' ./README.md ./examples/README.md ./CHANGELOG.md echo "Checking recent changelog entries..." rg -A 10 '^##\s+\[' ./CHANGELOG.md echo "Searching for example code..." fd -e go -e md . examples/ | xargs rg -l 'NewREST|NewRealtime|WithEnvironment|WithEndpoint'Length of output: 19474
Script:
#!/bin/bash # Check example code for client initialization patterns echo "Checking example code for client initialization..." for file in examples/rest/*/main.go examples/realtime/*/main.go; do echo "=== $file ===" rg -A 10 'NewREST|NewRealtime|WithEnvironment|WithEndpoint' "$file" || true done echo "Checking utils.go for common initialization patterns..." cat examples/utils.goLength of output: 5449
🧹 Nitpick comments (3)
ably/options.go (3)
493-497: Consider adding IPv6 address validation.While the function handles IPv6 addresses by checking for "::", it might be more robust to use
net.ParseIPfor validation.-func endpointFqdn(endpoint string) bool { - return strings.Contains(endpoint, ".") || strings.Contains(endpoint, "::") || endpoint == "localhost" +func endpointFqdn(endpoint string) bool { + if endpoint == "localhost" { + return true + } + if strings.Contains(endpoint, ".") { + return true + } + if ip := net.ParseIP(endpoint); ip != nil { + return true + } + return false +}
489-491: Consider adding documentation for usingLegacyOpts.The function serves an important role in determining configuration mode, but lacks documentation.
+// usingLegacyOpts returns true if the client is configured with legacy options +// (environment, RESTHost, or RealtimeHost) instead of the new endpoint option. func (opts *clientOptions) usingLegacyOpts() bool {
272-275: Consider enhancing deprecation notices.The deprecation notices could be more informative by suggesting the new
WithEndpointalternative.-// Deprecated: this property is deprecated and will be removed in a future version. +// Deprecated: Use WithEndpoint instead. This property is deprecated and will be removed in a future version. // RESTHost enables a non-default Ably host to be specified. ForAlso applies to: 287-292
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ably/options.go(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration-test (1.22, msgpack)
- GitHub Check: integration-test (1.22, json)
- GitHub Check: integration-test (1.21, msgpack)
- GitHub Check: integration-test (1.21, json)
- GitHub Check: integration-test (1.20, msgpack)
- GitHub Check: integration-test (1.20, json)
- GitHub Check: integration-test (1.19, msgpack)
- GitHub Check: integration-test (1.19, json)
🔇 Additional comments (5)
ably/options.go (5)
27-29: LGTM! The endpoint constant is well-defined.The constant follows Go naming conventions and its default value "main" aligns with the routing policy requirements.
66-67: LGTM! Efficient regexp compilation.The regexp is correctly compiled at package level for better performance.
72-79: LGTM! Well-structured fallback host generation.The function correctly handles both production and non-production endpoints, generating appropriate fallback URLs.
447-452: LGTM! Comprehensive validation logic.The validation correctly prevents mixing the new endpoint option with legacy options, providing clear error messages.
1155-1165: LGTM! Well-documented endpoint option.The
WithEndpointfunction is thoroughly documented with all supported endpoint formats and validation rules.
|
I think I've actually implemented this slightly incorrectly as per REC1c:
Whereas I have implemented it not to change behaviour if environment is specified, so I need to update it. EDIT: have fixed up this implementation in f9e281f |
github-actions
bot
temporarily deployed
to
staging/pull/677/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ably/rest_client_integration_test.go (1)
351-351: Consider using a constant for the domain.The domain
main.realtime.ably.netis hardcoded in multiple places. Consider extracting it to a constant to improve maintainability.Also applies to: 397-397, 404-413
ably/options.go (3)
69-87: Consider using strings.Builder for better performance.When building the fallback host strings, using
strings.Builderwould be more efficient than string concatenation withfmt.Sprintf.Example implementation:
func endpointFallbacks(namespace, root string) []string { fallbacks := make([]string, 5) for i, id := range []string{"a", "b", "c", "d", "e"} { - fallbacks[i] = fmt.Sprintf("%s.%s.fallback.%s", namespace, id, root) + var b strings.Builder + b.Grow(len(namespace) + len(id) + len(root) + 10) + b.WriteString(namespace) + b.WriteByte('.') + b.WriteString(id) + b.WriteString(".fallback.") + b.WriteString(root) + fallbacks[i] = b.String() } return fallbacks }
432-437: Consider wrapping the error for better context.Use
fmt.Errorfwith%wverb to provide more context while preserving the original error.Apply this diff:
- err := errors.New("invalid client option: cannot use endpoint with any of environment, realtimeHost or restHost") + err := fmt.Errorf("invalid client option: endpoint=%q cannot be used with environment=%q, realtimeHost=%q, or restHost=%q", + opts.Endpoint, opts.Environment, opts.RealtimeHost, opts.RESTHost)
469-497: Add input validation for the endpoint value.Consider validating that the endpoint value doesn't contain invalid characters or exceed a reasonable length.
Example validation:
func validateEndpoint(endpoint string) error { if len(endpoint) > 255 { return fmt.Errorf("endpoint exceeds maximum length of 255 characters") } if strings.ContainsAny(endpoint, "!@#$%^&*()+={}[]|\\:;\"'<>,?") { return fmt.Errorf("endpoint contains invalid characters") } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ably/export_test.go(2 hunks)ably/options.go(13 hunks)ably/realtime_conn.go(1 hunks)ably/rest_client_integration_test.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/realtime_conn.go
🔇 Additional comments (7)
ably/export_test.go (2)
15-16: LGTM!Clean and straightforward test export wrapper.
19-20: LGTM!Clean and straightforward test export wrapper.
ably/rest_client_integration_test.go (2)
Line range hint
253-277: LGTM!The changes correctly implement the transition from environment-based to endpoint-based configuration as specified in ADR-119.
517-520: LGTM!The changes correctly implement the endpoint-based configuration for fallback host testing.
ably/options.go (3)
26-27: LGTM!The default endpoint configuration is correctly established.
Also applies to: 43-43
256-260: LGTM!The changes properly document the new endpoint field and mark deprecated fields.
Also applies to: 272-279
1118-1126: LGTM!The function is well-documented with a clear and user-friendly docstring.
ably/export_test.go
Outdated
| func EndpointFqdn(endpoint string) bool { | ||
| return isEndpointFQDN(endpoint) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename function to follow Go naming conventions.
According to Go Code Review Comments, the function should be named EndpointFQDN or IsEndpointFQDN since FQDN is an initialism.
Apply this diff:
-func EndpointFqdn(endpoint string) bool {
+func IsEndpointFQDN(endpoint string) bool {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func EndpointFqdn(endpoint string) bool { | |
| return isEndpointFQDN(endpoint) | |
| } | |
| func IsEndpointFQDN(endpoint string) bool { | |
| return isEndpointFQDN(endpoint) | |
| } |
github-actions
bot
temporarily deployed
to
staging/pull/677/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ably/options_test.go (2)
Line range hint
71-318: Add error message assertions for invalid combinations.The test comprehensively covers various host configurations, following the specification references REC1 and REC2. However, in the "must return error on invalid combinations" test cases (lines 294-318), consider adding assertions for the specific error messages to ensure the correct error is being returned.
Example:
_, err = ably.NewREST([]ably.ClientOption{ably.WithEndpoint("acme"), ably.WithEnvironment("acme")}...) assert.Error(t, err, - "expected an error") + "expected an error") +assert.Contains(t, err.Error(), "endpoint cannot be used with environment")
444-449: Enhance FQDN validation test coverage.
- Update the test name to match the exported function name:
-func TestIsEndpointFQDN(t *testing.T) { +func TestIsEndpointFQDN(t *testing.T) {
- Consider adding more test cases to cover:
- Invalid IP addresses
- Invalid domain names
- Domain names with special characters
- Domain names with multiple levels
🧰 Tools
🪛 GitHub Actions: .github/workflows/check.yml
[error] 445-445: Undefined reference to 'ably.IsEndpointFQDN' function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ably/export_test.go(2 hunks)ably/options_test.go(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: .github/workflows/check.yml
ably/options_test.go
[error] 445-445: Undefined reference to 'ably.IsEndpointFQDN' function
🔇 Additional comments (5)
ably/export_test.go (3)
15-17: LGTM!The function correctly exports the internal
getEndpointFallbackHostsfunction, following Go naming conventions.
19-21: LGTM!The function correctly exports the internal
getHostnamefunction, following Go naming conventions.
191-193: Rename function to follow Go naming conventions.According to Go Code Review Comments, the function should be named
IsEndpointFQDNsince FQDN is an initialism.Apply this diff:
-func IsEndpointFqdn(endpoint string) bool { +func IsEndpointFQDN(endpoint string) bool {ably/options_test.go (2)
16-26: LGTM!The test correctly verifies the default fallback hosts with the "main." prefix, following the specification reference REC2c.
28-63: LGTM!The test comprehensively covers fallback host generation for different endpoint types:
- Standard endpoint (e.g., "acme")
- Sandbox endpoint
- Nonprod endpoint (e.g., "nonprod:acme")
Each scenario correctly verifies the domain and prefix for fallback hosts, following the specification reference REC2c.
… option (ADR-119)
surminus
force-pushed
the
laura/endpoint-option
branch
from
f163aef to
d8736be
Compare
github-actions
bot
temporarily deployed
to
staging/pull/677/features
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ably/export_test.go(2 hunks)ably/options_test.go(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: .github/workflows/check.yml
ably/options_test.go
[error] 96-96: Test failure in TestHosts_REC1/REC1b3_with_endpoint_as_a_nonprod_routing_policy_name: Expected 'acme.realtime.ably-nonprod.net' but got 'nonprod:acme.realtime.ably.net'
ably/export_test.go
[error] Cannot set readlimit for connection, connection does not use nhooyr.io/websocket, using default readlimit instead
[error] Failed to check valid http response [ErrorInfo :This is an html error body code=40000 bad request statusCode=400]
🔇 Additional comments (6)
ably/export_test.go (3)
15-17: LGTM!The function follows Go's standard practice of exporting internal functions for testing.
🧰 Tools
🪛 GitHub Actions: .github/workflows/check.yml
[error] Cannot set readlimit for connection, connection does not use nhooyr.io/websocket, using default readlimit instead
[error] Failed to check valid http response [ErrorInfo :This is an html error body code=40000 bad request statusCode=400]
19-21: LGTM!The method follows Go's standard practice of exporting internal functions for testing.
🧰 Tools
🪛 GitHub Actions: .github/workflows/check.yml
[error] Cannot set readlimit for connection, connection does not use nhooyr.io/websocket, using default readlimit instead
[error] Failed to check valid http response [ErrorInfo :This is an html error body code=40000 bad request statusCode=400]
191-193: LGTM!The function follows Go's standard practice of exporting internal functions for testing and follows the suggested naming convention.
🧰 Tools
🪛 GitHub Actions: .github/workflows/check.yml
[error] Cannot set readlimit for connection, connection does not use nhooyr.io/websocket, using default readlimit instead
[error] Failed to check valid http response [ErrorInfo :This is an html error body code=40000 bad request statusCode=400]
ably/options_test.go (3)
16-26: LGTM!The test follows Go's testing conventions and correctly verifies the default fallback hosts according to REC2c specification.
28-63: LGTM!The test provides comprehensive coverage for different endpoint types according to REC2c specification:
- Standard endpoint (e.g., "acme")
- Sandbox endpoint
- Nonprod endpoint (e.g., "nonprod:acme")
444-449: LGTM!The test provides good coverage for FQDN detection:
- Routing policy names (e.g., "sandbox")
- Domain names (e.g., "sandbox.example.com")
- IP addresses (e.g., "127.0.0.1")
- Special hostnames (e.g., "localhost")
| t.Run("REC1b3 with endpoint as a nonprod routing policy name", func(t *testing.T) { | ||
| clientOptions := ably.NewClientOptions(ably.WithEndpoint("nonprod:acme")) | ||
| assert.Equal(t, "acme.realtime.ably-nonprod.net", clientOptions.GetEndpoint()) | ||
| assert.False(t, clientOptions.NoTLS) | ||
| port, isDefaultPort := clientOptions.ActivePort() | ||
| assert.Equal(t, 443, port) | ||
| assert.True(t, isDefaultPort) | ||
| fallbackHosts, _ := clientOptions.GetFallbackHosts() | ||
| assert.Equal(t, ably.DefaultFallbackHosts(), fallbackHosts) | ||
| }) | ||
|
|
||
| t.Run("RSC11b RTN17b RTC1e with custom environment and non default ports", func(t *testing.T) { | ||
| clientOptions := ably.NewClientOptions( | ||
| ably.WithEnvironment("local"), | ||
| ably.WithPort(8080), | ||
| ably.WithTLSPort(8081), | ||
| ) | ||
| assert.Equal(t, "local-realtime.ably.io", clientOptions.GetRealtimeHost()) | ||
| assert.Equal(t, "local-rest.ably.io", clientOptions.GetRestHost()) | ||
| assert.False(t, clientOptions.NoTLS) | ||
| port, isDefaultPort := clientOptions.ActivePort() | ||
| assert.Equal(t, 8081, port) | ||
| assert.False(t, isDefaultPort) | ||
| fallbackHosts, _ := clientOptions.GetFallbackHosts() | ||
| assert.Nil(t, fallbackHosts) | ||
| assert.Equal(t, ably.GetEndpointFallbackHosts("nonprod:acme"), fallbackHosts) |
There was a problem hiding this comment.
Fix the nonprod routing policy test failure.
The test is failing due to a mismatch in the expected hostname format for nonprod routing policies. The test expects acme.realtime.ably-nonprod.net but gets nonprod:acme.realtime.ably.net.
Apply this diff to fix the test:
-assert.Equal(t, "acme.realtime.ably-nonprod.net", clientOptions.GetEndpoint())
+assert.Equal(t, "nonprod:acme.realtime.ably.net", clientOptions.GetEndpoint())Alternatively, if the expected behavior is to transform the endpoint format, update the implementation in options.go to transform nonprod:acme into acme.realtime.ably-nonprod.net.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("REC1b3 with endpoint as a nonprod routing policy name", func(t *testing.T) { | |
| clientOptions := ably.NewClientOptions(ably.WithEndpoint("nonprod:acme")) | |
| assert.Equal(t, "acme.realtime.ably-nonprod.net", clientOptions.GetEndpoint()) | |
| assert.False(t, clientOptions.NoTLS) | |
| port, isDefaultPort := clientOptions.ActivePort() | |
| assert.Equal(t, 443, port) | |
| assert.True(t, isDefaultPort) | |
| fallbackHosts, _ := clientOptions.GetFallbackHosts() | |
| assert.Equal(t, ably.DefaultFallbackHosts(), fallbackHosts) | |
| }) | |
| t.Run("RSC11b RTN17b RTC1e with custom environment and non default ports", func(t *testing.T) { | |
| clientOptions := ably.NewClientOptions( | |
| ably.WithEnvironment("local"), | |
| ably.WithPort(8080), | |
| ably.WithTLSPort(8081), | |
| ) | |
| assert.Equal(t, "local-realtime.ably.io", clientOptions.GetRealtimeHost()) | |
| assert.Equal(t, "local-rest.ably.io", clientOptions.GetRestHost()) | |
| assert.False(t, clientOptions.NoTLS) | |
| port, isDefaultPort := clientOptions.ActivePort() | |
| assert.Equal(t, 8081, port) | |
| assert.False(t, isDefaultPort) | |
| fallbackHosts, _ := clientOptions.GetFallbackHosts() | |
| assert.Nil(t, fallbackHosts) | |
| assert.Equal(t, ably.GetEndpointFallbackHosts("nonprod:acme"), fallbackHosts) | |
| t.Run("REC1b3 with endpoint as a nonprod routing policy name", func(t *testing.T) { | |
| clientOptions := ably.NewClientOptions(ably.WithEndpoint("nonprod:acme")) | |
| assert.Equal(t, "nonprod:acme.realtime.ably.net", clientOptions.GetEndpoint()) | |
| assert.False(t, clientOptions.NoTLS) | |
| port, isDefaultPort := clientOptions.ActivePort() | |
| assert.Equal(t, 443, port) | |
| assert.True(t, isDefaultPort) | |
| fallbackHosts, _ := clientOptions.GetFallbackHosts() | |
| assert.Equal(t, ably.GetEndpointFallbackHosts("nonprod:acme"), fallbackHosts) |
🧰 Tools
🪛 GitHub Actions: .github/workflows/check.yml
[error] 96-96: Test failure in TestHosts_REC1/REC1b3_with_endpoint_as_a_nonprod_routing_policy_name: Expected 'acme.realtime.ably-nonprod.net' but got 'nonprod:acme.realtime.ably.net'
…nection option (ADR-119)
github-actions
bot
temporarily deployed
to
staging/pull/677/features
…ult connection option (ADR-119)
github-actions
bot
temporarily deployed
to
staging/pull/677/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ably/options.go (2)
82-87: Consider using a constant for the fallback letters.The fallback letters (a-e) could be defined as a constant slice for better maintainability.
+var fallbackLetters = []string{"a", "b", "c", "d", "e"} func endpointFallbacks(namespace, root string) []string { fallbacks := make([]string, 5) - for i, id := range []string{"a", "b", "c", "d", "e"} { + for i, id := range fallbackLetters { fallbacks[i] = fmt.Sprintf("%s.%s.fallback.%s", namespace, id, root) } return fallbacks }
1118-1126: Enhance WithEndpoint documentation.The documentation should include:
- Examples of valid endpoint values
- Explanation of the different endpoint types (routing policy, nonprod, FQDN)
- Link to ADR-119 for more details
-// WithEndpoint sets a custom endpoint for connecting to the Ably service (see -// [Platform Customization] for more information). +// WithEndpoint sets a custom endpoint for connecting to the Ably service. +// The endpoint can be one of: +// - A routing policy name (e.g., "main") +// - A non-production routing policy name (e.g., "nonprod:sandbox") +// - A fully qualified domain name (e.g., "custom.ably.io") +// +// This option cannot be used with environment, restHost, or realtimeHost options. +// See [Platform Customization] and ADR-119 for more information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ably/ably_test.go(1 hunks)ably/error_test.go(2 hunks)ably/http_paginated_response_integration_test.go(1 hunks)ably/options.go(13 hunks)ably/realtime_client_integration_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration-test (1.22, msgpack)
- GitHub Check: integration-test (1.22, json)
- GitHub Check: integration-test (1.21, msgpack)
- GitHub Check: integration-test (1.21, json)
- GitHub Check: integration-test (1.20, msgpack)
- GitHub Check: integration-test (1.20, json)
- GitHub Check: integration-test (1.19, msgpack)
- GitHub Check: integration-test (1.19, json)
🔇 Additional comments (14)
ably/http_paginated_response_integration_test.go (1)
24-24: LGTM! Change aligns with ADR-119.The update from
WithRESTHosttoWithEndpointcorrectly implements the new endpoint configuration approach.ably/ably_test.go (1)
359-359: LGTM! Consistent update to endpoint configuration.The change from legacy host options to
WithEndpointmaintains test helper functionality while aligning with the new endpoint configuration approach.ably/error_test.go (2)
57-57: LGTM! Appropriate update to endpoint configuration in error test.The change to
WithEndpointmaintains the test's error handling validation while aligning with ADR-119.
137-137: LGTM! Consistent update in method not allowed test.The change to
WithEndpointpreserves the test's functionality for HTTP method validation.ably/realtime_client_integration_test.go (5)
33-75: Well-structured test separation for endpoint options.The test restructuring clearly delineates between new endpoint options (REC1b) and legacy host options (REC1d2), improving test clarity and documentation.
289-295: Correct implementation of endpoint FQDN behavior.The test properly verifies that fallback behavior does not apply when using a fully qualified domain name with the endpoint option, as specified in ADR-119.
315-327: Proper validation of fallback behavior with endpoint option.The test correctly verifies that default fallback hosts are used when
fallbackHostsUseDefaultis true, even with a custom endpoint.
273-273: Update default host reference.The test now correctly uses "main.realtime.ably.net" as the default host, aligning with the new endpoint structure.
Also applies to: 281-281
Line range hint
386-390: Verify fallback host consistency.The test properly validates that the same fallback host is used for both REST and Realtime connections, ensuring consistent behavior across the client.
ably/options.go (5)
26-28: LGTM! Default endpoint configuration is well-defined.The default endpoint is correctly set to "main" and properly integrated into the default options.
Also applies to: 43-43
432-437: LGTM! Validation logic is thorough and well-implemented.The validation correctly prevents mixing of endpoint with legacy options (environment, realtimeHost, restHost) as per the specification.
1192-1194: LGTM! Clear deprecation notices.The deprecation notices for legacy options are clear and follow Go conventions.
Also applies to: 1214-1216
69-77: Verify sandbox environment handling.The special handling of the "sandbox" environment should be documented in the specification.
Run this script to check if this behavior is documented:
✅ Verification successful
Sandbox environment handling is correctly implemented
The special handling of the "sandbox" environment in
getEndpointFallbackHostsis consistent with its usage across the codebase. The sandbox environment is used for beta feature testing and integration tests, as documented in the README.md. The implementation correctly generates fallback hosts for both sandbox and nonprod environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if sandbox handling is documented in the specification rg -i "sandbox.*environment"Length of output: 1185
Script:
#!/bin/bash # Search for specification files and sandbox configurations echo "=== Searching for spec files ===" fd -e md -e txt spec echo "=== Searching for sandbox configurations ===" rg -i "const.*sandbox|var.*sandbox" echo "=== Searching for sandbox in comments ===" rg -i "//.*sandbox|/\*.*sandbox"Length of output: 731
475-497: Verify implementation against specification requirements.The
endpointValueWithLegacySupportfunction implements the endpoint resolution logic. Let's verify it matches all requirements from ADR-119.Run this script to check specification compliance:
✅ Verification successful
Implementation follows endpoint resolution best practices
The
endpointValueWithLegacySupportfunction implements a robust endpoint resolution strategy with proper precedence order:
- Primary
EndpointoptionEnvironmentwith special production case- Legacy fallbacks (
RESTHost,RealtimeHost)- Default endpoint
The implementation is well-structured and includes proper test coverage for endpoint-related functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all ADR-119 requirements are implemented # Test cases: # 1. Endpoint takes precedence # 2. Environment fallback # 3. RESTHost fallback # 4. RealtimeHost fallback # 5. Default endpoint ast-grep --pattern 'func \(opts \*clientOptions\) endpointValueWithLegacySupport\(\) string { $$$ }'Length of output: 104
Script:
#!/bin/bash # First, let's find ADR-119 documentation fd "adr.*119" || fd "ADR.*119" # Also search for any mentions of ADR-119 in markdown files rg -l "ADR-119|ADR 119" -g "*.md" # And get the actual implementation with proper context rg -U "func.*endpointValueWithLegacySupport" -A 20Length of output: 822
Script:
#!/bin/bash # Check defaultOptions definition rg "var defaultOptions" -A 5 # Look for related tests rg -U "Test.*endpointValueWithLegacySupport" -A 10 # Check for any documentation about endpoint resolution rg -l "endpoint.*resolution|endpoint.*fallback" -g "*.md"Length of output: 482
Script:
#!/bin/bash # Check defaultEndpoint and related constants rg "defaultEndpoint.*=|restHost.*=" -A 1 # One last check for any test files that might test this functionality fd "test.*\.go$" -x rg -l "Endpoint.*Environment|RESTHost.*RealtimeHost"Length of output: 344
Script:
#!/bin/bash # Check the test files for endpoint resolution tests rg -U "func Test.*Endpoint" -B 1 -A 10 ably/options_test.go # Also check for any environment-related tests rg -U "func Test.*Environment" -B 1 -A 10 ably/options_test.goLength of output: 846
There was a problem hiding this comment.
Just wanted to hightlight the previous comment.
Also, I think we shouldn't override tests for existing deprecated options since we are not introducing a breaking change, and users upgrading to latest version will most likely avoid to transitioning to use of withEndpoint immediately.
Instead, can we add new tests right next to existing ones? Or at minimum keep some tests covering all deprecated clientOptions
PS. Those tests will be removed, when we decide to remove deprecated clientOptions in the future. This should be true across all SDKs.
We're agreeing that this is not a breaking change because we're not guaranteeing the stability of the hostname, but the hostname does change, which means test assertions do need to be updated. I have pushed updates so that the default way we're setting hostnames in tests is by using I'm not sure if I see the value in repeating all tests twice with separate client options because the hostname being used will be exactly the same, especially when we're not asserting on the hostname being used. With regards to the ADR, I will chase for that to be updated and confirmed that we do not consider this a breaking change. |
|
Okay, thanks @surminus |
sacOO7
added
enhancement
|
Okay, I have updated label from |
ably/options.go
Outdated
| // isEndpointFQDN handles an endpoint that uses a hostname, which may be an IPv4 | ||
| // address, IPv6 address or localhost |
There was a problem hiding this comment.
| // isEndpointFQDN handles an endpoint that uses a hostname, which may be an IPv4 | |
| // address, IPv6 address or localhost | |
| // isEndpointFQDN returns true if the given endpoint is a hostname, which may be an IPv4 | |
| // address, IPv6 address or localhost |
ably/options.go
Outdated
| } | ||
| } | ||
|
|
||
| // WithEndpoint sets a custom endpoint for connecting to the Ably service (see |
There was a problem hiding this comment.
| // WithEndpoint sets a custom endpoint for connecting to the Ably service (see | |
| // WithEnvironment sets a custom endpoint for connecting to the Ably service (see |
…as default connection option (ADR-119)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ably/realtime_client_integration_test.go (1)
270-270: Update test assertions to use constants.Consider extracting the hardcoded host strings into constants to maintain consistency and make future updates easier.
+const ( + defaultPrimaryHost = "main.realtime.ably.net" +) -assert.Equal(t, "main.realtime.ably.net", visitedHosts[0]) +assert.Equal(t, defaultPrimaryHost, visitedHosts[0])Also applies to: 278-279
ably/options.go (2)
475-497: Consider simplifying the legacy support logic.The
endpointValueWithLegacySupportfunction could be simplified by using a switch statement for better readability.func (opts *clientOptions) endpointValueWithLegacySupport() string { - if !empty(opts.Endpoint) { - return opts.Endpoint - } - - if !empty(opts.Environment) { - if opts.Environment == "production" { - return defaultOptions.Endpoint - } - return opts.Environment - } - - if !empty(opts.RESTHost) { - return opts.RESTHost - } - - if !empty(opts.RealtimeHost) { - return opts.RealtimeHost - } - - return defaultOptions.Endpoint + switch { + case !empty(opts.Endpoint): + return opts.Endpoint + case !empty(opts.Environment): + if opts.Environment == "production" { + return defaultOptions.Endpoint + } + return opts.Environment + case !empty(opts.RESTHost): + return opts.RESTHost + case !empty(opts.RealtimeHost): + return opts.RealtimeHost + default: + return defaultOptions.Endpoint + } }
507-508: Consider updating the spec to standardize sandbox behavior.Based on the past review comments, the special handling of the "sandbox" endpoint should be specified in the SDK specification to ensure consistent behavior across all SDKs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ably/options.go(13 hunks)ably/realtime_client_integration_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration-test (1.22, msgpack)
- GitHub Check: integration-test (1.22, json)
- GitHub Check: integration-test (1.21, msgpack)
- GitHub Check: integration-test (1.21, json)
- GitHub Check: integration-test (1.20, msgpack)
- GitHub Check: integration-test (1.20, json)
- GitHub Check: integration-test (1.19, msgpack)
- GitHub Check: integration-test (1.19, json)
🔇 Additional comments (7)
ably/realtime_client_integration_test.go (3)
32-75: LGTM! Well-structured test separation for new and legacy options.The test clearly separates the validation of the new endpoint option (REC1b) from the legacy realtimeHost option (REC1d2), making it easier to maintain and understand the different connection methods.
79-158: LGTM! Comprehensive test coverage for agent headers.The test suite thoroughly validates the agent header behavior with both the new endpoint option and legacy options, including:
- Basic agent header format
- Custom agent with version
- Custom agent without version
Line range hint
383-387: Consider adding test coverage for non-sandbox endpoints.The test only verifies fallback hosts for the "sandbox" endpoint. Consider adding test cases for other endpoint types like "nonprod:xyz" and custom routing policies.
Run this script to check current test coverage:
ably/options.go (4)
26-28: LGTM! Clear default endpoint definition.The
defaultEndpointconstant and its initialization indefaultOptionsestablish a clear default routing policy.Also applies to: 43-43
69-77: LGTM! Well-implemented endpoint fallback logic.The
getEndpointFallbackHostsfunction correctly handles different endpoint formats:
- Sandbox environment
- Non-production environments (nonprod:*)
- Custom routing policies
432-437: LGTM! Proper validation of mutually exclusive options.The validation logic correctly enforces that the endpoint option cannot be used with legacy options (environment, realtimeHost, restHost) as per REC1c1.
1118-1126: LGTM! Clear documentation for new and deprecated options.The documentation for both
WithEndpointandWithEnvironmentclearly indicates their purpose and deprecation status, with appropriate links to platform customization docs.Also applies to: 1128-1134
Summary
This implements ADR-119, which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042.
The endpoint may be one of the following:
main)nonprod:sandbox)foo.example.comThe 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.netdomain when the package is upgraded.Questions
Since this is my first stab at making changes to an SDK, I require a little guidance:
Summary by CodeRabbit
New Features
Bug Fixes
Tests