feat: auto-detect backend from environment variables#120
Open
27Bslash6 wants to merge 1 commit into
Open
Conversation
DefaultBackendProvider now resolves the backend from env vars with priority ordering instead of always defaulting to Redis: 1. CACHEKIT_API_KEY → CachekitIOBackend 2. CACHEKIT_REDIS_URL → RedisBackend 3. CACHEKIT_MEMCACHED_SERVERS → MemcachedBackend 4. CACHEKIT_FILE_CACHE_DIR → FileBackend 5. REDIS_URL (fallback) → RedisBackend Warns when multiple backends are configured (higher priority wins). Falls back to Redis when no env vars are set (existing behavior). Closes #87
📝 WalkthroughWalkthrough
ChangesBackend Auto-Detection and Provider Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/unit/backends/test_provider.py (1)
234-253: ⚡ Quick winMissing test for
REDIS_URLfallback priority.This test verifies
REDIS_URLworks when no other env vars are set, but doesn't test thatREDIS_URLhas lower priority thanCACHEKIT_MEMCACHED_SERVERSorCACHEKIT_FILE_CACHE_DIRas documented.Once the priority bug in
provider.pyis fixed, add a test that sets bothREDIS_URLandCACHEKIT_MEMCACHED_SERVERS, assertingMemcachedBackendis selected.Proposed additional test
def test_memcached_wins_over_redis_url_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None: """CACHEKIT_MEMCACHED_SERVERS has priority over REDIS_URL fallback.""" monkeypatch.setenv("REDIS_URL", "redis://fallback:6379") monkeypatch.setenv("CACHEKIT_MEMCACHED_SERVERS", '["127.0.0.1:11211"]') monkeypatch.delenv("CACHEKIT_API_KEY", raising=False) monkeypatch.delenv("CACHEKIT_REDIS_URL", raising=False) monkeypatch.delenv("CACHEKIT_FILE_CACHE_DIR", raising=False) with mock.patch("cachekit.backends.memcached.MemcachedBackend") as mock_backend: mock_instance = mock.MagicMock() mock_backend.return_value = mock_instance provider = DefaultBackendProvider() backend = provider.get_backend() assert backend is mock_instance mock_backend.assert_called_once()🤖 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 `@tests/unit/backends/test_provider.py` around lines 234 - 253, Add a unit test that verifies REDIS_URL is lower priority than CACHEKIT_MEMCACHED_SERVERS by setting both env vars, instantiating DefaultBackendProvider and calling get_backend(), and asserting the MemcachedBackend is chosen; specifically, set REDIS_URL and CACHEKIT_MEMCACHED_SERVERS via monkeypatch, clear CACHEKIT_API_KEY/CACHEKIT_REDIS_URL/CACHEKIT_FILE_CACHE_DIR, patch or mock cachekit.backends.memcached.MemcachedBackend to return a mock instance, call DefaultBackendProvider().get_backend(), assert the returned backend is the memcached mock instance and that MemcachedBackend was called once to ensure memcached wins over the redis fallback.
🤖 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/cachekit/backends/provider.py`:
- Around line 107-112: The selection logic currently treats REDIS_URL the same
as CACHEKIT_REDIS_URL by combining redis_url and redis_url_fallback, so
REDIS_URL can preempt Memcached/File backends; change the decision order to
match the docstring: first check CACHEKIT_API_KEY (CachekitIOBackend), then
CACHEKIT_REDIS_URL (RedisBackend), then CACHEKIT_MEMCACHED_SERVERS
(MemcachedBackend), then CACHEKIT_FILE_CACHE_DIR (FileBackend), and only if none
of those are present use REDIS_URL as a fallback. Concretely, adjust the code
that references redis_url and redis_url_fallback (the variables used to pick
RedisBackend) to ensure redis_url_fallback (REDIS_URL) is evaluated only after
checking CACHEKIT_MEMCACHED_SERVERS and CACHEKIT_FILE_CACHE_DIR, leaving the
existing constructors CachekitIOBackend, RedisBackend, MemcachedBackend, and
FileBackend intact.
---
Nitpick comments:
In `@tests/unit/backends/test_provider.py`:
- Around line 234-253: Add a unit test that verifies REDIS_URL is lower priority
than CACHEKIT_MEMCACHED_SERVERS by setting both env vars, instantiating
DefaultBackendProvider and calling get_backend(), and asserting the
MemcachedBackend is chosen; specifically, set REDIS_URL and
CACHEKIT_MEMCACHED_SERVERS via monkeypatch, clear
CACHEKIT_API_KEY/CACHEKIT_REDIS_URL/CACHEKIT_FILE_CACHE_DIR, patch or mock
cachekit.backends.memcached.MemcachedBackend to return a mock instance, call
DefaultBackendProvider().get_backend(), assert the returned backend is the
memcached mock instance and that MemcachedBackend was called once to ensure
memcached wins over the redis fallback.
🪄 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: 6f3ff744-2c4b-4d4b-afd0-5b72720d3f5e
📒 Files selected for processing (2)
src/cachekit/backends/provider.pytests/unit/backends/test_provider.py
Comment on lines
+107
to
+112
| Resolution priority (first match wins): | ||
| 1. CACHEKIT_API_KEY → CachekitIOBackend | ||
| 2. CACHEKIT_REDIS_URL → RedisBackend | ||
| 3. CACHEKIT_MEMCACHED_SERVERS → MemcachedBackend | ||
| 4. CACHEKIT_FILE_CACHE_DIR → FileBackend | ||
| 5. REDIS_URL (fallback) → RedisBackend |
There was a problem hiding this comment.
REDIS_URL priority does not match documentation.
The docstring states REDIS_URL should be priority 5 (checked after Memcached and File), but line 147 checks redis_url or redis_url_fallback together, giving REDIS_URL the same priority (2) as CACHEKIT_REDIS_URL.
This means if a user sets both REDIS_URL and CACHEKIT_MEMCACHED_SERVERS, Redis will be selected instead of Memcached, contradicting the documented fallback behavior.
Proposed fix to implement documented priority
- if redis_url or redis_url_fallback:
+ if redis_url:
from cachekit.backends.redis.config import RedisBackendConfig
from cachekit.backends.redis.provider import RedisBackendProvider, tenant_context
redis_config = RedisBackendConfig.from_env()
provider = RedisBackendProvider(redis_url=redis_config.redis_url)
if tenant_context.get() is None:
tenant_context.set("default")
return provider.get_backend()
if memcached_servers:
from cachekit.backends.memcached import MemcachedBackend
return MemcachedBackend() # reads from env via pydantic-settings
if file_cache_dir:
from cachekit.backends.file import FileBackend, FileBackendConfig
config = FileBackendConfig.from_env()
return FileBackend(config)
+ if redis_url_fallback:
+ from cachekit.backends.redis.config import RedisBackendConfig
+ from cachekit.backends.redis.provider import RedisBackendProvider, tenant_context
+
+ redis_config = RedisBackendConfig.from_env()
+ provider = RedisBackendProvider(redis_url=redis_config.redis_url)
+ if tenant_context.get() is None:
+ tenant_context.set("default")
+ return provider.get_backend()
+
# No backend env vars found — fall back to Redis (will fail at connection time
# with a clear error if no Redis is available)Also applies to: 147-155
🤖 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/cachekit/backends/provider.py` around lines 107 - 112, The selection
logic currently treats REDIS_URL the same as CACHEKIT_REDIS_URL by combining
redis_url and redis_url_fallback, so REDIS_URL can preempt Memcached/File
backends; change the decision order to match the docstring: first check
CACHEKIT_API_KEY (CachekitIOBackend), then CACHEKIT_REDIS_URL (RedisBackend),
then CACHEKIT_MEMCACHED_SERVERS (MemcachedBackend), then CACHEKIT_FILE_CACHE_DIR
(FileBackend), and only if none of those are present use REDIS_URL as a
fallback. Concretely, adjust the code that references redis_url and
redis_url_fallback (the variables used to pick RedisBackend) to ensure
redis_url_fallback (REDIS_URL) is evaluated only after checking
CACHEKIT_MEMCACHED_SERVERS and CACHEKIT_FILE_CACHE_DIR, leaving the existing
constructors CachekitIOBackend, RedisBackend, MemcachedBackend, and FileBackend
intact.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Summary
DefaultBackendProvidernow resolves the backend from environment variables instead of always defaulting to Redis.Priority order (first match wins):
CACHEKIT_API_KEY→ CachekitIOBackendCACHEKIT_REDIS_URL→ RedisBackendCACHEKIT_MEMCACHED_SERVERS→ MemcachedBackendCACHEKIT_FILE_CACHE_DIR→ FileBackendREDIS_URL(fallback) → RedisBackendWarns when multiple backends are configured. Falls back to Redis when no env vars set (existing behavior preserved).
Test plan
Closes #87
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes