diff --git a/bindery/orchestrator.py b/bindery/orchestrator.py index 3e8f219..6b5c0ef 100644 --- a/bindery/orchestrator.py +++ b/bindery/orchestrator.py @@ -530,17 +530,20 @@ def set_chapters_included(story_id: int, chapter_ids: list[int], included: bool) def move_chapters(story_id: int, chapter_ids: list[int], after_id: int | None) -> dict[str, Any]: """Move chapters, in their current order, to directly after the chapter `after_id`, or to the start with None.""" - order = [c["id"] for c in store.list_chapters(story_id)] - moving = [cid for cid in order if cid in set(chapter_ids)] - if not moving: - raise ValueError("Select the chapters to move") - if after_id is not None and after_id not in order: - raise KeyError(f"No chapter {after_id} in this story") - if after_id in moving: - raise ValueError("The chapters cannot go after a chapter that moves with them") - rest = [cid for cid in order if cid not in moving] - at = rest.index(after_id) + 1 if after_id is not None else 0 - store.set_positions(story_id, rest[:at] + moving + rest[at:]) + + def arrange(order: list[int]) -> list[int]: + moving = [cid for cid in order if cid in set(chapter_ids)] + if not moving: + raise ValueError("Select the chapters to move") + if after_id is not None and after_id not in order: + raise KeyError(f"No chapter {after_id} in this story") + if after_id in moving: + raise ValueError("The chapters cannot go after a chapter that moves with them") + rest = [cid for cid in order if cid not in moving] + at = rest.index(after_id) + 1 if after_id is not None else 0 + return rest[:at] + moving + rest[at:] + + store.reorder_chapters(story_id, arrange) return queue_story(story_id, DownloadMode.REBUILD.value) @@ -940,8 +943,9 @@ def _import_epub(story_id: int, contents: fff.EpubContents) -> int: for img in contents.images: store.put_image(story_id, img["url"], img["ext"], img["mime"], img["data"]) if new: - order = merge_chapter_lists(list(stored), epub_order, newest_last=False) - store.add_chapters(story_id, new, "import", [stored.get(u, u) for u in order]) + # The store keeps a chapter under the URL that it had at its download. + in_epub = [stored.get(u, u) for u in epub_order] + store.add_chapters(story_id, new, "import", lambda have: merge_chapter_lists(have, in_epub, newest_last=False)) return len(new) @@ -1180,7 +1184,6 @@ def _run_site_job(job: Job) -> str: total = len(new_urls) + len(refetch) done = 0 failed: list[str] = [] - order = merge_chapter_lists([c["url"] for c in chapters], list(site_titles)) stored_images = store.image_urls(story_id) def store_images() -> None: @@ -1205,8 +1208,8 @@ def _run_site_job(job: Job) -> str: continue store_images() chapter = {"url": url, "title": site_titles[url], "html": html, "text_hash": text_hash(html)} - by_url[url] = chapter - store.add_chapters(story_id, [chapter], mode.value, [u for u in order if u in by_url]) + # The chapter goes in the order that the story has now. A move or a delete by hand during this job stays. + store.add_chapters(story_id, [chapter], mode.value, lambda have: merge_chapter_lists(have, list(site_titles))) new_count += 1 save_progress() @@ -1220,6 +1223,10 @@ def _run_site_job(job: Job) -> str: done += 1 continue store_images() + revisions = store.list_revisions(c["id"]) + if not revisions: # the user deleted the chapter during this job + done += 1 + continue if c["id"] in renamed: c["title"] = renamed[c["id"]] store.update_chapter(c["id"], title=c["title"]) @@ -1227,7 +1234,7 @@ def _run_site_job(job: Job) -> str: new_hash = text_hash(html) # The site changed when its text differs from the text that it sent last. Until then, the book keeps # a revision that the user chose. - newest = store.list_revisions(c["id"])[-1] + newest = revisions[-1] if new_hash != newest["text_hash"]: if _rules_explain_change(story_id, newest, html): store.replace_revision(c["id"], newest["rev"], html, new_hash) diff --git a/bindery/store.py b/bindery/store.py index 4faeb35..1d39d07 100644 --- a/bindery/store.py +++ b/bindery/store.py @@ -8,7 +8,7 @@ site without losing text. import json import re import sqlite3 -from collections.abc import Iterator +from collections.abc import Callable, Iterator from contextlib import contextmanager from datetime import datetime from pathlib import Path @@ -282,13 +282,27 @@ class Store: ) return chapter_id - def add_chapters(self, story_id: int, chapters: list[dict[str, str]], source: str, order: list[str]) -> list[int]: + def add_chapters( + self, + story_id: int, + chapters: list[dict[str, str]], + source: str, + order: list[str] | Callable[[list[str]], list[str]], + ) -> list[int]: """Add chapters and put the story in the order of `order`, a list of chapter URLs, in one transaction. Each chapter needs url, title, html and text_hash. Chapters that `order` leaves out go after the others and keep their order. + + `order` can be a function. It gets the URLs of the stored chapters in book + order, and returns the list. It runs in the transaction, so it sees a move or + a delete that the user made a moment before. """ with self._connect() as conn: + conn.execute("BEGIN IMMEDIATE") + if callable(order): + stored = conn.execute("SELECT url FROM chapters WHERE story_id = ? ORDER BY position, id", (story_id,)) + order = order([r["url"] for r in stored]) ids = [] for c in chapters: cur = conn.execute( @@ -338,11 +352,20 @@ class Store: (*fields.values(), chapter_id), ) - def set_positions(self, story_id: int, chapter_ids: list[int]) -> None: + def reorder_chapters(self, story_id: int, arrange: Callable[[list[int]], list[int]]) -> None: + """Give the chapters of a story a new order. + + `arrange` gets the chapter ids in book order, and returns them in the new + order. The read and the write are one transaction, so a job that adds a + chapter at the same time cannot come between them. + """ with self._connect() as conn: + conn.execute("BEGIN IMMEDIATE") + rows = conn.execute("SELECT id FROM chapters WHERE story_id = ? ORDER BY position, id", (story_id,)) + order = arrange([r["id"] for r in rows]) conn.executemany( "UPDATE chapters SET position = ? WHERE id = ? AND story_id = ?", - [(i, cid, story_id) for i, cid in enumerate(chapter_ids)], + [(i, cid, story_id) for i, cid in enumerate(order)], ) def delete_chapters(self, story_id: int, chapter_ids: list[int]) -> None: diff --git a/tests/test_job_races.py b/tests/test_job_races.py new file mode 100644 index 0000000..7a4469b --- /dev/null +++ b/tests/test_job_races.py @@ -0,0 +1,92 @@ +"""Changes that the user makes to the chapters of a story while a job runs for it.""" + +import pytest +from test_deliver import CHAPTER, linked_story, site, titles, update +from test_deliver import fake as fake +from test_deliver import jobs as jobs +from test_deliver import story_epub as story_epub + +from bindery import fff, orchestrator +from bindery.jobs import DownloadMode +from bindery.store import Store + +THREE = [(1, "One", "a"), (2, "Two", "b"), (3, "Three", "c")] + + +def during_download(monkeypatch, number, action): + """Run `action` when the job downloads the chapter with this number.""" + fetch = fff.fetch_chapter + + def fetch_and_act(adapter, url, index): + if url == CHAPTER.format(number): + action() + return fetch(adapter, url, index) + + monkeypatch.setattr(fff, "fetch_chapter", fetch_and_act) + + +def chapter_id(story_id, title): + return next(c["id"] for c in orchestrator.store.list_chapters(story_id) if c["title"] == title) + + +def test_a_move_during_a_download_stays(fake, jobs, monkeypatch): + story_id = linked_story(THREE) + site(monkeypatch, [*THREE, (4, "Four", "d"), (5, "Five", "e")]) + during_download(monkeypatch, 4, lambda: orchestrator.move_chapters(story_id, [chapter_id(story_id, "Three")], None)) + assert "2 new chapters" in update(story_id) + assert titles(story_id) == ["Three", "One", "Two", "Four", "Five"] + + +def test_a_new_chapter_in_the_middle_goes_after_its_moved_predecessor(fake, jobs, monkeypatch): + story_id = linked_story(THREE) + site(monkeypatch, [(1, "One", "a"), (9, "Nine", "x"), (2, "Two", "b"), (3, "Three", "c")]) + during_download( + monkeypatch, + 9, + lambda: orchestrator.move_chapters(story_id, [chapter_id(story_id, "One")], chapter_id(story_id, "Three")), + ) + update(story_id) + assert titles(story_id) == ["Two", "Three", "One", "Nine"] + + +def test_a_delete_during_a_download_stays(fake, jobs, monkeypatch): + story_id = linked_story(THREE) + site(monkeypatch, [*THREE, (4, "Four", "d")]) + during_download(monkeypatch, 4, lambda: orchestrator.delete_chapters(story_id, [chapter_id(story_id, "Two")])) + assert "1 new chapter" in update(story_id) + assert titles(story_id) == ["One", "Three", "Four"] + + +def test_a_delete_during_a_recheck_does_not_stop_the_job(fake, jobs, monkeypatch): + story_id = linked_story(THREE) + site(monkeypatch, [(1, "One", "a2"), (2, "Two", "b2"), (3, "Three", "c2")]) + during_download(monkeypatch, 2, lambda: orchestrator.delete_chapters(story_id, [chapter_id(story_id, "Two")])) + assert "2 of 3 rechecked chapters changed" in update(story_id, DownloadMode.RECHECK_ALL) + assert titles(story_id) == ["One", "Three"] + + +def test_the_order_function_of_add_chapters_sees_the_stored_order(tmp_path): + store = Store(tmp_path / "db.sqlite") + sid = store.create_story("u", "s", "T", "A", []) + for n, url in enumerate(["u2", "u1"]): + store.add_chapter(sid, url, url, "
", "h", "new", n) + new = [{"url": "u3", "title": "u3", "html": "", "text_hash": "h"}] + seen = [] + store.add_chapters(sid, new, "new", lambda stored: seen.append(stored) or [*stored, "u3"]) + assert seen == [["u2", "u1"]] + assert [c["url"] for c in store.list_chapters(sid)] == ["u2", "u1", "u3"] + + +def test_a_refused_reorder_changes_nothing(tmp_path): + store = Store(tmp_path / "db.sqlite") + sid = store.create_story("u", "s", "T", "A", []) + ids = [store.add_chapter(sid, f"u{n}", str(n), "", "h", "new", n) for n in range(3)] + + def refuse(order): + raise ValueError("no") + + with pytest.raises(ValueError, match="no"): + store.reorder_chapters(sid, refuse) + assert [c["id"] for c in store.list_chapters(sid)] == ids + store.reorder_chapters(sid, lambda order: order[::-1]) + assert [c["id"] for c in store.list_chapters(sid)] == ids[::-1] diff --git a/tests/test_store.py b/tests/test_store.py index 1a6f405..41dba87 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -6,7 +6,7 @@ def test_chapters_revisions_and_rules(tmp_path): sid = store.create_story("https://example/s/1", "example", "T", "A", ["title"]) c1 = store.add_chapter(sid, "u1", "One", "1
", "h1", "new", 0) c2 = store.add_chapter(sid, "u2", "Two", "2
", "h2", "new", 1) - store.set_positions(sid, [c2, c1]) + store.reorder_chapters(sid, lambda order: [c2, c1]) assert [c["url"] for c in store.chapters_for_build(sid)] == ["u2", "u1"] assert store.add_revision(c1, "1b
", "h1b", "recheck") == 2