Standardize service errors with ProblemDetails envelope

This commit is contained in:
2026-02-07 01:23:54 +01:00
parent 79dc8f899f
commit f615ef3a4a
9 changed files with 63 additions and 33 deletions

View File

@@ -48,11 +48,11 @@ internal sealed class AdminWorkflowService(AppDbContext db)
{ {
var player = await db.Players.FirstOrDefaultAsync(p => p.Id == request.PlayerId); var player = await db.Players.FirstOrDefaultAsync(p => p.Id == request.PlayerId);
if (player is null) 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); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
if (phase != Phase.Vote) 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.HasJoker = true;
player.VotesFinal = false; 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); var player = await db.Players.Include(p => p.Suggestions).FirstOrDefaultAsync(p => p.Id == playerId);
if (player is null) 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(); await using var tx = await db.Database.BeginTransactionAsync();
@@ -95,20 +95,20 @@ internal sealed class AdminWorkflowService(AppDbContext db)
return EndpointHelpers.PhaseMismatch(Phase.Vote, phase); return EndpointHelpers.PhaseMismatch(Phase.Vote, phase);
if (request.SourceSuggestionId == request.TargetSuggestionId) 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 suggestions = await db.Suggestions.ToListAsync();
var source = suggestions.FirstOrDefault(s => s.Id == request.SourceSuggestionId); var source = suggestions.FirstOrDefault(s => s.Id == request.SourceSuggestionId);
var target = suggestions.FirstOrDefault(s => s.Id == request.TargetSuggestionId); var target = suggestions.FirstOrDefault(s => s.Id == request.TargetSuggestionId);
if (source is null || target is null) 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))); 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)) 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) 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<int> var affectedRootIds = new HashSet<int>
{ {

View File

@@ -16,11 +16,11 @@ public static class AuthEndpoints
group.MapPost("/register", async ([FromBody] RegisterRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => group.MapPost("/register", async ([FromBody] RegisterRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) =>
{ {
if (!AuthValidator.TryValidateRegistration(request, out var validated, out var registrationError)) 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); var exists = await db.Players.AnyAsync(p => p.NormalizedUsername == validated.NormalizedUsername);
if (exists) if (exists)
return Results.Conflict(new { error = "Username already taken." }); return EndpointHelpers.ConflictError("Username already taken.");
var (hash, salt) = PasswordHasher.HashPassword(request.Password); var (hash, salt) = PasswordHasher.HashPassword(request.Password);
var expectedAdminKey = config["ADMIN_PASSWORD"]; var expectedAdminKey = config["ADMIN_PASSWORD"];
@@ -28,7 +28,7 @@ public static class AuthEndpoints
if (wantsAdmin) if (wantsAdmin)
{ {
if (string.IsNullOrWhiteSpace(expectedAdminKey) || validated.AdminKey != expectedAdminKey) if (string.IsNullOrWhiteSpace(expectedAdminKey) || validated.AdminKey != expectedAdminKey)
return Results.BadRequest(new { error = "Invalid admin key." }); return EndpointHelpers.BadRequestError("Invalid admin key.");
} }
var isAdmin = wantsAdmin; var isAdmin = wantsAdmin;
@@ -63,11 +63,11 @@ public static class AuthEndpoints
group.MapPost("/login", async ([FromBody] LoginRequest request, HttpContext ctx, AppDbContext db) => group.MapPost("/login", async ([FromBody] LoginRequest request, HttpContext ctx, AppDbContext db) =>
{ {
if (!AuthValidator.TryValidateLogin(request, out _, out var normalizedUsername, out var loginError)) 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); var player = await db.Players.FirstOrDefaultAsync(p => p.NormalizedUsername == normalizedUsername);
if (player == null || !PasswordHasher.Verify(request.Password, player.PasswordHash, player.PasswordSalt)) 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)) if (string.IsNullOrWhiteSpace(player.DisplayName))
{ {

View File

@@ -1,5 +1,6 @@
using GameList.Data; using GameList.Data;
using GameList.Domain; using GameList.Domain;
using Microsoft.AspNetCore.Mvc;
using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore;
using System.Security.Claims; using System.Security.Claims;
@@ -84,7 +85,28 @@ internal static class EndpointHelpers
} }
public static IResult PhaseMismatch(Phase required, Phase current) => 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<string, object?>
{
["error"] = detail
}
);
}
public static string? TrimTo(string? input, int max) => 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; string.IsNullOrWhiteSpace(input) ? null : input.Trim() is { Length: > 0 } t ? t[..Math.Min(t.Length, max)] : null;

View File

@@ -11,7 +11,7 @@ internal sealed class ResultsWorkflowService(AppDbContext db)
{ {
var appState = await db.AppState.AsNoTracking().FirstAsync(); var appState = await db.AppState.AsNoTracking().FirstAsync();
if (!appState.ResultsOpen) 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); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
if (phase != Phase.Results) if (phase != Phase.Results)

View File

@@ -66,7 +66,7 @@ public static class StateEndpoints
{ {
if (reconciled) if (reconciled)
await db.SaveChangesAsync(); 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; player.CurrentPhase = next;
@@ -88,7 +88,7 @@ public static class StateEndpoints
var isAdmin = await EndpointHelpers.IsAdmin(ctx, db); var isAdmin = await EndpointHelpers.IsAdmin(ctx, db);
if (!isAdmin) 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(); var appState = await db.AppState.FirstAsync();

View File

@@ -40,7 +40,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact
{ {
var validationError = await SuggestionValidator.ValidateAsync(request, httpFactory); var validationError = await SuggestionValidator.ValidateAsync(request, httpFactory);
if (validationError is not null) if (validationError is not null)
return Results.BadRequest(new { error = validationError }); return EndpointHelpers.BadRequestError(validationError);
var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
var usingJoker = phase == Phase.Vote && player.HasJoker; var usingJoker = phase == Phase.Vote && player.HasJoker;
@@ -48,11 +48,11 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact
return EndpointHelpers.PhaseMismatch(Phase.Suggest, phase); return EndpointHelpers.PhaseMismatch(Phase.Suggest, phase);
if (string.IsNullOrWhiteSpace(player.DisplayName)) 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); var existingCount = await db.Suggestions.CountAsync(s => s.PlayerId == player.Id);
if (!usingJoker && existingCount >= 5) 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 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)
: await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId && s.PlayerId == player.Id); : await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId && s.PlayerId == player.Id);
if (suggestion == null) 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(); 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); var validationError = await SuggestionValidator.ValidateAsync(request, httpFactory);
if (validationError is not null) 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); var suggestion = await db.Suggestions.FirstOrDefaultAsync(s => s.Id == suggestionId);
if (suggestion == null) if (suggestion == null)
return Results.NotFound(new { error = "Suggestion not found." }); return EndpointHelpers.NotFoundError("Suggestion not found.");
if (!isAdmin) if (!isAdmin)
{ {
if (suggestion.PlayerId != player.Id) if (suggestion.PlayerId != player.Id)
return Results.Unauthorized(); return EndpointHelpers.UnauthorizedError();
var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
if (phase == Phase.Results) if (phase == Phase.Results)

View File

@@ -29,17 +29,17 @@ internal sealed class VoteWorkflowService(AppDbContext db)
public async Task<IResult> UpsertAsync(Player player, VoteRequest request) public async Task<IResult> UpsertAsync(Player player, VoteRequest request)
{ {
if (request.Score is < 0 or > 10) 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) 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); var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
if (phase != Phase.Vote) if (phase != Phase.Vote)
return EndpointHelpers.PhaseMismatch(Phase.Vote, phase); return EndpointHelpers.PhaseMismatch(Phase.Vote, phase);
if (string.IsNullOrWhiteSpace(player.DisplayName)) 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 var linkMap = await db.Suggestions
.AsNoTracking() .AsNoTracking()
@@ -51,7 +51,7 @@ internal sealed class VoteWorkflowService(AppDbContext db)
.ToListAsync(); .ToListAsync();
var rootIndex = EndpointHelpers.BuildLinkRoots(linkMap.Select(s => (s.Id, s.ParentSuggestionId))); var rootIndex = EndpointHelpers.BuildLinkRoots(linkMap.Select(s => (s.Id, s.ParentSuggestionId)));
if (!rootIndex.ContainsKey(request.SuggestionId)) 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); var linkedIds = EndpointHelpers.LinkedIdsFor(request.SuggestionId, rootIndex);
if (linkedIds.Count == 0) if (linkedIds.Count == 0)

View File

@@ -3,6 +3,7 @@ using GameList.Domain;
using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Diagnostics;
using Microsoft.AspNetCore.Mvc;
namespace GameList.Infrastructure; namespace GameList.Infrastructure;
@@ -44,7 +45,14 @@ public static class PlayerIdentityExtensions
context.Response.StatusCode = StatusCodes.Status500InternalServerError; context.Response.StatusCode = StatusCodes.Status500InternalServerError;
context.Response.ContentType = "application/json"; 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; return app;

View File

@@ -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: 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: 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`). - 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): 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. - Impact: every UI change risks regressions outside its feature area.
4. Endpoint contract consistency and error shaping are still uneven (High) 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. - Impact: API evolution and client compatibility changes are still high-friction.
5. Static-analysis and frontend lint guardrails remain incomplete (Medium) 5. Static-analysis and frontend lint guardrails remain incomplete (Medium)
@@ -165,9 +165,9 @@ Worst coupling points:
- Dependencies (if any): P0 redirect-hardening task. - Dependencies (if any): P0 redirect-hardening task.
[P1][In Progress] Standardize API response contracts and error envelopes [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). - 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`); frontend parser now supports `error`, `detail`, and `title` in `wwwroot/js/api.js:25`. - 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: migrate endpoint error responses to `ProblemDetails` consistently while keeping backward-compatible `error` fields during transition; continue replacing anonymous success payloads with DTOs. - 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. - 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`. - Effort / Risk: `M / Med`.
- Dependencies (if any): none. - Dependencies (if any): none.