fix: reuse existing link rows when updating book relationships
Assigning through the association proxy recreated links for targets the book already had, colliding with the link tables' unique constraints. Reconcile the collections in place instead, identifiers included.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user