From 6f54bfaa7c8598adaf02b13c7765b5702d49c8cf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Jan 2019 13:47:04 -0500 Subject: [PATCH 1/6] send config during apply --- plugin/grpc_provider.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugin/grpc_provider.go b/plugin/grpc_provider.go index 6bab719c8..373e07394 100644 --- a/plugin/grpc_provider.go +++ b/plugin/grpc_provider.go @@ -420,11 +420,17 @@ func (p *GRPCProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques resp.Diagnostics = resp.Diagnostics.Append(err) return resp } + configMP, err := msgpack.Marshal(r.Config, resSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp + } protoReq := &proto.ApplyResourceChange_Request{ TypeName: r.TypeName, PriorState: &proto.DynamicValue{Msgpack: priorMP}, PlannedState: &proto.DynamicValue{Msgpack: plannedMP}, + Config: &proto.DynamicValue{Msgpack: configMP}, PlannedPrivate: r.PlannedPrivate, } From 3677522a283f662ac76568f179f29b5e999d4326 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 5 Jan 2019 15:10:30 -0500 Subject: [PATCH 2/6] insert empty objects into config from empty blocks When creating a legacy config from a cty.Value, empty nested blocks should corespond to empty objects in the config. --- config/hcl2shim/values.go | 10 ---------- config/hcl2shim/values_test.go | 18 ++++++++++++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/config/hcl2shim/values.go b/config/hcl2shim/values.go index 2c5b2907e..000ad7ba8 100644 --- a/config/hcl2shim/values.go +++ b/config/hcl2shim/values.go @@ -80,11 +80,6 @@ func ConfigValueFromHCL2Block(v cty.Value, schema *configschema.Block) map[strin case configschema.NestingList, configschema.NestingSet: l := bv.LengthInt() - if l == 0 { - // skip empty collections to better mimic how HCL1 would behave - continue - } - elems := make([]interface{}, 0, l) for it := bv.ElementIterator(); it.Next(); { _, ev := it.Element() @@ -97,11 +92,6 @@ func ConfigValueFromHCL2Block(v cty.Value, schema *configschema.Block) map[strin ret[name] = elems case configschema.NestingMap: - if bv.LengthInt() == 0 { - // skip empty collections to better mimic how HCL1 would behave - continue - } - elems := make(map[string]interface{}) for it := bv.ElementIterator(); it.Next(); { ek, ev := it.Element() diff --git a/config/hcl2shim/values_test.go b/config/hcl2shim/values_test.go index 7c3011da0..805155f0b 100644 --- a/config/hcl2shim/values_test.go +++ b/config/hcl2shim/values_test.go @@ -151,7 +151,7 @@ func TestConfigValueFromHCL2Block(t *testing.T) { }, { cty.ObjectVal(map[string]cty.Value{ - "address": cty.ListValEmpty(cty.EmptyObject), // should be omitted altogether in result + "address": cty.ListValEmpty(cty.EmptyObject), }), &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ @@ -161,7 +161,9 @@ func TestConfigValueFromHCL2Block(t *testing.T) { }, }, }, - map[string]interface{}{}, + map[string]interface{}{ + "address": []interface{}{}, + }, }, { cty.ObjectVal(map[string]cty.Value{ @@ -193,7 +195,9 @@ func TestConfigValueFromHCL2Block(t *testing.T) { }, }, }, - map[string]interface{}{}, + map[string]interface{}{ + "address": []interface{}{}, + }, }, { cty.ObjectVal(map[string]cty.Value{ @@ -225,7 +229,9 @@ func TestConfigValueFromHCL2Block(t *testing.T) { }, }, }, - map[string]interface{}{}, + map[string]interface{}{ + "address": map[string]interface{}{}, + }, }, { cty.NullVal(cty.EmptyObject), @@ -234,8 +240,8 @@ func TestConfigValueFromHCL2Block(t *testing.T) { }, } - for _, test := range tests { - t.Run(fmt.Sprintf("%#v", test.Input), func(t *testing.T) { + for i, test := range tests { + t.Run(fmt.Sprintf("%d-%#v", i, test.Input), func(t *testing.T) { got := ConfigValueFromHCL2Block(test.Input, test.Schema) if !reflect.DeepEqual(got, test.Want) { t.Errorf("wrong result\ninput: %#v\ngot: %#v\nwant: %#v", test.Input, got, test.Want) From f3c80b4765b9f56c9b2c05c699046fa32c428bb6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Jan 2019 11:51:21 -0500 Subject: [PATCH 3/6] add zero values to sets with a flatmap count of 1 If a flatmap value has a count of 1 and no other attributes, it usually indicates the equivalent configuration of an empty (or default value) set block. Treat this as containing a single zero value object and insert that into the set. --- config/hcl2shim/flatmap.go | 32 +++++++++++++++++++---- config/hcl2shim/flatmap_test.go | 46 +++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/config/hcl2shim/flatmap.go b/config/hcl2shim/flatmap.go index bcecb30df..bb4228d98 100644 --- a/config/hcl2shim/flatmap.go +++ b/config/hcl2shim/flatmap.go @@ -347,10 +347,8 @@ func hcl2ValueFromFlatmapSet(m map[string]string, prefix string, ty cty.Type) (c return cty.UnknownVal(ty), nil } - // We actually don't really care about the "count" of a set for our - // purposes here, but we do need to check if it _exists_ in order to - // recognize the difference between null (not set at all) and empty. - if strCount, exists := m[prefix+"#"]; !exists { + strCount, exists := m[prefix+"#"] + if !exists { return cty.NullVal(ty), nil } else if strCount == UnknownVariableValue { return cty.UnknownVal(ty), nil @@ -394,7 +392,31 @@ func hcl2ValueFromFlatmapSet(m map[string]string, prefix string, ty cty.Type) (c vals = append(vals, val) } - if len(vals) == 0 { + if len(vals) == 0 && strCount == "1" { + // An empty set wouldn't be represented in the flatmap, so this must be + // a single empty object since the count is actually 1. + // Add an appropriately typed null value to the set. + var val cty.Value + switch { + case ety.IsMapType(): + val = cty.MapValEmpty(ety) + case ety.IsListType(): + val = cty.ListValEmpty(ety) + case ety.IsSetType(): + val = cty.SetValEmpty(ety) + case ety.IsObjectType(): + // TODO: cty.ObjectValEmpty + objectMap := map[string]cty.Value{} + for attr, ty := range ety.AttributeTypes() { + objectMap[attr] = cty.NullVal(ty) + } + val = cty.ObjectVal(objectMap) + default: + val = cty.NullVal(ety) + } + vals = append(vals, val) + + } else if len(vals) == 0 { return cty.SetValEmpty(ety), nil } diff --git a/config/hcl2shim/flatmap_test.go b/config/hcl2shim/flatmap_test.go index 07f93ac26..e2ae8a70f 100644 --- a/config/hcl2shim/flatmap_test.go +++ b/config/hcl2shim/flatmap_test.go @@ -679,10 +679,52 @@ func TestHCL2ValueFromFlatmap(t *testing.T) { }), }), }, + { + Flatmap: map[string]string{ + "foo.#": "1", + }, + Type: cty.Object(map[string]cty.Type{ + "foo": cty.Set(cty.Object(map[string]cty.Type{ + "bar": cty.String, + })), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "bar": cty.NullVal(cty.String), + }), + }), + }), + }, + { + Flatmap: map[string]string{ + "multi.#": "1", + "multi.2.set.#": "1", + "multi.2.set.3.required": "val", + }, + Type: cty.Object(map[string]cty.Type{ + "multi": cty.Set(cty.Object(map[string]cty.Type{ + "set": cty.Set(cty.Object(map[string]cty.Type{ + "required": cty.String, + })), + })), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "multi": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "required": cty.StringVal("val"), + }), + }), + }), + }), + }), + }, } - for _, test := range tests { - t.Run(fmt.Sprintf("%#v as %#v", test.Flatmap, test.Type), func(t *testing.T) { + for i, test := range tests { + t.Run(fmt.Sprintf("%d %#v as %#v", i, test.Flatmap, test.Type), func(t *testing.T) { got, err := HCL2ValueFromFlatmap(test.Flatmap, test.Type) if test.WantErr != "" { From 8300d65539b3db2277e26c97666f0db283e36dad Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Jan 2019 13:43:52 -0500 Subject: [PATCH 4/6] don't strip sets with count 1 when normalizing normalizeFlatmapContainers should retain sets with a count of 1, and convert sets with a count of 0 if they were 1 before the Apply step. --- helper/plugin/grpc_provider.go | 73 ++++++++++++++++++++++++----- helper/plugin/grpc_provider_test.go | 10 ++-- 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 0c95539f7..c3ba2d3cf 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -438,8 +438,6 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // helper/schema should always copy the ID over, but do it again just to be safe newInstanceState.Attributes["id"] = newInstanceState.ID - newInstanceState.Attributes = normalizeFlatmapContainers(instanceState.Attributes, newInstanceState.Attributes) - newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Attributes, block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -488,7 +486,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - priorPrivate := make(map[string]interface{}) if len(req.PriorPrivate) > 0 { if err := json.Unmarshal(req.PriorPrivate, &priorPrivate); err != nil { @@ -536,7 +533,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // now we need to apply the diff to the prior state, so get the planned state plannedAttrs, err := diff.Apply(priorState.Attributes, block) - plannedAttrs = normalizeFlatmapContainers(priorState.Attributes, plannedAttrs) + plannedAttrs = normalizeFlatmapContainers(priorState.Attributes, plannedAttrs, false) plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, block.ImpliedType()) if err != nil { @@ -683,6 +680,13 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } } + // strip out non-diffs + for k, v := range diff.Attributes { + if v.New == v.Old && !v.NewComputed && !v.NewRemoved { + delete(diff.Attributes, k) + } + } + // add NewExtra Fields that may have been stored in the private data if newExtra := private[newExtraKey]; newExtra != nil { for k, v := range newExtra.(map[string]interface{}) { @@ -718,7 +722,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A // here we use the planned state to check for unknown/zero containers values // when normalizing the flatmap. plannedState := hcl2shim.FlatmapValueFromHCL2(plannedStateVal) - newInstanceState.Attributes = normalizeFlatmapContainers(plannedState, newInstanceState.Attributes) + newInstanceState.Attributes = normalizeFlatmapContainers(plannedState, newInstanceState.Attributes, true) } newStateVal := cty.NullVal(block.ImpliedType()) @@ -898,24 +902,62 @@ func pathToAttributePath(path cty.Path) *proto.AttributePath { // set of flatmapped attributes. The prior value is used to determine if there // could be zero-length flatmap containers which we need to preserve. This // allows a provider to set an empty computed container in the state without -// creating perpetual diff. -func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string) map[string]string { - keyRx := regexp.MustCompile(`.\.[%#]$`) +// creating perpetual diff. This can differ slightly between plan and apply, so +// the apply flag is passed when called from ApplyResourceChange. +func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string, apply bool) map[string]string { + isCount := regexp.MustCompile(`.\.[%#]$`).MatchString - // while we can't determine if the value was actually computed here, we will + // While we can't determine if the value was actually computed here, we will // trust that our shims stored and retrieved a zero-value container // correctly. zeros := map[string]bool{} + // Empty blocks have a count of 1 with no other attributes. Just record all + // "1"s here to override 0-length blocks when setting the count below. + ones := map[string]bool{} for k, v := range prior { - if keyRx.MatchString(k) && (v == "0" || v == hcl2shim.UnknownVariableValue) { + if isCount(k) && (v == "0" || v == hcl2shim.UnknownVariableValue) { zeros[k] = true } + + // fixup any 1->0 conversions that happened during Apply + if apply && isCount(k) && v == "1" && attrs[k] == "0" { + attrs[k] = "1" + } + } + + for k, v := range attrs { + // store any "1" values, since if the length was 1 and there are no + // items, it was probably an empty set block. Hopefully checking for a 1 + // value with no items is sufficient, without cross-referencing the + // schema. + if isCount(k) && v == "1" { + ones[k] = true + } + } + + // The "ones" were stored to look for sets with an empty value, so we need + // to verify that we only store ones with no attrs. + expectedEmptySets := map[string]bool{} + for one := range ones { + prefix := one[:len(one)-1] + + found := 0 + for k := range attrs { + // since this can be recursive, we check that the attrs isn't also a #. + if strings.HasPrefix(k, prefix) && !isCount(k) { + found++ + } + } + + if found == 0 { + expectedEmptySets[one] = true + } } // find container keys var keys []string for k, v := range attrs { - if !keyRx.MatchString(k) { + if !isCount(k) { continue } @@ -967,6 +1009,9 @@ func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string // must have set the computed value to an empty container, and we // need to leave it in the flatmap. attrs[k] = "0" + case len(indexes) == 0 && ones[k]: + // We need to retain any empty blocks that had a 1 count with no attributes. + attrs[k] = "1" case len(indexes) > 0: attrs[k] = strconv.Itoa(len(indexes)) default: @@ -974,6 +1019,12 @@ func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string } } + for k := range expectedEmptySets { + if _, ok := attrs[k]; !ok { + attrs[k] = "1" + } + } + return attrs } diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index 8f807c51d..1e1d6f339 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -673,7 +673,7 @@ func TestNormalizeFlatmapContainers(t *testing.T) { }{ { attrs: map[string]string{"id": "1", "multi.2.set.#": "1", "multi.1.set.#": "0", "single.#": "0"}, - expect: map[string]string{"id": "1"}, + expect: map[string]string{"id": "1", "multi.2.set.#": "1"}, }, { attrs: map[string]string{"id": "1", "multi.2.set.#": "2", "multi.2.set.1.foo": "bar", "multi.1.set.#": "0", "single.#": "0"}, @@ -681,11 +681,15 @@ func TestNormalizeFlatmapContainers(t *testing.T) { }, { attrs: map[string]string{"id": "78629a0f5f3f164f", "multi.#": "1"}, + expect: map[string]string{"id": "78629a0f5f3f164f", "multi.#": "1"}, + }, + { + attrs: map[string]string{"id": "78629a0f5f3f164f", "multi.#": "0"}, expect: map[string]string{"id": "78629a0f5f3f164f"}, }, { - attrs: map[string]string{"multi.529860700.set.#": "1", "multi.#": "1", "id": "78629a0f5f3f164f"}, - expect: map[string]string{"id": "78629a0f5f3f164f"}, + attrs: map[string]string{"multi.529860700.set.#": "0", "multi.#": "1", "id": "78629a0f5f3f164f"}, + expect: map[string]string{"id": "78629a0f5f3f164f", "multi.#": "1"}, }, { attrs: map[string]string{"set.2.required": "bar", "set.2.list.#": "1", "set.2.list.0": "x", "set.1.list.#": "0", "set.#": "2"}, From b55ec74c27b3e9030e8c6598f91e190818b11217 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Jan 2019 13:50:23 -0500 Subject: [PATCH 5/6] add copyMissingValues for normalizing shimmed Vals Zero values and empty containers can be lost during the shimming process, and during the provider's Apply step. If we have known zero value containers and primitives in the source, which appear as null values in the destination, we copy over the zero value. Sets (and lists to an extent) are more difficult, since there before and after indexes may not correlate. In that case we take the entire container if it's wholly known, expecting the provider to have correctly handled the value. --- helper/plugin/grpc_provider.go | 87 ++++++++++++++++++ helper/plugin/grpc_provider_test.go | 135 +++++++++++++++++++++++++++- 2 files changed, 221 insertions(+), 1 deletion(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index c3ba2d3cf..c9ffa1a23 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -541,6 +541,8 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl return resp, nil } + plannedStateVal = copyMissingValues(plannedStateVal, proposedNewStateVal) + plannedStateVal, err = block.CoerceValue(plannedStateVal) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -737,6 +739,8 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } } + newStateVal = copyMissingValues(newStateVal, plannedStateVal) + newStateVal = copyTimeoutValues(newStateVal, plannedStateVal) newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType()) @@ -1094,3 +1098,86 @@ func stripSchema(s *schema.Schema) *schema.Schema { return newSchema } + +// Zero values and empty containers may be lost during apply. Copy zero values +// and empty containers from src to dst when they are missing in dst. +// This takes a little more liberty with set types, since we can't correlate +// modified set values. In the case of sets, if the src set was wholly known we +// assume the value was correctly applied and copy that entirely to the new +// value. +func copyMissingValues(dst, src cty.Value) cty.Value { + ty := dst.Type() + + // In this case the provider set an empty string which was lost in + // conversion. Since src is unknown, there must have been a corresponding + // value set. + if ty == cty.String && dst.IsNull() && !src.IsKnown() { + return cty.StringVal("") + } + + if src.IsNull() || !src.IsKnown() || !dst.IsKnown() { + return dst + } + + switch { + case ty.IsMapType(), ty.IsObjectType(): + var dstMap map[string]cty.Value + if dst.IsNull() { + dstMap = map[string]cty.Value{} + } else { + dstMap = dst.AsValueMap() + } + + ei := src.ElementIterator() + for ei.Next() { + k, v := ei.Element() + key := k.AsString() + + dstVal := dstMap[key] + if dstVal == cty.NilVal { + dstVal = cty.NullVal(ty.ElementType()) + } + dstMap[key] = copyMissingValues(dstVal, v) + } + + // you can't call MapVal/ObjectVal with empty maps, but nothing was + // copied in anyway. If the dst is nil, and the src is known, assume the + // src is correct. + if len(dstMap) == 0 { + if dst.IsNull() && src.IsWhollyKnown() { + return src + } + return dst + } + + if ty.IsMapType() { + return cty.MapVal(dstMap) + } + + return cty.ObjectVal(dstMap) + + case ty.IsSetType(): + // If the original was wholly known, then we expect that is what the + // provider applied. The apply process loses too much information to + // reliably re-create the set. + if src.IsWhollyKnown() { + return src + } + + case ty.IsListType(), ty.IsTupleType(): + // If the dst is nil, and the src is known, then we lost an empty value + // so take the original. This doesn't attempt to descend into the list + // values, since missing empty values may prevent us from correlating + // the correct src and dst indexes. + if dst.IsNull() && src.IsWhollyKnown() { + return src + } + + case ty.IsPrimitiveType(): + if dst.IsNull() && src.IsWhollyKnown() { + return src + } + } + + return dst +} diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index 1e1d6f339..4df64cdac 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -711,10 +711,143 @@ func TestNormalizeFlatmapContainers(t *testing.T) { }, } { t.Run(strconv.Itoa(i), func(t *testing.T) { - got := normalizeFlatmapContainers(tc.prior, tc.attrs) + got := normalizeFlatmapContainers(tc.prior, tc.attrs, false) if !reflect.DeepEqual(tc.expect, got) { t.Fatalf("expected:\n%#v\ngot:\n%#v\n", tc.expect, got) } }) } } + +func TestCopyMissingValues(t *testing.T) { + for i, tc := range []struct { + Src, Dst, Expect cty.Value + }{ + { + // The known set value is copied over the null set value + Src: cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "foo": cty.String, + }))), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + }), + }), + }, + { + // The empty map is copied over the null map + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapValEmpty(cty.String), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.NullVal(cty.Map(cty.String)), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapValEmpty(cty.String), + }), + }, + { + // A zerp value primitive is copied over a null primitive + Src: cty.ObjectVal(map[string]cty.Value{ + "string": cty.StringVal(""), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "string": cty.NullVal(cty.String), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "string": cty.StringVal(""), + }), + }, + { + // The null map is retained, because the src was unknown + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.UnknownVal(cty.Map(cty.String)), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.NullVal(cty.Map(cty.String)), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.NullVal(cty.Map(cty.String)), + }), + }, + { + // the nul set is retained, because the src set contains an unknown value + Src: cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), + }), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "foo": cty.String, + }))), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "foo": cty.String, + }))), + }), + }, + { + // Retain the zero value within the map + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + }, + { + // Recover the lost unknown key, assuming it was set to an empty + // string and lost. + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.UnknownVal(cty.String), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + }, + } { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + got := copyMissingValues(tc.Dst, tc.Src) + if !got.RawEquals(tc.Expect) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.Expect, got) + } + }) + } +} From 7455bf2a55d69081a3e25308d397d2025dd47dea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Jan 2019 14:18:53 -0500 Subject: [PATCH 6/6] provider tests for empty values Add tests to make limited use of empty container values and empty strings. --- builtin/providers/test/resource_nested_set.go | 26 ++- .../test/resource_nested_set_test.go | 160 ++++++++++++++++++ .../providers/test/resource_nested_test.go | 1 + builtin/providers/test/resource_test.go | 20 +++ 4 files changed, 206 insertions(+), 1 deletion(-) diff --git a/builtin/providers/test/resource_nested_set.go b/builtin/providers/test/resource_nested_set.go index b808b7c98..81d7ab0f5 100644 --- a/builtin/providers/test/resource_nested_set.go +++ b/builtin/providers/test/resource_nested_set.go @@ -28,6 +28,19 @@ func testResourceNestedSet() *schema.Resource { Optional: true, ForceNew: true, }, + "type_list": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "value": { + Type: schema.TypeString, + ForceNew: true, + Optional: true, + }, + }, + }, + }, "single": { Type: schema.TypeSet, Optional: true, @@ -98,7 +111,6 @@ func testResourceNestedSet() *schema.Resource { Type: schema.TypeString, Required: true, }, - "list": { Type: schema.TypeList, Optional: true, @@ -106,6 +118,18 @@ func testResourceNestedSet() *schema.Resource { Type: schema.TypeString, }, }, + "list_block": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "unused": { + Type: schema.TypeString, + Optional: true, + }, + }, + }, + }, }, }, }, diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index f85e2a190..99e7651ce 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -3,6 +3,7 @@ package test import ( "errors" "fmt" + "regexp" "strings" "testing" @@ -56,6 +57,118 @@ resource "test_resource_nested_set" "foo" { }) } +// the empty type_list must be passed to the provider with 1 nil element +func TestResourceNestedSet_emptyBlock(t *testing.T) { + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + for k, v := range res.Primary.Attributes { + if strings.HasPrefix(k, "type_list") && v != "1" { + return fmt.Errorf("unexpected set value: %s:%s", k, v) + } + } + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + type_list { + } +} + `), + Check: checkFunc, + }, + }, + }) +} + +func TestResourceNestedSet_emptyNestedListBlock(t *testing.T) { + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + found := false + for k, v := range res.Primary.Attributes { + if !regexp.MustCompile(`^with_list\.\d+\.list_block\.`).MatchString(k) { + continue + } + found = true + + if strings.HasSuffix(k, ".#") { + if v != "1" { + return fmt.Errorf("expected block with no objects: got %s:%s", k, v) + } + continue + } + + // there should be no other attribute values for an empty block + return fmt.Errorf("unexpected attribute: %s:%s", k, v) + } + if !found { + return fmt.Errorf("with_list.X.list_block not found") + } + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + with_list { + required = "ok" + list_block { + } + } +} + `), + Check: checkFunc, + }, + }, + }) +} +func TestResourceNestedSet_emptyNestedList(t *testing.T) { + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + found := false + for k, v := range res.Primary.Attributes { + if regexp.MustCompile(`^with_list\.\d+\.list\.#$`).MatchString(k) { + found = true + if v != "0" { + return fmt.Errorf("expected empty list: %s, got %s", k, v) + } + break + } + } + if !found { + return fmt.Errorf("with_list.X.nested_list not found") + } + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + with_list { + required = "ok" + list = [] + } +} + `), + Check: checkFunc, + }, + }, + }) +} + func TestResourceNestedSet_addRemove(t *testing.T) { var id string checkFunc := func(s *terraform.State) error { @@ -339,3 +452,50 @@ resource "test_resource_nested_set" "foo" { }, }) } + +// This is the same as forceNewEmptyString, but we start with the empty value, +// instead of changing it. +func TestResourceNestedSet_nestedSetEmptyString(t *testing.T) { + checkFunc := func(s *terraform.State) error { + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "" + } + } +} + `), + Check: checkFunc, + }, + }, + }) +} + +func TestResourceNestedSet_emptySet(t *testing.T) { + checkFunc := func(s *terraform.State) error { + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + } +} + `), + Check: checkFunc, + }, + }, + }) +} diff --git a/builtin/providers/test/resource_nested_test.go b/builtin/providers/test/resource_nested_test.go index d1a9c1b73..9237b52fc 100644 --- a/builtin/providers/test/resource_nested_test.go +++ b/builtin/providers/test/resource_nested_test.go @@ -142,6 +142,7 @@ resource "test_resource_nested" "foo" { "nested.0.nested_again.0.string": "a", "nested.1.string": "", "nested.1.optional": "false", + "nested.1.nested_again.#": "0", } delete(got, "id") // it's random, so not useful for testing diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index b9eb06455..12bf568fa 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -521,3 +521,23 @@ resource "test_resource" "two" { }, }) } + +func TestResource_emptyMapValue(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "ok" + required_map = { + a = "a" + b = "" + } +} + `), + }, + }, + }) +}