diff --git a/backend/src/chitai/controllers/book.py b/backend/src/chitai/controllers/book.py index 4ca146f..a2ebc71 100644 --- a/backend/src/chitai/controllers/book.py +++ b/backend/src/chitai/controllers/book.py @@ -219,6 +219,52 @@ class BookController(Controller): for group in groups ] + @post(path="merge") + async def merge_books( + self, books_service: BookService, library: m.Library, data: s.BookMerge + ) -> s.BookRead: + """ + Fold several books into one and delete the records folded in. + + The survivor keeps its id, so links and bookmarks still resolve. Files, reading + progress, shelves, tags and unheld identifiers move onto it; metadata is only + changed by what `metadata` names, because choosing between two titles is the + reader's judgement rather than this endpoint's. + + Nothing is removed from disk — a wrong merge should cost metadata that can be + retyped, not a book. + + Query Parameters: + library_id: The library the books belong to. + + Request Body: + data: The survivor, the books to fold in, and the resolved metadata. + + Injected Dependencies: + books_service: The book service for database operations. + library: The library the books belong to. + + Returns: + The surviving book. + + Raises: + HTTPException: 400 if fewer than two distinct books were named, one is + unknown, or they do not all belong to one library. + """ + try: + book = await books_service.merge_books( + data.survivor_id, + data.merged_ids, + library, + metadata=data.metadata.model_dump(exclude_unset=True) + if data.metadata + else None, + ) + except ValueError as exc: + raise HTTPException(status_code=HTTP_400_BAD_REQUEST, detail=str(exc)) + + return books_service.to_schema(book, schema_type=s.BookRead) + @post(path="duplicate-books/dismissals", status_code=HTTP_204_NO_CONTENT) async def dismiss_duplicate_books( self, books_service: BookService, data: s.DuplicateDismissal diff --git a/backend/src/chitai/schemas/__init__.py b/backend/src/chitai/schemas/__init__.py index 09b5306..f5358c1 100644 --- a/backend/src/chitai/schemas/__init__.py +++ b/backend/src/chitai/schemas/__init__.py @@ -5,6 +5,7 @@ from .book import ( BookProgressRead, BooksCreateFromFiles, BooksUploadResult, + BookMerge, BookMetadataUpdate, DuplicateBookGroupRead, DuplicateBookRead, diff --git a/backend/src/chitai/schemas/book.py b/backend/src/chitai/schemas/book.py index b7b9d7f..de511e0 100644 --- a/backend/src/chitai/schemas/book.py +++ b/backend/src/chitai/schemas/book.py @@ -247,6 +247,20 @@ class BookMetadataUpdate(BaseModel): return v +class BookMerge(BaseModel): + """ + Fold several books into one. + + `metadata` is the reader's resolution of the fields the records disagreed on. + Anything it does not name keeps the survivor's value — merging metadata is a + judgement, so nothing is guessed on the caller's behalf. + """ + + survivor_id: int + merged_ids: list[int] + metadata: Optional["BookMetadataUpdate"] = None + + class BookProgressCreate(BaseModel): percentage: float epub_cfi: str | None = None diff --git a/backend/src/chitai/services/book.py b/backend/src/chitai/services/book.py index bcf46c0..3399b41 100644 --- a/backend/src/chitai/services/book.py +++ b/backend/src/chitai/services/book.py @@ -24,7 +24,7 @@ from advanced_alchemy.service import ( ) from advanced_alchemy.repository import SQLAlchemyAsyncRepository from advanced_alchemy.filters import CollectionFilter -from sqlalchemy import and_, delete, inspect, or_, select, tuple_ +from sqlalchemy import and_, delete, inspect, or_, select, tuple_, update from sqlalchemy.orm import joinedload, selectinload from litestar.response import File from litestar.datastructures import UploadFile @@ -38,8 +38,11 @@ from chitai.database.models import ( Book, Author, BookAuthorLink, + BookListLink, + BookProgress, BookTagLink, DuplicateDismissal, + KosyncProgress, Tag, Publisher, BookSeries, @@ -622,6 +625,285 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): return matches + async def merge_books( + self, + survivor_id: int, + merged_ids: list[int], + library: Library, + metadata: ModelDictT[Book] | dict[str, Any] | None = None, + ) -> Book: + """ + Fold several books into one, and delete the records that were folded in. + + The survivor keeps its id, so every link, bookmark and shelf entry pointing at + it still resolves. Everything the others carried moves onto it: their files, + their reading progress where it is further along, their shelves, tags and any + identifier under a name the survivor lacks. + + **Metadata is not merged automatically.** The survivor's own columns are kept + unless `metadata` says otherwise, because guessing which of two titles is the + better one is exactly the judgement the caller is making. The review screen + sends what the reader chose; a scripted merge that sends nothing gets the + survivor's metadata verbatim, which is at least predictable. + + Nothing is deleted from disk. Files move into the survivor's directory and the + emptied directories are pruned, but no bytes and no cover are removed — there + is no undo, so a wrong merge should cost metadata that can be retyped rather + than a book that cannot be got back. + + Args: + survivor_id: The book to keep. + merged_ids: The books to fold into it and then delete. + library: The library they belong to. + metadata: Field values to write onto the survivor, as the reader resolved + them. + + Returns: + The surviving book. + + Raises: + ValueError: If fewer than two distinct books were named, one of them does + not exist, or they do not all belong to one library. + """ + merged_ids = [book_id for book_id in dict.fromkeys(merged_ids) if book_id != survivor_id] + + if not merged_ids: + raise ValueError("A merge needs at least two different books") + + books = await self.list(Book.id.in_([survivor_id, *merged_ids])) + by_id = {book.id: book for book in books} + + if len(by_id) != len(merged_ids) + 1: + raise ValueError("No such book") + + # Refused rather than handled: a cross-library merge moves files between two + # configured root paths, which is a surprise nobody asked this endpoint for. + if {book.library_id for book in books} != {library.id}: + raise ValueError("Every book in a merge must belong to the same library") + + survivor = by_id[survivor_id] + losers = [by_id[book_id] for book_id in merged_ids] + + await self._absorb_files(survivor, losers, library) + await self._absorb_rows(survivor_id, merged_ids) + + # Read before the rows go: expiring the session afterwards would send an + # attribute access looking for a book that no longer exists. + survivor_path = survivor.path + emptied = [loser.path for loser in losers if loser.path] + + session = self.repository.session + await session.flush() + + # The losers' rows are gone from here on; every child table either moved above + # or is removed by the FK cascade. + await self._sync(delete(Book).where(Book.id.in_(merged_ids))) + + if metadata: + # Refreshed first, and explicitly. `synchronize_session="fetch"` keeps an + # object's own columns honest but says nothing about a *collection* that + # gained a row, so the survivor still believes it holds the tags and + # identifiers it started with. `update_book` reconciles those collections, + # and against a stale one it builds a second link row that collides with + # the one just moved onto it. + await session.refresh( + survivor, + ["files", "author_links", "tag_links", "identifiers", "list_links"], + ) + await self.update_book(survivor_id, metadata, library) + + for path in emptied: + if path != survivor_path: + cleanup_empty_parent_directories(Path(path), Path(library.root_path)) + + # Re-read rather than trust the identity map: the survivor's collections were + # repointed by statements the ORM did not run through its own bookkeeping. + return await self.get( + survivor_id, execution_options={"populate_existing": True} + ) + + async def _sync(self, statement): + """ + Run a bulk update or delete and keep the session's loaded objects honest. + + `synchronize_session="fetch"` is what lets these statements run at all here: + without it the identity map goes on serving the values the rows held before, + and every caller still holding a book gets stale answers. The blunt + alternative, expiring the whole session, punishes callers that did nothing + wrong by making their next attribute access do IO. + """ + return await self.repository.session.execute( + statement.execution_options(synchronize_session="fetch") + ) + + async def _absorb_files( + self, survivor: Book, losers: list[Book], library: Library + ) -> None: + """ + Move the merged books' files into the survivor's directory. + + `FileMetadata.path` is a bare filename resolved against `book.path`, so moving + the row without moving the bytes leaves it describing a file that is not there. + + A row whose file has already gone missing is repointed anyway rather than + dropped: it is the only remaining record that the book had that format, and + `add_files` knows how to put the bytes back into a row that has lost them. + """ + if survivor.path is None: + path_gen = BookPathGenerator(library.root_path) + survivor.path = str( + await self._reserve_book_path( + path_gen.generate_path(survivor.to_dict()), survivor.id + ) + ) + + destination = Path(survivor.path) + destination.mkdir(parents=True, exist_ok=True) + + for loser in losers: + for file in loser.files: + if loser.path: + source = Path(loser.path) / file.path + if await aios.path.isfile(source): + target = _unused_path(destination / Path(file.path).name) + await move_file(source, target) + file.path = target.name + + file.book_id = survivor.id + + async def _absorb_rows(self, survivor_id: int, merged_ids: list[int]) -> None: + """ + Repoint everything hanging off the merged books onto the survivor. + + Core statements rather than the ORM: `Book.files` and the link collections are + `delete-orphan`, so reassigning them through loaded objects invites SQLAlchemy + to delete the very rows being moved. Whatever is not moved here is removed by + the `ondelete="cascade"` on its foreign key when the book row goes. + + Args: + survivor_id: The book everything is moving onto. + merged_ids: The books being emptied. + """ + session = self.repository.session + + # Reading progress is per user, and the furthest one is the true answer for a + # reader who has been through the EPUB and not the PDF. + rows = ( + await session.execute( + select(BookProgress) + .where(BookProgress.book_id.in_([survivor_id, *merged_ids])) + .order_by(BookProgress.percentage.desc()) + ) + ).scalars().all() + + furthest: dict[int, BookProgress] = {} + for progress in rows: + furthest.setdefault(progress.user_id, progress) + + # Every id, not just the merged ones: the row being beaten is often the + # survivor's own, and leaving it behind gives one user two progress rows. + keep = {progress.id for progress in furthest.values()} + await self._sync( + delete(BookProgress).where( + BookProgress.book_id.in_([survivor_id, *merged_ids]), + BookProgress.id.notin_(keep), + ) + ) + await self._sync( + update(BookProgress) + .where(BookProgress.id.in_(keep), BookProgress.book_id.in_(merged_ids)) + .values(book_id=survivor_id) + ) + + # Keyed by the KOReader document hash, which follows the file, so a device + # carries on syncing without noticing anything happened. + await self._sync( + update(KosyncProgress) + .where(KosyncProgress.book_id.in_(merged_ids)) + .values(book_id=survivor_id) + ) + + # Collections the survivor may already be in. The unique constraint on each + # would refuse a second link, so drop those before repointing the rest. + for model, column in ( + (BookListLink, BookListLink.list_id), + (BookTagLink, BookTagLink.tag_id), + ): + held = select(column).where(model.book_id == survivor_id) + await self._sync( + delete(model).where(model.book_id.in_(merged_ids), column.in_(held)) + ) + await self._sync( + update(model) + .where(model.book_id.in_(merged_ids)) + .values(book_id=survivor_id) + ) + + # Identifiers are unique per name, so only names the survivor lacks can move, + # and only one of them however many books offered it. + held_names = select(Identifier.name).where(Identifier.book_id == survivor_id) + incoming = ( + await session.execute( + select(Identifier) + .where( + Identifier.book_id.in_(merged_ids), Identifier.name.notin_(held_names) + ) + .order_by(Identifier.book_id, Identifier.id) + ) + ).scalars().all() + + taken: set[str] = set() + for identifier in incoming: + if identifier.name in taken: + continue + taken.add(identifier.name) + identifier.book_id = survivor_id + + await self._absorb_dismissals(survivor_id, merged_ids) + + async def _absorb_dismissals(self, survivor_id: int, merged_ids: list[int]) -> None: + """ + Carry over "not a duplicate" verdicts, without inventing new ones. + + A pair between two books being merged into each other stops meaning anything + and is dropped; a pair with some third book still holds, and would otherwise be + forgotten the moment its book was deleted. + """ + session = self.repository.session + merged = set(merged_ids) + + rows = ( + await session.execute( + select(DuplicateDismissal).where( + or_( + DuplicateDismissal.book_a_id.in_(merged_ids), + DuplicateDismissal.book_b_id.in_(merged_ids), + ) + ) + ) + ).scalars().all() + + existing = await self._dismissed_pairs() + doomed: list[int] = [] + + for row in rows: + first = survivor_id if row.book_a_id in merged else row.book_a_id + second = survivor_id if row.book_b_id in merged else row.book_b_id + + pair = DuplicateDismissal.pair(first, second) + + if pair[0] == pair[1] or pair in existing: + doomed.append(row.id) + continue + + existing.add(pair) + row.book_a_id, row.book_b_id = pair + + if doomed: + await self._sync( + delete(DuplicateDismissal).where(DuplicateDismissal.id.in_(doomed)) + ) + async def _load_for_matching(self, book_id: int) -> Book: """ Fetch a book with everything the comparison reads already loaded. diff --git a/backend/tests/integration/test_book.py b/backend/tests/integration/test_book.py index 29ab956..62139dd 100644 --- a/backend/tests/integration/test_book.py +++ b/backend/tests/integration/test_book.py @@ -706,6 +706,55 @@ class TestDuplicateBooks: response = await authenticated_client.get("/books/duplicate-books?library_id=1") assert len(response.json()) == 1 + async def test_two_books_merge_into_one( + self, authenticated_client: AsyncClient + ) -> None: + """The survivor keeps its id and gains the other's file; the other is gone.""" + first = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload("first.epub") + ) + second = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload("second.epub", pad=True) + ) + keep = first.json()["created"][0] + fold = second.json()["created"][0] + + response = await authenticated_client.post( + "/books/merge?library_id=1", + json={ + "survivor_id": keep["id"], + "merged_ids": [fold["id"]], + "metadata": {"title": "Metamorphosis", "edition": 2}, + }, + ) + + assert response.status_code == 201 + merged = response.json() + + assert merged["id"] == keep["id"] + assert merged["edition"] == 2 + assert len(merged["files"]) == 2 + + # The folded record is gone, and the group it formed with it. + assert (await authenticated_client.get(f"/books/{fold['id']}")).status_code == 404 + groups = await authenticated_client.get("/books/duplicate-books?library_id=1") + assert groups.json() == [] + + async def test_merging_an_unknown_book_is_refused( + self, authenticated_client: AsyncClient + ) -> None: + created = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload("first.epub") + ) + keep = created.json()["created"][0]["id"] + + response = await authenticated_client.post( + "/books/merge?library_id=1", + json={"survivor_id": keep, "merged_ids": [9999]}, + ) + + assert response.status_code == 400 + async def test_dismissing_an_unknown_book_is_refused( self, authenticated_client: AsyncClient ) -> None: diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index 535982c..bda3c37 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -7,6 +7,7 @@ from pathlib import Path import pytest import aiofiles.os as aios from litestar.datastructures import UploadFile +from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from chitai.config import DuplicateScope, settings @@ -961,6 +962,244 @@ class TestAuthorNames: assert not author.name.endswith((".epub", ".pdf", ".mobi")) +@pytest.mark.asyncio +class TestMergeBooks: + """Folding several books into one, and what has to come with them.""" + + async def test_files_move_onto_the_survivor( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The rows move and so do the bytes — `file.path` is relative to `book.path`.""" + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB, "one.epub")]), test_library + ) + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(PDF, "two.pdf")]), test_library + ) + keep, fold = first.books[0], second.books[0] + gone = Path(fold.path) + + merged = await books_service.merge_books(keep.id, [fold.id], test_library) + + assert len(merged.files) == 2 + for file in merged.files: + assert (Path(merged.path) / file.path).is_file() + + assert not gone.exists() + with pytest.raises(Exception): + await books_service.get(fold.id) + + async def test_a_filename_collision_is_given_its_own_name( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Two books' files can share a name; one must not overwrite the other.""" + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB, "book.epub")]), test_library + ) + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[edition(EPUB, "book.epub")]), test_library + ) + + merged = await books_service.merge_books( + first.books[0].id, [second.books[0].id], test_library + ) + + paths = sorted(file.path for file in merged.files) + assert len(paths) == 2 and paths[0] != paths[1] + for file in merged.files: + assert (Path(merged.path) / file.path).is_file() + + async def test_metadata_is_only_what_the_caller_resolved( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The survivor keeps its own fields unless the caller says otherwise.""" + keep = await store_book( + books_service, test_library, title="Building Microservices", authors=["Sam Newman"] + ) + fold = await store_book( + books_service, + test_library, + title="Building Microservices", + authors=["Sam Newman"], + publisher="O'Reilly", + edition=2, + ) + + merged = await books_service.merge_books(keep.id, [fold.id], test_library) + assert merged.edition is None + assert merged.publisher is None + + other = await store_book( + books_service, test_library, title="Monolith", authors=["Sam Newman"], edition=3 + ) + merged = await books_service.merge_books( + keep.id, [other.id], test_library, metadata={"edition": 3} + ) + assert merged.edition == 3 + + async def test_resolved_collections_do_not_collide_with_what_moved( + self, books_service: BookService, test_library: m.Library + ) -> None: + """ + Tags and identifiers move onto the survivor *and* appear in the resolution. + + The survivor's loaded collections do not learn about a row repointed by a bulk + statement, so reconciling them against stale state builds a second link and + breaches the unique constraint. Scalar-only metadata never sees it. + """ + keep = await store_book( + books_service, test_library, title="A", authors=["X"], tags=["Kept"] + ) + fold = await store_book( + books_service, + test_library, + title="B", + authors=["X"], + tags=["Moved"], + identifiers={"asin": "B000FC0PDA"}, + ) + + merged = await books_service.merge_books( + keep.id, + [fold.id], + test_library, + metadata={ + "tags": ["Kept", "Moved"], + "identifiers": {"asin": "B000FC0PDA"}, + "publisher": "O'Reilly", + }, + ) + + assert sorted(tag.name for tag in merged.tags) == ["Kept", "Moved"] + assert {i.name: i.value for i in merged.identifiers} == {"asin": "B000FC0PDA"} + assert merged.publisher is not None and merged.publisher.name == "O'Reilly" + + async def test_the_furthest_progress_survives( + self, + books_service: BookService, + test_library: m.Library, + test_user: m.User, + session: AsyncSession, + ) -> None: + """A reader who got 60% through the PDF has not gone back to page one.""" + keep = await store_book(books_service, test_library, title="A", authors=["X"]) + fold = await store_book(books_service, test_library, title="B", authors=["X"]) + + session.add_all( + [ + m.BookProgress(user_id=test_user.id, book_id=keep.id, percentage=0.1), + m.BookProgress(user_id=test_user.id, book_id=fold.id, percentage=0.6), + ] + ) + await session.commit() + + await books_service.merge_books(keep.id, [fold.id], test_library) + + rows = ( + await books_service.repository.session.execute( + select(m.BookProgress).where(m.BookProgress.book_id == keep.id) + ) + ).scalars().all() + + assert [row.percentage for row in rows] == [0.6] + + async def test_shelves_and_tags_come_across_without_duplicating( + self, + books_service: BookService, + test_library: m.Library, + test_user: m.User, + session: AsyncSession, + ) -> None: + """Both books on one shelf must not leave the survivor linked to it twice.""" + keep = await store_book( + books_service, test_library, title="A", authors=["X"], tags=["Shared", "Only Keep"] + ) + fold = await store_book( + books_service, test_library, title="B", authors=["X"], tags=["Shared", "Only Fold"] + ) + + shelf = m.BookList(title="Later", user_id=test_user.id, library_id=test_library.id) + session.add(shelf) + await session.commit() + session.add_all( + [ + m.BookListLink(book_id=keep.id, list_id=shelf.id, position=0), + m.BookListLink(book_id=fold.id, list_id=shelf.id, position=0), + ] + ) + await session.commit() + + merged = await books_service.merge_books(keep.id, [fold.id], test_library) + + assert sorted(tag.name for tag in merged.tags) == ["Only Fold", "Only Keep", "Shared"] + + links = ( + await books_service.repository.session.execute( + select(m.BookListLink).where(m.BookListLink.book_id == keep.id) + ) + ).scalars().all() + assert len(links) == 1 + + async def test_an_identifier_moves_only_under_a_name_the_survivor_lacks( + self, books_service: BookService, test_library: m.Library + ) -> None: + """`(name, book_id)` is unique, so a competing isbn-13 cannot come across.""" + keep = await store_book( + books_service, + test_library, + title="A", + authors=["X"], + identifiers={"isbn-13": "9780486282114"}, + ) + fold = await store_book( + books_service, + test_library, + title="B", + authors=["X"], + identifiers={"isbn-13": "9781492034025", "asin": "B000FC0PDA"}, + ) + + merged = await books_service.merge_books(keep.id, [fold.id], test_library) + + stored = {i.name: i.value for i in merged.identifiers} + assert stored == {"isbn-13": "9780486282114", "asin": "B000FC0PDA"} + + async def test_dismissals_follow_the_survivor( + self, books_service: BookService, test_library: m.Library + ) -> None: + """A verdict about some third book still holds after its partner is merged.""" + keep = await store_book(books_service, test_library, title="A", authors=["X"]) + fold = await store_book(books_service, test_library, title="B", authors=["X"]) + third = await store_book(books_service, test_library, title="C", authors=["X"]) + + # One pair between the two being merged, one pointing outside the merge. + await books_service.dismiss_duplicates(keep.id, fold.id) + await books_service.dismiss_duplicates(fold.id, third.id) + + await books_service.merge_books(keep.id, [fold.id], test_library) + + pairs = await books_service._dismissed_pairs() + assert pairs == {m.DuplicateDismissal.pair(keep.id, third.id)} + + @pytest.mark.parametrize("merged", [[], [1]]) + async def test_a_merge_needs_two_different_books( + self, books_service: BookService, test_library: m.Library, merged: list[int] + ) -> None: + keep = await store_book(books_service, test_library, title="A", authors=["X"]) + ids = [keep.id] if merged else [] + + with pytest.raises(ValueError): + await books_service.merge_books(keep.id, ids, test_library) + + async def test_an_unknown_book_is_refused( + self, books_service: BookService, test_library: m.Library + ) -> None: + keep = await store_book(books_service, test_library, title="A", authors=["X"]) + + with pytest.raises(ValueError): + await books_service.merge_books(keep.id, [keep.id + 10_000], test_library) + + @pytest.mark.asyncio class TestDuplicateBookGroups: """The pass over a library someone already has."""