Add a flag for using json.Number from state #662
Conversation
Go's JSON decoder, by default, uses a lossy conversion of JSON integers to float64s. For sufficiently large integers, this yields a loss of precision, and that causes problems with diffs and plans not matching. See #655 for more details. This PR proposes a solution: keeping the lossless json.Number representation of integers in state files. This will ensure that no precision is lost, but it comes at a cost. json.Number is a string type, not a float64 type. That means that existing state upgraders that (correctly) cast a value to float64 will break, as the value will now be surfaced to them as a json.Number, which cannot be cast to a float64. To handle this, the schema.Resource type gains a `UseJSONNumber` property. When set to new, users are opted into the new json.Number values. When left false, the default, users get the existing float64 behavior. If a resource with state upgraders wants to use the new json.Number behavior, it must update all the state upgraders for that resource to use json.Number instead of float64 or users with old state files will panic during upgrades. Note: the backwards compatibility properties of this commit are unknown at this time, and there may be a way for us to avoid using schema.Resource.UseJSONNumber, instead preserving the choice until we get to the point where TypeInt casts the number to an int. Further investigation needed.
Use the tagged release of tfjson, and opt into the json.Number handling.
|
@kmoe if you want to cut (or are okay with me cutting) a release of terraform-exec, we can use an actual released version of it in this PR. |
|
Why make this opt in for state upgraders that are just broken until it's set? |
They're only broken in theory. In practice, many of them work, and continue to work with float64 because they're not using large enough numbers to matter. Not using this flag would require a code change before their provider wouldn't panic at runtime, which feels like a breaking change to me, and that seems like overkill if we can avoid it. |
|
We're going to run with this as-is, to offer an immediate workaround for provider devs. In January, we'll look into turning it on by default for any provider that doesn't use StateUpgraders, as research indicates that may be safe and non-breaking. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
1 similar comment
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked as resolved and limited conversation to collaborators
Add a flag to schema.Resource that will parse numbers in state to json.Number. This should have no impact on testing, which uses a string representation of the number, anyways, or on MigrateStateFuncs, which similarly use a string representation. When the flag is flipped, however, the StateUpgraders will need to be updated to use json.Number instead of float64 in their type assertions. This is why this is off by default.
Turning this on, however, will prevent errors about plans not matching applies when using sufficiently large integers with Terraform. See #655 for more details on the problem that can arise from keeping float64s.
Most providers, especially on resources without StateUpgraders, should toggle the flag on to avoid these kinds of problems before they start.