Use DependencyGraph to compute dependents

Per code review feedback from @pgavlin.

pkg/resource/deploy/step_generator.go

@@ -776,36 +776,31 @@ func (sg *stepGenerator) GenerateDeletes(targetsOpt map[resource.URN]bool) ([]St
 // This includes both implicit and explicit dependents in the DAG itself, as well as children.
 func (sg *stepGenerator) getTargetDependents(targetsOpt map[resource.URN]bool) map[resource.URN]bool {
 	// Seed the list with the initial set of targets.
-	var frontier []resource.URN
-	for target := range targetsOpt {
-		frontier = append(frontier, target)
+	var frontier []*resource.State
+	for _, res := range sg.deployment.prev.Resources {
+		if _, has := targetsOpt[res.URN]; has {
+			frontier = append(frontier, res)
+		}
 	}
 
-	// Loop until we have explored the transitive closure of all implicated targets.
+	// Produce a dependency graph of resources.
+	dg := graph.NewDependencyGraph(sg.deployment.prev.Resources)
+
+	// Now accumulate a list of targets that are implicated because they depend upon the targets.
 	targets := make(map[resource.URN]bool)
 	for len(frontier) > 0 {
 		// Pop the next to explore, mark it, and skip any we've already seen.
 		next := frontier[0]
 		frontier = frontier[1:]
-		if _, has := targets[next]; has {
+		if _, has := targets[next.URN]; has {
 			continue
 		}
-		targets[next] = true
+		targets[next.URN] = true
 
-		// Walk the resources looking for children and anything implicitly or explicitly
-		// depending on the resource being deleted. Mark them, and add them for exploration.
-		for _, res := range sg.deployment.prev.Resources {
-			// Check whether the parent is being deleted; if yes, this one is implicated too.
-			if _, has := targets[res.Parent]; has {
-				frontier = append(frontier, res.URN)
-			}
-			// Check whether any of the dependencies are being deleted; if yes, this one is implicated too.
-			for _, dep := range res.Dependencies {
-				if _, has := targets[dep]; has {
-					frontier = append(frontier, res.URN)
-				}
-			}
-		}
+		// Compute the set of resources depending on this one, either implicitly, explicitly,
+		// or because it is a child resource. Add them to the frontier to keep exploring.
+		deps := dg.DependingOn(next, targets, true)
+		frontier = append(frontier, deps...)
 	}
 
 	return targets
@@ -1414,7 +1409,7 @@ func (sg *stepGenerator) calculateDependentReplacements(root *resource.State) ([
 	// that have already been registered must not depend on the root. Thus, we ignore these resources if they are
 	// encountered while walking the old dependency graph to determine the set of dependents.
 	impossibleDependents := sg.urns
-	for _, d := range sg.deployment.depGraph.DependingOn(root, impossibleDependents) {
+	for _, d := range sg.deployment.depGraph.DependingOn(root, impossibleDependents, false) {
 		replace, keys, res := requiresReplacement(d)
 		if res != nil {
 			return nil, res

pkg/resource/edit/operations.go

@@ -41,7 +41,7 @@ func DeleteResource(snapshot *deploy.Snapshot, condemnedRes *resource.State) err
 	}
 
 	dg := graph.NewDependencyGraph(snapshot.Resources)
-	dependencies := dg.DependingOn(condemnedRes, nil)
+	dependencies := dg.DependingOn(condemnedRes, nil, false)
 	if len(dependencies) != 0 {
 		return ResourceHasDependenciesError{Condemned: condemnedRes, Dependencies: dependencies}
 	}

pkg/resource/graph/dependency_graph.go

@@ -20,7 +20,8 @@ type DependencyGraph struct {
 // order with respect to the snapshot dependency graph.
 //
 // The time complexity of DependingOn is linear with respect to the number of resources.
-func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.URN]bool) []*resource.State {
+func (dg *DependencyGraph) DependingOn(res *resource.State,
+	ignore map[resource.URN]bool, includeChildren bool) []*resource.State {
 	// This implementation relies on the detail that snapshots are stored in a valid
 	// topological order.
 	var dependents []*resource.State
@@ -34,6 +35,14 @@ func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.
 		if ignore[candidate.URN] {
 			return false
 		}
+		if includeChildren && candidate.Parent == res.URN {
+			return true
+		}
+		for _, dependency := range candidate.Dependencies {
+			if dependentSet[dependency] {
+				return true
+			}
+		}
 		if candidate.Provider != "" {
 			ref, err := providers.ParseReference(candidate.Provider)
 			contract.Assert(err == nil)
@@ -41,11 +50,6 @@ func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.
 				return true
 			}
 		}
-		for _, dependency := range candidate.Dependencies {
-			if dependentSet[dependency] {
-				return true
-			}
-		}
 		return false
 	}
 

pkg/resource/graph/dependency_graph_test.go

@@ -63,58 +63,58 @@ func TestBasicGraph(t *testing.T) {
 
 	assert.Equal(t, []*resource.State{
 		a, b, pB, c, d,
-	}, dg.DependingOn(pA, nil))
+	}, dg.DependingOn(pA, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		b, pB, c, d,
-	}, dg.DependingOn(a, nil))
+	}, dg.DependingOn(a, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		pB, c, d,
-	}, dg.DependingOn(b, nil))
+	}, dg.DependingOn(b, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		c,
-	}, dg.DependingOn(pB, nil))
+	}, dg.DependingOn(pB, nil, false))
 
-	assert.Nil(t, dg.DependingOn(c, nil))
-	assert.Nil(t, dg.DependingOn(d, nil))
+	assert.Nil(t, dg.DependingOn(c, nil, false))
+	assert.Nil(t, dg.DependingOn(d, nil, false))
 
 	assert.Nil(t, dg.DependingOn(pA, map[resource.URN]bool{
 		a.URN: true,
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		a, pB, c,
 	}, dg.DependingOn(pA, map[resource.URN]bool{
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		b, pB, c, d,
 	}, dg.DependingOn(pA, map[resource.URN]bool{
 		a.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		c,
 	}, dg.DependingOn(a, map[resource.URN]bool{
 		b.URN:  true,
 		pB.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		pB, c,
 	}, dg.DependingOn(a, map[resource.URN]bool{
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		d,
 	}, dg.DependingOn(b, map[resource.URN]bool{
 		pB.URN: true,
-	}))
+	}, false))
 }
 
 // Tests that we don't add the same node to the DependingOn set twice.
@@ -133,7 +133,7 @@ func TestGraphNoDuplicates(t *testing.T) {
 
 	assert.Equal(t, []*resource.State{
 		b, c, d,
-	}, dg.DependingOn(a, nil))
+	}, dg.DependingOn(a, nil, false))
 }
 
 func TestDependenciesOf(t *testing.T) {