From 7fceccfbf746e37e6f6967a0a352c1666fee1010 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Thu, 26 Oct 2017 07:44:36 +0100 Subject: [PATCH] helper/schema: Loosen validation for 'id' field --- config/loader_hcl.go | 10 ++++++- helper/schema/resource.go | 36 ++++++++++++++++++---- helper/schema/resource_test.go | 55 +++++++++++++++++++++++++++------- 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/config/loader_hcl.go b/config/loader_hcl.go index a88406291..c8c82c312 100644 --- a/config/loader_hcl.go +++ b/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", } diff --git a/helper/schema/resource.go b/helper/schema/resource.go index ddba1096d..97b357706 100644 --- a/helper/schema/resource.go +++ b/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 - for k, _ := range tsm { - if isReservedResourceFieldName(k) { - return fmt.Errorf("%s is a reserved field name for a resource", k) + // Data source + if r.isTopLevel() && !writable { + tsm = schemaMap(r.Schema) + 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 diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go index 69748e0ed..b9332ccc6 100644 --- a/helper/schema/resource_test.go +++ b/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 { - t.Fatalf("%d: bad: %s", i, err) + if err != nil && !tc.Err { + t.Fatalf("%d: expected validation to pass: %s", i, err) + } + if err == nil && tc.Err { + t.Fatalf("%d: expected validation to fail", i) } }) }