From e11b4cc71faae5f69359ce63da546882e845d64e Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 24 Oct 2026 19:49:08 -0600 Subject: [PATCH] A removed Tag leaves no Download that would apply it The tags import removes a deleted Tag's name from every Download's tag_names and from the tag_names and channel_tag_name its settings pass on (a change to download_manager.yaml, which is then saved). Applying a Tag that does not exist fails the download, and it is retried. Once tags.yaml is imported, a newer download_manager.yaml drops names no Tag has, and is saved without them. A replaced Zim's entry tags move to the newest Zim of the same parsed name, by the date in its name (as migrate_outdated_zim_tags picks it), not by name prefix and path order. Co-Authored-By: Claude Opus 5.5 --- wrolpi/downloader.py | 30 ++++++++++++++++ wrolpi/tags.py | 45 +++++++++++++++++++++--- wrolpi/test/test_config_import.py | 32 ++++++++++++++++++ wrolpi/test/test_tags_config_import.py | 47 ++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 5 deletions(-) diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 90414bbb..49523175 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -2232,6 +2232,36 @@ class DownloadManagerConfig(ConfigFile): def downloads(self, value: List[dict]): self.update({'downloads': value}) + def prepare(self, diffs: dict): + """Drop Tag names that no Tag has (the Tag was removed) from the Downloads to write: applying one fails the + download. Only once tags.yaml was imported; until then its Tags may be missing.""" + from wrolpi.tags import Tag, get_tags_config, without_tag_names + if not get_tags_config().successful_import: + return + rows = [*diffs['downloads'].added.values(), *diffs['downloads'].changed.values()] + names = set() + for row in rows: + names.update(row['tag_names'] or []) + for key in ('tag_names', 'channel_tag_name'): + names.update((row['settings'] or dict()).get(key) or []) + if not names: + return + with get_db_session() as session: + unknown = names - {i for i, in session.query(Tag.name).filter(Tag.name.in_(list(names)))} + if not unknown: + return + logger.warning(f'Removing unknown Tags from Downloads: {sorted(unknown)}') + for row in rows: + tag_names, row['settings'] = without_tag_names(row['tag_names'], row['settings'], unknown) + row['tag_names'] = tag_names or None + # The file is saved without them. + self._config_outdated = True + + def after_import(self, diffs: dict): + if getattr(self, '_config_outdated', False): + self._config_outdated = False + self.background_dump.activate_switch() + @staticmethod def _filter_settings_for_yaml(settings: dict, global_defaults: dict = None) -> dict: """Remove inheritable video settings that match current global defaults. diff --git a/wrolpi/tags.py b/wrolpi/tags.py index c24f1340..096cea93 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -9,7 +9,7 @@ import pytz from cachetools.keys import hashkey from sqlalchemy import Column, Integer, String, ForeignKey, BigInteger, event, UniqueConstraint, insert from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm import Mapped, relationship, Session +from sqlalchemy.orm import Mapped, relationship, Session, load_only from wrolpi import dates, flags from wrolpi.common import loaded, ModelHelper, Base, logger, ConfigFile, get_media_directory, background_task, \ @@ -313,6 +313,32 @@ def _relative_path(path) -> str: return str(path.relative_to(get_media_directory()) if path.is_absolute() else path) +def without_tag_names(download_tag_names: Optional[List[str]], settings: Optional[dict], names: Set[str]) \ + -> Tuple[Optional[List[str]], Optional[dict]]: + """A Download's tag_names and settings without these Tag names, in the settings' tag_names and channel_tag_name + (passed on to the Downloads and Channels it creates) too. New objects, so an ORM change to the JSON is seen.""" + if download_tag_names: + download_tag_names = [i for i in download_tag_names if i not in names] + if settings: + settings = {key: [i for i in value if i not in names] if key in ('tag_names', 'channel_tag_name') and value + else value for key, value in settings.items()} + return download_tag_names, settings + + +def remove_tag_names_from_downloads(session: Session, names: Set[str]): + """Remove these Tag names from every Download that would apply them; applying a Tag that does not exist fails + the download. (Changes download_manager.yaml.)""" + from wrolpi.downloader import Download + changed = False + for download in session.query(Download).options(load_only(Download.id, Download.tag_names, Download.settings)): + tag_names, settings = without_tag_names(download.tag_names, download.settings, names) + if tag_names != download.tag_names or settings != download.settings: + download.tag_names, download.settings = tag_names, settings + changed = True + if changed: + save_downloads_config.activate_switch() + + def _tag_ids(session: Session, names) -> Dict[str, int]: tag_ids = dict() for chunk in chunks(names): @@ -351,6 +377,7 @@ class TagsSection(ConfigSection): """A removed Tag is deleted with its tagged files and Zim entries. A Collection using it keeps its directory, and loses only the Tag (its config is then saved).""" from wrolpi.collections import Collection + remove_tag_names_from_downloads(session, set(keys)) for tag in session.query(Tag).filter(Tag.name.in_(list(keys))): for collection in session.query(Collection).filter(Collection.tag_id == tag.id): logger.warning(f'Removing Tag {repr(tag.name)} from {collection}') @@ -695,8 +722,17 @@ class TagsConfig(ConfigFile): return media_directory = get_media_directory() with get_db_session() as session: - zim_paths = sorted(i for i, in session.query(Zim.path)) + zim_paths = [i for i, in session.query(Zim.path)] known = {str(i.relative_to(media_directory)) for i in zim_paths} + # {name: (date, path)} of the newest Zim of each name, like `migrate_outdated_zim_tags`. + newest = dict() + for path in zim_paths: + try: + name, date = zim_lib.parse_name(path) + except ValueError: + continue + if name not in newest or date > newest[name][0]: + newest[name] = (date, path) for key, row in list(diff.added.items()): tag_name, zim_path, zim_entry = key @@ -707,12 +743,11 @@ class TagsConfig(ConfigFile): name, _ = zim_lib.parse_name(media_directory / zim_path) except ValueError: name = None - newest = [i for i in zim_paths if name and i.name.startswith(name)] - if not newest: + if name not in newest: logger.warning(f'Cannot find Zim for {repr(zim_path)}') self.import_skipped += 1 continue - new_key = (tag_name, str(newest[-1].relative_to(media_directory)), zim_entry) + new_key = (tag_name, str(newest[name][1].relative_to(media_directory)), zim_entry) logger.warning(f'Migrating Zim entry tag {repr(tag_name)} from {zim_path} to {new_key[1]}') self._config_outdated = True diff.removed.discard(new_key) diff --git a/wrolpi/test/test_config_import.py b/wrolpi/test/test_config_import.py index b2f2a0fe..74b906ae 100644 --- a/wrolpi/test/test_config_import.py +++ b/wrolpi/test/test_config_import.py @@ -384,3 +384,35 @@ async def test_nothing_to_write_skips_prepare(test_session, saved_downloads, mon get_download_manager_config().import_config() assert prepared == [] + + +@pytest.mark.asyncio +async def test_unknown_tag_names_are_dropped(test_session, saved_downloads, test_tags_config, tag_factory, + await_switches): + """Once tags.yaml is imported, a Tag name the file gives a Download that no Tag has (the Tag was removed) is + dropped, and the file is saved without it: the next import writes nothing.""" + from wrolpi.tags import get_tags_config + await tag_factory('one') + test_session.commit() + get_tags_config().dump_config() + get_tags_config().import_config() + assert get_tags_config().successful_import is True + + def change(data): + for download in data['downloads']: + if download['url'] == 'https://example.com/recurring': + download['tag_names'] = ['one', 'gone'] + download['settings'] = dict(download.get('settings') or {}, channel_tag_name=['gone']) + + edit_config(saved_downloads, change) + config = get_download_manager_config() + config.import_config() + await await_switches() + + test_session.expire_all() + download = Download.get_by_url(test_session, 'https://example.com/recurring') + assert download.tag_names == ['one'] + assert download.settings['channel_tag_name'] == [] + with statements() as collected: + config.import_config() + assert writes(collected) == [] diff --git a/wrolpi/test/test_tags_config_import.py b/wrolpi/test/test_tags_config_import.py index 04a024a6..76c77dfa 100644 --- a/wrolpi/test/test_tags_config_import.py +++ b/wrolpi/test/test_tags_config_import.py @@ -256,3 +256,50 @@ async def test_refresh_changing_primary_file_counts(test_session, test_directory test_session.expire_all() assert test_session.query(FileGroup).one().primary_path != test_directory / 'video.mp4' assert get_config_version(test_session, 'tags.yaml') > 0 + + +@pytest.mark.asyncio +async def test_removed_tag_is_removed_from_downloads(test_session, saved_tags, test_download_manager_config): + """A removed Tag's name is removed from every Download that would apply it (its tag_names, and the tag_names and + channel_tag_name it passes on); download_manager.yaml is then newer than its file, so it is saved.""" + from wrolpi.config_version import set_config_version + from wrolpi.downloader import Download + test_session.add(Download(url='https://example.com/channel', downloader='video_channel', frequency=86400, + tag_names=['one', 'two'], + settings=dict(tag_names=['one'], channel_tag_name=['one'], video_resolutions=['720p']))) + test_session.commit() + set_config_version(test_session, 'download_manager.yaml', 1) + test_session.commit() + edit_config(saved_tags, lambda data: data['tags'].pop('one')) + + tags.get_tags_config().import_config() + + test_session.expire_all() + download = Download.get_by_url(test_session, 'https://example.com/channel') + assert download.tag_names == ['two'] + assert download.settings == dict(tag_names=[], channel_tag_name=[], video_resolutions=['720p']) + assert get_config_version(test_session, 'download_manager.yaml') > 1 + + +@pytest.mark.asyncio +async def test_zim_entry_migrates_by_name_and_date(test_session, test_directory, test_tags_config, zim_factory, + tag_factory, await_switches): + """The newest Zim of the same name is chosen by the date in its name, wherever it is, and a Zim whose name + merely starts with the same name is not the same Zim.""" + (test_directory / 'a').mkdir() + (test_directory / 'b').mkdir() + old = zim_factory('wikipedia_en_all_maxi_2020-01.zim') + newest = zim_factory('a/wikipedia_en_all_maxi_2020-03.zim') + zim_factory('b/wikipedia_en_all_maxi_2020-02.zim') + zim_factory('wikipedia_en_all_maxi_extra_2021-01.zim') + await tag_factory('one') + old.tag_entry(test_session, 'one', 'home') + test_session.commit() + tags.get_tags_config().dump_config() + await await_switches() + old.delete() + + tags.get_tags_config().import_config() + + test_session.expire_all() + assert [(i.tag.name, i.zim_id) for i in test_session.query(TagZimEntry)] == [('one', newest.id)]