[feature] Added generic solution for cache invalidation in config models #1095#1418
[feature] Added generic solution for cache invalidation in config models #1095#1418pandafy wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds a declarative cache invalidation system with Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Django
participant ConfigConfig
participant CacheInvalidationMixin
participant AbstractConfig
participant AbstractVpn
Django->>ConfigConfig: ready()
ConfigConfig->>CacheInvalidationMixin: register cache dependencies
CacheInvalidationMixin->>AbstractConfig: connect config dependencies
CacheInvalidationMixin->>AbstractVpn: connect VPN dependencies
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 6
🤖 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 `@openwisp_controller/config/apps.py`:
- Line 82: The inline comment in apps.py exceeds the repository’s 88-character
flake8 limit and needs to be wrapped. Update the comment near the
DeviceChecksumView cache invalidation logic in the relevant module so it fits
within max-line-length while preserving the same meaning.
- Around line 90-95: The delete-path cache invalidation in CacheDependency is
happening too early because it uses pre_delete with on_commit=False. Update the
dependency in apps.py to use DeviceChecksumView.invalidate_get_device_cache with
post_delete and commit-time invalidation so the cache is cleared only after the
delete transaction is committed.
In `@openwisp_controller/config/base/config.py`:
- Around line 225-240: Defer the checksum invalidation work for the DeviceGroup
and OrganizationConfigSettings CacheDependency entries until after the
transaction commits, because on_commit=False can let the async recomputation run
before the saved context is visible. Update the CacheDependency registrations in
the config.Config-related dependency list to use on_commit=True so
cls._invalidate_configs_in_group and cls._invalidate_configs_in_org run only
after the save is committed.
In `@openwisp_controller/config/base/vpn.py`:
- Around line 351-356: The VPN view cache invalidation in the CacheDependency
registration for config.Vpn is happening too early, before the delete is
committed. Update the signal from pre_delete to post_delete and keep on_commit
handling so cls._invalidate_vpn_view_cache runs only after the transaction
commits, preventing GetVpnView.get_vpn from caching a row that is already
deleted.
In `@openwisp_controller/config/handlers.py`:
- Around line 72-74: The cache invalidation task in the post-save handler is
enqueued too early and may run before the surrounding transaction is committed.
Update the handler that calls tasks.invalidate_devicegroup_cache_change.delay to
wrap the enqueue in transaction.on_commit so the task is scheduled only after a
successful commit. Use the existing post-save callback logic and keep the
instance.id and instance._meta.model_name arguments unchanged.
In `@openwisp_controller/config/tests/test_config.py`:
- Around line 1025-1099: Add test coverage for the default on_commit=True
behavior in TestCacheDependency, since the current tests only exercise
on_commit=False and miss the transaction.on_commit deferral path used by real
CacheDependency wiring. Add a test around a save-triggered dependency using
self.captureOnCommitCallbacks(execute=True) so the callback is actually executed
under TestCase, and verify the target is invoked only after commit; reference
CacheDependency, _connect, and the existing post_save save flow when adding the
new test.
🪄 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: 5ff567e8-9b77-4f24-bac9-2d85ca9c32ee
📒 Files selected for processing (9)
openwisp_controller/config/apps.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_controller.py
💤 Files with no reviewable changes (2)
- openwisp_controller/config/base/device_group.py
- openwisp_controller/config/base/multitenancy.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
🪛 ast-grep (0.44.0)
openwisp_controller/config/tests/test_config.py
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/base.py
[warning] 197-197: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model(app_label, model_name)
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (7)
openwisp_controller/config/tests/test_config.py (1)
2-2: LGTM!Also applies to: 14-14, 26-26
openwisp_controller/config/tests/test_controller.py (1)
478-489: 🩺 Stability & AvailabilityCheck the cache invalidation path in this test
Ifvpn.delete()still defersGetVpnViewinvalidation withtransaction.on_commit(),TestCasewon’t run that callback automatically; wrap the delete inself.captureOnCommitCallbacks(execute=True)or use a test case that commits transactions.openwisp_controller/config/base/base.py (1)
9-35: LGTM!Also applies to: 112-329
openwisp_controller/config/apps.py (1)
5-5: LGTM!Also applies to: 45-56, 57-89, 96-151, 352-383
openwisp_controller/config/base/config.py (1)
29-35: LGTM!Also applies to: 49-49, 177-222
openwisp_controller/config/base/vpn.py (1)
36-41: LGTM!Also applies to: 51-56, 329-350, 357-365
openwisp_controller/config/handlers.py (1)
44-71: LGTM!
| tasks.invalidate_devicegroup_cache_change.delay( | ||
| instance.id, instance._meta.model_name | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Schedule device-group cache invalidation after commit.
This handler is invoked from post-save cache dependencies. Enqueuing the task immediately can let the worker read pre-commit state or run for a transaction that later rolls back. Defer the task enqueue with transaction.on_commit().
Suggested fix
- tasks.invalidate_devicegroup_cache_change.delay(
- instance.id, instance._meta.model_name
+ transaction.on_commit(
+ lambda: tasks.invalidate_devicegroup_cache_change.delay(
+ instance.id, instance._meta.model_name
+ )
)📝 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.
| tasks.invalidate_devicegroup_cache_change.delay( | |
| instance.id, instance._meta.model_name | |
| ) | |
| transaction.on_commit( | |
| lambda: tasks.invalidate_devicegroup_cache_change.delay( | |
| instance.id, instance._meta.model_name | |
| ) | |
| ) |
🤖 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 `@openwisp_controller/config/handlers.py` around lines 72 - 74, The cache
invalidation task in the post-save handler is enqueued too early and may run
before the surrounding transaction is committed. Update the handler that calls
tasks.invalidate_devicegroup_cache_change.delay to wrap the enqueue in
transaction.on_commit so the task is scheduled only after a successful commit.
Use the existing post-save callback logic and keep the instance.id and
instance._meta.model_name arguments unchanged.
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 `@openwisp_controller/config/tests/test_config.py`:
- Around line 578-584: The test is updating cert.revoked via cert.save(), but
the transaction.on_commit cache invalidation callback never runs under TestCase,
so the assertion may not exercise _resolve_cert_dependency() as intended. In
test_config.py, wrap the cert.save() call in
captureOnCommitCallbacks(execute=True) so the deferred callback is executed
before config.refresh_from_db(), and keep the existing assertions around
_resolve_cert_dependency() and the VpnClient deletion path.
🪄 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: e656bc4e-934a-4c2e-b5ae-599ad2e1e2ab
📒 Files selected for processing (2)
openwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/config/tests/test_config.py
🔇 Additional comments (4)
openwisp_controller/config/apps.py (3)
91-96: Previously flagged: delete invalidation still runs before commit.This matches the existing review comment:
pre_deletewithon_commit=Falsecan clear the device cache before the delete transaction commits, allowing stale cache repopulation before commit.
45-56: LGTM!
57-90: LGTM!Also applies to: 97-150, 351-382
openwisp_controller/config/tests/test_config.py (1)
1032-1036: LGTM!
|
coverage: 98.374% (-0.2%) from 98.547% — issues/1095-generic-cache-invalidation into master |
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 `@openwisp_controller/config/base/vpn.py`:
- Around line 1111-1115: The loop in vpn.vpnclient_set.iterator() is causing
per-client N+1 queries because client.config and its render path fetch related
objects repeatedly. Update the queryset used in this block to preload config
plus config__device and the device’s organization/group relations before
iterating, so update_status_if_checksum_changed() can run without extra database
hits while preserving the _config_modified_action assignment.
🪄 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: 89cff386-fcf6-47ad-aa46-3e736cf376f7
📒 Files selected for processing (6)
openwisp_controller/config/base/config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/admin.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/config.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
🪛 ast-grep (0.44.0)
openwisp_controller/config/tests/test_config.py
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (7)
openwisp_controller/subnet_division/admin.py (1)
28-38: LGTM!openwisp_controller/config/tests/test_config.py (3)
579-584: 🩺 Stability & AvailabilityUnaddressed: deferred
on_commitcallback never executed in this test.
cert.save()schedules the cache-invalidation callback viatransaction.on_commit(perCacheDependency), but under plainTestCasethis callback never runs unless wrapped incaptureOnCommitCallbacks(execute=True). This was already flagged in a previous review round and remains unresolved in this version.Proposed fix
- cert.revoked = False - cert.save() + with self.captureOnCommitCallbacks(execute=True): + cert.revoked = False + cert.save()
1084-1160: 🩺 Stability & AvailabilityStill missing coverage for the default
on_commit=Truedeferral path.All four
TestCacheDependencytests passon_commit=False, so the default deferred-to-transaction.on_commit()behavior used by all real production wiring (Config/Vpn) is still never exercised. This was already raised in a prior review and remains open.Add at least one test using
self.captureOnCommitCallbacks(execute=True)around a save that triggers a dependency with the defaulton_commit=True.
1014-1073: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
541-559: LGTM!openwisp_controller/config/tests/utils.py (1)
180-180: 🎯 Functional CorrectnessNo follow-up needed. The WireGuard tests that use this helper only assert VPN/client behavior; none rely on the template carrying interface entries.
> Likely an incorrect or invalid review comment.openwisp_controller/config/base/config.py (1)
212-240: LGTM!
| for client in vpn.vpnclient_set.iterator(): | ||
| # invalidate cache for device | ||
| client.config._send_config_modified_signal( | ||
| action="related_template_changed" | ||
| ) | ||
| config = client.config | ||
| # keep the historical signal action for this related change | ||
| config._config_modified_action = "related_template_changed" | ||
| config.update_status_if_checksum_changed() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where invalidate_clients_cache is wired/called to confirm execution context
rg -nP -C4 'invalidate_clients_cache' --type=pyRepository: openwisp/openwisp-controller
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and surrounding code to assess the loop, query pattern,
# and whether the recompute path is synchronous or deferred.
file="openwisp_controller/config/base/vpn.py"
echo "== File size =="
wc -l "$file"
echo "== Relevant outline =="
ast-grep outline "$file" --view expanded || true
echo "== Lines around the reported section =="
sed -n '1080,1145p' "$file" | cat -n
echo "== Search for related cache invalidation / recompute wiring =="
rg -n -C 3 'update_status_if_checksum_changed|related_template_changed|invalidate_clients_cache|vpnclient_set|select_related|iterator\(' openwisp_controller -g '*.py'Repository: openwisp/openwisp-controller
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vpn.py: save / signal / invalidate path =="
sed -n '240,360p' openwisp_controller/config/base/vpn.py | cat -n
echo
sed -n '430,470p' openwisp_controller/config/base/vpn.py | cat -n
echo
sed -n '1101,1118p' openwisp_controller/config/base/vpn.py | cat -n
echo "== config.py: checksum recompute internals =="
sed -n '690,820p' openwisp_controller/config/base/config.py | cat -n
echo "== signal wiring for vpn modified / cache invalidation =="
rg -n -C 4 'post_save.*Vpn|vpn_modified|invalidate_clients_cache|update_vpn_server_configuration|related_template_changed' openwisp_controller -g '*.py'Repository: openwisp/openwisp-controller
Length of output: 26786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Where the VPN cache-invalidation task is triggered =="
rg -n -C 3 'invalidate_vpn_server_devices_cache_change\(' openwisp_controller -g '*.py'
echo
echo "== Config checksum / render path that runs during recompute =="
sed -n '820,980p' openwisp_controller/config/base/config.py | cat -n
echo
echo "== VpnClient model fields (to assess client.config and related selects) =="
sed -n '840,890p' openwisp_controller/config/base/vpn.py | cat -nRepository: openwisp/openwisp-controller
Length of output: 10554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checksum / render path in Config =="
rg -n -C 4 'def checksum|`@property`\s+def checksum|backend_instance|get_context\(' openwisp_controller/config/base/config.py -g '*.py'
echo
echo "== Task trigger / async wiring for VPN client cache invalidation =="
rg -n -C 4 'invalidate_vpn_server_devices_cache_change|vpn_server_modified|shared_task|delay\(' openwisp_controller/config -g '*.py'Repository: openwisp/openwisp-controller
Length of output: 43166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Device helper methods used by Config.get_context() =="
rg -n -C 4 'def _get_organization__config_settings|def _get_group|def _has_group|def get_context\(' openwisp_controller/config/base -g '*.py'
echo
echo "== Surrounding Device methods =="
sed -n '1,220p' openwisp_controller/config/base/device.py | cat -nRepository: openwisp/openwisp-controller
Length of output: 15868
Avoid the per-client N+1 here. client.config and the config render path both load related objects inside this loop; preloading config, config__device, and the device’s organization/group relations will cut the extra queries.
🤖 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 `@openwisp_controller/config/base/vpn.py` around lines 1111 - 1115, The loop in
vpn.vpnclient_set.iterator() is causing per-client N+1 queries because
client.config and its render path fetch related objects repeatedly. Update the
queryset used in this block to preload config plus config__device and the
device’s organization/group relations before iterating, so
update_status_if_checksum_changed() can run without extra database hits while
preserving the _config_modified_action assignment.
Source: Coding guidelines
Test Failures in Query CountsHello @pandafy, There are multiple test failures related to unexpected query counts:
These failures suggest that the code being tested is performing more database operations than the tests are configured to expect. You should review the test files mentioned and the corresponding code to identify and resolve the N+1 query issues or optimize the database interactions. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good progress, see my comments below.
| ), | ||
| CacheDependency( | ||
| source=self.device_model, | ||
| signal="pre_delete", |
There was a problem hiding this comment.
I would move this to post_delete and let it run on commit. With pre_delete plus on_commit=False, the cache is cleared while the row is still visible. A controller request during the delete transaction can repopulate DeviceChecksumView.get_device and leave a deleted device cached after commit. The smallest fix is to use post_delete with the default on_commit=True.
| source="config.DeviceGroup", | ||
| signal="post_save", | ||
| track_fields=["context"], | ||
| on_commit=False, |
There was a problem hiding this comment.
I would not enqueue this Celery recomputation before commit. If a group or organization context is saved inside a transaction, the worker can run against the old context and decide the checksums did not change. Please use the default on_commit=True for both this dependency and the OrganizationConfigSettings dependency below.
| ), | ||
| CacheDependency( | ||
| source="config.Vpn", | ||
| signal="pre_delete", |
There was a problem hiding this comment.
Same issue as the device view cache: this should be post_delete and deferred to commit. Otherwise a request can repopulate GetVpnView.get_vpn between the early invalidation and the transaction commit, leaving the deleted VPN cached.
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)
openwisp_controller/config/base/vpn.py (1)
366-372: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefer VPN view-cache invalidation to commit
post_saveand thevpn_server_modifiedpath emitted fromVpn.save()both run before the outer transaction commits, so a concurrentget_vpn()can repopulate the cache from the old row. Switch these cache dependencies toon_commit=True; the related-object cascade already runs after commit.🤖 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 `@openwisp_controller/config/base/vpn.py` around lines 366 - 372, The VPN view-cache invalidation for the Vpn model is still happening before the surrounding transaction commits, which allows stale cache repopulation. Update the CacheDependency registration in Vpn-related cache setup to use on_commit=True for the post_save path, and ensure the vpn_server_modified flow from Vpn.save() also defers invalidation until commit; keep the related-object cascade behavior aligned with the commit-time invalidation.
🤖 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 @.github/workflows/ci.yml:
- Around line 75-76: The CI workaround in the workflow step that installs
django-x509 from the issues/236-renew-signals tarball is using a mutable branch
ref, which can break reproducibility. Update the pip install in the CI job to
pin django-x509 to a specific commit SHA instead of the branch tarball, and keep
the existing TODO/removal tracking tied to the upstream django-x509 release. Use
the workflow step containing the temporary workaround install comment to locate
it.
---
Outside diff comments:
In `@openwisp_controller/config/base/vpn.py`:
- Around line 366-372: The VPN view-cache invalidation for the Vpn model is
still happening before the surrounding transaction commits, which allows stale
cache repopulation. Update the CacheDependency registration in Vpn-related cache
setup to use on_commit=True for the post_save path, and ensure the
vpn_server_modified flow from Vpn.save() also defers invalidation until commit;
keep the related-object cascade behavior aligned with the commit-time
invalidation.
🪄 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: 04131af7-bc5d-49d4-936a-f06ffe009c06
📒 Files selected for processing (4)
.github/workflows/ci.ymlopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/pki/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
🧠 Learnings (10)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🔇 Additional comments (9)
openwisp_controller/config/tests/test_config.py (4)
1117-1166: Missing coverage for the defaulton_commit=Truedeferral path.All three tests here pass
on_commit=False, so thetransaction.on_commit()deferral — the default and the mode used by all production wiring (Config/Vpn) — is never exercised. This was already flagged in a previous review of this file.
1086-1101: LGTM!
1168-1295: LGTM!
1104-1116: 🩺 Stability & AvailabilityShared test uid is safe here
disconnect()unwinds both the main receiver and the.snapshotpre_save receiver, so this helper does not leak handlers across tests.> Likely an incorrect or invalid review comment.openwisp_controller/config/base/vpn.py (4)
373-378:pre_delete+on_commit=Falseinvalidates the view cache before the row is deleted, allowing a concurrent request to repopulateGetVpnView.get_vpnwith the soon-to-be-deleted VPN. Usepost_deletewithon_commit=True. Already raised in prior review.
1141-1154:vpn.vpnclient_set.iterator()triggers per-client N+1 queries:client.configand the checksum recompute path load related objects inside the loop. Preloadconfig,config__device, and org/group relations. Already raised in prior review.
387-403: 🎯 Functional Correctness
x509_renewedisn’t a django-x509 signal, so this sender check doesn’t apply. Wire the cache invalidation to the actual renewal hook instead.> Likely an incorrect or invalid review comment.
52-57: 📐 Maintainability & Code QualityNo MRO issue. Only
clean()is shared, and the current order keepsShareableOrgMixinUniqueName.clean()beforeBaseConfig.clean().save,get_context,get_config, andchecksumaren’t impacted.> Likely an incorrect or invalid review comment.openwisp_controller/pki/tests/test_api.py (1)
164-164: LGTM!Also applies to: 292-292
| # TODO: Remove before merging | ||
| pip install --no-cache-dir --no-deps --force-reinstall https://github.com/openwisp/django-x509/tarball/issues/236-renew-signals |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Temporary workaround install — ensure it's removed/tracked before merge.
This force-reinstalls django-x509 from a mutable branch tarball (issues/236-renew-signals) to unblock the new x509_renewed-based VPN cache invalidation (see vpn.py's CacheDependency on x509_renewed). The TODO already flags this for removal, but since the branch ref is mutable, CI could silently break if that branch is force-pushed or deleted before the upstream fix is merged/released.
Consider pinning to a specific commit SHA instead of the branch name for reproducibility while this is pending, and track removal of this step against the corresponding django-x509 release.
💡 Suggested pin to commit SHA
- # TODO: Remove before merging
- pip install --no-cache-dir --no-deps --force-reinstall https://github.com/openwisp/django-x509/tarball/issues/236-renew-signals
+ # TODO: Remove once django-x509 issue `#236` fix is released; pinned to a commit for CI reproducibility.
+ pip install --no-cache-dir --no-deps --force-reinstall https://github.com/openwisp/django-x509/archive/<commit-sha>.tar.gz🤖 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 @.github/workflows/ci.yml around lines 75 - 76, The CI workaround in the
workflow step that installs django-x509 from the issues/236-renew-signals
tarball is using a mutable branch ref, which can break reproducibility. Update
the pip install in the CI job to pin django-x509 to a specific commit SHA
instead of the branch tarball, and keep the existing TODO/removal tracking tied
to the upstream django-x509 release. Use the workflow step containing the
temporary workaround install comment to locate it.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Test Failures in OpenWISP Controller CIHello @pandafy, There are test failures in the CI pipeline. 1. Django Test Failures: The CI logs indicate a failure during the Django test execution. The traceback points to an Fix: 2. Coverage Warning: A Fix: |
| old_serial_num = ca1.serial_number | ||
| path = reverse("pki_api:ca_renew", args=[ca1.pk]) | ||
| with self.assertNumQueries(4): | ||
| with self.assertNumQueries(5): |
There was a problem hiding this comment.
Number of queries has increased because renewing a CA also invalidates the VPN checksum. In order to find related Vpn, we need to perform another query.
| old_serial_num = cert1.serial_number | ||
| path = reverse("pki_api:cert_renew", args=[cert1.pk]) | ||
| with self.assertNumQueries(5): | ||
| with self.assertNumQueries(6): |
There was a problem hiding this comment.
Number of queries has increased because renewing a Cert also invalidates the VPN and Config checksum. In order to find related Vpn, we need to perform another query.
[
{"line": 2026, "message": "Could not find pki.Ca!"}
]
|
CI Failure: Test Failures and Configuration IssuesHello @pandafy, There are two main issues identified in the CI logs:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/base/vpn.py (1)
366-371: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefer VPN cache invalidation until commit.
post_save,pre_delete, andvpn_server_modifiedall clearGetVpnViewbefore commit; another request can repopulate the cache with pre-commit VPN data and leave stale responses after the transaction finishes.🤖 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 `@openwisp_controller/config/base/vpn.py` around lines 366 - 371, The VPN cache invalidation hooks are running before the database transaction commits, which can let stale pre-commit data be cached again by another request. Update the cache invalidation setup in the Vpn class so the CacheDependency entries for post_save, pre_delete, and vpn_server_modified defer clearing GetVpnView until commit by using the existing cls._invalidate_vpn_view_cache target with on_commit enabled.
🤖 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.
Outside diff comments:
In `@openwisp_controller/config/base/vpn.py`:
- Around line 366-371: The VPN cache invalidation hooks are running before the
database transaction commits, which can let stale pre-commit data be cached
again by another request. Update the cache invalidation setup in the Vpn class
so the CacheDependency entries for post_save, pre_delete, and
vpn_server_modified defer clearing GetVpnView until commit by using the existing
cls._invalidate_vpn_view_cache target with on_commit enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7e5a077-bdc0-4cc0-a2c3-1c3d7bc7cba6
📒 Files selected for processing (5)
.github/workflows/ci.ymlopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/pki/tests/test_api.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/apps.py (1)
139-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefer these
post_deleteinvalidations to commit. Running them inside the transaction leaves a window where a concurrent request can repopulateDeviceGroupCommonNamecache with stale data before the delete commits. The handler only needsorganization_id/common_name, so this can safely be deferred.🤖 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 `@openwisp_controller/config/apps.py` around lines 139 - 149, The post_delete cache invalidations for the CacheDependency entries using devicegroup_delete_handler are running too early. Update the CacheDependency configuration for self.devicegroup_model and self.cert_model to defer the invalidation until commit by enabling on_commit, so the handler runs after the transaction is finalized. Keep the change focused on the CacheDependency definitions in apps.py and preserve the existing handler target.
♻️ Duplicate comments (1)
openwisp_controller/config/base/vpn.py (1)
1147-1160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 queries per client still present.
Same structural issue flagged in a previous review (then targeting
_send_config_modified_signal, nowupdate_status_if_checksum_changed()):vpn.vpnclient_set.iterator()has noselect_related, soclient.configand the checksum recompute path will hit the DB per client.♻️ Proposed fix
- for client in vpn.vpnclient_set.iterator(): + for client in vpn.vpnclient_set.select_related( + "config", "config__device", "config__device__group" + ).iterator(): config = client.config # keep the historical signal action for this related change config._config_modified_action = "related_template_changed" config.update_status_if_checksum_changed()🤖 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 `@openwisp_controller/config/base/vpn.py` around lines 1147 - 1160, Repeated per-client database hits remain in invalidate_clients_cache because vpn.vpnclient_set.iterator() does not prefetch related config data, so client.config and update_status_if_checksum_changed() trigger N+1 queries. Update the queryset used in invalidate_clients_cache to fetch the related config up front (for example via select_related on the client relation), then keep the existing loop and _config_modified_action assignment so the checksum/status update still happens per client without extra queries.
🤖 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.
Outside diff comments:
In `@openwisp_controller/config/apps.py`:
- Around line 139-149: The post_delete cache invalidations for the
CacheDependency entries using devicegroup_delete_handler are running too early.
Update the CacheDependency configuration for self.devicegroup_model and
self.cert_model to defer the invalidation until commit by enabling on_commit, so
the handler runs after the transaction is finalized. Keep the change focused on
the CacheDependency definitions in apps.py and preserve the existing handler
target.
---
Duplicate comments:
In `@openwisp_controller/config/base/vpn.py`:
- Around line 1147-1160: Repeated per-client database hits remain in
invalidate_clients_cache because vpn.vpnclient_set.iterator() does not prefetch
related config data, so client.config and update_status_if_checksum_changed()
trigger N+1 queries. Update the queryset used in invalidate_clients_cache to
fetch the related config up front (for example via select_related on the client
relation), then keep the existing loop and _config_modified_action assignment so
the checksum/status update still happens per client without extra queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67c8d211-2a4d-4ef0-aecd-340045ec772f
📒 Files selected for processing (7)
openwisp_controller/config/apps.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_controller.py
💤 Files with no reviewable changes (1)
- openwisp_controller/config/base/config.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.py
🔇 Additional comments (10)
openwisp_controller/config/tests/test_config.py (5)
571-587: Still missingcaptureOnCommitCallbacks— assertion doesn't exercise_resolve_cert_dependency().Only the comments changed here;
cert.save()still isn't wrapped inself.captureOnCommitCallbacks(execute=True). SinceTestConfigextendsTestCase, thetransaction.on_commitcallback that would invoke_resolve_cert_dependency()never fires, so this test passes trivially regardless of whether the "returns early because VpnClient was deleted" logic is correct. This was already raised in a previous review of this test.
932-975: LGTM!
1059-1197: LGTM!
1365-1391: LGTM!
1207-1211: 🎯 Functional CorrectnessNo issue here Each test method calls
_connect()only once, so the fixeddispatch_uiddoes not create a collision in this class.> Likely an incorrect or invalid review comment.openwisp_controller/config/base/base.py (1)
5-50: LGTM!Also applies to: 263-283, 341-355, 377-387, 407-415
openwisp_controller/config/apps.py (1)
45-155: LGTM!Also applies to: 168-253, 356-387
openwisp_controller/config/base/vpn.py (1)
340-410: LGTM!openwisp_controller/config/tests/test_api.py (1)
6-6: LGTM!Also applies to: 20-20, 1492-1517
openwisp_controller/config/tests/test_controller.py (1)
187-188: LGTM!Also applies to: 479-491
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
…rganization change
…CacheDependency class
pandafy
force-pushed
the
issues/1095-generic-cache-invalidation
branch
from
86f5e0c to
d32576a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@openwisp_controller/config/apps.py`:
- Around line 139-149: The post_delete cache invalidations for the device group
and cert dependencies are running before the transaction commits, which can race
with concurrent reads. Update the CacheDependency entries in apps.py to use
on_commit for the delete handlers, and adjust devicegroup_delete_handler so it
receives the needed snapshot data (organization_id, common_name, and model name)
instead of relying on the soon-to-be-deleted object. Make sure the delete path
still targets devicegroup_delete_handler and defers the invalidation until
commit.
- Around line 84-108: The cache invalidations wired in `CacheDependency` for
`DeviceChecksumView.invalidate_get_device_cache` and
`DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated` should
run after transaction commit instead of immediately. Update the
`CacheDependency` entries for `post_save` and `config_deactivated` in the app
config setup so they defer to commit consistently with the existing
`post_delete` handling, preventing stale `DeviceChecksumView` data from being
repopulated before the transaction is finalized.
In `@openwisp_controller/config/base/base.py`:
- Around line 263-365: The tracked-field snapshot is being reused across later
saves, so `_snapshot_handler` should consume and clear the stored snapshot after
each post-save comparison. Update the snapshot lifecycle around
`_snapshot_handler`, `_tracked_fields_changed`, and `_should_skip` so a fresh
snapshot is taken per save and any old entry in
`instance.__dict__[self._SNAPSHOT_ATTR]` for `self._uid` is removed once it has
been evaluated. This prevents stale snapshot objects from affecting unrelated
future `save(update_fields=...)` calls.
In `@openwisp_controller/config/base/vpn.py`:
- Around line 367-391: Update the CacheDependency entries in Vpn view-cache
invalidation so that both the post_save path and the vpn_server_modified signal
defer invalidation until transaction commit. In the Vpn cache dependency setup,
change the on_commit behavior for cls._invalidate_vpn_view_cache so
GetVpnView.get_vpn is not invalidated/repopulated before the transaction is
committed.
In `@openwisp_controller/config/tests/test_vpn.py`:
- Around line 573-600: The two renew-cascade tests duplicate the same
VPN/template/device setup and initial config checksum assertions, so factor that
shared logic into a small helper in test_vpn.py and keep each test focused on
its specific action (`vpn.ca.renew()` vs `vpn.cert.renew()`). Use the existing
test methods `test_ca_renew_cascades_to_client_config` and
`test_cert_renew_cascades_to_client_config`, plus the helper builders like
`_create_vpn`, `_create_template`, and `_create_device_config`, to reduce
repetition while preserving the distinct post-renew assertions and signal check.
🪄 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: bfba3d59-70c2-4c62-8109-c22c977554d8
📒 Files selected for processing (18)
.github/workflows/ci.ymlopenwisp_controller/config/apps.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tests/test_models.py
💤 Files with no reviewable changes (2)
- openwisp_controller/config/base/multitenancy.py
- openwisp_controller/config/base/device_group.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
🧠 Learnings (10)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_config.py
🪛 ast-grep (0.44.0)
openwisp_controller/subnet_division/tests/test_models.py
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "VpnClient")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/config.py
[warning] 241-241: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Organization")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/base.py
[warning] 212-212: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model(app_label, model_name)
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/tests/test_config.py
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "OrganizationConfigSettings")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (17)
.github/workflows/ci.yml (1)
75-76: Same concern already raised.This TODO workaround using a mutable branch tarball was already flagged in a previous review requesting a pinned commit SHA for reproducibility.
openwisp_controller/subnet_division/admin.py (1)
28-38: LGTM!openwisp_controller/subnet_division/rule_types/vpn.py (1)
44-58: LGTM! Reordering correctly ensuresinstance.ipis persisted beforesuper().post_provision_handler()caches the checksum, resolving theip_address_<vpn_pk>template variable at the right time — validated by the new regression test intest_models.py.openwisp_controller/subnet_division/tests/test_models.py (1)
26-26: LGTM! Regression test correctly exercises thepost_provision_handlerordering fix by asserting theip_address_<vpn_pk>context resolves and thatchecksum_dbmatches the freshly computed checksum after a DB refresh.Also applies to: 66-94
openwisp_controller/config/tests/test_device.py (1)
606-663: LGTM! Moving these cache-invalidation tests intoTestTransactionDevice(aTransactionTestCase) is necessary sinceCacheDependencydefers invalidation viatransaction.on_commit, which never fires under the defaultTestCasetransaction wrapping.openwisp_controller/config/tests/test_config.py (1)
571-975: LGTM!Also applies to: 1059-1391
openwisp_controller/config/tests/test_api.py (1)
1492-1517: LGTM!openwisp_controller/config/tests/test_controller.py (1)
187-188: LGTM!Also applies to: 479-491
openwisp_controller/config/tests/test_vpn.py (1)
519-557: LGTM!Also applies to: 601-2008
openwisp_controller/pki/tests/test_api.py (1)
164-164: LGTM!Based on learnings, "treat changes to the expected query count in
assertNumQueries(...)calls as routine test maintenance" and avoid requiring justification comments for such deltas.Also applies to: 292-292
Source: Learnings
openwisp_controller/config/tests/utils.py (1)
174-181: 🎯 Functional CorrectnessNo issue with the shared WireGuard test fixture config override
openwisp_controller/config/handlers.py (2)
72-74: Duplicate: schedule this cache invalidation after commit.The direct
.delay()call can run before the surrounding transaction commits; this was already flagged in a previous review comment for this handler.
44-60: LGTM!Also applies to: 62-71
openwisp_controller/config/base/base.py (1)
5-51: LGTM!Also applies to: 127-262, 368-416
openwisp_controller/config/apps.py (1)
5-5: LGTM!Also applies to: 50-83, 110-138, 152-155, 356-387
openwisp_controller/config/base/config.py (1)
29-35: LGTM!Also applies to: 49-49, 177-299
openwisp_controller/config/base/vpn.py (1)
18-18: LGTM!Also applies to: 37-58, 331-360, 393-410
| CacheDependency( | ||
| source=self.device_model, | ||
| signal="post_save", | ||
| on_create=True, | ||
| on_commit=False, | ||
| target=DeviceChecksumView.invalidate_get_device_cache, | ||
| ), | ||
| # Deferred to commit so a concurrent request cannot repopulate the | ||
| # cache with a device that is about to be (or was just) deleted. | ||
| # ``post_delete`` + ``_resolve_pk_snapshot`` because Django clears | ||
| # ``instance.pk`` on deleted instances before the deferred | ||
| # on_commit callback runs (see ``_resolve_pk_snapshot``). | ||
| CacheDependency( | ||
| source=self.device_model, | ||
| signal="post_delete", | ||
| resolve=_resolve_pk_snapshot, | ||
| target=DeviceChecksumView.invalidate_get_device_cache, | ||
| ), | ||
| CacheDependency( | ||
| signal_obj=config_deactivated, | ||
| name="config_deactivated", | ||
| on_commit=False, | ||
| target=( | ||
| DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant definitions and inspect the surrounding code.
git ls-files | rg '(^|/)openwisp_controller/config/apps\.py$|(^|/)cache.*\.py$|(^|/)dependencies?\.py$|(^|/)signals?\.py$'
printf '\n--- apps.py outline ---\n'
ast-grep outline openwisp_controller/config/apps.py --view expanded || true
printf '\n--- apps.py relevant section ---\n'
cat -n openwisp_controller/config/apps.py | sed -n '70,170p'
printf '\n--- search CacheDependency definition/usages ---\n'
rg -n "class CacheDependency|def .*invalidate|get_device_cache|on_commit" openwisp_controller -SRepository: openwisp/openwisp-controller
Length of output: 16607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the first search was incomplete, inspect the cache dependency implementation.
fd -a "cache" openwisp_controller || true
rg -n "class CacheDependency|on_commit|post_delete|resolve=.*_resolve_pk_snapshot|invalidate_get_device_cache_on_config_deactivated" . -SRepository: openwisp/openwisp-controller
Length of output: 10895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the cache dependency implementation if it exists in-tree.
candidate=$(rg -l "class CacheDependency" . | head -n 1 || true)
if [ -n "${candidate}" ]; then
printf '--- candidate: %s ---\n' "$candidate"
cat -n "$candidate" | sed -n '1,260p'
fiRepository: openwisp/openwisp-controller
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow read of the target file around the comment locations.
cat -n openwisp_controller/config/apps.py | sed -n '80,160p'Repository: openwisp/openwisp-controller
Length of output: 4138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the CacheDependency implementation.
printf '\n--- openwisp_controller/config/apps.py (80-160) ---\n'
cat -n openwisp_controller/config/apps.py | sed -n '80,160p'
printf '\n--- CacheDependency references ---\n'
rg -n "class CacheDependency|on_commit|post_delete|_resolve_pk_snapshot|invalidate_get_device_cache_on_config_deactivated|invalidate_get_device_cache" openwisp_controller -SRepository: openwisp/openwisp-controller
Length of output: 15389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- openwisp_controller/config/apps.py (80-160) ---\n'
cat -n openwisp_controller/config/apps.py | sed -n '80,160p'
printf '\n--- CacheDependency references ---\n'
rg -n "class CacheDependency|on_commit|post_delete|_resolve_pk_snapshot|invalidate_get_device_cache_on_config_deactivated|invalidate_get_device_cache" openwisp_controller -SRepository: openwisp/openwisp-controller
Length of output: 15389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cache dependency implementation and the device cache invalidation path.
printf '\n--- openwisp_controller/config/base/base.py (class CacheDependency) ---\n'
cat -n openwisp_controller/config/base/base.py | sed -n '39,230p'
printf '\n--- openwisp_controller/config/controller/views.py (device cache methods) ---\n'
cat -n openwisp_controller/config/controller/views.py | sed -n '170,220p'
printf '\n--- openwisp_controller/config/tests/test_api.py relevant note ---\n'
cat -n openwisp_controller/config/tests/test_api.py | sed -n '1488,1510p'Repository: openwisp/openwisp-controller
Length of output: 12047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether there are tests or comments discussing commit timing for similar invalidations.
rg -n "on_commit=False|on_commit=True|concurrent request|repopulate|stale cache|invalidate_get_device_cache" openwisp_controller/config/tests openwisp_controller/config/base openwisp_controller/config/controller -SRepository: openwisp/openwisp-controller
Length of output: 1494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact test cases that mention on_commit=False in config tests.
cat -n openwisp_controller/config/tests/test_config.py | sed -n '1200,1265p'Repository: openwisp/openwisp-controller
Length of output: 2999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the deletion/invalidation helper to understand whether immediate invalidation is safe.
cat -n openwisp_controller/config/tasks.py | sed -n '190,235p'Repository: openwisp/openwisp-controller
Length of output: 1307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for explicit transaction.atomic() contexts around device/config save operations.
rg -n "transaction\.atomic\(|atomic\(" openwisp_controller/config -SRepository: openwisp/openwisp-controller
Length of output: 1087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where config_deactivated is emitted and what transaction timing it uses.
rg -n "config_deactivated" openwisp_controller/config -S
printf '\n--- config handlers around relevant lines ---\n'
cat -n openwisp_controller/config/handlers.py | sed -n '1,170p'Repository: openwisp/openwisp-controller
Length of output: 9626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace the config_deactivated signal and its emission path.
rg -n "config_deactivated|deactivated" openwisp_controller/config -SRepository: openwisp/openwisp-controller
Length of output: 29130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the analogous VPN cache dependencies and surrounding commentary.
cat -n openwisp_controller/config/base/vpn.py | sed -n '350,400p'
# Check the CacheDependency apply/connect logic to see when immediate invalidation runs.
cat -n openwisp_controller/config/base/base.py | sed -n '340,420p'Repository: openwisp/openwisp-controller
Length of output: 6158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any tests exercising immediate invalidation timing for device/config save paths.
rg -n "DeviceChecksumView|get_device.*invalidate|cache invalidat.*commit|on_commit=False" openwisp_controller/config/tests/test_controller.py openwisp_controller/config/tests/test_device.py openwisp_controller/config/tests/test_config.py -SRepository: openwisp/openwisp-controller
Length of output: 1161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect controller tests around DeviceChecksumView caching and invalidation timing.
cat -n openwisp_controller/config/tests/test_controller.py | sed -n '120,210p'
printf '\n---\n'
cat -n openwisp_controller/config/tests/test_controller.py | sed -n '1548,1615p'Repository: openwisp/openwisp-controller
Length of output: 8337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the config tests that intentionally use on_commit=False.
cat -n openwisp_controller/config/tests/test_config.py | sed -n '1208,1260p'Repository: openwisp/openwisp-controller
Length of output: 2427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect save/deactivation paths that emit the relevant signals inside transactions.
cat -n openwisp_controller/config/base/device.py | sed -n '180,225p'
printf '\n---\n'
cat -n openwisp_controller/config/base/config.py | sed -n '900,980p'
printf '\n---\n'
cat -n openwisp_controller/config/base/config.py | sed -n '430,500p'Repository: openwisp/openwisp-controller
Length of output: 9952
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the device-cache tests assert immediate invalidation after save/deactivation.
rg -n "invalidate_get_device_cache|get_device\(\)|config_deactivated|deactivated" openwisp_controller/config/tests/test_controller.py -SRepository: openwisp/openwisp-controller
Length of output: 1800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect when the config_deactivated signal is emitted relative to the save transaction.
cat -n openwisp_controller/config/base/config.py | sed -n '680,720p'
printf '\n---\n'
cat -n openwisp_controller/config/base/config.py | sed -n '860,885p'
# Inspect any test explicitly covering cache invalidation on config deactivation.
cat -n openwisp_controller/config/tests/test_config.py | sed -n '250,285p'Repository: openwisp/openwisp-controller
Length of output: 5095
Defer these controller-view invalidations to commit. on_commit=False on post_save and config_deactivated can let a concurrent request repopulate DeviceChecksumView with stale device/config data before the transaction commits, and that stale cache can linger afterward.
Proposed fix
CacheDependency(
source=self.device_model,
signal="post_save",
on_create=True,
- on_commit=False,
target=DeviceChecksumView.invalidate_get_device_cache,
),
@@
CacheDependency(
signal_obj=config_deactivated,
name="config_deactivated",
- on_commit=False,
target=(
DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated
),📝 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.
| CacheDependency( | |
| source=self.device_model, | |
| signal="post_save", | |
| on_create=True, | |
| on_commit=False, | |
| target=DeviceChecksumView.invalidate_get_device_cache, | |
| ), | |
| # Deferred to commit so a concurrent request cannot repopulate the | |
| # cache with a device that is about to be (or was just) deleted. | |
| # ``post_delete`` + ``_resolve_pk_snapshot`` because Django clears | |
| # ``instance.pk`` on deleted instances before the deferred | |
| # on_commit callback runs (see ``_resolve_pk_snapshot``). | |
| CacheDependency( | |
| source=self.device_model, | |
| signal="post_delete", | |
| resolve=_resolve_pk_snapshot, | |
| target=DeviceChecksumView.invalidate_get_device_cache, | |
| ), | |
| CacheDependency( | |
| signal_obj=config_deactivated, | |
| name="config_deactivated", | |
| on_commit=False, | |
| target=( | |
| DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated | |
| ), | |
| CacheDependency( | |
| source=self.device_model, | |
| signal="post_save", | |
| on_create=True, | |
| target=DeviceChecksumView.invalidate_get_device_cache, | |
| ), | |
| # Deferred to commit so a concurrent request cannot repopulate the | |
| # cache with a device that is about to be (or was just) deleted. | |
| # ``post_delete`` + ``_resolve_pk_snapshot`` because Django clears | |
| # ``instance.pk`` on deleted instances before the deferred | |
| # on_commit callback runs (see ``_resolve_pk_snapshot``). | |
| CacheDependency( | |
| source=self.device_model, | |
| signal="post_delete", | |
| resolve=_resolve_pk_snapshot, | |
| target=DeviceChecksumView.invalidate_get_device_cache, | |
| ), | |
| CacheDependency( | |
| signal_obj=config_deactivated, | |
| name="config_deactivated", | |
| target=( | |
| DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated | |
| ), |
🤖 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 `@openwisp_controller/config/apps.py` around lines 84 - 108, The cache
invalidations wired in `CacheDependency` for
`DeviceChecksumView.invalidate_get_device_cache` and
`DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated` should
run after transaction commit instead of immediately. Update the
`CacheDependency` entries for `post_save` and `config_deactivated` in the app
config setup so they defer to commit consistently with the existing
`post_delete` handling, preventing stale `DeviceChecksumView` data from being
repopulated before the transaction is finalized.
| CacheDependency( | ||
| source=self.devicegroup_model, | ||
| signal="post_delete", | ||
| on_commit=False, | ||
| target=devicegroup_delete_handler, | ||
| ), | ||
| CacheDependency( | ||
| source=self.cert_model, | ||
| signal="post_delete", | ||
| on_commit=False, | ||
| target=devicegroup_delete_handler, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== apps outline ==\n'
ast-grep outline openwisp_controller/config/apps.py --view expanded || true
printf '\n== relevant slice ==\n'
sed -n '110,190p' openwisp_controller/config/apps.py
printf '\n== search cache dependency / handler ==\n'
rg -n "devicegroup_delete_handler|CacheDependency|post_delete|on_commit|common_name" openwisp_controller -SRepository: openwisp/openwisp-controller
Length of output: 28934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files mentioning devicegroup_delete_handler ==\n'
rg -n "devicegroup_delete_handler" -S .
printf '\n== files mentioning DeviceGroupCommonName ==\n'
rg -n "DeviceGroupCommonName" -S openwisp_controller
printf '\n== files mentioning CacheDependency\(.*post_delete ==\n'
rg -n "CacheDependency\\(|signal=\"post_delete\"" -S openwisp_controllerRepository: openwisp/openwisp-controller
Length of output: 4421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== openwisp_controller/config/handlers.py relevant slice ==\n'
sed -n '1,180p' openwisp_controller/config/handlers.py
printf '\n== openwisp_controller/config/base/base.py relevant slice ==\n'
sed -n '1,460p' openwisp_controller/config/base/base.py
printf '\n== openwisp_controller/config/apps.py relevant slice around cache deps ==\n'
sed -n '60,160p' openwisp_controller/config/apps.py
printf '\n== regression tests around delete cache dependencies ==\n'
sed -n '920,980p' openwisp_controller/config/tests/test_config.py
sed -n '1488,1525p' openwisp_controller/config/tests/test_api.pyRepository: openwisp/openwisp-controller
Length of output: 31230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== openwisp_controller/config/tasks.py relevant slice ==\n'
sed -n '70,135p' openwisp_controller/config/tasks.py
printf '\n== openwisp_controller/config/api/views.py relevant slice around delete invalidation ==\n'
sed -n '160,300p' openwisp_controller/config/api/views.py
printf '\n== openwisp_controller/config/tests/test_api.py regression section around delete invalidation ==\n'
sed -n '1488,1518p' openwisp_controller/config/tests/test_api.pyRepository: openwisp/openwisp-controller
Length of output: 9155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("openwisp_controller/config/tasks.py")
text = p.read_text()
for needle in [
"invalidate_devicegroup_cache_delete",
"devicegroup_delete_invalidates_cache",
"certificate_delete_invalidates_cache",
]:
idx = text.find(needle)
print(f"== {needle} @ {idx} ==")
if idx != -1:
start = max(0, text.rfind("\n", 0, idx-400))
end = min(len(text), idx+800)
print(text[start:end])
print()
PYRepository: openwisp/openwisp-controller
Length of output: 4047
Defer these delete invalidations to on_commit
These post_delete hooks can race with concurrent reads and repopulate DeviceGroupCommonName with data for an object that is being deleted. Snapshot the needed delete attributes (organization_id, common_name, model name) and defer the task until commit.
🤖 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 `@openwisp_controller/config/apps.py` around lines 139 - 149, The post_delete
cache invalidations for the device group and cert dependencies are running
before the transaction commits, which can race with concurrent reads. Update the
CacheDependency entries in apps.py to use on_commit for the delete handlers, and
adjust devicegroup_delete_handler so it receives the needed snapshot data
(organization_id, common_name, and model name) instead of relying on the
soon-to-be-deleted object. Make sure the delete path still targets
devicegroup_delete_handler and defers the invalidation until commit.
| def _snapshot_handler(self, sender, instance, **kwargs): | ||
| """Store the old values of ``track_fields`` before the instance saves.""" | ||
| if instance._state.adding or instance.pk is None: | ||
| return | ||
| fields = self._get_fields_to_track(instance, **kwargs) | ||
| if not fields: | ||
| return | ||
| snapshot, db_fields = self._snapshot_track_fields_from_initial_values( | ||
| instance, fields=fields | ||
| ) | ||
| if db_fields: | ||
| db_snapshot = self._snapshot_track_fields_from_db( | ||
| sender, instance, fields=db_fields | ||
| ) | ||
| if db_snapshot is None: | ||
| return | ||
| snapshot.update(db_snapshot) | ||
| if snapshot is None: | ||
| return | ||
| snapshots = instance.__dict__.setdefault(self._SNAPSHOT_ATTR, {}) | ||
| snapshots[self._uid] = snapshot | ||
|
|
||
| def _get_fields_to_track(self, instance, **kwargs): | ||
| fields = list(self.track_fields or []) | ||
| if not fields: | ||
| return fields | ||
| update_fields = kwargs.get("update_fields") | ||
| # Full save: all tracked fields could have changed. | ||
| if update_fields is None: | ||
| return fields | ||
| # save(update_fields=[...]) narrows the set of potentially changed fields. | ||
| # Expand names to include both field.name and field.attname so a tracked | ||
| # field like ``organization_id`` matches ``organization`` updates. | ||
| expanded = set(update_fields) | ||
| for name in list(update_fields): | ||
| try: | ||
| model_field = instance._meta.get_field(name) | ||
| except FieldDoesNotExist: | ||
| continue | ||
| expanded.add(model_field.name) | ||
| expanded.add(model_field.attname) | ||
| return [field for field in fields if field in expanded] | ||
|
|
||
| def _snapshot_track_fields_from_initial_values(self, instance, fields=None): | ||
| """ | ||
| Returns a tuple ``(snapshot, db_fields)`` where ``snapshot`` contains | ||
| values obtained from ``_initial_<field>`` attrs (or ``models.DEFERRED`` | ||
| for still deferred fields), while ``db_fields`` contains unresolved | ||
| fields which must be fetched from DB. | ||
| """ | ||
| fields = fields or self.track_fields or [] | ||
| if not fields: | ||
| return dict(), [] | ||
| deferred_fields = instance.get_deferred_fields() | ||
| snapshot = dict() | ||
| db_fields = [] | ||
| missing = object() | ||
| for field in fields: | ||
| attr = f"_initial_{field}" | ||
| value = getattr(instance, attr, missing) | ||
| if value is not missing and value != models.DEFERRED: | ||
| snapshot[field] = value | ||
| elif field in deferred_fields: | ||
| snapshot[field] = models.DEFERRED | ||
| else: | ||
| db_fields.append(field) | ||
| return snapshot, db_fields | ||
|
|
||
| def _snapshot_track_fields_from_db(self, sender, instance, fields=None): | ||
| fields = fields or self.track_fields or [] | ||
| if not fields: | ||
| return dict() | ||
| try: | ||
| old = sender._default_manager.only(*fields).get(pk=instance.pk) | ||
| except sender.DoesNotExist: | ||
| return None | ||
| return {field: getattr(old, field) for field in fields} | ||
|
|
||
| def _tracked_fields_changed(self, instance): | ||
| snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) or {} | ||
| old = snapshots.get(self._uid) | ||
| if old is None: | ||
| # No snapshot (e.g. on creation) -> nothing to compare against. | ||
| return False | ||
| deferred_fields = instance.get_deferred_fields() | ||
| for field, old_value in old.items(): | ||
| if field in deferred_fields: | ||
| continue | ||
| if old_value == models.DEFERRED: | ||
| return True | ||
| if old_value != getattr(instance, field): | ||
| return True | ||
| return False | ||
|
|
||
| def _should_skip(self, instance, **kwargs): | ||
| if ( | ||
| self.signal is post_save | ||
| and kwargs.get("created", False) | ||
| and not self.on_create | ||
| ): | ||
| return True | ||
| if self.track_fields and not self._tracked_fields_changed(instance): | ||
| return True |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Consume tracked-field snapshots after each post-save comparison.
Line 267 can skip creating a fresh snapshot, but Line 343 reuses any old snapshot left on the instance. Reusing the same Python object after a tracked-field change can therefore trigger cache invalidation on later unrelated save(update_fields=...) calls.
Proposed fix
fields = self._get_fields_to_track(instance, **kwargs)
if not fields:
+ snapshots = getattr(instance, self._SNAPSHOT_ATTR, None)
+ if snapshots is not None:
+ snapshots.pop(self._uid, None)
return
@@
snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) or {}
- old = snapshots.get(self._uid)
+ old = snapshots.pop(self._uid, None)📝 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.
| def _snapshot_handler(self, sender, instance, **kwargs): | |
| """Store the old values of ``track_fields`` before the instance saves.""" | |
| if instance._state.adding or instance.pk is None: | |
| return | |
| fields = self._get_fields_to_track(instance, **kwargs) | |
| if not fields: | |
| return | |
| snapshot, db_fields = self._snapshot_track_fields_from_initial_values( | |
| instance, fields=fields | |
| ) | |
| if db_fields: | |
| db_snapshot = self._snapshot_track_fields_from_db( | |
| sender, instance, fields=db_fields | |
| ) | |
| if db_snapshot is None: | |
| return | |
| snapshot.update(db_snapshot) | |
| if snapshot is None: | |
| return | |
| snapshots = instance.__dict__.setdefault(self._SNAPSHOT_ATTR, {}) | |
| snapshots[self._uid] = snapshot | |
| def _get_fields_to_track(self, instance, **kwargs): | |
| fields = list(self.track_fields or []) | |
| if not fields: | |
| return fields | |
| update_fields = kwargs.get("update_fields") | |
| # Full save: all tracked fields could have changed. | |
| if update_fields is None: | |
| return fields | |
| # save(update_fields=[...]) narrows the set of potentially changed fields. | |
| # Expand names to include both field.name and field.attname so a tracked | |
| # field like ``organization_id`` matches ``organization`` updates. | |
| expanded = set(update_fields) | |
| for name in list(update_fields): | |
| try: | |
| model_field = instance._meta.get_field(name) | |
| except FieldDoesNotExist: | |
| continue | |
| expanded.add(model_field.name) | |
| expanded.add(model_field.attname) | |
| return [field for field in fields if field in expanded] | |
| def _snapshot_track_fields_from_initial_values(self, instance, fields=None): | |
| """ | |
| Returns a tuple ``(snapshot, db_fields)`` where ``snapshot`` contains | |
| values obtained from ``_initial_<field>`` attrs (or ``models.DEFERRED`` | |
| for still deferred fields), while ``db_fields`` contains unresolved | |
| fields which must be fetched from DB. | |
| """ | |
| fields = fields or self.track_fields or [] | |
| if not fields: | |
| return dict(), [] | |
| deferred_fields = instance.get_deferred_fields() | |
| snapshot = dict() | |
| db_fields = [] | |
| missing = object() | |
| for field in fields: | |
| attr = f"_initial_{field}" | |
| value = getattr(instance, attr, missing) | |
| if value is not missing and value != models.DEFERRED: | |
| snapshot[field] = value | |
| elif field in deferred_fields: | |
| snapshot[field] = models.DEFERRED | |
| else: | |
| db_fields.append(field) | |
| return snapshot, db_fields | |
| def _snapshot_track_fields_from_db(self, sender, instance, fields=None): | |
| fields = fields or self.track_fields or [] | |
| if not fields: | |
| return dict() | |
| try: | |
| old = sender._default_manager.only(*fields).get(pk=instance.pk) | |
| except sender.DoesNotExist: | |
| return None | |
| return {field: getattr(old, field) for field in fields} | |
| def _tracked_fields_changed(self, instance): | |
| snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) or {} | |
| old = snapshots.get(self._uid) | |
| if old is None: | |
| # No snapshot (e.g. on creation) -> nothing to compare against. | |
| return False | |
| deferred_fields = instance.get_deferred_fields() | |
| for field, old_value in old.items(): | |
| if field in deferred_fields: | |
| continue | |
| if old_value == models.DEFERRED: | |
| return True | |
| if old_value != getattr(instance, field): | |
| return True | |
| return False | |
| def _should_skip(self, instance, **kwargs): | |
| if ( | |
| self.signal is post_save | |
| and kwargs.get("created", False) | |
| and not self.on_create | |
| ): | |
| return True | |
| if self.track_fields and not self._tracked_fields_changed(instance): | |
| return True | |
| def _snapshot_handler(self, sender, instance, **kwargs): | |
| """Store the old values of ``track_fields`` before the instance saves.""" | |
| if instance._state.adding or instance.pk is None: | |
| return | |
| fields = self._get_fields_to_track(instance, **kwargs) | |
| if not fields: | |
| snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) | |
| if snapshots is not None: | |
| snapshots.pop(self._uid, None) | |
| return | |
| snapshot, db_fields = self._snapshot_track_fields_from_initial_values( | |
| instance, fields=fields | |
| ) | |
| if db_fields: | |
| db_snapshot = self._snapshot_track_fields_from_db( | |
| sender, instance, fields=db_fields | |
| ) | |
| if db_snapshot is None: | |
| return | |
| snapshot.update(db_snapshot) | |
| if snapshot is None: | |
| return | |
| snapshots = instance.__dict__.setdefault(self._SNAPSHOT_ATTR, {}) | |
| snapshots[self._uid] = snapshot | |
| def _get_fields_to_track(self, instance, **kwargs): | |
| fields = list(self.track_fields or []) | |
| if not fields: | |
| return fields | |
| update_fields = kwargs.get("update_fields") | |
| # Full save: all tracked fields could have changed. | |
| if update_fields is None: | |
| return fields | |
| # save(update_fields=[...]) narrows the set of potentially changed fields. | |
| # Expand names to include both field.name and field.attname so a tracked | |
| # field like ``organization_id`` matches ``organization`` updates. | |
| expanded = set(update_fields) | |
| for name in list(update_fields): | |
| try: | |
| model_field = instance._meta.get_field(name) | |
| except FieldDoesNotExist: | |
| continue | |
| expanded.add(model_field.name) | |
| expanded.add(model_field.attname) | |
| return [field for field in fields if field in expanded] | |
| def _snapshot_track_fields_from_initial_values(self, instance, fields=None): | |
| """ | |
| Returns a tuple ``(snapshot, db_fields)`` where ``snapshot`` contains | |
| values obtained from ``_initial_<field>`` attrs (or ``models.DEFERRED`` | |
| for still deferred fields), while ``db_fields`` contains unresolved | |
| fields which must be fetched from DB. | |
| """ | |
| fields = fields or self.track_fields or [] | |
| if not fields: | |
| return dict(), [] | |
| deferred_fields = instance.get_deferred_fields() | |
| snapshot = dict() | |
| db_fields = [] | |
| missing = object() | |
| for field in fields: | |
| attr = f"_initial_{field}" | |
| value = getattr(instance, attr, missing) | |
| if value is not missing and value != models.DEFERRED: | |
| snapshot[field] = value | |
| elif field in deferred_fields: | |
| snapshot[field] = models.DEFERRED | |
| else: | |
| db_fields.append(field) | |
| return snapshot, db_fields | |
| def _snapshot_track_fields_from_db(self, sender, instance, fields=None): | |
| fields = fields or self.track_fields or [] | |
| if not fields: | |
| return dict() | |
| try: | |
| old = sender._default_manager.only(*fields).get(pk=instance.pk) | |
| except sender.DoesNotExist: | |
| return None | |
| return {field: getattr(old, field) for field in fields} | |
| def _tracked_fields_changed(self, instance): | |
| snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) or {} | |
| old = snapshots.pop(self._uid, None) | |
| if old is None: | |
| # No snapshot (e.g. on creation) -> nothing to compare against. | |
| return False | |
| deferred_fields = instance.get_deferred_fields() | |
| for field, old_value in old.items(): | |
| if field in deferred_fields: | |
| continue | |
| if old_value == models.DEFERRED: | |
| return True | |
| if old_value != getattr(instance, field): | |
| return True | |
| return False | |
| def _should_skip(self, instance, **kwargs): | |
| if ( | |
| self.signal is post_save | |
| and kwargs.get("created", False) | |
| and not self.on_create | |
| ): | |
| return True | |
| if self.track_fields and not self._tracked_fields_changed(instance): | |
| return True |
🤖 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 `@openwisp_controller/config/base/base.py` around lines 263 - 365, The
tracked-field snapshot is being reused across later saves, so
`_snapshot_handler` should consume and clear the stored snapshot after each
post-save comparison. Update the snapshot lifecycle around `_snapshot_handler`,
`_tracked_fields_changed`, and `_should_skip` so a fresh snapshot is taken per
save and any old entry in `instance.__dict__[self._SNAPSHOT_ATTR]` for
`self._uid` is removed once it has been evaluated. This prevents stale snapshot
objects from affecting unrelated future `save(update_fields=...)` calls.
| CacheDependency( | ||
| source="config.Vpn", | ||
| signal="post_save", | ||
| on_create=True, | ||
| on_commit=False, | ||
| target=cls._invalidate_vpn_view_cache, | ||
| ), | ||
| # Deferred to commit so a concurrent request cannot repopulate the | ||
| # cache with a VPN that is about to be (or was just) deleted. | ||
| # ``post_delete`` + ``_resolve_pk_snapshot`` because Django clears | ||
| # ``instance.pk`` on deleted instances before the deferred | ||
| # on_commit callback runs (see ``_resolve_pk_snapshot``). | ||
| CacheDependency( | ||
| source="config.Vpn", | ||
| signal="post_delete", | ||
| resolve=_resolve_pk_snapshot, | ||
| target=cls._invalidate_vpn_view_cache, | ||
| ), | ||
| # A change to the VPN server configuration (e.g. via related objects) | ||
| # emits ``vpn_server_modified`` and must invalidate the view cache. | ||
| CacheDependency( | ||
| signal_obj=vpn_server_modified, | ||
| name="vpn_server_modified", | ||
| on_commit=False, | ||
| target=cls._invalidate_vpn_view_cache, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- openwisp_controller/config/base/vpn.py (around the cited lines) ---\n'
sed -n '330,430p' openwisp_controller/config/base/vpn.py
printf '\n--- locate CacheDependency definition/usages ---\n'
rg -n "class CacheDependency|CacheDependency\(" -S .
printf '\n--- find vpn_server_modified definition/usages ---\n'
rg -n "vpn_server_modified" -S .Repository: openwisp/openwisp-controller
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the CacheDependency implementation and any transaction helpers it uses.
for f in $(rg -l "class CacheDependency|def .*on_commit|transaction.on_commit" -S .); do
echo "--- $f ---"
wc -l "$f"
done
# Print the most relevant implementation slices if found.
for f in $(rg -l "class CacheDependency" -S .); do
echo "--- $f (definition) ---"
sed -n '1,260p' "$f"
doneRepository: openwisp/openwisp-controller
Length of output: 10887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- openwisp_controller/config/base/vpn.py around the cited block ---'
sed -n '350,410p' openwisp_controller/config/base/vpn.py
echo
echo '--- openwisp_controller/config/base/base.py handler methods around on_commit ---'
sed -n '260,430p' openwisp_controller/config/base/base.pyRepository: openwisp/openwisp-controller
Length of output: 9236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('openwisp_controller/config/base/vpn.py')
text = p.read_text()
for needle in [
'CacheDependency(',
'signal="post_save"',
'signal_obj=vpn_server_modified',
'on_commit=False',
'_invalidate_vpn_view_cache',
]:
print(f'--- occurrences for {needle!r} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: openwisp/openwisp-controller
Length of output: 977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- vpn_server_modified definition and emit sites ---'
rg -n -A3 -B3 "vpn_server_modified|send\\(|emit" openwisp_controller/config/base/vpn.py openwisp_controller/config/base -S
echo
echo '--- _invalidate_vpn_view_cache implementation ---'
sed -n '320,350p' openwisp_controller/config/base/vpn.pyRepository: openwisp/openwisp-controller
Length of output: 22147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for transaction.atomic around VPN saves or signal emissions.
rg -n -A4 -B4 "atomic\\(|vpn_server_modified|post_save|save\\(" openwisp_controller/config/base/vpn.py openwisp_controller/config/base/base.py openwisp_controller/config/base/config.py -SRepository: openwisp/openwisp-controller
Length of output: 24983
Defer VPN view-cache invalidation to commit.
post_save and vpn_server_modified can both run inside an open transaction; invalidating GetVpnView.get_vpn immediately lets a concurrent request repopulate the cache from the pre-commit row.
🤖 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 `@openwisp_controller/config/base/vpn.py` around lines 367 - 391, Update the
CacheDependency entries in Vpn view-cache invalidation so that both the
post_save path and the vpn_server_modified signal defer invalidation until
transaction commit. In the Vpn cache dependency setup, change the on_commit
behavior for cls._invalidate_vpn_view_cache so GetVpnView.get_vpn is not
invalidated/repopulated before the transaction is committed.
| def test_ca_renew_cascades_to_client_config(self): | ||
| vpn = self._create_vpn() | ||
| vpn_template = self._create_template( | ||
| name="vpn-template", type="vpn", vpn=vpn, config={} | ||
| ) | ||
| device = self._create_device_config() | ||
| device.config.templates.add(vpn_template) | ||
| config = Config.objects.get(pk=device.config.pk) | ||
| old_checksum_db = config.checksum_db | ||
| self.assertEqual(old_checksum_db, config.checksum) | ||
| vpn.ca.renew() | ||
| config = Config.objects.get(pk=device.config.pk) | ||
| self.assertNotEqual(config.checksum_db, old_checksum_db) | ||
| self.assertEqual(config.checksum_db, config.checksum) | ||
| self.assertEqual(config.status, "modified") | ||
|
|
||
| def test_cert_renew_cascades_to_client_config(self): | ||
| vpn = self._create_vpn() | ||
| vpn_template = self._create_template( | ||
| name="vpn-template", type="vpn", vpn=vpn, config={} | ||
| ) | ||
| device = self._create_device_config() | ||
| device.config.templates.add(vpn_template) | ||
| config = Config.objects.get(pk=device.config.pk) | ||
| self.assertEqual(config.checksum_db, config.checksum) | ||
| with catch_signal(vpn_server_modified) as mocked_vpn_server_modified: | ||
| vpn.cert.renew() | ||
| mocked_vpn_server_modified.assert_called_once() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor duplication between renew-cascade tests.
test_ca_renew_cascades_to_client_config and test_cert_renew_cascades_to_client_config share nearly identical setup (vpn/template/device/config creation and checksum assertions). Could be factored into a shared helper, but low priority given clarity of standalone tests.
🤖 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 `@openwisp_controller/config/tests/test_vpn.py` around lines 573 - 600, The two
renew-cascade tests duplicate the same VPN/template/device setup and initial
config checksum assertions, so factor that shared logic into a small helper in
test_vpn.py and keep each test focused on its specific action (`vpn.ca.renew()`
vs `vpn.cert.renew()`). Use the existing test methods
`test_ca_renew_cascades_to_client_config` and
`test_cert_renew_cascades_to_client_config`, plus the helper builders like
`_create_vpn`, `_create_template`, and `_create_device_config`, to reduce
repetition while preserving the distinct post-renew assertions and signal check.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
3 participants
Checklist
Reference to Existing Issue
Closes #1095
Blockers