Improve dynamic attribute internal validation handling (#1090)

* Remove unused `RequiredWriteOnlyNilsAttributePaths()` function

* Remove dynamic type conditional from write_only_nullification.go

* Add a link to private state page

* Add a return statement after diagnostic

* Add validation handling for dynamic underlying null values

* Remove null check

* Update logic to check null/unknowness in both the container and underlying value

* Add changelog entry

Author: SBGoods

.changes/unreleased/BUG FIXES-20250213-164700.yaml

@@ -0,0 +1,5 @@
+kind: BUG FIXES
+body: 'internal/fwserver: Fixed bug where dynamic attributes would not prompt invalid configuration error messages'
+time: 2025-02-13T16:47:00.4821-05:00
+custom:
+    Issue: "1090"

internal/fwserver/attribute_validation.go

@@ -101,12 +101,30 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
 		return
 	}
 
+	configHasNullValue := attributeConfig.IsNull()
+	configHasUnknownValue := attributeConfig.IsUnknown()
+	// If the value is dynamic, we still need to check if the underlying value is null or unknown
+	if dynamicValuable, isDynamic := attributeConfig.(basetypes.DynamicValuable); !configHasNullValue && !configHasUnknownValue && isDynamic {
+		dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx)
+		resp.Diagnostics.Append(diags...)
+		if diags.HasError() {
+			return
+		}
+		if dynamicConfigVal.IsUnderlyingValueNull() {
+			configHasNullValue = true
+		}
+
+		if dynamicConfigVal.IsUnderlyingValueUnknown() {
+			configHasUnknownValue = true
+		}
+	}
+
 	// Terraform CLI does not automatically perform certain configuration
 	// checks yet. If it eventually does, this logic should remain at least
 	// until Terraform CLI versions 0.12 through the release containing the
 	// checks are considered end-of-life.
 	// Reference: https://github.com/hashicorp/terraform/issues/30669
-	if a.IsComputed() && !a.IsOptional() && !attributeConfig.IsNull() {
+	if a.IsComputed() && !a.IsOptional() && !configHasNullValue {
 		resp.Diagnostics.AddAttributeError(
 			req.AttributePath,
 			"Invalid Configuration for Read-Only Attribute",
@@ -120,7 +138,7 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
 	// until Terraform CLI versions 0.12 through the release containing the
 	// checks are considered end-of-life.
 	// Reference: https://github.com/hashicorp/terraform/issues/30669
-	if a.IsRequired() && attributeConfig.IsNull() {
+	if a.IsRequired() && configHasNullValue {
 		resp.Diagnostics.AddAttributeError(
 			req.AttributePath,
 			"Missing Configuration for Required Attribute",
@@ -134,14 +152,13 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
 	//
 	// Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute
 	// is to upgrade their Terraform CLI version to v1.11.0 or later.
-	if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !attributeConfig.IsNull() {
+	if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !configHasNullValue {
 		resp.Diagnostics.AddAttributeError(
 			req.AttributePath,
 			"WriteOnly Attribute Not Allowed",
 			fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()),
 		)
 	}
-
 	req.AttributeConfig = attributeConfig
 
 	switch attributeWithValidators := a.(type) {
@@ -174,33 +191,13 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
 	AttributeValidateNestedAttributes(ctx, a, req, resp)
 
 	// Show deprecation warnings only for known values.
-	if a.GetDeprecationMessage() != "" && !attributeConfig.IsNull() && !attributeConfig.IsUnknown() {
-		// Dynamic values need to perform more logic to check the config value for null/unknown-ness
-		dynamicValuable, ok := attributeConfig.(basetypes.DynamicValuable)
-		if !ok {
-			resp.Diagnostics.AddAttributeWarning(
-				req.AttributePath,
-				"Attribute Deprecated",
-				a.GetDeprecationMessage(),
-			)
-			return
-		}
-
-		dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx)
-		resp.Diagnostics.Append(diags...)
-		if diags.HasError() {
-			return
-		}
-
-		// For dynamic values, it's possible to be known when only the type is known.
-		// The underlying value can still be null or unknown, so check for that here
-		if !dynamicConfigVal.IsUnderlyingValueNull() && !dynamicConfigVal.IsUnderlyingValueUnknown() {
-			resp.Diagnostics.AddAttributeWarning(
-				req.AttributePath,
-				"Attribute Deprecated",
-				a.GetDeprecationMessage(),
-			)
-		}
+	if a.GetDeprecationMessage() != "" && !configHasNullValue && !configHasUnknownValue {
+		resp.Diagnostics.AddAttributeWarning(
+			req.AttributePath,
+			"Attribute Deprecated",
+			a.GetDeprecationMessage(),
+		)
+		return
 	}
 }
 

internal/fwserver/attribute_validation_test.go

@@ -181,6 +181,29 @@ func TestAttributeValidate(t *testing.T) {
 				},
 			},
 		},
+		"config-computed-dynamic-underlying-null-value": {
+			req: ValidateAttributeRequest{
+				AttributePath: path.Root("test"),
+				Config: tfsdk.Config{
+					Raw: tftypes.NewValue(tftypes.Object{
+						AttributeTypes: map[string]tftypes.Type{
+							"test": tftypes.DynamicPseudoType,
+						},
+					}, map[string]tftypes.Value{
+						"test": tftypes.NewValue(tftypes.String, nil),
+					}),
+					Schema: testschema.Schema{
+						Attributes: map[string]fwschema.Attribute{
+							"test": testschema.Attribute{
+								Computed: true,
+								Type:     types.DynamicType,
+							},
+						},
+					},
+				},
+			},
+			resp: ValidateAttributeResponse{},
+		},
 		"config-optional-computed-null": {
 			req: ValidateAttributeRequest{
 				AttributePath: path.Root("test"),
@@ -331,6 +354,38 @@ func TestAttributeValidate(t *testing.T) {
 			},
 			resp: ValidateAttributeResponse{},
 		},
+		"config-required-dynamic-underlying-null-value": {
+			req: ValidateAttributeRequest{
+				AttributePath: path.Root("test"),
+				Config: tfsdk.Config{
+					Raw: tftypes.NewValue(tftypes.Object{
+						AttributeTypes: map[string]tftypes.Type{
+							"test": tftypes.DynamicPseudoType,
+						},
+					}, map[string]tftypes.Value{
+						"test": tftypes.NewValue(tftypes.String, nil),
+					}),
+					Schema: testschema.Schema{
+						Attributes: map[string]fwschema.Attribute{
+							"test": testschema.Attribute{
+								Required: true,
+								Type:     types.DynamicType,
+							},
+						},
+					},
+				},
+			},
+			resp: ValidateAttributeResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewAttributeErrorDiagnostic(
+						path.Root("test"),
+						"Missing Configuration for Required Attribute",
+						"Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+
+							"Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.",
+					),
+				},
+			},
+		},
 		"no-validation": {
 			req: ValidateAttributeRequest{
 				AttributePath: path.Root("test"),
@@ -1763,6 +1818,42 @@ func TestAttributeValidate(t *testing.T) {
 				},
 			},
 		},
+		"write-only-attr-with-required-dynamic-underlying-null-value": {
+			req: ValidateAttributeRequest{
+				ClientCapabilities: validator.ValidateSchemaClientCapabilities{
+					WriteOnlyAttributesAllowed: true,
+				},
+				AttributePath: path.Root("test"),
+				Config: tfsdk.Config{
+					Raw: tftypes.NewValue(tftypes.Object{
+						AttributeTypes: map[string]tftypes.Type{
+							"test": tftypes.DynamicPseudoType,
+						},
+					}, map[string]tftypes.Value{
+						"test": tftypes.NewValue(tftypes.String, nil),
+					}),
+					Schema: testschema.Schema{
+						Attributes: map[string]fwschema.Attribute{
+							"test": testschema.Attribute{
+								Type:      types.DynamicType,
+								WriteOnly: true,
+								Required:  true,
+							},
+						},
+					},
+				},
+			},
+			resp: ValidateAttributeResponse{
+				Diagnostics: diag.Diagnostics{
+					diag.NewAttributeErrorDiagnostic(
+						path.Root("test"),
+						"Missing Configuration for Required Attribute",
+						"Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+
+							"Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.",
+					),
+				},
+			},
+		},
 		"write-only-attr-with-optional": {
 			req: ValidateAttributeRequest{
 				ClientCapabilities: validator.ValidateSchemaClientCapabilities{

internal/fwserver/server_planresourcechange.go

@@ -13,7 +13,6 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
 	"github.com/hashicorp/terraform-plugin-framework/diag"
-	"github.com/hashicorp/terraform-plugin-framework/internal/fromtftypes"
 	"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
 	"github.com/hashicorp/terraform-plugin-framework/internal/fwschemadata"
 	"github.com/hashicorp/terraform-plugin-framework/internal/logging"
@@ -338,6 +337,7 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
 				"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.",
 		)
+		return
 	}
 
 	// Set any write-only attributes in the plan to null
@@ -513,67 +513,6 @@ func NormaliseRequiresReplace(ctx context.Context, rs path.Paths) path.Paths {
 	return ret[:j]
 }
 
-// RequiredWriteOnlyNilsAttributePaths returns a tftypes.Walk() function
-// that populates reqWriteOnlyNilsPaths with the paths of Required and WriteOnly
-// attributes that have a null value.
-func RequiredWriteOnlyNilsAttributePaths(ctx context.Context, schema fwschema.Schema, reqWriteOnlyNilsPaths *path.Paths) func(path *tftypes.AttributePath, value tftypes.Value) (bool, error) {
-	return func(attrPath *tftypes.AttributePath, value tftypes.Value) (bool, error) {
-		// we are only modifying attributes, not the entire resource
-		if len(attrPath.Steps()) < 1 {
-			return true, nil
-		}
-
-		ctx = logging.FrameworkWithAttributePath(ctx, attrPath.String())
-
-		attribute, err := schema.AttributeAtTerraformPath(ctx, attrPath)
-
-		if err != nil {
-			if errors.Is(err, fwschema.ErrPathInsideAtomicAttribute) {
-				// atomic attributes can be nested block objects that contain child writeOnly attributes
-				return true, nil
-			}
-
-			if errors.Is(err, fwschema.ErrPathIsBlock) {
-				// blocks do not have the writeOnly field but can contain child writeOnly attributes
-				return true, nil
-			}
-
-			if errors.Is(err, fwschema.ErrPathInsideDynamicAttribute) {
-				return false, nil
-			}
-
-			logging.FrameworkError(ctx, "couldn't find attribute in resource schema")
-			return false, fmt.Errorf("couldn't find attribute in resource schema: %w", err)
-		}
-
-		if attribute.IsWriteOnly() {
-			if attribute.IsRequired() && value.IsNull() {
-				fwPath, diags := fromtftypes.AttributePath(ctx, attrPath, schema)
-				if diags.HasError() {
-					for _, diagErr := range diags.Errors() {
-						logging.FrameworkError(ctx,
-							"Error converting tftypes.AttributePath to path.Path",
-							map[string]any{
-								logging.KeyError: diagErr.Detail(),
-							},
-						)
-					}
-
-					return false, fmt.Errorf("couldn't convert tftypes.AttributePath to path.Path")
-				}
-				reqWriteOnlyNilsPaths.Append(fwPath)
-
-				// if the value is nil, there is no need to continue walking
-				return false, nil
-			}
-			// check for any writeOnly child attributes
-			return true, nil
-		}
-
-		return false, nil
-	}
-}
-
 // planToState returns a *tfsdk.State with a copied value from a tfsdk.Plan.
 func planToState(plan tfsdk.Plan) *tfsdk.State {
 	return &tfsdk.State{