feat: detect books that may already be in the library
Match on a shared identifier, or on a normalised title credited to a shared author, and report candidates rather than refusing anything — a metadata match is a guess, and a second edition is not a mistake. Adds a library-wide review pass, dismissals, and renames the fingerprint pre-flight to duplicate-files.
This commit is contained in:
@@ -570,7 +570,7 @@ class TestDuplicateHandling:
|
||||
stored = book["files"][0]
|
||||
|
||||
response = await authenticated_client.post(
|
||||
"/books/duplicates?library_id=1",
|
||||
"/books/duplicate-files?library_id=1",
|
||||
json=[
|
||||
{
|
||||
"hash": stored["hash"],
|
||||
@@ -590,6 +590,117 @@ class TestDuplicateHandling:
|
||||
assert matches[0]["book_id"] == book["id"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
class TestDuplicateBooks:
|
||||
"""A second copy of a book is imported and reported, never refused."""
|
||||
|
||||
epub_path = Path("tests/data_files/Metamorphosis - Franz Kafka.epub")
|
||||
|
||||
def upload(self, name: str, pad: bool = False) -> list[tuple[str, tuple]]:
|
||||
"""
|
||||
The fixture, optionally padded so it is a different file and the same book.
|
||||
|
||||
Padding the archive changes its size and its sampled hash without disturbing
|
||||
the metadata, which is exactly the case the file-level check cannot see.
|
||||
"""
|
||||
data = self.epub_path.read_bytes() + (b"\0" * 64 if pad else b"")
|
||||
|
||||
return [("files", (name, data, "application/epub+zip"))]
|
||||
|
||||
async def test_a_second_edition_is_created_and_reported(
|
||||
self, authenticated_client: AsyncClient
|
||||
) -> None:
|
||||
first = await authenticated_client.post(
|
||||
"/books/fromFiles?library_id=1", files=self.upload("first.epub")
|
||||
)
|
||||
original = first.json()["created"][0]
|
||||
|
||||
second = await authenticated_client.post(
|
||||
"/books/fromFiles?library_id=1", files=self.upload("second.epub", pad=True)
|
||||
)
|
||||
|
||||
assert second.status_code == 201
|
||||
body = second.json()
|
||||
|
||||
# Created, not skipped: a metadata match is a guess, and refusing a legitimate
|
||||
# second edition costs more than a note does.
|
||||
assert len(body["created"]) == 1
|
||||
assert body["skipped"] == []
|
||||
|
||||
assert len(body["possible_duplicates"]) == 1
|
||||
possible = body["possible_duplicates"][0]
|
||||
assert possible["book_id"] == body["created"][0]["id"]
|
||||
|
||||
candidate = possible["candidates"][0]
|
||||
assert candidate["book_id"] == original["id"]
|
||||
assert candidate["title"] == original["title"]
|
||||
assert candidate["authors"] == ["Franz Kafka"]
|
||||
assert "title-author" in candidate["matched_on"]
|
||||
|
||||
async def test_the_review_screen_groups_them(
|
||||
self, authenticated_client: AsyncClient
|
||||
) -> None:
|
||||
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)
|
||||
)
|
||||
|
||||
book_ids = sorted(
|
||||
[first.json()["created"][0]["id"], second.json()["created"][0]["id"]]
|
||||
)
|
||||
|
||||
response = await authenticated_client.get("/books/duplicate-books?library_id=1")
|
||||
|
||||
assert response.status_code == 200
|
||||
groups = response.json()
|
||||
assert len(groups) == 1
|
||||
assert [book["book_id"] for book in groups[0]["books"]] == book_ids
|
||||
|
||||
async def test_a_dismissed_group_stays_dismissed(
|
||||
self, authenticated_client: AsyncClient
|
||||
) -> None:
|
||||
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)
|
||||
)
|
||||
|
||||
pair = {
|
||||
"book_a_id": first.json()["created"][0]["id"],
|
||||
"book_b_id": second.json()["created"][0]["id"],
|
||||
}
|
||||
|
||||
dismissed = await authenticated_client.post(
|
||||
"/books/duplicate-books/dismissals", json=pair
|
||||
)
|
||||
assert dismissed.status_code == 204
|
||||
|
||||
response = await authenticated_client.get("/books/duplicate-books?library_id=1")
|
||||
assert response.json() == []
|
||||
|
||||
restored = await authenticated_client.delete(
|
||||
"/books/duplicate-books/dismissals"
|
||||
f"?book_a_id={pair['book_b_id']}&book_b_id={pair['book_a_id']}"
|
||||
)
|
||||
assert restored.status_code == 204
|
||||
|
||||
response = await authenticated_client.get("/books/duplicate-books?library_id=1")
|
||||
assert len(response.json()) == 1
|
||||
|
||||
async def test_dismissing_an_unknown_book_is_refused(
|
||||
self, authenticated_client: AsyncClient
|
||||
) -> None:
|
||||
response = await authenticated_client.post(
|
||||
"/books/duplicate-books/dismissals",
|
||||
json={"book_a_id": 1, "book_b_id": 9999},
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
|
||||
|
||||
# NOTE: the multi-book ZIP download is covered at the service level, in
|
||||
# tests/unit/test_services/test_book_service.py. Driving `/books/download` through
|
||||
# AsyncTestClient hangs in fixture teardown: it is the only `Stream` endpoint in the
|
||||
|
||||
@@ -30,6 +30,21 @@ def upload(path: Path, name: str | None = None) -> UploadFile:
|
||||
)
|
||||
|
||||
|
||||
def edition(path: Path, name: str) -> UploadFile:
|
||||
"""
|
||||
Another edition of one of the fixtures: same book, different bytes.
|
||||
|
||||
Padding the archive changes the size and the sampled hash without disturbing
|
||||
anything a reader or a metadata extractor sees, which is the case file-level
|
||||
dedupe cannot answer and book-level detection exists for.
|
||||
"""
|
||||
return UploadFile(
|
||||
content_type="application/epub+zip",
|
||||
filename=name,
|
||||
file_data=path.read_bytes() + b"\0" * 64,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
class TestBookServiceCRUD:
|
||||
"""Test CRUD operation for libraries."""
|
||||
@@ -640,3 +655,343 @@ class TestMissingFiles:
|
||||
assert len(book.files) == 1
|
||||
assert path.is_file()
|
||||
assert path.read_bytes() == EPUB.read_bytes()
|
||||
|
||||
|
||||
async def store_book(
|
||||
books_service: BookService, library: m.Library, **metadata
|
||||
) -> m.Book:
|
||||
"""
|
||||
A book in the database carrying exactly the metadata given, and no files.
|
||||
|
||||
Book-level matching reads metadata only, so going through the file pipeline would
|
||||
just make every case depend on what a fixture EPUB happens to declare.
|
||||
"""
|
||||
book = await books_service.to_model_on_create(
|
||||
BookCreate(library_id=library.id, **metadata).model_dump()
|
||||
)
|
||||
|
||||
books_service.repository.session.add(book)
|
||||
await books_service.repository.session.commit()
|
||||
|
||||
return book
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
class TestDuplicateBooks:
|
||||
"""The same book arriving as different bytes: reported, never refused."""
|
||||
|
||||
async def test_an_identifier_alone_is_enough(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Two printings of one edition agree on the ISBN and nothing else."""
|
||||
stored = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Metamorphosis",
|
||||
authors=["Franz Kafka"],
|
||||
identifiers={"isbn-13": "9780486282114"},
|
||||
)
|
||||
|
||||
matches = await books_service.find_duplicate_books(
|
||||
{
|
||||
"title": "Die Verwandlung",
|
||||
"authors": ["Someone Else"],
|
||||
# The ISBN-10 of the same edition, written with its hyphens.
|
||||
"identifiers": {"isbn-10": "0-486-28211-2"},
|
||||
},
|
||||
test_library,
|
||||
)
|
||||
|
||||
assert [match.book_id for match in matches] == [stored.id]
|
||||
assert matches[0].matched_on == ["identifier"]
|
||||
assert matches[0].authors == ["Franz Kafka"]
|
||||
|
||||
async def test_a_title_and_a_shared_author_are_enough(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""A re-scan carries no identifier at all, only what is on the cover."""
|
||||
stored = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="The Metamorphosis",
|
||||
authors=["Kafka, Franz"],
|
||||
)
|
||||
|
||||
matches = await books_service.find_duplicate_books(
|
||||
{"title": "Metamorphosis", "authors": ["Franz Kafka"]}, test_library
|
||||
)
|
||||
|
||||
assert [match.book_id for match in matches] == [stored.id]
|
||||
assert matches[0].matched_on == ["title-author"]
|
||||
|
||||
async def test_an_author_left_in_the_title_still_matches(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""
|
||||
Files are named `Title - Author.epub`, and that name often became the title.
|
||||
|
||||
One copy stored as "Building Microservices" and another as "Building
|
||||
Microservices Sam Newman" are the same book, and comparing the title columns
|
||||
as they stand would never say so.
|
||||
"""
|
||||
stored = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Building Microservices - Sam Newman",
|
||||
authors=["Sam Newman"],
|
||||
)
|
||||
|
||||
matches = await books_service.find_duplicate_books(
|
||||
{"title": "Building Microservices", "authors": ["Newman, Sam;"]}, test_library
|
||||
)
|
||||
|
||||
assert [match.book_id for match in matches] == [stored.id]
|
||||
assert matches[0].matched_on == ["title-author"]
|
||||
|
||||
async def test_the_author_in_the_title_works_the_other_way_round(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Whichever copy arrived first, the comparison has to reach the other."""
|
||||
stored = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Building Microservices",
|
||||
authors=["Newman, Sam;"],
|
||||
)
|
||||
|
||||
matches = await books_service.find_duplicate_books(
|
||||
{"title": "Building Microservices - Sam Newman", "authors": ["Sam Newman"]},
|
||||
test_library,
|
||||
)
|
||||
|
||||
assert [match.book_id for match in matches] == [stored.id]
|
||||
|
||||
async def test_a_shared_author_is_still_required(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""The title variants must not become a way around the author requirement."""
|
||||
await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Building Microservices - Sam Newman",
|
||||
authors=["Sam Newman"],
|
||||
)
|
||||
|
||||
assert (
|
||||
await books_service.find_duplicate_books(
|
||||
{"title": "Building Microservices", "authors": ["Martin Fowler"]},
|
||||
test_library,
|
||||
)
|
||||
== []
|
||||
)
|
||||
|
||||
async def test_a_title_without_a_shared_author_is_not_enough(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Otherwise every book the extractors gave up on matches every other one."""
|
||||
await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
assert (
|
||||
await books_service.find_duplicate_books(
|
||||
{"title": "Metamorphosis", "authors": ["Peter Kuper"]}, test_library
|
||||
)
|
||||
== []
|
||||
)
|
||||
|
||||
async def test_a_book_with_no_authors_can_only_match_on_an_identifier(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
await store_book(
|
||||
books_service, test_library, title="Unknown", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
assert (
|
||||
await books_service.find_duplicate_books(
|
||||
{"title": "Unknown", "authors": []}, test_library
|
||||
)
|
||||
== []
|
||||
)
|
||||
|
||||
async def test_another_volume_of_a_series_is_not_a_duplicate(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""The position is the library saying outright that these are two books."""
|
||||
await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Foundation",
|
||||
authors=["Isaac Asimov"],
|
||||
series="Foundation",
|
||||
series_position="1",
|
||||
)
|
||||
|
||||
incoming = {
|
||||
"title": "Foundation",
|
||||
"authors": ["Isaac Asimov"],
|
||||
"series": "Foundation",
|
||||
}
|
||||
|
||||
assert await books_service.find_duplicate_books(
|
||||
incoming | {"series_position": "2"}, test_library
|
||||
) == []
|
||||
|
||||
# The same volume, written a little differently, still matches.
|
||||
assert len(
|
||||
await books_service.find_duplicate_books(
|
||||
incoming | {"series_position": "1.0"}, test_library
|
||||
)
|
||||
) == 1
|
||||
|
||||
async def test_a_book_is_not_its_own_duplicate(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
stored = await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
assert (
|
||||
await books_service.find_duplicate_books(
|
||||
stored, test_library, exclude_book_id=stored.id
|
||||
)
|
||||
== []
|
||||
)
|
||||
|
||||
async def test_scope_off_reports_nothing(
|
||||
self,
|
||||
books_service: BookService,
|
||||
test_library: m.Library,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
monkeypatch.setattr(settings, "duplicate_scope", DuplicateScope.OFF)
|
||||
|
||||
incoming = {"title": "Metamorphosis", "authors": ["Franz Kafka"]}
|
||||
|
||||
assert await books_service.find_duplicate_books(incoming, test_library) == []
|
||||
assert await books_service.find_duplicate_book_groups(test_library) == []
|
||||
|
||||
async def test_an_import_reports_the_copy_it_just_made(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Different bytes, same book — created, and said so."""
|
||||
await books_service.create_many_from_files(
|
||||
BooksCreateFromFiles(files=[upload(EPUB)]), test_library
|
||||
)
|
||||
|
||||
result = await books_service.create_many_from_files(
|
||||
BooksCreateFromFiles(files=[edition(EPUB, "second.epub")]), test_library
|
||||
)
|
||||
|
||||
assert len(result.books) == 1
|
||||
assert result.duplicates == []
|
||||
assert len(result.possible_duplicates) == 1
|
||||
|
||||
possible = result.possible_duplicates[0]
|
||||
assert possible.book_id == result.books[0].id
|
||||
assert "title-author" in possible.candidates[0].matched_on
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
class TestDuplicateBookGroups:
|
||||
"""The pass over a library someone already has."""
|
||||
|
||||
async def test_groups_join_transitively(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""A and B by ISBN, B and C by title and author, all three in one group."""
|
||||
first = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Metamorphosis",
|
||||
authors=["Franz Kafka"],
|
||||
identifiers={"isbn-13": "9780486282114"},
|
||||
)
|
||||
second = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="The Trial",
|
||||
authors=["Franz Kafka"],
|
||||
identifiers={"isbn-10": "0486282112"},
|
||||
)
|
||||
third = await store_book(
|
||||
books_service, test_library, title="Trial", authors=["Kafka, Franz"]
|
||||
)
|
||||
|
||||
groups = await books_service.find_duplicate_book_groups(test_library)
|
||||
|
||||
assert len(groups) == 1
|
||||
assert [book.book_id for book in groups[0]] == [first.id, second.id, third.id]
|
||||
|
||||
reasons = {book.book_id: book.matched_on for book in groups[0]}
|
||||
assert reasons[first.id] == ["identifier"]
|
||||
assert reasons[second.id] == ["identifier", "title-author"]
|
||||
assert reasons[third.id] == ["title-author"]
|
||||
|
||||
async def test_a_lone_book_is_not_a_group(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
assert await books_service.find_duplicate_book_groups(test_library) == []
|
||||
|
||||
async def test_a_dismissed_pair_is_left_out(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Disagreeing with one pairing must not break a group standing on another."""
|
||||
first = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="Metamorphosis",
|
||||
authors=["Franz Kafka"],
|
||||
identifiers={"isbn-13": "9780486282114"},
|
||||
)
|
||||
second = await store_book(
|
||||
books_service,
|
||||
test_library,
|
||||
title="The Trial",
|
||||
authors=["Franz Kafka"],
|
||||
identifiers={"isbn-10": "0486282112"},
|
||||
)
|
||||
third = await store_book(
|
||||
books_service, test_library, title="Trial", authors=["Kafka, Franz"]
|
||||
)
|
||||
|
||||
await books_service.dismiss_duplicates(second.id, first.id)
|
||||
|
||||
groups = await books_service.find_duplicate_book_groups(test_library)
|
||||
assert [[book.book_id for book in group] for group in groups] == [
|
||||
[second.id, third.id]
|
||||
]
|
||||
|
||||
await books_service.restore_duplicates(first.id, second.id)
|
||||
|
||||
groups = await books_service.find_duplicate_book_groups(test_library)
|
||||
assert [[book.book_id for book in group] for group in groups] == [
|
||||
[first.id, second.id, third.id]
|
||||
]
|
||||
|
||||
async def test_a_book_cannot_be_dismissed_against_itself(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
stored = await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError):
|
||||
await books_service.dismiss_duplicates(stored.id, stored.id)
|
||||
|
||||
async def test_dismissing_an_unknown_book_is_refused(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
stored = await store_book(
|
||||
books_service, test_library, title="Metamorphosis", authors=["Franz Kafka"]
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError):
|
||||
await books_service.dismiss_duplicates(stored.id, stored.id + 10_000)
|
||||
|
||||
Reference in New Issue
Block a user