Skip to content

Commit 38190dd

Browse files
bfladbflad
authored
tftypes: Return errors instead of panics with Value missing type in Transform() and (Value).Diff() (#104)
Reference: #92 Previously: ``` --- FAIL: TestTransform (0.00s) --- FAIL: TestTransform/testCase=empty:missingTypeError (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x11c0409] goroutine 45 [running]: testing.tRunner.func1.2({0x129d200, 0x14bb030}) /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1209 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1212 +0x218 panic({0x129d200, 0x14bb030}) /usr/local/Cellar/go/1.17/libexec/src/runtime/panic.go:1038 +0x215 github.com/hashicorp/terraform-plugin-go/tftypes.transform(0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/walk.go:179 +0x689 github.com/hashicorp/terraform-plugin-go/tftypes.Transform(...) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/walk.go:112 github.com/hashicorp/terraform-plugin-go/tftypes.TestTransform.func51(0xc0001d89c0) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/walk_test.go:1659 +0xcc testing.tRunner(0xc0001d89c0, 0xc000010a00) /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1259 +0x102 created by testing.(*T).Run /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1306 +0x35a FAIL github.com/hashicorp/terraform-plugin-go/tftypes 1.821s ``` Added the missing type exits to `transform()` and added `(AttributePathError).Error()` to fix go-cmp testing with that type's unexported `err` field. This then discovered `(Value).Diff()` can exhibit a similar panic due to missing type information: ``` --- FAIL: TestTransform (0.00s) --- FAIL: TestTransform/testCase=empty:missingTypeError (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11a400e] goroutine 66 [running]: testing.tRunner.func1.2({0x129ec80, 0x14bf030}) /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1209 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1212 +0x218 panic({0x129ec80, 0x14bf030}) /usr/local/Cellar/go/1.17/libexec/src/runtime/panic.go:1038 +0x215 github.com/hashicorp/terraform-plugin-go/tftypes.Value.Diff({{0x0, 0x0}, {0x0, 0x0}}, {{0x0, 0x0}, {0x0, 0x0}}) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/diff.go:78 +0x8e github.com/hashicorp/terraform-plugin-go/tftypes.TestTransform.func51(0xc000209380) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/walk_test.go:1674 +0x1c5 testing.tRunner(0xc000209380, 0xc000103b60) ``` Covering `TestValueDiffDiff` panics before fix: ``` --- FAIL: TestValueDiffDiff (0.00s) --- FAIL: TestValueDiffDiff/val1Empty (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11a400e] goroutine 21 [running]: testing.tRunner.func1.2({0x129ec80, 0x14bf030}) /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1209 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.17/libexec/src/testing/testing.go:1212 +0x218 panic({0x129ec80, 0x14bf030}) /usr/local/Cellar/go/1.17/libexec/src/runtime/panic.go:1038 +0x215 github.com/hashicorp/terraform-plugin-go/tftypes.Value.Diff({{0x0, 0x0}, {0x0, 0x0}}, {{0x135dea0, 0xc0000a2630}, {0x128e7c0, 0xc00009e840}}) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/diff.go:78 +0x8e github.com/hashicorp/terraform-plugin-go/tftypes.TestValueDiffDiff.func1(0xc000083ba0) /Users/bflad/src/github.com/hashicorp/terraform-plugin-go/tftypes/diff_test.go:554 +0x6b testing.tRunner(0xc000083ba0, 0xc0000dc9a0) ```
1 parent 657313a commit 38190dd

File tree

8 files changed

+923
-120
lines changed

8 files changed

+923
-120
lines changed

‎.changelog/104.txt

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
```release-note:bug
2+
tftypes: Return error instead of panic when calling `Transform()` with `Value` missing type
3+
```
4+
5+
```release-note:bug
6+
tftypes: Return error instead of panic when calling `(Value).Diff()` with either `Value` missing type
7+
```

‎tftypes/attribute_path_error.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,22 @@ type AttributePathError struct {
1111
err error
1212
}
1313

14+
func (a AttributePathError) Equal(o AttributePathError) bool {
15+
if !a.Path.Equal(o.Path) {
16+
return false
17+
}
18+
19+
if (a.err == nil && o.err != nil) || (a.err != nil && o.err == nil) {
20+
return false
21+
}
22+
23+
if a.err == nil {
24+
return true
25+
}
26+
27+
return a.err.Error() == o.err.Error()
28+
}
29+
1430
func (a AttributePathError) Error() string {
1531
var path string
1632
if len(a.Path.Steps()) > 0 {

‎tftypes/attribute_path_error_test.go

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package tftypes
2+
3+
import (
4+
"errors"
5+
"testing"
6+
7+
"github.com/google/go-cmp/cmp"
8+
)
9+
10+
func TestAttributePathErrorEqual(t *testing.T) {
11+
t.Parallel()
12+
13+
testCases := map[string]struct {
14+
ape AttributePathError
15+
other AttributePathError
16+
expected bool
17+
}{
18+
"equal": {
19+
ape: AttributePathError{
20+
Path: NewAttributePath().WithAttributeName("test"),
21+
err: errors.New("test error"),
22+
},
23+
other: AttributePathError{
24+
Path: NewAttributePath().WithAttributeName("test"),
25+
err: errors.New("test error"),
26+
},
27+
expected: true,
28+
},
29+
"err-different": {
30+
ape: AttributePathError{
31+
Path: NewAttributePath().WithAttributeName("test"),
32+
err: errors.New("test error"),
33+
},
34+
other: AttributePathError{
35+
Path: NewAttributePath().WithAttributeName("test"),
36+
err: errors.New("different error"),
37+
},
38+
expected: false,
39+
},
40+
"err-nil": {
41+
ape: AttributePathError{
42+
Path: NewAttributePath().WithAttributeName("test"),
43+
err: nil,
44+
},
45+
other: AttributePathError{
46+
Path: NewAttributePath().WithAttributeName("test"),
47+
err: nil,
48+
},
49+
expected: true,
50+
},
51+
"err-nil-ape": {
52+
ape: AttributePathError{
53+
Path: NewAttributePath().WithAttributeName("test"),
54+
err: nil,
55+
},
56+
other: AttributePathError{
57+
Path: NewAttributePath().WithAttributeName("test"),
58+
err: errors.New("test error"),
59+
},
60+
expected: false,
61+
},
62+
"err-nil-other": {
63+
ape: AttributePathError{
64+
Path: NewAttributePath().WithAttributeName("test"),
65+
err: errors.New("test error"),
66+
},
67+
other: AttributePathError{
68+
Path: NewAttributePath().WithAttributeName("test"),
69+
err: nil,
70+
},
71+
expected: false,
72+
},
73+
"path-different": {
74+
ape: AttributePathError{
75+
Path: NewAttributePath().WithAttributeName("test"),
76+
err: errors.New("test error"),
77+
},
78+
other: AttributePathError{
79+
Path: NewAttributePath().WithAttributeName("different"),
80+
err: errors.New("test error"),
81+
},
82+
expected: false,
83+
},
84+
}
85+
86+
for name, testCase := range testCases {
87+
name, testCase := name, testCase
88+
t.Run(name, func(t *testing.T) {
89+
t.Parallel()
90+
91+
got := testCase.ape.Equal(testCase.other)
92+
93+
if diff := cmp.Diff(testCase.expected, got); diff != "" {
94+
t.Errorf("Unexpected results (-wanted +got): %s", diff)
95+
}
96+
})
97+
}
98+
}

‎tftypes/diff.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,22 @@ func (v ValueDiff) Equal(o ValueDiff) bool {
7171
// a slice of ValueDiffs. The ValueDiffs in the struct will use `val1`'s values
7272
// as Value1 and `val2`'s values as Value2. An empty or nil slice means the two
7373
// Values can be considered equal. Values must be the same type when passed to
74-
// Diff; passing in Values of two different types will result in an error.
75-
// val1.Type().Is(val2.Type()) is a safe way to check that Values can be
76-
// compared with Diff.
74+
// Diff; passing in Values of two different types will result in an error. If
75+
// both Values are empty, they are considered equal. If one Value is missing
76+
// type, it will result in an error. val1.Type().Is(val2.Type()) is a safe way
77+
// to check that Values can be compared with Diff.
7778
func (val1 Value) Diff(val2 Value) ([]ValueDiff, error) {
79+
var diffs []ValueDiff
80+
81+
if val1.Type() == nil && val2.Type() == nil && val1.value == nil && val2.value == nil {
82+
return diffs, nil
83+
}
84+
if (val1.Type() == nil && val2.Type() != nil) || (val1.Type() != nil && val2.Type() == nil) {
85+
return nil, errors.New("cannot diff value missing type")
86+
}
7887
if !val1.Type().Is(val2.Type()) {
7988
return nil, errors.New("Can't diff values of different types")
8089
}
81-
var diffs []ValueDiff
8290

8391
// make sure everything in val2 is also in val1
8492
err := Walk(val2, func(path *AttributePath, value2 Value) (bool, error) {

‎tftypes/diff_test.go

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,65 @@ func TestValueDiffEqual(t *testing.T) {
351351
},
352352
equal: true,
353353
},
354+
"val1EmptyEqual": {
355+
diff1: ValueDiff{
356+
Path: NewAttributePath().WithElementKeyString("foo"),
357+
Value1: &Value{},
358+
Value2: valuePointer(NewValue(List{
359+
ElementType: Bool,
360+
}, []Value{
361+
NewValue(Bool, true),
362+
NewValue(Bool, false),
363+
})),
364+
},
365+
diff2: ValueDiff{
366+
Path: NewAttributePath().WithElementKeyString("foo"),
367+
Value1: &Value{},
368+
Value2: valuePointer(NewValue(List{
369+
ElementType: Bool,
370+
}, []Value{
371+
NewValue(Bool, true),
372+
NewValue(Bool, false),
373+
})),
374+
},
375+
equal: true,
376+
},
377+
"val2EmptyEqual": {
378+
diff1: ValueDiff{
379+
Path: NewAttributePath().WithElementKeyString("foo"),
380+
Value1: valuePointer(NewValue(List{
381+
ElementType: Bool,
382+
}, []Value{
383+
NewValue(Bool, false),
384+
NewValue(Bool, true),
385+
})),
386+
Value2: &Value{},
387+
},
388+
diff2: ValueDiff{
389+
Path: NewAttributePath().WithElementKeyString("foo"),
390+
Value1: valuePointer(NewValue(List{
391+
ElementType: Bool,
392+
}, []Value{
393+
NewValue(Bool, false),
394+
NewValue(Bool, true),
395+
})),
396+
Value2: &Value{},
397+
},
398+
equal: true,
399+
},
400+
"allValsEmptyEqual": {
401+
diff1: ValueDiff{
402+
Path: NewAttributePath().WithElementKeyString("foo"),
403+
Value1: &Value{},
404+
Value2: &Value{},
405+
},
406+
diff2: ValueDiff{
407+
Path: NewAttributePath().WithElementKeyString("foo"),
408+
Value1: &Value{},
409+
Value2: &Value{},
410+
},
411+
equal: true,
412+
},
354413
}
355414
for name, test := range tests {
356415
name, test := name, test
@@ -378,6 +437,30 @@ func TestValueDiffDiff(t *testing.T) {
378437
}
379438

380439
tests := map[string]testCase{
440+
"val1Empty": {
441+
val1: Value{},
442+
val2: NewValue(String, "bar"),
443+
err: errors.New("cannot diff value missing type"),
444+
},
445+
"val1EmptyTypeWithValue": {
446+
val1: Value{value: "foo"},
447+
val2: NewValue(String, "bar"),
448+
err: errors.New("cannot diff value missing type"),
449+
},
450+
"val2Empty": {
451+
val1: NewValue(String, "foo"),
452+
val2: Value{},
453+
err: errors.New("cannot diff value missing type"),
454+
},
455+
"val2EmptyTypeWithValue": {
456+
val1: NewValue(String, "foo"),
457+
val2: Value{value: "bar"},
458+
err: errors.New("cannot diff value missing type"),
459+
},
460+
"valsEmptyNoDiff": {
461+
val1: Value{},
462+
val2: Value{},
463+
},
381464
"primitiveDiff": {
382465
val1: NewValue(String, "foo"),
383466
val2: NewValue(String, "bar"),

‎tftypes/object.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,9 @@ func valueFromObject(types map[string]Type, optionalAttrs map[string]struct{}, i
180180
if !ok {
181181
return Value{}, fmt.Errorf("can't set a value on %q in tftypes.NewValue, key not part of the object type %s", k, Object{AttributeTypes: types})
182182
}
183+
if v.Type() == nil {
184+
return Value{}, NewAttributePath().WithAttributeName(k).NewErrorf("missing value type")
185+
}
183186
if v.Type().Is(DynamicPseudoType) && v.IsKnown() {
184187
return Value{}, NewAttributePath().WithAttributeName(k).NewErrorf("invalid value %s for %s", v, v.Type())
185188
} else if !v.Type().Is(DynamicPseudoType) && !v.Type().UsableAs(typ) {

‎tftypes/walk.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,10 @@ func transform(path *AttributePath, val Value, cb func(*AttributePath, Value) (V
116116
var newVal Value
117117
ty := val.Type()
118118

119+
if ty == nil {
120+
return val, path.NewError(errors.New("invalid transform: value missing type"))
121+
}
122+
119123
switch {
120124
case val.IsNull() || !val.IsKnown():
121125
newVal = val
@@ -142,7 +146,10 @@ func transform(path *AttributePath, val Value, cb func(*AttributePath, Value) (V
142146
elems = append(elems, newEl)
143147
path = path.WithoutLastStep()
144148
}
145-
newVal = NewValue(ty, elems)
149+
newVal, err = newValue(ty, elems)
150+
if err != nil {
151+
return val, path.NewError(err)
152+
}
146153
}
147154
case ty.Is(Map{}), ty.Is(Object{}):
148155
v := map[string]Value{}
@@ -167,7 +174,10 @@ func transform(path *AttributePath, val Value, cb func(*AttributePath, Value) (V
167174
elems[k] = newEl
168175
path = path.WithoutLastStep()
169176
}
170-
newVal = NewValue(ty, elems)
177+
newVal, err = newValue(ty, elems)
178+
if err != nil {
179+
return val, path.NewError(err)
180+
}
171181
}
172182
default:
173183
newVal = val
@@ -176,7 +186,11 @@ func transform(path *AttributePath, val Value, cb func(*AttributePath, Value) (V
176186
if err != nil {
177187
return res, path.NewError(err)
178188
}
179-
if !newVal.Type().UsableAs(ty) {
189+
newTy := newVal.Type()
190+
if newTy == nil {
191+
return val, path.NewError(errors.New("invalid transform: new value missing type"))
192+
}
193+
if !newTy.UsableAs(ty) {
180194
return val, path.NewError(errors.New("invalid transform: value changed type"))
181195
}
182196
return res, err

0 commit comments

Comments
 (0)