diff --git a/d2layouts/d2layouts.go b/d2layouts/d2layouts.go index 66cf1132f..90e3c0912 100644 --- a/d2layouts/d2layouts.go +++ b/d2layouts/d2layouts.go @@ -83,6 +83,7 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co extracted := make(map[string]*d2graph.Graph) var extractedOrder []string var extractedEdges []*d2graph.Edge + var extractedEdgeIDs []edgeIDs var constantNears []*d2graph.Graph restoreOrder := SaveOrder(g) @@ -114,7 +115,7 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ // │ └────┘ └───────────────┘ │ │ └────┘ │ // └───────────────────────────────┘ └───────────────────────────────┘ - nestedGraph, externalEdges := ExtractSubgraph(curr, true) + nestedGraph, externalEdges, externalEdgeIDs := ExtractSubgraph(curr, true) // Then we layout curr as a nested graph and re-inject it id := curr.AbsID() @@ -142,13 +143,13 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co if err != nil { return err } - for _, e := range externalEdges { - src, err := lookup(e.Src.AbsID()) + for i, e := range externalEdges { + src, err := lookup(externalEdgeIDs[i].srcID) if err != nil { return err } e.Src = src - dst, err := lookup(e.Dst.AbsID()) + dst, err := lookup(externalEdgeIDs[i].dstID) if err != nil { return err } @@ -182,8 +183,9 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co // │ │ │ │ └──┘ └──┘ │ │ │ │ │ │ │ │ // │ └────┘ └───────────────┘ │ │ └────┘ └───────────────┘ │ // └───────────────────────────────┘ └───────────────────────────────┘ - nestedGraph, externalEdges = ExtractSubgraph(curr, false) + nestedGraph, externalEdges, externalEdgeIDs = ExtractSubgraph(curr, false) extractedEdges = append(extractedEdges, externalEdges...) + extractedEdgeIDs = append(extractedEdgeIDs, externalEdgeIDs...) extracted[id] = nestedGraph extractedOrder = append(extractedOrder, id) @@ -197,8 +199,9 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co } // There is a nested diagram here, so extract its contents and process in the same way - nestedGraph, externalEdges := ExtractSubgraph(curr, gi.IsConstantNear) + nestedGraph, externalEdges, externalEdgeIDs := ExtractSubgraph(curr, gi.IsConstantNear) extractedEdges = append(extractedEdges, externalEdges...) + extractedEdgeIDs = append(extractedEdgeIDs, externalEdgeIDs...) log.Info(ctx, "layout nested", slog.F("level", curr.Level()), slog.F("child", curr.AbsID()), slog.F("gi", gi)) nestedInfo := gi @@ -299,16 +302,17 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co // Restore cross-graph edges and route them g.Edges = append(g.Edges, extractedEdges...) - for _, e := range extractedEdges { + for i, e := range extractedEdges { + ids := extractedEdgeIDs[i] // update object references - src, exists := idToObj[e.Src.AbsID()] + src, exists := idToObj[ids.srcID] if !exists { - return fmt.Errorf("could not find object %#v after layout", e.Src.AbsID()) + return fmt.Errorf("could not find object %#v after layout", ids.srcID) } e.Src = src - dst, exists := idToObj[e.Dst.AbsID()] + dst, exists := idToObj[ids.dstID] if !exists { - return fmt.Errorf("could not find object %#v after layout", e.Dst.AbsID()) + return fmt.Errorf("could not find object %#v after layout", ids.dstID) } e.Dst = dst } @@ -318,15 +322,16 @@ func LayoutNested(ctx context.Context, g *d2graph.Graph, graphInfo GraphInfo, co return err } // need to update pointers if plugin performs edge routing - for _, e := range extractedEdges { - src, exists := idToObj[e.Src.AbsID()] + for i, e := range extractedEdges { + ids := extractedEdgeIDs[i] + src, exists := idToObj[ids.srcID] if !exists { - return fmt.Errorf("could not find object %#v after routing", e.Src.AbsID()) + return fmt.Errorf("could not find object %#v after routing", ids.srcID) } e.Src = src - dst, exists := idToObj[e.Dst.AbsID()] + dst, exists := idToObj[ids.dstID] if !exists { - return fmt.Errorf("could not find object %#v after routing", e.Dst.AbsID()) + return fmt.Errorf("could not find object %#v after routing", ids.dstID) } e.Dst = dst } @@ -360,7 +365,11 @@ func NestedGraphInfo(obj *d2graph.Object) (gi GraphInfo) { return gi } -func ExtractSubgraph(container *d2graph.Object, includeSelf bool) (nestedGraph *d2graph.Graph, externalEdges []*d2graph.Edge) { +type edgeIDs struct { + srcID, dstID string +} + +func ExtractSubgraph(container *d2graph.Object, includeSelf bool) (nestedGraph *d2graph.Graph, externalEdges []*d2graph.Edge, externalEdgeIDs []edgeIDs) { // includeSelf: when we have a constant near or a grid cell that is a container, // we want to include itself in the nested graph, not just its descendants, nestedGraph = d2graph.NewGraph() @@ -399,6 +408,11 @@ func ExtractSubgraph(container *d2graph.Object, includeSelf bool) (nestedGraph * nestedGraph.Edges = append(nestedGraph.Edges, edge) } else if srcIsNested || dstIsNested { externalEdges = append(externalEdges, edge) + // we need these AbsIDs when reconnecting since parent references may become stale + externalEdgeIDs = append(externalEdgeIDs, edgeIDs{ + srcID: edge.Src.AbsID(), + dstID: edge.Dst.AbsID(), + }) } else { remainingEdges = append(remainingEdges, edge) } @@ -446,7 +460,7 @@ func ExtractSubgraph(container *d2graph.Object, includeSelf bool) (nestedGraph * container.ChildrenArray = nil } - return nestedGraph, externalEdges + return nestedGraph, externalEdges, externalEdgeIDs } func InjectNested(container *d2graph.Object, nestedGraph *d2graph.Graph, isRoot bool) { diff --git a/e2etests/e2e_test.go b/e2etests/e2e_test.go index 7d8a45c5d..2bf8e401a 100644 --- a/e2etests/e2e_test.go +++ b/e2etests/e2e_test.go @@ -79,6 +79,7 @@ type testCase struct { name string // if the test is just testing a render/style thing, no need to exercise both engines justDagre bool + testSerialization bool script string mtexts []*d2target.MText assertions func(t *testing.T, diagram *d2target.Diagram) @@ -142,10 +143,32 @@ func run(t *testing.T, tc testCase) { } layoutResolver := func(engine string) (d2graph.LayoutGraph, error) { + layout := d2dagrelayout.DefaultLayout if strings.EqualFold(engine, "elk") { - return d2elklayout.DefaultLayout, nil + layout = d2elklayout.DefaultLayout } - return d2dagrelayout.DefaultLayout, nil + if tc.testSerialization { + return func(ctx context.Context, g *d2graph.Graph) error { + bytes, err := d2graph.SerializeGraph(g) + if err != nil { + return err + } + err = d2graph.DeserializeGraph(bytes, g) + if err != nil { + return err + } + err = layout(ctx, g) + if err != nil { + return err + } + bytes, err = d2graph.SerializeGraph(g) + if err != nil { + return err + } + return d2graph.DeserializeGraph(bytes, g) + }, nil + } + return layout, nil } for _, layoutName := range layoutsTested { @@ -268,3 +291,10 @@ func loadFromFile(t *testing.T, name string) testCase { script: string(d2Text), } } + +func loadFromFileWithOptions(t *testing.T, name string, options testCase) testCase { + tc := options + tc.name = name + tc.script = loadFromFile(t, name).script + return tc +} diff --git a/e2etests/regression_test.go b/e2etests/regression_test.go index 093ebbf1c..10a96e263 100644 --- a/e2etests/regression_test.go +++ b/e2etests/regression_test.go @@ -1052,6 +1052,7 @@ cf many required: { loadFromFile(t, "glob_dimensions"), loadFromFile(t, "shaped_grid_positioning"), loadFromFile(t, "cloud_shaped_grid"), + loadFromFileWithOptions(t, "nested_layout_bug", testCase{testSerialization: true}), } runa(t, tcs) diff --git a/e2etests/testdata/files/nested_layout_bug.d2 b/e2etests/testdata/files/nested_layout_bug.d2 new file mode 100644 index 000000000..dc26f3fe0 --- /dev/null +++ b/e2etests/testdata/files/nested_layout_bug.d2 @@ -0,0 +1,9 @@ +grid-rows: 1 +grid-columns: 2 +a +b: { + grid-rows: 1 + grid-columns: 1 + AA.BB +} +a -> b.AA diff --git a/e2etests/testdata/regression/nested_layout_bug/dagre/board.exp.json b/e2etests/testdata/regression/nested_layout_bug/dagre/board.exp.json new file mode 100644 index 000000000..8562bc26a --- /dev/null +++ b/e2etests/testdata/regression/nested_layout_bug/dagre/board.exp.json @@ -0,0 +1,251 @@ +{ + "name": "", + "isFolderOnly": false, + "fontFamily": "SourceSansPro", + "shapes": [ + { + "id": "a", + "type": "rectangle", + "pos": { + "x": 0, + "y": 0 + }, + "width": 53, + "height": 282, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "a", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 8, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "b", + "type": "rectangle", + "pos": { + "x": 93, + "y": 0 + }, + "width": 244, + "height": 282, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B4", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "b", + "fontSize": 28, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 13, + "labelHeight": 36, + "labelPosition": "INSIDE_TOP_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "b.AA", + "type": "rectangle", + "pos": { + "x": 153, + "y": 96 + }, + "width": 124, + "height": 126, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B5", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "AA", + "fontSize": 24, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 27, + "labelHeight": 31, + "labelPosition": "OUTSIDE_TOP_CENTER", + "zIndex": 0, + "level": 2 + }, + { + "id": "b.AA.BB", + "type": "rectangle", + "pos": { + "x": 183, + "y": 126 + }, + "width": 64, + "height": 66, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "BB", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 19, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 3 + } + ], + "connections": [ + { + "id": "(a -> b.AA)[0]", + "src": "a", + "srcArrow": "none", + "dst": "b.AA", + "dstArrow": "triangle", + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "stroke": "B1", + "borderRadius": 10, + "label": "", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N2", + "italic": true, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "labelPosition": "", + "labelPercentage": 0, + "route": [ + { + "x": 53, + "y": 144 + }, + { + "x": 153, + "y": 153 + } + ], + "animated": false, + "tooltip": "", + "icon": null, + "zIndex": 0 + } + ], + "root": { + "id": "", + "type": "", + "pos": { + "x": 0, + "y": 0 + }, + "width": 0, + "height": 0, + "opacity": 0, + "strokeDash": 0, + "strokeWidth": 0, + "borderRadius": 0, + "fill": "N7", + "stroke": "", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "", + "fontSize": 0, + "fontFamily": "", + "language": "", + "color": "", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "zIndex": 0, + "level": 0 + } +} diff --git a/e2etests/testdata/regression/nested_layout_bug/dagre/sketch.exp.svg b/e2etests/testdata/regression/nested_layout_bug/dagre/sketch.exp.svg new file mode 100644 index 000000000..5fb8aed37 --- /dev/null +++ b/e2etests/testdata/regression/nested_layout_bug/dagre/sketch.exp.svg @@ -0,0 +1,105 @@ +abAABB + + + + + + \ No newline at end of file diff --git a/e2etests/testdata/regression/nested_layout_bug/elk/board.exp.json b/e2etests/testdata/regression/nested_layout_bug/elk/board.exp.json new file mode 100644 index 000000000..d2fb5269e --- /dev/null +++ b/e2etests/testdata/regression/nested_layout_bug/elk/board.exp.json @@ -0,0 +1,251 @@ +{ + "name": "", + "isFolderOnly": false, + "fontFamily": "SourceSansPro", + "shapes": [ + { + "id": "a", + "type": "rectangle", + "pos": { + "x": 0, + "y": 0 + }, + "width": 53, + "height": 286, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "a", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 8, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "b", + "type": "rectangle", + "pos": { + "x": 93, + "y": 0 + }, + "width": 284, + "height": 286, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B4", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "b", + "fontSize": 28, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 13, + "labelHeight": 36, + "labelPosition": "INSIDE_TOP_CENTER", + "zIndex": 0, + "level": 1 + }, + { + "id": "b.AA", + "type": "rectangle", + "pos": { + "x": 153, + "y": 60 + }, + "width": 164, + "height": 166, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B5", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "AA", + "fontSize": 24, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 27, + "labelHeight": 31, + "labelPosition": "INSIDE_TOP_CENTER", + "zIndex": 0, + "level": 2 + }, + { + "id": "b.AA.BB", + "type": "rectangle", + "pos": { + "x": 203, + "y": 110 + }, + "width": 64, + "height": 66, + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "borderRadius": 0, + "fill": "B6", + "stroke": "B1", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "BB", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N1", + "italic": false, + "bold": true, + "underline": false, + "labelWidth": 19, + "labelHeight": 21, + "labelPosition": "INSIDE_MIDDLE_CENTER", + "zIndex": 0, + "level": 3 + } + ], + "connections": [ + { + "id": "(a -> b.AA)[0]", + "src": "a", + "srcArrow": "none", + "dst": "b.AA", + "dstArrow": "triangle", + "opacity": 1, + "strokeDash": 0, + "strokeWidth": 2, + "stroke": "B1", + "borderRadius": 10, + "label": "", + "fontSize": 16, + "fontFamily": "DEFAULT", + "language": "", + "color": "N2", + "italic": true, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "labelPosition": "", + "labelPercentage": 0, + "route": [ + { + "x": 53, + "y": 143 + }, + { + "x": 153, + "y": 143 + } + ], + "animated": false, + "tooltip": "", + "icon": null, + "zIndex": 0 + } + ], + "root": { + "id": "", + "type": "", + "pos": { + "x": 0, + "y": 0 + }, + "width": 0, + "height": 0, + "opacity": 0, + "strokeDash": 0, + "strokeWidth": 0, + "borderRadius": 0, + "fill": "N7", + "stroke": "", + "shadow": false, + "3d": false, + "multiple": false, + "double-border": false, + "tooltip": "", + "link": "", + "icon": null, + "iconPosition": "", + "blend": false, + "fields": null, + "methods": null, + "columns": null, + "label": "", + "fontSize": 0, + "fontFamily": "", + "language": "", + "color": "", + "italic": false, + "bold": false, + "underline": false, + "labelWidth": 0, + "labelHeight": 0, + "zIndex": 0, + "level": 0 + } +} diff --git a/e2etests/testdata/regression/nested_layout_bug/elk/sketch.exp.svg b/e2etests/testdata/regression/nested_layout_bug/elk/sketch.exp.svg new file mode 100644 index 000000000..14f480b19 --- /dev/null +++ b/e2etests/testdata/regression/nested_layout_bug/elk/sketch.exp.svg @@ -0,0 +1,105 @@ +abAABB + + + + + + \ No newline at end of file