Skip to content

Commit 9f669b5

Browse files
mildwonkeymildwonkey
committed
configschema: extract and extend attribute validation
This commit adds an attribute-specific InternalValidate which was extracted directly from the block.InternalValidate logic and extended to verify any NestedTypes inside an Attribute. Only one error message changed, since it is now valid to have a cty.NilType for Attribute.Type as long as NestedType is set.
1 parent c5a903b commit 9f669b5

File tree

3 files changed

+100
-66
lines changed

3 files changed

+100
-66
lines changed

‎configs/configschema/decoder_spec.go‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,10 @@ func (b *Block) DecoderSpec() hcldec.Spec {
9292

9393
for name, blockS := range b.BlockTypes {
9494
if _, exists := ret[name]; exists {
95-
// This indicates an invalid schema, since it's not valid to
96-
// define both an attribute and a block type of the same name.
97-
// However, we don't raise this here since it's checked by
98-
// InternalValidate.
95+
// This indicates an invalid schema, since it's not valid to define
96+
// both an attribute and a block type of the same name. We assume
97+
// that the provider has already used something like
98+
// InternalValidate to validate their schema.
9999
continue
100100
}
101101

@@ -104,7 +104,7 @@ func (b *Block) DecoderSpec() hcldec.Spec {
104104
// We can only validate 0 or 1 for MinItems, because a dynamic block
105105
// may satisfy any number of min items while only having a single
106106
// block in the config. We cannot validate MaxItems because a
107-
// configuration may have any number of dynamic blocks
107+
// configuration may have any number of dynamic blocks.
108108
minItems := 0
109109
if blockS.MinItems > 1 {
110110
minItems = 1
@@ -145,10 +145,12 @@ func (b *Block) DecoderSpec() hcldec.Spec {
145145
}
146146
case NestingSet:
147147
// We forbid dynamically-typed attributes inside NestingSet in
148-
// InternalValidate, so we don't do anything special to handle
149-
// that here. (There is no set analog to tuple and object types,
150-
// because cty's set implementation depends on knowing the static
151-
// type in order to properly compute its internal hashes.)
148+
// InternalValidate, so we don't do anything special to handle that
149+
// here. (There is no set analog to tuple and object types, because
150+
// cty's set implementation depends on knowing the static type in
151+
// order to properly compute its internal hashes.) We assume that
152+
// the provider has already used something like InternalValidate to
153+
// validate their schema.
152154
ret[name] = &hcldec.BlockSetSpec{
153155
TypeName: name,
154156
Nested: childSpec,
@@ -174,7 +176,8 @@ func (b *Block) DecoderSpec() hcldec.Spec {
174176
}
175177
default:
176178
// Invalid nesting type is just ignored. It's checked by
177-
// InternalValidate.
179+
// InternalValidate. We assume that the provider has already used
180+
// something like InternalValidate to validate their schema.
178181
continue
179182
}
180183
}
@@ -190,9 +193,10 @@ func (a *Attribute) decoderSpec(name string) hcldec.Spec {
190193
}
191194

192195
if a.NestedType != nil {
193-
// FIXME: a panic() is a bad UX. Fix this, probably by extending
194-
// InternalValidate() to check Attribute schemas as well and calling it
195-
// when we get the schema from the provider in Context().
196+
// FIXME: a panic() is a bad UX. InternalValidate() can check Attribute
197+
// schemas as well so a fix might be to call it when we get the schema
198+
// from the provider in Context(). Since this could be a breaking
199+
// change, we'd need to communicate well before adding that call.
196200
if a.Type != cty.NilType {
197201
panic("Invalid attribute schema: NestedType and Type cannot both be set. This is a bug in the provider.")
198202
}

‎configs/configschema/internal_validate.go‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212
var validName = regexp.MustCompile(`^[a-z0-9_]+$`)
1313

1414
// InternalValidate returns an error if the receiving block and its child
15-
// schema definitions have any consistencies with the documented rules for
15+
// schema definitions have any inconsistencies with the documented rules for
1616
// valid schema.
1717
//
1818
// This is intended to be used within unit tests to detect when a given
@@ -31,21 +31,7 @@ func (b *Block) internalValidate(prefix string, err error) error {
3131
err = multierror.Append(err, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
3232
continue
3333
}
34-
if !validName.MatchString(name) {
35-
err = multierror.Append(err, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
36-
}
37-
if !attrS.Optional && !attrS.Required && !attrS.Computed {
38-
err = multierror.Append(err, fmt.Errorf("%s%s: must set Optional, Required or Computed", prefix, name))
39-
}
40-
if attrS.Optional && attrS.Required {
41-
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Optional and Required", prefix, name))
42-
}
43-
if attrS.Computed && attrS.Required {
44-
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Computed and Required", prefix, name))
45-
}
46-
if attrS.Type == cty.NilType {
47-
err = multierror.Append(err, fmt.Errorf("%s%s: Type must be set to something other than cty.NilType", prefix, name))
48-
}
34+
err = multierror.Append(err, attrS.internalValidate(name, prefix))
4935
}
5036

5137
for name, blockS := range b.BlockTypes {
@@ -105,7 +91,7 @@ func (b *Block) internalValidate(prefix string, err error) error {
10591
}
10692

10793
// InternalValidate returns an error if the receiving attribute and its child
108-
// schema definitions have any consistencies with the documented rules for
94+
// schema definitions have any inconsistencies with the documented rules for
10995
// valid schema.
11096
func (a *Attribute) InternalValidate(name string) error {
11197
if a == nil {
@@ -117,9 +103,6 @@ func (a *Attribute) InternalValidate(name string) error {
117103
func (a *Attribute) internalValidate(name, prefix string) error {
118104
var err error
119105

120-
if a == nil {
121-
err = multierror.Append(err, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
122-
}
123106
if !validName.MatchString(name) {
124107
err = multierror.Append(err, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
125108
}
@@ -133,6 +116,10 @@ func (a *Attribute) internalValidate(name, prefix string) error {
133116
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Computed and Required", prefix, name))
134117
}
135118

119+
if a.Type == cty.NilType && a.NestedType == nil {
120+
err = multierror.Append(err, fmt.Errorf("%s%s: either Type or NestedType must be defined", prefix, name))
121+
}
122+
136123
if a.Type != cty.NilType {
137124
if a.NestedType != nil {
138125
err = multierror.Append(fmt.Errorf("%s: Type and NestedType cannot both be set", name))
@@ -168,7 +155,14 @@ func (a *Attribute) internalValidate(name, prefix string) error {
168155
default:
169156
err = multierror.Append(err, fmt.Errorf("%s%s: invalid nesting mode %s", prefix, name, a.NestedType.Nesting))
170157
}
158+
for name, attrS := range a.NestedType.Attributes {
159+
if attrS == nil {
160+
err = multierror.Append(err, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
161+
continue
162+
}
163+
err = multierror.Append(err, attrS.internalValidate(name, prefix))
164+
}
171165
}
172166

173-
return fmt.Errorf("either Type or NestedType must be defined")
167+
return err
174168
}

‎configs/configschema/internal_validate_test.go‎

Lines changed: 68 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -20,54 +20,54 @@ func TestBlockInternalValidate(t *testing.T) {
2020
"valid": {
2121
&Block{
2222
Attributes: map[string]*Attribute{
23-
"foo": &Attribute{
23+
"foo": {
2424
Type: cty.String,
2525
Required: true,
2626
},
27-
"bar": &Attribute{
27+
"bar": {
2828
Type: cty.String,
2929
Optional: true,
3030
},
31-
"baz": &Attribute{
31+
"baz": {
3232
Type: cty.String,
3333
Computed: true,
3434
},
35-
"baz_maybe": &Attribute{
35+
"baz_maybe": {
3636
Type: cty.String,
3737
Optional: true,
3838
Computed: true,
3939
},
4040
},
4141
BlockTypes: map[string]*NestedBlock{
42-
"single": &NestedBlock{
42+
"single": {
4343
Nesting: NestingSingle,
4444
Block: Block{},
4545
},
46-
"single_required": &NestedBlock{
46+
"single_required": {
4747
Nesting: NestingSingle,
4848
Block: Block{},
4949
MinItems: 1,
5050
MaxItems: 1,
5151
},
52-
"list": &NestedBlock{
52+
"list": {
5353
Nesting: NestingList,
5454
Block: Block{},
5555
},
56-
"list_required": &NestedBlock{
56+
"list_required": {
5757
Nesting: NestingList,
5858
Block: Block{},
5959
MinItems: 1,
6060
},
61-
"set": &NestedBlock{
61+
"set": {
6262
Nesting: NestingSet,
6363
Block: Block{},
6464
},
65-
"set_required": &NestedBlock{
65+
"set_required": {
6666
Nesting: NestingSet,
6767
Block: Block{},
6868
MinItems: 1,
6969
},
70-
"map": &NestedBlock{
70+
"map": {
7171
Nesting: NestingMap,
7272
Block: Block{},
7373
},
@@ -78,7 +78,7 @@ func TestBlockInternalValidate(t *testing.T) {
7878
"attribute with no flags set": {
7979
&Block{
8080
Attributes: map[string]*Attribute{
81-
"foo": &Attribute{
81+
"foo": {
8282
Type: cty.String,
8383
},
8484
},
@@ -88,7 +88,7 @@ func TestBlockInternalValidate(t *testing.T) {
8888
"attribute required and optional": {
8989
&Block{
9090
Attributes: map[string]*Attribute{
91-
"foo": &Attribute{
91+
"foo": {
9292
Type: cty.String,
9393
Required: true,
9494
Optional: true,
@@ -100,7 +100,7 @@ func TestBlockInternalValidate(t *testing.T) {
100100
"attribute required and computed": {
101101
&Block{
102102
Attributes: map[string]*Attribute{
103-
"foo": &Attribute{
103+
"foo": {
104104
Type: cty.String,
105105
Required: true,
106106
Computed: true,
@@ -112,7 +112,7 @@ func TestBlockInternalValidate(t *testing.T) {
112112
"attribute optional and computed": {
113113
&Block{
114114
Attributes: map[string]*Attribute{
115-
"foo": &Attribute{
115+
"foo": {
116116
Type: cty.String,
117117
Optional: true,
118118
Computed: true,
@@ -124,28 +124,63 @@ func TestBlockInternalValidate(t *testing.T) {
124124
"attribute with missing type": {
125125
&Block{
126126
Attributes: map[string]*Attribute{
127-
"foo": &Attribute{
127+
"foo": {
128128
Optional: true,
129129
},
130130
},
131131
},
132-
[]string{"foo: Type must be set to something other than cty.NilType"},
132+
[]string{"foo: either Type or NestedType must be defined"},
133133
},
134134
"attribute with invalid name": {
135135
&Block{
136136
Attributes: map[string]*Attribute{
137-
"fooBar": &Attribute{
137+
"fooBar": {
138138
Type: cty.String,
139139
Optional: true,
140140
},
141141
},
142142
},
143143
[]string{"fooBar: name may contain only lowercase letters, digits and underscores"},
144144
},
145+
"attribute with invalid NestedType nesting": {
146+
&Block{
147+
Attributes: map[string]*Attribute{
148+
"foo": {
149+
NestedType: &Object{
150+
Nesting: NestingSingle,
151+
MinItems: 10,
152+
MaxItems: 10,
153+
},
154+
Optional: true,
155+
},
156+
},
157+
},
158+
[]string{"foo: MinItems and MaxItems must be set to either 0 or 1 in NestingSingle mode"},
159+
},
160+
"attribute with invalid NestedType attribute": {
161+
&Block{
162+
Attributes: map[string]*Attribute{
163+
"foo": {
164+
NestedType: &Object{
165+
Nesting: NestingSingle,
166+
Attributes: map[string]*Attribute{
167+
"foo": {
168+
Type: cty.String,
169+
Required: true,
170+
Optional: true,
171+
},
172+
},
173+
},
174+
Optional: true,
175+
},
176+
},
177+
},
178+
[]string{"foo: cannot set both Optional and Required"},
179+
},
145180
"block type with invalid name": {
146181
&Block{
147182
BlockTypes: map[string]*NestedBlock{
148-
"fooBar": &NestedBlock{
183+
"fooBar": {
149184
Nesting: NestingSingle,
150185
},
151186
},
@@ -155,13 +190,13 @@ func TestBlockInternalValidate(t *testing.T) {
155190
"colliding names": {
156191
&Block{
157192
Attributes: map[string]*Attribute{
158-
"foo": &Attribute{
193+
"foo": {
159194
Type: cty.String,
160195
Optional: true,
161196
},
162197
},
163198
BlockTypes: map[string]*NestedBlock{
164-
"foo": &NestedBlock{
199+
"foo": {
165200
Nesting: NestingSingle,
166201
},
167202
},
@@ -171,11 +206,11 @@ func TestBlockInternalValidate(t *testing.T) {
171206
"nested block with badness": {
172207
&Block{
173208
BlockTypes: map[string]*NestedBlock{
174-
"bad": &NestedBlock{
209+
"bad": {
175210
Nesting: NestingSingle,
176211
Block: Block{
177212
Attributes: map[string]*Attribute{
178-
"nested_bad": &Attribute{
213+
"nested_bad": {
179214
Type: cty.String,
180215
Required: true,
181216
Optional: true,
@@ -190,11 +225,11 @@ func TestBlockInternalValidate(t *testing.T) {
190225
"nested list block with dynamically-typed attribute": {
191226
&Block{
192227
BlockTypes: map[string]*NestedBlock{
193-
"bad": &NestedBlock{
228+
"bad": {
194229
Nesting: NestingList,
195230
Block: Block{
196231
Attributes: map[string]*Attribute{
197-
"nested_bad": &Attribute{
232+
"nested_bad": {
198233
Type: cty.DynamicPseudoType,
199234
Optional: true,
200235
},
@@ -208,11 +243,11 @@ func TestBlockInternalValidate(t *testing.T) {
208243
"nested set block with dynamically-typed attribute": {
209244
&Block{
210245
BlockTypes: map[string]*NestedBlock{
211-
"bad": &NestedBlock{
246+
"bad": {
212247
Nesting: NestingSet,
213248
Block: Block{
214249
Attributes: map[string]*Attribute{
215-
"nested_bad": &Attribute{
250+
"nested_bad": {
216251
Type: cty.DynamicPseudoType,
217252
Optional: true,
218253
},
@@ -253,11 +288,12 @@ func TestBlockInternalValidate(t *testing.T) {
253288
for _, err := range errs {
254289
t.Logf("- %s", err.Error())
255290
}
256-
}
257-
if len(errs) > 0 {
258-
for i := range errs {
259-
if errs[i].Error() != test.Errs[i] {
260-
t.Errorf("wrong error: got %s, want %s", errs[i].Error(), test.Errs[i])
291+
} else {
292+
if len(errs) > 0 {
293+
for i := range errs {
294+
if errs[i].Error() != test.Errs[i] {
295+
t.Errorf("wrong error: got %s, want %s", errs[i].Error(), test.Errs[i])
296+
}
261297
}
262298
}
263299
}

0 commit comments

Comments
 (0)
⚡