From 523117ec28a377d4bc8b090c1dda876c5f510b79 Mon Sep 17 00:00:00 2001 From: patrick Date: Sat, 15 Aug 2026 21:54:12 -0400 Subject: [PATCH] fix: keep the identifiers an EPUB declares DC:identifier was validated verbatim, so hyphenated and urn:isbn: forms never reached the checksum and every non-ISBN identifier was discarded. Normalise first, and name whatever survives. --- .../src/chitai/services/metadata_extractor.py | 90 +++++++++++++++++-- backend/tests/unit/test_matching.py | 43 +++++++++ 2 files changed, 127 insertions(+), 6 deletions(-) diff --git a/backend/src/chitai/services/metadata_extractor.py b/backend/src/chitai/services/metadata_extractor.py index eb6c77c..b48f15f 100644 --- a/backend/src/chitai/services/metadata_extractor.py +++ b/backend/src/chitai/services/metadata_extractor.py @@ -31,6 +31,76 @@ 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 + + class FileExtractor(Protocol): @classmethod async def extract_metadata( @@ -363,15 +433,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 diff --git a/backend/tests/unit/test_matching.py b/backend/tests/unit/test_matching.py index 1471380..55abe13 100644 --- a/backend/tests/unit/test_matching.py +++ b/backend/tests/unit/test_matching.py @@ -7,6 +7,7 @@ from chitai.services.matching import ( normalize_identifier, normalize_title, ) +from chitai.services.metadata_extractor import parse_identifier from chitai.services.utils import isbn10_to_isbn13 @@ -123,3 +124,45 @@ class TestIsbnConversion: @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