Skip to content

Commit 0724df1

Browse files
SBGoodsSBGoods
authored
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
1 parent d91ccc1 commit 0724df1

File tree

9 files changed

+142
-435
lines changed

9 files changed

+142
-435
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
kind: BUG FIXES
2+
body: 'internal/fwserver: Fixed bug where dynamic attributes would not prompt invalid configuration error messages'
3+
time: 2025-02-13T16:47:00.4821-05:00
4+
custom:
5+
Issue: "1090"

‎internal/fwserver/attribute_validation.go‎

Lines changed: 28 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,30 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
101101
return
102102
}
103103

104+
configHasNullValue := attributeConfig.IsNull()
105+
configHasUnknownValue := attributeConfig.IsUnknown()
106+
// If the value is dynamic, we still need to check if the underlying value is null or unknown
107+
if dynamicValuable, isDynamic := attributeConfig.(basetypes.DynamicValuable); !configHasNullValue && !configHasUnknownValue && isDynamic {
108+
dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx)
109+
resp.Diagnostics.Append(diags...)
110+
if diags.HasError() {
111+
return
112+
}
113+
if dynamicConfigVal.IsUnderlyingValueNull() {
114+
configHasNullValue = true
115+
}
116+
117+
if dynamicConfigVal.IsUnderlyingValueUnknown() {
118+
configHasUnknownValue = true
119+
}
120+
}
121+
104122
// Terraform CLI does not automatically perform certain configuration
105123
// checks yet. If it eventually does, this logic should remain at least
106124
// until Terraform CLI versions 0.12 through the release containing the
107125
// checks are considered end-of-life.
108126
// Reference: https://github.com/hashicorp/terraform/issues/30669
109-
if a.IsComputed() && !a.IsOptional() && !attributeConfig.IsNull() {
127+
if a.IsComputed() && !a.IsOptional() && !configHasNullValue {
110128
resp.Diagnostics.AddAttributeError(
111129
req.AttributePath,
112130
"Invalid Configuration for Read-Only Attribute",
@@ -120,7 +138,7 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
120138
// until Terraform CLI versions 0.12 through the release containing the
121139
// checks are considered end-of-life.
122140
// Reference: https://github.com/hashicorp/terraform/issues/30669
123-
if a.IsRequired() && attributeConfig.IsNull() {
141+
if a.IsRequired() && configHasNullValue {
124142
resp.Diagnostics.AddAttributeError(
125143
req.AttributePath,
126144
"Missing Configuration for Required Attribute",
@@ -134,14 +152,13 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
134152
//
135153
// 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
136154
// is to upgrade their Terraform CLI version to v1.11.0 or later.
137-
if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !attributeConfig.IsNull() {
155+
if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !configHasNullValue {
138156
resp.Diagnostics.AddAttributeError(
139157
req.AttributePath,
140158
"WriteOnly Attribute Not Allowed",
141159
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()),
142160
)
143161
}
144-
145162
req.AttributeConfig = attributeConfig
146163

147164
switch attributeWithValidators := a.(type) {
@@ -174,33 +191,13 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt
174191
AttributeValidateNestedAttributes(ctx, a, req, resp)
175192

176193
// Show deprecation warnings only for known values.
177-
if a.GetDeprecationMessage() != "" && !attributeConfig.IsNull() && !attributeConfig.IsUnknown() {
178-
// Dynamic values need to perform more logic to check the config value for null/unknown-ness
179-
dynamicValuable, ok := attributeConfig.(basetypes.DynamicValuable)
180-
if !ok {
181-
resp.Diagnostics.AddAttributeWarning(
182-
req.AttributePath,
183-
"Attribute Deprecated",
184-
a.GetDeprecationMessage(),
185-
)
186-
return
187-
}
188-
189-
dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx)
190-
resp.Diagnostics.Append(diags...)
191-
if diags.HasError() {
192-
return
193-
}
194-
195-
// For dynamic values, it's possible to be known when only the type is known.
196-
// The underlying value can still be null or unknown, so check for that here
197-
if !dynamicConfigVal.IsUnderlyingValueNull() && !dynamicConfigVal.IsUnderlyingValueUnknown() {
198-
resp.Diagnostics.AddAttributeWarning(
199-
req.AttributePath,
200-
"Attribute Deprecated",
201-
a.GetDeprecationMessage(),
202-
)
203-
}
194+
if a.GetDeprecationMessage() != "" && !configHasNullValue && !configHasUnknownValue {
195+
resp.Diagnostics.AddAttributeWarning(
196+
req.AttributePath,
197+
"Attribute Deprecated",
198+
a.GetDeprecationMessage(),
199+
)
200+
return
204201
}
205202
}
206203

‎internal/fwserver/attribute_validation_test.go‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,29 @@ func TestAttributeValidate(t *testing.T) {
181181
},
182182
},
183183
},
184+
"config-computed-dynamic-underlying-null-value": {
185+
req: ValidateAttributeRequest{
186+
AttributePath: path.Root("test"),
187+
Config: tfsdk.Config{
188+
Raw: tftypes.NewValue(tftypes.Object{
189+
AttributeTypes: map[string]tftypes.Type{
190+
"test": tftypes.DynamicPseudoType,
191+
},
192+
}, map[string]tftypes.Value{
193+
"test": tftypes.NewValue(tftypes.String, nil),
194+
}),
195+
Schema: testschema.Schema{
196+
Attributes: map[string]fwschema.Attribute{
197+
"test": testschema.Attribute{
198+
Computed: true,
199+
Type: types.DynamicType,
200+
},
201+
},
202+
},
203+
},
204+
},
205+
resp: ValidateAttributeResponse{},
206+
},
184207
"config-optional-computed-null": {
185208
req: ValidateAttributeRequest{
186209
AttributePath: path.Root("test"),
@@ -331,6 +354,38 @@ func TestAttributeValidate(t *testing.T) {
331354
},
332355
resp: ValidateAttributeResponse{},
333356
},
357+
"config-required-dynamic-underlying-null-value": {
358+
req: ValidateAttributeRequest{
359+
AttributePath: path.Root("test"),
360+
Config: tfsdk.Config{
361+
Raw: tftypes.NewValue(tftypes.Object{
362+
AttributeTypes: map[string]tftypes.Type{
363+
"test": tftypes.DynamicPseudoType,
364+
},
365+
}, map[string]tftypes.Value{
366+
"test": tftypes.NewValue(tftypes.String, nil),
367+
}),
368+
Schema: testschema.Schema{
369+
Attributes: map[string]fwschema.Attribute{
370+
"test": testschema.Attribute{
371+
Required: true,
372+
Type: types.DynamicType,
373+
},
374+
},
375+
},
376+
},
377+
},
378+
resp: ValidateAttributeResponse{
379+
Diagnostics: diag.Diagnostics{
380+
diag.NewAttributeErrorDiagnostic(
381+
path.Root("test"),
382+
"Missing Configuration for Required Attribute",
383+
"Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+
384+
"Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.",
385+
),
386+
},
387+
},
388+
},
334389
"no-validation": {
335390
req: ValidateAttributeRequest{
336391
AttributePath: path.Root("test"),
@@ -1763,6 +1818,42 @@ func TestAttributeValidate(t *testing.T) {
17631818
},
17641819
},
17651820
},
1821+
"write-only-attr-with-required-dynamic-underlying-null-value": {
1822+
req: ValidateAttributeRequest{
1823+
ClientCapabilities: validator.ValidateSchemaClientCapabilities{
1824+
WriteOnlyAttributesAllowed: true,
1825+
},
1826+
AttributePath: path.Root("test"),
1827+
Config: tfsdk.Config{
1828+
Raw: tftypes.NewValue(tftypes.Object{
1829+
AttributeTypes: map[string]tftypes.Type{
1830+
"test": tftypes.DynamicPseudoType,
1831+
},
1832+
}, map[string]tftypes.Value{
1833+
"test": tftypes.NewValue(tftypes.String, nil),
1834+
}),
1835+
Schema: testschema.Schema{
1836+
Attributes: map[string]fwschema.Attribute{
1837+
"test": testschema.Attribute{
1838+
Type: types.DynamicType,
1839+
WriteOnly: true,
1840+
Required: true,
1841+
},
1842+
},
1843+
},
1844+
},
1845+
},
1846+
resp: ValidateAttributeResponse{
1847+
Diagnostics: diag.Diagnostics{
1848+
diag.NewAttributeErrorDiagnostic(
1849+
path.Root("test"),
1850+
"Missing Configuration for Required Attribute",
1851+
"Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+
1852+
"Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.",
1853+
),
1854+
},
1855+
},
1856+
},
17661857
"write-only-attr-with-optional": {
17671858
req: ValidateAttributeRequest{
17681859
ClientCapabilities: validator.ValidateSchemaClientCapabilities{

‎internal/fwserver/server_planresourcechange.go‎

Lines changed: 1 addition & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
"github.com/hashicorp/terraform-plugin-framework/attr"
1515
"github.com/hashicorp/terraform-plugin-framework/diag"
16-
"github.com/hashicorp/terraform-plugin-framework/internal/fromtftypes"
1716
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
1817
"github.com/hashicorp/terraform-plugin-framework/internal/fwschemadata"
1918
"github.com/hashicorp/terraform-plugin-framework/internal/logging"
@@ -338,6 +337,7 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
338337
"This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n"+
339338
"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.",
340339
)
340+
return
341341
}
342342

343343
// Set any write-only attributes in the plan to null
@@ -513,67 +513,6 @@ func NormaliseRequiresReplace(ctx context.Context, rs path.Paths) path.Paths {
513513
return ret[:j]
514514
}
515515

516-
// RequiredWriteOnlyNilsAttributePaths returns a tftypes.Walk() function
517-
// that populates reqWriteOnlyNilsPaths with the paths of Required and WriteOnly
518-
// attributes that have a null value.
519-
func RequiredWriteOnlyNilsAttributePaths(ctx context.Context, schema fwschema.Schema, reqWriteOnlyNilsPaths *path.Paths) func(path *tftypes.AttributePath, value tftypes.Value) (bool, error) {
520-
return func(attrPath *tftypes.AttributePath, value tftypes.Value) (bool, error) {
521-
// we are only modifying attributes, not the entire resource
522-
if len(attrPath.Steps()) < 1 {
523-
return true, nil
524-
}
525-
526-
ctx = logging.FrameworkWithAttributePath(ctx, attrPath.String())
527-
528-
attribute, err := schema.AttributeAtTerraformPath(ctx, attrPath)
529-
530-
if err != nil {
531-
if errors.Is(err, fwschema.ErrPathInsideAtomicAttribute) {
532-
// atomic attributes can be nested block objects that contain child writeOnly attributes
533-
return true, nil
534-
}
535-
536-
if errors.Is(err, fwschema.ErrPathIsBlock) {
537-
// blocks do not have the writeOnly field but can contain child writeOnly attributes
538-
return true, nil
539-
}
540-
541-
if errors.Is(err, fwschema.ErrPathInsideDynamicAttribute) {
542-
return false, nil
543-
}
544-
545-
logging.FrameworkError(ctx, "couldn't find attribute in resource schema")
546-
return false, fmt.Errorf("couldn't find attribute in resource schema: %w", err)
547-
}
548-
549-
if attribute.IsWriteOnly() {
550-
if attribute.IsRequired() && value.IsNull() {
551-
fwPath, diags := fromtftypes.AttributePath(ctx, attrPath, schema)
552-
if diags.HasError() {
553-
for _, diagErr := range diags.Errors() {
554-
logging.FrameworkError(ctx,
555-
"Error converting tftypes.AttributePath to path.Path",
556-
map[string]any{
557-
logging.KeyError: diagErr.Detail(),
558-
},
559-
)
560-
}
561-
562-
return false, fmt.Errorf("couldn't convert tftypes.AttributePath to path.Path")
563-
}
564-
reqWriteOnlyNilsPaths.Append(fwPath)
565-
566-
// if the value is nil, there is no need to continue walking
567-
return false, nil
568-
}
569-
// check for any writeOnly child attributes
570-
return true, nil
571-
}
572-
573-
return false, nil
574-
}
575-
}
576-
577516
// planToState returns a *tfsdk.State with a copied value from a tfsdk.Plan.
578517
func planToState(plan tfsdk.Plan) *tfsdk.State {
579518
return &tfsdk.State{

0 commit comments

Comments
 (0)