From b1db71f04a4f21182f69aba7bf56c88338b90f51 Mon Sep 17 00:00:00 2001 From: Roland Date: Fri, 13 Mar 2026 15:27:43 -0600 Subject: [PATCH] Handling when skip_urls is a None. Logging a warning (and ignoring) when duplicate json files are in a Video FileGroup. --- modules/videos/common.py | 2 +- modules/videos/test/test_common.py | 20 ++++++++++++++++++++ modules/videos/test/test_models.py | 29 +++++++++++++++++++++++++---- wrolpi/downloader.py | 2 +- wrolpi/files/models.py | 7 +++++++ wrolpi/files/test/test_lib.py | 8 ++++---- wrolpi/test/test_downloader.py | 17 +++++++++++++++++ 7 files changed, 75 insertions(+), 10 deletions(-) diff --git a/modules/videos/common.py b/modules/videos/common.py index 3eacdc2d..9401eb05 100644 --- a/modules/videos/common.py +++ b/modules/videos/common.py @@ -345,7 +345,7 @@ async def update_view_counts_and_censored(channel_id: int): logger.info(f'No info_json for channel {channel.name}') return - view_counts = [{'id': i['id'], 'view_count': i['view_count']} for i in info['entries']] + view_counts = [{'id': i['id'], 'view_count': i['view_count']} for i in info['entries'] if i.get('view_count') is not None] view_counts_str = json.dumps(view_counts) with get_db_curs(commit=True) as curs: diff --git a/modules/videos/test/test_common.py b/modules/videos/test/test_common.py index d8f35acb..9c9dbf8d 100644 --- a/modules/videos/test/test_common.py +++ b/modules/videos/test/test_common.py @@ -129,6 +129,26 @@ async def test_update_view_count(test_session, channel_factory, video_factory): check_view_counts({'vid1': 10, 'vid2': 11, 'vid3': 13, 'vid4': 14}) +@pytest.mark.asyncio +async def test_update_view_count_missing_view_count(test_session, channel_factory, video_factory): + """Entries without a view_count key (e.g. private/unavailable videos) are skipped without error.""" + channel = channel_factory() + video_factory(channel_id=channel.id, with_poster_ext='jpg', title='vid1') + channel.info_json = {'entries': [ + {'id': 'vid1', 'view_count': 42}, + {'id': 'vid_private'}, # No view_count key. + {'id': 'vid_null', 'view_count': None}, + ]} + test_session.commit() + + # Should not raise KeyError. + await update_view_counts_and_censored(channel.id) + + test_session.expire_all() + video = test_session.query(Video).filter_by(source_id='vid1').one() + assert video.view_count == 42 + + def test_generate_video_poster(video_file): """ A poster can be generated from a video file. diff --git a/modules/videos/test/test_models.py b/modules/videos/test/test_models.py index b8a0ffda..fd9aae99 100644 --- a/modules/videos/test/test_models.py +++ b/modules/videos/test/test_models.py @@ -69,22 +69,43 @@ def test_replace_info_json_compact(test_session, make_files_structure): def test_replace_info_json_multiple_json_files(test_session, make_files_structure): - """FileGroup.replace_info_json raises ValueError when multiple .info.json files are tracked.""" + """FileGroup.replace_info_json raises ValueError when multiple ambiguous .info.json files are tracked.""" make_files_structure({ 'video.mp4': 'fake video', - 'video.info.json': '{"title": "one"}', - 'video.other.info.json': '{"title": "two"}', + 'video.a.info.json': '{"title": "one"}', + 'video.b.info.json': '{"title": "two"}', }) fg = FileGroup.from_paths(test_session, get_media_directory() / 'video.mp4') test_session.add(fg) test_session.flush() - fg.append_files(get_media_directory() / 'video.info.json', get_media_directory() / 'video.other.info.json') + fg.append_files(get_media_directory() / 'video.a.info.json', get_media_directory() / 'video.b.info.json') + # Both have dots in their stems, so neither is "clean" — still raises ValueError. with pytest.raises(ValueError, match='multiple .info.json files'): fg.replace_info_json({'title': 'new'}) +def test_find_unique_json_cleans_double_extension(test_session, make_files_structure): + """When both stem.info.json and stem.mp4.info.json exist, the clean one is preferred and duplicates are ignored.""" + make_files_structure({ + 'video.mp4': 'fake video', + 'video.info.json': '{"title": "clean"}', + 'video.mp4.info.json': '{"title": "double"}', + }) + fg = FileGroup.from_paths(test_session, get_media_directory() / 'video.mp4') + test_session.add(fg) + test_session.flush() + + fg.append_files(get_media_directory() / 'video.info.json', get_media_directory() / 'video.mp4.info.json') + + # Should return the clean one and ignore the double-extension file. + result = fg.info_json_path + assert result == get_media_directory() / 'video.info.json' + # The duplicate file still exists on disk, it's just ignored. + assert (get_media_directory() / 'video.mp4.info.json').exists() + + def test_update_wrolpi_json_merges(test_session, make_files_structure): """FileGroup.update_wrolpi_json merges new keys without clobbering existing ones.""" make_files_structure({'video.mp4': 'fake video'}) diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 0f706b46..42bab7f1 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -1532,7 +1532,7 @@ class DownloadManagerConfig(ConfigFile): @property def skip_urls(self) -> List[str]: - return self._config['skip_urls'] + return self._config['skip_urls'] or [] @skip_urls.setter def skip_urls(self, value: List[str]): diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index 8e3d39cc..bcbe4a83 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -361,8 +361,15 @@ class FileGroup(ModelHelper, Base): """Find a JSON file ending with `suffix` in this FileGroup's files. @raise ValueError: If multiple files matching the suffix are found.""" + from wrolpi.files.lib import split_path_stem_and_suffix matches = [f for f in self.my_json_files() if f['path'].name.endswith(suffix)] if len(matches) > 1: + # Prefer the file whose stem has no extra extension (e.g. video.info.json over video.mp4.info.json). + clean = [f for f in matches if '.' not in split_path_stem_and_suffix(f['path'].name)[0]] + if len(clean) == 1: + duplicates = [f['path'] for f in matches if f not in clean] + logger.warning(f'{self} has duplicate {suffix} files which will be ignored: {duplicates}') + return clean[0]['path'] raise ValueError(f'{self} has multiple {suffix} files: {[f["path"] for f in matches]}') if matches: return matches[0]['path'] diff --git a/wrolpi/files/test/test_lib.py b/wrolpi/files/test/test_lib.py index ed01ffb5..507e1c18 100644 --- a/wrolpi/files/test/test_lib.py +++ b/wrolpi/files/test/test_lib.py @@ -1,9 +1,7 @@ -import asyncio import json import pathlib import shutil import zipfile -from datetime import datetime from http import HTTPStatus from pathlib import Path from typing import List @@ -418,6 +416,7 @@ async def test_refresh_many_files(async_client, test_session, make_files_structu await refresh_files() assert test_session.query(FileGroup).count() == file_count + @pytest.mark.asyncio async def test_mime_type(async_client, test_session, make_files_structure, test_directory, assert_files, refresh_files): """Files module uses the `file` command to get the mimetype of each file.""" @@ -453,7 +452,8 @@ async def test_mime_type(async_client, test_session, make_files_structure, test_ @pytest.mark.asyncio -async def test_files_indexer(async_client, test_session, make_files_structure, test_directory, refresh_files): +async def test_files_indexer(async_client, test_session, make_files_structure, test_directory, refresh_files, + await_background_tasks): """An Indexer is provided for each file based on it's mimetype or contents.""" source_files: List[str] = [ 'a text file.txt', @@ -510,6 +510,7 @@ async def test_files_indexer(async_client, test_session, make_files_structure, t # Change the contents, the file should be re-indexed. text_path.write_text('new text contents') await refresh_files() + await await_background_tasks() files, total = lib.search_files('new', 10, 0) assert total == 1 @@ -1382,7 +1383,6 @@ async def test_rename_non_primary_file_renames_filegroup(async_client, test_sess assert len(file_group.files) == 3 - @pytest.mark.asyncio async def test_file_search_url(test_session, make_files_structure, refresh_files): """Test searching FileGroups by their URL.""" diff --git a/wrolpi/test/test_downloader.py b/wrolpi/test/test_downloader.py index 614b1875..24804629 100644 --- a/wrolpi/test/test_downloader.py +++ b/wrolpi/test/test_downloader.py @@ -509,6 +509,23 @@ async def test_skip_urls(test_session, test_download_manager, assert_download_ur assert get_download_manager_config().skip_urls == [] +@pytest.mark.asyncio +async def test_skip_urls_none(test_session, test_download_manager, test_downloader, test_download_manager_config): + """The DownloadManager does not crash when skip_urls is None in the config.""" + config = get_download_manager_config() + config._config['skip_urls'] = None + + test_downloader.set_test_success() + + # Should not raise TypeError. + test_download_manager.create_downloads(test_session, [ + 'https://example.com/1', + ], downloader_name=test_downloader.name) + + downloads = test_session.query(Download).all() + assert len(downloads) == 1 + + @pytest.mark.asyncio async def test_process_runner_timeout(async_client, test_session, test_directory): """A Downloader can cancel its download using a timeout."""