Merge pull request #32876 from hashicorp/jbardin/state-serialize-plan-error

Remove planned data source objects from state on error

internal/terraform/context_plan.go

@@ -286,18 +286,6 @@ func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts
 
 	plan, walkDiags := c.planWalk(config, prevRunState, opts)
 	diags = diags.Append(walkDiags)
-	if diags.HasErrors() {
-		// Non-nil plan along with errors indicates a non-applyable partial
-		// plan that's only suitable to be shown to the user as extra context
-		// to help understand the errors.
-		return plan, diags
-	}
-
-	// The refreshed state ends up with some placeholder objects in it for
-	// objects pending creation. We only really care about those being in
-	// the working state, since that's what we're going to use when applying,
-	// so we'll prune them all here.
-	plan.PriorState.SyncWrapper().RemovePlannedResourceInstanceObjects()
 
 	return plan, diags
 }
@@ -339,10 +327,6 @@ func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.S
 		))
 	}
 
-	// Prune out any placeholder objects we put in the state to represent
-	// objects that would need to be created.
-	plan.PriorState.SyncWrapper().RemovePlannedResourceInstanceObjects()
-
 	// We don't populate RelevantResources for a refresh-only plan, because
 	// they never have any planned actions and so no resource can ever be
 	// "relevant" per the intended meaning of that field.
@@ -580,9 +564,13 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, o
 	// we encountered errors, which we'll return as part of a non-nil plan
 	// so that e.g. the UI can show what was planned so far in case that extra
 	// context helps the user to understand the error messages we're returning.
-
 	prevRunState = walker.PrevRunState.Close()
+
+	// The refreshed state may have data resource objects which were deferred
+	// to apply and cannot be serialized.
+	walker.RefreshState.RemovePlannedResourceInstanceObjects()
 	priorState := walker.RefreshState.Close()
+
 	driftedResources, driftDiags := c.driftedResources(config, prevRunState, priorState, moveResults)
 	diags = diags.Append(driftDiags)
 

internal/terraform/context_plan2_test.go

@@ -4001,3 +4001,37 @@ output "out" {
 	})
 	assertNoErrors(t, diags)
 }
+
+// Make sure the data sources in the prior state are serializeable even if
+// there were an error in the plan.
+func TestContext2Plan_dataSourceReadPlanError(t *testing.T) {
+	m, snap := testModuleWithSnapshot(t, "data-source-read-with-plan-error")
+	awsProvider := testProvider("aws")
+	testProvider := testProvider("test")
+
+	testProvider.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) {
+		resp.PlannedState = req.ProposedNewState
+		resp.Diagnostics = resp.Diagnostics.Append(errors.New("oops"))
+		return resp
+	}
+
+	state := states.NewState()
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("aws"):  testProviderFuncFixed(awsProvider),
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(testProvider),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	if !diags.HasErrors() {
+		t.Fatalf("expected plan error")
+	}
+
+	// make sure we can serialize the plan even if there were an error
+	_, _, _, err := contextOptsForPlanViaFile(t, snap, plan)
+	if err != nil {
+		t.Fatalf("failed to round-trip through planfile: %s", err)
+	}
+}

internal/terraform/testdata/data-source-read-with-plan-error/main.tf

@@ -0,0 +1,12 @@
+resource "aws_instance" "foo" {
+}
+
+// this will be postponed until apply
+data "aws_data_source" "foo" {
+  foo = aws_instance.foo.id
+}
+
+// this will cause an error in the final plan
+resource "test_instance" "bar" {
+  foo = "error"
+}