feat: canonicalise author names
Extractors wrote whatever the file said, so one person held several rows: "Sam Newman" beside "Newman, Sam;" beside "Sam Newman.epub", the last because the upload path never stripped the file extension. Tidy on write, in the validator and in the uniqueness lookup alike, and merge the rows that collide.
This commit is contained in:
@@ -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.
|
||||||
|
"""
|
||||||
@@ -17,35 +17,55 @@ if TYPE_CHECKING:
|
|||||||
class Author(BigIntAuditBase, UniqueMixin):
|
class Author(BigIntAuditBase, UniqueMixin):
|
||||||
__tablename__ = "authors"
|
__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)
|
name: Mapped[str] = mapped_column(unique=True, index=True)
|
||||||
|
|
||||||
# Kept current by `_normalize_name` below — never assign it directly. Not unique:
|
# Kept current by `_canonicalize` too — never assign it directly. Not unique: two
|
||||||
# `Kafka, Franz` and `Franz Kafka` are two rows here and one person to a reader,
|
# spellings that survive canonicalization, "Steve Mcconnell" and "Steve McConnell",
|
||||||
# which is exactly what this column exists to express.
|
# are still one person to a reader, which is what this column exists to express.
|
||||||
normalized_name: Mapped[str] = mapped_column(default="", index=True)
|
normalized_name: Mapped[str] = mapped_column(default="", index=True)
|
||||||
|
|
||||||
description: Mapped[Optional[str]]
|
description: Mapped[Optional[str]]
|
||||||
|
|
||||||
@validates("name")
|
@validates("name")
|
||||||
def _normalize_name(self, _key: str, name: str) -> str:
|
def _canonicalize(self, _key: str, name: str) -> str:
|
||||||
"""Derive `normalized_name` from whatever writes the name."""
|
"""
|
||||||
|
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
|
# Imported here rather than at module scope: `chitai.services.matching` cannot
|
||||||
# be reached without initialising the `chitai.services` package, which imports
|
# be reached without initialising the `chitai.services` package, which imports
|
||||||
# the services, which import this module.
|
# 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)
|
self.normalized_name = normalize_author(name)
|
||||||
return 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
|
@classmethod
|
||||||
def unique_hash(cls, name: str) -> Hashable:
|
def unique_hash(cls, name: str) -> Hashable:
|
||||||
"""Generate a unique hash for deduplication."""
|
"""Generate a unique hash for deduplication."""
|
||||||
return name
|
return cls._tidy(name)
|
||||||
|
|
||||||
@classmethod
|
@classmethod
|
||||||
def unique_filter(cls, name: str) -> ColumnElement[bool]:
|
def unique_filter(cls, name: str) -> ColumnElement[bool]:
|
||||||
"""SQL filter for finding existing records."""
|
"""SQL filter for finding existing records."""
|
||||||
return cls.name == name
|
return cls.name == cls._tidy(name)
|
||||||
|
|
||||||
def __repr__(self) -> str:
|
def __repr__(self) -> str:
|
||||||
return f"Author({self.name!r})"
|
return f"Author({self.name!r})"
|
||||||
|
|||||||
@@ -51,6 +51,13 @@ _PUNCTUATION = re.compile(r"[^0-9a-z ]+")
|
|||||||
|
|
||||||
_WHITESPACE = re.compile(r"\s+")
|
_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.
|
# `J. R. R.` survives punctuation stripping as three one-letter words; `J.R.R.` as one.
|
||||||
# Joining any run of them makes both `jrr`.
|
# Joining any run of them makes both `jrr`.
|
||||||
_INITIAL_RUN = re.compile(r"\b(?:[a-z] )+[a-z]\b")
|
_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
|
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:
|
def normalize_author(name: str | None) -> str:
|
||||||
"""
|
"""
|
||||||
Reduce an author's name to the key their other books should share.
|
Reduce an author's name to the key their other books should share.
|
||||||
|
|||||||
@@ -567,7 +567,11 @@ class FilenameExtractor(FileExtractor):
|
|||||||
elif isinstance(input, Path):
|
elif isinstance(input, Path):
|
||||||
filename = get_filename(input, ext=False)
|
filename = get_filename(input, ext=False)
|
||||||
elif isinstance(input, UploadFile):
|
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:
|
else:
|
||||||
raise ValueError("Input type not supported")
|
raise ValueError("Input type not supported")
|
||||||
|
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from chitai.services.matching import (
|
from chitai.services.matching import (
|
||||||
|
format_author_name,
|
||||||
normalize_author,
|
normalize_author,
|
||||||
normalize_identifier,
|
normalize_identifier,
|
||||||
normalize_title,
|
normalize_title,
|
||||||
@@ -92,6 +93,59 @@ class TestNormalizeAuthor:
|
|||||||
assert normalize_author("Smith, John, Jr.") == "smith john jr"
|
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:
|
class TestNormalizeIdentifier:
|
||||||
"""Identifiers only help if the same edition produces the same key."""
|
"""Identifiers only help if the same edition produces the same key."""
|
||||||
|
|
||||||
|
|||||||
@@ -895,6 +895,72 @@ class TestDuplicateBooks:
|
|||||||
assert "title-author" in possible.candidates[0].matched_on
|
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
|
@pytest.mark.asyncio
|
||||||
class TestDuplicateBookGroups:
|
class TestDuplicateBookGroups:
|
||||||
"""The pass over a library someone already has."""
|
"""The pass over a library someone already has."""
|
||||||
|
|||||||
Reference in New Issue
Block a user