Prevent duplicate relationships; harden tree render against cycles #34
@@ -4,7 +4,7 @@ Writes require editor rights; reads go through the privacy engine."""
|
|||||||
import uuid
|
import uuid
|
||||||
from datetime import UTC, datetime
|
from datetime import UTC, datetime
|
||||||
|
|
||||||
from sqlalchemy import or_, select
|
from sqlalchemy import and_, or_, select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
from app.models.enums import ParentChildQualifier, RelationshipType
|
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):
|
if not await _person_in_tree(session, pid, tree.id):
|
||||||
raise NotFound("person not found in this tree")
|
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(
|
relationship = Relationship(
|
||||||
tree_id=tree.id,
|
tree_id=tree.id,
|
||||||
type=type,
|
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 [relKind, setRelKind] = useState<"parent" | "child" | "partner" | "sibling">("parent");
|
||||||
const [relOther, setRelOther] = useState("");
|
const [relOther, setRelOther] = useState("");
|
||||||
const [relQual, setRelQual] = useState<Qualifier>("biological");
|
const [relQual, setRelQual] = useState<Qualifier>("biological");
|
||||||
|
const [relErr, setRelErr] = useState<string | null>(null);
|
||||||
|
|
||||||
// Add-name form + inline edit.
|
// Add-name form + inline edit.
|
||||||
const [nameType, setNameType] = useState("married");
|
const [nameType, setNameType] = useState("married");
|
||||||
@@ -363,9 +364,12 @@ export default function PersonDetailPage() {
|
|||||||
async function addRel(e: React.FormEvent) {
|
async function addRel(e: React.FormEvent) {
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
if (!relOther) return;
|
if (!relOther) return;
|
||||||
|
setRelErr(null);
|
||||||
if (await linkRelative(relOther)) {
|
if (await linkRelative(relOther)) {
|
||||||
setRelOther("");
|
setRelOther("");
|
||||||
load();
|
load();
|
||||||
|
} else {
|
||||||
|
setRelErr("They're already linked that way.");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1142,6 +1146,7 @@ export default function PersonDetailPage() {
|
|||||||
<Button type="submit">Link</Button>
|
<Button type="submit">Link</Button>
|
||||||
</form>
|
</form>
|
||||||
)}
|
)}
|
||||||
|
{relErr && <p className="text-sm text-red-600">{relErr}</p>}
|
||||||
</CardContent>
|
</CardContent>
|
||||||
</Card>
|
</Card>
|
||||||
|
|
||||||
|
|||||||
@@ -39,6 +39,7 @@ export default function TreePage() {
|
|||||||
const [status, setStatus] = useState<"loading" | "empty" | "ready" | "error">("loading");
|
const [status, setStatus] = useState<"loading" | "empty" | "ready" | "error">("loading");
|
||||||
const [focusId, setFocusId] = useState<string | null>(null);
|
const [focusId, setFocusId] = useState<string | null>(null);
|
||||||
const [mode, setMode] = useState<Mode>("landscape");
|
const [mode, setMode] = useState<Mode>("landscape");
|
||||||
|
const [renderNote, setRenderNote] = useState<string | null>(null);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
let cancelled = false;
|
let cancelled = false;
|
||||||
@@ -108,11 +109,46 @@ export default function TreePage() {
|
|||||||
if (status !== "ready" || mode === "fan" || !containerRef.current) return;
|
if (status !== "ready" || mode === "fan" || !containerRef.current) return;
|
||||||
let cancelled = false;
|
let cancelled = false;
|
||||||
(async () => {
|
(async () => {
|
||||||
// Only link to people that still exist — a soft-deleted person leaves
|
// Sanitize the graph before handing it to family-chart, which recurses
|
||||||
// dangling relationship rows, and family-chart breaks on an id with no
|
// through parents and will blow the stack (blank tree) on a cycle — e.g. a
|
||||||
// matching datum. Filter them out so a deletion never blanks the tree.
|
// person edited into being their own ancestor.
|
||||||
const alive = new Set(people.map((pp) => pp.id));
|
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 data = people.map((pp) => {
|
||||||
const [fn, ln] = splitName(pp.primary_name);
|
const [fn, ln] = splitName(pp.primary_name);
|
||||||
return {
|
return {
|
||||||
@@ -124,32 +160,47 @@ export default function TreePage() {
|
|||||||
gender: pp.gender === "female" ? "F" : "M",
|
gender: pp.gender === "female" ? "F" : "M",
|
||||||
},
|
},
|
||||||
rels: {
|
rels: {
|
||||||
spouses: keep(partnersOf(pp.id)),
|
spouses: ok(partnersOf(pp.id), pp.id),
|
||||||
parents: keep(parentsOf(pp.id)),
|
parents: parentsMap.get(pp.id) ?? [],
|
||||||
children: keep(childrenOf(pp.id)),
|
children: childrenMap.get(pp.id) ?? [],
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
const f3 = await import("family-chart");
|
const f3 = await import("family-chart");
|
||||||
if (cancelled || !containerRef.current) return;
|
if (cancelled || !containerRef.current) return;
|
||||||
containerRef.current.innerHTML = "";
|
try {
|
||||||
const chart = f3.createChart(containerRef.current, data);
|
containerRef.current.innerHTML = "";
|
||||||
chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]);
|
const chart = f3.createChart(containerRef.current, data);
|
||||||
if (mode === "portrait") chart.setOrientationVertical();
|
chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]);
|
||||||
else chart.setOrientationHorizontal();
|
if (mode === "portrait") chart.setOrientationVertical();
|
||||||
// Show enough generations that a recenter reveals grandparents + children.
|
else chart.setOrientationHorizontal();
|
||||||
chart.setAncestryDepth?.(3);
|
// Show enough generations that a recenter reveals grandparents + children.
|
||||||
chart.setProgenyDepth?.(2);
|
chart.setAncestryDepth?.(3);
|
||||||
// Default card click recenters the whole hourglass; sync focus for the
|
chart.setProgenyDepth?.(2);
|
||||||
// "Open profile" link after every (re)build.
|
// Default card click recenters the whole hourglass; sync focus for the
|
||||||
chart.setAfterUpdate?.(() => {
|
// "Open profile" link after every (re)build.
|
||||||
const md = chart.getMainDatum?.();
|
chart.setAfterUpdate?.(() => {
|
||||||
const id = md?.id ?? md?.data?.id;
|
const md = chart.getMainDatum?.();
|
||||||
if (id) setFocusId(id);
|
const id = md?.id ?? md?.data?.id;
|
||||||
});
|
if (id) setFocusId(id);
|
||||||
chartRef.current = chart;
|
});
|
||||||
if (focusId) chart.updateMainId(focusId);
|
chartRef.current = chart;
|
||||||
chart.updateTree({ initial: true });
|
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 () => {
|
return () => {
|
||||||
cancelled = true;
|
cancelled = true;
|
||||||
@@ -242,6 +293,12 @@ export default function TreePage() {
|
|||||||
)}
|
)}
|
||||||
{status === "error" && <p className="text-[var(--muted)]">Could not render the tree.</p>}
|
{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 ? (
|
{status === "ready" && mode === "fan" && focusId ? (
|
||||||
<div className="rounded-xl border border-[var(--border)] bg-[var(--surface)] p-4">
|
<div className="rounded-xl border border-[var(--border)] bg-[var(--surface)] p-4">
|
||||||
<FanChart
|
<FanChart
|
||||||
|
|||||||
Reference in New Issue
Block a user