From 0d80a745390e67ac675403f52c68e60e1ec26fd1 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Thu, 15 Jul 2021 13:00:07 -0400 Subject: [PATCH] configs/configschema: fix missing "computed" attributes from NestedObject's ImpliedType (#29172) * configs/configschema: fix missing "computed" attributes from NestedObject's ImpliedType listOptionalAttrsFromObject was not including "computed" attributes in the list of optional object attributes. This is now fixed. I've also added some tests and fixed some panics and otherwise bad behavior when bad input is given. One natable change is in ImpliedType, which was panicking on an invalid nesting mode. The comment expressly states that it will return a result even when the schema is inconsistent, so I removed the panic and instead return an empty object. --- internal/configs/configschema/decoder_spec.go | 10 ++- .../configs/configschema/decoder_spec_test.go | 42 ++++++++++ internal/configs/configschema/implied_type.go | 2 +- .../configs/configschema/implied_type_test.go | 81 +++++++++++-------- internal/plans/objchange/objchange_test.go | 2 +- 5 files changed, 100 insertions(+), 37 deletions(-) diff --git a/internal/configs/configschema/decoder_spec.go b/internal/configs/configschema/decoder_spec.go index 333274503..19bea4917 100644 --- a/internal/configs/configschema/decoder_spec.go +++ b/internal/configs/configschema/decoder_spec.go @@ -211,9 +211,15 @@ func (a *Attribute) decoderSpec(name string) hcldec.Spec { // 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 + ret := make([]string, 0) + + // This is unlikely to happen outside of tests. + if o == nil { + return ret + } + for name, attr := range o.Attributes { - if attr.Optional == true { + if attr.Optional || attr.Computed { ret = append(ret, name) } } diff --git a/internal/configs/configschema/decoder_spec_test.go b/internal/configs/configschema/decoder_spec_test.go index a6571eaa6..12fdee761 100644 --- a/internal/configs/configschema/decoder_spec_test.go +++ b/internal/configs/configschema/decoder_spec_test.go @@ -1,10 +1,12 @@ package configschema import ( + "sort" "testing" "github.com/apparentlymart/go-dump/dump" "github.com/davecgh/go-spew/spew" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hcldec" @@ -885,3 +887,43 @@ func TestAttributeDecoderSpec_panic(t *testing.T) { attrS.decoderSpec("attr") t.Errorf("expected panic") } + +func TestListOptionalAttrsFromObject(t *testing.T) { + tests := []struct { + input *Object + want []string + }{ + { + nil, + []string{}, + }, + { + &Object{}, + []string{}, + }, + { + &Object{ + Nesting: NestingSingle, + 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, Computed: true}, + }, + }, + []string{"optional", "computed", "optional_computed"}, + }, + } + + for _, test := range tests { + got := listOptionalAttrsFromObject(test.input) + + // order is irrelevant + sort.Strings(got) + sort.Strings(test.want) + + if diff := cmp.Diff(got, test.want); diff != "" { + t.Fatalf("wrong result: %s\n", diff) + } + } +} diff --git a/internal/configs/configschema/implied_type.go b/internal/configs/configschema/implied_type.go index 4d4a042c3..58b995110 100644 --- a/internal/configs/configschema/implied_type.go +++ b/internal/configs/configschema/implied_type.go @@ -79,7 +79,7 @@ func (o *Object) ImpliedType() cty.Type { case NestingSet: return cty.Set(ret) default: // Should never happen - panic("invalid Nesting") + return cty.EmptyObject } } diff --git a/internal/configs/configschema/implied_type_test.go b/internal/configs/configschema/implied_type_test.go index 7cd0f7309..d36239615 100644 --- a/internal/configs/configschema/implied_type_test.go +++ b/internal/configs/configschema/implied_type_test.go @@ -37,6 +37,7 @@ func TestBlockImpliedType(t *testing.T) { "optional_computed": { Type: cty.Map(cty.Bool), Optional: true, + Computed: true, }, }, }, @@ -132,26 +133,18 @@ func TestObjectImpliedType(t *testing.T) { nil, cty.EmptyObject, }, + "empty": { + &Object{}, + cty.EmptyObject, + }, "attributes": { &Object{ Nesting: NestingSingle, 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": {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, Computed: true}, }, }, cty.ObjectWithOptionalAttrs( @@ -161,7 +154,7 @@ func TestObjectImpliedType(t *testing.T) { "computed": cty.List(cty.Bool), "optional_computed": cty.Map(cty.Bool), }, - []string{"optional", "optional_computed"}, + []string{"optional", "computed", "optional_computed"}, ), }, "nested attributes": { @@ -172,21 +165,42 @@ func TestObjectImpliedType(t *testing.T) { NestedType: &Object{ Nesting: NestingSingle, 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": {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, Computed: 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", "computed", "optional_computed"}), + }, []string{"nested_type"}), + }, + "nested object-type attributes": { + &Object{ + Nesting: NestingSingle, + Attributes: map[string]*Attribute{ + "nested_type": { + NestedType: &Object{ + Nesting: NestingSingle, + 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, Computed: true}, + "object": { + Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "optional": cty.String, + "required": cty.Number, + }, []string{"optional"}), }, }, }, @@ -200,7 +214,8 @@ func TestObjectImpliedType(t *testing.T) { "required": cty.Number, "computed": cty.List(cty.Bool), "optional_computed": cty.Map(cty.Bool), - }, []string{"optional", "optional_computed"}), + "object": cty.ObjectWithOptionalAttrs(map[string]cty.Type{"optional": cty.String, "required": cty.Number}, []string{"optional"}), + }, []string{"optional", "computed", "optional_computed"}), }, []string{"nested_type"}), }, "NestingList": { diff --git a/internal/plans/objchange/objchange_test.go b/internal/plans/objchange/objchange_test.go index 3e9cc2055..8221f99bd 100644 --- a/internal/plans/objchange/objchange_test.go +++ b/internal/plans/objchange/objchange_test.go @@ -1457,7 +1457,7 @@ func TestProposedNew(t *testing.T) { "computed": cty.String, "optional_computed": cty.String, "required": cty.String, - }, []string{"optional", "optional_computed"}), + }, []string{"computed", "optional", "optional_computed"}), }))), }), },