Improve API Error Handling #169
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR upgrades the solution from .NET 8.0 to .NET 10.0 across all projects and Docker images. It introduces a centralized exception handling framework with a marker interface Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as ASP.NET Core API
participant Middleware as Exception Handling<br/>Middleware
participant Handler as IExceptionHandler
participant Response as ProblemDetails<br/>Response
Client->>API: HTTP Request
API->>API: Process Request
alt Caller-Fixable Error
API->>API: Throw CallerFixableHttpRequestException<br/>(implements ICallerFixableException)
API->>Middleware: Exception caught
Middleware->>Handler: Retrieve IExceptionHandlerFeature
Handler->>Handler: Check: ICallerFixableException?
Handler->>Handler: Status = 400<br/>Title = "Bad Request"<br/>Detail = exception.Message
Handler->>Response: Create ProblemDetails
Response->>Client: 400 + ProblemDetails
else Server Error
API->>API: Throw generic Exception
API->>Middleware: Exception caught
Middleware->>Handler: Retrieve IExceptionHandlerFeature
Handler->>Handler: Check: ICallerFixableException?<br/>(false)
Handler->>Handler: Status = 500<br/>Title = "Internal Server Error"<br/>Detail = "generic message"
Handler->>Response: Create ProblemDetails
Response->>Client: 500 + ProblemDetails
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Elzik.Breef.Api/Program.cs (1)
48-48: LGTM! ProblemDetails middleware registered correctly.The registration enables standardized RFC 7807 error responses. The default configuration is appropriate and integrates well with the exception handling middleware added below.
For future enhancement, consider customizing ProblemDetails to include additional metadata:
-builder.Services.AddProblemDetails(); +builder.Services.AddProblemDetails(options => +{ + options.CustomizeProblemDetails = context => + { + context.ProblemDetails.Extensions["traceId"] = context.HttpContext.TraceIdentifier; + // Add other custom fields as needed + }; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Elzik.Breef.Api/Dockerfile(1 hunks)src/Elzik.Breef.Api/Program.cs(2 hunks)src/Elzik.Breef.Api/Properties/launchSettings.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Elzik.Breef.Api/Program.cs (1)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs (1)
UseAuth(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
src/Elzik.Breef.Api/Dockerfile (1)
29-29: LGTM! Production environment configured correctly.Setting the environment to Production in the Docker image ensures that the exception handling middleware will use the sanitized error responses configured in Program.cs rather than exposing detailed error information.
src/Elzik.Breef.Api/Properties/launchSettings.json (1)
11-18: LGTM! Valuable addition for testing production behavior locally.The new launch profile enables developers to test the production exception handling configuration (ProblemDetails responses, UseExceptionHandler) without deploying to a production environment.
src/Elzik.Breef.Api/Program.cs (2)
143-150: LGTM! Environment-specific exception handling configured correctly.The implementation properly separates development and production error handling:
- Development gets detailed exception pages for debugging
- Production uses the ProblemDetails middleware (registered on line 48) to return sanitized, standardized error responses
The parameterless
UseExceptionHandler()call correctly integrates with the ProblemDetails middleware.
152-152: LGTM! Status code pages middleware properly positioned.The middleware will generate ProblemDetails responses for HTTP status codes (404, 401, etc.) and is correctly placed after exception handling but before routing.
There was a problem hiding this comment.
Pull Request Overview
This PR adds explicit environment configuration and exception handling middleware to the API. The changes ensure proper error handling behavior based on the environment (Development vs Production) and provide a new launch profile for local testing in Production mode.
- Added environment-aware exception handling middleware (developer exception page for Development, standard exception handler for Production)
- Added ProblemDetails support and status code pages middleware
- Created a new launch profile "Local (Production)" for testing production behavior locally
- Explicitly set ASPNETCORE_ENVIRONMENT=Production in Dockerfile
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Elzik.Breef.Api/Properties/launchSettings.json | Added "Local (Production)" launch profile for testing production environment locally |
| src/Elzik.Breef.Api/Program.cs | Added environment-aware exception handling middleware, ProblemDetails service, and status code pages middleware |
| src/Elzik.Breef.Api/Dockerfile | Explicitly set ASPNETCORE_ENVIRONMENT to Production in the final Docker image |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (1)
82-90: Excellent validation of ProblemDetails response format.The test correctly validates the structured error response, checking Status, Title, and Type properties. The use of existing JsonSerializerOptions and consistent Shouldly assertions is good practice.
Consider adding Content-Type header validation to ensure the API returns the correct media type:
challenge.Parameter.ShouldContain("BREEF-API-KEY"); + response.Content.Headers.ContentType?.MediaType.ShouldBe("application/problem+json"); + var responseString = await response.Content.ReadAsStringAsync();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (1)
3-3: LGTM! Necessary import for ProblemDetails.The new using directive is required for the ProblemDetails type used in the test assertions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/breef into improve-api-error-handling
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Elzik.Breef.Api/Program.cs(2 hunks)tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Elzik.Breef.Api/Program.cs (1)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs (1)
UseAuth(26-30)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (1)
src/Elzik.Breef.Api/Program.cs (1)
Task(26-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
src/Elzik.Breef.Api/Program.cs (3)
48-48: LGTM: ProblemDetails middleware registered correctly.The registration of ProblemDetails services is standard and appropriate for structured error responses conforming to RFC 7807.
152-152: Verify middleware ordering forUseStatusCodePages.
UseStatusCodePages()is positioned before authentication/authorization middleware. This ordering may prevent proper ProblemDetails formatting for authentication failures (401) and authorization failures (403). Typically, status code pages middleware should be positioned after auth middleware or may not be needed at all since ProblemDetails middleware handles status code formatting automatically.Consider one of these approaches:
- Remove
UseStatusCodePages()if ProblemDetails middleware alone provides sufficient error formatting- Move it after
UseAuth()if explicit status code page handling is requiredTest whether 401/403 responses are properly formatted with the current ordering.
157-158: LGTM: Health endpoint configured correctly.The health endpoint mapping and
.AllowAnonymous()call are properly formatted and configured.tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (2)
3-3: LGTM: Required using statement added.The
Microsoft.AspNetCore.Mvcnamespace is correctly added forProblemDetailstype usage.
81-95: ProblemDetails validation logic is sound.The assertions properly verify RFC 7807 ProblemDetails format for unauthorized responses: content-type, status code, title, and type URI. However, this validation is only effective once the environment configuration issue (Line 71) is resolved.
Note: Different environments may produce different
ProblemDetailscontent (e.g., Development might include additional debug information). Verify that both environments return consistent fields that the test asserts on, or adjust assertions to be environment-aware.
…xcetion page" This reverts commit 4b34d4b.
…as development excetion page
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeDevelopment.cs (1)
12-17: Critical: Environment variable setting doesn't affect the WebApplicationFactory.Setting
ASPNETCORE_ENVIRONMENTat Line 14 has no effect on the injectedWebApplicationFactory<Program>. The factory's environment is determined when xUnit creates it (before the constructor runs), not during test initialization. The test will not actually run in Development environment as intended.Apply this diff to properly configure the environment:
public HealthTestsNativeDevelopment(WebApplicationFactory<Program> webAppFactory) { - Environment.SetEnvironmentVariable("ASPNETCORE_ENVIRONMENT", "Development"); - _webApplicationFactory = webAppFactory; + _webApplicationFactory = webAppFactory.WithWebHostBuilder(builder => + { + builder.UseEnvironment("Development"); + }); _client = _webApplicationFactory.CreateClient(); }
🧹 Nitpick comments (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeProduction.cs (1)
2-2: Optional: Remove unused System import after applying the fix.After applying the suggested fix to the constructor (removing
Environment.SetEnvironmentVariable), theSystemnamespace import will no longer be needed and can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs(2 hunks)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeDevelopment.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeProduction.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeProduction.cs (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsBase.cs (1)
HealthTestsBase(7-34)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeDevelopment.cs (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsBase.cs (1)
HealthTestsBase(7-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (2)
3-3: LGTM: Required using for ProblemDetails.The addition of
Microsoft.AspNetCore.Mvcnamespace is necessary to support the new ProblemDetails validation logic.
82-90: LGTM: Comprehensive ProblemDetails validation.The test correctly validates that unauthorized responses now return ProblemDetails with appropriate status, title, and type fields. This aligns well with the PR objective of improving API error handling.
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNativeProduction.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
10-10: Fix indentation for consistency.The line lacks proper indentation compared to the surrounding code.
Apply this diff:
-builder.UseEnvironment("Production"); + builder.UseEnvironment("Production");tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (3)
13-16: Fix indentation for consistency.Line 14 has an extra space before the opening brace.
Apply this diff:
- public HealthTestsNative(ITestOutputHelper output) - { + public HealthTestsNative(ITestOutputHelper output) + {
31-34: Consider removing unnecessary baseUrl constant.The
baseUrlconstant may be unnecessary, asCreateClient()typically configures the base address automatically. You can likely simplify toawait client.GetAsync("/health").If the current approach works, consider this diff:
- using var client = factory.CreateClient(); - const string baseUrl = "http://localhost"; - - // Act - var response = await client.GetAsync($"{baseUrl}/health"); + using var client = factory.CreateClient(); + + // Act + var response = await client.GetAsync("/health");
43-46: Consider extracting shared HealthResponse type.The
HealthResponseclass appears similar to the one inHealthTestsBase.cs(lines 29-32). If both are identical, consider extracting it to a shared location to reduce duplication. However, keeping separate copies may be acceptable if test isolation is preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
ConfigureWebHost(8-11)
tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
ConfigureWebHost(8-11)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (4)
tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
DevelopmentWebApplicationFactory(6-12)tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
ProductionWebApplicationFactory(6-12)src/Elzik.Breef.Api/Program.cs (2)
Program(19-178)Program(21-24)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsBase.cs (1)
HealthResponse(30-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
6-11: LGTM! Clean test factory implementation.The factory correctly configures the Production environment for functional testing and mirrors the Development factory pattern.
tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
6-11: LGTM! Well-structured test factory.The factory correctly configures the Development environment for functional testing with consistent formatting.
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (2)
18-22: Verify factory disposal in test data provider.The data provider creates new factory instances for each test execution. Ensure that xUnit properly disposes of these
WebApplicationFactoryinstances, as they implementIDisposable. If disposal issues arise, consider usingIClassFixtureor implementingIDisposableon the test class to manage factory lifecycle explicitly.
24-41: Well-structured parameterized test!The test effectively validates the health endpoint across both Development and Production environments using xUnit's Theory/MemberData pattern. The logging with
ITestOutputHelperaids debugging, and the assertions properly validate both status code and response body structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (2)
34-37: Remove unnecessary baseUrl constant.The
HttpClientreturned byfactory.CreateClient()is already configured with the appropriate base address. You can use a relative path directly.Apply this diff:
using var client = factory.CreateClient(); - const string baseUrl = "http://localhost"; // Act - var response = await client.GetAsync($"{baseUrl}/health"); + var response = await client.GetAsync("/health");
14-14: Fix inconsistent indentation.Several lines have inconsistent indentation with extra spaces. Consider running a code formatter to ensure consistent formatting throughout the file.
Lines affected: 14, 31, 32, 37, 41, 43, 48.
Also applies to: 31-32, 37-37, 41-41, 43-43, 48-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Elzik.Breef.Api/Program.cs(2 hunks)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Elzik.Breef.Api/Program.cs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (4)
src/Elzik.Breef.Api/Program.cs (2)
Program(19-178)Program(21-24)tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
DevelopmentWebApplicationFactory(6-12)tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
ProductionWebApplicationFactory(6-12)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsBase.cs (1)
HealthResponse(30-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (2)
11-16: LGTM!Standard xUnit pattern for capturing test output.
46-49: LGTM!Simple DTO for deserializing the health check response. The property name casing difference from the endpoint (lowercase "status" in Program.cs vs capital "Status" here) is handled correctly by .NET's case-insensitive JSON deserialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (1)
31-34: Remove unnecessary baseUrl constant.The
WebApplicationFactory.CreateClient()method returns anHttpClientthat's already configured with a base address pointing to the test server. ThebaseUrlconstant is redundant and potentially confusing.Apply this diff to simplify the code:
- using var client = factory.CreateClient(); - const string baseUrl = "http://localhost"; - + using var client = factory.CreateClient(); + // Act - var response = await client.GetAsync($"{baseUrl}/health"); + var response = await client.GetAsync("/health");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (4)
src/Elzik.Breef.Api/Program.cs (2)
Program(19-178)Program(21-24)tests/Elzik.Breef.Api.Tests.Functional/DevelopmentWebApplicationFactory.cs (1)
DevelopmentWebApplicationFactory(6-12)tests/Elzik.Breef.Api.Tests.Functional/ProductionWebApplicationFactory.cs (1)
ProductionWebApplicationFactory(6-12)tests/Elzik.Breef.Api.Tests.Functional/HealthTestsBase.cs (1)
HealthResponse(30-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
tests/Elzik.Breef.Api.Tests.Functional/HealthTestsNative.cs (3)
11-16: LGTM!Standard xUnit test constructor pattern for capturing test output.
26-30: Excellent fix for the resource leak!The
usingstatements for both the factory and client properly address the resource leak issue flagged in the previous review. Both disposable resources will now be cleaned up at test completion.
43-46: LGTM!Simple and appropriate DTO for deserializing the health endpoint response.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Elzik.Breef.sln(0 hunks)tests/SampleRequests/Elzik.Breef.Api.http(1 hunks)
💤 Files with no reviewable changes (1)
- Elzik.Breef.sln
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…xcetion page" This reverts commit 4b34d4b.
…as development excetion page
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Elzik.Breef.Domain/ICallerFixableException.cs (1)
1-8: Marker interface is clear; consider constraining usage by conventionThe
ICallerFixableExceptionmarker is simple and clearly documented; it should work well for centralised exception handling.Optionally, to avoid accidental misuse on non‑exception types, you might:
- Enforce via code review/convention, or
- Introduce a common base exception type (e.g.,
CallerFixableException : Exception) instead of a marker, if you later need shared behavior.src/Elzik.Breef.Infrastructure/ContentExtractors/HtmlContentExtractor.cs (1)
3-3: Remove unused import.The
Microsoft.Extensions.Loggingusing directive is not used in this file. Consider removing it to keep the code clean.using Elzik.Breef.Domain; using HtmlAgilityPack; -using Microsoft.Extensions.Logging; using Elzik.Breef.Infrastructure;src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (1)
13-50: Consider logging exceptions for observability.The exception handler converts exceptions to ProblemDetails responses but doesn't log them. Adding logging would improve debugging and monitoring capabilities, especially for 500-level errors.
Consider injecting an
ILoggerand logging exceptions:public static void UseExceptionHandling(this WebApplication app) { + var logger = app.Services.GetRequiredService<ILogger<ExceptionHandlingExtensions>>(); + app.UseExceptionHandler(errorApp => { errorApp.Run(async context => { var exceptionHandlerFeature = context.Features.Get<IExceptionHandlerFeature>(); var exception = exceptionHandlerFeature?.Error; int statusCode; string title; string detail; if (exception is CallerFixableHttpRequestException callerFixable) { statusCode = StatusCodes.Status400BadRequest; title = "There was a problem with your request."; detail = callerFixable.Message; + logger.LogWarning(callerFixable, "Caller-fixable error: {Message}", callerFixable.Message); } else { statusCode = StatusCodes.Status500InternalServerError; title = "An error occurred while processing your request."; detail = "Contact your Breef administrator for a solution."; + logger.LogError(exception, "Unhandled exception: {Message}", exception?.Message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Elzik.Breef.Api/Dockerfile(2 hunks)src/Elzik.Breef.Api/Elzik.Breef.Api.csproj(2 hunks)src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs(1 hunks)src/Elzik.Breef.Api/Program.cs(4 hunks)src/Elzik.Breef.Application/Elzik.Breef.Application.csproj(1 hunks)src/Elzik.Breef.Domain/Elzik.Breef.Domain.csproj(1 hunks)src/Elzik.Breef.Domain/ICallerFixableException.cs(1 hunks)src/Elzik.Breef.Infrastructure/CallerFixableHttpRequestException.cs(1 hunks)src/Elzik.Breef.Infrastructure/ContentExtractors/HtmlContentExtractor.cs(2 hunks)src/Elzik.Breef.Infrastructure/Elzik.Breef.Infrastructure.csproj(2 hunks)tests/Elzik.Breef.Api.Tests.Functional/Elzik.Breef.Api.Tests.Functional.csproj(2 hunks)tests/Elzik.Breef.Api.Tests.Integration/Elzik.Breef.Api.Tests.Integration.csproj(2 hunks)tests/Elzik.Breef.Infrastructure.Tests.Integration/Elzik.Breef.Infrastructure.Tests.Integration.csproj(2 hunks)tests/Elzik.Breef.Infrastructure.Tests.Unit/Elzik.Breef.Infrastructure.Tests.Unit.csproj(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (1)
src/Elzik.Breef.Infrastructure/CallerFixableHttpRequestException.cs (2)
CallerFixableHttpRequestException(5-11)CallerFixableHttpRequestException(7-10)
src/Elzik.Breef.Domain/ICallerFixableException.cs (6)
src/Elzik.Breef.Domain/IContentSummariser.cs (2)
Elzik(1-7)IContentSummariser(3-6)src/Elzik.Breef.Application/IBreefGenerator.cs (2)
Elzik(1-7)IBreefGenerator(3-6)src/Elzik.Breef.Domain/Breef.cs (2)
Elzik(1-8)Breef(3-7)src/Elzik.Breef.Infrastructure/AI/AiContentSummariser.cs (1)
AiContentSummariser(8-40)src/Elzik.Breef.Domain/IBreefPublisher.cs (2)
IBreefPublisher(3-6)Elzik(1-7)src/Elzik.Breef.Application/BreefGenerator.cs (1)
Elzik(3-26)
src/Elzik.Breef.Api/Program.cs (2)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (2)
AddExceptionHandling(8-11)UseExceptionHandling(13-50)src/Elzik.Breef.Api/Auth/AuthExtensions.cs (2)
UseAuth(26-30)AddAuth(8-24)
tests/Elzik.Breef.Infrastructure.Tests.Unit/Elzik.Breef.Infrastructure.Tests.Unit.csproj (1)
tests/Elzik.Breef.Infrastructure.Tests.Integration/AiContentSummariserTests.cs (2)
Theory(21-55)AiContentSummariserTests(15-56)
src/Elzik.Breef.Infrastructure/ContentExtractors/HtmlContentExtractor.cs (1)
src/Elzik.Breef.Infrastructure/CallerFixableHttpRequestException.cs (2)
CallerFixableHttpRequestException(5-11)CallerFixableHttpRequestException(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (16)
src/Elzik.Breef.Api/Elzik.Breef.Api.csproj (1)
4-20: Ensure net10.0 + package versions are compatible and deployed everywhereTargeting
net10.0and bumpingMicrosoft.SemanticKernelandSerilog.Sinks.Consoleis fine conceptually, but please confirm:
- All consuming projects and deployment environments are also on .NET 10.
- These specific package versions are supported with net10.0 and don’t introduce breaking changes in your current usage (especially SK).
src/Elzik.Breef.Application/Elzik.Breef.Application.csproj (1)
4-4: Verify solution-wide alignment on net10.0The application project now targets
net10.0. Please double‑check:
- All referenced projects and test projects also target net10.0 (or compatible) and build cleanly in CI.
- Build agents and runtime hosts (Docker images, app service, etc.) are configured with .NET 10.
src/Elzik.Breef.Api/Dockerfile (1)
4-4: Confirm .NET 10 images and environment behavior across environmentsThe move to
aspnet:10.0/sdk:10.0and settingASPNETCORE_ENVIRONMENT=Productionin the final stage look consistent with the net10.0 upgrade, but please verify:
- The 10.0 images you reference exist in your registries/CI and match the framework used by the binaries.
- For non‑prod runs (e.g., local docker-compose, staging), you’re explicitly overriding
ASPNETCORE_ENVIRONMENTas needed so this defaultProductiondoesn’t mask environment‑specific configurations.Also applies to: 11-11, 29-29
src/Elzik.Breef.Infrastructure/Elzik.Breef.Infrastructure.csproj (1)
4-26: Check infra-package compatibility and configuration behavior under net10.0Upgrading infra to
net10.0plusMicrosoft.Extensions.* 10.0.0and SK connectors1.67.1keeps things aligned, but please confirm:
- These versions are supported together under .NET 10 and don’t introduce breaking API/behavioral changes in your DI, options binding, and Semantic Kernel usage.
- The
SummarisationInstructions\**\*.mdNoneitem withLink=SummarisationInstructions\%(RecursiveDir)%(Filename)%(Extension)still matches how these files are loaded at runtime (paths/relative locations unchanged).Also applies to: 31-31
src/Elzik.Breef.Infrastructure/ContentExtractors/HtmlContentExtractor.cs (1)
18-26: LGTM - Good error handling pattern.The try-catch block appropriately wraps HTTP request exceptions with contextual information (the URL) and preserves the original exception. This aligns well with the centralized exception handling strategy.
src/Elzik.Breef.Infrastructure/CallerFixableHttpRequestException.cs (1)
1-12: LGTM - Clean exception type implementation.The exception class follows standard .NET exception patterns with proper base class chaining and the marker interface enables the exception handling middleware to route it correctly.
src/Elzik.Breef.Api/Program.cs (3)
2-2: LGTM - Necessary imports for exception handling.The new using directives support the exception handling middleware being wired up in this file.
Also applies to: 13-14
51-51: LGTM - Exception handling services registered.The
AddExceptionHandling()call properly registers ProblemDetails services for centralized error handling.
146-148: LGTM - Middleware pipeline correctly configured.The status code pages and exception handling middleware are properly positioned early in the pipeline, before CORS and authentication, ensuring all errors are handled consistently.
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (2)
8-11: LGTM - ProblemDetails services registered.The method correctly registers ASP.NET Core's ProblemDetails services for standardized error responses.
38-47: LGTM - ProblemDetails response correctly structured.The ProblemDetails response is properly configured with appropriate status codes, content type, and JSON serialization following RFC 7807 standards.
src/Elzik.Breef.Domain/Elzik.Breef.Domain.csproj (1)
4-4: No issues found—versions are valid and compatible..NET 10.0 is a valid target framework using the TFM net10.0, and Microsoft.SemanticKernel.Abstractions version 1.67.1 is published on NuGet and is compatible with net10.0.
tests/Elzik.Breef.Infrastructure.Tests.Unit/Elzik.Breef.Infrastructure.Tests.Unit.csproj (1)
4-4: No breaking changes detected for test execution; package upgrades are compatible.Analysis shows that while Microsoft.Extensions.Options 10.0.0 has a breaking change where null values are now preserved and bound, the test configurations in RedditOptionsTests.cs contain only non-null values and empty configurations. The test data does not exercise the null-handling behavior change, so these specific tests will not regress.
Microsoft.Extensions.Diagnostics.Testing 10.0.0 has no package-specific breaking changes, confirming compatibility.
The package versions align with .NET 10.0 ecosystem requirements. However, verify that production configuration files (if any) do not rely on the previous null-ignoring behavior, as that could surface at runtime even if tests pass.
tests/Elzik.Breef.Api.Tests.Functional/Elzik.Breef.Api.Tests.Functional.csproj (1)
21-23: Package version updates are appropriate for .NET 10.0 target.The patch and minor version updates (Microsoft.NET.Test.Sdk, Microsoft.SemanticKernel, xunit.analyzers) align with the framework migration and are low-risk. Coverage and test tooling versions remain stable.
Also applies to: 31-31
tests/Elzik.Breef.Api.Tests.Integration/Elzik.Breef.Api.Tests.Integration.csproj (1)
4-4: Framework and dependency updates align with .NET 10.0 migration.Microsoft.Extensions.* packages correctly aligned to 10.0.0 for net10.0 target. Test SDK and analyzer updates are routine and appropriate.
Also applies to: 18-20, 27-27
tests/Elzik.Breef.Infrastructure.Tests.Integration/Elzik.Breef.Infrastructure.Tests.Integration.csproj (1)
21-29: Infrastructure package updates are comprehensive and well-coordinated.Microsoft.Extensions.* packages (Configuration, DependencyInjection, Options, Diagnostics.Testing) are consistently aligned to 10.0.0, which is appropriate for the net10.0 target. The coordinated upgrade of this dependency cluster reduces version mismatch risks.
Confirm that the infrastructure project (Elzik.Breef.Infrastructure) has been updated to match these configuration/options/DI versions to avoid version conflicts.
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs
Outdated
Show resolved
Hide resolved
tests/Elzik.Breef.Api.Tests.Functional/Elzik.Breef.Api.Tests.Functional.csproj
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (2)
37-42: Consider logging internal server errors.For 500 errors, the actual exception details are hidden from the caller (correctly for security), but there's no logging of the exception for debugging purposes. Consider logging the exception before returning the generic response.
Example implementation:
else { var logger = context.RequestServices.GetRequiredService<ILogger<Program>>(); logger.LogError(exception, "Unhandled exception occurred while processing request"); statusCode = StatusCodes.Status500InternalServerError; title = "An error occurred while processing your request"; detail = "Contact your Breef administrator for a solution."; }
41-41: Consider adjusting error message for different deployment scenarios.The phrase "Contact your Breef administrator" assumes users have access to an administrator. For SaaS or public deployments, consider more generic wording like "Please contact support" or make the message configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/ExceptionHandlingTests.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build-ubuntu
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs
[warning] 27-27:
Remove this unnecessary check for null. (https://rules.sonarsource.com/csharp/RSPEC-2589)
[warning] 27-27:
Remove this unnecessary check for null. (https://rules.sonarsource.com/csharp/RSPEC-2589)
🪛 GitHub Check: SonarCloud Code Analysis
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs
[warning] 27-27: Remove this unnecessary check for null.
🔇 Additional comments (7)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (4)
8-11: LGTM!The service registration is clean and appropriately delegates to the built-in ProblemDetails services.
13-24: LGTM!The middleware setup follows ASP.NET Core conventions and properly extracts exception information via
IExceptionHandlerFeature.
44-54: LGTM!The ProblemDetails construction and response handling correctly follows RFC 7807 with the appropriate content type and status code.
35-35: Verification complete: ICallerFixableException implementation is secure.Found one implementation (
CallerFixableHttpRequestException) with one usage inHtmlContentExtractor.cs:25. The message construction includes only the caller-provided URL ("Failed to download content for URL: {webPageUrl}"), with no connection strings, API keys, internal paths, or PII exposure. The exception message appropriately reflects the download context for caller troubleshooting.tests/Elzik.Breef.Api.Tests.Functional/ExceptionHandlingTests.cs (3)
13-38: LGTM!The test effectively validates the 400 Bad Request response for caller-fixable errors. The use of a non-existent URL is an appropriate way to trigger the expected exception path, and the assertions comprehensively verify the ProblemDetails structure.
40-70: LGTM!The test effectively validates the 500 Internal Server Error response for non-fixable exceptions. The DI manipulation via
ForceBreefGeneratorExceptioncleanly isolates the error handling behavior. Note that while a real external URL is used in the request, the forced exception means the URL likely isn't accessed, so the test remains deterministic.
72-81: LGTM!The helper method correctly replaces the
IBreefGeneratorservice with a failing implementation for testing purposes. The pattern of removing the existing descriptor and registering a new one is appropriate for functional testing scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (1)
13-56: Log exceptions and optionally delegate to the ProblemDetails serviceRight now the handler only writes a ProblemDetails response and never logs the underlying exception. This makes production diagnostics harder, especially for the 500 branch. Consider resolving an
ILogger<ExceptionHandlingExtensions>(orILoggerFactory) fromcontext.RequestServicesand loggingexceptionbefore writing the response.Also, since you register
AddProblemDetails, you may want to (optionally) useIProblemDetailsServicehere instead of manually constructing and serializingProblemDetails, so any global configuration (extensions, trace IDs, etc.) is applied consistently across the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs (1)
1-56: Centralized handler and ProblemDetails wiring look solidThe distinction between caller-fixable exceptions (400 with specific detail) and all other exceptions (500 with a generic message) is clear and the ProblemDetails payload is well-structured and consistent.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsBase.cs (1)
5-28: Health test base namespace looks good; BaseUrl pattern is fine (optional simplification)Moving
HealthTestsBaseinto the.Healthnamespace keeps the health test suite cohesive, and the existingBaseUrl/HostPort/Clientpattern works for both native and Docker variants. If you ever want to reduce coupling to explicit ports for native tests, you could instead rely on the client’s configured base address and issue relative requests (like/health) from the base class, but that’s purely optional.tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs (1)
3-91: Unauthorized Breefs test now validates error payload shapePulling in MVC’s problem-details type and asserting against the deserialized 401 response (status, title, and non-empty type) gives good coverage of the new centralized error-handling contract and should catch regressions in the unauthorized case. As an optional enhancement, you could also assert that the response content type is
application/problem+jsonto fully lock in the error format, for example:problemDetails.Title.ShouldBe("Unauthorized"); problemDetails.Type.ShouldNotBeNullOrWhiteSpace(); + response.Content.Headers.ContentType?.MediaType.ShouldBe("application/problem+json");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Elzik.Breef.Api/Elzik.Breef.Api.csproj(2 hunks)src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs(1 hunks)src/Elzik.Breef.Api/Program.cs(5 hunks)src/Elzik.Breef.Api/Properties/launchSettings.json(0 hunks)tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs(2 hunks)tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsDocker.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsNative.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/ExceptionHandlingTests.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsBase.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsDocker.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsNative.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Elzik.Breef.Api/Properties/launchSettings.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Elzik.Breef.Api.Tests.Functional/ExceptionHandlingTests.cs
- src/Elzik.Breef.Api/Program.cs
- src/Elzik.Breef.Api/ExceptionHandling/ExceptionHandlingExtensions.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/Elzik.Breef.Api/Elzik.Breef.Api.csproj (4)
13-13: Microsoft.SemanticKernel 1.67.1 is compatible with .NET 10.0.The package is compatible with .NET 10.0, targeting .NET 8.0 but listing net10.0 as a computed compatible target. The update from 1.65.0 to 1.67.1 poses no compatibility concerns for the target framework.
18-18: Based on the web search results, I can now provide the rewritten review comment:Serilog.Enrichers.AspNetCore 1.0.0 is compatible with the specified versions.
The package requires Serilog (>= 2.6.0), so Serilog 4.3.0 satisfies this requirement. It targets .NETStandard and is compatible with .NET 10.0.
Note: Serilog.Enrichers.AspNetCore and Serilog.Extensions.Hosting are independent packages in the Serilog ecosystem—the enricher does not depend on Serilog.Extensions.Hosting 9.0.0. Both packages are compatible with Serilog 4.3.0 and can coexist without issues.
Verify that the enricher is properly registered in your centralized exception handling middleware pipeline.
21-21: Serilog.Sinks.Console 6.1.1 is compatible with 6.0.0 — no action needed.No breaking changes are documented for Serilog.Sinks.Console 6.1.1 versus 6.0.0, as confirmed by both NuGet and the GitHub release notes. The minor version update is safe and compatible with the exception handling improvements in this PR.
4-4: All .NET 10.0 compatibility concerns verified as non-issues.Serilog.Enrichers.AspNetCore 1.0.0 targets .NET Standard 2.0 and NuGet lists net10.0 as a computed compatible framework. Microsoft.SemanticKernel 1.67.1 is listed by NuGet as compatible with net10.0. Refit 8.0.0 does not include an explicit net10.0 build, but it targets net6.0, net8.0, and netstandard2.0—all of which are compatible with .NET 10—and NuGet marks net10.0 as a compatible runtime. All referenced projects (Domain, Application, Infrastructure) have been consistently updated to net10.0. The framework upgrade is safe to proceed.
tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsNative.cs (1)
2-7: Health native test namespace/imports aligned with new structureBringing this test under
Elzik.Breef.Api.Tests.Functional.Healthwith the added test-focused usings is consistent with the other Health test classes; no issues spotted.tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsDocker.cs (1)
6-6: Breefs Docker tests correctly moved into Breefs namespaceThe namespace now matches the Breefs-specific functional tests; behavior and Docker test wiring remain unchanged.
tests/Elzik.Breef.Api.Tests.Functional/Health/HealthTestsDocker.cs (1)
6-6: Health Docker tests namespace updated consistentlyAligning this class under
Elzik.Breef.Api.Tests.Functional.Healthkeeps all health-related Docker and native tests together; no further issues noticed.tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsNative.cs (1)
3-3: Breefs native tests correctly re-namespacedUpdating the namespace to
Elzik.Breef.Api.Tests.Functional.Breefskeeps the native tests aligned with the Breefs test base and Docker variant; no behavior change and looks good.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.