From ef086399f926d05b048cfaee099db70dd36bcb00 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Oct 2020 17:57:21 -0400 Subject: [PATCH] compare empty strings as null in sets The Legacy SDK cannot handle missing strings from objects in sets, and will insert an empty string when planning the missing value. This subverts the `couldHaveUnknownBlockPlaceholder` check, and causes errors when `dynamic` is used with NestingSet blocks. We don't have a separate codepath to handle the internals of AssertObjectCompatible differently for the legacy SDK, but we can treat empty strings as null strings within set objects to avoid the failed assertions. --- plans/objchange/compatible.go | 19 +++++- plans/objchange/compatible_test.go | 106 +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 3 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 6dc622dfa..6d92fca4d 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -344,7 +344,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl if nested && v.IsNull() { return true // for nested blocks, a single block being unset doesn't disqualify from being an unknown block placeholder } - return couldBeUnknownBlockPlaceholderElement(v, &blockS.Block) + return couldBeUnknownBlockPlaceholderElement(v, blockS) default: // These situations should be impossible for correct providers, but // we permit the legacy SDK to produce some incorrect outcomes @@ -360,7 +360,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl // For all other nesting modes, our value should be something iterable. for it := v.ElementIterator(); it.Next(); { _, ev := it.Element() - if couldBeUnknownBlockPlaceholderElement(ev, &blockS.Block) { + if couldBeUnknownBlockPlaceholderElement(ev, blockS) { return true } } @@ -374,7 +374,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl } } -func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.Block) bool { +func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.NestedBlock) bool { if v.IsNull() { return false // null value can never be a placeholder element } @@ -390,6 +390,19 @@ func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.Blo // non-placeholders can also match this, so this function can generate // false positives. if av.IsKnown() && !av.IsNull() { + + // FIXME: only required for the legacy SDK, but we don't have a + // separate codepath to switch the comparisons, and we still want + // the rest of the checks from AssertObjectCompatible to apply. + // + // The legacy SDK cannot handle missing strings from set elements, + // and will insert an empty string into the planned value. + // Skipping these treats them as null values in this case, + // preventing false alerts from AssertObjectCompatible. + if schema.Nesting == configschema.NestingSet && av.Type() == cty.String && av.AsString() == "" { + continue + } + return false } } diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 1eacdd8b1..9a6924441 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -991,6 +991,40 @@ func TestAssertObjectCompatible(t *testing.T) { }), nil, // as above, the presence of a block whose attrs are all unknown indicates dynamic block expansion, so our usual count checks don't apply }, + { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "key": { + Nesting: configschema.NestingList, + Block: schemaWithFooBar, + }, + }, + }, + // While we must make an exception for empty strings in sets due to + // the legacy SDK, lists should be compared more strictly. + // This does not count as a dynamic block placeholder + cty.ObjectVal(map[string]cty.Value{ + "key": cty.ListVal([]cty.Value{ + fooBarBlockValue, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), + "bar": cty.StringVal(""), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "key": cty.ListVal([]cty.Value{ + fooBlockValue, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("hello"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("world"), + }), + }), + }), + []string{".key: block count changed from 2 to 3"}, + }, // NestingSet blocks { @@ -1122,6 +1156,47 @@ func TestAssertObjectCompatible(t *testing.T) { // indicates this may be a dynamic block, and the length is unknown nil, }, + { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "block": { + Nesting: configschema.NestingSet, + Block: schemaWithFooBar, + }, + }, + }, + // The legacy SDK cannot handle missing strings in sets, and will + // insert empty strings to the planned value. Empty strings should + // be handled as nulls, and this object should represent a possible + // dynamic block. + cty.ObjectVal(map[string]cty.Value{ + "block": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), + "bar": cty.StringVal(""), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "block": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("hello"), + "bar": cty.StringVal(""), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("world"), + "bar": cty.StringVal(""), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("nope"), + "bar": cty.StringVal(""), + }), + }), + }), + // there is no error here, because the presence of unknowns + // indicates this may be a dynamic block, and the length is unknown + nil, + }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ @@ -1266,6 +1341,37 @@ func TestAssertObjectCompatible(t *testing.T) { }), nil, }, + { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "block": { + Nesting: configschema.NestingSet, + Block: schemaWithFooBar, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "block": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), + "bar": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "block": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("a"), + "bar": cty.StringVal(""), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("b"), + "bar": cty.StringVal(""), + }), + }), + }), + nil, + }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{