Skip to content

Commit

Permalink
prevent an empty string from being lost
Browse files Browse the repository at this point in the history
The helper/schema diff process loses empty strings, causing them to show
up as unset (null) during apply. Besides failing to show as set by
GetOk, the absence of the value also triggers the schema to insert a
default value again during apply.

It would also be be preferable if the defaults weren't re-evaluated
again during ApplyResourceChange, but that would require a more invasive
patch to the field readers, and ensuring the empty string is stored in
the plan should block the default.
  • Loading branch information
jbardin committed Jun 19, 2019
1 parent 1bba574 commit 4dcda04
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 2 deletions.
19 changes: 19 additions & 0 deletions builtin/providers/test/resource_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,22 @@ resource "test_resource_defaults" "foo" {
},
})
}

func TestDefaults_emptyString(t *testing.T) {
config := `
resource "test_resource_defaults" "test" {
default_string = ""
}
`
resource.UnitTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: config,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("test_resource_defaults.test", "default_string", ""),
),
},
},
})
}
10 changes: 8 additions & 2 deletions helper/plugin/grpc_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,8 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
}
}

// check the invariants that we need below, to ensure we are working with
// non-null and known values.
if src.IsNull() || !src.IsKnown() || !dst.IsKnown() {
return dst
}
Expand Down Expand Up @@ -1319,8 +1321,12 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
return cty.ListVal(dsts)
}

case ty.IsPrimitiveType():
if dst.IsNull() && src.IsWhollyKnown() && apply {
case ty == cty.String:
// The legacy SDK should not be able to remove a value during plan or
// apply, however we are only going to overwrite this if the source was
// an empty string, since that is what is often equated with unset and
// lost in the diff process.
if dst.IsNull() && src.AsString() == "" {
return src
}
}
Expand Down
25 changes: 25 additions & 0 deletions helper/plugin/grpc_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -984,15 +984,40 @@ func TestNormalizeNullValues(t *testing.T) {
},
{
// Plan primitives are kept
Src: cty.ObjectVal(map[string]cty.Value{
"string": cty.NumberIntVal(0),
}),
Dst: cty.ObjectVal(map[string]cty.Value{
"string": cty.NullVal(cty.Number),
}),
Expect: cty.ObjectVal(map[string]cty.Value{
"string": cty.NullVal(cty.Number),
}),
},
{
// Neither plan nor apply should remove empty strings
Src: cty.ObjectVal(map[string]cty.Value{
"string": cty.StringVal(""),
}),
Dst: cty.ObjectVal(map[string]cty.Value{
"string": cty.NullVal(cty.String),
}),
Expect: cty.ObjectVal(map[string]cty.Value{
"string": cty.StringVal(""),
}),
},
{
// Neither plan nor apply should remove empty strings
Src: cty.ObjectVal(map[string]cty.Value{
"string": cty.StringVal(""),
}),
Dst: cty.ObjectVal(map[string]cty.Value{
"string": cty.NullVal(cty.String),
}),
Expect: cty.ObjectVal(map[string]cty.Value{
"string": cty.StringVal(""),
}),
Apply: true,
},
{
// The null map is retained, because the src was unknown
Expand Down

0 comments on commit 4dcda04

Please sign in to comment.