From ead43b25620f0d2aa83c9808e6c4c430e98ab692 Mon Sep 17 00:00:00 2001 From: patrick Date: Mon, 14 Sep 2026 11:04:47 -0400 Subject: [PATCH] fix: download every selected book, not just the last The frontend built book_ids with `set` rather than `append`, so the archive only ever held one book. The proxy also sent an empty Content-Length, which is not valid HTTP/1.1, and buffered the whole zip before forwarding a byte of it. Entries now carry the file's own name, suffixed when two books claim the same one, and the books are resolved before the stream starts so a miss is a 404 instead of a truncated archive under a 200. --- backend/src/chitai/controllers/book.py | 8 +- backend/src/chitai/exceptions/handlers.py | 22 ++- backend/src/chitai/services/book.py | 146 +++++++++++++----- .../unit/test_services/test_book_service.py | 80 +++++++++- .../src/lib/state/bookOperations.svelte.ts | 5 +- frontend/src/routes/api/[...path]/+server.ts | 27 +++- 6 files changed, 230 insertions(+), 58 deletions(-) diff --git a/backend/src/chitai/controllers/book.py b/backend/src/chitai/controllers/book.py index a2ebc71..fdb5b0d 100644 --- a/backend/src/chitai/controllers/book.py +++ b/backend/src/chitai/controllers/book.py @@ -509,9 +509,15 @@ class BookController(Controller): Returns: A stream of the compressed ZIP file. + + Raises: + FileNotFoundError: If the selection turned up no file that is on disk, + which the handler renders as a 404. Awaiting here is what makes that + possible: the books are resolved before the response begins, so a + miss is still a status code rather than a truncated archive. """ return Stream( - books_service.get_files(book_ids, library_id), + await books_service.get_files(book_ids, library_id), headers={ "Content-Disposition": 'attachment; filename="download.zip"', "Content-Type": "application/zip", diff --git a/backend/src/chitai/exceptions/handlers.py b/backend/src/chitai/exceptions/handlers.py index ca78b20..ccdcadd 100644 --- a/backend/src/chitai/exceptions/handlers.py +++ b/backend/src/chitai/exceptions/handlers.py @@ -12,4 +12,24 @@ def not_found_exception_handler(_: Request, exc: NotFoundError) -> Response: ) -exception_handlers = {NotFoundError: not_found_exception_handler} +def file_not_found_exception_handler(_: Request, exc: FileNotFoundError) -> Response: + """ + Handler for a row that describes a file the caller cannot be given. + + The row exists, so `NotFoundError` is the wrong answer and the services raise + `FileNotFoundError` instead: the book is real, the file it names is not on disk, + or the file belongs to another book. All three are the caller asking for + something that is not there, which is a 404 — before this they escaped as + `ValueError` and reached the client as a 500. + """ + return Response( + media_type=MediaType.TEXT, + content=f"Not found error: {exc}", + status_code=404, + ) + + +exception_handlers = { + NotFoundError: not_found_exception_handler, + FileNotFoundError: file_not_found_exception_handler, +} diff --git a/backend/src/chitai/services/book.py b/backend/src/chitai/services/book.py index 42b4481..befc0a6 100644 --- a/backend/src/chitai/services/book.py +++ b/backend/src/chitai/services/book.py @@ -243,6 +243,67 @@ def _unused_path(path: Path) -> Path: return candidate +def _unused_name(name: str, taken: set[str]) -> str: + """ + The given name, or the first free `name (n).ext` beside it. + + The in-memory counterpart of `_unused_path`, for names within an archive rather + than a directory. Two books can hold a file called `Dune.epub` — nothing stops + them, since each lives in its own directory — and a zip happily stores the name + twice, which loses one of them on extraction. + + Args: + name: The filename to place. + taken: Names already placed; the chosen one is added to it. + + Returns: + A name that nothing in `taken` occupies. + """ + path = Path(name) + candidate = name + suffix = 1 + + while candidate in taken: + suffix += 1 + candidate = f"{path.stem} ({suffix}){path.suffix}" + + taken.add(candidate) + return candidate + + +async def _stream_zip(entries: list[tuple[str, Path]]) -> AsyncIterator[bytes]: + """ + Compress already-resolved files into a zip, a chunk at a time. + + Args: + entries: `(name within the archive, path on disk)` pairs, in archive order. + + Yields: + Chunks of compressed zip file data. + """ + stream = _ZipStream() + chunk_size = 32768 # 32 KiB + + with zipfile.ZipFile(stream, "w", zipfile.ZIP_DEFLATED) as zip_file: + for arcname, path in entries: + info = zipfile.ZipInfo.from_file(path, arcname=arcname) + info.compress_type = zipfile.ZIP_DEFLATED + + with zip_file.open(info, "w") as entry: + async with aiofiles.open(path, "rb") as source: + while content := await source.read(chunk_size): + entry.write(content) + if chunk := stream.drain(): + yield chunk + + if chunk := stream.drain(): + yield chunk + + # The central directory, written when ZipFile closes. + if chunk := stream.drain(): + yield chunk + + def _title_keys(title: str, authors: set[str]) -> set[str]: """ Every normalized title one book could reasonably be filed under. @@ -1815,11 +1876,11 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): A File object ready for download. Raises: - ValueError: If the file is missing or not found for the given book. + FileNotFoundError: If the file is missing or not found for the given book. """ book = await self.get(book_id) if book.path is None: - raise ValueError("Cannot download file: book has no path") + raise FileNotFoundError("Cannot download file: book has no path") for file in book.files: if file.id != file_id: @@ -1827,16 +1888,23 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): path = Path(book.path) / Path(file.path) if not await aios.path.isfile(path): - raise ValueError("The file is missing") + raise FileNotFoundError("The file is missing") # Rows written before `guess_content_type` existed can hold null here, and # so can `guess_content_type` itself. Litestar fills a null in from the # filename, falling back to `application/octet-stream` on the response. + # + # `filename` is what puts a name in the Content-Disposition. Without it + # Litestar sends `attachment; filename=""` and a client that has nothing + # else to go on — an OPDS reader, curl -O — names the file after the + # numeric URL, extension and all. return File( - path, media_type=file.content_type or guess_content_type(file.path) + path, + filename=Path(file.path).name, + media_type=file.content_type or guess_content_type(file.path), ) - raise ValueError("No such file for the given book") + raise FileNotFoundError("No such file for the given book") async def get_files( self, book_ids: list[int], library_id: int @@ -1844,55 +1912,51 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): """ Get all selected book files as a compressed zip file. - Streams the zip file in chunks to avoid loading the entire archive into memory. - Each book gets its own directory in the archive, so two books that happen to - share a filename do not overwrite one another. Files that have a row but no - longer exist on disk are skipped. + Every book and path is resolved here, up front, and only the compression is + left to the returned iterator. That ordering is the point: a generator that + queried on its first iteration would run *after* the response had already + started, since Litestar only drains a `Stream` once the headers are away — so + a missing book or a database error arrived as a truncated archive under a + `200` rather than as an error anyone could act on. + + Entries carry the file's own name. A name claimed twice — two books can each + hold a `Dune.epub`, living as they do in separate directories — gets the + second suffixed to `Dune (2).epub`, so nothing is overwritten on extraction. + Files that have a row but no longer exist on disk are skipped. Args: book_ids: List of book IDs to include in the zip. library_id: The ID of the library containing the books. - Yields: - Chunks of compressed zip file data. + Returns: + An async iterator over chunks of compressed zip file data. + + Raises: + FileNotFoundError: If the selection turned up no file that is on disk. """ books = await self.list(Book.id.in_(book_ids), Book.library_id == library_id) - stream = _ZipStream() - chunk_size = 32768 # 32 KiB + entries: list[tuple[str, Path]] = [] + taken: set[str] = set() - with zipfile.ZipFile(stream, "w", zipfile.ZIP_DEFLATED) as zip_file: - for book in books: - # Books without files have no path, and `file.path` is stored relative - # to it, so there is nothing to resolve against. - if book.path is None: + for book in books: + # Books without files have no path, and `file.path` is stored relative + # to it, so there is nothing to resolve against. + if book.path is None: + continue + + book_path = Path(book.path) + for file in book.files: + path = book_path / file.path + if not await aios.path.isfile(path): continue - book_path = Path(book.path) - for file in book.files: - path = book_path / file.path - if not await aios.path.isfile(path): - continue + entries.append((_unused_name(Path(file.path).name, taken), path)) - # Namespaced by the book's own directory to avoid collisions. - info = zipfile.ZipInfo.from_file( - path, arcname=f"{book_path.name}/{Path(file.path).name}" - ) - info.compress_type = zipfile.ZIP_DEFLATED + if not entries: + raise FileNotFoundError("No files to download for the given books") - with zip_file.open(info, "w") as entry: - async with aiofiles.open(path, "rb") as source: - while content := await source.read(chunk_size): - entry.write(content) - if chunk := stream.drain(): - yield chunk - - if chunk := stream.drain(): - yield chunk - - # The central directory, written when ZipFile closes. - if chunk := stream.drain(): - yield chunk + return _stream_zip(entries) async def update_book( self, book_id: int, update_data: ModelDictT[Book], library: Library diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index be8f59e..9c2cc9d 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -221,7 +221,9 @@ class TestBookServiceCRUD: archive_bytes = b"".join( [ chunk - async for chunk in books_service.get_files([book.id], test_library.id) + async for chunk in await books_service.get_files( + [book.id], test_library.id + ) ] ) @@ -231,13 +233,79 @@ class TestBookServiceCRUD: names = archive.namelist() assert len(names) == len(contents) - # Entries are namespaced by the book's directory, so two books sharing a - # filename cannot overwrite one another. - assert {Path(name).parent.name for name in names} == {"Dracula"} + # Entries carry the file's own name, with no directory in front of it. + assert set(names) == set(contents) for name, payload in contents.items(): - entry = next(n for n in names if Path(n).name == name) - assert archive.read(entry) == payload + assert archive.read(name) == payload + + async def test_get_files_suffixes_a_name_claimed_twice( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Two books can each hold a `Dune.epub`; the archive must keep both. + + Entries are named after the file, and a zip stores the same name twice + happily — leaving whichever copy was written last as the one that survives + extraction. The second is suffixed instead. + """ + payloads = { + "Herbert": b"herbert-payload-" * 512, + "Other": b"other-payload-" * 512, + } + book_ids = [] + + for author, payload in payloads.items(): + book_data = BookCreate( + library_id=test_library.id, + title="Dune", + authors=[author], + pages=412, + ) + book = await books_service.to_model_on_create(book_data.model_dump()) + assert isinstance(book, m.Book) + + book.path = f"{test_library.root_path}/{author}/Dune" + await aios.makedirs(book.path) # type: ignore[arg-type] + Path(book.path, "Dune.epub").write_bytes(payload) + book.files.append( + m.FileMetadata( + path="Dune.epub", + size=len(payload), + hash=f"hash-{author}", + content_type=None, + ) + ) + + books_service.repository.session.add(book) + await books_service.repository.session.commit() + book_ids.append(book.id) + + archive_bytes = b"".join( + [ + chunk + async for chunk in await books_service.get_files( + book_ids, test_library.id + ) + ] + ) + + with zipfile.ZipFile(BytesIO(archive_bytes)) as archive: + assert archive.testzip() is None + assert sorted(archive.namelist()) == ["Dune (2).epub", "Dune.epub"] + + stored = {archive.read(name) for name in archive.namelist()} + assert stored == set(payloads.values()) + + async def test_get_files_refuses_a_selection_with_nothing_in_it( + self, books_service: BookService, test_library: m.Library + ) -> None: + """A miss is a 404, raised before the response starts. + + Resolving lazily inside the stream meant an empty selection produced a valid + 22-byte archive under a 200, which tells the caller nothing went wrong. + """ + with pytest.raises(FileNotFoundError): + await books_service.get_files([424242], test_library.id) @pytest.mark.asyncio diff --git a/frontend/src/lib/state/bookOperations.svelte.ts b/frontend/src/lib/state/bookOperations.svelte.ts index 49bc5d8..f35a0f1 100644 --- a/frontend/src/lib/state/bookOperations.svelte.ts +++ b/frontend/src/lib/state/bookOperations.svelte.ts @@ -134,8 +134,11 @@ export class BookOperationsState { // eslint-disable-next-line svelte/prefer-svelte-reactivity -- local temporary, not reactive state const downloadUrl = new URL(`/api/books/download`, page.url.origin); downloadUrl.searchParams.set('library_id', this.libraryId); + // `append`, not `set` — `set` replaces the existing value, so every book but + // the last was dropped from the query string and the archive came back holding + // one book however many were selected. bookIds.forEach((id) => { - downloadUrl.searchParams.set('book_ids', id.toString()); + downloadUrl.searchParams.append('book_ids', id.toString()); }); this.download(downloadUrl, filename || 'download.zip'); diff --git a/frontend/src/routes/api/[...path]/+server.ts b/frontend/src/routes/api/[...path]/+server.ts index 88b827f..a9d8a6b 100644 --- a/frontend/src/routes/api/[...path]/+server.ts +++ b/frontend/src/routes/api/[...path]/+server.ts @@ -16,16 +16,27 @@ async function handleResponse(response: Response) { const data = await response.json(); return json(data); } else { - // Handle file or other non-JSON response - const responseBody = await response.arrayBuffer(); + // Only headers the backend actually sent are forwarded. Defaulting a missing + // one to '' put `Content-Length: ` on the wire — SvelteKit's `setResponse` + // hands every header to `res.setHeader` as-is, empty or not — and an empty + // Content-Length is not valid HTTP/1.1, so strict clients reject the whole + // response. The multi-book ZIP is streamed and carries no Content-Length, + // which made it the one download that always tripped over this. + const headers = new Headers({ 'Content-Type': contentType }); + for (const name of ['Content-Disposition', 'Content-Length']) { + const value = response.headers.get(name); + if (value) headers.set(name, value); + } - return new Response(responseBody, { + // The body is passed through as a stream, matching what `bodyOf` does for + // requests. `arrayBuffer()` held the whole response here first, so nothing + // reached the browser until the last byte had been read and this process + // carried a copy of it — survivable for one book, not for a ZIP of a whole + // shelf, which only starts arriving once the backend has finished + // compressing all of it. + return new Response(response.body, { status: response.status, - headers: { - 'Content-Type': contentType, - 'Content-Disposition': response.headers.get('Content-Disposition') || '', - 'Content-Length': response.headers.get('Content-Length') || '' - } + headers }); } }