From 79dc8f899f5cf51d71d655feb80092303e94f4b4 Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Sat, 7 Feb 2026 01:19:51 +0100 Subject: [PATCH] Introduce typed API responses and align workflow outputs --- Contracts/Responses.cs | 56 ++++++++++++++++++++++++ Endpoints/AdminWorkflowService.cs | 60 +++++--------------------- Endpoints/ResultsWorkflowService.cs | 18 ++++---- Endpoints/SuggestionWorkflowService.cs | 7 ++- Endpoints/VoteWorkflowService.cs | 8 +--- REVIEW.md | 25 +++++++---- wwwroot/js/api.js | 2 +- 7 files changed, 99 insertions(+), 77 deletions(-) create mode 100644 Contracts/Responses.cs diff --git a/Contracts/Responses.cs b/Contracts/Responses.cs new file mode 100644 index 0000000..436b4f8 --- /dev/null +++ b/Contracts/Responses.cs @@ -0,0 +1,56 @@ +using GameList.Domain; + +namespace GameList.Contracts; + +public record SuggestionCreatedResponse(int Id); + +public record SuggestionUpdatedResponse( + int Id, + string Name, + string? Genre, + string? Description, + string? ScreenshotUrl, + string? YoutubeUrl, + string? GameUrl, + int? MinPlayers, + int? MaxPlayers +); + +public record VoteUpsertResponse(IReadOnlyList SuggestionIds, int Score); + +public record VoteFinalizeResponse(bool VotesFinal); + +public record AdminResultsStateResponse(bool ResultsOpen, DateTimeOffset UpdatedAt); + +public record AdminGrantJokerResponse(Guid Id, bool HasJoker); + +public record AdminDeletePlayerResponse(Guid DeletedPlayerId); + +public record AdminLinkSuggestionsResponse(int RootId, IReadOnlyList LinkedSuggestionIds, int UnfinalizedPlayers); + +public record AdminUnlinkSuggestionsResponse(IReadOnlyList UnlinkedSuggestionIds, int UnfinalizedPlayers); + +public record AdminResetStateResponse(Phase Phase, bool ResultsOpen, DateTimeOffset UpdatedAt); + +public record VoteStatusResponse(IReadOnlyList Voters, bool Ready, IReadOnlyList Waiting); + +public record ResultItemDto( + int Id, + string Name, + string? Author, + int? MinPlayers, + int? MaxPlayers, + int Total, + int Count, + double Average, + IReadOnlyList Votes, + int? MyVote, + string? ScreenshotUrl, + string? YoutubeUrl, + string? GameUrl, + string? Description, + string? Genre, + int? ParentSuggestionId, + IReadOnlyList LinkedIds, + IReadOnlyList LinkedTitles +); diff --git a/Endpoints/AdminWorkflowService.cs b/Endpoints/AdminWorkflowService.cs index 3e448d9..13c471a 100644 --- a/Endpoints/AdminWorkflowService.cs +++ b/Endpoints/AdminWorkflowService.cs @@ -27,11 +27,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) await db.SaveChangesAsync(); await tx.CommitAsync(); var currentState = await db.AppState.AsNoTracking().FirstAsync(); - return Results.Ok(new - { - currentState.ResultsOpen, - currentState.UpdatedAt - }); + return Results.Ok(new AdminResultsStateResponse(currentState.ResultsOpen, currentState.UpdatedAt)); } public async Task GetVoteStatusAsync() @@ -45,12 +41,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) var waiting = voters.Where(v => !v.Finalized).Select(v => v.Name).ToList(); var ready = waiting.Count == 0; - return Results.Ok(new - { - voters, - ready, - waiting - }); + return Results.Ok(new VoteStatusResponse(voters, ready, waiting)); } public async Task GrantJokerAsync(GrantJokerRequest request) @@ -67,11 +58,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) player.VotesFinal = false; await db.SaveChangesAsync(); - return Results.Ok(new - { - player.Id, - player.HasJoker - }); + return Results.Ok(new AdminGrantJokerResponse(player.Id, player.HasJoker)); } public async Task DeletePlayerAsync(Guid playerId) @@ -98,7 +85,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) await db.SaveChangesAsync(); await tx.CommitAsync(); - return Results.Ok(new { DeletedPlayerId = playerId }); + return Results.Ok(new AdminDeletePlayerResponse(playerId)); } public async Task LinkSuggestionsAsync(Player adminPlayer, LinkSuggestionsRequest request) @@ -153,12 +140,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) await tx.CommitAsync(); - return Results.Ok(new - { - RootId = targetRoot, - LinkedSuggestionIds = affectedIds, - UnfinalizedPlayers = await db.Players.CountAsync() - }); + return Results.Ok(new AdminLinkSuggestionsResponse(targetRoot, affectedIds, await db.Players.CountAsync())); } public async Task UnlinkSuggestionsAsync(Player adminPlayer, UnlinkSuggestionsRequest request) @@ -170,19 +152,11 @@ internal sealed class AdminWorkflowService(AppDbContext db) var suggestions = await db.Suggestions.ToListAsync(); var target = suggestions.FirstOrDefault(s => s.Id == request.SuggestionId); if (target is null) - return Results.Ok(new - { - UnlinkedSuggestionIds = Array.Empty(), - UnfinalizedPlayers = 0 - }); + return Results.Ok(new AdminUnlinkSuggestionsResponse(Array.Empty(), 0)); var rootIndex = EndpointHelpers.BuildLinkRoots(suggestions.Select(s => (s.Id, s.ParentSuggestionId))); if (!rootIndex.TryGetValue(target.Id, out var rootId)) - return Results.Ok(new - { - UnlinkedSuggestionIds = Array.Empty(), - UnfinalizedPlayers = 0 - }); + return Results.Ok(new AdminUnlinkSuggestionsResponse(Array.Empty(), 0)); var groupIds = rootIndex.Where(kv => kv.Value == rootId).Select(kv => kv.Key).ToList(); @@ -201,11 +175,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) await tx.CommitAsync(); - return Results.Ok(new - { - UnlinkedSuggestionIds = groupIds, - UnfinalizedPlayers = await db.Players.CountAsync() - }); + return Results.Ok(new AdminUnlinkSuggestionsResponse(groupIds, await db.Players.CountAsync())); } public async Task ResetAsync() @@ -222,12 +192,7 @@ internal sealed class AdminWorkflowService(AppDbContext db) await db.SaveChangesAsync(); await tx.CommitAsync(); - return Results.Ok(new - { - Phase = Phase.Suggest, - state.ResultsOpen, - state.UpdatedAt - }); + return Results.Ok(new AdminResetStateResponse(Phase.Suggest, state.ResultsOpen, state.UpdatedAt)); } public async Task FactoryResetAsync() @@ -245,11 +210,6 @@ internal sealed class AdminWorkflowService(AppDbContext db) await tx.CommitAsync(); - return Results.Ok(new - { - Phase = Phase.Suggest, - fresh.ResultsOpen, - fresh.UpdatedAt - }); + return Results.Ok(new AdminResetStateResponse(Phase.Suggest, fresh.ResultsOpen, fresh.UpdatedAt)); } } diff --git a/Endpoints/ResultsWorkflowService.cs b/Endpoints/ResultsWorkflowService.cs index 3aff578..f4cd7e1 100644 --- a/Endpoints/ResultsWorkflowService.cs +++ b/Endpoints/ResultsWorkflowService.cs @@ -1,3 +1,4 @@ +using GameList.Contracts; using GameList.Data; using GameList.Domain; using Microsoft.EntityFrameworkCore; @@ -54,8 +55,12 @@ internal sealed class ResultsWorkflowService(AppDbContext db) .Where(id => id != r.Id) .ToList(); - return new - { + var linkedTitles = linkedIds + .Where(nameLookup.ContainsKey) + .Select(id => nameLookup[id]) + .ToList(); + + return new ResultItemDto( r.Id, r.Name, r.Author, @@ -72,12 +77,9 @@ internal sealed class ResultsWorkflowService(AppDbContext db) r.Description, r.Genre, r.ParentSuggestionId, - LinkedIds = linkedIds, - LinkedTitles = linkedIds - .Where(nameLookup.ContainsKey) - .Select(id => nameLookup[id]) - .ToList() - }; + linkedIds, + linkedTitles + ); }); return Results.Ok(shaped); diff --git a/Endpoints/SuggestionWorkflowService.cs b/Endpoints/SuggestionWorkflowService.cs index 6587a62..a80a91a 100644 --- a/Endpoints/SuggestionWorkflowService.cs +++ b/Endpoints/SuggestionWorkflowService.cs @@ -80,7 +80,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact await db.SaveChangesAsync(); await tx.CommitAsync(); - return Results.Created($"/api/suggestions/{suggestion.Id}", new { suggestion.Id }); + return Results.Created($"/api/suggestions/{suggestion.Id}", new SuggestionCreatedResponse(suggestion.Id)); } public async Task DeleteAsync(Player player, bool isAdmin, int suggestionId) @@ -150,8 +150,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact await db.SaveChangesAsync(); - return Results.Ok(new - { + return Results.Ok(new SuggestionUpdatedResponse( suggestion.Id, suggestion.Name, suggestion.Genre, @@ -161,7 +160,7 @@ internal sealed class SuggestionWorkflowService(AppDbContext db, IHttpClientFact suggestion.GameUrl, suggestion.MinPlayers, suggestion.MaxPlayers - }); + )); } public async Task GetAllAsync(Player player) diff --git a/Endpoints/VoteWorkflowService.cs b/Endpoints/VoteWorkflowService.cs index e0b4fb9..3971243 100644 --- a/Endpoints/VoteWorkflowService.cs +++ b/Endpoints/VoteWorkflowService.cs @@ -80,11 +80,7 @@ internal sealed class VoteWorkflowService(AppDbContext db) } await db.SaveChangesAsync(); - return Results.Ok(new - { - SuggestionIds = linkedIds, - request.Score - }); + return Results.Ok(new VoteUpsertResponse(linkedIds, request.Score)); } public async Task SetFinalizeAsync(Player player, VoteFinalizeRequest request) @@ -95,6 +91,6 @@ internal sealed class VoteWorkflowService(AppDbContext db) player.VotesFinal = request.Final; await db.SaveChangesAsync(); - return Results.Ok(new { player.VotesFinal }); + return Results.Ok(new VoteFinalizeResponse(player.VotesFinal)); } } diff --git a/REVIEW.md b/REVIEW.md index b8b14cc..4578d4b 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -10,6 +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`). Top 5 maintainability risks (priority order): @@ -19,7 +20,7 @@ Top 5 maintainability risks (priority order): - Impact: hard-to-debug regressions and fragile refactors in UI workflows. 2. Rule duplication still present between backend/frontend validations (High) -- Suggestion validation is centralized on the backend (`Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:109`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`). +- Suggestion validation is centralized on the backend (`Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`). - Auth validation is centralized on the backend (`Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`) while frontend length checks remain duplicated (`wwwroot/app.js:92`, `wwwroot/app.js:121`). - Impact: inconsistent behavior and repeated fixes across server/client. @@ -29,8 +30,8 @@ Top 5 maintainability risks (priority order): - Hidden module coupling through globals: `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, plus `window` callbacks consumed in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:696`, `wwwroot/js/ui.js:1009`. - Impact: every UI change risks regressions outside its feature area. -4. Endpoint contract consistency and response 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:7`), but response shapes are still mostly anonymous objects and ad-hoc error payloads. +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. - Impact: API evolution and client compatibility changes are still high-friction. 5. Static-analysis and frontend lint guardrails remain incomplete (Medium) @@ -77,7 +78,7 @@ Worst coupling points: [P0][Done] Make phase reads side-effect free and move reconciliation to explicit writes - Problem: Severity `Critical`, Category `Architecture`. Read endpoints/filters previously relied on mutating phase reads. Impact: unsafe refactors and non-deterministic behavior. -- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsWorkflowService.cs:9`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`. +- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsWorkflowService.cs:10`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`. - Recommendation: Split into `GetCurrentPhaseAsync` (pure read) and explicit `ReconcilePhaseAsync` (write command). Run reconciliation only on intentional transition points (admin toggle, phase change commands, migration job), not on GET paths. - Acceptance criteria (testable): GET `/api/state` and GET `/api/me` never call `SaveChangesAsync`; integration tests verify no phase mutations occur during read-only requests; filters perform one phase check path without side effects. - Effort / Risk: `M / Med`. @@ -101,7 +102,7 @@ Worst coupling points: [P0][Partial] Centralize validation rules to stop backend/frontend drift - Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules are duplicated in multiple backend endpoints and frontend forms. Impact: inconsistent behavior and repeated fixes. -- Evidence: backend centralized in `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:109`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `wwwroot/app.js:92`. +- Evidence: backend centralized in `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `wwwroot/app.js:92`. - Recommendation: Extract backend validators (e.g., `SuggestionValidator`, `AuthValidator`) and reuse in create/update paths; simplify frontend to UX-only prechecks and rely on server responses for source-of-truth. - Acceptance criteria (testable): create/update share one backend validator path; tests cover validator once and both endpoints; frontend no longer re-implements server-only security rules. - Effort / Risk: `M / Med`. @@ -117,7 +118,7 @@ Worst coupling points: [P1][Done] Extract service-layer workflows from endpoint lambdas - Problem: Severity `High`, Category `Architecture`. Endpoint files contain business orchestration, persistence, and policy logic inline; large lambdas are hard to reason about and reuse. -- Evidence: extraction completed for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:7`, `Program.cs:37`, `Program.cs:38`, `Program.cs:39`, `Program.cs:40`). +- Evidence: extraction completed for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`, `Program.cs:37`, `Program.cs:38`, `Program.cs:39`, `Program.cs:40`). - Recommendation: Introduce focused application services (`SuggestionService`, `VoteService`, `AdminWorkflowService`) and keep endpoints as transport adapters. - Acceptance criteria (testable): endpoint handlers reduced to routing + DTO mapping + service calls; domain rule tests target service methods directly; endpoint tests remain green. - Effort / Risk: `L / Med`. @@ -149,7 +150,7 @@ Worst coupling points: [P1] Make write workflows transaction-consistent and explicit - Problem: Severity `Medium`, Category `Correctness/Architecture`. Several multi-step state changes rely on multiple DB commands without explicit transaction grouping. -- Evidence: `Endpoints/SuggestionWorkflowService.cs:71`, `Endpoints/SuggestionWorkflowService.cs:75`, `Endpoints/AdminWorkflowService.cs:74`, `Endpoints/AdminWorkflowService.cs:208`, `Endpoints/AdminWorkflowService.cs:227`. +- Evidence: `Endpoints/SuggestionWorkflowService.cs:72`, `Endpoints/SuggestionWorkflowService.cs:77`, `Endpoints/AdminWorkflowService.cs:17`, `Endpoints/AdminWorkflowService.cs:181`, `Endpoints/AdminWorkflowService.cs:198`. - Recommendation: Wrap multi-entity updates in explicit transactions where consistency matters, or refactor into idempotent command handlers with compensating behavior. - Acceptance criteria (testable): fault-injection tests prove no partial state after exceptions; transaction boundaries documented per workflow. - Effort / Risk: `M / Med`. @@ -157,12 +158,20 @@ Worst coupling points: [P1] Strengthen test quality for flaky/time-sensitive cases and security edges - Problem: Severity `Medium`, Category `Testing`. Some tests depend on sleeps and do not cover realistic redirect behavior or overlapping refresh flows. -- Evidence: `GameList.Tests/SuggestionTests.cs:379`, `GameList.Tests/SuggestionTests.cs:575`, `GameList.Tests/HelperTests.cs:121`. +- Evidence: redirect and forwarded-header cases are covered (`GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`), and delay-based ordering checks were removed from `GameList.Tests/SuggestionTests.cs` and `GameList.Tests/AdminTests.cs`. - Recommendation: replace `Task.Delay` ordering checks with deterministic seeded timestamps where feasible; add explicit redirect-follow tests and concurrency-path tests. - Acceptance criteria (testable): no timing sleeps in endpoint tests for ordering; new tests cover redirect-chain rejection and race-sensitive refresh logic. - Effort / Risk: `M / Low`. - 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. +- 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. + [P2] Externalize i18n/FAQ content from executable JS modules - Problem: Severity `Low`, Category `Complexity/Documentation`. Translation and FAQ payloads are embedded in code, making review and localization hard. - Evidence: `wwwroot/js/i18n.js:1`-`wwwroot/js/i18n.js:799`. diff --git a/wwwroot/js/api.js b/wwwroot/js/api.js index 3b2a97a..2df4315 100644 --- a/wwwroot/js/api.js +++ b/wwwroot/js/api.js @@ -22,7 +22,7 @@ async function request(path, { method = "GET", body } = {}) { let msg = `${res.status}`; try { const data = await res.json(); - msg = data.error || JSON.stringify(data); + msg = data.error || data.detail || data.title || JSON.stringify(data); } catch { /* ignore */ } const err = new Error(msg); err.status = res.status;