diff --git a/backend/app/api/routes/nodes.py b/backend/app/api/routes/nodes.py index f2177a9..2d5385b 100644 --- a/backend/app/api/routes/nodes.py +++ b/backend/app/api/routes/nodes.py @@ -150,6 +150,11 @@ async def update_node( if not node: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Node not found") sent = body.model_dump(exclude_unset=True) + # A node can never be its own parent: the canvas' parent walks assume an + # acyclic tree and a self-parent row freezes the node on screen (#370). + # Dropped rather than rejected so the rest of the edit still lands. + if sent.get("parent_id") == node_id: + sent.pop("parent_id") for field, value in node_columns(sent).items(): setattr(node, field, value) # The user edited the device, not just its drawing: push the facts down. diff --git a/backend/app/db/database.py b/backend/app/db/database.py index df2b2e8..b3fccbc 100644 --- a/backend/app/db/database.py +++ b/backend/app/db/database.py @@ -495,6 +495,7 @@ async def init_db() -> None: await _drop_legacy_node_columns() await _seed_node_views() await _backfill_zone_size() + await _repair_self_parent_nodes() @@ -813,6 +814,47 @@ async def _backfill_zone_size() -> None: logger.warning("Backfilling zone width/height failed: %s", exc) +async def _repair_self_parent_nodes() -> None: + """Detach any node that is recorded as its own parent. + + Several write paths could persist ``parent_id = id`` before the guards + added alongside this repair: a YAML import resolving a parent by a label + that mapped back to the node, the node dedupe re-pointing a canonical node + that had been nested under one of its own duplicates, and any client PATCH + or canvas save, neither of which validated it (#370). + + The row is fatal on the canvas: the parent walks assume an acyclic tree, so + dragging the node overflowed the stack inside the change reducer and the + move was silently dropped — the node selected but would not move. It also + renders unparented, so it sits wherever its stored coordinates put it + rather than inside the container it appears to belong to. + + Clearing the column is the only safe repair: the real parent is not + recoverable from the row, and NULL simply returns the node to the top level + where the user can re-nest it. Idempotent — a second run matches nothing. + + Never fatal: a failure here leaves the row as it was, and the runtime cycle + guards keep the canvas usable either way. + """ + try: + async with engine.begin() as conn: + rows = ( + await conn.exec_driver_sql( + "SELECT id, label FROM nodes WHERE parent_id IS NOT NULL AND parent_id = id" + ) + ).fetchall() + if not rows: + return + await conn.exec_driver_sql( + "UPDATE nodes SET parent_id = NULL WHERE parent_id IS NOT NULL AND parent_id = id" + ) + for node_id, label in rows: + logger.info("Detached self-parented node %s (%s)", node_id, label) + logger.info("Repaired %d node(s) recorded as their own parent", len(rows)) + except Exception as exc: # pragma: no cover - defensive, boot must not die + logger.warning("Repairing self-parented nodes failed: %s", exc) + + async def get_db() -> AsyncGenerator[AsyncSession, None]: async with AsyncSessionLocal() as session: yield session diff --git a/backend/app/schemas/canvas.py b/backend/app/schemas/canvas.py index 5fa2bc7..4c2164c 100644 --- a/backend/app/schemas/canvas.py +++ b/backend/app/schemas/canvas.py @@ -1,6 +1,6 @@ from typing import Any -from pydantic import BaseModel, field_validator +from pydantic import BaseModel, field_validator, model_validator from app.schemas.edges import EdgeResponse from app.schemas.nodes import NodeResponse @@ -51,6 +51,22 @@ class NodeSave(BaseModel): pos_x: float = 0 pos_y: float = 0 + @model_validator(mode="after") + def drop_self_parent(self) -> "NodeSave": + """A node can never be its own parent. + + Every parent walk on the canvas — waypoint propagation, ordering + parents before children, collapse — assumes an acyclic tree, and a + self-parent row makes the node undraggable once it is reloaded (#370). + + Dropped rather than rejected: a canvas that already carries the bad row + must still be able to save, and a 422 here would cost the user the whole + save. The startup repair clears what is already persisted. + """ + if self.parent_id is not None and self.parent_id == self.id: + self.parent_id = None + return self + class EdgeSave(BaseModel): id: str diff --git a/backend/app/services/node_dedupe.py b/backend/app/services/node_dedupe.py index 25c928b..5c82f9e 100644 --- a/backend/app/services/node_dedupe.py +++ b/backend/app/services/node_dedupe.py @@ -121,7 +121,11 @@ async def dedupe_nodes_by_device(db: AsyncSession) -> int: await db.execute(select(Node).where(Node.parent_id.in_(dup_ids))) ).scalars().all() for child in children: - child.parent_id = canonical.id + # The canonical node may itself have been nested under one of its + # own duplicates. Re-pointing it here would make it its own parent, + # which freezes it on the canvas (#370) — detach it instead. Mirrors + # the self-loop edge deletion below. + child.parent_id = None if child.id == canonical.id else canonical.id all_edges = ( await db.execute( diff --git a/backend/tests/test_canvas.py b/backend/tests/test_canvas.py index abb51a6..48bacaf 100644 --- a/backend/tests/test_canvas.py +++ b/backend/tests/test_canvas.py @@ -706,3 +706,37 @@ async def test_save_canvas_custom_style_overwrite(client: AsyncClient, headers: canvas = (await client.get("/api/v1/canvas", headers=headers)).json() assert "proxmox" in canvas["custom_style"]["nodes"] assert "server" not in canvas["custom_style"]["nodes"] + + +# ── self-parent guard (#370) ───────────────────────────────────────────────── + +async def test_save_canvas_drops_a_node_parented_to_itself(client: AsyncClient, headers: dict): + # A self-parent row freezes the node on the canvas and survives a reload, so + # the save normalizes it away. Dropped rather than rejected: a canvas that + # already carries the bad row must still be able to save. + n = node_payload(type="lxc", label="pihole") + n["parent_id"] = n["id"] + + res = await client.post( + "/api/v1/canvas/save", json={"nodes": [n], "edges": [], "viewport": {}}, headers=headers + ) + assert res.status_code == 200 + + saved = (await client.get("/api/v1/canvas", headers=headers)).json()["nodes"] + assert len(saved) == 1 + assert saved[0]["parent_id"] is None + + +async def test_save_canvas_keeps_a_real_parent(client: AsyncClient, headers: dict): + host = node_payload(type="proxmox", label="pve", container_mode=True) + child = node_payload(type="lxc", label="pihole", parent_id=host["id"]) + + res = await client.post( + "/api/v1/canvas/save", + json={"nodes": [host, child], "edges": [], "viewport": {}}, + headers=headers, + ) + assert res.status_code == 200 + + saved = {n["label"]: n for n in (await client.get("/api/v1/canvas", headers=headers)).json()["nodes"]} + assert saved["pihole"]["parent_id"] == host["id"] diff --git a/backend/tests/test_node_dedupe.py b/backend/tests/test_node_dedupe.py index 65fb8a3..8097b61 100644 --- a/backend/tests/test_node_dedupe.py +++ b/backend/tests/test_node_dedupe.py @@ -134,6 +134,30 @@ async def test_repoints_child_parent(db_session): assert child.parent_id == keep.id +@pytest.mark.asyncio +async def test_canonical_nested_under_its_own_duplicate_is_detached(db_session): + """#370 — re-pointing the canonical node made it its own parent. + + The canonical node had been nested under one of its duplicates, so the + blanket re-point wrote `parent_id = id` and froze it on the canvas. + """ + d = await _design(db_session) + device = await _device(db_session, ieee="0xFFF") + keep = Node(label="Host", type="proxmox", design_id=d.id, device_id=device.id) + db_session.add(keep) + await db_session.flush() + dup = Node(label="Host", type="proxmox", design_id=d.id, device_id=device.id) + db_session.add(dup) + await db_session.flush() + # The oldest node is the one that survives, and it points at the newer twin. + keep.parent_id = dup.id + await db_session.flush() + + await dedupe_nodes_by_device(db_session) + await db_session.refresh(keep) + assert keep.parent_id is None + + @pytest.mark.asyncio async def test_idempotent_noop_when_unique(db_session): d = await _design(db_session) diff --git a/backend/tests/test_nodes.py b/backend/tests/test_nodes.py index ce61a0c..a211488 100644 --- a/backend/tests/test_nodes.py +++ b/backend/tests/test_nodes.py @@ -403,3 +403,41 @@ async def test_list_nodes_without_label_param_returns_all(client: AsyncClient, h res = await client.get("/api/v1/nodes", headers=headers) assert res.status_code == 200 assert len(res.json()) == 3 + + +# ── self-parent guard (#370) ───────────────────────────────────────────────── + +async def test_update_node_ignores_a_parent_id_pointing_at_itself(client: AsyncClient, headers: dict): + create = await client.post( + "/api/v1/nodes", json={"type": "lxc", "label": "pihole", "status": "unknown"}, headers=headers + ) + node_id = create.json()["id"] + + res = await client.patch( + f"/api/v1/nodes/{node_id}", json={"parent_id": node_id, "label": "renamed"}, headers=headers + ) + + assert res.status_code == 200 + assert res.json()["parent_id"] is None + # The key is dropped, not the whole edit. + assert res.json()["label"] == "renamed" + + +async def test_update_node_self_parent_leaves_a_real_parent_alone(client: AsyncClient, headers: dict): + host = await client.post( + "/api/v1/nodes", + json={"type": "proxmox", "label": "pve", "status": "unknown", "container_mode": True}, + headers=headers, + ) + host_id = host.json()["id"] + child = await client.post( + "/api/v1/nodes", + json={"type": "lxc", "label": "pihole", "status": "unknown", "parent_id": host_id}, + headers=headers, + ) + child_id = child.json()["id"] + + res = await client.patch(f"/api/v1/nodes/{child_id}", json={"parent_id": child_id}, headers=headers) + + assert res.status_code == 200 + assert res.json()["parent_id"] == host_id diff --git a/backend/tests/test_self_parent_repair.py b/backend/tests/test_self_parent_repair.py new file mode 100644 index 0000000..288f035 --- /dev/null +++ b/backend/tests/test_self_parent_repair.py @@ -0,0 +1,92 @@ +"""Detaching nodes recorded as their own parent (#370). + +A ``parent_id = id`` row is fatal on the canvas: every parent walk assumes an +acyclic tree, so dragging the node overflowed the stack inside the change +reducer and the move was silently dropped — the node selected but would not +move. It also renders unparented, sitting wherever its stored coordinates put +it rather than inside the container it appears to belong to. + +The repair runs at startup because the bad rows are already in users' databases; +the write-path guards shipped alongside it only stop new ones. +""" +import pytest +from sqlalchemy.ext.asyncio import create_async_engine + +import app.db.database as database + +pytestmark = pytest.mark.asyncio + +_DDL = "CREATE TABLE nodes (id VARCHAR PRIMARY KEY, label VARCHAR, parent_id VARCHAR)" + + +async def _engine(tmp_path, rows): + engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path / 'selfparent.db'}") + async with engine.begin() as conn: + await conn.exec_driver_sql(_DDL) + for node_id, parent_id in rows: + await conn.exec_driver_sql( + "INSERT INTO nodes (id, label, parent_id) VALUES (?, ?, ?)", + (node_id, node_id, parent_id), + ) + return engine + + +async def _parents(engine) -> dict[str, str | None]: + async with engine.begin() as conn: + rows = (await conn.exec_driver_sql("SELECT id, parent_id FROM nodes")).fetchall() + return {r[0]: r[1] for r in rows} + + +async def _run(engine, monkeypatch) -> None: + monkeypatch.setattr(database, "engine", engine) + await database._repair_self_parent_nodes() + + +async def test_detaches_a_self_parented_node(tmp_path, monkeypatch): + engine = await _engine(tmp_path, [("pihole", "pihole")]) + + await _run(engine, monkeypatch) + + assert (await _parents(engine))["pihole"] is None + + +async def test_leaves_real_parents_alone(tmp_path, monkeypatch): + engine = await _engine( + tmp_path, + [("pve", None), ("vm", "pve"), ("broken", "broken")], + ) + + await _run(engine, monkeypatch) + + parents = await _parents(engine) + assert parents == {"pve": None, "vm": "pve", "broken": None} + + +async def test_is_idempotent(tmp_path, monkeypatch): + engine = await _engine(tmp_path, [("a", "a"), ("b", None)]) + + await _run(engine, monkeypatch) + await _run(engine, monkeypatch) + + assert await _parents(engine) == {"a": None, "b": None} + + +async def test_does_not_touch_a_two_node_cycle(tmp_path, monkeypatch): + """Only the exact self-parent case is repairable here. + + A longer cycle has no single right answer for which link to cut, and the + runtime cycle guards keep the canvas usable, so the rows are left as they + are rather than guessed at. + """ + engine = await _engine(tmp_path, [("a", "b"), ("b", "a")]) + + await _run(engine, monkeypatch) + + assert await _parents(engine) == {"a": "b", "b": "a"} + + +async def test_survives_a_missing_table(tmp_path, monkeypatch): + """Boot must not die on a repair; the canvas works either way.""" + engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path / 'empty.db'}") + + await _run(engine, monkeypatch) diff --git a/frontend/src/stores/__tests__/canvasStore/containers.test.ts b/frontend/src/stores/__tests__/canvasStore/containers.test.ts index 0e46b00..ea55fa2 100644 --- a/frontend/src/stores/__tests__/canvasStore/containers.test.ts +++ b/frontend/src/stores/__tests__/canvasStore/containers.test.ts @@ -236,4 +236,47 @@ describe('canvasStore — containers & nesting', () => { const childIdx = nodes.findIndex((n) => n.id === 'c1') expect(parentIdx).toBeLessThan(childIdx) }) + + // #370 — nothing used to stop a node being written as its own parent, and + // the row is fatal: it freezes the node on the canvas and survives a save. + it('updateNode ignores a parent_id pointing at the node itself', () => { + const n = { ...makeNode('n1', { container_mode: true }), position: { x: 50, y: 50 } } + useCanvasStore.setState({ nodes: [n] }) + + useCanvasStore.getState().updateNode('n1', { parent_id: 'n1', label: 'renamed' }) + + const after = useCanvasStore.getState().nodes[0] + expect(after.data.parent_id).toBeUndefined() + expect(after.parentId).toBeUndefined() + // The rest of the edit still lands — the key is dropped, not the update. + expect(after.data.label).toBe('renamed') + expect(after.position).toEqual({ x: 50, y: 50 }) + }) + + it('updateNode self-parent does not detach a node from its real parent', () => { + const parent = { ...makeNode('p1', { container_mode: true }), position: { x: 100, y: 100 } } + useCanvasStore.setState({ nodes: [parent] }) + useCanvasStore.getState().addNode({ + ...makeNode('c1', { parent_id: 'p1' }), + position: { x: 140, y: 160 }, + } as Node) + + useCanvasStore.getState().updateNode('c1', { parent_id: 'c1' }) + + const child = useCanvasStore.getState().nodes.find((x) => x.id === 'c1')! + expect(child.parentId).toBe('p1') + expect(child.data.parent_id).toBe('p1') + }) + + it('addNode strips a parent_id pointing at the node itself', () => { + useCanvasStore.getState().addNode({ + ...makeNode('n1', { parent_id: 'n1', container_mode: true }), + position: { x: 30, y: 30 }, + } as Node) + + const added = useCanvasStore.getState().nodes[0] + expect(added.data.parent_id).toBeUndefined() + expect(added.parentId).toBeUndefined() + expect(added.position).toEqual({ x: 30, y: 30 }) + }) }) diff --git a/frontend/src/stores/__tests__/canvasStore/waypoints.test.ts b/frontend/src/stores/__tests__/canvasStore/waypoints.test.ts index 47cfb51..0c8e6e5 100644 --- a/frontend/src/stores/__tests__/canvasStore/waypoints.test.ts +++ b/frontend/src/stores/__tests__/canvasStore/waypoints.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach } from 'vitest' import { useCanvasStore } from '@/stores/canvasStore' import type { NodeChange, Node } from '@xyflow/react' import type { NodeData } from '@/types' -import { makeNode, makeEdge } from '@/test/factories' +import { makeNode, makeNodeData, makeEdge } from '@/test/factories' function resetStore() { useCanvasStore.setState({ @@ -102,4 +102,40 @@ describe('canvasStore — edge waypoints follow dragged nodes (#279)', () => { useCanvasStore.getState().onNodesChange([{ type: 'select', id: 'a', selected: true }]) expect(useCanvasStore.getState().edges[0].data!.waypoints).toEqual([{ x: 100, y: 100 }]) }) + + // #370 — a corrupt row pointing a node at itself made the child walk recurse + // forever. The overflow was thrown inside onNodesChange's reducer, so the + // whole state update was discarded: the node selected but never moved. + it('moves a node that is recorded as its own parent', () => { + useCanvasStore.setState({ + nodes: [makeNode({ id: 'a', position: { x: 0, y: 0 }, data: makeNodeData({ parent_id: 'a' }) })], + edges: [], + }) + useCanvasStore.getState().onNodesChange([posChange('a', 40, 40)]) + expect(useCanvasStore.getState().nodes[0].position).toEqual({ x: 40, y: 40 }) + }) + + it('translates waypoints once for a self-parented node instead of hanging', () => { + useCanvasStore.setState({ + nodes: [ + makeNode({ id: 'a', position: { x: 0, y: 0 }, data: makeNodeData({ parent_id: 'a' }) }), + makeNode('b', {}), + ], + edges: [makeEdge('e1', 'a', 'b', { data: { type: 'ethernet', waypoints: [{ x: 100, y: 100 }] } })], + }) + useCanvasStore.getState().onNodesChange([posChange('a', 10, 10)]) + expect(useCanvasStore.getState().edges[0].data!.waypoints).toEqual([{ x: 110, y: 110 }]) + }) + + it('survives a longer parent cycle (a -> b -> a)', () => { + useCanvasStore.setState({ + nodes: [ + makeNode({ id: 'a', position: { x: 0, y: 0 }, data: makeNodeData({ parent_id: 'b' }) }), + makeNode({ id: 'b', position: { x: 0, y: 0 }, data: makeNodeData({ parent_id: 'a' }) }), + ], + edges: [], + }) + useCanvasStore.getState().onNodesChange([posChange('a', 25, 25)]) + expect(useCanvasStore.getState().nodes[0].position).toEqual({ x: 25, y: 25 }) + }) }) diff --git a/frontend/src/stores/canvasStore.ts b/frontend/src/stores/canvasStore.ts index 93f0ea8..ef9ff46 100644 --- a/frontend/src/stores/canvasStore.ts +++ b/frontend/src/stores/canvasStore.ts @@ -125,7 +125,15 @@ function translateWaypointsForMovedNodes( arr.push(n.id) childrenByParent.set(pid, arr) } + // A corrupt row can make a node its own parent (or close a longer cycle), + // and the walk below would then never terminate — the resulting stack + // overflow is thrown inside onNodesChange's reducer, so the whole state + // update is dropped and the node stops moving while still selecting (#370). + // Mirrors the cycle guard orderParentsFirst already carries. + const walked = new Set() const propagate = (id: string, d: { dx: number; dy: number }) => { + if (walked.has(id)) return + walked.add(id) for (const childId of childrenByParent.get(id) ?? []) { // A directly-dragged child keeps its own delta; don't overwrite it. if (!deltaById.has(childId)) deltaById.set(childId, d) @@ -496,6 +504,10 @@ export const useCanvasStore = create((rawSet, get) => { addNode: (node) => set((state) => { + // Same rule as updateNode: never let a node parent itself (#370). + if (node.data.parent_id === node.id) { + node = { ...node, data: { ...node.data, parent_id: undefined }, parentId: undefined, extent: undefined } + } const parent = node.data.parent_id ? state.nodes.find((n) => n.id === node.data.parent_id) : null // A visual group — and a groupRect zone — nests its children just like a // container-mode host. @@ -526,8 +538,17 @@ export const useCanvasStore = create((rawSet, get) => { return { nodes: [...withoutNew, enriched], hasUnsavedChanges: true } }), - updateNode: (id, data) => + updateNode: (id, incoming) => set((state) => { + // A node can never be its own parent. Every parent walk (waypoint + // propagation, parents-before-children ordering, collapse) assumes an + // acyclic tree, and the row survives a save, so the canvas comes back + // broken on the next load (#370). Drop the key and apply the rest. + let data = incoming + if (incoming.parent_id === id) { + data = { ...incoming } + delete data.parent_id + } let nodes = state.nodes.map((n) => { if (n.id !== id) return n const updated: Node = { ...n, data: { ...n.data, ...data } } diff --git a/frontend/src/utils/__tests__/importYaml.test.ts b/frontend/src/utils/__tests__/importYaml.test.ts index 970b341..346e801 100644 --- a/frontend/src/utils/__tests__/importYaml.test.ts +++ b/frontend/src/utils/__tests__/importYaml.test.ts @@ -339,6 +339,26 @@ describe('parseYamlToCanvas', () => { warnSpy.mockRestore() }) + // #370 — parents resolve by label, so a node naming its own label mapped + // back to itself and imported as its own parent, which freezes it on canvas. + it('warns and skips a parent label that resolves to the node itself', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const yaml = ` +- nodeType: lxc + label: "pihole" + parent: + label: "pihole" + linkType: virtual +` + const { nodes, edges } = parseYamlToCanvas(yaml, empty, emptyEdges) + expect(nodes).toHaveLength(1) + expect(nodes[0].parentId).toBeUndefined() + expect(nodes[0].data.parent_id).toBeUndefined() + expect(edges).toHaveLength(0) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('its own parent')) + warnSpy.mockRestore() + }) + // Regression for issue #208. it('restores edge connection points from the YAML link', () => { const yaml = ` diff --git a/frontend/src/utils/importYaml.ts b/frontend/src/utils/importYaml.ts index f220cc4..7153d28 100644 --- a/frontend/src/utils/importYaml.ts +++ b/frontend/src/utils/importYaml.ts @@ -158,6 +158,11 @@ export function parseYamlToCanvas( const parentId = labelToId.get(yn.parent.label) if (!parentId) { console.warn(`[importYaml] parent label not found: "${yn.parent.label}" — skipping relationship`) + } else if (parentId === node.id) { + // Parents resolve by label, so a node naming itself — or naming a + // duplicate label that maps back to it — would nest it inside itself + // and freeze it on the canvas (#370). + console.warn(`[importYaml] node "${yn.parent.label}" is its own parent — skipping relationship`) } else { // Set React Flow parentId for nesting node.data = { ...node.data, parent_id: parentId }