mirror of
https://github.com/lukaszraczylo/graphql-monitoring-proxy.git
synced 2026-10-17 06:17:40 +00:00
fix(cache): escape glob metacharacters and aggregate errors in redis Clear
The SCAN MATCH pattern used the raw prefix, so a prefix containing Redis glob metacharacters cleared the wrong keys or none. A Del error also aborted the clear and discarded the remaining batches. Escape the prefix for MATCH, continue past a per-batch Del error, and return the joined error. Apply the same escaping to CountQueries.
This commit is contained in:
Vendored
+47
-6
@@ -5,6 +5,8 @@ package libpack_cache_redis
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
@@ -12,6 +14,24 @@ import (
|
||||
redis "github.com/redis/go-redis/v9"
|
||||
)
|
||||
|
||||
// globEscaper escapes Redis glob-pattern metacharacters (\, *, ?, [, ]) so a
|
||||
// literal string can be embedded in a SCAN/KEYS MATCH pattern without being
|
||||
// interpreted as a wildcard. Each character is matched once per input byte,
|
||||
// so the replacements cannot cascade into one another.
|
||||
var globEscaper = strings.NewReplacer(
|
||||
`\`, `\\`,
|
||||
`*`, `\*`,
|
||||
`?`, `\?`,
|
||||
`[`, `\[`,
|
||||
`]`, `\]`,
|
||||
)
|
||||
|
||||
// escapeGlobPattern returns s with Redis glob metacharacters escaped so it
|
||||
// can be used as a literal prefix inside a MATCH pattern.
|
||||
func escapeGlobPattern(s string) string {
|
||||
return globEscaper.Replace(s)
|
||||
}
|
||||
|
||||
type RedisConfig struct {
|
||||
ctx context.Context
|
||||
client *redis.Client
|
||||
@@ -90,16 +110,25 @@ func (c *RedisConfig) Delete(key string) error {
|
||||
// Clear removes only the keys owned by this cache (those under the configured
|
||||
// prefix). Unlike FlushDB it does not wipe unrelated keys that may share the
|
||||
// selected Redis DB when the database is not exclusively owned by this proxy.
|
||||
// The prefix is glob-escaped before use so a prefix containing Redis glob
|
||||
// metacharacters (*, ?, [, ], \) is matched literally instead of being
|
||||
// interpreted as a wildcard. A failure to delete one scanned batch does not
|
||||
// abort the clear: remaining batches are still attempted, and any errors
|
||||
// encountered (scan or delete) are joined and returned to the caller so a
|
||||
// partial clear is never silently reported as a success.
|
||||
func (c *RedisConfig) Clear() error {
|
||||
pattern := escapeGlobPattern(c.prefix) + "*"
|
||||
var cursor uint64
|
||||
var errs []error
|
||||
for {
|
||||
keys, next, err := c.client.Scan(c.ctx, cursor, c.prependKeyName("*"), 100).Result()
|
||||
keys, next, err := c.client.Scan(c.ctx, cursor, pattern, 100).Result()
|
||||
if err != nil {
|
||||
return err
|
||||
errs = append(errs, fmt.Errorf("redis: scan at cursor %d: %w", cursor, err))
|
||||
break
|
||||
}
|
||||
if len(keys) > 0 {
|
||||
if err := c.client.Del(c.ctx, keys...).Err(); err != nil {
|
||||
return err
|
||||
errs = append(errs, fmt.Errorf("redis: delete batch at cursor %d: %w", cursor, err))
|
||||
}
|
||||
}
|
||||
cursor = next
|
||||
@@ -107,19 +136,31 @@ func (c *RedisConfig) Clear() error {
|
||||
break
|
||||
}
|
||||
}
|
||||
return nil
|
||||
return errors.Join(errs...)
|
||||
}
|
||||
|
||||
// CountQueries returns the number of keys owned by this cache (those under
|
||||
// the configured prefix). The prefix is glob-escaped the same way Clear
|
||||
// escapes it, so a prefix containing Redis glob metacharacters is matched
|
||||
// literally instead of being interpreted as a wildcard, which would
|
||||
// otherwise undercount or overcount keys in admin-dashboard stats.
|
||||
func (c *RedisConfig) CountQueries() (int64, error) {
|
||||
keys, err := c.client.Keys(c.ctx, c.prependKeyName("*")).Result()
|
||||
pattern := escapeGlobPattern(c.prefix) + "*"
|
||||
keys, err := c.client.Keys(c.ctx, pattern).Result()
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return int64(len(keys)), nil
|
||||
}
|
||||
|
||||
// CountQueriesWithPattern returns the number of keys owned by this cache
|
||||
// whose suffix (after the prefix) matches pattern. Only the prefix is
|
||||
// glob-escaped, the same way Clear and CountQueries escape it; pattern
|
||||
// itself is left as given so a caller can still use glob metacharacters
|
||||
// intentionally in the suffix it supplies.
|
||||
func (c *RedisConfig) CountQueriesWithPattern(pattern string) (int, error) {
|
||||
keys, err := c.client.Keys(c.ctx, c.prependKeyName(pattern)).Result()
|
||||
fullPattern := escapeGlobPattern(c.prefix) + pattern
|
||||
keys, err := c.client.Keys(c.ctx, fullPattern).Result()
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
|
||||
Vendored
+194
@@ -0,0 +1,194 @@
|
||||
package libpack_cache_redis
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/alicebob/miniredis/v2"
|
||||
miniredis_server "github.com/alicebob/miniredis/v2/server"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Clear — prefix containing Redis glob metacharacters
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
// TestClear_PrefixWithGlobMetacharacters_ClearsOnlyOwnedKeys covers the
|
||||
// defect where a prefix containing Redis glob metacharacters (*, ?, [, ],
|
||||
// \) corrupted the MATCH pattern passed to SCAN, so Clear silently deleted
|
||||
// nothing (or the wrong keys). The prefix must now be matched literally.
|
||||
func TestClear_PrefixWithGlobMetacharacters_ClearsOnlyOwnedKeys(t *testing.T) {
|
||||
t.Parallel()
|
||||
s, err := miniredis.Run()
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(s.Close)
|
||||
|
||||
const globPrefix = "a[b]c*d?e\\f:"
|
||||
owned, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: globPrefix})
|
||||
require.NoError(t, err)
|
||||
|
||||
other, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: "unrelated:"})
|
||||
require.NoError(t, err)
|
||||
|
||||
require.NoError(t, owned.Set("k1", []byte("v1"), 0))
|
||||
require.NoError(t, owned.Set("k2", []byte("v2"), 0))
|
||||
require.NoError(t, other.Set("k1", []byte("untouched"), 0))
|
||||
|
||||
require.NoError(t, owned.Clear())
|
||||
|
||||
// Keys under the glob-metacharacter prefix are gone.
|
||||
_, found, err := owned.Get("k1")
|
||||
assert.NoError(t, err)
|
||||
assert.False(t, found, "key under glob-metacharacter prefix should be cleared")
|
||||
_, found, err = owned.Get("k2")
|
||||
assert.NoError(t, err)
|
||||
assert.False(t, found, "key under glob-metacharacter prefix should be cleared")
|
||||
|
||||
// Key under the unrelated prefix survives untouched.
|
||||
val, found, err := other.Get("k1")
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, found, "key outside the cleared prefix must not be touched")
|
||||
assert.Equal(t, []byte("untouched"), val)
|
||||
}
|
||||
|
||||
// TestCountQueries_PrefixWithGlobMetacharacters_CountsOnlyOwnedKeys covers
|
||||
// the same class of defect as TestClear_PrefixWithGlobMetacharacters_
|
||||
// ClearsOnlyOwnedKeys, but for CountQueries: an unescaped glob-metacharacter
|
||||
// prefix embedded directly in the KEYS pattern would report the wrong count
|
||||
// for the admin dashboard (matching too few or unrelated keys).
|
||||
func TestCountQueries_PrefixWithGlobMetacharacters_CountsOnlyOwnedKeys(t *testing.T) {
|
||||
t.Parallel()
|
||||
s, err := miniredis.Run()
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(s.Close)
|
||||
|
||||
const globPrefix = "a[b]c*d?e\\f:"
|
||||
owned, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: globPrefix})
|
||||
require.NoError(t, err)
|
||||
|
||||
other, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: "unrelated:"})
|
||||
require.NoError(t, err)
|
||||
|
||||
require.NoError(t, owned.Set("k1", []byte("v1"), 0))
|
||||
require.NoError(t, owned.Set("k2", []byte("v2"), 0))
|
||||
require.NoError(t, other.Set("k1", []byte("untouched"), 0))
|
||||
|
||||
count, err := owned.CountQueries()
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, int64(2), count, "CountQueries must count only keys under the glob-metacharacter prefix")
|
||||
|
||||
patternCount, err := owned.CountQueriesWithPattern("*")
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, 2, patternCount, "CountQueriesWithPattern must count only keys under the glob-metacharacter prefix")
|
||||
}
|
||||
|
||||
// TestClear_NormalPrefix_StillClearsAllOwnedKeys is a regression check that
|
||||
// the happy path (a prefix without glob metacharacters) still works exactly
|
||||
// as before.
|
||||
func TestClear_NormalPrefix_StillClearsAllOwnedKeys(t *testing.T) {
|
||||
t.Parallel()
|
||||
s, err := miniredis.Run()
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(s.Close)
|
||||
|
||||
rc, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: "normal:"})
|
||||
require.NoError(t, err)
|
||||
|
||||
require.NoError(t, rc.Set("k1", []byte("v1"), 0))
|
||||
require.NoError(t, rc.Set("k2", []byte("v2"), 0))
|
||||
require.NoError(t, rc.Set("k3", []byte("v3"), 0))
|
||||
|
||||
count, err := rc.CountQueries()
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, int64(3), count)
|
||||
|
||||
assert.NoError(t, rc.Clear())
|
||||
|
||||
count, err = rc.CountQueries()
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, int64(0), count)
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Clear — mid-scan DEL failure
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
// TestClear_AllBatchesDelFail_AttemptsEveryBatchAndAggregatesErrors covers
|
||||
// the defect where a mid-scan DEL error aborted Clear early, leaving a
|
||||
// partial clear with no error surfaced to the caller.
|
||||
//
|
||||
// miniredis exposes its underlying server.Server via Miniredis.Server(),
|
||||
// whose SetPreHook is a documented public hook point (used by miniredis
|
||||
// itself for auth/error simulation, see Miniredis.SetError). It lets a test
|
||||
// intercept DEL commands without a rewrite of RedisConfig into an
|
||||
// interface-backed mock, which the task's infra allows.
|
||||
//
|
||||
// Every DEL call is made to fail (not just one). A test that lets only one
|
||||
// batch's DEL succeed was tried first, and does not work against miniredis:
|
||||
// its SCAN implementation uses an index-based cursor over a sorted key
|
||||
// snapshot (cmd_generic.go cmdScan), which is invalidated as soon as a
|
||||
// preceding batch actually deletes keys mid-scan (the very "scan, then
|
||||
// delete this batch" loop Clear uses), causing miniredis to abort the SCAN
|
||||
// early with a synthetic empty result before Clear's loop reaches the next
|
||||
// batch. Real Redis's SCAN cursor (reverse binary iteration) is documented
|
||||
// to be robust against concurrent deletion during a single full iteration
|
||||
// and would not exhibit this; it is a miniredis simplification, not a
|
||||
// RedisConfig defect. Failing every batch's DEL sidesteps it (no batch is
|
||||
// ever actually deleted, so the key set never shrinks between scans) while
|
||||
// still proving Clear does not stop scanning after the first DEL error.
|
||||
func TestClear_AllBatchesDelFail_AttemptsEveryBatchAndAggregatesErrors(t *testing.T) {
|
||||
s, err := miniredis.Run()
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(s.Close)
|
||||
|
||||
rc, err := New(&RedisClientConfig{RedisServer: s.Addr(), Prefix: "pfx:"})
|
||||
require.NoError(t, err)
|
||||
|
||||
// miniredis SCAN batches deterministically by COUNT over the sorted,
|
||||
// match-filtered key set (see cmd_generic.go cmdScan), so 250 owned keys
|
||||
// with a SCAN COUNT of 100 (as used by Clear) yields exactly 3 batches:
|
||||
// [0:100), [100:200), [200:250).
|
||||
const totalKeys = 250
|
||||
const wantBatches = 3
|
||||
for i := 0; i < totalKeys; i++ {
|
||||
key := "k" + zeroPad(i)
|
||||
require.NoError(t, rc.Set(key, []byte("v"), 0))
|
||||
}
|
||||
|
||||
var seen int32
|
||||
s.Server().SetPreHook(func(c *miniredis_server.Peer, cmd string, _ ...string) bool {
|
||||
if cmd != "DEL" {
|
||||
return false // let miniredis handle everything else normally
|
||||
}
|
||||
n := atomic.AddInt32(&seen, 1)
|
||||
c.WriteError(fmt.Sprintf("SIMULATED DEL FAILURE (batch %d)", n))
|
||||
return true // command handled (as an error); miniredis skips its own DEL
|
||||
})
|
||||
|
||||
clearErr := rc.Clear()
|
||||
require.Error(t, clearErr, "Clear must surface DEL failures instead of swallowing them")
|
||||
assert.Contains(t, clearErr.Error(), "SIMULATED DEL FAILURE")
|
||||
|
||||
// Every batch was attempted: DEL was invoked once per batch even though
|
||||
// every prior call failed. The old code aborted after the first failure,
|
||||
// which would leave seen == 1 here.
|
||||
assert.Equal(t, int32(wantBatches), atomic.LoadInt32(&seen), "every batch must be attempted even after a DEL error")
|
||||
|
||||
remaining, err := rc.CountQueries()
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, int64(totalKeys), remaining, "no keys are removed when every DEL fails")
|
||||
}
|
||||
|
||||
func zeroPad(i int) string {
|
||||
// Zero-pad to 3 digits so lexicographic sort (used by miniredis SCAN)
|
||||
// matches numeric order, keeping batch membership predictable.
|
||||
digits := [3]byte{'0', '0', '0'}
|
||||
s := digits[:]
|
||||
for pos := len(s) - 1; i > 0 && pos >= 0; pos-- {
|
||||
s[pos] = byte('0' + i%10)
|
||||
i /= 10
|
||||
}
|
||||
return string(s)
|
||||
}
|
||||
Reference in New Issue
Block a user