Fix #1428: Make VpnClient objects immutable after creation#1431
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (12)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{py,html}📄 CodeRabbit inference engine (Custom checks)
Files:
🧠 Learnings (8)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-02-17T19:13:10.088ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
📚 Learning: 2026-06-07T12:07:08.468ZApplied to files:
📚 Learning: 2026-06-07T12:07:24.608ZApplied to files:
📚 Learning: 2026-06-25T12:20:18.414ZApplied to files:
📚 Learning: 2026-06-07T12:07:25.164ZApplied to files:
📚 Learning: 2026-06-25T12:20:45.387ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughThis change enforces post-creation immutability for Sequence Diagram(s)sequenceDiagram
participant TemplateOrCaller
participant AbstractVpnClient
participant Database
participant VpnSubnetDivisionRuleType
TemplateOrCaller->>AbstractVpnClient: full_clean()
AbstractVpnClient->>Database: load persisted row
Database-->>AbstractVpnClient: stored field values
alt immutable field changed
AbstractVpnClient-->>TemplateOrCaller: ValidationError
else update is allowed
TemplateOrCaller->>AbstractVpnClient: save()
AbstractVpnClient->>Database: persist changes
end
VpnSubnetDivisionRuleType->>AbstractVpnClient: set instance.ip
VpnSubnetDivisionRuleType->>AbstractVpnClient: save(update_fields=["ip"])
AbstractVpnClient->>Database: write ip only
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
sameerrandive-dev
force-pushed
the
sameerrandive-dev/issue-1428-vpnclient-block-modifications
branch
from
2e72025 to
749d9e6
Compare
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 911-923: The save() method in VpnClient currently uses only
_state.adding to decide whether this is a create, which can miss cases where a
new instance is constructed with an existing pk. Update the branching in save()
to treat objects with an existing self.pk as updates, and ensure
_check_immutable_fields() is called for those cases even when _state.adding is
still true. Keep the auto-provisioning path in save() for true creates, but gate
it on both _state.adding and absence of an existing primary key.
🪄 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: 1c0ce29a-b984-4dc9-aad2-6e6feb291f2d
📒 Files selected for processing (3)
openwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/subnet_division/rule_types/vpn.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.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~=4.2.0
- GitHub Check: Python==3.10 | 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/rule_types/vpn.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.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/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.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/rule_types/vpn.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.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/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.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/config/tests/test_vpn.pyopenwisp_controller/config/base/vpn.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_vpn.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_vpn.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_vpn.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.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_vpn.py
🔇 Additional comments (2)
openwisp_controller/subnet_division/rule_types/vpn.py (1)
55-56: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
275-302: LGTM!Also applies to: 512-584
| def save(self, *args, **kwargs): | ||
| """Performs automatic provisioning if ``auto_cert`` is True.""" | ||
| if self.auto_cert: | ||
| self._auto_x509() | ||
| self._auto_ip() | ||
| self._auto_wireguard() | ||
| self._auto_vxlan() | ||
| self._auto_secret() | ||
| if self._state.adding: | ||
| if self.auto_cert: | ||
| self._auto_x509() | ||
| self._auto_ip() | ||
| self._auto_wireguard() | ||
| self._auto_vxlan() | ||
| self._auto_secret() | ||
| else: | ||
| # not called automatically by save(): enforce immutability | ||
| # also for direct ORM saves which skip full_clean() | ||
| self._check_immutable_fields() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether repository tests cover saving a newly constructed VpnClient
# with an existing primary key and immutable-field changes.
rg -n -C3 'VpnClient\([^)]*pk=|VpnClient\([^)]*id=' openwisp_controller || true
rg -n -C3 '_state\.adding|force_insert|update_fields' openwisp_controller/config openwisp_controller/subnet_division || trueRepository: openwisp/openwisp-controller
Length of output: 36210
🌐 Web query:
For the Django version supported by this repository, how does Model.save() behave when a new model instance has a manually supplied primary key matching an existing row?
💡 Result:
In Django, when you manually supply a primary key value for a new model instance that matches an existing row in the database, Django assumes you intend to update the existing record rather than create a new one [1][2]. When the save method is called, Django uses an internal algorithm to decide whether to execute an SQL INSERT or UPDATE statement [1][3]. For models where the primary key does not define a default or db_default, Django checks the primary key attribute: - If the primary key is set to anything other than None, Django attempts an UPDATE [1][2]. - If the UPDATE fails to affect any rows (for instance, because the provided primary key does not actually exist in the database), Django then executes an INSERT [1][2]. For models that do define a default or db_default on the primary key, Django executes an UPDATE if the instance is considered "existing" and the primary key matches a value currently in the database; otherwise, it executes an INSERT [1][4]. Because of this behavior, it is strongly recommended to avoid manually specifying primary key values for new objects unless you can guarantee that the value is not already in use [1][2]. If you unintentionally use a primary key that matches an existing record, you will overwrite the data in that row [1]. If you instead intend to create a new record and ensure no overwrite occurs, you can use the force_insert=True argument with the save method [5].
Citations:
- 1: https://docs.djangoproject.com/en/5.2/ref/models/instances/
- 2: https://docs.djangoproject.com/en/6.1/ref/models/instances/
- 3: https://django.readthedocs.io/en/stable/%5Fsources/ref/models/instances.txt
- 4: https://docs.djangoproject.com/en/5.0/ref/models/instances/
- 5: https://code.djangoproject.com/ticket/29260
Don't rely on _state.adding alone here. A freshly constructed VpnClient(pk=<existing pk>, ...) can still take this branch, so save() may skip _check_immutable_fields() even when Django updates an existing row. Check whether self.pk already exists before treating it as a create.
🤖 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 911 - 923, The save()
method in VpnClient currently uses only _state.adding to decide whether this is
a create, which can miss cases where a new instance is constructed with an
existing pk. Update the branching in save() to treat objects with an existing
self.pk as updates, and ensure _check_immutable_fields() is called for those
cases even when _state.adding is still true. Keep the auto-provisioning path in
save() for true creates, but gate it on both _state.adding and absence of an
existing primary key.
749d9e6 to
d07983c
Compare
Code Review SummaryStatus: No Issues Found | Recommendation: Merge No new code changes since the previous review — current HEAD ( The immutability enforcement remains clean and well-targeted: Policy checks from the custom instructions are satisfied:
No critical bugs or security vulnerabilities were found in the changed lines. An existing reviewer comment already tracks the Files Reviewed (3 files)
Previous Review Summary (commit d07983c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d07983c)Status: No Issues Found | Recommendation: Merge Nice work — the immutability enforcement is clean and well-targeted. The policy checks from the custom instructions are satisfied:
No critical bugs or security vulnerabilities were found in the changed lines. An existing reviewer comment already tracks the Files Reviewed (3 files)
Reviewed by claude-opus-4.8 · Input: 20 · Output: 3.4K · Cached: 285.8K |
Make VpnClient objects immutable after creation while allowing managed IP updates. Fixes openwisp#1428
sameerrandive-dev
force-pushed
the
sameerrandive-dev/issue-1428-vpnclient-block-modifications
branch
from
d07983c to
8983946
Compare
|
coverage: 98.537% (-0.01%) from 98.547% — sameerrandive-dev:sameerrandive-dev/issue-1428-vpnclient-block-modifications into openwisp:master |
3 participants
Summary
Fixes #1428 by making
VpnClientobjects immutable after creation while allowing the internally managedipfield to be updated.Changes
VpnClientidentity fields by enforcing immutability in bothclean()andsave().ipupdates required by the asynchronous subnet provisioning workflow.VpnClientobjects.ipupdates.Validation
subnet_divisionand swapped-model test suites.