From fcbfc365e65bc7c6accb827ff08c799ab0c61729 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 20 Jan 2023 11:34:40 -0500 Subject: [PATCH 1/2] fix panics when handling null values in maps NestingMap structures are not well tested, and we panic in many situations when null crops up. Fix the first test cases and start refactoring best we can. This probably won't go so far as making all the objchange functions generic over Block and Object, but we can simplify a lot and verify parity in implementations for now. --- internal/plans/objchange/objchange.go | 101 +++++++++--------- internal/plans/objchange/objchange_test.go | 114 +++++++++++++++++++++ 2 files changed, 167 insertions(+), 48 deletions(-) diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index e46a1701a3..0bbd78b87b 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -275,6 +275,14 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. return newV } +func proposedNewObjectAttributes(attrs map[string]*configschema.Attribute, prior, config cty.Value) cty.Value { + if config.IsNull() { + return config + } + + return cty.ObjectVal(proposedNewAttributes(attrs, prior, config)) +} + func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, config cty.Value) map[string]cty.Value { newAttrs := make(map[string]cty.Value, len(attrs)) for name, attr := range attrs { @@ -324,7 +332,7 @@ func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, conf func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) cty.Value { // if the config isn't known at all, then we must use that value - if !config.IsNull() && !config.IsKnown() { + if !config.IsKnown() { return config } @@ -340,7 +348,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) break } - newV = cty.ObjectVal(proposedNewAttributes(schema.Attributes, prior, config)) + newV = proposedNewObjectAttributes(schema.Attributes, prior, config) case configschema.NestingList: // Nested blocks are correlated by index. @@ -361,8 +369,8 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } priorEV := prior.Index(idx) - newEV := proposedNewAttributes(schema.Attributes, priorEV, configEV) - newVals = append(newVals, cty.ObjectVal(newEV)) + newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newVals = append(newVals, newEV) } // Despite the name, a NestingList might also be a tuple, if // its nested schema contains dynamically-typed attributes. @@ -374,58 +382,55 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } case configschema.NestingMap: + configVLen := 0 + if config.IsKnown() && !config.IsNull() { + configVLen = config.LengthInt() + } + + if configVLen == 0 { + break + } + + newVals := make(map[string]cty.Value, configVLen) + // Despite the name, a NestingMap may produce either a map or // object value, depending on whether the nested schema contains // dynamically-typed attributes. if config.Type().IsObjectType() { // Nested blocks are correlated by key. - configVLen := 0 - if config.IsKnown() && !config.IsNull() { - configVLen = config.LengthInt() - } - if configVLen > 0 { - newVals := make(map[string]cty.Value, configVLen) - atys := config.Type().AttributeTypes() - for name := range atys { - configEV := config.GetAttr(name) - if !prior.IsKnown() || prior.IsNull() || !prior.Type().HasAttribute(name) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[name] = configEV - continue - } - priorEV := prior.GetAttr(name) - newEV := proposedNewAttributes(schema.Attributes, priorEV, configEV) - newVals[name] = cty.ObjectVal(newEV) + atys := config.Type().AttributeTypes() + for name := range atys { + configEV := config.GetAttr(name) + if !prior.IsKnown() || prior.IsNull() || !prior.Type().HasAttribute(name) { + // If there is no corresponding prior element then + // we just take the config value as-is. + newVals[name] = configEV + continue } - // Although we call the nesting mode "map", we actually use - // object values so that elements might have different types - // in case of dynamically-typed attributes. - newV = cty.ObjectVal(newVals) + priorEV := prior.GetAttr(name) + newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newVals[name] = newEV } + // Although we call the nesting mode "map", we actually use + // object values so that elements might have different types + // in case of dynamically-typed attributes. + newV = cty.ObjectVal(newVals) } else { - configVLen := 0 - if config.IsKnown() && !config.IsNull() { - configVLen = config.LengthInt() - } - if configVLen > 0 { - newVals := make(map[string]cty.Value, configVLen) - for it := config.ElementIterator(); it.Next(); { - idx, configEV := it.Element() - k := idx.AsString() - if prior.IsKnown() && (prior.IsNull() || !prior.HasIndex(idx).True()) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[k] = configEV - continue - } - priorEV := prior.Index(idx) - - newEV := proposedNewAttributes(schema.Attributes, priorEV, configEV) - newVals[k] = cty.ObjectVal(newEV) + for it := config.ElementIterator(); it.Next(); { + idx, configEV := it.Element() + k := idx.AsString() + if prior.IsKnown() && (prior.IsNull() || !prior.HasIndex(idx).True()) { + // If there is no corresponding prior element then + // we just take the config value as-is. + newVals[k] = configEV + continue } - newV = cty.MapVal(newVals) + priorEV := prior.Index(idx) + + newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newVals[k] = newEV } + newV = cty.MapVal(newVals) } case configschema.NestingSet: @@ -461,8 +466,8 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) if priorEV == cty.NilVal { newVals = append(newVals, configEV) } else { - newEV := proposedNewAttributes(schema.Attributes, priorEV, configEV) - newVals = append(newVals, cty.ObjectVal(newEV)) + newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newVals = append(newVals, newEV) } } newV = cty.SetVal(newVals) diff --git a/internal/plans/objchange/objchange_test.go b/internal/plans/objchange/objchange_test.go index 4711737627..a6eca0339e 100644 --- a/internal/plans/objchange/objchange_test.go +++ b/internal/plans/objchange/objchange_test.go @@ -752,6 +752,120 @@ func TestProposedNew(t *testing.T) { }), }), }, + + "prior optional computed nested map elem to null": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bloop": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "blop": { + Type: cty.String, + Optional: true, + }, + "bleep": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + Optional: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("glub"), + "bleep": cty.StringVal("computed"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("blub"), + "bleep": cty.StringVal("computed"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.MapVal(map[string]cty.Value{ + "a": cty.NullVal(cty.Object(map[string]cty.Type{ + "blop": cty.String, + "bleep": cty.String, + })), + "c": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("blub"), + "bleep": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.MapVal(map[string]cty.Value{ + "a": cty.NullVal(cty.Object(map[string]cty.Type{ + "blop": cty.String, + "bleep": cty.String, + })), + "c": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("blub"), + "bleep": cty.NullVal(cty.String), + }), + }), + }), + }, + + "prior optional computed nested map to null": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bloop": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "blop": { + Type: cty.String, + Optional: true, + }, + "bleep": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + Optional: true, + Computed: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("glub"), + "bleep": cty.StringVal("computed"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "blop": cty.StringVal("blub"), + "bleep": cty.StringVal("computed"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.NullVal(cty.Map( + cty.Object(map[string]cty.Type{ + "blop": cty.String, + "bleep": cty.String, + }), + )), + }), + cty.ObjectVal(map[string]cty.Value{ + "bloop": cty.NullVal(cty.Map( + cty.Object(map[string]cty.Type{ + "blop": cty.String, + "bleep": cty.String, + }), + )), + }), + }, + "prior nested map with dynamic": { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ From 375c2da3e32f798da8c5b3c091f871049b7deac3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Jan 2023 17:01:31 -0500 Subject: [PATCH 2/2] update NestingMap logic Simplify the logic in the NestingMap cases. Prevent uninitialized cty.NilVal from appearing in block case. --- internal/plans/objchange/objchange.go | 165 +++++++++++--------------- 1 file changed, 66 insertions(+), 99 deletions(-) diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index 0bbd78b87b..5ef72095f8 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -107,7 +107,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. return config } - var newV cty.Value + newV := config switch schema.Nesting { case configschema.NestingSingle: @@ -162,63 +162,43 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. } case configschema.NestingMap: - // Despite the name, a NestingMap may produce either a map or - // object value, depending on whether the nested schema contains - // dynamically-typed attributes. - if config.Type().IsObjectType() { - // Nested blocks are correlated by key. - configVLen := 0 - if config.IsKnown() && !config.IsNull() { - configVLen = config.LengthInt() - } - if configVLen > 0 { - newVals := make(map[string]cty.Value, configVLen) - atys := config.Type().AttributeTypes() - for name := range atys { - configEV := config.GetAttr(name) - if !prior.IsKnown() || prior.IsNull() || !prior.Type().HasAttribute(name) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[name] = configEV - continue - } - priorEV := prior.GetAttr(name) + newVals := map[string]cty.Value{} - newEV := ProposedNew(&schema.Block, priorEV, configEV) - newVals[name] = newEV - } - // Although we call the nesting mode "map", we actually use - // object values so that elements might have different types - // in case of dynamically-typed attributes. - newV = cty.ObjectVal(newVals) - } else { - newV = cty.EmptyObjectVal - } - } else { - configVLen := 0 - if config.IsKnown() && !config.IsNull() { - configVLen = config.LengthInt() - } - if configVLen > 0 { - newVals := make(map[string]cty.Value, configVLen) - for it := config.ElementIterator(); it.Next(); { - idx, configEV := it.Element() - k := idx.AsString() - if prior.IsKnown() && (prior.IsNull() || !prior.HasIndex(idx).True()) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[k] = configEV - continue - } - priorEV := prior.Index(idx) + if config.IsNull() || !config.IsKnown() || config.LengthInt() == 0 { + // We already assigned newVal and there's nothing to compare in + // config. + break + } + cfgMap := config.AsValueMap() - newEV := ProposedNew(&schema.Block, priorEV, configEV) - newVals[k] = newEV - } - newV = cty.MapVal(newVals) - } else { - newV = cty.MapValEmpty(schema.ImpliedType()) + // prior may be null or empty + priorMap := map[string]cty.Value{} + if !prior.IsNull() && prior.IsKnown() && prior.LengthInt() > 0 { + priorMap = prior.AsValueMap() + } + + for name, configEV := range cfgMap { + priorEV, inPrior := priorMap[name] + if !inPrior { + // If there is no corresponding prior element then + // we just take the config value as-is. + newVals[name] = configEV + continue } + + newEV := ProposedNew(&schema.Block, priorEV, configEV) + newVals[name] = newEV + } + + // The value must leave as the same type it came in as + switch { + case config.Type().IsObjectType(): + // Although we call the nesting mode "map", we actually use + // object values so that elements might have different types + // in case of dynamically-typed attributes. + newV = cty.ObjectVal(newVals) + default: + newV = cty.MapVal(newVals) } case configschema.NestingSet: @@ -275,12 +255,12 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. return newV } -func proposedNewObjectAttributes(attrs map[string]*configschema.Attribute, prior, config cty.Value) cty.Value { +func proposedNewObjectAttributes(schema *configschema.Object, prior, config cty.Value) cty.Value { if config.IsNull() { return config } - return cty.ObjectVal(proposedNewAttributes(attrs, prior, config)) + return cty.ObjectVal(proposedNewAttributes(schema.Attributes, prior, config)) } func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, config cty.Value) map[string]cty.Value { @@ -348,7 +328,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) break } - newV = proposedNewObjectAttributes(schema.Attributes, prior, config) + newV = proposedNewObjectAttributes(schema, prior, config) case configschema.NestingList: // Nested blocks are correlated by index. @@ -356,7 +336,6 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) if !config.IsNull() { configVLen = config.LengthInt() } - if configVLen > 0 { newVals := make([]cty.Value, 0, configVLen) for it := config.ElementIterator(); it.Next(); { @@ -369,7 +348,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } priorEV := prior.Index(idx) - newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newEV := proposedNewObjectAttributes(schema, priorEV, configEV) newVals = append(newVals, newEV) } // Despite the name, a NestingList might also be a tuple, if @@ -382,54 +361,42 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } case configschema.NestingMap: - configVLen := 0 - if config.IsKnown() && !config.IsNull() { - configVLen = config.LengthInt() - } + newVals := map[string]cty.Value{} - if configVLen == 0 { + if config.IsNull() || !config.IsKnown() || config.LengthInt() == 0 { + // We already assigned newVal and there's nothing to compare in + // config. break } + cfgMap := config.AsValueMap() - newVals := make(map[string]cty.Value, configVLen) + // prior may be null or empty + priorMap := map[string]cty.Value{} + if !prior.IsNull() && prior.IsKnown() && prior.LengthInt() > 0 { + priorMap = prior.AsValueMap() + } - // Despite the name, a NestingMap may produce either a map or - // object value, depending on whether the nested schema contains - // dynamically-typed attributes. - if config.Type().IsObjectType() { - // Nested blocks are correlated by key. - atys := config.Type().AttributeTypes() - for name := range atys { - configEV := config.GetAttr(name) - if !prior.IsKnown() || prior.IsNull() || !prior.Type().HasAttribute(name) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[name] = configEV - continue - } - priorEV := prior.GetAttr(name) - newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) - newVals[name] = newEV + for name, configEV := range cfgMap { + priorEV, inPrior := priorMap[name] + if !inPrior { + // If there is no corresponding prior element then + // we just take the config value as-is. + newVals[name] = configEV + continue } + + newEV := proposedNewObjectAttributes(schema, priorEV, configEV) + newVals[name] = newEV + } + + // The value must leave as the same type it came in as + switch { + case config.Type().IsObjectType(): // Although we call the nesting mode "map", we actually use // object values so that elements might have different types // in case of dynamically-typed attributes. newV = cty.ObjectVal(newVals) - } else { - for it := config.ElementIterator(); it.Next(); { - idx, configEV := it.Element() - k := idx.AsString() - if prior.IsKnown() && (prior.IsNull() || !prior.HasIndex(idx).True()) { - // If there is no corresponding prior element then - // we just take the config value as-is. - newVals[k] = configEV - continue - } - priorEV := prior.Index(idx) - - newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) - newVals[k] = newEV - } + default: newV = cty.MapVal(newVals) } @@ -466,7 +433,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) if priorEV == cty.NilVal { newVals = append(newVals, configEV) } else { - newEV := proposedNewObjectAttributes(schema.Attributes, priorEV, configEV) + newEV := proposedNewObjectAttributes(schema, priorEV, configEV) newVals = append(newVals, newEV) } }