Fspes 122#57
Merged
Merged
Conversation
Agent-Logs-Url: https://github.com/FrendsPlatform/Frends.HTTP/sessions/2c0a6a2a-230b-4d67-af17-87d3e474f51f Co-authored-by: MichalFrends1 <167774394+MichalFrends1@users.noreply.github.com>
Agent-Logs-Url: https://github.com/FrendsPlatform/Frends.HTTP/sessions/2c0a6a2a-230b-4d67-af17-87d3e474f51f Co-authored-by: MichalFrends1 <167774394+MichalFrends1@users.noreply.github.com>
…uest-options-logic Replicate Request proxy/protocol option model across DownloadFile, RequestBytes, SendBytes, and SendAndReceiveBytes
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds proxy routing and HTTP/SSL protocol version configuration across four HTTP packages. New HttpVersion and SslVersion enums enable explicit protocol selection, while proxy options support routing through HTTP proxies with credential handling. Extension methods configure handlers and request defaults, cache keys account for protocol and proxy variations, and comprehensive tests validate the new functionality. ChangesHTTP Proxy and Protocol Version Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.Tests/UnitTests.cs (1)
575-596: ⚡ Quick winConsider enhancing test to verify actual protocol usage and avoid external dependencies.
The test makes a real HTTP request to an external Azure endpoint and only verifies success, without confirming that TLS 1.2/1.3 or HTTP/2 were actually used. Consider:
- Using a mock HTTP handler (as done in RequestBytes tests) to avoid external dependencies and improve test reliability
- Verifying the actual protocol version negotiated (e.g., by inspecting handler configuration or response properties)
- Testing protocol fallback behavior
As per coding guidelines, tests should use mocking where real systems can't be simulated, and the current approach may fail in restricted network environments or if the external endpoint is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.Tests/UnitTests.cs` around lines 575 - 596, Test RequestShouldSetTls12And13WhenConfigured currently makes a real HTTP call and only asserts result.Success; replace it with a mocked HttpMessageHandler (like the handlers used in RequestBytes tests) and invoke HTTP.DownloadFile with a test handler so no external network is used, then assert that the request/handler saw the expected SslVersion (SslVersion.Tls12And13) and HttpVersion (Definitions.HttpVersion.Http20) via the handler's captured request/connection settings or response properties, and add additional assertions for fallback behavior if the handler simulates older protocol negotiation.Frends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.Tests/UnitTests.cs (2)
404-425: 💤 Low valueUse consistent assertion style throughout the test.
Line 419 uses
Assert.That(NUnit modern syntax) while lines 420-424 useClassicAssert. The rest of the test file consistently usesClassicAssert. For consistency, consider usingClassicAssertfor all assertions in this test method.♻️ Proposed fix for consistency
- Assert.That(proxy, Is.Not.Null); + ClassicAssert.IsNotNull(proxy);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.Tests/UnitTests.cs` around lines 404 - 425, In HandlerShouldUseConfiguredProxy replace the NUnit-style Assert.That(proxy, Is.Not.Null) with the project's existing ClassicAssert usage so all assertions in this test use ClassicAssert; specifically assert the proxy is not null using ClassicAssert (matching other assertions that validate handler.UseProxy, proxy.Address, and credentials) so the test consistently uses ClassicAssert for all assertions.
427-443: ⚡ Quick winConsider avoiding external HTTP dependencies and verifying actual protocol usage.
The test makes a real HTTP request to
https://httpbin.org/anythingand only verifies a 200 status code, without confirming that TLS 1.2/1.3 or HTTP/2 were actually negotiated. Consider:
- Using a mock handler (like
MockHttpMessageHandleralready available in this test suite) to eliminate the external dependency- Verifying the actual protocol version used (e.g., by checking request properties or handler configuration)
- Testing edge cases like protocol fallback scenarios
As per coding guidelines, tests should use mocking where real systems can't be simulated. The current approach may be unreliable in CI environments with restricted network access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.Tests/UnitTests.cs` around lines 427 - 443, Replace the external call in the RequestShouldSetTls12And13WhenConfigured test with a mocked HTTP handler: configure a MockHttpMessageHandler to return a 200 response and capture the outgoing HttpRequestMessage, then set HTTP.ClientFactory to return an HttpClient that uses a handler you can inspect; call HTTP.RequestBytes(input, options, ...) with Options.SslProtocolVersion = SslVersion.Tls12And13 and HttpProtocolVersion = Definitions.HttpVersion.Http20, and assert that the captured request.Version equals HTTP/2 and that the handler or HttpClientHandler used to create the client was configured with SslProtocols including Tls12 (and Tls13 if available) instead of making a real network call. Ensure the test name RequestShouldSetTls12And13WhenConfigured, method HTTP.RequestBytes, class Options and enum SslVersion.Tls12And13 are used to locate and update the test.Frends.HTTP.SendBytes/Frends.HTTP.SendBytes.Tests/UnitTests.cs (1)
363-386: ⚡ Quick winConsider isolating external dependency or moving to integration test suite.
This test makes a real HTTPS call to
httpbin.org, which introduces several concerns:
- Requires internet connectivity (may fail locally or in CI environments with restricted network access)
- Depends on external service availability
- Slower execution compared to unit tests with mocks
- Could be flaky due to network issues
While testing actual TLS/protocol negotiation is valuable, consider one of these approaches:
- Move to a separate integration test suite that runs conditionally
- Add logic to skip the test gracefully if the endpoint is unreachable
- Document in the test that it requires internet connectivity
- If possible, verify protocol configuration via handler inspection instead of making the actual call
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.HTTP.SendBytes/Frends.HTTP.SendBytes.Tests/UnitTests.cs` around lines 363 - 386, The test RequestShouldSetTls12And13WhenConfigured currently makes a real HTTPS call via HTTP.SendBytes to httpbin.org; instead move this case out of unit tests into an integration test suite or gate it behind an explicit runtime flag (e.g. check an environment variable like RUN_INTEGRATION_TESTS or add a [TestCategory("Integration")] and skip when not set) so local/CI runs won't require internet; alternatively, replace the external call with a unit-level approach by constructing the HttpClient via HttpClientFactory and asserting the configured Options.SslProtocolVersion (SslVersion.Tls12And13) is applied to the handler before calling HTTP.SendBytes.Frends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes.Tests/UnitTests.cs (1)
363-386: ⚡ Quick winConsider isolating external dependency or moving to integration test suite.
This test makes a real HTTPS call to
httpbin.org, which introduces several concerns:
- Requires internet connectivity (may fail locally or in CI environments with restricted network access)
- Depends on external service availability
- Slower execution compared to unit tests with mocks
- Could be flaky due to network issues
While testing actual TLS/protocol negotiation is valuable, consider one of these approaches:
- Move to a separate integration test suite that runs conditionally
- Add logic to skip the test gracefully if the endpoint is unreachable
- Document in the test that it requires internet connectivity
- If possible, verify protocol configuration via handler inspection instead of making the actual call
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes.Tests/UnitTests.cs` around lines 363 - 386, The test RequestShouldSetTls12And13WhenConfigured currently performs a real HTTPS call via HTTP.SendAndReceiveBytes which makes it flaky and unsuitable for unit tests; replace the external call by injecting a test HttpClientFactory (set HTTP.ClientFactory) that returns an HttpClient constructed with a custom HttpMessageHandler which captures/validates the Ssl/TLS configuration and requested HTTP version (or simulates a response), then call HTTP.SendAndReceiveBytes with the same Options (SslVersion.Tls12And13, Definitions.HttpVersion.Http20) and assert against the captured handler values; alternatively move this test into an integration suite and mark it to run conditionally if network/httpbin.org is reachable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/DownloadFile.cs`:
- Around line 138-140: The cache key builder currently appends plaintext
options.ProxyUsername and options.ProxyPassword directly into the long-lived key
string; replace those raw values with a stable, irreversible fingerprint (e.g.,
a SHA-256 hex or base64 of the combined credentials) before concatenation to
avoid secret exposure. Locate the string construction that appends
$":{options.ProxyUrl}:{options.ProxyUsername}:{options.ProxyPassword}" (in the
DownloadFile cache/key assembly) and compute a short hash of the sensitive parts
(ProxyUsername + ":" + ProxyPassword or an equivalent stable canonicalization)
and append that hash instead of the raw username/password; ensure the hashing
routine is deterministic and collision-resistant and reference it from the key
construction so the key remains stable across runs without exposing secrets.
In `@Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/Extensions.cs`:
- Around line 43-60: The proxy setup in Extensions.cs currently ignores an empty
ProxyUrl when options.UseProxy is true; add an explicit validation at the start
of the proxy block (checking options.UseProxy and
string.IsNullOrWhiteSpace(options.ProxyUrl)) and throw an ArgumentException if
ProxyUrl is missing so the API contract is enforced; keep the rest of the block
that creates handler.Proxy = new WebProxy(options.ProxyUrl) and sets credentials
(handler.Proxy.Credentials) unchanged.
---
Nitpick comments:
In `@Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.Tests/UnitTests.cs`:
- Around line 575-596: Test RequestShouldSetTls12And13WhenConfigured currently
makes a real HTTP call and only asserts result.Success; replace it with a mocked
HttpMessageHandler (like the handlers used in RequestBytes tests) and invoke
HTTP.DownloadFile with a test handler so no external network is used, then
assert that the request/handler saw the expected SslVersion
(SslVersion.Tls12And13) and HttpVersion (Definitions.HttpVersion.Http20) via the
handler's captured request/connection settings or response properties, and add
additional assertions for fallback behavior if the handler simulates older
protocol negotiation.
In `@Frends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.Tests/UnitTests.cs`:
- Around line 404-425: In HandlerShouldUseConfiguredProxy replace the
NUnit-style Assert.That(proxy, Is.Not.Null) with the project's existing
ClassicAssert usage so all assertions in this test use ClassicAssert;
specifically assert the proxy is not null using ClassicAssert (matching other
assertions that validate handler.UseProxy, proxy.Address, and credentials) so
the test consistently uses ClassicAssert for all assertions.
- Around line 427-443: Replace the external call in the
RequestShouldSetTls12And13WhenConfigured test with a mocked HTTP handler:
configure a MockHttpMessageHandler to return a 200 response and capture the
outgoing HttpRequestMessage, then set HTTP.ClientFactory to return an HttpClient
that uses a handler you can inspect; call HTTP.RequestBytes(input, options, ...)
with Options.SslProtocolVersion = SslVersion.Tls12And13 and HttpProtocolVersion
= Definitions.HttpVersion.Http20, and assert that the captured request.Version
equals HTTP/2 and that the handler or HttpClientHandler used to create the
client was configured with SslProtocols including Tls12 (and Tls13 if available)
instead of making a real network call. Ensure the test name
RequestShouldSetTls12And13WhenConfigured, method HTTP.RequestBytes, class
Options and enum SslVersion.Tls12And13 are used to locate and update the test.
In
`@Frends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes.Tests/UnitTests.cs`:
- Around line 363-386: The test RequestShouldSetTls12And13WhenConfigured
currently performs a real HTTPS call via HTTP.SendAndReceiveBytes which makes it
flaky and unsuitable for unit tests; replace the external call by injecting a
test HttpClientFactory (set HTTP.ClientFactory) that returns an HttpClient
constructed with a custom HttpMessageHandler which captures/validates the
Ssl/TLS configuration and requested HTTP version (or simulates a response), then
call HTTP.SendAndReceiveBytes with the same Options (SslVersion.Tls12And13,
Definitions.HttpVersion.Http20) and assert against the captured handler values;
alternatively move this test into an integration suite and mark it to run
conditionally if network/httpbin.org is reachable.
In `@Frends.HTTP.SendBytes/Frends.HTTP.SendBytes.Tests/UnitTests.cs`:
- Around line 363-386: The test RequestShouldSetTls12And13WhenConfigured
currently makes a real HTTPS call via HTTP.SendBytes to httpbin.org; instead
move this case out of unit tests into an integration test suite or gate it
behind an explicit runtime flag (e.g. check an environment variable like
RUN_INTEGRATION_TESTS or add a [TestCategory("Integration")] and skip when not
set) so local/CI runs won't require internet; alternatively, replace the
external call with a unit-level approach by constructing the HttpClient via
HttpClientFactory and asserting the configured Options.SslProtocolVersion
(SslVersion.Tls12And13) is applied to the handler before calling HTTP.SendBytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30b48ae5-69f6-4670-bd18-915bec41fdba
📒 Files selected for processing (31)
Frends.HTTP.DownloadFile/CHANGELOG.mdFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.Tests/UnitTests.csFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/Definitions/Enums.csFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/Definitions/Options.csFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/DownloadFile.csFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/Extensions.csFrends.HTTP.DownloadFile/Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.csprojFrends.HTTP.RequestBytes/CHANGELOG.mdFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.Tests/UnitTests.csFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/Definitions/HttpVersion.csFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/Definitions/Options.csFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/Definitions/SslVersion.csFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/Extensions.csFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/Frends.HTTP.RequestBytes.csprojFrends.HTTP.RequestBytes/Frends.HTTP.RequestBytes/RequestBytes.csFrends.HTTP.SendAndReceiveBytes/CHANGELOG.mdFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes.Tests/UnitTests.csFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/Definitions/HttpVersion.csFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/Definitions/Options.csFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/Definitions/SslVersion.csFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/Extensions.csFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes.csprojFrends.HTTP.SendAndReceiveBytes/Frends.HTTP.SendAndReceiveBytes/SendAndReceiveBytes.csFrends.HTTP.SendBytes/CHANGELOG.mdFrends.HTTP.SendBytes/Frends.HTTP.SendBytes.Tests/UnitTests.csFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/Definitions/HttpVersion.csFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/Definitions/Options.csFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/Definitions/SslVersion.csFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/Extensions.csFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/Frends.HTTP.SendBytes.csprojFrends.HTTP.SendBytes/Frends.HTTP.SendBytes/SendBytes.cs
MatteoDelOmbra
approved these changes
May 13, 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
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.
Please review my changes :)
Task Update PR template
Review Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation