diff --git a/.env.prod-example b/.env.prod-example index e6b6b40..3ebe479 100644 --- a/.env.prod-example +++ b/.env.prod-example @@ -5,6 +5,15 @@ CHITAI_TOKEN_SECRET=secret CHITAI_DEFAULT_LIBRARY_NAME=Books CHITAI_DEFAULT_LIBRARY_PATH="libraries/books" +# Duplicate detection when importing files (optional). +# Scope: "library" compares against the library being uploaded to, "global" against +# every library, "off" disables the check. +CHITAI_DUPLICATE_SCOPE=library + +# Where the consume watcher parks files it refused as duplicates. Keep it outside +# CHITAI_CONSUME_PATH, or the watcher picks them straight back up. +CHITAI_DUPLICATE_PATH="duplicates" + # You probably should not change these CHITAI_API_URL="http://backend:8000" CHITAI_API_DEBUG=false diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 28d9246..8133b78 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -88,6 +88,14 @@ The backend owns files on disk, not just rows: - **Layout** — `services/filesystem_library.py` (`BookPathGenerator`) renders a Jinja2 template against book metadata to decide where a book lives under the library's `root_path` (default: `author/series/position - title/`). +- **One directory per book, never shared.** The generated path is a pure function of the metadata, + so two books with the same author and title produce the same one — two editions, or an + `allow_duplicates` copy. `BookService._reserve_book_path` moves the later one to `title (2)` + before anything is written, and `update_book` reserves the same way so a rename cannot move a + book in on top of another. This matters because `book.path` is what deletes, moves and file + lookups act on: books sharing a directory means one overwrites the other's files, and deleting + either takes both. `_unused_path` does the same job for filenames within a directory. + A book that already has a `path` keeps it — `add_files` must follow the book, not the template. - **Metadata extraction** — `services/metadata_extractor.py` reads EPUB (ebooklib) and PDF (pypdfium2) files; extracted values fill only *empty* fields on the incoming payload. - **Covers** — converted to WebP with a UUID filename under `settings.book_cover_path`, served by a @@ -100,6 +108,140 @@ The backend owns files on disk, not just rows: if it differs, moves the directory contents and prunes empty parents. Keep that in mind before changing metadata handling. +## Duplicate detection + +Every ingest path screens incoming files against what is already stored, keyed on +**`(hash, size)`** — never the hash alone, because it samples 12 KiB (see below) and +EPUBs from one toolchain often share their first window. `FileMetadata.hash` carries a +plain, deliberately **non-unique** index: a collision must not be able to fail an import, +and older databases may already hold duplicates. + +Scope comes from `CHITAI_DUPLICATE_SCOPE` (`library`, the default | `global` | `off`). + +The policy differs by how deliberate the import is: + +| Path | Behaviour | +| --- | --- | +| `create_many_from_files` (browser bulk) | Skip per file, skip a whole group whose files are all known, report everything skipped in `ImportResult.duplicates`. Re-dropping a folder to pick up what is new is the case this serves. | +| `create_book` (single, with metadata) | All-or-nothing: raises `DuplicateFilesError`, which `controllers/book.py` renders as a **409** carrying the refused files in `extra`. | +| `add_files` | A file the book already carries is a no-op; one stored under another book raises `DuplicateFilesError`. | +| `create_many_from_existing_files` (consume watcher) | Skips, and **moves the file to `CHITAI_DUPLICATE_PATH//`** — nothing is deleted, and it cannot stay put because `watchfiles` only reports additions. That path must stay outside `consume_path` or the watcher re-imports it and tries to read the directory name as a library slug. | + +`allow_duplicates=true` overrides all of it, on every endpoint. Keep that working — the +hash is not proof of identity, so a wrong verdict has to be recoverable, and a scripted +import needs a way through. The **web UI deliberately does not offer it**: storing the +same bytes twice splits reading progress and shelf membership across two records that +can never converge, which is nothing anyone wants on purpose. + +Three things to preserve when touching this code: + +- **A match only counts while the file is on disk.** `find_duplicate_files` stats each + candidate, and `add_files` writes a missing file back into the row that already + describes it (`_restore_file`) instead of adding a second row beside it. The hash + lives in the database and the file does not, so without this a file deleted behind + the app's back would go on refusing its own replacement. + +- **Screening runs before anything is written.** `fingerprint_upload` reads the spooled + upload and rewinds it; the resulting fingerprints are handed to `_save_book_files`, + which skips its own `StreamingHasher` when it already has the answer. Passing them + through is what keeps the file from being read twice. +- **`_screen_for_duplicates` extends the `known` dict as it goes**, so the same bytes + submitted twice in one request are caught. Those duplicates report `book_id: None` — + there is no row to point at yet. + +`POST /books/duplicate-files` answers the same question from fingerprints alone, for +clients that want to ask before uploading anything. + +### Book-level detection — a different question + +The file check answers "are these the same bytes?". `find_duplicate_books` answers "is +this the same book?", which a re-scan, a re-zipped EPUB or another edition cannot be +asked with a hash. Two signals, either sufficient: a **shared identifier**, or a +**matching normalized title with at least one shared author**. + +It **never blocks**. A metadata match is a guess — a work shares title and author with +its own translation, its own second edition and its own audiobook — so the book is +created and the candidates are reported alongside it in +`ImportResult.possible_duplicates`. File-level dedupe keeps its refuse/skip behaviour; +that one is near-certain and this one is not. Do not "improve" this into a refusal. + +Two rules narrow it, both applied in Python over the small candidate set: + +- **A shared author is required for a title match.** Without it every book the + extractors gave up on and titled `Unknown` is a duplicate of every other one. A book + with no authors can therefore only match on an identifier. +- **The same series at a different `series_position` disqualifies a match.** A trilogy + shares an author and often most of its title; the position is the library saying + outright that these are two books. + +A book is compared under **several title keys, not one** (`_title_keys`). Ebook files +are overwhelmingly named `Title - Author.epub`, and wherever nothing inside the file +overrode that name the author ended up in the title column — so one copy is stored as +`Building Microservices` and another as `Building Microservices Sam Newman`. Both +directions are generated, the author stripped off and the author added on, which is why +the query is `normalized_title.in_(keys)` rather than `==`. This is still exact matching +on an indexed column: no similarity score, nothing to tune. It does not weaken the +shared-author requirement, which is a separate condition. + +`find_duplicate_book_groups` is the library-wide pass behind +`GET /books/duplicate-books`, since the import-time check says nothing about a +collection someone already has. It buckets books by every key they carry and merges the +buckets with union-find, so A~B by ISBN and B~C by title land in one group. Pairs in +`duplicate_dismissals` are never merged — a reader disagreeing with one pairing must not +silently break a group that stands on other evidence. + +### Author names have one stored form + +`Author.name` is always the canonical form, produced by `format_author_name`. Extractors +hand over whatever the file said — `Newman, Sam;` from a `DC:creator` list, `Sam Newman` +from a PDF, `Sam Newman.epub` from a filename — and storing those verbatim is how one +person becomes four rows in the sidebar, four entries in the author filter, and four +books that never look like each other. + +This is **display** canonicalization, distinct from `normalize_author`, which throws +away case, accents and spacing to build a comparison key nobody sees. Tidying only +removes what an extractor added: a trailing separator, a file extension, and the +`Surname, Given` ordering. It never touches case or accents — `Michał Płachta` and +`Steve McConnell` are the author's own spelling, not something to correct. + +Three places have to agree, and `Author` keeps them together: the `@validates("name")` +hook, `unique_hash`, and `unique_filter`. `as_unique_async` looks a row up with the +filter and then constructs with the validator, so if the lookup used the raw name and +the insert used the tidy one, every variant spelling would miss the existing row and +then collide with it on the unique index. + +A form the rule does not recognise is **left exactly as it was found** — `Dave Thomas, +Andy Hunt` is two people in one string, and flipping it would invent a third. Leaving a +mess visible beats rewriting it wrongly. + +### The normalized columns are written by validators + +`Book.normalized_title`, `Author.normalized_name` and `Identifier.normalized_value` are +derived from `services/matching.py` and kept current by **SQLAlchemy `@validates` hooks +on the models**, not by any service. Assigning them directly is always wrong. + +This is deliberate and it is invisible at the call sites: `BookService` writes titles +through at least three paths (`to_model_on_create`, `to_model_on_update`, and the +`setattr` loop in `_populate_with_unique_relationships`), and a validator is the only +thing a fourth cannot bypass. The columns carry plain, **non-unique** btree indexes — +two spellings collapsing onto one value is the entire point. + +`services/matching.py` is imported *inside* those validators rather than at module +scope: reaching it initialises the `chitai.services` package, which imports the +services, which import the models. Keep the local import. + +The validators only fire on write, so a migration that adds one of these columns must +backfill existing rows through the same helpers — see the `data_upgrades()` hook in +`2026-08-15_add_book_matching_keys_and_duplicate__4358e7d4743a.py`. + +**Changing anything in `services/matching.py` needs a revision that recomputes them.** +The keys are derived and already written, so a normalization change silently invalidates +every stored row: a book written under the old rules just stops matching one written +under the new rules, with nothing to show that anything is wrong. Copy +`2026-08-15_recompute_book_matching_keys_ed41acf21270.py`, which exists because +`normalize_title` learned to strip compact edition markers (`2E`, `5e`). It is +idempotent and safe to re-run. + ## KOReader hashing `services/utils.py` reimplements KOReader's partial-MD5 document identifier: 1 KiB samples at diff --git a/backend/migrations/versions/2026-08-13_add_file_hash_index_e9c2c7e875ae.py b/backend/migrations/versions/2026-08-13_add_file_hash_index_e9c2c7e875ae.py new file mode 100644 index 0000000..82d81cd --- /dev/null +++ b/backend/migrations/versions/2026-08-13_add_file_hash_index_e9c2c7e875ae.py @@ -0,0 +1,80 @@ +"""add file hash index + +Revision ID: e9c2c7e875ae +Revises: 6d72d1bbc0ee +Create Date: 2026-08-13 14:52:09.341906 + +""" + +import warnings +from typing import TYPE_CHECKING + +import sqlalchemy as sa +from alembic import op +from advanced_alchemy.types import EncryptedString, EncryptedText, GUID, ORA_JSONB, DateTimeUTC, StoredObject, PasswordHash, FernetBackend +from advanced_alchemy.types.encrypted_string import PGCryptoBackend +from advanced_alchemy.types.password_hash.argon2 import Argon2Hasher +from advanced_alchemy.types.password_hash.passlib import PasslibHasher +from advanced_alchemy.types.password_hash.pwdlib import PwdlibHasher +from sqlalchemy import Text # noqa: F401 + +if TYPE_CHECKING: + from collections.abc import Sequence + +__all__ = ["downgrade", "upgrade", "schema_upgrades", "schema_downgrades", "data_upgrades", "data_downgrades"] + +sa.GUID = GUID +sa.DateTimeUTC = DateTimeUTC +sa.ORA_JSONB = ORA_JSONB +sa.EncryptedString = EncryptedString +sa.EncryptedText = EncryptedText +sa.StoredObject = StoredObject +sa.PasswordHash = PasswordHash +sa.Argon2Hasher = Argon2Hasher +sa.PasslibHasher = PasslibHasher +sa.PwdlibHasher = PwdlibHasher +sa.FernetBackend = FernetBackend +sa.PGCryptoBackend = PGCryptoBackend + +# revision identifiers, used by Alembic. +revision = 'e9c2c7e875ae' +down_revision = '6d72d1bbc0ee' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + schema_upgrades() + data_upgrades() + +def downgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + data_downgrades() + schema_downgrades() + +def schema_upgrades() -> None: + """schema upgrade migrations go here.""" + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('file_metadata', schema=None) as batch_op: + batch_op.create_index('ix_file_metadata_hash', ['hash'], unique=False) + + # ### end Alembic commands ### + +def schema_downgrades() -> None: + """schema downgrade migrations go here.""" + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('file_metadata', schema=None) as batch_op: + batch_op.drop_index('ix_file_metadata_hash') + + # ### end Alembic commands ### + +def data_upgrades() -> None: + """Add any optional data upgrade migrations here!""" + +def data_downgrades() -> None: + """Add any optional data downgrade migrations here!""" diff --git a/backend/migrations/versions/2026-08-15_add_book_matching_keys_and_duplicate__4358e7d4743a.py b/backend/migrations/versions/2026-08-15_add_book_matching_keys_and_duplicate__4358e7d4743a.py new file mode 100644 index 0000000..e8d5443 --- /dev/null +++ b/backend/migrations/versions/2026-08-15_add_book_matching_keys_and_duplicate__4358e7d4743a.py @@ -0,0 +1,163 @@ +"""add book matching keys and duplicate dismissals + +Revision ID: 4358e7d4743a +Revises: e9c2c7e875ae +Create Date: 2026-08-15 15:09:02.708914 + +""" + +import warnings + +import sqlalchemy as sa +from alembic import op +from advanced_alchemy.types import EncryptedString, EncryptedText, GUID, ORA_JSONB, DateTimeUTC, StoredObject, PasswordHash, FernetBackend +from advanced_alchemy.types.encrypted_string import PGCryptoBackend +from advanced_alchemy.types.password_hash.argon2 import Argon2Hasher +from advanced_alchemy.types.password_hash.passlib import PasslibHasher +from advanced_alchemy.types.password_hash.pwdlib import PwdlibHasher +from sqlalchemy import Text # noqa: F401 + +__all__ = ["downgrade", "upgrade", "schema_upgrades", "schema_downgrades", "data_upgrades", "data_downgrades"] + +sa.GUID = GUID +sa.DateTimeUTC = DateTimeUTC +sa.ORA_JSONB = ORA_JSONB +sa.EncryptedString = EncryptedString +sa.EncryptedText = EncryptedText +sa.StoredObject = StoredObject +sa.PasswordHash = PasswordHash +sa.Argon2Hasher = Argon2Hasher +sa.PasslibHasher = PasslibHasher +sa.PwdlibHasher = PwdlibHasher +sa.FernetBackend = FernetBackend +sa.PGCryptoBackend = PGCryptoBackend + +# revision identifiers, used by Alembic. +revision = '4358e7d4743a' +down_revision = 'e9c2c7e875ae' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + schema_upgrades() + data_upgrades() + +def downgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + data_downgrades() + schema_downgrades() + +def schema_upgrades() -> None: + """schema upgrade migrations go here.""" + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('duplicate_dismissals', + sa.Column('id', sa.BigInteger().with_variant(sa.Integer(), 'sqlite'), nullable=False), + sa.Column('book_a_id', sa.BigInteger().with_variant(sa.Integer(), 'sqlite'), nullable=False), + sa.Column('book_b_id', sa.BigInteger().with_variant(sa.Integer(), 'sqlite'), nullable=False), + sa.ForeignKeyConstraint(['book_a_id'], ['books.id'], name=op.f('fk_duplicate_dismissals_book_a_id_books'), ondelete='cascade'), + sa.ForeignKeyConstraint(['book_b_id'], ['books.id'], name=op.f('fk_duplicate_dismissals_book_b_id_books'), ondelete='cascade'), + sa.PrimaryKeyConstraint('id', name=op.f('pk_duplicate_dismissals')), + sa.UniqueConstraint('book_a_id', 'book_b_id', name=op.f('uq_duplicate_dismissals_book_a_id')) + ) + with op.batch_alter_table('duplicate_dismissals', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_duplicate_dismissals_book_a_id'), ['book_a_id'], unique=False) + batch_op.create_index(batch_op.f('ix_duplicate_dismissals_book_b_id'), ['book_b_id'], unique=False) + + # `server_default` so the column can be added to a table that already has rows; + # `data_upgrades` fills in the real keys immediately afterwards. + with op.batch_alter_table('authors', schema=None) as batch_op: + batch_op.add_column(sa.Column('normalized_name', sa.String(), nullable=False, server_default='')) + batch_op.create_index(batch_op.f('ix_authors_normalized_name'), ['normalized_name'], unique=False) + + with op.batch_alter_table('books', schema=None) as batch_op: + batch_op.add_column(sa.Column('normalized_title', sa.String(), nullable=False, server_default='')) + batch_op.create_index(batch_op.f('ix_books_normalized_title'), ['normalized_title'], unique=False) + + with op.batch_alter_table('identifiers', schema=None) as batch_op: + batch_op.add_column(sa.Column('normalized_value', sa.String(), nullable=True)) + batch_op.create_index(batch_op.f('ix_identifiers_normalized_value'), ['normalized_value'], unique=False) + + # ### end Alembic commands ### + +def schema_downgrades() -> None: + """schema downgrade migrations go here.""" + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('identifiers', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('ix_identifiers_normalized_value')) + batch_op.drop_column('normalized_value') + + with op.batch_alter_table('books', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('ix_books_normalized_title')) + batch_op.drop_column('normalized_title') + + with op.batch_alter_table('authors', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('ix_authors_normalized_name')) + batch_op.drop_column('normalized_name') + + with op.batch_alter_table('duplicate_dismissals', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('ix_duplicate_dismissals_book_b_id')) + batch_op.drop_index(batch_op.f('ix_duplicate_dismissals_book_a_id')) + + op.drop_table('duplicate_dismissals') + # ### end Alembic commands ### + +def data_upgrades() -> None: + """ + Fill the matching keys in for rows that already exist. + + The validators on the models only fire when something is written, so without this + every book imported before today is invisible to duplicate detection. Run through + the same helpers the validators use, so a backfilled row and a freshly written one + are guaranteed to agree. + """ + from chitai.services.matching import ( + normalize_author, + normalize_identifier, + normalize_title, + ) + + connection = op.get_bind() + + books = connection.execute(sa.text("SELECT id, title FROM books")).fetchall() + _apply( + connection, + "UPDATE books SET normalized_title = :key WHERE id = :id", + [{"id": id, "key": normalize_title(title)} for id, title in books], + ) + + authors = connection.execute(sa.text("SELECT id, name FROM authors")).fetchall() + _apply( + connection, + "UPDATE authors SET normalized_name = :key WHERE id = :id", + [{"id": id, "key": normalize_author(name)} for id, name in authors], + ) + + identifiers = connection.execute( + sa.text("SELECT id, name, value FROM identifiers") + ).fetchall() + _apply( + connection, + "UPDATE identifiers SET normalized_value = :key WHERE id = :id", + [ + {"id": id, "key": normalize_identifier(name, value)} + for id, name, value in identifiers + ], + ) + + +def _apply(connection, statement: str, parameters: list[dict]) -> None: + """Run one update per row, in batches, skipping the work when there are none.""" + batch_size = 1000 + + for start in range(0, len(parameters), batch_size): + connection.execute(sa.text(statement), parameters[start : start + batch_size]) + + +def data_downgrades() -> None: + """Add any optional data downgrade migrations here!""" diff --git a/backend/migrations/versions/2026-08-15_canonicalize_author_names_49a9e85a0ffc.py b/backend/migrations/versions/2026-08-15_canonicalize_author_names_49a9e85a0ffc.py new file mode 100644 index 0000000..bf81633 --- /dev/null +++ b/backend/migrations/versions/2026-08-15_canonicalize_author_names_49a9e85a0ffc.py @@ -0,0 +1,130 @@ +"""canonicalize author names + +Revision ID: 49a9e85a0ffc +Revises: ed41acf21270 +Create Date: 2026-08-15 15:59:47.331545 + +""" + +import warnings + +import sqlalchemy as sa +from alembic import op +from advanced_alchemy.types import EncryptedString, EncryptedText, GUID, ORA_JSONB, DateTimeUTC, StoredObject, PasswordHash, FernetBackend +from advanced_alchemy.types.encrypted_string import PGCryptoBackend +from advanced_alchemy.types.password_hash.argon2 import Argon2Hasher +from advanced_alchemy.types.password_hash.passlib import PasslibHasher +from advanced_alchemy.types.password_hash.pwdlib import PwdlibHasher +from sqlalchemy import Text # noqa: F401 + +__all__ = ["downgrade", "upgrade", "schema_upgrades", "schema_downgrades", "data_upgrades", "data_downgrades"] + +sa.GUID = GUID +sa.DateTimeUTC = DateTimeUTC +sa.ORA_JSONB = ORA_JSONB +sa.EncryptedString = EncryptedString +sa.EncryptedText = EncryptedText +sa.StoredObject = StoredObject +sa.PasswordHash = PasswordHash +sa.Argon2Hasher = Argon2Hasher +sa.PasslibHasher = PasslibHasher +sa.PwdlibHasher = PwdlibHasher +sa.FernetBackend = FernetBackend +sa.PGCryptoBackend = PGCryptoBackend + +# revision identifiers, used by Alembic. +revision = '49a9e85a0ffc' +down_revision = 'ed41acf21270' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + schema_upgrades() + data_upgrades() + +def downgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + data_downgrades() + schema_downgrades() + +def schema_upgrades() -> None: + """schema upgrade migrations go here.""" + pass + +def schema_downgrades() -> None: + """schema downgrade migrations go here.""" + pass + +def data_upgrades() -> None: + """ + Rewrite every author into the canonical form, merging the rows that collide. + + `Author.name` is only now guaranteed tidy — until this revision extractors wrote + whatever the file said, so one person could hold several rows: "Sam Newman" beside + "Newman, Sam;" beside "Sam Newman.epub" (the last from a filename whose extension + was never stripped). Each showed up as its own author in the sidebar and its own + filter, and no amount of fixing the extractors repairs a row already written. + + Rows that canonicalize onto one name are merged into the lowest id, which keeps + whichever row the library has been referring to longest. `book.path` is stored, not + derived, so renaming an author moves nothing on disk. + """ + from chitai.services.matching import format_author_name, normalize_author + + connection = op.get_bind() + authors = connection.execute(sa.text("SELECT id, name FROM authors")).fetchall() + + groups: dict[str, list[int]] = {} + for id, name in sorted(authors): + # A name with nothing left of it after tidying is left exactly as it was: + # merging those together would invent one author out of several unrelated + # broken rows, which is worse than leaving the mess visible. + if canonical := format_author_name(name): + groups.setdefault(canonical, []).append(id) + + for canonical, ids in groups.items(): + winner, losers = ids[0], ids[1:] + + for loser in losers: + # A book credited to both rows would otherwise breach the + # (book_id, author_id) unique constraint the moment the link is repointed. + connection.execute( + sa.text( + "DELETE FROM book_author_links WHERE author_id = :loser AND book_id IN" + " (SELECT book_id FROM book_author_links WHERE author_id = :winner)" + ), + {"loser": loser, "winner": winner}, + ) + connection.execute( + sa.text( + "UPDATE book_author_links SET author_id = :winner" + " WHERE author_id = :loser" + ), + {"loser": loser, "winner": winner}, + ) + connection.execute( + sa.text("DELETE FROM authors WHERE id = :loser"), {"loser": loser} + ) + + # Only after the losers are gone, or this collides with the unique index. + connection.execute( + sa.text( + "UPDATE authors SET name = :name, normalized_name = :key WHERE id = :id" + ), + {"id": winner, "name": canonical, "key": normalize_author(canonical)}, + ) + + +def data_downgrades() -> None: + """ + Nothing to undo. + + The rows a merge removed are gone, and the spellings it replaced were never + recorded anywhere else — there is nothing to restore them from. + """ diff --git a/backend/migrations/versions/2026-08-15_recompute_book_matching_keys_ed41acf21270.py b/backend/migrations/versions/2026-08-15_recompute_book_matching_keys_ed41acf21270.py new file mode 100644 index 0000000..1b0f6fd --- /dev/null +++ b/backend/migrations/versions/2026-08-15_recompute_book_matching_keys_ed41acf21270.py @@ -0,0 +1,122 @@ +"""recompute book matching keys + +Revision ID: ed41acf21270 +Revises: 4358e7d4743a +Create Date: 2026-08-15 15:44:28.341020 + +""" + +import warnings + +import sqlalchemy as sa +from alembic import op +from advanced_alchemy.types import EncryptedString, EncryptedText, GUID, ORA_JSONB, DateTimeUTC, StoredObject, PasswordHash, FernetBackend +from advanced_alchemy.types.encrypted_string import PGCryptoBackend +from advanced_alchemy.types.password_hash.argon2 import Argon2Hasher +from advanced_alchemy.types.password_hash.passlib import PasslibHasher +from advanced_alchemy.types.password_hash.pwdlib import PwdlibHasher +from sqlalchemy import Text # noqa: F401 + +__all__ = ["downgrade", "upgrade", "schema_upgrades", "schema_downgrades", "data_upgrades", "data_downgrades"] + +sa.GUID = GUID +sa.DateTimeUTC = DateTimeUTC +sa.ORA_JSONB = ORA_JSONB +sa.EncryptedString = EncryptedString +sa.EncryptedText = EncryptedText +sa.StoredObject = StoredObject +sa.PasswordHash = PasswordHash +sa.Argon2Hasher = Argon2Hasher +sa.PasslibHasher = PasslibHasher +sa.PwdlibHasher = PwdlibHasher +sa.FernetBackend = FernetBackend +sa.PGCryptoBackend = PGCryptoBackend + +# revision identifiers, used by Alembic. +revision = 'ed41acf21270' +down_revision = '4358e7d4743a' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + schema_upgrades() + data_upgrades() + +def downgrade() -> None: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UserWarning) + with op.get_context().autocommit_block(): + data_downgrades() + schema_downgrades() + +def schema_upgrades() -> None: + """schema upgrade migrations go here.""" + pass + +def schema_downgrades() -> None: + """schema downgrade migrations go here.""" + pass + +def data_upgrades() -> None: + """ + Recompute every matching key against the current normalization. + + The keys are derived, so changing a helper in `services/matching.py` silently + invalidates every row already written — a book stored under the old rules simply + stops matching one stored under the new ones, with nothing to show that anything + is wrong. `normalize_title` learned to strip the compact edition markers a cover + actually carries ("2E", "5e"), which moved `Building Microservices, 2E` onto the + same key as `Building Microservices`. + + Any later change to those helpers wants a revision that looks exactly like this + one. It is idempotent and safe to re-run. + """ + from chitai.services.matching import ( + normalize_author, + normalize_identifier, + normalize_title, + ) + + connection = op.get_bind() + + books = connection.execute(sa.text("SELECT id, title FROM books")).fetchall() + _apply( + connection, + "UPDATE books SET normalized_title = :key WHERE id = :id", + [{"id": id, "key": normalize_title(title)} for id, title in books], + ) + + authors = connection.execute(sa.text("SELECT id, name FROM authors")).fetchall() + _apply( + connection, + "UPDATE authors SET normalized_name = :key WHERE id = :id", + [{"id": id, "key": normalize_author(name)} for id, name in authors], + ) + + identifiers = connection.execute( + sa.text("SELECT id, name, value FROM identifiers") + ).fetchall() + _apply( + connection, + "UPDATE identifiers SET normalized_value = :key WHERE id = :id", + [ + {"id": id, "key": normalize_identifier(name, value)} + for id, name, value in identifiers + ], + ) + + +def _apply(connection, statement: str, parameters: list[dict]) -> None: + """Run one update per row, in batches, skipping the work when there are none.""" + batch_size = 1000 + + for start in range(0, len(parameters), batch_size): + connection.execute(sa.text(statement), parameters[start : start + batch_size]) + + +def data_downgrades() -> None: + """Add any optional data downgrade migrations here!""" diff --git a/backend/src/chitai/config.py b/backend/src/chitai/config.py index 69d6723..c922cd6 100644 --- a/backend/src/chitai/config.py +++ b/backend/src/chitai/config.py @@ -1,9 +1,25 @@ +from enum import StrEnum + from pydantic import Field, PostgresDsn, computed_field from pydantic_settings import BaseSettings, SettingsConfigDict from advanced_alchemy.extensions.litestar import ( SQLAlchemyAsyncConfig, ) + +class DuplicateScope(StrEnum): + """How widely an incoming file is compared against what is already stored.""" + + LIBRARY = "library" + """Only files in the library being uploaded to count as duplicates.""" + + GLOBAL = "global" + """A file already held by any library counts as a duplicate.""" + + OFF = "off" + """No duplicate detection at all.""" + + class Settings(BaseSettings): version: str = Field("0.0.1") project_name: str = Field("chitai") @@ -33,6 +49,14 @@ class Settings(BaseSettings): # Path to consume directory consume_path: str = Field("./consume") + # Duplicate detection + duplicate_scope: DuplicateScope = Field(DuplicateScope.LIBRARY) + + # Where the consume watcher parks files it refused as duplicates. Must sit + # outside `consume_path`, or the watcher picks them straight back up and + # tries to resolve the directory name as a library slug. + duplicate_path: str = Field("./duplicates") + @computed_field @property def postgres_uri(self) -> PostgresDsn: diff --git a/backend/src/chitai/controllers/book.py b/backend/src/chitai/controllers/book.py index 12bc96f..a2ebc71 100644 --- a/backend/src/chitai/controllers/book.py +++ b/backend/src/chitai/controllers/book.py @@ -12,7 +12,12 @@ from litestar.params import Dependency, Body from litestar.enums import RequestEncodingType from litestar.response import File, Stream from litestar.exceptions import HTTPException -from litestar.status_codes import HTTP_400_BAD_REQUEST +from litestar.status_codes import ( + HTTP_200_OK, + HTTP_204_NO_CONTENT, + HTTP_400_BAD_REQUEST, + HTTP_409_CONFLICT, +) from litestar.datastructures import UploadFile from advanced_alchemy.service.pagination import OffsetPagination from advanced_alchemy.filters import CollectionFilter @@ -23,6 +28,24 @@ from chitai.services import dependencies as deps from chitai import schemas as s from chitai.database import models as m from chitai.services import BookService, BookProgressService +from chitai.services.book import DuplicateFilesError + + +def _duplicate_conflict(exc: DuplicateFilesError) -> HTTPException: + """ + Turn refused files into a 409 the caller can act on. + + The files ride along in `extra` so the client can name them and offer to send them + again with `allow_duplicates`, rather than being told only that something clashed. + """ + return HTTPException( + status_code=HTTP_409_CONFLICT, + detail="These files are already in the library", + extra=[ + s.DuplicateFileRead.model_validate(duplicate).model_dump(mode="json") + for duplicate in exc.duplicates + ], + ) class BookController(Controller): @@ -63,6 +86,7 @@ class BookController(Controller): books_service: BookService, library: m.Library, data: Annotated[s.BookCreate, Body(media_type=RequestEncodingType.MULTI_PART)], + allow_duplicates: bool = False, ) -> s.BookRead: """ Create a new book with metadata and files. @@ -73,6 +97,10 @@ class BookController(Controller): Path Parameters: library_id: The ID of the library the book belongs to. + Query Parameters: + allow_duplicates: If True, store the files even if the library already + holds them. + Request Body: data: Book creation data including metadata and files. @@ -83,9 +111,17 @@ class BookController(Controller): Returns: The created book as a BookRead schema. + Raises: + HTTPException: 409 if any of the files is already in the library. """ - result = await books_service.create_book(data, library) + try: + result = await books_service.create_book( + data, library, screen_duplicates=not allow_duplicates + ) + except DuplicateFilesError as exc: + raise _duplicate_conflict(exc) + book = await books_service.get(result.id) return books_service.to_schema(book, schema_type=s.BookRead) @@ -97,13 +133,20 @@ class BookController(Controller): data: Annotated[ s.BooksCreateFromFiles, Body(media_type=RequestEncodingType.MULTI_PART) ], - ) -> OffsetPagination[s.BookRead]: + allow_duplicates: bool = False, + ) -> s.BooksUploadResult: """ Create multiple books from uploaded files. Groups files by directory and creates separate books for each group. Metadata is automatically extracted from the files. + Files the library already holds are skipped rather than refused, and reported + back so the caller can say which ones did not make it in and why. + + Query Parameters: + allow_duplicates: If True, store every file, even one already held. + Request Body: data: Container with list of uploaded files. @@ -112,19 +155,200 @@ class BookController(Controller): library: The library the books belong to. Returns: - Paginated list of created books. + The books created, and the files skipped as duplicates. """ try: - results = await books_service.create_many_from_files(data, library) + result = await books_service.create_many_from_files( + data, library, allow_duplicates=allow_duplicates + ) except ValueError: raise HTTPException( status_code=HTTP_400_BAD_REQUEST, detail="Must upload at least one file" ) - books = await books_service.list( - CollectionFilter("id", [result.id for result in results]) + books = ( + await books_service.list( + CollectionFilter("id", [book.id for book in result.books]) + ) + if result.books + else [] ) - return books_service.to_schema(books, schema_type=s.BookRead) + + return s.BooksUploadResult( + created=[ + books_service.to_schema(book, schema_type=s.BookRead) for book in books + ], + skipped=[ + s.DuplicateFileRead.model_validate(duplicate) + for duplicate in result.duplicates + ], + possible_duplicates=[ + s.PossibleDuplicateRead.model_validate(possible) + for possible in result.possible_duplicates + ], + ) + + @get(path="duplicate-books") + async def list_duplicate_books( + self, books_service: BookService, library: m.Library + ) -> list[s.DuplicateBookGroupRead]: + """ + Report books already in the library that look like copies of one another. + + The import-time check only ever sees what is arriving, so this is what covers + a collection someone already has. Matching is on metadata and therefore a + guess: a group is a question for the reader, not a verdict. + + Query Parameters: + library_id: The library to review. + + Injected Dependencies: + books_service: The book service for database operations. + library: The library to review. + + Returns: + One entry per group of two or more books. Empty when there is nothing to + review, or when duplicate detection is switched off. + """ + groups = await books_service.find_duplicate_book_groups(library) + + return [ + s.DuplicateBookGroupRead( + books=[s.DuplicateBookRead.model_validate(book) for book in group] + ) + 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 + ) -> None: + """ + Record that two books are not the same book. + + Without this the review screen proposes the same wrong pair forever, which is + how a reader learns to stop looking at it. + + Request Body: + data: The two book IDs. Order does not matter. + + Injected Dependencies: + books_service: The book service for database operations. + + Raises: + HTTPException: 400 if the two IDs are the same or either book is unknown. + """ + try: + await books_service.dismiss_duplicates(data.book_a_id, data.book_b_id) + except ValueError as exc: + raise HTTPException(status_code=HTTP_400_BAD_REQUEST, detail=str(exc)) + + @delete(path="duplicate-books/dismissals") + async def restore_duplicate_books( + self, books_service: BookService, book_a_id: int, book_b_id: int + ) -> None: + """ + Undo a dismissal, so the pair is proposed again. + + Query Parameters: + book_a_id: One of the two books. + book_b_id: The other. Order does not matter. + + Injected Dependencies: + books_service: The book service for database operations. + """ + await books_service.restore_duplicates(book_a_id, book_b_id) + + # A question, not a change: 200 rather than the 201 a POST would default to. + @post(path="duplicate-files", status_code=HTTP_200_OK) + async def check_duplicate_files( + self, + books_service: BookService, + library: m.Library, + data: list[s.FileFingerprint], + ) -> list[s.DuplicateFileRead]: + """ + Report which of the given files the library already holds. + + Lets a client ask before it uploads anything, which is the difference between + re-sending a folder of books and re-sending twelve kilobytes of hashes. + + Query Parameters: + library_id: The library to check against. + + Request Body: + data: Hash and size for each file, optionally with the name to echo back. + + Injected Dependencies: + books_service: The book service for database operations. + library: The library to check against. + + Returns: + One entry per submitted file that is already stored. Files that are not + are absent. + """ + matches = await books_service.find_duplicate_files( + ((item.hash, item.size) for item in data), library + ) + + return [ + s.DuplicateFileRead( + filename=item.filename or match.filename, + hash=match.hash, + size=match.size, + library_id=match.library_id, + book_id=match.book_id, + book_title=match.book_title, + ) + for item in data + if (match := matches.get((item.hash, item.size))) is not None + ] @get(path="/{book_id:int}") async def get_book_by_id( @@ -303,13 +527,20 @@ class BookController(Controller): ], library: m.Library, books_service: BookService, + allow_duplicates: bool = False, ) -> s.BookRead: """ Add files to an existing book. + A file the book already carries is ignored, so re-sending one is harmless. + Path Parameters: book_id: The ID of the book to modify + Query Parameters: + allow_duplicates: If True, store the files even if the library already + holds them. + Request Body: files: The files to add to the book @@ -320,9 +551,17 @@ class BookController(Controller): Returns: The modified book + Raises: + HTTPException: 409 if a file is already stored under a different book. """ - await books_service.add_files(book_id, data, library) + try: + await books_service.add_files( + book_id, data, library, allow_duplicates=allow_duplicates + ) + except DuplicateFilesError as exc: + raise _duplicate_conflict(exc) + book = await books_service.get(book_id) return books_service.to_schema(book, schema_type=s.BookRead) diff --git a/backend/src/chitai/database/models/__init__.py b/backend/src/chitai/database/models/__init__.py index 16df64f..01a568a 100644 --- a/backend/src/chitai/database/models/__init__.py +++ b/backend/src/chitai/database/models/__init__.py @@ -3,6 +3,7 @@ from .book import Book, Identifier, FileMetadata from .book_list import BookList, BookListLink from .book_progress import BookProgress from .book_series import BookSeries +from .duplicate_dismissal import DuplicateDismissal from .kosync_device import KosyncDevice from .kosync_progress import KosyncProgress from .library import Library diff --git a/backend/src/chitai/database/models/author.py b/backend/src/chitai/database/models/author.py index b47fe4a..4a2981c 100644 --- a/backend/src/chitai/database/models/author.py +++ b/backend/src/chitai/database/models/author.py @@ -5,6 +5,7 @@ from sqlalchemy import ColumnElement, ForeignKey, UniqueConstraint from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column from sqlalchemy.orm import relationship +from sqlalchemy.orm import validates from advanced_alchemy.base import BigIntAuditBase, BigIntBase from advanced_alchemy.mixins import UniqueMixin @@ -16,18 +17,55 @@ if TYPE_CHECKING: class Author(BigIntAuditBase, UniqueMixin): __tablename__ = "authors" + # Always the canonical form — see `_canonicalize`. Extractors hand over whatever + # the file happened to say: "Newman, Sam;" from a `DC:creator` list, or + # "Sam Newman.epub" from a filename. Storing those verbatim is how one person ends + # up as several rows in the sidebar. name: Mapped[str] = mapped_column(unique=True, index=True) + + # Kept current by `_canonicalize` too — never assign it directly. Not unique: two + # spellings that survive canonicalization, "Steve Mcconnell" and "Steve McConnell", + # are still one person to a reader, which is what this column exists to express. + normalized_name: Mapped[str] = mapped_column(default="", index=True) + description: Mapped[Optional[str]] + @validates("name") + def _canonicalize(self, _key: str, name: str) -> str: + """ + Store the tidied name, and derive the matching key from it. + + A validator so no write can get around it, and `unique_hash` / `unique_filter` + below tidy the same way so `as_unique_async` looks the row up under the name it + would actually be stored as. All three have to agree: if the lookup used the + raw name and the insert used the tidy one, every variant spelling would miss + the existing row and then collide with it on the unique index. + """ + # Imported here rather than at module scope: `chitai.services.matching` cannot + # be reached without initialising the `chitai.services` package, which imports + # the services, which import this module. + from chitai.services.matching import format_author_name, normalize_author + + name = format_author_name(name) + self.normalized_name = normalize_author(name) + return name + + @classmethod + def _tidy(cls, name: str) -> str: + """The name as `_canonicalize` would store it.""" + from chitai.services.matching import format_author_name + + return format_author_name(name) + @classmethod def unique_hash(cls, name: str) -> Hashable: """Generate a unique hash for deduplication.""" - return name + return cls._tidy(name) @classmethod def unique_filter(cls, name: str) -> ColumnElement[bool]: """SQL filter for finding existing records.""" - return cls.name == name + return cls.name == cls._tidy(name) def __repr__(self) -> str: return f"Author({self.name!r})" diff --git a/backend/src/chitai/database/models/book.py b/backend/src/chitai/database/models/book.py index e647e70..0d61d93 100644 --- a/backend/src/chitai/database/models/book.py +++ b/backend/src/chitai/database/models/book.py @@ -2,13 +2,10 @@ from datetime import date from typing import TYPE_CHECKING, Any, Optional from sqlalchemy import Index, ForeignKey, UniqueConstraint -from sqlalchemy.orm import Mapped, mapped_column, relationship -from sqlalchemy.orm import mapped_column -from sqlalchemy.orm import relationship +from sqlalchemy.orm import Mapped, mapped_column, relationship, validates from sqlalchemy.ext.orderinglist import ordering_list from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.associationproxy import AssociationProxy -from sqlalchemy.orm.collections import attribute_keyed_dict from advanced_alchemy.base import BigIntAuditBase, BigIntBase @@ -44,6 +41,13 @@ class Book(BigIntAuditBase): library: Mapped["Library"] = relationship(back_populates="books") title: Mapped[str] + + # Kept current by `_normalize_title` below — never assign it directly. + # + # Deliberately not unique: two spellings collapsing onto one value is the whole + # point of the column, and a second edition is allowed to exist. + normalized_title: Mapped[str] = mapped_column(default="", index=True) + subtitle: Mapped[Optional[str]] description: Mapped[Optional[str]] published_date: Mapped[Optional[date]] @@ -111,6 +115,24 @@ class Book(BigIntAuditBase): def progress(self) -> Optional["BookProgress"]: return self.progress_records[0] if self.progress_records else None + @validates("title") + def _normalize_title(self, _key: str, title: str) -> str: + """ + Derive `normalized_title` from whatever writes the title. + + A validator rather than a service call because `BookService` sets titles from + at least three places — `to_model_on_create`, `to_model_on_update` and the + `setattr` loop in `_populate_with_unique_relationships` — and a fourth would + otherwise leave the key silently stale. + """ + # Imported here rather than at module scope: `chitai.services.matching` cannot + # be reached without initialising the `chitai.services` package, which imports + # the services, which import this module. + from chitai.services.matching import normalize_title + + self.normalized_title = normalize_title(title) + return title + def __repr__(self) -> str: return f"Book({self.title=!r})" @@ -132,6 +154,24 @@ class Identifier(BigIntBase): book_id: Mapped[int] = mapped_column(ForeignKey("books.id", ondelete="cascade")) value: Mapped[str] + # Kept current by `_normalize` below — never assign it directly. Null for an + # identifier that cannot carry a match: a per-build UUID, or an ISBN that fails + # its own checksum. + normalized_value: Mapped[Optional[str]] = mapped_column(index=True) + + @validates("name", "value") + def _normalize(self, key: str, value: str) -> str: + """Recompute `normalized_value` whenever either half of the pair changes.""" + from chitai.services.matching import normalize_identifier + + name = value if key == "name" else self.name + raw = value if key == "value" else self.value + + self.normalized_value = ( + normalize_identifier(name, raw) if name and raw else None + ) + return value + def __repr__(self): return f"Identifier({self.name!r} : {self.value!r})" @@ -139,6 +179,15 @@ class Identifier(BigIntBase): class FileMetadata(BigIntBase): __tablename__ = "file_metadata" + __table_args__ = ( + # Deliberately not unique. The hash is KOReader's partial MD5, which samples + # 12 KiB of the file, so two genuinely different files can collide — and an + # existing database may already hold duplicates, which a unique index would + # refuse to build over. Duplicate detection pairs it with `size` and treats a + # match as advisory, so this only has to make the lookup cheap. + Index("ix_file_metadata_hash", "hash"), + ) + book_id: Mapped[int] = mapped_column(ForeignKey("books.id", ondelete="cascade")) book: Mapped[Book] = relationship(back_populates="files") hash: Mapped[str] diff --git a/backend/src/chitai/database/models/duplicate_dismissal.py b/backend/src/chitai/database/models/duplicate_dismissal.py new file mode 100644 index 0000000..0aa1eeb --- /dev/null +++ b/backend/src/chitai/database/models/duplicate_dismissal.py @@ -0,0 +1,37 @@ +from sqlalchemy import ForeignKey, UniqueConstraint +from sqlalchemy.orm import Mapped, mapped_column + +from advanced_alchemy.base import BigIntBase + + +class DuplicateDismissal(BigIntBase): + """ + Two books a reader has said are not the same book. + + Title and author matching is probabilistic, so it will keep proposing a second + edition, a translation and a sequel that shares its predecessor's name. A review + screen with no way to disagree with it nags forever, which is how people learn to + ignore a screen. + + The pair is stored ordered — `book_a_id` is always the lower id — so "A and B" and + "B and A" are one row and the unique constraint can do its job. Use `pair()` rather + than assigning the columns directly. + """ + + __tablename__ = "duplicate_dismissals" + __table_args__ = (UniqueConstraint("book_a_id", "book_b_id"),) + + book_a_id: Mapped[int] = mapped_column( + ForeignKey("books.id", ondelete="cascade"), index=True + ) + book_b_id: Mapped[int] = mapped_column( + ForeignKey("books.id", ondelete="cascade"), index=True + ) + + @staticmethod + def pair(first: int, second: int) -> tuple[int, int]: + """The two book ids in the order this table stores them.""" + return (first, second) if first <= second else (second, first) + + def __repr__(self) -> str: + return f"DuplicateDismissal({self.book_a_id!r}, {self.book_b_id!r})" diff --git a/backend/src/chitai/schemas/__init__.py b/backend/src/chitai/schemas/__init__.py index 8b5d97a..f5358c1 100644 --- a/backend/src/chitai/schemas/__init__.py +++ b/backend/src/chitai/schemas/__init__.py @@ -4,7 +4,15 @@ from .book import ( BookProgressCreate, BookProgressRead, BooksCreateFromFiles, + BooksUploadResult, + BookMerge, BookMetadataUpdate, + DuplicateBookGroupRead, + DuplicateBookRead, + DuplicateDismissal, + DuplicateFileRead, + FileFingerprint, + PossibleDuplicateRead, FileMetadataRead, BookSeriesRead, ) diff --git a/backend/src/chitai/schemas/book.py b/backend/src/chitai/schemas/book.py index c252d53..de511e0 100644 --- a/backend/src/chitai/schemas/book.py +++ b/backend/src/chitai/schemas/book.py @@ -129,6 +129,83 @@ class BooksCreateFromFiles(BaseModel): model_config = ConfigDict(arbitrary_types_allowed=True) +class FileFingerprint(BaseModel): + """What a client can say about a file it has not uploaded yet.""" + + hash: str + size: int + filename: str = "" + + +class DuplicateFileRead(BaseModel): + """A file that was not stored because the library already holds its bytes.""" + + model_config = ConfigDict(from_attributes=True) + + filename: str + hash: str + size: int + library_id: int + + # Null when the match was another file in the same upload, which has no row yet. + book_id: int | None = None + book_title: str | None = None + + +class DuplicateBookRead(BaseModel): + """ + A stored book that may be the same book as another one. + + Unlike `DuplicateFileRead` this is a guess: the evidence is metadata two editions + of one work legitimately share. Nothing was refused on the strength of it. + """ + + model_config = ConfigDict(from_attributes=True) + + book_id: int + title: str + authors: list[str] + library_id: int + cover_image: Path | None = None + + # Why it matched: "identifier" and/or "title-author". + matched_on: list[str] + + +class PossibleDuplicateRead(BaseModel): + """A book that was imported, together with what it might be a second copy of.""" + + model_config = ConfigDict(from_attributes=True) + + book_id: int + title: str + candidates: list[DuplicateBookRead] + + +class DuplicateBookGroupRead(BaseModel): + """Books the library holds that all look like copies of one book.""" + + books: list[DuplicateBookRead] + + +class DuplicateDismissal(BaseModel): + """Two books a reader is saying are not the same book.""" + + book_a_id: int + book_b_id: int + + +class BooksUploadResult(BaseModel): + """The outcome of a multi-file upload: what was created, and what was skipped.""" + + created: list["BookRead"] + skipped: list[DuplicateFileRead] + + # Created, not skipped — these are books that went in and look like something the + # library already had. The reader decides what to do about it. + possible_duplicates: list[PossibleDuplicateRead] = Field(default_factory=list) + + class BookMetadataUpdate(BaseModel): title: str | None = None subtitle: str | None = None @@ -170,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 e045bd8..3399b41 100644 --- a/backend/src/chitai/services/book.py +++ b/backend/src/chitai/services/book.py @@ -5,12 +5,14 @@ from __future__ import annotations from collections import defaultdict import mimetypes -from collections.abc import Callable +from collections.abc import Callable, Iterable, Sequence +from dataclasses import dataclass, field, replace from io import BytesIO, RawIOBase +from itertools import combinations from pathlib import Path import uuid import zipfile -from typing import TYPE_CHECKING, Any, AsyncIterator +from typing import TYPE_CHECKING, Any, AsyncIterator, TypeVar # Third-party libraries from advanced_alchemy.extensions.litestar import service @@ -22,7 +24,8 @@ from advanced_alchemy.service import ( ) from advanced_alchemy.repository import SQLAlchemyAsyncRepository from advanced_alchemy.filters import CollectionFilter -from sqlalchemy import inspect +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 import aiofiles @@ -30,12 +33,16 @@ from aiofiles import os as aios from PIL import Image # Local imports -from chitai.config import settings +from chitai.config import DuplicateScope, settings from chitai.database.models import ( Book, Author, BookAuthorLink, + BookListLink, + BookProgress, BookTagLink, + DuplicateDismissal, + KosyncProgress, Tag, Publisher, BookSeries, @@ -45,11 +52,17 @@ from chitai.database.models import ( ) from chitai.schemas.book import BooksCreateFromFiles from chitai.services.filesystem_library import BookPathGenerator +from chitai.services.matching import ( + normalize_author, + normalize_identifier, + normalize_title, +) from chitai.services.metadata_extractor import Extractor as MetadataExtractor from chitai.services.utils import ( - calculate_koreader_hash, cleanup_empty_parent_directories, delete_file, + fingerprint_file, + fingerprint_upload, move_dir_contents, move_file, save_image, @@ -57,6 +70,294 @@ from chitai.services.utils import ( ) +# An incoming file, either uploaded or already sitting in the consume directory. +FileT = TypeVar("FileT", UploadFile, Path) + +# A file's identity for duplicate detection: its hash and its byte size. +# +# The hash on its own is not proof. It is KOReader's partial MD5, which samples +# twelve 1 KiB windows, so two different files can produce the same one — EPUBs are +# especially prone at offset 0, where the `mimetype` entry and `META-INF/container.xml` +# are often byte-identical across everything a given tool produced. Pairing it with the +# size makes a false match require both. +Fingerprint = tuple[str, int] + +# How much of a file is held in memory at a time while it is written to disk. +CHUNK_SIZE = 262144 # 256 KiB + + +@dataclass(frozen=True) +class DuplicateFile: + """ + An incoming file that was not stored because the library already holds its bytes. + + `book_id` and `book_title` are `None` when the match was another file in the same + import rather than something already in the database — there is no row to name yet. + """ + + filename: str + hash: str + size: int + library_id: int + book_id: int | None = None + book_title: str | None = None + + +# Why two books look like the same book. Both are reported when both apply. +MATCHED_ON_IDENTIFIER = "identifier" +MATCHED_ON_TITLE_AUTHOR = "title-author" + + +@dataclass(frozen=True) +class DuplicateBook: + """ + A book already in the library that may be the same book as another one. + + Unlike `DuplicateFile` this is a guess, not a fact: the evidence is metadata two + editions of one work legitimately share, and so do a work and its translation. It + is reported, never acted on. + """ + + book_id: int + title: str + authors: list[str] + library_id: int + matched_on: list[str] + cover_image: str | None = None + + +@dataclass(frozen=True) +class PossibleDuplicate: + """A book that was imported, together with what it might be a second copy of.""" + + book_id: int + title: str + candidates: list[DuplicateBook] + + +@dataclass +class ImportResult: + """What an import produced: the books created, and the files left out of them.""" + + books: list[Book] = field(default_factory=list) + duplicates: list[DuplicateFile] = field(default_factory=list) + + # Books that were created — nothing here was refused — and look like something the + # library already holds. The reader decides; see `find_duplicate_books`. + possible_duplicates: list[PossibleDuplicate] = field(default_factory=list) + + +class DuplicateFilesError(Exception): + """Raised when an import would store files that are already in the library.""" + + def __init__(self, duplicates: list[DuplicateFile]) -> None: + self.duplicates = duplicates + super().__init__(f"{len(duplicates)} file(s) are already in the library") + + +def _submitted_name(file: UploadFile | Path) -> str: + """The name an incoming file is tracked under while it is being screened.""" + return file.filename if isinstance(file, UploadFile) else file.name + + +def _unused_path(path: Path) -> Path: + """ + The given path, or the first free `name (n).ext` beside it. + + Filenames are generated from metadata, so two books that share an author and a + title produce the same one. Opening it for writing would replace the bytes another + `FileMetadata` row describes, leaving that row quietly lying about its own file. + + Args: + path: Where the file would go. + + Returns: + A path that nothing occupies. + """ + candidate = path + suffix = 1 + + while candidate.exists(): + suffix += 1 + candidate = path.with_name(f"{path.stem} ({suffix}){path.suffix}") + + return candidate + + +def _title_keys(title: str, authors: set[str]) -> set[str]: + """ + Every normalized title one book could reasonably be filed under. + + Ebook files are overwhelmingly named `Title - Author.epub`, and wherever nothing + inside the file overrode that name, the author ended up in the title column. So + one copy of a book is stored as "Building Microservices" and another as + "Building Microservices Sam Newman", and comparing titles alone never puts the + two together — which is most of what this feature is for. + + Both directions are generated, the author stripped off and the author added on, + so the comparison works whichever form each copy happens to hold. This stays an + exact match on an indexed column: no similarity score, nothing to tune. + + Args: + title: The book's normalized title. + authors: Its normalized author keys. + + Returns: + The title keys, or an empty set when there is no title to match on. + """ + if not title: + return set() + + keys = {title} + + for author in authors: + if title.endswith(f" {author}"): + # A title that is only its author's name is not a title once stripped. + if stripped := title[: -len(author)].strip(): + keys.add(stripped) + else: + keys.add(f"{title} {author}") + + return keys + + +def _matching_keys(data: Any) -> tuple[set[str], set[str], set[str]]: + """ + The keys a book is compared on: its title, its authors, and its identifiers. + + Accepts a `Book` — whose keys the model validators have already computed — or the + metadata dict an import is holding before any row exists. + + Args: + data: A `Book`, or a dict with `title`, `authors` and `identifiers`. + + Returns: + The set of title keys (empty if there is nothing to match on), the set of + author keys, and the set of identifier keys. + """ + if isinstance(data, Book): + authors = { + author.normalized_name for author in data.authors if author.normalized_name + } + return ( + _title_keys(data.normalized_title, authors), + authors, + { + identifier.normalized_value + for identifier in data.identifiers + if identifier.normalized_value + }, + ) + + authors = set() + for author in data.get("authors") or []: + key = ( + author.normalized_name + if isinstance(author, Author) + else normalize_author(author) + ) + if key: + authors.add(key) + + # Identifiers arrive as a `{name: value}` dict from the API and the extractors, and + # as `Identifier` rows once `_preprocess_book_data` has been over them. + raw = data.get("identifiers") or {} + pairs = ( + raw.items() + if isinstance(raw, dict) + else ((identifier.name, identifier.value) for identifier in raw) + ) + + identifiers = { + key for name, value in pairs if (key := normalize_identifier(name, value)) + } + + title = normalize_title(data.get("title")) + + return _title_keys(title, authors), authors, identifiers + + +def _series_position(data: Any) -> tuple[str, str | None]: + """The series a book belongs to and where it sits in it, both normalized.""" + if isinstance(data, Book): + series = data.series.title if data.series else None + position = data.series_position + else: + series = data.get("series") + series = series.title if isinstance(series, BookSeries) else series + position = data.get("series_position") + + return normalize_title(series), (position or "").strip() or None + + +def _is_different_volume(left: tuple[str, str | None], right: tuple[str, str | None]) -> bool: + """ + Whether two books are numbered entries of one series, and not the same entry. + + A trilogy shares an author, and often most of its title. Book two is emphatically + not a second copy of book one, and the position is the library saying so outright. + + Args: + left: One book's `(series key, position)`, from `_series_position`. + right: The other book's. + + Returns: + True when both name the same series at different positions. + """ + (left_series, left_position), (right_series, right_position) = left, right + + if not left_series or left_series != right_series: + return False + + if left_position is None or right_position is None: + return False + + try: + return float(left_position) != float(right_position) + except ValueError: + return left_position.casefold() != right_position.casefold() + + +class _DisjointSet: + """ + Union-find over book ids. + + Books are bucketed by each key they carry, so one book reaches a group through its + ISBN and another through its title. Merging the buckets is what puts A, B and C in + one group when A matches B on an ISBN and B matches C on its title. + """ + + def __init__(self) -> None: + self._parent: dict[int, int] = {} + + def find(self, item: int) -> int: + root = self._parent.setdefault(item, item) + + while root != self._parent[root]: + root = self._parent[root] + + while item != root: # Flatten the path behind us. + self._parent[item], item = root, self._parent[item] + + return root + + def union(self, first: int, second: int) -> None: + left, right = self.find(first), self.find(second) + if left != right: + self._parent[right] = left + + def groups(self) -> list[list[int]]: + """Every set of two or more members, in a stable order.""" + grouped: dict[int, list[int]] = defaultdict(list) + for item in sorted(self._parent): + grouped[self.find(item)].append(item) + + return sorted( + (members for members in grouped.values() if len(members) > 1), + key=lambda members: members[0], + ) + + class _ZipStream(RawIOBase): """ A write-only sink that hands whatever `ZipFile` writes back to the caller. @@ -106,7 +407,15 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): - async def create_book(self, data: ModelDictT[Book], library: Library, **kwargs) -> Book: + async def create_book( + self, + data: ModelDictT[Book], + library: Library, + *, + screen_duplicates: bool = True, + fingerprints: dict[str, Fingerprint] | None = None, + **kwargs, + ) -> Book: """ Create a new book entity. @@ -116,37 +425,769 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): Args: data: Book data as a dictionary or model. library: The library the book belongs to. - *args: Additional positional arguments passed to parent create. + screen_duplicates: Refuse the book if any of its files is already stored. + fingerprints: Pre-computed `(hash, size)` per submitted filename, so files + that have already been screened are not read a second time. **kwargs: Additional keyword arguments passed to parent create. Returns: The created Book entity. + + Raises: + DuplicateFilesError: If any file is already stored. Creating one book is a + deliberate act naming specific files, so this is all-or-nothing: + dropping one of them silently would be worse than refusing the lot. """ data = schema_dump(data) + + files = data.get("files") or [] + if screen_duplicates and files: + fingerprints = await self._fingerprint_uploads(files) + known = await self.find_duplicate_files(fingerprints.values(), library) + _, rejected = self._screen_for_duplicates( + files, fingerprints, known, library + ) + + if rejected: + raise DuplicateFilesError([duplicate for _, duplicate in rejected]) + await self._parse_metadata_from_files(data) await self._save_cover_image(data) - await self._save_book_files(library, data) + await self._save_book_files(library, data, fingerprints) return await super().create(data, **kwargs) + async def find_duplicate_files( + self, fingerprints: Iterable[Fingerprint], library: Library + ) -> dict[Fingerprint, DuplicateFile]: + """ + Look up which of the given fingerprints are already stored. + + A row only counts while its bytes are still on disk. The hash lives in the + database and the file does not, so a file removed behind the app's back would + otherwise go on refusing its own replacement — telling the reader the library + holds something it cannot open. + + Args: + fingerprints: The `(hash, size)` pairs to look for. + library: The library being imported into. The search is scoped to it + unless `duplicate_scope` widens or disables it. + + Returns: + A mapping from each matched fingerprint to the book already holding it. + Fingerprints with no match are absent. + """ + wanted = set(fingerprints) + + if not wanted or settings.duplicate_scope == DuplicateScope.OFF: + return {} + + statement = ( + select( + FileMetadata.hash, + FileMetadata.size, + FileMetadata.path, + Book.id, + Book.title, + Book.library_id, + Book.path, + ) + .join(Book, FileMetadata.book_id == Book.id) + .where(tuple_(FileMetadata.hash, FileMetadata.size).in_(wanted)) + # Databases predating duplicate detection can hold the same file twice; + # order so that the book reported for it is at least a stable one. + .order_by(Book.id) + ) + + if settings.duplicate_scope == DuplicateScope.LIBRARY: + statement = statement.where(Book.library_id == library.id) + + rows = await self.repository.session.execute(statement) + + matches: dict[Fingerprint, DuplicateFile] = {} + for file_hash, size, path, book_id, title, library_id, book_path in rows: + if (file_hash, size) in matches: + continue + + if not book_path or not await aios.path.isfile(Path(book_path) / path): + continue + + matches[(file_hash, size)] = DuplicateFile( + filename=Path(path).name, + hash=file_hash, + size=size, + library_id=library_id, + book_id=book_id, + book_title=title, + ) + + return matches + + # Everything a candidate has to be able to answer for: what it matched on, and + # enough about itself to be shown to a reader. + _MATCH_LOADS = ( + selectinload(Book.identifiers), + selectinload(Book.author_links).joinedload(BookAuthorLink.author), + joinedload(Book.series), + ) + + async def find_duplicate_books( + self, + data: ModelDictT[Book] | dict[str, Any], + library: Library, + exclude_book_id: int | None = None, + ) -> list[DuplicateBook]: + """ + Look for books the library already holds that may be the same book as this one. + + Two signals, either of which is enough: a shared identifier, or the same + normalized title credited to at least one of the same authors. Both are things + a second edition, a re-scan and a re-zipped EPUB carry when their bytes have + nothing in common, which is exactly the case file-level dedupe cannot see. + + Nothing here refuses anything. The evidence is metadata a work shares with its + own translation and with its own second edition, so a wrong answer would cost a + reader a book they meant to keep. + + Args: + data: The incoming book, as a `Book` or as the metadata dict an import is + holding before the row exists. + library: The library being imported into. The search is scoped to it unless + `duplicate_scope` widens or disables it. + exclude_book_id: A book to leave out — its own row, once it has one. + + Returns: + One entry per candidate, in id order, saying what it matched on. Empty when + the incoming book carries nothing to match on: no identifier, and either no + title or no authors. + """ + if settings.duplicate_scope == DuplicateScope.OFF: + return [] + + titles, authors, identifiers = _matching_keys(data) + + conditions = [] + if identifiers: + conditions.append( + Book.identifiers.any(Identifier.normalized_value.in_(identifiers)) + ) + + # A shared author is required for a title match, never optional: without it + # every book the extractors gave up on and called "Unknown" is a duplicate of + # every other one. + if titles and authors: + conditions.append( + and_( + Book.normalized_title.in_(titles), + Book.authors.any(Author.normalized_name.in_(authors)), + ) + ) + + if not conditions: + return [] + + statement = ( + select(Book).where(or_(*conditions)).options(*self._MATCH_LOADS).order_by(Book.id) + ) + + if exclude_book_id is not None: + statement = statement.where(Book.id != exclude_book_id) + + if settings.duplicate_scope == DuplicateScope.LIBRARY: + statement = statement.where(Book.library_id == library.id) + + candidates = ( + (await self.repository.session.execute(statement)).unique().scalars().all() + ) + + series = _series_position(data) + matches = [] + + for candidate in candidates: + if _is_different_volume(series, _series_position(candidate)): + continue + + ( + candidate_titles, + candidate_authors, + candidate_identifiers, + ) = _matching_keys(candidate) + + matched_on = [] + if identifiers & candidate_identifiers: + matched_on.append(MATCHED_ON_IDENTIFIER) + if titles & candidate_titles and authors & candidate_authors: + matched_on.append(MATCHED_ON_TITLE_AUTHOR) + + if matched_on: + matches.append(self._describe_candidate(candidate, matched_on)) + + 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. + + A book handed back by `create` carries its relationships only as far as the + caller happened to populate them, and touching an unloaded one from async code + raises rather than lazy-loading. Asking for them outright is the whole fix. + """ + statement = select(Book).where(Book.id == book_id).options(*self._MATCH_LOADS) + + return (await self.repository.session.execute(statement)).unique().scalar_one() + + async def find_duplicate_book_groups( + self, library: Library + ) -> list[list[DuplicateBook]]: + """ + Group everything already stored that looks like more than one copy of one book. + + `find_duplicate_books` only ever runs at import, so it says nothing about the + library someone already has. This is the pass over it, and the reason the + feature is worth having at all for an existing collection. + + Books are bucketed by each key they carry and the buckets are then merged, so a + group holds everything transitively connected: A and B sharing an ISBN, B and C + sharing a title and an author, all three in one group. Pairs a reader has + dismissed are never merged, so disagreeing with one pairing does not silently + break a group that stands on other evidence. + + Args: + library: The library to review. Scoped by `duplicate_scope`, exactly as the + import-time check is. + + Returns: + Groups of two or more, in a stable order. Empty when detection is off. + """ + if settings.duplicate_scope == DuplicateScope.OFF: + return [] + + statement = select(Book).options(*self._MATCH_LOADS).order_by(Book.id) + + if settings.duplicate_scope == DuplicateScope.LIBRARY: + statement = statement.where(Book.library_id == library.id) + + books = { + book.id: book + for book in (await self.repository.session.execute(statement)) + .unique() + .scalars() + .all() + } + + buckets: dict[tuple[str, str], list[int]] = defaultdict(list) + for book_id, book in books.items(): + titles, authors, identifiers = _matching_keys(book) + + for identifier in identifiers: + buckets[(MATCHED_ON_IDENTIFIER, identifier)].append(book_id) + + # One bucket per (title key, author) pair. A book sits in several, so two + # copies meet as long as they agree on any one of them. + for title in titles: + for author in authors: + buckets[(MATCHED_ON_TITLE_AUTHOR, f"{title}\x00{author}")].append(book_id) + + dismissed = await self._dismissed_pairs() + series = {book_id: _series_position(book) for book_id, book in books.items()} + + pairings = _DisjointSet() + matched_on: dict[int, set[str]] = defaultdict(set) + + for (reason, _), members in buckets.items(): + # Pairwise rather than wholesale: a dismissal and the series check both + # speak about two specific books, not about the bucket they landed in. + for left, right in combinations(members, 2): + if DuplicateDismissal.pair(left, right) in dismissed: + continue + if _is_different_volume(series[left], series[right]): + continue + + pairings.union(left, right) + matched_on[left].add(reason) + matched_on[right].add(reason) + + return [ + [ + self._describe_candidate(books[book_id], sorted(matched_on[book_id])) + for book_id in group + ] + for group in pairings.groups() + ] + + async def dismiss_duplicates(self, book_a_id: int, book_b_id: int) -> None: + """ + Record that two books are not the same book, and stop proposing them. + + Args: + book_a_id: One of the books. + book_b_id: The other. Order does not matter; the pair is stored ordered. + + Raises: + ValueError: If the two ids are the same, or either book does not exist. + """ + pair = DuplicateDismissal.pair(book_a_id, book_b_id) + + if pair[0] == pair[1]: + raise ValueError("A book cannot be dismissed against itself") + + found = ( + await self.repository.session.execute(select(Book.id).where(Book.id.in_(pair))) + ).scalars().all() + + if len(set(found)) != 2: + raise ValueError("No such book") + + if pair in await self._dismissed_pairs(): + return + + self.repository.session.add( + DuplicateDismissal(book_a_id=pair[0], book_b_id=pair[1]) + ) + await self.repository.session.flush() + + async def restore_duplicates(self, book_a_id: int, book_b_id: int) -> None: + """ + Undo a dismissal, so the pair is proposed again. + + Args: + book_a_id: One of the books. + book_b_id: The other. + """ + book_a_id, book_b_id = DuplicateDismissal.pair(book_a_id, book_b_id) + + await self.repository.session.execute( + delete(DuplicateDismissal).where( + DuplicateDismissal.book_a_id == book_a_id, + DuplicateDismissal.book_b_id == book_b_id, + ) + ) + + async def _dismissed_pairs(self) -> set[tuple[int, int]]: + """Every pair a reader has said is not a duplicate.""" + rows = await self.repository.session.execute( + select(DuplicateDismissal.book_a_id, DuplicateDismissal.book_b_id) + ) + + return set(rows.all()) + + @staticmethod + def _describe_candidate(book: Book, matched_on: list[str]) -> DuplicateBook: + """Enough of a book to show a reader who has to decide whether it is the same.""" + return DuplicateBook( + book_id=book.id, + title=book.title, + authors=[author.name for author in book.authors], + library_id=book.library_id, + matched_on=matched_on, + cover_image=book.cover_image, + ) + + async def _reserve_book_path( + self, parent: Path, book_id: int | None = None + ) -> Path: + """ + Find a directory for a book that no other book is already using. + + The path is generated from metadata alone, so two books that share an author + and title land on the same one — an `allow_duplicates` copy, or simply two + editions. Letting them share it is not a cosmetic problem: `book.path` is what + deletes, moves and file lookups act on, so one book's files overwrite the + other's and deleting either takes both. + + Args: + parent: The path the template generated. + book_id: The book being placed, if it already exists. Its own directory is + not a collision with itself. + + Returns: + `parent`, or the first free `parent (n)` beside it. + """ + candidate = parent + suffix = 1 + + while await self._path_belongs_to_another_book(candidate, book_id): + suffix += 1 + candidate = parent.with_name(f"{parent.name} ({suffix})") + + return candidate + + async def _path_belongs_to_another_book( + self, path: Path, book_id: int | None + ) -> bool: + """Whether a book other than `book_id` already claims this directory.""" + statement = select(Book.id).where(Book.path == str(path)).limit(1) + + if book_id is not None: + statement = statement.where(Book.id != book_id) + + return (await self.repository.session.execute(statement)).first() is not None + + async def _fingerprint_uploads( + self, files: Sequence[UploadFile] + ) -> dict[str, Fingerprint]: + """ + Fingerprint uploaded files, keyed by the name each was submitted under. + + Names carry the browser's relative path, so they are unique within a request + the same way the files themselves are — two entries under one name would + already be overwriting each other on disk. + """ + return {file.filename: await fingerprint_upload(file) for file in files} + + def _screen_for_duplicates( + self, + files: Sequence[FileT], + fingerprints: dict[str, Fingerprint], + known: dict[Fingerprint, DuplicateFile], + library: Library, + ) -> tuple[list[FileT], list[tuple[FileT, DuplicateFile]]]: + """ + Split incoming files into the ones to store and the ones already held. + + `known` is extended as it goes: an accepted file is recorded, so a second copy + of it later in the same import is refused too. The database cannot answer for + rows that do not exist yet. + + Args: + files: The incoming files, in the order they should be considered. + fingerprints: `(hash, size)` per submitted name, covering every file. + known: Fingerprints already accounted for, from `find_duplicate_files`. + library: The library being imported into. + + Returns: + The files to store, and the refused ones paired with a record of where + their bytes already live. + """ + if settings.duplicate_scope == DuplicateScope.OFF: + return list(files), [] + + accepted: list[FileT] = [] + rejected: list[tuple[FileT, DuplicateFile]] = [] + + for file in files: + name = _submitted_name(file) + fingerprint = fingerprints[name] + + if (match := known.get(fingerprint)) is not None: + rejected.append((file, replace(match, filename=name))) + continue + + accepted.append(file) + known[fingerprint] = DuplicateFile( + filename=name, + hash=fingerprint[0], + size=fingerprint[1], + library_id=library.id, + ) + + return accepted, rejected + async def create_many_from_files( - self, data: BooksCreateFromFiles, library: Library, **kwargs - ) -> list[Book]: + self, + data: BooksCreateFromFiles, + library: Library, + allow_duplicates: bool = False, + **kwargs, + ) -> ImportResult: """ Create multiple books from uploaded files. Groups files by their parent directory to organize books. Files in the root directory are treated as separate individual books. + Files the library already holds are skipped rather than refused: re-dropping a + folder to pick up the few books that are new to it is the thing this endpoint + exists for, so one already-stored file must not cost the rest of the import. + Args: data: Container with list of uploaded files. library: The library the books belong to. - *args: Additional positional arguments passed to create. + allow_duplicates: Store every file, even one already held. **kwargs: Additional keyword arguments passed to create. Returns: - List of created Book entities. + The books created, and the files skipped along the way. """ if not data.files: @@ -171,26 +1212,99 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): else: books[(filepath.parent, filepath.stem)].append(file) - return [ - await self.create_book( - {"files": [file for file in files], "library_id": library.id}, + # Hashed once for the whole request. Screening has to happen before anything is + # written, and `_save_book_files` then reuses these rather than reading every + # file a second time on its way to disk. + fingerprints = await self._fingerprint_uploads(data.files) + known = ( + {} + if allow_duplicates + else await self.find_duplicate_files(fingerprints.values(), library) + ) + + result = ImportResult() + for files in books.values(): + if allow_duplicates: + accepted, rejected = list(files), [] + else: + accepted, rejected = self._screen_for_duplicates( + files, fingerprints, known, library + ) + + result.duplicates.extend(duplicate for _, duplicate in rejected) + + # Nothing new in this folder. A book record with no files, and a directory + # to match, is worse than not importing it at all. + if not accepted: + continue + + book = await self.create_book( + {"files": accepted, "library_id": library.id}, library, + screen_duplicates=False, + fingerprints=fingerprints, **kwargs, ) - for files in books.values() - ] - + result.books.append(book) + await self._record_possible_duplicates(result, book, library) + + return result + + async def _record_possible_duplicates( + self, result: ImportResult, book: Book, library: Library + ) -> None: + """ + Note anything the library already holds that this book might be a copy of. + + Run after the book is created, not instead of creating it: a metadata match is + a guess, and the cost of acting on a wrong one is refusing a legitimate second + edition or a translation. The row is flushed by now, so the book before it in + the same import is a candidate too. + + Args: + result: The import being assembled, appended to in place. + book: The book that was just created. + library: The library it went into. + """ + candidates = await self.find_duplicate_books( + await self._load_for_matching(book.id), library, exclude_book_id=book.id + ) + + if candidates: + result.possible_duplicates.append( + PossibleDuplicate( + book_id=book.id, title=book.title, candidates=candidates + ) + ) + async def create_many_from_existing_files( self, file_paths: list[Path], consume_path: Path, library: Library, + allow_duplicates: bool = False, **kwargs - ) -> list[Book]: - + ) -> ImportResult: + """ + Import files that are already on disk, from the consume directory. + + Files the library already holds are moved aside rather than imported — see + `_quarantine_duplicate`, since there is nobody to ask what to do with them. + + Args: + file_paths: Absolute paths of the files to import. + consume_path: The library's consume directory, which the paths sit under. + library: The library the books belong to. + allow_duplicates: Import every file, even one already held. + **kwargs: Additional keyword arguments passed to create. + + Returns: + The books created, and the files moved aside as duplicates. + """ + # Group up files if they are in the same leaf directory - books = [] + result = ImportResult() file_groups: dict[Path, list[Path]] = defaultdict(list) @@ -211,22 +1325,39 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): # For each grouping for group, files in file_groups.items(): - data: dict[str, Any] = {'files': files} + # Fingerprinted before anything moves, since the paths are about to change. + fingerprints = {file.name: await fingerprint_file(file) for file in files} + + if allow_duplicates: + accepted, rejected = list(files), [] + else: + known = await self.find_duplicate_files(fingerprints.values(), library) + accepted, rejected = self._screen_for_duplicates( + files, fingerprints, known, library + ) + + for file, duplicate in rejected: + await self._quarantine_duplicate(file, consume_path, library) + result.duplicates.append(duplicate) + + if not accepted: + cleanup_empty_parent_directories(consume_path / group, consume_path) + continue + + data: dict[str, Any] = {'files': accepted} await self._parse_metadata_from_files(data, root_path=consume_path) await self._save_cover_image(data) # Get info from files path_gen = BookPathGenerator(library.root_path) - parent = path_gen.generate_path(data) + parent = await self._reserve_book_path(path_gen.generate_path(data)) data["path"] = str(parent) data["library_id"] = library.id file_metadata = [] - for file in files: - stats = await aios.stat(file) - file_size = stats.st_size + for file in accepted: + file_hash, file_size = fingerprints[file.name] content_type, _ = mimetypes.guess_type(file) - file_hash = await calculate_koreader_hash(file) filename = path_gen.generate_filename(data, Path(file.name)) @@ -241,21 +1372,49 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): data["files"] = file_metadata - # Move files to appropriate directory - if len(files) > 1: + # Move files to appropriate directory. A single file is moved by name + # rather than by group: the group key is a directory for files that + # arrived in one and the file's own relative path for files dropped at the + # top level, and moving the directory would nest the file one level below + # where its `FileMetadata.path` says it is. + if len(accepted) > 1: await move_dir_contents(consume_path / group, parent) else: - if len(group.parts) > 1: - await move_file(consume_path / group / files[0].name, parent / files[0].name) - else: - await move_file(consume_path / group, parent / group) + await move_file(accepted[0], parent / accepted[0].name) cleanup_empty_parent_directories(consume_path / group, consume_path) - books.append(await super().create(data)) + book = await super().create(data) + result.books.append(book) + await self._record_possible_duplicates(result, book, library) + await self.repository.session.commit() - return books + return result + + @staticmethod + async def _quarantine_duplicate( + file: Path, consume_path: Path, library: Library + ) -> None: + """ + Move a file the library already holds out of the consume directory. + + Nothing is deleted — the watcher has nobody to ask, and the file is the user's. + It cannot stay where it is either: `watchfiles` only reports additions, so a + file left behind is never looked at again and only accumulates. + + Args: + file: The refused file. + consume_path: The library's consume directory, which the file sits under. + library: The library it was offered to. + """ + destination = ( + Path(settings.duplicate_path) + / library.slug + / file.relative_to(consume_path) + ) + + await move_file(file, destination) async def delete_books( self, @@ -417,7 +1576,11 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): # Check if file path must be updated (only for books with files) if book.path is not None: path_gen = BookPathGenerator(library.root_path) - updated_path = path_gen.generate_path(book.to_dict() | data) + # Reserved rather than taken: an edit that renames this book onto another + # book's path would otherwise move its files in on top of theirs. + updated_path = await self._reserve_book_path( + path_gen.generate_path(book.to_dict() | data), book_id + ) if str(updated_path) != book.path: # TODO: Move only the files associated with the book instead of the whole directory await move_dir_contents(book.path, updated_path) @@ -427,7 +1590,11 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): return await super().update(data, item_id=book_id, execution_options={"populate_existing": True}) async def add_files( - self, book_id: int, files: list[UploadFile], library: Library + self, + book_id: int, + files: list[UploadFile], + library: Library, + allow_duplicates: bool = False, ) -> None: """ Add additional files to an existing book. @@ -436,14 +1603,77 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): book_id: The ID of the book. files: List of files to add. library: The library containing the book. + allow_duplicates: Store every file, even one already held. + + Raises: + DuplicateFilesError: If a file is already stored under a different book. """ book = await self.get(book_id) + fingerprints = await self._fingerprint_uploads(files) + + if not allow_duplicates: + # A file this book already carries is not a conflict — adding it again asks + # for a state that already holds, so there is nothing to do. Unless its + # bytes have gone missing, in which case that state does not hold: the + # upload fills the existing row back in rather than earning the book a + # second row for the same file. + own = {(file.hash, file.size): file for file in book.files} + remaining = [] + + for file in files: + stored = own.get(fingerprints[file.filename]) + if stored is None: + remaining.append(file) + else: + await self._restore_file(book, stored, file) + + files = remaining + + known = await self.find_duplicate_files( + (fingerprints[file.filename] for file in files), library + ) + files, rejected = self._screen_for_duplicates( + files, fingerprints, known, library + ) + + if rejected: + raise DuplicateFilesError([duplicate for _, duplicate in rejected]) + + if not files: + return + data = book.to_dict() data["files"] = files - new_files = await self._save_book_files(library, data) + new_files = await self._save_book_files(library, data, fingerprints) book.files.extend(new_files) await self.update_book(book.id, {"files": [file for file in book.files]}, library) + @staticmethod + async def _restore_file( + book: Book, stored: FileMetadata, upload: UploadFile + ) -> None: + """ + Put one of a book's own files back on disk, if it is no longer there. + + Args: + book: The book the file belongs to. + stored: The row saying where the file should be. + upload: The uploaded copy of it. + """ + if not book.path: + return + + path = Path(book.path) / stored.path + if await aios.path.isfile(path): + return + + path.parent.mkdir(parents=True, exist_ok=True) + await upload.seek(0) + + async with aiofiles.open(path, "wb") as dest: + while chunk := await upload.read(CHUNK_SIZE): + await dest.write(chunk) + async def remove_files( self, book_id: int, file_ids: list[int], delete_files: bool, library: Library ) -> None: @@ -664,7 +1894,12 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): book.identifiers[:] = reconciled - async def _save_book_files(self, library: Library, data: dict) -> list[FileMetadata]: + async def _save_book_files( + self, + library: Library, + data: dict, + fingerprints: dict[str, Fingerprint] | None = None, + ) -> list[FileMetadata]: """ Save uploaded book files to the filesystem. @@ -674,17 +1909,27 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): Args: library: The library containing the book. data: Book data with files to save. + fingerprints: `(hash, size)` per submitted filename, for files that have + already been hashed by duplicate screening. Anything not covered here + is hashed on its way to disk instead. Returns: The data with file paths and metadata populated. """ # Use the library template path with the book's data to generate a filepath path_gen = BookPathGenerator(library.root_path) - parent = path_gen.generate_path(data) - data["path"] = str(parent) + + # A book that already has a directory keeps it. `add_files` has to land beside + # the files that are already there, which is not necessarily where the template + # points now — the directory may have been renamed, or moved aside for a book + # that generated the same one. + if data.get("path"): + parent = Path(data["path"]) + else: + parent = await self._reserve_book_path(path_gen.generate_path(data)) + data["path"] = str(parent) file_metadata = [] - CHUNK_SIZE = 262144 # 256 KiB for file in data.pop("files", []): if TYPE_CHECKING: file: UploadFile @@ -694,15 +1939,22 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): # Store files in the correct directory and return the file's metadata await file.seek(0) - path = parent / filename - path.parent.mkdir(parents=True, exist_ok=True) + parent.mkdir(parents=True, exist_ok=True) + + path = _unused_path(parent / filename) + filename = path.name + + # Screening has usually hashed the file already; hashing it again on the + # way past would read every byte a second time for an answer we hold. + fingerprint = fingerprints.get(file.filename) if fingerprints else None hasher = StreamingHasher() async with aiofiles.open(path, "wb") as dest: # Read spooled file and save it to the local filesystem while chunk := await file.read(CHUNK_SIZE): await dest.write(chunk) - hasher.update(chunk) + if fingerprint is None: + hasher.update(chunk) stats = await aios.stat(path) file_size = stats.st_size @@ -711,7 +1963,7 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): FileMetadata( path=str(filename), size=file_size, - hash=hasher.hexdigest(), + hash=fingerprint[0] if fingerprint else hasher.hexdigest(), content_type=file.content_type, ) ) diff --git a/backend/src/chitai/services/consume.py b/backend/src/chitai/services/consume.py index 5fc9b62..8b79100 100644 --- a/backend/src/chitai/services/consume.py +++ b/backend/src/chitai/services/consume.py @@ -1,6 +1,7 @@ import asyncio from pathlib import Path from collections import defaultdict +from chitai.config import settings from chitai.database.models.library import Library from chitai.services import BookService, LibraryService from chitai.services.metadata_extractor import Extractor @@ -111,14 +112,33 @@ class ConsumeDirectoryWatcher: """Process a batch of files.""" try: - books = await self.book_service.create_many_from_existing_files( + result = await self.book_service.create_many_from_existing_files( list(file_paths), self.watch_path / Path(library_slug), library=await self._get_library(library_slug), ) - print(f"Created {len(books)} books!") - + print(f"Created {len(result.books)} books!") + + if result.duplicates: + print( + f"Moved {len(result.duplicates)} already-stored file(s) " + f"to {settings.duplicate_path}" + ) + + # Imported all the same — a metadata match is a guess, and there is nobody + # here to ask. The library's duplicates screen is where these get decided. + for possible in result.possible_duplicates: + names = ", ".join( + f"{candidate.title} (#{candidate.book_id})" + for candidate in possible.candidates + ) + print( + f"Imported {possible.title!r} (#{possible.book_id}), which may " + f"already be in the library as: {names}" + ) + + except Exception as e: print(f"Error processing batch: {e}") raise e diff --git a/backend/src/chitai/services/dependencies.py b/backend/src/chitai/services/dependencies.py index 63e5b6f..ead76e3 100644 --- a/backend/src/chitai/services/dependencies.py +++ b/backend/src/chitai/services/dependencies.py @@ -127,6 +127,31 @@ def create_book_filter_dependencies( # Get base filters first filters = create_filter_dependencies(config, dep_defaults) + # OVERRIDE: id filter typed by the configured id type, not always `str` + # + # advanced_alchemy's `provide_id_filter` annotates `ids` as `list[str]` and + # ignores `config["id_filter"]` entirely, so `?ids=12` reaches the database as + # the string "12" and Postgres refuses `bigint = character varying`. Nothing + # called `?ids=` until the duplicates screen needed to fetch a handful of books + # by id, which is why it went unnoticed. + if id_type := config.get("id_filter"): + id_field = config.get("id_field", "id") + + def provide_typed_id_filter( + ids=Parameter(query="ids", default=None, required=False), + ) -> CollectionFilter: + return CollectionFilter(field_name=id_field, values=ids) + + # Attached as a type object rather than written as an annotation: this module + # has `from __future__ import annotations`, so a written one is stored as the + # string "Optional[list[id_type]]" and resolved against module globals, where + # a local named `id_type` does not exist. + provide_typed_id_filter.__annotations__["ids"] = Optional[list[id_type]] + + filters[dep_defaults.ID_FILTER_DEPENDENCY_KEY] = Provide( + provide_typed_id_filter, sync_to_thread=False + ) + # OVERRIDE: Custom search filter with trigram search if config.get("search"): search_fields = config.get("search") diff --git a/backend/src/chitai/services/matching.py b/backend/src/chitai/services/matching.py new file mode 100644 index 0000000..103b1f1 --- /dev/null +++ b/backend/src/chitai/services/matching.py @@ -0,0 +1,207 @@ +# src/chitai/services/matching.py + +""" +Normalisation for book-level duplicate detection. + +Two copies of one book rarely agree on how it is written down. One says +`The Metamorphosis`, the other `Metamorphosis`; one credits `Kafka, Franz`, the other +`Franz Kafka`; one carries the ISBN-10 and the other the ISBN-13 of the same edition. +These functions reduce each of those to a single key, so the comparison is an equality +test the database can index rather than a similarity score nobody can explain. + +Everything here is pure: the keys are computed once and stored on the row (see +`Book.normalized_title`, `Author.normalized_name`, `Identifier.normalized_value`), so +no Postgres extension is needed at query time. +""" + +from __future__ import annotations + +import re +import unicodedata + +from chitai.services.utils import is_valid_isbn, isbn10_to_isbn13 + + +# Asides a title carries that say nothing about which book it is: +# "Frankenstein (Illustrated)", "Dune [Deluxe]". +_BRACKETED = re.compile(r"[(\[{][^)\]}]*[)\]}]") + +# Edition and format qualifiers, matched only as a *trailing* run of words. Anchoring +# to the end is what keeps "The Illustrated Man" a book and "Moby Dick Illustrated" a +# format note — a qualifier trails the title, it is never the thing the title is about. +_EDITION_NOISE = re.compile( + r"\s+(?:" + # "2nd edition", but also the compact forms publishers actually print on a + # cover: "2E", "3 Ed", "5e". The number alone is never enough — "Catch 22" is + # a title and must survive. + r"\d+(?:st|nd|rd|th)?\s*(?:edition|edn|ed|e)" + r"|(?:first|second|third|fourth|fifth|sixth|new|revised|expanded|updated|" + r"annotated|illustrated|unabridged|abridged|complete|definitive|deluxe|" + r"anniversary|collectors|international|kindle|paperback|hardcover|hardback|" + r"ebook|audiobook)" + r"(?:\s+(?:and|&)\s+\w+)*" + r"(?:\s+ed(?:ition|n)?)?" + r")$" +) + +_LEADING_ARTICLE = re.compile(r"^(?:the|a|an)\s+") + +# Anything that is not a letter, a digit or a space, once accents are gone. +_PUNCTUATION = re.compile(r"[^0-9a-z ]+") + +_WHITESPACE = re.compile(r"\s+") + +# Junk an extractor leaves on the end of a name: the `;` from a `DC:creator` list, the +# `.epub` from a filename the author's name was read out of. +_TRAILING_SEPARATORS = " ;,&/" +_FILE_EXTENSION = re.compile( + r"\.(?:epub|pdf|mobi|azw3?|djvu|fb2|txt|cbz|cbr)$", re.IGNORECASE +) + +# `J. R. R.` survives punctuation stripping as three one-letter words; `J.R.R.` as one. +# Joining any run of them makes both `jrr`. +_INITIAL_RUN = re.compile(r"\b(?:[a-z] )+[a-z]\b") + +# ISBNs are the same number under several names; everything else keeps its own. +_ISBN_NAMES = {"isbn", "isbn-10", "isbn10", "isbn-13", "isbn13"} + +# Generated fresh for every build of a file, so two copies of one book never share one. +# Matching on them would only re-find files the hash check already catches. +_PER_BUILD_NAMES = {"uuid", "urn:uuid"} + +# Below this an identifier is not specific enough to be evidence: a Calibre `id` of +# "42" would otherwise pair two unrelated books. +_MIN_IDENTIFIER_LENGTH = 4 + + +def _fold(text: str) -> str: + """Casefolded, accent-free, punctuation-free, single-spaced.""" + decomposed = unicodedata.normalize("NFKD", text.casefold()) + unaccented = "".join(c for c in decomposed if not unicodedata.combining(c)) + + return _WHITESPACE.sub(" ", _PUNCTUATION.sub(" ", unaccented)).strip() + + +def normalize_title(title: str | None) -> str: + """ + Reduce a title to the key two copies of one book should share. + + `Book.subtitle` is already split off by `Extractor.format_book_title`, so only what + is left in the title column is considered here. + + Args: + title: The title as it was stored. + + Returns: + The comparison key, or an empty string if nothing survives normalisation — + which is the signal not to match on the title at all. + """ + if not title: + return "" + + folded = _fold(_BRACKETED.sub(" ", title).replace("&", " and ")) + + # Repeated because qualifiers stack: "Dune Deluxe Edition Illustrated". + while (trimmed := _EDITION_NOISE.sub("", folded)) != folded: + folded = trimmed + + # An article says nothing, but a title that is only an article is not improved by + # having none, and neither is one that noise removal emptied out. + return _LEADING_ARTICLE.sub("", folded, count=1) or folded + + +def format_author_name(name: str | None) -> str: + """ + Tidy an author's name into the one form the library writes them in. + + Distinct from `normalize_author`, which throws away case, accents and spacing to + build a comparison key. This one is what a reader sees, so it keeps everything + that belongs to the name and only removes what an extractor added: a trailing + separator left over from a creator list, a file extension carried in from a + filename, and the `Surname, Given` ordering that EPUBs file names under. + + Args: + name: The name as the file or filename gave it. + + Returns: + The name to store, or an empty string if there is nothing left of it. + """ + if not name: + return "" + + tidied = _FILE_EXTENSION.sub("", name.strip().strip(_TRAILING_SEPARATORS).strip()) + + if tidied.count(",") == 1: + surname, given = (part.strip() for part in tidied.split(",")) + + # Only when the part before the comma is a single word. "Dave Thomas, Andy + # Hunt" is two people in one string, and flipping it would invent a third + # person who does not exist. Leaving an unrecognised form alone is the safe + # failure; rewriting it wrongly is not. + if surname and given and " " not in surname: + tidied = f"{given} {surname}" + + return _WHITESPACE.sub(" ", tidied).strip() + + +def normalize_author(name: str | None) -> str: + """ + Reduce an author's name to the key their other books should share. + + Deliberately not reduced to surname plus initial: that collides unrelated people, + and a wrong match here is a book pointed at a stranger's shelf. + + Args: + name: The name as it was stored, in either `Franz Kafka` or `Kafka, Franz` form. + + Returns: + The comparison key, or an empty string if nothing survives normalisation. + """ + if not name: + return "" + + # `Kafka, Franz` is one name written backwards. More than one comma is a list, or a + # suffix, and guessing at either does more harm than leaving it alone. + if name.count(",") == 1: + surname, forename = name.split(",") + name = f"{forename.strip()} {surname.strip()}" + + folded = _fold(name) + + return _INITIAL_RUN.sub(lambda run: run.group().replace(" ", ""), folded) + + +def normalize_identifier(name: str, value: str) -> str | None: + """ + Reduce one identifier to a `scheme:value` key, if it can carry a match at all. + + Args: + name: What kind of identifier it is, as stored. + value: The identifier itself. + + Returns: + The key, or None when the identifier is no use for matching: a per-build UUID, + something too short to be evidence, or an ISBN that fails its own checksum. + """ + name = (name or "").strip().casefold() + value = (value or "").strip() + + if not name or not value or name in _PER_BUILD_NAMES: + return None + + if name in _ISBN_NAMES: + digits = re.sub(r"[^0-9Xx]", "", value).upper() + + if not is_valid_isbn(digits): + return None + + # One scheme for both forms: a publisher prints whichever it likes, and the + # ISBN-10 and ISBN-13 of an edition are the same number written twice. + isbn = digits if len(digits) == 13 else isbn10_to_isbn13(digits) + return f"isbn:{isbn}" if isbn else None + + folded = _fold(value) or value.casefold() + if len(folded) < _MIN_IDENTIFIER_LENGTH: + return None + + return f"{name}:{folded}" diff --git a/backend/src/chitai/services/metadata_extractor.py b/backend/src/chitai/services/metadata_extractor.py index eb6c77c..1e6794f 100644 --- a/backend/src/chitai/services/metadata_extractor.py +++ b/backend/src/chitai/services/metadata_extractor.py @@ -3,7 +3,6 @@ # TODO: Code is a mess. Clean it up and add docstrings # Standard library -from abc import ABC, abstractmethod import datetime from pathlib import Path from io import BytesIO @@ -31,6 +30,147 @@ from chitai.services.utils import ( logger = logging.getLogger(__name__) +# Identifier schemes an EPUB can declare, mapped onto the names the rest of the app +# uses. A scheme arrives either as an `opf:scheme` attribute or as a prefix on the +# value itself (`urn:isbn:…`, `calibre:…`), and the two say the same thing. +_IDENTIFIER_SCHEMES = { + "isbn": "isbn", + "isbn10": "isbn-10", + "isbn-10": "isbn-10", + "isbn13": "isbn-13", + "isbn-13": "isbn-13", + "uuid": "uuid", + "calibre": "calibre", + "doi": "doi", + "asin": "asin", + "amazon": "asin", + "mobi-asin": "asin", + "google": "google", + "goodreads": "goodreads", +} + +# `scheme:rest`, with an optional `urn:` in front of it. +_SCHEME_PREFIX = re.compile(r"^(?:urn:)?([A-Za-z][A-Za-z0-9.-]*):(.+)$") + +_UUID = re.compile(r"^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$", re.IGNORECASE) + + +def parse_identifier(value: str, scheme: str | None = None) -> tuple[str, str] | None: + """ + Work out what one raw identifier is, and what it is worth storing as. + + EPUBs write the same ISBN as `9780486282114`, `978-0-486-28211-4` and + `urn:isbn:978-0-486-28211-4`, and carry plenty of identifiers that are not ISBNs + at all. Validating the string verbatim keeps only the first form and throws the + rest away, so normalise first and name whatever survives. + + Args: + value: The identifier as the file wrote it. + scheme: What the file said it is, if it said anything — an `opf:scheme` + attribute. A prefix on the value takes precedence over this. + + Returns: + The `(name, value)` to store, or `None` when there is nothing usable: an + empty value, or one declared to be an ISBN that fails its own checksum. + """ + value = (value or "").strip() + if not value: + return None + + name = _IDENTIFIER_SCHEMES.get((scheme or "").strip().casefold()) + + # An unrecognised prefix is part of the value rather than a scheme — + # "http://example.com/book" is not an identifier called "http". + if (match := _SCHEME_PREFIX.match(value)) and ( + prefixed := _IDENTIFIER_SCHEMES.get(match.group(1).casefold()) + ): + name = prefixed + value = match.group(2).strip() + + if name is None or name.startswith("isbn"): + digits = re.sub(r"[^0-9Xx]", "", value).upper() + if is_valid_isbn(digits): + return f"isbn-{len(digits)}", digits + + # Something that announced itself as an ISBN and is not one carries no + # information: storing it would link the reader to a page that does not exist. + if name is not None: + return None + + return name or ("uuid" if _UUID.match(value) else "id"), value + + +# Numbered editions, in the forms covers and catalogue records actually use: +# "3rd Edition", "2E", "4e", "8_e", "/6e", "(2nd edition)", "Third International Edition". +_ORDINAL_WORDS = { + "first": 1, "second": 2, "third": 3, "fourth": 4, "fifth": 5, "sixth": 6, + "seventh": 7, "eighth": 8, "ninth": 9, "tenth": 10, "eleventh": 11, "twelfth": 12, +} + +# Words that sit between the number and "Edition" and belong to the same statement. +_EDITION_QUALIFIER = ( + r"(?:international|global|revised|updated|expanded|anniversary|deluxe|student|" + r"instructors?|annotated|illustrated|reprint)" +) + +_EDITION = re.compile( + rf""" + [\s,;:/\-–—(\[]+ # the separator the statement hangs off + (?: + (?P\d{{1,2}})\s*(?:st|nd|rd|th)?[\s_]* + (?:{_EDITION_QUALIFIER}\s+)*(?:edition\b|edn\b|ed\b\.?|e\b) + | (?P{"|".join(_ORDINAL_WORDS)})\s+ + (?:{_EDITION_QUALIFIER}\s+)*(?:edition\b|edn\b|ed\b\.?) + ) + [\s)\]]* # and its closing bracket, if it had one + """, + re.IGNORECASE | re.VERBOSE, +) + + +def split_edition(title: str | None) -> tuple[str | None, int | None]: + """ + Separate a numbered edition statement from the title it is written into. + + "Fluent Python, 2nd Edition" is one book with a field for the edition, not a + title. Left in place it also splits the library: the second edition never looks + like the first, and neither matches the copy whose file simply did not mention it. + + The number is what makes this safe. Nothing is stripped without one, so + "Catch 22" and "Blade Runner 2049" keep their numbers and "Global Edition" — + which is a variant, not a numbered edition, and has nowhere to go in an + integer column — is left in the title where it can still be read. + + Args: + title: The title as the file or filename gave it. + + Returns: + The title without the edition statement, and the edition number. The title + unchanged and None when there is no numbered edition in it, or when removing + it would leave nothing behind. + """ + if not title: + return title, None + + if (match := _EDITION.search(title)) is None: + return title, None + + edition = ( + int(match["num"]) if match["num"] else _ORDINAL_WORDS[match["word"].casefold()] + ) + + stripped = _EDITION.sub(" ", title) + stripped = re.sub(r"\s{2,}", " ", stripped) + stripped = re.sub(r"\s+([,;:.!?])", r"\1", stripped) # "Works : What" → "Works: What" + stripped = stripped.strip(" ,;:-–—/") + + # A title that is only an edition statement is not improved by having none. + if not stripped: + return title, None + + return stripped, edition + + class FileExtractor(Protocol): @classmethod async def extract_metadata( @@ -54,15 +194,36 @@ class Extractor: # EPUB tends to give better metadata results over pdf sorted_files = sorted(files, key=lambda f: Extractor._get_file_priority(f)) + # Identifiers accumulate across formats instead of replacing each other. Every + # other field is a single value where the later, better-trusted format simply + # wins, but identifiers are a *collection*: a book holding an EPUB and a PDF + # genuinely carries what both of them declare, and merging the dict wholesale + # threw away everything the earlier format found. An EPUB that declares an + # ASIN, a Google volume id and a Calibre id kept none of them once a PDF + # contributed a single ISBN. + identifiers: dict[str, str] = {} + for file in sorted_files: match get_file_extension(file): case "epub": - metadata = metadata | await EpubExtractor.extract_metadata(file) + extracted = await EpubExtractor.extract_metadata(file) case "pdf": - metadata = metadata | await PdfExtractor.extract_metadata(file) + extracted = await PdfExtractor.extract_metadata(file) case _: break + # First writer wins per name, and the files are already ordered by how + # far their metadata can be trusted. A `dc:identifier` the publisher + # declared outranks an ISBN scraped out of a PDF's copyright page, which + # routinely prints the ISBNs of other formats and older editions too. + for name, value in (extracted.pop("identifiers", None) or {}).items(): + identifiers.setdefault(name, value) + + metadata = metadata | extracted + + if identifiers: + metadata["identifiers"] = identifiers + # Get metadata from file names for file in files: metadata = FilenameExtractor.extract_metadata(file) | metadata @@ -75,7 +236,14 @@ class Extractor: # format the title if metadata.get('title', None): - title, subtitle = Extractor.format_book_title(metadata["title"]) + # Before the subtitle split, so the edition cannot be mistaken for one: + # "How Linux Works, 3rd Edition: What Every Superuser Should Know" has to + # lose the edition first for the colon count to mean anything. + title, edition = split_edition(metadata["title"]) + if edition is not None: + metadata.setdefault("edition", edition) + + title, subtitle = Extractor.format_book_title(title) metadata["title"] = title metadata["subtitle"] = subtitle @@ -236,7 +404,7 @@ class PdfExtractor(FileExtractor): try: return datetime.datetime.strptime(date_portion, "%Y%m%d").date() - except Exception as e: + except Exception: return None @classmethod @@ -363,15 +531,23 @@ class EpubExtractor(FileExtractor): @classmethod def _extract_identifiers(cls, epub: epub.EpubBook) -> dict[str, str]: + """ + Every `DC:identifier` the file carries, keyed by what kind of thing it is. + + Non-ISBN identifiers are kept: `Identifier` is a free-form name/value pair, so + a Calibre id or an ASIN costs nothing to store and is one more thing two copies + of a book can be recognised by. + """ identifiers = {} - for id in epub.get_metadata("DC", "identifier"): - if is_valid_isbn(id[0]): - if len(id[0]) == 13: - identifiers.update({"isbn-13": id[0]}) + for value, attributes in epub.get_metadata("DC", "identifier"): + scheme = None + if isinstance(attributes, dict): + scheme = attributes.get("opf:scheme") or attributes.get("scheme") - elif len(id[0]) == 10: - identifiers.update({"isbn-10": id[0]}) + if (parsed := parse_identifier(value, scheme)) is not None: + name, parsed_value = parsed + identifiers[name] = parsed_value return identifiers @@ -380,7 +556,7 @@ class EpubExtractor(FileExtractor): try: return epub.get_metadata("DC", "description")[0][0] - except: + except Exception: return None @classmethod @@ -389,15 +565,15 @@ class EpubExtractor(FileExtractor): date_str = epub.get_metadata("DC", "date")[0][0].split("T")[0] return datetime.date.fromisoformat(date_str) - except: + except Exception: return None @classmethod def _extract_publisher(cls, epub: epub.EpubBook) -> str | None: try: - epub.get_metadata("DC", "publisher")[0][0] + return epub.get_metadata("DC", "publisher")[0][0] - except: + except Exception: return None @classmethod @@ -421,7 +597,7 @@ class EpubExtractor(FileExtractor): cover_item = epub.get_item_with_id(cover_id) if cover_item: return PIL.Image.open(BytesIO(cover_item.content)) - except Exception as e: + except Exception: pass # Fallback to next strategy # Strategy 2: Search image filenames for "cover" keyword @@ -489,7 +665,11 @@ class FilenameExtractor(FileExtractor): elif isinstance(input, Path): filename = get_filename(input, ext=False) elif isinstance(input, UploadFile): - filename = Path(input.filename).name + # `.stem`, not `.name`: the extension is not part of the metadata, and + # this is the browser upload path, so keeping it is how a library fills + # up with authors called "Sam Newman.epub". The other two branches have + # always stripped it. + filename = Path(input.filename).stem else: raise ValueError("Input type not supported") diff --git a/backend/src/chitai/services/utils.py b/backend/src/chitai/services/utils.py index 6d714ee..4d1d0d9 100644 --- a/backend/src/chitai/services/utils.py +++ b/backend/src/chitai/services/utils.py @@ -3,13 +3,11 @@ # Standard library from __future__ import annotations +import errno import hashlib from pathlib import Path import shutil -from typing import TYPE_CHECKING, BinaryIO - -if TYPE_CHECKING: - from hashlib import _Hash +from typing import BinaryIO # Third-party libraries import PIL @@ -32,6 +30,9 @@ KO_STEP = 1024 KO_SAMPLE_SIZE = 1024 KO_INDICES = range(-1, 11) # -1 to 10 inclusive +# How much is read at a time while hashing. +HASH_CHUNK_SIZE = 262144 # 256 KiB + def _lshift32(val: int, shift: int) -> int: """ @@ -100,10 +101,9 @@ async def calculate_koreader_hash(file_path: Path) -> str: offsets = _get_koreader_offsets() file_pos = 0 - chunk_size = 262144 # 256 KiB async with aiofiles.open(file_path, "rb") as f: - while chunk := await f.read(chunk_size): + while chunk := await f.read(HASH_CHUNK_SIZE): _partial_md5_from_chunk(chunk, hasher, offsets, file_pos) file_pos += len(chunk) @@ -132,6 +132,49 @@ class StreamingHasher: """Return the final hash.""" return self.hasher.hexdigest() + @property + def size(self) -> int: + """Total number of bytes fed in so far.""" + return self.position + + +async def fingerprint_upload(file: UploadFile) -> tuple[str, int]: + """ + Calculate the hash and byte size of an uploaded file without storing it. + + Duplicate detection has to answer before anything is written to the library, so + the file is read here and rewound for whoever writes it afterwards. + + Args: + file: The uploaded file to read. + + Returns: + The file's `(hash, size)` pair. + """ + hasher = StreamingHasher() + + await file.seek(0) + while chunk := await file.read(HASH_CHUNK_SIZE): + hasher.update(chunk) + await file.seek(0) + + return hasher.hexdigest(), hasher.size + + +async def fingerprint_file(file_path: Path) -> tuple[str, int]: + """ + Calculate the hash and byte size of a file already on disk. + + Args: + file_path: Path to the file to read. + + Returns: + The file's `(hash, size)` pair. + """ + stats = await aios.stat(file_path) + return await calculate_koreader_hash(file_path), stats.st_size + + ################################## # Filesystem related utilities # ################################## @@ -172,7 +215,7 @@ async def create_directory(dir_path: Path | str) -> None: async def move_file(src_path: Path, dest_path: Path, create_dirs=True) -> None: """ Move a file from source to destination asynchronously. - + Args: source_path: Path to the source file destination_path: Path to the destination file @@ -184,7 +227,16 @@ async def move_file(src_path: Path, dest_path: Path, create_dirs=True) -> None: if dest_dir: # Only create if there's a directory path await aios.makedirs(dest_dir, exist_ok=True) - await aios.rename(src_path, dest_path) + try: + await aios.rename(src_path, dest_path) + except OSError as exc: + if exc.errno != errno.EXDEV: + raise + + # Source and destination are on different filesystems, which rename cannot + # cross. Libraries, the consume directory and the duplicates directory are + # all configured separately, so they can easily be separate mounts. + shutil.move(str(src_path), str(dest_path)) async def move_dir_contents(source_dir: Path | str, target_dir: Path | str) -> None: """ @@ -432,7 +484,7 @@ def is_valid_isbn(isbn: str) -> bool: return is_valid_isbn13(isbn) else: return False - except: + except Exception: return False @@ -457,6 +509,29 @@ def is_valid_isbn10(isbn: str) -> bool: return str(check_digit) == isbn[-1] or (check_digit == 10 and isbn[-1] in "Xx") +def isbn10_to_isbn13(isbn: str) -> str | None: + """ + Convert an ISBN-10 to the ISBN-13 naming the same edition. + + The two are the same number written twice: prefix `978`, drop the ISBN-10 check + digit, recompute the check digit under the ISBN-13 rule. Matching only works if + both forms collapse onto one, since a publisher may print either. + + Args: + isbn: A 10-character ISBN, digits and an optional trailing `X` only. + + Returns: + The equivalent ISBN-13, or None if the input is not a valid ISBN-10. + """ + if not is_valid_isbn(isbn) or len(isbn) != 10: + return None + + digits = f"978{isbn[:9]}" + total = sum(int(digit) * (1 if i % 2 == 0 else 3) for i, digit in enumerate(digits)) + + return f"{digits}{(10 - total % 10) % 10}" + + def is_valid_isbn13(isbn: str) -> bool: """ Validate an ISBN-13 number using its check digit. diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 02911c3..c910385 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -40,7 +40,13 @@ pytest_plugins = [ @pytest.fixture(autouse=True) def _patch_settings(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(settings, "book_cover_path", f"{tmp_path}/covers") + # PIL will not create the directory it is asked to save into, so anything that + # imports a file carrying a cover needs it to exist first. + covers = tmp_path / "covers" + covers.mkdir() + + monkeypatch.setattr(settings, "book_cover_path", str(covers)) + monkeypatch.setattr(settings, "duplicate_path", str(tmp_path / "duplicates")) @pytest.fixture(name="engine") diff --git a/backend/tests/integration/test_book.py b/backend/tests/integration/test_book.py index 186a9d2..62139dd 100644 --- a/backend/tests/integration/test_book.py +++ b/backend/tests/integration/test_book.py @@ -37,7 +37,8 @@ from pathlib import Path ( Path("tests/data_files/Calculus Made Easy - Silvanus Thompson.pdf"), 2, - "The Project Gutenberg eBook #33283: Calculus Made Easy, 2nd Edition", + # The ", 2nd Edition" is split off into `edition`, not kept in the title. + "The Project Gutenberg eBook #33283: Calculus Made Easy", ["Silvanus Phillips Thompson"], ), ], @@ -208,6 +209,21 @@ async def test_get_book_file( assert downloaded_content == file_content +async def test_list_books_by_id(populated_authenticated_client: AsyncClient) -> None: + """ + `?ids=` has to reach the database as integers. + + advanced_alchemy's stock id filter annotates the parameter as `list[str]` whatever + the configured id type, so the ids arrived as strings and Postgres refused to + compare a bigint primary key against them. Nothing called it until a screen needed + to fetch a handful of books by id. + """ + response = await populated_authenticated_client.get("/books?ids=1&ids=2&pageSize=10") + + assert response.status_code == 200 + assert sorted(book["id"] for book in response.json()["items"]) == [1, 2] + + async def test_get_book_by_id(populated_authenticated_client: AsyncClient) -> None: """Test retrieving a specific book by ID.""" @@ -366,7 +382,8 @@ async def test_create_multiple_books_from_directory( assert response.status_code == 201 data = response.json() - assert len(data.get("items") or data.get("data")) >= 1 + assert len(data["created"]) == 2 + assert data["skipped"] == [] async def test_create_books_from_parent_directory_keeps_embedded_title( @@ -396,7 +413,7 @@ async def test_create_books_from_parent_directory_keeps_embedded_title( assert response.status_code == 201 - books = response.json()["items"] + books = response.json()["created"] assert len(books) == 1 assert books[0]["title"] == "Metamorphosis" @@ -419,11 +436,336 @@ async def test_create_books_groups_formats_within_one_folder( assert response.status_code == 201 - books = response.json()["items"] + books = response.json()["created"] assert len(books) == 1 assert len(books[0]["files"]) == 2 +class TestDuplicateHandling: + """A file the library already holds must not be stored a second time.""" + + epub_path = Path("tests/data_files/Metamorphosis - Franz Kafka.epub") + + def upload(self, name: str | None = None) -> list[tuple[str, tuple]]: + return [ + ( + "files", + ( + name or self.epub_path.name, + self.epub_path.read_bytes(), + "application/epub+zip", + ), + ) + ] + + async def test_bulk_upload_reports_skipped_files( + self, authenticated_client: AsyncClient + ) -> None: + """Re-dropping a folder must import what is new and name what was not.""" + first = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + assert first.status_code == 201 + created = first.json()["created"][0] + + second = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + + assert second.status_code == 201 + result = second.json() + assert result["created"] == [] + assert len(result["skipped"]) == 1 + + skipped = result["skipped"][0] + assert skipped["filename"] == self.epub_path.name + assert skipped["book_id"] == created["id"] + assert skipped["book_title"] == created["title"] + + async def test_bulk_upload_can_be_forced( + self, authenticated_client: AsyncClient + ) -> None: + await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + + response = await authenticated_client.post( + "/books/fromFiles?library_id=1&allow_duplicates=true", files=self.upload() + ) + + assert response.status_code == 201 + assert len(response.json()["created"]) == 1 + assert response.json()["skipped"] == [] + + async def test_single_book_create_conflicts( + self, authenticated_client: AsyncClient + ) -> None: + """Naming files deliberately earns a refusal rather than a silent drop.""" + await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + + response = await authenticated_client.post( + "/books?library_id=1", files=self.upload(), data={"library_id": 1} + ) + + assert response.status_code == 409 + assert response.json()["extra"][0]["filename"] == self.epub_path.name + + forced = await authenticated_client.post( + "/books?library_id=1&allow_duplicates=true", + files=self.upload(), + data={"library_id": 1}, + ) + assert forced.status_code == 201 + + async def test_adding_another_books_file_conflicts( + self, authenticated_client: AsyncClient + ) -> None: + created = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + book_id = created.json()["created"][0]["id"] + + other = await authenticated_client.post( + "/books?library_id=1", + files=[ + ( + "files", + ( + "war.epub", + Path("tests/data_files/The Art of War - Sun Tzu.epub").read_bytes(), + "application/epub+zip", + ), + ) + ], + data={"library_id": 1}, + ) + other_id = other.json()["id"] + + response = await authenticated_client.post( + f"/books/{book_id}/files", + files=[ + ( + "files", + ( + "war.epub", + Path("tests/data_files/The Art of War - Sun Tzu.epub").read_bytes(), + "application/epub+zip", + ), + ) + ], + ) + + assert response.status_code == 409 + assert response.json()["extra"][0]["book_id"] == other_id + + async def test_resending_a_books_own_file_changes_nothing( + self, authenticated_client: AsyncClient + ) -> None: + created = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + book_id = created.json()["created"][0]["id"] + + response = await authenticated_client.post( + f"/books/{book_id}/files", files=self.upload() + ) + + assert response.status_code == 201 + assert len(response.json()["files"]) == 1 + + async def test_duplicates_can_be_checked_before_uploading( + self, authenticated_client: AsyncClient + ) -> None: + """The pre-flight check answers from hashes alone, with no file sent.""" + created = await authenticated_client.post( + "/books/fromFiles?library_id=1", files=self.upload() + ) + book = created.json()["created"][0] + stored = book["files"][0] + + response = await authenticated_client.post( + "/books/duplicate-files?library_id=1", + json=[ + { + "hash": stored["hash"], + "size": stored["size"], + "filename": "local-copy.epub", + }, + {"hash": stored["hash"], "size": stored["size"] + 1}, + {"hash": "0" * 32, "size": 1234}, + ], + ) + + assert response.status_code == 200 + + matches = response.json() + assert len(matches) == 1 + assert matches[0]["filename"] == "local-copy.epub" + 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_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: + 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 diff --git a/backend/tests/unit/test_matching.py b/backend/tests/unit/test_matching.py new file mode 100644 index 0000000..fd707ec --- /dev/null +++ b/backend/tests/unit/test_matching.py @@ -0,0 +1,233 @@ +"""Tests for the normalization behind book-level duplicate detection.""" + +import pytest + +from chitai.services.matching import ( + format_author_name, + normalize_author, + normalize_identifier, + normalize_title, +) +from chitai.services.metadata_extractor import parse_identifier +from chitai.services.utils import isbn10_to_isbn13 + + +class TestNormalizeTitle: + """Two copies of one book rarely agree on how the title is written.""" + + @pytest.mark.parametrize( + ("title", "expected"), + [ + ("The Metamorphosis", "metamorphosis"), + ("Metamorphosis", "metamorphosis"), + ("METAMORPHOSIS", "metamorphosis"), + ("A Tale of Two Cities", "tale of two cities"), + ("An Enquiry", "enquiry"), + # Accents, punctuation and ampersands are spelling, not identity. + ("Les Misérables", "les miserables"), + ("Moby Dick; Or, The Whale", "moby dick or the whale"), + ("Sense & Sensibility", "sense and sensibility"), + # Bracketed asides and trailing edition noise say nothing about the book. + ("Frankenstein (Illustrated)", "frankenstein"), + ("Frankenstein [Kindle Edition]", "frankenstein"), + ("Frankenstein, 2nd Edition", "frankenstein"), + # The compact forms a cover actually carries. + ("Building Microservices, 2E", "building microservices"), + ("Building Microservices 2e", "building microservices"), + ("Frankenstein 3 Ed", "frankenstein"), + ("Dungeons & Dragons 5e", "dungeons and dragons"), + ("Frankenstein Revised Edition", "frankenstein"), + ("Dune Deluxe Edition Illustrated", "dune"), + ("", ""), + (None, ""), + ], + ) + def test_titles_that_should_agree(self, title: str | None, expected: str) -> None: + assert normalize_title(title) == expected + + def test_a_qualifier_that_is_the_title_survives(self) -> None: + """A trailing qualifier is noise; the same word at the front is the book.""" + assert normalize_title("The Illustrated Man") == "illustrated man" + + def test_normalization_never_empties_a_title(self) -> None: + """An article-only title is not improved by having no article left.""" + assert normalize_title("The") == "the" + + @pytest.mark.parametrize( + "title", ["Catch 22", "Fahrenheit 451", "Blade Runner 2049", "1984", "Apollo 13"] + ) + def test_a_number_is_not_an_edition(self, title: str) -> None: + """Edition stripping keys on the `e`; a bare number is part of the title.""" + assert normalize_title(title) == title.casefold() + + +class TestNormalizeAuthor: + """One person, written down several ways.""" + + @pytest.mark.parametrize( + ("name", "expected"), + [ + ("Franz Kafka", "franz kafka"), + ("Kafka, Franz", "franz kafka"), + ("KAFKA, FRANZ", "franz kafka"), + ("Émile Zola", "emile zola"), + ("Doyle, Arthur Conan", "arthur conan doyle"), + # Runs of initials are joined, so spacing them out changes nothing. + ("J.R.R. Tolkien", "jrr tolkien"), + ("J. R. R. Tolkien", "jrr tolkien"), + ("JRR Tolkien", "jrr tolkien"), + ("", ""), + (None, ""), + ], + ) + def test_names_that_should_agree(self, name: str | None, expected: str) -> None: + assert normalize_author(name) == expected + + def test_two_people_are_not_reduced_together(self) -> None: + """Surname plus initial would collide unrelated writers; it is not used.""" + assert normalize_author("Charles Dickens") != normalize_author("Colin Dexter") + + def test_a_list_is_left_alone(self) -> None: + """More than one comma is a list or a suffix, and guessing does more harm.""" + assert normalize_author("Smith, John, Jr.") == "smith john jr" + + +class TestFormatAuthorName: + """What gets stored and shown, as opposed to what gets compared.""" + + @pytest.mark.parametrize( + ("written", "expected"), + [ + # A leftover separator from a `DC:creator` list. + ("Newman, Sam;", "Sam Newman"), + ("Sam Newman ", "Sam Newman"), + (" Dan Vanderkam ", "Dan Vanderkam"), + # `Surname, Given` is how EPUBs file a name, not how anyone reads it. + ("Kleppmann, Martin", "Martin Kleppmann"), + ("Huxley, Aldous", "Aldous Huxley"), + ("Liu, Cixin", "Cixin Liu"), + # An extension carried in from the filename the name was read out of. + ("Sam Newman.epub", "Sam Newman"), + ("Franz Kafka.mobi", "Franz Kafka"), + ("Brian W. Kernighan.epub", "Brian W. Kernighan"), + ("", ""), + (None, ""), + ], + ) + def test_names_are_tidied(self, written: str | None, expected: str) -> None: + assert format_author_name(written) == expected + + @pytest.mark.parametrize( + "written", + [ + # Two people in one string. Flipping it would invent a third. + "Dave Thomas, Andy Hunt", + "Mark Richards, Neal Ford", + # A compound surname is not recognised, and is left alone rather than + # rearranged wrongly. + "García Márquez, Gabriel", + ], + ) + def test_an_unrecognised_form_is_left_alone(self, written: str) -> None: + assert format_author_name(written) == written + + def test_case_and_accents_belong_to_the_author(self) -> None: + """Tidying removes what an extractor added; it does not correct spelling.""" + assert format_author_name("Michał Płachta.epub") == "Michał Płachta" + assert format_author_name("Steve McConnell") == "Steve McConnell" + + @pytest.mark.parametrize( + "written", ["Newman, Sam;", "Sam Newman.epub", "Kleppmann, Martin"] + ) + def test_tidying_is_idempotent(self, written: str) -> None: + """`unique_filter` tidies a name that may already be tidy; it must not drift.""" + once = format_author_name(written) + assert format_author_name(once) == once + + +class TestNormalizeIdentifier: + """Identifiers only help if the same edition produces the same key.""" + + def test_isbn_10_and_isbn_13_are_one_key(self) -> None: + assert normalize_identifier("isbn-10", "0486282112") == "isbn:9780486282114" + assert normalize_identifier("isbn-13", "9780486282114") == "isbn:9780486282114" + + @pytest.mark.parametrize( + "written", ["978-0-486-28211-4", "978 0 486 28211 4", "9780486282114"] + ) + def test_formatting_is_not_part_of_an_isbn(self, written: str) -> None: + assert normalize_identifier("isbn", written) == "isbn:9780486282114" + + def test_an_isbn_that_fails_its_checksum_is_no_evidence(self) -> None: + assert normalize_identifier("isbn-13", "9780486282115") is None + + def test_uuids_are_refused(self) -> None: + """Generated per build, so they only re-find what the hash check catches.""" + assert normalize_identifier("uuid", "3f2b1c4e-1111-2222-3333-444455556666") is None + assert normalize_identifier("urn:uuid", "3f2b1c4e-1111-2222-3333-444455556666") is None + + def test_other_schemes_keep_their_own_key(self) -> None: + assert normalize_identifier("asin", "B000FC0PDA") == "asin:b000fc0pda" + assert normalize_identifier("ASIN", "b000fc0pda") == "asin:b000fc0pda" + + def test_something_too_short_is_not_evidence(self) -> None: + """A Calibre id of "42" would otherwise pair two unrelated books.""" + assert normalize_identifier("calibre", "42") is None + + @pytest.mark.parametrize(("name", "value"), [("", "1234567"), ("asin", "")]) + def test_half_an_identifier_is_no_identifier(self, name: str, value: str) -> None: + assert normalize_identifier(name, value) is None + + +class TestIsbnConversion: + def test_isbn_10_converts_to_its_isbn_13(self) -> None: + assert isbn10_to_isbn13("0486282112") == "9780486282114" + + def test_a_trailing_x_is_a_digit(self) -> None: + assert isbn10_to_isbn13("043942089X") == "9780439420891" + + @pytest.mark.parametrize("isbn", ["0486282113", "9780486282114", "nonsense"]) + def test_anything_that_is_not_an_isbn_10_converts_to_nothing(self, isbn: str) -> None: + assert isbn10_to_isbn13(isbn) is None + + +class TestParseIdentifier: + """What an EPUB writes, and what is worth storing for it.""" + + @pytest.mark.parametrize( + "written", + [ + "9780486282114", + "978-0-486-28211-4", + "urn:isbn:9780486282114", + "urn:isbn:978-0-486-28211-4", + "ISBN:978-0-486-28211-4", + ], + ) + def test_isbns_survive_however_they_are_written(self, written: str) -> None: + assert parse_identifier(written) == ("isbn-13", "9780486282114") + + def test_the_scheme_attribute_is_read_too(self) -> None: + assert parse_identifier("0-486-28211-2", "ISBN") == ("isbn-10", "0486282112") + + def test_non_isbn_identifiers_are_kept(self) -> None: + assert parse_identifier("urn:uuid:3f2b1c4e-1111-2222-3333-444455556666") == ( + "uuid", + "3f2b1c4e-1111-2222-3333-444455556666", + ) + assert parse_identifier("calibre:1234") == ("calibre", "1234") + assert parse_identifier("B000FC0PDA", "mobi-asin") == ("asin", "B000FC0PDA") + + def test_an_unrecognised_prefix_is_part_of_the_value(self) -> None: + """"http://example.com/book" is not an identifier called "http".""" + assert parse_identifier("http://www.gutenberg.org/5200") == ( + "id", + "http://www.gutenberg.org/5200", + ) + + def test_a_declared_isbn_that_is_not_one_is_dropped(self) -> None: + assert parse_identifier("urn:isbn:not-an-isbn") is None + + @pytest.mark.parametrize("written", ["", " ", None]) + def test_nothing_yields_nothing(self, written: str | None) -> None: + assert parse_identifier(written) is None diff --git a/backend/tests/unit/test_metadata_extractor.py b/backend/tests/unit/test_metadata_extractor.py index d8335cc..7070776 100644 --- a/backend/tests/unit/test_metadata_extractor.py +++ b/backend/tests/unit/test_metadata_extractor.py @@ -1,7 +1,13 @@ import pytest +from ebooklib import epub from pathlib import Path from datetime import date -from chitai.services.metadata_extractor import EpubExtractor +from chitai.services.metadata_extractor import ( + EpubExtractor, + Extractor, + PdfExtractor, + split_edition, +) @pytest.mark.asyncio() @@ -15,3 +21,172 @@ class TestEpubExtractor: assert metadata["authors"] == ["Herman Melville"] assert metadata["published_date"] == date(year=2001, month=7, day=1) + + +EPUB = Path("tests/data_files/Metamorphosis - Franz Kafka.epub") +PDF = Path("tests/data_files/Calculus Made Easy - Silvanus Thompson.pdf") + + +@pytest.mark.asyncio() +class TestIdentifierMerging: + """A book's formats each contribute identifiers; none of them replaces the rest.""" + + async def test_every_format_contributes(self, monkeypatch: pytest.MonkeyPatch) -> None: + """ + Identifiers are a collection, not a single value. + + Merging the whole dict meant the last format to report won outright: an EPUB + declaring an ASIN, a Google volume id and a Calibre id kept none of them once + a PDF contributed one ISBN. + """ + + async def epub(_file): + return { + "title": "How Linux Works", + "identifiers": {"isbn-13": "9781718500419", "asin": "1718500408"}, + } + + async def pdf(_file): + return {"identifiers": {"isbn-13": "9781718500402", "isbn-10": "1593270356"}} + + monkeypatch.setattr(EpubExtractor, "extract_metadata", epub) + monkeypatch.setattr(PdfExtractor, "extract_metadata", pdf) + + metadata = await Extractor.extract_metadata( + [Path("How Linux Works.epub"), Path("How Linux Works.pdf")] + ) + + assert metadata["identifiers"] == { + # Declared by the publisher's toolchain, so it outranks the PDF's, which + # was scraped off a copyright page that also prints the print edition's. + "isbn-13": "9781718500419", + "asin": "1718500408", + "isbn-10": "1593270356", + } + + async def test_a_second_format_that_finds_nothing_erases_nothing(self) -> None: + """The PDF fixture carries no ISBN, so it must leave the EPUB's alone.""" + metadata = await Extractor.extract_metadata([EPUB, PDF]) + + assert metadata["identifiers"] == {"id": "http://www.gutenberg.org/5200"} + + async def test_one_format_on_its_own_is_unaffected(self) -> None: + metadata = await Extractor.extract_metadata([EPUB]) + + assert metadata["identifiers"] == {"id": "http://www.gutenberg.org/5200"} + + async def test_no_identifiers_anywhere_leaves_the_field_absent(self) -> None: + """An empty dict would count as extracted metadata and overwrite nothing.""" + metadata = await Extractor.extract_metadata([PDF]) + + assert "identifiers" not in metadata + + +class TestSplitEdition: + """An edition is a field on the book, not part of what the book is called.""" + + @pytest.mark.parametrize( + ("title", "stripped", "edition"), + [ + # Every form below is one that turned up in a real library. + ("Fluent Python, 2nd Edition", "Fluent Python", 2), + ("Building Microservices, 2E", "Building Microservices", 2), + ("Digital Image Processing, 4e", "Digital Image Processing", 4), + ( + "Network Security Essentials: Applications and Standards/6e", + "Network Security Essentials: Applications and Standards", + 6, + ), + ( + "Refactoring: Improving the Design of Existing Code (2nd edition)", + "Refactoring: Improving the Design of Existing Code", + 2, + ), + # Ordinal words, including a qualifier sitting inside the statement. + ( + "The Art of Computer Programming: Volume 1 / Fundamental Algorithms, Third Edition", + "The Art of Computer Programming: Volume 1 / Fundamental Algorithms", + 3, + ), + ( + "Introduction to the Theory of Computation, Third International Edition", + "Introduction to the Theory of Computation", + 3, + ), + # Mid-title, before a subtitle and before a trailing author. + ( + "How Linux Works, 3rd Edition: What Every Superuser Should Know", + "How Linux Works: What Every Superuser Should Know", + 3, + ), + ( + "Code Complete, 2nd Edition - Steve McConnell", + "Code Complete - Steve McConnell", + 2, + ), + # An underscore between the number and the "e", beside an unnumbered + # qualifier that has nowhere to go in an integer column and so stays put. + ( + "Cryptography and Network Security, Global Edition, 8_e - Stallings", + "Cryptography and Network Security, Global Edition - Stallings", + 8, + ), + ], + ) + def test_editions_are_split_out(self, title: str, stripped: str, edition: int) -> None: + assert split_edition(title) == (stripped, edition) + + @pytest.mark.parametrize( + "title", + [ + # A number alone is never an edition — these are titles. + "Catch 22", + "Fahrenheit 451", + "Blade Runner 2049", + "1984", + "Apollo 13", + "Slaughterhouse 5", + "The Art of Computer Programming: Volume 1", + # "Edition" with no number cannot be stored, so it stays where it can + # still be read. + "Cryptography and Network Security: Principles and Practice, Global Edition", + "Building Microservices", + ], + ) + def test_titles_are_left_alone(self, title: str) -> None: + assert split_edition(title) == (title, None) + + def test_a_title_that_is_only_an_edition_is_kept(self) -> None: + """Stripping must never leave a book with no title at all.""" + assert split_edition("2nd Edition") == ("2nd Edition", None) + + @pytest.mark.parametrize("title", ["", None]) + def test_nothing_yields_nothing(self, title: str | None) -> None: + assert split_edition(title) == (title, None) + + +@pytest.mark.asyncio() +class TestEditionFromFiles: + async def test_extraction_moves_the_edition_off_the_title(self) -> None: + """The PDF fixture calls itself a 2nd edition in its own metadata title.""" + metadata = await Extractor.extract_metadata([PDF]) + + assert metadata["title"] == "The Project Gutenberg eBook #33283: Calculus Made Easy" + assert metadata["edition"] == 2 + + +class TestEpubPublisher: + """The publisher was looked up and then dropped on the floor.""" + + def test_a_declared_publisher_is_returned(self) -> None: + """ + The lookup discarded its own result and fell off the end of the function, so + every EPUB reported no publisher no matter what it said. + """ + book = epub.EpubBook() + book.add_metadata("DC", "publisher", "No Starch Press") + + assert EpubExtractor._extract_publisher(book) == "No Starch Press" + + def test_no_publisher_is_none(self) -> None: + assert EpubExtractor._extract_publisher(epub.EpubBook()) is None diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index aa31a90..bda3c37 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -6,11 +6,45 @@ 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.schemas import BookCreate +from chitai.config import DuplicateScope, settings +from chitai.schemas import BookCreate, BooksCreateFromFiles from chitai.services import BookService +from chitai.services.book import DuplicateFilesError from chitai.database import models as m +DATA_FILES = Path("tests/data_files") +EPUB = DATA_FILES / "Metamorphosis - Franz Kafka.epub" +OTHER_EPUB = DATA_FILES / "The Art of War - Sun Tzu.epub" +PDF = DATA_FILES / "Calculus Made Easy - Silvanus Thompson.pdf" + + +def upload(path: Path, name: str | None = None) -> UploadFile: + """An uploaded file carrying the bytes of one of the test fixtures.""" + return UploadFile( + content_type="application/pdf" if path.suffix == ".pdf" else "application/epub+zip", + filename=name or path.name, + file_data=path.read_bytes(), + ) + + +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: @@ -202,3 +236,1067 @@ class TestBookServiceCRUD: for name, payload in contents.items(): entry = next(n for n in names if Path(n).name == name) assert archive.read(entry) == payload + + +@pytest.mark.asyncio +class TestBookServiceDuplicates: + """Files that are already stored must not be stored again.""" + + async def test_duplicate_upload_is_skipped_and_reported( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The second import of a file creates nothing and names where it already is.""" + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + assert len(first.books) == 1 + assert first.duplicates == [] + + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + assert second.books == [] + assert len(second.duplicates) == 1 + + duplicate = second.duplicates[0] + assert duplicate.filename == EPUB.name + assert duplicate.book_id == first.books[0].id + assert duplicate.book_title == first.books[0].title + + async def test_new_format_beside_a_duplicate_is_still_imported( + self, books_service: BookService, test_library: m.Library + ) -> None: + """A folder is not all-or-nothing: the file that is new must still land.""" + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB, "Metamorphosis/book.epub")]), + test_library, + ) + + result = await books_service.create_many_from_files( + BooksCreateFromFiles( + files=[ + upload(EPUB, "Metamorphosis/book.epub"), + upload(PDF, "Metamorphosis/book.pdf"), + ] + ), + test_library, + ) + + assert len(result.books) == 1 + assert [d.filename for d in result.duplicates] == ["Metamorphosis/book.epub"] + + book = await books_service.get(result.books[0].id) + assert [file.path for file in book.files] == ["book.pdf"] + + async def test_duplicate_within_one_upload_is_caught( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The same bytes twice in one request has no row to match against yet.""" + result = await books_service.create_many_from_files( + BooksCreateFromFiles( + files=[upload(EPUB, "first.epub"), upload(EPUB, "second.epub")] + ), + test_library, + ) + + assert len(result.books) == 1 + assert len(result.duplicates) == 1 + assert result.duplicates[0].filename == "second.epub" + + # Nothing in the database holds it yet, so there is no book to point at. + assert result.duplicates[0].book_id is None + + async def test_allow_duplicates_stores_the_file_anyway( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The escape hatch has to work: the hash is not proof of identity.""" + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), + test_library, + allow_duplicates=True, + ) + + assert len(result.books) == 1 + assert result.duplicates == [] + + async def test_a_different_file_is_not_a_duplicate( + self, books_service: BookService, test_library: m.Library + ) -> None: + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB), upload(OTHER_EPUB)]), test_library + ) + + assert len(result.books) == 2 + assert result.duplicates == [] + + async def test_matching_hash_with_a_different_size_is_not_a_duplicate( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The hash samples 12 KiB, so the size is what makes a match trustworthy.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + stored = (await books_service.get(created.books[0].id)).files[0] + + matches = await books_service.find_duplicate_files( + [(stored.hash, stored.size), (stored.hash, stored.size + 1)], test_library + ) + + assert (stored.hash, stored.size) in matches + assert (stored.hash, stored.size + 1) not in matches + + async def test_create_book_refuses_and_writes_nothing( + self, books_service: BookService, test_library: m.Library + ) -> None: + """A single-book create names its files, so it is refused rather than trimmed.""" + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + before = sorted(Path(test_library.root_path).rglob("*")) + + with pytest.raises(DuplicateFilesError) as excinfo: + await books_service.create_book( + BookCreate( + library_id=test_library.id, + title="Metamorphosis", + authors=["Franz Kafka"], + files=[upload(EPUB)], + ).model_dump(), + test_library, + ) + + assert len(excinfo.value.duplicates) == 1 + assert sorted(Path(test_library.root_path).rglob("*")) == before + + async def test_scope_decides_whether_libraries_share( + self, + books_service: BookService, + test_library: m.Library, + session: AsyncSession, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A second library is a separate collection by default, and not under `global`.""" + other = m.Library( + name="Second Library", + slug="second-library", + root_path=str(tmp_path / "second"), + path_template=test_library.path_template, + read_only=False, + ) + session.add(other) + await session.commit() + + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), other + ) + assert len(result.books) == 1 + assert result.duplicates == [] + + monkeypatch.setattr(settings, "duplicate_scope", DuplicateScope.GLOBAL) + + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), other + ) + assert result.books == [] + assert len(result.duplicates) == 1 + + async def test_scope_off_disables_detection( + self, + books_service: BookService, + test_library: m.Library, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setattr(settings, "duplicate_scope", DuplicateScope.OFF) + + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB), upload(EPUB, "copy.epub")]), + test_library, + ) + + assert len(result.books) == 2 + assert result.duplicates == [] + + async def test_re_adding_a_file_to_its_own_book_does_nothing( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Asking for a state that already holds is not a conflict.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + book_id = created.books[0].id + + await books_service.add_files(book_id, [upload(EPUB)], test_library) + + book = await books_service.get(book_id) + assert len(book.files) == 1 + + async def test_adding_another_books_file_is_refused( + self, books_service: BookService, test_library: m.Library + ) -> None: + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB), upload(OTHER_EPUB)]), test_library + ) + first, second = created.books + + with pytest.raises(DuplicateFilesError) as excinfo: + await books_service.add_files(first.id, [upload(OTHER_EPUB)], test_library) + + assert excinfo.value.duplicates[0].book_id == second.id + assert len((await books_service.get(first.id)).files) == 1 + + async def test_consume_duplicate_is_moved_aside( + self, + books_service: BookService, + test_library: m.Library, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The watcher cannot ask, so a refused file is parked rather than dropped.""" + quarantine = tmp_path / "duplicates" + monkeypatch.setattr(settings, "duplicate_path", str(quarantine)) + + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + consume = tmp_path / "consume" + await aios.makedirs(consume) + dropped = consume / EPUB.name + dropped.write_bytes(EPUB.read_bytes()) + + result = await books_service.create_many_from_existing_files( + [dropped], consume, test_library + ) + + assert result.books == [] + assert len(result.duplicates) == 1 + assert not dropped.exists() + assert (quarantine / test_library.slug / EPUB.name).is_file() + + async def test_consume_imports_what_is_new( + self, + books_service: BookService, + test_library: m.Library, + tmp_path: Path, + ) -> None: + """The screening must not disturb the ordinary consume import.""" + consume = tmp_path / "consume" + await aios.makedirs(consume / "Metamorphosis") + dropped = consume / "Metamorphosis" / EPUB.name + dropped.write_bytes(EPUB.read_bytes()) + + result = await books_service.create_many_from_existing_files( + [dropped], consume, test_library + ) + + assert len(result.books) == 1 + assert result.duplicates == [] + + book = await books_service.get(result.books[0].id) + assert (Path(book.path) / book.files[0].path).is_file() + + +@pytest.mark.asyncio +class TestBookPathCollisions: + """Two books must never share a directory, whatever their metadata says.""" + + async def test_forced_duplicate_gets_its_own_copy( + self, books_service: BookService, test_library: m.Library + ) -> None: + """`allow_duplicates` must add a book, not overwrite the one already there. + + The path comes from the metadata alone, so a forced duplicate generates the + same directory and the same filename. Writing it lands on top of the original: + one file on disk, two books pointing at it, and deleting either takes both. + """ + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), + test_library, + allow_duplicates=True, + ) + + original = await books_service.get(first.books[0].id) + forced = await books_service.get(second.books[0].id) + + assert original.path != forced.path + + paths = { + Path(book.path) / book.files[0].path for book in (original, forced) + } + assert len(paths) == 2 + assert all(path.is_file() for path in paths) + + async def test_deleting_a_forced_duplicate_keeps_the_original( + self, books_service: BookService, test_library: m.Library + ) -> None: + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), + test_library, + allow_duplicates=True, + ) + + original = await books_service.get(first.books[0].id) + kept = Path(original.path) / original.files[0].path + + await books_service.delete_books( + [second.books[0].id], test_library, delete_files=True + ) + + assert kept.is_file() + + async def test_editing_metadata_cannot_merge_into_another_book( + self, books_service: BookService, test_library: m.Library + ) -> None: + """A rename that collides must step aside rather than move in on top.""" + first = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + second = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(OTHER_EPUB)]), test_library + ) + + original = await books_service.get(first.books[0].id) + + # Renamed onto the first book's author and title. + await books_service.update_book( + second.books[0].id, + {"title": original.title, "authors": [author.name for author in original.authors]}, + test_library, + ) + + moved = await books_service.get(second.books[0].id) + + assert moved.path != original.path + assert (Path(original.path) / original.files[0].path).is_file() + assert (Path(moved.path) / moved.files[0].path).is_file() + + async def test_adding_a_file_lands_in_the_books_own_directory( + self, books_service: BookService, test_library: m.Library + ) -> None: + """`add_files` must follow the book, not the template.""" + await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + forced = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), + test_library, + allow_duplicates=True, + ) + book_id = forced.books[0].id + + await books_service.add_files(book_id, [upload(PDF)], test_library) + + book = await books_service.get(book_id) + assert len(book.files) == 2 + for file in book.files: + assert (Path(book.path) / file.path).is_file() + + +@pytest.mark.asyncio +class TestMissingFiles: + """A row whose bytes are gone must not stand in for the file itself.""" + + async def test_a_missing_file_is_not_a_duplicate( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Otherwise the library refuses to take back a file it can no longer open.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + book = await books_service.get(created.books[0].id) + (Path(book.path) / book.files[0].path).unlink() + + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + assert len(result.books) == 1 + assert result.duplicates == [] + + restored = await books_service.get(result.books[0].id) + assert (Path(restored.path) / restored.files[0].path).is_file() + + async def test_re_adding_a_missing_file_puts_it_back( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The book already has a row for it, so the bytes go back where it says.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + book_id = created.books[0].id + + book = await books_service.get(book_id) + path = Path(book.path) / book.files[0].path + path.unlink() + + await books_service.add_files(book_id, [upload(EPUB)], test_library) + + book = await books_service.get(book_id) + 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 TestAuthorNames: + """One person is one row, however the file happened to spell them.""" + + async def test_a_variant_spelling_reuses_the_existing_author( + self, books_service: BookService, test_library: m.Library + ) -> None: + """ + `as_unique_async` looks a name up before inserting it, so the lookup and the + insert have to tidy identically. If they disagree, every variant misses the + existing row and then collides with it on the unique index. + """ + first = await store_book( + books_service, test_library, title="Building Microservices", authors=["Sam Newman"] + ) + second = await store_book( + books_service, + test_library, + title="Monolith to Microservices", + authors=["Newman, Sam;"], + ) + + assert [author.name for author in first.authors] == ["Sam Newman"] + assert [author.name for author in second.authors] == ["Sam Newman"] + assert first.authors[0].id == second.authors[0].id + + @pytest.mark.parametrize( + ("written", "stored"), + [ + ("Newman, Sam;", "Sam Newman"), + ("Sam Newman.epub", "Sam Newman"), + (" Sam Newman ", "Sam Newman"), + # Two people in one string is left exactly as it was found. + ("Dave Thomas, Andy Hunt", "Dave Thomas, Andy Hunt"), + ], + ) + async def test_the_stored_name_is_the_tidy_one( + self, + books_service: BookService, + test_library: m.Library, + written: str, + stored: str, + ) -> None: + book = await store_book( + books_service, test_library, title="Some Book", authors=[written] + ) + + assert [author.name for author in book.authors] == [stored] + + async def test_an_upload_does_not_keep_the_file_extension( + self, books_service: BookService, test_library: m.Library + ) -> None: + """ + A filename is the fallback when the file declares no author of its own, and + "Franz Kafka.epub" is not a person. + """ + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB, "Some Unknown Book - Ada Lovelace.epub")]), + test_library, + ) + + book = await books_service.get(result.books[0].id) + for author in book.authors: + 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.""" + + 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) diff --git a/docs/duplicates-in-library-settings.md b/docs/duplicates-in-library-settings.md new file mode 100644 index 0000000..640053c --- /dev/null +++ b/docs/duplicates-in-library-settings.md @@ -0,0 +1,146 @@ +# Implementation brief: move Duplicates into library settings + +Written for an agent picking this up cold. Read the repo-root `AGENTS.md` and +`frontend/AGENTS.md` first — this brief assumes both. + +**This is a frontend-only change.** The backend already scopes everything by library +(`GET /books/duplicate-books?library_id=`), so no endpoint, schema or migration is +involved. + +## Where this starts from + +The duplicates review screen exists and works. It currently lives at + +``` +frontend/src/routes/(root)/(library)/library/[libraryId]/duplicates/ + +page.server.ts loads the groups, plus the full Book records merge needs + +page.svelte group cards, "Not duplicates", "Merge…" +``` + +and is reached from a **Duplicates entry in the main sidebar** +(`frontend/src/lib/components/layout/nav-main.svelte`), which is what this change +removes. + +Settings today is a flat, entirely global four-item nav +(`frontend/src/routes/(root)/settings/+layout.svelte`): Account, Appearance, Libraries, +Devices. `settings/libraries/+page.svelte` is a single table of every library whose rows +link *out* to the library itself. **There is nowhere that means "settings for this +library"** — that is the gap this change fills. + +## What to build — option B + +Libraries expands in the settings nav. Every library is a sub-item; selecting one swaps +the pane; Duplicates is a section inside that pane. All libraries and all their sections +end up one click apart. + +``` +/settings/libraries the existing table (leave it as the index) +/settings/libraries/[libraryId] redirects to the first section +/settings/libraries/[libraryId]/duplicates the review screen, moved +``` + +Suggested files: + +| Path | What | +| --- | --- | +| `settings/libraries/[libraryId]/+layout.svelte` | Library name, and the section tabs | +| `settings/libraries/[libraryId]/+page.ts` | `redirect(303, …/duplicates)` | +| `settings/libraries/[libraryId]/duplicates/+page.server.ts` | Moved verbatim | +| `settings/libraries/[libraryId]/duplicates/+page.svelte` | Moved verbatim | + +Duplicates is the **only** real section today. Build the tab strip so General and Danger +zone have somewhere obvious to land, but do not invent them now — an empty tab is worse +than no tab. + +## The nav + +In `settings/+layout.svelte`, `items` is a flat `as const` array matched on +`page.route.id`. Libraries needs to render its children beneath it: + +```svelte +{#each libraryState.libraries as library (library.id)} + … +{/each} +``` + +`getLibraryState()` **is** available under `/settings` — it is set in +`(root)/+layout.svelte`, above the settings group, and `settings/libraries/+page.svelte` +already uses it. No new load function is needed to list the libraries. + +**Active state is matched on route id, not pathname.** There is a comment in +`settings/+layout.svelte` explaining why: `resolve()` returns an absolute path on the +client and a relative one during SSR, so a pathname comparison is false on the server and +true after hydration, and the highlight flashes in. A nested library item is active when +the route id matches **and** `page.params.libraryId === String(library.id)` — both, or +every library lights up at once. + +## Things that will bite + +1. **Remove the sidebar entry in the same change.** `nav-main.svelte` gained a + `Duplicates` item and a `CopyCheck` import when the screen was built. Delete both, and + delete the old route directory. Doing the removal and the move together is the point — + split across two commits the screen is unreachable in between. + +2. **Delete the old route, do not leave it.** Two live copies of a screen that both write + is how they drift. + +3. **`setBookSelectionState` is not available under `/settings`.** It is set in + `(root)/(library)/+layout.svelte`, which the settings group is not inside. This is + fine — the duplicates page uses `BookImage` directly, not `book-thumbnail.svelte`, and + `MergeBooks` takes its `libraryId` as a prop. **Verify this stays true** if you touch + either component; a `getBookSelectionState()` under settings returns `undefined` and + fails at the first access, not at import. + +4. **Keep `depends('app:duplicate-books')`.** Both the dismiss action and `MergeBooks` + call `invalidate('app:duplicate-books')` to make a resolved group leave the screen. + Drop it and the page silently stops refreshing. `MergeBooks` also invalidates + `app:books`, which is a no-op under settings and should stay that way. + +5. **The settings shell is height-constrained.** `settings/+layout.svelte` is + `h-[calc(100vh-var(--header-height)-2rem)]` with `overflow-auto` on the content pane. + The review screen is a long list of cards — it must scroll *inside* that pane. Its + current `mx-auto max-w-5xl` wrapper will want revisiting. + +6. **Three levels of nav is option B's known cost.** Nav → library → section, and the + pane is narrower than the full-width route the screen was designed against. The group + cards are `w-36` covers in a wrapping flex row, so they reflow, but check a group of + four at a narrow window before calling it done. + +7. **`resolve()` must be a direct call in markup** for `svelte/no-navigation-without-resolve`. + Where `nav-main.svelte` computes a url through a variable it carries an + `eslint-disable-next-line`; prefer the direct call over inheriting that. + +8. **The loader depends on the `?ids=` fix.** `+page.server.ts` fetches full `Book` + records with `listBooks({ ids, pageSize })` because the merge workbench needs + identifiers, description and publisher, which `DuplicateBookRead` does not carry. + advanced_alchemy's stock id filter types that parameter as `list[str]` regardless of + config, which made Postgres refuse `bigint = character varying`; the override lives in + `backend/src/chitai/services/dependencies.py` (`create_book_filter_dependencies`). + If `GET /books?ids=1&ids=2` 500s, that override is missing — do not work around it in + the loader. + +## Out of scope + +The **General** and **Danger zone** sections (rename, path template, read-only, consume +directory, delete), and any change to the merge workbench itself. The toolbar entry point +for merge — select 2+ books in the library view — is unrelated and stays where it is. + +## Verification + +```bash +cd frontend +pnpm check # baseline: 30 errors, 1 warning, 8 files — none of them yours +pnpm lint # not clean either; check the files you touched, not the tree +pnpm build +``` + +By hand, with a library that has a duplicate group: + +- Settings → Libraries lists every library beneath it; clicking one opens its pane. +- Duplicates shows the same groups the old route did, and scrolls inside the settings pane. +- **Not duplicates** removes the group and it stays gone after a reload. +- **Merge…** opens the workbench, merges, and the group leaves the screen. +- The main sidebar no longer has a Duplicates entry, and + `/library//duplicates` no longer resolves. +- A library with no duplicates shows the empty state, not a blank pane. diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index caeb146..14a2897 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -157,10 +157,12 @@ but take a baseline first, because neither is clean (see below). Observed in the current tree — don't mistake these for intentional patterns to copy: -- `src/lib/schema/openapi/schema.d.ts` is **stale** — it predates the `BookProgress` rework and has - no `BookProgressRead`, so `book.progress` types as `{}`. It is the source of most of the errors - `pnpm check` reports; regenerating it should clear them. **Baseline as of 2026-08-11: 37 errors, - 1 warning, 11 files.** Get your own baseline before assuming an error is yours. +- `pnpm check` is not clean. **Baseline as of 2026-08-13: 30 errors, 1 warning, 8 files**, most of + them in `src/routes/api/[...path]/+server.ts` (see below). Get your own baseline before assuming + an error is yours. `src/lib/schema/openapi/schema.d.ts` was regenerated on that date and is + current; regenerate it again after any backend API change, with + `pnpm exec openapi-typescript http://localhost:8000/schema/openapi.json -o src/lib/schema/openapi/schema.d.ts` + against a backend running **your** branch — a stale server silently writes a stale file. - `pnpm lint` does not pass either — 131 pre-existing ESLint errors, 35 of them `svelte/no-navigation-without-resolve` on plain `href`s, plus ~59 files Prettier would rewrite (mostly vendored shadcn components). Check the files you touched, not the whole tree. diff --git a/frontend/src/lib/api/book.remote.ts b/frontend/src/lib/api/book.remote.ts index 5095162..841a335 100644 --- a/frontend/src/lib/api/book.remote.ts +++ b/frontend/src/lib/api/book.remote.ts @@ -8,12 +8,31 @@ import { deleteBooksSchema, editBookMetadataSchema, updateBookProgressSchema, + duplicateDismissalSchema, + bookMergeSchema, type Book, + type BooksUploadResult, + type DuplicateBookGroup, bookFilesUpload } from '$lib/schema/index'; import { stringCoerce, type PaginatedResponse } from '$lib/schema/common'; import { createQueryParams } from '$lib/utils'; +/** + * The backend's own message for a failed response, rather than its JSON envelope. + * + * A refused duplicate answers 409 with a `detail` worth reading and the offending + * files in `extra`; passing the body through whole puts JSON in front of the reader. + */ +function detailOf(body: string): string { + try { + const parsed = JSON.parse(body); + return typeof parsed?.detail === 'string' ? parsed.detail : body; + } catch { + return body; + } +} + export const getBook = query(stringCoerce, async (id): Promise => { const { locals } = getRequestEvent(); @@ -68,26 +87,29 @@ export const updateBookCover = form(bookCoverUpload, async ({ book_id, file }) = return await response.json(); }); -export const uploadBooks = form(booksUpload, async ({ library_id, files }) => { - const { locals } = getRequestEvent(); +export const uploadBooks = form( + booksUpload, + async ({ library_id, files }): Promise => { + const { locals } = getRequestEvent(); - const formData = new FormData(); - files.forEach((file) => { - formData.append('files', file); - }); + const formData = new FormData(); + files.forEach((file) => { + formData.append('files', file); + }); - const response = await locals.api.postMultipart( - `/books/fromFiles?library_id=${library_id}`, - formData - ); + const response = await locals.api.postMultipart( + `/books/fromFiles?library_id=${library_id}`, + formData + ); - if (!response.ok) { - const message = await response.text(); - error(response.status, message); + if (!response.ok) { + const message = await response.text(); + error(response.status, message); + } + + return await response.json(); } - - return await response.json(); -}); +); export const uploadBookFiles = form(bookFilesUpload, async ({ book_id, files }) => { const { locals } = getRequestEvent(); @@ -100,8 +122,9 @@ export const uploadBookFiles = form(bookFilesUpload, async ({ book_id, files }) const response = await locals.api.postMultipart(`/books/${book_id}/files`, formData); if (!response.ok) { - const message = await response.text(); - error(response.status, message); + // 409 here means the file is already stored under a different book, which is + // something the reader can act on — so the message has to survive the trip. + error(response.status, detailOf(await response.text())); } return await response.json(); @@ -133,6 +156,65 @@ export const deleteBookFiles = command(deleteBookFilesSchema, async ({ book_id, } }); +/** + * Books already in the library that look like copies of one another. + * + * Metadata only, so every group is a question rather than a verdict — which is why + * the screen it feeds offers a way to disagree. + */ +export const listDuplicateBooks = query( + stringCoerce, + async (libraryId): Promise => { + const { locals } = getRequestEvent(); + + const response = await locals.api.get(`/books/duplicate-books?library_id=${libraryId}`); + + if (!response.ok) error(response.status, detailOf(await response.text())); + + return await response.json(); + } +); + +/** + * Fold several books into one and delete the records folded in. + * + * Irreversible, so the caller is expected to have shown what is about to happen. + */ +export const mergeBooks = command( + bookMergeSchema, + async ({ library_id, ...data }): Promise => { + const { locals } = getRequestEvent(); + + const response = await locals.api.post(`/books/merge?library_id=${library_id}`, data); + + if (!response.ok) error(response.status, detailOf(await response.text())); + + return await response.json(); + } +); + +/** Record that two books are not the same book, so the pair stops being proposed. */ +export const dismissDuplicateBooks = command(duplicateDismissalSchema, async (data) => { + const { locals } = getRequestEvent(); + + const response = await locals.api.post('/books/duplicate-books/dismissals', data); + + if (!response.ok) error(response.status, detailOf(await response.text())); +}); + +/** Undo a dismissal, so the pair is proposed again. */ +export const restoreDuplicateBooks = command(duplicateDismissalSchema, async (data) => { + const { locals } = getRequestEvent(); + + const params = createQueryParams(data); + + const response = await locals.api.delete( + `/books/duplicate-books/dismissals?${params.toString()}` + ); + + if (!response.ok) error(response.status, detailOf(await response.text())); +}); + export const updateBookProgress = command( updateBookProgressSchema, async ({ book_ids, ...data }) => { diff --git a/frontend/src/lib/components/forms/edit-book/edit-files.svelte b/frontend/src/lib/components/forms/edit-book/edit-files.svelte index 31dbf95..c9a4c1a 100644 --- a/frontend/src/lib/components/forms/edit-book/edit-files.svelte +++ b/frontend/src/lib/components/forms/edit-book/edit-files.svelte @@ -50,6 +50,22 @@ toast.error(`${file.name} was not added`, { description: reason }); }; + /** + * The API's own words, when it has any. + * + * Adding a file the library already holds under another book is refused with a + * 409 naming it — far more use than "failed to add files". SvelteKit hands an + * `error()` back as an HttpError on the client, so the message sits on `body`. + */ + function apiMessage(error: unknown): string | undefined { + if (typeof error !== 'object' || error === null) return undefined; + + const body = (error as { body?: { message?: string } }).body; + if (typeof body?.message === 'string') return body.message; + + return error instanceof Error ? error.message : undefined; + } + function confirmDelete(file: BookFile) { fileToDelete = file; confirmOpen = true; @@ -148,7 +164,8 @@ toast.success('Files added'); } catch (error) { console.error('Failed to add files: ', error); - toast.error('Failed to add files'); + toast.error(apiMessage(error) ?? 'Failed to add files'); + uploadBookFiles.fields.files.set([]); } })} enctype="multipart/form-data" diff --git a/frontend/src/lib/components/forms/merge-books/field-spec.ts b/frontend/src/lib/components/forms/merge-books/field-spec.ts new file mode 100644 index 0000000..2714370 --- /dev/null +++ b/frontend/src/lib/components/forms/merge-books/field-spec.ts @@ -0,0 +1,149 @@ +import type { Book } from '$lib/schema'; + +/** + * What kind of thing a metadata field holds, which is the only thing that decides + * what you can do with it when two records disagree. + */ +export type FieldKind = 'text' | 'number' | 'date' | 'list' | 'keyed' | 'longtext'; + +/** An action offered on a field, beyond replacing it outright. */ +export type FieldAction = 'replace' | 'merge' | 'append'; + +export interface FieldSpec { + /** The key sent in `BookMetadataUpdate`. */ + key: string; + label: string; + kind: FieldKind; + group: string; + /** Extra actions past `replace`, which every field has. */ + extra: FieldAction[]; +} + +/** + * The fields a merge can resolve, in the order and grouping the edit form uses. + * + * Deliberately a spec rather than markup: the merge workbench and, later, the + * provider review screen both render from this, so a field cannot exist in one and + * not the other. `cover` and `files` are absent because they are not choices — + * files always come across and the cover has its own endpoint. + */ +export const MERGE_FIELDS: FieldSpec[] = [ + { key: 'title', label: 'Title', kind: 'text', group: 'Identity', extra: [] }, + { key: 'subtitle', label: 'Subtitle', kind: 'text', group: 'Identity', extra: [] }, + { key: 'edition', label: 'Edition', kind: 'number', group: 'Identity', extra: [] }, + { key: 'series', label: 'Series', kind: 'text', group: 'Identity', extra: [] }, + { key: 'series_position', label: 'No.', kind: 'text', group: 'Identity', extra: [] }, + { key: 'language', label: 'Language', kind: 'text', group: 'Identity', extra: [] }, + + // Order is meaningful for authors, so the second list is appended rather than + // interleaved; tags are a set, so they merge. + { + key: 'authors', + label: 'Authors', + kind: 'list', + group: 'People and subjects', + extra: ['append'] + }, + { key: 'tags', label: 'Tags', kind: 'list', group: 'People and subjects', extra: ['merge'] }, + + { key: 'publisher', label: 'Publisher', kind: 'text', group: 'Publication', extra: [] }, + { key: 'published_date', label: 'Published', kind: 'date', group: 'Publication', extra: [] }, + { key: 'pages', label: 'Pages', kind: 'number', group: 'Publication', extra: [] }, + { + key: 'identifiers', + label: 'Identifiers', + kind: 'keyed', + group: 'Publication', + extra: ['merge'] + }, + + { + key: 'description', + label: 'Description', + kind: 'longtext', + group: 'Description', + extra: ['append'] + } +]; + +/** A field's value, in the shape `BookMetadataUpdate` expects to receive it. */ +export type FieldValue = string | number | string[] | Record | null; + +/** Read one field off a book, flattening the relations the API returns as objects. */ +export function readField(book: Book, key: string): FieldValue { + switch (key) { + case 'authors': + return book.authors.map((author) => author.name); + case 'tags': + return book.tags.map((tag) => tag.name); + case 'publisher': + return book.publisher?.name ?? null; + case 'series': + return book.series?.title ?? null; + case 'identifiers': + return book.identifiers ?? {}; + default: + return (book as unknown as Record)[key] ?? null; + } +} + +/** Whether a field holds nothing, and so has no decision attached to it. */ +export function isEmpty(value: FieldValue): boolean { + if (value === null || value === undefined || value === '') return true; + if (Array.isArray(value)) return value.length === 0; + if (typeof value === 'object') return Object.keys(value).length === 0; + return false; +} + +/** Whether two field values say the same thing, order included for lists. */ +export function isSame(left: FieldValue, right: FieldValue): boolean { + if (isEmpty(left) && isEmpty(right)) return true; + return JSON.stringify(left ?? null) === JSON.stringify(right ?? null); +} + +/** + * Apply an action to a pair of values and return what the target becomes. + * + * `merge` on a keyed collection is per name and the target wins a clash, because + * `Identifier` is unique on `(name, book_id)` — a book cannot hold both its print + * and its ebook ISBN, so the second one has nowhere to go. + */ +export function applyAction( + action: FieldAction, + kind: FieldKind, + target: FieldValue, + incoming: FieldValue +): FieldValue { + if (action === 'replace') return incoming; + + if (kind === 'list') { + const current = Array.isArray(target) ? target : []; + const extra = Array.isArray(incoming) ? incoming : []; + // Order preserved, duplicates dropped — works for both append and merge. + return [...new Set([...current, ...extra])]; + } + + if (kind === 'keyed') { + return { ...(incoming as Record), ...(target as Record) }; + } + + if (kind === 'longtext') { + const current = typeof target === 'string' ? target.trim() : ''; + const extra = typeof incoming === 'string' ? incoming.trim() : ''; + return [current, extra].filter(Boolean).join('\n\n'); + } + + return incoming; +} + +/** How a value reads in the inert reference column. */ +export function displayValue(value: FieldValue): string { + if (isEmpty(value)) return ''; + if (Array.isArray(value)) return value.join(', '); + if (typeof value === 'object') { + return Object.entries(value as Record) + .map(([name, id]) => `${name}: ${id}`) + .join(' · '); + } + return String(value); +} diff --git a/frontend/src/lib/components/forms/merge-books/merge-books.svelte b/frontend/src/lib/components/forms/merge-books/merge-books.svelte new file mode 100644 index 0000000..b2b245b --- /dev/null +++ b/frontend/src/lib/components/forms/merge-books/merge-books.svelte @@ -0,0 +1,394 @@ + + +{#snippet cover(book: Book, size: string)} + + {#if book.cover_image} + + {:else} + + + {/if} + +{/snippet} + + +{#snippet reference(field: FieldSpec, book: Book)} + {@const value = readField(book, field.key)} +
+ {field.label} + {#if isEmpty(value)} + empty + {:else if field.kind === 'list'} + + {#each value as string[] as item (item)} + {item} + {/each} + + {:else} + {displayValue(value)} + {/if} +
+{/snippet} + + + + + + Merge {books.length} books + + + {candidates.length} + {candidates.length === 1 ? 'record is' : 'records are'} deleted. Their files move onto the book + you keep — nothing is removed from disk. + + + +
+ + + +
+ +
+ Keeping + {#each books as book (book.id)} + + {/each} + +
+ +
+
+ +
+ {#if !candidate} +

Nothing left to fold in.

+ {:else if shown.length === 0} +

+ These records agree on every field. Merging keeps + #{survivor.id} and moves the others' files + onto it. +

+ {:else} + {#each groups as group (group)} +

+ {group} +

+ + {#each shown.filter((field) => field.group === group) as field (field.key)} + {@const value = current(field)} + {@const incoming = readField(candidate, field.key)} + {@const edited = field.key in draft} +
+ {@render reference(field, candidate)} + + +
+ {#if edited} + + {/if} + + {#if !isSame(value, incoming)} + {#if isEmpty(value)} + + + {:else} + + {#each field.extra as action (action)} + + {/each} + {/if} + {/if} +
+ + +
+ + {field.label} + {#if edited} + taken + {/if} + + + {#if field.kind === 'longtext'} +