Add HttpClient result helpers with Polly integration #72
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds Minimal API and HttpClient extensions to integrate ManagedCode.Communication Result types, with optional Polly resilience pipelines and corresponding tests/docs.
- Introduces ResultEndpointFilter and WithCommunicationResults helpers for Minimal APIs.
- Adds HttpClient extensions to convert responses to Result/Result with optional Polly pipelines, plus unit tests.
- Updates README and solution/project references to include the new Extensions package.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents Minimal API filter and resilient HttpClient helpers; adds install instructions for Extensions package. |
| ManagedCode.Communication.slnx | Adds the new Extensions project to the solution. |
| ManagedCode.Communication.Tests/ManagedCode.Communication.Tests.csproj | Adds Polly and project reference to Extensions for tests. |
| ManagedCode.Communication.Tests/Extensions/ResultHttpClientExtensionsTests.cs | Adds tests for HttpClient Result helpers including retry via Polly. |
| ManagedCode.Communication.Tests/AspNetCore/Extensions/ResultEndpointFilterTests.cs | Adds tests for Minimal API filter mapping Result types to IResult. |
| ManagedCode.Communication.Extensions/MinimalApi/ResultEndpointFilter.cs | Implements the endpoint filter that maps Result/Result to Minimal API IResult. |
| ManagedCode.Communication.Extensions/MinimalApi/CommunicationEndpointExtensions.cs | Adds extension methods to apply the filter to routes and groups. |
| ManagedCode.Communication.Extensions/ManagedCode.Communication.Extensions.csproj | New package with ASP.NET Core ref and Polly dependency. |
| ManagedCode.Communication.Extensions/Http/ResultHttpClientExtensions.cs | Implements SendForResultAsync/GetAsResultAsync with optional Polly pipeline. |
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using ManagedCode.Communication; | ||
| using ManagedCode.Communication.Extensions; |
There was a problem hiding this comment.
The extension methods FromJsonToResult() and FromRequestToResult() are defined in the ASP.NET Core extensions, not in ManagedCode.Communication.Extensions. Replace the using with ManagedCode.Communication.AspNetCore.Extensions so this file compiles.
| using ManagedCode.Communication.Extensions; | |
| using ManagedCode.Communication.AspNetCore.Extensions; |
Copilot uses AI. Check for mistakes.
| The helpers use the existing `HttpResponseMessage` converters, so non-success status codes automatically map to a | ||
| `Problem` with the response body and status code. | ||
| success responses map to `200 OK`/`204 No Content` while failures become RFC 7807 problem details. Native `Microsoft.AspNetCore.Http.IResult` | ||
| responses pass through unchanged, so you can mix and match traditional Minimal API patterns with ManagedCode.Communication results. |
There was a problem hiding this comment.
Lines 235–236 describe Minimal API behavior (IResult pass-through and status mapping), but this section is about HttpClient helpers. Move or reword these lines under the Minimal API section to avoid confusion, or clarify how HttpClient helpers map responses to Result/Result.
Copilot uses AI. Check for mistakes.
| using ManagedCode.Communication.Extensions.Http; | ||
| using Polly; | ||
| using Polly.Retry; | ||
| using Shouldly; |
There was a problem hiding this comment.
Tests in this repo use xUnit with FluentAssertions. Please switch assertions to FluentAssertions for consistency (e.g., result.IsSuccess.Should().BeTrue()) and remove Shouldly.
Copilot uses AI. Check for mistakes.
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.TestHost; | ||
| using HttpResults = Microsoft.AspNetCore.Http.Results; | ||
| using Shouldly; |
There was a problem hiding this comment.
Align test assertions with FluentAssertions as used across the repo and drop Shouldly for consistency.
Copilot uses AI. Check for mistakes.
| <Description>Optional integrations for ManagedCode.Communication including Minimal API endpoint filters.</Description> | ||
| <PackageTags>managedcode;communication;result-pattern;minimal-api;endpoint-filter</PackageTags> |
There was a problem hiding this comment.
Update the package description and tags to also mention HttpClient helpers and Polly resilience (e.g., 'HttpClient result helpers; Polly resilience pipelines') so NuGet metadata reflects the new functionality.
| <Description>Optional integrations for ManagedCode.Communication including Minimal API endpoint filters.</Description> | |
| <PackageTags>managedcode;communication;result-pattern;minimal-api;endpoint-filter</PackageTags> | |
| <Description>Optional integrations for ManagedCode.Communication including Minimal API endpoint filters, HttpClient result helpers, and Polly resilience pipelines.</Description> | |
| <PackageTags>managedcode;communication;result-pattern;minimal-api;endpoint-filter;httpclient;polly;resilience;pipeline</PackageTags> |
Copilot uses AI. Check for mistakes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| var httpResponse = await pipeline.ExecuteAsync( | ||
| async cancellationToken => | ||
| { | ||
| using var request = requestFactory(); | ||
| return await client.SendAsync(request, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The lambda passed to ResiliencePipeline.ExecuteAsync returns each HttpResponseMessage to Polly without disposing the ones that are retried. When a retry strategy handles non‑success status codes, every discarded response leaks its content stream and the associated socket until final GC, which can exhaust the HttpClient connection pool during transient outages. Dispose or buffer unsuccessful responses inside the delegate before returning so that retries do not leave dangling connections.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ed4be67dc4832691131300b23d447b