Merge pull request #20178 from hashicorp/jbardin/list-diffs

change copyMissingValues to normalizeNullValues

helper/plugin/grpc_provider.go

@@ -454,7 +454,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 	}
 
 	newStateVal = copyTimeoutValues(newStateVal, stateVal)
-	newStateVal = copyMissingValues(newStateVal, stateVal)
+	newStateVal = normalizeNullValues(newStateVal, stateVal, false)
 
 	newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType())
 	if err != nil {
@@ -508,7 +508,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 
 	// turn the proposed state into a legacy configuration
 	cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, block)
-
 	diff, err := s.provider.SimpleDiff(info, priorState, cfg)
 	if err != nil {
 		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
@@ -549,6 +548,9 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
 		return resp, nil
 	}
+
+	plannedStateVal = normalizeNullValues(plannedStateVal, proposedNewStateVal, true)
+
 	if err != nil {
 		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
 		return resp, nil
@@ -768,7 +770,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		return resp, nil
 	}
 
-	newStateVal = copyMissingValues(newStateVal, plannedStateVal)
+	newStateVal = normalizeNullValues(newStateVal, plannedStateVal, false)
 	newStateVal = copyTimeoutValues(newStateVal, plannedStateVal)
 
 	newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType())
@@ -1130,15 +1132,39 @@ func stripSchema(s *schema.Schema) *schema.Schema {
 	return newSchema
 }
 
-// Zero values and empty containers may be lost during apply. Copy zero values
+// Zero values and empty containers may be interchanged by the apply process.
-// and empty containers from src to dst when they are missing in dst.
+// When there is a discrepency between src and dst value being null or empty,
-// This takes a little more liberty with set types, since we can't correlate
+// prefer the src value. This takes a little more liberty with set types, since
-// modified set values. In the case of sets, if the src set was wholly known we
+// we can't correlate modified set values. In the case of sets, if the src set
-// assume the value was correctly applied and copy that entirely to the new
+// was wholly known we assume the value was correctly applied and copy that
-// value.
+// entirely to the new value.
-func copyMissingValues(dst, src cty.Value) cty.Value {
+// While apply prefers the src value, during plan we prefer dst whenever there
+// is an unknown or a set is involved, since the plan can alter the value
+// however it sees fit. This however means that a CustomizeDiffFunction may not
+// be able to change a null to an empty value or vice versa, but that should be
+// very uncommon nor was it reliable before 0.12 either.
+func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value {
 	ty := dst.Type()
 
+	if !src.IsNull() && !src.IsKnown() {
+		return dst
+	}
+
+	// handle null/empty changes for collections
+	if ty.IsCollectionType() {
+		if src.IsNull() && !dst.IsNull() && dst.IsKnown() {
+			if dst.LengthInt() == 0 {
+				return src
+			}
+		}
+
+		if dst.IsNull() && !src.IsNull() && src.IsKnown() {
+			if src.LengthInt() == 0 {
+				return src
+			}
+		}
+	}
+
 	if src.IsNull() || !src.IsKnown() || !dst.IsKnown() {
 		return dst
 	}
@@ -1159,16 +1185,20 @@ func copyMissingValues(dst, src cty.Value) cty.Value {
 
 			dstVal := dstMap[key]
 			if dstVal == cty.NilVal {
+				if plan && ty.IsMapType() {
+					// let plan shape this map however it wants
+					continue
+				}
 				dstVal = cty.NullVal(ty.ElementType())
 			}
-			dstMap[key] = copyMissingValues(dstVal, v)
+			dstMap[key] = normalizeNullValues(dstVal, v, plan)
 		}
 
 		// 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() {
+			if dst.IsNull() && src.IsWhollyKnown() && !plan {
 				return src
 			}
 			return dst
@@ -1184,21 +1214,39 @@ func copyMissingValues(dst, src cty.Value) cty.Value {
 		// 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() {
+		if src.IsWhollyKnown() && !plan {
 			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
+		// so take the original.
-		// values, since missing empty values may prevent us from correlating
+		if dst.IsNull() {
-		// the correct src and dst indexes.
+			if src.IsWhollyKnown() && !plan {
-		if dst.IsNull() && src.IsWhollyKnown() {
+				return src
-			return src
+			}
+			return dst
+		}
+
+		// if the lengths are identical, then iterate over each element in succession.
+		srcLen := src.LengthInt()
+		dstLen := dst.LengthInt()
+		if srcLen == dstLen && srcLen > 0 {
+			srcs := src.AsValueSlice()
+			dsts := dst.AsValueSlice()
+
+			for i := 0; i < srcLen; i++ {
+				dsts[i] = normalizeNullValues(dsts[i], srcs[i], plan)
+			}
+
+			if ty.IsTupleType() {
+				return cty.TupleVal(dsts)
+			}
+			return cty.ListVal(dsts)
 		}
 
 	case ty.IsPrimitiveType():
-		if dst.IsNull() && src.IsWhollyKnown() {
+		if dst.IsNull() && src.IsWhollyKnown() && !plan {
 			return src
 		}
 	}

helper/plugin/grpc_provider_test.go

@@ -719,9 +719,10 @@ func TestNormalizeFlatmapContainers(t *testing.T) {
 	}
 }
 
-func TestCopyMissingValues(t *testing.T) {
+func TestNormalizeNullValues(t *testing.T) {
 	for i, tc := range []struct {
 		Src, Dst, Expect cty.Value
+		Plan             bool
 	}{
 		{
 			// The known set value is copied over the null set value
@@ -745,6 +746,28 @@ func TestCopyMissingValues(t *testing.T) {
 				}),
 			}),
 		},
+		{
+			// 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,
+				}))),
+			}),
+			// If we're only in a plan, we can't compare sets at all
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{
+					"foo": cty.String,
+				}))),
+			}),
+			Plan: true,
+		},
 		{
 			// The empty map is copied over the null map
 			Src: cty.ObjectVal(map[string]cty.Value{
@@ -758,7 +781,7 @@ func TestCopyMissingValues(t *testing.T) {
 			}),
 		},
 		{
-			// A zerp value primitive is copied over a null primitive
+			// A zero value primitive is copied over a null primitive
 			Src: cty.ObjectVal(map[string]cty.Value{
 				"string": cty.StringVal(""),
 			}),
@@ -769,6 +792,19 @@ func TestCopyMissingValues(t *testing.T) {
 				"string": cty.StringVal(""),
 			}),
 		},
+		{
+			// Plan primitives are kept
+			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.NullVal(cty.String),
+			}),
+			Plan: true,
+		},
 		{
 			// The null map is retained, because the src was unknown
 			Src: cty.ObjectVal(map[string]cty.Value{
@@ -821,9 +857,128 @@ func TestCopyMissingValues(t *testing.T) {
 				}),
 			}),
 		},
+		{
+			// Retain don't re-add unexpected planned values in a 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"),
+				}),
+			}),
+			Plan: true,
+		},
+
+		// a list in an object in a list, going from null to empty
+		{
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.UnknownVal(cty.String),
+						"access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))),
+						"address":       cty.NullVal(cty.String),
+						"name":          cty.StringVal("nic0"),
+					})}),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.StringVal("10.128.0.64"),
+						"access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.StringVal("10.128.0.64"),
+						"access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+		},
+
+		// a list in an object in a list, going from empty to null
+		{
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.UnknownVal(cty.String),
+						"access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})),
+						"address":       cty.NullVal(cty.String),
+						"name":          cty.StringVal("nic0"),
+					})}),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.StringVal("10.128.0.64"),
+						"access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.StringVal("10.128.0.64"),
+						"access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+		},
+		// the empty list should be transferred, but the new unknown show not be overridden
+		{
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.StringVal("10.128.0.64"),
+						"access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})),
+						"address":       cty.NullVal(cty.String),
+						"name":          cty.StringVal("nic0"),
+					})}),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.UnknownVal(cty.String),
+						"access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"network_interface": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"network_ip":    cty.UnknownVal(cty.String),
+						"access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})),
+						"address":       cty.StringVal("address"),
+						"name":          cty.StringVal("nic0"),
+					}),
+				}),
+			}),
+			Plan: true,
+		},
 	} {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
-			got := copyMissingValues(tc.Dst, tc.Src)
+			got := normalizeNullValues(tc.Dst, tc.Src, tc.Plan)
 			if !got.RawEquals(tc.Expect) {
 				t.Fatalf("\nexpected: %#v\ngot:      %#v\n", tc.Expect, got)
 			}