Relationship inverse guard, audit command, widened direction fixer

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
+    sends programs to the device and receives logs), so per ADR-001 the PC
-    must be the source. Flips every active controls row whose source is a
+    must be the source. Flips every active controls row whose target is a
-    machine asset and target is a computer asset. If the flipped row already
+    computer and whose source is not. If the flipped row already exists, the
-    exists, the reversed duplicate is deactivated instead. Idempotent.
+    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'))

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,

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()