Skip to content

Commit 8136af2

Browse files
gabygaby
authored
🧹 chore: Improve error handling when using storage drivers (#3754)
* Add error propagation tests for storage-backed middlewares * Address lint issues in middleware error handling tests * Handle cache misses without nolint directives * Refactor cache miss handling to reduce nesting
1 parent 1471049 commit 8136af2

File tree

13 files changed

+714
-127
lines changed

13 files changed

+714
-127
lines changed

‎middleware/cache/cache.go‎

Lines changed: 49 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ package cache
44

55
import (
66
"context"
7+
"errors"
8+
"fmt"
79
"slices"
810
"strconv"
911
"strings"
@@ -98,12 +100,17 @@ func New(config ...Config) fiber.Handler {
98100
}()
99101

100102
// Delete key from both manager and storage
101-
deleteKey := func(dkey string) {
102-
manager.del(context.Background(), dkey)
103+
deleteKey := func(dkey string) error {
104+
if err := manager.del(context.Background(), dkey); err != nil {
105+
return err
106+
}
103107
// External storage saves body data with different key
104108
if cfg.Storage != nil {
105-
manager.del(context.Background(), dkey+"_body")
109+
if err := manager.del(context.Background(), dkey+"_body"); err != nil {
110+
return err
111+
}
106112
}
113+
return nil
107114
}
108115

109116
// Return new handler
@@ -126,7 +133,10 @@ func New(config ...Config) fiber.Handler {
126133
key := cfg.KeyGenerator(c) + "_" + requestMethod
127134

128135
// Get entry from pool
129-
e := manager.get(c, key)
136+
e, err := manager.get(c, key)
137+
if err != nil && !errors.Is(err, errCacheMiss) {
138+
return err
139+
}
130140

131141
// Lock entry
132142
mux.Lock()
@@ -143,7 +153,13 @@ func New(config ...Config) fiber.Handler {
143153

144154
// Check if entry is expired
145155
if e.exp != 0 && ts >= e.exp {
146-
deleteKey(key)
156+
if err := deleteKey(key); err != nil {
157+
if e != nil {
158+
manager.release(e)
159+
}
160+
mux.Unlock()
161+
return fmt.Errorf("cache: failed to delete expired key %q: %w", key, err)
162+
}
147163
if cfg.MaxBytes > 0 {
148164
_, size := heap.remove(e.heapidx)
149165
storedBytes -= size
@@ -152,7 +168,13 @@ func New(config ...Config) fiber.Handler {
152168
// Separate body value to avoid msgp serialization
153169
// We can store raw bytes with Storage 👍
154170
if cfg.Storage != nil {
155-
e.body = manager.getRaw(c, key+"_body")
171+
rawBody, err := manager.getRaw(c, key+"_body")
172+
if err != nil {
173+
manager.release(e)
174+
mux.Unlock()
175+
return cacheBodyFetchError(key, err)
176+
}
177+
e.body = rawBody
156178
}
157179
// Set response headers from cache
158180
c.Response().SetBodyRaw(e.body)
@@ -230,7 +252,9 @@ func New(config ...Config) fiber.Handler {
230252
if cfg.MaxBytes > 0 {
231253
for storedBytes+bodySize > cfg.MaxBytes {
232254
keyToRemove, size := heap.removeFirst()
233-
deleteKey(keyToRemove)
255+
if err := deleteKey(keyToRemove); err != nil {
256+
return fmt.Errorf("cache: failed to delete key %q while evicting: %w", keyToRemove, err)
257+
}
234258
storedBytes -= size
235259
}
236260
}
@@ -285,14 +309,22 @@ func New(config ...Config) fiber.Handler {
285309

286310
// For external Storage we store raw body separated
287311
if cfg.Storage != nil {
288-
manager.setRaw(c, key+"_body", e.body, expiration)
312+
if err := manager.setRaw(c, key+"_body", e.body, expiration); err != nil {
313+
manager.release(e)
314+
return err
315+
}
289316
// avoid body msgp encoding
290317
e.body = nil
291-
manager.set(c, key, e, expiration)
318+
if err := manager.set(c, key, e, expiration); err != nil {
319+
manager.release(e)
320+
return err
321+
}
292322
manager.release(e)
293323
} else {
294324
// Store entry in memory
295-
manager.set(c, key, e, expiration)
325+
if err := manager.set(c, key, e, expiration); err != nil {
326+
return err
327+
}
296328
}
297329

298330
c.Set(cfg.CacheHeader, cacheMiss)
@@ -325,6 +357,13 @@ func hasRequestDirective(c fiber.Ctx, directive string) bool {
325357
return false
326358
}
327359

360+
func cacheBodyFetchError(key string, err error) error {
361+
if errors.Is(err, errCacheMiss) {
362+
return fmt.Errorf("cache: no cached body for key %q: %w", key, err)
363+
}
364+
return err
365+
}
366+
328367
// parseMaxAge extracts the max-age directive from a Cache-Control header.
329368
func parseMaxAge(cc string) (time.Duration, bool) {
330369
for part := range strings.SplitSeq(cc, ",") {

‎middleware/cache/cache_test.go‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ package cache
44

55
import (
66
"bytes"
7+
"context"
8+
"errors"
79
"fmt"
810
"io"
911
"math"
@@ -21,6 +23,153 @@ import (
2123
"github.com/valyala/fasthttp"
2224
)
2325

26+
type failingCacheStorage struct {
27+
data map[string][]byte
28+
errs map[string]error
29+
}
30+
31+
func newFailingCacheStorage() *failingCacheStorage {
32+
return &failingCacheStorage{
33+
data: make(map[string][]byte),
34+
errs: make(map[string]error),
35+
}
36+
}
37+
38+
func (s *failingCacheStorage) GetWithContext(_ context.Context, key string) ([]byte, error) {
39+
if err, ok := s.errs["get|"+key]; ok && err != nil {
40+
return nil, err
41+
}
42+
if val, ok := s.data[key]; ok {
43+
return append([]byte(nil), val...), nil
44+
}
45+
return nil, nil
46+
}
47+
48+
func (s *failingCacheStorage) Get(key string) ([]byte, error) {
49+
return s.GetWithContext(context.Background(), key)
50+
}
51+
52+
func (s *failingCacheStorage) SetWithContext(_ context.Context, key string, val []byte, _ time.Duration) error {
53+
if err, ok := s.errs["set|"+key]; ok && err != nil {
54+
return err
55+
}
56+
s.data[key] = append([]byte(nil), val...)
57+
return nil
58+
}
59+
60+
func (s *failingCacheStorage) Set(key string, val []byte, exp time.Duration) error {
61+
return s.SetWithContext(context.Background(), key, val, exp)
62+
}
63+
64+
func (s *failingCacheStorage) DeleteWithContext(_ context.Context, key string) error {
65+
if err, ok := s.errs["del|"+key]; ok && err != nil {
66+
return err
67+
}
68+
delete(s.data, key)
69+
return nil
70+
}
71+
72+
func (s *failingCacheStorage) Delete(key string) error {
73+
return s.DeleteWithContext(context.Background(), key)
74+
}
75+
76+
func (s *failingCacheStorage) ResetWithContext(context.Context) error {
77+
s.data = make(map[string][]byte)
78+
s.errs = make(map[string]error)
79+
return nil
80+
}
81+
82+
func (s *failingCacheStorage) Reset() error {
83+
return s.ResetWithContext(context.Background())
84+
}
85+
86+
func (*failingCacheStorage) Close() error { return nil }
87+
88+
func TestCacheStorageGetError(t *testing.T) {
89+
t.Parallel()
90+
91+
storage := newFailingCacheStorage()
92+
storage.errs["get|/_GET"] = errors.New("boom")
93+
94+
var captured error
95+
app := fiber.New(fiber.Config{
96+
ErrorHandler: func(c fiber.Ctx, err error) error {
97+
captured = err
98+
return c.Status(fiber.StatusInternalServerError).SendString("storage failure")
99+
},
100+
})
101+
102+
app.Use(New(Config{Storage: storage, Expiration: time.Second}))
103+
app.Get("/", func(c fiber.Ctx) error {
104+
return c.SendString("ok")
105+
})
106+
107+
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
108+
require.NoError(t, err)
109+
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode)
110+
require.Error(t, captured)
111+
require.ErrorContains(t, captured, "cache: failed to get key")
112+
}
113+
114+
func TestCacheStorageSetError(t *testing.T) {
115+
t.Parallel()
116+
117+
storage := newFailingCacheStorage()
118+
storage.errs["set|/_GET_body"] = errors.New("boom")
119+
120+
var captured error
121+
app := fiber.New(fiber.Config{
122+
ErrorHandler: func(c fiber.Ctx, err error) error {
123+
captured = err
124+
return c.Status(fiber.StatusInternalServerError).SendString("storage failure")
125+
},
126+
})
127+
128+
app.Use(New(Config{Storage: storage, Expiration: time.Second}))
129+
app.Get("/", func(c fiber.Ctx) error {
130+
return c.SendString("ok")
131+
})
132+
133+
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
134+
require.NoError(t, err)
135+
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode)
136+
require.Error(t, captured)
137+
require.ErrorContains(t, captured, "cache: failed to store raw key")
138+
}
139+
140+
func TestCacheStorageDeleteError(t *testing.T) {
141+
t.Parallel()
142+
143+
storage := newFailingCacheStorage()
144+
storage.errs["del|/_GET"] = errors.New("boom")
145+
146+
// Use an obviously expired timestamp without relying on time-based conversions
147+
expired := &item{exp: 1}
148+
raw, err := expired.MarshalMsg(nil)
149+
require.NoError(t, err)
150+
151+
storage.data["/_GET"] = raw
152+
153+
var captured error
154+
app := fiber.New(fiber.Config{
155+
ErrorHandler: func(c fiber.Ctx, err error) error {
156+
captured = err
157+
return c.Status(fiber.StatusInternalServerError).SendString("storage failure")
158+
},
159+
})
160+
161+
app.Use(New(Config{Storage: storage, Expiration: time.Second}))
162+
app.Get("/", func(c fiber.Ctx) error {
163+
return c.SendString("ok")
164+
})
165+
166+
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
167+
require.NoError(t, err)
168+
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode)
169+
require.Error(t, captured)
170+
require.ErrorContains(t, captured, "cache: failed to delete expired key")
171+
}
172+
24173
func Test_Cache_CacheControl(t *testing.T) {
25174
t.Parallel()
26175

0 commit comments

Comments
 (0)