From 96d5aa3a99c8755eaae78beb0d1419dcbb8d80c2 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 25 Oct 2026 16:02:08 -0600 Subject: [PATCH] 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 --- modules/archive/lib.py | 6 +-- modules/repos/lib.py | 6 +-- modules/repos/test/test_config_import.py | 12 ++++++ modules/videos/lib.py | 8 ++-- wrolpi/collections/config.py | 19 ++++++++-- .../collections/test/test_playlists_config.py | 37 ++++++++++++++++++ wrolpi/config_import.py | 16 +++++++- wrolpi/downloader.py | 6 +-- wrolpi/tags.py | 10 ++--- wrolpi/test/test_config_malformed_entries.py | 38 +++++++++++++++++++ 10 files changed, 134 insertions(+), 24 deletions(-) diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 548229bc..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 @@ -67,8 +67,8 @@ class DomainsSection(ConfigSection): name = data.get('name') if isinstance(data, dict) else None 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 3c67d4e2..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 @@ -1400,8 +1400,8 @@ class ReposSection(ConfigSection): 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; never a Repo's key. - invalid.add(f'\0{position}') + # 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 6b60f0c1..7ddf3a34 100644 --- a/modules/repos/test/test_config_import.py +++ b/modules/repos/test/test_config_import.py @@ -118,6 +118,18 @@ async def test_malformed_entry_is_skipped(test_session, saved_repos): 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 d541553d..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!') @@ -401,7 +401,7 @@ class ChannelDownloadsSection(ConfigSection): invalid.add((key, '\0downloads')) continue for position, download in enumerate(downloads): - if isinstance(download, dict) and download.get('url'): + 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 d1673e11..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,6 +23,10 @@ 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 @@ -39,8 +43,8 @@ class PlaylistsSection(ConfigSection): name = _playlist_name(data) if not name: logger.error(f'Playlist config entry has no name, skipping: {data!r}') - # Counted as skipped; never a playlist's name. - invalid.add(f'\0{position}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue try: rows[name] = dict( @@ -118,7 +122,7 @@ class PlaylistItemsSection(ConfigSection): 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, 0, None)) + 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(items, start=1): @@ -267,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']) @@ -285,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 bb3f4f49..6d119cf0 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -426,6 +426,7 @@ async def test_import_skips_malformed_entries(test_session, test_directory, play 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 @@ -434,3 +435,39 @@ async def test_import_skips_malformed_entries(test_session, test_directory, play 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 06ffd2d2..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 @@ -2113,8 +2113,8 @@ class DownloadsSection(ConfigSection): 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; never a Download's URL. - invalid.add(f'\0{position}') + # Counted as skipped (see `Unreadable`). + invalid.add(Unreadable(position)) continue try: settings = dict(data.get('settings') or {}) diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 0dcf7e0b..912a01d7 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 @@ -405,8 +405,8 @@ class TagFilesSection(ConfigSection): key = (tag_name, media_relative(path)) except Exception as e: logger.error(f'Skipping invalid tagged file in config: {item!r}', exc_info=e) - # Counted as skipped; never a tagged file's key. - invalid.add((f'\0{position}',)) + # 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])}') @@ -508,8 +508,8 @@ class TagZimsSection(ConfigSection): key = (tag_name, media_relative(zim_path), zim_entry) except Exception as e: logger.error(f'Skipping invalid tagged Zim entry in config: {item!r}', exc_info=e) - # Counted as skipped; never a tagged Zim entry's key. - invalid.add((f'\0{position}',)) + # 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 index b003a5f9..b4929b7d 100644 --- a/wrolpi/test/test_config_malformed_entries.py +++ b/wrolpi/test/test_config_malformed_entries.py @@ -2,6 +2,8 @@ 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 @@ -60,3 +62,39 @@ def test_malformed_channel_downloads(test_directory): 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