Skip to content

Commit 55780f2

Browse files
UI: Fix Graph layout for TaskGroup tasks wired to external nodes (#67720)
Open ``@task_group`` rendered with vertically-stacked internals and edges crossing the boundary whenever an internal task had a direct dependency on a node outside the group (an "escape edge" that bypassed the group's entry/exit interface). Dag execution was unaffected. Two underlying issues, both in the ELK graph-layout refactor from #65031: 1. ``hasUniformExternalConnectivity`` was too lenient — it fired whenever externally-connected children separately shared the same external sources OR the same external targets, instead of the canonical fan-in/fan-out pattern where every child has the same full ``(sources, targets)`` profile. On mixed-profile groups (entry + exits), it incorrectly fired and collapsed the author's deliberately- wired escape edges into a single group-level edge, hiding the intent. 2. When the optimisation did fire on an open group, ``rewriteGroupEdges`` was tuned for closed groups and dropped the group's internal edges too, leaving ELK with no internal-layout information for the children (the visible symptom in #67714). Fix: tighten ``hasUniformExternalConnectivity`` to require the full profile to match across externally-connected children, and add a ``preserveInternal`` option to ``rewriteGroupEdges`` so the canonical fan-in/fan-out path keeps internals intact. Closes: #67714
1 parent 3532abd commit 55780f2

2 files changed

Lines changed: 425 additions & 61 deletions

File tree

Lines changed: 305 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,305 @@
1+
/*!
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
import { describe, expect, it } from "vitest";
20+
21+
import type { EdgeResponse, NodeResponse } from "openapi/requests/types.gen";
22+
23+
import { generateElkGraph, hasUniformExternalConnectivity } from "./elkGraphUtils";
24+
import type { FormattedNode } from "./elkGraphUtils";
25+
26+
// Minimal NodeResponse builder — fills the fields the layout pipeline actually reads.
27+
const buildNode = (overrides: Partial<NodeResponse> & Pick<NodeResponse, "id" | "label">): NodeResponse => ({
28+
asset_condition_type: null,
29+
children: null,
30+
is_mapped: null,
31+
operator: null,
32+
setup_teardown_type: null,
33+
tooltip: null,
34+
type: "task",
35+
...overrides,
36+
});
37+
38+
const buildEdge = (sourceId: string, targetId: string): EdgeResponse => ({
39+
is_setup_teardown: null,
40+
label: null,
41+
source_id: sourceId,
42+
target_id: targetId,
43+
});
44+
45+
describe("hasUniformExternalConnectivity", () => {
46+
it("returns false for the vanilla TaskGroup shape (only entry/exit cross the boundary)", () => {
47+
// ``a1`` has external source {start}; ``group_done`` has external target
48+
// {final_task}. Their profiles differ — not canonical fan-in/fan-out.
49+
const edges = [buildEdge("start", "a1"), buildEdge("group_done", "final_task")];
50+
const result = hasUniformExternalConnectivity(new Set(["a1", "branch", "a2", "a3", "group_done"]), edges);
51+
52+
expect(result).toBe(false);
53+
});
54+
55+
it("returns false when externally-connected children have mixed profiles (entry + exits)", () => {
56+
// The #67714 bug-trigger shape: a1 is the "entry" with external source
57+
// {start}, while a2/a3/group_done are "exits" with external target
58+
// {final_task}. Profiles differ → not canonical → return false so the
59+
// author's explicit escape edges remain visible.
60+
const edges = [
61+
buildEdge("start", "a1"),
62+
buildEdge("group_done", "final_task"),
63+
buildEdge("a2", "final_task"),
64+
buildEdge("a3", "final_task"),
65+
];
66+
const result = hasUniformExternalConnectivity(new Set(["a1", "branch", "a2", "a3", "group_done"]), edges);
67+
68+
expect(result).toBe(false);
69+
});
70+
71+
it("returns true for the canonical fan-in/fan-out shape", () => {
72+
// Every child has the same external source AND the same external target —
73+
// the "cleanup group" pattern that the optimisation is designed for.
74+
const edges = [
75+
buildEdge("upstream", "T1"),
76+
buildEdge("upstream", "T2"),
77+
buildEdge("upstream", "T3"),
78+
buildEdge("T1", "downstream"),
79+
buildEdge("T2", "downstream"),
80+
buildEdge("T3", "downstream"),
81+
];
82+
const result = hasUniformExternalConnectivity(new Set(["T1", "T2", "T3"]), edges);
83+
84+
expect(result).toBe(true);
85+
});
86+
});
87+
88+
describe("generateElkGraph — open TaskGroup with escape edges (#67714)", () => {
89+
// Mirrors the minimal reproducer from issue #67714:
90+
//
91+
// start ─→ group_a { a1 ─→ branch ─→ [a2, a3] ─→ group_done } ─→ final_task
92+
// ↘──── ↘────────┐
93+
// ───────→ final_task
94+
//
95+
// ``a1`` is the entry (external source {start}), ``a2``/``a3``/``group_done``
96+
// are exits (external target {final_task}). Their profiles differ, so
97+
// ``hasUniformExternalConnectivity`` correctly returns false and the
98+
// open-group rewrite branch never runs — every internal and escape edge
99+
// is rendered individually.
100+
const internalChildren: Array<NodeResponse> = [
101+
buildNode({ id: "a1", label: "task_a1" }),
102+
buildNode({ id: "branch", label: "branch_a" }),
103+
buildNode({ id: "a2", label: "task_a2" }),
104+
buildNode({ id: "a3", label: "task_a3" }),
105+
buildNode({ id: "group_done", label: "group_done" }),
106+
];
107+
108+
const nodes: Array<NodeResponse> = [
109+
buildNode({ id: "start", label: "start" }),
110+
buildNode({
111+
children: internalChildren,
112+
id: "group_a",
113+
label: "group_a",
114+
}),
115+
buildNode({ id: "final_task", label: "final_task" }),
116+
];
117+
118+
const edges: Array<EdgeResponse> = [
119+
buildEdge("start", "a1"),
120+
buildEdge("a1", "branch"),
121+
buildEdge("branch", "a2"),
122+
buildEdge("branch", "a3"),
123+
buildEdge("a2", "group_done"),
124+
buildEdge("a3", "group_done"),
125+
buildEdge("group_done", "final_task"),
126+
// The two "escape" edges that trip the bug:
127+
buildEdge("a2", "final_task"),
128+
buildEdge("a3", "final_task"),
129+
];
130+
131+
it("keeps internal group edges so ELK can lay out the children", () => {
132+
const root = generateElkGraph({
133+
direction: "RIGHT",
134+
edges,
135+
font: "12px sans-serif",
136+
nodes,
137+
openGroupIds: ["group_a"],
138+
});
139+
140+
const groupNode = (root.children as Array<FormattedNode>).find((child) => child.id === "group_a");
141+
142+
expect(groupNode).toBeDefined();
143+
expect(groupNode?.isOpen).toBe(true);
144+
145+
// All five internal edges must survive — that's what ELK needs to lay out
146+
// a1 → branch → [a2, a3] → group_done correctly inside the group.
147+
const internalEdgeIds = new Set(groupNode?.edges?.map((edge) => edge.id) ?? []);
148+
149+
expect(internalEdgeIds).toEqual(
150+
new Set(["a1-branch", "branch-a2", "branch-a3", "a2-group_done", "a3-group_done"]),
151+
);
152+
});
153+
154+
it("renders each crossing escape edge individually instead of collapsing them", () => {
155+
const root = generateElkGraph({
156+
direction: "RIGHT",
157+
edges,
158+
font: "12px sans-serif",
159+
nodes,
160+
openGroupIds: ["group_a"],
161+
});
162+
163+
// The deliberately-wired escape edges from a2 and a3 must remain visible
164+
// so the author's explicit dependency intent is preserved in the graph.
165+
const rootEdgeIds = new Set(root.edges?.map((edge) => edge.id) ?? []);
166+
167+
expect(rootEdgeIds).toEqual(
168+
new Set(["start-a1", "group_done-final_task", "a2-final_task", "a3-final_task"]),
169+
);
170+
});
171+
});
172+
173+
describe("generateElkGraph — open TaskGroup matching the canonical fan-in/fan-out shape", () => {
174+
// The "cleanup group" pattern the optimisation is designed for: every child
175+
// has the SAME external source AND the SAME external target. The collapse
176+
// optimisation should still fire here, and ``preserveInternal: true`` must
177+
// keep any internal edges intact.
178+
it("collapses crossing edges to a single group-level edge while keeping internal edges", () => {
179+
const nodes: Array<NodeResponse> = [
180+
buildNode({ id: "upstream", label: "upstream" }),
181+
buildNode({
182+
children: [
183+
buildNode({ id: "T1", label: "T1" }),
184+
buildNode({ id: "T2", label: "T2" }),
185+
buildNode({ id: "T3", label: "T3" }),
186+
buildNode({ id: "T_internal", label: "T_internal" }),
187+
],
188+
id: "cleanup_group",
189+
label: "cleanup_group",
190+
}),
191+
buildNode({ id: "downstream", label: "downstream" }),
192+
];
193+
194+
const edges: Array<EdgeResponse> = [
195+
buildEdge("upstream", "T1"),
196+
buildEdge("upstream", "T2"),
197+
buildEdge("upstream", "T3"),
198+
buildEdge("T1", "downstream"),
199+
buildEdge("T2", "downstream"),
200+
buildEdge("T3", "downstream"),
201+
// An internal edge within the group; must survive the optimisation so
202+
// ELK can lay it out inside the open group.
203+
buildEdge("T1", "T_internal"),
204+
];
205+
206+
const root = generateElkGraph({
207+
direction: "RIGHT",
208+
edges,
209+
font: "12px sans-serif",
210+
nodes,
211+
openGroupIds: ["cleanup_group"],
212+
});
213+
214+
const groupNode = (root.children as Array<FormattedNode>).find((child) => child.id === "cleanup_group");
215+
216+
// The internal T1 → T_internal edge must still be present in the group's
217+
// edges array (this is what the ``preserveInternal: true`` flag protects).
218+
const internalEdgeIds = new Set(groupNode?.edges?.map((edge) => edge.id) ?? []);
219+
220+
expect(internalEdgeIds).toEqual(new Set(["T1-T_internal"]));
221+
222+
// Six crossing edges (3 fan-in + 3 fan-out) collapse to one each.
223+
const rootEdgeIds = new Set(root.edges?.map((edge) => edge.id) ?? []);
224+
225+
expect(rootEdgeIds).toEqual(new Set(["upstream-cleanup_group", "cleanup_group-downstream"]));
226+
});
227+
});
228+
229+
describe("generateElkGraph — open TaskGroup without escape edges", () => {
230+
// Regression guard for the simple TaskGroup shape (only entry/exit cross the
231+
// boundary). ``hasUniformExternalConnectivity`` returns false here, so the
232+
// open-group rewrite branch never runs — internal edges should already survive.
233+
it("keeps internal group edges intact", () => {
234+
const nodes: Array<NodeResponse> = [
235+
buildNode({ id: "start", label: "start" }),
236+
buildNode({
237+
children: [
238+
buildNode({ id: "a1", label: "task_a1" }),
239+
buildNode({ id: "a2", label: "task_a2" }),
240+
buildNode({ id: "group_done", label: "group_done" }),
241+
],
242+
id: "group_a",
243+
label: "group_a",
244+
}),
245+
buildNode({ id: "final_task", label: "final_task" }),
246+
];
247+
248+
const edges: Array<EdgeResponse> = [
249+
buildEdge("start", "a1"),
250+
buildEdge("a1", "a2"),
251+
buildEdge("a2", "group_done"),
252+
buildEdge("group_done", "final_task"),
253+
];
254+
255+
const root = generateElkGraph({
256+
direction: "RIGHT",
257+
edges,
258+
font: "12px sans-serif",
259+
nodes,
260+
openGroupIds: ["group_a"],
261+
});
262+
263+
const groupNode = (root.children as Array<FormattedNode>).find((child) => child.id === "group_a");
264+
265+
const internalEdgeIds = new Set(groupNode?.edges?.map((edge) => edge.id) ?? []);
266+
267+
expect(internalEdgeIds).toEqual(new Set(["a1-a2", "a2-group_done"]));
268+
});
269+
});
270+
271+
describe("generateElkGraph — closed TaskGroup", () => {
272+
// Closed-group behaviour must still drop internal edges (they're not laid out
273+
// when the group is collapsed) and rewrite crossings to point at the group.
274+
it("drops internal edges and rewrites crossings to the group", () => {
275+
const nodes: Array<NodeResponse> = [
276+
buildNode({ id: "start", label: "start" }),
277+
buildNode({
278+
children: [buildNode({ id: "a1", label: "task_a1" }), buildNode({ id: "a2", label: "task_a2" })],
279+
id: "group_a",
280+
label: "group_a",
281+
}),
282+
buildNode({ id: "final_task", label: "final_task" }),
283+
];
284+
285+
const edges: Array<EdgeResponse> = [
286+
buildEdge("start", "a1"),
287+
buildEdge("a1", "a2"),
288+
buildEdge("a2", "final_task"),
289+
];
290+
291+
const root = generateElkGraph({
292+
direction: "RIGHT",
293+
edges,
294+
font: "12px sans-serif",
295+
nodes,
296+
openGroupIds: [],
297+
});
298+
299+
const rootEdgeIds = new Set(root.edges?.map((edge) => edge.id) ?? []);
300+
301+
// Internal a1 → a2 must not appear at the root level; crossings collapse to
302+
// single group-level edges.
303+
expect(rootEdgeIds).toEqual(new Set(["start-group_a", "group_a-final_task"]));
304+
});
305+
});

0 commit comments

Comments
 (0)