resource: Prevent private state data loss on Delete method error (#864)

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 24, 2023 · a6c07c2 · a6c07c2
1 parent cd4d1ec
commit a6c07c2
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 {