From f615ef3a4a0b130c471cf054220b1a8a4aae75c9 Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Sat, 7 Feb 2026 01:23:54 +0100 Subject: [PATCH] Standardize service errors with ProblemDetails envelope --- Endpoints/AdminWorkflowService.cs | 14 ++++++------- Endpoints/AuthEndpoints.cs | 10 ++++----- Endpoints/EndpointHelpers.cs | 24 +++++++++++++++++++++- Endpoints/ResultsWorkflowService.cs | 2 +- Endpoints/StateEndpoints.cs | 4 ++-- Endpoints/SuggestionWorkflowService.cs | 14 ++++++------- Endpoints/VoteWorkflowService.cs | 8 ++++---- Infrastructure/PlayerIdentityExtensions.cs | 10 ++++++++- REVIEW.md | 10 ++++----- 9 files changed, 63 insertions(+), 33 deletions(-) diff --git a/Endpoints/AdminWorkflowService.cs b/Endpoints/AdminWorkflowService.cs index 13c471a..041faae 100644 --- a/Endpoints/AdminWorkflowService.cs +++ b/Endpoints/AdminWorkflowService.cs @@ -48,11 +48,11 @@ internal sealed class AdminWorkflowService(AppDbContext db) { var player = await db.Players.FirstOrDefaultAsync(p => p.Id == request.PlayerId); if (player is null) - return Results.NotFound(new { error = "Player not found." }); + return EndpointHelpers.NotFoundError("Player not found."); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); if (phase != Phase.Vote) - return Results.BadRequest(new { error = "Player must be in the Vote phase to receive a joker." }); + return EndpointHelpers.BadRequestError("Player must be in the Vote phase to receive a joker."); player.HasJoker = true; player.VotesFinal = false; @@ -65,7 +65,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) { var player = await db.Players.Include(p => p.Suggestions).FirstOrDefaultAsync(p => p.Id == playerId); if (player is null) - return Results.NotFound(new { error = "Player not found." }); + return EndpointHelpers.NotFoundError("Player not found."); await using var tx = await db.Database.BeginTransactionAsync(); @@ -95,20 +95,20 @@ internal sealed class AdminWorkflowService(AppDbContext db) return EndpointHelpers.PhaseMismatch(Phase.Vote, phase); if (request.SourceSuggestionId == request.TargetSuggestionId) - return Results.BadRequest(new { error = "Pick two different games to link." }); + return EndpointHelpers.BadRequestError("Pick two different games to link."); var suggestions = await db.Suggestions.ToListAsync(); var source = suggestions.FirstOrDefault(s => s.Id == request.SourceSuggestionId); var target = suggestions.FirstOrDefault(s => s.Id == request.TargetSuggestionId); if (source is null || target is null) - return Results.NotFound(new { error = "Suggestion not found." }); + return EndpointHelpers.NotFoundError("Suggestion not found."); var rootIndex = EndpointHelpers.BuildLinkRoots(suggestions.Select(s => (s.Id, s.ParentSuggestionId))); if (!rootIndex.TryGetValue(source.Id, out var sourceRoot) || !rootIndex.TryGetValue(target.Id, out var targetRoot)) - return Results.NotFound(new { error = "Suggestion not found." }); + return EndpointHelpers.NotFoundError("Suggestion not found."); if (sourceRoot == targetRoot) - return Results.BadRequest(new { error = "These games are already linked." }); + return EndpointHelpers.BadRequestError("These games are already linked."); var affectedRootIds = new HashSet { diff --git a/Endpoints/AuthEndpoints.cs b/Endpoints/AuthEndpoints.cs index 63ec427..766f4d3 100644 --- a/Endpoints/AuthEndpoints.cs +++ b/Endpoints/AuthEndpoints.cs @@ -16,11 +16,11 @@ public static class AuthEndpoints group.MapPost("/register", async ([FromBody] RegisterRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => { if (!AuthValidator.TryValidateRegistration(request, out var validated, out var registrationError)) - return Results.BadRequest(new { error = registrationError }); + return EndpointHelpers.BadRequestError(registrationError); var exists = await db.Players.AnyAsync(p => p.NormalizedUsername == validated.NormalizedUsername); if (exists) - return Results.Conflict(new { error = "Username already taken." }); + return EndpointHelpers.ConflictError("Username already taken."); var (hash, salt) = PasswordHasher.HashPassword(request.Password); var expectedAdminKey = config["ADMIN_PASSWORD"]; @@ -28,7 +28,7 @@ public static class AuthEndpoints if (wantsAdmin) { if (string.IsNullOrWhiteSpace(expectedAdminKey) || validated.AdminKey != expectedAdminKey) - return Results.BadRequest(new { error = "Invalid admin key." }); + return EndpointHelpers.BadRequestError("Invalid admin key."); } var isAdmin = wantsAdmin; @@ -63,11 +63,11 @@ public static class AuthEndpoints group.MapPost("/login", async ([FromBody] LoginRequest request, HttpContext ctx, AppDbContext db) => { if (!AuthValidator.TryValidateLogin(request, out _, out var normalizedUsername, out var loginError)) - return Results.BadRequest(new { error = loginError }); + return EndpointHelpers.BadRequestError(loginError); var player = await db.Players.FirstOrDefaultAsync(p => p.NormalizedUsername == normalizedUsername); if (player == null || !PasswordHasher.Verify(request.Password, player.PasswordHash, player.PasswordSalt)) - return Results.Json(new { error = "Invalid username or password." }, statusCode: StatusCodes.Status401Unauthorized); + return EndpointHelpers.UnauthorizedError("Invalid username or password."); if (string.IsNullOrWhiteSpace(player.DisplayName)) { diff --git a/Endpoints/EndpointHelpers.cs b/Endpoints/EndpointHelpers.cs index 97ff085..5a7401d 100644 --- a/Endpoints/EndpointHelpers.cs +++ b/Endpoints/EndpointHelpers.cs @@ -1,5 +1,6 @@ using GameList.Data; using GameList.Domain; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using System.Security.Claims; @@ -84,7 +85,28 @@ internal static class EndpointHelpers } public static IResult PhaseMismatch(Phase required, Phase current) => - Results.BadRequest(new { error = $"This endpoint is available in the {required} phase. Your current phase is {current}." }); + BadRequestError($"This endpoint is available in the {required} phase. Your current phase is {current}."); + + public static IResult BadRequestError(string detail) => Problem(StatusCodes.Status400BadRequest, "Bad Request", detail); + + public static IResult NotFoundError(string detail) => Problem(StatusCodes.Status404NotFound, "Not Found", detail); + + public static IResult ConflictError(string detail) => Problem(StatusCodes.Status409Conflict, "Conflict", detail); + + public static IResult UnauthorizedError(string detail = "Unauthorized") => Problem(StatusCodes.Status401Unauthorized, "Unauthorized", detail); + + private static IResult Problem(int statusCode, string title, string detail) + { + return Results.Problem( + statusCode: statusCode, + title: title, + detail: detail, + extensions: new Dictionary + { + ["error"] = detail + } + ); + } public static string? TrimTo(string? input, int max) => string.IsNullOrWhiteSpace(input) ? null : input.Trim() is { Length: > 0 } t ? t[..Math.Min(t.Length, max)] : null; diff --git a/Endpoints/ResultsWorkflowService.cs b/Endpoints/ResultsWorkflowService.cs index f4cd7e1..5fe14eb 100644 --- a/Endpoints/ResultsWorkflowService.cs +++ b/Endpoints/ResultsWorkflowService.cs @@ -11,7 +11,7 @@ internal sealed class ResultsWorkflowService(AppDbContext db) { var appState = await db.AppState.AsNoTracking().FirstAsync(); if (!appState.ResultsOpen) - return Results.BadRequest(new { error = "Results are locked until the admin enables them." }); + return EndpointHelpers.BadRequestError("Results are locked until the admin enables them."); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); if (phase != Phase.Results) diff --git a/Endpoints/StateEndpoints.cs b/Endpoints/StateEndpoints.cs index 56b452c..dd5efef 100644 --- a/Endpoints/StateEndpoints.cs +++ b/Endpoints/StateEndpoints.cs @@ -66,7 +66,7 @@ public static class StateEndpoints { if (reconciled) await db.SaveChangesAsync(); - return Results.BadRequest(new { error = "Results are locked until the admin enables them." }); + return EndpointHelpers.BadRequestError("Results are locked until the admin enables them."); } player.CurrentPhase = next; @@ -88,7 +88,7 @@ public static class StateEndpoints var isAdmin = await EndpointHelpers.IsAdmin(ctx, db); if (!isAdmin) { - return Results.BadRequest(new { error = "Only admins can move backward." }); + return EndpointHelpers.BadRequestError("Only admins can move backward."); } var appState = await db.AppState.FirstAsync(); diff --git a/Endpoints/SuggestionWorkflowService.cs b/Endpoints/SuggestionWorkflowService.cs index a80a91a..dd5757a 100644 --- a/Endpoints/SuggestionWorkflowService.cs +++ b/Endpoints/SuggestionWorkflowService.cs @@ -40,7 +40,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact { var validationError = await SuggestionValidator.ValidateAsync(request, httpFactory); if (validationError is not null) - return Results.BadRequest(new { error = validationError }); + return EndpointHelpers.BadRequestError(validationError); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); var usingJoker = phase == Phase.Vote && player.HasJoker; @@ -48,11 +48,11 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact return EndpointHelpers.PhaseMismatch(Phase.Suggest, phase); if (string.IsNullOrWhiteSpace(player.DisplayName)) - return Results.BadRequest(new { error = "Set a display name before submitting suggestions." }); + return EndpointHelpers.BadRequestError("Set a display name before submitting suggestions."); var existingCount = await db.Suggestions.CountAsync(s => s.PlayerId == player.Id); if (!usingJoker && existingCount >= 5) - return Results.BadRequest(new { error = "You have reached the 5 suggestion limit." }); + return EndpointHelpers.BadRequestError("You have reached the 5 suggestion limit."); var suggestion = new Suggestion { @@ -96,7 +96,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact ? await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId) : await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId && s.PlayerId == player.Id); if (suggestion == null) - return Results.NotFound(new { error = "Suggestion not found." }); + return EndpointHelpers.NotFoundError("Suggestion not found."); await using var tx = await db.Database.BeginTransactionAsync(); @@ -116,16 +116,16 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact { var validationError = await SuggestionValidator.ValidateAsync(request, httpFactory); if (validationError is not null) - return Results.BadRequest(new { error = validationError }); + return EndpointHelpers.BadRequestError(validationError); var suggestion = await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId); if (suggestion == null) - return Results.NotFound(new { error = "Suggestion not found." }); + return EndpointHelpers.NotFoundError("Suggestion not found."); if (!isAdmin) { if (suggestion.PlayerId != player.Id) - return Results.Unauthorized(); + return EndpointHelpers.UnauthorizedError(); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); if (phase == Phase.Results) diff --git a/Endpoints/VoteWorkflowService.cs b/Endpoints/VoteWorkflowService.cs index 3971243..83a4a72 100644 --- a/Endpoints/VoteWorkflowService.cs +++ b/Endpoints/VoteWorkflowService.cs @@ -29,17 +29,17 @@ internal sealed class VoteWorkflowService(AppDbContext db) public async Task UpsertAsync(Player player, VoteRequest request) { if (request.Score is < 0 or > 10) - return Results.BadRequest(new { error = "Score must be between 0 and 10." }); + return EndpointHelpers.BadRequestError("Score must be between 0 and 10."); if (player.VotesFinal) - return Results.BadRequest(new { error = "Votes are finalized. Unfinalize before changing scores." }); + return EndpointHelpers.BadRequestError("Votes are finalized. Unfinalize before changing scores."); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); if (phase != Phase.Vote) return EndpointHelpers.PhaseMismatch(Phase.Vote, phase); if (string.IsNullOrWhiteSpace(player.DisplayName)) - return Results.BadRequest(new { error = "Set a display name before voting." }); + return EndpointHelpers.BadRequestError("Set a display name before voting."); var linkMap = await db.Suggestions .AsNoTracking() @@ -51,7 +51,7 @@ internal sealed class VoteWorkflowService(AppDbContext db) .ToListAsync(); var rootIndex = EndpointHelpers.BuildLinkRoots(linkMap.Select(s => (s.Id, s.ParentSuggestionId))); if (!rootIndex.ContainsKey(request.SuggestionId)) - return Results.BadRequest(new { error = "Suggestion not found." }); + return EndpointHelpers.BadRequestError("Suggestion not found."); var linkedIds = EndpointHelpers.LinkedIdsFor(request.SuggestionId, rootIndex); if (linkedIds.Count == 0) diff --git a/Infrastructure/PlayerIdentityExtensions.cs b/Infrastructure/PlayerIdentityExtensions.cs index 6e3a07d..e0011a7 100644 --- a/Infrastructure/PlayerIdentityExtensions.cs +++ b/Infrastructure/PlayerIdentityExtensions.cs @@ -3,6 +3,7 @@ using GameList.Domain; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Mvc; namespace GameList.Infrastructure; @@ -44,7 +45,14 @@ public static class PlayerIdentityExtensions context.Response.StatusCode = StatusCodes.Status500InternalServerError; context.Response.ContentType = "application/json"; - await context.Response.WriteAsJsonAsync(new { error = "Unexpected server error" }); + var problem = new ProblemDetails + { + Status = StatusCodes.Status500InternalServerError, + Title = "Internal Server Error", + Detail = "Unexpected server error" + }; + problem.Extensions["error"] = "Unexpected server error"; + await context.Response.WriteAsJsonAsync(problem); }); }); return app; diff --git a/REVIEW.md b/REVIEW.md index 4578d4b..1fd1534 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -10,7 +10,7 @@ Progress update (as of February 7, 2026): - Completed: build/test guardrails added (`.github/workflows/ci.yml`) and root ownership/setup docs added (`README.md:1`). - Completed: backend validators centralized for suggestions and auth (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`). - Completed: request safety hardened for redirects and forwarded headers (`Program.cs:44`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`). -- In progress: API/response contract standardization started with typed response DTOs and client parsing compatibility (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`, `wwwroot/js/api.js:25`). +- In progress: API/response contract standardization now includes typed response DTOs, shared `ProblemDetails` helpers, and client parsing compatibility (`Contracts/Responses.cs:5`, `Endpoints/EndpointHelpers.cs:90`, `wwwroot/js/api.js:25`). Top 5 maintainability risks (priority order): @@ -31,7 +31,7 @@ Top 5 maintainability risks (priority order): - Impact: every UI change risks regressions outside its feature area. 4. Endpoint contract consistency and error shaping are still uneven (High) -- Service-layer extraction is now in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`), and typed response DTO adoption has started (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`), but many endpoints still emit ad-hoc error payloads. +- Service-layer extraction is now in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`), and typed response DTO adoption is underway (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`), but not all auth/filter paths emit uniform error envelopes. - Impact: API evolution and client compatibility changes are still high-friction. 5. Static-analysis and frontend lint guardrails remain incomplete (Medium) @@ -165,9 +165,9 @@ Worst coupling points: - Dependencies (if any): P0 redirect-hardening task. [P1][In Progress] Standardize API response contracts and error envelopes -- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads are being typed, but error payloads are still inconsistent (`{ error }` objects, plain status codes, and mixed shapes). -- Evidence: typed response DTOs added in `Contracts/Responses.cs:5` and used in service workflows (`Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); frontend parser now supports `error`, `detail`, and `title` in `wwwroot/js/api.js:25`. -- Recommendation: migrate endpoint error responses to `ProblemDetails` consistently while keeping backward-compatible `error` fields during transition; continue replacing anonymous success payloads with DTOs. +- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads are being typed and service-level errors now use a shared `ProblemDetails` helper, but some auth/filter paths still return bare status codes. +- Evidence: typed response DTOs added in `Contracts/Responses.cs:5` and used in service workflows (`Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); shared error envelope helper added in `Endpoints/EndpointHelpers.cs:90` and used across workflows (`Endpoints/AdminWorkflowService.cs:98`, `Endpoints/SuggestionWorkflowService.cs:43`, `Endpoints/VoteWorkflowService.cs:32`, `Endpoints/ResultsWorkflowService.cs:14`); global exception handler now emits `ProblemDetails` with `error` compatibility field in `Infrastructure/PlayerIdentityExtensions.cs:48`; frontend parser supports `error`, `detail`, and `title` in `wwwroot/js/api.js:25`. +- Recommendation: finish migrating remaining auth/filter unauthorized paths to consistent problem payloads while keeping backward-compatible `error` fields during transition; continue replacing anonymous success payloads with DTOs. - Acceptance criteria (testable): all API endpoints return typed success DTOs; all non-2xx responses include `ProblemDetails` fields and remain consumable by current frontend. - Effort / Risk: `M / Med`. - Dependencies (if any): none.