From 2d3b646e45a06fba7cc110dbd08d8b68f96e1e9a Mon Sep 17 00:00:00 2001 From: Pouzor Date: Thu, 9 Jul 2026 14:41:53 +0200 Subject: [PATCH] fix: match scanned devices to canvas nodes by ip-token and mac (#258) Node.ip stores several comma-separated addresses once a user adds an IPv6 (e.g. "fe80::1, 192.168.1.5"). All placement/inventory matching did exact string equality on that field, so a device scanned as the plain IPv4 looked absent from the canvas: canvas_count stayed 0, the "In N canvas" badge and hide-on-canvas button vanished, and bulk-approve re-placed a duplicate node. Match per-address instead, and add MAC (a stable identifier immune to IP edits) as a cumulative match key alongside ieee/ip: - _canvas_correlation indexes ip per token + by_mac - bulk_approve skip detection tokenizes ip + tracks mac - find_duplicate_node narrows with Node.ip.contains then confirms per token in Python (guards the 10.0.0.4 / 10.0.0.40 substring false match) ha-relevant: maybe --- backend/app/api/routes/scan.py | 80 ++++++++++++++++++------ backend/app/services/node_dedupe.py | 54 ++++++++++++----- backend/tests/test_scan.py | 94 ++++++++++++++++++++++++++++- 3 files changed, 192 insertions(+), 36 deletions(-) diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 8ad1f4b..51c5032 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -27,6 +27,17 @@ _ZIGBEE_TYPES = {"zigbee_coordinator", "zigbee_router", "zigbee_enddevice"} _ZWAVE_TYPES = {"zwave_coordinator", "zwave_router", "zwave_enddevice"} +def _ip_tokens(ip: str | None) -> list[str]: + """Split a Node/device ``ip`` field into individual addresses. + + The canvas stores multiple addresses in one comma-separated string (e.g. + ``"fe80::1, 192.168.1.5"`` once a user adds an IPv6 address). Matching a + scanned device against that field must compare per-address, not against the + whole string, or the device looks absent from the canvas (issue #258). + """ + return [t.strip() for t in ip.split(",") if t.strip()] if ip else [] + + def _is_wireless(node_type: str | None) -> bool: """Zigbee + Z-Wave mesh devices share online status / no ICMP check.""" return node_type in _ZIGBEE_TYPES or node_type in _ZWAVE_TYPES @@ -215,12 +226,18 @@ def _agg(values: list[datetime], *, newest: bool) -> datetime | None: async def _canvas_correlation( db: AsyncSession, devices: list[PendingDevice] ) -> dict[str, dict[str, Any]]: - """Correlate each device to existing canvas nodes by ``ieee_address`` or ``ip``. + """Correlate each device to existing canvas nodes by ``ieee_address``, ``mac`` + or ``ip``. Returns, per device id: the number of distinct canvases (designs) it appears on, plus aggregated timestamps from every matching node — created_at (oldest), last_scan / updated_at / last_seen (newest). One node query, grouped in Python (node counts are small for a homelab), so no N+1 per device. + + IP matching is per-address: a node's ``ip`` may hold several comma-separated + addresses (e.g. an IPv6 added before the IPv4), so we index each token, not + the raw string. MAC is a stable identifier immune to such IP edits, so it is + matched too — cumulatively with ieee/ip (issue #258). """ if not devices: return {} @@ -228,6 +245,7 @@ async def _canvas_correlation( await db.execute( select( Node.ip, + Node.mac, Node.ieee_address, Node.design_id, Node.created_at, @@ -237,12 +255,15 @@ async def _canvas_correlation( ).where(Node.design_id.isnot(None)) ) ).all() - # Index matching nodes by ip and by ieee so a device can look up both. + # Index matching nodes by ip token, mac and ieee so a device can look up any. by_ip: dict[str, list[Any]] = {} + by_mac: dict[str, list[Any]] = {} by_ieee: dict[str, list[Any]] = {} for row in rows: - if row.ip: - by_ip.setdefault(row.ip, []).append(row) + for tok in _ip_tokens(row.ip): + by_ip.setdefault(tok, []).append(row) + if row.mac: + by_mac.setdefault(row.mac, []).append(row) if row.ieee_address: by_ieee.setdefault(row.ieee_address, []).append(row) @@ -251,9 +272,11 @@ async def _canvas_correlation( matched = [] if d.ieee_address: matched += by_ieee.get(d.ieee_address, []) - if d.ip: - matched += by_ip.get(d.ip, []) - # De-duplicate nodes matched by both ip and ieee. + if d.mac: + matched += by_mac.get(d.mac, []) + for tok in _ip_tokens(d.ip): + matched += by_ip.get(tok, []) + # De-duplicate nodes matched by more than one identifier. matched = list({id(m): m for m in matched}.values()) designs = {m.design_id for m in matched} info[d.id] = { @@ -335,17 +358,24 @@ async def bulk_approve_devices( devices = result.scalars().all() # What already sits on the target canvas, so we skip devices already placed - # here (by ip or ieee_address) instead of creating duplicate nodes. We map to - # the existing node id so the skip report can point the user at it. A value - # may be a Node still pending flush (in-batch duplicate) — resolved to its id - # after the flush below. + # here (by ip, mac or ieee_address) instead of creating duplicate nodes. We + # map to the existing node id so the skip report can point the user at it. A + # value may be a Node still pending flush (in-batch duplicate) — resolved to + # its id after the flush below. IPs are indexed per comma-separated token so + # a node whose ip is "fe80::1, 192.168.1.5" still matches a device scanned as + # 192.168.1.5 (issue #258). existing = ( await db.execute( - select(Node.id, Node.ip, Node.ieee_address).where(Node.design_id == default_design_id) + select(Node.id, Node.ip, Node.mac, Node.ieee_address).where( + Node.design_id == default_design_id + ) ) ).all() - placed_ips: dict[str, Any] = {ip: nid for nid, ip, _ in existing if ip} - placed_ieee: dict[str, Any] = {ieee: nid for nid, _, ieee in existing if ieee} + placed_ips: dict[str, Any] = { + tok: nid for nid, ip, _, _ in existing for tok in _ip_tokens(ip) + } + placed_mac: dict[str, Any] = {mac: nid for nid, _, mac, _ in existing if mac} + placed_ieee: dict[str, Any] = {ieee: nid for nid, _, _, ieee in existing if ieee} created_nodes: list[Node] = [] approved_devices: list[PendingDevice] = [] @@ -353,11 +383,12 @@ async def bulk_approve_devices( for device in devices: # Record which identifier collided so the caller can explain each skip # (and, for existing on-canvas nodes, link to the node already there). - if device.ip is not None and device.ip in placed_ips: + ip_hit = next((t for t in _ip_tokens(device.ip) if t in placed_ips), None) + if ip_hit is not None: skipped_devices.append({ "device_id": device.id, "label": device.hostname or device.friendly_name or device.ip or "device", - "match": "ip", "value": device.ip, "_ref": placed_ips[device.ip], + "match": "ip", "value": ip_hit, "_ref": placed_ips[ip_hit], }) continue if device.ieee_address is not None and device.ieee_address in placed_ieee: @@ -367,6 +398,13 @@ async def bulk_approve_devices( "match": "ieee", "value": device.ieee_address, "_ref": placed_ieee[device.ieee_address], }) continue + if device.mac is not None and device.mac in placed_mac: + skipped_devices.append({ + "device_id": device.id, + "label": device.hostname or device.friendly_name or device.mac or "device", + "match": "mac", "value": device.mac, "_ref": placed_mac[device.mac], + }) + continue device.status = "approved" node_type = device.suggested_type or "generic" is_wireless = _is_wireless(node_type) @@ -394,11 +432,13 @@ async def bulk_approve_devices( db.add(node) created_nodes.append(node) approved_devices.append(device) - # Track within this batch so a duplicate selection (same ip/ieee) is not - # placed twice on the same canvas. Store the Node so a later in-batch + # Track within this batch so a duplicate selection (same ip/mac/ieee) is + # not placed twice on the same canvas. Store the Node so a later in-batch # skip can resolve to its id after flush. - if device.ip: - placed_ips[device.ip] = node + for tok in _ip_tokens(device.ip): + placed_ips[tok] = node + if device.mac: + placed_mac[device.mac] = node if device.ieee_address: placed_ieee[device.ieee_address] = node await db.flush() # populates node.id from Python-side default before reading diff --git a/backend/app/services/node_dedupe.py b/backend/app/services/node_dedupe.py index be7b196..b27fa2e 100644 --- a/backend/app/services/node_dedupe.py +++ b/backend/app/services/node_dedupe.py @@ -29,6 +29,15 @@ from app.services.zigbee_service import merge_zigbee_properties logger = logging.getLogger(__name__) +def _ip_tokens(ip: str | None) -> list[str]: + """Split a Node ``ip`` field into individual, whitespace-trimmed addresses. + + The canvas stores several addresses in one comma-separated string once a + user adds e.g. an IPv6 address, so identity matching must compare per token. + """ + return [t.strip() for t in ip.split(",") if t.strip()] if ip else [] + + async def find_duplicate_node( db: AsyncSession, design_id: str | None, @@ -49,31 +58,48 @@ async def find_duplicate_node( (:func:`dedupe_nodes_by_ieee` still repairs *pre-existing* same-canvas IEEE duplicates; this guard prevents new ones unless the user forces them.) """ + ip_toks = _ip_tokens(ip) conds = [] if ieee: conds.append(Node.ieee_address == ieee) - if ip: - conds.append(Node.ip == ip) + # A node's ip may hold several comma-separated addresses (e.g. an IPv6 added + # before the IPv4), so narrow with a substring match then confirm per-token + # in Python — exact ``Node.ip == ip`` would miss those rows (issue #258). + for tok in ip_toks: + conds.append(Node.ip.contains(tok)) if mac: conds.append(Node.mac == mac) if not conds: return None - existing = ( + candidates = ( await db.execute( select(Node).where(Node.design_id == design_id, or_(*conds)) ) - ).scalars().first() - if existing is None: + ).scalars().all() + + # Confirm a real match (the ip ``contains`` above can false-positive, e.g. + # "1.2.3.4" inside "1.2.3.40"), preferring ieee > ip > mac. + def _matches(node: Node) -> tuple[str, str | None] | None: + if ieee and node.ieee_address == ieee: + return "ieee", node.ieee_address + node_toks = set(_ip_tokens(node.ip)) + hit = next((t for t in ip_toks if t in node_toks), None) + if hit is not None: + return "ip", hit + if mac and node.mac == mac: + return "mac", mac return None - # Report whichever field actually matched (ieee > ip > mac when several do). - match: str - value: str | None - if ieee and existing.ieee_address == ieee: - match, value = "ieee", existing.ieee_address - elif ip and existing.ip == ip: - match, value = "ip", existing.ip - else: - match, value = "mac", existing.mac + + existing = None + matched: tuple[str, str | None] | None = None + for node in candidates: + m = _matches(node) + if m is not None: + existing, matched = node, m + break + if existing is None or matched is None: + return None + match, value = matched return { "duplicate": True, "existing_node_id": existing.id, diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 19a0713..f95ded1 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -181,14 +181,38 @@ async def _add_design(db_session, name: str) -> str: return design.id -def _node(design_id: str, *, ip=None, ieee=None) -> Node: +def _node(design_id: str, *, ip=None, ieee=None, mac=None) -> Node: return Node( id=str(uuid.uuid4()), label="n", type="server", status="online", - ip=ip, ieee_address=ieee, services=[], pos_x=0.0, pos_y=0.0, + ip=ip, mac=mac, ieee_address=ieee, services=[], pos_x=0.0, pos_y=0.0, design_id=design_id, ) +@pytest.mark.asyncio +async def test_canvas_count_matches_ip_in_comma_list(client, headers, db_session, pending_device): + # Node.ip holds several comma-separated addresses (IPv6 added first). The + # device scanned as the plain IPv4 must still correlate (issue #258). + d1 = await _add_design(db_session, "Home") + db_session.add(_node(d1, ip="fe80::1, 192.168.1.100")) + await db_session.commit() + + data = (await client.get("/api/v1/scan/pending", headers=headers)).json() + assert data[0]["canvas_count"] == 1 + + +@pytest.mark.asyncio +async def test_canvas_count_correlates_by_mac(client, headers, db_session, pending_device): + # Node's ip differs entirely (user edited it) but the MAC still matches: + # the device is on the canvas (issue #258, MAC is the stable identifier). + d1 = await _add_design(db_session, "Home") + db_session.add(_node(d1, ip="10.9.9.9", mac="aa:bb:cc:dd:ee:ff")) + await db_session.commit() + + data = (await client.get("/api/v1/scan/pending", headers=headers)).json() + assert data[0]["canvas_count"] == 1 + + @pytest.mark.asyncio async def test_canvas_count_counts_distinct_designs_by_ip(client, headers, db_session, pending_device): # Same IP placed on two different canvases → canvas_count == 2. @@ -443,6 +467,50 @@ async def test_approve_device_conflicts_on_existing_mac( assert res.json()["detail"]["match"] == "mac" +@pytest.mark.asyncio +async def test_approve_device_conflicts_on_ip_in_comma_list( + client: AsyncClient, headers, db_session, pending_device +): + """The existing node's ip holds an IPv6 before the IPv4 the device scanned + as. Exact-string matching missed it (issue #258); per-token matching catches + the duplicate.""" + design = await _add_design(db_session, "Home") + existing = _node(design, ip="fe80::1, 192.168.1.100") + db_session.add(existing) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "dup", "type": "server", "ip": "192.168.1.100", + "status": "unknown", "services": [], "design_id": design}, + headers=headers, + ) + assert res.status_code == 409 + detail = res.json()["detail"] + assert detail["existing_node_id"] == existing.id + assert detail["match"] == "ip" + assert detail["value"] == "192.168.1.100" + + +@pytest.mark.asyncio +async def test_approve_device_no_conflict_on_ip_substring( + client: AsyncClient, headers, db_session, pending_device +): + """The ip guard must match whole addresses, not substrings: a node at + 10.0.0.40 is not a duplicate of a device at 10.0.0.4.""" + design = await _add_design(db_session, "Home") + db_session.add(_node(design, ip="10.0.0.40")) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "new", "type": "server", "ip": "10.0.0.4", + "mac": None, "status": "unknown", "services": [], "design_id": design}, + headers=headers, + ) + assert res.status_code == 200 + + @pytest.mark.asyncio async def test_approve_device_force_creates_duplicate( client: AsyncClient, headers, db_session, pending_device @@ -1085,6 +1153,28 @@ async def test_bulk_approve_skips_device_already_on_target_design( assert sorted(n.ip for n in nodes) == ["192.168.1.10", "192.168.1.11"] +@pytest.mark.asyncio +async def test_bulk_approve_skips_device_matching_ip_in_comma_list( + client: AsyncClient, headers, db_session, two_pending_devices +): + """The on-canvas node's ip holds an IPv6 before the IPv4; the device scanned + as the plain IPv4 is still recognised as already placed (issue #258).""" + ids = [d.id for d in two_pending_devices] + design = await _add_design(db_session, "Canvas") + db_session.add(_node(design, ip="fe80::1, 192.168.1.10")) + await db_session.commit() + + res = await client.post( + "/api/v1/scan/pending/bulk-approve", + json={"device_ids": ids, "design_id": design}, + headers=headers, + ) + data = res.json() + assert data["approved"] == 1 # only the second device (192.168.1.11) + assert data["skipped"] == 1 + assert data["skipped_devices"][0]["value"] == "192.168.1.10" + + @pytest.mark.asyncio async def test_bulk_approve_reports_skipped_devices( client: AsyncClient, headers, db_session, two_pending_devices