Skip to content

Conversation

@Viscous106
Copy link
Contributor

Summary

This pull request removes the django-jsonfield dependency by migrating to Django's native JSONField (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

  • The third-party django-jsonfield library is deprecated and no longer maintained.
  • Maintains unnecessary external dependency when Django has built-in JSON support.
  • Blocks compatibility with Django 5.x features and future versions.
  • Technical debt that impacts maintainability.

Solution

Migrated all JSONField implementations from django-jsonfield to Django's native django.db.models.JSONField:

  • Django's native JSONField has been stable and feature-complete since Django 3.1.
  • No functional difference for the current use cases in openwisp-controller.
  • Simplifies dependency management and reduces maintenance burden.

Changes Made

Model Files (5 files)

  • base.py
  • config.py
  • device_group.py
  • multitenancy.py
  • template.py
  • models.py

Specific Changes:

  • Updated all imports: django_jsonfield.fields.JSONFielddjango.db.models.JSONField.
  • Removed unsupported parameters: dump_kwargs and load_kwargs.
  • Added JSON string-to-dict conversion in clean() and full_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 JSONField requires dict objects for validation, unlike django-jsonfield which auto-converted JSON strings.

Migration Files (13 total)

Updated existing migrations (10 files) to reflect new import paths:

  • Config migrations: 0001 squashed, 0018, 0023, 0028, 0036, 0049, 0051
  • Connection migration: 0001
  • PKI migration: 0001

New 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

  • ✅ All 7 JSONField-related tests passing.
  • ✅ 951 of 953 total tests passing (2 unrelated Selenium infrastructure failures).
  • ✅ CSV import/export functionality verified.
  • ✅ JSON validation in admin panel tested.
  • ✅ REST API JSON endpoints verified.
  • ✅ Tested with Django 4.2+ and 5.x.

Breaking Changes

None. This is a transparent migration:

  • Data storage and format remain identical.
  • API responses unchanged.
  • Database schema updated via migrations.
  • No user-facing changes.

Dependencies

  • Removed: django-jsonfield (no longer in requirements).
  • Requires: Django >= 3.1 (project already uses Django 4.2+/5.x).

Files Changed

  • 22 files modified.
  • 3 new migration files created.
  • 10 existing migrations updated for compatible imports.

Migration Notes

After merging, developers should run:

python manage.py migrate

Copilot AI review requested due to automatic review settings February 3, 2026 13:54
@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

Walkthrough

This pull request replaces the third-party jsonfield package with Django's built-in JSONField across the OpenWISP controller codebase. The changes update imports in 6 model files and multiple migration files, remove custom serialization parameters (load_kwargs and dump_kwargs) that are not supported by Django's JSONField, and add pre-validation logic in clean() methods to parse JSON strings into dictionaries before validation. Test cases have been updated with proper JSON escaping for CSV imports to ensure data integrity during device imports.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title '[change] Replace django-jsonfield with Django native JSONField' clearly and specifically summarizes the main objective of the PR, making it immediately obvious to reviewers what the change entails.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering problem statement, solution, detailed changes, testing, and migration notes. It exceeds the basic template requirements with substantial technical detail.
Linked Issues check ✅ Passed All primary objectives from linked issues are met: replaced third-party jsonfield with Django's native JSONField across 6 model files, removed unsupported dump_kwargs and load_kwargs parameters, updated migrations safely, and fixed related validation logic in clean/full_clean methods.
Out of Scope Changes check ✅ Passed All changes remain within scope of jsonfield-to-Django migration. Test updates (CSV escaping, JSON assignments) and PKI field migrations are directly related to supporting the core dependency replacement and ensuring proper data handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🧹 Recent nitpick comments
openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py (1)

10-13: Redundant import of models on line 10.

Line 10 imports models from django.db, but line 13 already imports models (along with migrations) from the same module. The import on line 10 is immediately shadowed and serves no purpose.

🧹 Proposed fix to remove the redundant import
-from django.db import models
 import model_utils.fields
openwisp_controller/pki/migrations/0001_initial.py (2)

4-4: Unused import collections.

The collections import appears to be unused after removing the django-jsonfield dependency. It was likely required by the old jsonfield implementation.

🧹 Proposed fix to remove unused import
-import collections
-
 import django.db.models.deletion

9-12: Redundant import of models.

models is imported twice from django.db — on line 9 and again on line 12. Only the import on line 12 is needed.

🧹 Proposed fix to remove redundant import
 import django.db.models.deletion
 import django.utils.timezone
 import django_x509.base.models
-from django.db import models
 import model_utils.fields
 from django.conf import settings
 from django.db import migrations, models
openwisp_controller/config/base/base.py (1)

13-13: Import statement placement could be improved.

The JSONField import should be grouped with the other django.db import on line 9 for better organization following PEP8 import ordering conventions.

♻️ Suggested improvement
-from django.db import models
+from django.db import models
+from django.db.models import JSONField
 from django.utils.functional import cached_property
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
-from django.db.models import JSONField
openwisp_controller/config/tests/test_admin.py (1)

243-243: Minor: Trailing whitespace on blank line.

Line 243 appears to have trailing whitespace. Consider removing it for cleaner formatting.

openwisp_controller/config/base/device_group.py (2)

9-9: Import statement placement could be improved.

The JSONField import should be grouped with the other django.db import on line 7.

♻️ Suggested improvement
-from django.db import models
+from django.db import models
+from django.db.models import JSONField
 from django.utils.translation import gettext_lazy as _
-from django.db.models import JSONField

68-80: Consider adding pre-validation for context field as well.

The clean() method converts meta_data from JSON string to dict, but the context field (also a JSONField) doesn't have similar handling. For consistency with other modules in this PR (e.g., BaseConfig.config and BaseConfig.context in config.py), consider adding the same pre-validation for context if it can receive string input from forms or API.

♻️ Suggested improvement
 def clean(self):
     # Convert JSON string meta_data to dictionary before validation
     if isinstance(self.meta_data, str):
         try:
             self.meta_data = json.loads(self.meta_data)
         except ValueError:
             pass  # Let validators handle invalid JSON
+    # Convert JSON string context to dictionary before validation
+    if isinstance(self.context, str):
+        try:
+            self.context = json.loads(self.context)
+        except ValueError:
+            pass  # Let validators handle invalid JSON
     try:
         jsonschema.Draft4Validator(app_settings.DEVICE_GROUP_SCHEMA).validate(
             self.meta_data
         )
openwisp_controller/config/base/template.py (1)

9-9: Import statement placement could be improved.

The JSONField import should be grouped with the other django.db import on line 7.

♻️ Suggested improvement
-from django.db import models, transaction
+from django.db import models, transaction
+from django.db.models import JSONField
 from django.utils.translation import gettext_lazy as _
-from django.db.models import JSONField
openwisp_controller/connection/base/models.py (1)

14-14: Import statement placement could be improved.

The JSONField import should be grouped with the other django.db import on line 8.

♻️ Suggested improvement
-from django.db import models, transaction
+from django.db import models, transaction
+from django.db.models import JSONField
 from django.utils import timezone
 from django.utils.functional import cached_property
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext
 from django.utils.translation import gettext_lazy as _
-from django.db.models import JSONField
openwisp_controller/connection/migrations/0007_command.py (2)

3-3: Unused import: collections.

The collections import was previously used for object_pairs_hook=collections.OrderedDict in the jsonfield configuration. Since the JSONField no longer uses this, the import can be removed.

♻️ Suggested fix
 # Generated by Django 3.1.2 on 2020-10-09 22:01
 
-import collections
 import uuid

10-13: Duplicate import of models.

Line 10 imports models from django.db, but line 13 imports it again as part of from django.db import migrations, models. This creates a redundant import.

♻️ Suggested fix
-from django.db import models
 import model_utils.fields
 import swapper
 from django.db import migrations, models
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c5c4a5 and bdb7804.

📒 Files selected for processing (22)
  • openwisp_controller/config/base/base.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/base/multitenancy.py
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
  • openwisp_controller/config/migrations/0018_config_context.py
  • openwisp_controller/config/migrations/0023_update_context.py
  • openwisp_controller/config/migrations/0028_template_default_values.py
  • openwisp_controller/config/migrations/0036_device_group.py
  • openwisp_controller/config/migrations/0049_devicegroup_context.py
  • openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py
  • openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/connection/migrations/0001_initial.py
  • openwisp_controller/connection/migrations/0007_command.py
  • openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py
  • openwisp_controller/connection/tests/test_models.py
  • openwisp_controller/geo/tests/test_admin.py
  • openwisp_controller/pki/migrations/0001_initial.py
  • openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.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/pki/migrations/0001_initial.py
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py
  • openwisp_controller/connection/migrations/0001_initial.py
  • openwisp_controller/config/base/multitenancy.py
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/config/migrations/0036_device_group.py
  • openwisp_controller/config/migrations/0023_update_context.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/connection/tests/test_models.py
  • openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py
  • openwisp_controller/config/migrations/0028_template_default_values.py
  • openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py
  • openwisp_controller/config/base/base.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/connection/migrations/0007_command.py
  • openwisp_controller/geo/tests/test_admin.py
  • openwisp_controller/config/migrations/0049_devicegroup_context.py
  • openwisp_controller/config/migrations/0018_config_context.py
  • openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
  • openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py
  • openwisp_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/pki/migrations/0001_initial.py
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py
  • openwisp_controller/connection/migrations/0001_initial.py
  • openwisp_controller/config/base/multitenancy.py
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/config/migrations/0036_device_group.py
  • openwisp_controller/config/migrations/0023_update_context.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/connection/tests/test_models.py
  • openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py
  • openwisp_controller/config/migrations/0028_template_default_values.py
  • openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py
  • openwisp_controller/config/base/base.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/connection/migrations/0007_command.py
  • openwisp_controller/geo/tests/test_admin.py
  • openwisp_controller/config/migrations/0049_devicegroup_context.py
  • openwisp_controller/config/migrations/0018_config_context.py
  • openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
  • openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py
  • openwisp_controller/config/base/config.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/migrations/0062_alter_config_config_alter_config_context_and_more.py
  • openwisp_controller/connection/migrations/0001_initial.py
  • openwisp_controller/config/base/multitenancy.py
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/config/migrations/0023_update_context.py
  • openwisp_controller/connection/tests/test_models.py
  • openwisp_controller/config/migrations/0018_config_context.py
  • openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
🧬 Code graph analysis (3)
openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py (5)
openwisp_controller/config/migrations/0018_config_context.py (1)
  • Migration (9-26)
openwisp_controller/config/migrations/0023_update_context.py (1)
  • Migration (9-26)
openwisp_controller/config/migrations/0028_template_default_values.py (1)
  • Migration (9-27)
openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
  • Migration (9-28)
openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
  • Migration (9-28)
openwisp_controller/config/base/device_group.py (1)
openwisp_controller/config/base/base.py (1)
  • json (282-289)
openwisp_controller/config/base/config.py (1)
openwisp_controller/config/base/base.py (1)
  • json (282-289)
🪛 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 typing.ClassVar

(RUF012)


[warning] 12-91: Mutable class attributes should be annotated with typing.ClassVar

(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 typing.ClassVar

(RUF012)


[warning] 13-70: Mutable class attributes should be annotated with typing.ClassVar

(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 typing.ClassVar

(RUF012)


[warning] 12-37: Mutable class attributes should be annotated with typing.ClassVar

(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)
  • GitHub Check: CodeQL analysis (python)
  • GitHub Check: Agent
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (30)
openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py (1)

59-66: LGTM! JSONField migration looks correct.

The migration correctly replaces jsonfield.fields.JSONField with Django's native models.JSONField for the Config.config, Template.config, and Vpn.config fields. The field parameters (blank, default=dict, help_text, verbose_name) are valid for Django's JSONField, and the removal of dump_kwargs/load_kwargs is appropriate since Django's native implementation doesn't use these parameters.

Also applies to: 249-256, 344-350

openwisp_controller/config/base/multitenancy.py (2)

5-8: Import switch to Django’s JSONField looks good.

Using django.db.models.JSONField aligns with the migration goal and keeps imports clear.


34-41: JSONField definition cleanup is fine.

Removing load_kwargs/dump_kwargs is consistent with Django’s native JSONField behavior, and the defaults look correct.

openwisp_controller/pki/migrations/0001_initial.py (1)

113-120: LGTM — JSONField migration looks correct.

The extensions fields for both Ca and Cert models are correctly updated to use Django's native models.JSONField with appropriate defaults (default=list) and field options. This aligns with the PR objective of replacing django-jsonfield.

Also applies to: 264-271

openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py (3)

14-23: LGTM — JSONField migration for extensions fields.

The extensions fields for both Ca and Cert models are correctly altered to use Django's native models.JSONField with blank=True and default=list, matching the field definition style in other migrations within this PR.

Also applies to: 42-51


9-11: Static analysis warnings are false positives.

The Ruff warnings about mutable class attributes (dependencies and operations) not being annotated with typing.ClassVar are false positives for Django migration files. This is the standard pattern used by Django's migration framework and does not require ClassVar annotations.

Also applies to: 13-70


24-41: Changes to key_length field are intentional and part of a coordinated migration.

This migration logically follows 0011_disallowed_blank_key_length_or_digest, which disallowed blank key_length values. The changes here enforce that constraint by removing blank=True and the empty choice ("", "").

The removal of ("512", "512") and addition of ECDSA choices (256, 384, 521) represent an intentional feature expansion to support ECDSA keys alongside RSA. The identical changes applied to both the ca and cert models confirm this is a coordinated schema update, not accidental. Current code usage shows only 2048 (RSA) values in tests and APIs, indicating backward compatibility is maintained while new options are added.

openwisp_controller/config/base/base.py (2)

143-148: LGTM! Pre-validation pattern for JSON string conversion.

The approach of converting JSON strings to dicts before validation, and letting validators handle invalid JSON via pass, is consistent and appropriate for the migration to Django's native JSONField.


125-130: LGTM! JSONField declaration updated correctly.

The field declaration properly removes the load_kwargs and dump_kwargs parameters that were specific to django-jsonfield. Django's native JSONField handles serialization/deserialization automatically.

openwisp_controller/config/tests/test_admin.py (2)

238-259: LGTM! Proper CSV escaping for JSON fields.

The approach of escaping double quotes as "" for CSV embedding is correct and follows RFC 4180 for CSV formatting. This ensures that JSON strings like {"general": {}} are properly parsed when imported from CSV files.


701-723: LGTM! Consistent CSV escaping pattern.

The escaping logic matches the pattern used in test_device_import_config_not_templates, maintaining consistency across the test suite.

openwisp_controller/config/base/device_group.py (1)

40-57: LGTM! JSONField declarations updated correctly.

Both meta_data and context fields are properly updated to use Django's native JSONField with appropriate defaults and help text.

openwisp_controller/config/base/template.py (2)

224-235: LGTM! Pre-validation and type checking for default_values.

The string-to-dict conversion followed by explicit dict type validation provides robust handling for the JSONField migration.


95-105: LGTM! JSONField declaration updated correctly.

The default_values field properly uses Django's native JSONField with appropriate defaults and help text.

openwisp_controller/connection/base/models.py (3)

43-48: LGTM! Pre-validation for params in ConnectorMixin.

The hasattr check before accessing self.params is a good defensive pattern since this mixin is used by multiple classes.


468-492: Good error aggregation pattern for validation.

The refactored clean() method properly collects errors before raising them all at once, which provides better user feedback. The logic correctly appends schema validation errors to any existing JSON parsing errors.

One minor note: json.JSONDecodeError is a subclass of ValueError in Python 3.5+, so catching both is redundant but harmless. You could simplify to just ValueError.


107-111: LGTM! JSONField declarations updated correctly.

All three JSONField declarations (AbstractCredentials.params, AbstractDeviceConnection.params, AbstractCommand.input) are properly updated to use Django's native JSONField with appropriate attributes.

Also applies to: 245-253, 428-431

openwisp_controller/connection/migrations/0010_alter_command_input_alter_credentials_params_and_more.py (1)

1-37: LGTM! Migration correctly records JSONField transitions.

The migration properly alters the three JSONField declarations to use Django's native models.JSONField. The field attributes (blank, null, default, help_text, verbose_name) are correctly preserved.

Note: The static analysis warnings about ClassVar are false positives - Django migrations conventionally use class attributes for dependencies and operations without type annotations.

openwisp_controller/connection/migrations/0007_command.py (1)

78-83: LGTM! JSONField migration updated correctly.

The input field is properly migrated from jsonfield.fields.JSONField to Django's native models.JSONField with the same blank=True, null=True attributes.

openwisp_controller/connection/migrations/0001_initial.py (1)

8-8: LGTM

Also applies to: 69-75, 134-143

openwisp_controller/config/migrations/0062_alter_config_config_alter_config_context_and_more.py (1)

12-90: LGTM

openwisp_controller/config/migrations/0036_device_group.py (1)

8-8: LGTM

Also applies to: 58-68

openwisp_controller/config/migrations/0028_template_default_values.py (1)

5-6: LGTM

Also applies to: 16-25

openwisp_controller/connection/tests/test_models.py (1)

973-975: LGTM

Also applies to: 989-991, 1003-1005

openwisp_controller/geo/tests/test_admin.py (1)

210-235: LGTM

Also applies to: 277-300

openwisp_controller/config/base/config.py (1)

2-2: The downstream validation already exists and handles this concern. BaseConfig.clean() (line 151–152) explicitly validates that config is a dict and raises ValidationError if not. Similarly, AbstractConfig.clean() (lines 583–586) validates that context is a dict. If JSON parsing fails in full_clean(), the string will remain and be caught by these isinstance checks in clean(), raising ValidationError as expected. No changes needed.

Likely an incorrect or invalid review comment.

openwisp_controller/config/migrations/0023_update_context.py (1)

5-24: LGTM!

The migration correctly updates the config.context field from jsonfield.fields.JSONField to Django's native models.JSONField. The field attributes (blank, default, help_text) are properly preserved, and the removal of dump_kwargs/load_kwargs is appropriate since Django's JSONField handles serialization internally.

openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)

5-27: LGTM!

The migration correctly uses Django's native models.JSONField for the organizationconfigsettings.context field. Field attributes are properly configured.

openwisp_controller/config/migrations/0049_devicegroup_context.py (1)

5-27: LGTM!

The migration correctly uses Django's native models.JSONField for the devicegroup.context field. Field attributes are properly preserved.

openwisp_controller/config/migrations/0018_config_context.py (1)

5-25: LGTM!

The migration correctly uses Django's native models.JSONField for the initial config.context field addition. The field uses null=True here, with default=dict being added in the subsequent migration (0023), maintaining the correct migration sequence.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a 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 JSONField imports from django-jsonfield to django.db.models.JSONField across 6 model files
  • Removed unsupported dump_kwargs and load_kwargs parameters 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},

Copy link

Copilot AI Feb 3, 2026

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.

Suggested change

Copilot uses AI. Check for mistakes.
# Properly escape JSON quotes for CSV
config_escaped = config.replace('"', '""')
context_escaped = context.replace('"', '""')

Copy link

Copilot AI Feb 3, 2026

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.

Suggested change

Copilot uses AI. Check for mistakes.
# Properly escape JSON quotes for CSV
config_escaped = config.replace('"', '""')
context_escaped = context.replace('"', '""')

Copy link

Copilot AI Feb 3, 2026

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.

Suggested change

Copilot uses AI. Check for mistakes.
# Properly escape JSON quotes for CSV
config_escaped = config.replace('"', '""')
context_escaped = context.replace('"', '""')

Copy link

Copilot AI Feb 3, 2026

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.

Suggested change

Copilot uses AI. Check for mistakes.
Comment on lines +563 to +569
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
Copy link

Copilot AI Feb 3, 2026

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.

Suggested change
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,
)

Copilot uses AI. Check for mistakes.
Comment on lines +562 to +568
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:
Copy link

Copilot AI Feb 3, 2026

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.

Suggested change
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).

Copilot uses AI. Check for mistakes.
@Eeshu-Yadav
Copy link

@Viscous106 add comments where necessary , don't unecesaary comments on each line

@Viscous106 Viscous106 marked this pull request as draft February 4, 2026 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[change:controller] Replace thirdparty JSONField with Django built in JSONField

2 participants