Skip to content

Commit

Permalink
Fix transform property value to preserve nil arrays and objects (#2655)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
VenelinMartinov authored Nov 22, 2024
1 parent 37482e6 commit 83596bb
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 14 deletions.
46 changes: 32 additions & 14 deletions unstable/propertyvalue/propertyvalue.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,50 @@ 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),
value resource.PropertyValue,
) (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)
Expand Down
39 changes: 39 additions & 0 deletions unstable/propertyvalue/propertyvalue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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())
}

0 comments on commit 83596bb

Please sign in to comment.