-
-
Notifications
You must be signed in to change notification settings - Fork 257
[change] Replace django-jsonfield with Django native JSONField #1212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request replaces the third-party Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (22)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
🧬 Code graph analysis (3)openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py (5)
openwisp_controller/config/base/device_group.py (1)
openwisp_controller/config/base/config.py (1)
🪛 Ruff (0.14.14)openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py[warning] 8-10: Mutable class attributes should be annotated with (RUF012) [warning] 12-91: Mutable class attributes should be annotated with (RUF012) openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py[warning] 9-11: Mutable class attributes should be annotated with (RUF012) [warning] 13-70: Mutable class attributes should be annotated with (RUF012) openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py[warning] 8-10: Mutable class attributes should be annotated with (RUF012) [warning] 12-37: Mutable class attributes should be annotated with (RUF012) ⏰ 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). (13)
🔇 Additional comments (30)
✏️ Tip: You can disable this entire section by setting Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request migrates from the deprecated third-party django-jsonfield library to Django's native JSONField (available since Django 3.1), modernizing the codebase and eliminating an unnecessary external dependency while maintaining backward compatibility.
Changes:
- Replaced all
JSONFieldimports fromdjango-jsonfieldtodjango.db.models.JSONFieldacross 6 model files - Removed unsupported
dump_kwargsandload_kwargsparameters from field definitions - Added JSON string-to-dictionary conversion logic in validation methods to maintain compatibility
- Updated 10 existing migration files to use the new import path
- Created 3 new migration files to record the field type transition
- Fixed CSV import test cases to properly escape JSON field values per RFC 4180
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py | New migration recording JSONField type change for CA and Cert models |
| openwisp_controller/pki/migrations/0001_initial.py | Updated import path from jsonfield to django.db.models and removed dump_kwargs/load_kwargs |
| openwisp_controller/geo/tests/test_admin.py | Fixed CSV import tests with proper RFC 4180 quote escaping for JSON fields |
| openwisp_controller/connection/tests/test_models.py | Changed JSON string assignments to dictionary assignments for proper validation |
| openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py | New migration recording JSONField type change for Command, Credentials, and DeviceConnection models |
| openwisp_controller/connection/migrations/0007_command.py | Updated import path and removed unsupported parameters |
| openwisp_controller/connection/migrations/0001_initial.py | Updated import path and removed unsupported parameters |
| openwisp_controller/connection/base/models.py | Updated JSONField import, added JSON string conversion in ConnectorMixin.clean() and Command.clean() |
| openwisp_controller/config/tests/test_admin.py | Fixed CSV import tests with proper RFC 4180 quote escaping |
| openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py | New migration recording JSONField type change for Config, DeviceGroup, OrganizationConfigSettings, Template, and Vpn models |
| openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0049_devicegroup_context.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0036_device_group.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0028_template_default_values.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0023_update_context.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0018_config_context.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py | Updated import path and removed unsupported parameters |
| openwisp_controller/config/base/template.py | Updated JSONField import and added JSON string conversion in clean() |
| openwisp_controller/config/base/multitenancy.py | Updated JSONField import and removed unsupported parameters |
| openwisp_controller/config/base/device_group.py | Updated JSONField import, added JSON string conversion in clean() |
| openwisp_controller/config/base/config.py | Updated JSONField import and added JSON string conversions in full_clean() |
| openwisp_controller/config/base/base.py | Updated JSONField import, removed unsupported parameters, and added JSON string conversion in clean() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| help_text=_("configuration in NetJSON DeviceConfiguration format"), | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| dump_kwargs={"indent": 4}, | ||
|
|
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSONField definition has an unnecessary blank line (line 129) before the closing parenthesis. This is inconsistent with Python style conventions and should be removed for cleaner code formatting.
| # Properly escape JSON quotes for CSV | ||
| config_escaped = config.replace('"', '""') | ||
| context_escaped = context.replace('"', '""') | ||
|
|
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is trailing whitespace at the end of this line. This should be removed to comply with Python style guidelines and maintain clean code formatting.
| # Properly escape JSON quotes for CSV | ||
| config_escaped = config.replace('"', '""') | ||
| context_escaped = context.replace('"', '""') | ||
|
|
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is trailing whitespace at the end of this line. This should be removed to comply with Python style guidelines and maintain clean code formatting.
| # Properly escape JSON quotes for CSV | ||
| config_escaped = config.replace('"', '""') | ||
| context_escaped = context.replace('"', '""') | ||
|
|
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is trailing whitespace at the end of this line. This should be removed to comply with Python style guidelines and maintain clean code formatting.
| pass | ||
| # Convert JSON string context to dictionary before validation runs | ||
| if isinstance(self.context, str): | ||
| try: | ||
| self.context = json.loads(self.context) | ||
| except ValueError: | ||
| pass |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| # Convert JSON string context to dictionary before validation runs | |
| if isinstance(self.context, str): | |
| try: | |
| self.context = json.loads(self.context) | |
| except ValueError: | |
| pass | |
| # Leave invalid JSON as a string; field/serializer validation | |
| # will handle reporting any issues with this value later. | |
| logger.debug( | |
| "Invalid JSON in Config.config during full_clean; " | |
| "leaving value as string.", | |
| exc_info=True, | |
| ) | |
| # Convert JSON string context to dictionary before validation runs | |
| if isinstance(self.context, str): | |
| try: | |
| self.context = json.loads(self.context) | |
| except ValueError: | |
| # Leave invalid JSON as a string; field/serializer validation | |
| # will handle reporting any issues with this value later. | |
| logger.debug( | |
| "Invalid JSON in Config.context during full_clean; " | |
| "leaving value as string.", | |
| exc_info=True, | |
| ) |
| except ValueError: | ||
| pass | ||
| # Convert JSON string context to dictionary before validation runs | ||
| if isinstance(self.context, str): | ||
| try: | ||
| self.context = json.loads(self.context) | ||
| except ValueError: |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
| except ValueError: | |
| pass | |
| # Convert JSON string context to dictionary before validation runs | |
| if isinstance(self.context, str): | |
| try: | |
| self.context = json.loads(self.context) | |
| except ValueError: | |
| except ValueError: | |
| # Leave invalid JSON in 'config' as a string; any problems will | |
| # be caught by subsequent field/model validation. | |
| pass | |
| # Convert JSON string context to dictionary before validation runs | |
| if isinstance(self.context, str): | |
| try: | |
| self.context = json.loads(self.context) | |
| except ValueError: | |
| # Leave invalid JSON in 'context' as a string; clean() will raise | |
| # a ValidationError if 'context' is not a JSON object (dict). |
|
@Viscous106 add comments where necessary , don't unecesaary comments on each line |
Summary
This pull request removes the
django-jsonfielddependency by migrating to Django's nativeJSONField(available since Django 3.1). This modernizes the codebase, ensures compatibility with Django 5.x and future versions, and eliminates an unnecessary external dependency.Related to: Closes #1111 | Fixes #1061
Problem Statement
django-jsonfieldlibrary is deprecated and no longer maintained.Solution
Migrated all
JSONFieldimplementations fromdjango-jsonfieldto Django's nativedjango.db.models.JSONField:JSONFieldhas been stable and feature-complete since Django 3.1.openwisp-controller.Changes Made
Model Files (5 files)
base.pyconfig.pydevice_group.pymultitenancy.pytemplate.pymodels.pySpecific Changes:
django_jsonfield.fields.JSONField→django.db.models.JSONField.dump_kwargsandload_kwargs.clean()andfull_clean()methods for proper validation.Validation Logic (6 methods)
Config.clean(),Config.full_clean()Template.clean(),DeviceGroup.clean()Organization.clean(),DeviceConnection._validate_input()Credentials._validate_params()Reasoning: Django's native
JSONFieldrequires dict objects for validation, unlikedjango-jsonfieldwhich auto-converted JSON strings.Migration Files (13 total)
Updated existing migrations (10 files) to reflect new import paths:
0001squashed,0018,0023,0028,0036,0049,005100010001New auto-generated migrations (3 files) to record field type changes:
config/migrations/0062_alter_config_config_alter_config_context_and_more.py- Records JSONField type transition for Config and other models.connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py- Records JSONField type transition for Command and Credentials models.pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py- Records JSONField type transition for CA model.Test Fixes (3 files)
test_admin.py- Fixed CSV import tests with proper RFC 4180 quote escaping.test_models.py- Fixed JSON string assignments in connection tests.test_admin.py- Fixed CSV export tests with proper JSON formatting.Testing
Breaking Changes
None. This is a transparent migration:
Dependencies
django-jsonfield(no longer in requirements).Files Changed
Migration Notes
After merging, developers should run: