Implement Response Caching for Currency and Institution Endpoints#394
Draft
sundayonah wants to merge 17 commits intomainfrom
Draft
Implement Response Caching for Currency and Institution Endpoints#394sundayonah wants to merge 17 commits intomainfrom
sundayonah wants to merge 17 commits intomainfrom
Conversation
…d error handling and response validation
Collaborator
|
This should be a draft pr right @sundayonah |
Collaborator
Author
Yes. |
sundayonah
force-pushed
the
implement-response-caching-for-currency-and-institution-endpoints
branch
from
9e07b06 to
3bb24ef
Compare
…institution-endpoints
Contributor
WalkthroughAdds Redis-backed response caching with middleware, configuration, and tests. Introduces cache durations and versioning via new env vars/config fields, wires middleware for currencies, institutions, and pubkey routes, adds a cache warming routine at startup, and integrates Prometheus metrics. Updates dependencies to include Redis, mocks, and Prometheus. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Gin Router
participant M as CacheMiddleware
participant D as Redis
participant H as Handler (Upstream)
rect rgba(230,240,255,0.5)
Note over R,M: Request flow with cache check
C->>R: GET /v1/currencies
R->>M: Invoke middleware
M->>D: GET key (versioned)
alt Cache HIT (valid)
D-->>M: Cached payload + ETag
M-->>R: Write headers (X-Cache: HIT, Cache-Control, ETag)
R-->>C: 200 with cached body
else Cache MISS or STALE
D-->>M: Not found or expiring
M-->>R: Proceed, wrap writer
R->>H: Call handler
H-->>R: Response body
R-->>M: After response
M->>D: SET payload + ETag with TTL
M-->>R: X-Cache: MISS
R-->>C: 200 with fresh body
end
end
rect rgba(235,255,235,0.5)
Note over M,D: Stale-While-Revalidate
M->>D: Check TTL near expiry
M-->>M: Background revalidate fetch
M->>D: Update cached value/ETag
end
sequenceDiagram
autonumber
participant S as Server Startup
participant CS as CacheService
participant D as Redis
participant API as HTTP Endpoints
rect rgba(255,245,230,0.5)
Note over S,CS: WarmCache on init
S->>CS: NewCacheService(redis config)
CS->>D: PING
D-->>CS: PONG
S->>CS: WarmCache(ctx)
CS->>API: GET /v1/currencies
API-->>CS: 200 JSON + ETag
CS->>D: SET api:currencies:list
loop for each currency
CS->>API: GET /v1/institutions/:code
API-->>CS: 200 JSON + ETag
CS->>D: SET api:institutions:{code}
end
CS->>API: GET /v1/pubkey
API-->>CS: 200 JSON + ETag
CS->>D: SET api:aggregator:pubkey
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
go.mod (1)
9-9: Fix inconsistent miniredis versions (direct vs indirect).You require miniredis v2.34.0 directly and v2.35.0 indirectly. Go will pick v2.35.0, which can cause confusion and flaky builds. Align to a single version and drop the indirect entry.
Apply:
- github.com/alicebob/miniredis/v2 v2.34.0 + github.com/alicebob/miniredis/v2 v2.35.0And remove the indirect:
- github.com/alicebob/miniredis/v2 v2.35.0 // indirectAlso applies to: 39-39
.env.example (1)
13-16: Document units explicitly to prevent misconfiguration.Add inline comments to clarify:
- CURRENCIES_CACHE_DURATION, INSTITUTIONS_CACHE_DURATION = hours
- PUBKEY_CACHE_DURATION = days
- CACHE_VERSION = key prefix (e.g., v1)
Also applies to: 32-32
routers/middleware/caching.go (3)
31-36: Remove unused CacheConfig.Dead code adds noise.
Apply:
-type CacheConfig struct { - Host string - Port string - Password string - DB int -}
38-51: Avoid duplicate Prometheus registrations on repeated service init.NewCacheService registers counters each time it’s called; repeated inits (tests, hot-reload) will panic. Inject a prometheus.Registerer or register once in init.
Example:
func NewCacheService(cfg config.RedisConfiguration, reg prometheus.Registerer) (*CacheService, error) { if reg == nil { reg = prometheus.DefaultRegisterer } metrics := CacheMetrics{ ... } reg.MustRegister(metrics.hits, metrics.misses) ... }
9-11: Replace deprecated ioutil with io.[Suggest using io.ReadAll.]
Apply:
- "io/ioutil" + "io"- body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)- body, _ := ioutil.ReadAll(resp.Body) + body, _ := io.ReadAll(resp.Body)Also applies to: 215-218, 250-251
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.env.example(2 hunks)config/redis.go(2 hunks)config/server.go(2 hunks)go.mod(3 hunks)routers/index.go(3 hunks)routers/middleware/caching.go(1 hunks)routers/middleware/caching_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
routers/index.go (3)
config/redis.go (1)
RedisConfig(19-27)config/server.go (1)
ServerConfig(29-62)routers/middleware/caching.go (1)
NewCacheService(39-63)
routers/middleware/caching_test.go (1)
routers/middleware/caching.go (2)
CacheService(26-29)CacheMetrics(20-23)
routers/middleware/caching.go (2)
config/redis.go (2)
RedisConfiguration(10-16)RedisConfig(19-27)config/server.go (1)
ServerConfig(29-62)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 13-13: [UnorderedKey] The HOST_DOMAIN key should go before the JWT_ACCESS_LIFESPAN key
(UnorderedKey)
[warning] 14-14: [UnorderedKey] The CURRENCIES_CACHE_DURATION key should go before the DEBUG key
(UnorderedKey)
[warning] 15-15: [UnorderedKey] The PUBKEY_CACHE_DURATION key should go before the SECRET key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The INSTITUTIONS_CACHE_DURATION key should go before the JWT_ACCESS_LIFESPAN key
(UnorderedKey)
[warning] 32-32: [UnorderedKey] The CACHE_VERSION key should go before the REDIS_DB key
(UnorderedKey)
🪛 OSV Scanner (2.2.2)
go.mod
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: Denial of service in message decoding in github.com/btcsuite/btcd
(GO-2022-1098)
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: Consensus failures in github.com/btcsuite/btcd
(GO-2024-2818)
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: Consensus failure in github.com/btcsuite/btcd
(GO-2024-3189)
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: btcd did not correctly re-implement Bitcoin Core's "FindAndDelete()" functionality
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: btcd mishandles witness size checking
[CRITICAL] 11-11: github.com/btcsuite/btcd 0.22.1: btcd susceptible to consensus failures
[HIGH] 17-17: github.com/golang-jwt/jwt/v5 5.0.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 17-17: github.com/golang-jwt/jwt/v5 5.0.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
config/server.go (1)
37-39: Confirm unit semantics for PubKeyCacheDuration (days vs hours).Currencies/Institutions are hours; PubKey is days. Ensure all consumers multiply PubKey duration by 24h consistently (see WarmCache bug reported separately).
Also applies to: 58-61
Comment on lines
+15
to
16
| CacheVersion string | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against empty CACHE_VERSION to avoid “:api:*” keys.
If CACHE_VERSION is unset, keys become “:api:*”, which breaks consistency and tests. Default to "v1".
Apply:
type RedisConfiguration struct {
Host string
Port string
Password string
DB int
- CacheVersion string
+ CacheVersion string
}
func RedisConfig() RedisConfiguration {
- return RedisConfiguration{
+ cv := viper.GetString("CACHE_VERSION")
+ if cv == "" {
+ cv = "v1"
+ }
+ return RedisConfiguration{
Host: viper.GetString("REDIS_HOST"),
Port: viper.GetString("REDIS_PORT"),
Password: viper.GetString("REDIS_PASSWORD"),
DB: viper.GetInt("REDIS_DB"),
- CacheVersion: viper.GetString("CACHE_VERSION"),
+ CacheVersion: cv,
}
}Also applies to: 25-26
🤖 Prompt for AI Agents
In config/redis.go around lines 15-16 (and also apply the same change at lines
25-26), the CacheVersion field may be empty which yields keys like ":api:*";
guard against an empty CACHE_VERSION by defaulting it to "v1". Update the struct
initialization or constructor to set CacheVersion = "v1" when the env/config
value is empty and ensure any key-building helper uses CacheVersionFallback :=
CacheVersion; if CacheVersionFallback == "" { CacheVersionFallback = "v1" } so
generated keys never start with a leading colon.
| github.com/beorn7/perks v1.0.1 // indirect | ||
| github.com/bits-and-blooms/bitset v1.7.0 // indirect | ||
| github.com/bmatcuk/doublestar v1.3.4 // indirect | ||
| github.com/btcsuite/btcd v0.22.1 // indirect |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Critical upstream vulns present (btcd).
OSV flags multiple critical advisories for github.com/btcsuite/btcd v0.22.1 (indirect). Not introduced by this PR, but it’s a material risk. Consider upgrading or constraining transitive deps pulling btcd, or vendor a patched version.
Run to list advisories and reverse-deps:
🏁 Script executed:
#!/bin/bash
# Show advisories
gh api graphql -f query='
{ securityVulnerabilities(first: 20, ecosystem: GO, package: "github.com/btcsuite/btcd") {
nodes { advisory { summary severity permalink } vulnerableVersionRange firstPatchedVersion { identifier } }
}
}
'
# Who pulls btcd in module graph
go mod graph | rg -nP 'btcsuite/btcd@' -C2Length of output: 6138
Critical upstream vulns present (btcd v0.22.1).
OSV reports GHSA-2chg-86hq-7w38 (CRITICAL), GHSA-27vh-h6mc-q6g8 (HIGH) and GHSA-3jgf-r68h-xfqm (MODERATE). go.mod transitively pulls v0.22.1. Upgrade btcd to ≥0.24.2-beta.rc1 (or ≥0.23.2 to patch the critical issue), or constrain your transitive deps/vendor a patched fork.
🤖 Prompt for AI Agents
In go.mod around line 45, the project pulls github.com/btcsuite/btcd v0.22.1
which has critical/high OSV vulns; update the dependency to a patched release
(bump to >= v0.24.2-beta.rc1 or at minimum >= v0.23.2) by editing go.mod (or
adding a replace directive for transitive deps to force a patched fork), then
run go get github.com/btcsuite/btcd@v0.24.2-beta.rc1 (or @v0.23.2), followed by
go mod tidy (and go mod vendor if you vendor), verify the effective version with
go list -m all | grep btcd, and run CI/tests to ensure nothing breaks.
Comment on lines
+23
to
+26
| cacheService, err := middleware.NewCacheService(conf) | ||
| if err != nil { | ||
| log.Fatalf("Failed to initialize cache: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Don’t crash the app if Redis is unavailable; fall back to pass‑through.
Fatal here violates “gracefully handle cache failures by falling back to direct responses”. Provide a no‑op middleware when cache init fails.
Apply:
- cacheService, err := middleware.NewCacheService(conf)
- if err != nil {
- log.Fatalf("Failed to initialize cache: %v", err)
- }
+ cacheService, err := middleware.NewCacheService(conf)
+ var cacheMW func(time.Duration) gin.HandlerFunc
+ if err != nil {
+ log.Printf("Failed to initialize cache: %v. Proceeding without caching.", err)
+ cacheMW = func(_ time.Duration) gin.HandlerFunc {
+ return func(c *gin.Context) { c.Next() }
+ }
+ } else {
+ cacheMW = cacheService.CacheMiddleware
+ }And use cacheMW:
- cacheService.CacheMiddleware(time.Duration(serverConf.CurrenciesCacheDuration)*time.Hour),
+ cacheMW(time.Duration(serverConf.CurrenciesCacheDuration)*time.Hour),- cacheService.CacheMiddleware(time.Duration(serverConf.InstitutionsCacheDuration)*time.Hour),
+ cacheMW(time.Duration(serverConf.InstitutionsCacheDuration)*time.Hour),- v1.GET("pubkey",
- cacheService.CacheMiddleware(time.Duration(serverConf.PubKeyCacheDuration)*24*time.Hour),
- ctrl.GetAggregatorPublicKey,
- )
+ v1.GET("pubkey",
+ cacheMW(time.Duration(serverConf.PubKeyCacheDuration)*24*time.Hour),
+ ctrl.GetAggregatorPublicKey,
+ )Also, where is WarmCache invoked on startup? If not yet wired, consider calling it after routes are registered (non-blocking), or from main bootstrap.
Also applies to: 42-55
🏁 Script executed:
#!/bin/bash
set -e
# Locate routers/index.go
echo "Finding routers/index.go path..."
ROUTERS_FILE=$(rg -l "middleware.NewCacheService" -g "*.go")
echo "routers/index.go located at: $ROUTERS_FILE"
echo "Printing first 100 lines of routers/index.go..."
sed -n '1,100p' "$ROUTERS_FILE"
# Find NewCacheService implementation
echo "Searching for NewCacheService definition..."
SERVICE_FILE=$(rg -l "func NewCacheService" -g "*.go")
echo "NewCacheService defined in: $SERVICE_FILE"
echo "Printing the implementation of NewCacheService..."
sed -n '1,200p' "$SERVICE_FILE"Length of output: 9776
Use no-op cache middleware on Redis failure and invoke WarmCache on startup
- In routers/index.go (around lines 23–26), replace the
log.Fatalfonmiddleware.NewCacheServiceerror with acacheMWwrapper that returnsfunc(c *gin.Context){c.Next()}when init fails, then usecacheMW(...)instead ofcacheService.CacheMiddleware(...)in your routes. - After routes are registered (or in your main bootstrap), call
go cacheService.WarmCache(context.Background())to prime the cache asynchronously.
🤖 Prompt for AI Agents
In routers/index.go around lines 23 to 26, replace the current fatal log on
NewCacheService error with a no-op cache middleware function that simply calls
the next handler in Gin's context. Assign this no-op function to cacheMW if
initialization fails, otherwise assign cacheMW as cacheService.CacheMiddleware.
Then update route registrations to use cacheMW instead of
cacheService.CacheMiddleware directly. Finally, after all routes are registered
or in the main application startup, invoke go cacheService.WarmCache with a
background context to asynchronously warm the cache.
Comment on lines
+35
to
+41
| func TestCacheMiddleware(t *testing.T) { | ||
| // Setup | ||
| gin.SetMode(gin.TestMode) | ||
| mr, client := setupTestRedis() | ||
| defer mr.Close() | ||
|
|
||
| cacheService := &CacheService{ |
Contributor
There was a problem hiding this comment.
Set CACHE_VERSION in tests to match key assertions.
Without setting CACHE_VERSION, middleware may write keys prefixed with “:”, but the test expects “v1:*”.
Apply:
func TestCacheMiddleware(t *testing.T) {
// Setup
gin.SetMode(gin.TestMode)
+ viper.Set("CACHE_VERSION", "v1")
mr, client := setupTestRedis()
defer mr.Close()Also applies to: 63-66
🤖 Prompt for AI Agents
In routers/middleware/caching_test.go around lines 35-41 (and also apply same
change at 63-66), the test never sets the CACHE_VERSION env var so produced keys
are prefixed with ":" while assertions expect "v1:*"; set the environment
variable CACHE_VERSION to "v1" at the start of each affected test (use
os.Setenv("CACHE_VERSION", "v1") and defer restoring/unsetting it) before
creating the cache/middleware client so keys are generated with the "v1:" prefix
and assertions will match.
Comment on lines
+65
to
+78
| func generateCacheKey(c *gin.Context) string { | ||
| conf := config.RedisConfig() | ||
| path := c.Request.URL.Path | ||
| switch { | ||
| case path == "/v1/currencies": | ||
| return fmt.Sprintf("%s:api:currencies:list", conf.CacheVersion) | ||
| case path == "/v1/pubkey": | ||
| return fmt.Sprintf("%s:api:aggregator:pubkey", conf.CacheVersion) | ||
| case len(c.Param("currency_code")) > 0: | ||
| return fmt.Sprintf("%s:api:institutions:%s", conf.CacheVersion, c.Param("currency_code")) | ||
| default: | ||
| return fmt.Sprintf("%s:api:%s", conf.CacheVersion, path) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Key prefix may be empty; add fallback to “v1”.
Prevents “:api:*” keys and aligns with WarmCache.
Apply:
func generateCacheKey(c *gin.Context) string {
- conf := config.RedisConfig()
+ conf := config.RedisConfig()
+ prefix := conf.CacheVersion
+ if prefix == "" {
+ prefix = "v1"
+ }
path := c.Request.URL.Path
switch {
case path == "/v1/currencies":
- return fmt.Sprintf("%s:api:currencies:list", conf.CacheVersion)
+ return fmt.Sprintf("%s:api:currencies:list", prefix)
case path == "/v1/pubkey":
- return fmt.Sprintf("%s:api:aggregator:pubkey", conf.CacheVersion)
+ return fmt.Sprintf("%s:api:aggregator:pubkey", prefix)
case len(c.Param("currency_code")) > 0:
- return fmt.Sprintf("%s:api:institutions:%s", conf.CacheVersion, c.Param("currency_code"))
+ return fmt.Sprintf("%s:api:institutions:%s", prefix, c.Param("currency_code"))
default:
- return fmt.Sprintf("%s:api:%s", conf.CacheVersion, path)
+ return fmt.Sprintf("%s:api:%s", prefix, path)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func generateCacheKey(c *gin.Context) string { | |
| conf := config.RedisConfig() | |
| path := c.Request.URL.Path | |
| switch { | |
| case path == "/v1/currencies": | |
| return fmt.Sprintf("%s:api:currencies:list", conf.CacheVersion) | |
| case path == "/v1/pubkey": | |
| return fmt.Sprintf("%s:api:aggregator:pubkey", conf.CacheVersion) | |
| case len(c.Param("currency_code")) > 0: | |
| return fmt.Sprintf("%s:api:institutions:%s", conf.CacheVersion, c.Param("currency_code")) | |
| default: | |
| return fmt.Sprintf("%s:api:%s", conf.CacheVersion, path) | |
| } | |
| } | |
| func generateCacheKey(c *gin.Context) string { | |
| conf := config.RedisConfig() | |
| prefix := conf.CacheVersion | |
| if prefix == "" { | |
| prefix = "v1" | |
| } | |
| path := c.Request.URL.Path | |
| switch { | |
| case path == "/v1/currencies": | |
| return fmt.Sprintf("%s:api:currencies:list", prefix) | |
| case path == "/v1/pubkey": | |
| return fmt.Sprintf("%s:api:aggregator:pubkey", prefix) | |
| case len(c.Param("currency_code")) > 0: | |
| return fmt.Sprintf("%s:api:institutions:%s", prefix, c.Param("currency_code")) | |
| default: | |
| return fmt.Sprintf("%s:api:%s", prefix, path) | |
| } | |
| } |
🤖 Prompt for AI Agents
In routers/middleware/caching.go around lines 65 to 78, the cache key prefix
uses conf.CacheVersion which may be empty, producing keys like ":api:...";
ensure you set a fallback when CacheVersion is empty (e.g., version :=
conf.CacheVersion; if version == "" { version = "v1" }) and use that version
variable in all fmt.Sprintf calls so keys are always prefixed with a non-empty
version (aligns with WarmCache).
Comment on lines
+85
to
+107
| func (s *CacheService) CacheMiddleware(duration time.Duration) gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| key := generateCacheKey(c) | ||
| ctx := context.Background() | ||
|
|
||
| // Check ETag | ||
| if etag := c.GetHeader("If-None-Match"); etag != "" { | ||
| if storedETag, _ := s.client.Get(ctx, key+":etag").Result(); etag == storedETag { | ||
| c.Status(http.StatusNotModified) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Try to get from cache | ||
| val, err := s.client.Get(ctx, key).Result() | ||
| if err == nil { | ||
| s.metrics.hits.Inc() | ||
| etag, _ := s.client.Get(ctx, key+":etag").Result() | ||
|
|
||
| c.Header("X-Cache", "HIT") | ||
| c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds()))) | ||
| c.Header("ETag", etag) | ||
| c.String(200, val) |
Contributor
There was a problem hiding this comment.
Return correct headers for 304 and serve cached JSON with proper Content‑Type.
- 304 should include ETag and Cache-Control.
- Using c.String sets text/plain; serve JSON as application/json.
Apply:
func (s *CacheService) CacheMiddleware(duration time.Duration) gin.HandlerFunc {
return func(c *gin.Context) {
key := generateCacheKey(c)
- ctx := context.Background()
+ ctx := c.Request.Context()
// Check ETag
- if etag := c.GetHeader("If-None-Match"); etag != "" {
- if storedETag, _ := s.client.Get(ctx, key+":etag").Result(); etag == storedETag {
- c.Status(http.StatusNotModified)
- return
- }
- }
+ if inm := c.GetHeader("If-None-Match"); inm != "" {
+ if storedETag, _ := s.client.Get(ctx, key+":etag").Result(); inm == storedETag {
+ c.Header("ETag", storedETag)
+ c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds())))
+ c.Status(http.StatusNotModified)
+ return
+ }
+ }
// Try to get from cache
val, err := s.client.Get(ctx, key).Result()
if err == nil {
s.metrics.hits.Inc()
etag, _ := s.client.Get(ctx, key+":etag").Result()
c.Header("X-Cache", "HIT")
c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds())))
- c.Header("ETag", etag)
- c.String(200, val)
+ if etag != "" {
+ c.Header("ETag", etag)
+ }
+ c.Data(http.StatusOK, "application/json", []byte(val))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *CacheService) CacheMiddleware(duration time.Duration) gin.HandlerFunc { | |
| return func(c *gin.Context) { | |
| key := generateCacheKey(c) | |
| ctx := context.Background() | |
| // Check ETag | |
| if etag := c.GetHeader("If-None-Match"); etag != "" { | |
| if storedETag, _ := s.client.Get(ctx, key+":etag").Result(); etag == storedETag { | |
| c.Status(http.StatusNotModified) | |
| return | |
| } | |
| } | |
| // Try to get from cache | |
| val, err := s.client.Get(ctx, key).Result() | |
| if err == nil { | |
| s.metrics.hits.Inc() | |
| etag, _ := s.client.Get(ctx, key+":etag").Result() | |
| c.Header("X-Cache", "HIT") | |
| c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds()))) | |
| c.Header("ETag", etag) | |
| c.String(200, val) | |
| func (s *CacheService) CacheMiddleware(duration time.Duration) gin.HandlerFunc { | |
| return func(c *gin.Context) { | |
| key := generateCacheKey(c) | |
| ctx := c.Request.Context() | |
| // Check ETag | |
| if inm := c.GetHeader("If-None-Match"); inm != "" { | |
| if storedETag, _ := s.client.Get(ctx, key+":etag").Result(); inm == storedETag { | |
| c.Header("ETag", storedETag) | |
| c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds()))) | |
| c.Status(http.StatusNotModified) | |
| return | |
| } | |
| } | |
| // Try to get from cache | |
| val, err := s.client.Get(ctx, key).Result() | |
| if err == nil { | |
| s.metrics.hits.Inc() | |
| etag, _ := s.client.Get(ctx, key+":etag").Result() | |
| c.Header("X-Cache", "HIT") | |
| c.Header("Cache-Control", fmt.Sprintf("max-age=%d, stale-while-revalidate=60", int(duration.Seconds()))) | |
| if etag != "" { | |
| c.Header("ETag", etag) | |
| } | |
| c.Data(http.StatusOK, "application/json", []byte(val)) |
🤖 Prompt for AI Agents
In routers/middleware/caching.go around lines 85-107, the middleware currently
returns 304 without ETag and Cache-Control headers and serves cached JSON with
c.String which sets text/plain; change the 304 branch to set the ETag and
Cache-Control headers (use the same max-age from duration and
stale-while-revalidate) before calling c.Status(http.StatusNotModified) and
return, and when serving a cache hit set Content-Type to application/json (e.g.,
set c.Header("Content-Type","application/json") or use c.Data with content-type
application/json) instead of c.String so cached JSON responses are returned with
the correct content type.
Comment on lines
+109
to
+113
| // Background revalidation if approaching expiry | ||
| if ttl, _ := s.client.TTL(ctx, key).Result(); ttl < time.Minute { | ||
| go s.revalidateCache(c.Copy(), key, duration) | ||
| } | ||
| return |
Contributor
There was a problem hiding this comment.
Stale‑while‑revalidate is broken: invalid URL used for background fetch.
revalidateCache builds a request with only the path; http.Client requires a full URL. Use HOST_DOMAIN and add a timeout.
Apply:
- if ttl, _ := s.client.TTL(ctx, key).Result(); ttl < time.Minute {
- go s.revalidateCache(c.Copy(), key, duration)
- }
+ if ttl, _ := s.client.TTL(ctx, key).Result(); ttl > 0 && ttl < time.Minute {
+ go s.revalidateCache(c.Copy(), key, duration)
+ }-func (s *CacheService) revalidateCache(c *gin.Context, key string, duration time.Duration) {
- ctx := context.Background()
- req, _ := http.NewRequest(c.Request.Method, c.Request.URL.String(), nil)
- resp, err := http.DefaultClient.Do(req)
+func (s *CacheService) revalidateCache(c *gin.Context, key string, duration time.Duration) {
+ ctx := context.Background()
+ conf := config.ServerConfig()
+ if conf.HostDomain == "" {
+ return
+ }
+ url := fmt.Sprintf("%s%s", conf.HostDomain, c.Request.URL.RequestURI())
+ client := &http.Client{Timeout: 10 * time.Second}
+ req, _ := http.NewRequestWithContext(ctx, c.Request.Method, url, nil)
+ resp, err := client.Do(req)
if err != nil {
return
}
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
- body, _ := ioutil.ReadAll(resp.Body)
+ body, _ := io.ReadAll(resp.Body)
etag := generateETag(body)
- s.client.Set(ctx, key, string(body), duration)
- s.client.Set(ctx, key+":etag", etag, duration)
+ _ = s.client.Set(ctx, key, string(body), duration).Err()
+ _ = s.client.Set(ctx, key+":etag", etag, duration).Err()
}
}Also applies to: 240-255
🤖 Prompt for AI Agents
In routers/middleware/caching.go around lines 109-113 (and likewise apply fix to
240-255): revalidateCache is issuing an HTTP request using only the path which
yields an invalid URL for http.Client; update the code to build a full absolute
URL by prefixing the path with the configured HOST_DOMAIN (including scheme,
e.g. https://HOST_DOMAIN) and ensure the request has the proper context (use the
copied gin.Context's request context or attach ctx via req.WithContext); also
give the background fetch a bounded timeout by using an http.Client with a
Timeout or creating a context with timeout for the request so the goroutine
cannot hang indefinitely.
Comment on lines
+176
to
+199
| // Cache currencies | ||
| currenciesKey := fmt.Sprintf("v1:api:currencies:list") | ||
| currenciesData, err := json.Marshal(currencies) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal currencies: %v", err) | ||
| } | ||
| if err := s.client.Set(ctx, currenciesKey, string(currenciesData), time.Duration(conf.CurrenciesCacheDuration)*time.Hour).Err(); err != nil { | ||
| return fmt.Errorf("failed to cache currencies: %v", err) | ||
| } | ||
|
|
||
| // Cache pubkey | ||
| pubkeyURL := fmt.Sprintf("%s/v1/pubkey", baseURL) | ||
| if err := s.cacheEndpoint(ctx, pubkeyURL, "v1:api:aggregator:pubkey", time.Duration(conf.PubKeyCacheDuration)*time.Hour); err != nil { | ||
| return fmt.Errorf("failed to cache pubkey: %v", err) | ||
| } | ||
|
|
||
| // Cache institutions for each currency | ||
| for _, currency := range currencies { | ||
| institutionsURL := fmt.Sprintf("%s/v1/institutions/%s", baseURL, currency) | ||
| key := fmt.Sprintf("v1:api:institutions:%s", currency) | ||
| if err := s.cacheEndpoint(ctx, institutionsURL, key, time.Duration(conf.InstitutionsCacheDuration)*time.Hour); err != nil { | ||
| return fmt.Errorf("failed to cache institutions for %s: %v", currency, err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
WarmCache uses hardcoded “v1:” and wrong PubKey TTL units.
- Keys ignore CACHE_VERSION.
- PubKey TTL should be days (×24h), not hours.
Apply:
- // Cache currencies
- currenciesKey := fmt.Sprintf("v1:api:currencies:list")
+ // Cache currencies
+ prefix := config.RedisConfig().CacheVersion
+ if prefix == "" {
+ prefix = "v1"
+ }
+ currenciesKey := fmt.Sprintf("%s:api:currencies:list", prefix)
currenciesData, err := json.Marshal(currencies)
...
- // Cache pubkey
+ // Cache pubkey
pubkeyURL := fmt.Sprintf("%s/v1/pubkey", baseURL)
- if err := s.cacheEndpoint(ctx, pubkeyURL, "v1:api:aggregator:pubkey", time.Duration(conf.PubKeyCacheDuration)*time.Hour); err != nil {
+ if err := s.cacheEndpoint(ctx, pubkeyURL, fmt.Sprintf("%s:api:aggregator:pubkey", prefix), time.Duration(conf.PubKeyCacheDuration)*24*time.Hour); err != nil {
return fmt.Errorf("failed to cache pubkey: %v", err)
}
...
- key := fmt.Sprintf("v1:api:institutions:%s", currency)
+ key := fmt.Sprintf("%s:api:institutions:%s", prefix, currency)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Cache currencies | |
| currenciesKey := fmt.Sprintf("v1:api:currencies:list") | |
| currenciesData, err := json.Marshal(currencies) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal currencies: %v", err) | |
| } | |
| if err := s.client.Set(ctx, currenciesKey, string(currenciesData), time.Duration(conf.CurrenciesCacheDuration)*time.Hour).Err(); err != nil { | |
| return fmt.Errorf("failed to cache currencies: %v", err) | |
| } | |
| // Cache pubkey | |
| pubkeyURL := fmt.Sprintf("%s/v1/pubkey", baseURL) | |
| if err := s.cacheEndpoint(ctx, pubkeyURL, "v1:api:aggregator:pubkey", time.Duration(conf.PubKeyCacheDuration)*time.Hour); err != nil { | |
| return fmt.Errorf("failed to cache pubkey: %v", err) | |
| } | |
| // Cache institutions for each currency | |
| for _, currency := range currencies { | |
| institutionsURL := fmt.Sprintf("%s/v1/institutions/%s", baseURL, currency) | |
| key := fmt.Sprintf("v1:api:institutions:%s", currency) | |
| if err := s.cacheEndpoint(ctx, institutionsURL, key, time.Duration(conf.InstitutionsCacheDuration)*time.Hour); err != nil { | |
| return fmt.Errorf("failed to cache institutions for %s: %v", currency, err) | |
| } | |
| } | |
| // Cache currencies | |
| prefix := config.RedisConfig().CacheVersion | |
| if prefix == "" { | |
| prefix = "v1" | |
| } | |
| currenciesKey := fmt.Sprintf("%s:api:currencies:list", prefix) | |
| currenciesData, err := json.Marshal(currencies) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal currencies: %v", err) | |
| } | |
| if err := s.client.Set(ctx, currenciesKey, string(currenciesData), time.Duration(conf.CurrenciesCacheDuration)*time.Hour).Err(); err != nil { | |
| return fmt.Errorf("failed to cache currencies: %v", err) | |
| } | |
| // Cache pubkey | |
| pubkeyURL := fmt.Sprintf("%s/v1/pubkey", baseURL) | |
| if err := s.cacheEndpoint( | |
| ctx, | |
| pubkeyURL, | |
| fmt.Sprintf("%s:api:aggregator:pubkey", prefix), | |
| time.Duration(conf.PubKeyCacheDuration)*24*time.Hour, | |
| ); err != nil { | |
| return fmt.Errorf("failed to cache pubkey: %v", err) | |
| } | |
| // Cache institutions for each currency | |
| for _, currency := range currencies { | |
| institutionsURL := fmt.Sprintf("%s/v1/institutions/%s", baseURL, currency) | |
| key := fmt.Sprintf("%s:api:institutions:%s", prefix, currency) | |
| if err := s.cacheEndpoint( | |
| ctx, | |
| institutionsURL, | |
| key, | |
| time.Duration(conf.InstitutionsCacheDuration)*time.Hour, | |
| ); err != nil { | |
| return fmt.Errorf("failed to cache institutions for %s: %v", currency, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In routers/middleware/caching.go around lines 176 to 199, keys are built with a
hardcoded "v1:" prefix and PubKey TTL uses hours not days; replace the hardcoded
prefix with the shared CACHE_VERSION (e.g. fmt.Sprintf("%s:api:...",
CACHE_VERSION)) for currencies, pubkey and institutions keys so all cache keys
honor the version, and change the PubKey TTL calculation to use days
(time.Duration(conf.PubKeyCacheDuration)*24*time.Hour) instead of hours; update
the three key constructions (currenciesKey, pubkey key, and institutions key)
and the pubkey cache set call to use those corrected values.
Comment on lines
+204
to
+218
| func (s *CacheService) cacheEndpoint(ctx context.Context, url, key string, duration time.Duration) error { | ||
| resp, err := http.Get(url) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch from %s: %v", url, err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("received non-200 status code (%d) from %s", resp.StatusCode, url) | ||
| } | ||
|
|
||
| body, err := ioutil.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response body from %s: %v", url, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Use an HTTP client with timeout and context in cacheEndpoint.
http.Get has no timeout; can hang startup.
Apply:
-func (s *CacheService) cacheEndpoint(ctx context.Context, url, key string, duration time.Duration) error {
- resp, err := http.Get(url)
+func (s *CacheService) cacheEndpoint(ctx context.Context, url, key string, duration time.Duration) error {
+ client := &http.Client{Timeout: 10 * time.Second}
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+ if err != nil {
+ return fmt.Errorf("failed to build request for %s: %v", url, err)
+ }
+ resp, err := client.Do(req)
if err != nil {
return fmt.Errorf("failed to fetch from %s: %v", url, err)
}
defer resp.Body.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *CacheService) cacheEndpoint(ctx context.Context, url, key string, duration time.Duration) error { | |
| resp, err := http.Get(url) | |
| if err != nil { | |
| return fmt.Errorf("failed to fetch from %s: %v", url, err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("received non-200 status code (%d) from %s", resp.StatusCode, url) | |
| } | |
| body, err := ioutil.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read response body from %s: %v", url, err) | |
| } | |
| func (s *CacheService) cacheEndpoint(ctx context.Context, url, key string, duration time.Duration) error { | |
| client := &http.Client{Timeout: 10 * time.Second} | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) | |
| if err != nil { | |
| return fmt.Errorf("failed to build request for %s: %v", url, err) | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return fmt.Errorf("failed to fetch from %s: %v", url, err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("received non-200 status code (%d) from %s", resp.StatusCode, url) | |
| } | |
| body, err := ioutil.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read response body from %s: %v", url, err) | |
| } | |
| // ... rest of function unchanged ... | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In routers/middleware/caching.go around lines 204 to 218, replace the blocking
http.Get call with an HTTP request that uses the provided ctx and a client with
a timeout: create an http.Request using http.NewRequestWithContext(ctx, "GET",
url, nil), create an http.Client with a sensible timeout (or derive a timeout
from ctx.Deadline if present) and use client.Do(req) instead of http.Get, then
handle errors, response status, body read and defer resp.Body.Close() as before;
this ensures the fetch respects context cancellation and won't hang
indefinitely.
chibie
force-pushed
the
main
branch
8 times, most recently
from
88f28e7 to
5d8eb49
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements Redis-based response caching for frequently accessed static data endpoints to improve API performance and reduce backend load.
Key Changes:
Cache Configuration:
Cache Keys:
Testing:
closes #381
Summary by CodeRabbit
New Features
Performance
Configuration
Observability