From 724433baf17f517749fc24d8d52a43bc22058dd7 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Wed, 19 Aug 2026 17:40:35 -0500 Subject: [PATCH] fix(diagram): collapsed-to-collapsed edges carry no per-foreign-key style 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. --- src/datajoint/diagram.py | 55 ++++- tests/integration/test_diagram_bundle_edge.py | 211 ++++++++++++++++++ 2 files changed, 257 insertions(+), 9 deletions(-) create mode 100644 tests/integration/test_diagram_bundle_edge.py diff --git a/src/datajoint/diagram.py b/src/datajoint/diagram.py index 9dc9e50cc..6ff60d061 100644 --- a/src/datajoint/diagram.py +++ b/src/datajoint/diagram.py @@ -1372,19 +1372,39 @@ def _apply_collapse(self, graph: nx.MultiDiGraph) -> tuple[nx.MultiDiGraph, dict else: new_graph.add_node(new_node, **graph.nodes[old_node]) - # Add edges (avoiding self-loops). Edges touching a collapsed schema - # node are merged to a single arrow per node pair (many tables → one - # box), but parallel foreign keys between two *expanded* tables (e.g. two - # renamed FKs to the same parent) are preserved as distinct edges by key. + # Add edges (avoiding self-loops). Edges touching a collapsed node are + # merged to a single edge per node pair (many tables → one box), but + # parallel foreign keys between two *expanded* tables (e.g. two renamed + # FKs to the same parent) are preserved as distinct edges by key. for src, dest, key, data in graph.edges(keys=True, data=True): new_src = node_mapping[src] new_dest = node_mapping[dest] if new_src == new_dest: continue - touches_collapsed = new_src in collapsed_counts or new_dest in collapsed_counts - if touches_collapsed: - # Many tables → one schema box: collapse to a single arrow. + src_collapsed = new_src in collapsed_counts + dest_collapsed = new_dest in collapsed_counts + if src_collapsed or dest_collapsed: + # At least one endpoint stands for several tables, so several + # foreign keys can land on the same node pair; keep one edge. if not new_graph.has_edge(new_src, new_dest): + if src_collapsed and dest_collapsed: + # Both ends are groups: this edge is a *bundle*, standing for + # every foreign key between the two sets of tables. It must + # not carry any single member's per-FK properties -- + # cardinality (`multi`), primary-vs-secondary (`primary`) and + # renaming (`aliased`) describe one foreign key and say + # nothing about a set of them. Inheriting them from whichever + # member was visited first made the drawn style depend on + # traversal order. Rendering gives every bundle edge the same + # appearance instead. + # + # Note this is about collapsed *nodes*, not schemas: a + # collapsed node may stand for any subset of tables (e.g. + # `Diagram(schema).collapse() + Diagram(OneTable)`). + data = {k: v for k, v in data.items() if k not in ("primary", "multi", "aliased")} + data["bundle"] = True + # With one end expanded the edge still names a single table, so + # its style stays meaningful and is preserved as-is. new_graph.add_edge(new_src, new_dest, **data) else: # Both endpoints expanded: keep each FK as its own keyed edge. @@ -1656,6 +1676,18 @@ def _master_of(part_stripped): # pydot edge — to_pydot stringifies the edge data, so booleans arrive # as "True"/"False". This is parallel-edge-safe: each FK between the # same pair of tables is its own pydot edge. + # A bundle edge is incident to a collapsed node -- one standing for + # a set of tables -- and represents every foreign key crossing to the + # other end. Cardinality and primary-vs-secondary are properties of an + # individual foreign key, so they are not claimed here: every bundle + # edge is drawn identically, whatever its members are. + if str(edge.get("bundle")) == "True": + edge.set_color(theme["edge"] + theme["edge_alpha"]) + edge.set_style("solid") + edge.set_penwidth(2) + edge.set_weight(3) + edge.set_arrowhead("none") + continue primary = str(edge.get("primary")) == "True" multi = str(edge.get("multi")) == "True" aliased = str(edge.get("aliased")) == "True" @@ -1964,8 +1996,13 @@ def node_line(node, data, indent, label): link_styles = [] for idx, (src, dest, data) in enumerate(graph.edges(data=True)): lines.append(f" {safe(src)} --> {safe(dest)}") - color = theme["edge_renamed"] if data.get("aliased") else theme["edge"] - width = "1px" if data.get("multi") else "2px" + # Bundle edges (see _apply_collapse) claim no cardinality, so they + # all render alike. + if data.get("bundle"): + color, width = theme["edge"], "2px" + else: + color = theme["edge_renamed"] if data.get("aliased") else theme["edge"] + width = "1px" if data.get("multi") else "2px" link_styles.append(f" linkStyle {idx} stroke:{color},stroke-width:{width}") lines.extend(link_styles) diff --git a/tests/integration/test_diagram_bundle_edge.py b/tests/integration/test_diagram_bundle_edge.py new file mode 100644 index 000000000..e8d42de6e --- /dev/null +++ b/tests/integration/test_diagram_bundle_edge.py @@ -0,0 +1,211 @@ +""" +Guards the collapsed-edge (bundle) rule. + +A collapsed node stands for a set of tables -- a whole schema, or any subset of +one, since expanding a table back out of a collapsed diagram leaves the rest +collapsed. When *both* ends of an edge are collapsed nodes, the edge is a +*bundle*: it stands for every foreign key between the two sets of tables, so the +per-foreign-key properties do not apply to it: + +- cardinality (thick = 1:1, thin = one-to-many) is defined for one foreign key; +- solid-vs-dashed distinguishes a primary from a secondary foreign key; +- the renamed-foreign-key hue marks one renamed reference. + +A bundle may mix all of these, so it claims none of them: every bundle edge is +drawn identically, whether its endpoints stand for whole schemas or parts of +them. Before this rule the collapsed edge inherited whichever member foreign key +``_apply_collapse`` happened to visit first, which made the drawn style depend on +graph traversal order -- the same schemas could render a bundle solid in one +process and dashed in another, and a two-foreign-key bundle could come out either +thick or thin. + +An edge with only *one* collapsed end is not a bundle: it still names a single +table, so its style remains meaningful and is preserved. +""" + +import time + +import pytest + +import datajoint as dj + +BUNDLE_PENWIDTH = 2.0 +THIN = 0.75 + + +@pytest.fixture(scope="function") +def two_schemas(connection_by_backend, db_creds_by_backend): + backend = db_creds_by_backend["backend"] + test_id = str(int(time.time() * 1000))[-8:] + names = [f"djtest_bundle_{backend}_{test_id}_{i}"[:64] for i in (0, 1)] + + def drop(): + if not connection_by_backend.is_connected: + return + for name in reversed(names): + try: + connection_by_backend.query(f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(name)}") + except Exception: + pass + + drop() + schemas = [dj.Schema(name, connection=connection_by_backend) for name in names] + yield schemas + drop() + + +def _bundle_edges(dot): + """Every edge in a collapsed diagram, as (source, dest, penwidth, style).""" + out = [] + for edge in dot.get_edges(): + try: + pw = float(edge.get_penwidth()) + except (TypeError, ValueError): + pw = None + out.append( + ( + edge.get_source().strip('"').lower(), + edge.get_destination().strip('"').lower(), + pw, + (edge.get_style() or "solid").strip('"'), + ) + ) + return out + + +def test_bundle_edges_are_uniform(two_schemas): + """A bundle mixing every per-FK property still renders as one uniform edge.""" + if not dj.diagram.diagram_active: + pytest.skip("networkx/pydot not available") + + upstream, downstream = two_schemas + + @upstream + class Root(dj.Manual): + definition = """ + root_id : int32 + """ + + @upstream + class Tag(dj.Manual): + definition = """ + tag_id : int32 + """ + + @downstream + class Mixed(dj.Manual): + # The bundle upstream -> downstream deliberately contains one primary, + # 1:1-eligible foreign key and one secondary (nullable) one, so an + # order-sensitive implementation could pick either style. + definition = """ + -> Root + --- + -> [nullable] Tag + """ + + @downstream + class Leaf(dj.Manual): + definition = """ + -> Mixed + leaf_id : int32 + """ + + collapsed = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse() + edges = _bundle_edges(collapsed.make_dot()) + assert edges, "collapsed diagram produced no edges" + + # Every edge here crosses between two collapsed schema nodes, so all are bundles. + for source, dest, penwidth, style in edges: + assert penwidth == BUNDLE_PENWIDTH, ( + f"bundle edge {source} -> {dest} has penwidth {penwidth}; every bundle " + f"edge must be {BUNDLE_PENWIDTH} regardless of its members' cardinality" + ) + assert "dashed" not in style, ( + f"bundle edge {source} -> {dest} is {style!r}; a bundle mixing primary " + "and secondary foreign keys must not claim either" + ) + + +def test_bundle_style_is_order_independent(two_schemas): + """Declaring the bundle's members in the opposite order changes nothing.""" + if not dj.diagram.diagram_active: + pytest.skip("networkx/pydot not available") + + upstream, downstream = two_schemas + + @upstream + class Root(dj.Manual): + definition = """ + root_id : int32 + """ + + @upstream + class Tag(dj.Manual): + definition = """ + tag_id : int32 + """ + + # Secondary reference declared on the table created *first* this time, so the + # two foreign keys in the bundle are reached in the opposite order. + @downstream + class SecondaryFirst(dj.Manual): + definition = """ + -> Tag + --- + -> [nullable] Root + """ + + @downstream + class PrimaryLater(dj.Manual): + definition = """ + -> Root + """ + + collapsed = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse() + widths = {pw for _, _, pw, _ in _bundle_edges(collapsed.make_dot())} + styles = {"dashed" in style for _, _, _, style in _bundle_edges(collapsed.make_dot())} + + assert widths == {BUNDLE_PENWIDTH}, ( + f"bundle penwidths {widths} depend on declaration order; expected only " f"{BUNDLE_PENWIDTH}" + ) + assert styles == {False}, "bundle solid/dashed depends on declaration order" + + +def test_one_collapsed_end_preserves_edge_style(two_schemas): + """An edge with a single expanded endpoint keeps its own style. + + Only edges between two collapsed nodes are bundles. When one end is a real + table the edge still describes a specific foreign key, so its cardinality and + primary-vs-secondary styling stay. + """ + if not dj.diagram.diagram_active: + pytest.skip("networkx/pydot not available") + + upstream, downstream = two_schemas + + @upstream + class Root(dj.Manual): + definition = """ + root_id : int32 + """ + + @downstream + class Multi(dj.Manual): + # Adds its own key attribute -> multi-valued -> thin, which differs from the + # uniform bundle weight, so a bundle rule leaking in here would show up. + definition = """ + -> Root + sub_id : int32 + """ + + diagram = (dj.Diagram(upstream) + dj.Diagram(downstream)).collapse() + dj.Diagram(Multi) + edges = {(src, dest): (pw, style) for src, dest, pw, style in _bundle_edges(diagram.make_dot())} + assert edges, "partially collapsed diagram produced no edges" + + into_multi = [(pw, style) for (src, dest), (pw, style) in edges.items() if dest.endswith("multi")] + assert into_multi, f"expected an edge into the expanded Multi table; got {list(edges)}" + for penwidth, style in into_multi: + assert penwidth == THIN, ( + "an edge with one expanded end is not a bundle: it must keep its own " + f"cardinality weight ({THIN} for multi-valued), got {penwidth}" + )