Skip to content

Commit

Permalink
types: Ensure List/Map/Object/Set Attributes/AttributeTypes/Elements …
Browse files Browse the repository at this point in the history 
…returns cannot mutate underlying data (#591)

Reference: #556
Reference: #582

This will ensure that the `Attributes()`, `AttributeTypes()`, and `Elements()` methods return a copy of the underlying map or slice of data, rather than a direct reference to the map or slice. This also prevents `Object`-based plan modification from returning a panic since the updated `Object` `Attributes()` implementation will return an empty map instead of a `nil` map. Provider implementations should always rely on `IsNull()` and `IsUnknown()` for verifying whether types are known, rather than a nil comparison. This is considered a bug fix for the intended behavior of these type implementations rather than a breaking change as it standardizes type handling expectations.

New unit testing failures before code updates:

```
--- FAIL: TestMapValueElements_immutable (0.00s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/types/basetypes/map_test.go:604: unexpected Elements mutation

--- FAIL: TestNestedAttributeObjectPlanModify (0.00s)
    --- FAIL: TestNestedAttributeObjectPlanModify/response-planvalue-unknown-to-known-nested (0.00s)
panic: assignment to entry in nil map [recovered]
	panic: assignment to entry in nil map

goroutine 35 [running]:
testing.tRunner.func1.2({0x1010f0180, 0x10115b648})
	/opt/homebrew/Cellar/go/1.19.4/libexec/src/testing/testing.go:1396 +0x1c8
testing.tRunner.func1()
	/opt/homebrew/Cellar/go/1.19.4/libexec/src/testing/testing.go:1399 +0x378
panic({0x1010f0180, 0x10115b648})
	/opt/homebrew/Cellar/go/1.19.4/libexec/src/runtime/panic.go:884 +0x204
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.NestedAttributeObjectPlanModify({_, _}, {_, _}, {{{0x1400010d190, 0x1, 0x1}}, {0x1, {0x1400010d1b0, 0x1, ...}}, ...}, ...)
	/Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification.go:1748 +0x510
```
  • Loading branch information
@bflad
bflad committed Dec 19, 2022
1 parent 88e1c5b commit 85f4a77
Show file tree
Hide file tree
Showing 10 changed files with 303 additions and 24 deletions.
15 changes: 15 additions & 0 deletions .changelog/591.txt
View file
@@ -0,0 +1,15 @@
```release-note:bug
types/basetypes: Prevented value mutation via the `ListValue`, `MapValue`, and `SetValue` type `Elements()` method return
```

```release-note:bug
types/basetypes: Prevented type mutation via the `ObjectType` type `AttributeTypes()` method return
```

```release-note:bug
types/basetypes: Prevented value mutation via the `ObjectValue` type `AttributeTypes()` and `Attributes()` method returns
```

```release-note:bug
resource/schema/planmodifier: Prevented `assignment to entry in nil map` panic for `Object` type plan modifiers
```
152 changes: 152 additions & 0 deletions internal/fwserver/attribute_plan_modification_test.go
View file
Expand Up @@ -7764,6 +7764,158 @@ func TestNestedAttributeObjectPlanModify(t *testing.T) {
),
},
},
"response-planvalue-unknown-to-known": {
object: testschema.NestedAttributeObjectWithPlanModifiers{
PlanModifiers: []planmodifier.Object{
testplanmodifier.Object{
PlanModifyObjectMethod: func(ctx context.Context, req planmodifier.ObjectRequest, resp *planmodifier.ObjectResponse) {
resp.PlanValue = types.ObjectValueMust(
map[string]attr.Type{
"testattr": types.StringType,
},
map[string]attr.Value{
"testattr": types.StringValue("newtestvalue"),
},
)
},
},
},
},
request: planmodifier.ObjectRequest{
Config: tfsdk.Config{
Raw: tftypes.NewValue(
tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
},
},
map[string]tftypes.Value{
"test": tftypes.NewValue(
tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
nil,
),
},
),
Schema: fwSchema,
},
ConfigValue: types.ObjectNull(
map[string]attr.Type{"testattr": types.StringType},
),
Path: path.Root("test"),
PathExpression: path.MatchRoot("test"),
Plan: tfsdk.Plan{
Raw: tftypes.NewValue(
tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
},
},
map[string]tftypes.Value{
"test": tftypes.NewValue(
tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
tftypes.UnknownValue,
),
},
),
Schema: fwSchema,
},
PlanValue: types.ObjectUnknown(
map[string]attr.Type{"testattr": types.StringType},
),
State: testState,
StateValue: fwValue,
},
response: &ModifyAttributePlanResponse{
AttributePlan: types.ObjectUnknown(
map[string]attr.Type{"testattr": types.StringType},
),
},
expected: &ModifyAttributePlanResponse{
AttributePlan: types.ObjectValueMust(
map[string]attr.Type{
"testattr": types.StringType,
},
map[string]attr.Value{
"testattr": types.StringValue("newtestvalue"),
},
),
},
},
"response-planvalue-unknown-to-known-nested": {
object: testschema.NestedAttributeObject{
Attributes: map[string]fwschema.Attribute{
"testattr": testschema.AttributeWithStringPlanModifiers{
Required: true,
PlanModifiers: []planmodifier.String{
testplanmodifier.String{
PlanModifyStringMethod: func(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) {
resp.PlanValue = types.StringValue("newtestvalue") // should win over object
},
},
},
},
},
},
request: planmodifier.ObjectRequest{
Config: tfsdk.Config{
Raw: tftypes.NewValue(
tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
},
},
map[string]tftypes.Value{
"test": tftypes.NewValue(
tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
nil,
),
},
),
Schema: fwSchema,
},
ConfigValue: types.ObjectNull(
map[string]attr.Type{"testattr": types.StringType},
),
Path: path.Root("test"),
PathExpression: path.MatchRoot("test"),
Plan: tfsdk.Plan{
Raw: tftypes.NewValue(
tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
},
},
map[string]tftypes.Value{
"test": tftypes.NewValue(
tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}},
tftypes.UnknownValue,
),
},
),
Schema: fwSchema,
},
PlanValue: types.ObjectUnknown(
map[string]attr.Type{"testattr": types.StringType},
),
State: testState,
StateValue: fwValue,
},
response: &ModifyAttributePlanResponse{
AttributePlan: types.ObjectUnknown(
map[string]attr.Type{"testattr": types.StringType},
),
},
expected: &ModifyAttributePlanResponse{
AttributePlan: types.ObjectValueMust(
map[string]attr.Type{
"testattr": types.StringType,
},
map[string]attr.Value{
"testattr": types.StringValue("newtestvalue"),
},
),
},
},
"response-private": {
object: testschema.NestedAttributeObjectWithPlanModifiers{
PlanModifiers: []planmodifier.Object{
Expand Down
9 changes: 6 additions & 3 deletions types/basetypes/list.go
View file
Expand Up @@ -309,10 +309,13 @@ type ListValue struct {
state attr.ValueState
}

// Elements returns the collection of elements for the List. Returns nil if the
// List is null or unknown.
// Elements returns a copy of the collection of elements for the List.
func (l ListValue) Elements() []attr.Value {
return l.elements
// Ensure callers cannot mutate the internal elements
result := make([]attr.Value, 0, len(l.elements))
result = append(result, l.elements...)

return result
}

// ElementsAs populates `target` with the elements of the ListValue, throwing an
Expand Down
15 changes: 13 additions & 2 deletions types/basetypes/list_test.go
View file
Expand Up @@ -589,11 +589,11 @@ func TestListValueElements(t *testing.T) {
},
"null": {
input: NewListNull(StringType{}),
expected: nil,
expected: []attr.Value{},
},
"unknown": {
input: NewListUnknown(StringType{}),
expected: nil,
expected: []attr.Value{},
},
}

Expand All @@ -612,6 +612,17 @@ func TestListValueElements(t *testing.T) {
}
}

func TestListValueElements_immutable(t *testing.T) {
t.Parallel()

value := NewListValueMust(StringType{}, []attr.Value{NewStringValue("original")})
value.Elements()[0] = NewStringValue("modified")

if !value.Equal(NewListValueMust(StringType{}, []attr.Value{NewStringValue("original")})) {
t.Fatal("unexpected Elements mutation")
}
}

func TestListValueElementType(t *testing.T) {
t.Parallel()

Expand Down
12 changes: 9 additions & 3 deletions types/basetypes/map.go
View file
Expand Up @@ -314,10 +314,16 @@ type MapValue struct {
state attr.ValueState
}

// Elements returns the mapping of elements for the Map. Returns nil if the
// Map is null or unknown.
// Elements returns a copy of the mapping of elements for the Map.
func (m MapValue) Elements() map[string]attr.Value {
return m.elements
// Ensure callers cannot mutate the internal elements
result := make(map[string]attr.Value, len(m.elements))

for key, value := range m.elements {
result[key] = value
}

return result
}

// ElementsAs populates `target` with the elements of the MapValue, throwing an
Expand Down
15 changes: 13 additions & 2 deletions types/basetypes/map_test.go
View file
Expand Up @@ -571,11 +571,11 @@ func TestMapValueElements(t *testing.T) {
},
"null": {
input: NewMapNull(StringType{}),
expected: nil,
expected: map[string]attr.Value{},
},
"unknown": {
input: NewMapUnknown(StringType{}),
expected: nil,
expected: map[string]attr.Value{},
},
}

Expand All @@ -594,6 +594,17 @@ func TestMapValueElements(t *testing.T) {
}
}

func TestMapValueElements_immutable(t *testing.T) {
t.Parallel()

value := NewMapValueMust(StringType{}, map[string]attr.Value{"test": NewStringValue("original")})
value.Elements()["test"] = NewStringValue("modified")

if !value.Equal(NewMapValueMust(StringType{}, map[string]attr.Value{"test": NewStringValue("original")})) {
t.Fatal("unexpected Elements mutation")
}
}

func TestMapValueElementType(t *testing.T) {
t.Parallel()

Expand Down
34 changes: 27 additions & 7 deletions types/basetypes/object.go
View file
Expand Up @@ -50,9 +50,16 @@ func (o ObjectType) WithAttributeTypes(typs map[string]attr.Type) attr.TypeWithA
}
}

// AttributeTypes returns the type's attribute types.
// AttributeTypes returns a copy of the type's attribute types.
func (o ObjectType) AttributeTypes() map[string]attr.Type {
return o.AttrTypes
// Ensure callers cannot mutate the value
result := make(map[string]attr.Type, len(o.AttrTypes))

for key, value := range o.AttrTypes {
result[key] = value
}

return result
}

// TerraformType returns the tftypes.Type that should be used to
Expand Down Expand Up @@ -356,15 +363,28 @@ func (o ObjectValue) As(ctx context.Context, target interface{}, opts ObjectAsOp
}, path.Empty())
}

// Attributes returns the mapping of known attribute values for the Object.
// Returns nil if the Object is null or unknown.
// Attributes returns a copy of the mapping of known attribute values for the Object.
func (o ObjectValue) Attributes() map[string]attr.Value {
return o.attributes
// Ensure callers cannot mutate the internal attributes
result := make(map[string]attr.Value, len(o.attributes))

for name, value := range o.attributes {
result[name] = value
}

return result
}

// AttributeTypes returns the mapping of attribute types for the Object.
// AttributeTypes returns a copy of the mapping of attribute types for the Object.
func (o ObjectValue) AttributeTypes(_ context.Context) map[string]attr.Type {
return o.attributeTypes
// Ensure callers cannot mutate the internal attribute types
result := make(map[string]attr.Type, len(o.attributeTypes))

for name, typ := range o.attributeTypes {
result[name] = typ
}

return result
}

// Type returns an ObjectType with the same attribute types as `o`.
Expand Down

0 comments on commit 85f4a77

Please sign in to comment.