Support plan modifiers returning warning and error diagnostics on res…

…ource destruction with Terraform 1.3 and later

Reference: #364

.changelog/pending.txt

@@ -0,0 +1,7 @@
+```release-note:feature
+Support plan modifiers returning warning and error diagnostics on resource destruction with Terraform 1.3 and later
+```
+
+```release-note:breaking-change
+tfsdk: The `PlanResourceChange` RPC on destroy is now enabled. To prevent unexpected Terraform errors, the framework attempts to catch errant provider logic in plan modifiers when destroying. Resource level plan modifiers may require updates to handle a completely null proposed new state (plan) and ensure it remains completely null on resource destruction.
+```

internal/fwserver/server_capabilities.go

@@ -0,0 +1,14 @@
+package fwserver
+
+// ServerCapabilities is a combination of tfprotov5.ServerCapabilities and
+// tfprotov6.ServerCapabilties, which may diverge over time. If that happens,
+// the toproto5 conversion logic will handle the appropriate filtering and the
+// proto5server/fwserver logic will need to account for missing features.
+type ServerCapabilities struct {
+	// PlanDestroy signals that the provider is ready for the
+	// PlanResourceChange RPC on resource destruction.
+	//
+	// This should always be enabled in framework providers and requires
+	// Terraform 1.3 or later.
+	PlanDestroy bool
+}

internal/fwserver/server_getproviderschema.go

@@ -14,15 +14,20 @@ type GetProviderSchemaRequest struct{}
 // GetProviderSchemaResponse is the framework server response for the
 // GetProviderSchema RPC.
 type GetProviderSchemaResponse struct {
-	Provider          *tfsdk.Schema
-	ProviderMeta      *tfsdk.Schema
-	ResourceSchemas   map[string]*tfsdk.Schema
-	DataSourceSchemas map[string]*tfsdk.Schema
-	Diagnostics       diag.Diagnostics
+	ServerCapabilities *ServerCapabilities
+	Provider           *tfsdk.Schema
+	ProviderMeta       *tfsdk.Schema
+	ResourceSchemas    map[string]*tfsdk.Schema
+	DataSourceSchemas  map[string]*tfsdk.Schema
+	Diagnostics        diag.Diagnostics
 }
 
 // GetProviderSchema implements the framework server GetProviderSchema RPC.
 func (s *Server) GetProviderSchema(ctx context.Context, req *GetProviderSchemaRequest, resp *GetProviderSchemaResponse) {
+	resp.ServerCapabilities = &ServerCapabilities{
+		PlanDestroy: true,
+	}
+
 	providerSchema, diags := s.ProviderSchema(ctx)
 
 	resp.Diagnostics.Append(diags...)

internal/fwserver/server_getproviderschema_test.go

@@ -28,6 +28,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 				DataSourceSchemas: map[string]*tfsdk.Schema{},
 				Provider:          &tfsdk.Schema{},
 				ResourceSchemas:   map[string]*tfsdk.Schema{},
+				ServerCapabilities: &fwserver.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"datasourceschemas": {
@@ -85,6 +88,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 				},
 				Provider:        &tfsdk.Schema{},
 				ResourceSchemas: map[string]*tfsdk.Schema{},
+				ServerCapabilities: &fwserver.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"provider": {
@@ -114,6 +120,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfsdk.Schema{},
+				ServerCapabilities: &fwserver.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"providermeta": {
@@ -145,6 +154,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfsdk.Schema{},
+				ServerCapabilities: &fwserver.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"resourceschemas": {
@@ -202,6 +214,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 						},
 					},
 				},
+				ServerCapabilities: &fwserver.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 	}

internal/fwserver/server_planresourcechange.go

@@ -219,6 +219,16 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
 
 	// Ensure deterministic RequiresReplace by sorting and deduplicating
 	resp.RequiresReplace = NormaliseRequiresReplace(ctx, resp.RequiresReplace)
+
+	// If this was a destroy resource plan, ensure the plan remained null.
+	if req.ProposedNewState.Raw.IsNull() && !resp.PlannedState.Raw.IsNull() {
+		resp.Diagnostics.AddError(
+			"Unexpected Planned Resource State on Destroy",
+			"The Terraform Provider unexpectedly returned resource state data when the resource was planned for destruction. "+
+				"This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n"+
+				"Ensure all resource plan modifiers do not attempt to change resource plan data from being a null value if the request plan is a null value.",
+		)
+	}
 }
 
 func MarkComputedNilsAsUnknown(ctx context.Context, config tftypes.Value, resourceSchema tfsdk.Schema) func(*tftypes.AttributePath, tftypes.Value) (tftypes.Value, error) {

internal/fwserver/server_planresourcechange_test.go

@@ -1034,44 +1034,26 @@ func TestServerPlanResourceChange(t *testing.T) {
 					NewResourceMethod: func(_ context.Context, _ tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) {
 						return &testprovider.ResourceWithModifyPlan{
 							ModifyPlanMethod: func(ctx context.Context, req tfsdk.ModifyResourcePlanRequest, resp *tfsdk.ModifyResourcePlanResponse) {
-								var data testSchemaData
-
-								resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
-
-								// TODO: This is invalid logic to run during deletion, but the framework
-								// does not currently prevent you from doing something like this. When
-								// the protocol implements PlanResourceChange on destroy support, doing
-								// anything with the PlannedState that is not equal to the
-								// empty request ProposedNewState should raise an error.
-								// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-								data.TestComputed = types.String{Value: "test-plannedstate-value"}
-
-								resp.Diagnostics.Append(resp.Plan.Set(ctx, &data)...)
+								// This is invalid logic to run during deletion.
+								resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root("test_computed"), types.String{Value: "test-plannedstate-value"})...)
 							},
 						}, nil
 					},
 				},
 			},
 			expectedResponse: &fwserver.PlanResourceChangeResponse{
-				// TODO: When the protocol implements PlanResourceChange on destroy support,
-				// PlannedState should always be empty or an error should be raised while
-				// still returning the empty state.
-				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-				// Diagnostics: diag.Diagnostics{ /* friendlier error diagnostic */ }
-				// PlannedState: testEmptyState,
 				Diagnostics: diag.Diagnostics{
-					diag.WithPath(
-						path.Empty(),
-						diag.NewErrorDiagnostic(
-							"Value Conversion Error",
-							"An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:\n\nunhandled null value",
-						),
+					diag.NewErrorDiagnostic(
+						"Unexpected Planned Resource State on Destroy",
+						"The Terraform Provider unexpectedly returned resource state data when the resource was planned for destruction. "+
+							"This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n"+
+							"Ensure all resource plan modifiers do not attempt to change resource plan data from being a null value if the request plan is a null value.",
 					),
 				},
 				PlannedState: &tfsdk.State{
 					Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
 						"test_computed": tftypes.NewValue(tftypes.String, "test-plannedstate-value"),
-						"test_required": tftypes.NewValue(tftypes.String, ""),
+						"test_required": tftypes.NewValue(tftypes.String, nil),
 					}),
 					Schema: testSchema,
 				},

internal/proto5server/server_getproviderschema_test.go

@@ -91,6 +91,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					Block: &tfprotov5.SchemaBlock{},
 				},
 				ResourceSchemas: map[string]*tfprotov5.Schema{},
+				ServerCapabilities: &tfprotov5.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"provider": {
@@ -125,6 +128,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfprotov5.Schema{},
+				ServerCapabilities: &tfprotov5.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"providermeta": {
@@ -163,6 +169,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfprotov5.Schema{},
+				ServerCapabilities: &tfprotov5.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"resourceschemas": {
@@ -230,6 +239,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 						},
 					},
 				},
+				ServerCapabilities: &tfprotov5.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 	}

internal/proto5server/server_planresourcechange_test.go

@@ -529,19 +529,8 @@ func TestServerPlanResourceChange(t *testing.T) {
 									NewResourceMethod: func(_ context.Context, _ tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) {
 										return &testprovider.ResourceWithModifyPlan{
 											ModifyPlanMethod: func(ctx context.Context, req tfsdk.ModifyResourcePlanRequest, resp *tfsdk.ModifyResourcePlanResponse) {
-												var data testSchemaData
-
-												resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
-
-												// TODO: This is invalid logic to run during deletion, but the framework
-												// does not currently prevent you from doing something like this. When
-												// the protocol implements PlanResourceChange on destroy support, doing
-												// anything with the PlannedState that is not equal to the
-												// empty request ProposedNewState should raise an error.
-												// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-												data.TestComputed = types.String{Value: "test-plannedstate-value"}
-
-												resp.Diagnostics.Append(resp.Plan.Set(ctx, &data)...)
+												// This is invalid logic to run during deletion.
+												resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root("test_computed"), types.String{Value: "test-plannedstate-value"})...)
 											},
 										}, nil
 									},
@@ -560,22 +549,18 @@ func TestServerPlanResourceChange(t *testing.T) {
 				TypeName: "test_resource",
 			},
 			expectedResponse: &tfprotov5.PlanResourceChangeResponse{
-				// TODO: When the protocol implements PlanResourceChange on destroy support,
-				// PlannedState should always be empty or an error should be raised while
-				// still returning the empty state.
-				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-				// Diagnostics: []*tfprotov5.Diagnostic{ /* friendlier error diagnostic */ }
-				// PlannedState: &testEmptyDynamicValue,
 				Diagnostics: []*tfprotov5.Diagnostic{
 					{
 						Severity: tfprotov5.DiagnosticSeverityError,
-						Summary:  "Value Conversion Error",
-						Detail:   "An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:\n\nunhandled null value",
+						Summary:  "Unexpected Planned Resource State on Destroy",
+						Detail: "The Terraform Provider unexpectedly returned resource state data when the resource was planned for destruction. " +
+							"This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n" +
+							"Ensure all resource plan modifiers do not attempt to change resource plan data from being a null value if the request plan is a null value.",
 					},
 				},
 				PlannedState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{
 					"test_computed": tftypes.NewValue(tftypes.String, "test-plannedstate-value"),
-					"test_required": tftypes.NewValue(tftypes.String, ""),
+					"test_required": tftypes.NewValue(tftypes.String, nil),
 				}),
 			},
 		},

internal/proto6server/server_getproviderschema_test.go

@@ -91,6 +91,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					Block: &tfprotov6.SchemaBlock{},
 				},
 				ResourceSchemas: map[string]*tfprotov6.Schema{},
+				ServerCapabilities: &tfprotov6.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"provider": {
@@ -125,6 +128,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfprotov6.Schema{},
+				ServerCapabilities: &tfprotov6.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"providermeta": {
@@ -163,6 +169,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 					},
 				},
 				ResourceSchemas: map[string]*tfprotov6.Schema{},
+				ServerCapabilities: &tfprotov6.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 		"resourceschemas": {
@@ -230,6 +239,9 @@ func TestServerGetProviderSchema(t *testing.T) {
 						},
 					},
 				},
+				ServerCapabilities: &tfprotov6.ServerCapabilities{
+					PlanDestroy: true,
+				},
 			},
 		},
 	}

internal/proto6server/server_planresourcechange_test.go

@@ -529,19 +529,8 @@ func TestServerPlanResourceChange(t *testing.T) {
 									NewResourceMethod: func(_ context.Context, _ tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) {
 										return &testprovider.ResourceWithModifyPlan{
 											ModifyPlanMethod: func(ctx context.Context, req tfsdk.ModifyResourcePlanRequest, resp *tfsdk.ModifyResourcePlanResponse) {
-												var data testSchemaData
-
-												resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
-
-												// TODO: This is invalid logic to run during deletion, but the framework
-												// does not currently prevent you from doing something like this. When
-												// the protocol implements PlanResourceChange on destroy support, doing
-												// anything with the PlannedState that is not equal to the
-												// empty request ProposedNewState should raise an error.
-												// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-												data.TestComputed = types.String{Value: "test-plannedstate-value"}
-
-												resp.Diagnostics.Append(resp.Plan.Set(ctx, &data)...)
+												// This is invalid logic to run during deletion.
+												resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root("test_computed"), types.String{Value: "test-plannedstate-value"})...)
 											},
 										}, nil
 									},
@@ -560,22 +549,18 @@ func TestServerPlanResourceChange(t *testing.T) {
 				TypeName: "test_resource",
 			},
 			expectedResponse: &tfprotov6.PlanResourceChangeResponse{
-				// TODO: When the protocol implements PlanResourceChange on destroy support,
-				// PlannedState should always be empty or an error should be raised while
-				// still returning the empty state.
-				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/364
-				// Diagnostics: []*tfprotov6.Diagnostic{ /* friendlier error diagnostic */ }
-				// PlannedState: &testEmptyDynamicValue,
 				Diagnostics: []*tfprotov6.Diagnostic{
 					{
 						Severity: tfprotov6.DiagnosticSeverityError,
-						Summary:  "Value Conversion Error",
-						Detail:   "An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:\n\nunhandled null value",
+						Summary:  "Unexpected Planned Resource State on Destroy",
+						Detail: "The Terraform Provider unexpectedly returned resource state data when the resource was planned for destruction. " +
+							"This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n" +
+							"Ensure all resource plan modifiers do not attempt to change resource plan data from being a null value if the request plan is a null value.",
 					},
 				},
 				PlannedState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{
 					"test_computed": tftypes.NewValue(tftypes.String, "test-plannedstate-value"),
-					"test_required": tftypes.NewValue(tftypes.String, ""),
+					"test_required": tftypes.NewValue(tftypes.String, nil),
 				}),
 			},
 		},