From aacfaa4fd735c1347b48a6a33e82c04ed02a1002 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Tue, 24 Sep 2019 17:09:29 -0400
Subject: [PATCH] ensure data sources are always written to state

The old logic for `depends_on` was to short-circuit evaluation of the
data source, but that prevented a plan and state from being recorded.
Use the (currently unused) ForcePlanRead to ensure that the plan is
recorded when the config contains `depends_on`.

This does not fix the fact that depends on does not work with data
sources, and will still produce a perpetual diff. This is only to fix
evaluation errors when an indexed data source is evaluated during
refresh.
---
 terraform/context_refresh_test.go             | 58 +++++++++++++++++++
 terraform/node_data_refresh.go                | 22 ++-----
 .../testdata/plan-data-depends-on/main.tf     | 14 +++++
 3 files changed, 78 insertions(+), 16 deletions(-)
 create mode 100644 terraform/testdata/plan-data-depends-on/main.tf

diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go
index 45c4ae2eb..dbd57eb53 100644
--- a/terraform/context_refresh_test.go
+++ b/terraform/context_refresh_test.go
@@ -1906,3 +1906,61 @@ data "aws_data_source" "foo" {
 		t.Fatal("ValidateDataSourceConfig not called during plan")
 	}
 }
+
+func TestContext2Refresh_dataResourceDependsOn(t *testing.T) {
+	m := testModule(t, "plan-data-depends-on")
+	p := testProvider("test")
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":  {Type: cty.String, Computed: true},
+					"foo": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+		DataSources: map[string]*configschema.Block{
+			"test_data": {
+				Attributes: map[string]*configschema.Attribute{
+					"compute": {Type: cty.String, Computed: true},
+				},
+			},
+		},
+	}
+	p.DiffFn = testDiffFn
+
+	s := MustShimLegacyState(&State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"test_resource.a": &ResourceState{
+						Type:     "test_resource",
+						Provider: "provider.test",
+						Primary: &InstanceState{
+							ID: "a",
+							Attributes: map[string]string{
+								"id": "a",
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+		State: s,
+	})
+
+	_, diags := ctx.Refresh()
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors: %s", diags.Err())
+	}
+}
diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go
index 12f8bbce5..2a4327f91 100644
--- a/terraform/node_data_refresh.go
+++ b/terraform/node_data_refresh.go
@@ -163,22 +163,6 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 				ProviderSchema: &providerSchema,
 			},
 
-			&EvalIf{
-				If: func(ctx EvalContext) (bool, error) {
-					// If the config explicitly has a depends_on for this
-					// data source, assume the intention is to prevent
-					// refreshing ahead of that dependency, and therefore
-					// we need to deal with this resource during the apply
-					// phase..
-					if len(n.Config.DependsOn) > 0 {
-						return true, EvalEarlyExitError{}
-					}
-
-					return true, nil
-				},
-				Then: EvalNoop{},
-			},
-
 			// EvalReadData will _attempt_ to read the data source, but may
 			// generate an incomplete planned object if the configuration
 			// includes values that won't be known until apply.
@@ -192,6 +176,12 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 				OutputChange:      &change,
 				OutputConfigValue: &configVal,
 				OutputState:       &state,
+				// If the config explicitly has a depends_on for this data
+				// source, assume the intention is to prevent refreshing ahead
+				// of that dependency, and therefore we need to deal with this
+				// resource during the apply phase. We do that by forcing this
+				// read to result in a plan.
+				ForcePlanRead: len(n.Config.DependsOn) > 0,
 			},
 
 			&EvalIf{
diff --git a/terraform/testdata/plan-data-depends-on/main.tf b/terraform/testdata/plan-data-depends-on/main.tf
new file mode 100644
index 000000000..c7332ad29
--- /dev/null
+++ b/terraform/testdata/plan-data-depends-on/main.tf
@@ -0,0 +1,14 @@
+resource "test_resource" "a" {
+}
+
+data "test_data" "d" {
+  count = 1
+  depends_on = [
+    test_resource.a
+  ]
+}
+
+resource "test_resource" "b" {
+  count = 1
+  foo = data.test_data.d[count.index].compute
+}