feat: hypermedia links and actions across all REST endpoints (Phase 4) #8
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/hypermedia-responses"
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?
Phase 4: Hypermedia Links and Actions
Replaces all
EnvelopeDefaults.EmptyLinks/EmptyActionswith actual hypermedia data across every REST endpoint, per ADR-0007.What changed
New:
HypermediaHelpers.cs— Reusable static builders for every resource type:BuildCollectionLinks(self/next/prev pagination) andBuildCollectionActions(create)Updated: All 4 endpoint files — Every
ResourceResponseandCollectionResponsenow returns proper links and actions:Example response
Tests (23 new, 173 total)
HypermediaHelpers— pagination logic, all resource link/action sets, multipart encoding typesZero remaining
EnvelopeDefaults.EmptyLinks/EmptyActionsin endpoint code.Replace EnvelopeDefaults.EmptyLinks/EmptyActions with actual hypermedia data in every endpoint response per the ADR-0007 hypermedia map. New file: RestAdapter/Envelopes/HypermediaHelpers.cs - Static class with reusable Build{Resource}Links / Build{Resource}Actions builders for doujins, variants, chapters, pages, titles, tags, people, circles, plus generic collection link/action builders. - All methods have XML doc comments. Endpoints updated: - DoujinEndpoints: collection + resource + title creation now emit full link/action sets (self, variants, titles; update/delete/addTitle/ addVariant/assignTag/assignPerson/assignCircle). - VariantEndpoints: nested collection + create, flat GET/PUT, and chapter create/update/list now emit hypermedia. Variant links include doujin, pages, chapters; actions include uploadPages/uploadZip (multipart) and reorderPages. - MetadataEndpoints: tags, people, circles collections and create/update endpoints now emit hypermedia. - ImageEndpoints: page collection now emits collection links + upload/ reorder actions. - DoujinEndpoints.BuildDoujinLinks/Actions now delegate to HypermediaHelpers (kept for backward compatibility). Tests: - New HypermediaHelpersTests.cs: 14 unit tests covering every builder. - New HypermediaIntegrationTests.cs: 8 integration tests verifying links/actions appear in actual HTTP responses for doujin/variant/chapter/ tag/page resources and collections. - DoujinCrudIntegrationTests.cs: added assertions that links/actions are non-empty on doujin create/get, variant create, and the empty collection response. Build: 0 errors, 0 warnings. Tests: 173 passed (150 existing + 23 new).Summary
Summary
Coverage
DoujinManager.ApplicationCore - 81.9%
DoujinManager.Infrastructure - 91.3%
pshot
DoujinManager.RestAdapter - 81%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 18%
🤖 Hermes automated review: changes requested
Reviewed the full diff (8 files, +841/−42) for head
64d3f4fcagainst base67e37fda. The overall structure ofHypermediaHelpers.csis clean and the unit tests are thorough on shape, but cross-referencing every emitted href against the actualMapGroup(...)route registrations in the four endpoint files reveals broken hypermedia links — the core correctness property this PR exists to provide. These are served in live HTTP responses, so they should be fixed before merge.🔴 Major — dead/wrong hrefs served in responses
1.
HypermediaHelpers.cs:66—BuildDoujinLinksadds atitleslink that 404sBuildDoujinLinksemits["titles"] = new("/api/doujins/{id}/titles", "GET"), and this builder is served by theGET /api/doujins/{id}handler (DoujinEndpoints.cs:42). But there is noGET /api/doujins/{id}/titlesroute — onlyPOST /{id:guid}/titlesandDELETE /{id:guid}/titles/{titleId}under the/api/doujinsgroup. This PR introduces the dead link (the old privateBuildDoujinLinksonly hadself/variants). Clients following it will get a 404.Suggested fix: either drop the
titlesentry (titles are already embedded in the doujin detail DTO) or add the missingGETroute.2.
HypermediaHelpers.cs:146— chapterselflink points to a non-existent routeBuildChapterLinksemits["self"] = new("/api/chapters/{id}", "GET"), but there is no/api/chaptersroute group at all. Chapters are only reachable via the nested group/api/variants/{variantId}/chapters(list). This builder is actively served on chapter create (201,VariantEndpoints.cs:108) and update (200,VariantEndpoints.cs:120), so every chapter mutation response advertises aselflink that 404s.3.
HypermediaHelpers.cs:156-157— chapterupdate/deleteactions use the wrong pathBuildChapterActionsemitsPUT /api/chapters/{id}andDELETE /api/chapters/{id}, but the real routes arePUT /api/variants/{variantId}/chapters/{chapterId}andDELETE /api/variants/{variantId}/chapters/{chapterId}(VariantEndpoints.cs:118,131). The hrefs are missing the entirevariants/{variantId}prefix, so a hypermedia client that follows the advertisedupdate/deleteaction to mutate a chapter will hit a 404 (or, worse, a future unrelated flat route). Served on the same create/update responses as #2.Suggested fix for #2 and #3: thread
variantIdintoBuildChapterActions(it already takes it for links) and emit the nested/api/variants/{variantId}/chapters/{chapterId}hrefs; forself, either use the nested list route or add a real single-chapter GET.🟡 Minor (non-blocking)
HypermediaHelpers.cs:186—BuildPageLinksself→GET /api/pages/{id}has no GET route (the/api/pagesgroup only registersDELETE /{id}). Not currently wired into any endpoint (only the*PageCollection*builders are used inImageEndpoints.cs), so it's latent — but the unit testBuildPageLinks_IncludesSelfImageAndThumbnailasserts the dead href, which will mislead future callers. Worth aligning if a single-page GET is added.HypermediaHelpers.cs:230—BuildTitleLinks(Guid titleId, ...)ignorestitleId(XML doc acknowledges this). Fine as-is, just flagging the unused param.Why the tests didn't catch this
The 14
HypermediaHelpersTestsassert the href strings match the builder output (e.g.Assert.Equal("/api/chapters/{SampleId}", links["self"].Href)) — they verify the builder returns what it returns, not that the href resolves to a registered route. The 8 integration tests assert links/actions are non-empty, not that specific hrefs are reachable. A route-existence check (or asserting hrefs against the same constants used inMapGroup) would prevent this class of bug.✅ Verification
64d3f4fc(generated 06/28 17:18, 4 assemblies, line 82.6% / branch 45.2%,HypermediaHelpers86% line / 100% branch). Build+test ran successfully for this head; local build/test skipped per CI-evidence policy.Note: this is a PR conversation comment with file:line references — not a formal Forgejo review approval (the MCP integration cannot create approval states or inline diff comments).
Automated daily review. I never merge PRs.
Fix 3 major + 1 minor finding from automated review: 1. Doujin 'titles' link removed — no GET /api/doujins/{id}/titles route exists. Titles are already embedded in the doujin detail DTO. 2. Chapter links fixed — chapters have no standalone GET route, so 'self' replaced with 'collection' pointing to the nested chapter collection at /api/variants/{variantId}/chapters. 3. Chapter actions fixed — update/delete now correctly use the nested path /api/variants/{variantId}/chapters/{chapterId} instead of the non-existent /api/chapters/{id}. BuildChapterActions now requires variantId parameter. 4. Page links fixed — no GET /api/pages/{id} route exists, so 'self' replaced with 'collection' pointing to /api/variants/{variantId}/pages. BuildPageLinks now requires variantId parameter. Updated all unit tests and integration tests to match corrected hrefs.Fixed all broken hypermedia hrefs in commit
37ef835. 173 tests pass.Major fixes
#1 — Dead
titleslink removedRemoved
["titles"]fromBuildDoujinLinks. There is noGET /api/doujins/{id}/titlesroute — titles are embedded in the doujin detail DTO.#2 — Chapter
selflink →collectionlinkChapters have no standalone GET route. Changed
["self"] = /api/chapters/{id}to["collection"] = /api/variants/{variantId}/chapters— the actual route that exists.#3 — Chapter
update/deleteactions use nested pathsBuildChapterActionsnow requiresvariantIdand emits:PUT /api/variants/{variantId}/chapters/{chapterId}DELETE /api/variants/{variantId}/chapters/{chapterId}Minor fix
#4 — Page
selflink →collectionlinkNo
GET /api/pages/{id}route exists. Changed to["collection"] = /api/variants/{variantId}/pages.BuildPageLinksnow requiresvariantId.Tests updated
All unit tests and integration tests updated to assert the corrected hrefs. Added
Assert.DoesNotContain("titles", ...)to explicitly guard against reintroducing the dead link.🤖 Hermes automated review: no blocking issues found (fixes verified)
Follow-up review of head
37ef8354(thefix: correct all broken hypermedia hrefscommit). I statically verified each of the three major + one minor finding from my previous review against the actualMapGroup(...)/MapVerb(...)route registrations.✅ All previously-reported issues resolved
37ef835titleslink inBuildDoujinLinks["titles"]removed (comment explains why)GET /api/doujins/{id}/titlesroute exists (only POST/DELETE) — removal is correctself→ nonexistent/api/chapters/{id}["collection"] = /api/variants/{variantId}/chaptersGET /registered on that group (VariantEndpoints.cs:87,90)update/deletemissingvariants/{variantId}prefixBuildChapterActionsnow takesvariantId; emits nested pathsPUT/DELETE /{chapterId}registered under/api/variants/{variantId:guid}/chapters(VariantEndpoints.cs:118,131)BuildPageLinksself→ nonexistent/api/pages/{id}["collection"] = /api/variants/{variantId}/pagesGET /registered on that group (ImageEndpoints.cs:24,27)All call sites were updated for the new signatures (
VariantEndpoints.cs:113-114,126-127;HypermediaHelpersTests.cs:147). No remaining callers of the old signatures. Tests updated to assert the corrected hrefs and addedAssert.DoesNotContain("titles", ...)as a regression guard — good.No new blocking issues found in the fix delta.
✅ Verification
37ef8354(generated 06/28 17:49, 4 assemblies, line 82.7% / branch 45.2%,HypermediaHelpers86.1% line / 100% branch). Build+test ran successfully for this head; local build/test skipped per CI-evidence policy.Note: this is a PR conversation comment — not a formal Forgejo review approval (the MCP integration cannot create approval states or inline diff comments).
Automated daily review. I never merge PRs.