From cc49ffd09e5fb570e8921599b46309ca785baf6c Mon Sep 17 00:00:00 2001 From: Miles Yucht Date: Thu, 5 Dec 2024 09:16:14 +0100 Subject: [PATCH] [Internal] Add ConvertToAttribute() to convert blocks in a resource/data source schema to attributes (#4284) ## Changes Blocks in the Plugin Framework cannot be computed. Terraform checks that the number of blocks in the plan matches the number in the config (https://github.com/hashicorp/terraform/blob/81441564dd29b4aa5fb9576323ef90b9787d1967/internal/plans/objchange/plan_valid.go#L115). Today, for compatibility with existing resources in the TF provider implemented with the SDKv2, we treat nested structures as lists with a single element. As a result, this precludes any nested structures from being read-only (since those would be converted into ListNestedBlocks, which cannot be computed). This blocks databricks_app from being implemented on the plugin framework, as its app_status and compute_status fields are objects and not primitives. Note that TF recommends computing computed-only blocks to attributes [here](https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/blocks-computed). Other useful discussions can be found [here](https://discuss.hashicorp.com/t/set-list-attribute-migration-from-sdkv2-to-framework/56472) and [here](https://discuss.hashicorp.com/t/optional-computed-block-handling-in-plugin-framework/56337). The migration guide for blocks can be found [here](https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/blocks). As an intermediate step, we add `ConvertToAttribute()` in `CustomizableSchema`. This allows resource maintainers to convert specific, known read-only blocks to attributes in accordance with [the migration guide](https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/blocks-computed), which then can be marked as read-only. As blocks can contain other blocks, this method also recursively converts nested blocks into nested attributes. This is based on https://github.com/databricks/terraform-provider-databricks/pull/4283, so we should wait for that PR to merge & rebase this before merging this. ## Tests Unit tests validate that ListNestedBlocks are converted to ListNestedAttributes and that SingleNestedBlocks are converted to SingleNestedAttributes. --- .../pluginfw/tfschema/attribute_converter.go | 6 + .../pluginfw/tfschema/block_builder.go | 6 + .../pluginfw/tfschema/customizable_schema.go | 30 +++ .../tfschema/customizable_schema_test.go | 217 ++++++++++++++++++ .../tfschema/list_nested_attribute.go | 10 +- .../pluginfw/tfschema/list_nested_block.go | 27 +++ .../pluginfw/tfschema/nested_block_object.go | 13 ++ .../pluginfw/tfschema/single_nested_block.go | 25 ++ 8 files changed, 329 insertions(+), 5 deletions(-) create mode 100644 internal/providers/pluginfw/tfschema/attribute_converter.go diff --git a/internal/providers/pluginfw/tfschema/attribute_converter.go b/internal/providers/pluginfw/tfschema/attribute_converter.go new file mode 100644 index 0000000000..db2b1bda5c --- /dev/null +++ b/internal/providers/pluginfw/tfschema/attribute_converter.go @@ -0,0 +1,6 @@ +package tfschema + +type BlockToAttributeConverter interface { + // ConvertBlockToAttribute converts a contained block to its corresponding attribute type. + ConvertBlockToAttribute(string) BaseSchemaBuilder +} diff --git a/internal/providers/pluginfw/tfschema/block_builder.go b/internal/providers/pluginfw/tfschema/block_builder.go index abf3f38cb6..33f3dae67a 100644 --- a/internal/providers/pluginfw/tfschema/block_builder.go +++ b/internal/providers/pluginfw/tfschema/block_builder.go @@ -10,6 +10,12 @@ import ( // This common interface prevents us from keeping two copies of StructToSchema and CustomizableSchema. type BlockBuilder interface { BaseSchemaBuilder + + // ToAttribute converts a block to its corresponding attribute type. Currently, ResourceStructToSchema converts all + // nested struct fields and slices to blocks. This method is used to convert those blocks to their corresponding + // attribute type. The resulting attribute will not have any of the Computed/Optional/Required/Sensitive flags set. + ToAttribute() AttributeBuilder + BuildDataSourceBlock() dataschema.Block BuildResourceBlock() schema.Block } diff --git a/internal/providers/pluginfw/tfschema/customizable_schema.go b/internal/providers/pluginfw/tfschema/customizable_schema.go index be297a28bb..4ef249cf43 100644 --- a/internal/providers/pluginfw/tfschema/customizable_schema.go +++ b/internal/providers/pluginfw/tfschema/customizable_schema.go @@ -10,6 +10,7 @@ import ( ) // CustomizableSchema is a wrapper struct on top of BaseSchemaBuilder that can be used to navigate through nested schema add customizations. +// The methods of CustomizableSchema that modify the underlying schema should return the same CustomizableSchema object to allow chaining. type CustomizableSchema struct { attr BaseSchemaBuilder } @@ -199,6 +200,35 @@ func (s *CustomizableSchema) SetReadOnly(path ...string) *CustomizableSchema { return s } +// ConvertToAttribute converts the last element of the path from a block to an attribute. +// It panics if the path is empty, if the path does not exist in the schema, or if the path +// points to an attribute, not a block. +func (s *CustomizableSchema) ConvertToAttribute(path ...string) *CustomizableSchema { + if len(path) == 0 { + panic(fmt.Errorf("ConvertToAttribute called on root schema. %s", common.TerraformBugErrorMessage)) + } + field := path[len(path)-1] + + cb := func(attr BaseSchemaBuilder) BaseSchemaBuilder { + switch a := attr.(type) { + case BlockToAttributeConverter: + return a.ConvertBlockToAttribute(field) + default: + panic(fmt.Errorf("ConvertToAttribute called on invalid attribute type: %s. %s", reflect.TypeOf(attr).String(), common.TerraformBugErrorMessage)) + } + } + + // We have to go only as far as the second-to-last entry, since we need to change the parent schema + // by moving the last entry from a block to an attribute. + if len(path) == 1 { + s.attr = cb(s.attr) + } else { + navigateSchemaWithCallback(&s.attr, cb, path[0:len(path)-1]...) + } + + return s +} + // navigateSchemaWithCallback navigates through schema attributes and executes callback on the target, panics if path does not exist or invalid. func navigateSchemaWithCallback(s *BaseSchemaBuilder, cb func(BaseSchemaBuilder) BaseSchemaBuilder, path ...string) (BaseSchemaBuilder, error) { currentScm := s diff --git a/internal/providers/pluginfw/tfschema/customizable_schema_test.go b/internal/providers/pluginfw/tfschema/customizable_schema_test.go index 262d041fae..512e3d93a1 100644 --- a/internal/providers/pluginfw/tfschema/customizable_schema_test.go +++ b/internal/providers/pluginfw/tfschema/customizable_schema_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" @@ -175,3 +176,219 @@ func TestCustomizeSchema_SetComputed_PanicOnBlock(t *testing.T) { }) }) } + +type mockPlanModifier struct{} + +// Description implements planmodifier.List. +func (m mockPlanModifier) Description(context.Context) string { + panic("unimplemented") +} + +// MarkdownDescription implements planmodifier.List. +func (m mockPlanModifier) MarkdownDescription(context.Context) string { + panic("unimplemented") +} + +// PlanModifyList implements planmodifier.List. +func (m mockPlanModifier) PlanModifyList(context.Context, planmodifier.ListRequest, *planmodifier.ListResponse) { + panic("unimplemented") +} + +// PlanModifyList implements planmodifier.List. +func (m mockPlanModifier) PlanModifyObject(context.Context, planmodifier.ObjectRequest, *planmodifier.ObjectResponse) { + panic("unimplemented") +} + +var _ planmodifier.List = mockPlanModifier{} +var _ planmodifier.Object = mockPlanModifier{} + +type mockValidator struct{} + +// Description implements validator.List. +func (m mockValidator) Description(context.Context) string { + panic("unimplemented") +} + +// MarkdownDescription implements validator.List. +func (m mockValidator) MarkdownDescription(context.Context) string { + panic("unimplemented") +} + +// ValidateList implements validator.List. +func (m mockValidator) ValidateList(context.Context, validator.ListRequest, *validator.ListResponse) { + panic("unimplemented") +} + +// ValidateList implements validator.Object. +func (m mockValidator) ValidateObject(context.Context, validator.ObjectRequest, *validator.ObjectResponse) { + panic("unimplemented") +} + +var _ validator.List = mockValidator{} +var _ validator.Object = mockValidator{} + +func TestCustomizeSchema_ConvertToAttribute(t *testing.T) { + v := mockValidator{} + pm := mockPlanModifier{} + testCases := []struct { + name string + baseSchema NestedBlockObject + path []string + want NestedBlockObject + expectPanic bool + }{ + { + name: "ListNestedBlock", + baseSchema: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "list": ListNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + DeprecationMessage: "deprecated", + Validators: []validator.List{v}, + PlanModifiers: []planmodifier.List{pm}, + }, + }, + }, + path: []string{"list"}, + want: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "list": ListNestedAttributeBuilder{ + NestedObject: NestedAttributeObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + DeprecationMessage: "deprecated", + Validators: []validator.List{v}, + PlanModifiers: []planmodifier.List{pm}, + }, + }, + }, + }, + { + name: "ListNestedBlock/CalledOnInnerBlock", + baseSchema: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "list": ListNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "nested_block": ListNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + }, + }, + }, + }, + }, + }, + path: []string{"list", "nested_block"}, + want: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "list": ListNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "nested_block": ListNestedAttributeBuilder{ + NestedObject: NestedAttributeObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "SingleNestedBlock", + baseSchema: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "single": SingleNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + DeprecationMessage: "deprecated", + Validators: []validator.Object{v}, + PlanModifiers: []planmodifier.Object{pm}, + }, + }, + }, + path: []string{"single"}, + want: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "single": SingleNestedAttributeBuilder{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + DeprecationMessage: "deprecated", + Validators: []validator.Object{v}, + PlanModifiers: []planmodifier.Object{pm}, + }, + }, + }, + }, + { + name: "SingleNestedBlock/RecursiveBlocks", + baseSchema: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "single": SingleNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Blocks: map[string]BlockBuilder{ + "nested_block": ListNestedBlockBuilder{ + NestedObject: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + }, + }, + }, + }, + }, + }, + path: []string{"single"}, + want: NestedBlockObject{ + Attributes: map[string]AttributeBuilder{ + "single": SingleNestedAttributeBuilder{ + Attributes: map[string]AttributeBuilder{ + "nested_block": ListNestedAttributeBuilder{ + NestedObject: NestedAttributeObject{ + Attributes: map[string]AttributeBuilder{ + "attr": StringAttributeBuilder{}, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "PanicOnEmptyPath", + path: nil, + expectPanic: true, + }, + } + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + if c.expectPanic { + assert.Panics(t, func() { + ConstructCustomizableSchema(c.baseSchema).ConvertToAttribute(c.path...) + }) + } else { + got := ConstructCustomizableSchema(c.baseSchema).ConvertToAttribute(c.path...) + assert.Equal(t, c.want, got.attr.(SingleNestedBlockBuilder).NestedObject) + } + }) + } +} diff --git a/internal/providers/pluginfw/tfschema/list_nested_attribute.go b/internal/providers/pluginfw/tfschema/list_nested_attribute.go index 5efc6105f0..9cd882990c 100644 --- a/internal/providers/pluginfw/tfschema/list_nested_attribute.go +++ b/internal/providers/pluginfw/tfschema/list_nested_attribute.go @@ -44,7 +44,7 @@ func (a ListNestedAttributeBuilder) BuildResourceAttribute() schema.Attribute { } } -func (a ListNestedAttributeBuilder) SetOptional() BaseSchemaBuilder { +func (a ListNestedAttributeBuilder) SetOptional() AttributeBuilder { if a.Optional && !a.Required { panic("attribute is already optional") } @@ -53,7 +53,7 @@ func (a ListNestedAttributeBuilder) SetOptional() BaseSchemaBuilder { return a } -func (a ListNestedAttributeBuilder) SetRequired() BaseSchemaBuilder { +func (a ListNestedAttributeBuilder) SetRequired() AttributeBuilder { if !a.Optional && a.Required { panic("attribute is already required") } @@ -62,7 +62,7 @@ func (a ListNestedAttributeBuilder) SetRequired() BaseSchemaBuilder { return a } -func (a ListNestedAttributeBuilder) SetSensitive() BaseSchemaBuilder { +func (a ListNestedAttributeBuilder) SetSensitive() AttributeBuilder { if a.Sensitive { panic("attribute is already sensitive") } @@ -70,7 +70,7 @@ func (a ListNestedAttributeBuilder) SetSensitive() BaseSchemaBuilder { return a } -func (a ListNestedAttributeBuilder) SetComputed() BaseSchemaBuilder { +func (a ListNestedAttributeBuilder) SetComputed() AttributeBuilder { if a.Computed { panic("attribute is already computed") } @@ -78,7 +78,7 @@ func (a ListNestedAttributeBuilder) SetComputed() BaseSchemaBuilder { return a } -func (a ListNestedAttributeBuilder) SetReadOnly() BaseSchemaBuilder { +func (a ListNestedAttributeBuilder) SetReadOnly() AttributeBuilder { if a.Computed && !a.Optional && !a.Required { panic("attribute is already read only") } diff --git a/internal/providers/pluginfw/tfschema/list_nested_block.go b/internal/providers/pluginfw/tfschema/list_nested_block.go index 7f75bdc4c8..91c7af0367 100644 --- a/internal/providers/pluginfw/tfschema/list_nested_block.go +++ b/internal/providers/pluginfw/tfschema/list_nested_block.go @@ -1,6 +1,8 @@ package tfschema import ( + "fmt" + dataschema "github.com/hashicorp/terraform-plugin-framework/datasource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" @@ -16,6 +18,15 @@ type ListNestedBlockBuilder struct { PlanModifiers []planmodifier.List } +func (a ListNestedBlockBuilder) ToAttribute() AttributeBuilder { + return ListNestedAttributeBuilder{ + NestedObject: a.NestedObject.ToNestedAttributeObject(), + DeprecationMessage: a.DeprecationMessage, + Validators: a.Validators, + PlanModifiers: a.PlanModifiers, + } +} + func (a ListNestedBlockBuilder) BuildDataSourceBlock() dataschema.Block { return dataschema.ListNestedBlock{ NestedObject: a.NestedObject.BuildDataSourceAttribute(), @@ -47,3 +58,19 @@ func (a ListNestedBlockBuilder) AddPlanModifier(v planmodifier.List) BaseSchemaB a.PlanModifiers = append(a.PlanModifiers, v) return a } + +func (a ListNestedBlockBuilder) ConvertBlockToAttribute(field string) BaseSchemaBuilder { + elem, ok := a.NestedObject.Blocks[field] + if !ok { + panic(fmt.Errorf("field %s does not exist in nested block", field)) + } + if a.NestedObject.Attributes == nil { + a.NestedObject.Attributes = make(map[string]AttributeBuilder) + } + a.NestedObject.Attributes[field] = elem.ToAttribute() + delete(a.NestedObject.Blocks, field) + if len(a.NestedObject.Blocks) == 0 { + a.NestedObject.Blocks = nil + } + return a +} diff --git a/internal/providers/pluginfw/tfschema/nested_block_object.go b/internal/providers/pluginfw/tfschema/nested_block_object.go index 2f9853eb76..2be8935d6b 100644 --- a/internal/providers/pluginfw/tfschema/nested_block_object.go +++ b/internal/providers/pluginfw/tfschema/nested_block_object.go @@ -11,6 +11,19 @@ type NestedBlockObject struct { Blocks map[string]BlockBuilder } +func (a NestedBlockObject) ToNestedAttributeObject() NestedAttributeObject { + attributes := make(map[string]AttributeBuilder) + for k, v := range a.Attributes { + attributes[k] = v + } + for k, v := range a.Blocks { + attributes[k] = v.ToAttribute() + } + return NestedAttributeObject{ + Attributes: attributes, + } +} + func (a NestedBlockObject) BuildDataSourceAttribute() dataschema.NestedBlockObject { dataSourceAttributes := BuildDataSourceAttributeMap(a.Attributes) dataSourceBlocks := BuildDataSourceBlockMap(a.Blocks) diff --git a/internal/providers/pluginfw/tfschema/single_nested_block.go b/internal/providers/pluginfw/tfschema/single_nested_block.go index aac8a73eb8..59508235b6 100644 --- a/internal/providers/pluginfw/tfschema/single_nested_block.go +++ b/internal/providers/pluginfw/tfschema/single_nested_block.go @@ -18,6 +18,15 @@ type SingleNestedBlockBuilder struct { PlanModifiers []planmodifier.Object } +func (a SingleNestedBlockBuilder) ToAttribute() AttributeBuilder { + return SingleNestedAttributeBuilder{ + Attributes: a.NestedObject.ToNestedAttributeObject().Attributes, + DeprecationMessage: a.DeprecationMessage, + Validators: a.Validators, + PlanModifiers: a.PlanModifiers, + } +} + func (a SingleNestedBlockBuilder) BuildDataSourceAttribute() dataschema.Attribute { panic(fmt.Errorf("BuildDataSourceBlock should never be called for SingleNestedBlockBuilder. %s", common.TerraformBugErrorMessage)) } @@ -59,3 +68,19 @@ func (a SingleNestedBlockBuilder) AddPlanModifier(v planmodifier.Object) BaseSch a.PlanModifiers = append(a.PlanModifiers, v) return a } + +func (a SingleNestedBlockBuilder) ConvertBlockToAttribute(field string) BaseSchemaBuilder { + elem, ok := a.NestedObject.Blocks[field] + if !ok { + panic(fmt.Errorf("field %s does not exist in nested block", field)) + } + if a.NestedObject.Attributes == nil { + a.NestedObject.Attributes = make(map[string]AttributeBuilder) + } + a.NestedObject.Attributes[field] = elem.ToAttribute() + delete(a.NestedObject.Blocks, field) + if len(a.NestedObject.Blocks) == 0 { + a.NestedObject.Blocks = nil + } + return a +}