Read needs a non-empty ID (#9243)

* Read needs a non-empty ID

* Add to CHANGELOG

* Fix TestRecordingReadFailurePreviousResource

* Fix TestRecordingReadFailureNoPreviousResource

* Fix TestRecordingReadSuccessNoPreviousResource

* Fix TestRecordingReadSuccessPreviousResource

* lint

CHANGELOG_PENDING.md

@@ -1,6 +1,6 @@
 ### Improvements
 
-- [area/cli] - Implement `pulumi stack unselect`.
+- [cli] - Implement `pulumi stack unselect`.
   [#9179](https://github.com/pulumi/pulumi/pull/9179)
 
 - [language/dotnet] - Updated Pulumi dotnet packages to use grpc-dotnet instead of grpc.
@@ -15,7 +15,7 @@
 - [cli] - Speed up `pulumi stack --show-name` by skipping unneeded snapshot loading.
   [#9199](https://github.com/pulumi/pulumi/pull/9199)
 
-- Improved error message for missing plugins.
+- [cli/plugins] - Improved error message for missing plugins.
   [#5208](https://github.com/pulumi/pulumi/pull/5208)
 
 ### Bug Fixes
@@ -24,4 +24,7 @@
   [#9065](https://github.com/pulumi/pulumi/issues/9065)
 
 - [sdk/go] - Fix a panic in `pulumi.All` when using pointer inputs.
-  [#9197](https://github.com/pulumi/pulumi/issues/9197)
+  [#9197](https://github.com/pulumi/pulumi/issues/9197)
+
+- [cli/engine] - Fix a panic due to passing `""` as the ID for a resource read.
+  [#9243](https://github.com/pulumi/pulumi/pull/9243)

pkg/backend/snapshot_test.go

@@ -800,7 +800,8 @@ func TestRecordingDeleteFailure(t *testing.T) {
 func TestRecordingReadSuccessNoPreviousResource(t *testing.T) {
 	t.Parallel()
 
-	resourceA := NewResource("a")
+	resourceA := NewResource("b")
+	resourceA.ID = "some-b"
 	resourceA.External = true
 	resourceA.Custom = true
 	snap := NewSnapshot(nil)
@@ -832,11 +833,13 @@ func TestRecordingReadSuccessNoPreviousResource(t *testing.T) {
 func TestRecordingReadSuccessPreviousResource(t *testing.T) {
 	t.Parallel()
 
-	resourceA := NewResource("a")
+	resourceA := NewResource("c")
+	resourceA.ID = "some-c"
 	resourceA.External = true
 	resourceA.Custom = true
 	resourceA.Inputs["key"] = resource.NewStringProperty("old")
-	resourceANew := NewResource("a")
+	resourceANew := NewResource("c")
+	resourceANew.ID = "some-other-c"
 	resourceANew.External = true
 	resourceANew.Custom = true
 	resourceANew.Inputs["key"] = resource.NewStringProperty("new")
@@ -877,7 +880,8 @@ func TestRecordingReadSuccessPreviousResource(t *testing.T) {
 func TestRecordingReadFailureNoPreviousResource(t *testing.T) {
 	t.Parallel()
 
-	resourceA := NewResource("a")
+	resourceA := NewResource("d")
+	resourceA.ID = "some-d"
 	resourceA.External = true
 	resourceA.Custom = true
 	snap := NewSnapshot(nil)
@@ -908,11 +912,13 @@ func TestRecordingReadFailureNoPreviousResource(t *testing.T) {
 func TestRecordingReadFailurePreviousResource(t *testing.T) {
 	t.Parallel()
 
-	resourceA := NewResource("a")
+	resourceA := NewResource("e")
+	resourceA.ID = "some-e"
 	resourceA.External = true
 	resourceA.Custom = true
 	resourceA.Inputs["key"] = resource.NewStringProperty("old")
-	resourceANew := NewResource("a")
+	resourceANew := NewResource("e")
+	resourceANew.ID = "some-new-e"
 	resourceANew.External = true
 	resourceANew.Custom = true
 	resourceANew.Inputs["key"] = resource.NewStringProperty("new")

pkg/engine/lifeycletest/pulumi_test.go

@@ -4470,3 +4470,33 @@ func TestRetainOnDelete(t *testing.T) {
 	assert.NotNil(t, snap)
 	assert.Len(t, snap.Resources, 0)
 }
+
+func TestInvalidGetIDReportsUserError(t *testing.T) {
+	t.Parallel()
+
+	loaders := []*deploytest.ProviderLoader{
+		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+			return &deploytest.Provider{}, nil
+		}, deploytest.WithoutGrpc),
+	}
+
+	program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+		_, _, err := monitor.ReadResource("pkgA:m:typA", "resA", "", "", resource.PropertyMap{}, "", "")
+		assert.Error(t, err)
+		return nil
+	})
+	host := deploytest.NewPluginHost(nil, nil, program, loaders...)
+
+	p := &TestPlan{
+		Options: UpdateOptions{Host: host},
+	}
+
+	project := p.GetProject()
+
+	validate := ExpectDiagMessage(t, "<{%reset%}>Expected an ID for urn:pulumi:test::test::pkgA:m:typA::resA<{%reset%}>")
+
+	snap, res := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, validate)
+	assert.NotNil(t, res)
+	assert.NotNil(t, snap)
+	assert.Len(t, snap.Resources, 1)
+}

pkg/resource/deploy/builtins.go

@@ -147,7 +147,8 @@ func (p *builtinProvider) Delete(urn resource.URN, id resource.ID,
 
 func (p *builtinProvider) Read(urn resource.URN, id resource.ID,
 	inputs, state resource.PropertyMap) (plugin.ReadResult, resource.Status, error) {
-
+	contract.Assertf(urn != "", "Read URN was empty")
+	contract.Assertf(id != "", "Read ID was empty")
 	contract.Assert(urn.Type() == stackReferenceType)
 
 	outputs, err := p.readStackReference(state)

pkg/resource/deploy/deploytest/provider.go

@@ -167,6 +167,8 @@ func (prov *Provider) Delete(urn resource.URN,
 
 func (prov *Provider) Read(urn resource.URN, id resource.ID,
 	inputs, state resource.PropertyMap) (plugin.ReadResult, resource.Status, error) {
+	contract.Assertf(urn != "", "Read URN was empty")
+	contract.Assertf(id != "", "Read ID was empty")
 	if prov.ReadF == nil {
 		return plugin.ReadResult{
 			Outputs: resource.PropertyMap{},

pkg/resource/deploy/step.go

@@ -570,6 +570,8 @@ type ReadStep struct {
 // NewReadStep creates a new Read step.
 func NewReadStep(deployment *Deployment, event ReadResourceEvent, old, new *resource.State) Step {
 	contract.Assert(new != nil)
+	contract.Assertf(new.URN != "", "Read URN was empty")
+	contract.Assertf(new.ID != "", "Read ID was empty")
 	contract.Assertf(new.External, "target of Read step must be marked External")
 	contract.Assertf(new.Custom, "target of Read step must be Custom")
 
@@ -592,6 +594,8 @@ func NewReadStep(deployment *Deployment, event ReadResourceEvent, old, new *reso
 // it will pend deletion of the "old" resource, which must not be an external resource.
 func NewReadReplacementStep(deployment *Deployment, event ReadResourceEvent, old, new *resource.State) Step {
 	contract.Assert(new != nil)
+	contract.Assertf(new.URN != "", "Read URN was empty")
+	contract.Assertf(new.ID != "", "Read ID was empty")
 	contract.Assertf(new.External, "target of ReadReplacement step must be marked External")
 	contract.Assertf(new.Custom, "target of ReadReplacement step must be Custom")
 	contract.Assert(old != nil)

pkg/resource/deploy/step_generator.go

@@ -144,6 +144,10 @@ func (sg *stepGenerator) GenerateReadSteps(event ReadResourceEvent) ([]Step, res
 		newState.SequenceNumber = old.SequenceNumber
 	}
 
+	if newState.ID == "" {
+		return nil, result.Errorf("Expected an ID for %v", urn)
+	}
+
 	// If the snapshot has an old resource for this URN and it's not external, we're going
 	// to have to delete the old resource and conceptually replace it with the resource we
 	// are about to read.

sdk/go/common/resource/plugin/provider_plugin.go

@@ -801,8 +801,8 @@ func (p *provider) Create(urn resource.URN, props resource.PropertyMap, timeout
 func (p *provider) Read(urn resource.URN, id resource.ID,
 	inputs, state resource.PropertyMap) (ReadResult, resource.Status, error) {
 
-	contract.Assert(urn != "")
-	contract.Assert(id != "")
+	contract.Assertf(urn != "", "Read URN was empty")
+	contract.Assertf(id != "", "Read ID was empty")
 
 	label := fmt.Sprintf("%s.Read(%s,%s)", p.label(), id, urn)
 	logging.V(7).Infof("%s executing (#inputs=%v, #state=%v)", label, len(inputs), len(state))