diff --git a/backend/src/chitai/services/book.py b/backend/src/chitai/services/book.py index d8cde00..8cc828d 100644 --- a/backend/src/chitai/services/book.py +++ b/backend/src/chitai/services/book.py @@ -5,6 +5,7 @@ from __future__ import annotations from collections import defaultdict import mimetypes +from collections.abc import Callable from io import BytesIO from pathlib import Path import uuid @@ -33,6 +34,8 @@ from chitai.config import settings from chitai.database.models import ( Book, Author, + BookAuthorLink, + BookTagLink, Tag, Publisher, BookSeries, @@ -475,10 +478,16 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): model_data = await super().to_model(data) if "authors" in data: - model_data.authors = [ + authors = [ await Author.as_unique_async(self.repository.session, name=author) for author in data["authors"] ] + self._sync_link_collection( + model_data.author_links, + authors, + "author", + lambda author: BookAuthorLink(author=author), + ) if "series" in data: if data["series"]: @@ -497,19 +506,96 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): model_data.publisher = None if "tags" in data: - model_data.tags = [ + tags = [ await Tag.as_unique_async(self.repository.session, name=tag) for tag in data["tags"] ] + self._sync_link_collection( + model_data.tag_links, + tags, + "tag", + lambda tag: BookTagLink(tag=tag), + ) if "identifiers" in data: - model_data.identifiers = data["identifiers"] + self._sync_identifiers(model_data, data["identifiers"]) if "files" in data: model_data.files = data["files"] return model_data + @staticmethod + def _sync_link_collection( + links: list[Any], + targets: list[Any], + attr: str, + creator: Callable[[Any], Any], + ) -> None: + """ + Reconcile an association-proxy link collection against the desired targets. + + Assigning through the association proxy runs its ``creator`` for every target, + so a target that is already attached gets a brand new link row while the old + one is orphaned. SQLAlchemy flushes a mapper's INSERTs before its DELETEs, so + the new row hits the ``(book_id, tag_id)`` / ``(book_id, author_id)`` unique + constraint while its predecessor is still in the table. Reusing the existing + link keeps that from happening, and holds on to its id. + + Args: + links: The link collection to reconcile in place. + targets: The entities the collection should end up pointing at. + attr: Name of the attribute on a link that holds the target. + creator: Builds a new link for a target that is not attached yet. + """ + existing = {getattr(link, attr): link for link in links} + reconciled: list[Any] = [] + seen: set[Any] = set() + + for target in targets: + if target in seen: + continue + seen.add(target) + reconciled.append(existing.get(target) or creator(target)) + + # Slice assignment so ordering_list renumbers `position` from the new order. + links[:] = reconciled + + @staticmethod + def _sync_identifiers(book: Book, identifiers: Any) -> None: + """ + Reconcile a book's identifiers against the incoming ones, keyed by name. + + Same hazard as `_sync_link_collection`: `Identifier` is unique on + ``(name, book_id)``, so replacing the collection wholesale re-inserts a name + the book already carries. Existing rows are updated in place instead. + + Args: + book: The book whose identifiers are being reconciled. + identifiers: The incoming `Identifier` instances. + """ + if not all(isinstance(identifier, Identifier) for identifier in identifiers): + book.identifiers = identifiers + return + + existing = {identifier.name: identifier for identifier in book.identifiers} + reconciled: list[Identifier] = [] + seen: set[str] = set() + + for incoming in identifiers: + if incoming.name in seen: + continue + seen.add(incoming.name) + + current = existing.get(incoming.name) + if current is None: + reconciled.append(incoming) + else: + current.value = incoming.value + reconciled.append(current) + + book.identifiers[:] = reconciled + async def _save_book_files(self, library: Library, data: dict) -> list[FileMetadata]: """ Save uploaded book files to the filesystem. diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index 98b0ff7..92cd741 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -67,3 +67,74 @@ class TestBookServiceCRUD: assert updated_book.publisher.name == "Tolkien Estate" assert len(updated_book.tags) == 2 + + async def test_update_book_reuses_existing_links( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Re-submitting a relationship a book already has must not duplicate its link. + + The link tables are unique on (book_id, tag_id) / (book_id, author_id), and + identifiers on (name, book_id), so replacing a collection wholesale used to + insert a row that collided with the one it was replacing. + """ + book_data = BookCreate( + library_id=1, + title="The Two Towers", + authors=["J.R.R Tolkien"], + tags=["Fantasy"], + identifiers={"isbn-13": "9780261102358"}, + pages=352, + ) + + book = await books_service.to_model_on_create(book_data.model_dump()) + assert isinstance(book, m.Book) + + # Matches what the default template generates, so these updates move nothing. + book.path = f"{test_library.root_path}/J.R.R Tolkien/The Two Towers" + await aios.makedirs(book.path) # type: ignore[arg-type] + + books_service.repository.session.add(book) + await books_service.repository.session.commit() + await books_service.repository.session.refresh(book) + + # Through the service, so the link collections come back eagerly loaded. + created_book = await books_service.get(book.id) + original_tag_link_id = created_book.tag_links[0].id + + # Every collection resubmitted unchanged. + await books_service.update_book( + book.id, + { + "authors": ["J.R.R Tolkien"], + "tags": ["Fantasy"], + "identifiers": {"isbn-13": "9780261102358"}, + }, + test_library, + ) + + updated_book = await books_service.get(book.id) + assert [tag.name for tag in updated_book.tags] == ["Fantasy"] + assert [author.name for author in updated_book.authors] == ["J.R.R Tolkien"] + assert len(updated_book.identifiers) == 1 + # The existing link is reused, not deleted and reinserted. + assert updated_book.tag_links[0].id == original_tag_link_id + + # Keeping one tag while adding another. + await books_service.update_book( + book.id, {"tags": ["Fantasy", "Adventure"]}, test_library + ) + updated_book = await books_service.get(book.id) + assert [tag.name for tag in updated_book.tags] == ["Fantasy", "Adventure"] + + # Dropping one while keeping the other. + await books_service.update_book(book.id, {"tags": ["Adventure"]}, test_library) + updated_book = await books_service.get(book.id) + assert [tag.name for tag in updated_book.tags] == ["Adventure"] + + # A name the book already carries has its value updated in place. + await books_service.update_book( + book.id, {"identifiers": {"isbn-13": "9780261102999"}}, test_library + ) + updated_book = await books_service.get(book.id) + assert len(updated_book.identifiers) == 1 + assert updated_book.identifiers[0].value == "9780261102999"