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.

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

types: Ensure List/Map/Object/Set Attributes/AttributeTypes/Elements returns cannot mutate underlying data #591

Merged
merged 4 commits into from Dec 19, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Dec 16, 2022

Closes #556
Closes #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

…returns cannot mutate underlying data

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
```
@bflad bflad added the bug Something isn't working label Dec 16, 2022
@bflad bflad added this to the v1.0.1 milestone Dec 16, 2022
@bflad bflad requested a review from a team as a code owner December 16, 2022 22:23
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM 👍

},
},
},
// PlanModifiers: []planmodifier.Object{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the commented code be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should, thanks.

@bflad bflad merged commit 85f4a77 into main Dec 19, 2022
@bflad bflad deleted the bflad/basetypes-immutable-slices-and-maps branch December 19, 2022 17:12
@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 Jan 19, 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
2 participants