diff --git a/modules/videos/lib.py b/modules/videos/lib.py index dbfe0f5b..777c07db 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -24,7 +24,7 @@ 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 @@ -388,18 +388,20 @@ class ChannelDownloadsSection(ConfigSection): name = 'downloads' def file_rows(self, config: dict) -> Tuple[Dict[tuple, dict], Set[tuple]]: - rows = dict() + rows, invalid = dict(), set() for data in config.get('channels') or []: try: key = _channel_key(data) except Exception: continue - for download in data.get('downloads') or []: + 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}') - return rows, set() + # 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 @@ -491,22 +493,44 @@ class ChannelsConfig(ConfigFile): 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 # 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). + # 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 + 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]} + + # 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} + # 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: + 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 + def _channel_entries(self) -> List[Tuple[str, dict]]: """The valid entries of the file, keyed by directory.""" entries = list() @@ -535,7 +559,8 @@ class ChannelsConfig(ConfigFile): .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]) - for id_, url, source_id, download_missing_data 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_}') @@ -555,6 +580,9 @@ class ChannelsConfig(ConfigFile): 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.""" @@ -1041,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) diff --git a/modules/videos/test/test_channels_config_import.py b/modules/videos/test/test_channels_config_import.py index f5bab281..735a697a 100644 --- a/modules/videos/test/test_channels_config_import.py +++ b/modules/videos/test/test_channels_config_import.py @@ -204,3 +204,83 @@ async def test_no_links_while_entries_are_skipped(test_session, saved_channels, 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 3e033cad..4a344734 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -378,9 +378,10 @@ class Download(ModelHelper, Base): # noqa download_manager.add_to_skip_list(self.url) def delete(self, add_to_skip_list: bool = True, commit: bool = True): - """`commit`: False only flushes (the caller's write commits).""" + """`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) @@ -390,10 +391,10 @@ class Download(ModelHelper, Base): # noqa collection_kind = collection.kind if collection else None session.delete(self) - if commit: - session.commit() - else: + if not commit: session.flush([self]) + return + session.commit() # Save appropriate config based on collection kind. Channels/domains own their configs; # other kinds have no generic collections config to save.