From adbcf944a51c95d56e80b2dc1bd47a2e919753cd Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:42:12 -0600 Subject: [PATCH 1/9] playlists.yaml imports by diffing (#869) * playlists.yaml imports by diffing playlists.yaml declares two sections: playlists, keyed by name, and their items, keyed by what each points to (a file's or Zim's media-relative path and entry, or a URL) with position and title compared. A playlist has at most one item per reference, so a moved item changes only its position and keeps its row; an unchanged import writes nothing (every import deleted and re-created every item). prepare skips items whose file or Zim is not indexed before the write (their place is kept), drops a Tag that no longer exists once tags.yaml is imported (the config is then saved), and, without deletes, keeps the database's order and adds the file's other items after the item that precedes them. Moving a FileGroup in a playlist counts as a playlists.yaml change (ORM, the bulk move, and a refresh that changes the primary file). media_relative moves to wrolpi.config_import for both configs. Co-Authored-By: Claude Opus 5.5 * Playlist items: keep database order without deletes; empty lists delete Without a database version, every playlist with an added or changed item keeps the database's order (the file's new items go after their predecessor; only titles come from the file), and a row whose merged position and title equal the database's is not written. Before, only playlists that gained an item were merged, so a file that only reordered (or lacked an item the database had) gave two items one position. A section can say it is present without rows (`ConfigSection.present`): a listed playlist with an empty items list has its items removed; an empty or missing playlists list still removes nothing. An item that cannot be read, or repeats another, is counted in import_skipped, so the config is not saved over it. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- wrolpi/collections/config.py | 402 ++++++++++++------ .../collections/test/test_playlists_config.py | 22 + .../test/test_playlists_config_import.py | 250 +++++++++++ wrolpi/common.py | 7 +- wrolpi/config_import.py | 24 +- wrolpi/files/models.py | 14 + wrolpi/files/worker.py | 13 + wrolpi/tags.py | 12 +- 8 files changed, 596 insertions(+), 148 deletions(-) create mode 100644 wrolpi/collections/test/test_playlists_config_import.py diff --git a/wrolpi/collections/config.py b/wrolpi/collections/config.py index d94e94da..662cdd87 100644 --- a/wrolpi/collections/config.py +++ b/wrolpi/collections/config.py @@ -1,12 +1,15 @@ import pathlib import shutil from dataclasses import dataclass, field -from typing import List +from typing import Dict, List, Optional, Set, Tuple + +from sqlalchemy import insert +from sqlalchemy.orm import Session from wrolpi.common import ConfigFile, get_media_directory, get_relative_to_media_directory, logger -from wrolpi.config_version import IMPORT, SAVE, count_config_change +from wrolpi.config_import import ConfigSection, SectionDiff, chunks, media_relative +from wrolpi.config_version import count_config_change from wrolpi.db import get_db_session -from wrolpi.events import Events from wrolpi.switches import ActivateSwitchMethod, register_switch_handler from wrolpi.tags import TagFile, TagsConfig, save_tags_config @@ -15,9 +18,195 @@ logger = logger.getChild(__name__) __all__ = ['PlaylistsConfig', 'playlists_config', 'save_playlists_config', 'get_playlists_config'] -def _item_key(item) -> tuple: - """What a playlist item points to; a playlist has at most one item per key (see CollectionItem's constraints).""" - return item.item_kind, item.file_group_id, item.zim_id, item.zim_entry, item.url +def _playlist_tag_names(rows) -> Set[str]: + return {i['tag_name'] for i in rows if i['tag_name']} + + +class PlaylistsSection(ConfigSection): + """playlists.yaml's playlists (Collections with kind 'playlist'), keyed by name.""" + name = 'playlists' + + def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: + rows, invalid = dict(), set() + for data in config.get('playlists') or []: + name = data.get('name') + if not name: + logger.error(f'Playlist config entry has no name, skipping: {data}') + continue + try: + rows[name] = dict( + description=data.get('description') or None, + # Optional custom directory (media-relative); None when not customized. + directory=media_relative(data['directory']) if data.get('directory') else None, + tag_name=data.get('tag_name') or None, + ) + except Exception as e: + logger.error(f'Skipping invalid playlist in config: {name}', exc_info=e) + invalid.add(name) + return rows, invalid + + def db_rows(self, session: Session) -> Dict[str, dict]: + from wrolpi.tags import Tag + from .models import Collection + query = session.query(Collection.name, Collection.description, Collection.directory, Tag.name) \ + .outerjoin(Tag, Tag.id == Collection.tag_id) \ + .filter(Collection.kind == 'playlist') + return {name: dict(description=description or None, + directory=media_relative(directory) if directory else None, + tag_name=tag_name) + for name, description, directory, tag_name in query} + + def upsert(self, session: Session, rows: Dict[str, dict]): + from wrolpi.tags import Tag + from .models import Collection + existing = {i.name: i for i in + session.query(Collection).filter(Collection.kind == 'playlist', Collection.name.in_(list(rows)))} + tags = {i.name: i for i in session.query(Tag).filter(Tag.name.in_(list(_playlist_tag_names(rows.values()))))} + for name, row in rows.items(): + if not (collection := existing.get(name)): + collection = Collection(name=name, kind='playlist') + session.add(collection) + collection.description = row['description'] + collection.directory = (get_media_directory() / row['directory']) if row['directory'] else None + collection.tag = tags.get(row['tag_name']) + session.flush() + + def delete(self, session: Session, keys: Set[str]): + from .models import Collection + for collection in session.query(Collection).filter(Collection.kind == 'playlist', + Collection.name.in_(list(keys))): + logger.info(f'Deleting playlist {collection.name!r} (no longer in config)') + session.delete(collection) + + +class PlaylistItemsSection(ConfigSection): + """playlists.yaml's playlist items, keyed by (playlist name, kind, reference, Zim entry): a file's or Zim's + media-relative path, or a URL. A playlist has at most one item per reference (CollectionItem's constraints), so + moving an item changes only its position, and it keeps its row.""" + name = 'items' + + @staticmethod + def item_key(name: str, data: dict) -> Optional[tuple]: + if 'file' in data: + return name, 'file', media_relative(data['file']), None + elif 'zim' in data: + if not data.get('entry'): + raise ValueError('A zim item requires an entry') + return name, 'zim', media_relative(data['zim']), data['entry'] + elif 'url' in data: + return name, 'url', data['url'], None + raise ValueError('An item requires a file, zim, or url') + + def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: + rows, invalid = dict(), set() + for data in config.get('playlists') or []: + if not (name := data.get('name')): + continue + # Positions follow the file's order; an item skipped now keeps its place for a later import. + for position, item in enumerate(data.get('items') or [], start=1): + try: + key = self.item_key(name, item) + except Exception as e: + logger.error(f'Skipping invalid item of playlist {name!r}: {item}', exc_info=e) + key = None + if key is None or key in rows: + if key: + logger.warning(f'Skipping duplicate item of playlist {name!r}: {item}') + # Counted as skipped (the config is not saved over it); a kind of None is never an item's key. + invalid.add((name, None, position, None)) + continue + rows[key] = dict(position=position, title=item.get('title')) + return rows, invalid + + def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: + # A listed playlist without items has none: only an empty or missing playlists list removes nothing. + return any(i.get('name') for i in config.get('playlists') or []) + + @staticmethod + def _query(session: Session, *columns): + from modules.zim.models import Zim + from wrolpi.files.models import FileGroup + from .models import Collection, CollectionItem + return session.query(*columns, Collection.name, CollectionItem.item_kind, FileGroup.primary_path, Zim.path, + CollectionItem.zim_entry, CollectionItem.url) \ + .join(Collection, Collection.id == CollectionItem.collection_id) \ + .outerjoin(FileGroup, FileGroup.id == CollectionItem.file_group_id) \ + .outerjoin(Zim, Zim.id == CollectionItem.zim_id) \ + .filter(Collection.kind == 'playlist') + + @staticmethod + def _db_key(name, item_kind, primary_path, zim_path, zim_entry, url) -> Optional[tuple]: + if item_kind == 'file' and primary_path: + return name, 'file', media_relative(primary_path), None + elif item_kind == 'zim' and zim_path: + return name, 'zim', media_relative(zim_path), zim_entry + elif item_kind == 'url': + return name, 'url', url, None + # Not saved (see `CollectionItem.to_config`), so never "removed". + return None + + def db_rows(self, session: Session) -> Dict[tuple, dict]: + from .models import CollectionItem + rows = dict() + for position, title, *values in self._query(session, CollectionItem.position, CollectionItem.title): + if key := self._db_key(*values): + rows[key] = dict(position=position, title=title) + return rows + + def upsert(self, session: Session, rows: Dict[tuple, dict]): + from modules.zim.models import Zim + from wrolpi.files.models import FileGroup + from .models import Collection, CollectionItem + media_directory = get_media_directory() + collection_ids = dict(session.query(Collection.name, Collection.id).filter( + Collection.kind == 'playlist', Collection.name.in_(list({i[0] for i in rows})))) + file_group_ids = dict() + for paths in chunks({str(media_directory / i[2]) for i in rows if i[1] == 'file'}): + file_group_ids.update((media_relative(path), id_) for path, id_ in + session.query(FileGroup.primary_path, FileGroup.id) + .filter(FileGroup.primary_path.in_(paths))) + zim_ids = {media_relative(path): id_ for id_, path in session.query(Zim.id, Zim.path)} \ + if any(i[1] == 'zim' for i in rows) else dict() + existing = {(i.collection_id, i.item_kind, i.file_group_id, i.zim_id, i.zim_entry, i.url): i for i in + session.query(CollectionItem).filter(CollectionItem.collection_id.in_(collection_ids.values()))} + + new = list() + for key, row in rows.items(): + name, kind, reference, zim_entry = key + values = dict(collection_id=collection_ids.get(name), item_kind=kind, file_group_id=None, zim_id=None, + zim_entry=None, url=None) + if kind == 'file': + values['file_group_id'] = file_group_ids.get(reference) + elif kind == 'zim': + values.update(zim_id=zim_ids.get(reference), zim_entry=zim_entry) + else: + values['url'] = reference + if not values['collection_id'] or (kind == 'file' and not values['file_group_id']) \ + or (kind == 'zim' and not values['zim_id']): + logger.warning(f'Playlist {name!r}: cannot find {reference!r}, skipping') + continue + if item := existing.get(tuple(values.values())): + item.position, item.title = row['position'], row['title'] + else: + new.append(dict(values, position=row['position'], title=row['title'])) + if new: + # One statement for all new items (a rebuilt database has thousands). Not flushed: count it. + session.execute(insert(CollectionItem), new) + count_config_change(session, PlaylistsConfig.file_name) + session.flush() + + def delete(self, session: Session, keys: Set[tuple]): + from .models import CollectionItem + for item, *values in self._query(session, CollectionItem) \ + .filter(CollectionItem.collection_id.in_(self._collection_ids(session, keys))): + if self._db_key(*values) in keys: + session.delete(item) + + @staticmethod + def _collection_ids(session: Session, keys) -> List[int]: + from .models import Collection + return [i for i, in session.query(Collection.id).filter( + Collection.kind == 'playlist', Collection.name.in_(list({i[0] for i in keys})))] @dataclass @@ -48,6 +237,7 @@ class PlaylistsConfig(ConfigFile): title: "The spot" """ file_name = 'playlists.yaml' + sections = (PlaylistsSection(), PlaylistItemsSection()) validator = PlaylistsConfigValidator default_config = dict(version=0, playlists=[]) width = 120 @@ -56,140 +246,83 @@ class PlaylistsConfig(ConfigFile): def playlists(self) -> List[dict]: return self._config.get('playlists', []) - def import_config(self, file: pathlib.Path = None, send_events=False): - """Import playlists (and their ordered items) from the config into the database.""" + def prepare(self, diffs: dict): + self._drop_unknown_tags(diffs['playlists']) + self._drop_missing_items(diffs['items']) + if not self.import_deletes: + self._merge_items(diffs['items']) + + def _drop_unknown_tags(self, diff: SectionDiff): + """A Tag that no longer exists (it was removed) is dropped, and the config is saved without it. Only once + tags.yaml was imported; until then its Tags may be missing.""" + from wrolpi.tags import Tag, get_tags_config + rows = [*diff.added.values(), *diff.changed.values()] + if not (names := _playlist_tag_names(rows)) or not get_tags_config().successful_import: + return + with get_db_session() as session: + unknown = names - {i for i, in session.query(Tag.name).filter(Tag.name.in_(list(names)))} + for row in rows: + if row['tag_name'] in unknown: + logger.warning(f'Removing unknown Tag {row["tag_name"]!r} from a playlist') + row['tag_name'] = None + self._config_outdated = True + + def _drop_missing_items(self, diff: SectionDiff): + """An added item whose file or Zim is not in the database (not indexed yet, after a rebuild) is skipped + before the write; a later import (after the refresh) adds it.""" from modules.zim.models import Zim from wrolpi.files.models import FileGroup - from wrolpi.tags import Tag + media_directory = get_media_directory() + files = {i[2] for i in diff.added if i[1] == 'file'} + zims = {i[2] for i in diff.added if i[1] == 'zim'} + known = set() + with get_db_session() as session: + for paths in chunks({str(media_directory / i) for i in files}): + known.update(('file', media_relative(i)) for i, in + session.query(FileGroup.primary_path).filter(FileGroup.primary_path.in_(paths))) + if zims: + known.update(('zim', media_relative(i)) for i, in session.query(Zim.path)) + for key in list(diff.added): + if key[1] in ('file', 'zim') and (key[1], key[2]) not in known: + logger.warning(f'Playlist {key[0]!r}: {key[1]} not indexed, skipping {key[2]!r}') + del diff.added[key] + self.import_skipped += 1 - from .models import Collection, CollectionItem - - file = file or self.get_file() - file_str = str(self.get_relative_file()) - - # A missing config is NOT an empty config (mirrors ChannelsConfig). Deleting here would - # cascade: the sync prunes the on-disk playlist dirs and the next dump writes an empty - # config -- a single lost file would permanently destroy every playlist. - if not file.is_file(): - logger.info(f'No playlists config file, skipping import: {file_str}') - self.successful_import = True + def _merge_items(self, diff: SectionDiff): + """Without deletes, a playlist's items in the database keep their order, and the file's other items are + added after the item that precedes them in the file. Only titles are taken from the file's other items.""" + section = PlaylistItemsSection() + if not (names := {i[0] for i in (*diff.added, *diff.changed)}): return + with get_db_session() as session: + db_rows = section.db_rows(session) + file_rows, _ = section.file_rows(self._config) + for name in names: + merged = sorted((i for i in db_rows if i[0] == name), key=lambda i: db_rows[i]['position']) + previous = None + for key in sorted((i for i in file_rows if i[0] == name), key=lambda i: file_rows[i]['position']): + if key in db_rows: + previous = key + elif key in diff.added: + merged.insert(merged.index(previous) + 1 if previous else 0, key) + previous = key + for position, key in enumerate(merged, start=1): + if key in diff.added: + diff.added[key]['position'] = position + continue + row = dict(position=position, title=(diff.changed.get(key) or db_rows[key])['title']) + if row == db_rows[key]: + diff.changed.pop(key, None) + else: + diff.changed[key] = row + def import_config(self, file: pathlib.Path = None, send_events=False): + self._config_outdated = False super().import_config(file, send_events) - decision = self.import_decision() - if decision == SAVE: - self.save_database_instead() - return - - playlists_data = self._config.get('playlists', []) - - # An empty playlists list never deletes DB records (mirrors ChannelsConfig). Deleting all - # playlists is done through the API, which dumps the resulting (empty) config itself. - if not playlists_data: - logger.info(f'No playlists in config, preserving existing DB playlists: {file_str}') - self.successful_import = True - return - - try: - with get_db_session(commit=True) as session: - imported_names = set() - for data in playlists_data: - name = data.get('name') - if not name: - logger.error(f'Playlist config entry has no name, skipping: {data}') - self.import_skipped += 1 - continue - - collection = session.query(Collection).filter_by( - name=name, kind='playlist').one_or_none() - new = not collection - if new: - collection = Collection(name=name, kind='playlist') - session.add(collection) - collection.description = data.get('description') - - # Optional custom directory (media-relative); None when not customized. - directory = data.get('directory') - collection.directory = (get_media_directory() / directory) if directory else None - - tag_name = data.get('tag_name') - collection.tag = Tag.get_by_name(session, tag_name) if tag_name else None - session.flush([collection]) - - file_items = [] - for item_data in (data.get('items') or []): - item = None - if 'file' in item_data: - path = get_media_directory() / item_data['file'] - fg = session.query(FileGroup).filter_by(primary_path=str(path)).one_or_none() - if fg: - item = CollectionItem(collection_id=collection.id, item_kind='file', - file_group_id=fg.id) - else: - logger.warning(f'Playlist {name!r}: file not indexed, skipping ' - f'{item_data["file"]!r}') - self.import_skipped += 1 - elif 'zim' in item_data: - zpath = get_media_directory() / item_data['zim'] - zim = session.query(Zim).filter_by(path=str(zpath)).one_or_none() - if zim: - item = CollectionItem(collection_id=collection.id, item_kind='zim', - zim_id=zim.id, zim_entry=item_data.get('entry')) - else: - logger.warning(f'Playlist {name!r}: zim not found, skipping ' - f'{item_data["zim"]!r}') - self.import_skipped += 1 - elif 'url' in item_data: - item = CollectionItem(collection_id=collection.id, item_kind='url', - url=item_data['url']) - if item is not None: - item.title = item_data.get('title') - file_items.append(item) - - if decision == IMPORT or new: - # Rebuild items to match the config's order exactly. - for item in list(collection.items): - session.delete(item) - session.flush() - items = file_items - else: - # Without deletes, keep the database's items in their order; add the file's other items - # after the item that precedes them in the file. - items = sorted(collection.items, key=lambda i: i.position) - existing = {_item_key(i): i for i in items} - previous = None - for item in file_items: - if found := existing.get(_item_key(item)): - previous = found - continue - items.insert(items.index(previous) + 1 if previous else 0, item) - previous = item - - for position, item in enumerate(items, start=1): - item.position = position - session.add(item) - - session.flush() - imported_names.add(name) - - # Delete playlists that are no longer present in the config. - for collection in session.query(Collection).filter_by(kind='playlist').all() \ - if decision == IMPORT else []: - if collection.name not in imported_names: - logger.info(f'Deleting playlist {collection.name!r} (no longer in config)') - session.delete(collection) - - logger.info(f'Imported {len(imported_names)} playlists from {file_str}') - self.record_import_version() - self.successful_import = True - except Exception as e: - self.successful_import = False - message = f'Failed to import {file_str} config!' - logger.error(message, exc_info=e) - if send_events: - Events.send_config_import_failed(message) - raise + def after_import(self, diffs: dict): + if self._config_outdated: + self.background_dump.activate_switch() def preview_backup_import(self, backup_date: str, mode: str) -> dict: """Preview which playlists a backup import would add/remove (keyed by playlist name).""" @@ -302,7 +435,7 @@ def file_groups_moved(session, file_group_ids): playlists.yaml references file items by path, so it must be re-saved (and the playlist directories re-synced) or the next import will not find the file and will drop the item. - tags.yaml saves tagged files by path too; the move (often raw SQL) is counted as its change. + tags.yaml saves tagged files by path too. The move (often raw SQL) is counted as each config's change. `file_group_ids` may be an iterable of ids or a select of ids.""" from .models import Collection, CollectionItem @@ -316,6 +449,7 @@ def file_groups_moved(session, file_group_ids): .filter(Collection.kind == 'playlist', CollectionItem.file_group_id.in_(file_group_ids)) \ .first() if in_playlist: + count_config_change(session, PlaylistsConfig.file_name) save_playlists_config.activate_switch() from .sync import sync_playlists_directory sync_playlists_directory.activate_switch() diff --git a/wrolpi/collections/test/test_playlists_config.py b/wrolpi/collections/test/test_playlists_config.py index 474c0507..f7f55ef3 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -392,3 +392,25 @@ async def test_import_without_deletes_keeps_database_items(test_session, test_di test_session.expire_all() collection = test_session.query(Collection).filter_by(name='Mix', kind='playlist').one() assert [i.url for i in collection.items] == ['/b', '/a', '/c'] + + +@pytest.mark.asyncio +async def test_import_without_deletes_keeps_database_order(test_session, test_directory, playlists_config): + """Without a database version, a file that only reorders a playlist's items (and lacks one the database has) + leaves the database's order as it is.""" + collection = Collection(name='Mix', kind='playlist') + test_session.add(collection) + test_session.flush([collection]) + collection.add_url(test_session, '/a', title='A') + collection.add_url(test_session, '/x', title='X') # Added after the file was saved. + collection.add_url(test_session, '/b', title='B') + test_session.commit() + playlists_config.get_file().write_text(yaml.dump(dict(version=1, playlists=[ + dict(name='Mix', items=[dict(url='/b', title='B'), dict(url='/a', title='A')]), + ]))) + + playlists_config.import_config() + + test_session.expire_all() + collection = test_session.query(Collection).filter_by(name='Mix', kind='playlist').one() + assert [(i.url, i.position) for i in collection.items] == [('/a', 1), ('/x', 2), ('/b', 3)] diff --git a/wrolpi/collections/test/test_playlists_config_import.py b/wrolpi/collections/test/test_playlists_config_import.py new file mode 100644 index 00000000..1c391858 --- /dev/null +++ b/wrolpi/collections/test/test_playlists_config_import.py @@ -0,0 +1,250 @@ +"""playlists.yaml imports by diffing its playlists and their items against the database, and writing only the +differences (see wrolpi.config_import). An item is keyed by what it points to, so moving it changes only its +position.""" +import pytest +import yaml + +from wrolpi.collections.config import get_playlists_config +from wrolpi.collections.models import Collection, CollectionItem +from wrolpi.config_version import get_config_version +from wrolpi.conftest import config_was_imported +from wrolpi.files.models import FileGroup +from wrolpi.test.test_config_import import edit_config, statements, writes + +ONE = ['url:/one', 'file:a.txt', 'zim:home'] + + +@pytest.fixture +async def saved_playlists(test_session, test_directory, async_client, make_files_structure, test_zim, tag_factory, + await_switches): + """Playlist `One` (a URL, a file, and a Zim entry, tagged `one`) and `Two` (a URL), saved to the config (which + records its version).""" + from copy import deepcopy + config = get_playlists_config() + config._config = deepcopy(config.default_config) + tag = await tag_factory('one') + a, = make_files_structure({'a.txt': 'a'}) + a = FileGroup.from_paths(test_session, a) + one = Collection(name='One', kind='playlist', description='The first', tag_id=tag.id) + two = Collection(name='Two', kind='playlist') + test_session.add_all([a, one, two]) + test_session.flush([a, one, two]) + one.add_url(test_session, '/one', title='One URL') + one.add_file_group(a, session=test_session) + one.add_zim_entry(test_session, test_zim.id, 'home') + two.add_url(test_session, '/two') + test_session.commit() + + config.dump_config() + await await_switches() + assert get_config_version(test_session, 'playlists.yaml') is not None + return config.get_file() + + +def items(test_session, name='One') -> list: + """`kind:reference` of each item of the playlist, in order.""" + test_session.expire_all() + collection = test_session.query(Collection).filter_by(name=name, kind='playlist').one() + return [f'{i.item_kind}:{i.url or i.zim_entry or i.file_group.primary_path.name}' for i in collection.items] + + +def item_ids(test_session, name='One') -> dict: + test_session.expire_all() + collection = test_session.query(Collection).filter_by(name=name, kind='playlist').one() + return {i.id for i in collection.items} + + +def edit_playlist(path, name, change): + def change_data(data): + change(next(i for i in data['playlists'] if i['name'] == name)) + + edit_config(path, change_data) + + +@pytest.mark.asyncio +async def test_unchanged_import_writes_nothing(test_session, saved_playlists): + """Importing a config the database already matches opens no write transaction.""" + config = get_playlists_config() + with statements() as collected: + config.import_config() + + assert config.successful_import is True + assert writes(collected) == [] + # The version, then one snapshot per section. + assert len(collected) <= 4, collected + assert items(test_session) == ONE + + +@pytest.mark.asyncio +async def test_reordered_items_keep_their_rows(test_session, saved_playlists): + """Moving and inserting items changes only positions: every item keeps its row (and ID).""" + before = item_ids(test_session) + + def change(playlist): + playlist['items'].reverse() + playlist['items'].insert(0, dict(url='/new', title='New')) + + edit_playlist(saved_playlists, 'One', change) + config = get_playlists_config() + with statements() as collected: + config.import_config() + + assert config.successful_import is True + assert items(test_session) == ['url:/new', 'zim:home', 'file:a.txt', 'url:/one'] + assert before < item_ids(test_session) + assert not [i for i in writes(collected) if i.upper().startswith('DELETE FROM COLLECTION_ITEM')] + + +@pytest.mark.asyncio +async def test_removed_rows_are_deleted(test_session, saved_playlists): + """A removed item and a removed playlist are deleted; a changed description and title are saved.""" + def change(data): + data['playlists'] = [i for i in data['playlists'] if i['name'] != 'Two'] + one = data['playlists'][0] + one['description'] = 'Changed' + one['items'] = [i for i in one['items'] if 'zim' not in i] + one['items'][0]['title'] = 'Renamed' + + edit_config(saved_playlists, change) + config = get_playlists_config() + config.import_config() + + assert config.successful_import is True + assert {i.name for i in test_session.query(Collection).filter_by(kind='playlist')} == {'One'} + assert items(test_session) == ['url:/one', 'file:a.txt'] + one = test_session.query(Collection).filter_by(name='One', kind='playlist').one() + assert (one.description, one.items[0].title) == ('Changed', 'Renamed') + + +@pytest.mark.asyncio +async def test_unindexed_file_is_skipped_without_a_write(test_session, saved_playlists): + """An item whose file is not indexed (yet) is skipped before the write, and keeps its place: the items after it + keep their file positions. The next import, with nothing else to do, writes nothing.""" + edit_playlist(saved_playlists, 'Two', lambda playlist: playlist['items'].insert(0, dict(file='later.txt'))) + config = get_playlists_config() + config.import_config() + test_session.expire_all() + assert [i.position for i in test_session.query(Collection).filter_by(name='Two').one().items] == [2] + + with statements() as collected: + config.import_config() + + assert config.successful_import is True + assert config.import_skipped == 1 + assert writes(collected) == [] + assert items(test_session, 'Two') == ['url:/two'] + + +@pytest.mark.asyncio +async def test_new_playlist_is_created_with_its_items(test_session, saved_playlists): + edit_config(saved_playlists, lambda data: data['playlists'].append(dict( + name='Three', description='New', tag_name='one', items=[dict(url='/three'), dict(file='a.txt')]))) + + get_playlists_config().import_config() + + three = test_session.query(Collection).filter_by(name='Three', kind='playlist').one() + assert (three.description, three.tag_name) == ('New', 'one') + assert items(test_session, 'Three') == ['url:/three', 'file:a.txt'] + + +@pytest.mark.asyncio +async def test_unknown_tag_is_dropped(test_session, saved_playlists, test_tags_config, await_switches): + """Once tags.yaml is imported, a playlist's Tag that no longer exists is dropped and the config is saved without + it: the next import writes nothing.""" + from wrolpi.tags import get_tags_config + get_tags_config().dump_config() + get_tags_config().import_config() + edit_playlist(saved_playlists, 'One', lambda playlist: playlist.update(tag_name='gone')) + + config = get_playlists_config() + config.import_config() + await await_switches() + + assert test_session.query(Collection).filter_by(name='One', kind='playlist').one().tag_id is None + assert 'gone' not in saved_playlists.read_text() + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_moved_playlist_file_counts(test_session, test_directory, saved_playlists, make_files_structure): + """playlists.yaml saves file items by path: moving a FileGroup in a playlist makes the database newer than the + file, so a restart before the save does not drop the item. Moving another FileGroup does not.""" + before = get_config_version(test_session, 'playlists.yaml') + a = test_session.query(FileGroup).filter_by(primary_path=test_directory / 'a.txt').one() + a.move(test_directory / 'moved.txt') + test_session.commit() + assert get_config_version(test_session, 'playlists.yaml') == before + 1 + + other = FileGroup.from_paths(test_session, *make_files_structure({'other.txt': 'other'})) + test_session.commit() + other.move(test_directory / 'other-moved.txt') + test_session.commit() + assert get_config_version(test_session, 'playlists.yaml') == before + 1 + + +@pytest.mark.asyncio +async def test_bulk_moved_playlist_file_counts(test_session, test_directory, saved_playlists): + """A move written with raw SQL counts too.""" + from wrolpi.files.lib import _bulk_update_file_groups_db + before = get_config_version(test_session, 'playlists.yaml') + _bulk_update_file_groups_db(test_session, {test_directory / 'a.txt': test_directory / 'moved/a.txt'}) + test_session.commit() + + assert get_config_version(test_session, 'playlists.yaml') > before + + +@pytest.mark.asyncio +async def test_refresh_changing_primary_file_counts(test_session, test_directory, make_files_structure, video_bytes, + refresh_files): + """A refresh that changes a playlist FileGroup's primary file counts as a change of the item's path.""" + make_files_structure({'video.mp4': video_bytes, 'video.jpg': 'poster'}) + await refresh_files() + playlist = Collection(name='Videos', kind='playlist') + test_session.add(playlist) + test_session.flush([playlist]) + playlist.add_file_group(test_session.query(FileGroup).one(), session=test_session) + test_session.commit() + config_was_imported(test_session, 'playlists.yaml') + test_session.commit() + + (test_directory / 'video.mp4').unlink() + await refresh_files() + + test_session.expire_all() + assert test_session.query(FileGroup).one().primary_path != test_directory / 'video.mp4' + assert get_config_version(test_session, 'playlists.yaml') > 0 + + +@pytest.mark.asyncio +async def test_empty_items_are_removed(test_session, saved_playlists): + """A listed playlist with no items has its items removed, even when no playlist in the file has an item (only + an empty or missing playlists list deletes nothing).""" + def change(data): + for playlist in data['playlists']: + playlist['items'] = [] + + edit_config(saved_playlists, change) + config = get_playlists_config() + config.import_config() + + assert config.successful_import is True + assert items(test_session, 'One') == [] + assert items(test_session, 'Two') == [] + + +@pytest.mark.asyncio +async def test_invalid_items_are_skipped(test_session, saved_playlists): + """An item that cannot be read (a Zim item without an entry), or that repeats another, is counted as skipped, + so the config is not saved over it.""" + def change(playlist): + playlist['items'].append(dict(zim='other.zim')) + playlist['items'].append(dict(url='/one')) + + edit_playlist(saved_playlists, 'One', change) + config = get_playlists_config() + config.import_config() + + assert config.import_skipped == 2 + assert items(test_session) == ONE diff --git a/wrolpi/common.py b/wrolpi/common.py index 961982f3..0a09f368 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -844,10 +844,12 @@ class ConfigFile: file_rows = {i.name: i.file_rows(self._config) for i in self.sections} with get_db_session() as session: 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, same=i.same) + diffs = {i.name: diff_section(*file_rows[i.name], db_rows[i.name], deletes=decision == IMPORT, same=i.same, + present=i.present(self._config, *file_rows[i.name])) for i in self.sections} # `prepare` may skip rows it cannot apply. self.import_skipped = sum(len(invalid) for _, invalid in file_rows.values()) + self.import_deletes = decision == IMPORT if any(diffs.values()): self.prepare(diffs) @@ -889,7 +891,8 @@ class ConfigFile: def prepare(self, diffs: dict): """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 may drop rows it cannot apply - (count them in `import_skipped`), or add what it found to a row for `upsert`. + (count them in `import_skipped`), or add what it found to a row for `upsert`. `import_deletes`: whether + this import deletes what the file no longer lists. 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 diff --git a/wrolpi/config_import.py b/wrolpi/config_import.py index bd7f36c5..fb29ab09 100644 --- a/wrolpi/config_import.py +++ b/wrolpi/config_import.py @@ -6,11 +6,14 @@ Rows are compared as plain data keyed the way the file names them (a URL, a name ORM objects and the read can finish before any slow preparation starts. """ import operator +import pathlib from dataclasses import dataclass, field from typing import Any, Callable, Dict, Hashable, Iterable, List, Set, Tuple from sqlalchemy.orm import Session +from wrolpi.common import get_media_directory + @dataclass class SectionDiff: @@ -46,6 +49,11 @@ class ConfigSection: deleted since the snapshot.""" raise NotImplementedError() + def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: + """Whether the file lists this section at all; an empty section cannot be told apart from a lost or blanked + file, so it removes nothing.""" + return bool(file_rows or invalid_keys) + def same(self, file_row: dict, db_row: dict) -> bool: """Whether a file row is already in the database.""" return file_row == db_row @@ -57,20 +65,30 @@ class ConfigSection: def diff_section(file_rows: Dict[Hashable, dict], invalid_keys: Set[Hashable], db_rows: Dict[Hashable, dict], - deletes: bool, same: Callable[[dict, dict], bool] = operator.eq) -> SectionDiff: + deletes: bool, same: Callable[[dict, dict], bool] = operator.eq, present: bool = None) \ + -> SectionDiff: """Compare a section's file rows with its database rows. `deletes`: rows the file no longer lists are removed. - An empty section never removes anything: it cannot be told apart from a lost or blanked file.""" + A section that is not `present` (by default, an empty one) never removes anything: it cannot be told apart from + a lost or blanked file.""" diff = SectionDiff() for key, row in file_rows.items(): if key not in db_rows: diff.added[key] = row elif not same(row, db_rows[key]): diff.changed[key] = row - if deletes and (file_rows or invalid_keys): + if present is None: + present = bool(file_rows or invalid_keys) + if deletes and present: diff.removed = db_rows.keys() - file_rows.keys() - invalid_keys return diff +def media_relative(path) -> str: + """A path as a config names it: relative to the media directory.""" + path = pathlib.Path(path) + return str(path.relative_to(get_media_directory()) if path.is_absolute() else path) + + def chunks(items: Iterable[Any], size: int = 500) -> Iterable[List[Any]]: """Split `items` for `IN (...)` queries (SQLite limits the parameters of one statement).""" items = list(items) diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index f78e72db..bc70b0c9 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -905,6 +905,20 @@ def _tagged_file_group_moved(file_group: FileGroup) -> bool: register_ownership('tags.yaml', FileGroup, columns={'primary_path'}, row=_tagged_file_group_moved) +def _playlist_file_group_moved(file_group: FileGroup) -> bool: + """An existing FileGroup in a playlist changed path (playlists.yaml saves file items by path).""" + if not inspect(file_group).attrs.primary_path.history.deleted: + return False + session = Session.object_session(file_group) + return session.connection().execute( + text('SELECT 1 FROM collection_item JOIN collection ON collection.id = collection_item.collection_id' + " WHERE collection_item.file_group_id = :id AND collection.kind = 'playlist' LIMIT 1"), + {'id': file_group.id}).first() is not None + + +register_ownership('playlists.yaml', FileGroup, columns={'primary_path'}, row=_playlist_file_group_moved) + + class Directory(ModelHelper, Base): """A representation of a file directory in the media directory.""" __tablename__ = 'directory' diff --git a/wrolpi/files/worker.py b/wrolpi/files/worker.py index ecd3ae09..32141a02 100644 --- a/wrolpi/files/worker.py +++ b/wrolpi/files/worker.py @@ -759,6 +759,19 @@ class FileWorker(FileMoveMixin, FileReorganizeMixin): {'updates': updates}) if curs.fetchone(): count_config_change_curs(curs, 'tags.yaml') + # playlists.yaml saves file items by path too. + curs.execute( + '''SELECT 1 + FROM collection_item + JOIN collection ON collection.id = collection_item.collection_id + JOIN file_group ON file_group.id = collection_item.file_group_id + JOIN json_each(:updates) AS v ON file_group.id = json_extract(v.value, '$[0]') + WHERE collection.kind = 'playlist' + AND file_group.primary_path != json_extract(v.value, '$[1]') + LIMIT 1''', + {'updates': updates}) + if curs.fetchone(): + count_config_change_curs(curs, 'playlists.yaml') curs.execute( '''UPDATE file_group SET primary_path = v.new_path diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 096cea93..d94fdd89 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -14,7 +14,7 @@ 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, \ get_relative_to_media_directory, is_valid_hex_color, walk, INVALID_FILE_CHARS, get_wrolpi_config -from wrolpi.config_import import ConfigSection, SectionDiff, chunks +from wrolpi.config_import import ConfigSection, SectionDiff, chunks, media_relative from wrolpi.config_version import count_config_change, register_ownership from wrolpi.dates import TZDateTime from wrolpi.db import get_db_session, named_placeholders, session_curs, serializer @@ -307,12 +307,6 @@ def _utc_or_none(value) -> Optional[datetime]: return value.astimezone(pytz.UTC) if value.tzinfo else value.replace(tzinfo=pytz.UTC) -def _relative_path(path) -> str: - """A path as the config names it: relative to the media directory.""" - path = pathlib.Path(path) - 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 @@ -398,7 +392,7 @@ class TagFilesSection(ConfigSection): for item in config.get('tag_files') or []: try: tag_name, path, *rest = item - key = (tag_name, _relative_path(path)) + key = (tag_name, media_relative(path)) except Exception as e: logger.error(f'Skipping invalid tagged file in config: {item}', exc_info=e) continue @@ -497,7 +491,7 @@ class TagZimsSection(ConfigSection): for item in config.get('tag_zims') or []: try: tag_name, zim_path, zim_entry, *rest = item - key = (tag_name, _relative_path(zim_path), zim_entry) + key = (tag_name, media_relative(zim_path), zim_entry) except Exception as e: logger.error(f'Skipping invalid tagged Zim entry in config: {item}', exc_info=e) continue From 5cdb4aaac0c07bc529d735d36c687e286ac71b44 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:35:57 -0600 Subject: [PATCH 2/9] repos.yaml imports by diffing (#870) * repos.yaml imports by diffing repos.yaml declares one section, its Repos keyed by repo_url_key (another URL of the same Repo is that Repo), comparing name, description, directory, Tag, mode, branch, submodules, frequency and clone token (a missing directory or clone token keeps the database's). An unchanged import writes nothing (every import rewrote every Repo and its Download). An invalid entry is skipped and counted, and its Repo is kept; before, a Repo whose entry failed validation was deleted. New Repos are created inside the import's one write: recurring_download (via get_or_create_download and _set_repository_download) takes commit=False, and the URL is not looked up again for each new Repo (O(N^2)). Co-Authored-By: Claude Opus 5.5 * repos.yaml: a branch URL is keyed by its Repo's URL file_rows keyed the raw URL, but stores parse_repo_url's URL, which turns a forge's branch URL (GitHub /tree/, GitLab /-/tree/) into the repo URL and a branch. The entry looked like a new Repo: its insert hit the existing Repo's name (rolling back every import of that file), and an invalid such entry did not protect its Repo from deletion. Rows and invalid keys now use the parsed URL; a URL that cannot be parsed is skipped by its raw key. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- modules/repos/lib.py | 227 ++++++++++++----------- modules/repos/test/test_config_import.py | 151 +++++++++++++++ modules/repos/test/test_lib.py | 8 +- modules/repos/test/test_search.py | 4 +- wrolpi/collections/models.py | 5 +- wrolpi/downloader.py | 9 +- 6 files changed, 291 insertions(+), 113 deletions(-) create mode 100644 modules/repos/test/test_config_import.py diff --git a/modules/repos/lib.py b/modules/repos/lib.py index b56ef585..1dddd1b2 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -14,10 +14,10 @@ import subprocess import tempfile from dataclasses import dataclass, field from datetime import datetime, timedelta, timezone -from typing import Callable, ContextManager, List, NamedTuple, Optional, Tuple +from typing import Callable, ContextManager, Dict, List, NamedTuple, Optional, Set, Tuple from urllib.parse import urlparse -from sqlalchemy import func, text +from sqlalchemy import func, select, text from sqlalchemy.orm import Session from wrolpi import fts @@ -25,7 +25,8 @@ from wrolpi.cmd import GIT_BIN, run_command from wrolpi.collections.models import Collection from wrolpi.common import ConfigFile, get_media_directory, get_relative_to_media_directory, logger, \ escape_file_name, background_task, get_wrolpi_config -from wrolpi.config_version import IMPORT, SAVE, register_ownership +from wrolpi.config_import import ConfigSection, media_relative +from wrolpi.config_version import register_ownership from wrolpi.db import get_db_session from wrolpi.downloader import Download, DownloadFrequency, download_manager, save_downloads_config from wrolpi.errors import UnknownDirectory, ValidationError @@ -45,7 +46,7 @@ __all__ = ['ParsedRepoURL', 'parse_repo_url', 'repo_url_key', 'git_command', 'gi 'find_readme', 'repos_destination_root', 'validate_repos_destination', 'is_valid_branch', 'submodule_url_error', 'HostAddresses', 'resolve_repo_addresses', 'pin_config', 'is_public_address', 'submodule_fetch_config', 'read_readme_text', 'search_repos', 'count_repos', 'search_repos_by_name', - 'get_repo_log', 'get_repo_root', 'repo_archive_name', 'stream_repo_archive', 'ReposConfig', + 'get_repo_log', 'get_repo_root', 'repo_archive_name', 'stream_repo_archive', 'ReposConfig', 'ReposSection', 'repos_config', 'get_repos_config', 'save_repos_config', 'import_repos_config', 'DEFAULT_REPO_FREQUENCY'] DEFAULT_REPO_FREQUENCY = DownloadFrequency.weekly.value @@ -734,9 +735,11 @@ def create_repository(session: Session, url: str, tag_name: str = None, frequenc return repo -def _set_repository_download(session: Session, repo: Repository, frequency: int, clone: bool = False): +def _set_repository_download(session: Session, repo: Repository, frequency: int, clone: bool = False, + commit: bool = True): """Create (or change) the recurring Download which keeps the Repo up to date. A Repo which is never updated - (`frequency` is NEVER) has none; with `clone`, it is cloned once if its clone is missing.""" + (`frequency` is NEVER) has none; with `clone`, it is cloned once if its clone is missing. `commit`: False + leaves committing to the caller's write.""" if not frequency: if download := repo.download: # A deleted object stays in loaded collections; remove it so the Repo reads as never updated. @@ -747,7 +750,8 @@ def _set_repository_download(session: Session, repo: Repository, frequency: int, return once = repo.once_download - download = repo.collection.get_or_create_download(session, repo.url, frequency, downloader_name='git') + download = repo.collection.get_or_create_download(session, repo.url, frequency, downloader_name='git', + commit=commit) # The Download may already exist (e.g. imported from download_manager.yaml, or the Repo's one-time Download). download.frequency = frequency if once: @@ -766,8 +770,9 @@ def _create_repository_download(session: Session, repo: Repository, frequency: i def _create_repository_records(session: Session, url: str, tag_name: str, mode: str, branch: Optional[str], name: Optional[str], description: Optional[str], directory: Optional[pathlib.Path], - submodules: bool, clone_token: str = None) -> Repository: - """Create (but do not commit) a Repo's Collection and Repository, in their directory.""" + submodules: bool, clone_token: str = None, check_url: bool = True) -> Repository: + """Create (but do not commit) a Repo's Collection and Repository, in their directory. `check_url`: refuse a URL + another Repo has (the config import already knows it is new).""" parsed = parse_repo_url(url) _validate_mode(mode) branch = branch or parsed.branch or None @@ -776,7 +781,7 @@ def _create_repository_records(session: Session, url: str, tag_name: str, mode: if name in ('', '.', '..'): raise InvalidRepo(f'Invalid repo name: {name!r}') - if get_repository_by_url(session, parsed.url): + if check_url and get_repository_by_url(session, parsed.url): raise RepoConflict(f'A repo with this URL already exists: {parsed.url}') if session.query(Collection).filter_by(kind='repo', name=name).one_or_none(): suggestion = f'{parsed.owner.replace("/", "-")}-{name}' if parsed.owner else f'{parsed.host}-{name}' @@ -1368,6 +1373,113 @@ def get_repo_size(directory: pathlib.Path) -> int: return total +def _repo_row(url, name, description, directory, tag_name, mode, branch, submodules, frequency, clone_token) -> dict: + return dict( + url=url, + name=name, + description=description or None, + directory=media_relative(directory) if directory else None, + tag_name=tag_name or None, + mode=mode, + branch=branch or None, + submodules=bool(submodules), + frequency=frequency or NEVER, + clone_token=clone_token or None, + ) + + +class ReposSection(ConfigSection): + """repos.yaml's Repos (a Collection with kind 'repo', its Repository, and its recurring Download), keyed by + `repo_url_key`, so another URL of the same Repo is that Repo. An invalid entry is skipped, and kept.""" + name = 'repos' + + def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: + media_directory = get_media_directory() + rows, invalid = dict(), set() + for position, data in enumerate(config.get('repos') or []): + url = data.get('url') + if not url: + logger.error(f'Repo config entry has no url, skipping: {data}') + # Counted as skipped; never a Repo's key. + invalid.add(f'\0{position}') + continue + try: + parsed = parse_repo_url(url) + except Exception as e: + logger.error(f'Skipping invalid repo in config: {data}', exc_info=e) + invalid.add(repo_url_key(url)) + continue + # The URL that is stored: a forge's branch URL is its Repo's URL (and a branch). + key = repo_url_key(parsed.url) + try: + if key in rows: + raise InvalidRepo(f'Another entry has the same repo: {url}') + mode = data.get('mode') or 'full' + _validate_mode(mode) + frequency = data.get('frequency') + frequency = NEVER if frequency in (NEVER_CONFIG, NEVER) else (frequency or DEFAULT_REPO_FREQUENCY) + _validate_frequency(frequency) + directory = media_directory / data['directory'] if data.get('directory') else None + if directory: + _validate_directory(directory) + branch = data.get('branch') or parsed.branch or None + _validate_branch(branch) + rows[key] = _repo_row(parsed.url, data.get('name') or parsed.name, data.get('description'), + directory, data.get('tag_name'), mode, branch, data.get('submodules'), + frequency, data.get('clone_token')) + except Exception as e: + logger.error(f'Skipping invalid repo in config: {data}', exc_info=e) + invalid.add(key) + return rows, invalid + + def db_rows(self, session: Session) -> Dict[str, dict]: + # The recurring Download (see `Repository.download`). + frequency = select(Download.frequency) \ + .where(Download.collection_id == Collection.id, Download.frequency > 0) \ + .limit(1).scalar_subquery() + query = session.query(Repository.url, Collection.name, Collection.description, Collection.directory, + Tag.name, Repository.mode, Repository.branch, Repository.submodules, frequency, + Repository.clone_token) \ + .join(Collection, Collection.id == Repository.collection_id) \ + .outerjoin(Tag, Tag.id == Collection.tag_id) + return {repo_url_key(url): _repo_row(url, *values) for url, *values in query} + + def same(self, file_row: dict, db_row: dict) -> bool: + # The key is the URL. A file without a directory or clone token keeps the database's. + return all(value == db_row[name] for name, value in file_row.items() + if name != 'url' and not (name in ('directory', 'clone_token') and value is None)) + + def upsert(self, session: Session, rows: Dict[str, dict]): + media_directory = get_media_directory() + existing = {repo_url_key(i.url): i for i in session.query(Repository)} + for key, row in rows.items(): + directory = media_directory / row['directory'] if row['directory'] else None + if not (repo := existing.get(key)): + repo = _create_repository_records(session, row['url'], row['tag_name'], row['mode'], row['branch'], + row['name'], row['description'], directory, row['submodules'], + row['clone_token'], check_url=False) + _set_repository_download(session, repo, row['frequency'], clone=True, commit=False) + continue + collection = repo.collection + collection.name = row['name'] + repo.update_search_name() + collection.description = row['description'] + collection.tag = Tag.get_or_create_tag(session, row['tag_name']) if row['tag_name'] else None + if directory: + collection.directory = directory + repo.mode, repo.branch, repo.submodules = row['mode'], row['branch'], row['submodules'] + if row['clone_token']: + repo.clone_token = row['clone_token'] + _set_repository_download(session, repo, row['frequency'], commit=False) + session.flush() + + def delete(self, session: Session, keys: Set[str]): + for repo in session.query(Repository): + if repo_url_key(repo.url) in keys: + logger.info(f'Deleting repo {repo.name!r} (no longer in config)') + _delete_repository(session, repo) + + @dataclass class ReposConfigValidator: version: int = 0 @@ -1389,6 +1501,7 @@ class ReposConfig(ConfigFile): frequency: 604800 """ file_name = 'repos.yaml' + sections = (ReposSection(),) validator = ReposConfigValidator default_config = dict(version=0, repos=[]) width = 120 @@ -1397,100 +1510,6 @@ class ReposConfig(ConfigFile): def repos(self) -> List[dict]: return self._config.get('repos', []) - def import_config(self, file: pathlib.Path = None, send_events=False): - """Import Repos from the config into the database.""" - file = file or self.get_file() - file_str = str(self.get_relative_file()) - - # A missing config is not an empty config; never delete Repos because a file is lost. - if not file.is_file(): - logger.info(f'No repos config file, skipping import: {file_str}') - self.successful_import = True - return - - super().import_config(file, send_events) - - decision = self.import_decision() - if decision == SAVE: - self.save_database_instead() - return - - repos_data = self._config.get('repos', []) - - # An empty repos list never deletes DB records. Deleting all repos is done through the API. - if not repos_data: - logger.info(f'No repos in config, preserving existing DB repos: {file_str}') - self.successful_import = True - return - - try: - with get_db_session(commit=True) as session: - imported_ids = set() - for data in repos_data: - try: - repo = self._import_repo(session, data) - except Exception as e: - logger.error(f'Failed to import repo from config: {data}', exc_info=e) - self.import_skipped += 1 - continue - imported_ids.add(repo.id) - - for repo in session.query(Repository).all() if decision == IMPORT else []: - if repo.id not in imported_ids: - logger.info(f'Deleting repo {repo.name!r} (no longer in config)') - _delete_repository(session, repo) - - logger.info(f'Imported {len(imported_ids)} repos from {file_str}') - self.record_import_version() - self.successful_import = True - except Exception as e: - self.successful_import = False - message = f'Failed to import {file_str} config!' - logger.error(message, exc_info=e) - if send_events: - Events.send_config_import_failed(message) - raise - - @staticmethod - def _import_repo(session: Session, data: dict) -> Repository: - parsed = parse_repo_url(data['url']) - name = data.get('name') or parsed.name - mode = data.get('mode') or 'full' - _validate_mode(mode) - frequency = data.get('frequency') - frequency = NEVER if frequency in (NEVER_CONFIG, NEVER) else (frequency or DEFAULT_REPO_FREQUENCY) - _validate_frequency(frequency) - directory = data.get('directory') - directory = get_media_directory() / directory if directory else None - if directory: - _validate_directory(directory) - tag_name = data.get('tag_name') - - repo = get_repository_by_url(session, parsed.url) - if not repo: - return create_repository(session, parsed.url, tag_name=tag_name, frequency=frequency, mode=mode, - branch=data.get('branch'), name=name, description=data.get('description'), - directory=directory, submodules=bool(data.get('submodules')), - clone_token=data.get('clone_token')) - - collection = repo.collection - collection.name = name - repo.update_search_name() - collection.description = data.get('description') - collection.tag = Tag.get_or_create_tag(session, tag_name) if tag_name else None - if directory: - collection.directory = directory - repo.mode = mode - repo.submodules = bool(data.get('submodules')) - if data.get('clone_token'): - repo.clone_token = data['clone_token'] - branch = data.get('branch') or None - _validate_branch(branch) - repo.branch = branch - _set_repository_download(session, repo, frequency) - session.flush() - return repo - def dump_config(self, file: pathlib.Path = None, send_events=False, overwrite=False): """Dump all Repos to the config file.""" with get_db_session() as session: diff --git a/modules/repos/test/test_config_import.py b/modules/repos/test/test_config_import.py new file mode 100644 index 00000000..caf400d3 --- /dev/null +++ b/modules/repos/test/test_config_import.py @@ -0,0 +1,151 @@ +"""repos.yaml imports by diffing its Repos against the database, and writing only the differences (see +wrolpi.config_import). A Repo is keyed by its URL (`repo_url_key`).""" +import pytest +import yaml + +from modules.repos import lib +from modules.repos.models import Repository +from wrolpi.config_version import get_config_version +from wrolpi.downloader import DownloadFrequency +from wrolpi.test.test_config_import import edit_config, statements, writes + +ONE = 'https://github.com/a/one' +TWO = 'https://github.com/a/two' + + +@pytest.fixture +async def saved_repos(test_session, async_client, test_wrolpi_config, await_switches): + """Repo `one` (tagged, daily) and `two` (a snapshot of a branch), saved to the config (which records its + version).""" + lib.create_repository(test_session, ONE, tag_name='software', description='One', frequency=DownloadFrequency.daily) + lib.create_repository(test_session, TWO, mode='snapshot', branch='dev') + await await_switches() + config = lib.get_repos_config() + config.dump_config() + await await_switches() + assert get_config_version(test_session, 'repos.yaml') is not None + return config.get_file() + + +def edit_repo(path, repo_url, **values): + def change(data): + next(i for i in data["repos"] if i["url"] == repo_url).update(values) + + edit_config(path, change) + + +def repos(test_session) -> dict: + test_session.expire_all() + return {i.url: i for i in test_session.query(Repository)} + + +@pytest.mark.asyncio +async def test_unchanged_import_writes_nothing(test_session, saved_repos): + """Importing a config the database already matches opens no write transaction.""" + config = lib.get_repos_config() + with statements() as collected: + config.import_config() + + assert config.successful_import is True + assert writes(collected) == [] + # The version, then one snapshot of the Repos. + assert len(collected) <= 3, collected + + +@pytest.mark.asyncio +async def test_same_repo_by_another_url_is_unchanged(test_session, saved_repos): + """A URL of the same Repo (a `.git` suffix, the host's case) is that Repo.""" + edit_repo(saved_repos, TWO, url='https://GitHub.com/a/two.git') + config = lib.get_repos_config() + with statements() as collected: + config.import_config() + + assert writes(collected) == [] + assert set(repos(test_session)) == {ONE, TWO} + + +@pytest.mark.asyncio +async def test_import_writes_only_the_differences(test_session, saved_repos): + """A changed Repo is updated, a removed one is deleted (with its Download), an added one is created.""" + def change(data): + one = next(i for i in data['repos'] if i['url'] == ONE) + one.update(description='Changed', frequency='never', mode='snapshot', branch='main') + data['repos'] = [one, dict(url='https://github.com/a/three', mode='full')] + + edit_config(saved_repos, change) + config = lib.get_repos_config() + config.import_config() + + assert config.successful_import is True + found = repos(test_session) + assert set(found) == {ONE, 'https://github.com/a/three'} + one = found[ONE] + assert (one.collection.description, one.mode, one.branch, one.download) == ('Changed', 'snapshot', 'main', None) + assert found['https://github.com/a/three'].download.frequency == lib.DEFAULT_REPO_FREQUENCY + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_invalid_entry_is_kept(test_session, saved_repos): + """A Repo whose entry is invalid is skipped, not deleted; the others are still imported.""" + edit_repo(saved_repos, TWO, mode='bogus') + edit_repo(saved_repos, ONE, description='Changed') + + config = lib.get_repos_config() + config.import_config() + + assert config.import_skipped == 1 + found = repos(test_session) + assert set(found) == {ONE, TWO} + assert (found[TWO].mode, found[ONE].collection.description) == ('snapshot', 'Changed') + + +@pytest.mark.asyncio +async def test_failed_import_creates_nothing(test_session, saved_repos, monkeypatch): + """A new Repo is created in the import's one write: a failure later in the write leaves nothing (each new Repo + was committed on its own).""" + edit_config(saved_repos, lambda data: data['repos'].append(dict(url='https://github.com/a/three'))) + edit_config(saved_repos, lambda data: data.update(repos=[i for i in data['repos'] if i['url'] != TWO])) + + def fail(*args, **kwargs): + raise RuntimeError('Write failed') + + monkeypatch.setattr(lib, '_delete_repository', fail) + with pytest.raises(RuntimeError): + lib.get_repos_config().import_config() + + assert set(repos(test_session)) == {ONE, TWO} + + +@pytest.mark.asyncio +async def test_branch_url_is_its_repo(test_session, saved_repos): + """A forge's branch URL (GitHub `/tree/`, GitLab `/-/tree/`) is its Repo, with that branch; + the next import writes nothing.""" + edit_repo(saved_repos, TWO, url=f'{TWO}/tree/main', branch=None) + edit_config(saved_repos, lambda data: data['repos'].append( + dict(url='https://gitlab.com/group/sub/project/-/tree/dev'))) + + config = lib.get_repos_config() + config.import_config() + + assert config.successful_import is True + found = repos(test_session) + assert set(found) == {ONE, TWO, 'https://gitlab.com/group/sub/project'} + assert (found[TWO].branch, found['https://gitlab.com/group/sub/project'].branch) == ('main', 'dev') + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_invalid_branch_url_entry_is_kept(test_session, saved_repos): + """An invalid entry named by its Repo's branch URL keeps that Repo.""" + edit_repo(saved_repos, TWO, url=f'{TWO}/tree/main', mode='bogus') + + config = lib.get_repos_config() + config.import_config() + + assert config.import_skipped == 1 + assert set(repos(test_session)) == {ONE, TWO} diff --git a/modules/repos/test/test_lib.py b/modules/repos/test/test_lib.py index 454bb520..46859a76 100644 --- a/modules/repos/test/test_lib.py +++ b/modules/repos/test/test_lib.py @@ -277,11 +277,11 @@ async def test_repo_directory_traversal(test_session, test_directory, async_clie lib.create_repository(test_session, 'https://github.com/a/b', directory=test_directory) test_session.rollback() - # The config can not move an existing Repo outside the media directory. + # The config can not move an existing Repo outside the media directory: the entry is invalid (and skipped). lib.create_repository(test_session, 'https://github.com/a/b') - with pytest.raises(InvalidRepo): - lib.ReposConfig._import_repo(test_session, dict(url='https://github.com/a/b', directory='../outside')) - test_session.rollback() + entry = dict(url='https://github.com/a/b', directory='../outside') + rows, invalid = lib.ReposSection().file_rows(dict(repos=[entry])) + assert (rows, invalid) == (dict(), {lib.repo_url_key('https://github.com/a/b')}) assert test_session.query(Repository).one().directory == test_directory / 'repos/b' diff --git a/modules/repos/test/test_search.py b/modules/repos/test/test_search.py index 9ff59f0d..78c5ac34 100644 --- a/modules/repos/test/test_search.py +++ b/modules/repos/test/test_search.py @@ -63,7 +63,9 @@ async def test_search_index_follows_changes(test_session, test_directory, async_ assert lib.count_repos(test_session, 'first') == 0 assert lib.count_repos(test_session, 'second') == 1 - lib.ReposConfig._import_repo(test_session, dict(url='https://github.com/a/b', name='renamed')) + section = lib.ReposSection() + rows, _ = section.file_rows(dict(repos=[dict(url='https://github.com/a/b', name='renamed')])) + section.upsert(test_session, rows) test_session.commit() assert lib.count_repos(test_session, 'renamed') == 1 diff --git a/wrolpi/collections/models.py b/wrolpi/collections/models.py index 047cb35c..d43ed432 100644 --- a/wrolpi/collections/models.py +++ b/wrolpi/collections/models.py @@ -250,7 +250,8 @@ class Collection(ModelHelper, Base): def get_or_create_download(self, session: Session, url: str, frequency: int, reset_attempts: bool = False, downloader_name: str = None, - sub_downloader_name: str = None) -> 'Download': + sub_downloader_name: str = None, + commit: bool = True) -> 'Download': """Get or create a Download for this Collection. Args: @@ -260,6 +261,7 @@ class Collection(ModelHelper, Base): reset_attempts: Reset download attempts if True downloader_name: Name of the downloader to use sub_downloader_name: Name of the sub-downloader for child downloads + commit: Commit a new Download (False: the caller's write commits) Returns: The Download instance @@ -283,6 +285,7 @@ class Collection(ModelHelper, Base): sub_downloader_name=sub_downloader_name, destination=str(self.directory) if self.directory else None, reset_attempts=reset_attempts, + commit=commit, ) if reset_attempts: download.attempts = 0 diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 49523175..559c7ec6 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -1043,8 +1043,8 @@ class DownloadManager: sub_downloader_name: str = None, reset_attempts: bool = False, destination: str | pathlib.Path = None, tag_names: List[str] = None, settings: Dict = None, collection_id: int = None, - override_skip: bool = False) -> Download: - """Schedule a recurring download.""" + override_skip: bool = False, commit: bool = True) -> Download: + """Schedule a recurring download. `commit`: False only flushes (the caller's write commits).""" if not frequency or not isinstance(frequency, int): raise ValueError('Recurring download must have a frequency!') @@ -1065,7 +1065,10 @@ class DownloadManager: if channel := Channel.get_by_url(session, url=download.url): download.collection_id = channel.collection_id - session.commit() + if commit: + session.commit() + else: + session.flush([download]) return download From 57c2a29022be8798da8f9389b7eb97259588ff0b Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 11:41:46 -0600 Subject: [PATCH 3/9] domains.yaml imports by diffing (#871) * domains.yaml imports by diffing domains.yaml declares one section, its domain Collections keyed by name, comparing description, directory, Tag and file format. An unchanged import writes nothing. A removed domain without Archives is deleted; one with Archives is reset to its default settings (decision 7c: a refresh of its Archives would re-create it), and the config is saved with it. An invalid entry (name or directory) is skipped, counted, and its domain kept. Once tags.yaml is imported, a domain's Tag that no longer exists is dropped and the config saved without it (`unknown_tag_names`). Linking Downloads to domains reads first (the domains once, not one query per RSS Download) and opens a short write only when there is something to link; the config is then saved with the links. Co-Authored-By: Claude Opus 5.5 * test_bookmarks_config_import starts the app like its siblings It imports a config without `async_client`, so it fails (no shared_ctx.configs_imported) whenever it is the first test on its worker. Co-Authored-By: Claude Opus 5.5 * domains.yaml: converge without a save; no links while entries are skipped A save after the import is refused while an entry is skipped, so the import must converge without one: - a removed domain with Archives that is already reset is left as it is (prepare reads `import_db_rows`); before, every import reset it again; - after prepare, the framework drops changed rows that now match the database (a dropped unknown Tag), so they are not written again. Linking Downloads changes domains.yaml after its version is recorded; while entries are skipped the save is refused, the database stays newer, and the next startup would save over the skipped entries. Linking now waits for an import without skipped entries. An entry that is not a mapping is skipped and counted, instead of failing the whole file. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- modules/archive/lib.py | 297 +++++++++--------- .../test/test_domains_config_import.py | 233 ++++++++++++++ wrolpi/common.py | 9 +- wrolpi/tags.py | 8 + wrolpi/test/test_bookmarks.py | 2 +- 5 files changed, 407 insertions(+), 142 deletions(-) create mode 100644 modules/archive/test/test_domains_config_import.py diff --git a/modules/archive/lib.py b/modules/archive/lib.py index d852829c..78adffed 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -12,7 +12,7 @@ import zipfile from dataclasses import dataclass, field from datetime import datetime from json import JSONDecodeError -from typing import Optional, Tuple, List, Union, Dict +from typing import Optional, Tuple, List, Union, Dict, Set from urllib.parse import urlparse import pytz @@ -27,7 +27,8 @@ from wrolpi.collections import Collection from wrolpi.common import get_media_directory, get_relative_to_media_directory, logger, extract_domain, \ escape_file_name, aiohttp_post, format_html_string, split_lines_by_length, get_html_soup, get_title_from_html, \ get_wrolpi_config, html_screenshot, html_file_screenshot, ConfigFile, trim_file_name, chunks -from wrolpi.config_version import IMPORT, SAVE +from wrolpi.collections.models import validate_collection_directory +from wrolpi.config_import import ConfigSection, media_relative from wrolpi.dates import now, Seconds from wrolpi.db import get_db_session, get_db_curs, session_curs from wrolpi.errors import UnknownArchive, InvalidOrderBy, InvalidDatetime @@ -36,7 +37,7 @@ from wrolpi.files.lib import handle_file_group_search_results, cached_search_tot search_filter_cache_key, count_file_groups, get_kv_value, set_kv_value from wrolpi.files.models import FileGroup from wrolpi.switches import register_switch_handler, ActivateSwitchMethod -from wrolpi.tags import tag_append_sub_select_where +from wrolpi.tags import Tag, tag_append_sub_select_where, unknown_tag_names from wrolpi.vars import PYTEST, DOCKERIZED logger = logger.getChild(__name__) @@ -46,6 +47,81 @@ __all__ = ['DomainsConfig', 'domains_config', 'get_domains_config', 'save_domain ARCHIVE_SERVICE = 'http://archive:8080' +def _domain_row(description, directory, tag_name, file_format) -> dict: + return dict( + description=description or None, + directory=media_relative(directory) if directory else None, + tag_name=tag_name or None, + file_format=file_format or None, + ) + + +class DomainsSection(ConfigSection): + """domains.yaml's domain Collections, keyed by name. (Their Downloads are linked after the import, see + `link_domain_and_downloads`.)""" + name = 'domains' + + def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: + rows, invalid = dict(), set() + for position, data in enumerate(config.get('collections') or []): + name = data.get('name') if isinstance(data, dict) else None + if not name: + logger.error(f'Domain collection at index {position} has no name, skipping: {data!r}') + # Counted as skipped; never a domain's name. + invalid.add(f'\0{position}') + continue + try: + if not Collection.is_valid_domain_name(name): + raise ValueError(f"Invalid domain name {name!r} (must contain at least one '.' and not start/end" + f" with '.')") + if name in rows: + raise ValueError(f'Another entry has the same domain: {name!r}') + directory = validate_collection_directory(data['directory']) if data.get('directory') else None + rows[name] = _domain_row(data.get('description'), directory, data.get('tag_name'), + data.get('file_format')) + except Exception as e: + logger.error(f'Skipping invalid domain collection at index {position}', exc_info=e) + invalid.add(name) + return rows, invalid + + def db_rows(self, session: Session) -> Dict[str, dict]: + query = session.query(Collection.name, Collection.description, Collection.directory, Tag.name, + Collection.file_format) \ + .outerjoin(Tag, Tag.id == Collection.tag_id) \ + .filter(Collection.kind == 'domain') + return {name: _domain_row(*values) for name, *values in query} + + def upsert(self, session: Session, rows: Dict[str, dict]): + existing = {i.name: i for i in + session.query(Collection).filter(Collection.kind == 'domain', Collection.name.in_(list(rows)))} + tags = {i.name: i for i in + session.query(Tag).filter(Tag.name.in_(list({i['tag_name'] for i in rows.values()} - {None})))} + for name, row in rows.items(): + if not (collection := existing.get(name)): + logger.info(f'Creating domain collection {name!r}') + collection = Collection(name=name, kind='domain') + session.add(collection) + collection.description = row['description'] + collection.directory = get_media_directory() / row['directory'] if row['directory'] else None + collection.tag = tags.get(row['tag_name']) + collection.file_format = row['file_format'] + session.flush() + + def delete(self, session: Session, keys: Set[str]): + """A domain with Archives is reset to its default settings (a refresh of its Archives would re-create it, + and deleting it would delete them); any other is deleted.""" + with_archives = {i for i, in session.query(Archive.collection_id).distinct()} + for collection in session.query(Collection).filter(Collection.kind == 'domain', + Collection.name.in_(list(keys))): + if collection.id in with_archives: + logger.warning(f'Resetting domain collection {collection.name!r} which is not in the config, because' + f' it has archives') + collection.description = collection.directory = collection.tag = collection.file_format = None + else: + logger.info(f'Deleting domain collection {collection.name!r} (no longer in config)') + session.delete(collection) + + @dataclass class DomainsConfigValidator: """Validator for domains config file.""" @@ -73,6 +149,7 @@ class DomainsConfig(ConfigFile): Items are managed dynamically when Archives are indexed. """ file_name = 'domains.yaml' + sections = (DomainsSection(),) validator = DomainsConfigValidator default_config = dict( version=0, @@ -92,99 +169,38 @@ class DomainsConfig(ConfigFile): """Get list of collection configs.""" return self._config.get('collections', []) + def prepare(self, diffs: dict): + """A Tag that no longer exists (it was removed) is dropped, and the config is saved without it. A removed + domain with Archives which is already reset (see `DomainsSection.delete`) is left as it is; one which is not + is reset, and the config is saved with it.""" + diff = diffs['domains'] + if diff.removed: + with get_db_session() as session: + with_archives = {name for name, in session.query(Collection.name).join( + Archive, Archive.collection_id == Collection.id).filter( + Collection.kind == 'domain', Collection.name.in_(list(diff.removed))).distinct()} + reset = _domain_row(None, None, None, None) + for name in with_archives: + if self.import_db_rows['domains'][name] == reset: + diff.removed.discard(name) + else: + self._config_outdated = True + + rows = [*diff.added.values(), *diff.changed.values()] + unknown = unknown_tag_names({i['tag_name'] for i in rows if i['tag_name']}) + for row in rows: + if row['tag_name'] in unknown: + logger.warning(f'Removing unknown Tag {row["tag_name"]!r} from a domain') + row['tag_name'] = None + self._config_outdated = True + def import_config(self, file: pathlib.Path = None, send_events=False): - """Import domain collections from config file into database using batch operations.""" - file = file or self.get_file() - file_str = str(get_relative_to_media_directory(file)) + self._config_outdated = False + super().import_config(file, send_events) - # If config file doesn't exist, mark import as successful (nothing to import) - if not file.is_file(): - logger.info('No domains config file, skipping import') - self.successful_import = True - return - - ConfigFile.import_config(self, file, send_events) - - decision = self.import_decision() - if decision == SAVE: - self.save_database_instead() - return - - collections_data = self._config.get('collections', []) - - # Empty collections list = never delete DB records - if not collections_data: - logger.info(f'No domain collections in config, preserving existing DB domain collections') - self.successful_import = True - return - - logger.info(f'Importing {len(collections_data)} domain collections from {file_str}') - - try: - with get_db_session(commit=True) as session: - # Validate and prepare data first - valid_data_list = [] - for idx, collection_data in enumerate(collections_data): - try: - name = collection_data.get('name') - if not name: - logger.error(f'Domain collection at index {idx} has no name, skipping') - self.import_skipped += 1 - continue - - # Validate domain name format before batching - if not Collection.is_valid_domain_name(name): - logger.error( - f"Domain collection at index {idx} has invalid name '{name}' " - f"(must contain at least one '.' and not start/end with '.'), skipping" - ) - self.import_skipped += 1 - continue - - # Ensure kind is 'domain' - collection_data = collection_data.copy() - collection_data['kind'] = 'domain' - - # Note: Collections can now be tagged even without a directory - # Tags enable UI search/filtering for directory-less collections - - valid_data_list.append(collection_data) - except Exception as e: - logger.error(f'Failed to validate domain collection at index {idx}', exc_info=e) - self.import_skipped += 1 - continue - - # Batch create/update collections - imported_domains = set() - if valid_data_list: - collections = Collection.batch_from_config(session, valid_data_list) - imported_domains = {c.name for c in collections} - - # Delete domain collections that are no longer in config. A domain with Archives is kept: Archives - # create their domain (e.g. one downloaded before this import), and deleting it would delete them. - with_archives = {i for i, in session.query(Archive.collection_id).distinct()} - all_domain_collections = session.query(Collection).filter_by(kind='domain').all() - for collection in all_domain_collections if decision == IMPORT else []: - if collection.name in imported_domains: - continue - if collection.id in with_archives: - logger.warning(f'Keeping domain collection {repr(collection.name)} which is not in config,' - f' because it has archives') - continue - logger.info(f'Deleting domain collection {repr(collection.name)} (no longer in config)') - session.delete(collection) - - logger.info(f'Successfully imported {len(imported_domains)} domain collections from {file_str}') - self.record_import_version() - self.successful_import = True - - except Exception as e: - self.successful_import = False - message = f'Failed to import {file_str} config!' - logger.error(message, exc_info=e) - if send_events: - Events.send_config_import_failed(message) - raise + def after_import(self, diffs: dict): + if self._config_outdated: + self.background_dump.activate_switch() def dump_config(self, file: pathlib.Path = None, send_events=False, overwrite=False): """Dump all domain collections from database to config file.""" @@ -295,13 +311,22 @@ save_domains_config: ActivateSwitchMethod def import_domains_config(): - """Import domain collections from config file into database.""" + """Import domain collections from config file into database, then link their Downloads (a short write, only + when there is something to link).""" logger.info('Importing domains config') domains_config.import_config() + if domains_config.import_skipped: + # Linking changes domains.yaml, which cannot be saved while it has skipped entries: the database would be + # newer than the file, and the next startup would save over them. + logger.warning('Not linking Downloads to domains while domains.yaml has skipped entries') + return - # Link downloads to domain collections after import with get_db_session() as session: - link_domain_and_downloads(session) + links = domain_download_links(session) + if links: + with get_db_session(commit=True) as session: + _link_downloads(session, links) + save_domains_config.activate_switch() logger.info('Importing domains config completed') @@ -978,60 +1003,52 @@ def get_or_create_domain_collection(session: Session, url, directory: pathlib.Pa return collection -def link_domain_and_downloads(session: Session): - """Associate any Download related to a Domain Collection. +def domain_download_links(session: Session) -> Dict[int, int]: + """The Downloads to link to a Domain Collection: {Download id: Collection id}. - Downloads are linked to Collections (via collection_id). - This function finds Domain Collections and links their Downloads. - - Matching criteria: - - Recurring downloads whose destination is within the domain collection's directory (including subdirectories) + Matching criteria (only recurring Downloads without a Collection): + - a destination within the domain collection's directory (including subdirectories) - RSS downloads with sub_downloader='archive' (matched by URL domain) """ # Local import to avoid circular import: downloader -> archive -> downloader from wrolpi.downloader import Download - # Only Downloads with a frequency can be a Collection Download. - downloads = list(session.query(Download).filter(Download.frequency.isnot(None)).all()) + domains = session.query(Collection.id, Collection.name, Collection.directory) \ + .filter(Collection.kind == 'domain').all() + by_name = {name: id_ for id_, name, _ in domains} + # Ensure each directory ends with / for proper prefix matching. + directories = [(id_, str(directory), str(directory).rstrip('/') + '/') + for id_, _, directory in domains if directory] - # Get domain collections that have a directory - domain_collections = session.query(Collection).filter( - Collection.kind == 'domain', - Collection.directory.isnot(None) - ).all() + links = dict() + downloads = session.query(Download.id, Download.url, Download.downloader, Download.sub_downloader, + Download.settings) \ + .filter(Download.frequency.isnot(None), Download.collection_id.is_(None)) + for id_, url, downloader, sub_downloader, settings in downloads: + if destination := (settings or dict()).get('destination'): + for collection_id, directory, prefix in directories: + if destination == directory or destination.startswith(prefix): + links[id_] = collection_id + break + if id_ not in links and downloader == 'rss' and sub_downloader == 'archive': + if collection_id := by_name.get(extract_domain(url)): + links[id_] = collection_id + return links - need_commit = False - # Match downloads by destination directory (including subdirectories) - downloads_with_destination = [d for d in downloads if (d.settings or {}).get('destination')] - for collection in domain_collections: - # Ensure directory ends with / for proper prefix matching - directory = str(collection.directory) - directory_prefix = directory if directory.endswith('/') else directory + '/' - for download in downloads_with_destination: - dest = download.settings['destination'] - # Match if destination equals directory or is a subdirectory - if not download.collection_id and (dest == directory or dest.startswith(directory_prefix)): - download.collection_id = collection.id - need_commit = True +def _link_downloads(session: Session, links: Dict[int, int]): + # Local import to avoid circular import: downloader -> archive -> downloader + from wrolpi.downloader import Download + for ids in chunks(list(links), 500): + for download in session.query(Download).filter(Download.id.in_(ids)): + download.collection_id = links[download.id] + session.flush() - # Match RSS downloads with archive sub_downloader by URL domain - rss_archive_downloads = [ - d for d in downloads - if d.downloader == 'rss' and d.sub_downloader == 'archive' and not d.collection_id - ] - for download in rss_archive_downloads: - # Extract domain from the RSS URL and find matching domain collection - domain_name = extract_domain(download.url) - collection = session.query(Collection).filter_by( - name=domain_name, - kind='domain' - ).one_or_none() - if collection: - download.collection_id = collection.id - need_commit = True - if need_commit: +def link_domain_and_downloads(session: Session): + """Associate any Download related to a Domain Collection (see `domain_download_links`).""" + if links := domain_download_links(session): + _link_downloads(session, links) session.commit() diff --git a/modules/archive/test/test_domains_config_import.py b/modules/archive/test/test_domains_config_import.py new file mode 100644 index 00000000..2bc2fc98 --- /dev/null +++ b/modules/archive/test/test_domains_config_import.py @@ -0,0 +1,233 @@ +"""domains.yaml imports by diffing its domain Collections against the database, and writing only the differences +(see wrolpi.config_import). A domain removed from the file is deleted, unless it has Archives: then it is reset to +its default settings (a refresh of its Archives would re-create it).""" +import pytest +import yaml + +from modules.archive.lib import get_domains_config, import_domains_config +from modules.archive.models import Archive +from wrolpi.collections import Collection +from wrolpi.config_version import get_config_version +from wrolpi.downloader import Download +from wrolpi.test.test_config_import import edit_config, statements, writes + + +@pytest.fixture +async def saved_domains(test_session, test_directory, async_client, archive_factory, tag_factory, await_switches): + """`example.com` (tagged, in a custom directory), `other.org`, and `archived.net` (which has an Archive and a + description), saved to the config (which records its version).""" + tag = await tag_factory('news') + archive_factory(domain='archived.net', url='https://archived.net/a') + test_session.commit() + archived = test_session.query(Collection).filter_by(name='archived.net', kind='domain').one() + archived.description = 'Has archives' + test_session.add_all([ + Collection(name='example.com', kind='domain', description='Example', tag_id=tag.id, + directory=test_directory / 'archive/news/example.com'), + Collection(name='other.org', kind='domain'), + ]) + test_session.commit() + get_domains_config().dump_config() + await await_switches() + assert get_config_version(test_session, 'domains.yaml') is not None + return get_domains_config().get_file() + + +def domains(test_session) -> dict: + test_session.expire_all() + return {i.name: i for i in test_session.query(Collection).filter_by(kind='domain')} + + +def edit_domain(path, name, **values): + def change(data): + next(i for i in data['collections'] if i['name'] == name).update(values) + + edit_config(path, change) + + +@pytest.mark.asyncio +async def test_unchanged_import_writes_nothing(test_session, saved_domains): + """Importing a config the database already matches opens no write transaction, nor does linking Downloads when + there are none to link.""" + config = get_domains_config() + with statements() as collected: + import_domains_config() + + assert config.successful_import is True + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_import_writes_only_the_differences(test_session, test_directory, saved_domains): + """A changed domain is updated, a removed one (without Archives) is deleted, an added one is created; the next + import writes nothing.""" + def change(data): + data['collections'] = [i for i in data['collections'] if i['name'] != 'other.org'] + data['collections'].append(dict(name='new.com', kind='domain', description='New')) + example = next(i for i in data['collections'] if i['name'] == 'example.com') + example.update(description='Changed', tag_name=None, directory='archive/example.com') + + edit_config(saved_domains, change) + config = get_domains_config() + config.import_config() + + assert config.successful_import is True + found = domains(test_session) + assert set(found) == {'example.com', 'archived.net', 'new.com'} + example = found['example.com'] + assert (example.description, example.tag_id, example.directory) == \ + ('Changed', None, test_directory / 'archive/example.com') + assert found['new.com'].description == 'New' + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_removed_domain_with_archives_is_reset(test_session, saved_domains, await_switches): + """A domain with Archives that is removed from the config keeps its Archives, and is reset to its default + settings; the config is saved with it, so the next import writes nothing.""" + edit_config(saved_domains, lambda data: data.update( + collections=[i for i in data['collections'] if i['name'] != 'archived.net'])) + + config = get_domains_config() + config.import_config() + await await_switches() + + archived = domains(test_session)['archived.net'] + assert archived.description is None + assert test_session.query(Archive).filter_by(collection_id=archived.id).count() == 1 + assert 'archived.net' in saved_domains.read_text() + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_invalid_entries_are_kept(test_session, saved_domains): + """An entry with an invalid name or directory is skipped (counted), and its domain is not deleted.""" + edit_domain(saved_domains, 'other.org', directory='../outside') + edit_config(saved_domains, lambda data: data['collections'].append(dict(name='nodot', kind='domain'))) + + config = get_domains_config() + config.import_config() + + assert config.import_skipped == 2 + assert set(domains(test_session)) == {'example.com', 'other.org', 'archived.net'} + + +@pytest.mark.asyncio +async def test_unknown_tag_is_dropped(test_session, saved_domains, test_tags_config, await_switches): + """Once tags.yaml is imported, a domain's Tag that no longer exists is dropped and the config is saved without + it: the next import writes nothing.""" + from wrolpi.tags import get_tags_config + get_tags_config().dump_config() + get_tags_config().import_config() + edit_domain(saved_domains, 'other.org', tag_name='gone') + + config = get_domains_config() + config.import_config() + await await_switches() + + assert domains(test_session)['other.org'].tag_id is None + assert 'gone' not in saved_domains.read_text() + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_downloads_are_linked(test_session, saved_domains, await_switches): + """After the import, an RSS archive Download of a domain is linked to it (the domains are read once, not once + per Download), and the config is saved with it.""" + test_session.add_all([ + Download(url='https://other.org/feed.xml', downloader='rss', sub_downloader='archive', frequency=86400), + Download(url='https://example.com/feed.xml', downloader='rss', sub_downloader='archive', frequency=86400), + ]) + test_session.commit() + + with statements() as collected: + import_domains_config() + await await_switches() + + found = domains(test_session) + assert {i.url: i.collection_id for i in test_session.query(Download)} == { + 'https://other.org/feed.xml': found['other.org'].id, + 'https://example.com/feed.xml': found['example.com'].id, + } + assert len([i for i in collected if 'FROM collection' in i and 'WHERE collection.name' in i]) == 0 + assert 'https://other.org/feed.xml' in saved_domains.read_text() + + +def add_invalid_entry(path): + edit_config(path, lambda data: data['collections'].append(dict(name='nodot', kind='domain'))) + + +@pytest.mark.asyncio +async def test_reset_with_a_skipped_entry_converges(test_session, saved_domains, await_switches): + """A skipped entry stops the config from being saved; a domain reset by this import is not reset again by the + next one (which writes nothing).""" + edit_config(saved_domains, lambda data: data.update( + collections=[i for i in data['collections'] if i['name'] != 'archived.net'])) + add_invalid_entry(saved_domains) + + config = get_domains_config() + config.import_config() + await await_switches() + assert domains(test_session)['archived.net'].description is None + + with statements() as collected: + config.import_config() + assert config.import_skipped == 1 + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_unknown_tag_with_a_skipped_entry_converges(test_session, saved_domains, test_tags_config, + await_switches): + """A dropped Tag the config cannot be saved without (an entry was skipped) is not dropped again.""" + from wrolpi.tags import get_tags_config + get_tags_config().dump_config() + get_tags_config().import_config() + edit_domain(saved_domains, 'other.org', tag_name='gone') + add_invalid_entry(saved_domains) + + config = get_domains_config() + config.import_config() + await await_switches() + + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_no_links_while_entries_are_skipped(test_session, saved_domains, await_switches): + """Linking changes domains.yaml; while an entry is skipped the config cannot be saved, so Downloads are not + linked (the database would be newer than the file, and the next startup would save over the skipped entry).""" + test_session.add(Download(url='https://other.org/feed.xml', downloader='rss', sub_downloader='archive', + frequency=86400)) + test_session.commit() + add_invalid_entry(saved_domains) + + import_domains_config() + await await_switches() + + test_session.expire_all() + assert test_session.query(Download).one().collection_id is None + assert get_config_version(test_session, 'domains.yaml') == yaml.safe_load(saved_domains.read_text())['version'] + assert 'nodot' in saved_domains.read_text() + + +@pytest.mark.asyncio +async def test_entry_that_is_not_a_mapping_is_skipped(test_session, saved_domains): + """An entry that is not a mapping is skipped (counted); the rest of the file is imported.""" + edit_config(saved_domains, lambda data: data['collections'].extend(['example.net', None])) + edit_domain(saved_domains, 'other.org', description='Changed') + + config = get_domains_config() + config.import_config() + + assert config.successful_import is True + assert config.import_skipped == 2 + assert domains(test_session)['other.org'].description == 'Changed' diff --git a/wrolpi/common.py b/wrolpi/common.py index 0a09f368..0d366c86 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -850,8 +850,15 @@ class ConfigFile: # `prepare` may skip rows it cannot apply. self.import_skipped = sum(len(invalid) for _, invalid in file_rows.values()) self.import_deletes = decision == IMPORT + self.import_db_rows = db_rows if any(diffs.values()): self.prepare(diffs) + # A row `prepare` brought in line with the database (e.g. it dropped a Tag the file names but no longer + # exists) is not written. + for section in self.sections: + changed = diffs[section.name].changed + for key in [i for i in changed if section.same(changed[i], db_rows[section.name][i])]: + del changed[key] file_version = self.version or 0 if any(diffs.values()) or (decision == IMPORT and db_version != file_version): @@ -892,7 +899,7 @@ class ConfigFile: """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 may drop rows it cannot apply (count them in `import_skipped`), or add what it found to a row for `upsert`. `import_deletes`: whether - this import deletes what the file no longer lists. + this import deletes what the file no longer lists. `import_db_rows`: each section's database rows. 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 diff --git a/wrolpi/tags.py b/wrolpi/tags.py index d94fdd89..2d38c5c9 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -333,6 +333,14 @@ def remove_tag_names_from_downloads(session: Session, names: Set[str]): save_downloads_config.activate_switch() +def unknown_tag_names(names: Set[str]) -> Set[str]: + """The names no Tag has (the Tag was removed). None until tags.yaml was imported: its Tags may be missing.""" + if not names or not get_tags_config().successful_import: + return set() + with get_db_session() as session: + return names - {i for i, in session.query(Tag.name).filter(Tag.name.in_(list(names)))} + + def _tag_ids(session: Session, names) -> Dict[str, int]: tag_ids = dict() for chunk in chunks(names): diff --git a/wrolpi/test/test_bookmarks.py b/wrolpi/test/test_bookmarks.py index 26f427e4..cc7ae858 100644 --- a/wrolpi/test/test_bookmarks.py +++ b/wrolpi/test/test_bookmarks.py @@ -138,7 +138,7 @@ async def test_bookmarks_move_guards(async_client, test_directory): [outer['id'], inner['id'], leaf['id']] -def test_bookmarks_config_import(test_directory, test_wrolpi_config): +async def test_bookmarks_config_import(async_client, test_directory, test_wrolpi_config): """A hand-edited bookmarks.yaml is imported as-is, including nested directories.""" config_file = test_directory / 'config/bookmarks.yaml' config_file.parent.mkdir(exist_ok=True) From 315b60e9eb180f3dd28f9ee45c3239e6bd56fcec Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 13:09:51 -0600 Subject: [PATCH 4/9] channels.yaml imports by diffing (#872) * channels.yaml imports by diffing channels.yaml declares two sections: Channels keyed by directory (comparing name, URL, Tag, file format, and the source id and flags, which keep the database's value when missing), and the Downloads listed under each Channel, keyed by (directory, URL); an existing Download keeps its own frequency. An unchanged import writes nothing (it ran about a thousand queries and committed several times). Removed Channels are deleted with their Collection (and Downloads); a Download removed from a Channel is deleted. A channel Collection without a Channel is deleted unless listed (then it gets its Channel). A Channel whose directory changed, with the same name, is moved. An invalid entry is skipped and counted, and keeps its Channel (by name when its directory is invalid) and that Channel's Downloads; so does an entry without a list of Downloads. Duplicate directories still refuse the import. Unknown Tags are dropped once tags.yaml is imported. New Downloads are created in the import's one write (commit=False on Channel.get_or_create_download, Download.delete and delete_with_videos). After the import, Videos are claimed and source ids fetched only for new and changed Channels; Downloads are linked without a query per Download, and not while entries are skipped. Co-Authored-By: Claude Opus 5.5 * channels.yaml: moves, invalid Downloads, and saves after the write - A moved Channel's Download diff is retargeted to its new directory: one its new entry lists is the same Download, one it does not is deleted in the same import (the old directory was treated as kept). - An unreadable Download is counted as skipped, and its Channel keeps its Downloads (it was dropped, and the Channel's Download deleted). - A new Download without a frequency is dropped before the write and counted (it re-opened a write on every import). - No source id is fetched while entries are skipped (its save would be refused, leaving the database newer than the file); a fetched one is saved with dump_config, which records the database's version. - Download.delete(commit=False) neither changes the skip list nor starts config saves, which would read the database before the import commits (and write a removed Download back into channels.yaml). The import saves download_manager.yaml after its write. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- modules/videos/lib.py | 546 +++++++++++------- modules/videos/models.py | 15 +- .../test/test_channels_config_import.py | 286 +++++++++ wrolpi/downloader.py | 9 +- 4 files changed, 631 insertions(+), 225 deletions(-) create mode 100644 modules/videos/test/test_channels_config_import.py diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 2a64b7aa..777c07db 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -6,11 +6,11 @@ import pathlib import re from dataclasses import dataclass, field from datetime import datetime -from typing import Generator, Type, List +from typing import Dict, Generator, Type, List, Set from typing import Tuple, Optional import pytz -from sqlalchemy.orm import Session +from sqlalchemy.orm import Session, joinedload from yt_dlp import YoutubeDL from modules.videos.models import Video @@ -18,14 +18,18 @@ from wrolpi import dates, flags from wrolpi.captions import extract_captions from wrolpi.common import ConfigFile, extract_domain, logger, \ escape_file_name, get_media_directory, background_task, Base, get_wrolpi_config, trim_file_name -from wrolpi.config_version import IMPORT, SAVE, register_ownership +from wrolpi.collections import Collection +from wrolpi.collections.models import validate_collection_directory +from wrolpi.config_import import ConfigSection, chunks, media_relative +from wrolpi.config_version import register_ownership from wrolpi.dates import Seconds, from_timestamp from wrolpi.db import get_db_curs, get_db_session, session_curs -from wrolpi.downloader import Download, download_manager +from wrolpi.downloader import Download, download_manager, save_downloads_config from wrolpi.errors import UnknownDirectory from wrolpi.events import Events from wrolpi.files.lib import split_path_stem_and_suffix from wrolpi.switches import register_switch_handler, ActivateSwitchMethod +from wrolpi.tags import Tag, unknown_tag_names from wrolpi.vars import YTDLP_CACHE_DIR, PYTEST from .common import is_valid_poster, convert_image, \ generate_video_poster, ConfigError, \ @@ -257,6 +261,189 @@ class ChannelDictValidator: url: str = None +def _channel_key(data: dict) -> str: + """A Channel's key: its directory, relative to the media directory.""" + if not isinstance(data, dict) or not data.get('directory'): + raise ValueError('A Channel requires a directory') + return media_relative(validate_collection_directory(data['directory'])) + + +# The key of a channel Collection without a Channel or a directory. +ORPHAN_KEY = '\0orphan:' +# Missing from an entry: the Channel keeps the database's value. +CHANNEL_KEPT_IF_MISSING = ('source_id', 'generate_posters', 'calculate_duration', 'download_missing_data') +CHANNEL_COMPARED = ('name', 'url', 'tag_name', 'file_format') + CHANNEL_KEPT_IF_MISSING + + +def _channel_row(name, url, source_id, tag_name, file_format, generate_posters, calculate_duration, + download_missing_data) -> dict: + return dict(name=name, url=url or None, source_id=source_id or None, tag_name=tag_name or None, + file_format=file_format or None, generate_posters=generate_posters, + calculate_duration=calculate_duration, download_missing_data=download_missing_data) + + +class ChannelsSection(ConfigSection): + """channels.yaml's Channels (a Collection with kind 'channel' and its Channel), keyed by directory. A channel + Collection without a Channel is in the database's rows too: listed, it gets its Channel; not listed, it is + deleted.""" + name = 'channels' + + def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: + rows, invalid = dict(), set() + for position, data in enumerate(config.get('channels') or []): + try: + key = _channel_key(data) + except Exception as e: + channel_import_logger.error(f'Skipping invalid Channel in config: {data!r}', exc_info=e) + # Counted as skipped; never a directory. (The entry's name keeps its Channel, see `prepare`.) + invalid.add(f'\0{position}') + continue + if key in rows or key in invalid: + raise RuntimeError('Refusing to import channels config because it contains duplicate directories!') + if not data.get('name'): + channel_import_logger.error(f'Channel {key!r} is required to have "name", skipping') + invalid.add(key) + continue + rows[key] = _channel_row(data['name'], data.get('url'), data.get('source_id'), data.get('tag_name'), + data.get('file_format'), data.get('generate_posters'), + data.get('calculate_duration'), data.get('download_missing_data')) + return rows, invalid + + def db_rows(self, session: Session) -> Dict[str, dict]: + query = session.query(Collection.directory, Collection.name, Channel.url, Channel.source_id, Tag.name, + Collection.file_format, Channel.generate_posters, Channel.calculate_duration, + Channel.download_missing_data, Collection.id, Channel.id) \ + .select_from(Collection) \ + .outerjoin(Channel, Channel.collection_id == Collection.id) \ + .outerjoin(Tag, Tag.id == Collection.tag_id) \ + .filter(Collection.kind == 'channel') + rows = dict() + for directory, *values, collection_id, channel_id in query: + if directory: + key = media_relative(directory) + elif channel_id is None: + # An orphan without a directory: no entry can list it (`ORPHAN_KEY`). + key = f'{ORPHAN_KEY}{collection_id}' + else: + continue + rows[key] = row = _channel_row(*values) + if channel_id is None: + row['orphan'] = True + return rows + + def same(self, file_row: dict, db_row: dict) -> bool: + if db_row.get('orphan') or file_row.get('directory'): + return False + return all(file_row[i] == db_row[i] for i in CHANNEL_COMPARED + if not (i in CHANNEL_KEPT_IF_MISSING and file_row[i] is None)) + + def upsert(self, session: Session, rows: Dict[str, dict]): + media_directory = get_media_directory() + collections = {media_relative(i.directory): i for i in session.query(Collection).filter( + Collection.kind == 'channel', Collection.directory.in_([str(media_directory / i) for i in rows]))} + channels = {i.collection_id: i for i in session.query(Channel).filter( + Channel.collection_id.in_([i.id for i in collections.values()]))} + tags = {i.name: i for i in + session.query(Tag).filter(Tag.name.in_(list({i['tag_name'] for i in rows.values()} - {None})))} + for key, row in rows.items(): + if not (collection := collections.get(key)): + collection = Collection(kind='channel') + session.add(collection) + # A moved Channel's new directory (see `ChannelsConfig.prepare`). + collection.directory = media_directory / row.get('directory', key) + collection.name = row['name'] + collection.tag = tags.get(row['tag_name']) + collection.file_format = row['file_format'] + if not (channel := channels.get(collection.id) if collection.id else None): + channel = Channel(collection=collection) + session.add(channel) + channel.url = row['url'] + for name in CHANNEL_KEPT_IF_MISSING: + if row[name] is not None: + setattr(channel, name, row[name]) + session.flush() + + def delete(self, session: Session, keys: Set[str]): + media_directory = get_media_directory() + orphan_ids = [int(i.removeprefix(ORPHAN_KEY)) for i in keys if i.startswith(ORPHAN_KEY)] + collections = session.query(Collection).filter( + Collection.kind == 'channel', + Collection.directory.in_([str(media_directory / i) for i in keys if not i.startswith(ORPHAN_KEY)]) + | Collection.id.in_(orphan_ids)).all() + channels = {i.collection_id: i for i in session.query(Channel).filter( + Channel.collection_id.in_([i.id for i in collections]))} + for collection in collections: + if channel := channels.get(collection.id): + logger.warning(f'Deleting {channel} because it is not in the config.') + channel.delete_with_videos(commit=False) + else: + logger.warning(f'Deleting orphaned Collection {collection.name} (id={collection.id})') + session.delete(collection) + session.flush() + + +class ChannelDownloadsSection(ConfigSection): + """The Downloads listed under each Channel, keyed by (Channel directory, URL). An existing Download keeps its own + frequency (download_manager.yaml's); the file's is used to create one.""" + name = 'downloads' + + def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: + rows, invalid = dict(), set() + for data in config.get('channels') or []: + try: + key = _channel_key(data) + except Exception: + continue + for position, download in enumerate(data.get('downloads') or []): + if isinstance(download, dict) and download.get('url'): + rows[(key, download['url'])] = dict(frequency=download.get('frequency')) + else: + channel_import_logger.error(f'Skipping invalid Download of Channel {key!r}: {download!r}') + # Counted as skipped; the Channel keeps its Downloads (see `ChannelsConfig.prepare`). + invalid.add((key, download if isinstance(download, str) else f'\0{position}')) + return rows, invalid + + def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: + # A Channel listed with an empty list of Downloads has none (see `ChannelsConfig.prepare` for entries without + # a list). + return bool(config.get('channels')) + + def db_rows(self, session: Session) -> Dict[tuple, dict]: + query = session.query(Collection.directory, Download.url, Download.frequency) \ + .join(Collection, Collection.id == Download.collection_id) \ + .filter(Collection.kind == 'channel', Collection.directory.isnot(None)) + return {(media_relative(directory), url): dict(frequency=frequency) for directory, url, frequency in query} + + def same(self, file_row: dict, db_row: dict) -> bool: + return True + + def upsert(self, session: Session, rows: Dict[tuple, dict]): + media_directory = get_media_directory() + channels = {media_relative(i.directory): i for i in session.query(Channel).join(Collection).filter( + Collection.directory.in_([str(media_directory / i) for i, _ in rows]))} + for (key, url), row in rows.items(): + if not (channel := channels.get(key)): + channel_import_logger.warning(f'Cannot find Channel {key!r} for Download {url}') + continue + if existing := Download.get_by_url(session, url): + existing.collection_id = channel.collection_id + elif row['frequency']: + channel.get_or_create_download(session, url, row['frequency'], reset_attempts=True, commit=False) + else: + logger.error(f'Refusing to create Download for Channel without frequency: {url}') + session.flush() + + def delete(self, session: Session, keys: Set[tuple]): + media_directory = get_media_directory() + query = session.query(Download, Collection.directory) \ + .join(Collection, Collection.id == Download.collection_id) \ + .filter(Collection.kind == 'channel', Download.url.in_([url for _, url in keys])) + for download, directory in query.all(): + if (media_relative(directory), download.url) in keys: + logger.warning(f'Deleting {download} because it is no longer in its Channel') + download.delete(add_to_skip_list=False, commit=False) + + @dataclass class ChannelsConfigValidator: version: int = None @@ -265,6 +452,7 @@ class ChannelsConfigValidator: class ChannelsConfig(ConfigFile): file_name = 'channels.yaml' + sections = (ChannelsSection(), ChannelDownloadsSection()) default_config = dict( channels=[dict( name='WROLPi', @@ -285,172 +473,116 @@ class ChannelsConfig(ConfigFile): self.update({'channels': value}) def import_config(self, file: pathlib.Path = None, send_events=False): - """Import channels from config file into database using batch operations.""" - file = file or self.get_file() + self._config_outdated = False + super().import_config(file, send_events) - # If config file doesn't exist, mark import as successful (nothing to import) - if not file.is_file(): - channel_import_logger.info('No channels config file, skipping import') - self.successful_import = True - return + def prepare(self, diffs: dict): + channels, downloads = diffs['channels'], diffs['downloads'] + db_rows = self.import_db_rows['channels'] - super().import_config(file) + # A Tag that no longer exists (it was removed) is dropped, and the config is saved without it. + rows = [*channels.added.values(), *channels.changed.values()] + unknown = unknown_tag_names({i['tag_name'] for i in rows if i['tag_name']}) + for row in rows: + if row['tag_name'] in unknown: + channel_import_logger.warning(f'Removing unknown Tag {row["tag_name"]!r} from a Channel') + row['tag_name'] = None + self._config_outdated = True - decision = self.import_decision() - if decision == SAVE: - self.save_database_instead() - return + # A Channel whose directory changed (its name did not) is moved, not deleted and re-created. + removed_by_name = dict() + for key in channels.removed: + removed_by_name.setdefault(db_rows[key]['name'], []).append(key) + moves = dict() + for key, row in list(channels.added.items()): + if len(old := removed_by_name.get(row['name']) or []) == 1: + channels.removed.discard(old[0]) + del channels.added[key] + channels.changed[old[0]] = dict(row, directory=key) + moves[old[0]] = key - try: - channels = self.channels - # Empty channels list = never delete DB records - if not channels: - channel_import_logger.info('No channels in config, preserving existing DB channels') - self.successful_import = True - return + # An invalid entry keeps its Channel (found by the entry's name when its directory is invalid), and the + # Channel's Downloads; so does an entry without a list of Downloads (an old config), or with an unreadable + # Download. + entries = [i for i in self._config.get('channels') or [] if isinstance(i, dict)] + invalid_names = {i.get('name') for i in entries if i.get('name')} - {i['name'] for i in self._channel_rows()} + for key in [i for i in channels.removed if db_rows[i]['name'] in invalid_names]: + channels.removed.discard(key) + kept = set(db_rows) - set(self._channel_rows_by_key()) - channels.removed - set(moves) + kept |= {key for key, data in self._channel_entries() if 'downloads' not in data} + kept |= {directory for directory, _ in ChannelDownloadsSection().file_rows(self._config)[1]} - channel_directories = [i['directory'] for i in channels] - if len(channel_directories) != len(set(channel_directories)): - raise RuntimeError('Refusing to import channels config because it contains duplicate directories!') + # A moved Channel's Downloads are deleted after the move, under its new directory; one its new entry lists + # is the same Download. + for directory, url in [i for i in downloads.removed if i[0] in moves]: + downloads.removed.discard((directory, url)) + if (moves[directory], url) in downloads.added: + del downloads.added[(moves[directory], url)] + else: + downloads.removed.add((moves[directory], url)) + downloads.removed = {i for i in downloads.removed if i[0] not in kept} - # Validate all channels have required options - for data in channels: - directory = get_media_directory() / data['directory'] - for option in (i for i in REQUIRED_OPTIONS if i not in data): - raise ConfigError(f'Channel "{directory}" is required to have "{option}"') - - updated_channel_ids = set() - - with get_db_session(commit=True) as session: - # Prepare data with kind='channel' for all entries - channel_data_list = [] - for data in channels: - data = data.copy() - data['kind'] = 'channel' - channel_data_list.append(data) - - # Batch create/update Collections - from wrolpi.collections import Collection - collections = Collection.batch_from_config(session, channel_data_list) - - # Pre-fetch existing Channels by collection_id in ONE query - collection_ids = [c.id for c in collections] - existing_channels_list = session.query(Channel).filter( - Channel.collection_id.in_(collection_ids) - ).all() - existing_channels = {ch.collection_id: ch for ch in existing_channels_list} - - # Pre-fetch all tags for channel update in ONE query - tag_names = {d.get('tag_name') for d in channels if d.get('tag_name')} - tags_by_name = {} - if tag_names: - from wrolpi.tags import Tag - tags = session.query(Tag).filter(Tag.name.in_(tag_names)).all() - tags_by_name = {t.name: t for t in tags} - - # Pre-fetch existing downloads for update in ONE query - download_urls = set() - for data in channels: - for dl in data.get('downloads', []): - if isinstance(dl, dict): - download_urls.add(dl['url']) - existing_downloads = {} - if download_urls: - from wrolpi.downloader import Download - downloads = session.query(Download).filter(Download.url.in_(download_urls)).all() - existing_downloads = {d.url: d for d in downloads} - - # Process channels - new_channels = [] - for collection, data in zip(collections, channel_data_list): - channel = existing_channels.get(collection.id) - - if not channel: - # Create new Channel - channel = Channel( - collection_id=collection.id, - url=data.get('url'), - source_id=data.get('source_id'), - ) - new_channels.append(channel) - existing_channels[collection.id] = channel - - # Batch add new channels - if new_channels: - session.add_all(new_channels) - session.flush() - - # Now update all channels (both new and existing) - for collection, data in zip(collections, channel_data_list): - channel = existing_channels[collection.id] - # Call batch_update with pre-fetched data - channel.batch_update(data, tags_by_name, existing_downloads) - updated_channel_ids.add(channel.id) - - # Background task for missing source_id - if not channel.source_id and channel.url and flags.have_internet.is_set(): - if download_manager.can_download and channel.download_missing_data: - channel_import_logger.info(f'Fetching channel source id for {channel}') - background_task(fetch_channel_source_id(channel.id)) - - channel_import_logger.debug(f'Updated {repr(channel.name)}' - f' url={channel.url}' - f' source_id={channel.source_id}' - f' directory={channel.directory}' - ) - - with get_db_session(commit=True) as session: - # Delete any Channels that were deleted from the config, along with their Collections. - orphaned_collection_ids = [] - for channel in session.query(Channel) if decision == IMPORT else []: - if channel.id not in updated_channel_ids: - logger.warning(f'Deleting {channel} because it is not in the config.') - orphaned_collection_ids.append(channel.collection_id) - channel.delete_with_videos() - - # Delete orphaned Collections (kind='channel') that no longer have a Channel. - # This prevents Collections from appearing in the UI with null channel_id. - from wrolpi.collections import Collection - for collection_id in orphaned_collection_ids: - collection = session.query(Collection).filter_by(id=collection_id).one_or_none() - if collection: - logger.warning(f'Deleting orphaned Collection {collection.name} (id={collection.id})') - session.delete(collection) - - # Also clean up any pre-existing orphaned Collections (kind='channel') - # that have no associated Channel. This handles orphans created before - # this cleanup logic was added. - with get_db_session(commit=True) as session: - from wrolpi.collections import Collection - # Find all Collections with kind='channel' that no Channel references - orphaned_collections = session.query(Collection).outerjoin( - Channel, Channel.collection_id == Collection.id - ).filter( - Collection.kind == 'channel', - Channel.id.is_(None) - ).all() - - for collection in orphaned_collections: - logger.warning(f'Deleting pre-existing orphaned Collection {collection.name} (id={collection.id})') - session.delete(collection) - - # Assign Videos to Channels using bulk SQL - from modules.videos import claim_videos_for_channels - claim_videos_for_channels(list(updated_channel_ids)) - - # Create any missing Downloads. Associate Downloads with any necessary Channels. + # A new Download needs a frequency. + if missing := {url for (_, url), row in downloads.added.items() if not row['frequency']}: with get_db_session() as session: - link_channel_and_downloads(session) + missing -= {i for i, in session.query(Download.url).filter(Download.url.in_(list(missing)))} + for key in [i for i in downloads.added if i[1] in missing]: + channel_import_logger.error(f'Refusing to create Download for Channel without frequency: {key[1]}') + del downloads.added[key] + self.import_skipped += 1 - self.record_import_version() - self.successful_import = True - except Exception as e: - self.successful_import = False - message = f'Failed to import {self.get_relative_file()} config!' - if send_events: - channel_import_logger.warning(message, exc_info=e) - Events.send_config_import_failed(message) - raise + def _channel_entries(self) -> List[Tuple[str, dict]]: + """The valid entries of the file, keyed by directory.""" + entries = list() + for data in self._config.get('channels') or []: + try: + entries.append((_channel_key(data), data)) + except Exception: + pass + return entries + + def _channel_rows_by_key(self) -> dict: + return ChannelsSection().file_rows(self._config)[0] + + def _channel_rows(self) -> list: + return list(self._channel_rows_by_key().values()) + + def after_import(self, diffs: dict): + """Claim the Videos of new and changed Channels, find their missing source ids, and link Downloads.""" + changed = {row.get('directory', key) for key, row in + {**diffs['channels'].added, **diffs['channels'].changed}.items()} + if changed: + media_directory = get_media_directory() + with get_db_session() as session: + updated = session.query(Channel.id, Channel.url, Channel.source_id, Channel.download_missing_data) \ + .join(Collection, Collection.id == Channel.collection_id) \ + .filter(Collection.directory.in_([str(media_directory / i) for i in changed])).all() + from modules.videos import claim_videos_for_channels + claim_videos_for_channels([i.id for i in updated]) + # A fetched source id changes channels.yaml, which cannot be saved while it has skipped entries. + for id_, url, source_id, download_missing_data in [] if self.import_skipped else updated: + if not source_id and url and download_missing_data and flags.have_internet.is_set() \ + and download_manager.can_download: + channel_import_logger.info(f'Fetching channel source id for Channel id={id_}') + background_task(fetch_channel_source_id(id_)) + + if self.import_skipped: + # Linking changes channels.yaml, which cannot be saved while it has skipped entries: the database would be + # newer than the file, and the next startup would save over them. + channel_import_logger.warning('Not linking Downloads to Channels while channels.yaml has skipped entries') + else: + with get_db_session() as session: + links = channel_download_links(session) + if links: + with get_db_session(commit=True) as session: + _link_downloads(session, links) + self._config_outdated = True + + if self._config_outdated: + self.background_dump.activate_switch() + if diffs['downloads']: + # The import's Downloads changed download_manager.yaml; saved after the import's write commits. + save_downloads_config.activate_switch() def dump_config(self, file: pathlib.Path = None, send_events=False, overwrite=False): """Dump all channels from database to config file.""" @@ -937,7 +1069,9 @@ async def fetch_channel_source_id(channel_id: int): with get_db_session(commit=True) as session: Channel.find_by_id(session, channel_id).source_id = source_id - save_channels_config.activate_switch() + # Dumped with the database's version (see wrolpi.config_version). + if config := get_channels_config(): + config.background_dump.activate_switch() except Exception as e: logger.error(f'Failed to get Channel source id of id={channel_id}', exc_info=e) @@ -953,67 +1087,45 @@ def import_channels_config(): channel_import_logger.info('Importing channels config completed') -def link_channel_and_downloads(session: Session, channel_: Type[Base] = Channel, download_: Type[Base] = Download): - """Associate any Download related to a Channel via its Collection. +def channel_download_links(session: Session) -> Dict[int, int]: + """The recurring Downloads without a Collection which belong to a Channel: {Download id: Collection id}. - Downloads are linked to Collections (via collection_id) rather than directly to Channels. - This function finds Channels and links their Downloads to the Channel's Collection. - """ - - def get_destination(d): - """Get destination from download.destination column or settings['destination'].""" - if d.destination: - return str(d.destination) - return (d.settings or dict()).get('destination') - - # Only Downloads with a frequency can be a Collection Download. - downloads = list(session.query(download_).filter(download_.frequency.isnot(None)).all()) - # Download.url is unique and cannot be null. - downloads_by_url = {i.url: i for i in downloads} - # Many Downloads may share the same destination (from either column or settings). - downloads_with_destination = [(d, get_destination(d)) for d in downloads if get_destination(d)] - channels = session.query(channel_).all() - - need_commit = False + A Download belongs to the Channel whose directory is its destination, whose URL (or RSS feed) is its URL, or whose + source id is the channel id of its info json.""" + channels = session.query(Channel).options(joinedload(Channel.collection)).all() + by_directory = {str(i.directory): i.collection_id for i in channels if i.directory} + by_url = {i.url: i.collection_id for i in channels if i.url} for channel in channels: - directory = str(channel.directory) - for download, destination in downloads_with_destination: - if destination == directory and not download.collection_id: - download.collection_id = channel.collection_id - need_commit = True + if rss_url := channel.get_rss_url(): + by_url.setdefault(rss_url, channel.collection_id) + by_source_id = {i.source_id: i.collection_id for i in channels if i.source_id} - download = downloads_by_url.get(channel.url) - if download and not download.collection_id: - download.collection_id = channel.collection_id - need_commit = True + links = dict() + downloads = session.query(Download.id, Download.url, Download.destination, Download.settings, Download.info_json) \ + .filter(Download.frequency.isnot(None), Download.collection_id.is_(None)) + for id_, url, destination, settings, info_json in downloads: + destination = str(destination) if destination else (settings or dict()).get('destination') + collection_id = (by_directory.get(destination) if destination else None) or by_url.get(url) + if not collection_id: + info_json = info_json or dict() + if source_id := info_json.get('channel_id') or info_json.get('id'): + collection_id = by_source_id.get(source_id) + if collection_id: + links[id_] = collection_id + return links - # Get any Downloads for a Channel's RSS feed. - rss_url = channel.get_rss_url() - if rss_url and (download := downloads_by_url.get(rss_url)): - if not download.collection_id: - download.collection_id = channel.collection_id - need_commit = True - # Associate any Download which shares a Channel's URL. - for download in downloads: - channel = channel_.get_by_url(session, download.url) - if channel and not download.collection_id: - download.collection_id = channel.collection_id - need_commit = True +def _link_downloads(session: Session, links: Dict[int, int]): + for ids in chunks(list(links), 500): + for download in session.query(Download).filter(Download.id.in_(ids)): + download.collection_id = links[download.id] + session.flush() - # Associate any Download which has info_json channel_id matching a Channel's source_id. - for download in downloads: - if download.collection_id: - continue - info_json = download.info_json or {} - channel_source_id = info_json.get('channel_id') or info_json.get('id') - if channel_source_id: - channel = channel_.get_by_source_id(session, channel_source_id) - if channel: - download.collection_id = channel.collection_id - need_commit = True - if need_commit: +def link_channel_and_downloads(session: Session): + """Associate any Download related to a Channel via its Collection (see `channel_download_links`).""" + if links := channel_download_links(session): + _link_downloads(session, links) session.commit() diff --git a/modules/videos/models.py b/modules/videos/models.py index 8e75bbe2..9121e594 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -731,8 +731,9 @@ class Channel(ModelHelper, Base): raise FileNotFoundError(f'Cannot create Channel info json because directory is not defined: {self}') - def delete_with_videos(self): - """Delete all Video records (but not video files) related to this Channel. Then delete the Channel and its Collection.""" + def delete_with_videos(self, commit: bool = True): + """Delete all Video records (but not video files) related to this Channel. Then delete the Channel and its + Collection. `commit`: False leaves committing to the caller's write.""" session = Session.object_session(self) # Disown the videos. @@ -742,7 +743,7 @@ class Channel(ModelHelper, Base): for download in self.downloads: download: Download - download.delete() + download.delete(commit=commit) # Get the Collection before deleting the Channel collection = self.collection @@ -1139,11 +1140,12 @@ class Channel(ModelHelper, Base): raise RuntimeError(f'More than one info_json entry matches {video_source_id}') def get_or_create_download(self, session: Session, url: str, frequency: int, - reset_attempts: bool = False) -> Download: + reset_attempts: bool = False, commit: bool = True) -> Download: """Get a Download record, if it does not exist, create it. Create a Download if necessary which goes into this Channel's directory. - Delegates to Collection.get_or_create_download() with Channel-specific downloaders. + Delegates to Collection.get_or_create_download() with Channel-specific downloaders. `commit`: False leaves + committing a new Download to the caller's write. """ from modules.videos.downloader import ChannelDownloader, VideoDownloader @@ -1151,7 +1153,8 @@ class Channel(ModelHelper, Base): session, url, frequency, reset_attempts=reset_attempts, downloader_name=ChannelDownloader.name, - sub_downloader_name=VideoDownloader.name + sub_downloader_name=VideoDownloader.name, + commit=commit, ) def get_rss_url(self) -> str | None: diff --git a/modules/videos/test/test_channels_config_import.py b/modules/videos/test/test_channels_config_import.py new file mode 100644 index 00000000..735a697a --- /dev/null +++ b/modules/videos/test/test_channels_config_import.py @@ -0,0 +1,286 @@ +"""channels.yaml imports by diffing its Channels (and their Downloads) against the database, and writing only the +differences (see wrolpi.config_import). A Channel is keyed by its directory.""" +import pytest +import yaml + +from modules.videos import lib +from modules.videos.lib import get_channels_config, import_channels_config +from modules.videos.models import Channel +from wrolpi.collections import Collection +from wrolpi.common import get_relative_to_media_directory +from wrolpi.config_version import get_config_version +from wrolpi.downloader import Download, DownloadFrequency +from wrolpi.test.test_config_import import edit_config, statements, writes + + +@pytest.fixture +async def saved_channels(test_session, test_directory, test_channels_config, test_download_manager_config, + channel_factory, tag_factory, await_switches): + """Channel `one` (tagged, with a weekly Download) and `two`, saved to the config (which records its + version).""" + await tag_factory('news') + channel_factory(name='one', url='https://example.com/one', tag_name='news', + download_frequency=DownloadFrequency.weekly) + channel_factory(name='two', url='https://example.com/two') + get_channels_config().dump_config() + await await_switches() + assert get_config_version(test_session, 'channels.yaml') is not None + return test_channels_config + + +def channels(test_session) -> dict: + test_session.expire_all() + return {i.name: i for i in test_session.query(Channel)} + + +def edit_channel(path, name, **values): + def change(data): + next(i for i in data['channels'] if i['name'] == name).update(values) + + edit_config(path, change) + + +def directory_of(test_session, name) -> str: + return str(get_relative_to_media_directory(channels(test_session)[name].directory)) + + +@pytest.mark.asyncio +async def test_unchanged_import_writes_nothing(test_session, saved_channels): + """Importing a config the database already matches opens no write transaction, nor does linking Downloads when + there are none to link.""" + config = get_channels_config() + with statements() as collected: + import_channels_config() + + assert config.successful_import is True + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_import_writes_only_the_differences(test_session, saved_channels): + """A changed Channel is updated, a removed one is deleted (with its Collection), an added one is created; the + next import writes nothing.""" + def change(data): + data['channels'] = [i for i in data['channels'] if i['name'] != 'two'] + one = data['channels'][0] + one.update(name='One', url='https://example.com/new-one', generate_posters=False) + data['channels'].append(dict(name='three', directory='videos/three', url='https://example.com/three')) + + edit_config(saved_channels, change) + config = get_channels_config() + config.import_config() + + assert config.successful_import is True + found = channels(test_session) + assert set(found) == {'One', 'three'} + assert (found['One'].url, found['One'].generate_posters, found['One'].tag_name) == \ + ('https://example.com/new-one', False, 'news') + assert {i.name for i in test_session.query(Collection).filter_by(kind='channel')} == {'One', 'three'} + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_downloads_follow_the_config(test_session, saved_channels): + """A Download removed from a Channel is deleted; an added one is created (with the file's frequency) in the + import's one write. An existing Download keeps its own frequency.""" + edit_channel(saved_channels, 'one', downloads=[]) + edit_channel(saved_channels, 'two', downloads=[dict(url='https://example.com/two/feed', frequency=86400)]) + + config = get_channels_config() + config.import_config() + + test_session.expire_all() + assert {(i.url, i.frequency, i.collection_id) for i in test_session.query(Download)} == { + ('https://example.com/two/feed', 86400, channels(test_session)['two'].collection_id)} + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_failed_import_creates_nothing(test_session, saved_channels, monkeypatch): + """A new Download is created in the import's one write: a failure later in the write leaves nothing (it was + committed on its own).""" + edit_channel(saved_channels, 'two', downloads=[dict(url='https://example.com/two/feed', frequency=86400)]) + edit_config(saved_channels, lambda data: data.update(channels=[i for i in data['channels'] if i['name'] != 'one'])) + + def fail(*args, **kwargs): + raise RuntimeError('Write failed') + + monkeypatch.setattr(lib.ChannelsSection, 'delete', fail) + with pytest.raises(RuntimeError): + get_channels_config().import_config() + + test_session.expire_all() + assert {i.url for i in test_session.query(Download)} == {'https://example.com/one'} + assert set(channels(test_session)) == {'one', 'two'} + + +@pytest.mark.asyncio +async def test_moved_channel_is_the_same_channel(test_session, saved_channels): + """A Channel whose directory changed in the config (its name did not) is moved, not deleted and re-created.""" + before = channels(test_session)['two'].id + edit_channel(saved_channels, 'two', directory='videos/moved/two') + + get_channels_config().import_config() + + two = channels(test_session)['two'] + assert (two.id, directory_of(test_session, 'two')) == (before, 'videos/moved/two') + + +@pytest.mark.asyncio +async def test_invalid_entries_are_kept(test_session, saved_channels): + """An entry without a name, or with an invalid directory, is skipped (counted), and its Channel is kept.""" + one_directory = directory_of(test_session, 'one') + + def change(data): + next(i for i in data['channels'] if i['name'] == 'one').pop('name') + next(i for i in data['channels'] if i.get('name') == 'two')['directory'] = '../outside' + + edit_config(saved_channels, change) + config = get_channels_config() + config.import_config() + + assert config.import_skipped == 2 + assert set(channels(test_session)) == {'one', 'two'} + assert directory_of(test_session, 'one') == one_directory + + +@pytest.mark.asyncio +async def test_orphan_collections(test_session, test_directory, saved_channels): + """A channel Collection without a Channel is deleted when the config does not list it; one the config lists + gets its Channel.""" + test_session.add_all([ + Collection(name='orphan', kind='channel', directory=test_directory / 'videos/orphan'), + Collection(name='listed', kind='channel', directory=test_directory / 'videos/listed'), + ]) + test_session.commit() + # Saved: the config lists only Channels. + get_channels_config().dump_config() + edit_config(saved_channels, lambda data: data['channels'].append( + dict(name='listed', directory='videos/listed', url='https://example.com/listed'))) + + get_channels_config().import_config() + + test_session.expire_all() + assert {i.name for i in test_session.query(Collection).filter_by(kind='channel')} == {'one', 'two', 'listed'} + assert channels(test_session)['listed'].url == 'https://example.com/listed' + + +@pytest.mark.asyncio +async def test_unknown_tag_with_a_skipped_entry_converges(test_session, saved_channels, test_tags_config, + await_switches): + """Once tags.yaml is imported, a Channel's Tag that no longer exists is dropped; with a skipped entry (the config + cannot be saved) the next import writes nothing.""" + from wrolpi.tags import get_tags_config + get_tags_config().dump_config() + get_tags_config().import_config() + edit_channel(saved_channels, 'two', tag_name='gone') + edit_config(saved_channels, lambda data: data['channels'].append(dict(directory='videos/nameless'))) + + config = get_channels_config() + config.import_config() + await await_switches() + assert channels(test_session)['two'].tag_name is None + + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_no_links_while_entries_are_skipped(test_session, saved_channels, await_switches): + """Linking changes channels.yaml; while an entry is skipped the config cannot be saved, so Downloads are not + linked (the next startup would save over the skipped entry).""" + test_session.add(Download(url='https://example.com/two', downloader='video_channel', frequency=86400)) + test_session.commit() + edit_config(saved_channels, lambda data: data['channels'].append(dict(directory='videos/nameless'))) + + import_channels_config() + await await_switches() + + test_session.expire_all() + assert Download.get_by_url(test_session, 'https://example.com/two').collection_id is None + assert get_config_version(test_session, 'channels.yaml') == yaml.safe_load(saved_channels.read_text())['version'] + + +@pytest.mark.asyncio +async def test_moved_channel_downloads(test_session, saved_channels): + """A moved Channel keeps the Downloads its new entry lists, and loses the ones it does not, in the same import; + the next import writes nothing.""" + edit_channel(saved_channels, 'one', directory='videos/moved/one', downloads=[]) + edit_channel(saved_channels, 'two', directory='videos/moved/two', + downloads=[dict(url='https://example.com/two/feed', frequency=86400)]) + config = get_channels_config() + config.import_config() + + test_session.expire_all() + assert {i.url for i in test_session.query(Download)} == {'https://example.com/two/feed'} + assert directory_of(test_session, 'one') == 'videos/moved/one' + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_invalid_download_keeps_the_channel_downloads(test_session, saved_channels): + """An unreadable Download of a Channel is counted as skipped, and the Channel's Downloads are kept.""" + edit_channel(saved_channels, 'one', downloads=[dict(uri='https://example.com/one')]) + + config = get_channels_config() + config.import_config() + + assert config.import_skipped == 1 + test_session.expire_all() + assert {i.url for i in test_session.query(Download)} == {'https://example.com/one'} + + +@pytest.mark.asyncio +async def test_new_download_without_frequency_is_skipped(test_session, saved_channels): + """A new Download without a frequency cannot be created: it is skipped (counted), and the next import writes + nothing.""" + edit_channel(saved_channels, 'two', downloads=[dict(url='https://example.com/two/feed')]) + + config = get_channels_config() + with statements() as collected: + config.import_config() + + assert config.import_skipped == 1 + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_no_source_id_fetch_while_entries_are_skipped(test_session, saved_channels, monkeypatch): + """Fetching a Channel's source id changes channels.yaml; while an entry is skipped it is not fetched.""" + from wrolpi import flags + scheduled = [] + monkeypatch.setattr(lib, 'background_task', lambda coro: scheduled.append(coro.close())) + monkeypatch.setattr(flags.have_internet, 'is_set', lambda: True) + monkeypatch.setattr(type(lib.download_manager), 'can_download', property(lambda self: True)) + edit_config(saved_channels, lambda data: data['channels'].extend([ + dict(name='three', directory='videos/three', url='https://example.com/three', download_missing_data=True), + dict(directory='videos/nameless'), + ])) + + get_channels_config().import_config() + + assert set(channels(test_session)) == {'one', 'two', 'three'} + assert scheduled == [] + + +@pytest.mark.asyncio +async def test_import_does_not_save_during_its_write(test_session, saved_channels, test_download_manager_config, + monkeypatch): + """Deleting a Download inside the import's write neither skips its URL nor starts a config save (which would read + the database before the import commits).""" + from wrolpi.downloader import get_download_manager_config + saves = [] + monkeypatch.setattr(lib.save_channels_config, 'activate_switch', lambda *a, **kw: saves.append('channels')) + edit_channel(saved_channels, 'one', downloads=[]) + + get_channels_config().import_config() + + assert saves == [] + assert 'https://example.com/one' not in (get_download_manager_config().skip_urls or []) diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 559c7ec6..4a344734 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -377,9 +377,11 @@ class Download(ModelHelper, Base): # noqa def add_to_skip_list(self): download_manager.add_to_skip_list(self.url) - def delete(self, add_to_skip_list: bool = True): + def delete(self, add_to_skip_list: bool = True, commit: bool = True): + """`commit`: False only flushes; the caller's write commits, and saves the configs afterward (a save now + would read the database before that commit), and does not change the skip list.""" # Do not download this automatically again. This saves the config. - if add_to_skip_list: + if add_to_skip_list and commit: self.add_to_skip_list() session = Session.object_session(self) @@ -389,6 +391,9 @@ class Download(ModelHelper, Base): # noqa collection_kind = collection.kind if collection else None session.delete(self) + if not commit: + session.flush([self]) + return session.commit() # Save appropriate config based on collection kind. Channels/domains own their configs; From ee10b34b1502c088fe3748da2e218440e144eb4f Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 14:39:22 -0600 Subject: [PATCH 5/9] recently_viewed.yaml imports by diffing (#873) * recently_viewed.yaml imports by diffing recently_viewed.yaml declares one section, its files keyed by media-relative path, comparing viewed, progress and position. The database's rows are the window the config saves (the newest RECENTLY_VIEWED_LIMIT views, inside the media directory), so an unchanged import writes nothing. A file removed from the config has its viewing history cleared (decision 6), but only when its view is at least as recent as the config's oldest entry: an older view fell off the end of the window. A malformed entry keeps its file's history and is not counted as skipped (that would stop every recently viewed save). Without deletes, progress written after the config is kept, and a newer plain view keeps its `viewed` but takes the config's position (as before). prepare finds the files of new entries on disk before the write; a deleted file's history goes with it. Moving a viewed FileGroup counts as a recently_viewed.yaml change (ORM, the bulk move, and a refresh that changes the primary file). The media directory checks are batched (each resolved the media directory again). Co-Authored-By: Claude Opus 5.5 * recently_viewed.yaml: the no-delete merge covers files outside the window Without a database version, newer history in the database wins (its progress is kept; a newer plain view keeps its `viewed` and takes the config's position). That applied only to files among the newest views; a file listed in the config that had fallen out of them was "added", and its stale entry replaced the newer history. prepare now reads the database's history of added files and applies the same rule. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- wrolpi/collections/config.py | 9 +- wrolpi/files/models.py | 4 + wrolpi/files/progress.py | 201 +++++++++++------- .../files/test/test_recently_viewed_import.py | 187 ++++++++++++++++ wrolpi/files/worker.py | 11 + 5 files changed, 340 insertions(+), 72 deletions(-) create mode 100644 wrolpi/files/test/test_recently_viewed_import.py diff --git a/wrolpi/collections/config.py b/wrolpi/collections/config.py index 662cdd87..80368d33 100644 --- a/wrolpi/collections/config.py +++ b/wrolpi/collections/config.py @@ -11,6 +11,7 @@ from wrolpi.config_import import ConfigSection, SectionDiff, chunks, media_relat from wrolpi.config_version import count_config_change from wrolpi.db import get_db_session from wrolpi.switches import ActivateSwitchMethod, register_switch_handler +from wrolpi.files.progress import RecentlyViewedConfig, save_recently_viewed_config from wrolpi.tags import TagFile, TagsConfig, save_tags_config logger = logger.getChild(__name__) @@ -435,9 +436,11 @@ def file_groups_moved(session, file_group_ids): playlists.yaml references file items by path, so it must be re-saved (and the playlist directories re-synced) or the next import will not find the file and will drop the item. - tags.yaml saves tagged files by path too. The move (often raw SQL) is counted as each config's change. + tags.yaml and recently_viewed.yaml save files by path too. The move (often raw SQL) is counted as each config's + change. `file_group_ids` may be an iterable of ids or a select of ids.""" + from wrolpi.files.models import FileGroup from .models import Collection, CollectionItem if session.query(TagFile.file_group_id).filter(TagFile.file_group_id.in_(file_group_ids)).first(): @@ -448,6 +451,10 @@ def file_groups_moved(session, file_group_ids): .join(Collection, Collection.id == CollectionItem.collection_id) \ .filter(Collection.kind == 'playlist', CollectionItem.file_group_id.in_(file_group_ids)) \ .first() + if session.query(FileGroup.id).filter(FileGroup.id.in_(file_group_ids), FileGroup.viewed.isnot(None)).first(): + count_config_change(session, RecentlyViewedConfig.file_name) + save_recently_viewed_config.activate_switch() + if in_playlist: count_config_change(session, PlaylistsConfig.file_name) save_playlists_config.activate_switch() diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index bc70b0c9..e8482cd3 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -917,6 +917,10 @@ def _playlist_file_group_moved(file_group: FileGroup) -> bool: register_ownership('playlists.yaml', FileGroup, columns={'primary_path'}, row=_playlist_file_group_moved) +# recently_viewed.yaml saves viewed files by path. +register_ownership('recently_viewed.yaml', FileGroup, columns={'primary_path'}, + row=lambda file_group: bool(inspect(file_group).attrs.primary_path.history.deleted) + and file_group.viewed is not None) class Directory(ModelHelper, Base): diff --git a/wrolpi/files/progress.py b/wrolpi/files/progress.py index b831e7f9..37e7e704 100644 --- a/wrolpi/files/progress.py +++ b/wrolpi/files/progress.py @@ -18,13 +18,15 @@ import os import pathlib from dataclasses import dataclass, field from datetime import datetime -from typing import List, Optional, Tuple +from typing import Dict, List, Optional, Set, Tuple import pytz from wrolpi import dates -from wrolpi.common import ConfigFile, logger, get_media_directory, get_paths_in_media_directory -from wrolpi.config_version import SAVE +from sqlalchemy.orm import Session + +from wrolpi.common import ConfigFile, logger, get_media_directory, get_paths_in_media_directory, chunks +from wrolpi.config_import import ConfigSection from wrolpi.db import get_db_session from wrolpi.errors import ValidationError, NoPrimaryFile from wrolpi.events import Events @@ -128,8 +130,92 @@ def _parse_entry(entry) -> Optional[Tuple[str, datetime, Optional[float], Option return None +def _viewed_row(viewed, progress, position) -> dict: + return dict(viewed=viewed, progress=progress, position=position or None) + + +def _entry_keys(paths: List[str]) -> Dict[str, str]: + """{entry path: its path relative to the media directory}, without those outside it (an absolute `path` replaces + the media directory in the join, and `..` can leave it).""" + media_directory = get_media_directory() + absolute_paths = {i: pathlib.Path(os.path.normpath(media_directory / i)) for i in paths} + inside = set(get_paths_in_media_directory(list(absolute_paths.values()))) + return {i: str(j.relative_to(media_directory)) for i, j in absolute_paths.items() if j in inside} + + +def _malformed_keys(config: dict) -> set: + paths = [i['path'] for i in config.get('files') or [] + if isinstance(i, dict) and isinstance(i.get('path'), str) and not _parse_entry(i)] + return set(_entry_keys(paths).values()) + + +class ViewedSection(ConfigSection): + """recently_viewed.yaml's files, keyed by path (relative to the media directory), comparing when each was viewed, + and how far it was read or watched. Only the newest views are saved (`RECENTLY_VIEWED_LIMIT`); older ones are + not in the database's rows.""" + name = 'viewed' + + def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: + # Malformed entries are not counted as skipped: recently viewed is saved after every view, and a skipped + # entry would stop that. (`prepare` keeps their history.) + rows = dict() + entries = list(filter(None, map(_parse_entry, config.get('files') or []))) + keys = _entry_keys([i[0] for i in entries]) + for path, viewed, progress, position in entries: + if not (key := keys.get(path)): + logger.warning(f'Not importing recently viewed file outside the media directory: {path}') + continue + # The first (newest) entry of a file wins. + rows.setdefault(key, _viewed_row(viewed, progress, position)) + return rows, set() + + def db_rows(self, session: Session) -> Dict[str, dict]: + # wrolpi.files.models imports this module. + from wrolpi.files.models import FileGroup + media_directory = get_media_directory() + query = session.query(FileGroup.primary_path, FileGroup.viewed, FileGroup.progress, FileGroup.position) \ + .filter(FileGroup.viewed.isnot(None)) \ + .order_by(FileGroup.viewed.desc(), FileGroup.id) \ + .limit(RECENTLY_VIEWED_LIMIT) + rows = query.all() + # A row outside the media directory is not saved (see `dump_config`). + inside = set(get_paths_in_media_directory([i[0] for i in rows])) + return {str(path.relative_to(media_directory)): _viewed_row(*values) + for path, *values in rows if path in inside} + + def upsert(self, session: Session, rows: Dict[str, dict]): + from wrolpi.files.models import FileGroup + media_directory = get_media_directory() + file_groups = dict() + for paths in chunks([str(media_directory / i) for i in rows], 500): + file_groups.update((str(i.primary_path.relative_to(media_directory)), i) for i in + session.query(FileGroup).filter(FileGroup.primary_path.in_(paths))) + for key, row in rows.items(): + if not (file_group := file_groups.get(key)): + if not row.get('files'): + continue + try: + file_group = FileGroup.from_paths(session, *row['files']) + except NoPrimaryFile: + logger.warning(f'Cannot import recently viewed file: {key}') + continue + session.add(file_group) + file_group.viewed, file_group.progress, file_group.position = \ + row['viewed'], row['progress'], row['position'] + session.flush() + + def delete(self, session: Session, keys: Set[str]): + """A file removed from the config was not viewed.""" + from wrolpi.files.models import FileGroup + media_directory = get_media_directory() + for paths in chunks([str(media_directory / i) for i in keys], 500): + for file_group in session.query(FileGroup).filter(FileGroup.primary_path.in_(paths)): + file_group.viewed = file_group.progress = file_group.position = None + + class RecentlyViewedConfig(ConfigFile): file_name = 'recently_viewed.yaml' + sections = (ViewedSection(),) default_config = dict( version=0, files=[], @@ -180,78 +266,51 @@ class RecentlyViewedConfig(ConfigFile): if send_events: Events.send_config_save_failed(message) - def import_config(self, file=None, send_events=False): - """Apply the config to the database. Progress written after the config (e.g. while WROLPi was starting) - is kept; a newer plain view keeps its `viewed`, but takes the config's position.""" - super().import_config(file, send_events) - if self.import_decision() == SAVE: - self.save_database_instead() - return - try: - # wrolpi.files.models imports this module. - from wrolpi.files.lib import glob_shared_stem - from wrolpi.files.models import FileGroup + def prepare(self, diffs: dict): + diff, db_rows = diffs['viewed'], self.import_db_rows['viewed'] + file_rows = ViewedSection().file_rows(self._config)[0] - media_directory = get_media_directory() - entries = list() - for path, viewed, progress, position in filter(None, map(_parse_entry, self.files)): - # An absolute `path` replaces the media directory in the join, and `..` can leave it. - absolute_path = pathlib.Path(os.path.normpath(media_directory / path)) - if not get_paths_in_media_directory([absolute_path]): - logger.warning(f'Not importing recently viewed file outside the media directory: {path}') - continue - entries.append((absolute_path, viewed, progress, position)) - absolute_paths = [i[0] for i in entries] + # Only views at least as recent as the oldest entry of the file are removed: an older view fell off the end + # of the saved window. A malformed entry keeps its file's history. + if diff.removed: + oldest = min(i['viewed'] for i in file_rows.values()) if file_rows else None + malformed = _malformed_keys(self._config) + diff.removed = {i for i in diff.removed + if oldest and db_rows[i]['viewed'] >= oldest and i not in malformed} - # Glob the files which are not yet in the DB before the write transaction; stat'ing a thousand files on - # a slow disk would block every other writer. - with get_db_session() as session: - known = {i for (i,) in session.query(FileGroup.primary_path) - .filter(FileGroup.primary_path.in_(absolute_paths))} - new_files = {i: glob_shared_stem(i) for i in absolute_paths if i not in known and i.is_file()} + # New entries which are in the database (outside the newest views), and which are not. + from wrolpi.files.lib import glob_shared_stem + from wrolpi.files.models import FileGroup + media_directory = get_media_directory() + known = dict() + with get_db_session() as session: + for paths in chunks([str(media_directory / i) for i in diff.added], 500): + known.update((str(path.relative_to(media_directory)), _viewed_row(*values)) for path, *values in + session.query(FileGroup.primary_path, FileGroup.viewed, FileGroup.progress, + FileGroup.position).filter(FileGroup.primary_path.in_(paths))) - with get_db_session(commit=True) as session: - file_groups = session.query(FileGroup).filter(FileGroup.primary_path.in_(absolute_paths)) - file_groups_by_path = {i.primary_path: i for i in file_groups} + # Without deletes (no database version), progress written after the config is kept; a newer plain view keeps + # its `viewed`, but it is not a newer place to resume. This applies to a file that fell out of the newest + # views too. + if not self.import_deletes: + for rows, current in ((diff.changed, db_rows), (diff.added, known)): + for key, row in list(rows.items()): + if key in current and current[key]['viewed'] and current[key]['viewed'] >= row['viewed']: + if current[key]['progress'] is not None: + del rows[key] + else: + rows[key] = dict(row, viewed=current[key]['viewed']) - applied = 0 - for absolute_path, viewed, progress, position in entries: - file_group = file_groups_by_path.get(absolute_path) - if not file_group: - if not (files := new_files.pop(absolute_path, None)): - # The file was deleted; its history goes with it. - logger.debug(f'Not importing recently viewed file which does not exist: {absolute_path}') - continue - try: - file_group = FileGroup.from_paths(session, *files) - except NoPrimaryFile: - logger.warning(f'Cannot import recently viewed file: {absolute_path}') - continue - session.add(file_group) - session.flush([file_group]) - file_groups_by_path[absolute_path] = file_group - - db_is_newer = file_group.viewed is not None and file_group.viewed >= viewed - if db_is_newer and file_group.progress is not None: - # Progress was written after the config (e.g. while WROLPi was starting). - continue - # A newer plain view is kept, but it is not a newer place to resume. - if not db_is_newer: - file_group.viewed = viewed - file_group.progress = progress - file_group.position = position - applied += 1 - - logger.info(f'Imported {applied} of {len(entries)} recently viewed files') - self.record_import_version() - self.successful_import = True - except Exception as e: - self.successful_import = False - message = f'Failed to import {self.get_relative_file()} config' - logger.error(message, exc_info=e) - if send_events: - Events.send_config_import_failed(message) - raise + # The files of new entries which are not yet in the database; a deleted file's history goes with it. + for key, row in list(diff.added.items()): + absolute_path = media_directory / key + if key in known: + continue + if absolute_path.is_file(): + row['files'] = glob_shared_stem(absolute_path) + else: + logger.debug(f'Not importing recently viewed file which does not exist: {absolute_path}') + del diff.added[key] RECENTLY_VIEWED_CONFIG: RecentlyViewedConfig = RecentlyViewedConfig() diff --git a/wrolpi/files/test/test_recently_viewed_import.py b/wrolpi/files/test/test_recently_viewed_import.py new file mode 100644 index 00000000..977c3b84 --- /dev/null +++ b/wrolpi/files/test/test_recently_viewed_import.py @@ -0,0 +1,187 @@ +"""recently_viewed.yaml imports by diffing the viewed files against the database, and writing only the differences +(see wrolpi.config_import). A file removed from the config has its viewing history cleared, but only within the +window the config saves: an older view just fell off the end.""" +from datetime import timedelta + +import pytest +import yaml + +from wrolpi import dates +from wrolpi.config_version import get_config_version +from wrolpi.conftest import config_was_imported +from wrolpi.files.models import FileGroup +from wrolpi.files.progress import get_recently_viewed_config +from wrolpi.test.test_config_import import edit_config, statements, writes + + +@pytest.fixture +def saved_viewed(test_session, test_directory, async_client, make_files_structure): + """`a.txt`, `b.txt` (with progress) and `c.txt`, viewed in that order, saved to the config (which records its + version).""" + now = dates.now() + for name, hours in (('a.txt', 3), ('b.txt', 2), ('c.txt', 1)): + file_group = FileGroup.from_paths(test_session, *make_files_structure([name])) + file_group.viewed = now - timedelta(hours=hours) + if name == 'b.txt': + file_group.progress, file_group.position = 0.5, dict(kind='page', page=5) + test_session.commit() + config = get_recently_viewed_config() + config.successful_import = True + config.dump_config() + config.save() + assert get_config_version(test_session, 'recently_viewed.yaml') is not None + return config.get_file() + + +def viewed(test_session) -> dict: + test_session.expire_all() + return {i.primary_path.name: i for i in test_session.query(FileGroup) if i.viewed} + + +def edit_entry(path, name, **values): + def change(data): + next(i for i in data['files'] if i['path'] == name).update(values) + + edit_config(path, change) + + +@pytest.mark.asyncio +async def test_unchanged_import_writes_nothing(test_session, saved_viewed): + """Importing a config the database already matches opens no write transaction.""" + config = get_recently_viewed_config() + with statements() as collected: + config.import_config() + + assert config.successful_import is True + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_import_writes_only_the_differences(test_session, saved_viewed): + """Changed progress is applied; a removed file's history is cleared; the next import writes nothing.""" + edit_entry(saved_viewed, 'b.txt', progress=0.75, position=dict(kind='page', page=7)) + edit_config(saved_viewed, lambda data: data.update(files=[i for i in data['files'] if i['path'] != 'c.txt'])) + + config = get_recently_viewed_config() + config.import_config() + + found = viewed(test_session) + assert set(found) == {'a.txt', 'b.txt'} + assert (found['b.txt'].progress, found['b.txt'].position) == (0.75, dict(kind='page', page=7)) + c = test_session.query(FileGroup).filter(FileGroup.primary_path.like('%c.txt')).one() + assert (c.viewed, c.progress, c.position) == (None, None, None) + with statements() as collected: + config.import_config() + assert writes(collected) == [] + + +@pytest.mark.asyncio +async def test_older_views_are_not_removed(test_session, test_directory, async_client, make_files_structure, + monkeypatch): + """A view older than the config's oldest entry fell off the end of the saved window; it is kept, and an unchanged + import writes nothing.""" + monkeypatch.setattr('wrolpi.files.progress.RECENTLY_VIEWED_LIMIT', 2) + now = dates.now() + for name, hours in (('a.txt', 3), ('b.txt', 2), ('c.txt', 1)): + FileGroup.from_paths(test_session, *make_files_structure([name])).viewed = now - timedelta(hours=hours) + test_session.commit() + config = get_recently_viewed_config() + config.successful_import = True + config.dump_config() + config.save() + assert [i['path'] for i in yaml.safe_load(config.get_file().read_text())['files']] == ['c.txt', 'b.txt'] + + with statements() as collected: + config.import_config() + + assert writes(collected) == [] + assert set(viewed(test_session)) == {'a.txt', 'b.txt', 'c.txt'} + + +@pytest.mark.asyncio +async def test_malformed_entry_keeps_its_history(test_session, saved_viewed): + """A malformed entry keeps its file's history (it is not removed), and does not stop the config from being + saved (it is not counted as skipped: recently viewed is saved after every view).""" + edit_entry(saved_viewed, 'c.txt', viewed='not a date') + + config = get_recently_viewed_config() + config.import_config() + + assert config.import_skipped == 0 + assert set(viewed(test_session)) == {'a.txt', 'b.txt', 'c.txt'} + + +@pytest.mark.asyncio +async def test_moved_viewed_file_counts(test_session, test_directory, saved_viewed, make_files_structure): + """recently_viewed.yaml saves files by path: moving a viewed FileGroup makes the database newer than the file, so + a restart before the save does not clear its history. Moving a file that was never viewed does not.""" + before = get_config_version(test_session, 'recently_viewed.yaml') + a = test_session.query(FileGroup).filter_by(primary_path=test_directory / 'a.txt').one() + a.move(test_directory / 'moved.txt') + test_session.commit() + assert get_config_version(test_session, 'recently_viewed.yaml') == before + 1 + + never = FileGroup.from_paths(test_session, *make_files_structure(['never.txt'])) + test_session.commit() + never.move(test_directory / 'never-moved.txt') + test_session.commit() + assert get_config_version(test_session, 'recently_viewed.yaml') == before + 1 + + +@pytest.mark.asyncio +async def test_bulk_moved_viewed_file_counts(test_session, test_directory, saved_viewed): + """A move written with raw SQL counts too.""" + from wrolpi.files.lib import _bulk_update_file_groups_db + before = get_config_version(test_session, 'recently_viewed.yaml') + _bulk_update_file_groups_db(test_session, {test_directory / 'a.txt': test_directory / 'moved/a.txt'}) + test_session.commit() + + assert get_config_version(test_session, 'recently_viewed.yaml') > before + + +@pytest.mark.asyncio +async def test_refresh_changing_primary_file_counts(test_session, test_directory, make_files_structure, video_bytes, + refresh_files): + """A refresh that changes a viewed FileGroup's primary file counts as a change of its path.""" + make_files_structure({'video.mp4': video_bytes, 'video.jpg': 'poster'}) + await refresh_files() + test_session.query(FileGroup).one().viewed = dates.now() + test_session.commit() + config_was_imported(test_session, 'recently_viewed.yaml') + test_session.commit() + + (test_directory / 'video.mp4').unlink() + await refresh_files() + + test_session.expire_all() + assert test_session.query(FileGroup).one().primary_path != test_directory / 'video.mp4' + assert get_config_version(test_session, 'recently_viewed.yaml') > 0 + + +@pytest.mark.asyncio +async def test_without_deletes_newer_history_outside_the_window_is_kept(test_session, test_directory, async_client, + make_files_structure, monkeypatch): + """Without a database version, a newer view in the database wins even when it fell out of the newest views: its + progress is kept, or (a plain view) its `viewed` is kept and the config's position is taken.""" + monkeypatch.setattr('wrolpi.files.progress.RECENTLY_VIEWED_LIMIT', 2) + now = dates.now() + file_groups = {name: FileGroup.from_paths(test_session, *make_files_structure([name])) + for name in ('progress.txt', 'plain.txt', 'b.txt', 'c.txt')} + file_groups['progress.txt'].viewed, file_groups['progress.txt'].progress = now - timedelta(hours=3), 0.8 + file_groups['plain.txt'].viewed = now - timedelta(hours=3) + file_groups['b.txt'].viewed, file_groups['c.txt'].viewed = now - timedelta(hours=2), now - timedelta(hours=1) + test_session.commit() + config = get_recently_viewed_config() + config.get_file().parent.mkdir(exist_ok=True) + config.get_file().write_text(yaml.dump(dict(version=1, files=[ + dict(path='progress.txt', viewed=(now - timedelta(days=1)).isoformat(), progress=0.1), + dict(path='plain.txt', viewed=(now - timedelta(days=1)).isoformat(), progress=0.5, + position=dict(kind='time', seconds=60)), + ]))) + + config.import_config() + + found = viewed(test_session) + assert (found['progress.txt'].viewed, found['progress.txt'].progress) == (now - timedelta(hours=3), 0.8) + assert (found['plain.txt'].viewed, found['plain.txt'].progress, found['plain.txt'].position) == \ + (now - timedelta(hours=3), 0.5, dict(kind='time', seconds=60.0)) diff --git a/wrolpi/files/worker.py b/wrolpi/files/worker.py index 32141a02..cffdf76c 100644 --- a/wrolpi/files/worker.py +++ b/wrolpi/files/worker.py @@ -772,6 +772,17 @@ class FileWorker(FileMoveMixin, FileReorganizeMixin): {'updates': updates}) if curs.fetchone(): count_config_change_curs(curs, 'playlists.yaml') + # recently_viewed.yaml saves viewed files by path too. + curs.execute( + '''SELECT 1 + FROM file_group + JOIN json_each(:updates) AS v ON file_group.id = json_extract(v.value, '$[0]') + WHERE file_group.viewed IS NOT NULL + AND file_group.primary_path != json_extract(v.value, '$[1]') + LIMIT 1''', + {'updates': updates}) + if curs.fetchone(): + count_config_change_curs(curs, 'recently_viewed.yaml') curs.execute( '''UPDATE file_group SET primary_path = v.new_path From c3e25b84d64c4ef9a7e4df9d94c6892021a7b331 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 15:17:18 -0600 Subject: [PATCH 6/9] Configs without database rows read and validate only; imports run off the event loop (#874) The base import for a config with no sections now reads its file once, reports a 0-byte file as empty, treats a missing file as the defaults, and records success or failure itself (sending the failure Event on request). The per-config overrides that only set successful_import are removed. wrolpi.yaml and the downloader settings import in a thread at startup (import_file_configs), and the Import, Save and backup Import buttons run their config work in a thread. The pre-diff import path (import_decision, record_import_version) has no callers left and is removed. Co-authored-by: Claude Opus 5.5 --- main.py | 37 +----- modules/archive/lib.py | 4 - modules/flasher/config.py | 5 - modules/inventory/catalog.py | 1 - modules/map/pins.py | 4 - modules/videos/lib.py | 4 - wrolpi/bookmarks.py | 6 - wrolpi/common.py | 132 +++++++++---------- wrolpi/config_api.py | 14 +- wrolpi/config_version.py | 6 +- wrolpi/downloader.py | 8 -- wrolpi/test/test_file_config_import.py | 175 +++++++++++++++++++++++++ 12 files changed, 251 insertions(+), 145 deletions(-) create mode 100644 wrolpi/test/test_file_config_import.py diff --git a/main.py b/main.py index c6066e6a..b083f8d6 100755 --- a/main.py +++ b/main.py @@ -4,7 +4,6 @@ import asyncio import logging import os import sys -from contextlib import contextmanager from sanic import Sanic from sanic.signals import Event @@ -15,7 +14,7 @@ from wrolpi import root_api # noqa from wrolpi.api_utils import api_app, perpetual_signal, per_worker_task from wrolpi.common import logger, check_media_directory, set_log_level, limit_concurrent, \ cancel_refresh_tasks, cancel_background_tasks, get_wrolpi_config, can_connect_to_server, wrol_mode_enabled, \ - create_empty_config_files, TRACE_LEVEL + create_empty_config_files, import_file_configs, TRACE_LEVEL from wrolpi.contexts import attach_shared_contexts, reset_shared_contexts, initialize_configs_contexts from wrolpi.dates import Seconds from modules.videos.cookies import lock_cookies @@ -29,15 +28,6 @@ from wrolpi.version import get_version_string logger = logger.getChild('wrolpi-main') -@contextmanager -def log_and_suppress(*exceptions, message="Exception suppressed"): - """Like contextlib.suppress but logs the exception at INFO level first.""" - try: - yield - except exceptions as e: - logger.info(f'{message}: {e}', exc_info=e) - - def db_main(args): """ Handle database migrations. This uses Alembic, supported commands are "upgrade", "downgrade", and "check". @@ -266,29 +256,10 @@ async def start_single_tasks(app: Sanic): logger.debug(f'start_single_tasks started') - # Import configs, ignore errors so the service will start. Configs will refuse to save if they failed to import. - with log_and_suppress(Exception, message='Failed to import wrolpi config'): - get_wrolpi_config().import_config() - logger.debug('wrolpi config imported') - + # Import configs that don't require the database. Errors are logged so the service will start; configs refuse + # to save if they failed to import. + await import_file_configs() wrolpi_config = get_wrolpi_config() - if wrolpi_config.successful_import: - # Only import other configs if WROLPi config was imported, and WROL mode is not enabled. - if wrolpi_config.wrol_mode: - logger.warning('Refusing to import other configs when WROL mode is enabled!') - else: - # Import configs that don't require the database. - with log_and_suppress(Exception, message='Failed to import videos downloader config'): - get_videos_downloader_config().import_config() - logger.debug('videos downloader config imported') - with log_and_suppress(Exception, message='Failed to import archive downloader config'): - from modules.archive.lib import get_archive_downloader_config - get_archive_downloader_config().import_config() - logger.debug('archive downloader config imported') - with log_and_suppress(Exception, message='Failed to import download cache config'): - from wrolpi.downloader import get_download_cache_config - get_download_cache_config().import_config() - logger.debug('download cache config imported') logger.debug('start_single_tasks waiting for db...') async with flags.db_up.wait_for(): diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 78adffed..5112efe9 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -418,10 +418,6 @@ class ArchiveDownloaderConfig(ConfigFile): """Get the custom user agent, or None for system default.""" return self._config.get('user_agent') - def import_config(self, file: pathlib.Path = None, send_events=False): - super().import_config(file, send_events) - self.successful_import = True - # Global instance for archive downloader config ARCHIVE_DOWNLOADER_CONFIG: ArchiveDownloaderConfig = ArchiveDownloaderConfig() diff --git a/modules/flasher/config.py b/modules/flasher/config.py index a6bbd390..837b4c3b 100644 --- a/modules/flasher/config.py +++ b/modules/flasher/config.py @@ -26,11 +26,6 @@ class FlasherConfig(ConfigFile): ) validator = FlasherConfigValidator - def import_config(self, file=None, send_events=False): - super().import_config(file, send_events) - # YAML-only config (no database); a successful read is a successful import. - self.successful_import = True - @property def configurations(self) -> List[dict]: return list(self._config.get('configurations', [])) diff --git a/modules/inventory/catalog.py b/modules/inventory/catalog.py index fdd65a9d..b5d2a4aa 100644 --- a/modules/inventory/catalog.py +++ b/modules/inventory/catalog.py @@ -105,7 +105,6 @@ def import_catalog_config(): config = get_catalog_config() if config.get_file().is_file(): config.import_config() - config.successful_import = True seed_catalog_defaults(config) diff --git a/modules/map/pins.py b/modules/map/pins.py index a04014f4..d392f4e0 100644 --- a/modules/map/pins.py +++ b/modules/map/pins.py @@ -22,10 +22,6 @@ class MapPinsConfig(ConfigFile): ) validator = MapPinsConfigValidator - def import_config(self, file=None, send_events=False): - super().import_config(file, send_events) - self.successful_import = True - @property def pins(self) -> List[dict]: return list(self._config.get('pins', [])) diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 777c07db..550ead92 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -909,10 +909,6 @@ class VideoDownloaderConfig(ConfigFile): def strict_codecs(self, value: bool): self.update({**self._config, 'strict_codecs': value}) - def import_config(self, file: pathlib.Path = None, send_events=False): - super().import_config(file, send_events) - self.successful_import = True - VIDEOS_DOWNLOADER_CONFIG: VideoDownloaderConfig = VideoDownloaderConfig() TEST_VIDEOS_DOWNLOADER_CONFIG: VideoDownloaderConfig = None diff --git a/wrolpi/bookmarks.py b/wrolpi/bookmarks.py index b3e4a34a..1fd53237 100644 --- a/wrolpi/bookmarks.py +++ b/wrolpi/bookmarks.py @@ -149,12 +149,6 @@ class BookmarksConfig(ConfigFile): return False return True - def import_config(self, file=None, send_events=False): - # An invalid file raises here, leaving successful_import False so save() will not - # overwrite it. A missing file is imported as empty, so the first save creates it. - super().import_config(file, send_events) - self.successful_import = True - @property def bookmarks(self) -> List[dict]: # A deep copy: the tree is nested, and callers mutate what they get back before diff --git a/wrolpi/common.py b/wrolpi/common.py index 0d366c86..62e23d61 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -772,26 +772,33 @@ class ConfigFile: if self.sections: return self.import_sections(file, send_events) + # A config without sections keeps no database rows: read and validate its file, nothing more. + from wrolpi.events import Events file = file or self.get_file() file_str = str(get_relative_to_media_directory(file)) - # Caller will set to successful if it works. self.successful_import = False - # Subclasses count entries they cannot import; a fresh import starts clean. self.import_skipped = 0 - if file.is_file(): - if not self.is_valid(file): - raise InvalidConfig(f'Config is invalid: {file_str}') - - data = self.read_config_file(file) - new_data, extra_data = partition(lambda i: i[0] in self.default_config, data.items()) - new_data, extra_data = dict(new_data), dict(extra_data) - if extra_data: - logger_.warning(f'Ignoring extra config data ({file_str}): {extra_data}') - self._config.update(new_data) - # Import call above this will set successful_import. - # self.successful_import = True - else: - logger_.error(f'Failed to import {file_str} because it does not exist.') + try: + if file.is_file(): + data = self.read_config_file(file) if file.stat().st_size else None + if not data or not self.validate(data): + raise InvalidConfig(f'Config is {"invalid" if data else "empty"}: {file_str}') + new_data, extra_data = partition(lambda i: i[0] in self.default_config, data.items()) + new_data, extra_data = dict(new_data), dict(extra_data) + if extra_data: + logger_.warning(f'Ignoring extra config data ({file_str}): {extra_data}') + self._config.update(new_data) + else: + # The defaults apply; the first save creates the file. + logger_.info(f'No config file, using the defaults: {file_str}') + self.successful_import = True + except Exception as e: + self.successful_import = False + message = f'Failed to import {file_str}' + logger_.error(message, exc_info=e) + if send_events: + Events.send_config_import_failed(message) + raise def import_sections(self, file: pathlib.Path = None, send_events=False): """Import a config with `sections`: read and validate the file once, decide what its version allows (see @@ -891,7 +898,7 @@ class ConfigFile: f'{len(diff.removed)} removed' for name, diff in changed.items()) or 'no changes')) self.after_import(diffs) if decision != IMPORT: - # The database may have rows the file does not list; save them (see `record_import_version`). + # The database may have rows the file does not list; save them (see `record_saved_version`). self.background_dump.activate_switch() return True @@ -924,41 +931,16 @@ class ConfigFile: try: self.import_config(send_events=send_events) finally: - # An import that returned before `import_decision` must not force a later one. + # An import that returned before reading `force_import` must not force a later one. self.force_import = False def is_versioned_by_database(self) -> bool: from wrolpi.config_version import versioned_config_names return self.file_name in versioned_config_names() - # What the last `import_decision` allowed; `record_import_version` records the version that follows from it. - last_import_decision: str = None - - def import_decision(self) -> str: - """After reading the file (`ConfigFile.import_config`), decide what importing it may do: IMPORT, - IMPORT_WITHOUT_DELETES, or SAVE (the database is newer; call `save_database_instead` and import nothing). - - On IMPORT the database's version is removed until `record_import_version`: the import's own writes must not - count, or an import that fails partway would leave the database "newer" than the file it did not finish - applying, and the next startup would save that partial database over the file.""" - from wrolpi.config_version import IMPORT, decide, delete_config_version, get_config_version - from wrolpi.db import get_db_session - with get_db_session(commit=True) as session: - db_version = get_config_version(session, self.file_name) - if self.force_import: - self.force_import = False - decision = IMPORT - else: - decision = decide(self.version or 0, db_version) - if decision == IMPORT: - delete_config_version(session, self.file_name) - logger_.debug(f'{self.file_name}: file version {self.version}, database version {db_version}: {decision}') - self.last_import_decision = decision - return decision - def version_decision(self, forced: bool = False) -> Tuple[str, Optional[int]]: - """What importing this config's file (already read) may do, and the database version that decided it. For - `import_sections`, which records the version in its own write (`import_decision` is for the others).""" + """What importing this config's file (already read) may do (IMPORT, IMPORT_WITHOUT_DELETES, or SAVE), and the + database version that decided it. `import_sections` records the file's version in its own write.""" from wrolpi.config_version import IMPORT, decide, get_config_version from wrolpi.db import get_db_session with get_db_session() as session: @@ -968,27 +950,11 @@ class ConfigFile: return decision, db_version def save_database_instead(self): - """The database has changes this config's file does not (see `import_decision`); save them.""" + """The database has changes this config's file does not (see `version_decision`); save them.""" logger_.warning(f'{self.file_name} is older than the database; saving the database instead of importing it') self.successful_import = True self.dump_config() - def record_import_version(self): - """The database now matches this config's file: record the file's version. Call last, after every write - the import makes. - - Not after IMPORT_WITHOUT_DELETES: the database may keep rows the file does not list (changes that were never - saved), so recording the file's version would let the next import delete them. Save them into the file - instead; that save records the version (`record_saved_version`). A save after a partial import is refused, - so the version stays unknown until an import applies the whole file.""" - from wrolpi.config_version import IMPORT, set_config_version - from wrolpi.db import get_db_session - if self.last_import_decision != IMPORT: - self.background_dump.activate_switch() - return - with get_db_session(commit=True) as session: - set_config_version(session, self.file_name, self.version or 0) - def db_version_for_dump(self, session) -> int: """The version `dump_config` writes: the database's, read in the same transaction as the rows it saves. Before the database has one, the file's next version (`save` records it).""" @@ -997,8 +963,8 @@ class ConfigFile: return db_version if db_version is not None else (self.version or 0) + 1 def record_saved_version(self, version: int): - """The file was just written from the database. If the database's version was unknown (see - `record_import_version`), the two are now in step: record it, so changes from here on count against it.""" + """The file was just written from the database. If the database's version was unknown (an import without + deletes), the two are now in step: record it, so changes from here on count against it.""" from wrolpi.config_version import create_config_version from wrolpi.db import get_db_session with get_db_session(commit=True) as session: @@ -1466,6 +1432,38 @@ def get_all_configs() -> Dict[str, ConfigFile]: return all_configs +async def import_file_configs(): + """Import the configs that do not need the database: wrolpi.yaml, then the downloader settings (not when + wrolpi.yaml failed, or while WROL mode is enabled). Errors are logged so the service starts; a config that failed + to import refuses to save. + + This runs from a worker's after_server_start while the worker is already accepting requests, so no import may + block the event loop.""" + from modules.archive.lib import get_archive_downloader_config + from modules.videos.lib import get_videos_downloader_config + from wrolpi.downloader import get_download_cache_config + + wrolpi_config = get_wrolpi_config() + try: + await asyncio.to_thread(wrolpi_config.import_config) + logger.debug('wrolpi config imported') + except Exception as e: + logger.warning(f'Failed to import wrolpi config: {e}') + + if not wrolpi_config.successful_import: + return + if wrolpi_config.wrol_mode: + logger.warning('Refusing to import other configs when WROL mode is enabled!') + return + + for config in (get_videos_downloader_config(), get_archive_downloader_config(), get_download_cache_config()): + try: + await asyncio.to_thread(config.import_config) + logger.debug(f'{config.file_name} imported') + except Exception as e: + logger.warning(f'Failed to import {config.file_name}: {e}') + + async def import_all_db_configs() -> dict[str, bool]: """ Import all configs that require the database. @@ -1670,10 +1668,8 @@ class WROLPiConfig(ConfigFile): return default_config_path def import_config(self, file: pathlib.Path = None, send_events=False): + super().import_config(file, send_events) try: - # WROLPiConfig does not sync to database. - super().import_config(file) - # Destinations cannot be empty. self._config['archive_destination'] = self.archive_destination or self.default_config['archive_destination'] self._config['map_destination'] = self.map_destination or self.default_config['map_destination'] @@ -1699,8 +1695,6 @@ class WROLPiConfig(ConfigFile): if repos_root not in ignored: ignored.append(repos_root) self._config['ignored_directories'] = ignored - - self.successful_import = True except Exception as e: self.successful_import = False message = f'Failed to import {self.file_name}' diff --git a/wrolpi/config_api.py b/wrolpi/config_api.py index 6f23424c..7564c49e 100644 --- a/wrolpi/config_api.py +++ b/wrolpi/config_api.py @@ -1,3 +1,5 @@ +import asyncio + from sanic import response, Blueprint from sanic.request import Request from sanic_ext.extensions.openapi import openapi @@ -46,11 +48,11 @@ def post_config_update(request: Request, body: config_schema.ConfigUpdateRequest body=config_schema.ConfigsRequest, validate=True, ) -def post_config_import(_: Request, body: config_schema.ConfigsRequest): +async def post_config_import(_: Request, body: config_schema.ConfigsRequest): config = get_config_by_file_name(body.file_name) try: # The user chose to apply the file, whatever its version. - config.import_forced(send_events=True) + await asyncio.to_thread(config.import_forced, send_events=True) except Exception as e: logger.error(f'Failed to import config: {body.file_name}', exc_info=e) raise InvalidConfig(f'Failed to import config {body.file_name}') @@ -64,10 +66,10 @@ def post_config_import(_: Request, body: config_schema.ConfigsRequest): body=config_schema.ConfigsRequest, validate=True, ) -def post_config_dump(_: Request, body: config_schema.ConfigsRequest): +async def post_config_dump(_: Request, body: config_schema.ConfigsRequest): config = get_config_by_file_name(body.file_name) try: - config.dump_config(send_events=True, overwrite=body.overwrite) + await asyncio.to_thread(config.dump_config, send_events=True, overwrite=body.overwrite) except Exception as e: logger.error(f'Failed to dump config: {body.file_name}', exc_info=e) raise InvalidConfig(f'Failed to dump config {body.file_name}') @@ -114,7 +116,7 @@ def post_config_backup_preview(_: Request, body: config_schema.ConfigBackupPrevi body=config_schema.ConfigBackupImportRequest, validate=True, ) -def post_config_backup_import(_: Request, body: config_schema.ConfigBackupImportRequest): +async def post_config_backup_import(_: Request, body: config_schema.ConfigBackupImportRequest): if body.mode not in ('merge', 'overwrite'): raise InvalidConfig('mode must be "merge" or "overwrite"') config = get_config_by_file_name(body.file_name) @@ -124,7 +126,7 @@ def post_config_backup_import(_: Request, body: config_schema.ConfigBackupImport if not backup_file.is_file(): raise InvalidConfig(f'Backup file not found for date {body.backup_date}') try: - config.import_backup(body.backup_date, body.mode, send_events=True) + await asyncio.to_thread(config.import_backup, body.backup_date, body.mode, send_events=True) except Exception as e: logger.error(f'Failed to import backup: {body.file_name} {body.backup_date}', exc_info=e) raise InvalidConfig(f'Failed to import backup {body.file_name}') diff --git a/wrolpi/config_version.py b/wrolpi/config_version.py index c56e371e..acb4139c 100644 --- a/wrolpi/config_version.py +++ b/wrolpi/config_version.py @@ -5,7 +5,7 @@ A database config (tags.yaml, channels.yaml, ...) saves rows the database owns. config's database version, in the same transaction (`_count_config_changes`). `dump_config` writes the database's version into the file it saves, and an import records the file's version once it has applied it. -So an import can tell which side is newer by comparing two integers (see `ConfigFile.import_decision`): +So an import can tell which side is newer by comparing two integers (see `ConfigFile.version_decision`): * file == database: in step. The file is current; import it, deleting what it no longer lists. * file > database: the file is newer (a restored or wiped database, a copied config); import it the same way. @@ -153,10 +153,6 @@ def get_config_version(session: Session, name: str) -> Optional[int]: return row[0] if row else None -def delete_config_version(session: Session, name: str): - session.connection().execute(text('DELETE FROM config_version WHERE name = :name'), {'name': name}) - - def create_config_version(session: Session, name: str, version: int): """Record `version` unless the database already has one.""" session.connection().execute( diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 4a344734..4323b259 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -2431,14 +2431,6 @@ class DownloadCacheConfig(ConfigFile): ) validator = DownloadCacheConfigValidator - def import_config(self, file: pathlib.Path = None, send_events=False): - file = file or self.get_file() - if not file.is_file(): - self.successful_import = True - return - super().import_config(file, send_events) - self.successful_import = True - @property def video_durations(self) -> List[list]: return self._config['video_durations'] or list() diff --git a/wrolpi/test/test_file_config_import.py b/wrolpi/test/test_file_config_import.py new file mode 100644 index 00000000..e3cd11eb --- /dev/null +++ b/wrolpi/test/test_file_config_import.py @@ -0,0 +1,175 @@ +"""A config that keeps no database rows imports by reading and validating its file, nothing more. The Import button +and the startup imports read files and may write the database, so they run off the event loop.""" +import asyncio +from http import HTTPStatus + +import pytest + +from modules.archive.lib import ArchiveDownloaderConfig +from modules.map.pins import MapPinsConfig +from modules.videos.lib import VideoDownloaderConfig +from wrolpi.common import ConfigFile, WROLPiConfig +from wrolpi import common +from wrolpi.downloader import DownloadCacheConfig +from wrolpi.errors import InvalidConfig +from wrolpi.events import Events + + +@pytest.fixture +def pins_config(async_client): + config = MapPinsConfig() + config.get_file().parent.mkdir(parents=True, exist_ok=True) + return config + + +def on_event_loop() -> bool: + try: + asyncio.get_running_loop() + return True + except RuntimeError: + return False + + +@pytest.mark.asyncio +async def test_import(pins_config): + """A valid file is applied, and the import succeeds without the subclass saying so.""" + pins_config.get_file().write_text('version: 3\npins: [one, two]\n') + + pins_config.import_config() + + assert pins_config.successful_import is True + assert pins_config._config['pins'] == ['one', 'two'] + + +@pytest.mark.asyncio +async def test_import_reads_once(pins_config, monkeypatch): + """The file is parsed once, not once to validate and again to apply.""" + pins_config.get_file().write_text('version: 3\npins: [one]\n') + reads = [] + read = ConfigFile.read_config_file + monkeypatch.setattr(MapPinsConfig, 'read_config_file', lambda self, *a: reads.append(1) or read(self, *a)) + + pins_config.import_config() + + assert len(reads) == 1 + + +@pytest.mark.asyncio +async def test_import_missing(pins_config): + """A missing file is imported as the defaults; the first save creates it.""" + pins_config.import_config() + + assert pins_config.successful_import is True + assert pins_config._config['pins'] == [] + + +@pytest.mark.asyncio +async def test_import_empty(pins_config): + """A 0-byte file is reported as empty, and never saved over.""" + pins_config.get_file().write_text('') + + with pytest.raises(InvalidConfig, match='empty'): + pins_config.import_config() + + assert pins_config.successful_import is False + + +@pytest.mark.asyncio +async def test_import_invalid(pins_config, monkeypatch): + """An invalid file raises, is never saved over, and the Import button reports it.""" + pins_config.get_file().write_text('- not\n- a mapping\n') + sent = [] + monkeypatch.setattr(Events, 'send_config_import_failed', lambda message: sent.append(message)) + + with pytest.raises(InvalidConfig): + pins_config.import_config(send_events=True) + + assert pins_config.successful_import is False + assert sent == ['Failed to import config/map_pins.yaml'] + with pytest.raises(RuntimeError, match='never successfully imported'): + pins_config.save() + + +@pytest.mark.asyncio +async def test_startup_imports_off_event_loop(test_wrolpi_config, monkeypatch): + """wrolpi.yaml and the downloader settings are imported in a thread at startup.""" + imported = [] + + def record(self, *args, **kwargs): + imported.append((self.file_name, on_event_loop())) + self.successful_import = True + + for cls in (WROLPiConfig, VideoDownloaderConfig, ArchiveDownloaderConfig, DownloadCacheConfig): + monkeypatch.setattr(cls, 'import_config', record) + + await common.import_file_configs() + + assert imported == [ + ('wrolpi.yaml', False), + ('videos_downloader.yaml', False), + ('archives_downloader.yaml', False), + ('download_cache.yaml', False), + ] + + +@pytest.mark.asyncio +async def test_startup_imports_wrol_mode(test_wrolpi_config, monkeypatch): + """No other config is imported when wrolpi.yaml failed, or while WROL mode is enabled.""" + imported = [] + + def record(self, *args, **kwargs): + imported.append(self.file_name) + + for cls in (VideoDownloaderConfig, ArchiveDownloaderConfig, DownloadCacheConfig): + monkeypatch.setattr(cls, 'import_config', record) + + def fail(self, *args, **kwargs): + self.successful_import = False + raise InvalidConfig('invalid') + + monkeypatch.setattr(WROLPiConfig, 'import_config', fail) + await common.import_file_configs() + assert imported == [] + + def wrol_mode(self, *args, **kwargs): + self.successful_import = True + self._config['wrol_mode'] = True + + monkeypatch.setattr(WROLPiConfig, 'import_config', wrol_mode) + await common.import_file_configs() + assert imported == [] + + +@pytest.mark.asyncio +@pytest.mark.parametrize('path,method,body', [ + ('/api/config/import', 'import_forced', dict(file_name='tags.yaml')), + ('/api/config/dump', 'dump_config', dict(file_name='tags.yaml')), +]) +async def test_api_off_event_loop(async_client, test_tags_config, monkeypatch, path, method, body): + """The Import and Save buttons read the database and the file in a thread, not on the event loop.""" + from wrolpi.tags import TagsConfig + calls = [] + monkeypatch.setattr(TagsConfig, method, lambda self, *args, **kwargs: calls.append(on_event_loop())) + + request, response = await async_client.post(path, json=body) + + assert response.status_code == HTTPStatus.NO_CONTENT + assert calls == [False] + + +@pytest.mark.asyncio +async def test_backup_import_off_event_loop(async_client, test_tags_config, monkeypatch): + """Importing a backup runs in a thread, not on the event loop.""" + from wrolpi.tags import TagsConfig, get_tags_config + config = get_tags_config() + backup = config._get_backup_file('20260101') + backup.parent.mkdir(parents=True, exist_ok=True) + backup.write_text('version: 1\ntags: {}\n') + calls = [] + monkeypatch.setattr(TagsConfig, 'import_backup', lambda self, *args, **kwargs: calls.append(on_event_loop())) + + body = dict(file_name='tags.yaml', backup_date='20260101', mode='merge') + request, response = await async_client.post('/api/config/backup/import', json=body) + + assert response.status_code == HTTPStatus.NO_CONTENT + assert calls == [False] From 0dd61b8e643d6cd9abaf8a333cd61fcfebbb308f Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:22:16 -0600 Subject: [PATCH 7/9] Strip whitespace from Tag names (#876) * Tag names have leading and trailing whitespace stripped A padded name now finds the existing Tag instead of creating a near-duplicate, and a whitespace-only name is rejected. Co-Authored-By: Claude Opus 5.5 * Migration strips whitespace from Tag names and merges collisions Tags whose names match once stripped are merged (tagged files, Zim entries, Collections); the first color by id is kept. Downloads and the configs that name Tags are rewritten too, so the next import is in step. Co-Authored-By: Claude Opus 5.5 * tags.yaml keeps the database's color for merged Tags The file was merged in sorted key order, not by Tag id, and the next import copies the file's color over the database's. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../2026_10_25_1200_strip_tag_names.py | 175 ++++++++++++++++++ wrolpi/tags.py | 5 +- wrolpi/test/test_db_bootstrap.py | 75 ++++++++ wrolpi/test/test_tags.py | 26 +++ 4 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/2026_10_25_1200_strip_tag_names.py diff --git a/alembic/versions/2026_10_25_1200_strip_tag_names.py b/alembic/versions/2026_10_25_1200_strip_tag_names.py new file mode 100644 index 00000000..a076bbf8 --- /dev/null +++ b/alembic/versions/2026_10_25_1200_strip_tag_names.py @@ -0,0 +1,175 @@ +"""Tag names have no leading or trailing whitespace. + +A Tag whose name had whitespace is renamed. Tags whose names are the same once stripped are merged into one: it +keeps every tagged file, Zim entry and Collection, and the first color (by Tag id) of the merged Tags. Downloads +and the config files that name Tags are rewritten the same way, so the next import finds them in step with the +database (an import of the old names would create the padded Tags again, or delete the stripped ones). + +Revision ID: 2026_10_25_1200 +Revises: 2026_10_24_1200 +""" +import json +import logging + +import yaml +from alembic import op + +from wrolpi.common import get_media_directory, write_config_data + +# revision identifiers, used by Alembic. +revision = '2026_10_25_1200' +down_revision = '2026_10_24_1200' +branch_labels = None +depends_on = None + +logger = logging.getLogger(__name__) + +# The configs which name Tags. +CONFIG_FILES = ('tags.yaml', 'download_manager.yaml', 'channels.yaml', 'domains.yaml', 'playlists.yaml', + 'repos.yaml', 'videos_downloader.yaml', 'archives_downloader.yaml') + + +def _strip(name): + """A Tag name without its whitespace; a name that would be empty is kept (it cannot be renamed).""" + if isinstance(name, str) and name.strip(): + return name.strip() + return name + + +def _strip_names(names: list) -> list: + stripped = list() + for name in map(_strip, names): + if name not in stripped: + stripped.append(name) + return stripped + + +def _strip_nested(data): + """Strip the Tag names in a config's (or a Download's settings') `tag_name`, `tag_names` and + `channel_tag_name` values, at any depth.""" + if isinstance(data, list): + return [_strip_nested(i) for i in data] + if not isinstance(data, dict): + return data + result = dict() + for key, value in data.items(): + if key == 'tag_name': + value = _strip(value) + elif key in ('tag_names', 'channel_tag_name') and isinstance(value, list): + value = _strip_names(value) + else: + value = _strip_nested(value) + result[key] = value + return result + + +def _strip_rows(rows: list, key_length: int) -> list: + """tags.yaml's tag_files or tag_zims, each row keyed by its first `key_length` values (the Tag name first).""" + result, seen = list(), set() + for row in rows: + if isinstance(row, list) and row: + row = [_strip(row[0]), *row[1:]] + key = json.dumps(row[:key_length], default=str) + if key in seen: + continue + seen.add(key) + result.append(row) + return result + + +def _strip_tags_config(config: dict, colors: dict) -> dict: + """`colors`: each renamed Tag's color in the database, which the file must match (an import copies the file's + color into the database).""" + config = dict(config) + if isinstance(config.get('tags'), dict): + tags = dict() + for name, attrs in config['tags'].items(): + stripped = _strip(name) + if stripped not in tags: + tags[stripped] = attrs + elif isinstance(attrs, dict) and attrs.get('color') and not (tags[stripped] or dict()).get('color'): + # The first color of the merged Tags. + tags[stripped] = {**(tags[stripped] or dict()), 'color': attrs['color']} + for name in tags.keys() & colors.keys(): + tags[name] = {**(tags[name] or dict()), 'color': colors[name]} + config['tags'] = tags + if isinstance(config.get('tag_files'), list): + config['tag_files'] = _strip_rows(config['tag_files'], 2) + if isinstance(config.get('tag_zims'), list): + config['tag_zims'] = _strip_rows(config['tag_zims'], 3) + return config + + +def strip_tag_names_in_configs(colors: dict): + config_directory = get_media_directory() / 'config' + for file_name in CONFIG_FILES: + file = config_directory / file_name + if not file.is_file(): + continue + try: + with file.open('rt') as fh: + config = yaml.load(fh, Loader=yaml.CSafeLoader) + except Exception as e: + logger.error(f'Cannot strip Tag names of invalid config: {file}', exc_info=e) + continue + if not isinstance(config, dict): + continue + stripped = _strip_tags_config(config, colors) if file_name == 'tags.yaml' else config + stripped = _strip_nested(stripped) + if stripped != config: + logger.warning(f'Stripping whitespace from Tag names in {file}') + write_config_data(stripped, file, width=500 if file_name == 'tags.yaml' else 90) + + +def strip_tag_names_in_db(connection) -> dict: + """Returns the color of each renamed Tag, by its new name.""" + groups, colors = dict(), dict() + for id_, name, color in connection.exec_driver_sql('SELECT id, name, color FROM tag ORDER BY id').fetchall(): + groups.setdefault(_strip(name), list()).append((id_, name, color)) + + for stripped, tags in groups.items(): + if all(name == stripped for _, name, _ in tags): + continue + # Keep the Tag already named this, if any, otherwise the oldest. + survivor_id = next((id_ for id_, name, _ in tags if name == stripped), tags[0][0]) + color = colors[stripped] = next((color for _, _, color in tags if color), None) + for id_, name, _ in tags: + if id_ == survivor_id: + continue + logger.warning(f'Merging Tag {name!r} into {stripped!r}') + connection.exec_driver_sql( + 'INSERT OR IGNORE INTO tag_file (tag_id, file_group_id, created_at)' + ' SELECT ?, file_group_id, created_at FROM tag_file WHERE tag_id = ?', (survivor_id, id_)) + connection.exec_driver_sql( + 'INSERT OR IGNORE INTO tag_zim (tag_id, zim_id, zim_entry, created_at)' + ' SELECT ?, zim_id, zim_entry, created_at FROM tag_zim WHERE tag_id = ?', (survivor_id, id_)) + connection.exec_driver_sql('DELETE FROM tag_file WHERE tag_id = ?', (id_,)) + connection.exec_driver_sql('DELETE FROM tag_zim WHERE tag_id = ?', (id_,)) + connection.exec_driver_sql('UPDATE collection SET tag_id = ? WHERE tag_id = ?', (survivor_id, id_)) + connection.exec_driver_sql('DELETE FROM tag WHERE id = ?', (id_,)) + logger.warning(f'Renaming Tag to {stripped!r}') + connection.exec_driver_sql('UPDATE tag SET name = ?, color = ? WHERE id = ?', (stripped, color, survivor_id)) + + # The Downloads which apply them (download_manager.yaml is stripped the same way). + downloads = connection.exec_driver_sql( + 'SELECT id, tag_names, settings FROM download WHERE tag_names IS NOT NULL OR settings IS NOT NULL').fetchall() + for id_, tag_names, settings in downloads: + tag_names, settings = json.loads(tag_names) if tag_names else None, json.loads(settings) if settings else None + new_tag_names = _strip_names(tag_names) if isinstance(tag_names, list) else tag_names + new_settings = _strip_nested(settings) + if new_tag_names != tag_names or new_settings != settings: + connection.exec_driver_sql( + 'UPDATE download SET tag_names = ?, settings = ? WHERE id = ?', + (json.dumps(new_tag_names) if new_tag_names is not None else None, + json.dumps(new_settings) if new_settings is not None else None, id_)) + return colors + + +def upgrade(): + colors = strip_tag_names_in_db(op.get_bind()) + strip_tag_names_in_configs(colors) + + +def downgrade(): + # Whitespace that was stripped is not restored. + pass diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 2d38c5c9..0c51c2e2 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -166,7 +166,9 @@ class Tag(ModelHelper, Base): # Replace forward-slash (linux directories) with Unicode Big Solidus (U+29F8) name = name.replace('/', '⧸') - name = INVALID_FILE_CHARS.sub('', name) + name = INVALID_FILE_CHARS.sub('', name).strip() + if not name: + raise InvalidTag('Tag name cannot be empty') if (other := self.get_by_name(session, name)) and other.id != self.id: raise InvalidTag('Tag name already taken') @@ -266,6 +268,7 @@ class Tag(ModelHelper, Base): @warning: does not commit""" if name and id_: raise RuntimeError('Tag name and id cannot be used together') + name = name.strip() if name else name if tag := session.query(Tag).filter(Tag.id == id_).one_or_none(): return tag diff --git a/wrolpi/test/test_db_bootstrap.py b/wrolpi/test/test_db_bootstrap.py index 4eb18b07..4ac0def2 100644 --- a/wrolpi/test/test_db_bootstrap.py +++ b/wrolpi/test/test_db_bootstrap.py @@ -302,6 +302,81 @@ def test_markdown_mimetype_migration_retags_existing_rows(test_directory): } +def test_strip_tag_names_migration_merges_tags(test_directory): + """Whitespace is stripped from Tag names; Tags with the same stripped name are merged (keeping the first color, + by id) in the database and in the configs which name them.""" + import json + import sqlite3 + import yaml + from alembic.config import Config + from alembic import command + from wrolpi.db import get_db_uri, get_db_file + from wrolpi.vars import PROJECT_DIR + + config_directory = test_directory / 'config' + config_directory.mkdir(parents=True, exist_ok=True) + config = Config(str(PROJECT_DIR / 'alembic.ini')) + config.set_main_option('sqlalchemy.url', get_db_uri()) + command.upgrade(config, '2026_10_24_1200') + + db_file = get_db_file() + with sqlite3.connect(db_file) as conn: + conn.executemany('INSERT INTO tag (id, name, color) VALUES (?,?,?)', ( + # Merged: the first has no color, so the second's is kept. + (1, ' one', None), (2, 'one ', '#222222'), (3, 'one', '#333333'), + # Merged: both have colors, so the first is kept. + (4, 'two\t', '#444444'), (5, ' two ', '#555555'), + (6, 'three ', None), + (7, 'unchanged', '#777777'), + )) + for id_ in (10, 11): + conn.execute('INSERT INTO file_group (id, directory, primary_path, files, stem, indexed)' + ' VALUES (?,?,?,?,?,0)', + (id_, str(test_directory), str(test_directory / f'{id_}.mp4'), '[]', str(id_))) + conn.executemany('INSERT INTO tag_file (tag_id, file_group_id) VALUES (?,?)', + ((1, 10), (2, 10), (3, 11), (5, 11), (6, 10), (7, 10))) + conn.execute("INSERT INTO collection (id, name, kind, tag_id, item_count, total_size) VALUES (20, 'channel', 'channel', 1, 0, 0)") + conn.execute('INSERT INTO download (url, tag_names, settings) VALUES (?,?,?)', ( + 'https://example.com/feed', json.dumps(['one ', 'one', 'three ']), + json.dumps({'channel_tag_name': [' two'], 'video_resolutions': ['720p']}))) + conn.commit() + + (config_directory / 'tags.yaml').write_text(yaml.dump(dict( + # Sorted, ' two ' comes before 'two\t'; the database's color (by id) is kept anyway. + tags={' one': dict(color=None), 'one ': dict(color='#222222'), ' two ': dict(color='#555555'), + 'two\t': dict(color='#444444'), 'unchanged': dict(color='#777777')}, + tag_files=[[' one', '10.mp4', None], ['one ', '10.mp4', None], ['unchanged', '10.mp4', None]], + tag_zims=[], + version=3, + ))) + (config_directory / 'channels.yaml').write_text(yaml.dump(dict( + channels=[dict(name='channel', directory='videos/channel', tag_name=' one')], version=2))) + # A config which names no padded Tag is not rewritten. + domains = 'domains: []\nversion: 1\n' + (config_directory / 'domains.yaml').write_text(domains) + + command.upgrade(config, 'head') + + with sqlite3.connect(db_file) as conn: + assert sorted(conn.execute('SELECT id, name, color FROM tag')) == [ + (3, 'one', '#222222'), (4, 'two', '#444444'), (6, 'three', None), (7, 'unchanged', '#777777')] + assert sorted(conn.execute('SELECT tag_id, file_group_id FROM tag_file')) == [ + (3, 10), (3, 11), (4, 11), (6, 10), (7, 10)] + assert conn.execute('SELECT tag_id FROM collection').fetchone()[0] == 3 + tag_names, settings = conn.execute('SELECT tag_names, settings FROM download').fetchone() + assert json.loads(tag_names) == ['one', 'three'] + assert json.loads(settings) == {'channel_tag_name': ['two'], 'video_resolutions': ['720p']} + + tags_config = yaml.safe_load((config_directory / 'tags.yaml').read_text()) + assert tags_config['tags'] == {'one': dict(color='#222222'), 'two': dict(color='#444444'), + 'unchanged': dict(color='#777777')} + assert tags_config['tag_files'] == [['one', '10.mp4', None], ['unchanged', '10.mp4', None]] + assert tags_config['version'] == 3 + channels_config = yaml.safe_load((config_directory / 'channels.yaml').read_text()) + assert channels_config['channels'][0]['tag_name'] == 'one' + assert (config_directory / 'domains.yaml').read_text() == domains + + def test_migrations_match_models(test_directory): """`alembic check`: a database migrated to head matches the SQLAlchemy models (tables, columns, indexes). Fails when a model gains an index or column without a migration, or vice versa.""" diff --git a/wrolpi/test/test_tags.py b/wrolpi/test/test_tags.py index 2ca7d715..41d400fe 100644 --- a/wrolpi/test/test_tags.py +++ b/wrolpi/test/test_tags.py @@ -406,6 +406,32 @@ async def test_invalid_tag(async_client, test_session, test_directory): with pytest.raises(UnknownTag): await tags.upsert_tag(test_session, 'Tag ID does not exist', '#fff', 1) + with pytest.raises(InvalidTag): + await tags.upsert_tag(test_session, ' ', '#fff') + with pytest.raises(InvalidTag): + tags.Tag.get_or_create_tag(test_session, ' \t') + + +@pytest.mark.asyncio +async def test_tag_name_whitespace_stripped(test_session, test_directory, tag_factory): + """Leading and trailing whitespace is never part of a Tag's name.""" + await tags.upsert_tag(test_session, ' one ', '#fff') + tag = tags.Tag.find_by_name(test_session, 'one') + assert tag.name == 'one' + + # Renaming strips too. + await tags.upsert_tag(test_session, '\ttwo\n', '#fff', tag.id) + assert tag.name == 'two' + + # A padded name finds the existing Tag rather than creating a near-duplicate. + assert tags.Tag.get_or_create_tag(test_session, 'two ').id == tag.id + assert tags.Tag.get_or_create_tag(test_session, ' three ').name == 'three' + assert {i.name for i in test_session.query(tags.Tag)} == {'two', 'three'} + + # A padded name conflicts with an existing Tag. + with pytest.raises(InvalidTag): + await tags.upsert_tag(test_session, 'three ', '#fff', tag.id) + @pytest.mark.asyncio async def test_tags_directory(test_session, test_directory, tag_factory, video_factory, example_pdf, await_switches, From 79526afebefc3ed24fe95aad7669e2b85ecd2a95 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:28:30 -0600 Subject: [PATCH 8/9] A malformed config entry is skipped and counted instead of failing the import (#875) * A malformed config entry is skipped and counted instead of failing the import An entry that is not a mapping (or whose key is not text, or a list that is not a list) made the whole file fail to import in playlists.yaml, repos.yaml, download_manager.yaml and tags.yaml (tag_files/tag_zims), and a non-text name did in domains.yaml. Each is now skipped and counted in import_skipped, so the config is not saved over it, and the rest of the file imports. A Channel whose `downloads` is not a list keeps its Downloads. Co-Authored-By: Claude Opus 5.5 * An unreadable entry keeps no row and does not make a section present The key of an entry too malformed to name a row is now `Unreadable(position)`. It is still counted in import_skipped, but `present` ignores it, so a section whose entries are all unreadable removes nothing (as an empty section), instead of deleting every row. A playlist whose `items` is not a list keeps its items, and a Channel Download whose url is not text is counted (its Channel keeps its Downloads) instead of failing the import. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- modules/archive/lib.py | 8 +- modules/repos/lib.py | 12 +-- modules/repos/test/test_config_import.py | 28 +++++ modules/videos/lib.py | 16 ++- wrolpi/collections/config.py | 44 ++++++-- .../collections/test/test_playlists_config.py | 57 ++++++++++ wrolpi/config_import.py | 16 ++- wrolpi/downloader.py | 11 +- wrolpi/tags.py | 22 ++-- wrolpi/test/test_config_malformed_entries.py | 100 ++++++++++++++++++ 10 files changed, 279 insertions(+), 35 deletions(-) create mode 100644 wrolpi/test/test_config_malformed_entries.py diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 5112efe9..2c271228 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -28,7 +28,7 @@ from wrolpi.common import get_media_directory, get_relative_to_media_directory, escape_file_name, aiohttp_post, format_html_string, split_lines_by_length, get_html_soup, get_title_from_html, \ get_wrolpi_config, html_screenshot, html_file_screenshot, ConfigFile, trim_file_name, chunks from wrolpi.collections.models import validate_collection_directory -from wrolpi.config_import import ConfigSection, media_relative +from wrolpi.config_import import ConfigSection, Unreadable, media_relative from wrolpi.dates import now, Seconds from wrolpi.db import get_db_session, get_db_curs, session_curs from wrolpi.errors import UnknownArchive, InvalidOrderBy, InvalidDatetime @@ -65,10 +65,10 @@ class DomainsSection(ConfigSection): rows, invalid = dict(), set() for position, data in enumerate(config.get('collections') or []): name = data.get('name') if isinstance(data, dict) else None - if not name: + if not name or not isinstance(name, str): logger.error(f'Domain collection at index {position} has no name, skipping: {data!r}') - # Counted as skipped; never a domain's name. - invalid.add(f'\0{position}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue try: if not Collection.is_valid_domain_name(name): diff --git a/modules/repos/lib.py b/modules/repos/lib.py index 1dddd1b2..2b2f8126 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -25,7 +25,7 @@ from wrolpi.cmd import GIT_BIN, run_command from wrolpi.collections.models import Collection from wrolpi.common import ConfigFile, get_media_directory, get_relative_to_media_directory, logger, \ escape_file_name, background_task, get_wrolpi_config -from wrolpi.config_import import ConfigSection, media_relative +from wrolpi.config_import import ConfigSection, Unreadable, media_relative from wrolpi.config_version import register_ownership from wrolpi.db import get_db_session from wrolpi.downloader import Download, DownloadFrequency, download_manager, save_downloads_config @@ -1397,11 +1397,11 @@ class ReposSection(ConfigSection): media_directory = get_media_directory() rows, invalid = dict(), set() for position, data in enumerate(config.get('repos') or []): - url = data.get('url') - if not url: - logger.error(f'Repo config entry has no url, skipping: {data}') - # Counted as skipped; never a Repo's key. - invalid.add(f'\0{position}') + url = data.get('url') if isinstance(data, dict) else None + if not url or not isinstance(url, str): + logger.error(f'Repo config entry has no url, skipping: {data!r}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue try: parsed = parse_repo_url(url) diff --git a/modules/repos/test/test_config_import.py b/modules/repos/test/test_config_import.py index caf400d3..7ddf3a34 100644 --- a/modules/repos/test/test_config_import.py +++ b/modules/repos/test/test_config_import.py @@ -102,6 +102,34 @@ async def test_invalid_entry_is_kept(test_session, saved_repos): assert (found[TWO].mode, found[ONE].collection.description) == ('snapshot', 'Changed') +@pytest.mark.asyncio +async def test_malformed_entry_is_skipped(test_session, saved_repos): + """An entry that is not a mapping is skipped and counted; the rest of the file still imports.""" + edit_repo(saved_repos, ONE, description='Changed') + edit_config(saved_repos, lambda data: data['repos'].insert(0, 'not a mapping')) + + config = lib.get_repos_config() + config.import_config() + + assert config.successful_import is True + assert config.import_skipped == 1 + found = repos(test_session) + assert set(found) == {ONE, TWO} + assert found[ONE].collection.description == 'Changed' + + +@pytest.mark.asyncio +async def test_only_malformed_entries_delete_nothing(test_session, saved_repos): + """A file whose entries are all unreadable deletes no Repo (like an empty list).""" + edit_config(saved_repos, lambda data: data.update(repos=['not a mapping', dict(url=['a', 'list'])])) + + config = lib.get_repos_config() + config.import_config() + + assert config.import_skipped == 2 + assert set(repos(test_session)) == {ONE, TWO} + + @pytest.mark.asyncio async def test_failed_import_creates_nothing(test_session, saved_repos, monkeypatch): """A new Repo is created in the import's one write: a failure later in the write leaves nothing (each new Repo diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 550ead92..bcc36bd6 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -20,7 +20,7 @@ from wrolpi.common import ConfigFile, extract_domain, logger, \ escape_file_name, get_media_directory, background_task, Base, get_wrolpi_config, trim_file_name from wrolpi.collections import Collection from wrolpi.collections.models import validate_collection_directory -from wrolpi.config_import import ConfigSection, chunks, media_relative +from wrolpi.config_import import ConfigSection, Unreadable, chunks, media_relative from wrolpi.config_version import register_ownership from wrolpi.dates import Seconds, from_timestamp from wrolpi.db import get_db_curs, get_db_session, session_curs @@ -295,8 +295,8 @@ class ChannelsSection(ConfigSection): key = _channel_key(data) except Exception as e: channel_import_logger.error(f'Skipping invalid Channel in config: {data!r}', exc_info=e) - # Counted as skipped; never a directory. (The entry's name keeps its Channel, see `prepare`.) - invalid.add(f'\0{position}') + # Counted as skipped (see `Unreadable`). The entry's name keeps its Channel (see `prepare`). + invalid.add(Unreadable(position)) continue if key in rows or key in invalid: raise RuntimeError('Refusing to import channels config because it contains duplicate directories!') @@ -394,8 +394,14 @@ class ChannelDownloadsSection(ConfigSection): key = _channel_key(data) except Exception: continue - for position, download in enumerate(data.get('downloads') or []): - if isinstance(download, dict) and download.get('url'): + downloads = data.get('downloads') or [] + if not isinstance(downloads, list): + channel_import_logger.error(f'Skipping the Downloads of Channel {key!r}, they are not a list') + # Counted as skipped; the Channel keeps its Downloads (see `ChannelsConfig.prepare`). + invalid.add((key, '\0downloads')) + continue + for position, download in enumerate(downloads): + if isinstance(download, dict) and download.get('url') and isinstance(download['url'], str): rows[(key, download['url'])] = dict(frequency=download.get('frequency')) else: channel_import_logger.error(f'Skipping invalid Download of Channel {key!r}: {download!r}') diff --git a/wrolpi/collections/config.py b/wrolpi/collections/config.py index 80368d33..6478928a 100644 --- a/wrolpi/collections/config.py +++ b/wrolpi/collections/config.py @@ -7,7 +7,7 @@ from sqlalchemy import insert from sqlalchemy.orm import Session from wrolpi.common import ConfigFile, get_media_directory, get_relative_to_media_directory, logger -from wrolpi.config_import import ConfigSection, SectionDiff, chunks, media_relative +from wrolpi.config_import import ConfigSection, SectionDiff, Unreadable, chunks, media_relative from wrolpi.config_version import count_config_change from wrolpi.db import get_db_session from wrolpi.switches import ActivateSwitchMethod, register_switch_handler @@ -23,16 +23,28 @@ def _playlist_tag_names(rows) -> Set[str]: return {i['tag_name'] for i in rows if i['tag_name']} +# The position in an invalid item key that stands for all of a playlist's items (its `items` is not a list). +ITEMS_UNREADABLE = 0 + + +def _playlist_name(data) -> Optional[str]: + """The name of a playlist entry, or None when the entry is malformed.""" + name = data.get('name') if isinstance(data, dict) else None + return name if isinstance(name, str) and name else None + + class PlaylistsSection(ConfigSection): """playlists.yaml's playlists (Collections with kind 'playlist'), keyed by name.""" name = 'playlists' def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: rows, invalid = dict(), set() - for data in config.get('playlists') or []: - name = data.get('name') + for position, data in enumerate(config.get('playlists') or []): + name = _playlist_name(data) if not name: - logger.error(f'Playlist config entry has no name, skipping: {data}') + logger.error(f'Playlist config entry has no name, skipping: {data!r}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue try: rows[name] = dict( @@ -88,23 +100,32 @@ class PlaylistItemsSection(ConfigSection): @staticmethod def item_key(name: str, data: dict) -> Optional[tuple]: + if not isinstance(data, dict): + raise ValueError('An item must be a mapping') if 'file' in data: return name, 'file', media_relative(data['file']), None elif 'zim' in data: - if not data.get('entry'): + if not data.get('entry') or not isinstance(data['entry'], str): raise ValueError('A zim item requires an entry') return name, 'zim', media_relative(data['zim']), data['entry'] elif 'url' in data: + if not isinstance(data['url'], str): + raise ValueError('An item\'s url must be text') return name, 'url', data['url'], None raise ValueError('An item requires a file, zim, or url') def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: rows, invalid = dict(), set() for data in config.get('playlists') or []: - if not (name := data.get('name')): + if not (name := _playlist_name(data)): + continue + items = data.get('items') or [] + if not isinstance(items, list): + logger.error(f'Skipping the items of playlist {name!r}, they are not a list: {items!r}') + invalid.add((name, None, ITEMS_UNREADABLE, None)) continue # Positions follow the file's order; an item skipped now keeps its place for a later import. - for position, item in enumerate(data.get('items') or [], start=1): + for position, item in enumerate(items, start=1): try: key = self.item_key(name, item) except Exception as e: @@ -121,7 +142,7 @@ class PlaylistItemsSection(ConfigSection): def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: # A listed playlist without items has none: only an empty or missing playlists list removes nothing. - return any(i.get('name') for i in config.get('playlists') or []) + return any(map(_playlist_name, config.get('playlists') or [])) @staticmethod def _query(session: Session, *columns): @@ -250,6 +271,7 @@ class PlaylistsConfig(ConfigFile): def prepare(self, diffs: dict): self._drop_unknown_tags(diffs['playlists']) self._drop_missing_items(diffs['items']) + self._keep_unreadable_items(diffs['items']) if not self.import_deletes: self._merge_items(diffs['items']) @@ -268,6 +290,12 @@ class PlaylistsConfig(ConfigFile): row['tag_name'] = None self._config_outdated = True + def _keep_unreadable_items(self, diff: SectionDiff): + """A playlist whose items cannot be read keeps the items it has.""" + _, invalid = PlaylistItemsSection().file_rows(self._config) + if names := {i[0] for i in invalid if i[2] == ITEMS_UNREADABLE}: + diff.removed = {i for i in diff.removed if i[0] not in names} + def _drop_missing_items(self, diff: SectionDiff): """An added item whose file or Zim is not in the database (not indexed yet, after a rebuild) is skipped before the write; a later import (after the refresh) adds it.""" diff --git a/wrolpi/collections/test/test_playlists_config.py b/wrolpi/collections/test/test_playlists_config.py index f7f55ef3..6d119cf0 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -414,3 +414,60 @@ async def test_import_without_deletes_keeps_database_order(test_session, test_di test_session.expire_all() collection = test_session.query(Collection).filter_by(name='Mix', kind='playlist').one() assert [(i.url, i.position) for i in collection.items] == [('/a', 1), ('/x', 2), ('/b', 3)] + + +@pytest.mark.asyncio +async def test_import_skips_malformed_entries(test_session, test_directory, playlists_config): + """A playlist entry or item that is not a mapping is skipped and counted; the rest of the file still imports.""" + _make_playlist(test_session) + + file = playlists_config.get_file() + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text('version: 1\nplaylists:\n- not a mapping\n- name: Survives\n items:\n - also not a mapping\n' + ' - url: /map?lat=1&lon=2&z=3\n title: spot\n- name: New\n') + + config_was_imported(test_session, 'playlists.yaml') + playlists_config.import_config() + + assert playlists_config.successful_import is True + assert playlists_config.import_skipped == 2 + test_session.expire_all() + playlists = {i.name: i for i in test_session.query(Collection).filter_by(kind='playlist')} + assert set(playlists) == {'Survives', 'New'} + assert len(playlists['Survives'].items) == 1 + + +@pytest.mark.asyncio +async def test_import_only_malformed_entries_preserves_db(test_session, test_directory, playlists_config): + """A file whose playlist entries are all unreadable deletes no playlist (like an empty list).""" + _make_playlist(test_session) + + file = playlists_config.get_file() + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text('version: 1\nplaylists:\n- not a mapping\n- name: [a, list]\n') + + config_was_imported(test_session, 'playlists.yaml') + playlists_config.import_config() + + assert playlists_config.import_skipped == 2 + test_session.expire_all() + collection = test_session.query(Collection).filter_by(kind='playlist').one() + assert (collection.name, len(collection.items)) == ('Survives', 1) + + +@pytest.mark.asyncio +async def test_import_unreadable_items_keeps_items(test_session, test_directory, playlists_config): + """A playlist whose items are not a list keeps the items it has.""" + _make_playlist(test_session) + + file = playlists_config.get_file() + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text('version: 1\nplaylists:\n- name: Survives\n items: 5\n') + + config_was_imported(test_session, 'playlists.yaml') + playlists_config.import_config() + + assert playlists_config.import_skipped == 1 + test_session.expire_all() + collection = test_session.query(Collection).filter_by(kind='playlist').one() + assert len(collection.items) == 1 diff --git a/wrolpi/config_import.py b/wrolpi/config_import.py index fb29ab09..1a10a5ac 100644 --- a/wrolpi/config_import.py +++ b/wrolpi/config_import.py @@ -27,6 +27,18 @@ class SectionDiff: return bool(self.added or self.changed or self.removed) +@dataclass(frozen=True) +class Unreadable: + """The invalid key of an entry too malformed to name a row (not a mapping, no name). It is counted as skipped, so + the file is not saved over it, but it keeps no row, and on its own it does not make a section present.""" + position: int + + +def listed(file_rows: dict, invalid_keys: set) -> bool: + """Whether a section lists anything: a row, or an invalid row it names.""" + return bool(file_rows) or any(not isinstance(i, Unreadable) for i in invalid_keys) + + class ConfigSection: """One kind of row a database config saves. @@ -52,7 +64,7 @@ class ConfigSection: def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: """Whether the file lists this section at all; an empty section cannot be told apart from a lost or blanked file, so it removes nothing.""" - return bool(file_rows or invalid_keys) + return listed(file_rows, invalid_keys) def same(self, file_row: dict, db_row: dict) -> bool: """Whether a file row is already in the database.""" @@ -77,7 +89,7 @@ def diff_section(file_rows: Dict[Hashable, dict], invalid_keys: Set[Hashable], d elif not same(row, db_rows[key]): diff.changed[key] = row if present is None: - present = bool(file_rows or invalid_keys) + present = listed(file_rows, invalid_keys) if deletes and present: diff.removed = db_rows.keys() - file_rows.keys() - invalid_keys return diff diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 4323b259..bf0ef65a 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -34,7 +34,7 @@ from wrolpi.cmd import which, run_command, CommandResult from wrolpi.common import loaded, Base, ModelHelper, logger, wrol_mode_check, zig_zag, ConfigFile, \ wrol_mode_enabled, background_task, get_absolute_media_path, timer, aiohttp_get, \ get_download_info, trim_file_name, get_wrolpi_config, TRACE_LEVEL, normalize_domain -from wrolpi.config_import import ConfigSection, chunks +from wrolpi.config_import import ConfigSection, Unreadable, chunks from wrolpi.config_version import collection_kinds, count_config_change, register_ownership from wrolpi.dates import TZDateTime, now, Seconds from wrolpi.db import get_db_session, session_curs, serializer @@ -2109,8 +2109,13 @@ class DownloadsSection(ConfigSection): def file_rows(self, config: dict) -> Tuple[Dict[str, dict], Set[str]]: defaults = inheritable_video_defaults() rows, invalid = dict(), set() - for data in config.get('downloads') or []: - url = data.get('url') + for position, data in enumerate(config.get('downloads') or []): + url = data.get('url') if isinstance(data, dict) else None + if not url or not isinstance(url, str): + logger.error(f'Download config entry has no url, skipping: {data!r}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) + continue try: settings = dict(data.get('settings') or {}) # An old config kept `destination` and `tag_names` in the settings. diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 0c51c2e2..462b3adf 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -14,7 +14,7 @@ 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, \ get_relative_to_media_directory, is_valid_hex_color, walk, INVALID_FILE_CHARS, get_wrolpi_config -from wrolpi.config_import import ConfigSection, SectionDiff, chunks, media_relative +from wrolpi.config_import import ConfigSection, SectionDiff, Unreadable, chunks, media_relative from wrolpi.config_version import count_config_change, register_ownership from wrolpi.dates import TZDateTime from wrolpi.db import get_db_session, named_placeholders, session_curs, serializer @@ -400,12 +400,16 @@ class TagFilesSection(ConfigSection): def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: tag_names = set(config.get('tags') or dict()) rows, invalid = dict(), set() - for item in config.get('tag_files') or []: + for position, item in enumerate(config.get('tag_files') or []): try: - tag_name, path, *rest = item + tag_name, path, *rest = item if isinstance(item, list) else () + if not isinstance(tag_name, str): + raise ValueError('A Tag name must be text') key = (tag_name, media_relative(path)) except Exception as e: - logger.error(f'Skipping invalid tagged file in config: {item}', exc_info=e) + logger.error(f'Skipping invalid tagged file in config: {item!r}', exc_info=e) + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue if tag_names and tag_name not in tag_names: logger.warning(f'Cannot find Tag for {repr(key[1])}') @@ -499,12 +503,16 @@ class TagZimsSection(ConfigSection): def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: tag_names = set(config.get('tags') or dict()) rows, invalid = dict(), set() - for item in config.get('tag_zims') or []: + for position, item in enumerate(config.get('tag_zims') or []): try: - tag_name, zim_path, zim_entry, *rest = item + tag_name, zim_path, zim_entry, *rest = item if isinstance(item, list) else () + if not isinstance(tag_name, str) or not isinstance(zim_entry, str): + raise ValueError('A Tag name and a Zim entry must be text') key = (tag_name, media_relative(zim_path), zim_entry) except Exception as e: - logger.error(f'Skipping invalid tagged Zim entry in config: {item}', exc_info=e) + logger.error(f'Skipping invalid tagged Zim entry in config: {item!r}', exc_info=e) + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue if tag_names and tag_name not in tag_names: logger.warning(f'Cannot find Tag for {repr(key[1])}') diff --git a/wrolpi/test/test_config_malformed_entries.py b/wrolpi/test/test_config_malformed_entries.py new file mode 100644 index 00000000..b4929b7d --- /dev/null +++ b/wrolpi/test/test_config_malformed_entries.py @@ -0,0 +1,100 @@ +"""A malformed entry in a config (not a mapping, or a key that is not a string) is skipped and counted, so the config +is never saved over it; the rest of the file still imports.""" +import pytest + +from wrolpi.config_import import Unreadable, diff_section + +from modules.archive.lib import DomainsSection +from modules.repos.lib import ReposSection +from modules.videos.lib import ChannelDownloadsSection, ChannelsSection +from wrolpi.collections.config import PlaylistItemsSection, PlaylistsSection +from wrolpi.downloader import DownloadsSection +from wrolpi.tags import TagFilesSection, TagsSection, TagZimsSection + +MALFORMED = ['a string', 5, ['a', 'list'], None] + + +def assert_skipped(section, config, valid_key, malformed_count): + rows, invalid = section.file_rows(config) + assert valid_key in rows + assert len(invalid) >= malformed_count, 'every malformed entry must be counted, so the config is not saved over it' + + +@pytest.mark.parametrize('section,config,valid_key,malformed_count', [ + (PlaylistsSection(), dict(playlists=[*MALFORMED, dict(name=['a', 'list']), dict(name='good')]), 'good', 5), + (ReposSection(), dict(repos=[*MALFORMED, dict(url=['a', 'list']), dict(url='https://example.com/a/b.git')]), + 'example.com/a/b', 5), + (DomainsSection(), dict(collections=[*MALFORMED, dict(name=['a', 'list']), dict(name='example.com')]), + 'example.com', 5), + (ChannelsSection(), dict(channels=[*MALFORMED, dict(name='c', directory=['a', 'list']), + dict(name='good', directory='videos/good')]), 'videos/good', 5), + (DownloadsSection(), dict(downloads=[*MALFORMED, dict(url=['a', 'list'], downloader='archive'), + dict(url='https://example.com/good', downloader='archive')]), + 'https://example.com/good', 5), + (TagFilesSection(), dict(tags=dict(one=dict()), tag_files=[*MALFORMED, [['a', 'list'], 'foo.txt'], + ['one', 'good.txt']]), ('one', 'good.txt'), 5), + (TagZimsSection(), dict(tags=dict(one=dict()), tag_zims=[*MALFORMED, [['a', 'list'], 'a.zim', 'A'], + ['one', 'good.zim', 'A']]), ('one', 'good.zim', 'A'), 5), +]) +def test_malformed_entries(test_directory, section, config, valid_key, malformed_count): + assert_skipped(section, config, valid_key, malformed_count) + + +def test_malformed_tags(test_directory): + """A Tag whose attributes are not a mapping is skipped and counted.""" + assert_skipped(TagsSection(), dict(tags={'one': dict(color='#fff'), 'two': 'a string', 'three': 5}), 'one', 2) + + +def test_malformed_playlist_items(test_directory): + """A playlist's malformed items are skipped and counted; so is an `items` that is not a list.""" + config = dict(playlists=[ + dict(name='good', items=[*MALFORMED, dict(url=['a', 'list']), dict(url='https://example.com/good')]), + dict(name='bad', items=5), + ]) + rows, invalid = PlaylistItemsSection().file_rows(config) + assert list(rows) == [('good', 'url', 'https://example.com/good', None)] + assert len(invalid) == 6 + + +def test_malformed_channel_downloads(test_directory): + """A Channel whose `downloads` is not a list keeps its Downloads (counted).""" + config = dict(channels=[dict(name='good', directory='videos/good', downloads=5)]) + rows, invalid = ChannelDownloadsSection().file_rows(config) + assert rows == dict() + assert invalid + + +@pytest.mark.parametrize('section,config', [ + (PlaylistsSection(), dict(playlists=MALFORMED)), + (PlaylistItemsSection(), dict(playlists=MALFORMED)), + (ReposSection(), dict(repos=MALFORMED)), + (DomainsSection(), dict(collections=[*MALFORMED, dict(name=['a', 'list'])])), + (ChannelsSection(), dict(channels=MALFORMED)), + (DownloadsSection(), dict(downloads=MALFORMED)), + (TagsSection(), dict(tags=dict())), + # A list of two texts is a tagged file's line (of an unknown Tag), not an unreadable one. + (TagFilesSection(), dict(tags=dict(one=dict()), tag_files=['a string', 5, None, [['a'], 'b.txt']])), + (TagZimsSection(), dict(tags=dict(one=dict()), tag_zims=MALFORMED)), +]) +def test_only_malformed_entries_remove_nothing(test_directory, section, config): + """A section whose entries are all unreadable is not present: like an empty section, it removes nothing (it + cannot be told apart from a damaged file).""" + rows, invalid = section.file_rows(config) + assert not rows + assert not section.present(config, rows, invalid) + + +def test_diff_unreadable(): + """An unreadable entry keeps no row and does not make the section present; a named invalid row does both.""" + db_rows = dict(one=dict(), two=dict()) + assert diff_section(dict(), {Unreadable(0)}, db_rows, deletes=True).removed == set() + assert diff_section(dict(), {Unreadable(0), 'one'}, db_rows, deletes=True).removed == {'two'} + + +def test_malformed_channel_download_url(test_directory): + """A Channel's Download whose url is not text is counted, so the Channel keeps its Downloads.""" + config = dict(channels=[dict(name='good', directory='videos/good', downloads=[dict(url=['a', 'list']), dict(url=5)])]) + rows, invalid = ChannelDownloadsSection().file_rows(config) + assert rows == dict() + assert {i[0] for i in invalid} == {'videos/good'} + assert len(invalid) == 2 From c577cae9d87a9bdbe322fa8542bdad2a93ae508a Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:31:27 -0600 Subject: [PATCH 9/9] Renaming a Tag renames it in Download settings (#877) update_tag never renamed settings.tag_names, and its channel_tag_name edit mutated the JSON in place, so it was never saved. The old name stayed, and a Download that later found a new Channel failed (UnknownTag). Co-authored-by: Claude Opus 5.5 --- wrolpi/tags.py | 25 ++++++++++++++++--------- wrolpi/test/test_tags.py | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 462b3adf..0c75c80f 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -189,15 +189,9 @@ class Tag(ModelHelper, Base): from wrolpi.downloader import Download to_flush = list() for download in session.query(Download): - if download.tag_names: - download.tag_names = [i if i != old_name else name for i in download.tag_names] - to_flush.append(download) - if download.settings and 'channel_tag_name' in download.settings: - settings = download.settings - tag_name = settings.pop('channel_tag_name') - tag_name = [i for i in tag_name if i != old_name] - settings['channel_tag_name'] = tag_name - download.settings = settings + tag_names, settings = with_tag_name_renamed(download.tag_names, download.settings, old_name, name) + if tag_names != download.tag_names or settings != download.settings: + download.tag_names, download.settings = tag_names, settings to_flush.append(download) if to_flush: @@ -322,6 +316,19 @@ def without_tag_names(download_tag_names: Optional[List[str]], settings: Optiona return download_tag_names, settings +def with_tag_name_renamed(download_tag_names: Optional[List[str]], settings: Optional[dict], old_name: str, + new_name: str) -> Tuple[Optional[List[str]], Optional[dict]]: + """A Download's tag_names and settings with a Tag's old name replaced, in the settings' tag_names and + channel_tag_name too. New objects, so an ORM change to the JSON is seen.""" + if download_tag_names: + download_tag_names = [new_name if i == old_name else i for i in download_tag_names] + if settings: + settings = {key: [new_name if i == old_name else i for i in value] + 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.)""" diff --git a/wrolpi/test/test_tags.py b/wrolpi/test/test_tags.py index 41d400fe..d1ef8fad 100644 --- a/wrolpi/test/test_tags.py +++ b/wrolpi/test/test_tags.py @@ -595,6 +595,30 @@ async def test_tag_rename_with_channel(test_session, test_directory, video_facto tag.delete() +@pytest.mark.asyncio +async def test_tag_rename_downloads(test_session, tag_factory, await_background_tasks): + """Renaming a Tag renames it everywhere a Download applies it, including the settings passed on to the + Downloads (and Channels) it creates.""" + from wrolpi.downloader import Download + tag, other = await tag_factory('one'), await tag_factory('two') + download = Download(url='https://example.com/playlist', downloader='video_channel', tag_names=['one', 'two'], + settings=dict(tag_names=['two', 'one'], channel_tag_name=['one'], video_resolutions=['720p'])) + untouched = Download(url='https://example.com/other', downloader='video_channel', tag_names=['two'], + settings=dict(tag_names=['two'], channel_tag_name=['two'])) + test_session.add_all([download, untouched]) + test_session.commit() + + await tag.update_tag(test_session, 'New Name', None) + await await_background_tasks() + test_session.commit() + + assert download.tag_names == ['New Name', 'two'] + assert download.settings == dict(tag_names=['two', 'New Name'], channel_tag_name=['New Name'], + video_resolutions=['720p']) + assert untouched.tag_names == ['two'] + assert untouched.settings == dict(tag_names=['two'], channel_tag_name=['two']) + + @pytest.mark.asyncio async def test_tags_directory_file_replaced(test_session, test_directory, tag_factory, video_bytes, await_switches): """Tags directory hardlinks should update when source file is replaced."""