Skip to content

Commit

Permalink
internal/fwserver: Use response plan value instead of request with ne…
Browse files Browse the repository at this point in the history 
…sted attributes and blocks (#669)

Reference: #644
Reference: https://discuss.hashicorp.com/t/framework-v1-1-1-strategies-for-diagnosing-resource-state-churn/49688

The attribute and block plan modification logic must preserve any changes made by the "top level" list/map/set/single nested attribute/block plan modifiers before continuing on to nested attributes/blocks.

New unit testing failures prior to the logic change:

```
--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-usestateforunknown (0.00s)
        attribute_plan_modification_test.go:1107: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue1"}]`,
            + 	AttributePlan:   s"[]",
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-map-nested-usestateforunknown (0.00s)
        attribute_plan_modification_test.go:1107: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`{"key1":{"nested_computed":"statevalue1"}}`,
            + 	AttributePlan:   s"{}",
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-single-nested-usestateforunknown (0.00s)
        attribute_plan_modification_test.go:1107: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`{"nested_computed":"statevalue1"}`,
            + 	AttributePlan:   s`{"nested_computed":<unknown>}`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-usestateforunknown (0.00s)
        attribute_plan_modification_test.go:1107: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue1"}]`,
            + 	AttributePlan:   s"[]",
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-usestateforunknown (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:2338: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue1"}]`,
            + 	AttributePlan:   s"[]",
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-usestateforunknown (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:2338: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue1"}]`,
            + 	AttributePlan:   s"[]",
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
FAIL
```
  • Loading branch information
@bflad
bflad authored Feb 10, 2023
1 parent d8f5d5a commit 757f965
Show file tree
Hide file tree
Showing 5 changed files with 463 additions and 7 deletions.
5 changes: 5 additions & 0 deletions .changes/unreleased/BUG FIXES-20230210-092136.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
kind: BUG FIXES
body: 'resource: Prevented nested attribute and block plan modifications from being undone'
time: 2023-02-10T09:21:36.848573-05:00
custom:
Issue: "669"
16 changes: 12 additions & 4 deletions internal/fwserver/attribute_plan_modification.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req ModifyAt
return
}

planList, diags := coerceListValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with list
// plan modifiers.
planList, diags := coerceListValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down Expand Up @@ -220,7 +222,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req ModifyAt
return
}

planSet, diags := coerceSetValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with set
// plan modifiers.
planSet, diags := coerceSetValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down Expand Up @@ -305,7 +309,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req ModifyAt
return
}

planMap, diags := coerceMapValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with map
// plan modifiers.
planMap, diags := coerceMapValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down Expand Up @@ -390,7 +396,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req ModifyAt
return
}

planObject, diags := coerceObjectValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with object
// plan modifiers.
planObject, diags := coerceObjectValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down
258 changes: 258 additions & 0 deletions internal/fwserver/attribute_plan_modification_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ import (
"github.com/hashicorp/terraform-plugin-framework/internal/testing/testschema"
"github.com/hashicorp/terraform-plugin-framework/path"
"github.com/hashicorp/terraform-plugin-framework/resource/schema"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/listplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/mapplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/objectplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/tfsdk"
"github.com/hashicorp/terraform-plugin-framework/types"
Expand Down Expand Up @@ -216,6 +220,75 @@ func TestAttributeModifyPlan(t *testing.T) {
Private: testProviderData,
},
},
"attribute-list-nested-usestateforunknown": {
attribute: testschema.NestedAttributeWithListPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
"nested_computed": testschema.Attribute{
Type: types.StringType,
Computed: true,
},
},
},
Computed: true,
PlanModifiers: []planmodifier.List{
listplanmodifier.UseStateForUnknown(),
},
},
req: ModifyAttributePlanRequest{
AttributeConfig: types.ListNull(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributePath: path.Root("test"),
AttributePlan: types.ListUnknown(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributeState: types.ListValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
[]attr.Value{
types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.ListValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
[]attr.Value{
types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
},
"attribute-set-nested-private": {
attribute: testschema.NestedAttributeWithSetPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Expand Down Expand Up @@ -310,6 +383,75 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
"attribute-set-nested-usestateforunknown": {
attribute: testschema.NestedAttributeWithSetPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
"nested_computed": testschema.Attribute{
Type: types.StringType,
Computed: true,
},
},
},
Computed: true,
PlanModifiers: []planmodifier.Set{
setplanmodifier.UseStateForUnknown(),
},
},
req: ModifyAttributePlanRequest{
AttributeConfig: types.SetNull(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributePath: path.Root("test"),
AttributePlan: types.SetUnknown(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributeState: types.SetValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
[]attr.Value{
types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.SetValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
[]attr.Value{
types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
},
"attribute-set-nested-nested-usestateforunknown": {
attribute: testschema.NestedAttribute{
NestedObject: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
Expand Down Expand Up @@ -547,6 +689,75 @@ func TestAttributeModifyPlan(t *testing.T) {
Private: testProviderData,
},
},
"attribute-map-nested-usestateforunknown": {
attribute: testschema.NestedAttributeWithMapPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
"nested_computed": testschema.Attribute{
Type: types.StringType,
Computed: true,
},
},
},
Computed: true,
PlanModifiers: []planmodifier.Map{
mapplanmodifier.UseStateForUnknown(),
},
},
req: ModifyAttributePlanRequest{
AttributeConfig: types.MapNull(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributePath: path.Root("test"),
AttributePlan: types.MapUnknown(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
),
AttributeState: types.MapValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
map[string]attr.Value{
"key1": types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.MapValueMust(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
},
},
map[string]attr.Value{
"key1": types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
),
},
},
"attribute-single-nested-private": {
attribute: testschema.NestedAttributeWithObjectPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Expand Down Expand Up @@ -604,6 +815,53 @@ func TestAttributeModifyPlan(t *testing.T) {
Private: testProviderData,
},
},
"attribute-single-nested-usestateforunknown": {
attribute: testschema.NestedAttributeWithObjectPlanModifiers{
NestedObject: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
"nested_computed": testschema.Attribute{
Type: types.StringType,
Computed: true,
},
},
},
Computed: true,
PlanModifiers: []planmodifier.Object{
objectplanmodifier.UseStateForUnknown(),
},
},
req: ModifyAttributePlanRequest{
AttributeConfig: types.ObjectNull(
map[string]attr.Type{
"nested_computed": types.StringType,
},
),
AttributePath: path.Root("test"),
AttributePlan: types.ObjectUnknown(
map[string]attr.Type{
"nested_computed": types.StringType,
},
),
AttributeState: types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.ObjectValueMust(
map[string]attr.Type{
"nested_computed": types.StringType,
},
map[string]attr.Value{
"nested_computed": types.StringValue("statevalue1"),
},
),
},
},
"requires-replacement": {
attribute: testschema.AttributeWithStringPlanModifiers{
Required: true,
Expand Down
12 changes: 9 additions & 3 deletions internal/fwserver/block_plan_modification.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ func BlockModifyPlan(ctx context.Context, b fwschema.Block, req ModifyAttributeP
return
}

planList, diags := coerceListValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with list
// plan modifiers.
planList, diags := coerceListValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down Expand Up @@ -140,7 +142,9 @@ func BlockModifyPlan(ctx context.Context, b fwschema.Block, req ModifyAttributeP
return
}

planSet, diags := coerceSetValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with set
// plan modifiers.
planSet, diags := coerceSetValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down Expand Up @@ -225,7 +229,9 @@ func BlockModifyPlan(ctx context.Context, b fwschema.Block, req ModifyAttributeP
return
}

planObject, diags := coerceObjectValue(ctx, req.AttributePath, req.AttributePlan)
// Use response as the planned value may have been modified with object
// plan modifiers.
planObject, diags := coerceObjectValue(ctx, req.AttributePath, resp.AttributePlan)

resp.Diagnostics.Append(diags...)

Expand Down
Loading

0 comments on commit 757f965

Please sign in to comment.