From a787c02908206b5c4dc12b93535b12ed6639009d Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 25 Oct 2026 15:25:31 -0600 Subject: [PATCH] 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 --- modules/archive/lib.py | 2 +- modules/repos/lib.py | 6 +- modules/repos/test/test_config_import.py | 16 +++++ modules/videos/lib.py | 8 ++- wrolpi/collections/config.py | 31 +++++++--- .../collections/test/test_playlists_config.py | 20 ++++++ wrolpi/downloader.py | 9 ++- wrolpi/tags.py | 20 ++++-- wrolpi/test/test_config_malformed_entries.py | 62 +++++++++++++++++++ 9 files changed, 154 insertions(+), 20 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..548229bc 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -65,7 +65,7 @@ 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}') diff --git a/modules/repos/lib.py b/modules/repos/lib.py index 1dddd1b2..3c67d4e2 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -1397,9 +1397,9 @@ 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}') + 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}') continue diff --git a/modules/repos/test/test_config_import.py b/modules/repos/test/test_config_import.py index caf400d3..6b60f0c1 100644 --- a/modules/repos/test/test_config_import.py +++ b/modules/repos/test/test_config_import.py @@ -102,6 +102,22 @@ 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_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..d541553d 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -394,7 +394,13 @@ class ChannelDownloadsSection(ConfigSection): key = _channel_key(data) except Exception: continue - for position, download in enumerate(data.get('downloads') or []): + 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'): rows[(key, download['url'])] = dict(frequency=download.get('frequency')) else: diff --git a/wrolpi/collections/config.py b/wrolpi/collections/config.py index 80368d33..d1673e11 100644 --- a/wrolpi/collections/config.py +++ b/wrolpi/collections/config.py @@ -23,16 +23,24 @@ def _playlist_tag_names(rows) -> Set[str]: return {i['tag_name'] for i in rows if i['tag_name']} +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; never a playlist's name. + invalid.add(f'\0{position}') continue try: rows[name] = dict( @@ -88,23 +96,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, 0, 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 +138,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): diff --git a/wrolpi/collections/test/test_playlists_config.py b/wrolpi/collections/test/test_playlists_config.py index f7f55ef3..bb3f4f49 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -414,3 +414,23 @@ 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') + + 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 diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 4323b259..06ffd2d2 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -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; never a Download's URL. + invalid.add(f'\0{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 2d38c5c9..0dcf7e0b 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -397,12 +397,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; never a tagged file's key. + invalid.add((f'\0{position}',)) continue if tag_names and tag_name not in tag_names: logger.warning(f'Cannot find Tag for {repr(key[1])}') @@ -496,12 +500,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; never a tagged Zim entry's key. + invalid.add((f'\0{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..b003a5f9 --- /dev/null +++ b/wrolpi/test/test_config_malformed_entries.py @@ -0,0 +1,62 @@ +"""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 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