Skip to content

Commit

Permalink
all: Refactor schema validation to support type-specific logic and pr…
Browse files Browse the repository at this point in the history
…event AttributeTypes/ElementType field panics

Reference: #574
Reference: #699
Reference: #704
Reference: #705

The goal of this change is to enable easier schema validation within the framework logic. This is achieved by implementing new internal interfaces that implement shared logic across all schema types, then introduce methods on attribute types which currently implement type-specific validation logic.

The new `ValidateImplementation()` methods on attribute types, while technically exported, cannot be implemented externally due to their usage of internal types. This follows the existing implementation of the framework where attribute types already cannot be extended due to the `Equal(fwschema.Attribute)` method requirement. Therefore these new `ValidateImplementation()` methods do not need to worry about compatibility promises and can be modified or removed in the future.

This change also includes some breadcrumbs for other schema validation requests.
  • Loading branch information
bflad committed Mar 29, 2023
1 parent 4fc07b7 commit 1f275d6
Show file tree
Hide file tree
Showing 83 changed files with 4,111 additions and 2,393 deletions.
7 changes: 7 additions & 0 deletions .changes/unreleased/BUG FIXES-20230329-102850.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: BUG FIXES
body: 'datasource/schema: Raise errors with `ListAttribute`, `MapAttribute`, `ObjectAttribute`,
and `SetAttribute` implementations instead of panics when missing required `AttributeTypes` or
`ElementTypes` fields'
time: 2023-03-29T10:28:50.525346-04:00
custom:
Issue: "699"
7 changes: 7 additions & 0 deletions .changes/unreleased/BUG FIXES-20230329-102851.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: BUG FIXES
body: 'provider/metaschema: Raise errors with `ListAttribute`, `MapAttribute`, `ObjectAttribute`,
and `SetAttribute` implementations instead of panics when missing required `AttributeTypes` or
`ElementTypes` fields'
time: 2023-03-29T10:28:51.525346-04:00
custom:
Issue: "699"
7 changes: 7 additions & 0 deletions .changes/unreleased/BUG FIXES-20230329-102852.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: BUG FIXES
body: 'provider/schema: Raise errors with `ListAttribute`, `MapAttribute`, `ObjectAttribute`,
and `SetAttribute` implementations instead of panics when missing required `AttributeTypes` or
`ElementTypes` fields'
time: 2023-03-29T10:28:52.525346-04:00
custom:
Issue: "699"
7 changes: 7 additions & 0 deletions .changes/unreleased/BUG FIXES-20230329-102853.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: BUG FIXES
body: 'resource/schema: Raise errors with `ListAttribute`, `MapAttribute`, `ObjectAttribute`,
and `SetAttribute` implementations instead of panics when missing required `AttributeTypes` or
`ElementTypes` fields'
time: 2023-03-29T10:28:53.525346-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230329-102106.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: ENHANCEMENTS
body: 'datasource/schema: Added `Schema` type `ValidateImplementation()` method, which performs framework-defined
schema validation and can be used in unit testing'
time: 2023-03-29T10:21:06.251285-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230329-102107.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: ENHANCEMENTS
body: 'provider/metaschema: Added `Schema` type `ValidateImplementation()` method, which performs framework-defined
schema validation and can be used in unit testing'
time: 2023-03-29T10:21:07.251285-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230329-102108.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: ENHANCEMENTS
body: 'provider/schema: Added `Schema` type `ValidateImplementation()` method, which performs framework-defined
schema validation and can be used in unit testing'
time: 2023-03-29T10:21:08.251285-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230329-102109.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: ENHANCEMENTS
body: 'resource/schema: Added `Schema` type `ValidateImplementation()` method, which performs framework-defined
schema validation and can be used in unit testing'
time: 2023-03-29T10:21:09.251285-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/NOTES-20230329-124336.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: NOTES
body: 'datasource/schema: The `Schema` type `Validate()` method has been deprecated
in preference of `ValidateImplementation()`'
time: 2023-03-29T12:43:36.636825-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/NOTES-20230329-124337.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: NOTES
body: 'provider/metaschema: The `Schema` type `Validate()` method has been deprecated
in preference of `ValidateImplementation()`'
time: 2023-03-29T12:43:37.636825-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/NOTES-20230329-124338.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: NOTES
body: 'provider/schema: The `Schema` type `Validate()` method has been deprecated
in preference of `ValidateImplementation()`'
time: 2023-03-29T12:43:38.636825-04:00
custom:
Issue: "699"
6 changes: 6 additions & 0 deletions .changes/unreleased/NOTES-20230329-124339.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: NOTES
body: 'resource/schema: The `Schema` type `Validate()` method has been deprecated
in preference of `ValidateImplementation()`'
time: 2023-03-29T12:43:39.636825-04:00
custom:
Issue: "699"
17 changes: 15 additions & 2 deletions datasource/schema/list_attribute.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package schema

import (
"context"

"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema/fwxschema"
Expand All @@ -12,8 +14,9 @@ import (

// Ensure the implementation satisifies the desired interfaces.
var (
_ Attribute = ListAttribute{}
_ fwxschema.AttributeWithListValidators = ListAttribute{}
_ Attribute = ListAttribute{}
_ fwschema.AttributeWithValidateImplementation = ListAttribute{}
_ fwxschema.AttributeWithListValidators = ListAttribute{}
)

// ListAttribute represents a schema attribute that is a list with a single
Expand Down Expand Up @@ -195,3 +198,13 @@ func (a ListAttribute) IsSensitive() bool {
func (a ListAttribute) ListValidators() []validator.List {
return a.Validators
}

// ValidateImplementation contains logic for validating the
// provider-defined implementation of the attribute to prevent unexpected
// errors or panics. This logic runs during the GetProviderSchema RPC
// and should never include false positives.
func (a ListAttribute) ValidateImplementation(ctx context.Context, req fwschema.ValidateImplementationRequest, resp *fwschema.ValidateImplementationResponse) {
if a.CustomType == nil && a.ElementType == nil {
resp.Diagnostics.Append(fwschema.AttributeMissingElementTypeDiag(req.Path))
}
}
72 changes: 72 additions & 0 deletions datasource/schema/list_attribute_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
package schema_test

import (
"context"
"fmt"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/datasource/schema"
"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
"github.com/hashicorp/terraform-plugin-framework/internal/testing/testschema"
testtypes "github.com/hashicorp/terraform-plugin-framework/internal/testing/types"
"github.com/hashicorp/terraform-plugin-framework/path"
"github.com/hashicorp/terraform-plugin-framework/schema/validator"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-go/tftypes"
Expand Down Expand Up @@ -424,3 +428,71 @@ func TestListAttributeListValidators(t *testing.T) {
})
}
}

func TestListAttributeValidateImplementation(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
attribute schema.ListAttribute
request fwschema.ValidateImplementationRequest
expected *fwschema.ValidateImplementationResponse
}{
"customtype": {
attribute: schema.ListAttribute{
Computed: true,
CustomType: testtypes.ListType{},
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{},
},
"elementtype": {
attribute: schema.ListAttribute{
Computed: true,
ElementType: types.StringType,
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{},
},
"elementtype-missing": {
attribute: schema.ListAttribute{
Computed: true,
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{
Diagnostics: diag.Diagnostics{
diag.NewErrorDiagnostic(
"Invalid Attribute Implementation",
"When validating the schema, an implementation issue was found. "+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
"\"test\" is missing the CustomType or ElementType field on a collection Attribute. "+
"One of these fields is required to prevent other unexpected errors or panics.",
),
},
},
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := &fwschema.ValidateImplementationResponse{}
testCase.attribute.ValidateImplementation(context.Background(), testCase.request, got)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}
17 changes: 15 additions & 2 deletions datasource/schema/map_attribute.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package schema

import (
"context"

"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema/fwxschema"
Expand All @@ -12,8 +14,9 @@ import (

// Ensure the implementation satisifies the desired interfaces.
var (
_ Attribute = MapAttribute{}
_ fwxschema.AttributeWithMapValidators = MapAttribute{}
_ Attribute = MapAttribute{}
_ fwschema.AttributeWithValidateImplementation = MapAttribute{}
_ fwxschema.AttributeWithMapValidators = MapAttribute{}
)

// MapAttribute represents a schema attribute that is a list with a single
Expand Down Expand Up @@ -198,3 +201,13 @@ func (a MapAttribute) IsSensitive() bool {
func (a MapAttribute) MapValidators() []validator.Map {
return a.Validators
}

// ValidateImplementation contains logic for validating the
// provider-defined implementation of the attribute to prevent unexpected
// errors or panics. This logic runs during the GetProviderSchema RPC
// and should never include false positives.
func (a MapAttribute) ValidateImplementation(ctx context.Context, req fwschema.ValidateImplementationRequest, resp *fwschema.ValidateImplementationResponse) {
if a.CustomType == nil && a.ElementType == nil {
resp.Diagnostics.Append(fwschema.AttributeMissingElementTypeDiag(req.Path))
}
}
72 changes: 72 additions & 0 deletions datasource/schema/map_attribute_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
package schema_test

import (
"context"
"fmt"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/datasource/schema"
"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
"github.com/hashicorp/terraform-plugin-framework/internal/testing/testschema"
testtypes "github.com/hashicorp/terraform-plugin-framework/internal/testing/types"
"github.com/hashicorp/terraform-plugin-framework/path"
"github.com/hashicorp/terraform-plugin-framework/schema/validator"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-go/tftypes"
Expand Down Expand Up @@ -424,3 +428,71 @@ func TestMapAttributeMapValidators(t *testing.T) {
})
}
}

func TestMapAttributeValidateImplementation(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
attribute schema.MapAttribute
request fwschema.ValidateImplementationRequest
expected *fwschema.ValidateImplementationResponse
}{
"customtype": {
attribute: schema.MapAttribute{
Computed: true,
CustomType: testtypes.MapType{},
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{},
},
"elementtype": {
attribute: schema.MapAttribute{
Computed: true,
ElementType: types.StringType,
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{},
},
"elementtype-missing": {
attribute: schema.MapAttribute{
Computed: true,
},
request: fwschema.ValidateImplementationRequest{
Name: "test",
Path: path.Root("test"),
},
expected: &fwschema.ValidateImplementationResponse{
Diagnostics: diag.Diagnostics{
diag.NewErrorDiagnostic(
"Invalid Attribute Implementation",
"When validating the schema, an implementation issue was found. "+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
"\"test\" is missing the CustomType or ElementType field on a collection Attribute. "+
"One of these fields is required to prevent other unexpected errors or panics.",
),
},
},
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := &fwschema.ValidateImplementationResponse{}
testCase.attribute.ValidateImplementation(context.Background(), testCase.request, got)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}
17 changes: 15 additions & 2 deletions datasource/schema/object_attribute.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package schema

import (
"context"

"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema"
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema/fwxschema"
Expand All @@ -12,8 +14,9 @@ import (

// Ensure the implementation satisifies the desired interfaces.
var (
_ Attribute = ObjectAttribute{}
_ fwxschema.AttributeWithObjectValidators = ObjectAttribute{}
_ Attribute = ObjectAttribute{}
_ fwschema.AttributeWithValidateImplementation = ObjectAttribute{}
_ fwxschema.AttributeWithObjectValidators = ObjectAttribute{}
)

// ObjectAttribute represents a schema attribute that is an object with only
Expand Down Expand Up @@ -197,3 +200,13 @@ func (a ObjectAttribute) IsSensitive() bool {
func (a ObjectAttribute) ObjectValidators() []validator.Object {
return a.Validators
}

// ValidateImplementation contains logic for validating the
// provider-defined implementation of the attribute to prevent unexpected
// errors or panics. This logic runs during the GetProviderSchema RPC
// and should never include false positives.
func (a ObjectAttribute) ValidateImplementation(ctx context.Context, req fwschema.ValidateImplementationRequest, resp *fwschema.ValidateImplementationResponse) {
if a.AttributeTypes == nil && a.CustomType == nil {
resp.Diagnostics.Append(fwschema.AttributeMissingAttributeTypesDiag(req.Path))
}
}
Loading

0 comments on commit 1f275d6

Please sign in to comment.