Skip to content

Commit 2faa7ed

Browse files
authored
fix: add comma-ok guards for type assertion panics (#30)
* fix: add comma-ok guards to prevent type assertion panics in plan execution plan.go has two unsafe type assertions on reflect.Value outputs that could panic if the second return value is not an error type. Although IsValidBuilder validates at registration time, this adds defense-in-depth comma-ok guards at both processWork (line 144) and doWorkAndGetResult (line 182). * fix: use %T instead of %v in comma-ok error messages, fix indentation Address review feedback: - Use type-only (%T) instead of value (%v) to avoid leaking sensitive data or bloating output in the defense-in-depth error path - Store Interface() result once to avoid double call - Fix indentation in doWorkAndGetResult comma-ok block * fix: typo "occured" → "occurred", use assert.ErrorContains
1 parent b8d8f57 commit 2faa7ed

3 files changed

Lines changed: 44 additions & 9 deletions

File tree

README.md

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import "github.com/go-coldbrew/data-builder"
1515

1616
## Index
1717

18+
- [Constants](<#constants>)
1819
- [Variables](<#variables>)
1920
- [func AddResultToCtx\(ctx context.Context, r Result\) context.Context](<#AddResultToCtx>)
2021
- [func BuildGraph\(executionPlan Plan, format, file string\) error](<#BuildGraph>)
@@ -29,6 +30,14 @@ import "github.com/go-coldbrew/data-builder"
2930
- [func \(r Result\) Get\(obj any\) any](<#Result.Get>)
3031

3132

33+
## Constants
34+
35+
<a name="SupportPackageIsVersion1"></a>SupportPackageIsVersion1 is a compile\-time assertion constant. Downstream packages reference this to enforce version compatibility.
36+
37+
```go
38+
const SupportPackageIsVersion1 = true
39+
```
40+
3241
## Variables
3342

3443
<a name="ErrInvalidBuilder"></a>
@@ -80,7 +89,7 @@ AddResultToCtx adds the given result object to context
8089
this function should ideally only be used in your tests and/or for debugging modification made to Result obj will NOT persist
8190

8291
<a name="BuildGraph"></a>
83-
## func [BuildGraph](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L305>)
92+
## func [BuildGraph](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L319>)
8493

8594
```go
8695
func BuildGraph(executionPlan Plan, format, file string) error
@@ -100,7 +109,7 @@ GetFromResult allows builders to access data built by other builders
100109
this function enables optional access to data, your code should not rely on values being present, if you have explicit dependency please add them to your function parameters
101110

102111
<a name="IsValidBuilder"></a>
103-
## func [IsValidBuilder](<https://github.com/go-coldbrew/data-builder/blob/main/databuilder.go#L90>)
112+
## func [IsValidBuilder](<https://github.com/go-coldbrew/data-builder/blob/main/databuilder.go#L94>)
104113

105114
```go
106115
func IsValidBuilder(builder any) error
@@ -109,7 +118,7 @@ func IsValidBuilder(builder any) error
109118
IsValidBuilder checks if the given function is valid or not
110119

111120
<a name="MaxPlanParallelism"></a>
112-
## func [MaxPlanParallelism](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L317>)
121+
## func [MaxPlanParallelism](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L331>)
113122

114123
```go
115124
func MaxPlanParallelism(pl Plan) (uint, error)
@@ -284,7 +293,7 @@ welcome to singapore
284293
</details>
285294

286295
<a name="New"></a>
287-
### func [New](<https://github.com/go-coldbrew/data-builder/blob/main/databuilder.go#L174>)
296+
### func [New](<https://github.com/go-coldbrew/data-builder/blob/main/databuilder.go#L178>)
288297

289298
```go
290299
func New() DataBuilder
@@ -378,7 +387,7 @@ GetResultFromCtx gives access to result object at this point in execution
378387
this function should ideally only be used in your tests and/or for debugging modification made to Result obj may or may not persist
379388

380389
<a name="Result.Get"></a>
381-
### func \(Result\) [Get](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L233>)
390+
### func \(Result\) [Get](<https://github.com/go-coldbrew/data-builder/blob/main/plan.go#L247>)
382391

383392
```go
384393
func (r Result) Get(obj any) any

plan.go

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package databuilder
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"reflect"
78
"strconv"
89
"sync"
@@ -141,7 +142,12 @@ func processWork(ctx context.Context, w work) {
141142
}
142143
o.outputs = fn.Call(args)
143144
if len(o.outputs) > 1 && !o.outputs[1].IsNil() {
144-
span.SetError(o.outputs[1].Interface().(error)) // nolint: errcheck
145+
secondReturn := o.outputs[1].Interface()
146+
if errVal, ok := secondReturn.(error); ok {
147+
span.SetError(errVal) //nolint:errcheck
148+
} else {
149+
span.SetError(fmt.Errorf("builder %s: second return value is not an error (type %T)", w.builder.Name, secondReturn)) //nolint:errcheck
150+
}
145151
}
146152
w.out <- o
147153
}
@@ -171,15 +177,20 @@ func doWorkAndGetResult(ctx context.Context, builders []*builder, dataMap map[st
171177
errs := make([]error, 0)
172178
for o := range outChan {
173179
if o.err != nil {
174-
// error occured, return it back and stop processing
180+
// error occurred, return it back and stop processing
175181
return o.err
176182
}
177183
outputs := o.outputs
178184
// we should only ever have two outputs
179185
// 0-> data, 1-> error
180186
if !outputs[1].IsNil() {
181-
// error occured, add it to the list of errors and continue processing
182-
errs = append(errs, outputs[1].Interface().(error))
187+
// error occurred, add it to the list of errors and continue processing
188+
secondReturn := outputs[1].Interface()
189+
if errVal, ok := secondReturn.(error); ok {
190+
errs = append(errs, errVal)
191+
} else {
192+
errs = append(errs, fmt.Errorf("builder %s: second return value is not an error (type %T)", o.builder.Name, secondReturn))
193+
}
183194
continue
184195
}
185196
// add result

plan_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,21 @@ func TestPlanRunPartialSuccess(t *testing.T) {
9494
goleak.VerifyNone(t)
9595
}
9696

97+
func TestPlanRun_ErrorFlowWithCommaOk(t *testing.T) {
98+
// Verify that the comma-ok type assertion guards in processWork and
99+
// doWorkAndGetResult correctly propagate errors from builders.
100+
d := testNew(t)
101+
err := d.AddBuilders(DBTestFuncErr)
102+
assert.NoError(t, err)
103+
executionPlan, err := d.Compile(TestStruct1{})
104+
assert.NotNil(t, executionPlan)
105+
assert.NoError(t, err)
106+
107+
_, err = executionPlan.Run(context.Background(), TestStruct1{Value: "test"})
108+
assert.ErrorContains(t, err, "encountered an error")
109+
goleak.VerifyNone(t)
110+
}
111+
97112
func ExamplePlan() {
98113
b := New()
99114
err := b.AddBuilders(DBTestFunc, DBTestFunc4)

0 commit comments

Comments
 (0)