From 83596bb12f75a170bfd1265caae18a659a121560 Mon Sep 17 00:00:00 2001
From: VenelinMartinov <venelin.v.martinov@gmail.com>
Date: Fri, 22 Nov 2024 19:17:22 +0000
Subject: [PATCH] Fix transform property value to preserve nil arrays and
 objects (#2655)

The `propertyvalue.Transform` functions had a bug where it would always
mutate nil arrays and maps into empty ones:

```
value=resource.PropertyValue{}
```

would become:
```
value=resource.PropertyValue{
     V:  []resource.PropertyValue{
     },
 }
```

This had to do with typed nil interfaces in go:
https://dave.cheney.net/2017/08/09/typed-nils-in-go-2
The `V` in `resource.PropertyValue` is an interface, so it might be
non-nil `!= nil` but the underlying value is nil.


This change adds a fix and some tests for the problem.
---
 unstable/propertyvalue/propertyvalue.go      | 46 ++++++++++++++------
 unstable/propertyvalue/propertyvalue_test.go | 39 +++++++++++++++++
 2 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/unstable/propertyvalue/propertyvalue.go b/unstable/propertyvalue/propertyvalue.go
index 66aadb128..d04951a48 100644
--- a/unstable/propertyvalue/propertyvalue.go
+++ b/unstable/propertyvalue/propertyvalue.go
@@ -39,6 +39,18 @@ func TransformErr(
 		}, value)
 }
 
+// isNilArray returns true if a property value is not nil but its underlying array is nil.
+// See https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 for more details.
+func isNilArray(value resource.PropertyValue) bool {
+	return value.IsArray() && !value.IsNull() && value.ArrayValue() == nil
+}
+
+// isNilObject returns true if a property value is not nil but its underlying object is nil.
+// See https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 for more details.
+func isNilObject(value resource.PropertyValue) bool {
+	return value.IsObject() && !value.IsNull() && value.ObjectValue() == nil
+}
+
 func TransformPropertyValue(
 	path resource.PropertyPath,
 	transformer func(resource.PropertyPath, resource.PropertyValue) (resource.PropertyValue, error),
@@ -46,25 +58,31 @@ func TransformPropertyValue(
 ) (resource.PropertyValue, error) {
 	switch {
 	case value.IsArray():
-		tvs := []resource.PropertyValue{}
-		for i, v := range value.ArrayValue() {
-			tv, err := TransformPropertyValue(extendPath(path, i), transformer, v)
-			if err != nil {
-				return resource.NewNullProperty(), err
+		// preserve nil arrays
+		if !isNilArray(value) {
+			tvs := []resource.PropertyValue{}
+			for i, v := range value.ArrayValue() {
+				tv, err := TransformPropertyValue(extendPath(path, i), transformer, v)
+				if err != nil {
+					return resource.NewNullProperty(), err
+				}
+				tvs = append(tvs, tv)
 			}
-			tvs = append(tvs, tv)
+			value = resource.NewArrayProperty(tvs)
 		}
-		value = resource.NewArrayProperty(tvs)
 	case value.IsObject():
-		pm := make(resource.PropertyMap)
-		for k, v := range value.ObjectValue() {
-			tv, err := TransformPropertyValue(extendPath(path, string(k)), transformer, v)
-			if err != nil {
-				return resource.NewNullProperty(), err
+		// preserve nil objects
+		if !isNilObject(value) {
+			pm := make(resource.PropertyMap)
+			for k, v := range value.ObjectValue() {
+				tv, err := TransformPropertyValue(extendPath(path, string(k)), transformer, v)
+				if err != nil {
+					return resource.NewNullProperty(), err
+				}
+				pm[k] = tv
 			}
-			pm[k] = tv
+			value = resource.NewObjectProperty(pm)
 		}
-		value = resource.NewObjectProperty(pm)
 	case value.IsOutput():
 		o := value.OutputValue()
 		te, err := TransformPropertyValue(path, transformer, o.Element)
diff --git a/unstable/propertyvalue/propertyvalue_test.go b/unstable/propertyvalue/propertyvalue_test.go
index 62d42d417..291839cc8 100644
--- a/unstable/propertyvalue/propertyvalue_test.go
+++ b/unstable/propertyvalue/propertyvalue_test.go
@@ -17,7 +17,9 @@ package propertyvalue
 import (
 	"testing"
 
+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
 	rtesting "github.com/pulumi/pulumi/sdk/v3/go/common/resource/testing"
+	"github.com/stretchr/testify/require"
 	"pgregory.net/rapid"
 )
 
@@ -30,3 +32,40 @@ func TestRemoveSecrets(t *testing.T) {
 		}
 	})
 }
+
+func TestIsNilArray(t *testing.T) {
+	t.Parallel()
+
+	require.True(t, isNilArray(resource.PropertyValue{V: []resource.PropertyValue(nil)}))
+	require.False(t, isNilArray(resource.PropertyValue{V: []resource.PropertyValue{}}))
+}
+
+func TestIsNilObject(t *testing.T) {
+	t.Parallel()
+
+	require.True(t, isNilObject(resource.PropertyValue{V: resource.PropertyMap(nil)}))
+	require.False(t, isNilObject(resource.PropertyValue{V: resource.PropertyMap{}}))
+}
+
+func TestTransformPreservesNilArrays(t *testing.T) {
+	t.Parallel()
+
+	nilArrayPV := resource.PropertyValue{V: []resource.PropertyValue(nil)}
+	result := Transform(func(value resource.PropertyValue) resource.PropertyValue {
+		return value
+	}, nilArrayPV)
+
+	require.True(t, result.IsArray())
+	require.Nil(t, result.ArrayValue())
+}
+
+func TestTransformPreservesNilObjects(t *testing.T) {
+	t.Parallel()
+
+	nilObjectPV := resource.PropertyValue{V: resource.PropertyMap(nil)}
+	result := Transform(func(value resource.PropertyValue) resource.PropertyValue {
+		return value
+	}, nilObjectPV)
+	require.True(t, result.IsObject())
+	require.Nil(t, result.ObjectValue())
+}