[change] Replaced thirdparty JSONField with Django built-in JSONField#1214
Conversation
WalkthroughThis PR replaces the external Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (4)📚 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:
📚 Learning: 2026-01-12T22:27:40.078ZApplied to files:
🧬 Code graph analysis (5)openwisp_controller/config/base/template.py (1)
openwisp_controller/config/base/device_group.py (1)
openwisp_controller/config/base/config.py (1)
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (2)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (4)
🪛 Ruff (0.14.14)tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py[warning] 9-11: Mutable class attributes should be annotated with (RUF012) [warning] 13-47: Mutable class attributes should be annotated with (RUF012) tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py[warning] 9-11: Mutable class attributes should be annotated with (RUF012) [warning] 13-117: Mutable class attributes should be annotated with (RUF012) openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py[warning] 9-11: Mutable class attributes should be annotated with (RUF012) [warning] 13-117: Mutable class attributes should be annotated with (RUF012) openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py[warning] 9-11: Mutable class attributes should be annotated with (RUF012) [warning] 13-47: 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). (11)
🔇 Additional comments (19)
✏️ 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 |
There was a problem hiding this comment.
Pull request overview
This PR modernizes OpenWISP Controller models by replacing the unmaintained third-party jsonfield package with Django’s built-in models.JSONField, and adds migrations to alter the affected database columns accordingly.
Changes:
- Replaced
from jsonfield import JSONFieldusages with Django built-inJSONFieldin controller model base classes. - Added migrations (including test app migrations) to
AlterFieldthe affected columns tomodels.JSONField. - Removed
jsonfieldfromrequirements.txtand droppedload_kwargs/dump_kwargsparameters.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
requirements.txt |
Removes the jsonfield dependency. |
openwisp_controller/config/base/base.py |
Switches config field to Django JSONField and removes jsonfield-specific kwargs. |
openwisp_controller/config/base/config.py |
Switches context field to Django JSONField and removes jsonfield-specific kwargs. |
openwisp_controller/config/base/device_group.py |
Switches meta_data / context fields to Django JSONField. |
openwisp_controller/config/base/multitenancy.py |
Switches organization context field to Django JSONField. |
openwisp_controller/config/base/template.py |
Switches default_values to Django JSONField. |
openwisp_controller/connection/base/models.py |
Switches params / input fields to Django JSONField. |
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py |
Alters config-related fields to models.JSONField. |
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py |
Alters connection-related fields to models.JSONField. |
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py |
Updates sample config app fields to models.JSONField. |
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py |
Updates sample connection app fields to models.JSONField. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Generated by Django 5.2.11 on 2026-02-04 02:53 | ||
|
|
||
| from django.db import migrations, models |
There was a problem hiding this comment.
The PR description references a 0061_replace_jsonfield_with_django_builtin.py migration, but this change introduces 0062_replace_jsonfield_with_django_builtin.py (dependency 0061_config_checksum_db). Consider updating the PR description (or renaming/rebasing the migration if needed) to avoid confusion for reviewers and downstream backports.
coderabbitai
bot
added
the
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Eeshu-Yadav
force-pushed
the
1061-replace-thirdparty-jsonfield-with-django-builtin1
branch
from
cec77ae to
0578eba
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/migrations/0049_devicegroup_context.py (1)
1-26:⚠️ Potential issue | 🟡 MinorModifying an already-applied migration may cause issues for existing deployments.
This migration (
0049_devicegroup_context) may have already been applied in production environments. Modifying its field type fromjsonfield.fields.JSONFieldtomodels.JSONFieldwon't trigger a re-run of the migration for existing deployments, potentially leaving their schema inconsistent with new deployments.The dedicated migration
0062_replace_jsonfield_with_django_builtin.pyalready handles altering this field for existing deployments. Consider whether modifying this historical migration is necessary, or if it would be safer to leave the original migration unchanged and rely solely on the new0062migration for the field type conversion.If the intent is to ensure fresh deployments use Django's JSONField from the start, this approach works but should be documented in upgrade notes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
openwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/connection/migrations/0007_command.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pyopenwisp_controller/pki/migrations/0001_initial.pyrequirements.txttests/openwisp2/sample_config/migrations/0001_initial.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_pki/migrations/0001_initial.py
💤 Files with no reviewable changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- openwisp_controller/config/base/base.py
- openwisp_controller/config/base/config.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/template.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/connection/migrations/0007_command.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.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/template.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/connection/migrations/0007_command.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/connection/base/models.pytests/openwisp2/sample_pki/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/base/device_group.pytests/openwisp2/sample_config/migrations/0001_initial.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.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/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/base/multitenancy.pytests/openwisp2/sample_pki/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_config/migrations/0001_initial.py
🧬 Code graph analysis (2)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (7)
openwisp_controller/config/migrations/0018_config_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0028_template_default_values.py (1)
Migration(7-25)openwisp_controller/config/migrations/0036_device_group.py (1)
Migration(16-104)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
Migration(7-26)openwisp_controller/config/base/config.py (1)
name(130-137)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (4)
openwisp_controller/config/migrations/0018_config_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)
🪛 Ruff (0.14.14)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-110: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-110: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-40: 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). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (24)
openwisp_controller/connection/migrations/0001_initial.py (1)
66-72: LGTM!The migration correctly uses Django's built-in
models.JSONFieldforCredentials.paramsandDeviceConnection.paramsfields. The field attributes (default=dict,help_text,verbose_name,blank) are properly preserved.Also applies to: 131-141
openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
4-4: LGTM!The migration correctly updates to Django's built-in
models.JSONFieldwith proper import and field attributes preserved.Also applies to: 16-24
openwisp_controller/connection/base/models.py (1)
7-7: LGTM!The model file correctly imports and uses Django's built-in
JSONFieldfor all three fields:
Credentials.paramsDeviceConnection.paramsCommand.inputField attributes are appropriately preserved with
default=dict,blank,null, andhelp_textwhere applicable.Also applies to: 99-103, 237-245, 420-423
openwisp_controller/config/migrations/0023_update_context.py (1)
4-4: LGTM!The migration correctly updates the
config.contextfield to use Django's built-inmodels.JSONFieldwith all attributes preserved.Also applies to: 14-22
openwisp_controller/connection/migrations/0007_command.py (1)
75-81: LGTM!The
Command.inputfield correctly uses Django's built-inmodels.JSONFieldwith appropriateblank=Trueandnull=Trueattributes.openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py (1)
57-65: LGTM!The squashed migration correctly updates all three
configfields to use Django's built-inmodels.JSONField:
Config.configTemplate.configVpn.configAll field attributes (
default=dict,blank,help_text,verbose_name) are properly preserved.Also applies to: 247-255, 342-349
openwisp_controller/config/migrations/0028_template_default_values.py (1)
4-4: LGTM!The migration correctly updates the
template.default_valuesfield to use Django's built-inmodels.JSONFieldwith all attributes preserved.Also applies to: 14-23
openwisp_controller/config/base/template.py (2)
8-8: LGTM!The import is correctly updated to use Django's built-in
JSONFieldfromdjango.db.models.
95-105: LGTM!The
default_valuesfield is correctly migrated to Django's built-inJSONField. The removal ofload_kwargsanddump_kwargsis appropriate since Django's JSONField handles serialization internally, and Python 3.7+ dicts preserve insertion order, making theOrderedDicthook unnecessary.openwisp_controller/pki/migrations/0001_initial.py (2)
262-269: Field definition looks correct.The
Cert.extensionsfield correctly usesmodels.JSONFieldwith appropriate parameters (blank=True,default=list, help text, and verbose name).
111-118: Migration strategy properly handles both fresh and existing deployments.The corresponding
AlterFieldmigration already exists in migration 0012 (0012_alter_ca_extensions_alter_ca_key_length_and_more.py), which correctly converts theextensionsfield tomodels.JSONFieldfor both theCaandCertmodels. This ensures existing deployments that already ran the initial migration can upgrade seamlessly.openwisp_controller/config/base/multitenancy.py (2)
5-5: LGTM!The import is correctly updated to use Django's built-in
JSONField.
33-40: LGTM!The
contextfield is correctly migrated to Django's built-inJSONFieldwith appropriate parameters. The removal ofload_kwargs/dump_kwargsis correct since Django handles JSON serialization internally.openwisp_controller/config/base/device_group.py (2)
6-6: LGTM!The import is correctly updated to use Django's built-in
JSONField.
38-55: LGTM!Both
meta_dataandcontextfields are correctly migrated to Django's built-inJSONFieldwith appropriate parameters (blank=True,default=dict, help text, and verbose names).openwisp_controller/config/migrations/0036_device_group.py (1)
56-67: Field definition is correct; same migration modification consideration applies.The
meta_datafield correctly usesmodels.JSONFieldwith appropriate parameters. As noted for other migration files, ensure the dedicated0061_replace_jsonfield_with_django_builtin.pymigration handles the field type conversion for existing deployments.openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (1)
1-40: LGTM! This is the correct approach for migrating existing field types.This dedicated migration properly uses
AlterFieldoperations to convert existingjsonfieldcolumns to Django's built-inJSONFieldfor theconnectionapp models. This ensures existing deployments will have their field types updated when running migrations.The Ruff RUF012 warnings about mutable class attributes (
dependenciesandoperations) are false positives—Django migrations use class attributes by design, and these are not intended to be modified at runtime.openwisp_controller/config/migrations/0018_config_context.py (1)
14-22: No changes needed — the migration modification is safe and properly handled.The concern about modifying existing migrations doesn't apply here. The 0018 migration was changed to use Django's built-in
JSONFieldinstead of the third-partyjsonfieldlibrary, which is safe because both implementations store JSON identically at the database level. Users with existing deployments will experience no database inconsistency:
- Their database already has the JSON column from the original
0018migration- The migration won't re-run (Django tracks applied migrations by name)
- The follow-up migration
0062_replace_jsonfield_with_django_builtin.pyproperly usesAlterFieldoperations to update field definitions withdefault=dictand other properties, which is the correct approach for safe schema refinementLikely an incorrect or invalid review comment.
tests/openwisp2/sample_pki/migrations/0001_initial.py (1)
128-135: LGTM!The
extensionsfields for bothCaandCertmodels are correctly migrated to Django's built-inmodels.JSONField. The field options (blank=True,default=list,help_text,verbose_name) are properly preserved. Since this is an initial migration creating new tables, there are no data migration concerns.Also applies to: 303-311
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
1-110: LGTM!This migration correctly converts all JSONField declarations from the third-party
jsonfieldpackage to Django's built-inmodels.JSONField. The field configurations (blank, default, help_text, verbose_name) are consistent with the existing migrations in the codebase (verified against 0023_update_context.py, 0049_devicegroup_context.py, 0028_template_default_values.py, etc.).The static analysis warnings about mutable class attributes (RUF012) are false positives—Django migrations conventionally use
dependenciesandoperationsas class attributes without type annotations.tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
1-40: LGTM!This test migration correctly converts the JSONField declarations for
Command.input,Credentials.params, andDeviceConnection.paramsto Django's built-inmodels.JSONField. The field configurations are appropriate and consistent with the main application's migration patterns.The RUF012 static analysis warnings are false positives for Django migrations.
tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
69-76: LGTM!The JSONField declarations for
Credentials.params,DeviceConnection.params, andCommand.inputare correctly migrated to Django's built-inmodels.JSONField. The field options are properly configured for each use case.Since this is a test migration under
tests/openwisp2/, modifying the initial migration is acceptable as test environments are typically reset more frequently.Also applies to: 152-163, 251-257
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
1-110: LGTM!This test migration correctly mirrors the main application's
0062_replace_jsonfield_with_django_builtin.pymigration, ensuring the sample_config test models use Django's built-inmodels.JSONField. The field configurations are consistent with both the main migration and the relevant code snippets.The RUF012 static analysis warnings are false positives for Django migrations.
tests/openwisp2/sample_config/migrations/0001_initial.py (1)
74-82: LGTM!All JSONField declarations across the models (
Config.config,Config.context,Vpn.config,Template.config,Template.default_values,OrganizationConfigSettings.context,DeviceGroup.meta_data,DeviceGroup.context) are correctly migrated to Django's built-inmodels.JSONFieldwith appropriate field options preserved.Since this is a test migration under
tests/openwisp2/, modifying the initial migration is acceptable for ensuring fresh test environments use the correct field types.Also applies to: 113-124, 238-245, 502-510, 565-577, 693-704, 746-758, 759-770
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
1061-replace-thirdparty-jsonfield-with-django-builtin1
branch
from
0578eba to
8c91a92
Compare
| _("configuration"), | ||
| default=dict, | ||
| help_text=_("configuration in NetJSON DeviceConfiguration format"), | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, |
There was a problem hiding this comment.
Let's add: encoder=DjangoJSONEncoder, see openwisp/openwisp-notifications#438, applies to all JSONField instances
| # pass non-empty string or None | ||
| kwargs[key] = value or None | ||
| # parse JSON strings for JSONField fields | ||
| elif isinstance(field, models.JSONField) and isinstance(value, str): |
There was a problem hiding this comment.
try removing this after adding encoder=DjangoJSONEncoder
There was a problem hiding this comment.
@nemesifier I tested removing the JSON parsing code but it breaks the preview functionality. The DjangoJSONEncoder solves lazy translation serialization but doesn't handle form data parsing. When the admin preview receives form data, JSONField values come as JSON strings that need to be parsed to Python objects before model validation.
Without the parsing code, tests fail with "Unexpected configuration format" because the model expects a dict but receives a string. The encoder and parsing code solve different problems:
DjangoJSONEncoder: Handles lazy translations during model save- Admin parsing: Converts form strings to Python objects for preview
Both are needed for full functionality. Should I keep the current implementation?
nemesifier
changed the title
Eeshu-Yadav
force-pushed
the
1061-replace-thirdparty-jsonfield-with-django-builtin1
branch
2 times, most recently
from
d2f68b5 to
008d787
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/base.py`:
- Around line 125-128: The JSONField declaration for the variable config has
multiple keyword args and the closing parenthesis crammed onto a single overlong
line (JSONField, default=dict, help_text, encoder=DjangoJSONEncoder); split the
arguments across multiple lines so each kwarg (help_text, encoder, etc.) is on
its own line and place the closing parenthesis on its own line to satisfy
line-length rules and restore proper formatting for the JSONField(config,
default=dict, help_text, encoder) call.
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 34-42: The migration/Model mismatch is caused by
AbstractOrganizationConfigSettings.context, AbstractDeviceGroup.meta_data and
AbstractDeviceGroup.context declaring encoder=DjangoJSONEncoder in the models
but the migration that creates OrganizationConfigSettings and DeviceGroup fields
omits that kwarg; update the migration's field definitions for
OrganizationConfigSettings.context and DeviceGroup.meta_data and
DeviceGroup.context to include encoder=DjangoJSONEncoder (or alternatively
remove encoder from the model fields if you prefer) so the migration's field
kwargs match the model's JSONField.deconstruct() output.
In `@openwisp_controller/config/migrations/0049_devicegroup_context.py`:
- Around line 16-24: The AlterField for the JSONField named "context" must
include the same encoder used in the model: add encoder=DjangoJSONEncoder to the
models.JSONField(...) call in the migration AlterField, and import
DjangoJSONEncoder at the top of the migration (from django.core.serializers.json
import DjangoJSONEncoder) so the migration matches the model's context field
definition in device_group.py.
In `@openwisp_controller/connection/migrations/0001_initial.py`:
- Around line 65-71: The migration JSONField definitions for Credentials.params
and DeviceConnection.params are missing encoder=DjangoJSONEncoder; update both
JSONField calls in openwisp_controller/connection/migrations/0001_initial.py to
include encoder=DjangoJSONEncoder and ensure the migration imports
DjangoJSONEncoder (from django.core.serializers.json import DjangoJSONEncoder)
so the migration matches the model definitions (update the JSONField for the
"params" field in both the Credentials and DeviceConnection model declarations
in the migration).
🧹 Nitpick comments (1)
openwisp_controller/config/base/multitenancy.py (1)
4-6: Minor style note:JSONFieldcan be accessed via the already-importedmodelsmodule.Line 5 imports
modelsand line 6 separately importsJSONFieldfromdjango.db.models. You could usemodels.JSONFielddirectly (as is done in all the migration files) to avoid the extra import. This is a minor consistency point — the current approach works but differs from the migration convention.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
openwisp_controller/config/admin.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/template.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/connection/migrations/0007_command.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/pki/migrations/0001_initial.pyrequirements.txttests/openwisp2/sample_config/migrations/0001_initial.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_pki/migrations/0001_initial.py
💤 Files with no reviewable changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- openwisp_controller/config/migrations/0028_template_default_values.py
- openwisp_controller/config/tests/test_api.py
- openwisp_controller/config/admin.py
- tests/openwisp2/sample_config/migrations/0001_initial.py
- openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py
- openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
- openwisp_controller/connection/tests/test_models.py
- openwisp_controller/connection/base/models.py
- openwisp_controller/connection/migrations/0007_command.py
- openwisp_controller/config/migrations/0023_update_context.py
- openwisp_controller/config/migrations/0036_device_group.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/template.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.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/template.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/migrations/0018_config_context.pytests/openwisp2/sample_pki/migrations/0001_initial.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/base/device_group.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.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/base/multitenancy.pyopenwisp_controller/config/migrations/0018_config_context.pytests/openwisp2/sample_pki/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py
🧬 Code graph analysis (9)
openwisp_controller/config/base/multitenancy.py (1)
openwisp_controller/config/base/base.py (1)
json(274-281)
tests/openwisp2/sample_pki/migrations/0001_initial.py (1)
openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py (1)
Migration(7-70)
openwisp_controller/config/base/config.py (1)
openwisp_controller/config/base/base.py (1)
json(274-281)
openwisp_controller/config/base/device_group.py (1)
openwisp_controller/config/base/base.py (1)
json(274-281)
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (2)
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (1)
Migration(6-40)tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
Migration(19-284)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (7)
openwisp_controller/config/migrations/0018_config_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0028_template_default_values.py (1)
Migration(7-25)openwisp_controller/config/migrations/0036_device_group.py (1)
Migration(16-104)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
Migration(7-26)tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)
openwisp_controller/pki/migrations/0001_initial.py (1)
openwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py (1)
Migration(7-70)
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (4)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)openwisp_controller/connection/migrations/0001_initial.py (1)
Migration(16-170)openwisp_controller/connection/migrations/0007_command.py (1)
Migration(17-112)tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
Migration(6-40)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (5)
openwisp_controller/config/migrations/0018_config_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)tests/openwisp2/sample_config/migrations/0001_initial.py (1)
Migration(22-1039)
🪛 Ruff (0.14.14)
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-110: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-110: 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). (9)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | 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~=4.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.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (12)
openwisp_controller/config/base/config.py (2)
9-11: LGTM — imports updated correctly.The switch to
DjangoJSONEncoderand Django's built-inJSONFieldis clean. One minor note: sincemodelsis already imported on line 10, you could usemodels.JSONFielddirectly instead of the separate import on line 11, which would be consistent with how other model fields are referenced. But this is purely stylistic and consistent with other files in this PR.
85-95: Context field migration looks correct.The
encoder=DjangoJSONEncoderis a reasonable choice — it handles edge-case types (datetime, Decimal, UUID) that plainjson.dumpswould reject. Thedefault=dictandblank=Trueare preserved from the previous declaration.openwisp_controller/config/base/base.py (1)
8-10: Imports updated consistently with the rest of the PR.openwisp_controller/config/base/template.py (1)
7-9: LGTM —default_valuesfield correctly migrated to Django's JSONField.The
encoder=DjangoJSONEncoderand removal ofload_kwargs/dump_kwargsis consistent with the other model files in this PR. Existing validation logic inclean()(lines 226-231) correctly handles the field as a native Python dict.Also applies to: 96-107
openwisp_controller/pki/migrations/0001_initial.py (1)
110-118: LGTM —extensionsfields migrated tomodels.JSONField.Both
Ca.extensionsandCert.extensionsare correctly updated. Noencoderis specified, which is appropriate since these fields store plain lists. The relevant snippet from migration0012confirms a laterAlterFieldalready covers existing databases with the samemodels.JSONFieldsignature.Also applies to: 261-269
openwisp_controller/config/migrations/0018_config_context.py (1)
4-22: In-place modification of historical migration is safe — dedicated transition migration exists.The changes to
0018_config_context.pyare properly handled by migration0062_replace_jsonfield_with_django_builtin.py, which includes a dedicatedAlterFieldoperation for thecontextfield on theconfigmodel (lines 23–35). This ensures:
- Existing databases: The
AlterFieldin 0062 transitions the field withdefault=dictand removesnull=True- Fresh installs: Running both migrations in sequence produces the correct final state
Both conditions for safe in-place modification of historical migrations are met.
tests/openwisp2/sample_pki/migrations/0001_initial.py (1)
128-135: LGTM!Both
extensionsfields (Ca and Cert) are correctly migrated tomodels.JSONFieldwithblank=True, default=list, consistent with the production migration inopenwisp_controller/pki/migrations/0012_alter_ca_extensions_alter_ca_key_length_and_more.py. Removal ofdump_kwargs/load_kwargsis appropriate since Django's built-in JSONField handles serialization internally.Also applies to: 303-311
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (1)
1-40: LGTM!The migration correctly alters all three connection JSON fields to Django's built-in
JSONFieldwith appropriate field options. The operations are consistent with the parallel test migration (tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py) and the existing initial migration field definitions. Dependency chain is correct.tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
1-40: LGTM!Test migration correctly mirrors the production migration (
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py) with identical field definitions for all three connection JSON fields.tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
69-76: LGTM!All three JSON fields (
Credentials.params,DeviceConnection.params,Command.input) are correctly updated tomodels.JSONFieldwith field options matching the production initial migration. Clean removal ofjsonfield-specific imports and parameters.Also applies to: 152-163, 251-257
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
1-110: LGTM!This test migration correctly mirrors the production migration (
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py) across all 8AlterFieldoperations. Field definitions, help texts, defaults, andblank/verbose_namesettings are consistent. Note thatVPN.configcorrectly omitsblank=True(line 104) since VPN configuration is required, unlike the other config fields.The only pending concern is the
encoder=DjangoJSONEncodermismatch already raised in themultitenancy.pyreview — if the encoder is kept in the model definitions, it needs to be added here as well.openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
1-110: Migration looks correct and consistent.All 8
AlterFieldoperations match the corresponding sample_config test migration (tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py) exactly. The field parameters (blank,default,help_text,verbose_name) are consistent with the earlier migrations that originally defined these fields. The dependency on0061_config_checksum_dbis correct.The RUF012 static analysis warnings are false positives —
dependenciesandoperationsas plain class attributes is the standard Django migration convention.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ( | ||
| "params", | ||
| jsonfield.fields.JSONField( | ||
| models.JSONField( | ||
| default=dict, | ||
| dump_kwargs={"indent": 4}, | ||
| help_text="global connection parameters", | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| verbose_name="parameters", | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "JSONField|encoder" openwisp_controller/connection/base/models.pyRepository: openwisp/openwisp-controller
Length of output: 312
Missing encoder=DjangoJSONEncoder in migration — inconsistent with model definitions.
Both Credentials.params and DeviceConnection.params JSONField definitions in the migration omit encoder=DjangoJSONEncoder, but the actual model definitions in connection/base/models.py include this encoder. The migration must be updated to match:
models.JSONField(
default=dict,
encoder=DjangoJSONEncoder, # Add this
help_text="global connection parameters",
verbose_name="parameters",
)This applies to both occurrences (Credentials at lines 65-71 and DeviceConnection at lines 130-141).
🤖 Prompt for AI Agents
In `@openwisp_controller/connection/migrations/0001_initial.py` around lines 65 -
71, The migration JSONField definitions for Credentials.params and
DeviceConnection.params are missing encoder=DjangoJSONEncoder; update both
JSONField calls in openwisp_controller/connection/migrations/0001_initial.py to
include encoder=DjangoJSONEncoder and ensure the migration imports
DjangoJSONEncoder (from django.core.serializers.json import DjangoJSONEncoder)
so the migration matches the model definitions (update the JSONField for the
"params" field in both the Credentials and DeviceConnection model declarations
in the migration).
008d787 to
3177b52
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/connection/tests/test_models.py (1)
601-610:⚠️ Potential issue | 🟡 MinorStale test data:
input='["echo test"]'is now stored as a string, not a list.Line 603 was not updated in this PR. With the old
jsonfield,'["echo test"]'was auto-parsed to a Python list["echo test"]. With Django'sJSONField, it will be stored as the literal string'["echo test"]'. While this test still passes (theTypeErroris triggered bytype="custom", not the input value), theinputno longer holds the semantically intended value. Consider updating it for consistency with the rest of the test changes:- command = Command(input='["echo test"]', type="custom") + command = Command(input=["echo test"], type="custom")
🧹 Nitpick comments (2)
tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.py (1)
1-117: Two-step migration in tests vs single-step in main app — consider squashing.The test app uses two migrations to achieve what the main
configapp does in one:
0008converts fields tomodels.JSONFieldwithoutencoder0009(this file) addsencoder=DjangoJSONEncoderMeanwhile,
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pydoes both in a single migration. This means there's an intermediate state (after0008, before0009) where the test app's fields lack the encoder that the models specify — Django would detect a pending migration in that window.Consider squashing
0008and0009into a single migration to mirror the main app's approach and avoid the intermediate inconsistency. That said, this is cosmetic and doesn't affect correctness at runtime once both migrations run.tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
1-40: Redundant migration — consider squashing into0005.This migration alters all three fields to
models.JSONFieldwithoutencoder, and the immediately following0005re-alters the exact same fields to addencoder=DjangoJSONEncoder. Since both are new, unapplied migrations introduced in this PR, they can be safely merged into a single migration, avoiding unnecessaryALTER TABLEoperations.The production equivalent (
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py) achieves this in a single migration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
openwisp_controller/config/admin.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/template.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/config/migrations/0018_config_context.pyopenwisp_controller/config/migrations/0023_update_context.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/config/migrations/0036_device_group.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0051_organizationconfigsettings_context.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0001_initial.pyopenwisp_controller/connection/migrations/0007_command.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/pki/migrations/0001_initial.pyrequirements.txttests/openwisp2/sample_config/migrations/0001_initial.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_pki/migrations/0001_initial.py
💤 Files with no reviewable changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- openwisp_controller/config/base/template.py
- openwisp_controller/config/admin.py
- openwisp_controller/connection/base/models.py
- openwisp_controller/config/migrations/0036_device_group.py
- openwisp_controller/config/migrations/0018_config_context.py
- openwisp_controller/config/base/config.py
- openwisp_controller/config/base/device_group.py
- openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py
- openwisp_controller/config/migrations/0023_update_context.py
- openwisp_controller/config/tests/test_api.py
- openwisp_controller/connection/migrations/0001_initial.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/connection/migrations/0007_command.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/config/migrations/0028_template_default_values.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/config/base/base.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.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/connection/migrations/0007_command.pytests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.pyopenwisp_controller/pki/migrations/0001_initial.pyopenwisp_controller/config/migrations/0028_template_default_values.pytests/openwisp2/sample_connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/base/multitenancy.pytests/openwisp2/sample_config/migrations/0001_initial.pytests/openwisp2/sample_pki/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/config/base/base.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/migrations/0049_devicegroup_context.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
📚 Learning: 2026-01-12T22:27:40.078Z
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:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_config/migrations/0001_initial.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.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/pki/migrations/0001_initial.pyopenwisp_controller/config/migrations/0028_template_default_values.pytests/openwisp2/sample_connection/migrations/0001_initial.pyopenwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/config/base/multitenancy.pytests/openwisp2/sample_pki/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
🧬 Code graph analysis (4)
tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.py (4)
openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
Migration(7-117)tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (8)
openwisp_controller/config/migrations/0018_config_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0023_update_context.py (1)
Migration(7-24)openwisp_controller/config/migrations/0028_template_default_values.py (1)
Migration(7-25)openwisp_controller/config/migrations/0049_devicegroup_context.py (1)
Migration(7-26)openwisp_controller/config/migrations/0051_organizationconfigsettings_context.py (1)
Migration(7-26)openwisp_controller/connection/migrations/0001_initial.py (1)
Migration(16-170)tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
Migration(6-110)tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.py (1)
Migration(7-117)
openwisp_controller/config/base/multitenancy.py (1)
openwisp_controller/config/base/base.py (1)
json(276-283)
tests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.py (3)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
Migration(7-117)openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (1)
Migration(7-47)tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.py (1)
Migration(7-117)
🪛 Ruff (0.14.14)
tests/openwisp2/sample_config/migrations/0009_replace_jsonfield_with_django_builtin.py
[warning] 9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-117: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py
[warning] 9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-117: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.py
[warning] 9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-110: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py
[warning] 9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-40: 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). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | 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.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (20)
openwisp_controller/connection/tests/test_models.py (3)
503-511: LGTM — test correctly updated for native Python objects.With Django's built-in
JSONField, the field accepts native Python objects directly. Passing["test"]instead of a JSON string'["test"]'is the correct approach, and the updated assertion matches the new validation behavior.
529-548: LGTM — validation error messages updated correctly.With Django's
JSONField, the"notjson"string is stored as-is (a Python string), so the JSON schema validator now sees a string type rather than a parse error. The updated error messages ("'notjson' is not of type 'object'"and"[] is not of type 'object'") correctly reflect the new behavior.
973-1003: LGTM — config values correctly changed from JSON strings to Python dicts.openwisp_controller/pki/migrations/0001_initial.py (2)
110-118: LGTM —Ca.extensionsfield correctly migrated to Django'sJSONField.The field definition matches the later
0012migration's target state (includinghelp_text), ensuring consistency for both fresh installs and upgrades.
261-269: LGTM —Cert.extensionsfield correctly migrated.Consistent with
Ca.extensionsand the0012migration.openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py (3)
57-65: LGTM —Config.configfield correctly updated in initial migration.The
encoderis intentionally absent here since it will be added by the later0062migration.
247-255: LGTM —Template.configfield correctly updated.
342-349: LGTM —Vpn.configfield correctly updated.Correctly preserves the distinction that
Vpn.configdoes not haveblank=True, unlikeConfig.configandTemplate.config.openwisp_controller/config/base/multitenancy.py (1)
4-6: LGTM — imports updated correctly.Clean switch from
jsonfieldto Django's built-inJSONFieldwithDjangoJSONEncoder.openwisp_controller/config/base/base.py (1)
8-10: LGTM — imports cleanly updated.Correctly replaces
jsonfieldimports with Django's built-inJSONFieldand addsDjangoJSONEncoder.openwisp_controller/connection/migrations/0007_command.py (1)
75-81: The migration mismatch has been resolved in a subsequent migration. Migration 0010_replace_jsonfield_with_django_builtin.py (applied after 0007_command.py) already addedencoder=DjangoJSONEncoderto theinputfield via AlterField, bringing it in sync with the model definition inbase/models.py.tests/openwisp2/sample_connection/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
1-47: LGTM!All three fields include
encoder=DjangoJSONEncoder, matching the production migration (0010). Field definitions (defaults, help_text, verbose_name) are consistent.openwisp_controller/config/migrations/0028_template_default_values.py (1)
4-24: LGTM!Clean replacement of
jsonfield.fields.JSONFieldwithmodels.JSONField. The encoder is correctly deferred to the later0062migration. Field attributes (blank, default, help_text, verbose_name) are preserved.openwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.py (1)
1-47: LGTM!Single, clean migration with
encoder=DjangoJSONEncoderon all three connection fields. Dependency chain is correct, and field attributes match the base model definitions.tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
71-75: LGTM!All three JSONField replacements in the initial migration are correct. The encoder is appropriately deferred to the later dedicated migration (
0004/0005). Field attributes are preserved.Also applies to: 154-162, 253-256
tests/openwisp2/sample_config/migrations/0001_initial.py (1)
76-81: LGTM!All eight
JSONFieldreplacements in the initial migration are correct. Field attributes are preserved, and the encoder is appropriately handled by the later dedicated migrations.Also applies to: 115-123, 240-244, 504-509, 567-576, 695-703, 748-757, 761-769
tests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.py (1)
12-109: No action needed — migration chain produces correct final state.Migration 0008 defers the
encoderparameter to follow-up migration 0009, which addsencoder=django.core.serializers.json.DjangoJSONEncoderto all 8 fields (not just 2 as claimed). The final schema matches production migration 0062 exactly, with all fields properly configured to serialize datetimes, Decimals, UUIDs, and lazy translation objects.tests/openwisp2/sample_pki/migrations/0001_initial.py (2)
1-10: Removed imports and field parameter cleanup look correct.The
jsonfield.fieldsandcollectionsimports have been properly removed, andload_kwargs/dump_kwargsparameters are no longer present. The migration header still says "Generated by Django 3.0.6" — this is expected since only field definitions were edited in-place, not the migration itself regenerated.
128-134: Consider addingencoder=DjangoJSONEncoderto theextensionsfield or evaluate if it's necessary for this specific field type.The review comment's claim about encoder consistency is inaccurate:
sample_config/0008(also in this PR) similarly lacks the encoder—it was added only in the follow-up migration0009. Additionally, the production migrationpki/0012_alter_ca_extensions_...for the actual Ca and Cert models also omits the encoder on theextensionsfields.If DjangoJSONEncoder is needed to handle non-JSON-native types (UUID, Decimal, datetime) that may appear in X.509 extensions, it should be added here. Otherwise, if extensions data are always JSON-serializable lists/dicts (as the
default=listsuggests), the current approach aligns with the production code and initial config approach (before its follow-up encoder migration).openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.py (1)
1-117: Migration looks correct and well-structured.All eight
AlterFieldoperations consistently applymodels.JSONFieldwithencoder=DjangoJSONEncoder, and the field attributes (defaults, help texts, verbose names, blank) align with both the existing migration history and the parallel test migration insample_config. The dependency on0061_config_checksum_dbis correct.The RUF012 static analysis warnings about
dependenciesandoperationsare false positives — these are standard Django migration class attributes that follow the framework's conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…penwisp#1061 Replaced jsonfield package's JSONField with Django's built-in JSONField across all models to use the modern, maintained Django implementation. Changes: - Updated imports from 'jsonfield import JSONField' to 'django.db.models import JSONField' - Removed jsonfield-specific parameters (load_kwargs, dump_kwargs) which are not needed - Created migrations to alter field types while preserving data - Removed unnecessary collections imports where no longer needed Django's JSONField preserves the same data format and is backward compatible, so no data migration is required. The built-in JSONField provides better performance and is actively maintained as part of Django core. Fixes openwisp#1061
Eeshu-Yadav
force-pushed
the
1061-replace-thirdparty-jsonfield-with-django-builtin1
branch
from
3177b52 to
5a81fdf
Compare
3 participants
Checklist
Reference to Existing Issue
Closes #1061.
Description of Changes
This PR replaces the third-party
jsonfieldpackage with Django's built-inJSONFieldacross all OpenWISP controller models to modernize the codebase and remove dependency on an unmaintained package.Changes Made:
Updated 6 model files to use
django.db.models.JSONFieldinstead of third-partyjsonfield.JSONField:openwisp_controller/config/base/base.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_group.pyopenwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/base/template.pyopenwisp_controller/connection/base/models.pyCreated 4 migration files to safely alter field types:
openwisp_controller/config/migrations/0062_replace_jsonfield_with_django_builtin.pyopenwisp_controller/connection/migrations/0010_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_config/migrations/0008_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_connection/migrations/0004_replace_jsonfield_with_django_builtin.pyAdded DjangoJSONEncoder to all JSONField instances to handle lazy translation objects during serialization
Updated 20+ old migration files to use Django's JSONField and remove jsonfield-specific parameters
Enhanced admin preview functionality to handle JSONField data parsing from form submissions
Updated test cases to work with Django's JSONField behavior (passing Python objects instead of JSON strings)
Fixed code quality issues by removing unused imports and duplicate imports
Technical Details:
JSONFieldhandles JSON serialization internally without needing external parametersencoder=DjangoJSONEncoderto prevent serialization errors with lazy translation objects (see openwisp-notifications#438)load_kwargs={"object_pairs_hook": collections.OrderedDict}is no longer needed as Python 3.7+ dicts maintain insertion orderdump_kwargs={"indent": 4}formatting should be handled at the serialization/display layer, not the model level