[fix] Attach netjsonconfig validation errors to config field Fixes #744 #1215
Conversation
WalkthroughThe change modifies netjsonconfig validation error shape: Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🪛 Ruff (0.14.14)openwisp_controller/config/base/base.py[warning] 206-206: Within an (B904) 🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/base.py (1)
196-206:⚠️ Potential issue | 🟡 MinorPreserve the original SchemaError via exception chaining.
Line 206 raises a new ValidationError without chaining, which obscures the root cause and triggers Ruff's B904 rule. Use
raise ... from eto preserve the exception context.🔧 Suggested change
- raise ValidationError({"config": message}) + raise ValidationError({"config": message}) from e
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_template.py
🧰 Additional context used
🧠 Learnings (3)
📚 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/base/base.pyopenwisp_controller/config/tests/test_template.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/base/base.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_template.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_template.py (1)
openwisp_controller/config/base/config.py (1)
full_clean(558-562)
🪛 Ruff (0.14.14)
openwisp_controller/config/base/base.py
[warning] 206-206: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.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~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
🔇 Additional comments (1)
openwisp_controller/config/tests/test_template.py (1)
804-825: Test coverage aligns with the new ValidationError shape.
The assertions verify the error is attached toconfigand include the expected trigger path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Viscous106
force-pushed
the
vpn-template-validation
branch
from
77a4c2f to
6ed2edb
Compare
Viscous106
force-pushed
the
vpn-template-validation
branch
from
6ed2edb to
d734d2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/base.py (1)
191-206:⚠️ Potential issue | 🟡 MinorAdd exception chaining to preserve the original SchemaError.
This satisfies ruff's B904 rule, which flags raising a new exception inside an except block without explicit chaining. Since you're wrapping
SchemaErrorinValidationError, usefrom eto make the relationship explicit and preserve debugging information in tracebacks.♻️ Proposed fix
- raise ValidationError({"config": message}) + raise ValidationError({"config": message}) from e
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/base/base.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_template.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/tests/test_template.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/base/base.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/base/base.py
🪛 Ruff (0.14.14)
openwisp_controller/config/base/base.py
[warning] 206-206: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.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.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
🔇 Additional comments (1)
openwisp_controller/config/tests/test_config.py (1)
146-156: LGTM — assertion now targets the config field (Line 155).This aligns with the updated ValidationError payload.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Viscous106
force-pushed
the
vpn-template-validation
branch
3 times, most recently
from
b1cf945 to
f36a9c6
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes a user experience issue in the Django admin where netjsonconfig validation errors were not properly highlighted on the specific field that caused the error. The PR changes how ValidationError is raised in the clean_netjsonconfig_backend method to attach errors to the config field instead of raising them as non-field errors.
Changes:
- Modified
clean_netjsonconfig_backendinbase.pyto raise field-specific ValidationError - Added regression test to verify errors are properly attached to the
configfield - Updated existing test to reflect the new error reporting behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openwisp_controller/config/base/base.py | Changed ValidationError from non-field error to field-specific error attached to config field |
| openwisp_controller/config/tests/test_template.py | Added regression test verifying errors are attached to config field |
| openwisp_controller/config/tests/test_config.py | Updated existing test assertion to expect error on config field instead of all |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Viscous106
force-pushed
the
vpn-template-validation
branch
from
74c345a to
87f3569
Compare
Viscous106
force-pushed
the
vpn-template-validation
branch
from
fc5de7a to
2b443e8
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This PR is not solving the issue, it's merely moving the validation error message which is not what we want right now.
3 participants
Checklist
Reference to Existing Issue
Closes #744.
Description of Changes
Problem
When saving a Template with invalid netjsonconfig (e.g., OpenVPN server mode with missing required fields), the Django admin shows a generic "Please correct all validation errors below" banner without highlighting any specific field. This makes it difficult for users to identify what needs to be fixed.
Root Cause
The clean_netjsonconfig_backend() method in BaseConfig was raising
ValidationError(message)as a non-field error instead of attaching it to the config field.Solution
Modified openwisp_controller/config/base/base.py to raise
ValidationError({'config': message}), ensuring the error is attached to the config field and properly highlighted in the admin interface.Changes
Screenshot
_Before:
_After:
