chore(oauth): OAuth handler to support functional configuration options #442
Conversation
|
Warning Rate limit exceeded@appleboy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughNew functional options support was added to OAuthHandler: an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ezynda3 , any feedback? |
…ion options - Refactor NewOAuthHandler to accept functional options for configuration - Add support for injecting a custom HTTP client via WithOAuthHTTPClient option fix mark3labs#440 Signed-off-by: Bo-Yi Wu <[email protected]>
appleboy
force-pushed
the
http
branch
2 times, most recently
from
24b8f8a to
4258c83
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 (1)
client/transport/oauth.go (1)
149-158: LGTM! Clean functional options implementation.The
OAuthHandlerOptiontype andWithOAuthHTTPClientfunction correctly implement the functional options pattern. The nil check at line 154 is good defensive programming, preventing accidental override of the default client.Consider adding unit tests to verify:
- Default client behavior (30s timeout) when no options provided
- Custom client properly overrides default when passed via
WithOAuthHTTPClient- Nil client passed to
WithOAuthHTTPClientpreserves default client- Multiple options applied in order (last wins)
Example test structure:
func TestWithOAuthHTTPClient(t *testing.T) { customClient := &http.Client{Timeout: 60 * time.Second} handler := NewOAuthHandler(OAuthConfig{}, WithOAuthHTTPClient(customClient)) if handler.httpClient != customClient { t.Error("custom client not applied") } } func TestWithOAuthHTTPClient_Nil(t *testing.T) { handler := NewOAuthHandler(OAuthConfig{}, WithOAuthHTTPClient(nil)) if handler.httpClient.Timeout != 30*time.Second { t.Error("default client should be preserved when nil passed") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/oauth.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/transport/oauth.go (1)
client/oauth.go (3)
OAuthConfig(11-11)TokenStore(17-17)NewMemoryTokenStore(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (1)
client/transport/oauth.go (1)
160-177: NewOAuthHandler backward compatibility verified. All existing calls with only aconfigargument compile and behave as before; variadic options don’t introduce breaking changes.
- Add comprehensive tests for the WithOAuthHTTPClient option in OAuth handler - Verify custom HTTP client is set and used when provided - Ensure default HTTP client is used when nil or no custom client is given - Test preservation of custom transport in provided HTTP client - Confirm multiple options can be combined when creating the handler Signed-off-by: Bo-Yi Wu <[email protected]>
- Add validation to automatically set a 30-second timeout if a custom OAuth HTTP client is provided without a timeout - Extend documentation to clarify usage and behavior of custom HTTP clients for OAuth operations - Add tests to verify default timeout is set for zero-timeout clients, that non-zero timeouts are preserved, and that custom transports are retained Signed-off-by: Bo-Yi Wu <[email protected]>
|
ping @ezynda3 |
1 participant
Description
NewOAuthHandlerfunction to use the functional options pattern, making configuration more flexible and extensible.WithOAuthHTTPClientoption, allowing advanced control over authentication requests (e.g., timeout, proxy).fix #440
Motivation
Changes
NewOAuthHandlernow accepts functional options.WithOAuthHTTPClientfor injecting a customhttp.Client.Testing
Notes
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests