Fix diffing assets and archives (#3115)

This PR fixes the diffing of resource properties which contain Pulumi
Assets or Archives. We were not correctly handling the assets coming
from the old properties both for with RawStateDeltas enabled and
without. Instead we were using the new property assets in place of the
old ones and that produced no diff for these properties.

This fixes both:
- Without raw state deltas, the asset table is parsed as part of
transforming the property values to the TF domain - we can use these in
the diff algorithm.
- With raw state deltas, we now add a recursive pass over the old
property values, which produces an asset table.

fixes #3102

Author: VenelinMartinov

pkg/tests/schema_pulumi_test.go

@@ -836,6 +836,8 @@ func TestAssetDiff(t *testing.T) {
 
 		prev := pt.Preview(t, optpreview.Diff())
 
+		require.Contains(t, prev.StdOut, "- testPath: asset(file:2cf24db)")
+		require.Contains(t, prev.StdOut, "+ testPath: asset(file:486ea46)")
 		require.Contains(t, prev.StdOut, "~ 1 to update")
 
 		pt.Up(t)
@@ -875,7 +877,10 @@ func TestAssetDiff(t *testing.T) {
     ~ prov:index/test:Test: (update)
         [id=newid]
         [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
-        testPath  : asset(text:2cf24db) {
+      - testPath: asset(text:2cf24db) {
+            <contents elided>
+        }
+      + testPath: asset(text:486ea46) {
             <contents elided>
         }
 Resources:
@@ -950,6 +955,7 @@ func TestArchiveDiff(t *testing.T) {
 
 		prev := pt.Preview(t, optpreview.Diff())
 
+		require.Contains(t, prev.StdOut, "~ testPath: archive(file:8933f25->e22d32b)")
 		require.Contains(t, prev.StdOut, "~ 1 to update")
 
 		pt.Up(t)
@@ -997,12 +1003,10 @@ func TestArchiveDiff(t *testing.T) {
     ~ prov:index/test:Test: (update)
         [id=newid]
         [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
-        testPath  : archive(assets:2a03253) {
-            "file1": asset(text:2cf24db) {
-                <contents elided>
+      ~ testPath: archive(assets:2a03253->4c74cbf) {
+          ~ "file1": asset(text:2cf24db->91e9240) {"<contents elided>" => "<contents elided>"
             }
-            "file2": asset(text:486ea46) {
-                <contents elided>
+          ~ "file2": asset(text:486ea46->da4c6d4) {"<contents elided>" => "<contents elided>"
             }
         }
 Resources:

pkg/tfbridge/detailed_diff.go

@@ -829,6 +829,7 @@ func makeDetailedDiffV2(
 	state shim.InstanceState,
 	diff shim.InstanceDiff,
 	assets AssetTable,
+	oldAssets AssetTable,
 	supportsSecrets bool,
 	newInputs resource.PropertyMap,
 	replaceOverride *bool,
@@ -848,7 +849,7 @@ func makeDetailedDiffV2(
 	if err != nil {
 		return nil, err
 	}
-	priorProps, err := MakeTerraformResult(ctx, prov, prior, tfs, ps, assets, supportsSecrets)
+	priorProps, err := MakeTerraformResult(ctx, prov, prior, tfs, ps, oldAssets, supportsSecrets)
 	if err != nil {
 		return nil, err
 	}

pkg/tfbridge/property_path.go

@@ -11,6 +11,8 @@ import (
 	"github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue"
 )
 
+// propertyPath is a wrapper around resource.PropertyPath that adds some convenience methods.
+// If the path is constructed using the propertyPath methods, it will only have string and int elements.
 type propertyPath resource.PropertyPath
 
 func isForceNew(tfs shim.Schema, ps *SchemaInfo) bool {

pkg/tfbridge/provider.go

@@ -1125,13 +1125,19 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	}
 
 	var state shim.InstanceState
+	var oldAssets AssetTable
 	if pNew, enabled := makeTerraformStateViaUpgradeEnabled(p.info, p.tf, olds); enabled {
 		state, err = makeTerraformStateViaUpgrade(ctx, pNew, res, olds)
 		if err != nil {
 			return nil, errors.Wrapf(err, "unmarshaling %s's instance state via upgrade", urn)
 		}
+
+		if res.Schema != nil {
+			oldAssets = getAssetTable(olds, res.Schema.Fields, res.TF.Schema())
+		}
 	} else {
-		state, err = makeTerraformStateWithOpts(ctx, res, req.GetId(), olds, makeTerraformStateOptions(opts))
+		state, oldAssets, err = makeTerraformStateWithAssetsWithOpts(
+			ctx, res, req.GetId(), olds, makeTerraformStateOptions(opts))
 		if err != nil {
 			return nil, errors.Wrapf(err, "unmarshaling %s's instance state", urn)
 		}
@@ -1176,7 +1182,7 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 
 		replaceDecision := diff.RequiresNew()
 		detailedDiff, err = makeDetailedDiffV2(
-			ctx, schema, fields, res.TF, p.tf, state, diff, assets, p.supportsSecrets, news, &replaceDecision)
+			ctx, schema, fields, res.TF, p.tf, state, diff, assets, oldAssets, p.supportsSecrets, news, &replaceDecision)
 		if err != nil {
 			return nil, err
 		}

pkg/tfbridge/schema.go

@@ -35,6 +35,7 @@ import (
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/log"
 	shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/schema"
+	"github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue"
 )
 
 // This file deals with translating between the Pulumi representations of a resource's configuration and state and the
@@ -1345,27 +1346,43 @@ func makeTerraformStateViaUpgradeEnabled(
 	return pp, true
 }
 
-// The old method used when [makeTerraformStateViaUpgrade] is not available.
-func makeTerraformStateWithOpts(
+// makeTerraformStateWithAssetsWithOpts is used in Diff when [makeTerraformStateViaUpgrade] is not available.
+func makeTerraformStateWithAssetsWithOpts(
 	ctx context.Context,
 	res Resource,
 	id string,
 	m resource.PropertyMap,
 	opts makeTerraformStateOptions,
-) (shim.InstanceState, error) {
+) (shim.InstanceState, AssetTable, error) {
 	// Turn the resource properties into a map. For the most part, this is a straight
 	// Mappable, but we use MapReplace because we use float64s and Terraform uses
 	// ints, to represent numbers.
-	inputs, _, err := makeTerraformInputsWithOptions(ctx, nil, nil, nil, m, res.TF.Schema(), res.Schema.Fields,
+	inputs, assets, err := makeTerraformInputsWithOptions(ctx, nil, nil, nil, m, res.TF.Schema(), res.Schema.Fields,
 		makeTerraformInputsOptions{DisableDefaults: true, DisableTFDefaults: true})
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	meta, err := parseMeta(m, res, opts)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
+	}
+	instanceState, err := res.TF.InstanceState(id, inputs, meta)
+	if err != nil {
+		return nil, nil, err
 	}
-	return res.TF.InstanceState(id, inputs, meta)
+	return instanceState, assets, nil
+}
+
+// The old method used when [makeTerraformStateViaUpgrade] is not available.
+func makeTerraformStateWithOpts(
+	ctx context.Context,
+	res Resource,
+	id string,
+	m resource.PropertyMap,
+	opts makeTerraformStateOptions,
+) (shim.InstanceState, error) {
+	state, _, err := makeTerraformStateWithAssetsWithOpts(ctx, res, id, m, opts)
+	return state, err
 }
 
 // The preferred method for recreating TF state that is used when the Pulumi state was written with a recent enough
@@ -1866,3 +1883,23 @@ func ExtractInputsFromOutputs(oldInputs, outs resource.PropertyMap,
 	// Otherwise, take a schema-directed approach that fills out all input-only properties.
 	return extractSchemaInputsObject(outs, tfs, ps), nil
 }
+
+func getAssetTable(m resource.PropertyMap, ps map[string]*SchemaInfo, tfs shim.SchemaMap) AssetTable {
+	assets := AssetTable{}
+	val := resource.NewObjectProperty(m)
+	_, err := propertyvalue.TransformPropertyValue(
+		resource.PropertyPath{},
+		func(p resource.PropertyPath, v resource.PropertyValue) (resource.PropertyValue, error) {
+			if v.IsAsset() || v.IsArchive() {
+				schPath := PropertyPathToSchemaPath(p, tfs, ps)
+				info := LookupSchemaInfoMapPath(schPath, ps)
+				contract.Assertf(info != nil, "unexpected asset %s", p.String())
+				assets[info] = v
+			}
+			return v, nil
+		},
+		val,
+	)
+	contract.AssertNoErrorf(err, "failed to transform property value")
+	return assets
+}

pkg/tfbridge/schema_test.go

@@ -37,6 +37,7 @@ import (
 
 	"github.com/pulumi/pulumi-terraform-bridge/v3/internal/testprovider"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/reservedkeys"
+	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info"
 	shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/schema"
 	shimv1 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v1"
@@ -4222,3 +4223,167 @@ func Test_makeTerraformStateWithOptsMaxItemsOneAdded(t *testing.T) {
 		autogold.Expect(map[string]interface{}{"props": []interface{}{"X"}}).Equal(t, inputs)
 	})
 }
+
+func TestGetAssetTable(t *testing.T) {
+	t.Parallel()
+
+	t.Run("simple asset", func(t *testing.T) {
+		asset, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		assetProp := resource.NewAssetProperty(asset)
+		props := resource.PropertyMap{"foo": assetProp}
+		ps := map[string]*SchemaInfo{"foo": {Asset: &AssetTranslation{Kind: FileAsset}}}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"foo": {
+				Type:     schemav2.TypeString,
+				Optional: true,
+			},
+		})
+		assets := getAssetTable(props, ps, tfs)
+		require.Len(t, assets, 1)
+		for info, v := range assets {
+			assert.Same(t, ps["foo"], info)
+			assert.True(t, v.DeepEquals(assetProp))
+		}
+	})
+
+	t.Run("no assets present", func(t *testing.T) {
+		props := resource.PropertyMap{"bar": resource.NewStringProperty("baz")}
+		ps := map[string]*SchemaInfo{"bar": {}}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"bar": {
+				Type:     schemav2.TypeString,
+				Optional: true,
+			},
+		})
+		assets := getAssetTable(props, ps, tfs)
+		assert.Empty(t, assets)
+	})
+
+	t.Run("archive present", func(t *testing.T) {
+		asset, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		archive, err := resource.NewAssetArchive(map[string]interface{}{"file.txt": asset})
+		require.NoError(t, err)
+		archiveProp := resource.NewArchiveProperty(archive)
+		props := resource.PropertyMap{"arch": archiveProp}
+		ps := map[string]*SchemaInfo{"arch": {Asset: &AssetTranslation{Kind: FileArchive}}}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"arch": {
+				Type:     schemav2.TypeString,
+				Optional: true,
+			},
+		})
+		assets := getAssetTable(props, ps, tfs)
+		require.Len(t, assets, 1)
+		for info, v := range assets {
+			assert.Same(t, ps["arch"], info)
+			assert.True(t, v.DeepEquals(archiveProp))
+		}
+	})
+
+	t.Run("asset with no matching SchemaInfo", func(t *testing.T) {
+		asset, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		assetProp := resource.NewAssetProperty(asset)
+		props := resource.PropertyMap{"missing": assetProp}
+		ps := map[string]*SchemaInfo{}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"bar": {
+				Type:     schemav2.TypeString,
+				Optional: true,
+			},
+		})
+		defer func() {
+			if r := recover(); r == nil {
+				assert.Fail(t, "panic did not occur", r)
+			}
+		}()
+		getAssetTable(props, ps, tfs)
+	})
+
+	t.Run("nested asset", func(t *testing.T) {
+		asset, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		nestedAsset := resource.NewAssetProperty(asset)
+		nestedProps := resource.PropertyMap{"outer": resource.NewObjectProperty(resource.PropertyMap{"inner": nestedAsset})}
+		nestedPS := map[string]*SchemaInfo{
+			"outer": {
+				Elem: &info.Schema{Fields: map[string]*SchemaInfo{
+					"inner": {Asset: &AssetTranslation{Kind: FileAsset}},
+				}},
+			},
+		}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"outer": {
+				Type:     schemav2.TypeList,
+				Optional: true,
+				MaxItems: 1,
+				Elem: &schemav2.Resource{
+					Schema: map[string]*schemav2.Schema{
+						"inner": {Type: schemav2.TypeString, Optional: true},
+					},
+				},
+			},
+		})
+		assets := getAssetTable(nestedProps, nestedPS, tfs)
+		found := false
+		for info, v := range assets {
+			if info == nestedPS["outer"].Elem.Fields["inner"] && v.DeepEquals(nestedAsset) {
+				found = true
+			}
+		}
+		assert.True(t, found)
+	})
+
+	t.Run("multiple assets", func(t *testing.T) {
+		asset1, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		asset2, err := resource.NewTextAsset("another asset")
+		require.NoError(t, err)
+		assetProp1 := resource.NewAssetProperty(asset1)
+		assetProp2 := resource.NewAssetProperty(asset2)
+		props := resource.PropertyMap{"foo": assetProp1, "bar": assetProp2}
+		ps := map[string]*SchemaInfo{
+			"foo": {Asset: &AssetTranslation{Kind: FileAsset}},
+			"bar": {Asset: &AssetTranslation{Kind: FileAsset}},
+		}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"foo": {Type: schemav2.TypeString, Optional: true},
+			"bar": {Type: schemav2.TypeString, Optional: true},
+		})
+		assets := getAssetTable(props, ps, tfs)
+		assert.Len(t, assets, 2)
+		assert.Contains(t, assets, ps["foo"])
+		assert.Contains(t, assets, ps["bar"])
+		assert.True(t, assets[ps["foo"]].DeepEquals(assetProp1))
+		assert.True(t, assets[ps["bar"]].DeepEquals(assetProp2))
+	})
+
+	t.Run("asset with nil SchemaInfo", func(t *testing.T) {
+		asset, err := resource.NewTextAsset("hello world")
+		require.NoError(t, err)
+		assetProp := resource.NewAssetProperty(asset)
+		props := resource.PropertyMap{"foo": assetProp}
+		ps := map[string]*SchemaInfo{"foo": nil}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"foo": {Type: schemav2.TypeString, Optional: true},
+		})
+		defer func() {
+			if r := recover(); r == nil {
+				assert.Fail(t, "panic did not occur", r)
+			}
+		}()
+		getAssetTable(props, ps, tfs)
+	})
+
+	t.Run("non-asset value with asset SchemaInfo", func(t *testing.T) {
+		props := resource.PropertyMap{"foo": resource.NewStringProperty("not an asset")}
+		ps := map[string]*SchemaInfo{"foo": {Asset: &AssetTranslation{Kind: FileAsset}}}
+		tfs := shimv2.NewSchemaMap(map[string]*schemav2.Schema{
+			"foo": {Type: schemav2.TypeString, Optional: true},
+		})
+		assets := getAssetTable(props, ps, tfs)
+		assert.Empty(t, assets)
+	})
+}