resource: Prevent private state data loss on Delete method error

Reference: #863 The previous implementation assumed that Terraform core would preserve private state similar to "regular" state when a provider returned an error diagnostic while destroying a resource. The core implementation instead always uses the response data, which the framework was previously always discarding. This change corrects the errant design while also enabling provider logic to potentially update the private state in this situation.
hashicorp · Oct 23, 2023 · c1193c3 · c1193c3
1 parent cd4d1ec
commit c1193c3
Show file tree

Hide file tree

Showing 8 changed files with 484 additions and 2 deletions.
diff --git a/.changes/unreleased/BUG FIXES-20231023-165712.yaml b/.changes/unreleased/BUG FIXES-20231023-165712.yaml
@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'resource: Add `Private` field to `DeleteResource` type, which was missing to
+  allow provider logic to update private state on errors'
+time: 2023-10-23T16:57:12.447739-04:00
+custom:
+  Issue: "863"
diff --git a/.changes/unreleased/BUG FIXES-20231023-165814.yaml b/.changes/unreleased/BUG FIXES-20231023-165814.yaml
@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'resource: Prevented private state data loss if resource destruction returned
+  an error'
+time: 2023-10-23T16:58:14.585227-04:00
+custom:
+  Issue: "863"
diff --git a/internal/fwserver/server_applyresourcechange_test.go b/internal/fwserver/server_applyresourcechange_test.go
@@ -638,6 +638,90 @@ func TestServerApplyResourceChange(t *testing.T) {
 					}),
 					Schema: testSchema,
 				},
+				Private: testEmptyPrivate,
+			},
+		},
+		"delete-response-private": {
+			server: &fwserver.Server{
+				Provider: &testprovider.Provider{},
+			},
+			request: &fwserver.ApplyResourceChangeRequest{
+				PriorState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				ResourceSchema: testSchema,
+				Resource: &testprovider.Resource{
+					DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
+						diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`))
+
+						resp.Diagnostics.Append(diags...)
+
+						// Must return error to prevent automatic private state clearing
+						resp.Diagnostics.AddError("error summary", "error detail")
+					},
+				},
+			},
+			expectedResponse: &fwserver.ApplyResourceChangeResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewErrorDiagnostic(
+						"error summary",
+						"error detail",
+					),
+				},
+				NewState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				Private: testPrivateProvider,
+			},
+		},
+		"delete-response-private-updated": {
+			server: &fwserver.Server{
+				Provider: &testprovider.Provider{},
+			},
+			request: &fwserver.ApplyResourceChangeRequest{
+				PlannedPrivate: testPrivateFramework,
+				PriorState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				ResourceSchema: testSchema,
+				Resource: &testprovider.Resource{
+					DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
+						diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`))
+
+						resp.Diagnostics.Append(diags...)
+
+						// Must return error to prevent automatic private state clearing
+						resp.Diagnostics.AddError("error summary", "error detail")
+					},
+				},
+			},
+			expectedResponse: &fwserver.ApplyResourceChangeResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewErrorDiagnostic(
+						"error summary",
+						"error detail",
+					),
+				},
+				NewState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				Private: testPrivate,
 			},
 		},
 		"delete-response-newstate": {

diff --git a/internal/fwserver/server_deleteresource.go b/internal/fwserver/server_deleteresource.go
@@ -82,19 +82,42 @@ func (s *Server) DeleteResource(ctx context.Context, req *DeleteResourceRequest,
 		deleteReq.ProviderMeta = *req.ProviderMeta
 	}
 
+	privateProviderData := privatestate.EmptyProviderData(ctx)
+
+	deleteReq.Private = privateProviderData
+	deleteResp.Private = privateProviderData
+
 	if req.PlannedPrivate != nil {
-		deleteReq.Private = req.PlannedPrivate.Provider
+		if req.PlannedPrivate.Provider != nil {
+			deleteReq.Private = req.PlannedPrivate.Provider
+			deleteResp.Private = req.PlannedPrivate.Provider
+		}
+
+		resp.Private = req.PlannedPrivate
 	}
 
 	logging.FrameworkTrace(ctx, "Calling provider defined Resource Delete")
 	req.Resource.Delete(ctx, deleteReq, &deleteResp)
 	logging.FrameworkTrace(ctx, "Called provider defined Resource Delete")
 
 	if !deleteResp.Diagnostics.HasError() {
-		logging.FrameworkTrace(ctx, "No provider defined Delete errors detected, ensuring State is cleared")
+		logging.FrameworkTrace(ctx, "No provider defined Delete errors detected, ensuring State and Priavate are cleared")
 		deleteResp.State.RemoveResource(ctx)
+
+		// Preserve prior behavior of always returning nil.
+		// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/863
+		deleteResp.Private = nil
+		resp.Private = nil
 	}
 
 	resp.Diagnostics = deleteResp.Diagnostics
 	resp.NewState = &deleteResp.State
+
+	if deleteResp.Private != nil {
+		if resp.Private == nil {
+			resp.Private = &privatestate.Data{}
+		}
+
+		resp.Private.Provider = deleteResp.Private
+	}
 }
diff --git a/internal/fwserver/server_deleteresource_test.go b/internal/fwserver/server_deleteresource_test.go
@@ -81,12 +81,35 @@ func TestServerDeleteResource(t *testing.T) {
 		TestProviderMetaAttribute types.String `tfsdk:"test_provider_meta_attribute"`
 	}
 
+	testPrivateFrameworkMap := map[string][]byte{
+		".frameworkKey": []byte(`{"fk": "framework value"}`),
+	}
+
 	testProviderKeyValue := privatestate.MustMarshalToJson(map[string][]byte{
 		"providerKeyOne": []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`),
 	})
 
 	testProviderData := privatestate.MustProviderData(context.Background(), testProviderKeyValue)
 
+	testPrivate := &privatestate.Data{
+		Framework: testPrivateFrameworkMap,
+		Provider:  testProviderData,
+	}
+
+	testPrivateFramework := &privatestate.Data{
+		Framework: testPrivateFrameworkMap,
+	}
+
+	testPrivateProvider := &privatestate.Data{
+		Provider: testProviderData,
+	}
+
+	testEmptyProviderData := privatestate.EmptyProviderData(context.Background())
+
+	testEmptyPrivate := &privatestate.Data{
+		Provider: testEmptyProviderData,
+	}
+
 	testCases := map[string]struct {
 		server           *fwserver.Server
 		request          *fwserver.DeleteResourceRequest
@@ -245,6 +268,7 @@ func TestServerDeleteResource(t *testing.T) {
 					}),
 					Schema: testSchema,
 				},
+				Private: testEmptyPrivate,
 			},
 		},
 		"resource-configure-data": {
@@ -310,6 +334,89 @@ func TestServerDeleteResource(t *testing.T) {
 				NewState: testEmptyState,
 			},
 		},
+		"response-private": {
+			server: &fwserver.Server{
+				Provider: &testprovider.Provider{},
+			},
+			request: &fwserver.DeleteResourceRequest{
+				PriorState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				ResourceSchema: testSchema,
+				Resource: &testprovider.Resource{
+					DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
+						diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`))
+
+						resp.Diagnostics.Append(diags...)
+
+						// Must return error to prevent automatic private state clearing
+						resp.Diagnostics.AddError("error summary", "error detail")
+					},
+				},
+			},
+			expectedResponse: &fwserver.DeleteResourceResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewErrorDiagnostic(
+						"error summary",
+						"error detail",
+					),
+				},
+				NewState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				Private: testPrivateProvider,
+			},
+		},
+		"response-private-updated": {
+			server: &fwserver.Server{
+				Provider: &testprovider.Provider{},
+			},
+			request: &fwserver.DeleteResourceRequest{
+				PlannedPrivate: testPrivateFramework,
+				PriorState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				ResourceSchema: testSchema,
+				Resource: &testprovider.Resource{
+					DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
+						diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`))
+
+						resp.Diagnostics.Append(diags...)
+
+						// Must return error to prevent automatic private state clearing
+						resp.Diagnostics.AddError("error summary", "error detail")
+					},
+				},
+			},
+			expectedResponse: &fwserver.DeleteResourceResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewErrorDiagnostic(
+						"error summary",
+						"error detail",
+					),
+				},
+				NewState: &tfsdk.State{
+					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
+						"test_computed": tftypes.NewValue(tftypes.String, nil),
+						"test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"),
+					}),
+					Schema: testSchema,
+				},
+				Private: testPrivate,
+			},
+		},
 	}
 
 	for name, testCase := range testCases {