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: Prevent panics when ValueFromTerraform received nil values #208

Merged
merged 5 commits into from
Nov 11, 2021
Merged

types: Prevent panics when ValueFromTerraform received nil values #208

merged 5 commits into from
Nov 11, 2021

Conversation

bluekeyes
Copy link
Contributor

When getting an object under a missing (null) attribute, GetAttribute would panic instead of returning null. Also add a unit test for this case.

Fixes #207.

When getting an object under a missing (null) attribute, GetAttribute
would panic instead of returning null.
@bflad bflad self-assigned this Nov 2, 2021
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @bluekeyes 👋 Thank you for submitting this. Fixing the panic certainly seems ideal in this situation.

In terms of implementation, the preference should likely be the following for the type implementations:

  • Check for nil first, return null type in that situation
  • Perform the type check; types should ensure they align correctly upfront
  • Perform the null/unknown checks

This class of panic fix should be performed in all framework defined types in the types package and a quick nil input unit test should be included for each type's ValueFromTerraform such as TestObjectTypeValueFromTerraform. The TestStateGetAttribute unit testing addition should likely also get replicated across Config and Plan to try and prevent future regressions should those implementations diverge for whatever reason.

Please let us know if you have time to make these changes and thanks again for reporting this and contributing! 😄

@bflad bflad added bug Something isn't working types Issues and pull requests about our types abstraction and implementations. labels Nov 10, 2021
@bflad
Copy link
Contributor

bflad commented Nov 10, 2021

Oh! Also feel free to create a CHANGELOG entry as well for this by creating a .changelog/208.txt file with contents such as:

```release-note:bug
types: Prevented panics when `ValueFromTerraform` received `nil` values
```

Also add some type tests for the container types that perform type
checks but had no tests for this. This also required fixing some bad
error conditions in the test code.
@bluekeyes bluekeyes changed the title tfsdk: Fix State.GetAttribute panic on nested objects types: Prevent panics when ValueFromTerraform received nil values Nov 11, 2021
@bluekeyes bluekeyes changed the title types: Prevent panics when ValueFromTerraform received nil values types: Prevent panics when ValueFromTerraform received nil values Nov 11, 2021
Copy link
Contributor Author

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Thanks for the review - I think I've addressed your main concerns. I did not add the nil type check to any of the primitive types, as they don't do any explicit type checks right now, deferring to tftypes.Value.As instead, but I could go through and do this if you'd like. It felt a bit weird without any other existing type checks.

types/list_test.go Show resolved Hide resolved
tfsdk/config_test.go Show resolved Hide resolved
types/object.go Show resolved Hide resolved
Copy link
Contributor

@bflad bflad 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 again, @bluekeyes, looks good to me 🚀

types/list_test.go Show resolved Hide resolved
tfsdk/config_test.go Show resolved Hide resolved
@bflad bflad added this to the v0.5.0 milestone Nov 11, 2021
@bflad bflad merged commit 33080f2 into hashicorp:main Nov 11, 2021
@bluekeyes bluekeyes deleted the bkeyes/fix-object-panic branch November 11, 2021 19:17
@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 Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tfsdk: GetAttribute panic for nested object in missing/null attribute
2 participants