From 1cf4909b280d9df6333fb6d324103dd3225b4832 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 12 Feb 2021 13:34:25 -0500 Subject: [PATCH] configschema: fix various issues with NestedTypes A handful of bugs popped up while extending the testing in plans/objchange. The main themes were failing to recurse through deeply nested NestedType attributes and improperly building up the ImpliedType. This commit fixes those issues and extends the test coverage to match. --- configs/configschema/decoder_spec.go | 28 ++---- configs/configschema/decoder_spec_test.go | 107 ++++++++++++++++++++-- configs/configschema/empty_value.go | 11 +-- configs/configschema/empty_value_test.go | 26 ------ configs/configschema/implied_type.go | 26 +++++- configs/configschema/implied_type_test.go | 84 ++++++++++++++++- 6 files changed, 212 insertions(+), 70 deletions(-) diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 8325eaf60..f758d27d2 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -197,21 +197,9 @@ func (a *Attribute) decoderSpec(name string) hcldec.Spec { panic("Invalid attribute schema: NestedType and Type cannot both be set. This is a bug in the provider.") } - var optAttrs []string - optAttrs = listOptionalAttrsFromObject(a.NestedType, optAttrs) ty := a.NestedType.ImpliedType() - - switch a.NestedType.Nesting { - case NestingList: - ret.Type = cty.List(ty) - case NestingSet: - ret.Type = cty.Set(ty) - case NestingMap: - ret.Type = cty.Map(ty) - default: // NestingSingle, NestingGroup, or no NestingMode - ret.Type = ty - } - ret.Required = a.NestedType.MinItems > 0 + ret.Type = ty + ret.Required = a.Required || a.NestedType.MinItems > 0 return ret } @@ -220,12 +208,16 @@ func (a *Attribute) decoderSpec(name string) hcldec.Spec { return ret } -func listOptionalAttrsFromObject(o *Object, optAttrs []string) []string { +// listOptionalAttrsFromObject is a helper function which does *not* recurse +// into NestedType Attributes, because the optional types for each of those will +// belong to their own cty.Object definitions. It is used in other functions +// which themselves handle that recursion. +func listOptionalAttrsFromObject(o *Object) []string { + var ret []string for name, attr := range o.Attributes { if attr.Optional == true { - optAttrs = append(optAttrs, name) + ret = append(ret, name) } } - - return optAttrs + return ret } diff --git a/configs/configschema/decoder_spec_test.go b/configs/configschema/decoder_spec_test.go index 1f3691ed2..1eefc4d8b 100644 --- a/configs/configschema/decoder_spec_test.go +++ b/configs/configschema/decoder_spec_test.go @@ -451,7 +451,7 @@ func TestAttributeDecoderSpec(t *testing.T) { Type: cty.String, Optional: true, }, - hcl.EmptyBody(), + hcltest.MockBody(&hcl.BodyContent{}), cty.NullVal(cty.String), 0, }, @@ -474,6 +474,7 @@ func TestAttributeDecoderSpec(t *testing.T) { "NestedType with required string": { &Attribute{ NestedType: &Object{ + Nesting: NestingSingle, Attributes: map[string]*Attribute{ "foo": { Type: cty.String, @@ -501,6 +502,7 @@ func TestAttributeDecoderSpec(t *testing.T) { "NestedType with optional attributes": { &Attribute{ NestedType: &Object{ + Nesting: NestingSingle, Attributes: map[string]*Attribute{ "foo": { Type: cty.String, @@ -533,6 +535,7 @@ func TestAttributeDecoderSpec(t *testing.T) { "NestedType with missing required string": { &Attribute{ NestedType: &Object{ + Nesting: NestingSingle, Attributes: map[string]*Attribute{ "foo": { Type: cty.String, @@ -575,11 +578,9 @@ func TestAttributeDecoderSpec(t *testing.T) { Name: "attr", Expr: hcltest.MockExprLiteral(cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("bar"), }), cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("baz"), }), })), @@ -638,11 +639,9 @@ func TestAttributeDecoderSpec(t *testing.T) { Name: "attr", Expr: hcltest.MockExprLiteral(cty.SetVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("bar"), }), cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("baz"), }), })), @@ -701,11 +700,9 @@ func TestAttributeDecoderSpec(t *testing.T) { Name: "attr", Expr: hcltest.MockExprLiteral(cty.MapVal(map[string]cty.Value{ "one": cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("bar"), }), "two": cty.ObjectVal(map[string]cty.Value{ - // "foo" should be a string, not a list "foo": cty.StringVal("baz"), }), })), @@ -747,6 +744,102 @@ func TestAttributeDecoderSpec(t *testing.T) { cty.UnknownVal(cty.Map(cty.Object(map[string]cty.Type{"foo": cty.String}))), 1, }, + "deeply NestedType NestingModeList valid": { + &Attribute{ + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "foo": { + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.String, + Required: true, + }, + }, + }, + Required: true, + }, + }, + }, + Optional: true, + }, + hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "attr": { + Name: "attr", + Expr: hcltest.MockExprLiteral(cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("baz")}), + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("boz")}), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("biz")}), + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("buz")}), + }), + }), + })), + }, + }, + }), + cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("baz")}), + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("boz")}), + })}), + cty.ObjectVal(map[string]cty.Value{"foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("biz")}), + cty.ObjectVal(map[string]cty.Value{"bar": cty.StringVal("buz")}), + })}), + }), + 0, + }, + "deeply NestedType NestingList invalid": { + &Attribute{ + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "foo": { + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "bar": { + Type: cty.Number, + Required: true, + }, + }, + }, + Required: true, + }, + }, + }, + Optional: true, + }, + hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "attr": { + Name: "attr", + Expr: hcltest.MockExprLiteral(cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + // bar should be a Number + cty.ObjectVal(map[string]cty.Value{"bar": cty.True}), + cty.ObjectVal(map[string]cty.Value{"bar": cty.False}), + }), + }), + })), + }, + }, + }), + cty.UnknownVal(cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.List(cty.Object(map[string]cty.Type{"bar": cty.Number})), + }))), + 1, + }, } for name, test := range tests { diff --git a/configs/configschema/empty_value.go b/configs/configschema/empty_value.go index a51468494..ea8cdbbcc 100644 --- a/configs/configschema/empty_value.go +++ b/configs/configschema/empty_value.go @@ -27,16 +27,7 @@ func (b *Block) EmptyValue() cty.Value { // at all, ignoring any required constraint. func (a *Attribute) EmptyValue() cty.Value { if a.NestedType != nil { - switch a.NestedType.Nesting { - case NestingList: - return cty.NullVal(cty.List(a.NestedType.ImpliedType())) - case NestingSet: - return cty.NullVal(cty.Set(a.NestedType.ImpliedType())) - case NestingMap: - return cty.NullVal(cty.Map(a.NestedType.ImpliedType())) - default: // NestingSingle, NestingGroup, or no NestingMode - return cty.NullVal(a.NestedType.ImpliedType()) - } + return cty.NullVal(a.NestedType.ImpliedType()) } return cty.NullVal(a.Type) } diff --git a/configs/configschema/empty_value_test.go b/configs/configschema/empty_value_test.go index a11ab4cdb..a9d8e3074 100644 --- a/configs/configschema/empty_value_test.go +++ b/configs/configschema/empty_value_test.go @@ -186,19 +186,6 @@ func TestAttributeEmptyValue(t *testing.T) { }, cty.NullVal(cty.String), }, - { - &Attribute{ - NestedType: &Object{ - // no Nesting set should behave the same as NestingSingle - Attributes: map[string]*Attribute{ - "str": {Type: cty.String, Required: true}, - }, - }, - }, - cty.NullVal(cty.Object(map[string]cty.Type{ - "str": cty.String, - })), - }, { &Attribute{ NestedType: &Object{ @@ -212,19 +199,6 @@ func TestAttributeEmptyValue(t *testing.T) { "str": cty.String, })), }, - { - &Attribute{ - NestedType: &Object{ - Nesting: NestingGroup, // functionally equivalent to NestingSingle in a NestedType - Attributes: map[string]*Attribute{ - "str": {Type: cty.String, Required: true}, - }, - }, - }, - cty.NullVal(cty.Object(map[string]cty.Type{ - "str": cty.String, - })), - }, { &Attribute{ NestedType: &Object{ diff --git a/configs/configschema/implied_type.go b/configs/configschema/implied_type.go index a9efa9658..1c8cbcd87 100644 --- a/configs/configschema/implied_type.go +++ b/configs/configschema/implied_type.go @@ -55,13 +55,29 @@ func (o *Object) ImpliedType() cty.Type { attrTys := make(map[string]cty.Type, len(o.Attributes)) for name, attrS := range o.Attributes { - attrTys[name] = attrS.Type + if attrS.NestedType != nil { + attrTys[name] = attrS.NestedType.ImpliedType() + } else { + attrTys[name] = attrS.Type + } + } + optAttrs := listOptionalAttrsFromObject(o) + if len(optAttrs) > 0 { + return cty.ObjectWithOptionalAttrs(attrTys, optAttrs) } - var optAttrs []string - optAttrs = listOptionalAttrsFromObject(o, optAttrs) - - return cty.ObjectWithOptionalAttrs(attrTys, optAttrs) + switch o.Nesting { + case NestingSingle: + return cty.Object(attrTys) + case NestingList: + return cty.List(cty.Object(attrTys)) + case NestingMap: + return cty.Map(cty.Object(attrTys)) + case NestingSet: + return cty.Set(cty.Object(attrTys)) + default: // Should never happen + panic("invalid Nesting") + } } // ContainsSensitive returns true if any of the attributes of the receiving diff --git a/configs/configschema/implied_type_test.go b/configs/configschema/implied_type_test.go index 595d9dd32..70ad9fa56 100644 --- a/configs/configschema/implied_type_test.go +++ b/configs/configschema/implied_type_test.go @@ -132,10 +132,6 @@ func TestObjectImpliedType(t *testing.T) { nil, cty.EmptyObject, }, - "empty": { - &Object{}, - cty.EmptyObject, - }, "attributes": { &Object{ Attributes: map[string]*Attribute{ @@ -167,6 +163,86 @@ func TestObjectImpliedType(t *testing.T) { []string{"optional", "optional_computed"}, ), }, + "nested attributes": { + &Object{ + Attributes: map[string]*Attribute{ + "nested_type": { + NestedType: &Object{ + Attributes: map[string]*Attribute{ + "optional": { + Type: cty.String, + Optional: true, + }, + "required": { + Type: cty.Number, + Required: true, + }, + "computed": { + Type: cty.List(cty.Bool), + Computed: true, + }, + "optional_computed": { + Type: cty.Map(cty.Bool), + Optional: true, + }, + }, + }, + Optional: true, + }, + }, + }, + cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "nested_type": cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "optional": cty.String, + "required": cty.Number, + "computed": cty.List(cty.Bool), + "optional_computed": cty.Map(cty.Bool), + }, []string{"optional", "optional_computed"}), + }, []string{"nested_type"}), + }, + "NestingList": { + &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "foo": {Type: cty.String}, + }, + }, + cty.List(cty.Object(map[string]cty.Type{"foo": cty.String})), + }, + "NestingMap": { + &Object{ + Nesting: NestingMap, + Attributes: map[string]*Attribute{ + "foo": {Type: cty.String}, + }, + }, + cty.Map(cty.Object(map[string]cty.Type{"foo": cty.String})), + }, + "NestingSet": { + &Object{ + Nesting: NestingSet, + Attributes: map[string]*Attribute{ + "foo": {Type: cty.String}, + }, + }, + cty.Set(cty.Object(map[string]cty.Type{"foo": cty.String})), + }, + "deeply nested NestingList": { + &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "foo": { + NestedType: &Object{ + Nesting: NestingList, + Attributes: map[string]*Attribute{ + "bar": {Type: cty.String}, + }, + }, + }, + }, + }, + cty.List(cty.Object(map[string]cty.Type{"foo": cty.List(cty.Object(map[string]cty.Type{"bar": cty.String}))})), + }, } for name, test := range tests {