Prevent duplicate relationships; harden tree render against cycles #34

Merged
justin merged 1 commits from prevent-duplicate-links into main 2026-06-08 11:35:12 -04:00
4 changed files with 185 additions and 26 deletions
+33 -1
View File
@@ -4,7 +4,7 @@ Writes require editor rights; reads go through the privacy engine."""
import uuid
from datetime import UTC, datetime
from sqlalchemy import or_, select
from sqlalchemy import and_, or_, select
from sqlalchemy.ext.asyncio import AsyncSession
from app.models.enums import ParentChildQualifier, RelationshipType
@@ -49,6 +49,38 @@ async def create_relationship(
if not await _person_in_tree(session, pid, tree.id):
raise NotFound("person not found in this tree")
# Reject an equivalent existing edge so the same two people can't be linked
# the same way twice. parent_child is directional (parent -> child);
# partnership/sibling are symmetric, so match the pair in either order.
if type is RelationshipType.parent_child:
pair = and_(
Relationship.person_from_id == person_from_id,
Relationship.person_to_id == person_to_id,
)
else:
pair = or_(
and_(
Relationship.person_from_id == person_from_id,
Relationship.person_to_id == person_to_id,
),
and_(
Relationship.person_from_id == person_to_id,
Relationship.person_to_id == person_from_id,
),
)
existing = (
await session.execute(
select(Relationship.id).where(
Relationship.tree_id == tree.id,
Relationship.type == type,
Relationship.deleted_at.is_(None),
pair,
)
)
).scalar_one_or_none()
if existing is not None:
raise Conflict("these two people are already linked that way")
relationship = Relationship(
tree_id=tree.id,
type=type,
+65
View File
@@ -0,0 +1,65 @@
"""Duplicate relationships are rejected (no double-linking)."""
from tests.conftest import auth, register
async def _setup(client, email):
h = auth(await register(client, email))
tid = (await client.post("/api/v1/trees", json={"name": "T"}, headers=h)).json()["id"]
async def person(given):
return (
await client.post(f"/api/v1/trees/{tid}/persons", json={"given": given}, headers=h)
).json()["id"]
return h, tid, person
async def test_duplicate_parent_child_rejected(client):
h, tid, person = await _setup(client, "dup-pc@example.com")
karl = await person("Karl")
kid = await person("Kid")
body = {"type": "parent_child", "person_from_id": karl, "person_to_id": kid}
first = await client.post(f"/api/v1/trees/{tid}/relationships", json=body, headers=h)
assert first.status_code == 201
dup = await client.post(f"/api/v1/trees/{tid}/relationships", json=body, headers=h)
assert dup.status_code == 409
async def test_duplicate_partnership_either_direction_rejected(client):
h, tid, person = await _setup(client, "dup-sp@example.com")
a = await person("A")
b = await person("B")
first = await client.post(
f"/api/v1/trees/{tid}/relationships",
json={"type": "partnership", "person_from_id": a, "person_to_id": b},
headers=h,
)
assert first.status_code == 201
# Same couple, reversed order — still a duplicate.
dup = await client.post(
f"/api/v1/trees/{tid}/relationships",
json={"type": "partnership", "person_from_id": b, "person_to_id": a},
headers=h,
)
assert dup.status_code == 409
async def test_reverse_parent_child_is_allowed(client):
"""A->B as parent_child shouldn't block B->A (different meaning)."""
h, tid, person = await _setup(client, "dup-rev@example.com")
a = await person("A")
b = await person("B")
r1 = await client.post(
f"/api/v1/trees/{tid}/relationships",
json={"type": "parent_child", "person_from_id": a, "person_to_id": b},
headers=h,
)
r2 = await client.post(
f"/api/v1/trees/{tid}/relationships",
json={"type": "parent_child", "person_from_id": b, "person_to_id": a},
headers=h,
)
assert r1.status_code == 201 and r2.status_code == 201
@@ -150,6 +150,7 @@ export default function PersonDetailPage() {
const [relKind, setRelKind] = useState<"parent" | "child" | "partner" | "sibling">("parent");
const [relOther, setRelOther] = useState("");
const [relQual, setRelQual] = useState<Qualifier>("biological");
const [relErr, setRelErr] = useState<string | null>(null);
// Add-name form + inline edit.
const [nameType, setNameType] = useState("married");
@@ -363,9 +364,12 @@ export default function PersonDetailPage() {
async function addRel(e: React.FormEvent) {
e.preventDefault();
if (!relOther) return;
setRelErr(null);
if (await linkRelative(relOther)) {
setRelOther("");
load();
} else {
setRelErr("They're already linked that way.");
}
}
@@ -1142,6 +1146,7 @@ export default function PersonDetailPage() {
<Button type="submit">Link</Button>
</form>
)}
{relErr && <p className="text-sm text-red-600">{relErr}</p>}
</CardContent>
</Card>
+82 -25
View File
@@ -39,6 +39,7 @@ export default function TreePage() {
const [status, setStatus] = useState<"loading" | "empty" | "ready" | "error">("loading");
const [focusId, setFocusId] = useState<string | null>(null);
const [mode, setMode] = useState<Mode>("landscape");
const [renderNote, setRenderNote] = useState<string | null>(null);
useEffect(() => {
let cancelled = false;
@@ -108,11 +109,46 @@ export default function TreePage() {
if (status !== "ready" || mode === "fan" || !containerRef.current) return;
let cancelled = false;
(async () => {
// Only link to people that still exist — a soft-deleted person leaves
// dangling relationship rows, and family-chart breaks on an id with no
// matching datum. Filter them out so a deletion never blanks the tree.
// Sanitize the graph before handing it to family-chart, which recurses
// through parents and will blow the stack (blank tree) on a cycle — e.g. a
// person edited into being their own ancestor.
const alive = new Set(people.map((pp) => pp.id));
const keep = (ids: string[]) => ids.filter((id) => alive.has(id));
const ok = (ids: string[], self: string) =>
[...new Set(ids)].filter((id) => alive.has(id) && id !== self);
// Build an acyclic set of parent edges: skip any edge that would make a
// person their own ancestor. Children are derived from the kept edges so
// parent/child stays consistent.
const parentsMap = new Map<string, string[]>();
const childrenMap = new Map<string, string[]>();
let dropped = 0;
const isAncestorOf = (ancestor: string, of: string): boolean => {
const stack = [...(parentsMap.get(of) ?? [])];
const seen = new Set<string>();
while (stack.length) {
const n = stack.pop()!;
if (n === ancestor) return true;
if (seen.has(n)) continue;
seen.add(n);
for (const p of parentsMap.get(n) ?? []) stack.push(p);
}
return false;
};
for (const pp of people) {
const accepted: string[] = [];
for (const par of ok(parentsOf(pp.id), pp.id)) {
// Edge "pp has parent par" loops if pp is already an ancestor of par.
if (isAncestorOf(pp.id, par)) {
dropped++;
continue;
}
accepted.push(par);
parentsMap.set(pp.id, accepted);
childrenMap.set(par, [...(childrenMap.get(par) ?? []), pp.id]);
}
parentsMap.set(pp.id, accepted);
}
const data = people.map((pp) => {
const [fn, ln] = splitName(pp.primary_name);
return {
@@ -124,32 +160,47 @@ export default function TreePage() {
gender: pp.gender === "female" ? "F" : "M",
},
rels: {
spouses: keep(partnersOf(pp.id)),
parents: keep(parentsOf(pp.id)),
children: keep(childrenOf(pp.id)),
spouses: ok(partnersOf(pp.id), pp.id),
parents: parentsMap.get(pp.id) ?? [],
children: childrenMap.get(pp.id) ?? [],
},
};
});
const f3 = await import("family-chart");
if (cancelled || !containerRef.current) return;
containerRef.current.innerHTML = "";
const chart = f3.createChart(containerRef.current, data);
chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]);
if (mode === "portrait") chart.setOrientationVertical();
else chart.setOrientationHorizontal();
// Show enough generations that a recenter reveals grandparents + children.
chart.setAncestryDepth?.(3);
chart.setProgenyDepth?.(2);
// Default card click recenters the whole hourglass; sync focus for the
// "Open profile" link after every (re)build.
chart.setAfterUpdate?.(() => {
const md = chart.getMainDatum?.();
const id = md?.id ?? md?.data?.id;
if (id) setFocusId(id);
});
chartRef.current = chart;
if (focusId) chart.updateMainId(focusId);
chart.updateTree({ initial: true });
try {
containerRef.current.innerHTML = "";
const chart = f3.createChart(containerRef.current, data);
chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]);
if (mode === "portrait") chart.setOrientationVertical();
else chart.setOrientationHorizontal();
// Show enough generations that a recenter reveals grandparents + children.
chart.setAncestryDepth?.(3);
chart.setProgenyDepth?.(2);
// Default card click recenters the whole hourglass; sync focus for the
// "Open profile" link after every (re)build.
chart.setAfterUpdate?.(() => {
const md = chart.getMainDatum?.();
const id = md?.id ?? md?.data?.id;
if (id) setFocusId(id);
});
chartRef.current = chart;
if (focusId) chart.updateMainId(focusId);
chart.updateTree({ initial: true });
setRenderNote(
dropped > 0
? `Skipped ${dropped} conflicting parent link${dropped === 1 ? "" : "s"} (a person can't be their own ancestor). Open the people involved to fix the relationship.`
: null,
);
} catch (err) {
// Never leave a blank canvas — show a message and let them fix via the
// Family view / person pages.
console.error("tree render failed", err);
if (containerRef.current) containerRef.current.innerHTML = "";
setRenderNote(
"The tree couldn't be drawn — a relationship may be conflicting. Use the Family view to open the affected people and check their parents/children.",
);
}
})();
return () => {
cancelled = true;
@@ -242,6 +293,12 @@ export default function TreePage() {
)}
{status === "error" && <p className="text-[var(--muted)]">Could not render the tree.</p>}
{renderNote && mode !== "fan" && (
<p className="rounded-md border border-bronze/40 bg-bronze/[0.06] px-3 py-2 text-sm text-bronze">
{renderNote}
</p>
)}
{status === "ready" && mode === "fan" && focusId ? (
<div className="rounded-xl border border-[var(--border)] bg-[var(--surface)] p-4">
<FanChart