core: Retain prior state if update fails with no new state

In an ideal world, providers are supposed to respond to errors during
apply by returning a partial new state alongside the error diagnostics.
In practice though, our SDK leaves the new value set to nil for certain
errors, which was causing Terraform to "forget" the object altogether by
assuming that the provider intended to say "null".

We now adjust that assumption to apply only in the delete case. In all
other cases (including updates) we retain the prior state if the new
state is given as nil. Although we could potentially fix this in the SDK
itself, I expect this is a likely bug in other future SDKs for other
languages too, so this new assumption is a safer one to make to be
resilient to data loss when providers don't behave perfectly.

Providers that return both nil new value and no errors are considered
buggy, but unfortunately that applies to the mocks in many of our tests,
so for pragmatic reasons we can't generate an error for that case as we do
for other "should never happen" situations. Instead, we'll just retain the
prior value in the state so the user can retry.

terraform/context_apply_test.go

@@ -9941,3 +9941,91 @@ func TestContext2Apply_scaleInMultivarRef(t *testing.T) {
 	_, diags = ctx.Apply()
 	assertNoErrors(t, diags)
 }
+
+// Issue 19908 was about retaining an existing object in the state when an
+// update to it fails and the provider does not return a partially-updated
+// value for it. Previously we were incorrectly removing it from the state
+// in that case, but instead it should be retained so the update can be
+// retried.
+func TestContext2Apply_issue19908(t *testing.T) {
+	m := testModule(t, "apply-issue19908")
+	p := testProvider("test")
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test": {
+				Attributes: map[string]*configschema.Attribute{
+					"baz": {Type: cty.String, Required: true},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
+		var diags tfdiags.Diagnostics
+		diags = diags.Append(fmt.Errorf("update failed"))
+		return providers.ApplyResourceChangeResponse{
+			Diagnostics: diags,
+		}
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State: states.BuildState(func(s *states.SyncState) {
+			s.SetResourceInstanceCurrent(
+				addrs.Resource{
+					Mode: addrs.ManagedResourceMode,
+					Type: "test",
+					Name: "foo",
+				}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+				&states.ResourceInstanceObjectSrc{
+					AttrsJSON: []byte(`{"baz":"old"}`),
+					Status:    states.ObjectReady,
+				},
+				addrs.ProviderConfig{
+					Type: "test",
+				}.Absolute(addrs.RootModuleInstance),
+			)
+		}),
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	if _, diags := ctx.Plan(); diags.HasErrors() {
+		t.Fatalf("plan errors: %s", diags.Err())
+	}
+
+	state, diags := ctx.Apply()
+	if !diags.HasErrors() {
+		t.Fatalf("apply succeeded; want error")
+	}
+	if got, want := diags.Err().Error(), "update failed"; !strings.Contains(got, want) {
+		t.Fatalf("wrong error\ngot: %s\nshould contain: %s", got, want)
+	}
+
+	mod := state.RootModule()
+	rs := mod.Resources["test.foo"]
+	if rs == nil {
+		t.Fatalf("test.foo not in state after apply, but should be")
+	}
+	is := rs.Instances[addrs.NoKey]
+	if is == nil {
+		t.Fatalf("test.foo not in state after apply, but should be")
+	}
+	obj := is.Current
+	if obj == nil {
+		t.Fatalf("test.foo has no current object in state after apply, but should do")
+	}
+
+	if got, want := obj.Status, states.ObjectReady; got != want {
+		t.Errorf("test.foo has wrong status %s after apply; want %s", got, want)
+	}
+	if got, want := obj.AttrsJSON, []byte(`"old"`); !bytes.Contains(got, want) {
+		t.Errorf("test.foo attributes JSON doesn't contain %s after apply\ngot: %s", want, got)
+	}
+}

terraform/eval_apply.go

@@ -88,11 +88,28 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	// incomplete.
 	newVal := resp.NewState
 
-	// newVal should never be cty.NilVal in a real case, but it can happen
-	// sometimes in sloppy mocks in tests where error diagnostics are returned
-	// and the mock implementation doesn't populate the value at all.
 	if newVal == cty.NilVal {
-		newVal = cty.NullVal(schema.ImpliedType())
+		// Providers are supposed to return a partial new value even when errors
+		// occur, but sometimes they don't and so in that case we'll patch that up
+		// by just using the prior state, so we'll at least keep track of the
+		// object for the user to retry.
+		newVal = change.Before
+
+		// As a special case, we'll set the new value to null if it looks like
+		// we were trying to execute a delete, because the provider in this case
+		// probably left the newVal unset intending it to be interpreted as "null".
+		if change.After.IsNull() {
+			newVal = cty.NullVal(schema.ImpliedType())
+		}
+
+		// Ideally we'd produce an error or warning here if newVal is nil and
+		// there are no errors in diags, because that indicates a buggy
+		// provider not properly reporting its result, but unfortunately many
+		// of our historical test mocks behave in this way and so producing
+		// a diagnostic here fails hundreds of tests. Instead, we must just
+		// silently retain the old value for now. Returning a nil value with
+		// no errors is still always considered a bug in the provider though,
+		// and should be fixed for any "real" providers that do it.
 	}
 
 	var conformDiags tfdiags.Diagnostics
@@ -101,7 +118,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 			tfdiags.Error,
 			"Provider produced invalid object",
 			fmt.Sprintf(
-				"Provider %q planned an invalid value after apply for %s. The result cannot not be saved in the Terraform state.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				"Provider %q produced an invalid value after apply for %s. The result cannot not be saved in the Terraform state.\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()),
 			),
 		))
@@ -158,7 +175,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	// we still want to save that but it often causes some confusing behaviors
 	// where it seems like Terraform is failing to take any action at all,
 	// so we'll generate some errors to draw attention to it.
-	if !applyDiags.HasErrors() {
+	if !diags.HasErrors() {
 		if change.Action == plans.Delete && !newVal.IsNull() {
 			diags = diags.Append(tfdiags.Sourceless(
 				tfdiags.Error,

terraform/test-fixtures/apply-issue19908/issue19908.tf

@@ -0,0 +1,3 @@
+resource "test" "foo" {
+  baz = "updated"
+}