From 1ded249fc7e58cbcfd4aca656316c89a8e86bf96 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:33:51 -0600 Subject: [PATCH] Collections: changes wait for their config's import; domains with archives survive the import (#784) * collections: refuse playlist changes until playlists.yaml is imported While WROLPi starts, the API accepts requests before every config is imported. A playlist created then could not be saved, and the import which followed deleted it as "no longer in config" (and rebuilt every playlist's items from the config, undoing any change). Playlist changes now wait for the import (503, "still starting"). Co-Authored-By: Claude Opus 5.5 * collections: refuse domain changes until domains.yaml is imported A domain created, changed, tagged or deleted through the API before domains.yaml was imported (e.g. while WROLPi starts) was undone by the import. These now wait for the import (503, "still starting"). Co-Authored-By: Claude Opus 5.5 * archive: importing domains.yaml keeps domains which have archives Archives create their domain (e.g. one uploaded, or found by a refresh, while WROLPi starts, before domains.yaml is imported). The import deleted every domain missing from the config, and the database deleted that domain's Archives with it. A domain with Archives is now kept (and saved to the config with the next change). Co-Authored-By: Claude Opus 5.5 * videos: refuse Channel changes until channels.yaml is imported A Channel created through the API before channels.yaml was imported (e.g. while WROLPi starts) could not be saved, and the import which followed deleted it, and its Downloads, as "not in the config". Creating, changing, tagging and deleting a Channel (through the Channel or Collection API) now wait for the import (503, "still starting"). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- modules/archive/lib.py | 15 +++- modules/videos/channel/lib.py | 13 +++- modules/videos/test/test_channels_config.py | 44 +++++++++++ wrolpi/collections/api.py | 8 +- wrolpi/collections/lib.py | 23 ++++++ .../collections/test/test_domains_config.py | 75 +++++++++++++++++++ .../collections/test/test_playlists_config.py | 42 +++++++++++ wrolpi/common.py | 10 ++- wrolpi/errors.py | 6 ++ 9 files changed, 229 insertions(+), 7 deletions(-) diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 0b5af39a..e5562577 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -153,12 +153,19 @@ class DomainsConfig(ConfigFile): 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 + # 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 collection.name not in imported_domains: - logger.info(f'Deleting domain collection {repr(collection.name)} (no longer in config)') - session.delete(collection) + 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.successful_import = True diff --git a/modules/videos/channel/lib.py b/modules/videos/channel/lib.py index f32d31c1..b4e983e2 100644 --- a/modules/videos/channel/lib.py +++ b/modules/videos/channel/lib.py @@ -17,12 +17,19 @@ from wrolpi.tags import Tag from .. import schema from ..common import check_for_channel_conflicts from ..errors import UnknownChannel -from ..lib import save_channels_config +from ..lib import save_channels_config, get_channels_config from ..models import Channel, Video logger = logger.getChild(__name__) +def check_channels_config_imported(): + """Raise ConfigNotImported until channels.yaml is imported; the import would undo a change made before it.""" + # A test may have no channels config. + if config := get_channels_config(): + config.check_imported() + + async def get_minimal_channels(session: Session) -> List[dict]: """ Get the minimum amount of information necessary about all channels. @@ -112,6 +119,7 @@ def get_channel(session: Session, *, channel_id: int = None, source_id: str = No async def update_channel(session: Session, *, data: schema.ChannelPutRequest, channel_id: int) -> Channel: """Update a Channel's DB record""" + check_channels_config_imported() channel = Channel.find_by_id(session, channel_id) # Verify that the URL/Name/directory aren't taken @@ -149,6 +157,7 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic """ Create a new Channel. Check for conflicts with existing Channels. """ + check_channels_config_imported() try: # Verify that the URL/Name/directory aren't taken check_for_channel_conflicts( @@ -189,6 +198,7 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic def delete_channel(session: Session, *, channel_id: int): + check_channels_config_imported() channel = Channel.find_by_id(session, channel_id) channel_dict = channel.dict() @@ -292,6 +302,7 @@ async def tag_channel(session: Session, tag_name: str | None, directory: pathlib """Add a Tag to a Channel, or remove a Tag from a Channel if no `tag_name` is provided. Move the Channel to the new directory, if provided.""" + check_channels_config_imported() if directory and flags.file_worker_busy.is_set(): raise FileWorkerConflict('Refusing to move channel while FileWorker is busy') diff --git a/modules/videos/test/test_channels_config.py b/modules/videos/test/test_channels_config.py index dbd53ca4..c1bf085d 100644 --- a/modules/videos/test/test_channels_config.py +++ b/modules/videos/test/test_channels_config.py @@ -6,6 +6,9 @@ Tests cover: 2. Upgrade - config file exists with data 3. Edge cases - empty config, missing config with DB data """ +import json +from http import HTTPStatus + import pytest import yaml @@ -322,3 +325,44 @@ class TestChannelsConfigEdgeCases: # Verify file_format restored test_session.refresh(channel) assert channel.collection.file_format == '%(upload_date)s_%(title)s.%(ext)s' + + +@pytest.mark.asyncio +async def test_channel_changes_wait_for_config_import(test_session, test_directory, async_client, channel_factory, + test_channels_config): + """Until channels.yaml is imported (e.g. while WROLPi starts), Channels cannot change; the import would undo the + change (a new Channel would be deleted, with its Downloads).""" + channel = channel_factory(name='Kept') + test_session.commit() + config = get_channels_config() + config.dump_config() + assert config.get_file().is_file() + config.successful_import = False + + cid, collection_id = channel.id, channel.collection_id + requests = [ + ('post', '/api/videos/channels', dict(name='New', directory='videos/new', url='https://example.com/new')), + ('put', f'/api/videos/channels/{cid}', dict(name='Changed', directory=str(channel.directory))), + ('post', f'/api/videos/channels/{cid}/tag', dict(tag_name='Tagged')), + ('delete', f'/api/videos/channels/{cid}', None), + ('put', f'/api/collections/{collection_id}', dict(description='changed')), + ('post', f'/api/collections/{collection_id}/tag', dict(tag_name='Tagged')), + ('delete', f'/api/collections/{collection_id}', None), + ] + for method, url, body in requests: + kwargs = dict(content=json.dumps(body)) if body else {} + request, response = await getattr(async_client, method)(url, **kwargs) + assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE, (method, url, response.json) + assert response.json['code'] == 'CONFIG_NOT_IMPORTED' + + test_session.expire_all() + channel = test_session.query(Channel).one() + assert channel.name == 'Kept' + assert channel.tag_name is None + assert channel.collection.description is None + + # Once imported, Channels can change again. + config.import_config() + request, response = await async_client.put(f'/api/collections/{collection_id}', + content=json.dumps(dict(description='x'))) + assert response.status_code == HTTPStatus.OK, response.json diff --git a/wrolpi/collections/api.py b/wrolpi/collections/api.py index 9cee9b70..980bb21c 100644 --- a/wrolpi/collections/api.py +++ b/wrolpi/collections/api.py @@ -10,7 +10,7 @@ from sanic_ext.extensions.openapi import openapi from wrolpi.api_utils import json_response from wrolpi.common import wrol_mode_check -from wrolpi.errors import ValidationError +from wrolpi.errors import ConfigNotImported, ValidationError from wrolpi.schema import JSONErrorResponse from . import lib, schema from .errors import UnknownCollection @@ -211,6 +211,8 @@ async def put_collection_endpoint(request: Request, collection_id: int, body: sc except UnknownCollection as e: return json_response({'error': str(e)}, status=HTTPStatus.NOT_FOUND) + except ConfigNotImported: + raise except Exception as e: return json_response({'error': str(e)}, status=HTTPStatus.BAD_REQUEST) @@ -236,6 +238,8 @@ async def refresh_collection_endpoint(_: Request, collection_id: int): except UnknownCollection as e: return json_response({'error': str(e)}, status=HTTPStatus.NOT_FOUND) + except ConfigNotImported: + raise except Exception as e: return json_response({'error': str(e)}, status=HTTPStatus.BAD_REQUEST) @@ -273,6 +277,8 @@ async def tag_collection_endpoint(request: Request, collection_id: int, body: sc except UnknownCollection as e: return json_response({'error': str(e)}, status=HTTPStatus.NOT_FOUND) + except ConfigNotImported: + raise except Exception as e: return json_response({'error': str(e)}, status=HTTPStatus.BAD_REQUEST) diff --git a/wrolpi/collections/lib.py b/wrolpi/collections/lib.py index 837ef872..001e2320 100644 --- a/wrolpi/collections/lib.py +++ b/wrolpi/collections/lib.py @@ -39,6 +39,22 @@ __all__ = [ ] +def check_config_imported(kind: str): + """Raise ConfigNotImported unless the config which holds this kind of Collection was imported. A change made + before the import (e.g. while WROLPi starts) would be undone by it.""" + if kind == 'playlist': + from .config import get_playlists_config + get_playlists_config().check_imported() + elif kind == 'domain': + # Local import to avoid circular import: collections -> archive -> collections + from modules.archive.lib import get_domains_config + get_domains_config().check_imported() + elif kind == 'channel': + # Local import to avoid circular import: collections -> videos -> collections + from modules.videos.channel.lib import check_channels_config_imported + check_channels_config_imported() + + def _activate_config_save(kind: str): """Activate the config-save switch for the config that owns this kind of Collection. @@ -272,6 +288,7 @@ def update_collection( ValidationError: If validation fails """ collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) # The managed location before any rename/retag. The API reports this location for a playlist # whose stored directory is None, so the UI round-trips it back; recognize it below and keep @@ -372,6 +389,7 @@ def create_collection(session: Session, name: str, description: Optional[str] = raise ValidationError('Collection name is required') if not collection_type_registry.is_registered(kind): raise ValidationError(f'Unknown collection kind: {kind!r}') + check_config_imported(kind) if not collection_type_registry.validate(kind, name): description_msg = collection_type_registry.get_description(kind) or 'Invalid name format' raise ValidationError(f'Invalid {kind} name {name!r}. {description_msg}') @@ -414,6 +432,7 @@ def add_collection_item(session: Session, collection_id: int, item_kind: str, """ from .models import CollectionItem collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) if item_kind == 'file': from wrolpi.files.models import FileGroup @@ -458,6 +477,7 @@ def add_collection_item(session: Session, collection_id: int, item_kind: str, def remove_collection_item(session: Session, collection_id: int, item_id: int) -> bool: """Remove an item from a collection by CollectionItem id. Returns True if removed.""" collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) removed = collection.remove_item(session, item_id) if removed: from .config import save_playlists_config @@ -470,6 +490,7 @@ def remove_collection_item(session: Session, collection_id: int, item_id: int) - def reorder_collection_items(session: Session, collection_id: int, item_ids: List[int]): """Reorder a collection's items from a full list of CollectionItem ids.""" collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) collection.reorder_items(session, item_ids) from .config import save_playlists_config save_playlists_config.activate_switch() @@ -593,6 +614,7 @@ async def tag_collection( FileWorkerConflict: If a file operation is in progress and directory change is requested """ collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) # Track old directory before any changes for potential file moving old_directory = collection.directory @@ -758,6 +780,7 @@ def delete_collection( UnknownCollection: If collection not found """ collection = Collection.find_by_id(session, collection_id) + check_config_imported(collection.kind) collection_dict = { 'id': collection.id, diff --git a/wrolpi/collections/test/test_domains_config.py b/wrolpi/collections/test/test_domains_config.py index d96b096b..ee1abbc3 100644 --- a/wrolpi/collections/test/test_domains_config.py +++ b/wrolpi/collections/test/test_domains_config.py @@ -1,6 +1,9 @@ """Tests for DomainsConfig functionality.""" +import json import pathlib +from http import HTTPStatus +import pytest import yaml from sqlalchemy.orm import Session @@ -256,3 +259,75 @@ class TestDomainsConfig: assert not config['directory'].startswith('/'), \ f"Expected relative path but got absolute: {config['directory']}" assert config['directory'] == 'archive/example.com' + + +@pytest.mark.asyncio +async def test_domain_changes_wait_for_config_import(test_session: Session, test_directory: pathlib.Path, + async_client): + """Until domains.yaml is imported (e.g. while WROLPi starts), domains cannot be changed through the API; the + import would undo the change (or delete a new domain).""" + domain = Collection.from_config(test_session, {'name': 'kept.com', 'kind': 'domain'}) + test_session.commit() + domains_config.dump_config() + assert domains_config.get_file().is_file() + domains_config.successful_import = False + + did = domain.id + requests = [ + ('post', '/api/collections', dict(name='new.com', kind='domain')), + ('put', f'/api/collections/{did}', dict(description='changed')), + ('post', f'/api/collections/{did}/tag', dict(tag_name='Tagged', directory='archive/Tagged/kept.com')), + ('delete', f'/api/collections/{did}', None), + ] + for method, url, body in requests: + kwargs = dict(content=json.dumps(body)) if body else {} + request, response = await getattr(async_client, method)(url, **kwargs) + assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE, (method, url, response.json) + assert response.json['code'] == 'CONFIG_NOT_IMPORTED' + + test_session.expire_all() + domain = test_session.query(Collection).one() + assert domain.name == 'kept.com' + assert domain.description is None + assert domain.tag is None + assert domain.directory is None + + # Once imported, domains can change again. + domains_config.import_config() + request, response = await async_client.put(f'/api/collections/{did}', content=json.dumps(dict(description='x'))) + assert response.status_code == HTTPStatus.OK, response.json + + +@pytest.mark.asyncio +async def test_import_keeps_domains_with_archives(test_session: Session, test_directory: pathlib.Path, async_client, + archive_factory): + """A domain missing from domains.yaml is deleted by the import, unless it has Archives. Archives create their + domain (e.g. an upload or refresh while WROLPi starts, before domains.yaml is imported), and deleting it would + delete its Archives.""" + from modules.archive.models import Archive + archive_factory(domain='kept.com', url='https://kept.com/1') + Collection.from_config(test_session, {'name': 'empty.com', 'kind': 'domain'}) + test_session.commit() + domains_config.dump_config() + domains_config.successful_import = False + + # Created before the import. + archive_factory(domain='new.com', url='https://new.com/1') + Collection.from_config(test_session, {'name': 'unused.com', 'kind': 'domain'}) + test_session.commit() + assert test_session.query(Archive).count() == 2 + + domains_config.import_config() + test_session.expire_all() + assert {i.name for i in test_session.query(Collection)} == {'kept.com', 'empty.com', 'new.com'} + assert {i.collection.name for i in test_session.query(Archive)} == {'kept.com', 'new.com'} + assert domains_config.successful_import + + # A domain with Archives which was removed from the config (by hand) is kept too. + config = yaml.safe_load(domains_config.get_file().read_text()) + config['collections'] = [i for i in config['collections'] if i['name'] != 'kept.com'] + domains_config.get_file().write_text(yaml.dump(config)) + domains_config.import_config() + test_session.expire_all() + assert {i.name for i in test_session.query(Collection)} == {'kept.com', 'empty.com', 'new.com'} + assert test_session.query(Archive).count() == 2 diff --git a/wrolpi/collections/test/test_playlists_config.py b/wrolpi/collections/test/test_playlists_config.py index 6f1f851c..5af1e74c 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -326,3 +326,45 @@ async def test_move_file_updates_playlists_config( await await_switches() await _assert_playlist_survives_reimport(test_session, playlists_config, 'docs/guide.pdf') + + +@pytest.mark.asyncio +async def test_playlist_changes_wait_for_config_import(test_session, test_directory, async_client, playlists_config): + """Until playlists.yaml is imported (e.g. while WROLPi starts), playlists cannot change; the import would undo + the change (a new playlist would be deleted, items rebuilt from the config).""" + import json + collection = _make_playlist(test_session, 'Kept') + collection.add_url(test_session, '/map?lat=3&lon=4&z=5', title='other') + test_session.commit() + item_ids = [i.id for i in collection.items] + playlists_config.dump_config() + assert playlists_config.get_file().is_file() + playlists_config.successful_import = False + + cid = collection.id + requests = [ + ('post', '/api/collections', dict(name='New')), + ('post', f'/api/collections/{cid}/items', dict(item_kind='url', url='/map?lat=5&lon=6&z=7')), + ('delete', f'/api/collections/{cid}/items/{item_ids[0]}', None), + ('put', f'/api/collections/{cid}/items/order', dict(item_ids=list(reversed(item_ids)))), + ('put', f'/api/collections/{cid}', dict(description='changed')), + ('post', f'/api/collections/{cid}/tag', dict(tag_name='Tagged')), + ('delete', f'/api/collections/{cid}', None), + ] + for method, url, body in requests: + kwargs = dict(content=json.dumps(body)) if body else {} + request, response = await getattr(async_client, method)(url, **kwargs) + assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE, (method, url, response.json) + assert response.json['code'] == 'CONFIG_NOT_IMPORTED' + + test_session.expire_all() + assert [i.name for i in test_session.query(Collection)] == ['Kept'] + collection = test_session.query(Collection).one() + assert [i.id for i in collection.items] == item_ids + assert collection.description is None + assert collection.tag is None + + # Once imported, playlists can change again. + playlists_config.import_config() + request, response = await async_client.post('/api/collections', content=json.dumps(dict(name='New'))) + assert response.status_code == HTTPStatus.CREATED, response.json diff --git a/wrolpi/common.py b/wrolpi/common.py index 42ba5acd..593fb15b 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -40,7 +40,7 @@ from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import Session from wrolpi.dates import now, from_timestamp -from wrolpi.errors import WROLModeEnabled, NativeOnly, LogLevelError, InvalidConfig, \ +from wrolpi.errors import WROLModeEnabled, NativeOnly, LogLevelError, InvalidConfig, ConfigNotImported, \ ValidationError from wrolpi.log_levels import TRACE_LEVEL from wrolpi.vars import PYTEST, DOCKERIZED, CONFIG_DIR, MEDIA_DIRECTORY, DEFAULT_HTTP_HEADERS @@ -608,6 +608,14 @@ class ConfigFile: todays_backup.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(config_file, todays_backup) + def check_imported(self): + """Raise ConfigNotImported unless what this config holds may be changed: the config was imported (or does not + exist yet). A change before the import (e.g. while WROLPi starts) could not be saved, and the import would + undo it.""" + if self.get_file().exists() and not self.successful_import: + raise ConfigNotImported(f'WROLPi is still starting ({self.file_name} has not been loaded), try again in' + f' a moment') + def import_backup(self, backup_date: str, mode: str, send_events: bool = False): """Import a backup file. Subclasses override this.""" raise NotImplementedError(f'{self.__class__.__name__} does not support backup import') diff --git a/wrolpi/errors.py b/wrolpi/errors.py index da7a6939..4c1ad124 100644 --- a/wrolpi/errors.py +++ b/wrolpi/errors.py @@ -159,6 +159,12 @@ class InvalidConfig(APIError): status_code = HTTPStatus.BAD_REQUEST +class ConfigNotImported(APIError): + code = 'CONFIG_NOT_IMPORTED' + summary = 'WROLPi is still starting, try again in a moment' + status_code = HTTPStatus.SERVICE_UNAVAILABLE + + class NoPrimaryFile(APIError): code = 'NO_PRIMARY_FILE' summary = 'Could not find a primary file in the provided group'