From 268959f4be869edd64588f248a50c7951e4456e5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 11 Jun 2020 09:49:47 -0400 Subject: [PATCH 1/3] make UpEdges and DownEdges return a copy The public functions for the graph UpEdges and DownEdges is returning the internal Set from the graph, meaning that callers could inadvertently corrupt the graph structure by editing the returned Sets. Make UpEdges and DownEdges return a copy of the set, while retaining the efficient no-copy behavior for internal callers. --- dag/dag.go | 20 ++++++++++---------- dag/graph.go | 29 ++++++++++++++++++++++------- dag/set.go | 9 +++++++++ dag/tarjan.go | 2 +- 4 files changed, 42 insertions(+), 18 deletions(-) diff --git a/dag/dag.go b/dag/dag.go index 8ca4e910e2..f16a459f62 100644 --- a/dag/dag.go +++ b/dag/dag.go @@ -36,7 +36,7 @@ func (g *AcyclicGraph) Ancestors(v Vertex) (Set, error) { return nil } - if err := g.DepthFirstWalk(g.DownEdges(v), memoFunc); err != nil { + if err := g.DepthFirstWalk(g.downEdgesNoCopy(v), memoFunc); err != nil { return nil, err } @@ -52,7 +52,7 @@ func (g *AcyclicGraph) Descendents(v Vertex) (Set, error) { return nil } - if err := g.ReverseDepthFirstWalk(g.UpEdges(v), memoFunc); err != nil { + if err := g.ReverseDepthFirstWalk(g.upEdgesNoCopy(v), memoFunc); err != nil { return nil, err } @@ -65,7 +65,7 @@ func (g *AcyclicGraph) Descendents(v Vertex) (Set, error) { func (g *AcyclicGraph) Root() (Vertex, error) { roots := make([]Vertex, 0, 1) for _, v := range g.Vertices() { - if g.UpEdges(v).Len() == 0 { + if g.upEdgesNoCopy(v).Len() == 0 { roots = append(roots, v) } } @@ -101,10 +101,10 @@ func (g *AcyclicGraph) TransitiveReduction() { // // For each v-prime reachable from v, remove the edge (u, v-prime). for _, u := range g.Vertices() { - uTargets := g.DownEdges(u) + uTargets := g.downEdgesNoCopy(u) - g.DepthFirstWalk(g.DownEdges(u), func(v Vertex, d int) error { - shared := uTargets.Intersection(g.DownEdges(v)) + g.DepthFirstWalk(g.downEdgesNoCopy(u), func(v Vertex, d int) error { + shared := uTargets.Intersection(g.downEdgesNoCopy(v)) for _, vPrime := range shared { g.RemoveEdge(BasicEdge(u, vPrime)) } @@ -208,7 +208,7 @@ func (g *AcyclicGraph) DepthFirstWalk(start Set, f DepthWalkFunc) error { return err } - for _, v := range g.DownEdges(current.Vertex) { + for _, v := range g.downEdgesNoCopy(current.Vertex) { frontier = append(frontier, &vertexAtDepth{ Vertex: v, Depth: current.Depth + 1, @@ -248,7 +248,7 @@ func (g *AcyclicGraph) SortedDepthFirstWalk(start []Vertex, f DepthWalkFunc) err } // Visit targets of this in a consistent order. - targets := AsVertexList(g.DownEdges(current.Vertex)) + targets := AsVertexList(g.downEdgesNoCopy(current.Vertex)) sort.Sort(byVertexName(targets)) for _, t := range targets { @@ -285,7 +285,7 @@ func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error { } seen[current.Vertex] = struct{}{} - for _, t := range g.UpEdges(current.Vertex) { + for _, t := range g.upEdgesNoCopy(current.Vertex) { frontier = append(frontier, &vertexAtDepth{ Vertex: t, Depth: current.Depth + 1, @@ -325,7 +325,7 @@ func (g *AcyclicGraph) SortedReverseDepthFirstWalk(start []Vertex, f DepthWalkFu seen[current.Vertex] = struct{}{} // Add next set of targets in a consistent order. - targets := AsVertexList(g.UpEdges(current.Vertex)) + targets := AsVertexList(g.upEdgesNoCopy(current.Vertex)) sort.Sort(byVertexName(targets)) for _, t := range targets { frontier = append(frontier, &vertexAtDepth{ diff --git a/dag/graph.go b/dag/graph.go index 4ce0dbccb7..1d0544354b 100644 --- a/dag/graph.go +++ b/dag/graph.go @@ -111,10 +111,10 @@ func (g *Graph) Remove(v Vertex) Vertex { g.vertices.Delete(v) // Delete the edges to non-existent things - for _, target := range g.DownEdges(v) { + for _, target := range g.downEdgesNoCopy(v) { g.RemoveEdge(BasicEdge(v, target)) } - for _, source := range g.UpEdges(v) { + for _, source := range g.upEdgesNoCopy(v) { g.RemoveEdge(BasicEdge(source, v)) } @@ -137,10 +137,10 @@ func (g *Graph) Replace(original, replacement Vertex) bool { // Add our new vertex, then copy all the edges g.Add(replacement) - for _, target := range g.DownEdges(original) { + for _, target := range g.downEdgesNoCopy(original) { g.Connect(BasicEdge(replacement, target)) } - for _, source := range g.UpEdges(original) { + for _, source := range g.upEdgesNoCopy(original) { g.Connect(BasicEdge(source, replacement)) } @@ -166,14 +166,29 @@ func (g *Graph) RemoveEdge(edge Edge) { } } -// DownEdges returns the outward edges from the source Vertex v. +// UpEdges returns the vertices connected to the outward edges from the source +// Vertex v. +func (g *Graph) UpEdges(v Vertex) Set { + return g.upEdgesNoCopy(v).Copy() +} + +// DownEdges returns the vertices connected from the inward edges to Vertex v. func (g *Graph) DownEdges(v Vertex) Set { + return g.downEdgesNoCopy(v).Copy() +} + +// downEdgesNoCopy returns the outward edges from the source Vertex v as a Set. +// This Set is the same as used internally bu the Graph to prevent a copy, and +// must not be modified by the caller. +func (g *Graph) downEdgesNoCopy(v Vertex) Set { g.init() return g.downEdges[hashcode(v)] } -// UpEdges returns the inward edges to the destination Vertex v. -func (g *Graph) UpEdges(v Vertex) Set { +// upEdgesNoCopy returns the inward edges to the destination Vertex v as a Set. +// This Set is the same as used internally bu the Graph to prevent a copy, and +// must not be modified by the caller. +func (g *Graph) upEdgesNoCopy(v Vertex) Set { g.init() return g.upEdges[hashcode(v)] } diff --git a/dag/set.go b/dag/set.go index f3fd704ba4..c5c1af1205 100644 --- a/dag/set.go +++ b/dag/set.go @@ -103,3 +103,12 @@ func (s Set) List() []interface{} { return r } + +// Copy returns a shallow copy of the set. +func (s Set) Copy() Set { + c := make(Set) + for k, v := range s { + c[k] = v + } + return c +} diff --git a/dag/tarjan.go b/dag/tarjan.go index 330abd589f..fb4d4a7732 100644 --- a/dag/tarjan.go +++ b/dag/tarjan.go @@ -24,7 +24,7 @@ func stronglyConnected(acct *sccAcct, g *Graph, v Vertex) int { index := acct.visit(v) minIdx := index - for _, raw := range g.DownEdges(v) { + for _, raw := range g.downEdgesNoCopy(v) { target := raw.(Vertex) targetIdx := acct.VertexIndex[target] From 8e4bd669e8ab238781fddbd3994314f8a6c62688 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 23:01:00 -0400 Subject: [PATCH 2/3] test Set.Copy, Graph.UpEdges, Graph.DownEdges --- dag/graph_test.go | 36 ++++++++++++++++++++++++++++++++++++ dag/set_test.go | 20 ++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/dag/graph_test.go b/dag/graph_test.go index 2979744316..76c47641da 100644 --- a/dag/graph_test.go +++ b/dag/graph_test.go @@ -170,6 +170,42 @@ func TestGraphEdgesTo(t *testing.T) { } } +func TestGraphUpdownEdges(t *testing.T) { + // Verify that we can't inadvertently modify the internal graph sets + var g Graph + g.Add(1) + g.Add(2) + g.Add(3) + g.Connect(BasicEdge(1, 2)) + g.Connect(BasicEdge(2, 3)) + + up := g.UpEdges(2) + if up.Len() != 1 || !up.Include(1) { + t.Fatalf("expected only an up edge of '1', got %#v", up) + } + // modify the up set + up.Add(9) + + orig := g.UpEdges(2) + diff := up.Difference(orig) + if diff.Len() != 1 || !diff.Include(9) { + t.Fatalf("expected a diff of only '9', got %#v", diff) + } + + down := g.DownEdges(2) + if down.Len() != 1 || !down.Include(3) { + t.Fatalf("expected only a down edge of '3', got %#v", down) + } + // modify the down set + down.Add(8) + + orig = g.DownEdges(2) + diff = down.Difference(orig) + if diff.Len() != 1 || !diff.Include(8) { + t.Fatalf("expected a diff of only '8', got %#v", diff) + } +} + type hashVertex struct { code interface{} } diff --git a/dag/set_test.go b/dag/set_test.go index 63b72e3239..36bd6a65b8 100644 --- a/dag/set_test.go +++ b/dag/set_test.go @@ -99,3 +99,23 @@ func TestSetFilter(t *testing.T) { }) } } + +func TestSetCopy(t *testing.T) { + a := make(Set) + a.Add(1) + a.Add(2) + + b := a.Copy() + b.Add(3) + + diff := b.Difference(a) + + if diff.Len() != 1 { + t.Fatalf("expected single diff value, got %#v", diff) + } + + if !diff.Include(3) { + t.Fatalf("diff does not contain 3, got %#v", diff) + } + +} From c0a5214aec0ccdfec1c5fc7365bbec7357cabe99 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Jun 2020 22:35:24 -0400 Subject: [PATCH 3/3] do not look for all descendants from root outputs The output destroy node only needs to connect to each of the output's up-edges in order to be connected transitively to all of the outputs dependencies. In large, highly-connected graphs, this may save considerable time for each output. --- terraform/transform_output.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/terraform/transform_output.go b/terraform/transform_output.go index 4d51dabd63..b926b2fd15 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -86,10 +86,7 @@ func (t *destroyRootOutputTransformer) Transform(g *Graph) error { log.Printf("[TRACE] creating %s", node.Name()) g.Add(node) - deps, err := g.Descendents(v) - if err != nil { - return err - } + deps := g.UpEdges(v) // the destroy node must depend on the eval node deps.Add(v)