Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions router/pkg/entitycaching/cache/redis.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package cache

import (
"context"
"errors"
"fmt"

"github.com/redis/go-redis/v9"
enginecache "github.com/wundergraph/graphql-go-tools/v2/pkg/entitycaching"
)

// RedisCache stores entries in Redis. Redis owns expiry, so unlike
// InMemoryCache there is nothing to sweep here.
Comment on lines +12 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mention the in memory store here. Has nothing to do with this implementation + I always like to mention the interface this intends to implement.

Suggested change
// RedisCache stores entries in Redis. Redis owns expiry, so unlike
// InMemoryCache there is nothing to sweep here.
// RedisCache stores entries in Redis. It implements enginecache.Cache

type RedisCache struct {
client redis.UniversalClient
prefix string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add godoc to fields as well? I.e. whats prefix good for?

}

// NewRedisCache returns a cache backed by client, namespacing every key with
// prefix. The caller keeps ownership of client and is responsible for closing
// it. rediscloser.RDCloser satisfies redis.UniversalClient, so a client built
// by rediscloser.NewRedisCloser can be passed straight in.
func NewRedisCache(client redis.UniversalClient, prefix string) (*RedisCache, error) {
if client == nil {
return nil, errors.New("redis client is nil")
}

return &RedisCache{client: client, prefix: prefix}, nil
}

// GetMany returns one result per key, in the same order as keys.
func (c *RedisCache) GetMany(ctx context.Context, keys []string) ([]enginecache.Result, error) {
if len(keys) == 0 {
return nil, nil
}

// A pipeline of GETs rather than a single MGET: go-redis splits a pipeline
// across cluster nodes, while MGET fails with CROSSSLOT as soon as the keys
// span slots. It also gives each duplicate key its own command, so they are
// looked up independently.
pipe := c.client.Pipeline()
cmds := make([]*redis.StringCmd, len(keys))
for i, key := range keys {
cmds[i] = pipe.Get(ctx, c.prefix+key)
}

// A miss surfaces as redis.Nil, which is not a failure of the batch.
if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: too much happening in one line. Can you split it up?

Suggested change
if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) {
_, err := pipe.Exec(ctx)
if err != nil && !errors.Is(err, redis.Nil) {

return nil, err
}

results := make([]enginecache.Result, len(keys))
for i, cmd := range cmds {
value, err := cmd.Bytes()
if errors.Is(err, redis.Nil) {
continue
}
if err != nil {
// There is no partial read to salvage, the whole batch fails.
return nil, fmt.Errorf("get %q: %w", keys[i], err)
}
results[i] = enginecache.Result{Value: value, Found: true}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd.Bytes() does not copy the value but instead returns the value as an unsafe.Pointer. As you return this value its possible someone outside this package will modify the value under the hood. Better copy it.

Suggested change
results[i] = enginecache.Result{Value: value, Found: true}
results[i] = enginecache.Result{Value: bytes.Clone(value), Found: true}

}
Comment on lines +52 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect go-redis StringCmd.Bytes() implementation in the module cache / vendor dir.
set -euo pipefail

# Locate the go-redis version in use.
fd -t f 'go.mod' | xargs rg -n 'redis/go-redis/v9'

# Try vendored source first.
fd -t d 'go-redis' | head -20

# Search any available copy of the library for the Bytes implementation.
rg -nP -C 6 'func \(cmd \*StringCmd\) Bytes\(\)' $(go env GOMODCACHE 2>/dev/null || echo /root/go/pkg/mod) 2>/dev/null | head -40

Repository: wundergraph/cosmo

Length of output: 411


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- target implementation and test ---'
sed -n '1,100p' router/pkg/entitycaching/cache/redis.go
sed -n '300,355p' router/pkg/entitycaching/cache/redis_test.go

printf '%s\n' '--- dependency metadata ---'
sed -n '1,45p' router/go.mod

printf '%s\n' '--- local module source candidates ---'
modcache="$(go env GOMODCACHE 2>/dev/null || true)"
printf 'GOMODCACHE=%s\n' "$modcache"
if [ -n "$modcache" ] && [ -d "$modcache" ]; then
  find "$modcache" -path '*redis*go-redis*v9*' -type f -maxdepth 8 2>/dev/null | head -30
  rg -n -P -C 8 'func \(cmd \*StringCmd\) Bytes\(\)' "$modcache" 2>/dev/null | head -80 || true
fi

printf '%s\n' '--- upstream v9.7.3 source ---'
curl -fsSL https://raw.githubusercontent.com/redis/go-redis/v9.7.3/command.go \
  | rg -n -C 8 'func \(cmd \*StringCmd\) Bytes\(\)|StringToBytes|func \(cmd \*StringCmd\) Result' || true
curl -fsSL https://raw.githubusercontent.com/redis/go-redis/v9.7.3/internal/util/unsafe.go \
  | sed -n '1,120p' || true

Repository: wundergraph/cosmo

Length of output: 7786


🏁 Script executed:

cat <<'GO' | go run /dev/stdin
package main

import (
	"fmt"
	"unsafe"
)

func stringToBytes(s string) []byte {
	return *(*[]byte)(unsafe.Pointer(&struct {
		string
		Cap int
	}{s, len(s)}))
}

func main() {
	s := "value"
	b := stringToBytes(s)
	b[0] = 'V'
	fmt.Printf("string=%q bytes=%q\n", s, b)
}
GO

Repository: wundergraph/cosmo

Length of output: 273


Clone StringCmd.Bytes() before returning it. In go-redis v9.7.3, StringCmd.Bytes() uses an unsafe zero-copy conversion. GetMany therefore returns a slice that aliases the command's string storage. Use bytes.Clone(value) to match InMemoryCache.GetMany ownership semantics.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@router/pkg/entitycaching/cache/redis.go` around lines 52 - 63, Update
GetMany’s successful StringCmd.Bytes() result handling to assign
bytes.Clone(value) to enginecache.Result.Value, ensuring returned values do not
alias Redis command storage while preserving the existing error and missing-key
behavior.


return results, nil
}

// SetMany stores every item, all of which must carry a positive TTL. A single
// item without one fails the whole batch with an ErrMissingTTL and nothing is
// written.
func (c *RedisCache) SetMany(ctx context.Context, items []enginecache.Item) error {
if len(items) == 0 {
return nil
}

// Queuing writes nothing to redis, only Exec below does, so validating as
// we go is enough: returning early abandons the whole pipeline unsent and
// the batch is never applied in part.
pipe := c.client.Pipeline()
for _, item := range items {
if item.TTL <= 0 {
return fmt.Errorf("%w: key %q", ErrMissingTTL, item.Key)
}
pipe.Set(ctx, c.prefix+item.Key, item.Value, item.TTL)
}

_, err := pipe.Exec(ctx)
return err
}
Loading
Loading