[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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ably/options_test.go (4)
16-26: Add specification annotations to improve traceabilityConsider adding comments referencing the relevant specification items to enhance clarity and maintainability. For example:
func TestDefaultFallbacks_REC2c(t *testing.T) { // REC2c: Verify default fallback hosts for the 'main' endpoint expectedFallBackHosts := []string{ "main.a.fallback.ably-realtime.com", "main.b.fallback.ably-realtime.com", "main.c.fallback.ably-realtime.com", "main.d.fallback.ably-realtime.com", "main.e.fallback.ably-realtime.com", } hosts := ably.DefaultFallbackHosts() assert.Equal(t, expectedFallBackHosts, hosts) }
28-63: Annotate test cases with specification referencesTo align with reviewer recommendations and improve code readability, please add comments indicating compliance with specific specifications. For instance:
t.Run("standard endpoint", func(t *testing.T) { // REC2c: Verify fallback hosts for a standard endpoint expectedFallBackHosts := []string{ "acme.a.fallback.ably-realtime.com", "acme.b.fallback.ably-realtime.com", "acme.c.fallback.ably-realtime.com", "acme.d.fallback.ably-realtime.com", "acme.e.fallback.ably-realtime.com", } hosts := ably.GetEndpointFallbackHosts("acme") assert.Equal(t, expectedFallBackHosts, hosts) })
Line range hint
71-105: Enhance test clarity with spec annotationsAdding specification references to the
TestHosts_REC1test cases can improve understanding and ensure alignment with the documentation. For example:t.Run("REC1a with default options", func(t *testing.T) { clientOptions := ably.NewClientOptions() // REC1a: Default endpoint should be 'main.realtime.ably.net' assert.Equal(t, "main.realtime.ably.net", clientOptions.GetEndpoint()) // Additional assertions... })
294-318: Include specification references in invalid combinations testTo provide context and align tests with specifications, consider annotating invalid combination tests with relevant spec items:
t.Run("must return error on invalid combinations", func(t *testing.T) { // REC1b1: Invalid to use endpoint with environment and host options _, err := ably.NewREST([]ably.ClientOption{ ably.WithEndpoint("acme"), ably.WithEnvironment("acme"), ably.WithRealtimeHost("foo.example.com"), ably.WithRESTHost("foo.example.com"), }...) assert.Error(t, err, "expected an error") // Additional test cases... })
📜 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(12 hunks)ably/options_test.go(6 hunks)ably/realtime_conn.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/realtime_conn.go
🔇 Additional comments (9)
ably/export_test.go (3)
15-16: FunctionGetEndpointFallbackHostsis correctly implementedThe function correctly exposes
getEndpointFallbackHostsfor testing purposes.
19-20: FunctionGetEndpointis correctly implementedThe method
GetEndpointaccurately provides access to the endpoint value.
191-193: FunctionEndpointFqdnis correctly implementedThe function properly checks if an endpoint is a fully qualified domain name.
ably/options_test.go (1)
444-449: Unit testTestEndpointFqdnis correctly implementedThe test accurately verifies the behavior of the
EndpointFqdnfunction with various inputs.ably/options.go (5)
66-67: Precompiled regular expression improves performanceGood use of precompiling the
nonprodRegexpat package initialization, which enhances performance by avoiding repeated compilation.
259-265: Well-documentedEndpointfield aligns with specificationsThe
Endpointfield is well-documented, detailing its usage and constraints according to the specifications REC1a and REC1b.
440-445: Proper validation of conflicting client optionsThe
validatefunction correctly enforces thatEndpointcannot be used with deprecated optionsEnvironment,RealtimeHost, orRESTHost, adhering to REC1b1.
477-481: FunctionendpointFqdncorrectly determines if an endpoint is an FQDNThe
endpointFqdnfunction accurately checks whether the endpoint is a fully qualified domain name, in line with the specified logic.
1126-1138: FunctionWithEndpointis correctly implementedThe
WithEndpointfunction properly sets theEndpointoption, with thorough documentation aligning with REC1a and REC1b.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ably/realtime_client_integration_test.go (2)
33-53: LGTM! Well-structured test for the new endpoint option.The test case properly validates the endpoint functionality with clear REC1b annotation. The test structure follows best practices with proper cleanup.
Consider adding a brief comment explaining what REC1b represents from the specification for better maintainability.
79-243: Consider refactoring to reduce test duplication.The test cases for endpoint and legacy options have significant code duplication. Consider extracting the common test logic into a helper function that takes the client options as parameters.
Example refactor:
+func testAblyAgentHeader(t *testing.T, opts ...ably.ClientOption) { + 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) + })) + defer server.Close() + serverURL, err := url.Parse(server.URL) + assert.NoError(t, err) + + // Add server URL to the provided options + opts = append(opts, + ably.WithTLS(false), + ably.WithToken("fake:token"), + ably.WithUseTokenAuth(true)) + + client, err := ably.NewRealtime(opts...) + assert.NoError(t, err) + defer client.Close() + + expectedAgentHeaderValue := ably.AblySDKIdentifier + " " + ably.GoRuntimeIdentifier + " " + ably.GoOSIdentifier() + ablytest.Wait(ablytest.ConnWaiter(client, nil, ably.ConnectionEventDisconnected), nil) + + assert.Equal(t, expectedAgentHeaderValue, agentHeaderValue) +}Then use it in the test cases:
t.Run("using endpoint option", func(t *testing.T) { t.Run("RSC7d3", func(t *testing.T) { testAblyAgentHeader(t, ably.WithEndpoint(serverURL.Host)) }) // ... other test cases })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ably/realtime_client_integration_test.go(4 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 (2)
ably/realtime_client_integration_test.go (2)
Line range hint
1-11: LGTM! Comprehensive test coverage for endpoint functionality.The test file provides thorough coverage of the new endpoint functionality while maintaining backward compatibility with legacy options. The tests are well-structured with proper cleanup and clear assertions.
Line range hint
386-390: Verify the implementation of GetEndpointFallbackHosts.The test uses a new function
GetEndpointFallbackHoststo retrieve fallback hosts. Let's verify its implementation.✅ Verification successful
GetEndpointFallbackHosts implementation is correct and well-tested
The function correctly generates fallback hosts based on the endpoint type:
- For "sandbox" endpoint: uses "ably-realtime-nonprod.com" domain
- For "nonprod:*" endpoints: uses "ably-realtime-nonprod.com" with the specified namespace
- For other endpoints: uses "ably-realtime.com" domain
The implementation generates 5 fallback hosts (a-e) following the pattern: "{namespace}.{letter}.fallback.{domain}" and is properly tested across different scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of GetEndpointFallbackHosts # Search for the function definition ast-grep --pattern 'func GetEndpointFallbackHosts($endpoint string) []string { $$$ }' # Search for any tests specific to this function rg -l "TestGetEndpointFallbackHosts"Length of output: 126
Script:
#!/bin/bash # Try different search patterns to locate the function ast-grep --pattern 'func $_ GetEndpointFallbackHosts($_) []string' # Search for any mentions of fallback hosts in the codebase rg -A 3 "fallbackHosts.*=.*" # Look for the package definition to understand the context rg -l "package ably"Length of output: 6893
Script:
#!/bin/bash # Search for the implementation in options.go cat ably/options.go # Search for any fallback-related constants or variables rg -A 3 "const.*[Ff]allback|var.*[Ff]allback" # Search for any environment-specific endpoint logic rg -A 5 "func.*[Ee]ndpoint"Length of output: 63975
| @@ -13,52 +13,65 @@ import ( | |||
| "github.com/stretchr/testify/assert" | |||
| ) | |||
|
|
|||
| func TestDefaultFallbacks_RSC15h(t *testing.T) { | |||
| func TestDefaultFallbacks_REC2c(t *testing.T) { | |||
There was a problem hiding this comment.
I don't think we should override existing tests for deprecated clientOptions, instead they should be updated.
This is to make sure, deprecated features also work as expected.
Can you instead append new tests at the end of the test file and mark old tests as deprecated.
This is to make sure both old and new clientOptions work as expected.
In the future, when we decide to remove deprecated clientOptions, we will remove both deprecated clientOptions and related tests.
There was a problem hiding this comment.
Unfortunately, the official ably doc for clientOptions and current README still recommends use of old clientOptions.
i.e. environment
There was a problem hiding this comment.
Those docs won't be updated until we update all SDKs to use endpoint instead of environment
There was a problem hiding this comment.
Can you instead append new tests at the end of the test file and mark old tests as deprecated.
This is to make sure both old and new clientOptions work as expected.
The changes here will change the behavour of the legacy clientOptions, so keeping the old tests as they were will mean they will fail. How should we handle this?
Unfortunately, the official ably doc for clientOptions and current README still recommends use of old clientOptions.
I can update the current README. For the SDK docs, this is the first client we are making this change on, and I am not an SDK developer, so will probably need some guidance and support for the process in updating docs, releasing new versions etc.
There was a problem hiding this comment.
Ideally, newly introduced clientOptions shouldn't break deprecated clientOptions.
Does new spec introduces breaking changes for deprecated clientOptions?
I would recommend to update the code in a way that, if endpoint is not specified, hosts will be calculated based on deprecated clientOptions
There was a problem hiding this comment.
See the discussion in the ADR about updating the current behaviour of the clientOptions.
Also, not sure if we have internal priorities for updating clientOptions across SDKs.
Yes, I guess that's why an SRE is doing this work rather than an SDK developer ;)
There was a problem hiding this comment.
Okay, ADR is still in progress 🤔, and seems decided to go with breaking option 2.
Do we have the spec updated accordingly since ADR is still in progress?
If implemented with option2 ( also make sure we have spec updated accordingly ), we also need to update UPDATING.md and will mark a major release. i.e. 1.3.0
There was a problem hiding this comment.
The spec you linked to before does appear to be updated, yes. It describes the breaking change behaviour (that environment will become the routing policy name).
If implemented with option2 ( also make sure we have spec updated accordingly ), we also need to update UPDATING.md and will mark a major release. i.e. 1.3.0
👍 Is 1.3.0 a major release? I would have thought that would be 2.0.0. Or is there some weird Ably context I'm not aware of 😅
There was a problem hiding this comment.
Or instead of updating UPDATING.md, you should add section right above
breaking-api-changes-in-version-12x named as Breaking API Changes in Version 1.3.x and specify the given endpoint usage with respect to deprecation clientOptions, since we don't have much stuff to add to UPDATING.md
There was a problem hiding this comment.
Having discussed this with @paddybyers and @SimonWoolf, we don't necessarily think this is a breaking change. The tests do need to change because the returned hostnames will change, but it's not actually causing any impact to the user.
The only thing that might cause a conflict is if both WithEnvironment and WithRealtimeHost or WithRESTHost are used together which previously was just ignored, but now will throw an error. I've updated the integration tests accordingly to use WithEndpoint.
With this in mind, do we still mark this at 1.3.0? We don't actually specify clientOptions anywhere in docs as far as I can see, so not sure what I need to update, other than CHANGELOG.md.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ably/realtime_conn_spec_integration_test.go (4)
225-228: Ensure test cases are annotated for specification complianceConsider adding inline comments to the test options, such as
// REC1a, to indicate compliance with the specifications outlined in ADR-119. This enhances traceability and understanding of the test's purpose.
1738-1739: Include specification annotations in test initializationTo improve clarity and maintainability, add comments next to
ably.WithEndpoint(ablytest.Endpoint)indicating the relevant specification, such as// REC1a, as suggested in the PR comments.
1801-1802: Add inline comments for specification adherenceAdding comments like
// REC1anext to theably.WithEndpoint(ablytest.Endpoint)option can help developers quickly understand the test's alignment with specific requirements.
3040-3041: Enhance test clarity with specification referencesInclude inline comments such as
// REC1anext toably.WithEndpoint(ablytest.Endpoint)to indicate compliance with the required specifications. This practice aids in future maintenance and review processes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ably/auth_integration_test.go(4 hunks)ably/realtime_conn_spec_integration_test.go(4 hunks)ably/rest_channel_integration_test.go(3 hunks)ably/rest_client_integration_test.go(4 hunks)ablytest/sandbox.go(5 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 (12)
ablytest/sandbox.go (3)
103-108: Verify initialization of newEndpointfield inSandboxstructThe addition of the
Endpointfield is appropriate. Ensure that all instances ofSandboxare updated to initialize this new field to prevent potentialnilreference errors.
141-141: Confirm correct usage ofEndpointandEnvironmentvariablesVerify that the
EndpointandEnvironmentvariables passed toNewSandboxWithEndpointare correctly defined and initialized. Incorrect values may lead to unexpected behaviors or failed connections.
243-243: Update client options withEndpointconfigurationThe inclusion of
ably.WithEndpoint(app.Endpoint)in the client options ensures that the client connects to the correct endpoint. This update aligns with the new configuration requirements.ably/rest_channel_integration_test.go (2)
145-145: Update toNewSandboxWithEndpointin test setupThe change to use
ablytest.NewSandboxWithEndpointreflects the updated configuration. Ensure thatablytest.Endpointandablytest.Environmentare correctly set for the test environment.
298-298: Consistent use ofNewSandboxWithEndpointacross testsUsing
ablytest.NewSandboxWithEndpointin test initialization maintains consistency with the new endpoint configuration and ensures tests run against the correct target.ably/rest_client_integration_test.go (3)
474-476: LGTM! Endpoint configuration update.The test case correctly implements the endpoint-based configuration using
ably.WithEndpoint("main")as specified in ADR-119. The test verifies that the client falls back to alternative hosts when the primary host is unavailable.
517-520: LGTM! Test configuration update.The test case correctly uses
ably.WithEndpoint(ablytest.Endpoint)and includes appropriate fallback hosts for testing the host fallback functionality.
351-351: Verify the primary host assertion.The test now expects
main.realtime.ably.netas the primary host. This change aligns with ADR-119 which specifies the new endpoint options including themainrouting policy name.Run the following script to verify the host configuration:
✅ Verification successful
Primary host assertion is correctly implemented
The assertion of
main.realtime.ably.netas the primary host in this test is verified to be correct. The test is part of the RSC15 (Host Fallback) specification implementation, specifically testing the fallback mechanism when the primary host fails. The test validates:
- Proper retry count (1 primary + 5 fallback attempts)
- Correct fallback host sequence
- Uniqueness of selected fallback hosts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the primary host configuration in the codebase # Test: Search for host configuration patterns to ensure consistency # Look for other occurrences of the old and new host patterns echo "Searching for old host pattern..." rg "rest\.ably\.io" echo "Searching for new host pattern..." rg "main\.realtime\.ably\.net"Length of output: 1567
Script:
#!/bin/bash # Search for ADR-119 references and endpoint configuration patterns echo "Searching for ADR-119 references..." rg -i "ADR-119|ADR119" echo "Searching for endpoint configuration patterns..." rg -B2 -A2 "func.*GetEndpoint|const.*Host|var.*Host" echo "Searching for routing policy documentation..." fd "README|CHANGELOG" --exec rg -l "routing|endpoint|host"Length of output: 1602
Script:
#!/bin/bash # Check documentation for endpoint configuration details echo "Checking README for endpoint information..." cat README.md | rg -A5 -B5 "endpoint|host|rest\.ably\.io|realtime\.ably\.net" echo "Checking CHANGELOG for recent endpoint changes..." cat CHANGELOG.md | rg -A5 -B5 "endpoint|host|rest\.ably\.io|realtime\.ably\.net" # Check the specific test context echo "Checking the specific test context..." rg -B10 -A10 "main\.realtime\.ably\.net" ably/rest_client_integration_test.goLength of output: 6807
ably/auth_integration_test.go (4)
392-394: LGTM! JWT token test configuration.The test case correctly uses
ably.WithEndpoint(app.Endpoint)for configuring the REST client in JWT token tests.
417-419: LGTM! AuthURL test configuration.The test case correctly uses
ably.WithEndpoint(app.Endpoint)for testing authentication using AuthURL.
460-463: LGTM! AuthCallback test configuration.The test case correctly uses
ably.WithEndpoint(app.Endpoint)for testing authentication using AuthCallback.
488-491: LGTM! Invalid JWT test configuration.The test case correctly uses
ably.WithEndpoint(app.Endpoint)for testing error handling with invalid JWT tokens.
| var nonprodRegexp = regexp.MustCompile(`^nonprod:(.*)$`) | ||
|
|
||
| func (app *Sandbox) URL(paths ...string) string { | ||
| return "https://" + app.Environment + "-rest.ably.io/" + path.Join(paths...) | ||
| if match := nonprodRegexp.FindStringSubmatch(app.Endpoint); match != nil { | ||
| return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", match[1], path.Join(paths...)) | ||
| } | ||
|
|
||
| return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) |
There was a problem hiding this comment.
Ensure safe and robust endpoint URL construction
The use of nonprodRegexp to parse the Endpoint value and construct the URL is reasonable. However, consider adding validation for the Endpoint value to handle unexpected formats or malicious inputs, preventing potential URL injection vulnerabilities.
Apply this diff to implement input validation:
func (app *Sandbox) URL(paths ...string) string {
+ if app.Endpoint == "" {
+ log.Println("Endpoint is empty")
+ return ""
+ }
if match := nonprodRegexp.FindStringSubmatch(app.Endpoint); match != nil {
return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", match[1], path.Join(paths...))
}
return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...))
}📝 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.
| var nonprodRegexp = regexp.MustCompile(`^nonprod:(.*)$`) | |
| func (app *Sandbox) URL(paths ...string) string { | |
| return "https://" + app.Environment + "-rest.ably.io/" + path.Join(paths...) | |
| if match := nonprodRegexp.FindStringSubmatch(app.Endpoint); match != nil { | |
| return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", match[1], path.Join(paths...)) | |
| } | |
| return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) | |
| var nonprodRegexp = regexp.MustCompile(`^nonprod:(.*)$`) | |
| func (app *Sandbox) URL(paths ...string) string { | |
| if app.Endpoint == "" { | |
| log.Println("Endpoint is empty") | |
| return "" | |
| } | |
| if match := nonprodRegexp.FindStringSubmatch(app.Endpoint); match != nil { | |
| return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", match[1], path.Join(paths...)) | |
| } | |
| return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) | |
| } |
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