[ClientOption] Add support for endpoint according to new routing policy #679
Conversation
WalkthroughThis pull request introduces a comprehensive modification to the Ably SDK's configuration approach, transitioning from environment-based and host-specific settings to a more unified endpoint-based configuration. The changes span multiple files, replacing methods like Changes
Possibly related PRs
Suggested reviewers
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/679/features
github-actions
bot
temporarily deployed
to
staging/pull/679/features
github-actions
bot
temporarily deployed
to
staging/pull/679/features
github-actions
bot
temporarily deployed
to
staging/pull/679/features
sacOO7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ably/realtime_conn.go (1)
Line range hint
391-420: LGTM! Robust implementation of fallback host handling.The implementation correctly follows the RTN17 specification requirements:
- Attempts primary host first
- Implements proper fallback logic with internet connectivity check
- Manages active host state correctly
- Includes proper error handling and state management
However, there's one potential improvement to consider:
Consider adding debug logging for fallback attempts to aid in troubleshooting. For example:
for hostCounter, host := range hosts { + c.log().Debugf("Attempting connection to host %s (%d/%d)", host, hostCounter+1, len(hosts)) u, err := url.Parse(c.opts.realtimeURL(host)) if err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ably/ably_test.go(1 hunks)ably/auth_integration_test.go(4 hunks)ably/error_test.go(2 hunks)ably/export_test.go(3 hunks)ably/http_paginated_response_integration_test.go(1 hunks)ably/options.go(14 hunks)ably/options_test.go(6 hunks)ably/realtime_client_integration_test.go(8 hunks)ably/realtime_conn.go(1 hunks)ably/realtime_conn_spec_integration_test.go(4 hunks)ably/realtime_presence_integration_test.go(1 hunks)ably/rest_channel_integration_test.go(3 hunks)ably/rest_client_integration_test.go(11 hunks)ablytest/ablytest.go(2 hunks)ablytest/sandbox.go(6 hunks)
🔇 Additional comments (78)
ably/options_test.go (21)
16-22: Validate naming convention for fallback hosts.
This test checks that the default fallback hosts for the “main” endpoint follow the updated naming convention. The lookup matches “main.a.fallback.ably-realtime.com” and so forth. Implementation looks correct.
28-39: Thorough test coverage of standard endpoint fallback logic.
All added lines confirm that the “acme” endpoint yields expected fallback hosts ending with “ably-realtime.com.” The test accurately validates the return value ofably.GetEndpointFallbackHosts("acme").
41-51: Sandbox endpoint fallback hosts are correctly checked.
This test ensures that thenonprod-prefixed endpoint "sandbox" yields fallback hosts with the “ably-realtime-nonprod.com” domain. Nicely structured sub-test logic.
53-63: Nonprod endpoint fallback logic.
Likewise,"nonprod:acme"is validated to produce fallback hosts on “ably-realtime-nonprod.com.” No issues found.
71-74: REC1a default options test.
Verifies that without overrides,clientOptions.GetHostnameFromEndpoint()returns"main.realtime.ably.net". This is the correct new default.
83-91: Custom routing policy name test (REC1b).
Ensuresably.WithEndpoint("acme")sets the primary host as"acme.realtime.ably.net"and fallback hosts are generated byably.GetEndpointFallbackHosts("acme"). The assertion aligns with expected domain naming.
94-102: Nonprod routing policy name test (REC1b3).
Checksably.WithEndpoint("nonprod:acme")yields “acme.realtime.ably-nonprod.net” and default fallback hosts fromably.GetEndpointFallbackHosts("nonprod:acme"). Everything looks correct.
105-113: FQDN endpoint with no fallback hosts (REC1b2).
This test confirms that if an endpoint is a fully qualified domain name (foo.example.com), fallback hosts arenil. Appropriately covers the FQDN logic.
117-126: FQDN endpoint with fallback hosts specified (REC1b2 & REC2a2).
Verifies that user-defined fallback hosts override the auto-generated list. This test is consistent and correct: “fallback.foo.example.com” is used explicitly.
129-139: Legacy production environment test (REC1c).
Ensures settingWithEnvironment("production")yields"main.realtime.ably.net"as the host and uses default fallback hosts. Solid coverage for deprecated environment usage.
141-150: Legacy custom environment test.
Validates that specifyingWithEnvironment("acme")results in a primary REST host of"acme.realtime.ably.net"and fallback hosts from “acme.” Behavior is correct, though environment usage is deprecated.
152-161: Custom environment + fallbackHostsUseDefault (REC1c REC2a1).
Checks that fallback hosts become the library’s default, ignoring the environment’s custom fallback pattern. Good coverage of the deprecated combination.
163-172: Custom REST host test (REC1d1).
Ensures specifyingably.WithRESTHost("test.org")sets the REST host and keeps fallback hostsnil. No issues found.
174-183: Custom Realtime host test (REC1d2).
Likewise,ably.WithRealtimeHost("ws.test.org")sets the realtime host and discards fallback hosts. Matches the older approach for dev usage.
185-194: Simultaneous custom REST & Realtime host test (REC1d).
Confirms both are set individually without fallback hosts. Implementation matches expectations.
196-208: REC1d + REC2b with fallbackHostsUseDefault.
Tests that if you combine custom restHost/realtimeHost withWithFallbackHostsUseDefault(true), the fallback domain reverts to the default set. This logic is correct.
211-217: Error on mixing endpoint with fallbackHostsUseDefault.
VerifiesValidate()returns an error ifEndpointis combined with any deprecated host-based option. This enforces the new approach.
Line range hint
220-231: REC2a with fallbackHosts.
Specifically tests user-specified fallback hosts[a.example.com b.example.com]while default production environment is “main.realtime.ably.net.” The fallback usage is correct.
Line range hint
240-259: REC2a1 with fallbackHostsUseDefault and custom port.
Checks that settingWithTLSPort(8081)or a custom port invalidates the default fallback usage. Proper error handling is verified in the test.
277-301: Invalid combination checks for endpoint and legacy options.
Ensures that specifying environment, realtimeHost, restHost, etc., alongsideWithEndpoint(...)triggers errors. Thorough coverage of corner cases.
427-432: TestIsEndpointFQDN covers host validation logic.
Checks both “sandbox” (non-FQDN) and “sandbox.example.com” (FQDN), plus IP addresses/localhost. This confirms the logic behindably.IsEndpointFQDN(...).ably/realtime_client_integration_test.go (15)
32-53: REC1b check with endpoint option.
ReplacesWithRealtimeHostusage withWithEndpointfor test coverage. Confirms that each provided host (IPv4, localhost, IPv6) is used as the primary connection. This ensures new endpoint logic is correct.
55-75: REC1d2 check with legacy realtimeHost.
Retains coverage for the older option. Confirms identical logic for the same hosts. This backward compatibility test is valuable.
79-103: RSC7d3 : AblyAgent header with endpoint.
Ensures the correct agent header is sent when usingably.WithEndpoint(...). The test checks the “ablyAgent” header in the server’s response. No issues found.
104-130: RSC7d6 : Custom agent identifiers using endpoint.
Validates that user-defined “foo/1.2.3” merges with the default agent identifiers. This is consistent with the library’s approach to custom agent strings.
132-158: RSC7d6 : Missing version custom agent with endpoint.
Ensures that if an agent version is empty, the identifier is appended without “/”. The logic of constructing the final header string is correct and tested thoroughly.
161-184: RSC7d3 & RSC7d6 with legacy realtimeHost.
Similarly verifies the AblyAgent header for older usage, ensuring backward compatibility. No issues or regressions found.
186-240: Legacy custom agent tests with realtimeHost.
Same logic as above, verifying custom agent fields and missing version behavior. All consistent with new approach.
270-278: RTN17a & RTN17b fallback behavior with default primary host.
These lines confirm the first visited host is"main.realtime.ably.net"and subsequent fallback hosts are fromably.DefaultFallbackHosts(). Implementation is correct.
286-293: No fallback usage when a custom FQDN endpoint is provided (RTN17b).
Validates that specifyingably.WithEndpoint("custom-realtime.ably.io")yields a single host attempt with no fallback. This matches the intended logic.
Line range hint
294-301: No fallback usage with legacy custom realtimeHost (RTN17b).
Ensures that ifWithRealtimeHost("custom-realtime.ably.io")is set, fallback logic is skipped. This is consistent with the new approach.
305-312: User-provided fallback hosts scenario.
Checks that a custom fallback set, e.g.,[fallback0 fallback1 fallback2], is used after failing on “main.realtime.ably.net.” Confirmed correct.
Line range hint
312-323: fallbackHostsUseDefault + custom RealtimeHost forcibly merges fallback logic.
Ensures that even if a single custom realtimeHost is specified,WithFallbackHostsUseDefault(true)triggers the default fallback list. This is correct per spec.
369-369: Validation of fallback host usage in nonprod environment.
ably.GetEndpointFallbackHosts("nonprod:sandbox")is used to check the chosen fallback host matches the Realtime connection host. This test ensures synchronization between Realtime and REST fallback logic.
Line range hint
391-415: RTN17: Integration test with server returning Internal Server Error.
Confirms that after failing onserverURL.Host, the library tries the fallback host (sandbox-a-fallback.ably-realtime.com). The test ensures the correct fallback activation.
Line range hint
440-467: RTN17: Integration test with a timeout scenario.
First attempt times out onserverURL.Host, then connection proceeds with fallback host. The test verifies requestTimeout usage. Proper behavior is confirmed.ably/options.go (12)
26-29: New constants for default endpoint and primary host (REC1a).
IntroducesdefaultEndpoint = "main"anddefaultPrimaryHost = "main.realtime.ably.net". This aligns with the new naming pattern for production.
40-45: Initialize defaultOptions with updated fallback hosts.
Sets endpoint to “main,” restHost/realtimeHost to “main.realtime.ably.net,” and fallback hosts fromgetEndpointFallbackHosts(defaultEndpoint). This centralizes domain naming.
62-74: getPrimaryProdHost & getPrimaryNonProdHost.
Produces either “%s.realtime.ably.net” or “%s.realtime.ably-nonprod.net” from a root. Straightforward utility for endpoint-based hosts.
78-85: endpointFallbacks generator.
Returns fallback domains in the “a.fallback” through “e.fallback” pattern. Clear design to systematize fallback generation.
Line range hint
255-301: Endpoint & environment-related fields, plus deprecations.
DefinesEndpoint stringas the primary new approach, withEnvironment,RESTHost, andRealtimeHostmarked deprecated. The doc comments clearly state future removal.
431-436: Validation logic to prevent mixing endpoint with deprecated options.
validate()raises an error ifopts.Endpointis used along withopts.Environment,opts.RealtimeHost, or fallback defaults. This ensures a consistent approach.
557-565: Fallback host resolution with a custom endpoint.
If the endpoint is an FQDN, fallback hosts remainnil; otherwise, fallback hosts come fromgetEndpointFallbackHosts(...). This properly handles advanced routing.
586-586: Non-production environment fallback generation.
Fallbacks for a custom environment are derived by callinggetEndpointFallbackHosts(opts.Environment). This preserves older behavior for non-default environment usage.
1131-1140: WithEndpoint(...)
Introduces a modern approach to define a custom endpoint name or FQDN, clarifying in doc comments that it supersedes older environment/host fields.
1141-1147: WithEnvironment(...) is now deprecated.
Warns about future removal, referencing new “endpoint” usage. This keeps backward compatibility for older integrations.
1205-1207: WithRESTHost(...) is clearly deprecated.
Documented as dev/test-only, scheduled for removal. Implementation remains functional for legacy usage.
1412-1412: Reset default fields in applyOptionsWithDefaults.
Zeros outEndpoint,RESTHost,RealtimeHost, andFallbackHostsso that user-specified values can be set. Ensures a fresh state for each new client.ably/realtime_conn_spec_integration_test.go (1)
225-225: Validate endpoint usage consistency.
Replacing the deprecated environment option withably.WithEndpoint(ablytest.Endpoint)aligns with the new endpoint-based configuration approach. Ensure that any remaining references to environment-based options are removed or updated accordingly to avoid potential configuration clashes.✅ Verification successful
Endpoint usage is consistent and properly managed ✅
The deprecatedWithEnvironmentis only used in test files to verify legacy support and validate error cases when mixing old and new configuration approaches. The deprecation is properly documented, and the codebase correctly enforces clean separation between the approaches.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no leftover references to environment-based options across the codebase. rg "WithEnvironment" -A 3 -B 3Length of output: 3396
ably/http_paginated_response_integration_test.go (1)
24-24: Confirm fallback endpoint behavior.
Usingably.WithEndpoint("ably.invalid")as a fallback test endpoint is a valid approach. Just confirm it correctly triggers fallback logic in all relevant tests and doesn't inadvertently break deeper client fallback pathways.ablytest/ablytest.go (2)
20-20: Adopt “Endpoint” variable with caution.
DefiningEndpoint = "nonprod:sandbox"replaces the previous environment-centric approach. Verify that references to any old environment variables (likeABLY_ENV) are fully transitioned out for consistency and clarity.
43-44: Check environment variable reading logic.
ReadingABLY_ENDPOINTfor theEndpointvariable is sensible. If you wish to maintain backward compatibility withABLY_ENV, consider either gracefully handling it or updating all references to ensure no surprise behavior changes when setting up the tests.ably/error_test.go (1)
57-57: LGTM! Configuration method updated correctly.The changes correctly migrate from
WithRESTHosttoWithEndpointwhile maintaining the same test behavior.Also applies to: 137-137
ably/export_test.go (4)
15-16: LGTM! Function renamed to reflect endpoint-based configuration.The function has been correctly renamed from
GetEnvFallbackHoststoGetEndpointFallbackHoststo align with the new endpoint-based configuration approach.
27-33: LGTM! New validation methods added.The new methods
ValidateandGetHostnameFromEndpointprovide necessary access to internal validation logic for testing purposes.
203-205: LGTM! New FQDN validation method added.The new
IsEndpointFQDNmethod provides necessary access to internal FQDN validation logic for testing purposes.
214-214: LGTM! Simplified default fallback hosts retrieval.The change to use
defaultOptions.FallbackHostsdirectly simplifies the code while maintaining the same functionality.ably/realtime_presence_integration_test.go (1)
60-60: Verify the reason for skipping this test.The test has been renamed to be skipped. According to the PR objectives, this will be addressed as part of issue #676.
Run the following script to verify the issue:
✅ Verification successful
Test skip is correctly linked to issue #676
The test is temporarily skipped due to documented rate limiting issues (permitted rate: 250, current rate: 300) that are being tracked in issue #676. The skip is justified while the rate limiting problem is being addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify issue #676 exists and is related to this test. gh issue view 676 --json title,body,stateLength of output: 1411
ablytest/sandbox.go (3)
103-106: LGTM! Updated struct to use endpoint-based configuration.The
Sandboxstruct has been correctly updated to useEndpointinstead ofEnvironment, with clear documentation.
259-264: LGTM! Improved URL construction with nonprod support.The URL construction logic now correctly handles both production and non-production endpoints, with proper prefix handling for "nonprod:" endpoints.
281-281: LGTM! Updated JWT auth params to use endpoint.The JWT authentication parameters have been correctly updated to use
endpointinstead ofenvironment.ably/rest_channel_integration_test.go (2)
145-145: LGTM! Consistent with the endpoint-based configuration approach.The change from
NewSandboxWithEnvtoNewSandboxWithEndpointaligns with the PR's objective of transitioning to endpoint-based configuration.
298-298: LGTM! Consistent with the endpoint-based configuration approach.The change from
NewSandboxWithEnvtoNewSandboxWithEndpointin the retry test maintains consistency with the new endpoint-based configuration.ably/ably_test.go (1)
359-359: LGTM! Consistent with the endpoint-based configuration approach.The change from
WithRealtimeHosttoWithEndpointin theHostRecorder.Optionsmethod aligns with the PR's objective of transitioning to endpoint-based configuration.ably/rest_client_integration_test.go (8)
253-255: LGTM! Consistent endpoint configuration in RSC7d2 test.The test correctly uses
WithEndpointfor configuring the client, maintaining consistency with the new approach.
Line range hint
277-283: LGTM! Consistent endpoint configuration in RSC7d6 test.The test correctly uses
WithEndpointalong with custom agent configuration.
Line range hint
304-310: LGTM! Consistent endpoint configuration in RSC7d6 test with missing version.The test correctly uses
WithEndpointalong with custom agent configuration without version.
350-353: LGTM! Updated test assertions for RSC15 host fallback test.The test assertions have been correctly updated to reflect the new endpoint-based configuration:
- Expected retry count remains at 6 (1 primary + 5 fallback hosts)
- Primary host updated to "sandbox.realtime.ably-nonprod.net"
- Fallback host assertions maintained
396-399: LGTM! Updated test assertions for RSC15l2 timeout test.The test assertions have been correctly updated to match the RSC15 host fallback test, maintaining consistency.
Line range hint
404-411: LGTM! Consistent endpoint configuration in RSC15l1 test.The test correctly uses
WithEndpointfor configuring the client with fallback hosts.
428-430: LGTM! Consistent endpoint configuration across RSC15b test cases.All test cases have been updated to use
WithEndpointconsistently, maintaining the same test coverage and functionality.Also applies to: 442-445, 458-461, 474-477, 490-493
Line range hint
517-521: LGTM! Consistent endpoint configuration in host fallback test.The test correctly uses
WithEndpointalong with fallback host configuration.ably/auth_integration_test.go (4)
392-394: LGTM! Consistent endpoint configuration in JWT token test.The test correctly uses
WithEndpointfor configuring the REST client when testing JWT token usage.
417-419: LGTM! Consistent endpoint configuration in RSA8g, RSA3d test.The test correctly uses
WithEndpointwhen testing authentication using authURL.
460-463: LGTM! Consistent endpoint configuration in RSA8g, RSA3d test with authCallback.The test correctly uses
WithEndpointwhen testing authentication using authCallback.
488-491: LGTM! Consistent endpoint configuration in RSA4e, RSA4b test.The test correctly uses
WithEndpointwhen testing invalid JWT error cases.ably/realtime_conn.go (2)
Line range hint
391-420: Ensure test coverage for fallback scenarios.The fallback host handling logic introduces several new code paths that should be thoroughly tested:
- Successful fallback scenarios
- Internet connectivity check behavior
- Host state management
Let's verify the test coverage:
✅ Verification successful
Test coverage for fallback scenarios is comprehensive.
The codebase includes thorough test coverage for:
- Host fallback scenarios (RTN17)
- Internet connectivity checks (RTN17c)
- Error conditions (timeouts, DNS errors, server errors)
- Fallback host configuration and management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for fallback-related code # Search for fallback-related tests echo "Checking fallback-related tests..." rg -A 10 "Test.*Fallback" --type go # Search for connection-related tests echo "Checking connection-related tests..." rg -A 10 "Test.*Connection" --type goLength of output: 12335
Line range hint
391-420: Verify error handling for edge cases.Please ensure the following edge cases are handled correctly:
- DNS resolution failures
- Network timeouts during fallback attempts
- Rate limiting scenarios
Let's verify the error handling implementation:
✅ Verification successful
Error handling implementation verified successfully
The codebase implements robust error handling for all specified edge cases:
- DNS failures are handled through fallback hosts mechanism with proper host rotation
- Network timeouts use configurable timeouts with exponential backoff
- Rate limiting implements proper retry mechanism with fallback logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for error handling patterns in connection-related code # Check error handling patterns echo "Checking error handling patterns..." rg -A 5 "if err != nil" --type go # Check timeout handling echo "Checking timeout handling..." rg -A 5 "context.*WithTimeout|time.*After" --type go # Check rate limiting handling echo "Checking rate limiting response handling..." rg -A 5 "429|Too Many Requests" --type goLength of output: 67809
Script:
#!/bin/bash # Search for timeout and network error handling patterns echo "Checking timeout configurations..." rg -A 5 "realtimeRequestTimeout|requestTimeout" --type go echo "Checking DNS and network error handling..." rg -A 5 "net.Error|syscall.ECONNRESET|context.DeadlineExceeded" --type go echo "Checking rate limit and retry handling..." rg -A 5 "StatusTooManyRequests|retryCount|backoff" --type goLength of output: 29124
github-actions
bot
temporarily deployed
to
staging/pull/679/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ably/options.go (4)
26-29: LGTM! Consider enhancing the documentation.The new constants and default options are well-structured for the new routing policy. The changes align with the PR objectives.
Consider adding more detailed documentation for the new constants:
// defaultEndpoint is the default routing policy used to connect to Ably +// It represents the primary routing namespace for production environments. defaultEndpoint = "main" +// defaultPrimaryHost is the default production host as per REC1a specification defaultPrimaryHost = "main.realtime.ably.net" // REC1aAlso applies to: 40-42
62-85: LGTM! Consider adding input validation.The endpoint-related functions are well-structured and implement the routing policy requirements effectively.
Consider adding input validation to prevent potential issues with malformed input:
func getPrimaryProdHost(root string) string { + if empty(root) { + return defaultPrimaryHost + } return fmt.Sprintf("%s.realtime.ably.net", root) } func getPrimaryNonProdHost(root string) string { + if empty(root) { + return defaultPrimaryHost + } return fmt.Sprintf("%s.realtime.ably-nonprod.net", root) }
Line range hint
255-278: LGTM! Consider adding version information to deprecation notices.The client options struct changes are well-documented with clear deprecation notices.
Consider adding version information to the deprecation notices to help users plan their migration:
-// Deprecated: this property is deprecated and will be removed in a future version. +// Deprecated: this property is deprecated and will be removed in v2.0.0. // If the restHost option is specified the primary domain is the value of the restHost option REC1d1).
Line range hint
1143-1231: Consider adding version information to deprecated options.The deprecation notices are clear, but adding version information would help users plan their migration.
Apply this pattern to all deprecated options:
-// Deprecated: this option is deprecated and will be removed in a future version. +// Deprecated: this option is deprecated and will be removed in v2.0.0. Use WithEndpoint instead.ablytest/sandbox.go (1)
278-278: Update function documentation to reflect endpoint parameter.The change from environment to endpoint parameter is correct, but the function's documentation should be updated to mention the endpoint parameter in the authentication parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ably/options.go(14 hunks)ablytest/sandbox.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 (8)
ably/options.go (4)
431-437: LGTM! Validation logic is thorough and well-implemented.The validation prevents mixing of new endpoint option with deprecated options, with clear error messages and proper logging.
Line range hint
475-526: LGTM! Host resolution is well-implemented.The host resolution methods correctly implement the routing policy specification with proper fallbacks and clear separation of concerns.
1133-1141: LGTM! WithEndpoint is well-implemented and documented.The new option follows the established pattern and includes comprehensive documentation.
1414-1417: LGTM! Options initialization is clean and safe.The initialization properly clears all host-related fields to prevent unintended option mixing.
ablytest/sandbox.go (4)
103-105: LGTM! Clean transition from Environment to Endpoint field.The struct changes align well with the new endpoint-based configuration approach.
135-135: LGTM! Constructor changes maintain backward compatibility.The transition to endpoint-based configuration is handled cleanly while preserving the existing initialization logic and error handling.
Also applies to: 138-142
256-261: Add validation for empty endpoint.While the URL construction logic for both production and non-production environments is sound, consider adding validation to handle the case where
app.Endpointis empty.Consider adding:
func (app *Sandbox) URL(paths ...string) string { + if app.Endpoint == "" { + panic("Endpoint cannot be empty") + } if strings.HasPrefix(app.Endpoint, "nonprod:") {
Line range hint
1-350: Verify complete transition from environment to endpoint.Let's ensure all references to the old environment-based configuration have been updated across the codebase.
✅ Verification successful
Environment-related code is intentionally maintained for legacy support
The presence of Environment-related code is by design:
- WithEnvironment is documented and maintained for legacy support
- The code includes proper validation preventing mixed usage of Environment and Endpoint
- Test cases verify that invalid combinations (mixing old and new approaches) result in errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to Environment in the codebase echo "Checking for remaining Environment references..." rg -i "environment.*string" --type go echo "Checking for WithEnvironment usage..." rg "WithEnvironment" --type go echo "Checking for environment-related function names..." rg "WithEnv|NewSandboxWithEnv" --type goLength of output: 2702
Script:
#!/bin/bash # Check the implementation of WithEnvironment and related code echo "Checking WithEnvironment implementation..." ast-grep --pattern 'func WithEnvironment($_) ClientOption { $$$ }' echo "\nChecking ClientOptions struct and related code..." ast-grep --pattern 'type ClientOptions struct { $$$ }' echo "\nChecking for any migration or deprecation comments..." rg -B 2 -A 2 "WithEnvironment" --type goLength of output: 3565
sacOO7
force-pushed
the
feature/endpoint-option
branch
from
ec6802b to
69d2c0e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/679/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ablytest/sandbox.go (1)
256-261: Consider making string manipulation more robust.While the implementation works, consider these improvements:
- Add validation for empty endpoint
- Use constants for the "nonprod:" prefix and domain suffixes
- Consider using url.Parse for more robust URL construction
Example improvement:
+const ( + nonprodPrefix = "nonprod:" + prodDomain = "realtime.ably.net" + nonprodDomain = "realtime.ably-nonprod.net" +) + func (app *Sandbox) URL(paths ...string) string { - if strings.HasPrefix(app.Endpoint, "nonprod:") { - namespace := strings.TrimPrefix(app.Endpoint, "nonprod:") - return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", namespace, path.Join(paths...)) + if app.Endpoint == "" { + return fmt.Sprintf("https://%s/%s", prodDomain, path.Join(paths...)) } - - return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) + + endpoint := app.Endpoint + domain := prodDomain + if strings.HasPrefix(endpoint, nonprodPrefix) { + endpoint = strings.TrimPrefix(endpoint, nonprodPrefix) + domain = nonprodDomain + } + + return fmt.Sprintf("https://%s.%s/%s", endpoint, domain, path.Join(paths...)) }ably/options.go (1)
505-509: Consider enhancing FQDN validation.The current FQDN validation could be more robust. Consider:
- Using
net.ParseIPfor IP address validation- Adding validation for maximum hostname length
- Validating hostname characters
Example improvement:
+func isValidHostname(hostname string) bool { + if len(hostname) > 255 { + return false + } + if ip := net.ParseIP(hostname); ip != nil { + return true + } + if hostname == "localhost" { + return true + } + for _, part := range strings.Split(hostname, ".") { + if len(part) > 63 { + return false + } + if !regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?$`).MatchString(part) { + return false + } + } + return strings.Contains(hostname, ".") +} func isEndpointFQDN(endpoint string) bool { - return strings.Contains(endpoint, ".") || strings.Contains(endpoint, "::") || endpoint == "localhost" + return isValidHostname(endpoint) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ably/options.go(14 hunks)ablytest/sandbox.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 (9)
ablytest/sandbox.go (5)
103-105: LGTM! Clean transition from environment to endpoint-based configuration.The struct changes align well with the PR objective of transitioning to endpoint-based configuration.
135-135: LGTM! Consistent with endpoint-based approach.The function correctly delegates to
NewSandboxWithEndpoint, maintaining the new configuration pattern.
138-142: LGTM! Clean implementation of endpoint-based initialization.The function properly initializes the sandbox with the new endpoint configuration.
236-236: LGTM! Consistent use of endpoint-based configuration.The method correctly uses
ably.WithEndpointfor configuration.
278-278: LGTM! Consistent use of endpoint in auth params.The method correctly uses endpoint instead of environment in authentication parameters.
ably/options.go (4)
26-29: LGTM! Well-documented constants with specification references.The constants are properly defined with clear documentation and specification references (REC1a).
Line range hint
255-278: LGTM! Clear deprecation notices and documentation.The struct changes properly document the transition to endpoint-based configuration with clear deprecation notices for old fields.
431-437: LGTM! Thorough validation of configuration conflicts.The validation properly prevents mixing of new endpoint-based configuration with deprecated options (REC1b1).
1133-1141: LGTM! Well-documented endpoint configuration function.The function is properly implemented with clear documentation and references to platform customization documentation.
sacOO7
requested review from
lmars and
SimonWoolf
and removed request for
SimonWoolf
surminus
force-pushed
the
feature/endpoint-option
branch
from
69d2c0e to
55b8050
Compare
github-actions
bot
temporarily deployed
to
staging/pull/679/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ably/options.go (2)
507-509: Consider additional IPv6 address cases.The
isEndpointFQDNfunction might miss some valid IPv6 address formats. Consider usingnet.ParseIPfor more robust IP address validation.-func isEndpointFQDN(endpoint string) bool { - return strings.Contains(endpoint, ".") || strings.Contains(endpoint, "::") || endpoint == "localhost" +func isEndpointFQDN(endpoint string) bool { + if endpoint == "localhost" { + return true + } + if ip := net.ParseIP(endpoint); ip != nil { + return true + } + return strings.Contains(endpoint, ".") +}
1143-1154: Consider adding migration guide link in deprecation notice.While the deprecation notice is clear, consider adding a link to a migration guide to help users transition from the deprecated
WithEnvironmentto the newWithEndpoint.// Deprecated: this option is deprecated and will be removed in a future // version. +// +// Migration guide: https://github.com/ably/docs/pull/XXX
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ably/ably_test.go(1 hunks)ably/auth_integration_test.go(4 hunks)ably/error_test.go(2 hunks)ably/export_test.go(3 hunks)ably/http_paginated_response_integration_test.go(1 hunks)ably/options.go(14 hunks)ably/options_test.go(6 hunks)ably/realtime_client_integration_test.go(8 hunks)ably/realtime_conn_spec_integration_test.go(4 hunks)ably/realtime_presence_integration_test.go(1 hunks)ably/rest_channel_integration_test.go(3 hunks)ably/rest_client_integration_test.go(11 hunks)ablytest/ablytest.go(2 hunks)ablytest/sandbox.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ably/error_test.go
- ably/rest_channel_integration_test.go
- ably/realtime_presence_integration_test.go
- ably/http_paginated_response_integration_test.go
- ably/ably_test.go
- ably/export_test.go
- ably/auth_integration_test.go
- ably/rest_client_integration_test.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 (20)
ablytest/ablytest.go (2)
20-20: LGTM! The new default endpoint aligns with the routing policy changes.The change from environment-based to endpoint-based configuration is correctly implemented with a sensible default value.
43-45: LGTM! Environment variable update is consistent with the new endpoint approach.The change from
ABLY_ENVtoABLY_ENDPOINTmaintains backward compatibility while moving to the new endpoint-based configuration.ablytest/sandbox.go (3)
102-106: LGTM! Sandbox struct updated to use endpoint-based configuration.The struct changes correctly reflect the transition from environment to endpoint-based configuration.
255-262: LGTM! URL construction logic handles both production and non-production endpoints.The URL construction correctly handles the "nonprod:" prefix and constructs appropriate URLs for both environments.
278-278: LGTM! Auth params updated to use endpoint.The authentication parameters now correctly use the endpoint instead of environment.
ably/options_test.go (4)
16-26: LGTM! Updated fallback host test with new naming convention.The test correctly verifies the new fallback host naming pattern.
28-64: LGTM! Comprehensive test coverage for endpoint fallbacks.The test suite thoroughly covers different endpoint scenarios:
- Standard endpoint
- Sandbox endpoint
- Non-production endpoint
277-427: LGTM! Thorough testing of endpoint behavior with fallbacks.The test suite comprehensively covers:
- Default endpoint behavior
- FQDN endpoint behavior
- Legacy option support
- Fallback host configurations
427-432: LGTM! Added FQDN validation tests.The test suite correctly verifies FQDN endpoint detection for various cases.
ably/realtime_client_integration_test.go (3)
33-53: LGTM! Added test coverage for endpoint-based configuration.The test correctly verifies realtime host behavior with the new endpoint option.
79-159: LGTM! Comprehensive test coverage for Ably agent header.The test suite thoroughly verifies agent header behavior with:
- Standard agent identifiers
- Custom agents with version
- Custom agents without version
286-292: LGTM! Added test coverage for FQDN endpoint fallback behavior.The test correctly verifies that fallback behavior is disabled for FQDN endpoints.
ably/options.go (4)
26-28: LGTM! Well-documented constants.The new constants are properly documented with clear references to specifications.
255-279: LGTM! Clear deprecation notices.The changes properly document both the new
Endpointfield and the deprecated fields with clear migration paths.
430-446: LGTM! Robust validation logic.The validation logic correctly prevents mixing of endpoint and deprecated options, with proper error logging.
1133-1141: LGTM! Well-documented new endpoint option.The new
WithEndpointfunction is properly documented with a link to the platform customization documentation.ably/realtime_conn_spec_integration_test.go (4)
225-225: LGTM: Environment to endpoint transition in RTN4f testThe change aligns with the PR objective of enhancing the endpoint option while maintaining test coverage for connection failure scenarios.
1738-1738: LGTM: Environment to endpoint transition in RTN22a testThe change maintains test coverage for server-initiated authentication while adapting to the new endpoint-based configuration.
3040-3040: LGTM: Environment to endpoint transition in RTN24 testThe change preserves test coverage for JWT token reauthorization while updating to the endpoint-based configuration.
225-225: Verify backward compatibility with deprecated client optionsThe PR objectives mention ensuring changes don't disrupt existing code paths for deprecated client options.
Also applies to: 1738-1738, 3040-3040
✅ Verification successful
Final Verification: Deprecated Client Options Backward Compatibility is Intact
- The output confirms that
WithEnvironmentis still used in tests (e.g., inably/options_test.go) ensuring its backward compatibility.- The usage of
WithEndpointinably/realtime_conn_spec_integration_test.goand other integration test files is consistent and remains unaffected.- Overall, deprecated options are preserved in the codebase as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of WithEnvironment and verify WithEndpoint usage # Expected: No remaining WithEnvironment calls, consistent WithEndpoint usage echo "Checking for any remaining WithEnvironment usage..." rg --type go "WithEnvironment\(" echo "Verifying WithEndpoint usage..." rg --type go "WithEndpoint\("Length of output: 5158
sacOO7
force-pushed
the
feature/endpoint-option
branch
from
55b8050 to
b84ae73
Compare
github-actions
bot
temporarily deployed
to
staging/pull/679/features
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
1. Reverted to use of old RestHost and RealtimeHost 2. Reverted to use of old getFallbackHosts 3. Added precheck for non-empty endpoint for all methods 4. Updated tests accordingly
1. Added one more check for deprecated clientOption FallbackHostsUseDefault 2. Moved fallbackHostUseDefault endpoint integration test to unit test 3. Fixed rest_client_integration tests RSC15 fallbacks
1. Refactored `getPrimaryProdHost` to `getPrimaryHost`, included non-prod impl 2. Added missing spec annotations for endpoint spec in options.go 3. Fixed linting for sandbox test filet
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ablytest/sandbox.go (1)
255-262: Add endpoint validation to prevent invalid URLs.While the URL construction logic is correct, consider adding validation for the endpoint value to prevent invalid URLs.
func (app *Sandbox) URL(paths ...string) string { + // Validate endpoint + if app.Endpoint == "" { + panic("endpoint cannot be empty") + } + endpoint := strings.TrimSpace(app.Endpoint) + if strings.ContainsAny(endpoint, "/?#") { + panic("invalid characters in endpoint") + } + if strings.HasPrefix(app.Endpoint, "nonprod:") { - namespace := strings.TrimPrefix(app.Endpoint, "nonprod:") + namespace := strings.TrimPrefix(endpoint, "nonprod:") + if namespace == "" { + panic("namespace cannot be empty when using nonprod prefix") + } return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", namespace, path.Join(paths...)) } - return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) + return fmt.Sprintf("https://%s.realtime.ably.net/%s", endpoint, path.Join(paths...)) }ably/realtime_client_integration_test.go (1)
286-292: Consider adding a comment explaining the FQDN test case.While the test is well-structured, adding a comment explaining why fallback behavior doesn't apply when using a fully qualified domain name (FQDN) would improve test readability and maintainability.
t.Run("does not apply when endpoint with fqdn is used", func(t *testing.T) { + // When using a fully qualified domain name (FQDN) with WithEndpoint, + // fallback behavior is disabled as the endpoint is considered authoritative visitedHosts := initClientWithConnError(getTimeoutErr(), ably.WithEndpoint("custom-realtime.ably.io")) expectedHost := "custom-realtime.ably.io"ably/options_test.go (2)
71-209: LGTM! Comprehensive test coverage for hostname generation.The test suite thoroughly verifies hostname generation and fallback host behavior for various endpoint configurations, including default options, custom routing policies, nonprod routing policies, FQDNs, and legacy support.
Consider adding a test case for an invalid endpoint format to verify error handling.
+ t.Run("invalid endpoint format", func(t *testing.T) { + clientOptions := ably.NewClientOptions(ably.WithEndpoint("invalid:format:endpoint")) + err := clientOptions.Validate() + assert.Error(t, err) + })
427-432: Add more test cases for FQDN detection.While the current test cases cover basic scenarios, consider adding more edge cases:
+ assert.Equal(t, true, ably.IsEndpointFQDN("sub1.sub2.example.com")) + assert.Equal(t, true, ably.IsEndpointFQDN("2001:0db8:85a3:0000:0000:8a2e:0370:7334")) + assert.Equal(t, false, ably.IsEndpointFQDN("")) + assert.Equal(t, false, ably.IsEndpointFQDN("invalid@domain"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ably/ably_test.go(1 hunks)ably/auth_integration_test.go(4 hunks)ably/error_test.go(2 hunks)ably/export_test.go(3 hunks)ably/http_paginated_response_integration_test.go(1 hunks)ably/options.go(14 hunks)ably/options_test.go(6 hunks)ably/realtime_client_integration_test.go(8 hunks)ably/realtime_conn_spec_integration_test.go(4 hunks)ably/realtime_presence_integration_test.go(1 hunks)ably/rest_channel_integration_test.go(3 hunks)ably/rest_client_integration_test.go(11 hunks)ablytest/ablytest.go(2 hunks)ablytest/sandbox.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ably/auth_integration_test.go
- ably/realtime_presence_integration_test.go
- ably/rest_channel_integration_test.go
- ably/error_test.go
- ably/ably_test.go
- ably/http_paginated_response_integration_test.go
- ably/export_test.go
- 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 (19)
ablytest/sandbox.go (4)
16-16: LGTM!The addition of the "strings" package import is necessary for the new endpoint URL construction logic.
102-106: LGTM!The struct changes correctly implement the transition from environment-based to endpoint-based configuration.
134-143: LGTM!The changes correctly implement the transition to endpoint-based configuration while maintaining backward compatibility.
275-279: LGTM!The auth params have been correctly updated to use "endpoint" instead of "environment", maintaining consistency with the new configuration approach.
ably/realtime_client_integration_test.go (4)
33-53: LGTM! Well-structured test for the new endpoint option.The test case properly validates the new
WithEndpointoption while maintaining clear references to the specification (REC1b).
79-159: LGTM! Comprehensive test coverage for ablyAgent header with endpoint option.The test cases thoroughly validate the ablyAgent header behavior with the new
WithEndpointoption, including custom agents and version handling.
391-395: LGTM! Well-structured integration test for fallback behavior.The test properly validates the fallback behavior with internal server errors using JWT authentication and the new
WithEndpointoption.Also applies to: 396-416
440-444: LGTM! Well-structured integration test for timeout fallback behavior.The test properly validates the fallback behavior with timeouts using JWT authentication and the new
WithEndpointoption.Also applies to: 445-468
ably/rest_client_integration_test.go (3)
252-254: LGTM! Configuration options for RSC7d2 test case.The change from environment-based to endpoint-based configuration aligns with the PR objectives to enhance the endpoint option.
404-405: LGTM! Configuration for RSC15l1 test case.The test case correctly uses
WithEndpointto test host unresolvable or unreachable scenarios.
350-353: Verify test assertions for host fallback behavior.The test assertions have been updated to reflect the new routing policy:
- Primary host is now
sandbox.realtime.ably-nonprod.net- Fallback hosts are verified using
DefaultFallbackHosts()- Uniqueness of hosts is verified
Run the following script to verify the default fallback hosts:
✅ Verification successful
Verified fallback host assertions align with the new routing policy.
- Confirmed that
DefaultFallbackHosts()returns the fallback host configuration viadefaultOptions.FallbackHosts.- The test assertions correctly validate the primary host as
"sandbox.realtime.ably-nonprod.net", ensure the expected total count (1 primary + 5 fallbacks), and verify the uniqueness of hosts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default fallback hosts configuration. # Test: Search for DefaultFallbackHosts() usage and its configuration. ast-grep --pattern 'func DefaultFallbackHosts() []string { $$$ }'Length of output: 219
ably/realtime_conn_spec_integration_test.go (2)
225-226: LGTM! Configuration for RTN4a test case.The change from environment-based to endpoint-based configuration aligns with the PR objectives.
1738-1739: LGTM! Configuration for server-initiated auth test cases.The changes consistently replace environment-based configuration with endpoint-based configuration across multiple test cases:
- RTN22a_RTN15h2_Integration_ServerInitiatedAuth
- RTN22_RTC8_Integration_ServerInitiatedAuth
- RTC8a4, RSA3d
Also applies to: 1801-1802, 3040-3041
ably/options_test.go (2)
16-26: LGTM! Test updated to reflect new fallback host naming convention.The test has been correctly updated to use the new naming convention for fallback hosts, aligning with the new routing policy.
28-64: LGTM! Comprehensive test coverage for endpoint fallbacks.The test thoroughly verifies fallback host generation for different endpoint types:
- Standard endpoint (e.g., "acme")
- Sandbox endpoint (e.g., "nonprod:sandbox")
- Nonprod endpoint (e.g., "nonprod:acme")
ably/options.go (4)
26-29: LGTM! Clear and well-documented constants.The new constants are appropriately defined and documented with their purpose.
256-280: LGTM! Well-documented struct changes with proper deprecation notices.The changes properly introduce the new
Endpointfield while marking the legacy fields as deprecated. The documentation clearly explains the purpose and future plans.
62-88: LGTM! Clean implementation of endpoint-related helper functions.The functions provide a clear and consistent way to:
- Generate primary host from endpoint
- Generate fallback hosts based on endpoint type
- Generate fallback host patterns
1129-1137: LGTM! Well-documented endpoint configuration function.The function follows the established pattern for option functions and includes proper documentation with relevant links.
surminus
force-pushed
the
feature/endpoint-option
branch
from
b84ae73 to
ffd50e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ablytest/sandbox.go (1)
255-262: Consider extracting domain suffixes as constants.While the URL construction logic is correct, consider extracting the domain suffixes as constants to improve maintainability:
+const ( + nonprodDomainSuffix = "realtime.ably-nonprod.net" + prodDomainSuffix = "realtime.ably.net" +) func (app *Sandbox) URL(paths ...string) string { if strings.HasPrefix(app.Endpoint, "nonprod:") { namespace := strings.TrimPrefix(app.Endpoint, "nonprod:") - return fmt.Sprintf("https://%s.realtime.ably-nonprod.net/%s", namespace, path.Join(paths...)) + return fmt.Sprintf("https://%s.%s/%s", namespace, nonprodDomainSuffix, path.Join(paths...)) } - return fmt.Sprintf("https://%s.realtime.ably.net/%s", app.Endpoint, path.Join(paths...)) + return fmt.Sprintf("https://%s.%s/%s", app.Endpoint, prodDomainSuffix, path.Join(paths...)) }ably/options_test.go (1)
427-432: Consider adding test case for invalid endpoint format.While the current test cases cover valid endpoint formats, consider adding a test case for invalid endpoint formats to ensure proper error handling.
t.Run("with invalid endpoint format", func(t *testing.T) { clientOptions := ably.NewClientOptions(ably.WithEndpoint("invalid:format:endpoint")) err := clientOptions.Validate() assert.Error(t, err) assert.Contains(t, err.Error(), "invalid endpoint format") })ably/rest_client_integration_test.go (1)
402-421: Consider adding test for endpoint validation.While testing fallback behavior, consider adding a test case to verify that invalid endpoints are properly validated.
t.Run("must validate endpoint format", func(t *testing.T) { options := []ably.ClientOption{ ably.WithEndpoint("invalid:format:endpoint"), ably.WithTLS(false), ably.WithUseTokenAuth(true), } _, err := ably.NewREST(app.Options(options...)...) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid endpoint format") })ably/options.go (2)
79-88: Consider adding validation for root and domain parameters.While the function works correctly, it might benefit from input validation.
func endpointFallbacks(root, domain string) []string { + if empty(root) || empty(domain) { + return nil + } return []string{ fmt.Sprintf("%s.a.fallback.%s", root, domain), fmt.Sprintf("%s.b.fallback.%s", root, domain), fmt.Sprintf("%s.c.fallback.%s", root, domain), fmt.Sprintf("%s.d.fallback.%s", root, domain), fmt.Sprintf("%s.e.fallback.%s", root, domain), } }
475-486: Consider consolidating duplicate host resolution logic.The
getRestHostandgetRealtimeHostmethods share similar logic that could be consolidated.+func (opts *clientOptions) resolveHost(host string, isRealtime bool) string { + if !empty(opts.Endpoint) { + return opts.getHostnameFromEndpoint() + } + if !empty(host) { + return host + } + if !opts.isProductionEnvironment() { + return getPrimaryHost(opts.Environment) + } + if isRealtime { + return defaultOptions.RealtimeHost + } + return defaultOptions.RESTHost +} func (opts *clientOptions) getRestHost() string { - if !empty(opts.Endpoint) { - return opts.getHostnameFromEndpoint() - } - if !empty(opts.RESTHost) { - return opts.RESTHost - } - if !opts.isProductionEnvironment() { - return getPrimaryHost(opts.Environment) - } - return defaultOptions.RESTHost + return opts.resolveHost(opts.RESTHost, false) } func (opts *clientOptions) getRealtimeHost() string { - if !empty(opts.Endpoint) { - return opts.getHostnameFromEndpoint() - } - if !empty(opts.RealtimeHost) { - return opts.RealtimeHost - } + host := opts.resolveHost(opts.RealtimeHost, true) if !empty(opts.RESTHost) { logger := opts.LogHandler logger.Printf(LogWarning, "restHost is set to %s but realtimeHost is not set so setting realtimeHost to %s too. If this is not what you want, please set realtimeHost explicitly.", opts.RESTHost, opts.RealtimeHost) return opts.RESTHost } - if !opts.isProductionEnvironment() { - return getPrimaryHost(opts.Environment) - } - return defaultOptions.RealtimeHost + return host }Also applies to: 488-504
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ably/ably_test.go(1 hunks)ably/auth_integration_test.go(4 hunks)ably/error_test.go(2 hunks)ably/export_test.go(3 hunks)ably/http_paginated_response_integration_test.go(1 hunks)ably/options.go(14 hunks)ably/options_test.go(6 hunks)ably/realtime_client_integration_test.go(8 hunks)ably/realtime_conn_spec_integration_test.go(4 hunks)ably/realtime_presence_integration_test.go(1 hunks)ably/rest_channel_integration_test.go(3 hunks)ably/rest_client_integration_test.go(11 hunks)ablytest/ablytest.go(2 hunks)ablytest/sandbox.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ably/realtime_presence_integration_test.go
- ably/auth_integration_test.go
- ably/error_test.go
- ably/http_paginated_response_integration_test.go
- ably/ably_test.go
- ably/export_test.go
- ablytest/ablytest.go
- ably/rest_channel_integration_test.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 (17)
ablytest/sandbox.go (2)
102-106: LGTM! Struct field changes align with the new endpoint-based approach.The removal of
Environmentand addition ofEndpointfield in theSandboxstruct reflects the transition to endpoint-based configuration.
275-288: LGTM! Auth params updated correctly for endpoint-based configuration.The change from environment to endpoint in auth params aligns with the new configuration approach.
ably/options_test.go (2)
16-26: LGTM! Test updated to reflect new fallback host naming convention.The test correctly verifies the new fallback host format with the "main" prefix.
28-64: LGTM! Comprehensive test coverage for different endpoint scenarios.The test cases thoroughly cover:
- Standard endpoint
- Sandbox endpoint
- Nonprod endpoint
ably/realtime_client_integration_test.go (2)
33-53: LGTM! Test coverage for endpoint-based connection.The test correctly verifies endpoint-based connection with various host types.
286-292: LGTM! Test verifies fallback behavior with FQDN endpoints.The test correctly verifies that fallback doesn't apply when using FQDN endpoints.
ably/rest_client_integration_test.go (1)
252-256: LGTM! Test updated for endpoint-based configuration.The test correctly uses the new endpoint-based configuration approach.
ably/options.go (8)
26-28: LGTM! Well-documented constants.The new constants are well-documented and follow the specification requirements.
40-42: LGTM! Default options properly configured.The default options are correctly set up with the new endpoint-based configuration.
62-69: LGTM! Clear and concise host resolution.The function correctly handles both production and non-production environments according to REC1b3.
71-77: LGTM! Well-structured fallback host generation.The function properly implements REC2c3 and REC2c4 specifications for fallback host generation.
506-510: Previous review comment still applies.The function needs a more robust validation as suggested in the previous review.
432-438: LGTM! Proper validation of mutually exclusive options.The validation correctly implements REC1b1 by preventing the use of endpoint with deprecated options.
1129-1137: LGTM! Well-documented new endpoint option.The new
WithEndpointfunction is properly documented with a link to platform customization docs.
1139-1150: LGTM! Clear deprecation notices.The deprecated options are properly marked with clear notices about future removal.
Also applies to: 1204-1206, 1226-1228
ably/realtime_conn_spec_integration_test.go (2)
225-225: LGTM! Endpoint configuration update.The change from
WithEnvironmenttoWithEndpointaligns with the new routing policy implementation.
1738-1738: LGTM! Consistent endpoint configuration.The change maintains consistency with the updated configuration approach across the test suite.
endpointoption in a way that it doesn't break code path for deprecated clientOptions.Summary =>
endpointsupportSummary by CodeRabbit
Release Notes
Configuration Changes
Testing Improvements
Deprecation Notice