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 <noreply@anthropic.com>

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

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():

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, 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
@@ -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 or not isinstance(name, str):
+                logger.error(f'Domain collection at index {position} has no name, skipping: {data!r}')
+                # Counted as skipped (see `Unreadable`).
+                invalid.add(Unreadable(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')
 
@@ -393,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()
@@ -978,60 +999,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()
 
 

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'

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', []))

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)
 
 

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', []))

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, 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
 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 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)
+            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:

modules/repos/test/test_config_import.py

@@ -0,0 +1,179 @@
+"""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_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
+    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/<branch>`, GitLab `/-/tree/<branch>`) 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}

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'
 
 

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
 

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, 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
-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,195 @@ 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 (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!')
+            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
+            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}')
+                    # 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 +458,7 @@ class ChannelsConfigValidator:
 
 class ChannelsConfig(ConfigFile):
     file_name = 'channels.yaml'
+    sections = (ChannelsSection(), ChannelDownloadsSection())
     default_config = dict(
         channels=[dict(
             name='WROLPi',
@@ -285,172 +479,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."""
@@ -777,10 +915,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
@@ -937,7 +1071,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 +1089,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()
 
 

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:

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 [])

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

wrolpi/collections/config.py

@@ -1,13 +1,17 @@
 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, Unreadable, 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.files.progress import RecentlyViewedConfig, save_recently_viewed_config
 from wrolpi.tags import TagFile, TagsConfig, save_tags_config
 
 logger = logger.getChild(__name__)
@@ -15,9 +19,216 @@ 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']}
+
+
+# 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 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!r}')
+                # Counted as skipped (see `Unreadable`).
+                invalid.add(Unreadable(position))
+                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 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') 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 := _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(items, 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(map(_playlist_name, 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 +259,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 +268,90 @@ 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'])
+        self._keep_unreadable_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 _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."""
         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,9 +464,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 its 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():
@@ -315,7 +479,12 @@ 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()
         from .sync import sync_playlists_directory
         sync_playlists_directory.activate_switch()

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

wrolpi/collections/test/test_playlists_config.py

@@ -392,3 +392,82 @@ 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)]
+
+
+@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

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

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
+        try:
             if file.is_file():
-            if not self.is_valid(file):
-                raise InvalidConfig(f'Config is invalid: {file_str}')
-
-            data = self.read_config_file(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)
-            # 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.')
+                # 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
@@ -844,12 +851,21 @@ 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
+        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):
@@ -882,14 +898,15 @@ 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
 
     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.  `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
@@ -914,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:
@@ -958,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)."""
@@ -987,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:
@@ -1456,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.
@@ -1660,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']
@@ -1689,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}'

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}')

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:
@@ -24,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.
 
@@ -46,6 +61,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 listed(file_rows, 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 +77,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 = listed(file_rows, 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)

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(

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
@@ -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;
@@ -1043,8 +1048,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 +1070,10 @@ class DownloadManager:
         if channel := Channel.get_by_url(session, url=download.url):
             download.collection_id = channel.collection_id
 
+        if commit:
             session.commit()
+        else:
+            session.flush([download])
 
         return download
 
@@ -2101,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.
@@ -2423,14 +2436,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()

wrolpi/files/models.py

@@ -905,6 +905,24 @@ 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)
+# 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):
     """A representation of a file directory in the media directory."""
     __tablename__ = 'directory'

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.
+    def prepare(self, diffs: dict):
+        diff, db_rows = diffs['viewed'], self.import_db_rows['viewed']
+        file_rows = ViewedSection().file_rows(self._config)[0]
+
+        # 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}
+
+        # 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()
-            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]
-
-            # 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.
+        known = dict()
         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()}
+            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.
+        # 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}')
-                            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
+                del diff.added[key]
 
 
 RECENTLY_VIEWED_CONFIG: RecentlyViewedConfig = RecentlyViewedConfig()

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))

wrolpi/files/worker.py

@@ -759,6 +759,30 @@ 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')
+                    # 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

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, 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
@@ -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')
@@ -187,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:
@@ -266,6 +262,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
@@ -307,12 +304,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
@@ -325,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.)"""
@@ -339,6 +343,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):
@@ -395,12 +407,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
-                key = (tag_name, _relative_path(path))
+                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])}')
@@ -494,12 +510,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
-                key = (tag_name, _relative_path(zim_path), zim_entry)
+                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])}')

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)

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

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."""

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]

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,
@@ -569,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."""