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

Use json.Number instead of float64 when parsing state. #113

Merged
merged 5 commits into from
Dec 17, 2020
Merged

Conversation

paddycarver
Copy link
Contributor

State values can contain numbers that are larger than float64 can
precisely represent. json.Unmarshal when unmarshaled into an
interface{} uses float64 to represent numbers by default. To
properly retain precision for numbers, we need to use a custom
json.Decoder with the UseNumber() method called on it, which will
use json.Number (a string alias) to store the numbers, preserving
their precision.

This does this at the runTerraformCmdJSON level, meaning anyone using that method with v as interface{} in the arguments will suddenly find that their float64s are now json.Numbers, which may break them. The solution to this is to use the Float64() method on the json.Number, but it is a code change they will need to make.

State values can contain numbers that are larger than `float64` can
precisely represent. `json.Unmarshal` when unmarshaled into an
`interface{}` uses `float64` to represent numbers by default. To
properly retain precision for numbers, we need to use a custom
`json.Decoder` with the `UseNumber()` method called on it, which will
use `json.Number` (a `string` alias) to store the numbers, preserving
their precision.
@paddycarver paddycarver requested a review from kmoe December 16, 2020 19:18
@paultyng
Copy link
Contributor

Can you put a test with like random or something in here that simulates this?

@paddycarver
Copy link
Contributor Author

I opened hashicorp/terraform-provider-corner#16, but I can see about doing something inline here, as well?

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

LGTM. Test added in #114, confirmed working with a version of the random provider compiled with hashicorp/terraform-plugin-sdk@b78b8ce.

@kmoe kmoe merged commit d95ab21 into master Dec 17, 2020
@kmoe kmoe deleted the paddy_jsonnumber branch December 17, 2020 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants