helper/resource: Remove data source and resource id attribute requirement (#164)

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.

Author: bflad

.changes/unreleased/ENHANCEMENTS-20230820-195300.yaml

@@ -0,0 +1,7 @@
+kind: ENHANCEMENTS
+body: 'helper/resource: Added `TestStep` type `ImportStateVerifyIdentifierAttribute`
+  field, which can override the default `id` attribute used for matching prior
+  resource state with imported resource state'
+time: 2023-08-20T19:53:00.488517-04:00
+custom:
+  Issue: "84"

.changes/unreleased/FEATURES-20230820-195624.yaml

@@ -0,0 +1,6 @@
+kind: FEATURES
+body: 'helper/resource: Removed data resource and managed resource `id` attribute
+  requirement'
+time: 2023-08-20T19:56:24.356163-04:00
+custom:
+  Issue: "84"

helper/resource/state_shim.go

@@ -193,15 +193,36 @@ func shimResourceState(res *tfjson.StateResource) (*terraform.ResourceState, err
 	}
 	attributes := sf.Flatmap()
 
-	if _, ok := attributes["id"]; !ok {
-		return nil, fmt.Errorf("no %q found in attributes", "id")
+	// The instance state identifier was a Terraform versions 0.11 and earlier
+	// concept which helped core and the then SDK determine if the resource
+	// should be removed and as an identifier value in the human readable
+	// output. This concept unfortunately carried over to the testing logic when
+	// the testing logic was mostly changed to use the public, machine-readable
+	// JSON interface with Terraform, rather than reusing prior internal logic
+	// from Terraform. Using the "id" attribute value for this identifier was
+	// the default implementation and therefore those older versions of
+	// Terraform required the attribute. This is no longer necessary after
+	// Terraform versions 0.12 and later.
+	//
+	// If the "id" attribute is not found, set the instance state identifier to
+	// a synthetic value that can hopefully lead someone encountering the value
+	// to these comments. The prior logic used to raise an error if the
+	// attribute was not present, but this value should now only be present in
+	// legacy logic of this Go module, such as unintentionally exported logic in
+	// the terraform package, and not encountered during normal testing usage.
+	//
+	// Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/84
+	instanceStateID, ok := attributes["id"]
+
+	if !ok {
+		instanceStateID = "id-attribute-not-set"
 	}
 
 	return &terraform.ResourceState{
 		Provider: res.ProviderName,
 		Type:     res.Type,
 		Primary: &terraform.InstanceState{
-			ID:         attributes["id"],
+			ID:         instanceStateID,
 			Attributes: attributes,
 			Meta: map[string]interface{}{
 				"schema_version": int(res.SchemaVersion),

helper/resource/testcase_test.go

@@ -0,0 +1,115 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package resource
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
+	"github.com/hashicorp/terraform-plugin-go/tftypes"
+	"github.com/hashicorp/terraform-plugin-testing/internal/testing/testprovider"
+	"github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/datasource"
+	"github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/providerserver"
+	"github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/resource"
+)
+
+// Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/84
+func TestTestCase_NoDataSourceIdRequirement(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		Steps: []TestStep{
+			{
+				Check: ComposeAggregateTestCheckFunc(
+					TestCheckNoResourceAttr("data.test_datasource.test", "id"),
+					TestCheckResourceAttr("data.test_datasource.test", "not_id", "test"),
+				),
+				Config: `data "test_datasource" "test" {}`,
+				ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
+					"test": providerserver.NewProviderServer(testprovider.Provider{
+						DataSources: map[string]testprovider.DataSource{
+							"test_datasource": {
+								ReadResponse: &datasource.ReadResponse{
+									State: tftypes.NewValue(
+										tftypes.Object{
+											AttributeTypes: map[string]tftypes.Type{
+												"not_id": tftypes.String,
+											},
+										},
+										map[string]tftypes.Value{
+											"not_id": tftypes.NewValue(tftypes.String, "test"),
+										},
+									),
+								},
+								SchemaResponse: &datasource.SchemaResponse{
+									Schema: &tfprotov6.Schema{
+										Block: &tfprotov6.SchemaBlock{
+											Attributes: []*tfprotov6.SchemaAttribute{
+												{
+													Name:     "not_id",
+													Type:     tftypes.String,
+													Computed: true,
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					}),
+				},
+			},
+		},
+	})
+}
+
+// Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/84
+func TestTestCase_NoResourceIdRequirement(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		Steps: []TestStep{
+			{
+				Check: ComposeAggregateTestCheckFunc(
+					TestCheckNoResourceAttr("test_resource.test", "id"),
+					TestCheckResourceAttr("test_resource.test", "not_id", "test"),
+				),
+				Config: `resource "test_resource" "test" {}`,
+				ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
+					"test": providerserver.NewProviderServer(testprovider.Provider{
+						Resources: map[string]testprovider.Resource{
+							"test_resource": {
+								CreateResponse: &resource.CreateResponse{
+									NewState: tftypes.NewValue(
+										tftypes.Object{
+											AttributeTypes: map[string]tftypes.Type{
+												"not_id": tftypes.String,
+											},
+										},
+										map[string]tftypes.Value{
+											"not_id": tftypes.NewValue(tftypes.String, "test"),
+										},
+									),
+								},
+								SchemaResponse: &resource.SchemaResponse{
+									Schema: &tfprotov6.Schema{
+										Block: &tfprotov6.SchemaBlock{
+											Attributes: []*tfprotov6.SchemaAttribute{
+												{
+													Name:     "not_id",
+													Type:     tftypes.String,
+													Computed: true,
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					}),
+				},
+			},
+		},
+	})
+}

helper/resource/testing.go

@@ -604,10 +604,24 @@ type TestStep struct {
 	// IDs returned by the Import.  Note that this checks for strict equality
 	// and does not respect DiffSuppressFunc or CustomizeDiff.
 	//
+	// By default, the prior resource state and import resource state are
+	// matched by the "id" attribute. If the "id" attribute is not implemented
+	// or another attribute more uniquely identifies the resource, set the
+	// ImportStateVerifyIdentifierAttribute field to adjust the attribute for
+	// matching.
+	//
+	// If certain attributes cannot be correctly imported, set the
+	// ImportStateVerifyIgnore field.
+	ImportStateVerify bool
+
+	// ImportStateVerifyIdentifierAttribute is the resource attribute for
+	// matching the prior resource state and import resource state during import
+	// verification. By default, the "id" attribute is used.
+	ImportStateVerifyIdentifierAttribute string
+
 	// ImportStateVerifyIgnore is a list of prefixes of fields that should
 	// not be verified to be equal. These can be set to ephemeral fields or
 	// fields that can't be refreshed and don't matter.
-	ImportStateVerify       bool
 	ImportStateVerifyIgnore []string
 
 	// ImportStatePersist, if true, will update the persisted state with the

helper/resource/testing_new_import_state.go

@@ -182,20 +182,41 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest
 			}
 		}
 
+		identifierAttribute := step.ImportStateVerifyIdentifierAttribute
+
+		if identifierAttribute == "" {
+			identifierAttribute = "id"
+		}
+
 		for _, r := range newResources {
+			rIdentifier, ok := r.Primary.Attributes[identifierAttribute]
+
+			if !ok {
+				t.Fatalf("ImportStateVerify: New resource missing identifier attribute %q, ensure attribute value is properly set or use ImportStateVerifyIdentifierAttribute to choose different attribute", identifierAttribute)
+			}
+
 			// Find the existing resource
 			var oldR *terraform.ResourceState
 			for _, r2 := range oldResources {
+				if r2.Primary == nil || r2.Type != r.Type || r2.Provider != r.Provider {
+					continue
+				}
+
+				r2Identifier, ok := r2.Primary.Attributes[identifierAttribute]
+
+				if !ok {
+					t.Fatalf("ImportStateVerify: Old resource missing identifier attribute %q, ensure attribute value is properly set or use ImportStateVerifyIdentifierAttribute to choose different attribute", identifierAttribute)
+				}
 
-				if r2.Primary != nil && r2.Primary.ID == r.Primary.ID && r2.Type == r.Type && r2.Provider == r.Provider {
+				if r2Identifier == rIdentifier {
 					oldR = r2
 					break
 				}
 			}
 			if oldR == nil || oldR.Primary == nil {
 				t.Fatalf(
 					"Failed state verification, resource with ID %s not found",
-					r.Primary.ID)
+					rIdentifier)
 			}
 
 			// don't add empty flatmapped containers, so we can more easily