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'