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..9e5a630 --- /dev/null +++ b/backend/migrations/versions/2026-08-15_canonicalize_author_names_49a9e85a0ffc.py @@ -0,0 +1,134 @@ +"""canonicalize author names + +Revision ID: 49a9e85a0ffc +Revises: ed41acf21270 +Create Date: 2026-08-15 15:59:47.331545 + +""" + +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 = '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/src/chitai/database/models/author.py b/backend/src/chitai/database/models/author.py index 1e2f111..4a2981c 100644 --- a/backend/src/chitai/database/models/author.py +++ b/backend/src/chitai/database/models/author.py @@ -17,35 +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 `_normalize_name` below — never assign it directly. Not unique: - # `Kafka, Franz` and `Franz Kafka` are two rows here and one person to a reader, - # which is exactly what this column exists to express. + # 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 _normalize_name(self, _key: str, name: str) -> str: - """Derive `normalized_name` from whatever writes the 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 normalize_author + 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/services/matching.py b/backend/src/chitai/services/matching.py index b0222b0..103b1f1 100644 --- a/backend/src/chitai/services/matching.py +++ b/backend/src/chitai/services/matching.py @@ -51,6 +51,13 @@ _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") @@ -103,6 +110,40 @@ def normalize_title(title: str | None) -> str: 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. diff --git a/backend/src/chitai/services/metadata_extractor.py b/backend/src/chitai/services/metadata_extractor.py index b48f15f..409b6da 100644 --- a/backend/src/chitai/services/metadata_extractor.py +++ b/backend/src/chitai/services/metadata_extractor.py @@ -567,7 +567,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/tests/unit/test_matching.py b/backend/tests/unit/test_matching.py index 7230359..9e52235 100644 --- a/backend/tests/unit/test_matching.py +++ b/backend/tests/unit/test_matching.py @@ -3,6 +3,7 @@ import pytest from chitai.services.matching import ( + format_author_name, normalize_author, normalize_identifier, normalize_title, @@ -92,6 +93,59 @@ class TestNormalizeAuthor: 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.""" diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index 35d2c45..535982c 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -895,6 +895,72 @@ class TestDuplicateBooks: 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 TestDuplicateBookGroups: """The pass over a library someone already has."""