Merge pull request #34740 from hashicorp/jbardin/render-string-null-changes

try to render some changes between `""` and `null`

internal/command/jsonformat/differ/attribute.go

@@ -44,6 +44,18 @@ func computeDiffForNestedAttribute(change structured.Change, nested *jsonprovide
 }
 
 func ComputeDiffForType(change structured.Change, ctype cty.Type) computed.Diff {
+	if !change.NonLegacySchema {
+		// Empty strings in blocks should be considered null, because the legacy
+		// SDK can't always differentiate between null and empty strings and may
+		// return either.
+		if before, ok := change.Before.(string); ok && len(before) == 0 {
+			change.Before = nil
+		}
+		if after, ok := change.After.(string); ok && len(after) == 0 {
+			change.After = nil
+		}
+	}
+
 	if sensitive, ok := checkForSensitiveType(change, ctype); ok {
 		return sensitive
 	}

internal/command/jsonformat/differ/block.go

@@ -21,6 +21,10 @@ func ComputeDiffForBlock(change structured.Change, block *jsonprovider.Block) co
 		return unknown
 	}
 
+	// NonLegacyValue is only ever switched from false to true, since the
+	// behavior would be for the entire resource.
+	change.NonLegacySchema = change.NonLegacySchema || containsNonLegacyFeatures(block)
+
 	current := change.GetDefaultActionForIteration()
 
 	blockValue := change.AsMap()
@@ -34,15 +38,6 @@ func ComputeDiffForBlock(change structured.Change, block *jsonprovider.Block) co
 			childValue = childValue.AsNoOp()
 		}
 
-		// Empty strings in blocks should be considered null for legacy reasons.
-		// The SDK doesn't support null strings yet, so we work around this now.
-		if before, ok := childValue.Before.(string); ok && len(before) == 0 {
-			childValue.Before = nil
-		}
-		if after, ok := childValue.After.(string); ok && len(after) == 0 {
-			childValue.After = nil
-		}
-
 		// Always treat changes to blocks as implicit.
 		childValue.BeforeExplicit = false
 		childValue.AfterExplicit = false
@@ -119,3 +114,40 @@ func ComputeDiffForBlock(change structured.Change, block *jsonprovider.Block) co
 
 	return computed.NewDiff(renderers.Block(attributes, blocks), current, change.ReplacePaths.Matches())
 }
+
+// containsNonLegacyFeatures checks for features not supported by the legacy
+// SDK, so that we can skip the empty string -> null fixup for them.
+func containsNonLegacyFeatures(block *jsonprovider.Block) bool {
+	for _, blockType := range block.BlockTypes {
+		switch NestingMode(blockType.NestingMode) {
+		case nestingModeMap, nestingModeGroup:
+			// these block types were not possible in the SDK
+			return true
+		}
+	}
+
+	for _, attribute := range block.Attributes {
+		//nested object types were not possible in the SDK
+		if attribute.AttributeNestedType != nil {
+			return true
+		}
+
+		ty := unmarshalAttribute(attribute)
+		// these types were not possible in the SDK
+		switch {
+		case ty.HasDynamicTypes():
+			return true
+		case ty.IsTupleType() || ty.IsObjectType():
+			return true
+		case ty.IsCollectionType():
+			// Nested collections were not really supported, but could be
+			// generated with string types (though we conservatively limit this
+			// to primitive types)
+			ety := ty.ElementType()
+			if ety.IsCollectionType() && !ety.ElementType().IsPrimitiveType() {
+				return true
+			}
+		}
+	}
+	return false
+}

internal/command/jsonformat/plan_test.go

@@ -565,7 +565,8 @@ func TestResourceChange_primitiveTypes(t *testing.T) {
 			RequiredReplace: cty.NewPathSet(),
 			ExpectedOutput: `  # test_instance.example will be destroyed
   - resource "test_instance" "example" {
-      - id = "i-02ae66f368e8518a9" -> null
+      - id                 = "i-02ae66f368e8518a9" -> null
+        # (1 unchanged attribute hidden)
     }`,
 		},
 		"forget": {
@@ -724,24 +725,95 @@ func TestResourceChange_primitiveTypes(t *testing.T) {
 				"id":        cty.StringVal("i-02ae66f368e8518a9"),
 				"ami":       cty.StringVal("ami-BEFORE"),
 				"unchanged": cty.NullVal(cty.String),
+				"empty":     cty.StringVal(""),
 			}),
 			After: cty.ObjectVal(map[string]cty.Value{
 				"id":        cty.StringVal("i-02ae66f368e8518a9"),
 				"ami":       cty.StringVal("ami-AFTER"),
 				"unchanged": cty.NullVal(cty.String),
+				"empty":     cty.NullVal(cty.String),
 			}),
 			Schema: &configschema.Block{
 				Attributes: map[string]*configschema.Attribute{
 					"id":        {Type: cty.String, Optional: true, Computed: true},
 					"ami":       {Type: cty.String, Optional: true},
 					"unchanged": {Type: cty.String, Optional: true},
+					"empty":     {Type: cty.String, Optional: true},
+				},
+			},
+			RequiredReplace: cty.NewPathSet(),
+			ExpectedOutput: `  # test_instance.example will be updated in-place
+  ~ resource "test_instance" "example" {
+      ~ ami   = "ami-BEFORE" -> "ami-AFTER"
+        id    = "i-02ae66f368e8518a9"
+        # (1 unchanged attribute hidden)
+    }`,
+		},
+		"string update (non-legacy)": {
+			Action: plans.Update,
+			Mode:   addrs.ManagedResourceMode,
+			Before: cty.ObjectVal(map[string]cty.Value{
+				"id":        cty.StringVal("i-02ae66f368e8518a9"),
+				"from_null": cty.NullVal(cty.String),
+				"to_null":   cty.StringVal(""),
+			}),
+			After: cty.ObjectVal(map[string]cty.Value{
+				"id":        cty.StringVal("i-02ae66f368e8518a9"),
+				"from_null": cty.StringVal(""),
+				"to_null":   cty.NullVal(cty.String),
+			}),
+			Schema: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id":        {Type: cty.String, Optional: true, Computed: true},
+					"from_null": {Type: cty.DynamicPseudoType, Optional: true},
+					"to_null":   {Type: cty.String, Optional: true},
+				},
+			},
+			RequiredReplace: cty.NewPathSet(),
+			ExpectedOutput: `  # test_instance.example will be updated in-place
+  ~ resource "test_instance" "example" {
+      + from_null = ""
+        id        = "i-02ae66f368e8518a9"
+      - to_null   = "" -> null
+    }`,
+		},
+		"string update (non-legacy nested object)": {
+			Action: plans.Update,
+			Mode:   addrs.ManagedResourceMode,
+			Before: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("i-02ae66f368e8518a9"),
+				"obj": cty.ObjectVal(map[string]cty.Value{
+					"from_null": cty.NullVal(cty.String),
+					"to_null":   cty.StringVal(""),
+				}),
+			}),
+			After: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("i-02ae66f368e8518a9"),
+				"obj": cty.ObjectVal(map[string]cty.Value{
+					"from_null": cty.StringVal(""),
+					"to_null":   cty.NullVal(cty.String),
+				}),
+			}),
+			Schema: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id": {Type: cty.String, Optional: true, Computed: true},
+					"obj": {NestedType: &configschema.Object{
+						Nesting: configschema.NestingSingle,
+						Attributes: map[string]*configschema.Attribute{
+							"from_null": {Type: cty.DynamicPseudoType, Optional: true},
+							"to_null":   {Type: cty.String, Optional: true},
+						},
+					}},
 				},
 			},
 			RequiredReplace: cty.NewPathSet(),
 			ExpectedOutput: `  # test_instance.example will be updated in-place
   ~ resource "test_instance" "example" {
-      ~ ami = "ami-BEFORE" -> "ami-AFTER"
         id  = "i-02ae66f368e8518a9"
+      ~ obj = {
+          + from_null = ""
+          - to_null   = "" -> null
+        }
     }`,
 		},
 		"in-place update of multi-line string field": {

internal/command/jsonformat/structured/change.go

@@ -90,6 +90,14 @@ type Change struct {
 	// that we should display. Any element/attribute not matched by this Matcher
 	// should be skipped.
 	RelevantAttributes attribute_path.Matcher
+
+	// NonLegacySchema must only be used when rendering the change to the CLI,
+	// and is otherwise ignored. This flag is set when we can be sure that the
+	// change originated from a resource which is not using the legacy SDK, so
+	// we don't need to hide changes between empty and null strings.
+	// NonLegacySchema is only switched to true by the renderer, because that is
+	// where we have most of the schema information to detect the condition.
+	NonLegacySchema bool
 }
 
 // FromJsonChange unmarshals the raw []byte values in the jsonplan.Change

internal/command/jsonformat/structured/map.go

@@ -33,6 +33,10 @@ type ChangeMap struct {
 
 	// RelevantAttributes matches the same attributes in Change exactly.
 	RelevantAttributes attribute_path.Matcher
+
+	// this reflects the parent NonLegacyValue, so that any behavior is
+	// automatically inherited into child changes.
+	nonLegacySchema bool
 }
 
 // AsMap converts the Change into an object or map representation by converting
@@ -47,6 +51,7 @@ func (change Change) AsMap() ChangeMap {
 		AfterSensitive:     genericToMap(change.AfterSensitive),
 		ReplacePaths:       change.ReplacePaths,
 		RelevantAttributes: change.RelevantAttributes,
+		nonLegacySchema:    change.NonLegacySchema,
 	}
 }
 
@@ -69,6 +74,7 @@ func (m ChangeMap) GetChild(key string) Change {
 		AfterSensitive:     afterSensitive,
 		ReplacePaths:       m.ReplacePaths.GetChildWithKey(key),
 		RelevantAttributes: m.RelevantAttributes.GetChildWithKey(key),
+		NonLegacySchema:    m.nonLegacySchema,
 	}
 }