Skip to content

Commit 8e6ef31

Browse files
dimitri-yatsenkodimitri-yatsenko
authored
fix(diagram): collapsed-to-collapsed edges carry no per-foreign-key style (#1545)
A collapsed node stands for a set of tables, so an edge between two collapsed nodes represents a bundle: every foreign key between the two sets. `_apply_collapse` gave that edge the attributes of whichever member it visited first, with no aggregation, so cardinality (`multi`), primary-vs-secondary (`primary`) and renaming (`aliased`) -- all properties of a single foreign key -- were attributed to a set of them, and the drawn style depended on graph traversal order. Two schemas with identical structure but opposite declaration order rendered the same bundle as penwidth 2 solid and as penwidth 0.75 dashed. Bundle edges are now marked and rendered uniformly (solid, penwidth 2) in both the graphviz and mermaid paths. This is about collapsed nodes, not schemas: a collapsed node may stand for any subset of tables, as with `Diagram(schema).collapse() + Diagram(OneTable)`. An edge with only one collapsed end is unchanged. It still names a single table, so its cardinality and primary-vs-secondary styling remain meaningful and are preserved. Extends #1533, which established that weight encodes cardinality and only cardinality; a bundle has no single cardinality to report. Surfaced while adding a generator for the diagrams in datajoint-docs#265.
1 parent f0bc987 commit 8e6ef31

2 files changed

Lines changed: 257 additions & 9 deletions

File tree

‎src/datajoint/diagram.py‎

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,19 +1372,39 @@ def _apply_collapse(self, graph: nx.MultiDiGraph) -> tuple[nx.MultiDiGraph, dict
13721372
else:
13731373
new_graph.add_node(new_node, **graph.nodes[old_node])
13741374

1375-
# Add edges (avoiding self-loops). Edges touching a collapsed schema
1376-
# node are merged to a single arrow per node pair (many tables → one
1377-
# box), but parallel foreign keys between two *expanded* tables (e.g. two
1378-
# renamed FKs to the same parent) are preserved as distinct edges by key.
1375+
# Add edges (avoiding self-loops). Edges touching a collapsed node are
1376+
# merged to a single edge per node pair (many tables → one box), but
1377+
# parallel foreign keys between two *expanded* tables (e.g. two renamed
1378+
# FKs to the same parent) are preserved as distinct edges by key.
13791379
for src, dest, key, data in graph.edges(keys=True, data=True):
13801380
new_src = node_mapping[src]
13811381
new_dest = node_mapping[dest]
13821382
if new_src == new_dest:
13831383
continue
1384-
touches_collapsed = new_src in collapsed_counts or new_dest in collapsed_counts
1385-
if touches_collapsed:
1386-
# Many tables → one schema box: collapse to a single arrow.
1384+
src_collapsed = new_src in collapsed_counts
1385+
dest_collapsed = new_dest in collapsed_counts
1386+
if src_collapsed or dest_collapsed:
1387+
# At least one endpoint stands for several tables, so several
1388+
# foreign keys can land on the same node pair; keep one edge.
13871389
if not new_graph.has_edge(new_src, new_dest):
1390+
if src_collapsed and dest_collapsed:
1391+
# Both ends are groups: this edge is a *bundle*, standing for
1392+
# every foreign key between the two sets of tables. It must
1393+
# not carry any single member's per-FK properties --
1394+
# cardinality (`multi`), primary-vs-secondary (`primary`) and
1395+
# renaming (`aliased`) describe one foreign key and say
1396+
# nothing about a set of them. Inheriting them from whichever
1397+
# member was visited first made the drawn style depend on
1398+
# traversal order. Rendering gives every bundle edge the same
1399+
# appearance instead.
1400+
#
1401+
# Note this is about collapsed *nodes*, not schemas: a
1402+
# collapsed node may stand for any subset of tables (e.g.
1403+
# `Diagram(schema).collapse() + Diagram(OneTable)`).
1404+
data = {k: v for k, v in data.items() if k not in ("primary", "multi", "aliased")}
1405+
data["bundle"] = True
1406+
# With one end expanded the edge still names a single table, so
1407+
# its style stays meaningful and is preserved as-is.
13881408
new_graph.add_edge(new_src, new_dest, **data)
13891409
else:
13901410
# Both endpoints expanded: keep each FK as its own keyed edge.
@@ -1656,6 +1676,18 @@ def _master_of(part_stripped):
16561676
# pydot edge — to_pydot stringifies the edge data, so booleans arrive
16571677
# as "True"/"False". This is parallel-edge-safe: each FK between the
16581678
# same pair of tables is its own pydot edge.
1679+
# A bundle edge is incident to a collapsed node -- one standing for
1680+
# a set of tables -- and represents every foreign key crossing to the
1681+
# other end. Cardinality and primary-vs-secondary are properties of an
1682+
# individual foreign key, so they are not claimed here: every bundle
1683+
# edge is drawn identically, whatever its members are.
1684+
if str(edge.get("bundle")) == "True":
1685+
edge.set_color(theme["edge"] + theme["edge_alpha"])
1686+
edge.set_style("solid")
1687+
edge.set_penwidth(2)
1688+
edge.set_weight(3)
1689+
edge.set_arrowhead("none")
1690+
continue
16591691
primary = str(edge.get("primary")) == "True"
16601692
multi = str(edge.get("multi")) == "True"
16611693
aliased = str(edge.get("aliased")) == "True"
@@ -1964,8 +1996,13 @@ def node_line(node, data, indent, label):
19641996
link_styles = []
19651997
for idx, (src, dest, data) in enumerate(graph.edges(data=True)):
19661998
lines.append(f" {safe(src)} --> {safe(dest)}")
1967-
color = theme["edge_renamed"] if data.get("aliased") else theme["edge"]
1968-
width = "1px" if data.get("multi") else "2px"
1999+
# Bundle edges (see _apply_collapse) claim no cardinality, so they
2000+
# all render alike.
2001+
if data.get("bundle"):
2002+
color, width = theme["edge"], "2px"
2003+
else:
2004+
color = theme["edge_renamed"] if data.get("aliased") else theme["edge"]
2005+
width = "1px" if data.get("multi") else "2px"
19692006
link_styles.append(f" linkStyle {idx} stroke:{color},stroke-width:{width}")
19702007
lines.extend(link_styles)
19712008

‎tests/integration/test_diagram_bundle_edge.py‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
"""
2+
Guards the collapsed-edge (bundle) rule.
3+
4+
A collapsed node stands for a set of tables -- a whole schema, or any subset of
5+
one, since expanding a table back out of a collapsed diagram leaves the rest
6+
collapsed. When *both* ends of an edge are collapsed nodes, the edge is a
7+
*bundle*: it stands for every foreign key between the two sets of tables, so the
8+
per-foreign-key properties do not apply to it:
9+
10+
- cardinality (thick = 1:1, thin = one-to-many) is defined for one foreign key;
11+
- solid-vs-dashed distinguishes a primary from a secondary foreign key;
12+
- the renamed-foreign-key hue marks one renamed reference.
13+
14+
A bundle may mix all of these, so it claims none of them: every bundle edge is
15+
drawn identically, whether its endpoints stand for whole schemas or parts of
16+
them. Before this rule the collapsed edge inherited whichever member foreign key
17+
``_apply_collapse`` happened to visit first, which made the drawn style depend on
18+
graph traversal order -- the same schemas could render a bundle solid in one
19+
process and dashed in another, and a two-foreign-key bundle could come out either
20+
thick or thin.
21+
22+
An edge with only *one* collapsed end is not a bundle: it still names a single
23+
table, so its style remains meaningful and is preserved.
24+
"""
25+
26+
import time
27+
28+
import pytest
29+
30+
import datajoint as dj
31+
32+
BUNDLE_PENWIDTH = 2.0
33+
THIN = 0.75
34+
35+
36+
@pytest.fixture(scope="function")
37+
def two_schemas(connection_by_backend, db_creds_by_backend):
38+
backend = db_creds_by_backend["backend"]
39+
test_id = str(int(time.time() * 1000))[-8:]
40+
names = [f"djtest_bundle_{backend}_{test_id}_{i}"[:64] for i in (0, 1)]
41+
42+
def drop():
43+
if not connection_by_backend.is_connected:
44+
return
45+
for name in reversed(names):
46+
try:
47+
connection_by_backend.query(f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(name)}")
48+
except Exception:
49+
pass
50+
51+
drop()
52+
schemas = [dj.Schema(name, connection=connection_by_backend) for name in names]
53+
yield schemas
54+
drop()
55+
56+
57+
def _bundle_edges(dot):
58+
"""Every edge in a collapsed diagram, as (source, dest, penwidth, style)."""
59+
out = []
60+
for edge in dot.get_edges():
61+
try:
62+
pw = float(edge.get_penwidth())
63+
except (TypeError, ValueError):
64+
pw = None
65+
out.append(
66+
(
67+
edge.get_source().strip('"').lower(),
68+
edge.get_destination().strip('"').lower(),
69+
pw,
70+
(edge.get_style() or "solid").strip('"'),
71+
)
72+
)
73+
return out
74+
75+
76+
def test_bundle_edges_are_uniform(two_schemas):
77+
"""A bundle mixing every per-FK property still renders as one uniform edge."""
78+
if not dj.diagram.diagram_active:
79+
pytest.skip("networkx/pydot not available")
80+
81+
upstream, downstream = two_schemas
82+
83+
@upstream
84+
class Root(dj.Manual):
85+
definition = """
86+
root_id : int32
87+
"""
88+
89+
@upstream
90+
class Tag(dj.Manual):
91+
definition = """
92+
tag_id : int32
93+
"""
94+
95+
@downstream
96+
class Mixed(dj.Manual):
97+
# The bundle upstream -> downstream deliberately contains one primary,
98+
# 1:1-eligible foreign key and one secondary (nullable) one, so an
99+
# order-sensitive implementation could pick either style.
100+
definition = """
101+
-> Root
102+
---
103+
-> [nullable] Tag
104+
"""
105+
106+
@downstream
107+
class Leaf(dj.Manual):
108+
definition = """
109+
-> Mixed
110+
leaf_id : int32
111+
"""
112+
113+
collapsed = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse()
114+
edges = _bundle_edges(collapsed.make_dot())
115+
assert edges, "collapsed diagram produced no edges"
116+
117+
# Every edge here crosses between two collapsed schema nodes, so all are bundles.
118+
for source, dest, penwidth, style in edges:
119+
assert penwidth == BUNDLE_PENWIDTH, (
120+
f"bundle edge {source} -> {dest} has penwidth {penwidth}; every bundle "
121+
f"edge must be {BUNDLE_PENWIDTH} regardless of its members' cardinality"
122+
)
123+
assert "dashed" not in style, (
124+
f"bundle edge {source} -> {dest} is {style!r}; a bundle mixing primary "
125+
"and secondary foreign keys must not claim either"
126+
)
127+
128+
129+
def test_bundle_style_is_order_independent(two_schemas):
130+
"""Declaring the bundle's members in the opposite order changes nothing."""
131+
if not dj.diagram.diagram_active:
132+
pytest.skip("networkx/pydot not available")
133+
134+
upstream, downstream = two_schemas
135+
136+
@upstream
137+
class Root(dj.Manual):
138+
definition = """
139+
root_id : int32
140+
"""
141+
142+
@upstream
143+
class Tag(dj.Manual):
144+
definition = """
145+
tag_id : int32
146+
"""
147+
148+
# Secondary reference declared on the table created *first* this time, so the
149+
# two foreign keys in the bundle are reached in the opposite order.
150+
@downstream
151+
class SecondaryFirst(dj.Manual):
152+
definition = """
153+
-> Tag
154+
---
155+
-> [nullable] Root
156+
"""
157+
158+
@downstream
159+
class PrimaryLater(dj.Manual):
160+
definition = """
161+
-> Root
162+
"""
163+
164+
collapsed = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse()
165+
widths = {pw for _, _, pw, _ in _bundle_edges(collapsed.make_dot())}
166+
styles = {"dashed" in style for _, _, _, style in _bundle_edges(collapsed.make_dot())}
167+
168+
assert widths == {BUNDLE_PENWIDTH}, (
169+
f"bundle penwidths {widths} depend on declaration order; expected only " f"{BUNDLE_PENWIDTH}"
170+
)
171+
assert styles == {False}, "bundle solid/dashed depends on declaration order"
172+
173+
174+
def test_one_collapsed_end_preserves_edge_style(two_schemas):
175+
"""An edge with a single expanded endpoint keeps its own style.
176+
177+
Only edges between two collapsed nodes are bundles. When one end is a real
178+
table the edge still describes a specific foreign key, so its cardinality and
179+
primary-vs-secondary styling stay.
180+
"""
181+
if not dj.diagram.diagram_active:
182+
pytest.skip("networkx/pydot not available")
183+
184+
upstream, downstream = two_schemas
185+
186+
@upstream
187+
class Root(dj.Manual):
188+
definition = """
189+
root_id : int32
190+
"""
191+
192+
@downstream
193+
class Multi(dj.Manual):
194+
# Adds its own key attribute -> multi-valued -> thin, which differs from the
195+
# uniform bundle weight, so a bundle rule leaking in here would show up.
196+
definition = """
197+
-> Root
198+
sub_id : int32
199+
"""
200+
201+
diagram = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse() + dj.Diagram(Multi)
202+
edges = {(src, dest): (pw, style) for src, dest, pw, style in _bundle_edges(diagram.make_dot())}
203+
assert edges, "partially collapsed diagram produced no edges"
204+
205+
into_multi = [(pw, style) for (src, dest), (pw, style) in edges.items() if dest.endswith("multi")]
206+
assert into_multi, f"expected an edge into the expanded Multi table; got {list(edges)}"
207+
for penwidth, style in into_multi:
208+
assert penwidth == THIN, (
209+
"an edge with one expanded end is not a bundle: it must keep its own "
210+
f"cardinality weight ({THIN} for multi-valued), got {penwidth}"
211+
)

0 commit comments

Comments
 (0)