[change] Replace third-party JSONField with Django built-in JSONField#742
[change] Replace third-party JSONField with Django built-in JSONField#742pushpitkamboj wants to merge 4 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughReplaces instances of the third-party jsonfield.JSONField with Django's built-in models.JSONField across the codebase. Updated model fields include Check.params and Metric.main_tags/extra_tags, with removal of custom Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
openwisp_monitoring/check/base/models.pyopenwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.pyopenwisp_monitoring/monitoring/base/models.pyopenwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py
📜 Review details
⏰ 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.11 | django~=5.1.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.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/base/models.pyopenwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.pyopenwisp_monitoring/monitoring/base/models.pyopenwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py
🧬 Code graph analysis (4)
openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (3)
openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
Migration(6-31)
tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (3)
openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (1)
Migration(7-32)tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)
tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (3)
openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (1)
Migration(7-32)tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
Migration(6-31)
openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py (3)
openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (1)
Migration(7-32)tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
Migration(7-23)tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
Migration(6-31)
🔇 Additional comments (6)
openwisp_monitoring/monitoring/base/models.py (1)
93-103: LGTM — Field declarations are correct.The switch to
models.JSONFieldis clean. The existing__setattr__override (Line 126-129) ensures_sort_dictis still called when values are set, which preserves theOrderedDictwrapping that the oldload_kwargs={'object_pairs_hook': OrderedDict}provided. Since Django callssetattrduring model instance construction from DB rows, ordering consistency is maintained.openwisp_monitoring/check/base/models.py (1)
37-42: LGTM!Clean replacement — the field declaration matches the corresponding migration, and the
default=dict/blank=True/help_textare all preserved.tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
1-23: LGTM!Migration is consistent with the main app migration at
openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py. Field definition and dependency chain are correct.openwisp_monitoring/check/migrations/0012_replace_jsonfield_with_django_builtin.py (1)
1-23: LGTM!Dependency chain and field definition are correct and consistent with the model at
openwisp_monitoring/check/base/models.py.openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (1)
1-32: LGTM — this is the production-path migration.Field definitions correctly match the model, dependency is correct, and
db_index=Trueonmain_tagsis properly preserved. The same data-safety consideration noted on the sample migration applies here — ensure no existing rows contain invalid JSON text before running this against production data.tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
1-31: Migration is consistent with the main app migration — no data safety concerns.The
AlterFieldoperations and field definitions correctly mirroropenwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py. The oldjsonfield.fields.JSONField(from migration 0004) enforced JSON validity during serialization withdump_kwargscontrolling only formatting (indentation). PostgreSQL can safely cast the formatted JSON text tojsonbduring theAlterFieldoperation. NoRunPythondata sanitization step is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/monitoring/base/models.py`:
- Around line 93-103: Migrations reference the external jsonfield package which
is no longer a project dependency; update the listed migration files (the
Migration classes in 0004_metric_main_and_extra_tags,
0001_initial_squashed_0002_check_unique_together, tests.../0001_initial.py
files) to remove jsonfield imports and use Django's built-in JSONField instead
(e.g., import JSONField from django.db.models or reference models.JSONField for
fields like main_tags and extra_tags), ensuring the field definitions and
deconstruction in those Migration classes use JSONField so migrations run
without adding jsonfield as a dependency.
| main_tags = models.JSONField( | ||
| _("main tags"), | ||
| default=dict, | ||
| blank=True, | ||
| load_kwargs={"object_pairs_hook": OrderedDict}, | ||
| dump_kwargs={"indent": 4}, | ||
| db_index=True, | ||
| ) | ||
| extra_tags = JSONField( | ||
| extra_tags = models.JSONField( | ||
| _("extra tags"), | ||
| default=dict, | ||
| blank=True, | ||
| load_kwargs={"object_pairs_hook": OrderedDict}, | ||
| dump_kwargs={"indent": 4}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any remaining imports of the third-party jsonfield package
echo "=== Remaining jsonfield imports ==="
rg -n 'from jsonfield' --type=py
rg -n 'import jsonfield' --type=py
echo ""
echo "=== jsonfield in dependency files ==="
rg -n 'jsonfield' -g 'setup.*' -g 'pyproject.toml' -g 'requirements*' -g 'Pipfile*'Repository: openwisp/openwisp-monitoring
Length of output: 527
Remove jsonfield imports from migration files or restore package as a dependency.
The original jsonfield package is no longer listed in project dependencies (setup.py/setup.cfg/pyproject.toml/requirements*.txt), but it remains imported in migration files:
openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.py
These migrations will fail when executed because the jsonfield package is unavailable. Either update the migration files to remove jsonfield usage or restore the package as a dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/base/models.py` around lines 93 - 103,
Migrations reference the external jsonfield package which is no longer a project
dependency; update the listed migration files (the Migration classes in
0004_metric_main_and_extra_tags,
0001_initial_squashed_0002_check_unique_together, tests.../0001_initial.py
files) to remove jsonfield imports and use Django's built-in JSONField instead
(e.g., import JSONField from django.db.models or reference models.JSONField for
fields like main_tags and extra_tags), ensuring the field definitions and
deconstruction in those Migration classes use JSONField so migrations run
without adding jsonfield as a dependency.
pushpitkamboj
changed the title
- Updated models to use django.db.models.JSONField - Added migrations for check and monitoring apps - Added migrations for sample apps used in tests
pushpitkamboj
force-pushed
the
change/jsonfield-clean
branch
from
258ee35 to
951dc5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py`:
- Around line 16-21: The migration sets db_index=True on the JSONField
`main_tags` (models.JSONField), which creates a B-tree index; change this to use
a GIN index instead by removing db_index=True from the JSONField and adding an
Index entry using django.contrib.postgres.indexes.GinIndex for the `main_tags`
column (e.g. add a migrations.AddIndex that references
GinIndex(fields=['main_tags']) in the same migration so queries using
containment/existence operators use a proper GIN index).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py
📜 Review details
⏰ 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.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
🧬 Code graph analysis (1)
tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py (1)
Migration(7-32)
🔇 Additional comments (1)
tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py (1)
12-32: Field definition already provides safety safeguards—RunPython not requiredThe migration is safe despite lacking explicit
RunPythonvalidation. Themain_tagsandextra_tagsfields were created in the initial migration withblank=Trueanddefault=dict—and critically, nonull=True. This means:
- The database column cannot store NULL values (enforced by the default)
- Any unsaved instance defaults to
{}(empty dict)- The text-backed column contains only valid JSON or defaults
The text→jsonb cast (
main_tags::jsonb) will succeed on all existing rows. This mirrors the production migration atopenwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.py, which uses the identical pattern and was apparently accepted by the team. No test data evidence suggests empty strings or NULL values in these fields.While a
RunPythonsanitization step is a best practice for defensive migrations, it is not necessary here due to the field definition's inherent safeguards.
| field=models.JSONField( | ||
| blank=True, | ||
| db_index=True, | ||
| default=dict, | ||
| verbose_name="main tags", | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
db_index=True on JSONField creates a B-tree index, not GIN
On PostgreSQL, db_index=True on a jsonb column produces a B-tree index on the binary JSON representation. If main_tags is queried using key containment or key existence operators (@>, ?), a GIN index would be significantly more effective. This mirrors the original jsonfield behavior and is not a regression, but worth revisiting if monitoring queries filter by individual tag keys.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/openwisp2/sample_monitoring/migrations/0005_replace_jsonfield_with_django_builtin.py`
around lines 16 - 21, The migration sets db_index=True on the JSONField
`main_tags` (models.JSONField), which creates a B-tree index; change this to use
a GIN index instead by removing db_index=True from the JSONField and adding an
Index entry using django.contrib.postgres.indexes.GinIndex for the `main_tags`
column (e.g. add a migrations.AddIndex that references
GinIndex(fields=['main_tags']) in the same migration so queries using
containment/existence operators use a proper GIN index).
|
@pandafy please review |
Eeshu-Yadav
left a comment
Eeshu-Yadav
left a comment
There was a problem hiding this comment.
Missing encoder=DjangoJSONEncoder
| object_id = models.CharField(max_length=36, db_index=True, blank=True, null=True) | ||
| content_object = GenericForeignKey("content_type", "object_id") | ||
| main_tags = JSONField( | ||
| main_tags = models.JSONField( |
There was a problem hiding this comment.
Please add encoder=DjangoJSONEncoder to all JSONField instances (this one, extra_tags, and params in check/base/models.py).
This is needed to prevent serialization errors with lazy translation objects. See openwisp/openwisp-notifications#438 and the reference commit: openwisp/openwisp-notifications@bad5232
@nemesifier requested this same change on both the controller PR (openwisp/openwisp-controller#1214) and the radius PR (openwisp/openwisp-radius#619). It should be applied consistently across all openwisp repos.
from django.core.serializers.json import DjangoJSONEncoder
main_tags = models.JSONField(
_("main tags"),
default=dict,
blank=True,
db_index=True,
encoder=DjangoJSONEncoder,
)The AlterField migrations (0012, 0013, and the test migrations) will also need encoder=django.core.serializers.json.DjangoJSONEncoder in their field definitions.
| db_index=True, | ||
| ) | ||
| extra_tags = JSONField( | ||
| extra_tags = models.JSONField( |
| max_length=128, | ||
| ) | ||
| params = JSONField( | ||
| params = models.JSONField( |
|
Also, old migration files that still |
2 participants
Checklist
Reference to Existing Issue
Closes #673
Description of Changes
jsonfieldwith Django's built-inmodels.JSONField.