Fix set element order in state

pf/tests/go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
 	github.com/hashicorp/terraform-provider-tls/shim v0.0.0-00010101000000-000000000000
 	github.com/hexops/autogold/v2 v2.2.1
-	github.com/pulumi/providertest v0.1.2
+	github.com/pulumi/providertest v0.1.3
 	github.com/pulumi/pulumi-terraform-bridge/pf v0.0.0
 	github.com/pulumi/pulumi-terraform-bridge/v3 v3.92.0
 	github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests v0.0.0-00010101000000-000000000000

pf/tests/go.sum

@@ -1994,8 +1994,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE=
 github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c=
 github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
 github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
-github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng=
-github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
+github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg=
+github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
 github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns=
 github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM=
 github.com/pulumi/pulumi-yaml v1.10.3 h1:j5cjPiE32ILmjrWnC1cfZ0MWdqCZ8fg9wlaWk7HOtM4=

pkg/tests/go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
 	github.com/hexops/autogold/v2 v2.2.1
 	github.com/hexops/valast v1.4.4
-	github.com/pulumi/providertest v0.1.2
+	github.com/pulumi/providertest v0.1.3
 	github.com/pulumi/pulumi-terraform-bridge/v3 v3.80.0
 	github.com/pulumi/pulumi/sdk v1.14.1
 	github.com/stretchr/testify v1.9.0

pkg/tests/go.sum

@@ -1980,8 +1980,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE=
 github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c=
 github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
 github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
-github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng=
-github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
+github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg=
+github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
 github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns=
 github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM=
 github.com/pulumi/pulumi-yaml v1.10.3 h1:j5cjPiE32ILmjrWnC1cfZ0MWdqCZ8fg9wlaWk7HOtM4=

pkg/tests/schema_pulumi_test.go

@@ -17,7 +17,9 @@ import (
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/pulcheck"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/tfcheck"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge"
+	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info"
 	shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2"
+	"github.com/pulumi/pulumi/sdk/v3/go/auto"
 	"github.com/pulumi/pulumi/sdk/v3/go/auto/optpreview"
 	"github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh"
 	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
@@ -4159,3 +4161,110 @@ func TestMakeTerraformResultNilVsEmptyMap(t *testing.T) {
 		assert.True(t, props["test"].DeepEquals(emptyMap))
 	})
 }
+
+func TestSetElementOrderInState(t *testing.T) {
+	sch := map[string]*schema.Schema{
+		"test": {
+			Type:     schema.TypeSet,
+			Optional: true,
+			Elem: &schema.Schema{
+				Type: schema.TypeString,
+			},
+		},
+	}
+
+	resMap := map[string]*schema.Resource{
+		"prov_test": {
+			Schema: sch,
+		},
+	}
+
+	tfp := &schema.Provider{ResourcesMap: resMap}
+	prov := pulcheck.BridgedProvider(t, "prov", tfp)
+
+	getStringOutputsFromResult := func(res auto.UpResult) []string {
+		outputs := res.Outputs["testOut"].Value.([]interface{})
+		stringOutputs := make([]string, len(outputs))
+		for i, v := range outputs {
+			stringOutputs[i] = v.(string)
+		}
+		return stringOutputs
+	}
+
+	// Test that the order of elements in the state is preserved when created and updated.
+	runTest := func(t *testing.T, prov info.Provider, props1, props2 []string) {
+		props1JSON, err := json.Marshal(props1)
+		require.NoError(t, err)
+		program := `
+        name: test
+        runtime: yaml
+        resources:
+          mainRes:
+            type: prov:index:Test
+            properties:
+                tests: %s
+        outputs:
+          testOut: ${mainRes.tests}`
+
+		program1 := fmt.Sprintf(program, props1JSON)
+
+		props2JSON, err := json.Marshal(props2)
+		require.NoError(t, err)
+		program2 := fmt.Sprintf(program, props2JSON)
+
+		pt := pulcheck.PulCheck(t, prov, program1)
+		res := pt.Up(t)
+
+		require.Equal(t, props1, getStringOutputsFromResult(res))
+
+		pt.WritePulumiYaml(t, program2)
+		res = pt.Up(t)
+		require.Equal(t, props2, getStringOutputsFromResult(res))
+	}
+
+	t.Run("ordered unchanged", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val1", "val2", "val3"},
+			[]string{"val1", "val2", "val3"},
+		)
+	})
+
+	t.Run("unordered unchanged", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val2", "val3", "val1"},
+			[]string{"val2", "val3", "val1"},
+		)
+	})
+
+	t.Run("ordered added", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val1", "val2"},
+			[]string{"val1", "val2", "val3"},
+		)
+	})
+
+	t.Run("unordered added", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val2", "val1"},
+			[]string{"val2", "val1", "val3"},
+		)
+	})
+
+	t.Run("ordered removed", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val1", "val2", "val3"},
+			[]string{"val1", "val2"},
+		)
+	})
+
+	t.Run("unordered removed", func(t *testing.T) {
+		runTest(t, prov,
+			[]string{"val2", "val3", "val1"},
+			[]string{"val2", "val1"},
+		)
+	})
+}
+
+// TODO: Test unknown sets, unknowns elements in sets and nested unknowns in set elements.
+// TODO: Test sets with defaults and nested defaults
+// TODO: Test computed sets, computed elements and nested computed elements

pkg/tfbridge/detailed_diff.go

@@ -393,14 +393,17 @@ func makeDetailedDiffV2(
 	diff shim.InstanceDiff,
 	assets AssetTable,
 	supportsSecrets bool,
+	oldInputs, newInputs resource.PropertyMap,
 ) (map[string]*pulumirpc.PropertyDiff, error) {
 	// We need to compare the new and olds after all transformations have been applied.
 	// ex. state upgrades, implementation-specific normalizations etc.
 	proposedState, err := diff.ProposedState(res, state)
 	if err != nil {
 		return nil, err
 	}
-	props, err := MakeTerraformResult(ctx, prov, proposedState, tfs, ps, assets, supportsSecrets)
+	props, err := makeTerraformResultWithOpts(
+		ctx, prov, proposedState, tfs, ps, assets, supportsSecrets,
+		makeTerraformResultOpts{originalInputs: newInputs})
 	if err != nil {
 		return nil, err
 	}
@@ -409,7 +412,9 @@ func makeDetailedDiffV2(
 	if err != nil {
 		return nil, err
 	}
-	priorProps, err := MakeTerraformResult(ctx, prov, prior, tfs, ps, assets, supportsSecrets)
+	priorProps, err := makeTerraformResultWithOpts(
+		ctx, prov, prior, tfs, ps, assets, supportsSecrets,
+		makeTerraformResultOpts{originalInputs: oldInputs})
 	if err != nil {
 		return nil, err
 	}

pkg/tfbridge/provider.go

@@ -1169,7 +1169,10 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 			changes = pulumirpc.DiffResponse_DIFF_SOME
 		}
 
-		detailedDiff, err = makeDetailedDiffV2(ctx, schema, fields, res.TF, p.tf, state, diff, assets, p.supportsSecrets)
+		detailedDiff, err = makeDetailedDiffV2(
+			ctx, schema, fields, res.TF, p.tf, state, diff, assets, p.supportsSecrets, olds, news,
+		)
+
 		if err != nil {
 			return nil, err
 		}
@@ -1362,7 +1365,10 @@ func (p *Provider) Create(ctx context.Context, req *pulumirpc.CreateRequest) (*p
 	}
 
 	// Create the ID and property maps and return them.
-	props, err = MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
+	props, err = makeTerraformResultWithOpts(
+		ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
+		makeTerraformResultOpts{originalInputs: props},
+	)
 	if err != nil {
 		reasons = append(reasons, errors.Wrapf(err, "converting result for %s", urn).Error())
 	}
@@ -1465,7 +1471,10 @@ func (p *Provider) Read(ctx context.Context, req *pulumirpc.ReadRequest) (*pulum
 	// Store the ID and properties in the output.  The ID *should* be the same as the input ID, but in the case
 	// that the resource no longer exists, we will simply return the empty string and an empty property map.
 	if newstate != nil {
-		props, err := MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
+		props, err := makeTerraformResultWithOpts(
+			ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
+			makeTerraformResultOpts{},
+		)
 		if err != nil {
 			return nil, err
 		}
@@ -1707,7 +1716,10 @@ func (p *Provider) Update(ctx context.Context, req *pulumirpc.UpdateRequest) (*p
 		}
 	}
 
-	props, err := MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
+	props, err := makeTerraformResultWithOpts(
+		ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
+		makeTerraformResultOpts{originalInputs: news},
+	)
 	if err != nil {
 		reasons = append(reasons, errors.Wrapf(err, "converting result for %s", urn).Error())
 	}
@@ -1860,7 +1872,10 @@ func (p *Provider) Invoke(ctx context.Context, req *pulumirpc.InvokeRequest) (*p
 		}
 
 		// Add the special "id" attribute if it wasn't listed in the schema
-		props, err := MakeTerraformResult(ctx, p.tf, invoke, ds.TF.Schema(), ds.Schema.Fields, nil, p.supportsSecrets)
+		props, err := makeTerraformResultWithOpts(
+			ctx, p.tf, invoke, ds.TF.Schema(), ds.Schema.Fields, nil, p.supportsSecrets,
+			makeTerraformResultOpts{originalInputs: args},
+		)
 		if err != nil {
 			return nil, err
 		}

pkg/tfbridge/recover_set_order.go

@@ -0,0 +1,106 @@
+package tfbridge
+
+import (
+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
+
+	shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
+	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk"
+)
+
+// a variant of PropertyPath.Get which works on PropertyMaps
+func getPathFromPropertyMap(
+	path resource.PropertyPath, propertyMap resource.PropertyMap,
+) (resource.PropertyValue, bool) {
+	if len(path) == 0 {
+		return resource.NewNullProperty(), false
+	}
+
+	rootKeyStr, ok := path[0].(string)
+	contract.Assertf(ok && rootKeyStr != "", "root key must be a non-empty string")
+	rootKey := resource.PropertyKey(rootKeyStr)
+	restPath := path[1:]
+
+	if len(restPath) == 0 {
+		return propertyMap[rootKey], true
+	}
+
+	if !propertyMap.HasValue(rootKey) {
+		return resource.NewNullProperty(), false
+	}
+
+	return restPath.Get(propertyMap[rootKey])
+}
+
+// a variant of PropertyPath.Set which works on PropertyMaps
+func setPathInPropertyMap(
+	path resource.PropertyPath, propertyMap resource.PropertyMap, value resource.PropertyValue,
+) bool {
+	if len(path) == 0 {
+		return false
+	}
+
+	rootKeyStr, ok := path[0].(string)
+	contract.Assertf(ok && rootKeyStr != "", "root key must be a non-empty string")
+	rootKey := resource.PropertyKey(rootKeyStr)
+	restPath := path[1:]
+
+	if len(restPath) == 0 {
+		propertyMap[rootKey] = value
+		return true
+	}
+
+	if !propertyMap.HasValue(rootKey) {
+		return false
+	}
+
+	return restPath.Set(propertyMap[rootKey], value)
+}
+
+// Sets in Terraform are unordered but are represented as ordered lists in the Pulumi world, and detailed diff must
+// return indices into this list to indicate the elements that have changed.
+func recoverSetOrderSingle(
+	target resource.PropertyMap, origin resource.PropertyMap, setPath resource.PropertyPath,
+) {
+	targetSet, ok := getPathFromPropertyMap(setPath, target)
+	if !ok || !targetSet.IsArray() || targetSet.IsComputed() {
+		return
+	}
+	originSet, ok := getPathFromPropertyMap(setPath, origin)
+	if !ok || !originSet.IsArray() {
+		return
+	}
+
+	// No need to recover the order if the set is empty or has only one element
+	if len(targetSet.ArrayValue()) == 0 || len(originSet.ArrayValue()) == 0 || len(targetSet.ArrayValue()) == 1 {
+		return
+	}
+
+	ok = setPathInPropertyMap(setPath, target, originSet)
+	contract.Assertf(ok, "failed to set the origin set but it should exist!")
+}
+
+// TODO: Should we cache this?
+func getSetPaths(schema shim.SchemaMap) []SchemaPath {
+	paths := make([]SchemaPath, 0)
+
+	walk.VisitSchemaMap(schema, func(path SchemaPath, schema shim.Schema) {
+		if schema.Type() == shim.TypeSet {
+			paths = append(paths, path)
+		}
+	})
+
+	return paths
+}
+
+func recoverSetOrder(
+	target resource.PropertyMap, origin resource.PropertyMap, tfs shim.SchemaMap, ps map[string]*SchemaInfo,
+) {
+	setPaths := getSetPaths(tfs)
+	for _, setPath := range setPaths {
+		propertyPath := SchemaPathToPropertyPath(setPath, tfs, ps)
+		recoverSetOrderSingle(target, origin, propertyPath)
+	}
+}
+
+// TODO: unit tests