Skip to content

Conversation

@bflad
Copy link
Contributor

@bflad bflad commented Jun 13, 2023

Closes #754
Reference: #715 (precursor)
Reference: #767 (followup)

The framework recently was updated to perform stricter value type checking against the defined schema type when setting data. This was to prevent panics or other potentially confusing behaviors with mismatched types. The attribute-based plan modification logic in the framework however was missing some value type conversion, which could generate unavoidable Value Conversion Error diagnostics for provider developers when attributes/blocks used both CustomType and PlanModifiers fields.

This changeset ensures that attribute-based plan modification will always return the custom value type implementation of a value after a plan modifier response. All plan modifier responses are currently using the base value type, so the framework logic must handle converting it back. Even if the plan modifier responses were updated to use the Valuable interfaces, the framework would still need to perform this logic in case a plan modifier implementation opted to return the base value type, since all base value types implement the Valuable interfaces currently.

These changes handle custom types on all attribute and block implementations, however a non-trivial amount of internal code refactoring is necessary to fix this same issue for NestedAttributeObject and NestedBlockObject implementations that contain both CustomType and PlanModifiers fields. That effort will follow this one separately to reduce review burden.

Previously before logic updates:

--- FAIL: TestServerPlanResourceChange (0.00s)
    --- FAIL: TestServerPlanResourceChange/create-attributeplanmodifier-response-attributeplan-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/server_planresourcechange_test.go:3848: unexpected difference:   &fwserver.PlanResourceChangeResponse{
            - 	Diagnostics: diag.Diagnostics{
            - 		diag.withPath{
            - 			Diagnostic: diag.ErrorDiagnostic{detail: "An unexpected error was encounte"..., summary: "Value Conversion Error"},
            - 			path:       s"test_computed",
            - 		},
            - 	},
            + 	Diagnostics:    nil,
              	PlannedPrivate: &{Provider: &{data: {}}},
              	PlannedState: &tfsdk.State{
            - 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<unknown>, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.String<"test-config-value">>`,
            + 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<"test-attributeplanmodifier-value">, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.`...,
              		Schema: schema.Schema{Attributes: {"test_computed": schema.StringAttribute{CustomType: s"StringTypeWithSemanticEquals(false)", Computed: true, PlanModifiers: {...}}, "test_other_computed": schema.StringAttribute{Computed: true}, "test_required": schema.StringAttribute{Required: true}}},
              	},
              	RequiresReplace: s"[]",
              }

--- FAIL: TestAttributePlanModifyString (0.00s)
    --- FAIL: TestAttributePlanModifyString/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:8315: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   basetypes.StringValue{state: 2, value: "testvalue"},
            + 	AttributePlan:   types.StringValueWithSemanticEquals{StringValue: basetypes.StringValue{state: 2, value: "testvalue"}},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:1733: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3099: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockPlanModifyList (0.00s)
    --- FAIL: TestBlockPlanModifyList/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3528: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: basetypes.ListValue{
            - 		elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            - 		elementType: basetypes.StringType{},
            - 		state:       2,
            - 	},
            + 	AttributePlan: types.ListValueWithSemanticEquals{
            + 		ListValue: basetypes.ListValue{
            + 			elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            + 			elementType: basetypes.StringType{},
            + 			state:       2,
            + 		},
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

…ns custom value type implementations

Reference: #754
Reference: #715 (precursor)
Reference: #767 (followup)

The framework recently was updated to perform stricter value type checking against the defined schema type when setting data. This was to prevent panics or other potentially confusing behaviors with mismatched types. The attribute-based plan modification logic in the framework however was missing some value type conversion, which could generate unavoidable `Value Conversion Error` diagnostics for provider developers when attributes/blocks used both `CustomType` and `PlanModifiers` fields.

This changeset ensures that attribute-based plan modification will always return the custom value type implementation of a value after a plan modifier response. All plan modifier responses are currently using the base value type, so the framework logic must handle converting it back. Even if the plan modifier responses were updated to use the `Valuable` interfaces, the framework would still need to perform this logic in case a plan modifier implementation opted to return the base value type, since all base value types implement the `Valuable` interfaces currently.

These changes handle custom types on all attribute and block implementations, however a non-trivial amount of internal code refactoring is necessary to fix this same issue for `NestedAttributeObject` and `NestedBlockObject` implementations that contain both `CustomType` and `PlanModifiers` fields. That effort will follow this one separately to reduce review burden.

Previously before logic updates:

```
--- FAIL: TestServerPlanResourceChange (0.00s)
    --- FAIL: TestServerPlanResourceChange/create-attributeplanmodifier-response-attributeplan-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/server_planresourcechange_test.go:3848: unexpected difference:   &fwserver.PlanResourceChangeResponse{
            - 	Diagnostics: diag.Diagnostics{
            - 		diag.withPath{
            - 			Diagnostic: diag.ErrorDiagnostic{detail: "An unexpected error was encounte"..., summary: "Value Conversion Error"},
            - 			path:       s"test_computed",
            - 		},
            - 	},
            + 	Diagnostics:    nil,
              	PlannedPrivate: &{Provider: &{data: {}}},
              	PlannedState: &tfsdk.State{
            - 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<unknown>, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.String<"test-config-value">>`,
            + 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<"test-attributeplanmodifier-value">, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.`...,
              		Schema: schema.Schema{Attributes: {"test_computed": schema.StringAttribute{CustomType: s"StringTypeWithSemanticEquals(false)", Computed: true, PlanModifiers: {...}}, "test_other_computed": schema.StringAttribute{Computed: true}, "test_required": schema.StringAttribute{Required: true}}},
              	},
              	RequiresReplace: s"[]",
              }

--- FAIL: TestAttributePlanModifyString (0.00s)
    --- FAIL: TestAttributePlanModifyString/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:8315: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   basetypes.StringValue{state: 2, value: "testvalue"},
            + 	AttributePlan:   types.StringValueWithSemanticEquals{StringValue: basetypes.StringValue{state: 2, value: "testvalue"}},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:1733: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3099: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockPlanModifyList (0.00s)
    --- FAIL: TestBlockPlanModifyList/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3528: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: basetypes.ListValue{
            - 		elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            - 		elementType: basetypes.StringType{},
            - 		state:       2,
            - 	},
            + 	AttributePlan: types.ListValueWithSemanticEquals{
            + 		ListValue: basetypes.ListValue{
            + 			elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            + 			elementType: basetypes.StringType{},
            + 			state:       2,
            + 		},
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
```
@bflad bflad added the bug Something isn't working label Jun 13, 2023
@bflad bflad added this to the v1.3.1 milestone Jun 13, 2023
@bflad bflad requested a review from a team as a code owner June 13, 2023 17:37
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for breaking up the changeset 😆 - Looks good to me 👍🏻

@bflad bflad merged commit 22f77e2 into main Jun 14, 2023
@bflad bflad deleted the bflad/planmodifier-custom-types branch June 14, 2023 15:52
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plan Modifiers Doesn't Work with Custom String Type

2 participants