fix(api): describe the binary endpoints truthfully and make book downloads resumable - #41
Merged
Merged
Conversation
Deploying codex with
|
| Latest commit: |
db0d6da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b135ce3c.codex-asm.pages.dev |
| Branch Preview URL: | https://fix-openapi-description-defe.codex-asm.pages.dev |
…grammar correctly Three OpenAPI description defects, reported by a strict generated client. None changes the wire format; all three make the document match what the handlers already do. Series listings were documented as pages of books. `PaginatedResponse<T>` named through a `pub type` alias loses its type argument and collapses to the base component, which held whichever instantiation the registry rendered into that slot: `data: [BookDto]`. Four series operations therefore claimed to return books, and a strict client threw on the first item of the first page. All alias-named bodies now name the generic inline. The nine book listings were correct only by accident of that slot's contents, so they are fixed too, which is what stops them becoming a silent regression later. Four library book endpoints documented an `IntoParams` struct they never extracted. utoipa infers a parameter's location from the handler's extractor, and with nothing to infer from it falls back to `Path`, so `page` and `pageSize` rendered as path parameters of a path with no such segments: an operation no client could construct. They now name a struct describing exactly what those routes honour, with an explicit `parameter_in`, which also exposes `sort` and `full` for the first time. The struct that existed only to be named incorrectly is deleted. The filter grammar was erased to an untyped object by a `value_type` override, so the primary query interface reached generated clients as a freeform dictionary and was discoverable only by reading the source. The override was not load-bearing: the recursive schemas render correctly and the web build is clean without it. Adds document invariants covering all three, each a whole-document walk so it holds for operations added later: no operation may reference a generic wrapper with its argument discarded, path parameters and path templates must agree in both directions, and the filter conditions must reference their grammar.
Six operations annotated a single concrete content type and then set a
different Content-Type at runtime. The document was internally consistent,
generated without warning, and described a response the server never writes.
A strict generated client validates the response content type before
decoding and throws on a mismatch, so these were runtime failures on
responses the server considers successful:
get_book_file declared application/octet-stream, sends zip, rar,
epub or pdf chosen from books.format
get_page_image declared image/jpeg, sends whatever the archive holds
opds_book_page_image same, it delegates to get_page_image
get_book_thumbnail declared image/jpeg, sends image/svg+xml for any
get_series_thumbnail thumbnail whose generation task has not finished
download_export declared application/octet-stream, sends text/csv,
text/markdown or application/json
The thumbnail pair is the sharp one: the placeholder fires on timing rather
than content, so every cover in a freshly scanned library took a branch the
document said could not happen.
Enumerate where the set is bounded and the distinction is information the
caller wants (get_book_file, download_export), and use a wildcard where it
is open-ended and the caller cannot act on it (the three image routes),
following the image/* already used by komga_get_page. get_book_file lists
application/octet-stream alongside the four concrete types so the annotation
covers its catch-all arm rather than only the formats the scanner writes.
The server was not changed to match the document. Forcing get_book_file to
octet-stream would discard the format hint, and rendering the placeholder as
JPEG would replace a small vector with a raster to satisfy an annotation.
This invariant cannot be checked from inside the document, since nothing in
it is inconsistent, so it is asserted from the other side: a test per
operation makes the request and compares the response Content-Type against
the set ApiDoc declares for it, covering the placeholder and non-JPEG paths
rather than only the happy one. Each case also pins which branch it
exercises, so one that stops reaching the placeholder fails instead of
passing on nothing.
Also adds an orphan-component invariant. A schema no operation can reach
transitively is the signature the last four defects shared, so the set is
now pinned against an explicit allowlist grouped by why each name is
acceptable. It fails both ways: a new orphan needs a decision, and an
allowlisted name that becomes reachable has to be removed. Two entries are
findings rather than exemptions, recorded rather than fixed: the library
jobs routes carry no utoipa::path at all, and several registered DTOs are
referenced by no handler.
An interrupted download of a 200 MB volume had to start again from zero, and
the only way to read one page without fetching the whole archive was
GET /books/{id}/pages/{n}, which reopens and re-extracts on every request.
So the endpoint that costs least to serve was the one clients could not use
efficiently, and the one that costs most was the only one offering random
access. Range support inverts that.
GET /books/{book_id}/file now advertises Accept-Ranges: bytes and a strong
ETag on every response, and answers 206 to bytes=a-b, bytes=a- and bytes=-n,
416 to a range that names no byte, and 304 to a current If-None-Match. A
request without a Range is byte-for-byte what it was.
The suffix form is the one that is easy to skip and should not be: bytes=-n
is how a client reads a ZIP central directory, and a CBZ is a ZIP. Handling
only bytes=a-b would satisfy resume and none of the partial-read case.
The ETag is books.file_hash, a non-null column the scanner already computes,
so the validator costs no I/O, survives a rescan, and survives the file being
moved on disk. An mtime validator would do none of those and would invalidate
every client's cache on any rescan that touches timestamps.
If-Range is honoured against it: a stale validator yields the full 200 rather
than a 206, because splicing fresh bytes into a partially downloaded old file
produces something that is neither version. A multi-range request would need
a multipart/byteranges body no client here asks for, and RFC 9110 permits
ignoring a range the server declines, so it gets the whole file. A malformed
Range is ignored rather than rejected, for the same reason.
Authorisation, the permission check and the content filter all still run
before the file is opened, so a 206 is never a way around a check a 200 has
to pass.
Streaming uses 64 KiB chunks rather than ReaderStream's 4 KiB default. That
default was found by the measurement this work called for: it turns a 700 KiB
page into ~170 chunks and a 40 MiB volume into ~10,000, and it was the whole
of a 2-3x gap against the page endpoint on identical bytes. On a 41 MB
60-page CBZ over a keep-alive connection, one page by range went from 21.1 ms
to 4.7 ms and reading all 60 went from 1941 ms to 295 ms, against 10.2 ms and
552 ms for the page endpoint. The whole-file download runs at ~290 MB/s.
Folds in RFC 6266 filename encoding, which this route lacked. The encoder is
now shared with the Komga download rather than duplicated, and it emits an
ASCII-transliterated fallback for the quoted parameter: a header value may
only hold visible ASCII, so the previous code put raw UTF-8 on the wire and
produced exactly the latin-1 mangling that filename* exists to prevent. Both
routes were affected.
Verified against a running server as well as in tests. A 42 MB download
interrupted at 16,830,464 bytes resumed with 206 and transferred 25,175,438
more, exactly the remainder, and the reassembled file matched the source
SHA-256. Reading one page out of 60 by way of the central directory
transferred 765,566 bytes, 1.82% of the archive.
Range, If-Range and If-None-Match are declared as header parameters and 206,
304 and 416 as responses, so a generated client can construct a resumable
download without dropping to a raw request.
… at all Seven routes and eight operations were absent from the OpenAPI document entirely: the per-library job CRUD, run-now, dry-run, and the field-group catalog its editor is built from. None of the handlers in handlers/library_jobs.rs carried a #[utoipa::path], and the module was never listed in docs.rs paths(). Half the work had been done already, which is what made it hard to notice: all fourteen DTOs were registered in docs.rs schemas(), so they shipped in the document as components that no operation could reach. To anyone reading the component list the API looked described; to anyone generating a client it did not exist. The orphan-component invariant added on this branch is what surfaced it, and it is also what verified the fix. Annotating the paths made those fourteen components reachable, so the check's stale-entry half failed and named every one of them, and the allowlist entry that recorded the gap could be deleted rather than edited. That is the shape this check was meant to have: a finding it records is a finding it later insists you close. The utoipa parameter-location trap does not apply here. Every handler extracts its parameters as Path<Uuid> or Path<(Uuid, Uuid)> and none take a Query, so the derive has an extractor to infer from and no explicit parameter_in is needed. Beyond the mechanical transcription, the annotations record three things that are true of the handlers but not evident from their signatures: patch_job's timezone is tri-state, where absent leaves it, null clears it to the server default, and a value sets it; run_job_now answers 409 rather than queueing twice when a run for the job is already in flight; and dry_run_job's configOverride plans against a config the job does not have yet, which is what lets an editor preview an edit before saving it. 353 paths to 358.
…ilds again
The docs site has failed to build since the filter grammar stopped being an
untyped object. Making SeriesListRequest.condition and BookListRequest.condition
`$ref`s exposed the grammar to the docs generator for the first time, and
docusaurus-theme-openapi-docs 4.7.1 cannot render it:
Can't render static file for pathname "/docs/api/schemas/serieslistrequest"
TypeError: schema[key]?.map is not a function at AnyOneOf
Two things trip it, and the document is valid OpenAPI on both counts. The
plugin dereferences `$ref`s eagerly and replaces the recursion in
SeriesCondition with the literal string "circular()", so `schema.oneOf` is a
string rather than an array. And the grammar uses `allOf` and `anyOf` as
property names, since that is how the Rust enum variants serialize, so a
`properties` map reads as a schema carrying an `anyOf` keyword whose value is
an object. `AnyOneOf` picks its key with `schema.oneOf ? "oneOf" : "anyOf"` and
calls `.map` on the result without checking it is an array.
Nothing caught this because the docs build runs on Cloudflare Pages against
pull requests, and this branch had no PR until now.
Renaming the wire fields was not an option: it addresses the keyword collision
but not the recursion, and it would break the filter grammar for both the web
app and the iOS client.
Upgrading fixes it. 5.x still contains the same unguarded line but no longer
feeds it a string, so both schema pages render. The upgrade cascades: the
plugin's 5.x peers require @docusaurus/* ^3.10, and 3.10 requires an explicit
@docusaurus/faster dependency for the `future.v4` flag this site already sets.
docusaurus-plugin-sass is a new peer of the theme.
Verified by running the Cloudflare build command, `npm run build`, against a
tree with no generated API docs, matching a fresh checkout. Both previously
failing pages now render.
AshDevFr
force-pushed
the
fix/openapi-description-defects
branch
from
August 21, 2026 18:46
d1ac3d7 to
c8b7618
Compare
utoipa renders a concrete instantiation of a generic by expanding its type argument inline, even when that argument is registered as its own component. Every generic wrapper in the document did it: all ten PaginatedResponse_* components and all four KomgaPage_* ones. The document stayed correct, so nothing flagged it, but any generator that has to name an inline schema invents a fresh type for it. swift-openapi-generator produced PaginatedResponseSeriesDto.DataPayloadPayload where getSeries produced SeriesDto: structurally identical, nominally distinct, and not interchangeable. A client could not pass a row from a list into anything typed on the detail DTO without a hand-written conversion, once per paginated endpoint. GenericArgumentReferencer walks each Base_Argument component and replaces any subschema byte-identical to Argument's own component with a $ref to it. Requiring an exact match is what makes the rewrite safe: it can only collapse a copy of a schema the document already contains under that name, so nothing about the described wire format changes. It joins the existing Modify pipeline alongside NullableRefFlattener, so it needs no handler or DTO changes and covers every generic rather than only pagination. Verified against the generator that found the problem: a probe taking a series from a paginated list and one from getSeries as the same type now compiles. SeriesExternalIndexDto was an orphan only because its wrapper inlined it, so it becomes reachable and leaves the accepted-orphan list. The invariant added here fails if any generic wrapper starts inlining again.
`?full=true` switches the response to a different schema. OpenAPI cannot
express a body whose shape depends on a parameter *value*, so the alternate
shape was invisible to every generated client, and FullBookResponse and
FullSeriesResponse sat in the document as components no operation referenced.
The two shapes are not lean versus rich, which rules out the tidy fixes. BookDto
carries chapter, summary and volume that FullBookResponse lacks; SeriesDto
carries title, titleSort, publisher, summary and year that FullSeriesResponse
lacks, because the full shape moves them inside `metadata`. They are different
projections that partially overlap, so they cannot be merged into one schema
with optional fields, and a oneOf union would force callers to switch between
two arms that put the same field at different paths.
Adds three routes, each with one concrete response schema:
GET /api/v1/books/{book_id}/full -> FullBookResponse
GET /api/v1/series/{series_id}/full -> FullSeriesResponse
GET /api/v1/series/full -> PaginatedResponse<FullSeriesResponse>
The listing is there because the parameter has a live external consumer, not on
principle: shisho pages GET /api/v1/series?full=true across a whole library for
its candidate pools, and calls the series detail form too. Both now have a
describable route to move to. Only these three were added — the other nineteen
operations accept `full` and nobody sends it, and splitting all twenty-two
would oblige every future list endpoint to be added twice.
`/series/full` shares a path slot with `/series/{series_id}`, which is the same
shape as the thirteen static siblings already there, `/series/external-index`
among them. A test pins that the literal route wins.
The listing shares one implementation with the plain one rather than
duplicating it, and deliberately does not echo `full=true` into its pagination
links: the route already means full.
`full` is now marked deprecated on all twenty-two operations. Nothing breaks;
removal is planned for 3.0, once shisho has moved.
The web app is migrated off it. `booksApi.getFull` and `seriesApi.getFull`
replace the conditional-return-type `getDetail<T>` / `getById<T>` pair, which
existed only to model in TypeScript what the document could not express.
Each new route is tested against the `?full=true` form it replaces and asserted
byte-identical, so removing the parameter later cannot silently change a
payload. The book comparison ignores the metadata row's own timestamps, since a
book with no metadata gets one created on read and two requests produce two
different values.
All four `full=true` orphans become reachable and leave the accepted-orphan
list.
The frontend suite failed a different test on every full run while every one of them passed in isolation: three consecutive runs failed BulkMetadataEditModal, then AddLibraryModal twice, then InstallNudgeModal and MediaCard. That signature is a budget problem, not broken tests. Vitest's 5s default is sized for fast unit tests. 74 of the 221 files drive the UI through userEvent, which awaits a React render per keystroke, and the runner gives each of the machine's cores its own jsdom environment. Under that contention the heavier interaction tests genuinely exceed 5s — the worst offender types thirteen characters, walks a file-browser flow with three async lookups, switches tabs and sets four selects. Raises testTimeout and hookTimeout to 20s. Individual findBy* calls keep their own shorter timeouts, so a missing element still fails fast; what this costs is that a truly hung test now takes 20s to report rather than 5s. Three consecutive full runs are green at 3627/3627, against three consecutive red ones before.
…full plumbing
BookDetail and SeriesDetail were migrated onto GET /books/{id}/full and
GET /series/{id}/full, but the MSW handlers still only knew the deprecated
`?full=true` form, so `make frontend-mock` would have 404'd both pages. Nothing
caught it: MSW is wired through setupWorker, which only runs in a browser, so
vitest never loads the handlers — and neither page has a test.
Adds handlers for all three new routes and a coverage test that asserts they
exist. The test also pins that the literal /series/full is declared before the
/series/:id pattern, since MSW resolves in array order and would otherwise read
"full" as a series id — the same ordering constraint the real router has.
Writing that assertion caught a flaw in the assertion rather than the code: an
early `indexOf` matched the PATCH handler for /series/:id and reported an order
violation that cannot exist, because MSW matches on method and path together.
It now compares within GET handlers only.
Also removes the `full` plumbing from six series API wrappers — getByLibrary,
getInProgress, getRecentlyAdded, getRecentlyUpdated, getBooks and search. Each
accepted `full?: T` and returned a conditional type, the TypeScript workaround
for a response shape the document could not express, and no caller passed it.
They collapse to their plain return types, so series.ts no longer references a
parameter that goes away in 3.0.
Caps the vitest worker pool at 50% of cores in the same change. Raising the
per-test timeout earlier was not enough: the suite kept failing one test per run
— InstallNudgeModal, TemplateSelector, MediaCard, AddLibraryModal — never the
same one twice, and each passed in isolation. Vitest defaults to one worker per
logical core, and each runs a full jsdom + React + Mantine pipeline, so twelve of
them starve each other. Failures landing on whichever test lost the race is the
signature of contention rather than a broken test.
The cap halves aggregate test time, 369-403s against 660-681s, because the tests
get real CPU instead of fighting for it; wall clock costs about 13s. Seven
consecutive green runs, against a rate that had been failing roughly four in ten.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two things, both driven by
codex-reader-ioshitting them on a generated Swift client.Describe the media types the binary endpoints actually send. Six operations declared one concrete content type and set a different
Content-Typeat runtime. The document was internally consistent and generated without warning, so nothing flagged it, but a strict generated client validates the response content type before decoding and throws on a mismatch — a hard failure on a response the server considers successful.get_book_fileapplication/octet-streambooks.formatget_page_imageimage/jpegimage/*opds_book_page_imageimage/jpegget_page_imageimage/*get_book_thumbnailimage/jpegimage/svg+xmluntil generation finishesimage/*get_series_thumbnailimage/jpegimage/*download_exportapplication/octet-streamtext/csv/text/markdown/application/jsonThe thumbnail pair is the sharp one: the placeholder fires on timing, not content, so every cover in a freshly scanned library took a branch the document said could not happen. The last two rows are outside the original report — same defect, found while reading, closed rather than left half-open.
Make
GET /books/{book_id}/filerange-capable and conditional. An interrupted download of a 200 MB volume had to restart from zero, and the only way to read one page without the whole archive wasGET /books/{id}/pages/{n}, which reopens and re-extracts every time. It now advertisesAccept-Ranges: bytesand a strongETag, and answers 206 tobytes=a-b,bytes=a-andbytes=-n, 416 to an unsatisfiable range, and 304 to a currentIf-None-Match. A request with noRangeis byte-for-byte what it was.Notes for review
The ETag is
books.file_hash. A non-null column the scanner already computes, so the validator costs no I/O, survives a rescan, and survives the file moving on disk. It happens to be the file's SHA-256, so it is strong in the RFC sense and a client can verify a resumed download against it.If-Rangeis honoured. A stale validator yields the full 200 rather than a 206, because splicing fresh bytes into a partially downloaded old file produces something that is neither version.Suffix ranges are the point, not a nicety.
bytes=-nis how a client reads a ZIP central directory, and a CBZ is a ZIP. Handling onlybytes=a-bwould satisfy resume and none of the partial-read case.Access control runs before the file is opened, so a 206 is never a way around a check a 200 has to pass.
A latent perf defect fell out of the measurement.
ReaderStream's 4 KiB default turns a 700 KiB page into ~170 chunks and a 40 MiB volume into ~10,000. The first measurement showed a ranged read slower than the extract endpoint on identical bytes. At 64 KiB, on a 41 MB 60-page CBZ over keep-alive:GET /pages/{n}This also sped up the plain whole-file download, which is pre-existing behaviour.
A filename-encoding bug, fixed in both routes. Folding RFC 6266 encoding into the v1 route surfaced that the Komga copy — cited as the working example to follow — put the raw filename into the quoted
filenameparameter. A header value may only carry visible ASCII, so a non-ASCII name went onto the wire as raw UTF-8 and was read as latin-1, which is exactly the manglingfilename*prevents. The encoder is now shared, and transliterates the quoted parameter.Verification
cargo clippy --all-targets -- -D warningsclean.tsc -bclean.pre-commit rungreen, including theopenapi-synchook.swift-openapi-generator1.13 with the iOS client's own config — clean, zero warnings — and compile-time probes confirm each fix, including that a filtered series list decodes as series and thatimage/*accepts the SVG placeholder.Third commit: document the library-jobs routes
Seven paths and eight operations were absent from the document entirely — per-library job CRUD, run-now, dry-run, and the field-group catalog its editor is built from. None of the handlers in
handlers/library_jobs.rscarried a#[utoipa::path], and the module was never listed indocs.rspaths().Half the work had been done, which is what made it hard to notice: all fourteen DTOs were registered in
schemas(), so they shipped as components no operation could reach. Reading the component list, the API looked described. Generating a client, it did not exist.The annotations also record three behaviours the signatures do not show:
patch_job'stimezoneis tri-state (absent leaves it,nullclears it to the server default, a value sets it),run_job_nowanswers 409 rather than double-queueing when a run is already in flight, anddry_run_job'sconfigOverrideplans against a config the job does not have yet, which is what lets an editor preview an edit before saving.353 paths → 358.
Also added
An orphan-component invariant. A schema no operation can reach transitively is the signature the last four defects shared, so the set is now pinned against an explicit allowlist grouped by why each name is acceptable. It fails both ways — a new orphan needs a decision, and an allowlisted name that becomes reachable has to be removed — and both directions were verified by perturbation.
It found the library-jobs gap, and then verified the fix. Annotating those paths made the fourteen components reachable, so the check's stale-entry half failed and named every one of them, and the allowlist entry recording the gap was deleted rather than edited. A finding it records is a finding it later insists you close — that property is worth more than the initial detection.
Remaining entries include several registered DTOs referenced by no handler (
TokenResponse,SharingTagListResponse,ReleaseLedgerListResponse), each with the reason recorded inline.Deliberately not in scope
full=true's undocumented alternate shape. Needs an API decision about whether one operation may return two shapes, not an annotation fix.$ref-ing it, found during the generator check. EveryPaginatedResponse_*inlines, so a series from a list is nominally a different generated type from a series fromgetSeries, despite being identical. Not blocking; filed separately.