From d51127841d23d4dd18566989644ee7e30cab844b Mon Sep 17 00:00:00 2001 From: cproudlock Date: Mon, 24 Aug 2026 14:49:24 -0400 Subject: [PATCH] Merge catalog rows that are the same name typed two ways The legacy data carried space-stripped twins: 'DellInc.' beside 'Dell Inc.', 'Windows10' beside 'Windows 10', 'OptiPlex7080' beside 'OptiPlex 7080'. They rode into the shipped seed catalog, so every site that ran `seed catalog` inherited them, and they stay invisible until somebody opens a vendor picker and finds the same company listed twice. Not a code fault. The collector strips only leading and trailing whitespace, matches with ilike, and treats the OS list as lookup-only, so it can neither have created these nor be creating more. They arrived with the classic-ASP import, which holds the same three pairs today. `flask catalog dedupe` merges only whitespace-and-punctuation twins, because those are one name typed two ways. 'Dell' and 'Dell Inc.' normalise apart and are left alone: that is a naming choice and only a person can make it. The surviving row is the most referenced, tie-broken toward the spaced spelling, which is the one somebody pressed the space bar for. `flask catalog merge` makes that judgment call explicitly, and refuses to invent a target that does not exist, so a typo cannot move rows into a new vendor. `flask catalog rename` handles the case neither can reach: a stripped name with NO twin, where there is nothing to merge into. All three report and change nothing unless --apply is passed, and dedupe is idempotent so it is safe in a deploy script. References are discovered from the LIVE schema rather than listed in code. There are 15 foreign keys into these three tables across 7 tables, and machines.controllervendorid and printerdrivers.modelnumberid are not ones anybody would enumerate by hand. Every reference is repointed before the row is deleted, so nothing is orphaned, and a plugin adding a table is covered without editing this command. --- shopdb/__init__.py | 4 +- shopdb/cli/__init__.py | 300 +++++++++++++++++++++++++ tests/test_core/test_catalog_dedupe.py | 239 ++++++++++++++++++++ 3 files changed, 542 insertions(+), 1 deletion(-) create mode 100644 tests/test_core/test_catalog_dedupe.py diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 3e7ee884..69883c03 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -243,13 +243,15 @@ def register_blueprints(app: Flask): def register_cli_commands(app: Flask): """Register Flask CLI commands.""" from .plugins.cli import plugin_cli - from .cli import db_cli, seed_cli, relationships_cli, csv_cli + from .cli import (db_cli, seed_cli, relationships_cli, csv_cli, + catalog_cli) app.cli.add_command(plugin_cli) app.cli.add_command(db_cli) app.cli.add_command(seed_cli) app.cli.add_command(relationships_cli) app.cli.add_command(csv_cli) + app.cli.add_command(catalog_cli) def register_error_handlers(app: Flask): diff --git a/shopdb/cli/__init__.py b/shopdb/cli/__init__.py index 05f40cb3..460c5561 100644 --- a/shopdb/cli/__init__.py +++ b/shopdb/cli/__init__.py @@ -1535,3 +1535,303 @@ def audit_relationships(): 'knowing which side is authoritative.') else: click.echo(click.style('No relationship faults found.', fg='green')) + + +# --- catalog hygiene -------------------------------------------------------- + +@click.group('catalog') +def catalog_cli(): + """Clean up the shared vendor/model/OS catalog.""" + pass + + +# Catalog tables that carry a human-typed name, with the column that names them. +# Each is free vocabulary somebody can add to, which is how twins appear. +_CATALOG_TABLES = ( + ('vendors', 'vendor'), + ('models', 'modelnumber'), + ('operatingsystems', 'osname'), +) + + +def _catalog_norm(value): + """Collapse a name to what it would be with no spacing or punctuation. + + Only whitespace and punctuation are removed, and case is folded. That is + deliberately narrow: 'Dell Inc.' and 'DellInc.' are the SAME name typed two + ways, while 'Dell' and 'Dell Inc.' are a naming CHOICE and must never be + merged automatically. Use `catalog merge` for that. + """ + import re + return re.sub(r'[\s\-_.,/]+', '', str(value or '').strip().lower()) + + +def _inbound_foreign_keys(inspector, table): + """Every (table, column) in the schema that points at `table`'s key. + + Discovered from the live schema rather than listed here, because a plugin + can add one: `machines.controllervendorid` and `printerdrivers. + modelnumberid` both reference the catalog and neither is obvious. + """ + refs = [] + for candidate in inspector.get_table_names(): + for fk in inspector.get_foreign_keys(candidate): + if fk['referred_table'] == table: + refs.append((candidate, + fk['constrained_columns'][0], + fk['referred_columns'][0])) + return refs + + +def _reference_counts(connection, refs, keyvalue): + """How many rows across the schema point at one catalog row.""" + import sqlalchemy as sa + total = 0 + detail = [] + for tablename, column, _ in refs: + count = connection.execute( + sa.text(f'SELECT COUNT(*) FROM {tablename} WHERE {column} = :k'), + {'k': keyvalue}).scalar() + if count: + detail.append(f'{tablename}.{column}={count}') + total += count + return total, detail + + +def _merge_catalog_rows(connection, table, keycolumn, refs, loser, winner): + """Repoint every reference off `loser` onto `winner`, then delete it.""" + import sqlalchemy as sa + moved = 0 + for tablename, column, _ in refs: + result = connection.execute( + sa.text(f'UPDATE {tablename} SET {column} = :w WHERE {column} = :l'), + {'w': winner, 'l': loser}) + moved += result.rowcount or 0 + connection.execute( + sa.text(f'DELETE FROM {table} WHERE {keycolumn} = :l'), {'l': loser}) + return moved + + +@catalog_cli.command('dedupe') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the changes. Without this the command only reports.') +@with_appcontext +def catalog_dedupe(apply_changes): + """Merge catalog rows that are the same name typed two ways. + + Legacy data arrived carrying space-stripped twins - 'DellInc.' beside + 'Dell Inc.', 'Windows10' beside 'Windows 10' - which then rode into the + shipped seed catalog and out to every site that ran `seed catalog`. They + are invisible until somebody opens a vendor picker and finds the same + company twice. + + ONLY whitespace-and-punctuation twins are touched. 'Dell' and 'Dell Inc.' + are a naming choice, not a typo, and are left alone; use `catalog merge` + to make that call explicitly. + + The surviving row is the one with the most references, tie-broken toward + the spaced spelling, because that is the one a person typed deliberately. + Every foreign key pointing at the loser is repointed first, so nothing is + orphaned; references are counted across the LIVE schema, which picks up + plugin tables such as machines.controllervendorid. + + Reports only unless --apply is passed. Idempotent. + """ + import sqlalchemy as sa + from collections import defaultdict + from shopdb.extensions import db + + engine = db.session.get_bind() + inspector = sa.inspect(engine) + existing = set(inspector.get_table_names()) + + planned = [] + with engine.begin() as connection: + for table, namecolumn in _CATALOG_TABLES: + if table not in existing: + continue + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + refs = [r for r in _inbound_foreign_keys(inspector, table) + if r[0] in existing] + + rows = connection.execute(sa.text( + f'SELECT {keycolumn}, {namecolumn} FROM {table}')).fetchall() + groups = defaultdict(list) + for key, name in rows: + if name: + groups[_catalog_norm(name)].append((key, name)) + + for _, members in sorted(groups.items()): + if len(members) < 2: + continue + scored = [] + for key, name in members: + count, detail = _reference_counts(connection, refs, key) + # most-referenced wins; then the spelling with whitespace, + # which is the one somebody typed on purpose; then longest. + scored.append((count, ' ' in str(name), len(str(name)), + key, name, detail)) + scored.sort(reverse=True) + winner = scored[0] + for loser in scored[1:]: + planned.append({ + 'table': table, 'keycolumn': keycolumn, 'refs': refs, + 'winner_key': winner[3], 'winner_name': winner[4], + 'loser_key': loser[3], 'loser_name': loser[4], + 'loser_refs': loser[0], 'loser_detail': loser[5], + }) + + if not planned: + click.echo(click.style('No catalog twins found.', fg='green')) + return + + click.echo(f'{len(planned)} duplicate row(s):') + click.echo('') + for item in planned: + where = (', '.join(item['loser_detail']) if item['loser_detail'] + else 'nothing references it') + click.echo(f" {item['table']}: {item['loser_name']!r} " + f"-> {item['winner_name']!r}") + click.echo(f" {item['loser_refs']} reference(s): {where}") + + if not apply_changes: + click.echo('') + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + moved = 0 + for item in planned: + moved += _merge_catalog_rows( + connection, item['table'], item['keycolumn'], item['refs'], + item['loser_key'], item['winner_key']) + + click.echo('') + click.echo(click.style( + f'Merged {len(planned)} row(s), repointed {moved} reference(s).', + fg='green')) + + +@catalog_cli.command('merge') +@click.argument('table', type=click.Choice([t for t, _ in _CATALOG_TABLES])) +@click.argument('source') +@click.argument('target') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the changes. Without this the command only reports.') +@with_appcontext +def catalog_merge(table, source, target, apply_changes): + """Merge one catalog row into another by name: SOURCE is absorbed by TARGET. + + For the judgment calls `catalog dedupe` deliberately refuses to make - + 'Dell' and 'Dell Inc.' are different names, and only a person knows the + site means one company. + + flask catalog merge vendors "Dell" "Dell Inc." --apply + + Every reference to SOURCE is repointed to TARGET, then SOURCE is deleted. + Reports only unless --apply is passed. + """ + import sqlalchemy as sa + from shopdb.extensions import db + + namecolumn = dict(_CATALOG_TABLES)[table] + engine = db.session.get_bind() + inspector = sa.inspect(engine) + existing = set(inspector.get_table_names()) + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + refs = [r for r in _inbound_foreign_keys(inspector, table) if r[0] in existing] + + with engine.begin() as connection: + def lookup(value): + return connection.execute( + sa.text(f'SELECT {keycolumn} FROM {table} ' + f'WHERE {namecolumn} = :v'), {'v': value}).fetchone() + + sourcerow, targetrow = lookup(source), lookup(target) + if not sourcerow: + click.echo(click.style(f'No {table} row named {source!r}.', fg='yellow')) + return + if not targetrow: + click.echo(click.style(f'No {table} row named {target!r}. ' + f'Refusing to invent it.', fg='red')) + raise SystemExit(1) + if sourcerow[0] == targetrow[0]: + click.echo(click.style('Source and target are the same row.', fg='yellow')) + return + + count, detail = _reference_counts(connection, refs, sourcerow[0]) + where = ', '.join(detail) if detail else 'nothing references it' + click.echo(f'{table}: {source!r} -> {target!r}') + click.echo(f' {count} reference(s): {where}') + + if not apply_changes: + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + moved = _merge_catalog_rows(connection, table, keycolumn, refs, + sourcerow[0], targetrow[0]) + + click.echo(click.style( + f'Merged {source!r} into {target!r}, repointed {moved} reference(s).', + fg='green')) + + +@catalog_cli.command('rename') +@click.argument('table', type=click.Choice([t for t, _ in _CATALOG_TABLES])) +@click.argument('source') +@click.argument('target') +@click.option('--apply', 'apply_changes', is_flag=True, + help='Write the change. Without this the command only reports.') +@with_appcontext +def catalog_rename(table, source, target, apply_changes): + """Rename one catalog row in place, keeping every reference to it. + + A space-stripped name with NO twin cannot be deduped - there is nothing to + merge it into - so 'OptiPlex5000' would survive a `catalog dedupe` that + correctly removed 'OptiPlex7080'. This fixes that case. + + flask catalog rename models "OptiPlex5000" "OptiPlex 5000" --apply + + Refuses when the target name already exists, because that is a merge and + has to move references rather than rename a row: use `catalog merge`. + """ + import sqlalchemy as sa + from shopdb.extensions import db + + namecolumn = dict(_CATALOG_TABLES)[table] + engine = db.session.get_bind() + inspector = sa.inspect(engine) + keycolumn = inspector.get_pk_constraint(table)['constrained_columns'][0] + + with engine.begin() as connection: + def lookup(value): + return connection.execute( + sa.text(f'SELECT {keycolumn} FROM {table} ' + f'WHERE {namecolumn} = :v'), {'v': value}).fetchone() + + sourcerow = lookup(source) + if not sourcerow: + click.echo(click.style(f'No {table} row named {source!r}.', fg='yellow')) + return + if lookup(target): + click.echo(click.style( + f'{table} already has a row named {target!r}. That is a merge, ' + f'not a rename - use `catalog merge`.', fg='red')) + raise SystemExit(1) + + click.echo(f'{table}: rename {source!r} -> {target!r} ' + f'(id {sourcerow[0]}, references unaffected)') + if not apply_changes: + click.echo(click.style( + 'Dry run. Nothing was changed. Re-run with --apply to write.', + fg='yellow')) + return + + connection.execute( + sa.text(f'UPDATE {table} SET {namecolumn} = :t WHERE {keycolumn} = :k'), + {'t': target, 'k': sourcerow[0]}) + + click.echo(click.style(f'Renamed {source!r} to {target!r}.', fg='green')) diff --git a/tests/test_core/test_catalog_dedupe.py b/tests/test_core/test_catalog_dedupe.py new file mode 100644 index 00000000..1dc1011e --- /dev/null +++ b/tests/test_core/test_catalog_dedupe.py @@ -0,0 +1,239 @@ +"""Catalog hygiene: merging vendor/model/OS rows that are the same name twice. + +Legacy data arrived carrying space-stripped twins - 'DellInc.' beside +'Dell Inc.', 'Windows10' beside 'Windows 10'. They rode into the shipped seed +catalog and out to every site that ran `seed catalog`, and stay invisible until +somebody opens a vendor picker and finds the same company listed twice. + +The distinction these tests exist to protect: a twin is the same name TYPED two +ways and may be merged mechanically, while 'Dell' and 'Dell Inc.' are two +different names and merging them is a judgment only a person can make. +""" + +import pytest + +from shopdb.extensions import db +from shopdb.core.models import Vendor, Model +from shopdb.cli import _catalog_norm + + +def _run(app, args): + return app.test_cli_runner().invoke(args=args) + + +# --- the normaliser draws the line ------------------------------------------ + +def test_whitespace_and_punctuation_twins_normalise_together(): + assert _catalog_norm('Dell Inc.') == _catalog_norm('DellInc.') + assert _catalog_norm('Windows 10') == _catalog_norm('Windows10') + assert _catalog_norm('OptiPlex 7080') == _catalog_norm('OptiPlex7080') + + +def test_a_different_name_does_not_normalise_together(): + """'Dell' is not a typo of 'Dell Inc.', it is a shorter name. + + If these collapsed, `dedupe` would silently pick one and delete a name the + site may have chosen deliberately. + """ + assert _catalog_norm('Dell') != _catalog_norm('Dell Inc.') + assert _catalog_norm('Precision 5560') != _catalog_norm('Precision 5570') + + +# --- dedupe ------------------------------------------------------------------ + +@pytest.fixture +def dell_twins(db): + """The real shape found in production: a twin, plus a genuine third name.""" + canonical = Vendor(vendor='Dell Inc.') + twin = Vendor(vendor='DellInc.') + shortname = Vendor(vendor='Dell') + db.session.add_all([canonical, twin, shortname]) + db.session.flush() + # the twin owns a model, so the merge has a reference to repoint + db.session.add(Model(modelnumber='OptiPlex7080', vendorid=twin.vendorid)) + db.session.add(Model(modelnumber='OptiPlex 7080', vendorid=canonical.vendorid)) + db.session.commit() + return {'canonical': canonical.vendorid, 'twin': twin.vendorid, + 'shortname': shortname.vendorid} + + +def test_dedupe_reports_without_changing_anything(app, dell_twins): + """The default is a dry run: a cleanup that writes by surprise is a trap.""" + result = _run(app, ['catalog', 'dedupe']) + + assert result.exit_code == 0 + assert 'DellInc.' in result.output + assert 'Dry run' in result.output + # every row still stands + assert db.session.get(Vendor, dell_twins['twin']) is not None + assert Vendor.query.filter_by(vendor='DellInc.').count() == 1 + + +def test_dedupe_merges_the_twin_and_repoints_its_model(app, dell_twins): + """The twin disappears and its model moves, rather than being orphaned.""" + result = _run(app, ['catalog', 'dedupe', '--apply']) + + assert result.exit_code == 0 + assert Vendor.query.filter_by(vendor='DellInc.').count() == 0 + assert Vendor.query.filter_by(vendor='Dell Inc.').count() == 1 + + # nothing points at a vendor that no longer exists + vendorids = {v.vendorid for v in Vendor.query.all()} + for model in Model.query.all(): + assert model.vendorid is None or model.vendorid in vendorids + + +def test_dedupe_leaves_a_genuinely_different_name_alone(app, dell_twins): + """'Dell' survives. It is a naming choice, not a typo.""" + _run(app, ['catalog', 'dedupe', '--apply']) + + assert Vendor.query.filter_by(vendor='Dell').count() == 1 + + +def test_dedupe_keeps_the_spelling_a_person_typed(app, db): + """With no references either way, the spaced spelling wins. + + Whitespace is evidence of intent: somebody pressed the space bar. + """ + db.session.add_all([Vendor(vendor='AcmeCorp'), Vendor(vendor='Acme Corp')]) + db.session.commit() + + _run(app, ['catalog', 'dedupe', '--apply']) + + assert Vendor.query.filter_by(vendor='Acme Corp').count() == 1 + assert Vendor.query.filter_by(vendor='AcmeCorp').count() == 0 + + +def test_dedupe_keeps_the_most_referenced_row(app, db): + """References outrank spelling: the row in use is the one that survives.""" + bare = Vendor(vendor='ZZTop') + spaced = Vendor(vendor='ZZ Top') + db.session.add_all([bare, spaced]) + db.session.flush() + for i in range(3): + db.session.add(Model(modelnumber=f'ZZ-{i}', vendorid=bare.vendorid)) + db.session.commit() + + _run(app, ['catalog', 'dedupe', '--apply']) + + assert Vendor.query.filter_by(vendor='ZZTop').count() == 1 + assert Vendor.query.filter_by(vendor='ZZ Top').count() == 0 + + +def test_dedupe_is_idempotent(app, dell_twins): + """Running it twice is a no-op, so it is safe in a deploy script.""" + _run(app, ['catalog', 'dedupe', '--apply']) + second = _run(app, ['catalog', 'dedupe', '--apply']) + + assert second.exit_code == 0 + assert 'No catalog twins found' in second.output + + +def test_dedupe_on_a_clean_catalog_says_so(app, db): + db.session.add(Vendor(vendor='Solo Vendor')) + db.session.commit() + + result = _run(app, ['catalog', 'dedupe']) + + assert result.exit_code == 0 + assert 'No catalog twins found' in result.output + + +# --- explicit merge ---------------------------------------------------------- + +def test_merge_absorbs_a_deliberately_different_name(app, dell_twins): + """The judgment call dedupe refuses: 'Dell' means 'Dell Inc.' at this site.""" + result = _run(app, ['catalog', 'merge', 'vendors', 'Dell', 'Dell Inc.', + '--apply']) + + assert result.exit_code == 0 + assert Vendor.query.filter_by(vendor='Dell').count() == 0 + assert Vendor.query.filter_by(vendor='Dell Inc.').count() == 1 + + +def test_merge_dry_runs_by_default(app, dell_twins): + result = _run(app, ['catalog', 'merge', 'vendors', 'Dell', 'Dell Inc.']) + + assert 'Dry run' in result.output + assert Vendor.query.filter_by(vendor='Dell').count() == 1 + + +def test_merge_refuses_to_invent_the_target(app, dell_twins): + """A typo in the target must not create a new vendor and move rows into it.""" + result = _run(app, ['catalog', 'merge', 'vendors', 'Dell', 'Dell Incorporated', + '--apply']) + + assert result.exit_code == 1 + assert 'Refusing to invent' in result.output + assert Vendor.query.filter_by(vendor='Dell').count() == 1 + + +def test_merge_reports_a_missing_source_without_failing(app, dell_twins): + """Already merged is not an error: the deploy script can just run again.""" + result = _run(app, ['catalog', 'merge', 'vendors', 'Nonexistent', + 'Dell Inc.', '--apply']) + + assert result.exit_code == 0 + assert 'No vendors row named' in result.output + + +# --- rename ------------------------------------------------------------------ + +def test_rename_fixes_a_stripped_name_with_no_twin(app, db): + """'OptiPlex5000' has nothing to merge into, so dedupe cannot reach it.""" + vendor = Vendor(vendor='Dell Inc.') + db.session.add(vendor) + db.session.flush() + db.session.add(Model(modelnumber='OptiPlex5000', vendorid=vendor.vendorid)) + db.session.commit() + + _run(app, ['catalog', 'dedupe', '--apply']) + assert Model.query.filter_by(modelnumber='OptiPlex5000').count() == 1, \ + 'dedupe should not touch a name with no twin' + + result = _run(app, ['catalog', 'rename', 'models', 'OptiPlex5000', + 'OptiPlex 5000', '--apply']) + + assert result.exit_code == 0 + assert Model.query.filter_by(modelnumber='OptiPlex 5000').count() == 1 + assert Model.query.filter_by(modelnumber='OptiPlex5000').count() == 0 + + +def test_rename_keeps_the_row_and_its_references(app, db): + """A rename is in place: the id does not change, so nothing is repointed.""" + vendor = Vendor(vendor='Acme') + db.session.add(vendor) + db.session.flush() + model = Model(modelnumber='Widget100', vendorid=vendor.vendorid) + db.session.add(model) + db.session.commit() + original = model.modelnumberid + + _run(app, ['catalog', 'rename', 'models', 'Widget100', 'Widget 100', '--apply']) + + renamed = Model.query.filter_by(modelnumber='Widget 100').one() + assert renamed.modelnumberid == original + assert renamed.vendorid == vendor.vendorid + + +def test_rename_refuses_when_the_target_already_exists(app, db): + """Renaming onto an existing name would need a merge, not an UPDATE.""" + db.session.add_all([Vendor(vendor='Acme'), Vendor(vendor='Acme Corp')]) + db.session.commit() + + result = _run(app, ['catalog', 'rename', 'vendors', 'Acme', 'Acme Corp', + '--apply']) + + assert result.exit_code == 1 + assert 'use `catalog merge`' in result.output + assert Vendor.query.filter_by(vendor='Acme').count() == 1 + + +def test_rename_dry_runs_by_default(app, db): + db.session.add(Vendor(vendor='Acme')) + db.session.commit() + + result = _run(app, ['catalog', 'rename', 'vendors', 'Acme', 'Acme Corp']) + + assert 'Dry run' in result.output + assert Vendor.query.filter_by(vendor='Acme').count() == 1