Conversation
ruattd
requested changes
Mar 14, 2026
Contributor
Author
🤔暂时没想出第二行 using 在哪里。目前只要 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Member
|
有一些问题： 还有一个就是连续两次 ThrowIfNull(value)，而 name 未校验，这个是不是应该做修改 |
Contributor
看错了，那俩上层的更改是删除 于是现在变成了另一个问题：上一层还有什么用？ |
Contributor
Author
|
@sourcery-ai review |
Reviewer's Guide在整个代码库中重构 HTTP 请求/响应处理，统一使用新的 新 HTTP 请求/响应辅助 API 的类图classDiagram
direction LR
class HttpRequest {
<<static>>
+HttpRequestMessage Create(url)
+HttpRequestMessage CreateHead(url)
+HttpRequestMessage CreatePost(url)
+HttpRequestMessage CreatePut(url)
+HttpRequestMessage CreateDelete(url)
+Task~string~ GetStringAsync(url)
+Task~T~ GetJsonAsync~T~(url)
+Task~HttpResponseMessage~ PostJsonAsync~T~(url, data, contentType)
}
class HttpSenderExtension {
<<static, extension(HttpRequestMessage)>>
+Task~HttpResponseMessage~ SendAsync(httpClient, addMetedata, enableLogging, httpCompletionOption, retryTimes, cancellationToken)
}
class HttpResponseExtension {
<<static, extension(HttpResponseMessage)>>
+bool IsSuccess
+string AsString()
+Task~string~ AsStringAsync(ct)
+Task~Stream~ AsStreamAsync(ct)
+Task~byte[]~ AsByteArrayAsync(ct)
+Task~T~ AsJsonAsync~T~(options, cancellationToken)
+Dictionary~string,string[]~ GetHeaders()
+Dictionary~string,string[]~ GetContentHeaders()
+string[] GetHeader(name)
+bool TryGetHeader(name, out values)
+string GetFirstHeaderValue(name)
+bool TryGetFirstHeaderValue(name, out value)
+void EnsureSuccessStatusCode()
+Task EnsureSuccessStatusCodeWithContentAsync(ct)
}
class HttpContentExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithContent(HttpContent, contentType)
+HttpRequestMessage WithContent(string, contentType)
+HttpRequestMessage WithBinaryContent(byte[])
+HttpRequestMessage WithJsonContent~T~(content, contentType)
+HttpRequestMessage WithFormContent(string)
+HttpRequestMessage WithFormContent(IEnumerable~KeyValuePair~)
}
class HttpHeaderHandler {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithHeader(key, value)
+HttpRequestMessage WithHeaders(IDictionary~string,string~)
+HttpRequestMessage WithHeader(KeyValuePair~string,string~)
+HttpRequestMessage WithAuthentication(scheme, token)
+HttpRequestMessage WithBearerToken(token)
}
class HttpCookieExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithCookie(name, value)
-string _GetSafeCookieValue(value)
-char[] _ForbiddenCookieValueChar
}
class HttpBasicExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithHttpVersionOption(httpVersion)
}
class NetworkService {
<<partial, LifecycleScope(network)>>
-static ServiceProvider _provider
-static IHttpClientFactory _factory
-const int BaseRetryDelayMs
-const int MaxRetryDelayMs
+static HttpClient GetClient(wantClientType)
+static AsyncPolicy GetRetryPolicy(retry, retryPolicy)
-static void _Start()
-static void _Stop()
-static TimeSpan _DefaultSleepDurationProvider(attempt)
}
class LogWrapper {
+static void Info(category, message)
+static void Debug(category, message)
+static void Error(exception, message)
+static void Error(exception, message, detail)
}
class Basics {
+static string VersionName
+static string VersionCode
}
HttpRequest ..> HttpSenderExtension : uses SendAsync()
HttpRequest ..> HttpContentExtension : uses WithJsonContent()
HttpRequest ..> HttpResponseExtension : uses AsStringAsync(), AsJsonAsync()
HttpSenderExtension ..> NetworkService : uses GetClient(), GetRetryPolicy()
HttpSenderExtension ..> LogWrapper : logging
HttpSenderExtension ..> Basics : request metadata
HttpContentExtension ..> HttpHeaderHandler : WithHeader()
HttpHeaderHandler ..> HttpRequestMessage : modifies
HttpCookieExtension ..> HttpRequestMessage : modifies
HttpBasicExtension ..> HttpRequestMessage : modifies
HttpResponseExtension ..> HttpResponseMessage : extends
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 以：
Getting HelpOriginal review guide in EnglishReviewer's GuideRefactors HTTP request/response handling across the codebase to use a new HttpRequest + extension-based API, updates NetworkService lifecycle and retry policy, and adjusts server-side HTTP helpers and call sites for clearer, safer, and more consistent networking behavior. Class diagram for new HTTP request/response helper APIclassDiagram
direction LR
class HttpRequest {
<<static>>
+HttpRequestMessage Create(url)
+HttpRequestMessage CreateHead(url)
+HttpRequestMessage CreatePost(url)
+HttpRequestMessage CreatePut(url)
+HttpRequestMessage CreateDelete(url)
+Task~string~ GetStringAsync(url)
+Task~T~ GetJsonAsync~T~(url)
+Task~HttpResponseMessage~ PostJsonAsync~T~(url, data, contentType)
}
class HttpSenderExtension {
<<static, extension(HttpRequestMessage)>>
+Task~HttpResponseMessage~ SendAsync(httpClient, addMetedata, enableLogging, httpCompletionOption, retryTimes, cancellationToken)
}
class HttpResponseExtension {
<<static, extension(HttpResponseMessage)>>
+bool IsSuccess
+string AsString()
+Task~string~ AsStringAsync(ct)
+Task~Stream~ AsStreamAsync(ct)
+Task~byte[]~ AsByteArrayAsync(ct)
+Task~T~ AsJsonAsync~T~(options, cancellationToken)
+Dictionary~string,string[]~ GetHeaders()
+Dictionary~string,string[]~ GetContentHeaders()
+string[] GetHeader(name)
+bool TryGetHeader(name, out values)
+string GetFirstHeaderValue(name)
+bool TryGetFirstHeaderValue(name, out value)
+void EnsureSuccessStatusCode()
+Task EnsureSuccessStatusCodeWithContentAsync(ct)
}
class HttpContentExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithContent(HttpContent, contentType)
+HttpRequestMessage WithContent(string, contentType)
+HttpRequestMessage WithBinaryContent(byte[])
+HttpRequestMessage WithJsonContent~T~(content, contentType)
+HttpRequestMessage WithFormContent(string)
+HttpRequestMessage WithFormContent(IEnumerable~KeyValuePair~)
}
class HttpHeaderHandler {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithHeader(key, value)
+HttpRequestMessage WithHeaders(IDictionary~string,string~)
+HttpRequestMessage WithHeader(KeyValuePair~string,string~)
+HttpRequestMessage WithAuthentication(scheme, token)
+HttpRequestMessage WithBearerToken(token)
}
class HttpCookieExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithCookie(name, value)
-string _GetSafeCookieValue(value)
-char[] _ForbiddenCookieValueChar
}
class HttpBasicExtension {
<<static, extension(HttpRequestMessage)>>
+HttpRequestMessage WithHttpVersionOption(httpVersion)
}
class NetworkService {
<<partial, LifecycleScope(network)>>
-static ServiceProvider _provider
-static IHttpClientFactory _factory
-const int BaseRetryDelayMs
-const int MaxRetryDelayMs
+static HttpClient GetClient(wantClientType)
+static AsyncPolicy GetRetryPolicy(retry, retryPolicy)
-static void _Start()
-static void _Stop()
-static TimeSpan _DefaultSleepDurationProvider(attempt)
}
class LogWrapper {
+static void Info(category, message)
+static void Debug(category, message)
+static void Error(exception, message)
+static void Error(exception, message, detail)
}
class Basics {
+static string VersionName
+static string VersionCode
}
HttpRequest ..> HttpSenderExtension : uses SendAsync()
HttpRequest ..> HttpContentExtension : uses WithJsonContent()
HttpRequest ..> HttpResponseExtension : uses AsStringAsync(), AsJsonAsync()
HttpSenderExtension ..> NetworkService : uses GetClient(), GetRetryPolicy()
HttpSenderExtension ..> LogWrapper : logging
HttpSenderExtension ..> Basics : request metadata
HttpContentExtension ..> HttpHeaderHandler : WithHeader()
HttpHeaderHandler ..> HttpRequestMessage : modifies
HttpCookieExtension ..> HttpRequestMessage : modifies
HttpBasicExtension ..> HttpRequestMessage : modifies
HttpResponseExtension ..> HttpResponseMessage : extends
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
你好——我这里发现了 3 个问题，并给出了一些整体性反馈：
- 新的
HttpSenderExtension.SendAsync使用了NetworkService.GetRetryPolicy（其返回的是非泛型的AsyncPolicy），却把它当成AsyncPolicy<HttpResponseMessage>来用，在委托里直接返回了HttpResponseMessage；这在 Polly 中是无法编译通过的。要么让GetRetryPolicy变成泛型（例如返回AsyncPolicy<HttpResponseMessage>），要么在SendAsync中将结果包装进一个非泛型的重试策略里。 - 一些已经接受
CancellationToken的异步辅助方法（例如YggdrasilLegacyClient.RefreshAsync中的部分内容、一些AsJsonAsync/AsStringAsync的调用，以及OpenIdOptions.InitializeAsync），在调用新的响应扩展方法时没有始终把该 token 继续往下传。建议在所有能拿到 token 的地方都把它沿用下去，以保持取消行为的一致性。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- The new `HttpSenderExtension.SendAsync` uses `NetworkService.GetRetryPolicy` (which returns a non-generic `AsyncPolicy`) as if it were `AsyncPolicy<HttpResponseMessage>` by returning an `HttpResponseMessage` from the delegate; this won’t compile with Polly and either `GetRetryPolicy` should become generic (e.g. `AsyncPolicy<HttpResponseMessage>`) or `SendAsync` should wrap the result in a non-generic retry policy.
- Several asynchronous helpers that already accept a `CancellationToken` (e.g. in `YggdrasilLegacyClient.RefreshAsync`, some `AsJsonAsync`/`AsStringAsync` calls, and `OpenIdOptions.InitializeAsync`) don’t consistently pass the token down to the new response extension methods; consider threading the token through everywhere it’s available to keep cancellation behavior consistent.
## Individual Comments
### Comment 1
<location path="PCL.Core/IO/Net/Http/HttpResponseExtension.cs" line_range="14" />
<code_context>
+
+public static class HttpResponseExtension
+{
+ extension(HttpResponseMessage responseMessage)
+ {
+ public bool IsSuccess => responseMessage.IsSuccessStatusCode;
</code_context>
<issue_to_address>
**issue (bug_risk):** The `extension(HttpResponseMessage ...)` construct is not valid C# and will prevent these helpers from compiling or being used as extension methods.
Extension methods in C# must be `public static` methods with a `this` parameter inside a non-generic `static` class, e.g. `public static string AsString(this HttpResponseMessage responseMessage)`. The `extension(HttpResponseMessage responseMessage)` block syntax is not valid C# and will not compile, so usages like `response.AsString()` / `request.WithJsonContent(...)` will not bind. Please convert these helpers to standard extension-method declarations.
</issue_to_address>
### Comment 2
<location path="PCL.Core/Minecraft/IdentityModel/Yggdrasil/Client.cs" line_range="138" />
<code_context>
+ using var response = await HttpRequest
+ .CreatePost(address)
+ .WithHeaders(options.Headers ?? [])
+ .WithJsonContent(signoutData)
+ .SendAsync(options.GetClient.Invoke(), cancellationToken: token)
+ .ConfigureAwait(false);
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing an already-serialized JSON string to `WithJsonContent` will double-encode the payload.
Here `signoutData` is already serialized via `.ToJsonString()`, so passing it to `WithJsonContent` causes it to be serialized a second time. That yields a JSON string literal (quoted JSON) instead of the expected JSON object, which will likely break the Yggdrasil signout API.
Either pass the anonymous object / `JsonObject` directly to `WithJsonContent`, or switch to `WithContent(signoutData, "application/json")` to use the pre-serialized string as-is.
</issue_to_address>
### Comment 3
<location path="PCL.Core/Minecraft/IdentityModel/Extensions/OpenId/OpenIdOptions.cs" line_range="62-65" />
<code_context>
- .Create("https://pcl2ce.pysio.online/post", HttpMethod.Post)
- .WithAuthentication(telemetryKey).WithJsonContent(telemetry)
- .SendAsync().ConfigureAwait(false);
+ using var response = await HttpRequest
+ .CreatePost("https://pcl2ce.pysio.online/post")
+ .WithHeader("Authorization", telemetryKey)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The cancellation token is no longer honored when sending the discovery request.
Previously, `InitializeAsync` called `SendAsync(request, token)`, so the caller’s `CancellationToken` could cancel the HTTP request itself. The new code calls `SendAsync(GetClient.Invoke())` without the token, meaning cancellation now only affects deserialization, not long/stuck network calls.
Please pass the token through to `SendAsync` (e.g. `SendAsync(GetClient.Invoke(), cancellationToken: token)`) to preserve the original cancellation behavior.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The new
HttpSenderExtension.SendAsyncusesNetworkService.GetRetryPolicy(which returns a non-genericAsyncPolicy) as if it wereAsyncPolicy<HttpResponseMessage>by returning anHttpResponseMessagefrom the delegate; this won’t compile with Polly and eitherGetRetryPolicyshould become generic (e.g.AsyncPolicy<HttpResponseMessage>) orSendAsyncshould wrap the result in a non-generic retry policy. - Several asynchronous helpers that already accept a
CancellationToken(e.g. inYggdrasilLegacyClient.RefreshAsync, someAsJsonAsync/AsStringAsynccalls, andOpenIdOptions.InitializeAsync) don’t consistently pass the token down to the new response extension methods; consider threading the token through everywhere it’s available to keep cancellation behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `HttpSenderExtension.SendAsync` uses `NetworkService.GetRetryPolicy` (which returns a non-generic `AsyncPolicy`) as if it were `AsyncPolicy<HttpResponseMessage>` by returning an `HttpResponseMessage` from the delegate; this won’t compile with Polly and either `GetRetryPolicy` should become generic (e.g. `AsyncPolicy<HttpResponseMessage>`) or `SendAsync` should wrap the result in a non-generic retry policy.
- Several asynchronous helpers that already accept a `CancellationToken` (e.g. in `YggdrasilLegacyClient.RefreshAsync`, some `AsJsonAsync`/`AsStringAsync` calls, and `OpenIdOptions.InitializeAsync`) don’t consistently pass the token down to the new response extension methods; consider threading the token through everywhere it’s available to keep cancellation behavior consistent.
## Individual Comments
### Comment 1
<location path="PCL.Core/IO/Net/Http/HttpResponseExtension.cs" line_range="14" />
<code_context>
+
+public static class HttpResponseExtension
+{
+ extension(HttpResponseMessage responseMessage)
+ {
+ public bool IsSuccess => responseMessage.IsSuccessStatusCode;
</code_context>
<issue_to_address>
**issue (bug_risk):** The `extension(HttpResponseMessage ...)` construct is not valid C# and will prevent these helpers from compiling or being used as extension methods.
Extension methods in C# must be `public static` methods with a `this` parameter inside a non-generic `static` class, e.g. `public static string AsString(this HttpResponseMessage responseMessage)`. The `extension(HttpResponseMessage responseMessage)` block syntax is not valid C# and will not compile, so usages like `response.AsString()` / `request.WithJsonContent(...)` will not bind. Please convert these helpers to standard extension-method declarations.
</issue_to_address>
### Comment 2
<location path="PCL.Core/Minecraft/IdentityModel/Yggdrasil/Client.cs" line_range="138" />
<code_context>
+ using var response = await HttpRequest
+ .CreatePost(address)
+ .WithHeaders(options.Headers ?? [])
+ .WithJsonContent(signoutData)
+ .SendAsync(options.GetClient.Invoke(), cancellationToken: token)
+ .ConfigureAwait(false);
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing an already-serialized JSON string to `WithJsonContent` will double-encode the payload.
Here `signoutData` is already serialized via `.ToJsonString()`, so passing it to `WithJsonContent` causes it to be serialized a second time. That yields a JSON string literal (quoted JSON) instead of the expected JSON object, which will likely break the Yggdrasil signout API.
Either pass the anonymous object / `JsonObject` directly to `WithJsonContent`, or switch to `WithContent(signoutData, "application/json")` to use the pre-serialized string as-is.
</issue_to_address>
### Comment 3
<location path="PCL.Core/Minecraft/IdentityModel/Extensions/OpenId/OpenIdOptions.cs" line_range="62-65" />
<code_context>
- .Create("https://pcl2ce.pysio.online/post", HttpMethod.Post)
- .WithAuthentication(telemetryKey).WithJsonContent(telemetry)
- .SendAsync().ConfigureAwait(false);
+ using var response = await HttpRequest
+ .CreatePost("https://pcl2ce.pysio.online/post")
+ .WithHeader("Authorization", telemetryKey)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The cancellation token is no longer honored when sending the discovery request.
Previously, `InitializeAsync` called `SendAsync(request, token)`, so the caller’s `CancellationToken` could cancel the HTTP request itself. The new code calls `SendAsync(GetClient.Invoke())` without the token, meaning cancellation now only affects deserialization, not long/stuck network calls.
Please pass the token through to `SendAsync` (e.g. `SendAsync(GetClient.Invoke(), cancellationToken: token)`) to preserve the original cancellation behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PCL.Core/Minecraft/IdentityModel/Extensions/OpenId/OpenIdOptions.cs
Outdated
Show resolved
Hide resolved
ruattd
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
在整个代码库中重构 HTTP 请求/响应处理逻辑，改为使用新的流式（fluent）辅助 API，同时改进重试行为、错误处理，以及内部 HTTP 服务器命名空间。
Bug 修复：
DlSourceLoader传入正确的加载器实例，确保下载源选择能正确反映调用方的状态。增强与改进：
HttpRequest辅助类和基于扩展方法的流式 API，用于构建、发送和处理 HTTP 请求与响应，替代之前的HttpRequestBuilder/HttpResponseHandler工具。EnsureSuccessStatusCode等便捷方法、统一的 JSON 反序列化辅助工具、更丰富的超时处理，以及一致的请求头/Cookie 工具方法。NetworkService的生命周期集成和重试策略，使用声明式生命周期特性（attributes）以及带有上限的指数退避（exponential backoff）策略。HttpServer、HttpRoute、HttpRouteResponse），将其整合到统一的PCL.Core.IO.Net.Http命名空间中，以获得更清晰的组织结构和更好的复用性。Original summary in English
Summary by Sourcery
Refactor HTTP request/response handling to use a new fluent helper API across the codebase, while improving retry behavior, error handling, and internal HTTP server namespaces.
Bug Fixes:
Enhancements: