Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PlanModifiers on Custom Type NestedAttributeObject/NestedBlockObject #767

Closed
bflad opened this issue Jun 13, 2023 · 1 comment · Fixed by #823
Closed

PlanModifiers on Custom Type NestedAttributeObject/NestedBlockObject #767

bflad opened this issue Jun 13, 2023 · 1 comment · Fixed by #823
Labels
bug Something isn't working

Comments

@bflad
Copy link
Contributor

bflad commented Jun 13, 2023

Module version

v1.3.0

Relevant provider source code

(Pending confirmation) The following provider code could be problematic with the framework's attribute-based plan modification logic:

schema.ListNestedAttribute{
  // ... other fields ...
  NestedObject: schema.NestedAttributeObject{
    // ... other fields ...
    CustomType: /* basetypes.ObjectTypable */,
    PlanModifiers: []planmodifier.Object{/* a plan modifier that returns basetypes.ObjectValue */},
  }
}

Expected Behavior

The framework should roundtrip custom value type implementations through the plan modification logic.

Actual Behavior

The basetypes.ObjectValue returned from a plan modifier is preserved. This likely will generate a Value Conversion Error diagnostic from the framework or some other confusing behavior.

Fixing this issue will require some hefty internal code refactoring, so opting to split this effort from #754.

Steps to Reproduce

Should be reproducible with additional unit test cases in internal/fwserver/attribute_plan_modification_test.go / internal/fwserver/block_plan_modification_test.go and running go test.

References
@bflad bflad added the bug Something isn't working label Jun 13, 2023
bflad added a commit that referenced this issue Jun 13, 2023
@bflad
internal/fwserver: Ensure Attribute and Block plan modification retur…
5ed8e8e 
…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 mentioned this issue Jun 13, 2023
Merged
bflad added a commit that referenced this issue Jun 14, 2023
@bflad
internal/fwserver: Ensure Attribute and Block plan modification retur…
22f77e2 
…ns custom value type implementations (#768)

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,
              }
```
@bendbennett bendbennett mentioned this issue Aug 15, 2023
Closed
bendbennett added a commit that referenced this issue Aug 15, 2023
@bendbennett
internal/fwserver: Attribute plan modification returns custom value t…
fd35298 
…ype implementations for list, map, set nested attributes using a nested object with a custom type (#821)

Reference: #768
Reference: #767
@bendbennett bendbennett mentioned this issue Aug 15, 2023
Merged
bendbennett added a commit that referenced this issue Aug 16, 2023
@bendbennett
internal/fwserver: Block plan modification returns custom value type …
3f4d956 
…implementations for list, set nested blocks using a nested object with a custom type (#821)

Reference: #768
Reference: #767
bendbennett added a commit that referenced this issue Aug 17, 2023
@bendbennett
internal/fwserver: Ensure Attribute and Block Plan Modification Retur…
4ee15c8 
…ns Custom Value Type Implementations When Using Custom Type NestedAttributeObject/NestedBlockObject (#823)

* internal/fwserver: Attribute plan modification returns custom value type implementations for list, map, set nested attributes using a nested object with a custom type (#821)

Reference: #768
Reference: #767

* internal/fwserver: Block plan modification returns custom value type implementations for list, set nested blocks using a nested object with a custom type (#821)

Reference: #768
Reference: #767

* Using testschema throughout tests for attribute and block plan modifier (#821)

* Renaming attr and updating tests (#821)

* Add changelog entry (#821)

* Removing hardcoded AttrTypes (#821)
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
1 participant
@bflad