-
Notifications
You must be signed in to change notification settings - Fork 449
完善社区#276的提议(by qoder) #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: net10
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a token refresh window feature for OAuth token management (community proposal #276 by qoder). The feature allows tokens to be refreshed proactively before they expire, preventing race conditions where tokens might expire between validation and actual use.
- Adds configurable refresh window with three strategies: fixed seconds, percentage-based, and auto (minimum of both)
- Provides multiple configuration approaches: inline via HttpApiOptions, standalone via IOptionsMonitor, and from configuration files
- Includes comprehensive unit tests for the token expiration logic
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/guide/6_auth-token-extension.md | Documents the new token refresh window feature with configuration examples |
| WebApiClientCore.Test.csproj | Adds project reference to OAuths extension for testing |
| TokenResultTest.cs | Comprehensive test suite for token expiration and refresh window logic |
| TokenResult.cs | Adds overloaded IsExpired method accepting refresh window parameter |
| TokenProvider.cs | Implements refresh window logic with configuration fallback chain |
| RefreshWindowStrategy.cs | Defines enum for refresh window calculation strategies |
| OAuthTokenOptions.cs | Configuration class for refresh window settings |
| HttpApiOptionsExtensions.cs | Extension methods to configure OAuth tokens via HttpApiOptions.Properties |
| OAuthTokenOptionsExtensions.cs | Dependency injection extensions for standalone configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (httpApiOptions != null) | ||
| { | ||
| return httpApiOptions.GetOAuthTokenOptions(); | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check on line 146 is redundant. The IOptionsMonitor<T>.Get(string name) method never returns null - it creates a new default instance if no configuration exists. This check should be removed.
| if (httpApiOptions != null) | |
| { | |
| return httpApiOptions.GetOAuthTokenOptions(); | |
| } | |
| return httpApiOptions.GetOAuthTokenOptions(); |
| /// <summary> | ||
| /// 判断是否应该刷新Token | ||
| /// 混合方案:优先从独立配置读取,其次从 HttpApiOptions.Properties 读取,最后使用默认行为 | ||
| /// </summary> | ||
| /// <param name="token">token结果</param> | ||
| /// <returns></returns> | ||
| private bool ShouldRefreshToken(TokenResult token) | ||
| { | ||
| // 获取配置 | ||
| var tokenOptions = this.GetTokenRefreshOptions(); | ||
|
|
||
| if (!tokenOptions.UseTokenRefreshWindow) | ||
| { | ||
| // 如果禁用刷新窗口,使用原有行为 | ||
| return token.IsExpired(); | ||
| } | ||
|
|
||
| // 计算刷新窗口 | ||
| var refreshWindow = this.CalculateRefreshWindow(token, tokenOptions); | ||
| return token.IsExpired(refreshWindow); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 获取Token刷新配置 | ||
| /// 优先级:独立配置(IOptionsMonitor<OAuthTokenOptions>) > HttpApiOptions.Properties > 默认配置 | ||
| /// </summary> | ||
| /// <returns>Token刷新配置</returns> | ||
| private OAuthTokenOptions GetTokenRefreshOptions() | ||
| { | ||
| // 优先从独立配置读取 (方案2:独立配置类) | ||
| var oauthOptionsMonitor = this.services.GetService<IOptionsMonitor<OAuthTokenOptions>>(); | ||
| if (oauthOptionsMonitor != null) | ||
| { | ||
| try | ||
| { | ||
| var options = oauthOptionsMonitor.Get(this.Name); | ||
| // 检查是否为默认配置,如果不是则使用 | ||
| if (options != null) | ||
| { | ||
| return options; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // 如果获取失败,继续尝试下一种方式 | ||
| } | ||
| } | ||
|
|
||
| // 其次从 HttpApiOptions.Properties 读取 (方案1:Properties 字典) | ||
| var httpApiOptionsMonitor = this.services.GetService<IOptionsMonitor<HttpApiOptions>>(); | ||
| if (httpApiOptionsMonitor != null) | ||
| { | ||
| try | ||
| { | ||
| var httpApiOptions = httpApiOptionsMonitor.Get(this.Name); | ||
| if (httpApiOptions != null) | ||
| { | ||
| return httpApiOptions.GetOAuthTokenOptions(); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // 如果获取失败,使用默认配置 | ||
| } | ||
| } | ||
|
|
||
| // 最后使用默认配置 | ||
| return new OAuthTokenOptions(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 根据配置策略计算刷新窗口 | ||
| /// </summary> | ||
| /// <param name="token">Token结果</param> | ||
| /// <param name="options">配置选项</param> | ||
| /// <returns>刷新窗口时间</returns> | ||
| private TimeSpan CalculateRefreshWindow(TokenResult token, OAuthTokenOptions options) | ||
| { | ||
| var fixedWindow = TimeSpan.FromSeconds(options.RefreshWindowSeconds); | ||
| var percentageWindow = TimeSpan.FromSeconds(token.Expires_in * options.RefreshWindowPercentage); | ||
|
|
||
| return options.RefreshWindowStrategy switch | ||
| { | ||
| RefreshWindowStrategy.FixedSeconds => fixedWindow, | ||
| RefreshWindowStrategy.Percentage => percentageWindow, | ||
| RefreshWindowStrategy.Auto => fixedWindow < percentageWindow ? fixedWindow : percentageWindow, | ||
| _ => fixedWindow < percentageWindow ? fixedWindow : percentageWindow // 默认使用Auto | ||
| }; | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new token refresh window feature lacks test coverage for key scenarios:
- Tests for
ShouldRefreshTokenmethod - Tests for
GetTokenRefreshOptionsmethod's configuration priority logic - Tests for
CalculateRefreshWindowmethod with different strategies - Integration tests verifying the configuration from both
HttpApiOptionsandIOptionsMonitor<OAuthTokenOptions>
Consider adding tests that verify the configuration fallback chain and the refresh window calculation strategies work correctly.
| @@ -0,0 +1,41 @@ | |||
| using Microsoft.Extensions.DependencyInjection; | |||
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing using Microsoft.Extensions.Options; import. The code calls builder.Services.Configure<OAuthTokenOptions> on lines 22 and 37, which is an extension method from the Microsoft.Extensions.Options namespace. Without this import, the code will not compile.
| using Microsoft.Extensions.DependencyInjection; | |
| using Microsoft.Extensions.DependencyInjection; | |
| using Microsoft.Extensions.Options; |
| // 检查是否为默认配置,如果不是则使用 | ||
| if (options != null) | ||
| { | ||
| return options; | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check on line 128 is redundant. The IOptionsMonitor<T>.Get(string name) method never returns null - it creates a new default instance if no configuration exists. This check should either be removed or the logic should verify if the options are actually configured (e.g., by checking specific property values differ from defaults).
| // 检查是否为默认配置,如果不是则使用 | |
| if (options != null) | |
| { | |
| return options; | |
| } | |
| // 直接返回 options,无需检查 null(IOptionsMonitor.Get 永不返回 null) | |
| return options; |
| catch | ||
| { | ||
| // 如果获取失败,继续尝试下一种方式 | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch blocks on lines 133 and 151 silently suppress all exceptions, making debugging difficult. Consider at least logging the exception or catching specific exception types that are expected. If any exception is acceptable here, add a comment explaining why it's safe to ignore.
| catch | ||
| { | ||
| // 如果获取失败,使用默认配置 | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block silently suppresses all exceptions, making debugging difficult. Consider at least logging the exception or catching specific exception types that are expected. If any exception is acceptable here, add a comment explaining why it's safe to ignore.
| /// </summary> | ||
| /// <param name="token">token结果</param> | ||
| /// <returns></returns> | ||
| private bool ShouldRefreshToken(TokenResult token) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local scope variable 'token' shadows TokenProvider.token.
| private TimeSpan CalculateRefreshWindow(TokenResult token, OAuthTokenOptions options) | ||
| { | ||
| var fixedWindow = TimeSpan.FromSeconds(options.RefreshWindowSeconds); | ||
| var percentageWindow = TimeSpan.FromSeconds(token.Expires_in * options.RefreshWindowPercentage); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local scope variable 'token' shadows TokenProvider.token.
| private TimeSpan CalculateRefreshWindow(TokenResult token, OAuthTokenOptions options) | |
| { | |
| var fixedWindow = TimeSpan.FromSeconds(options.RefreshWindowSeconds); | |
| var percentageWindow = TimeSpan.FromSeconds(token.Expires_in * options.RefreshWindowPercentage); | |
| private TimeSpan CalculateRefreshWindow(TokenResult tokenResult, OAuthTokenOptions options) | |
| { | |
| var fixedWindow = TimeSpan.FromSeconds(options.RefreshWindowSeconds); | |
| var percentageWindow = TimeSpan.FromSeconds(tokenResult.Expires_in * options.RefreshWindowPercentage); |
No description provided.