From 07531041fba35816771ec52fa0a79d386f7f9c53 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 10 Aug 2026 14:22:22 -0500 Subject: [PATCH] fix(#1532): edge weight encodes cardinality, not master-part MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Line weight is binary and encodes cardinality only: thick when the foreign key constitutes the child's entire primary key (1:1), thin when the child has primary-key attributes beyond those the FK contributes (multi-valued) — newly declared or inherited from another FK. penwidth already followed this via multi; remove the misleading master-part conflation in the layout weight and drive it from the same predicate so the two never diverge. Rename-safe: multi compares the child's referencing columns to the child primary key (both child-column space), so a renamed FK that is the child's whole PK is correctly 1:1/thick. Adds a guardrail test (1:1, multi, master-part, renamed-1:1). --- src/datajoint/diagram.py | 17 ++- tests/integration/test_diagram_edge_weight.py | 125 ++++++++++++++++++ 2 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 tests/integration/test_diagram_edge_weight.py diff --git a/src/datajoint/diagram.py b/src/datajoint/diagram.py index 20cd34b58..35ca02d65 100644 --- a/src/datajoint/diagram.py +++ b/src/datajoint/diagram.py @@ -1540,19 +1540,24 @@ def make_dot(self): # 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. - src = edge.get_source() - dest = edge.get_destination() primary = str(edge.get("primary")) == "True" multi = str(edge.get("multi")) == "True" aliased = str(edge.get("aliased")) == "True" # Renamed FK → distinct color; others → the usual translucent black. edge.set_color("#FF8800" if aliased else "#00000040") edge.set_style("solid" if primary else "dashed") - dest_node_type = graph.nodes[dest].get("node_type") - master_part = dest_node_type is Part and dest.startswith(src + ".") - edge.set_weight(3 if master_part else 1) - edge.set_arrowhead("none") + # Line weight encodes cardinality, and only cardinality. `multi` is + # True when the child has primary-key attributes beyond those this + # foreign key contributes — whether newly declared or inherited from + # another foreign key — i.e. a one-to-many dependency, drawn thin. + # When the foreign key constitutes the child's *entire* primary key + # the dependency is 1:1, drawn thick. Master-part is NOT a weight: a + # part almost always adds a key attribute, so its edge is thin under + # this same rule. penwidth is the visible thickness; the layout + # `weight` hint follows the same predicate so the two never diverge. edge.set_penwidth(0.75 if multi else 2) + edge.set_weight(1 if multi else 3) + edge.set_arrowhead("none") # Group nodes into schema clusters (always on) if schema_map: diff --git a/tests/integration/test_diagram_edge_weight.py b/tests/integration/test_diagram_edge_weight.py new file mode 100644 index 000000000..e787b270e --- /dev/null +++ b/tests/integration/test_diagram_edge_weight.py @@ -0,0 +1,125 @@ +""" +Guards the diagram edge-weight (cardinality) rule (#1532). + +Line weight encodes cardinality only, and it is binary: +- **thick** (penwidth 2): the foreign key constitutes the child's *entire* + primary key -> a 1:1 dependency. +- **thin** (penwidth 0.75): the child has primary-key attributes beyond those + the foreign key contributes (newly declared, or inherited from another foreign + key) -> a one-to-many dependency. + +Master-part is NOT a weight: a part almost always adds a key attribute, so its +edge is thin under this same rule. This test pins that, since the historical +documentation inverted it ("thick = master-part"). +""" + +import time + +import pytest + +import datajoint as dj + +THICK = 2.0 +THIN = 0.75 + + +@pytest.fixture(scope="function") +def schema_by_backend(connection_by_backend, db_creds_by_backend): + backend = db_creds_by_backend["backend"] + test_id = str(int(time.time() * 1000))[-8:] + schema_name = f"djtest_edgewt_{backend}_{test_id}"[:64] + if connection_by_backend.is_connected: + try: + connection_by_backend.query( + f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}" + ) + except Exception: + pass + schema = dj.Schema(schema_name, connection=connection_by_backend) + yield schema + if connection_by_backend.is_connected: + try: + connection_by_backend.query( + f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}" + ) + except Exception: + pass + + +def _penwidth_by_dest(dot): + """Map each edge's destination-node tail -> penwidth (float).""" + out = {} + for edge in dot.get_edges(): + dest = edge.get_destination().strip('"').lower() + try: + pw = float(edge.get_penwidth()) + except (TypeError, ValueError): + pw = None + out.setdefault(dest, []).append((edge.get_source().strip('"').lower(), pw)) + return out + + +def _penwidth_for(edges_by_dest, dest_name): + matches = edges_by_dest.get(dest_name, []) + assert matches, f"no edge found into node {dest_name!r}; nodes: {list(edges_by_dest)}" + return matches + + +def test_edge_weight_encodes_cardinality(schema_by_backend): + if not dj.diagram.diagram_active: + pytest.skip("networkx/pydot not available") + + @schema_by_backend + class Parent(dj.Manual): + definition = """ + parent_id : int32 + """ + + class Part(dj.Part): + definition = """ + -> master + part_id : int32 + """ + + @schema_by_backend + class OneToOne(dj.Manual): + definition = """ + -> Parent + """ + + @schema_by_backend + class OneToMany(dj.Manual): + definition = """ + -> Parent + sub_id : int32 + """ + + @schema_by_backend + class RenamedOneToOne(dj.Manual): + # A renamed foreign key can still be 1:1: the renamed column is + # RenamedOneToOne's entire primary key, so the dependency is 1:1 -> thick. + # The rule must compare child columns to the child PK, not parent-PK + # names to child-PK names (which renaming would break). + definition = """ + -> Parent.proj(alt_parent_id='parent_id') + """ + + dot = dj.Diagram(schema_by_backend).make_dot() + edges = _penwidth_by_dest(dot) + + # 1:1 — the FK is OneToOne's entire primary key -> thick. + assert all( + pw == THICK for _, pw in _penwidth_for(edges, "onetoone") + ), f"1:1 dependency must be thick ({THICK}); edges={edges}" + # multi-valued — OneToMany adds `sub_id` -> thin. + assert all( + pw == THIN for _, pw in _penwidth_for(edges, "onetomany") + ), f"multi-valued dependency must be thin ({THIN}); edges={edges}" + # master -> part — the part adds `part_id` -> thin (NOT thick). + assert all( + pw == THIN for _, pw in _penwidth_for(edges, "parent.part") + ), f"master-part edge must be thin ({THIN}); it is not a 1:1 dependency; edges={edges}" + # renamed FK that is the child's whole primary key — still 1:1 -> thick. + assert all( + pw == THICK for _, pw in _penwidth_for(edges, "renamedonetoone") + ), f"a renamed 1:1 foreign key must be thick ({THICK}); the rule must be rename-safe; edges={edges}"