diff --git a/shopdb/cli/__init__.py b/shopdb/cli/__init__.py index c53d092..01b652e 100644 --- a/shopdb/cli/__init__.py +++ b/shopdb/cli/__init__.py @@ -680,14 +680,18 @@ def propagate_relationships(): @relationships_cli.command('fix-controls-direction') @with_appcontext def fix_controls_direction(): - """Flip reversed legacy controls rows to PC -> machine. + """Flip reversed legacy controls rows so the PC is the source. - Legacy import stores controls as machine -> PC, which reads as the machine + Legacy import stores controls as device -> PC, which reads as the device having authority over the PC. In reality the PC is the controller (it - sends programs to the machine and receives logs), so per ADR-001 the PC - must be the source. Flips every active controls row whose source is a - machine asset and target is a computer asset. If the flipped row already - exists, the reversed duplicate is deactivated instead. Idempotent. + sends programs to the device and receives logs), so per ADR-001 the PC + must be the source. Flips every active controls row whose target is a + computer and whose source is not. If the flipped row already exists, the + reversed duplicate is deactivated instead. Idempotent. + + Covers machines, measuring tools, printers and network devices. It used to + match machines ONLY, so a CMM PC kept showing "<- controls from CMM4" + alongside its own outgoing link and no amount of running this fixed it. """ from sqlalchemy.orm import aliased from shopdb.extensions import db @@ -713,7 +717,13 @@ def fix_controls_direction(): .filter( AssetRelationship.relationshiptypeid == controls.relationshiptypeid, AssetRelationship.isactive == True, - sourcetype.assettype == 'machine', + # ANY non-computer controlled BY a computer, not just machines. The + # original filter said assettype == 'machine', which left every + # measuring_tool -> computer row untouched - a CMM PC showing + # "<- controls from CMM4" beside its own outgoing link, which is + # exactly the shape this command exists to clean. Printers and + # network devices reach shopdb the same way and had the same gap. + sourcetype.assettype != 'computer', targettype.assettype == 'computer', ) .all() @@ -1302,3 +1312,195 @@ def check_shared_machines(): 'device becomes its own asset.') else: click.echo(click.style('Every shared number has child assets.', fg='green')) + + +@relationships_cli.command('audit') +@with_appcontext +def audit_relationships(): + """Report relationship rows that cannot all be true. Read-only. + + Three faults share one symptom - an asset page listing the same peer more + than once - and they need different fixes, so this names which is which. + + 1. RECIPROCAL PAIRS. Both `A controls B` and `B controls A` exist. Only one + can be true: a PC drives a machine, never the reverse. The create path + only ever checked (source, target, type), so the inverse inserted + cleanly, and the Add Relationship dialog offers an `incoming` direction + that writes exactly that. The legacy import stores controls the wrong way + round as well. `flask relationships fix-controls-direction` cleans the + machine->computer ones; anything else is listed here for a decision. + + 2. SELF-LINKS. An asset pointing at ITSELF, which renders as a duplicate on + its own page and is never meaningful. + + 3. DUPLICATE DEVICES. One PC controlling several assets of the same type - + three measuring tools for one physical CMM, say. The collector keys its + idempotency on its own label, so a device somebody created by hand, or + the legacy import created, is invisible to it and it mints another one on + every fresh PC. + + The `label` on each row usually names the writer outright, so it is printed: + `collector:*` means this code made it, anything else means a person or the + import did. Nothing is modified - decide from the output which side of a + pair is authoritative before deleting anything. + """ + from shopdb.extensions import db + from shopdb.core.models import Asset, AssetType + from shopdb.core.models.relationship import AssetRelationship, RelationshipType + from sqlalchemy.orm import aliased + from collections import defaultdict + + faults = 0 + + def assetlabel(asset): + if not asset: + return '?' + return f'{asset.assetnumber or asset.name or asset.assetid}' + + # ---- 1. reciprocal pairs (directional types only) ---------------------- + # Symmetric types store both directions ON PURPOSE, so they are excluded - + # flagging them would bury the real faults in noise. + directional = {t.relationshiptypeid: t.relationshiptype + for t in RelationshipType.query.all() if t.isdirectional} + rows = (AssetRelationship.query + .filter(AssetRelationship.isactive.is_(True), + AssetRelationship.relationshiptypeid.in_(directional or [0])) + .all()) if directional else [] + + bykey = {} + for rel in rows: + bykey[(rel.sourceassetid, rel.targetassetid, rel.relationshiptypeid)] = rel + + seen = set() + reciprocal = [] + for (source, target, typeid), rel in bykey.items(): + # A self-link is its own inverse. It is a fault, but a DIFFERENT one, + # reported below - counting it here would double-report it and print + # the same row twice as though it were a pair. + if source == target: + continue + inverse = bykey.get((target, source, typeid)) + if inverse and (target, source, typeid) not in seen: + seen.add((source, target, typeid)) + reciprocal.append((rel, inverse)) + + click.echo(click.style('\n== Reciprocal pairs (both directions stored) ==', + bold=True)) + if reciprocal: + faults += len(reciprocal) + for rel, inverse in reciprocal: + typename = directional.get(rel.relationshiptypeid, '?') + a = db.session.get(Asset, rel.sourceassetid) + b = db.session.get(Asset, rel.targetassetid) + click.echo(f' {assetlabel(a)} -{typename}-> {assetlabel(b)} ' + f'[id {rel.relationshipid}, label={rel.label or "-"}]') + click.echo(f' {assetlabel(b)} -{typename}-> {assetlabel(a)} ' + f'[id {inverse.relationshipid}, label={inverse.label or "-"}]') + click.echo('') + click.echo(click.style( + f' {len(reciprocal)} pair(s). Only one direction can be true.', + fg='yellow')) + else: + click.echo(click.style(' none', fg='green')) + + # ---- 2. self-links ------------------------------------------------------ + selflinks = (AssetRelationship.query + .filter(AssetRelationship.isactive.is_(True), + AssetRelationship.sourceassetid + == AssetRelationship.targetassetid) + .all()) + click.echo(click.style('\n== Self-links (asset pointing at itself) ==', + bold=True)) + if selflinks: + faults += len(selflinks) + for rel in selflinks: + asset = db.session.get(Asset, rel.sourceassetid) + typename = (rel.relationshiptype.relationshiptype + if rel.relationshiptype else '?') + click.echo(f' {assetlabel(asset)} -{typename}-> itself ' + f'[id {rel.relationshipid}, label={rel.label or "-"}]') + click.echo(click.style(f' {len(selflinks)} row(s). Never meaningful.', + fg='yellow')) + else: + click.echo(click.style(' none', fg='green')) + + # ---- 3. duplicate devices per PC --------------------------------------- + controls = RelationshipType.query.filter_by(relationshiptype='controls').first() + click.echo(click.style( + '\n== PCs controlling several assets of the SAME type ==', bold=True)) + if not controls: + click.echo(click.style(' no controls type; run flask seed reference-data', + fg='yellow')) + else: + sourceasset = aliased(Asset) + targetasset = aliased(Asset) + targettype = aliased(AssetType) + pairs = (db.session.query(AssetRelationship, sourceasset, + targetasset, targettype.assettype) + .select_from(AssetRelationship) + .join(sourceasset, + AssetRelationship.sourceassetid == sourceasset.assetid) + .join(targetasset, + AssetRelationship.targetassetid == targetasset.assetid) + .join(targettype, + targetasset.assettypeid == targettype.assettypeid) + .filter(AssetRelationship.relationshiptypeid + == controls.relationshiptypeid, + AssetRelationship.isactive.is_(True)) + .all()) + + # Devices that are SYMMETRIC partners of each other (Dualpath) are one + # physical machine with one controller, and `controls` is propagated to + # both bays on purpose - see propagate_relationship. Reporting those as + # duplicates buries the real faults: on this dev database they are the + # overwhelming majority, consecutive bay numbers pair by pair. + siblings = set() + symmetric = [t.relationshiptypeid for t in RelationshipType.query.all() + if not t.isdirectional] + if symmetric: + for rel in (AssetRelationship.query + .filter(AssetRelationship.isactive.is_(True), + AssetRelationship.relationshiptypeid.in_(symmetric)) + .all()): + siblings.add((rel.sourceassetid, rel.targetassetid)) + siblings.add((rel.targetassetid, rel.sourceassetid)) + + def all_siblings(deviceids): + return all((a, b) in siblings + for index, a in enumerate(deviceids) + for b in deviceids[index + 1:]) + + grouped = defaultdict(list) + for rel, pcasset, device, devicetype in pairs: + grouped[(pcasset.assetid, devicetype)].append((rel, pcasset, device)) + + dupes = {key: value for key, value in grouped.items() + if len(value) > 1 + and not all_siblings([item[2].assetid for item in value])} + if dupes: + faults += len(dupes) + for (pcassetid, devicetype), items in sorted( + dupes.items(), key=lambda kv: -len(kv[1])): + pcasset = items[0][1] + click.echo(f' {assetlabel(pcasset)} controls ' + f'{len(items)} x {devicetype}:') + for rel, _, device in items: + click.echo(f' {assetlabel(device)}' + f' (name={device.name or "-"})' + f' [id {rel.relationshipid}, ' + f'label={rel.label or "-"}]') + click.echo(click.style( + f' {len(dupes)} PC/type group(s). A label of "-" or a ' + 'non-collector value means the collector cannot see that row ' + 'and will keep minting its own.', fg='yellow')) + else: + click.echo(click.style(' none', fg='green')) + + click.echo('') + if faults: + click.echo(click.style(f'{faults} fault group(s) found. Nothing was ' + 'changed.', fg='yellow')) + click.echo('Do NOT bulk-delete one side of a reciprocal pair before ' + 'knowing which side is authoritative.') + else: + click.echo(click.style('No relationship faults found.', fg='green')) diff --git a/shopdb/core/api/assets.py b/shopdb/core/api/assets.py index 4081eff..9e07966 100644 --- a/shopdb/core/api/assets.py +++ b/shopdb/core/api/assets.py @@ -610,6 +610,12 @@ def _add_propagated(sourceid, targetid, rel, created): return if _relationship_exists(sourceid, targetid, rel.relationshiptypeid): return + # Never fan out INTO an existing inverse. The same one-way rule the create + # path enforces: if target-T->source is already stored, adding source-T-> + # target would manufacture a reciprocal pair that no user asked for, on a + # rail that is meant to spread one direction across sibling bays. + if _relationship_exists(targetid, sourceid, rel.relationshiptypeid): + return newrel = AssetRelationship( sourceassetid=sourceid, targetassetid=targetid, @@ -754,6 +760,14 @@ def create_asset_relationship(): if not db.session.get(RelationshipType, type_id): return error_response(ErrorCodes.NOT_FOUND, f'Relationship type {type_id} not found', http_code=404) + # An asset cannot relate to itself. It renders as a duplicate row on that + # asset's own page (the card lists both ends) and means nothing. + if source_id == target_id: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'An asset cannot be related to itself' + ) + # Check for duplicate relationship existing = AssetRelationship.query.filter_by( sourceassetid=source_id, @@ -768,6 +782,32 @@ def create_asset_relationship(): http_code=409 ) + # And it cannot relate BOTH WAYS on a directional type. Only one direction + # can be true - a PC drives a machine, never the reverse - but the check + # above is keyed on (source, target, type), so the inverse used to insert + # cleanly and both rows then rendered on both asset pages. The dialog offers + # an `incoming` direction, so this is reachable by hand, and the legacy + # import stores controls the wrong way round. + # + # Symmetric types are exempt: they store both directions ON PURPOSE and the + # card collapses them to one entry per peer. + reltype = db.session.get(RelationshipType, type_id) + if reltype is not None and reltype.isdirectional: + inverse = AssetRelationship.query.filter_by( + sourceassetid=target_id, + targetassetid=source_id, + relationshiptypeid=type_id + ).first() + if inverse is not None: + return error_response( + ErrorCodes.CONFLICT, + f"The reverse of this relationship already exists " + f"(relationship {inverse.relationshipid}). A " + f"'{reltype.relationshiptype}' link points one way only - " + f"remove the existing one first if the direction is wrong.", + http_code=409 + ) + rel = AssetRelationship( sourceassetid=source_id, targetassetid=target_id, diff --git a/tests/test_core/test_relationship_inverse_guard.py b/tests/test_core/test_relationship_inverse_guard.py new file mode 100644 index 0000000..3e2c51b --- /dev/null +++ b/tests/test_core/test_relationship_inverse_guard.py @@ -0,0 +1,100 @@ +"""A directional relationship points ONE way. + +Prod grew rows saying both `PC controls 2005` and `2005 controls PC`, and a CMM +PC showing `<- controls from CMM4` beside its own outgoing link. Only one +direction can be true - a PC drives a machine, never the reverse - but the +duplicate check was keyed on (source, target, type), so the inverse inserted +cleanly and both rows then rendered on both asset pages. + +Symmetric types are exempt on purpose: they store both directions and the card +collapses them to one entry per peer. +""" + +from shopdb.extensions import db +from shopdb.core.models import Asset, AssetType +from shopdb.core.models.relationship import RelationshipType + + +def _asset(assetnumber): + assettype = AssetType.query.filter_by(assettype='machine').first() + if not assettype: + assettype = AssetType(assettype='machine') + db.session.add(assettype) + db.session.flush() + asset = Asset(assetnumber=assetnumber, assettypeid=assettype.assettypeid) + db.session.add(asset) + db.session.flush() + return asset + + +def _reltype(name, directional=True): + existing = RelationshipType.query.filter_by(relationshiptype=name).first() + if existing: + existing.isdirectional = directional + db.session.flush() + return existing + reltype = RelationshipType(relationshiptype=name, isdirectional=directional) + db.session.add(reltype) + db.session.flush() + return reltype + + +def _create(client, headers, source, target, typeid): + return client.post('/api/assets/relationships', + json={'sourceassetid': source, 'targetassetid': target, + 'relationshiptypeid': typeid}, + headers=headers) + + +def test_inverse_of_a_directional_link_is_refused(client, db, auth_headers): + pc, machine = _asset('PC-INV-1'), _asset('MACH-INV-1') + controls = _reltype('controls') + db.session.commit() + + first = _create(client, auth_headers, pc.assetid, machine.assetid, + controls.relationshiptypeid) + assert first.status_code == 201, first.get_json() + + inverse = _create(client, auth_headers, machine.assetid, pc.assetid, + controls.relationshiptypeid) + assert inverse.status_code == 409, inverse.get_json() + message = inverse.get_json()['data']['error']['message'].lower() + assert 'reverse' in message + assert 'one way' in message + + +def test_the_same_direction_is_still_refused_as_a_duplicate(client, db, auth_headers): + pc, machine = _asset('PC-INV-2'), _asset('MACH-INV-2') + controls = _reltype('controls') + db.session.commit() + + assert _create(client, auth_headers, pc.assetid, machine.assetid, + controls.relationshiptypeid).status_code == 201 + again = _create(client, auth_headers, pc.assetid, machine.assetid, + controls.relationshiptypeid) + assert again.status_code == 409 + assert 'already exists' in again.get_json()['data']['error']['message'].lower() + + +def test_symmetric_types_may_store_both_directions(client, db, auth_headers): + """Dualpath bays link both ways by design - the guard must not touch them.""" + baya, bayb = _asset('BAY-INV-A'), _asset('BAY-INV-B') + dualpath = _reltype('Dualpath', directional=False) + db.session.commit() + + assert _create(client, auth_headers, baya.assetid, bayb.assetid, + dualpath.relationshiptypeid).status_code == 201 + both = _create(client, auth_headers, bayb.assetid, baya.assetid, + dualpath.relationshiptypeid) + assert both.status_code == 201, both.get_json() + + +def test_an_asset_cannot_relate_to_itself(client, db, auth_headers): + lonely = _asset('SELF-INV-1') + controls = _reltype('controls') + db.session.commit() + + resp = _create(client, auth_headers, lonely.assetid, lonely.assetid, + controls.relationshiptypeid) + assert resp.status_code == 400, resp.get_json() + assert 'itself' in resp.get_json()['data']['error']['message'].lower()