diff --git a/alembic/versions/2026_10_25_1200_strip_tag_names.py b/alembic/versions/2026_10_25_1200_strip_tag_names.py new file mode 100644 index 00000000..a076bbf8 --- /dev/null +++ b/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 diff --git a/wrolpi/tags.py b/wrolpi/tags.py index 2d38c5c9..0c51c2e2 100644 --- a/wrolpi/tags.py +++ b/wrolpi/tags.py @@ -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') @@ -266,6 +268,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 diff --git a/wrolpi/test/test_db_bootstrap.py b/wrolpi/test/test_db_bootstrap.py index 4eb18b07..4ac0def2 100644 --- a/wrolpi/test/test_db_bootstrap.py +++ b/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.""" diff --git a/wrolpi/test/test_tags.py b/wrolpi/test/test_tags.py index 2ca7d715..41d400fe 100644 --- a/wrolpi/test/test_tags.py +++ b/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,