diff --git a/CHANGELOG.md b/CHANGELOG.md index 52f1ea9..cef2fe1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,17 @@ All notable changes to Tome are documented here. Format loosely follows ## [Unreleased] +### Fixed +- **Chapter maps now extract from EPUB2 books.** Older EPUBs keep their table + of contents in an NCX file rather than an EPUB3 nav document; chapter + extraction ignored the NCX, so classics and older rips silently produced no + Time-per-chapter data. Extraction now falls back to the NCX whenever the nav + yields nothing — re-run Admin → Word Counts to pick up the affected books. +- **TOC-less EPUBs no longer re-queue in the Word Counts backfill forever.** + Books whose files genuinely have no usable table of contents were re-parsed + on every run because "done" was inferred from having chapter rows; a + dedicated attempt marker now records "tried, nothing there" once. + ### Added - **Sync closed books (KOReader plugin, build 34).** A new TomeSync menu action walks the device for books the plugin has never synced — read before diff --git a/backend/main.py b/backend/main.py index babd25e..8dd0113 100644 --- a/backend/main.py +++ b/backend/main.py @@ -73,6 +73,18 @@ async def lifespan(app: FastAPI): if "page_count" not in cols: conn.execute(text("ALTER TABLE books ADD COLUMN page_count INTEGER")) conn.commit() + # Chapter-extraction attempt marker (see Book.chapters_extracted_at). + # Distinguishes "never tried" from "tried, no usable TOC" so TOC-less + # EPUBs stop re-queuing in the backfill. Books that already have + # chapter rows are marked done, so upgraded installs don't re-extract + # their whole library once. + if "chapters_extracted_at" not in cols: + conn.execute(text("ALTER TABLE books ADD COLUMN chapters_extracted_at DATETIME")) + conn.execute(text( + "UPDATE books SET chapters_extracted_at = CURRENT_TIMESTAMP " + "WHERE id IN (SELECT DISTINCT book_id FROM book_chapters)" + )) + conn.commit() user_cols = {r[1] for r in conn.execute(text("PRAGMA table_info(users)")).fetchall()} if "role" not in user_cols: conn.execute(text("ALTER TABLE users ADD COLUMN role VARCHAR(16) NOT NULL DEFAULT 'guest'")) diff --git a/backend/models/book.py b/backend/models/book.py index ee4d2dc..b8a9b8a 100644 --- a/backend/models/book.py +++ b/backend/models/book.py @@ -33,6 +33,11 @@ class Book(Base): # and must never be stored as a book property (KOReader page-stats carry # their own total_pages per row for that). page_count: Mapped[Optional[int]] = mapped_column(Integer) + # When chapter extraction last RAN for this book (found chapters or not). + # Distinguishes "never tried" from "tried, no usable TOC" — without it, a + # TOC-less EPUB re-queues in the backfill forever, since pending can't be + # inferred from BookChapter rows it will never have. + chapters_extracted_at: Mapped[Optional[datetime]] = mapped_column(DateTime) # Hardcover identity — book-level because a title's Hardcover book/edition is # user-independent. Matched lazily by the sync worker (ISBN first, strict # title+author search as fallback). match_method "none" + matched_at set = diff --git a/backend/services/chapters.py b/backend/services/chapters.py index 4678448..6f9ff4f 100644 --- a/backend/services/chapters.py +++ b/backend/services/chapters.py @@ -1,18 +1,31 @@ """Persist a book's chapter map (see BookChapter for the anchor design).""" import logging +from datetime import datetime from sqlalchemy.orm import Session -from backend.models.book import BookChapter +from backend.models.book import Book, BookChapter logger = logging.getLogger(__name__) def replace_book_chapters(db: Session, book_id: int, chapters: list[dict] | None) -> int: - """Replace the stored chapter map for a book with the given extraction - result (as produced by metadata._extract_epub_chapters). An empty/None - extraction clears nothing — no TOC today doesn't invalidate a map extracted - from the same file yesterday. Returns the number of rows written.""" + """Persist a chapter-extraction RESULT for a book. + + - ``None`` → extraction never ran (non-EPUB callers): nothing happens. + - ``[]`` → extraction ran and found no usable TOC: the attempt is + stamped (``chapters_extracted_at``) so the backfill stops re-queuing the + book, but existing rows are left alone — a missing TOC today doesn't + invalidate a map extracted from the same file yesterday. + - ``[...]`` → rows are replaced and the attempt stamped. + + Returns the number of chapter rows written. + """ + if chapters is None: + return 0 + book = db.get(Book, book_id) + if book is not None: + book.chapters_extracted_at = datetime.utcnow() if not chapters: return 0 db.query(BookChapter).filter(BookChapter.book_id == book_id).delete() diff --git a/backend/services/metadata.py b/backend/services/metadata.py index cec9fed..0cf8885 100644 --- a/backend/services/metadata.py +++ b/backend/services/metadata.py @@ -103,42 +103,32 @@ def count_words_epub(path: Path) -> Optional[int]: return _count_words_from_zip(path) -def _extract_epub_chapters(book) -> list[dict]: - """TOC → device-independent chapter boundaries as fraction-of-book. - - Fractions come from cumulative word offsets of spine items: a chapter whose - TOC entry points into spine item k starts at words(items 0..k-1) / total. - Fragment anchors inside a file are ignored — top-level TOC entries almost - always sit at a file boundary, and per-file granularity is what the - time-per-chapter stat needs. Returns [] when there's no usable structure - (no TOC, unresolvable hrefs, or fewer than two distinct chapters). +def _build_chapter_map( + doc_names: list[str], + counts: list[int], + toc_entries: list[tuple[str, str]], +) -> list[dict]: + """Shared fraction math: ordered spine document names + their word counts + + TOC entries as (title, href) → chapter boundaries as fraction-of-book. + + A chapter whose TOC entry points into spine item k starts at + words(items 0..k-1) / total. Fragment anchors are ignored — top-level TOC + entries almost always sit at a file boundary, and per-file granularity is + what the time-per-chapter stat needs. Returns [] when there's no usable + structure (no TOC, unresolvable hrefs, fewer than two distinct chapters). """ - import ebooklib + from urllib.parse import unquote - id_to_item = {item.get_id(): item for item in book.get_items_of_type(ebooklib.ITEM_DOCUMENT)} - spine_ids = [sid for sid, _linear in book.spine if sid in id_to_item] - if not spine_ids: - return [] - - counts: list[int] = [] - href_to_idx: dict[str, int] = {} - for i, sid in enumerate(spine_ids): - item = id_to_item[sid] - href_to_idx[item.get_name()] = i - try: - raw = item.get_content() - counts.append(count_words_text(_html_to_text(raw.decode("utf-8", "ignore")))) - except Exception: # noqa: BLE001 — unreadable spine item contributes no words - counts.append(0) total = sum(counts) if total <= 0: return [] cum = [0] for c in counts: cum.append(cum[-1] + c) + href_to_idx = {name: i for i, name in enumerate(doc_names)} def resolve(href: str) -> Optional[int]: - href = href.split("#", 1)[0] + href = unquote(href.split("#", 1)[0]) if href in href_to_idx: return href_to_idx[href] # Tolerate OPF-dir path differences by unique basename. @@ -147,24 +137,12 @@ def resolve(href: str) -> Optional[int]: return hits[0] if len(hits) == 1 else None chapters: list[dict] = [] - for node in book.toc or []: - # Entries are Link or (Section, [children]); a Section may carry its own - # href, otherwise its first child anchors it. Only the top level counts — - # nested subsections are noise for a per-chapter time split. - head = node[0] if isinstance(node, tuple) else node - href = getattr(head, "href", None) - if not href and isinstance(node, tuple) and node[1]: - first = node[1][0] - first = first[0] if isinstance(first, tuple) else first - href = getattr(first, "href", None) - if not href: - continue + for title, href in toc_entries: idx = resolve(href) if idx is None: continue - title = (getattr(head, "title", None) or "").strip() chapters.append({ - "title": title or f"Chapter {len(chapters) + 1}", + "title": (title or "").strip() or f"Chapter {len(chapters) + 1}", "start_fraction": cum[idx] / total, }) @@ -187,6 +165,176 @@ def resolve(href: str) -> Optional[int]: return out +def _flatten_ebooklib_toc(raw) -> list[tuple[str, str]]: + """Normalize ebooklib's book.toc into (title, href) top-level entries. + + Real-world shapes seen: a proper list of Link/(Section, children) nodes; a + BARE Link (single-entry nav); a single (Section, children) tuple; and a + lone wrapping Section whose children are the actual chapters — unwrap that + one level, or every such book collapses to "one chapter" and is dropped. + """ + def is_node_tuple(n) -> bool: + return isinstance(n, tuple) and len(n) >= 2 and isinstance(n[1], (list, tuple)) + + if raw is None: + nodes = [] + elif isinstance(raw, list): + nodes = raw + elif is_node_tuple(raw): + nodes = [raw] + elif isinstance(raw, tuple): + nodes = list(raw) + else: + nodes = [raw] # bare Link + + # Unwrap a single all-enclosing Section (or a single href-less entry with + # children): its children are the real chapter list. + while len(nodes) == 1 and is_node_tuple(nodes[0]): + nodes = list(nodes[0][1]) + + entries: list[tuple[str, str]] = [] + for node in nodes: + # Entries are Link or (Section, [children]); a Section may carry its + # own href, otherwise its first child anchors it. Only the top level + # counts — nested subsections are noise for a per-chapter time split. + head = node[0] if isinstance(node, tuple) else node + href = getattr(head, "href", None) + if not href and is_node_tuple(node) and node[1]: + first = node[1][0] + first = first[0] if isinstance(first, tuple) else first + href = getattr(first, "href", None) + if href: + entries.append((getattr(head, "title", None) or "", href)) + return entries + + +def _extract_epub_chapters(book) -> list[dict]: + """TOC → chapter boundaries, from an already-open ebooklib book.""" + import ebooklib + + id_to_item = {item.get_id(): item for item in book.get_items_of_type(ebooklib.ITEM_DOCUMENT)} + spine_ids = [sid for sid, _linear in book.spine if sid in id_to_item] + if not spine_ids: + return [] + + doc_names: list[str] = [] + counts: list[int] = [] + for sid in spine_ids: + item = id_to_item[sid] + doc_names.append(item.get_name()) + try: + raw = item.get_content() + counts.append(count_words_text(_html_to_text(raw.decode("utf-8", "ignore")))) + except Exception: # noqa: BLE001 — unreadable spine item contributes no words + counts.append(0) + + return _build_chapter_map(doc_names, counts, _flatten_ebooklib_toc(book.toc)) + + +def _chapters_from_zip(path: Path) -> list[dict]: + """Chapter extraction straight from the EPUB zip, for the files ebooklib + mangles or refuses outright (observed live: an EPUB3 nav without an
    + crashes read_epub; another file's empty nav shadows a perfectly good NCX). + + Parses container.xml → OPF → spine order + word counts per document, then + takes TOC entries from the NCX (navPoints) or, failing that, the EPUB3 nav + document's toc list. Same fraction math as the ebooklib path. + """ + import posixpath + import xml.etree.ElementTree as ET + + NS = { + "cnt": "urn:oasis:names:tc:opendocument:xmlns:container", + "opf": "http://www.idpf.org/2007/opf", + "ncx": "http://www.daisy.org/z3986/2005/ncx/", + "xhtml": "http://www.w3.org/1999/xhtml", + "ops": "http://www.idpf.org/2007/ops", + } + try: + with zipfile.ZipFile(path) as zf: + container = ET.fromstring(zf.read("META-INF/container.xml")) + rootfile = container.find(".//cnt:rootfile", NS) + if rootfile is None: + return [] + opf_path = rootfile.get("full-path", "") + opf_dir = posixpath.dirname(opf_path) + opf = ET.fromstring(zf.read(opf_path)) + + def from_opf_dir(href: str) -> str: + return posixpath.normpath(posixpath.join(opf_dir, href)) if opf_dir else href + + manifest: dict[str, tuple[str, str]] = {} # id -> (href, media-type) + for item in opf.findall(".//opf:manifest/opf:item", NS): + manifest[item.get("id", "")] = (item.get("href", ""), item.get("media-type", "")) + + spine = opf.find(".//opf:spine", NS) + if spine is None: + return [] + doc_names: list[str] = [] + counts: list[int] = [] + for ref in spine.findall("opf:itemref", NS): + href, media = manifest.get(ref.get("idref", ""), ("", "")) + if not href or "xml" not in media and "html" not in media: + continue + member = from_opf_dir(href) + doc_names.append(href) # TOC hrefs are OPF-relative, like these + try: + counts.append(count_words_text( + _html_to_text(zf.read(member).decode("utf-8", "ignore")))) + except Exception: # noqa: BLE001 + counts.append(0) + + # TOC source 1: the NCX (spine@toc, or any manifest ncx item) + toc_entries: list[tuple[str, str]] = [] + ncx_id = spine.get("toc") + ncx_href = None + if ncx_id and ncx_id in manifest: + ncx_href = manifest[ncx_id][0] + else: + for href, media in manifest.values(): + if media == "application/x-dtbncx+xml": + ncx_href = href + break + if ncx_href: + try: + ncx = ET.fromstring(zf.read(from_opf_dir(ncx_href))) + for np in ncx.findall("./ncx:navMap/ncx:navPoint", NS): + label = np.find("./ncx:navLabel/ncx:text", NS) + content = np.find("./ncx:content", NS) + if content is not None and content.get("src"): + toc_entries.append(( + (label.text or "") if label is not None else "", + content.get("src", ""), + )) + except Exception: # noqa: BLE001 — fall through to the nav + toc_entries = [] + + # TOC source 2: the EPUB3 nav document + if not toc_entries: + nav_href = None + for item in opf.findall(".//opf:manifest/opf:item", NS): + if "nav" in (item.get("properties") or "").split(): + nav_href = item.get("href") + break + if nav_href: + try: + nav = ET.fromstring(zf.read(from_opf_dir(nav_href))) + nav_dir = posixpath.dirname(nav_href) + for a in nav.findall(".//xhtml:nav//xhtml:li/xhtml:a", NS): + href = a.get("href") + if href: + # nav hrefs are nav-relative; rebase to OPF-relative + rebased = posixpath.normpath(posixpath.join(nav_dir, href)) if nav_dir else href + toc_entries.append(("".join(a.itertext()), rebased)) + except Exception: # noqa: BLE001 + toc_entries = [] + + return _build_chapter_map(doc_names, counts, toc_entries) + except Exception as e: # noqa: BLE001 — no chapters is always a valid outcome + logger.info("zip-level chapter extraction failed for %s: %s", path, e) + return [] + + def count_pages_fixed_layout(path: Path) -> Optional[int]: """Intrinsic page count for fixed-layout formats (PDF/CBZ/CBR), without doing any of the cover/metadata work extract_metadata does. None for @@ -216,15 +364,24 @@ def count_pages_fixed_layout(path: Path) -> Optional[int]: def extract_chapters_epub(path: Path) -> list[dict]: """Open an EPUB from disk and extract its chapter map. Used by the backfill - job; ingest reuses the already-open book via _extract_epub_chapters.""" + job and as the ingest fallback. + + ebooklib first (fast, and consistent with the rest of this module); when + it yields nothing — or refuses the file entirely — the zip-level parser + takes over. That second path is what rescues EPUB2 books whose TOC lives + only in the NCX, files whose empty nav shadows a good NCX, and the + malformed-nav EPUBs read_epub crashes on.""" + chapters: list[dict] = [] try: from ebooklib import epub book = epub.read_epub(str(path), options={"ignore_ncx": True}) - return _extract_epub_chapters(book) - except Exception as e: # noqa: BLE001 — no chapters is always a valid outcome - logger.info("chapter extraction failed for %s: %s", path, e) - return [] + chapters = _extract_epub_chapters(book) + except Exception as e: # noqa: BLE001 — the zip path gets its turn + logger.info("ebooklib chapter extraction failed for %s: %s", path, e) + if chapters: + return chapters + return _chapters_from_zip(path) def _opf_meta_by_name(book, name: str) -> Optional[str]: @@ -386,8 +543,14 @@ def _first(items): # key: creation sites persist it as BookChapter rows; API previews # strip underscore keys. chapters = _extract_epub_chapters(book) - if chapters: - meta["_chapters"] = chapters + if not chapters: + # The already-open book gave nothing — let the zip-level parser + # have a go (NCX-only EPUB2, empty/malformed nav — see + # extract_chapters_epub). + chapters = _chapters_from_zip(path) + # Always set for EPUBs (possibly []) — the empty list records "tried, + # no usable TOC" downstream so the backfill stops re-queuing the book. + meta["_chapters"] = chapters # Cover extraction cover_id = None diff --git a/backend/services/word_count_job.py b/backend/services/word_count_job.py index 7ab9df6..94a8d0e 100644 --- a/backend/services/word_count_job.py +++ b/backend/services/word_count_job.py @@ -32,7 +32,7 @@ from sqlalchemy import func from backend.core.database import SessionLocal -from backend.models.book import Book, BookChapter, BookFile +from backend.models.book import Book, BookFile from backend.services.chapters import replace_book_chapters from backend.services.metadata import ( count_pages_fixed_layout, @@ -58,13 +58,18 @@ class WordCountAlreadyRunning(Exception): def _epub_books() -> dict[int, tuple[Optional[int], int, str, bool]]: """One row per book that has an EPUB file: - book_id → (word_count, size, path, has_chapters). - If a book has several EPUB files we keep the first.""" + book_id → (word_count, size, path, chapters_checked). + If a book has several EPUB files we keep the first. + + chapters_checked comes from the attempt marker, NOT from having chapter + rows — a TOC-less EPUB never gains rows and would otherwise re-queue on + every run forever.""" with SessionLocal() as db: rows = ( db.query( Book.id, Book.word_count, + Book.chapters_extracted_at, BookFile.file_size, BookFile.file_path, ) @@ -72,13 +77,10 @@ def _epub_books() -> dict[int, tuple[Optional[int], int, str, bool]]: .filter(func.lower(BookFile.format) == "epub") .all() ) - chaptered = { - r[0] for r in db.query(BookChapter.book_id).distinct().all() - } out: dict[int, tuple[Optional[int], int, str, bool]] = {} - for book_id, wc, size, path in rows: + for book_id, wc, checked_at, size, path in rows: if book_id not in out: - out[book_id] = (wc, size or 0, path, book_id in chaptered) + out[book_id] = (wc, size or 0, path, checked_at is not None) return out diff --git a/tests/test_book_chapters.py b/tests/test_book_chapters.py index cea2afa..cd26f7b 100644 --- a/tests/test_book_chapters.py +++ b/tests/test_book_chapters.py @@ -213,3 +213,113 @@ def test_endpoint_carries_chapters_block(self, db, client, admin_user, make_book assert r.status_code == 200, r.text chapters = r.json()["chapters"] assert chapters[0]["seconds"] == 120 and chapters[1]["seconds"] == 0 + + +# ── EPUB2 / NCX-only fallback + the re-queue marker ────────────────────────── + +def _make_epub2_ncx_only(path: Path, chapters: list[tuple[str, str]]) -> None: + """A hand-built EPUB2: TOC lives ONLY in the NCX (no EPUB3 nav document) — + the Tolkien-shaped case the nav-only extraction missed.""" + manifest_items, spine_refs, navpoints, files = [], [], [], [] + for i, (title, txt) in enumerate(chapters): + fn = f"c{i}.xhtml" + manifest_items.append(f'') + spine_refs.append(f'') + navpoints.append( + f'{title}' + f'' + ) + files.append((f"OEBPS/{fn}", + f'' + f'{title}

    {title}

    {txt}

    ')) + opf = f''' + + + epub2-ncx-test + NCX Onlyen + + {''.join(manifest_items)} + + + {''.join(spine_refs)} +''' + ncx = f''' + + + NCX Only + {''.join(navpoints)} +''' + with zipfile.ZipFile(path, "w") as z: + z.writestr("mimetype", "application/epub+zip", compress_type=zipfile.ZIP_STORED) + z.writestr("META-INF/container.xml", + '' + '' + '') + z.writestr("OEBPS/content.opf", opf) + z.writestr("OEBPS/toc.ncx", ncx) + for name, content in files: + z.writestr(name, content) + + +class TestNcxFallback: + def test_epub2_ncx_only_toc_extracts(self, tmp_path): + path = tmp_path / "epub2.epub" + _make_epub2_ncx_only(path, [("Chapter I", "alpha " * 100), ("Chapter II", "beta " * 100)]) + chapters = extract_chapters_epub(path) + assert [c["title"] for c in chapters] == ["Chapter I", "Chapter II"] + assert chapters[0]["start_fraction"] == 0.0 + assert chapters[-1]["end_fraction"] == 1.0 + + def test_ingest_meta_carries_empty_list_for_toc_less_epub(self, tmp_path): + # A single-chapter book has no usable structure — the meta must still + # say "tried" ([]), not stay silent (absent key), or the backfill + # re-queues it forever. + path = tmp_path / "b.epub" + _make_epub(path, [("Only", "some words here")]) + meta = extract_metadata(path, tmp_path) + assert meta["_chapters"] == [] + + +class TestExtractionMarker: + def test_replace_semantics(self, db, make_book): + from backend.services.chapters import replace_book_chapters + book = make_book(title="Marked") + + # None → untouched (non-EPUB caller) + replace_book_chapters(db, book.id, None) + db.flush() + assert book.chapters_extracted_at is None + + # [] → attempt stamped, no rows + replace_book_chapters(db, book.id, []) + db.flush() + assert book.chapters_extracted_at is not None + assert db.query(BookChapter).filter_by(book_id=book.id).count() == 0 + + # [...] → rows written; a later [] keeps them (stale-TOC protection) + replace_book_chapters(db, book.id, [ + {"idx": 0, "title": "One", "start_fraction": 0.0, "end_fraction": 1.0}, + ]) + db.flush() + assert db.query(BookChapter).filter_by(book_id=book.id).count() == 1 + replace_book_chapters(db, book.id, []) + db.flush() + assert db.query(BookChapter).filter_by(book_id=book.id).count() == 1 + + def test_backfill_pending_predicate(self, db, make_book): + from backend.services.chapters import replace_book_chapters + # The job pends a book iff words are missing OR it was never + # chapter-checked — a checked TOC-less book must NOT pend again. + checked = make_book(title="NoTocChecked", file_format="epub") + checked.word_count = 1000 + replace_book_chapters(db, checked.id, []) + fresh = make_book(title="FreshEpub", file_format="epub") + fresh.word_count = 1000 + db.commit() + + def pending(b): + return b.word_count is None or b.chapters_extracted_at is None + + assert pending(checked) is False + assert pending(fresh) is True