[change] Replace third-party JSONField with Django built-in JSONField#742
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 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.
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 |
|
Hi @pushpitkamboj 👋, This pull request has been automatically closed due to 68 days of inactivity. After changes were requested, the PR remained inactive. We understand that life gets busy, and we appreciate your initial contribution! 💙 The door is always open for you to come back:
If you have any questions or need help, don't hesitate to reach out. We're here to support you! Thank you for your interest in contributing to OpenWISP! 🙏 |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
Hi @pushpitkamboj 👋, This pull request has been automatically closed due to 69 days of inactivity. After changes were requested, the PR remained inactive. We understand that life gets busy, and we appreciate your initial contribution! 💙 The door is always open for you to come back:
If you have any questions or need help, don't hesitate to reach out. We're here to support you! Thank you for your interest in contributing to OpenWISP! 🙏 |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
There was a problem hiding this comment.
Thanks @pushpitkamboj for pushing this change forward, and thanks to everyone who reviewed it and flagged the missing encoder details. I pushed a small follow-up to align this with the approach we used in the other OpenWISP repositories: the converted JSON fields now use DjangoJSONEncoder, the replacement migrations serialize the same encoder, the old migrations no longer require jsonfield, and there are regression tests covering date serialization so we catch this class of issue in the future.
I re-ran the relevant migration checks, formatting/lint checks on the touched files, and the targeted production/sample tests. This looks safe to merge to me.
3 participants
Checklist
Reference to Existing Issue
Closes #673
Description of Changes
jsonfieldwith Django's built-inmodels.JSONField.