diff --git a/modules/archive/lib.py b/modules/archive/lib.py index e2b6fe9f..78adffed 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -64,8 +64,9 @@ class DomainsSection(ConfigSection): 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 []): - if not (name := data.get('name')): - logger.error(f'Domain collection at index {position} has no name, skipping') + name = data.get('name') if isinstance(data, dict) else None + if not name: + logger.error(f'Domain collection at index {position} has no name, skipping: {data!r}') # Counted as skipped; never a domain's name. invalid.add(f'\0{position}') continue @@ -169,8 +170,22 @@ class DomainsConfig(ConfigFile): 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 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: @@ -184,8 +199,7 @@ class DomainsConfig(ConfigFile): super().import_config(file, send_events) def after_import(self, diffs: dict): - # A removed domain with Archives was reset, not deleted; the config lists it again. - if self._config_outdated or diffs['domains'].removed: + if self._config_outdated: self.background_dump.activate_switch() def dump_config(self, file: pathlib.Path = None, send_events=False, overwrite=False): @@ -301,6 +315,11 @@ def import_domains_config(): 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 with get_db_session() as session: links = domain_download_links(session) diff --git a/modules/archive/test/test_domains_config_import.py b/modules/archive/test/test_domains_config_import.py index a6f93c70..2bc2fc98 100644 --- a/modules/archive/test/test_domains_config_import.py +++ b/modules/archive/test/test_domains_config_import.py @@ -157,3 +157,77 @@ async def test_downloads_are_linked(test_session, saved_domains, await_switches) } 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' diff --git a/wrolpi/common.py b/wrolpi/common.py index 0a09f368..0d366c86 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -850,8 +850,15 @@ class ConfigFile: # `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): @@ -892,7 +899,7 @@ class ConfigFile: """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`. `import_deletes`: whether - this import deletes what the file no longer lists. + 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