From bdd5a2cd33b766745c1b406525b0f83e5a1711c2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 27 Feb 2024 16:04:37 -0500 Subject: [PATCH] try to render some empty string -> null changes Changes between empty strings and `null` were hidden in the CLI output, because the SDK could not reliably detect the difference and may return either value depending on the situation. This legacy behavior can be confusing for authors of new provider which can correctly handle `null`, and it would be preferable to be able to render those changes in the CLI. While we don't have enough information to detect when the legacy behavior is required, we can detect a number of cases where it's certain that we are not dealing with a legacy schema and should output the full diff. --- .../command/jsonformat/differ/attribute.go | 12 +++ internal/command/jsonformat/differ/block.go | 50 +++++++++--- internal/command/jsonformat/plan_test.go | 76 ++++++++++++++++++- .../command/jsonformat/structured/change.go | 8 ++ internal/command/jsonformat/structured/map.go | 6 ++ 5 files changed, 141 insertions(+), 11 deletions(-) diff --git a/internal/command/jsonformat/differ/attribute.go b/internal/command/jsonformat/differ/attribute.go index 017fc02096..698c3a494c 100644 --- a/internal/command/jsonformat/differ/attribute.go +++ b/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 } diff --git a/internal/command/jsonformat/differ/block.go b/internal/command/jsonformat/differ/block.go index c6d0510706..a53e11ac50 100644 --- a/internal/command/jsonformat/differ/block.go +++ b/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 +} diff --git a/internal/command/jsonformat/plan_test.go b/internal/command/jsonformat/plan_test.go index 3668b1f91a..754db9156d 100644 --- a/internal/command/jsonformat/plan_test.go +++ b/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": { diff --git a/internal/command/jsonformat/structured/change.go b/internal/command/jsonformat/structured/change.go index 65af43e45c..5a225be842 100644 --- a/internal/command/jsonformat/structured/change.go +++ b/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 diff --git a/internal/command/jsonformat/structured/map.go b/internal/command/jsonformat/structured/map.go index f1fed0c5bb..6d507b625a 100644 --- a/internal/command/jsonformat/structured/map.go +++ b/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, } }