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

helper/schema: ResourceDiff ForceNew attribute correctness #17811

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

vancluever
Copy link
Contributor

@vancluever vancluever commented Apr 7, 2018

A couple of bugs have been discovered in ResourceDiff.ForceNew:

  • NewRemoved is not preserved when a diff for a key is already present.
    This is because the second diff that happens after customization
    performs a second getChange on not just state and config, but also on
    the pre-existing diff. This results in Exists == true, meaning nil is
    never returned as a new value.
  • ForceNew was doing the work of adding the key to the list of changed
    keys by doing a full SetNew on the existing value. This has a side
    effect of fetching zero values from what were otherwise undefined values
    and creating diffs for these values where there should not have been
    (example: "" => "0").

This update fixes these scenarios by:

  • Adding a new private function to check the existing diff for
    NewRemoved keys. This is included in the check on new values in
    diffChange.
  • Keys that have been flagged as ForceNew (or parent keys of lists and
    sets that have been flagged as ForceNew) are now maintained in a
    separate map. UpdatedKeys now returns the results of both of these maps,
    but otherwise these keys are ignored by ResourceDiff.
  • Pursuant the above, values are no longer pushed into the newDiff
    writer by ForceNew. This prevents the zero value problem, and makes for
    a cleaner implementation where the provider has to "manually" SetNew to
    update the appropriate values in the writer. It also prevents
    non-computed keys from winding up in the diff, which ResourceDiff
    normally blocks by design.

There are also a couple of tests for cases that should never come up
right now involving Optional/Computed values and NewRemoved, for which
explanations are given in annotations of each test. These are here to
guard against future regressions.

A couple of bugs have been discovered in ResourceDiff.ForceNew:

* NewRemoved is not preserved when a diff for a key is already present.
This is because the second diff that happens after customization
performs a second getChange on not just state and config, but also on
the pre-existing diff. This results in Exists == true, meaning nil is
never returned as a new value.
* ForceNew was doing the work of adding the key to the list of changed
keys by doing a full SetNew on the existing value. This has a side
effect of fetching zero values from what were otherwise undefined values
and creating diffs for these values where there should not have been
(example: "" => "0").

This update fixes these scenarios by:

* Adding a new private function to check the existing diff for
NewRemoved keys. This is included in the check on new values in
diffChange.
* Keys that have been flagged as ForceNew (or parent keys of lists and
sets that have been flagged as ForceNew) are now maintained in a
separate map. UpdatedKeys now returns the results of both of these maps,
but otherwise these keys are ignored by ResourceDiff.
* Pursuant the above, values are no longer pushed into the newDiff
writer by ForceNew. This prevents the zero value problem, and makes for
a cleaner implementation where the provider has to "manually" SetNew to
update the appropriate values in the writer. It also prevents
non-computed keys from winding up in the diff, which ResourceDiff
normally blocks by design.

There are also a couple of tests for cases that should never come up
right now involving Optional/Computed values and NewRemoved, for which
explanations are given in annotations of each test. These are here to
guard against future regressions.
@vancluever vancluever force-pushed the b-resourcediff-forcenew-newremoved-values branch from 47e1563 to d7048cb Compare April 8, 2018 14:56
@vancluever
Copy link
Contributor Author

Thanks @jbardin!

@vancluever vancluever merged commit b2a83f0 into master Apr 9, 2018
@pselle pselle deleted the b-resourcediff-forcenew-newremoved-values branch June 25, 2019 16:32
@ghost
Copy link

ghost commented Jul 24, 2019

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 ghost locked and limited conversation to collaborators Jul 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants