[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 24 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces declarative cache invalidation through Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors)
✅ Passed checks (5 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!
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.45% (-0.1%) 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!
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
|
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. |
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
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Flake8 E501 Line Too LongHello @pandafy, The CI build failed because of a To fix this:
After fixing the line, please commit the changes and push again. |
Migrations Check FailedHello @pandafy, There are multiple CI jobs failing due to a "Migrations check failed" error. This indicates that the database migrations have not been applied correctly. Fix: ./manage.py makemigrations
./manage.py migrateAfter applying the migrations locally, commit the changes and push again. |
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_vpn.py`:
- Around line 1564-1611: The test currently only checks the checksum outcome and
skips verifying that `vpn.save()` schedules the invalidation task through the
`on_commit` path. Update `test_zerotier_vpn_name_change_updates_client_checksum`
to mirror the `test_config.py` regression tests by mocking
`invalidate_vpn_server_devices_cache_change.delay`, wrapping the save in
`captureOnCommitCallbacks(execute=True)`, and asserting the task is queued from
`vpn.save()` before checking the checksum change. Keep the existing downstream
checksum assertions, but remove the direct manual call to
`invalidate_vpn_server_devices_cache_change(vpn.pk)` so the `Vpn.name`
cache-dependency wiring is exercised end to end.
🪄 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: 00e6ed00-927c-4cde-afc4-34522c0e3c25
📒 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
⚠️ CI failures not shown inline (11)
GitHub Actions: OpenWISP Controller CI Build / 3_Python==3.13 django~=5.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.13.14/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 7_Python==3.10 django~=5.1.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.10.20/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 4_Python==3.13 django~=5.1.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.13.14/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 5_Python==3.12 django~=4.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 1_Python==3.12 django~=5.1.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 6_Python==3.10 django~=5.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.10.20/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 8_Python==3.10 django~=4.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.10.20/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 2_Python==3.11 django~=4.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 10_Python==3.11 django~=5.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 9_Python==3.11 django~=5.1.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
GitHub Actions: OpenWISP Controller CI Build / 11_Python==3.12 django~=5.2.0.txt: [feature] Added generic solution for cache invalidation in config models #1095
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Migration name check on "./openwisp_controller/config/migrations" with migrations-to-ignore: 12 successful!
SUCCESS: Migration name check on "./openwisp_controller/connection/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/geo/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./openwisp_controller/pki/migrations" with migrations-to-ignore: 4 successful!
SUCCESS: Migration name check on "./openwisp_controller/subnet_division/migrations" with migrations-to-ignore: 0 successful!
SUCCESS: Isort check successful!
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Checking formatting...
All matched files use Prettier code style!
SUCCESS: Prettier check successful!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
SUCCESS: Migrations check successful!
Running checks for SAMPLE_APP
SUCCESS: Migration name check on "./tests/openwisp2/sample_config/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_pki/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_connection/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tests/openwisp2/sample_geo/migrations/" with migrations-to-ignore: 0 successful!
SUCCESS: Migration name check on "./tes...
🧰 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/rule_types/vpn.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/rule_types/vpn.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/rule_types/vpn.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/rule_types/vpn.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.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
🪛 ast-grep (0.44.1)
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)
openwisp_controller/config/tests/test_vpn.py
[info] 1585-1585: Do not hardcode temporary file or directory names
Context: "/tmp/test"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🔇 Additional comments (27)
.github/workflows/ci.yml (1)
75-76: Duplicate of previously flagged mutable-ref workaround.Already flagged: force-reinstalling
django-x509from a mutable branch tarball is fragile for CI reproducibility; pin to a commit SHA instead.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 resolved before the superclass caches the checksum, matching the new regression test.openwisp_controller/subnet_division/tests/test_models.py (1)
26-26: LGTM! Solid regression test tying the IP-resolution-before-checksum-caching fix to an explicit assertion.Also applies to: 66-94
openwisp_controller/config/tests/utils.py (1)
168-181: LGTM!openwisp_controller/config/tests/test_device.py (1)
606-663: LGTM! Moving these toTestTransactionDeviceis consistent with the new commit-deferred cache invalidation model.openwisp_controller/pki/tests/test_api.py (1)
160-169: LGTM! Query count increases are expected and already explained (see prior maintainer comments) — renewing a CA/Cert now triggers an additional query to locate the related VPN for checksum invalidation.Also applies to: 288-297
openwisp_controller/config/base/base.py (3)
320-343: 🩺 Stability & Availability | ⚡ Quick winStale tracked-field snapshot can trigger spurious invalidation.
_tracked_fields_changedreadssnapshots.get(self._uid)but the snapshot is never consumed/cleared after a post-save comparison. A latersave(update_fields=[...])that excludes tracked fields skips creating a fresh snapshot (_may_track_fields_changereturnsFalse), so_should_skipcompares the current value against a leftover snapshot, which can fire the target on an unrelated save.Proposed fix
def _tracked_fields_changed(self, instance): snapshots = getattr(instance, self._SNAPSHOT_ATTR, None) or {} - old = snapshots.get(self._uid) + old = snapshots.pop(self._uid, None) if old is None:
39-51: LGTM!
127-235: LGTM!Also applies to: 236-261, 344-394
openwisp_controller/config/base/vpn.py (3)
1157-1161: 🚀 Performance & Scalability | 💤 Low valuePer-client N+1 in
invalidate_clients_cache.
client.configand the checksum render path load related objects inside the loop. Preloadingconfig,config__device, and the device's organization/group relations on the queryset would cut the extra queries.
53-58: LGTM!Also applies to: 322-411
18-18: 🩺 Stability & AvailabilityCheck the pinned
django-x509snapshot exportsx509_renewed
Make surerefs/heads/1.4.tar.gzincludesdjango_x509.signals.x509_renewed; otherwise this import will fail at startup.openwisp_controller/config/handlers.py (2)
62-74: 🗄️ Data Integrity & Integration | ⚡ Quick winChange-path cache invalidation is enqueued before commit.
Wired via
CacheDependency(on_commit=False)for thedevice_group_changed,DeviceGroup.post_save, andCert.post_savepaths,.delay()runs at signal time. A worker can pick up the task and read pre-commit (or rolled-back) state. Unlike thepost_deletehandlers, these change paths have no cascade constraint preventingtransaction.on_commit().
44-59: LGTM!openwisp_controller/config/apps.py (1)
57-159: LGTM!Also applies to: 360-391
openwisp_controller/config/base/config.py (1)
29-35: LGTM!Also applies to: 49-49, 177-298
openwisp_controller/config/tests/test_vpn.py (3)
521-524: 🎯 Functional CorrectnessSame parenthesized multi-context-manager syntax concern as flagged in
test_controller.py.These occurrences use the same
with (a, b):syntax that requires Python 3.10 to be officially supported. Same verification applies here; see the companion comment and script intest_controller.py.Also applies to: 901-906, 923-926, 2013-2019, 2025-2028, 2079-2082
573-601: 📐 Maintainability & Code QualityDuplication between renew-cascade tests remains unaddressed.
test_ca_renew_cascades_to_client_configandtest_cert_renew_cascades_to_client_configstill share nearly identical vpn/template/device setup and checksum assertions, as flagged in a prior review round.
540-559: LGTM!Also applies to: 1538-1561
openwisp_controller/config/tests/test_config.py (4)
571-587: Comment now correctly documents the early-return behavior.The updated comments correctly explain why wrapping
cert.save()incaptureOnCommitCallbacksisn't needed here: since theVpnClientwas already deleted during deactivation,resolve()returns no objects, so_handlernever schedules anon_commitcallback in the first place. This addresses the earlier review concern without needing the wrap.
932-975: Good addition: closes the previously-flagged on_commit=True coverage gap.These two tests exercise the default
on_commit=Truedeferral path via real model wiring (DeviceGroup/OrganizationConfigSettings) usingcaptureOnCommitCallbacks(execute=True)plus assertion that the delayed call isn't made until commit. This directly addresses the earlier reviewer request for coverage of the default on_commit path.
1059-1197: LGTM!
1200-1389: LGTM!openwisp_controller/config/tests/test_api.py (1)
1492-1517: Solid regression test for the pk-reset-before-on_commit hazard.Good regression coverage for the cascade-delete scenario the docstring describes (Django resets
pkon collected instances only after allpost_deletesignals fire, soresolve()inside_handlermust capture what it needs synchronously, before scheduling_applyon commit). Since this is aTransactionTestCase,org.delete()auto-commits and theon_commitcallback fires without needingcaptureOnCommitCallbacks, which is correct here.openwisp_controller/config/tests/test_controller.py (2)
298-309: LGTM!Also applies to: 1408-1656
1662-1667: 🎯 Functional CorrectnessNo issue: the suite is Python 3.10+ only
The project declares Python >=3.10 in the docs and CI, so this
with (...)form is valid here.> Likely an incorrect or invalid review comment.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
pandafy
force-pushed
the
issues/1095-generic-cache-invalidation
branch
from
d609b0e to
699c63d
Compare
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
|
@coderabbitai full review! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/base.py`:
- Around line 222-240: The build_dispatch_uid method currently omits
dependency-defining attributes, allowing declarations with different resolvers,
tracked fields, or timing to collide and overwrite registry entries. Update
build_dispatch_uid to incorporate all attributes that distinguish a dependency,
including resolver, tracked fields, and timing, or require and include an
explicit unique name; ensure the resulting UID remains stable and uniquely
identifies each declaration.
- Around line 301-333: Update the human-readable branch of
CacheDependency.render_registered to wrap the empty-state message and labels
such as “target”, “resolve”, “track_fields”, “on_create”, “on_commit”, and “uid”
with Django gettext, using the module’s existing translation import/convention.
Keep the JSON branch and machine-readable keys unchanged.
In `@openwisp_controller/config/handlers.py`:
- Around line 72-75: Remove the early return for list instances in the
device-group change handler and invalidate caches for every affected
device-group ID. Use a bulk-capable invalidation task if available, otherwise
enqueue the existing invalidation task once per ID, while preserving the
existing single-instance behavior.
In `@openwisp_controller/config/tests/test_vpn.py`:
- Around line 605-616: Update test_cert_renew_cascades_to_client_config to
mirror test_ca_renew_cascades_to_client_config: capture the initial
config.checksum_db, renew vpn.cert, refetch the Config from the database, and
assert checksum_db changed and equals the freshly computed checksum, while
retaining the vpn_server_modified signal assertion.
🪄 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: 370df7b4-11d4-45c1-a4c4-4ed55360a659
📒 Files selected for processing (20)
docs/developer/utils.rstopenwisp_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/management/commands/print_cache_dependencies.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/tasks.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
🧰 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/pki/tests/test_api.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.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/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.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/pki/tests/test_api.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.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/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.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/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/subnet_division/admin.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.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/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/pki/tests/test_api.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/pki/tests/test_api.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/pki/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/pki/tests/test_api.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
🪛 ast-grep (0.44.1)
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/base.py
[info] 310-310: use jsonify instead of json.dumps for JSON output
Context: json.dumps([dep.describe() for dep in dependencies], indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 218-218: 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_vpn.py
[info] 1600-1600: Do not hardcode temporary file or directory names
Context: "/tmp/test"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
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/tests/test_config.py
[warning] 30-30: 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] 31-31: 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 (33)
openwisp_controller/config/management/commands/print_cache_dependencies.py (2)
1-6: LGTM!Also applies to: 12-16, 18-21
7-17: 📐 Maintainability & Code QualityWrap user-facing management-command help text in
gettext_lazy.
Use Django i18n for thehelpstrings here if they are exposed to users.docs/developer/utils.rst (1)
410-499: LGTM!openwisp_controller/pki/tests/test_api.py (1)
164-164: LGTM!Also applies to: 292-292
openwisp_controller/subnet_division/admin.py (1)
28-38: LGTM!openwisp_controller/subnet_division/tasks.py (2)
41-47: LGTM!
81-85: LGTM!openwisp_controller/subnet_division/tests/test_models.py (3)
366-372: LGTM!
413-419: LGTM!
66-94: 🎯 Functional CorrectnessConfirm
BaseSubnetDivisionRuleremains VPN-only
This regression test depends onself.vpn_serverand_get_vpn_subdivision_rule(), so it belongs in a VPN-specific subclass unless no non-VPN tests inherit this mixin.openwisp_controller/config/tests/utils.py (1)
168-181: 🎯 Functional CorrectnessDouble-check the shared WireGuard template helper
_create_wireguard_vpn_template()is shared across tests, so switchingconfigto{}changes the generated template payload for every caller. Make sure no existing assertions still depend on the previous static config.openwisp_controller/subnet_division/rule_types/vpn.py (1)
44-58: 🎯 Functional CorrectnessSaving
instancehere firespost_savea second time. Make sure any otherVpnClientreceivers are idempotent or guarded, otherwise this reorder can duplicate side effects.openwisp_controller/config/base/base.py (4)
335-414: Consume each tracked-field snapshot after evaluating the save.The stored snapshot can still be reused by a later unrelated save. This is the same unresolved snapshot-lifecycle issue raised previously.
5-51: LGTM!
439-465: LGTM!
426-434: 🗄️ Data Integrity & IntegrationSnapshot the delete target before deferring
post_deletecallbacks. The default resolver may keep a live instance whose primary key is cleared before the commit callback runs, so generic delete dependencies can invalidate the wrong key or miss it.openwisp_controller/config/apps.py (4)
118-135: Defer device-group change tasks until commit.These save/change dependencies still execute the task target before commit. This is the same unresolved transaction-timing issue raised previously.
137-153: Snapshot and defer device-group deletion invalidation.The delete handlers still invalidate before commit despite having all required values available for snapshotting. This duplicates the previously raised delete-race finding.
5-107: LGTM!Also applies to: 156-160, 360-391
108-115: 🗄️ Data Integrity & IntegrationMake the organization-disable invalidation commit-safe.
organization_disabled_handlerruns onpre_save; it should use the old state and defer cache clearing until after commit, or concurrent reads can repopulate stale controller data.openwisp_controller/config/base/vpn.py (2)
1162-1169: Preload configuration relations before recomputing clients.The per-client query amplification in this loop was already raised in a previous review and remains unresolved.
18-412: LGTM!openwisp_controller/config/handlers.py (2)
76-78: Schedule this task only after commit.The immediate Celery enqueue remains the same transaction-timing issue raised previously.
44-62: LGTM!openwisp_controller/config/base/config.py (1)
29-49: LGTM!Also applies to: 177-298
openwisp_controller/config/tests/test_vpn.py (2)
589-616: Duplicated setup between the two renew-cascade tests.Same concern already raised in a previous review round (shared VPN/template/device setup between
test_ca_renew_cascades_to_client_configandtest_cert_renew_cascades_to_client_config).
23-27: LGTM!Also applies to: 502-587, 917-921, 939-941, 1554-1627, 2029-2044, 2095-2098
openwisp_controller/config/tests/test_api.py (1)
6-6: LGTM!Also applies to: 20-20, 1492-1517
openwisp_controller/config/tests/test_config.py (3)
1-31: LGTM!Also applies to: 583-589, 935-978, 1062-1201
1203-1309: LGTM!Also applies to: 1366-1456
1310-1364: 🎯 Functional CorrectnessClarify deferred-field snapshot behavior
test_snapshot_falls_back_to_db_when_initial_fields_are_deferredsays the snapshot falls back to DB, but the assertions keepmodels.DEFERREDand never call_snapshot_from_db. Rename the test or update the implementation so the contract is explicit.openwisp_controller/config/tests/test_controller.py (1)
8-8: LGTM!Also applies to: 298-309, 1297-1328, 1406-1697
openwisp_controller/config/tests/test_device.py (1)
385-402: LGTM!Also applies to: 612-663
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Incremental review of commits since A regression test was added ( Files Reviewed (2 files)
Previous Review Summaries (3 snapshots, latest commit 9c9a4a5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9c9a4a5)Status: 1 Issue Found | Recommendation: Address before merge Overview
Incremental review of commits since Issue Details (click to expand)CRITICAL
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit aea229e)Status: No Issues Found | Recommendation: Merge Incremental review of commits since
Under lenient review mode (critical bugs and security vulnerabilities only), no new issues were found in the changed code. Existing open inline discussion threads were not duplicated. Files Reviewed (6 files)
Previous review (commit 0fadcfb)Status: No Issues Found | Recommendation: Merge This feature PR (closes #1095) replaces scattered
Existing CodeRabbit inline threads (transaction-timing/on-commit nuances, dispatch-uid collisions, i18n of command output, test duplication) are already part of the open discussion and were not duplicated here. Note the CodeRabbit pre-merge check flags an out-of-scope wording tweak in Files Reviewed (20 files)
Reviewed by claude-opus-4.8 · Input: 24 · Output: 3.3K · Cached: 313.3K |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I left one remaining cache invalidation concern inline.
| source=self.devicegroup_model, | ||
| signal="post_delete", | ||
| on_commit=False, | ||
| target=devicegroup_delete_handler, |
There was a problem hiding this comment.
I would not keep this delete invalidation immediate. The handler already has the data it needs on the deleted instance here, so we can snapshot the primitive values and enqueue the actual cache invalidation with transaction.on_commit(). Otherwise a request during the delete transaction can repopulate DeviceGroupCommonName from rows that are about to be deleted, leaving stale cache after commit.
| @@ -66,12 +89,6 @@ def devicegroup_delete_handler(instance, **kwargs): | |||
| tasks.invalidate_devicegroup_cache_delete.delay(instance.id, model_name, **kwargs) | |||
|
|
|||
There was a problem hiding this comment.
This is the point where I would enqueue after commit instead of calling .delay() immediately. Capture instance.id, model_name, organization_id, and common_name before returning from the signal handler, then call tasks.invalidate_devicegroup_cache_delete.delay(...) inside transaction.on_commit(). That keeps the current delete-time lookup behavior without opening the pre-commit cache repopulation race.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
pandafy
force-pushed
the
issues/1095-generic-cache-invalidation
branch
from
49c07f9 to
9c9a4a5
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| elif model_name == Cert._meta.model_name: | ||
| DeviceGroupCommonName.certificate_delete_invalidates_cache( | ||
| kwargs["organization_id"], kwargs["common_name"] | ||
| kwargs["common_name"], kwargs["organization_slug"] |
There was a problem hiding this comment.
CRITICAL: KeyError when deleting a shared cert (organization=None).
Nice work extending invalidation to cover shared certs. However, the handler in handlers.py now only sets kwargs["organization_slug"] when instance.organization is not None (lines 112-113). For a shared cert that key is absent, so this line raises KeyError: 'organization_slug' inside the Celery task. Since certificate_delete_invalidates_cache already accepts organization_slug=None, use .get() here:
| kwargs["common_name"], kwargs["organization_slug"] | |
| kwargs["common_name"], kwargs.get("organization_slug") |
The new test_shared_cert_delete_invalidates_devicegroup_wildcard_cache test mocks .delay, so the task body never runs and does not catch this.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Multiple Test Failures DetectedHello @pandafy, The CI build failed due to the following issues:
Recommendations:
|
| elif model_name == Cert._meta.model_name: | ||
| DeviceGroupCommonName.certificate_delete_invalidates_cache( | ||
| kwargs["organization_id"], kwargs["common_name"] | ||
| kwargs["common_name"], kwargs["organization_slug"] |
There was a problem hiding this comment.
This still breaks shared certificates. When Cert.organization is None, devicegroup_delete_handler() passes common_name but does not include organization_slug, so this lookup raises KeyError before certificate_delete_invalidates_cache() can invalidate the no-org cache entry. Since that method already accepts organization_slug=None, I would use kwargs.get("organization_slug") here and add a test that executes the task body for a shared certificate.
| import json | ||
| import logging | ||
| from copy import deepcopy | ||
| from types import SimpleNamespace |
There was a problem hiding this comment.
@pandafy what do you think of renaming this file cache.py?
|
@coderabbitai any critical issue remaining? Do one last review. Focus on major issues only. |
|
✅ Action performedComments resolved and changes approved. Full review finished. |
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/management/commands/print_cache_dependencies.py`:
- Around line 6-18: Mark the Command.help text and the --format argument help in
Command.add_arguments for translation using the project’s Django i18n helper,
matching the existing pattern used by CacheDependency.render_registered. Add the
required import and wrap both user-facing strings with the translation function.
🪄 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: e8b140b3-6859-4c0c-806c-2d11b0880634
📒 Files selected for processing (22)
docs/developer/utils.rstopenwisp_controller/config/api/views.pyopenwisp_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/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.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/tasks.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. (12)
- GitHub Check: CodeRabbit
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.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.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | 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/subnet_division/admin.pyopenwisp_controller/config/tests/utils.pyopenwisp_controller/config/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.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/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.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/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.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/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.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/management/commands/print_cache_dependencies.pyopenwisp_controller/config/tasks.pyopenwisp_controller/subnet_division/rule_types/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.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_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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/utils.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_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_api.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_config.py
🪛 ast-grep (0.44.1)
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/base.py
[info] 323-323: use jsonify instead of json.dumps for JSON output
Context: json.dumps([dep.describe() for dep in dependencies], indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 218-218: 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_vpn.py
[info] 1612-1612: Do not hardcode temporary file or directory names
Context: "/tmp/test"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
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/tests/test_config.py
[warning] 33-33: 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] 34-34: 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)
[warning] 35-35: 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", "Vpn")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: 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("django_x509", "Ca")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: 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("django_x509", "Cert")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (55)
openwisp_controller/config/tests/test_api.py (1)
6-6: LGTM!Also applies to: 20-20, 1492-1517
openwisp_controller/config/tests/test_config.py (1)
1-9: LGTM!Also applies to: 19-22, 33-38, 587-595, 942-985, 986-1073, 1157-1216, 1227-1296, 1298-1360, 1362-1387, 1388-1399, 1400-1485, 1487-1513, 1515-1577, 1579-1589
openwisp_controller/config/tests/test_controller.py (9)
8-8: LGTM!
298-309: LGTM!
1297-1311: LGTM!
1408-1417: LGTM!
1422-1460: LGTM!
1462-1511: LGTM!
1513-1583: LGTM!
1584-1634: LGTM!
1635-1697: LGTM!openwisp_controller/config/tests/test_device.py (4)
385-402: LGTM!
587-614: LGTM!
616-640: LGTM!
642-665: LGTM!openwisp_controller/config/tests/test_vpn.py (14)
23-27: LGTM!
523-542: LGTM!
544-565: LGTM!
566-580: LGTM!
581-593: LGTM!
595-610: LGTM!
612-618: LGTM!
620-629: LGTM!
929-934: LGTM!
951-954: LGTM!
1566-1589: LGTM!
1590-1639: LGTM!
2041-2056: LGTM!
2107-2110: LGTM!openwisp_controller/config/handlers.py (3)
65-81: LGTM! Both previously flagged issues (missing bulk fan-out, and enqueueing before commit) appear resolved — the bulk path now delays a task per device ID, matching the test intest_config.py:1579-1589, and the deferred-commit concern is now handled upstream byCacheDependency's defaulton_commit=True, which defers invocation of this handler itself until commit (seeapps.pyconnect_cache_dependencies).
83-121: LGTM! This resolves the previously flagged critical issue (deferring task enqueue viatransaction.on_commit()while still capturing live instance data synchronously) and the shared-certKeyErrorconcern, now paired withtasks.py'skwargs.get("organization_slug").
44-62: LGTM!openwisp_controller/config/tasks.py (1)
106-119: LGTM! This resolves the previously flagged criticalKeyErrorfor shared/org-less certificate deletes, confirmed bytest_shared_cert_delete_task_invalidates_devicegroup_wildcard_cache.openwisp_controller/config/management/commands/print_cache_dependencies.py (1)
20-21: LGTM!docs/developer/utils.rst (2)
410-458: LGTM!
494-499: 📐 Maintainability & Code QualityNo issue:
:ref:OPENWISP_CONTROLLER_CONTEXT <context_setting>`` points to an existing label indocs/user/settings.rst.> Likely an incorrect or invalid review comment.openwisp_controller/config/base/base.py (3)
34-51: LGTM!
350-431: LGTM!
433-493: LGTM!openwisp_controller/config/apps.py (2)
57-157: LGTM!
357-382: LGTM!openwisp_controller/config/base/config.py (2)
177-248: LGTM!
251-298: LGTM!openwisp_controller/config/base/vpn.py (3)
322-363: LGTM!
1150-1171: LGTM!
396-411: 🎯 Functional CorrectnessNo action needed
openwisp_controller/config/api/views.py (1)
276-281: 🗄️ Data Integrity & IntegrationNo issue: the receiver arg doesn’t affect the cache key.
get_cached_devicegroup_args_rewrite()only usesorg_slugsandcommon_name, soclsvsNoneininvalidate()doesn’t change the memoized key.> Likely an incorrect or invalid review comment.openwisp_controller/subnet_division/tests/test_models.py (3)
26-27: Static-analysis false positive dismissed.The ast-grep "keras-load-model" hint on
load_model("config", "VpnClient")is a false positive — this is OpenWISP's swappable-model loader (django-swapper), unrelated to Keras/TensorFlow deserialization.
66-94: LGTM! Directly exercises thepost_provision_handlerordering fix and confirms the cached checksum matches the freshly-rendered config.
366-372: LGTM! Assertions correctly mirror thelist/setargument types used insubnet_division/tasks.py.Also applies to: 413-419
openwisp_controller/config/tests/utils.py (1)
168-184: LGTM!openwisp_controller/subnet_division/rule_types/vpn.py (1)
44-58: LGTM! Reordering ensures the checksum cached bysuper().post_provision_handlerreflects the newly-assigned client IP, fixing the staleip_address_<vpn_pk>placeholder issue.openwisp_controller/subnet_division/tasks.py (1)
41-47: LGTM! Checksum invalidation correctly targets the configs affected by keyword rewrites and newly-provisioned IPs, matching thebulk_invalidate_get_cached_checksumcontract inconfig/base/config.py.Also applies to: 80-86
openwisp_controller/pki/tests/test_api.py (1)
160-169: LGTM! Query-count updates and the newTransactionTestCase-based class correctly reflect the on-commit cache invalidation introduced by this cohort, consistent with prior reviewer explanation.Also applies to: 247-253, 324-383
openwisp_controller/subnet_division/admin.py (1)
28-38: LGTM!
| class Command(BaseCommand): | ||
| help = ( | ||
| "Prints every cache dependency wired in the project, so the whole cache" | ||
| " invalidation graph can be inspected at a glance." | ||
| ) | ||
|
|
||
| def add_arguments(self, parser): | ||
| parser.add_argument( | ||
| "--format", | ||
| choices=["text", "json"], | ||
| default="text", | ||
| help="Output format (default: text).", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mark command help text for translation.
help (Lines 7-10) and the --format argument's help (Line 17) are plain, untranslated strings, while the sibling CacheDependency.render_registered output (base.py) already wraps equivalent user-facing text with _(). As per coding guidelines, **/*.py: "Mark user-facing strings for translation with Django i18n helpers in Django code."
♻️ Proposed fix
from django.core.management.base import BaseCommand
+from django.utils.translation import gettext_lazy as _
from openwisp_controller.config.base.base import CacheDependency
class Command(BaseCommand):
help = (
- "Prints every cache dependency wired in the project, so the whole cache"
- " invalidation graph can be inspected at a glance."
+ _(
+ "Prints every cache dependency wired in the project, so the whole"
+ " cache invalidation graph can be inspected at a glance."
+ )
)
def add_arguments(self, parser):
parser.add_argument(
"--format",
choices=["text", "json"],
default="text",
- help="Output format (default: text).",
+ help=_("Output format (default: text)."),
)📝 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.
| class Command(BaseCommand): | |
| help = ( | |
| "Prints every cache dependency wired in the project, so the whole cache" | |
| " invalidation graph can be inspected at a glance." | |
| ) | |
| def add_arguments(self, parser): | |
| parser.add_argument( | |
| "--format", | |
| choices=["text", "json"], | |
| default="text", | |
| help="Output format (default: text).", | |
| ) | |
| from django.core.management.base import BaseCommand | |
| from django.utils.translation import gettext_lazy as _ | |
| from openwisp_controller.config.base.base import CacheDependency | |
| class Command(BaseCommand): | |
| help = ( | |
| _( | |
| "Prints every cache dependency wired in the project, so the whole" | |
| " cache invalidation graph can be inspected at a glance." | |
| ) | |
| ) | |
| def add_arguments(self, parser): | |
| parser.add_argument( | |
| "--format", | |
| choices=["text", "json"], | |
| default="text", | |
| help=_("Output format (default: text)."), | |
| ) |
🤖 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/management/commands/print_cache_dependencies.py`
around lines 6 - 18, Mark the Command.help text and the --format argument help
in Command.add_arguments for translation using the project’s Django i18n helper,
matching the existing pattern used by CacheDependency.render_registered. Add the
required import and wrap both user-facing strings with the translation function.
Source: Coding guidelines
3 participants
Checklist
Reference to Existing Issue
Closes #1095
Blockers