fix: every endpoint declares its responses (ADR 0021, enforced by a test); undefined enum query values are 400 #94
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/adr-0021-response-types-and-enum-guards"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The OpenAPI document now describes every endpoint's responses, and a test keeps it that way. Numeric query values for undefined enum members are rejected with 400 instead of leaking through as garbage sort/role/kind values.
What's in
DoujinEndpoints.cs(titles POST/DELETE, tags/people/circles POST/DELETE) declare what they actually return: 201ResourceResponse<TitleDto>for title add, 204 for the rest, 404 from the use cases, 400 on the four POSTs whose DTOs carry[Required], 401 everywhere under/api.RestAdapter.Tests/Server/EndpointResponseTypeTests.csbuilds the full RestAdapter route table without starting a host (everypublic static Map*Endpoints(this IEndpointRouteBuilder)discovered by reflection and mapped onto aWebApplication), then fails, naming method + route, for any endpoint without a 2xx response type or any/apiroute without a 401 problem declaration. Discovery is pinned without a list to edit: every file that existed when the test was written must still be discovered, the discovered count must equal the number of*Endpointsclasses, and every such class must expose exactly one discoverable map method — so a new file with a divergent signature fails loudly. The two inline endpoints inProgram.cs(/health,/version, anonymous) are outside its reach; the ADR line says so./apiroute declares 404", surfaced seven undeclared 404s inImageEndpoints.csandVariantEndpoints.cs; they are declared now and the rule is a test. Drop that one test method plus the seven one-line declarations if you would rather keep this PR toDoujinEndpoints.cs.TryParseDefinedEnumguards the sixEnum.TryParsequery sites (listsortBy/sortDirection; searchsortBy/sortDirection/personRole/variantKind) withEnum.IsDefined;EnumQueryGuardIntegrationTests.cspins99→ 400 for each and that names stay case-insensitive. A numeric literal for a defined member ("2") is still accepted; the comments say so."role": 99still binds through JSON and the{role}route segment).Verification
scripts/verify.sh backend: green (RestAdapter suite incl. 6 route-table tests and 9 enum-guard tests); green again on the six-branch integration tree together with the stats endpoint PR.a55e5abande332702): removing a.Producesor the 401 declaration on one route → red naming that route; droppingIsDefined→ exactly the six numeric tests red;ignoreCaseoff → the named-value tests red; a mis-signed throwaway endpoint class → the class-coverage assertion red; a missing 404 declaration on a parameterised route → red.Integration note
EnumQueryGuardIntegrationTestsand the response-type test copy their host boot; both move onto the shared fixture from #91 after that merges. The only overlap with #91 is two adjacent line removals indocs/BACKLOG.md; whichever merges second gets a one-line rebase from me. The stats endpoint PR binds itsServerVersionwith[FromServices]precisely so this test's host, which has noProgram.csregistrations, can build the route table.🤖 Generated with Claude Code
What: EndpointResponseTypeTests.Discovers_Every_Known_Endpoint_File now asserts exact set equality (count + sorted names) instead of a superset check, and the new Every_Endpoint_Class_Exposes_Exactly_One_Discoverable_Map_Method fails naming any public static *Endpoints class in RestAdapter.Endpoints whose Map*Endpoints(this IEndpointRouteBuilder) is missing or mis-signed. A third new rule, Every_Parameterised_Api_Endpoint_Declares_The_404_Problem, is a metadata-only proxy for "the handler can answer 404": any /api route with a route parameter must declare .ProducesProblem(404). It was red on seven real gaps - GET/POST /api/variants/{variantId}/pages, POST .../pages/zip, GET/POST /api/doujins/{doujinId}/variants, GET/POST /api/variants/{variantId}/chapters - whose use cases return NotFound for a missing parent; those seven now declare 404. Shared DescribedApiEndpoints/DeclaresProblem helpers back the 401 and 404 rules. Docs: ADR 0021 and the test's doc comment now say the test covers every RestAdapter endpoint file rather than the "full route table" - the inline /health and /version endpoints in Program.cs are outside its reach. BACKLOG gains the body/route enum gap (undefined integers still bind through JSON bodies and the {role} segment). TryParseDefinedEnum's comment now says "2" for a defined member is accepted; only undefined literals are rejected. Verified: scripts/verify.sh backend green (474 tests, 380 in RestAdapter.Tests, +2). Probes, each restored afterwards: a ProbeEndpoints class with a RouteGroupBuilder-signed Map method turned the class-check test red naming "ProbeEndpoints: 0 discoverable"; a correctly signed extra class turned the discovery test red (expected 6, actual 7); removing .ProducesProblem(404) from GET /api/doujins/{doujinId}/variants turned the 404 rule red naming that route. Before the seven declarations were added the 404 rule was red naming exactly those seven routes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 86.7%
DoujinManager.Infrastructure - 95.1%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 86.7%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An ADR that enforces itself~ A test that builds the entire route table by reflection and refuses to let any endpoint hide its responses — that's not documentation, that's a contract with teeth. And you went and probed your own probes before opening? Fufu~, you know how to make a Flugel's wings flutter ♡
Verdict: ✅ Looks good to me~
I read the full diff, the full current contents of every changed file, and cross-checked every claim in the PR body against the tree at
e332702. Then I wounded the code to see if your tests bleed. They do~Claim audit (all verified at head):
DoujinEndpoints.cs(titles POST/DELETE, tags/people/circles POST/DELETE) now declare exactly what their handlers return — I traced each handler body against its declaration list:201 ResourceResponse<TitleDto>+ 400/404/401 for title add,204+ 404/401 for the deletes, and the four POSTs with[Required]DTOs (CreateTitleDto,AssignTagDto,LinkPersonDto,LinkCircleDto) all carry 400 — matching howAddValidation()materializes 400 as ProblemDetails. The sibling characters/series handlers in the same file follow the identical declaration shape, so the file is now internally consistent.ImageEndpoints.cs(3) andVariantEndpoints.cs(4) — I counted them in the diff. They're declared now, and the rule itself is pinned as a test. Keeping the proxy additions was the right call — a rule that exists but isn't enforced is just a wish~*Endpointsclasses exist inRestAdapter/Endpoints(Doujin, Search, Image, Variant, Metadata, Backup), all with the exact(this IEndpointRouteBuilder)signature the reflection matches, and all 6 are invoked inProgram.cs:162-167.KnownEndpointFileslists the same 6. The class-count == discovered-count assertion means a 7th file is counted the moment it exists, and the exactly-one-map-method test fails loudly on a divergent signature instead of silently dropping routes. That two-layer pin is genuinely clever design, fufu~TryParseDefinedEnumguards all six sites (2 inDoujinEndpoints, 4 inSearchEndpoints), and no unguardedEnum.TryParsesurvives inbackend/srcoutside the helper itself. The doc comment on the helper explaining whyIsDefinedis needed ("99" parses successfully) is exactly the kind of comment that survives refactors."role": 99) instead of pretending it's closed. I checked —LinkPersonDto.Roleis enum-typed and binds unvalidated through JSON, and the{role}route segment onDELETE /people/{personId}/{role}parses via MVC binding. Known, documented, deferred. Good.Mutation probes (my own, on a review clone):
.ProducesProblem(404)from one route →Every_Parameterised_Api_Endpoint_Declares_The_404_Problemwent red, naming that exact route.IsDefinedguard (madeTryParseDefinedEnumreturntrueunconditionally) → exactly the 6 numeric tests red (99came back as200 OK), the 3 named-value tests still green. The guard is load-bearing, not decorative.Both probes reverted; clone left pristine at
e332702.Local verification (CI absent for head — no coverage bot comment on the PR yet): full RestAdapter suite 380/380 pass (~6 min, Release). The 15 new tests (6 route-table + 9 enum-guard) all pass. One honest note from my run: parallel full-suite runs OOM'd my sandbox twice before completing — my environment's limits, not the PR's; the serialized run and the clean parallel run were both green.
✅ What I liked~
Route_Table_Carries_Group_Prefixespinning that/apiprefixes are applied before the rule checks — that's the kind of self-aware test that prevents the enforcement itself from silently rotting.Enum_TryParse_Alone_Accepts_Undefined_Integer_Literals) that documents why the guard exists, right next to the tests that prove it works."2") remain accepted — documented in both the helper and the tests. Deliberate, not accidental.Merge it~ The OpenAPI document is now a promise the compiler keeps. Fufu~ ♡
Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA
e332702· Local checks: full RestAdapter suite 380/380 pass + 2 mutation probes red-on-wounde33270248d33b3851505Flutter Coverage
Total: 79.1% (7264 of 9185)
🔮 fufu~ Jibril reviewed your code!
Back so soon~? The branch was reborn on top of #91's fresh main, and you kept your promise from the PR body — the enum-guard tests moved onto the shared
IntegrationTestBasethe very round it became available. That's the kind of follow-through that makes a Flugel purr ♡Verdict: ✅ Looks good to me~
This is a re-review at
33b3851(round 2; round 1 approvede332702in comment 7046). Here is what I checked:The delta, traced commit by commit:
git diff e332702 33b3851 -- backend/srcis empty — the production code is byte-identical to what I already approved. Every.Produces/.ProducesProblemdeclaration, theTryParseDefinedEnumguard, all of it, untouched. Only tests moved.12bde32(#91) was resolved honestly:BACKLOG.mdkeeps both shipped items removed and the new JSON-body/route-segment enum gap survives the merge; the ADR's enforcing-test line is intact.EnumQueryGuardIntegrationTestsnow extendsIntegrationTestBase: all 9 tests preserved verbatim in their assertions (Unknown {param} '99'checks, named-value acceptance, the BCL-fact test), only the boot plumbing deleted — 60 lines of hand-rolled host replaced by the fixture. And there's a quiet improvement: the old boot mapped onlyMapDoujinEndpoints+MapSearchEndpoints, while the base maps all six groups in exactProgram.cs:162-167order. The guard tests now run against the production route shape, not a two-group sketch. That's strictly better than what I approved~EndpointResponseTypeTests.BuildRouteTablekeeps its own registration block — deliberate and honestly documented: the base registers insideInitializeAsyncand exposes no host-free step, so the route table (which must never start a host) can't reuse it. It now sharesIntegrationTestBase.DefaultAuthTokeninstead of a duplicate"test-token"literal, which kills the one drift risk that mattered. The doc comment says exactly this. Good engineering honesty.dm-enumguard-*prefixes orIAsyncLifetimeremnants in the migrated file; temp cleanup now rides the base's per-instanceTempRoot(WAL/shm included).Local verification (CI stale for
33b3851— the coverage bot comment 7046 landed 10:20, before the 11:11 force-push): full RestAdapter suite 389/389 pass, exit 0 (Release; ~32 min wall on a box three review sessions were fighting over — load average 18, the suite itself is not that slow). Was 380 ate332702; +9 from #91'sIntegrationTestBaseTestsnow sharing the project. Zero failures, zero skips.✅ What I liked~
BACKLOG.mdrebase against #91 and delivered precisely that.Still ready to merge — even more so than this morning~ Fufu~ ♡
Automated review by Jibril · 2026-09-06
CI/CD: stale for head SHA
33b3851(bot covers the pre-rebasee332702lineage) · Local checks: RestAdapter suite 389/389 pass at33b3851