[chores] Separated session redis bucket from cache bucket#233
[chores] Separated session redis bucket from cache bucket#233nemesifier wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds test-mode-aware configuration to the Django settings module. It detects when tests are running by checking command-line arguments and uses this flag to toggle backend services: Channels and ASGI use Redis in production but an in-memory backend during testing; caching and sessions default to Redis; and Celery runs with eager task execution and memory broker during testing, but with Redis broker in production. All configuration changes are additive and conditional on the newly introduced Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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
🤖 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 `@tests/openwisp2/settings.py`:
- Around line 77-91: The cache/session config correctly separates Redis DBs
using CACHES, SESSION_ENGINE and SESSION_CACHE_ALIAS, but tests will still
require Redis; update the settings to detect TESTING (or DJANGO_SETTINGS_TEST)
and switch CACHES to in-memory backends during tests (use
django.core.cache.backends.locmem.LocMemCache for both "default" and "sessions")
or add clear documentation/guards to ensure Redis is available in CI; modify the
module where CACHES, SESSION_ENGINE and SESSION_CACHE_ALIAS are defined to
branch on TESTING and set the alternate backends so tests run without a Redis
dependency.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 982e3185-b844-4d2b-8598-ca7f8c5d8de7
📒 Files selected for processing (1)
tests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Preserve object-level permissions and swappable model support when present
Be careful with certificate authority state, certificate revocation, serial numbers, extensions, private key handling, admin actions, and migrations
Watch for private key exposure, unsafe file paths, weak certificate options, invalid extensions, and secrets
Preserve validation around CA material, certificate material, revocation, downloads, and uploaded/generated files
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
tests/openwisp2/settings.py
{django_x509/tests/**,tests/**}
📄 CodeRabbit inference engine (AGENTS.md)
{django_x509/tests/**,tests/**}: Add or update tests for every behavior change
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix
Prefer in-process tests so coverage tools can measure changed code
Files:
tests/openwisp2/settings.py
🧠 Learnings (1)
📚 Learning: 2026-01-16T21:54:25.644Z
Learnt from: stktyagi
Repo: openwisp/django-x509 PR: 201
File: tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py:18-26
Timestamp: 2026-01-16T21:54:25.644Z
Learning: Do not review or suggest adding 224-bit ECDSA support in django-x509 code. SECP224R1 provides only 112 bits of cryptographic strength and is deprecated for asymmetric use by NIST SP 800-131A Rev.3 after 2030. Prefer and enforce the use of modern curves (e.g., SECP256R1/P-256) as the minimum. This guideline applies broadly to Python files in this repository; ensure reviews advocate using at least 256-bit curves and document any cryptographic choices accordingly.
Applied to files:
tests/openwisp2/settings.py
🔇 Additional comments (5)
tests/openwisp2/settings.py (5)
2-2: LGTM!
93-98: LGTM!
77-91: 🏗️ Heavy liftConsider adding integration tests for session/cache separation.
Per the coding guidelines for test files: "Add or update tests for every behavior change." The separation of session storage from cache storage is a behavioral change that could be verified with integration tests:
- Clear the default cache and verify sessions persist
- Store data in the default cache and sessions cache, then verify they're isolated
Example test:
from django.core.cache import caches from django.contrib.sessions.backends.cache import SessionStore def test_session_cache_separation(): # Store in default cache caches['default'].set('test_key', 'value') # Store in session session = SessionStore() session['user_id'] = 123 session.save() # Clear default cache caches['default'].clear() # Verify session persists restored = SessionStore(session_key=session.session_key) assert restored.get('user_id') == 123 assert caches['default'].get('test_key') is NoneIf this was manually tested (as noted in the PR description), consider adding an automated regression test to prevent future configuration drift.
Source: Coding guidelines
5-5: Update: TESTING detection covers the project’s test runners.
tests/openwisp2/settings.pyusesTESTING = sys.argv[1:2] == ["test"]. This matches both documented entrypoints:runtests.pyinserts"test"atargs[1], and./manage.py test --parallel ...runsexecute_from_command_line(sys.argv)so"test"is also atsys.argv[1]. Only non-Django runners that don’t puttestat argv index 1 would miss it (the repo doesn’t appear to configure pytest/tox for running tests).
39-48: Fix/confirmASGI_APPLICATIONtarget (tests/openwisp2/settings.py, lines 39-48)
ASGI_APPLICATION = "openwisp2.routing.application"is set unconditionally, but there is norouting.py(and no top-levelapplication = ...) undertests/openwisp2/. Ifopenwisp2.routing.applicationis not provided by an external dependency in the runtime environment, ASGI startup will fail—updateASGI_APPLICATIONto an existing module/callable in this project or ensure the required routing module is present.
| CACHES = { | ||
| "default": { | ||
| "BACKEND": "django_redis.cache.RedisCache", | ||
| "LOCATION": "redis://localhost/0", | ||
| "OPTIONS": {"CLIENT_CLASS": "django_redis.client.DefaultClient"}, | ||
| }, | ||
| "sessions": { | ||
| "BACKEND": "django_redis.cache.RedisCache", | ||
| "LOCATION": "redis://localhost/1", | ||
| "OPTIONS": {"CLIENT_CLASS": "django_redis.client.DefaultClient"}, | ||
| }, | ||
| } | ||
|
|
||
| SESSION_ENGINE = "django.contrib.sessions.backends.cache" | ||
| SESSION_CACHE_ALIAS = "sessions" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Cache and session separation correctly implemented.
The configuration successfully achieves the PR objective by using separate Redis databases:
defaultcache → Redis database 0sessionscache → Redis database 1SESSION_CACHE_ALIAS = "sessions"correctly points Django sessions to the dedicated cache
This ensures clearing the default cache won't terminate user sessions.
Note that both caches require Redis even during testing. Ensure Redis is available in all test environments, or consider adding conditional in-memory cache backends for TESTING mode if needed:
if TESTING:
CACHES = {
"default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"},
"sessions": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"},
}🤖 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/openwisp2/settings.py` around lines 77 - 91, The cache/session config
correctly separates Redis DBs using CACHES, SESSION_ENGINE and
SESSION_CACHE_ALIAS, but tests will still require Redis; update the settings to
detect TESTING (or DJANGO_SETTINGS_TEST) and switch CACHES to in-memory backends
during tests (use django.core.cache.backends.locmem.LocMemCache for both
"default" and "sessions") or add clear documentation/guards to ensure Redis is
available in CI; modify the module where CACHES, SESSION_ENGINE and
SESSION_CACHE_ALIAS are defined to branch on TESTING and set the alternate
backends so tests run without a Redis dependency.
Test Failures: Missing
|
This reverts commit 958b583.
Commit Message Format FailureHello @nemesifier, The CI failed because the commit message does not follow the required format. Fix: For example: |
1 participant
Checklist
Reference to Existing Issue
Not needed, small improvement being done in all repositories.
Description of Changes
Small improvement to the test env which separates cache from session storage to avoid terminating sessions when clearing the cache.