From 902987061321e6860329d121178d166c254b8f46 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Sep 2021 15:55:20 -0400 Subject: [PATCH 1/2] handle NestedTypes in Block.CoerceValue The CoerceValue code was not updated to handle NestedTypes, and while none of the new codepaths make use of this method, there are still some internal uses. --- internal/configs/configschema/coerce_value.go | 48 +++++++-------- .../configs/configschema/coerce_value_test.go | 61 +++++++++++++++++++ 2 files changed, 84 insertions(+), 25 deletions(-) diff --git a/internal/configs/configschema/coerce_value.go b/internal/configs/configschema/coerce_value.go index 41a533745c..66804c3752 100644 --- a/internal/configs/configschema/coerce_value.go +++ b/internal/configs/configschema/coerce_value.go @@ -27,16 +27,19 @@ func (b *Block) CoerceValue(in cty.Value) (cty.Value, error) { } func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { + convType := b.specType() + impliedType := convType.WithoutOptionalAttributesDeep() + switch { case in.IsNull(): - return cty.NullVal(b.ImpliedType()), nil + return cty.NullVal(impliedType), nil case !in.IsKnown(): - return cty.UnknownVal(b.ImpliedType()), nil + return cty.UnknownVal(impliedType), nil } ty := in.Type() if !ty.IsObjectType() { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("an object is required") + return cty.UnknownVal(impliedType), path.NewErrorf("an object is required") } for name := range ty.AttributeTypes() { @@ -46,29 +49,32 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { if _, defined := b.BlockTypes[name]; defined { continue } - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("unexpected attribute %q", name) + return cty.UnknownVal(impliedType), path.NewErrorf("unexpected attribute %q", name) } attrs := make(map[string]cty.Value) for name, attrS := range b.Attributes { + attrType := impliedType.AttributeType(name) + attrConvType := convType.AttributeType(name) + var val cty.Value switch { case ty.HasAttribute(name): val = in.GetAttr(name) case attrS.Computed || attrS.Optional: - val = cty.NullVal(attrS.Type) + val = cty.NullVal(attrType) default: - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("attribute %q is required", name) + return cty.UnknownVal(impliedType), path.NewErrorf("attribute %q is required", name) } - val, err := attrS.coerceValue(val, append(path, cty.GetAttrStep{Name: name})) + val, err := convert.Convert(val, attrConvType) if err != nil { - return cty.UnknownVal(b.ImpliedType()), err + return cty.UnknownVal(impliedType), append(path, cty.GetAttrStep{Name: name}).NewError(err) } - attrs[name] = val } + for typeName, blockS := range b.BlockTypes { switch blockS.Nesting { @@ -79,7 +85,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { val := in.GetAttr(typeName) attrs[typeName], err = blockS.coerceValue(val, append(path, cty.GetAttrStep{Name: typeName})) if err != nil { - return cty.UnknownVal(b.ImpliedType()), err + return cty.UnknownVal(impliedType), err } default: attrs[typeName] = blockS.EmptyValue() @@ -100,7 +106,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { } if !coll.CanIterateElements() { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a list") + return cty.UnknownVal(impliedType), path.NewErrorf("must be a list") } l := coll.LengthInt() @@ -116,7 +122,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { idx, val := it.Element() val, err = blockS.coerceValue(val, append(path, cty.IndexStep{Key: idx})) if err != nil { - return cty.UnknownVal(b.ImpliedType()), err + return cty.UnknownVal(impliedType), err } elems = append(elems, val) } @@ -141,7 +147,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { } if !coll.CanIterateElements() { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a set") + return cty.UnknownVal(impliedType), path.NewErrorf("must be a set") } l := coll.LengthInt() @@ -157,7 +163,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { idx, val := it.Element() val, err = blockS.coerceValue(val, append(path, cty.IndexStep{Key: idx})) if err != nil { - return cty.UnknownVal(b.ImpliedType()), err + return cty.UnknownVal(impliedType), err } elems = append(elems, val) } @@ -182,7 +188,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { } if !coll.CanIterateElements() { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a map") + return cty.UnknownVal(impliedType), path.NewErrorf("must be a map") } l := coll.LengthInt() if l == 0 { @@ -196,11 +202,11 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { var err error key, val := it.Element() if key.Type() != cty.String || key.IsNull() || !key.IsKnown() { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a map") + return cty.UnknownVal(impliedType), path.NewErrorf("must be a map") } val, err = blockS.coerceValue(val, append(path, cty.IndexStep{Key: key})) if err != nil { - return cty.UnknownVal(b.ImpliedType()), err + return cty.UnknownVal(impliedType), err } elems[key.AsString()] = val } @@ -240,11 +246,3 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { return cty.ObjectVal(attrs), nil } - -func (a *Attribute) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { - val, err := convert.Convert(in, a.Type) - if err != nil { - return cty.UnknownVal(a.Type), path.NewError(err) - } - return val, nil -} diff --git a/internal/configs/configschema/coerce_value_test.go b/internal/configs/configschema/coerce_value_test.go index 3f57b174be..37f81b7698 100644 --- a/internal/configs/configschema/coerce_value_test.go +++ b/internal/configs/configschema/coerce_value_test.go @@ -538,6 +538,67 @@ func TestCoerceValue(t *testing.T) { }), ``, }, + "nested types": { + // handle NestedTypes + &Block{ + Attributes: map[string]*Attribute{ + "foo": { + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.String, + Required: true, + }, + "baz": { + Type: cty.Map(cty.String), + Optional: true, + }, + }, + }, + Optional: true, + }, + "fob": { + NestedType: &Object{ + Nesting: NestingSet, + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.String, + Optional: true, + }, + }, + }, + Optional: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("beep"), + "baz": cty.NullVal(cty.Map(cty.String)), + }), + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("boop"), + "baz": cty.NullVal(cty.Map(cty.String)), + }), + }), + "fob": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "bar": cty.String, + }))), + }), + ``, + }, } for name, test := range tests { From 331dc8b14ccdb75d6ed19e740803bf839c218842 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Sep 2021 16:36:45 -0400 Subject: [PATCH 2/2] handle empty containers in ProposedNew NestedTypes Empty containers of NestedTypes were not handled in ProposedNew, causing plans to be submitted with null values where there was configuration present. --- internal/plans/objchange/objchange.go | 13 ++-- internal/plans/objchange/objchange_test.go | 75 ++++++++++++++++++++++ 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index 281b9dd13a..739d666480 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -308,7 +308,9 @@ func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, conf } func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) cty.Value { - var newV cty.Value + // If the config is null or empty, we will be using this default value. + newV := config + switch schema.Nesting { case configschema.NestingSingle: if !config.IsNull() { @@ -323,6 +325,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) if config.IsKnown() && !config.IsNull() { configVLen = config.LengthInt() } + if configVLen > 0 { newVals := make([]cty.Value, 0, configVLen) for it := config.ElementIterator(); it.Next(); { @@ -345,8 +348,6 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } else { newV = cty.ListVal(newVals) } - } else { - newV = cty.NullVal(schema.ImpliedType()) } case configschema.NestingMap: @@ -378,8 +379,6 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) // object values so that elements might have different types // in case of dynamically-typed attributes. newV = cty.ObjectVal(newVals) - } else { - newV = cty.NullVal(schema.ImpliedType()) } } else { configVLen := 0 @@ -403,8 +402,6 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) newVals[k] = cty.ObjectVal(newEV) } newV = cty.MapVal(newVals) - } else { - newV = cty.NullVal(schema.ImpliedType()) } } @@ -446,8 +443,6 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) } } newV = cty.SetVal(newVals) - } else { - newV = cty.NullVal(schema.ImpliedType()) } } diff --git a/internal/plans/objchange/objchange_test.go b/internal/plans/objchange/objchange_test.go index 77147989b8..b0021fb14b 100644 --- a/internal/plans/objchange/objchange_test.go +++ b/internal/plans/objchange/objchange_test.go @@ -1461,6 +1461,81 @@ func TestProposedNew(t *testing.T) { }))), }), }, + "expected empty NestedTypes": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "set": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSet, + Attributes: map[string]*configschema.Attribute{ + "bar": {Type: cty.String}, + }, + }, + Optional: true, + }, + "map": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "bar": {Type: cty.String}, + }, + }, + Optional: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + "set": cty.SetValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + }), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + "set": cty.SetValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + }), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + "set": cty.SetValEmpty(cty.Object(map[string]cty.Type{"bar": cty.String})), + }), + }, + "optional types set replacement": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "set": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSet, + Attributes: map[string]*configschema.Attribute{ + "bar": { + Type: cty.String, + Required: true, + }, + }, + }, + Optional: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("old"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("new"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.StringVal("new"), + }), + }), + }), + }, } for name, test := range tests {