Merge pull request #26638 from hashicorp/jbardin/assert-compat-dynamic-blocks

Treat empty strings as null in NestingSet objs when looking for dynamic blocks in AssertObjectCompatible

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
 		}
 	}

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{