Merge pull request #16456 from hashicorp/f-loosen-schema-validation

helper/schema: Loosen validation for 'id' field

config/loader_hcl.go

@@ -17,6 +17,15 @@ type hclConfigurable struct {
 	Root *ast.File
 }
 
+var ReservedDataSourceFields = []string{
+	"connection",
+	"count",
+	"depends_on",
+	"lifecycle",
+	"provider",
+	"provisioner",
+}
+
 var ReservedResourceFields = []string{
 	"connection",
 	"count",
@@ -29,7 +38,6 @@ var ReservedResourceFields = []string{
 
 var ReservedProviderFields = []string{
 	"alias",
-	"id",
 	"version",
 }
 

helper/schema/resource.go

@@ -393,19 +393,43 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error
 				return err
 			}
 		}
+
+		for k, f := range tsm {
+			if isReservedResourceFieldName(k, f) {
+				return fmt.Errorf("%s is a reserved field name", k)
+			}
+		}
 	}
 
-	// Resource-specific checks
+	// Data source
-	for k, _ := range tsm {
+	if r.isTopLevel() && !writable {
-		if isReservedResourceFieldName(k) {
+		tsm = schemaMap(r.Schema)
-			return fmt.Errorf("%s is a reserved field name for a resource", k)
+		for k, _ := range tsm {
+			if isReservedDataSourceFieldName(k) {
+				return fmt.Errorf("%s is a reserved field name", k)
+			}
 		}
 	}
 
 	return schemaMap(r.Schema).InternalValidate(tsm)
 }
 
-func isReservedResourceFieldName(name string) bool {
+func isReservedDataSourceFieldName(name string) bool {
+	for _, reservedName := range config.ReservedDataSourceFields {
+		if name == reservedName {
+			return true
+		}
+	}
+	return false
+}
+
+func isReservedResourceFieldName(name string, s *Schema) bool {
+	// Allow phasing out "id"
+	// See https://github.com/terraform-providers/terraform-provider-aws/pull/1626#issuecomment-328881415
+	if name == "id" && (s.Deprecated != "" || s.Removed != "") {
+		return false
+	}
+
 	for _, reservedName := range config.ReservedResourceFields {
 		if name == reservedName {
 			return true
@@ -450,7 +474,7 @@ func (r *Resource) TestResourceData() *ResourceData {
 // Returns true if the resource is "top level" i.e. not a sub-resource.
 func (r *Resource) isTopLevel() bool {
 	// TODO: This is a heuristic; replace with a definitive attribute?
-	return r.Create != nil
+	return (r.Create != nil || r.Read != nil)
 }
 
 // Determines if a given InstanceState needs to be migrated by checking the

helper/schema/resource_test.go

@@ -591,14 +591,14 @@ func TestResourceInternalValidate(t *testing.T) {
 		Writable bool
 		Err      bool
 	}{
-		{
+		0: {
 			nil,
 			true,
 			true,
 		},
 
 		// No optional and no required
-		{
+		1: {
 			&Resource{
 				Schema: map[string]*Schema{
 					"foo": &Schema{
@@ -613,7 +613,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// Update undefined for non-ForceNew field
-		{
+		2: {
 			&Resource{
 				Create: func(d *ResourceData, meta interface{}) error { return nil },
 				Schema: map[string]*Schema{
@@ -628,7 +628,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// Update defined for ForceNew field
-		{
+		3: {
 			&Resource{
 				Create: func(d *ResourceData, meta interface{}) error { return nil },
 				Update: func(d *ResourceData, meta interface{}) error { return nil },
@@ -645,7 +645,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// non-writable doesn't need Update, Create or Delete
-		{
+		4: {
 			&Resource{
 				Schema: map[string]*Schema{
 					"goo": &Schema{
@@ -659,7 +659,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// non-writable *must not* have Create
-		{
+		5: {
 			&Resource{
 				Create: func(d *ResourceData, meta interface{}) error { return nil },
 				Schema: map[string]*Schema{
@@ -674,7 +674,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// writable must have Read
-		{
+		6: {
 			&Resource{
 				Create: func(d *ResourceData, meta interface{}) error { return nil },
 				Update: func(d *ResourceData, meta interface{}) error { return nil },
@@ -691,7 +691,7 @@ func TestResourceInternalValidate(t *testing.T) {
 		},
 
 		// writable must have Delete
-		{
+		7: {
 			&Resource{
 				Create: func(d *ResourceData, meta interface{}) error { return nil },
 				Read:   func(d *ResourceData, meta interface{}) error { return nil },
@@ -765,6 +765,38 @@ func TestResourceInternalValidate(t *testing.T) {
 			true,
 			false,
 		},
+
+		11: { // ID should be allowed in data source
+			&Resource{
+				Read: func(d *ResourceData, meta interface{}) error { return nil },
+				Schema: map[string]*Schema{
+					"id": &Schema{
+						Type:     TypeString,
+						Optional: true,
+					},
+				},
+			},
+			false,
+			false,
+		},
+
+		12: { // Deprecated ID should be allowed in resource
+			&Resource{
+				Create: func(d *ResourceData, meta interface{}) error { return nil },
+				Read:   func(d *ResourceData, meta interface{}) error { return nil },
+				Update: func(d *ResourceData, meta interface{}) error { return nil },
+				Delete: func(d *ResourceData, meta interface{}) error { return nil },
+				Schema: map[string]*Schema{
+					"id": &Schema{
+						Type:       TypeString,
+						Optional:   true,
+						Deprecated: "Use x_id instead",
+					},
+				},
+			},
+			true,
+			false,
+		},
 	}
 
 	for i, tc := range cases {
@@ -775,8 +807,11 @@ func TestResourceInternalValidate(t *testing.T) {
 			}
 
 			err := tc.In.InternalValidate(sm, tc.Writable)
-			if err != nil != tc.Err {
+			if err != nil && !tc.Err {
-				t.Fatalf("%d: bad: %s", i, err)
+				t.Fatalf("%d: expected validation to pass: %s", i, err)
+			}
+			if err == nil && tc.Err {
+				t.Fatalf("%d: expected validation to fail", i)
 			}
 		})
 	}