fix: collapse duplicate inventory rows for the same device
A re-scan of an already-approved device created a fresh pending row each time (the upsert only matched status=pending), so one device could show several times in the inventory. Now the scanner keeps one non-hidden row per IP, refreshing it in place and preserving an approved status, and collapses any pre-existing duplicates at scan start. ha-relevant: maybe
This commit is contained in:
@@ -410,6 +410,38 @@ def _mock_scan(target: str) -> list[dict[str, Any]]:
|
||||
]
|
||||
|
||||
|
||||
async def _dedupe_pending_by_ip(db: AsyncSession) -> int:
|
||||
"""Collapse duplicate non-hidden inventory rows that share an IP into one.
|
||||
|
||||
Keeps an ``approved`` row when present (it carries canvas-link semantics),
|
||||
otherwise the oldest row, and deletes the rest. Returns the number deleted.
|
||||
"""
|
||||
rows = (await db.execute(
|
||||
select(PendingDevice)
|
||||
.where(PendingDevice.status != "hidden", PendingDevice.ip.isnot(None))
|
||||
.order_by(PendingDevice.discovered_at)
|
||||
)).scalars().all()
|
||||
|
||||
by_ip: dict[str, list[PendingDevice]] = {}
|
||||
for row in rows:
|
||||
if row.ip is None: # guarded by the query, but keeps the type checker happy
|
||||
continue
|
||||
by_ip.setdefault(row.ip, []).append(row)
|
||||
|
||||
deleted = 0
|
||||
for group in by_ip.values():
|
||||
if len(group) < 2:
|
||||
continue
|
||||
keep = next((r for r in group if r.status == "approved"), group[0])
|
||||
for dup in group:
|
||||
if dup is not keep:
|
||||
await db.delete(dup)
|
||||
deleted += 1
|
||||
if deleted:
|
||||
await db.commit()
|
||||
return deleted
|
||||
|
||||
|
||||
async def run_scan(
|
||||
ranges: list[str],
|
||||
db: AsyncSession,
|
||||
@@ -440,6 +472,11 @@ async def run_scan(
|
||||
)
|
||||
hidden_ips: set[str] = {row[0] for row in hidden_ips_result.fetchall()}
|
||||
|
||||
# Collapse any pre-existing duplicate inventory rows (same IP, non-hidden)
|
||||
# left over from older scans, so the device shows up exactly once even if
|
||||
# it isn't re-discovered this run (e.g. now offline).
|
||||
await _dedupe_pending_by_ip(db)
|
||||
|
||||
# Start mDNS discovery in the background while nmap scans run
|
||||
mdns_task = asyncio.create_task(_mdns_discover())
|
||||
|
||||
@@ -468,19 +505,30 @@ async def run_scan(
|
||||
services = fingerprint_ports(open_ports)
|
||||
suggested_type = suggest_node_type(open_ports, host.get("mac"))
|
||||
|
||||
existing_result = await db.execute(
|
||||
select(PendingDevice).where(
|
||||
PendingDevice.ip == ip,
|
||||
PendingDevice.status == "pending",
|
||||
)
|
||||
)
|
||||
existing = existing_result.scalar_one_or_none()
|
||||
if existing:
|
||||
existing.mac = host.get("mac") or existing.mac
|
||||
existing.hostname = host.get("hostname") or existing.hostname
|
||||
existing.os = host.get("os") or existing.os
|
||||
existing.services = services
|
||||
existing.suggested_type = suggested_type
|
||||
# One inventory row per device (by IP). Match across pending AND
|
||||
# approved so a re-scan of an already-approved device refreshes its
|
||||
# row instead of spawning a fresh "pending" duplicate. Hidden rows
|
||||
# are already skipped above.
|
||||
existing_rows = (await db.execute(
|
||||
select(PendingDevice)
|
||||
.where(PendingDevice.ip == ip, PendingDevice.status != "hidden")
|
||||
.order_by(PendingDevice.discovered_at)
|
||||
)).scalars().all()
|
||||
|
||||
if existing_rows:
|
||||
# Prefer an approved row (it owns the canvas link semantics),
|
||||
# otherwise the oldest. Collapse any leftover duplicates created
|
||||
# by earlier scans.
|
||||
keep = next((r for r in existing_rows if r.status == "approved"), existing_rows[0])
|
||||
for dup in existing_rows:
|
||||
if dup is not keep:
|
||||
await db.delete(dup)
|
||||
keep.mac = host.get("mac") or keep.mac
|
||||
keep.hostname = host.get("hostname") or keep.hostname
|
||||
keep.os = host.get("os") or keep.os
|
||||
keep.services = services
|
||||
keep.suggested_type = suggested_type
|
||||
# status preserved — an approved device stays approved.
|
||||
else:
|
||||
db.add(PendingDevice(
|
||||
ip=ip,
|
||||
|
||||
@@ -476,6 +476,59 @@ async def test_run_scan_records_ip_already_in_canvas(db_session: AsyncSession):
|
||||
assert device.status == "pending"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_scan_refreshes_approved_device_without_duplicating(db_session: AsyncSession):
|
||||
"""Re-scanning an already-approved device updates its row in place instead of
|
||||
spawning a fresh pending duplicate, and keeps it approved."""
|
||||
approved = PendingDevice(
|
||||
id=str(uuid.uuid4()), ip="192.168.1.50", mac=None, hostname="old",
|
||||
os=None, services=[], suggested_type="server", status="approved",
|
||||
)
|
||||
db_session.add(approved)
|
||||
run_id = str(uuid.uuid4())
|
||||
db_session.add(ScanRun(id=run_id, status="running", ranges=["192.168.1.0/24"]))
|
||||
await db_session.commit()
|
||||
|
||||
with (
|
||||
patch("app.services.scanner._nmap_scan", return_value=[MOCK_HOST]),
|
||||
patch("app.api.routes.status.broadcast_scan_update", new_callable=AsyncMock),
|
||||
):
|
||||
await run_scan(["192.168.1.0/24"], db_session, run_id)
|
||||
|
||||
rows = (await db_session.execute(
|
||||
select(PendingDevice).where(PendingDevice.ip == "192.168.1.50")
|
||||
)).scalars().all()
|
||||
assert len(rows) == 1
|
||||
assert rows[0].status == "approved"
|
||||
assert rows[0].hostname == "myhost.lan" # refreshed from the scan
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_scan_collapses_existing_duplicate_rows(db_session: AsyncSession):
|
||||
"""Pre-existing duplicate inventory rows for one IP are collapsed to a single
|
||||
row at scan start, even if the device is not re-discovered."""
|
||||
for status in ("approved", "pending", "pending"):
|
||||
db_session.add(PendingDevice(
|
||||
id=str(uuid.uuid4()), ip="192.168.1.77", mac=None, hostname=None,
|
||||
os=None, services=[], suggested_type="server", status=status,
|
||||
))
|
||||
run_id = str(uuid.uuid4())
|
||||
db_session.add(ScanRun(id=run_id, status="running", ranges=["192.168.1.0/24"]))
|
||||
await db_session.commit()
|
||||
|
||||
with (
|
||||
patch("app.services.scanner._nmap_scan", return_value=[]),
|
||||
patch("app.api.routes.status.broadcast_scan_update", new_callable=AsyncMock),
|
||||
):
|
||||
await run_scan(["192.168.1.0/24"], db_session, run_id)
|
||||
|
||||
rows = (await db_session.execute(
|
||||
select(PendingDevice).where(PendingDevice.ip == "192.168.1.77")
|
||||
)).scalars().all()
|
||||
assert len(rows) == 1
|
||||
assert rows[0].status == "approved" # approved row is the one kept
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_scan_skips_hidden_device(db_session: AsyncSession):
|
||||
"""Devices previously hidden by the user must not re-appear in pending on re-scan."""
|
||||
|
||||
Reference in New Issue
Block a user