From 18d354223e5a4b039fdf2d10c2c7c6dba6269992 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Oct 2021 14:21:16 -0400 Subject: [PATCH 1/2] objchange: fix ProposedNew from null objects The codepath for AllAttributesNull was not correct for any nested object types with collections, and should create single null values for the correct NestingMode rather than a single object with null attributes. Since there is no reason to descend into nested object types to create nullv alues, we can drop the AllAttributesNull function altogether and create null values as needed during ProposedNew. The corresponding AllBlockAttributesNull was only called internally in 1 location, and simply delegated to schema.EmptyValue. We can reduce the package surface area by dropping that function too and calling EmptyValue directly. --- internal/plans/objchange/all_null.go | 33 ------- internal/plans/objchange/objchange.go | 16 +++- internal/plans/objchange/objchange_test.go | 104 +++++++++++++++++++++ 3 files changed, 115 insertions(+), 38 deletions(-) delete mode 100644 internal/plans/objchange/all_null.go diff --git a/internal/plans/objchange/all_null.go b/internal/plans/objchange/all_null.go deleted file mode 100644 index fb7ec4cfb..000000000 --- a/internal/plans/objchange/all_null.go +++ /dev/null @@ -1,33 +0,0 @@ -package objchange - -import ( - "github.com/hashicorp/terraform/internal/configs/configschema" - "github.com/zclconf/go-cty/cty" -) - -// AllBlockAttributesNull constructs a non-null cty.Value of the object type implied -// by the given schema that has all of its leaf attributes set to null and all -// of its nested block collections set to zero-length. -// -// This simulates what would result from decoding an empty configuration block -// with the given schema, except that it does not produce errors -func AllBlockAttributesNull(schema *configschema.Block) cty.Value { - // "All attributes null" happens to be the definition of EmptyValue for - // a Block, so we can just delegate to that. - return schema.EmptyValue() -} - -// AllAttributesNull returns a cty.Value of the object type implied by the given -// attriubutes that has all of its leaf attributes set to null. -func AllAttributesNull(attrs map[string]*configschema.Attribute) cty.Value { - newAttrs := make(map[string]cty.Value, len(attrs)) - - for name, attr := range attrs { - if attr.NestedType != nil { - newAttrs[name] = AllAttributesNull(attr.NestedType.Attributes) - } else { - newAttrs[name] = cty.NullVal(attr.Type) - } - } - return cty.ObjectVal(newAttrs) -} diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index 739d66648..63e846451 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -37,7 +37,10 @@ func ProposedNew(schema *configschema.Block, prior, config cty.Value) cty.Value // similar to the result of decoding an empty configuration block, // which simplifies our handling of the top-level attributes/blocks // below by giving us one non-null level of object to pull values from. - prior = AllBlockAttributesNull(schema) + // + // "All attributes null" happens to be the definition of EmptyValue for + // a Block, so we can just delegate to that + prior = schema.EmptyValue() } return proposedNew(schema, prior, config) } @@ -258,12 +261,15 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. } func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, config cty.Value) map[string]cty.Value { - if prior.IsNull() { - prior = AllAttributesNull(attrs) - } newAttrs := make(map[string]cty.Value, len(attrs)) for name, attr := range attrs { - priorV := prior.GetAttr(name) + var priorV cty.Value + if prior.IsNull() { + priorV = cty.NullVal(prior.Type().AttributeType(name)) + } else { + priorV = prior.GetAttr(name) + } + configV := config.GetAttr(name) var newV cty.Value switch { diff --git a/internal/plans/objchange/objchange_test.go b/internal/plans/objchange/objchange_test.go index b0021fb14..32a56320c 100644 --- a/internal/plans/objchange/objchange_test.go +++ b/internal/plans/objchange/objchange_test.go @@ -1536,6 +1536,110 @@ func TestProposedNew(t *testing.T) { }), }), }, + "prior null nested objects": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "single": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSingle, + Attributes: map[string]*configschema.Attribute{ + "list": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + }, + }, + }, + Optional: true, + }, + }, + }, + Optional: true, + }, + "map": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "map": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + }, + }, + }, + Optional: true, + }, + }, + }, + Optional: true, + }, + }, + }, + cty.NullVal(cty.Object(map[string]cty.Type{ + "single": cty.Object(map[string]cty.Type{ + "list": cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + }), + "map": cty.Map(cty.Object(map[string]cty.Type{ + "list": cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + })), + })), + cty.ObjectVal(map[string]cty.Value{ + "single": cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("a"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("b"), + }), + }), + }), + "map": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("a"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("b"), + }), + }), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "single": cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("a"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("b"), + }), + }), + }), + "map": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("a"), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("b"), + }), + }), + }), + }), + }), + }, } for name, test := range tests { From 58d85fcc2e21c2b68d4b0c391422f5b001be0a9b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 5 Oct 2021 12:31:23 -0400 Subject: [PATCH 2/2] add test for planing unknown data source values --- internal/plans/objchange/objchange_test.go | 48 ++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/internal/plans/objchange/objchange_test.go b/internal/plans/objchange/objchange_test.go index 32a56320c..4c434d3e4 100644 --- a/internal/plans/objchange/objchange_test.go +++ b/internal/plans/objchange/objchange_test.go @@ -1640,6 +1640,54 @@ func TestProposedNew(t *testing.T) { }), }), }, + + // data sources are planned with an unknown value + "unknown prior nested objects": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "list": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "list": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + }, + }, + }, + Computed: true, + }, + }, + }, + Computed: true, + }, + }, + }, + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "List": cty.List(cty.Object(map[string]cty.Type{ + "list": cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + })), + })), + cty.NullVal(cty.Object(map[string]cty.Type{ + "List": cty.List(cty.Object(map[string]cty.Type{ + "list": cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + })), + })), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "List": cty.List(cty.Object(map[string]cty.Type{ + "list": cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + })), + })), + }, } for name, test := range tests {