[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing #14007

Zaid-Ajaj · 2023-09-21T14:24:34Z

The problem in the issue the distinction between Assets or an archive being nil vs. being an empty map of assets. When creating and deserializing Archives, nil was allowed which leads to panics when asserting archive.IsAssets() because the Assets is nil even though it is a proper archive.

This PR fixes the issue by ensuring that an instance of Archive will have a non-nil Assets field whether that is during construction or marshalling.

Checklist

I have run make tidy to update any new dependencies
I have run make lint to verify my code passes the lint check
- I have formatted my code using gofumpt

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

pulumi-bot · 2023-09-21T14:25:32Z

Changelog

[uncommitted] (2023-09-22)

Bug Fixes

[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing
#14007

Frassle

We should do a similar fix for Asset

Frassle · 2023-09-21T14:30:08Z

sdk/go/common/resource/asset_test.go

+
+	emptyArchive, err := NewAssetArchive(nil)
+	assert.True(t, emptyArchive.IsAssets(), "even empty archives should be have empty assets")
+	assert.NoError(t, err, "Creating an empty archive should work")


A) Do the err check first
B) Use require.NoError, generally if we have an error the other asserts will just end up panic'ing (due to like nil access exceptions and the like)

Frassle · 2023-09-21T14:46:35Z

sdk/go/common/resource/asset.go

@@ -577,6 +582,10 @@ func (a *Archive) Serialize() map[string]interface{} {
 			}
 		}
 		result[ArchiveAssetsProperty] = assets
+	} else {
+		// Ensure that the assets property is always present, even if empty.


The JSON/YAML attributes have omitempty so this would just get dropped anyway. But that's expected we only want the field when this is an asset archive, and if assets is nil then it isn't. i.e. just remove this block.

I didn't notice those since we are dealing with a custom serialization/deserialization here but indeed, this seems redundant so removed this block

Frassle · 2023-09-21T14:47:54Z

sdk/go/common/resource/asset.go

-			if !ok {
-				return &Archive{}, false, fmt.Errorf("unexpected archive contents of type %T", v)
-			}
+	assets := make(map[string]interface{})


Not sure this is safe? Looks like we could create an Archive with assets and path set?

I updated the DeserializeArchive method such that it is not possible to construct conflicting fields:

When we check for the path property and it exists, we immediately return NewPathArchive

When we check for the uri property and it exists, we immediately return NewURIArchive

Finally if we didn't find path nor uri, we get the assets and default to an empty map

At the start of the function, we check that the input object must have one of path, uri or assets

Edit: no longer using New{...}Archive functions, because they recompute the hash, instead just constructing the archive using the deserialized values

Frassle · 2023-09-22T13:09:31Z

sdk/go/common/resource/asset.go

 	a := &Archive{Sig: ArchiveSig, URI: uri}
 	err := a.EnsureHash()
 	return a, err
 }

-func (a *Archive) IsAssets() bool { return a.Assets != nil }
+func (a *Archive) IsAssets() bool { return !a.IsPath() && !a.IsURI() }


This should just be a.Assets != nil. Whenever we construct an Archive object we'll ensure it's a non-nil map.

…zing

# Description  Fixes #15729. This fixes Assets and Archives to allow four states, as opposed to the three that #14007 had enforced. An asset can either be Text, Path, Uri, or none. That is `IsText`, `IsPath`, and `IsURI` can all return false. Similarly for archives except with Assets instead of Text. This happens when a provider returns an Assert (or Archive) with a hash value set, but no other contents. The Asset and Archive objects have been updated to handle this case, and a number of places in the CLI that asserted that one of IsText/IsPath/IsURI were true have been fixed up to handle the case of all three being false. ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt`  - [x] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

Zaid-Ajaj requested a review from a team September 21, 2023 14:24

Frassle reviewed Sep 21, 2023

View reviewed changes

Zaid-Ajaj force-pushed the zaid/ensure-archive-assets-are-non-nil branch from a31308e to 936d415 Compare September 22, 2023 00:22

Zaid-Ajaj changed the title ~~[sdk/go] Ensure Assets of AssetArchive are non-nil when creating, serializing and deserializing~~ [sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing Sep 22, 2023

Zaid-Ajaj force-pushed the zaid/ensure-archive-assets-are-non-nil branch 2 times, most recently from 173898a to 320ccd5 Compare September 22, 2023 09:39

Zaid-Ajaj requested a review from Frassle September 22, 2023 10:06

Zaid-Ajaj force-pushed the zaid/ensure-archive-assets-are-non-nil branch from b689485 to cd259a1 Compare September 22, 2023 13:04

Frassle reviewed Sep 22, 2023

View reviewed changes

Ensure Assets of AssetArchive are non-nil when creating and deseriali…

662301d

…zing

Zaid-Ajaj force-pushed the zaid/ensure-archive-assets-are-non-nil branch from 21d4297 to 662301d Compare September 22, 2023 13:18

Frassle approved these changes Sep 22, 2023

View reviewed changes

Zaid-Ajaj added this pull request to the merge queue Sep 22, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 22, 2023

Zaid-Ajaj added this pull request to the merge queue Sep 22, 2023

Merged via the queue into master with commit a44a536 Sep 22, 2023
44 checks passed

Zaid-Ajaj deleted the zaid/ensure-archive-assets-are-non-nil branch September 22, 2023 16:33

This was referenced Mar 19, 2024

InvalidParameterValueException: Uploaded file must be a non-empty zip pulumi/pulumi-aws#3622

Closed

HasContents returns true on an empty Archive #15729

Closed

Frassle mentioned this pull request Mar 20, 2024

Asset and Archive can have missing contents #15736

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing #14007

[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing #14007

Zaid-Ajaj commented Sep 21, 2023 •

edited

pulumi-bot commented Sep 21, 2023 •

edited

Frassle left a comment

Frassle Sep 21, 2023

Zaid-Ajaj Sep 22, 2023

Frassle Sep 21, 2023

Zaid-Ajaj Sep 22, 2023

Frassle Sep 21, 2023

Zaid-Ajaj Sep 22, 2023 •

edited

Frassle Sep 22, 2023

[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing #14007

[sdk/go] Ensure Assets of AssetArchive are non-nil when creating and deserializing #14007

Conversation

Zaid-Ajaj commented Sep 21, 2023 • edited

Checklist

pulumi-bot commented Sep 21, 2023 • edited

Changelog

[uncommitted] (2023-09-22)

Bug Fixes

Frassle left a comment

Choose a reason for hiding this comment

Frassle Sep 21, 2023

Choose a reason for hiding this comment

Zaid-Ajaj Sep 22, 2023

Choose a reason for hiding this comment

Frassle Sep 21, 2023

Choose a reason for hiding this comment

Zaid-Ajaj Sep 22, 2023

Choose a reason for hiding this comment

Frassle Sep 21, 2023

Choose a reason for hiding this comment

Zaid-Ajaj Sep 22, 2023 • edited

Choose a reason for hiding this comment

Frassle Sep 22, 2023

Choose a reason for hiding this comment

Zaid-Ajaj commented Sep 21, 2023 •

edited

pulumi-bot commented Sep 21, 2023 •

edited

Zaid-Ajaj Sep 22, 2023 •

edited