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 == []