Skip to content

all: Refactor schema validation to support type-specific logic and prevent AttributeTypes/ElementType field panics #706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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