Conversation
- Remove legacy/obsolete code and custom ProblemDetailsFactory - Add UnhandledException middleware for consistent error responses - Centralize ProblemDetails defaults and RFC mappings - Add extension methods for Result to IActionResult/IResult - Honor JSON naming policies in ProblemDetails serialization - Update service registration for Minimal APIs and MVC - Require .NET 9+, update README and project files - Clean up and modernize codebase throughout
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Arcturus ResultObjects AspNetCore library for .NET 9, removing legacy code and refactoring error/result handling to use built-in ASP.NET Core services. The changes centralize ProblemDetails generation, add middleware for unhandled exceptions, and improve JSON serialization support for naming policies.
Changes:
- Removed obsolete custom ProblemDetailsFactory and replaced with internal factory using ASP.NET Core's IProblemDetailsService
- Added UnhandledExceptionMiddleware for consistent HTTP 500 error responses
- Refactored service registration to support both Minimal APIs and MVC with optional JSON naming policy application
- Removed custom ObjectResult classes in favor of built-in Results.Json() helpers
- Centralized ProblemDetails defaults and added support for correlation IDs, trace IDs, and timestamps
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| Startup/ServiceExtensions.cs | New service registration with ProblemDetailsService and optional JSON naming policy support |
| Startup/ApplicationBuilderExtensions.cs | New middleware registration for unhandled exception handling |
| ServiceExtensions.cs | Deleted legacy service registration with custom factory |
| Results/ProblemDetailsResult.cs | Updated to use new internal factory and IProblemDetailsService |
| Results/ObjectResult.cs | Deleted custom ObjectResult classes |
| ResultObjectOptions.cs | Deleted obsolete options class |
| ResultExtensions.cs | Updated to use Results.Json() instead of custom ObjectResult |
| README.md | Updated .NET requirement from 8 to 9 |
| Middleware/UnhandledExceptionMiddleware.cs | New middleware for consistent unhandled exception responses |
| Json/ProblemDetailsHonorJsonResolver.cs | New resolver to apply JSON naming policies to ProblemDetails |
| Internals/ProblemDetailsFactory.cs | New internal factory for creating ProblemDetails from Result objects |
| Internals/ProblemDetailDefaults.cs | New centralized defaults with RFC mappings and extension properties |
| Internals/FallbackProblemDetailsWriter.cs | New fallback writer when IProblemDetailsService is not registered |
| Internals/JsonSerializerExtensions.cs | Deleted unused JSON serializer extensions |
| Internals/HttpResultsHelper.cs | Deleted obsolete helper methods |
| ArcturusAspNetCoreProblemDetailsFactory.cs | Deleted custom ProblemDetailsFactory implementation |
| Arcturus.Extensions.ResultObjects.AspNetCore.csproj | Code formatting cleanup |
| ActionResults/ProblemDetailsActionResult.cs | Updated to use new factory and IProblemDetailsService |
| ActionResultExtensions.cs | Updated to use centralized DefaultStatusCode constant |
Comments suppressed due to low confidence (2)
src/Arcturus.Extensions.ResultObjects.AspNetCore/ResultExtensions.cs:83
- Dead code comment should be removed. The commented line referring to the old
ObjectResultclass is no longer needed since that class has been deleted as part of this refactoring.
src/Arcturus.Extensions.ResultObjects.AspNetCore/ResultExtensions.cs:82 - Missing
returnstatement. This line creates an IResult but doesn't return it, causing the method to fall through to the error handling logic even when the result is successful with a custom status code. AddreturnbeforeMicrosoft.AspNetCore.Http.Results.Json.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var prop in ti.Properties) | ||
| { | ||
| // Apply the naming policy to the original property name | ||
| prop.Name = namingPolicy.ConvertName(prop.Name); | ||
| } |
There was a problem hiding this comment.
Potential thread safety issue. The code modifies prop.Name for each property in the ti.Properties collection (lines 31-35). If the same JsonTypeInfo instance is reused across multiple requests (which is typical for performance), this could lead to race conditions where multiple threads modify property names concurrently. Consider checking if the property names have already been converted, or ensure that this resolver creates a new type info instance rather than modifying a shared one.
| internal static Task Write(ProblemDetails problemDetails, HttpContext httpContext) | ||
| { | ||
| if (httpContext.Response.HasStarted) | ||
| throw new Exception($"Response has already started."); |
There was a problem hiding this comment.
Using generic Exception type for error handling. Line 11 throws a generic Exception when the response has already started. This should use a more specific exception type like InvalidOperationException to better communicate the nature of the error and allow for proper exception handling by callers.
| internal static Task Write(ProblemDetails problemDetails, HttpContext httpContext) | ||
| { | ||
| if (httpContext.Response.HasStarted) | ||
| throw new Exception($"Response has already started."); |
There was a problem hiding this comment.
Inconsistent error handling compared to UnhandledExceptionMiddleware. The FallbackProblemDetailsWriter.Write method throws an exception when the response has already started (line 11), while the UnhandledExceptionMiddleware silently returns with just a log warning (lines 45-48). This inconsistency means that calling code needs to handle these cases differently. Consider aligning the behavior - either both should throw or both should handle gracefully.
| throw new Exception($"Response has already started."); | |
| return Task.CompletedTask; |
…handledExceptionMiddleware.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…blemDetailDefaults.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…blemDetailDefaults.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ceExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ceExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…handledExceptionMiddleware.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary & Motivation
A brief description of the changes in this pull request explaining why these changes are necessary. Please delete this paragraph.
Checklist