From 9b331e7a84e1116cb7232a3cdf66fd602398232c Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 24 Oct 2026 17:37:58 -0600 Subject: [PATCH] Sync Zim subscriptions directly during the downloads import A row now compares its subscription's (name, language), derived from the URL, instead of whether one exists. The import moves a subscription of that name onto the written Download (a changed language or URL), removes one the file no longer implies, and leaves the Download's own columns as the file has them; change_download renamed the old Download's URL into the one just inserted, and reset attempts and the downloaders. A Kiwix URL that names no known subscription is a plain Download instead of failing the whole write. prepare runs only when there are differences, and its docstring states that it must be safe to repeat and to abandon. Co-Authored-By: Claude Opus 5.5 --- wrolpi/common.py | 11 +++- wrolpi/downloader.py | 72 +++++++++++++++++------ wrolpi/test/test_config_import.py | 94 +++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 20 deletions(-) diff --git a/wrolpi/common.py b/wrolpi/common.py index 3ea5233f..bcefab5a 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -846,7 +846,8 @@ class ConfigFile: db_rows = {i.name: i.db_rows(session) for i in self.sections} diffs = {i.name: diff_section(*file_rows[i.name], db_rows[i.name], deletes=decision == IMPORT) for i in self.sections} - self.prepare(diffs) + if any(diffs.values()): + self.prepare(diffs) file_version = self.version or 0 if any(diffs.values()) or (decision == IMPORT and db_version != file_version): @@ -885,8 +886,12 @@ class ConfigFile: return True def prepare(self, diffs: dict): - """Slow work the write needs (files, network), run with no transaction open. `diffs` maps each section's - name to its `SectionDiff`.""" + """Slow work the write needs (files, network), run with no transaction open, and only when there are + differences. `diffs` maps each section's name to its `SectionDiff`. + + It must be safe to repeat and to abandon: when the database changes before the write, the import decides + again (up to three passes), and the diffs it was given may never be written (the write fails, or the next + pass saves the database instead).""" def after_import(self, diffs: dict): """Run after the write: activate switches, send events, start background tasks.""" diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 61270675..90414bbb 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -2062,19 +2062,28 @@ def _utc(value) -> Optional[datetime]: return (value if value.tzinfo else value.replace(tzinfo=pytz.utc)).astimezone(pytz.utc) -def is_zim_subscription_download(url: str, frequency: Optional[int]) -> bool: - """A recurring Kiwix Download belongs to a ZimSubscription.""" - return bool(frequency) and url.startswith('https://download.kiwix.org/') +def zim_subscription_for(url: str, frequency: Optional[int]) -> Optional[Tuple[str, str]]: + """The (name, language) of the ZimSubscription a recurring Kiwix Download belongs to. None for any other + Download, including a Kiwix URL that names no known subscription.""" + from modules.zim.lib import zim_download_url_to_name + if not frequency or not url.startswith('https://download.kiwix.org/'): + return None + try: + return zim_download_url_to_name(url) + except Exception as e: + logger.warning(f'Download {url} is not a known Zim subscription: {e}') + return None class DownloadsSection(ConfigSection): """download_manager.yaml's Downloads, keyed by URL. A recurring Kiwix Download's ZimSubscription is derived - from it: `subscribed` compares whether the subscription exists, so a missing one is restored.""" + from it: `subscription` compares the subscription's (name, language), so a missing, stale or unwanted one is + corrected.""" name = 'downloads' @staticmethod def _row(url, destination, downloader, frequency, last_successful_download, next_download, settings, status, - sub_downloader, tag_names, subscribed, defaults: dict) -> dict: + sub_downloader, tag_names, subscription, defaults: dict) -> dict: return dict( destination=str(destination) if destination else None, downloader=downloader, @@ -2086,7 +2095,7 @@ class DownloadsSection(ConfigSection): status=status or DownloadStatus.new, sub_downloader=sub_downloader, tag_names=list(tag_names) if tag_names else None, - subscribed=subscribed, + subscription=tuple(subscription) if subscription else None, ) def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: @@ -2106,11 +2115,19 @@ class DownloadsSection(ConfigSection): rows[url] = self._row( url, destination, data['downloader'], data.get('frequency'), data.get('last_successful_download'), data.get('next_download'), settings, data.get('status'), - data.get('sub_downloader'), tag_names, is_zim_subscription_download(url, data.get('frequency')), + data.get('sub_downloader'), tag_names, zim_subscription_for(url, data.get('frequency')), defaults) except Exception as e: logger.error(f'Skipping invalid Download in config: {data}', exc_info=e) invalid.add(url) + # A subscription's name is unique: the first Download listed for it gets it. + names = set() + for url, row in rows.items(): + if row['subscription'] and row['subscription'][0] in names: + logger.warning(f'Download {url} is a second Download for Zim subscription {row["subscription"][0]}') + row['subscription'] = None + elif row['subscription']: + names.add(row['subscription'][0]) return rows, invalid def db_rows(self, session: Session) -> Dict[str, dict]: @@ -2119,15 +2136,13 @@ class DownloadsSection(ConfigSection): query = session.query( Download.url, Download.destination, Download.downloader, Download.frequency, Download.last_successful_download, Download.next_download, Download.settings, Download.status, - Download.sub_downloader, Download.tag_names, ZimSubscription.id, + Download.sub_downloader, Download.tag_names, ZimSubscription.name, ZimSubscription.language, ).outerjoin(ZimSubscription, ZimSubscription.download_id == Download.id) \ .filter(*Download.saved_by_config_filter()) - return {url: self._row(url, *values, subscribed=subscription_id is not None, defaults=defaults) - for url, *values, subscription_id in query} + return {url: self._row(url, *values, subscription=(name, language) if name else None, defaults=defaults) + for url, *values, name, language in query} def upsert(self, session: Session, rows: Dict[str, dict]): - from modules.zim.lib import zim_download_url_to_name - from modules.zim.models import ZimSubscription existing = dict() for urls in chunks(rows): existing.update({i.url: i for i in session.query(Download).filter(Download.url.in_(urls))}) @@ -2150,14 +2165,37 @@ class DownloadsSection(ConfigSection): for name, value in values[url].items(): setattr(download, name, value) session.flush() + self._sync_subscriptions(session, rows) + @staticmethod + def _sync_subscriptions(session: Session, rows: Dict[str, dict]): + """Give each written Download the ZimSubscription its row names, and no other. A subscription of that name + on another Download (the URL changed) is moved to this one. The Download's own columns are the file's.""" + from modules.zim.models import ZimSubscription + ids = dict() + for urls in chunks(rows): + ids.update(session.query(Download.url, Download.id).filter(Download.url.in_(urls))) + by_download = dict() + for download_ids in chunks(ids.values()): + by_download.update({i.download_id: i for i in + session.query(ZimSubscription).filter(ZimSubscription.download_id.in_(download_ids))}) for url, row in rows.items(): - if row['subscribed']: - name, language = zim_download_url_to_name(url) - subscription = session.query(ZimSubscription).filter_by(name=name).one_or_none() \ - or ZimSubscription(name=name, language=language) - subscription.change_download(session, url, row['frequency']) + download_id = ids[url] + current = by_download.get(download_id) + if current is not None and current.download_id != download_id: + current = None # Moved to an earlier row's Download. + if current is not None and (not row['subscription'] or current.name != row['subscription'][0]): + session.delete(current) + session.flush() + current = None + if row['subscription']: + name, language = row['subscription'] + subscription = current or session.query(ZimSubscription).filter_by(name=name).one_or_none() \ + or ZimSubscription(name=name) + subscription.language = language + subscription.download_id = download_id session.add(subscription) + session.flush() def delete(self, session: Session, keys: Set[str]): for urls in chunks(keys): diff --git a/wrolpi/test/test_config_import.py b/wrolpi/test/test_config_import.py index 65e1bcfb..b2f2a0fe 100644 --- a/wrolpi/test/test_config_import.py +++ b/wrolpi/test/test_config_import.py @@ -290,3 +290,97 @@ async def test_relative_destination(test_session, test_directory, saved_download with statements() as collected: config.import_config() assert writes(collected) == [] + + +def edit_kiwix(path, **values): + def change(data): + for download in data['downloads']: + if download['url'] == KIWIX_URL: + download.update(values) + + edit_config(path, change) + + +@pytest.mark.asyncio +async def test_zim_subscription_follows_a_language_change(test_session, saved_downloads): + """A Kiwix URL changed to another language moves the existing subscription (its name is unique) to the new + Download, with the new language; the old Download is removed.""" + french = KIWIX_URL.replace('_en_', '_fr_') + edit_kiwix(saved_downloads, url=french) + + config = get_download_manager_config() + config.import_config() + + assert config.successful_import is True + subscription = test_session.query(ZimSubscription).one() + assert (subscription.name, subscription.language, subscription.download.url) == ('Wikisource', 'fr', french) + assert KIWIX_URL not in urls(test_session) + + +@pytest.mark.asyncio +async def test_zim_subscription_download_keeps_its_state(test_session, saved_downloads): + """Changing a subscribed Download in the file writes only the file's values: its attempts are kept, and the + next import is in step.""" + test_session.query(Download).filter_by(url=KIWIX_URL).update(dict(attempts=3)) + test_session.commit() + edit_kiwix(saved_downloads, frequency=604800) + + config = get_download_manager_config() + config.import_config() + + test_session.expire_all() + download = Download.get_by_url(test_session, KIWIX_URL) + assert (download.frequency, download.attempts) == (604800, 3) + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_unsubscribed_download_drops_its_subscription(test_session, saved_downloads): + """A Kiwix Download the file keeps, but no longer as recurring, loses its subscription; the next import is in + step.""" + edit_kiwix(saved_downloads, frequency=None) + + config = get_download_manager_config() + config.import_config() + + assert test_session.query(ZimSubscription).count() == 0 + assert KIWIX_URL in urls(test_session) + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_unknown_kiwix_url_is_imported(test_session, saved_downloads): + """A recurring Kiwix URL that names no known subscription is imported as a plain Download; it does not fail the + other Downloads.""" + unknown = 'https://download.kiwix.org/zim/other/unknown.zim' + edit_config(saved_downloads, lambda data: data['downloads'].extend([ + dict(url=unknown, downloader='kiwix_zim', frequency=604800, status='new', last_successful_download=None, + next_download=None), + dict(url='https://example.com/added', downloader='archive', status='new', last_successful_download=None, + next_download=None), + ])) + + config = get_download_manager_config() + config.import_config() + + assert config.successful_import is True + assert {unknown, 'https://example.com/added'} <= urls(test_session) + assert test_session.query(ZimSubscription).count() == 1 + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_nothing_to_write_skips_prepare(test_session, saved_downloads, monkeypatch): + """An import with no differences does not start any preparation.""" + prepared = [] + monkeypatch.setattr(DownloadManagerConfig, 'prepare', lambda self, diffs: prepared.append(diffs)) + + get_download_manager_config().import_config() + + assert prepared == []