use Dependencies to connect creator and destroyer

The DestroyEdgeTransformer cannot determine ordering from the graph when
the destroyers are from orphaned resources, because there are no
references to resolve. The new stored Dependencies provides what we need
to connect the instances in this case.

We also add the StateDependencies method directly in the
GraphNodeResourceInstance interface, since all instances already
implement this, and we don't need another optional interface to check.

The old code in DestroyEdgeTransformer may no longer be needed in the
long run, but that can be determined separately, since too many of the
tests start with an incomplete state and rely on the Dependencies being
determined from the configuration alone.

command/command_test.go

@@ -260,8 +260,10 @@ func testState() *states.State {
 				// The weird whitespace here is reflective of how this would
 				// get written out in a real state file, due to the indentation
 				// of all of the containing wrapping objects and arrays.
-				AttrsJSON: []byte("{\n            \"id\": \"bar\"\n          }"),
+				AttrsJSON:    []byte("{\n            \"id\": \"bar\"\n          }"),
-				Status:    states.ObjectReady,
+				Status:       states.ObjectReady,
+				Dependencies: []addrs.AbsResource{},
+				DependsOn:    []addrs.Referenceable{},
 			},
 			addrs.ProviderConfig{
 				Type: "test",

terraform/context_apply_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/hashicorp/terraform/providers"
 	"github.com/hashicorp/terraform/provisioners"
 	"github.com/hashicorp/terraform/states"
+	"github.com/hashicorp/terraform/states/statefile"
 	"github.com/hashicorp/terraform/tfdiags"
 	"github.com/zclconf/go-cty/cty"
 )
@@ -2984,7 +2985,9 @@ func TestContext2Apply_orphanResource(t *testing.T) {
 			AttrsJSON: []byte(`{}`),
 		}, providerAddr)
 	})
-	if !cmp.Equal(state, want) {
+
+	// compare the marshaled form to easily remove empty and nil slices
+	if !statefile.StatesMarshalEqual(state, want) {
 		t.Fatalf("wrong state after step 1\n%s", cmp.Diff(want, state))
 	}
 

terraform/graph_builder_apply_test.go

@@ -572,8 +572,10 @@ test_object.b
 `)
 
 	instanceGraph := filterInstances(g)
-	if instanceGraph.String() != expected {
+	got := strings.TrimSpace(instanceGraph.String())
-		t.Fatalf("expected:\n%s\ngot:\n%s", expected, instanceGraph)
+
+	if got != expected {
+		t.Fatalf("expected:\n%s\ngot:\n%s", expected, got)
 	}
 }
 

terraform/node_resource_abstract.go

@@ -34,6 +34,10 @@ type ConcreteResourceInstanceNodeFunc func(*NodeAbstractResourceInstance) dag.Ve
 // configuration.
 type GraphNodeResourceInstance interface {
 	ResourceInstanceAddr() addrs.AbsResourceInstance
+
+	// StateDependencies returns any inter-resource dependencies that are
+	// stored in the state.
+	StateDependencies() []addrs.AbsResource
 }
 
 // NodeAbstractResource represents a resource that has no associated

terraform/transform_destroy_edge.go

@@ -58,30 +58,35 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 	destroyers := make(map[string][]GraphNodeDestroyer)
 	destroyerAddrs := make(map[string]addrs.AbsResourceInstance)
 
+	// Record the creators, which will need to depend on the destroyers if they
+	// are only being updated.
+	creators := make(map[string]GraphNodeCreator)
+
 	// destroyersByResource records each destroyer by the AbsResourceAddress.
 	// We use this because dependencies are only referenced as resources, but we
 	// will want to connect all the individual instances for correct ordering.
 	destroyersByResource := make(map[string][]GraphNodeDestroyer)
 	for _, v := range g.Vertices() {
-		dn, ok := v.(GraphNodeDestroyer)
+		switch n := v.(type) {
-		if !ok {
+		case GraphNodeDestroyer:
-			continue
+			addrP := n.DestroyAddr()
+			if addrP == nil {
+				log.Printf("[WARN] DestroyEdgeTransformer: %q (%T) has no destroy address", dag.VertexName(n), v)
+				continue
+			}
+			addr := *addrP
+
+			key := addr.String()
+			log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(n), v, key)
+			destroyers[key] = append(destroyers[key], n)
+			destroyerAddrs[key] = addr
+
+			resAddr := addr.Resource.Absolute(addr.Module).String()
+			destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n)
+		case GraphNodeCreator:
+			addr := n.CreateAddr()
+			creators[addr.String()] = n
 		}
-
-		addrP := dn.DestroyAddr()
-		if addrP == nil {
-			log.Printf("[WARN] DestroyEdgeTransformer: %q (%T) has no destroy address", dag.VertexName(dn), v)
-			continue
-		}
-		addr := *addrP
-
-		key := addr.String()
-		log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(dn), v, key)
-		destroyers[key] = append(destroyers[key], dn)
-		destroyerAddrs[key] = addr
-
-		resAddr := addr.Resource.Absolute(addr.Module).String()
-		destroyersByResource[resAddr] = append(destroyersByResource[resAddr], dn)
 	}
 
 	// If we aren't destroying anything, there will be no edges to make
@@ -90,22 +95,17 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
-	type withDeps interface {
+	// Connect destroy despendencies as stored in the state
-		StateDependencies() []addrs.AbsResource
-	}
-
-	// Connect destroy dependencies directly as stored in the state in case
-	// there's no configuration to create the edges otherwise.
 	for _, ds := range destroyers {
 		for _, des := range ds {
-			ri, ok := des.(withDeps)
+			ri, ok := des.(GraphNodeResourceInstance)
 			if !ok {
 				continue
 			}
 
 			for _, resAddr := range ri.StateDependencies() {
 				for _, desDep := range destroyersByResource[resAddr.String()] {
-					log.Printf("[TRACE] DestroyEdgeTransformer: %s depends on %s\n", dag.VertexName(des), dag.VertexName(desDep))
+					log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(des))
 					g.Connect(dag.BasicEdge(desDep, des))
 
 				}
@@ -113,6 +113,22 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		}
 	}
 
+	// connect creators to any destroyers on which they may depend
+	for _, c := range creators {
+		ri, ok := c.(GraphNodeResourceInstance)
+		if !ok {
+			continue
+		}
+
+		for _, resAddr := range ri.StateDependencies() {
+			for _, desDep := range destroyersByResource[resAddr.String()] {
+				log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
+				g.Connect(dag.BasicEdge(c, desDep))
+
+			}
+		}
+	}
+
 	// Go through and connect creators to destroyers. Going along with
 	// our example, this makes: A_d => A
 	for _, v := range g.Vertices() {
@@ -149,8 +165,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		}
 	}
 
-	// FIXME: connect StateDependencies in here somewhere
-
 	// This is strange but is the easiest way to get the dependencies
 	// of a node that is being destroyed. We use another graph to make sure
 	// the resource is in the graph and ask for references. We have to do this

terraform/transform_reference.go

@@ -3,6 +3,7 @@ package terraform
 import (
 	"fmt"
 	"log"
+	"sort"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
@@ -158,11 +159,14 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error {
 		for _, d := range depMap {
 			deps = append(deps, d)
 		}
+		sort.Slice(deps, func(i, j int) bool {
+			return deps[i].String() < deps[j].String()
+		})
 
 		log.Printf("[TRACE] AttachDependenciesTransformer: %s depends on %s", attacher.ResourceAddr(), deps)
 		attacher.AttachDependencies(deps)
-
 	}
+
 	return nil
 }