From b55ec74c27b3e9030e8c6598f91e190818b11217 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Jan 2019 13:50:23 -0500 Subject: [PATCH] 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) + } + }) + } +}