do not cache 500 responses#61
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRemoves in-memory Redis test-store fallbacks, makes the Redis service the single implementation for caching, queueing, locking, and test helpers, adds a Redis container and REDIS_URL to CI, updates cache middleware to avoid caching 5xx responses, and ensures tests close Redis connections on teardown. ChangesRedis Integration and Test-Store Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middleware/cache.middleware.mjs (1)
6-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider the
NODE_ENV === 'test'bypass incache.middleware.mjs
src/middleware/cache.middleware.mjscurrently skips caching entirely in test mode (if (process.env.NODE_ENV === 'test') return next()), so route-level caching never runs under normal test execution.src/tests/cache.middleware.test.mjsexplicitly setsprocess.env.NODE_ENV = 'development'inbeforeEach, meaning the unit tests only cover the caching path by bypassing the bypass.- Since Redis is used in tests too (e.g.,
src/services/redis.service.mjs’sclearRedisTestData()callsflushDb()), the test-mode bypass prevents exercising the real Redis-backed cache behavior during CI tests—especially the newres.statusCode < 500behavior.Either remove the test bypass (or gate it on Redis availability rather than
NODE_ENV) and/or add tests that run caching inNODE_ENV='test'(mock Redis or use the test Redis instance) and assertsetJsonValueis not called for 5xx responses.🤖 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 `@src/middleware/cache.middleware.mjs` around lines 6 - 8, The current cache middleware short-circuits caching when NODE_ENV === 'test' which prevents exercising Redis-backed behavior; remove that environment-only bypass in src/middleware/cache.middleware.mjs (delete the `if (process.env.NODE_ENV === 'test') return next()` branch) or replace it with a runtime check for Redis availability (e.g., verify the Redis client/connection used by the middleware is present/connected before returning early), and ensure the middleware still applies the `res.statusCode < 500` guard before calling the cache writer (e.g., where it invokes `setJsonValue`). Also update tests to run the caching path under test environment (either keep setting NODE_ENV='test' and mock Redis client methods like `setJsonValue` and assert it is not called for 5xx responses, or use the real test Redis and assert no cache writes on 5xx).
🤖 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 `@src/services/redis.service.mjs`:
- Around line 156-158: clearRedisTestData currently calls getClient() then
flushDb(), which will wipe DB 0 (the default) and can delete unrelated keys;
change it so it only touches a dedicated test DB or targeted keys: either
require/validate that getRedisUrl() points to a test-only DB and call
client.select(testDb) before flushDb(), or replace flushDb() with a safe cleanup
that SCANs and deletes only test prefixes (e.g., keys matching "test_*" or the
module's prefix) using client.scan() + DEL. Update clearRedisTestData and its
callers (e.g., tests invoking clearRedisTestData, getClient/getRedisUrl) to
enforce the test-DB or prefix constraint to prevent wiping shared session/cache
keys like auth_session_*.
---
Outside diff comments:
In `@src/middleware/cache.middleware.mjs`:
- Around line 6-8: The current cache middleware short-circuits caching when
NODE_ENV === 'test' which prevents exercising Redis-backed behavior; remove that
environment-only bypass in src/middleware/cache.middleware.mjs (delete the `if
(process.env.NODE_ENV === 'test') return next()` branch) or replace it with a
runtime check for Redis availability (e.g., verify the Redis client/connection
used by the middleware is present/connected before returning early), and ensure
the middleware still applies the `res.statusCode < 500` guard before calling the
cache writer (e.g., where it invokes `setJsonValue`). Also update tests to run
the caching path under test environment (either keep setting NODE_ENV='test' and
mock Redis client methods like `setJsonValue` and assert it is not called for
5xx responses, or use the real test Redis and assert no cache writes on 5xx).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b0ab372-2d77-47ea-90b0-2fb5e345aa7c
📒 Files selected for processing (3)
.github/workflows/tests.ymlsrc/middleware/cache.middleware.mjssrc/services/redis.service.mjs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests