Prevent duplicate relationships; harden tree render against bad graphs
Root cause of the blank Jung tree: a child double-linked to the same parent
(and, generally, any cycle) made family-chart recurse forever.
Backend (the real fix):
- create_relationship now rejects an equivalent existing edge → 409.
parent_child is directional (parent→child); partnership/sibling match the
pair in either order. So you can't link the same two people the same way
twice. (GEDCOM import already deduped; manual creates didn't.)
Frontend (defense in depth so data can never blank the view):
- Tree view sanitizes the graph before rendering: dedupes parents/spouses,
drops self-links, and greedily breaks ancestor cycles (a person can't be
their own ancestor); children are derived from the kept edges. The render is
wrapped in try/catch and shows a note instead of a blank canvas, telling you
which conflicting links were skipped.
- Person page surfaces the 409 ("They're already linked that way.").
59 backend tests pass (incl. dup-rejection + reverse-parent-child allowed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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>
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user