mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-25 18:40:06 -04:00
Strip whitespace from Tag names (#876)
* Tag names have leading and trailing whitespace stripped A padded name now finds the existing Tag instead of creating a near-duplicate, and a whitespace-only name is rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Migration strips whitespace from Tag names and merges collisions Tags whose names match once stripped are merged (tagged files, Zim entries, Collections); the first color by id is kept. Downloads and the configs that name Tags are rewritten too, so the next import is in step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * tags.yaml keeps the database's color for merged Tags The file was merged in sorted key order, not by Tag id, and the next import copies the file's color over the database's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
c3e25b84d6
commit
0dd61b8e64
4 changed files with 280 additions and 1 deletions
175
alembic/versions/2026_10_25_1200_strip_tag_names.py
Normal file
175
alembic/versions/2026_10_25_1200_strip_tag_names.py
Normal file
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue