core: Properly handle deferral (or non-deferral) of data resources

(this is a WIP prototype)

terraform/context_plan_test.go

@@ -1907,6 +1907,13 @@ func TestContext2Plan_computedInFunction(t *testing.T) {
 		},
 	}
 	p.DiffFn = testDiffFn
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"computed": cty.ListVal([]cty.Value{
+				cty.StringVal("foo"),
+			}),
+		}),
+	}
 
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
@@ -1917,34 +1924,26 @@ func TestContext2Plan_computedInFunction(t *testing.T) {
 		),
 	})
 
-	defer ctx.acquireRun("validate")()
+	diags := ctx.Validate()
+	assertNoErrors(t, diags)
 
-	pgb := &PlanGraphBuilder{
-		Config:     ctx.config,
-		State:      ctx.state,
-		Components: ctx.components,
-		Schemas:    ctx.schemas,
-		Targets:    ctx.targets,
-	}
-	graph, _ := pgb.Build(addrs.RootModuleInstance)
+	state, diags := ctx.Refresh() // data resource is read in this step
+	assertNoErrors(t, diags)
 
-	// walk
-	walker := &ContextGraphWalker{
-		Context:            ctx,
-		State:              ctx.state.SyncWrapper(),
-		Changes:            ctx.changes.SyncWrapper(),
-		Operation:          walkPlan,
-		StopContext:        ctx.runContext,
-		RootVariableValues: ctx.variables,
+	if !p.ReadDataSourceCalled {
+		t.Fatalf("ReadDataSource was not called on provider during refresh; should've been called")
 	}
-	watchStop, watchWait := ctx.watchStop(walker)
-	diags := graph.Walk(walker)
-	close(watchStop)
-	<-watchWait
+	p.ReadDataSourceCalled = false // reset for next call
 
-	if diags.HasErrors() {
-		t.Fatalf("unexpected errors: %s", diags.Err())
+	t.Logf("state after refresh:\n%s", state)
+
+	_, diags = ctx.Plan() // should do nothing with data resource in this step, since it was already read
+	assertNoErrors(t, diags)
+
+	if p.ReadDataSourceCalled {
+		t.Fatalf("ReadDataSource was called on provider during plan; should not have been called")
 	}
+
 }
 
 func TestContext2Plan_computedDataCountResource(t *testing.T) {

terraform/eval_read_data.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"log"
 
 	"github.com/zclconf/go-cty/cty"
 
@@ -14,6 +15,250 @@ import (
 	"github.com/hashicorp/terraform/tfdiags"
 )
 
+// EvalReadData is an EvalNode implementation that deals with the main part
+// of the data resource lifecycle: either actually reading from the data source
+// or generating a plan to do so.
+type EvalReadData struct {
+	Addr           addrs.ResourceInstance
+	Config         *configs.Resource
+	Provider       *providers.Interface
+	ProviderAddr   addrs.AbsProviderConfig
+	ProviderSchema **ProviderSchema
+
+	// Planned is set when dealing with data resources that were deferred to
+	// the apply walk, to let us see what was planned. If this is set, the
+	// evaluation of the config is required to produce a wholly-known
+	// configuration which is consistent with the partial object included
+	// in this planned change.
+	Planned **plans.ResourceInstanceChange
+
+	// The result from this EvalNode has a few different possibilities
+	// depending on the input:
+	// - If Planned is nil then we assume we're aiming to _produce_ the plan,
+	//   and so the following two outcomes are possible:
+	//     - OutputChange.Action is plans.NoOp and OutputState is the complete
+	//       result of reading from the data source. This is the easy path.
+	//     - OutputChange.Action is plans.Read and OutputState is a planned
+	//       object placeholder (states.ObjectPlanned). In this case, the
+	//       returned change must be recorded in the overral changeset and
+	//       eventually passed to another instance of this struct during the
+	//       apply walk.
+	// - If Planned is non-nil then we assume we're aiming to complete a
+	//   planned read from an earlier plan walk. In this case the only possible
+	//   non-error outcome is to set Output.Action (if non-nil) to a plans.NoOp
+	//   change and put the complete resulting state in OutputState, ready to
+	//   be saved in the overall state and used for expression evaluation.
+	OutputChange      **plans.ResourceInstanceChange
+	OutputValue       *cty.Value
+	OutputConfigValue *cty.Value
+	OutputState       **states.ResourceInstanceObject
+}
+
+func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
+	absAddr := n.Addr.Absolute(ctx.Path())
+	log.Printf("[TRACE] EvalReadData: working on %s", absAddr)
+
+	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
+		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
+	}
+
+	var diags tfdiags.Diagnostics
+	var change *plans.ResourceInstanceChange
+	var configVal cty.Value
+
+	// TODO: Do we need to handle Delete changes here? EvalReadDataDiff and
+	// EvalReadDataApply did, but it seems like we should handle that via a
+	// separate mechanism since it boils down to just deleting the object from
+	// the state... and we do that on every plan anyway, forcing the data
+	// resource to re-read.
+
+	config := *n.Config
+	provider := *n.Provider
+	providerSchema := *n.ProviderSchema
+	schema := providerSchema.DataSources[n.Addr.Resource.Type]
+	if schema == nil {
+		// Should be caught during validation, so we don't bother with a pretty error here
+		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.ProviderConfig.Type, n.Addr.Resource.Type)
+	}
+
+	// We'll always start by evaluating the configuration. What we do after
+	// that will depend on the evaluation result along with what other inputs
+	// we were given.
+	objTy := schema.ImpliedType()
+	priorVal := cty.NullVal(objTy) // for data resources, prior is always null because we start fresh every time
+
+	keyData := EvalDataForInstanceKey(n.Addr.Key)
+
+	var configDiags tfdiags.Diagnostics
+	configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData)
+	diags = diags.Append(configDiags)
+	if configDiags.HasErrors() {
+		return nil, diags.Err()
+	}
+
+	proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal)
+
+	// If our configuration contains any unknown values then we must defer the
+	// read to the apply phase by producing a "Read" change for this resource,
+	// and a placeholder value for it in the state.
+	if !configVal.IsWhollyKnown() {
+		// If the configuration is still unknown when we're applying a planned
+		// change then that indicates a bug in Terraform, since we should have
+		// everything resolved by now.
+		if n.Planned != nil && *n.Planned != nil {
+			return nil, fmt.Errorf(
+				"configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)",
+				absAddr,
+			)
+		}
+		log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr)
+
+		err := ctx.Hook(func(h Hook) (HookAction, error) {
+			return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		change = &plans.ResourceInstanceChange{
+			Addr:         absAddr,
+			ProviderAddr: n.ProviderAddr,
+			Change: plans.Change{
+				Action: plans.Read,
+				Before: priorVal,
+				After:  proposedNewVal,
+			},
+		}
+
+		err = ctx.Hook(func(h Hook) (HookAction, error) {
+			return h.PostDiff(absAddr, states.CurrentGen, change.Action, priorVal, proposedNewVal)
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		if n.OutputChange != nil {
+			*n.OutputChange = change
+		}
+		if n.OutputValue != nil {
+			*n.OutputValue = change.After
+		}
+		if n.OutputConfigValue != nil {
+			*n.OutputConfigValue = configVal
+		}
+		if n.OutputState != nil {
+			state := &states.ResourceInstanceObject{
+				Value:  change.After,
+				Status: states.ObjectPlanned, // because the partial value in the plan must be used for now
+			}
+			*n.OutputState = state
+		}
+
+		return nil, diags.ErrWithWarnings()
+	}
+
+	if n.Planned != nil && *n.Planned != nil && (*n.Planned).Action != plans.Read {
+		// If any other action gets in here then that's always a bug; this
+		// EvalNode only deals with reading.
+		return nil, fmt.Errorf(
+			"invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)",
+			(*n.Planned).Action, absAddr,
+		)
+	}
+
+	// If we get down here then our configuration is complete and we're read
+	// to actually call the provider to read the data.
+	log.Printf("[TRACE] EvalReadData: %s configuration is complete, so reading from provider", absAddr)
+
+	err := ctx.Hook(func(h Hook) (HookAction, error) {
+		// We don't have a state yet, so we'll just give the hook an
+		// empty one to work with.
+		return h.PreRefresh(absAddr, states.CurrentGen, cty.NullVal(cty.DynamicPseudoType))
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	resp := provider.ReadDataSource(providers.ReadDataSourceRequest{
+		TypeName: n.Addr.Resource.Type,
+		Config:   configVal,
+	})
+	diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config))
+	if diags.HasErrors() {
+		return nil, diags.Err()
+	}
+	newVal := resp.State
+	if newVal == cty.NilVal {
+		// This can happen with incompletely-configured mocks. We'll allow it
+		// and treat it as an alias for a properly-typed null value.
+		newVal = cty.NullVal(schema.ImpliedType())
+	}
+
+	for _, err := range newVal.Type().TestConformance(schema.ImpliedType()) {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Provider produced invalid object",
+			fmt.Sprintf(
+				"Provider %q produced an invalid value for %s.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				n.ProviderAddr.ProviderConfig.Type, tfdiags.FormatErrorPrefixed(err, absAddr.String()),
+			),
+		))
+	}
+	if diags.HasErrors() {
+		return nil, diags.Err()
+	}
+
+	if newVal.IsNull() {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Provider produced null object",
+			fmt.Sprintf(
+				"Provider %q produced a null value for %s.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				n.ProviderAddr.ProviderConfig.Type, absAddr,
+			),
+		))
+	}
+
+	// Since we've completed the read, we actually have no change to make, but
+	// we'll produce a NoOp one anyway to preserve the usual flow of the
+	// plan phase and allow it to produce a complete plan.
+	change = &plans.ResourceInstanceChange{
+		Addr:         absAddr,
+		ProviderAddr: n.ProviderAddr,
+		Change: plans.Change{
+			Action: plans.NoOp,
+			Before: newVal,
+			After:  newVal,
+		},
+	}
+	state := &states.ResourceInstanceObject{
+		Value:  change.After,
+		Status: states.ObjectReady, // because we completed the read from the provider
+	}
+
+	err = ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostRefresh(absAddr, states.CurrentGen, change.Before, newVal)
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	if n.OutputChange != nil {
+		*n.OutputChange = change
+	}
+	if n.OutputValue != nil {
+		*n.OutputValue = change.After
+	}
+	if n.OutputConfigValue != nil {
+		*n.OutputConfigValue = configVal
+	}
+	if n.OutputState != nil {
+		*n.OutputState = state
+	}
+
+	return nil, diags.ErrWithWarnings()
+}
+
 // EvalReadDataDiff is an EvalNode implementation that executes a data
 // resource's ReadDataDiff method to discover what attributes it exports.
 type EvalReadDataDiff struct {

terraform/node_data_refresh.go

@@ -144,29 +144,13 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 				ProviderSchema: &providerSchema,
 			},
 
-			&EvalReadDataDiff{
-				Addr:              addr.Resource,
-				Config:            n.Config,
-				ProviderAddr:      n.ResolvedProvider,
-				ProviderSchema:    &providerSchema,
-				Output:            &change,
-				OutputConfigValue: &configVal,
-				OutputState:       &state,
-			},
-
-			// The rest of this pass can proceed only if there are no
-			// computed values in our config.
-			// (If there are, we'll deal with this during the plan and
-			// apply phases.)
 			&EvalIf{
 				If: func(ctx EvalContext) (bool, error) {
-					if !configVal.IsWhollyKnown() {
-						return true, EvalEarlyExitError{}
-					}
-
 					// If the config explicitly has a depends_on for this
 					// data source, assume the intention is to prevent
-					// refreshing ahead of that dependency.
+					// 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{}
 					}
@@ -176,25 +160,60 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 				Then: EvalNoop{},
 			},
 
-			&EvalReadDataApply{
-				Addr:            addr.Resource,
-				Config:          n.Config,
-				Change:          &change,
-				Provider:        &provider,
-				ProviderAddr:    n.ResolvedProvider,
-				ProviderSchema:  &providerSchema,
-				Output:          &state,
-				StateReferences: n.StateReferences(),
+			// 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.
+			&EvalReadData{
+				Addr:              addr.Resource,
+				Config:            n.Config,
+				Provider:          &provider,
+				ProviderAddr:      n.ResolvedProvider,
+				ProviderSchema:    &providerSchema,
+				OutputChange:      &change,
+				OutputConfigValue: &configVal,
+				OutputState:       &state,
 			},
 
-			&EvalWriteState{
-				Addr:           addr.Resource,
-				ProviderAddr:   n.ResolvedProvider,
-				State:          &state,
-				ProviderSchema: &providerSchema,
+			&EvalIf{
+				If: func(ctx EvalContext) (bool, error) {
+					return (*state).Status != states.ObjectPlanned, nil
+				},
+				Then: &EvalSequence{
+					Nodes: []EvalNode{
+						&EvalWriteState{
+							Addr:           addr.Resource,
+							ProviderAddr:   n.ResolvedProvider,
+							State:          &state,
+							ProviderSchema: &providerSchema,
+						},
+						&EvalUpdateStateHook{},
+					},
+				},
+				Else: &EvalSequence{
+					// We can't deal with this yet, so we'll repeat this step
+					// during the plan walk to produce a planned change to read
+					// this during the apply walk. However, we do still need to
+					// save the generated change and partial state so that
+					// results from it can be included in other data resources
+					// or provider configurations during the refresh walk.
+					// (The planned object we save in the state here will be
+					// pruned out at the end of the refresh walk, returning
+					// it back to being unset again for subsequent walks.)
+					Nodes: []EvalNode{
+						&EvalWriteDiff{
+							Addr:           addr.Resource,
+							Change:         &change,
+							ProviderSchema: &providerSchema,
+						},
+						&EvalWriteState{
+							Addr:           addr.Resource,
+							ProviderAddr:   n.ResolvedProvider,
+							State:          &state,
+							ProviderSchema: &providerSchema,
+						},
+					},
+				},
 			},
-
-			&EvalUpdateStateHook{},
 		},
 	}
 }

terraform/node_resource_apply_instance.go

@@ -123,7 +123,6 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 	var providerSchema *ProviderSchema
 	var change *plans.ResourceInstanceChange
 	var state *states.ResourceInstanceObject
-	var configVal cty.Value
 
 	return &EvalSequence{
 		Nodes: []EvalNode{
@@ -151,29 +150,19 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 				Then: EvalNoop{},
 			},
 
-			// Make a new diff, in case we've learned new values in the state
-			// during apply which we can now incorporate.
-			&EvalReadDataDiff{
+			// In this particular call to EvalReadData we include our planned
+			// change, which signals that we expect this read to complete fully
+			// with no unknown values; it'll produce an error if not.
+			&EvalReadData{
 				Addr:           addr.Resource,
 				Config:         n.Config,
+				Planned:        &change, // setting this indicates that the result must be complete
+				Provider:       &provider,
 				ProviderAddr:   n.ResolvedProvider,
 				ProviderSchema: &providerSchema,
-				Output:         &change,
-				OutputValue:    &configVal,
 				OutputState:    &state,
 			},
 
-			&EvalReadDataApply{
-				Addr:            addr.Resource,
-				Config:          n.Config,
-				Change:          &change,
-				Provider:        &provider,
-				ProviderAddr:    n.ResolvedProvider,
-				ProviderSchema:  &providerSchema,
-				Output:          &state,
-				StateReferences: n.StateReferences(),
-			},
-
 			&EvalWriteState{
 				Addr:           addr.Resource,
 				ProviderAddr:   n.ResolvedProvider,

terraform/node_resource_plan_instance.go

@@ -76,18 +76,32 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 				Output: &state,
 			},
 
+			// If we already have a non-planned state then we already dealt
+			// with this during the refresh walk and so we have nothing to do
+			// here.
+			&EvalIf{
+				If: func(ctx EvalContext) (bool, error) {
+					if state != nil && state.Status != states.ObjectPlanned {
+						return true, EvalEarlyExitError{}
+					}
+					return true, nil
+				},
+				Then: EvalNoop{},
+			},
+
 			&EvalValidateSelfRef{
 				Addr:           addr.Resource,
 				Config:         config.Config,
 				ProviderSchema: &providerSchema,
 			},
 
-			&EvalReadDataDiff{
+			&EvalReadData{
 				Addr:           addr.Resource,
 				Config:         n.Config,
+				Provider:       &provider,
 				ProviderAddr:   n.ResolvedProvider,
 				ProviderSchema: &providerSchema,
-				Output:         &change,
+				OutputChange:   &change,
 				OutputValue:    &configVal,
 				OutputState:    &state,
 			},