Skip to content

Commit 3843b3f

Browse files
committed
feat: add MustSafecast helper for type conversions
Adds MustSafecast function following the MustBugf pattern: - Panics in tests when conversion fails - Returns zero value in production with warning log - Replaces manual safecast error handling across codebase Closes #2714
1 parent b55a9f6 commit 3843b3f

File tree

9 files changed

+103
-47
lines changed

9 files changed

+103
-47
lines changed

.gitignore

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,5 @@ coverage*.txt
99
.claude/
1010
e2e/newenemy/spicedb
1111
e2e/newenemy/cockroach
12-
e2e/newenemy/chaosd
12+
e2e/newenemy/chaosd
13+
__pycache__/

internal/datastore/crdb/pool/pool.go

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"sync"
99
"time"
1010

11-
"github.com/ccoveille/go-safecast/v2"
1211
"github.com/jackc/pgx/v5"
1312
"github.com/jackc/pgx/v5/pgconn"
1413
"github.com/jackc/pgx/v5/pgxpool"
@@ -17,6 +16,7 @@ import (
1716

1817
"github.com/authzed/spicedb/internal/datastore/postgres/common"
1918
log "github.com/authzed/spicedb/internal/logging"
19+
"github.com/authzed/spicedb/pkg/spiceerrors"
2020
)
2121

2222
// pgxPool interface is the subset of pgxpool.Pool that RetryPool needs
@@ -156,21 +156,13 @@ func (p *RetryPool) ID() string {
156156
// MaxConns returns the MaxConns configured on the underlying pool
157157
func (p *RetryPool) MaxConns() uint32 {
158158
// This should be non-negative
159-
maxConns, err := safecast.Convert[uint32](p.pool.Config().MaxConns)
160-
if err != nil {
161-
maxConns = 0
162-
}
163-
return maxConns
159+
return spiceerrors.MustSafecast[uint32](p.pool.Config().MaxConns)
164160
}
165161

166162
// MinConns returns the MinConns configured on the underlying pool
167163
func (p *RetryPool) MinConns() uint32 {
168164
// This should be non-negative
169-
minConns, err := safecast.Convert[uint32](p.pool.Config().MinConns)
170-
if err != nil {
171-
minConns = 0
172-
}
173-
return minConns
165+
return spiceerrors.MustSafecast[uint32](p.pool.Config().MinConns)
174166
}
175167

176168
// ExecFunc is a replacement for pgxpool.pgxPool.Exec that allows resetting the

internal/datastore/postgres/snapshot.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ import (
77
"strconv"
88
"strings"
99

10-
"github.com/ccoveille/go-safecast/v2"
1110
"github.com/jackc/pgx/v5/pgtype"
11+
12+
"github.com/authzed/spicedb/pkg/spiceerrors"
1213
)
1314

1415
// RegisterTypes registers pgSnapshot and xid8 with a pgtype.ConnInfo.
@@ -279,10 +280,7 @@ func (s pgSnapshot) markInProgress(txid uint64) pgSnapshot {
279280
startingXipLen := len(newSnapshot.xipList)
280281
for numToDrop = 0; numToDrop < startingXipLen; numToDrop++ {
281282
// numToDrop should be nonnegative
282-
uintNumToDrop, err := safecast.Convert[uint64](numToDrop)
283-
if err != nil {
284-
uintNumToDrop = 0
285-
}
283+
uintNumToDrop := spiceerrors.MustSafecast[uint64](numToDrop)
286284

287285
if newSnapshot.xipList[startingXipLen-1-numToDrop] != newSnapshot.xmax-uintNumToDrop-1 {
288286
break

internal/developmentmembership/onrset.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
package developmentmembership
22

33
import (
4-
"github.com/ccoveille/go-safecast/v2"
5-
64
"github.com/authzed/spicedb/pkg/genutil/mapz"
5+
"github.com/authzed/spicedb/pkg/spiceerrors"
76
"github.com/authzed/spicedb/pkg/tuple"
87
)
98

@@ -26,11 +25,7 @@ func NewONRSet(onrs ...tuple.ObjectAndRelation) ONRSet {
2625
// Length returns the size of the set.
2726
func (ons ONRSet) Length() uint64 {
2827
// This is the length of a set so we should never fall out of bounds.
29-
length, err := safecast.Convert[uint64](ons.onrs.Len())
30-
if err != nil {
31-
return 0
32-
}
33-
return length
28+
return spiceerrors.MustSafecast[uint64](ons.onrs.Len())
3429
}
3530

3631
// IsEmpty returns whether the set is empty.

pkg/cache/cache.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ package cache
33
import (
44
"time"
55

6-
"github.com/ccoveille/go-safecast/v2"
76
"github.com/dustin/go-humanize"
87
"github.com/rs/zerolog"
8+
9+
"github.com/authzed/spicedb/pkg/spiceerrors"
910
)
1011

1112
// KeyString is an interface for keys that can be converted to strings.
@@ -53,10 +54,7 @@ type Config struct {
5354
}
5455

5556
func (c *Config) MarshalZerologObject(e *zerolog.Event) {
56-
maxCost, err := safecast.Convert[uint64](c.MaxCost)
57-
if err != nil {
58-
maxCost = 0
59-
}
57+
maxCost := spiceerrors.MustSafecast[uint64](c.MaxCost)
6058
e.
6159
Str("maxCost", humanize.IBytes(maxCost)).
6260
Int64("numCounters", c.NumCounters).

pkg/composableschemadsl/compiler/translator.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"slices"
1010
"strings"
1111

12-
"github.com/ccoveille/go-safecast/v2"
1312
"github.com/jzelinskie/stringz"
1413
"github.com/rs/zerolog/log"
1514

@@ -20,6 +19,7 @@ import (
2019
"github.com/authzed/spicedb/pkg/genutil/mapz"
2120
"github.com/authzed/spicedb/pkg/namespace"
2221
core "github.com/authzed/spicedb/pkg/proto/core/v1"
22+
"github.com/authzed/spicedb/pkg/spiceerrors"
2323
)
2424

2525
type translationContext struct {
@@ -334,14 +334,8 @@ func getSourcePosition(dslNode *dslNode, mapper input.PositionMapper) *core.Sour
334334
return nil
335335
}
336336

337-
uintLine, err := safecast.Convert[uint64](line)
338-
if err != nil {
339-
uintLine = 0
340-
}
341-
uintCol, err := safecast.Convert[uint64](col)
342-
if err != nil {
343-
uintCol = 0
344-
}
337+
uintLine := spiceerrors.MustSafecast[uint64](line)
338+
uintCol := spiceerrors.MustSafecast[uint64](col)
345339

346340
return &core.SourcePosition{
347341
ZeroIndexedLineNumber: uintLine,

pkg/schemadsl/compiler/translator.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"slices"
88
"strings"
99

10-
"github.com/ccoveille/go-safecast/v2"
1110
"github.com/jzelinskie/stringz"
1211

1312
"github.com/authzed/spicedb/pkg/caveats"
@@ -16,6 +15,7 @@ import (
1615
core "github.com/authzed/spicedb/pkg/proto/core/v1"
1716
"github.com/authzed/spicedb/pkg/schemadsl/dslshape"
1817
"github.com/authzed/spicedb/pkg/schemadsl/input"
18+
"github.com/authzed/spicedb/pkg/spiceerrors"
1919
)
2020

2121
type translationContext struct {
@@ -287,14 +287,8 @@ func getSourcePosition(dslNode *dslNode, mapper input.PositionMapper) *core.Sour
287287
return nil
288288
}
289289

290-
uintLine, err := safecast.Convert[uint64](line)
291-
if err != nil {
292-
uintLine = 0
293-
}
294-
uintCol, err := safecast.Convert[uint64](col)
295-
if err != nil {
296-
uintCol = 0
297-
}
290+
uintLine := spiceerrors.MustSafecast[uint64](line)
291+
uintCol := spiceerrors.MustSafecast[uint64](col)
298292

299293
return &core.SourcePosition{
300294
ZeroIndexedLineNumber: uintLine,

pkg/spiceerrors/bug.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@ import (
55
"os"
66
"strings"
77

8+
"github.com/ccoveille/go-safecast/v2"
89
"github.com/go-errors/errors"
10+
11+
log "github.com/authzed/spicedb/internal/logging"
912
)
1013

1114
// IsInTests returns true if go test is running
@@ -33,3 +36,27 @@ func MustBugf(format string, args ...any) error {
3336
e := errors.Errorf(format, args...)
3437
return fmt.Errorf("BUG: %s", e.ErrorStack())
3538
}
39+
40+
// MustSafecast converts a value from one numeric type to another using safecast.
41+
// If the conversion fails (value out of range), it panics in tests and returns
42+
// the zero value in production. This should only be used where the value is
43+
// expected to always be convertible (e.g., converting from a statically defined
44+
// value or a value known to be non-negative).
45+
func MustSafecast[To, From safecast.Number](from From) To {
46+
result, err := safecast.Convert[To](from)
47+
if err != nil {
48+
if IsInTests() {
49+
panic(fmt.Sprintf("safecast conversion failed: %v (from %v to %T)", err, from, result))
50+
}
51+
// In production, log a warning and return the zero value
52+
var zero To
53+
log.Warn().
54+
Interface("from_value", from).
55+
Str("from_type", fmt.Sprintf("%T", from)).
56+
Str("to_type", fmt.Sprintf("%T", zero)).
57+
Err(err).
58+
Msg("MustSafecast conversion failed in production, returning zero value")
59+
return zero
60+
}
61+
return result
62+
}

pkg/spiceerrors/bug_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package spiceerrors
22

33
import (
4+
"os"
45
"testing"
56

67
"github.com/stretchr/testify/assert"
@@ -14,3 +15,59 @@ func TestMustBug(t *testing.T) {
1415
require.Error(t, err)
1516
}, "The code did not panic")
1617
}
18+
19+
func TestMustSafecast(t *testing.T) {
20+
require.True(t, IsInTests())
21+
22+
// Test successful conversion
23+
t.Run("successful conversion", func(t *testing.T) {
24+
result := MustSafecast[uint64](42)
25+
assert.Equal(t, uint64(42), result)
26+
27+
result2 := MustSafecast[int32](100)
28+
assert.Equal(t, int32(100), result2)
29+
})
30+
31+
// Test that conversion failure panics in tests
32+
t.Run("conversion failure panics in tests", func(t *testing.T) {
33+
assert.Panics(t, func() {
34+
// Try to convert a negative number to unsigned
35+
MustSafecast[uint64](-1)
36+
}, "Expected panic on invalid conversion")
37+
})
38+
39+
// Test conversion from larger to smaller type that fits
40+
t.Run("conversion within range", func(t *testing.T) {
41+
result := MustSafecast[uint32](uint64(100))
42+
assert.Equal(t, uint32(100), result)
43+
})
44+
45+
// Test production behavior (returns zero value without panic)
46+
t.Run("production behavior returns zero on failure", func(t *testing.T) {
47+
// Temporarily simulate production by removing test flags from os.Args
48+
originalArgs := os.Args
49+
defer func() {
50+
os.Args = originalArgs
51+
}()
52+
53+
// Remove all -test.* flags to simulate production
54+
var nonTestArgs []string
55+
for _, arg := range os.Args {
56+
if len(arg) < 6 || arg[:6] != "-test." {
57+
nonTestArgs = append(nonTestArgs, arg)
58+
}
59+
}
60+
os.Args = nonTestArgs
61+
62+
// Verify we're now simulating production
63+
require.False(t, IsInTests(), "Should simulate production mode")
64+
65+
// Test that conversion failure returns zero in production
66+
result := MustSafecast[uint64](-1)
67+
assert.Equal(t, uint64(0), result, "Expected zero value in production mode")
68+
69+
// Test overflow case
70+
result2 := MustSafecast[uint8](300)
71+
assert.Equal(t, uint8(0), result2, "Expected zero value for overflow in production mode")
72+
})
73+
}

0 commit comments

Comments
 (0)