feat: redis adapter for entity caching cache - #3139
Conversation
WalkthroughAdded ChangesRedis cache
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
SkArchon
changed the title
Router image scan passed✅ No security vulnerabilities found in image: |
3845440 to
3562724
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## milinda/eng-9922-in-memory-adapter #3139 +/- ##
======================================================================
+ Coverage 62.39% 62.46% +0.06%
======================================================================
Files 263 264 +1
Lines 31043 31075 +32
======================================================================
+ Hits 19370 19410 +40
+ Misses 10163 10158 -5
+ Partials 1510 1507 -3
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router/pkg/entitycaching/cache/redis_test.go (1)
203-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test does not test what its name says.
The subtest is named "cache tags are accepted and ignored". The item carries no tag field, and no assertion refers to tags. The body is identical to "namespaces keys with the prefix" at Lines 108-119.
Either remove the subtest, or add a real tag field to the item once the cache item type exposes one.
♻️ Proposed removal
- t.Run("cache tags are accepted and ignored", func(t *testing.T) { - t.Parallel() - - c, mr := newTestRedisCache(t) - - err := c.SetMany(ctx, []enginecache.Item{ - { - Key: "a", - Value: []byte("value"), - TTL: time.Minute, - }, - }) - require.NoError(t, err) - - require.Equal(t, []string{testPrefix + "a"}, mr.Keys()) - }) -🤖 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_test.go` around lines 203 - 218, Remove the redundant “cache tags are accepted and ignored” subtest from the Redis cache tests, since its item has no tags and it duplicates the existing namespace-prefix coverage.router/pkg/entitycaching/cache/redis.go (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError context is inconsistent between the two failure paths.
If
Execreports a command error other thanredis.Nil,GetManyreturns it unwrapped. The per-command loop below wraps the same class of error with the failing key. The result is that identical Redis failures produce different messages depending on whether aredis.Nilmiss appears earlier in the batch. The test atredis_test.goLines 374-388 depends on that ordering detail to get the key into the message.Consider dropping the
Execerror early return for command-level failures and letting the per-command loop classify every error, so the key is always present. Keep a check for transport-level failures.🤖 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 47 - 50, Update GetMany so the pipe.Exec error handling does not return command-level failures before the per-command loop can add the failing key; retain only the transport-level failure check at the Exec stage. Let the existing per-command classification and wrapping logic handle each command error consistently, including batches containing redis.Nil misses.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@router/pkg/entitycaching/cache/redis.go`:
- Around line 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.
---
Nitpick comments:
In `@router/pkg/entitycaching/cache/redis_test.go`:
- Around line 203-218: Remove the redundant “cache tags are accepted and
ignored” subtest from the Redis cache tests, since its item has no tags and it
duplicates the existing namespace-prefix coverage.
In `@router/pkg/entitycaching/cache/redis.go`:
- Around line 47-50: Update GetMany so the pipe.Exec error handling does not
return command-level failures before the per-command loop can add the failing
key; retain only the transport-level failure check at the Exec stage. Let the
existing per-command classification and wrapping logic handle each command error
consistently, including batches containing redis.Nil misses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4fbd543-f34b-46c0-92d8-b9fbf1d6e9e1
📒 Files selected for processing (2)
router/pkg/entitycaching/cache/redis.gorouter/pkg/entitycaching/cache/redis_test.go
| 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} | ||
| } |
There was a problem hiding this comment.
🩺 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 -40Repository: 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' || trueRepository: 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)
}
GORepository: 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.
| // RedisCache stores entries in Redis. Redis owns expiry, so unlike | ||
| // InMemoryCache there is nothing to sweep here. |
There was a problem hiding this comment.
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.
| // 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 |
| // InMemoryCache there is nothing to sweep here. | ||
| type RedisCache struct { | ||
| client redis.UniversalClient | ||
| prefix string |
There was a problem hiding this comment.
Can you add godoc to fields as well? I.e. whats prefix good for?
| } | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
Nit: too much happening in one line. Can you split it up?
| if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) { | |
| _, err := pipe.Exec(ctx) | |
| if err != nil && !errors.Is(err, redis.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} |
There was a problem hiding this comment.
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.
| results[i] = enginecache.Result{Value: value, Found: true} | |
| results[i] = enginecache.Result{Value: bytes.Clone(value), Found: true} |
| func TestRedisCache(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() |
| require.Empty(t, mr.Keys()) | ||
| }) | ||
|
|
||
| t.Run("one bad item rejects the whole batch", func(t *testing.T) { |
There was a problem hiding this comment.
This test contradicts the contract defined in the godoc of SetMany where it states "An error means an unspecified subset of the items may already have been stored"
| require.Empty(t, mr.Keys()) | ||
| }) | ||
|
|
||
| t.Run("cache tags are accepted and ignored", func(t *testing.T) { |
There was a problem hiding this comment.
Where exactly are cache tags used in this test? I don't know much about these tags but it does not look obvious from looking at the test
| err := c.SetMany(ctx, []enginecache.Item{ | ||
| {Key: "a", Value: []byte("value"), TTL: time.Minute}, | ||
| }) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Use assert.Contains to broadly verify the error is about failure to reach redis
2 participants
This PR contains a redis adapter to be used for entity caching, currently it does not have support for managing cache tags.
Summary by CodeRabbit
New Features
Bug Fixes
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.